From 3e18a8a337f313030a9f2731c5e572984dc50e5f Mon Sep 17 00:00:00 2001 From: Evan Hahn <69474926+EvanHahn-Signal@users.noreply.github.com> Date: Thu, 2 Sep 2021 17:34:38 -0500 Subject: [PATCH] Don't ring large groups --- ts/components/CallManager.stories.tsx | 1 - ts/components/CallManager.tsx | 3 - ts/components/CallingLobby.stories.tsx | 1 - ts/components/CallingLobby.tsx | 19 ++++-- .../isConversationTooBigToRing.ts | 14 +++++ ts/groups/limits.ts | 4 -- ts/services/calling.ts | 4 ++ ts/state/ducks/calling.ts | 50 +++++++++++++-- ts/state/ducks/conversations.ts | 2 +- ts/state/smart/CallManager.tsx | 2 - .../isConversationTooBigToRing_test.ts | 61 +++++++++++++++++++ ts/test-both/groups/limits_test.ts | 22 ------- ts/test-electron/state/ducks/calling_test.ts | 5 ++ 13 files changed, 144 insertions(+), 44 deletions(-) create mode 100644 ts/conversations/isConversationTooBigToRing.ts create mode 100644 ts/test-both/conversations/isConversationTooBigToRing_test.ts diff --git a/ts/components/CallManager.stories.tsx b/ts/components/CallManager.stories.tsx index 324d8085c..f972df0d1 100644 --- a/ts/components/CallManager.stories.tsx +++ b/ts/components/CallManager.stories.tsx @@ -70,7 +70,6 @@ const createProps = (storyProps: Partial = {}): PropsType => ({ i18n, isGroupCallOutboundRingEnabled: true, keyChangeOk: action('key-change-ok'), - maxGroupCallRingSize: 16, me: { ...getDefaultConversation({ color: select( diff --git a/ts/components/CallManager.tsx b/ts/components/CallManager.tsx index 7f0e51265..5eb18bb9e 100644 --- a/ts/components/CallManager.tsx +++ b/ts/components/CallManager.tsx @@ -81,7 +81,6 @@ export type PropsType = { declineCall: (_: DeclineCallType) => void; i18n: LocalizerType; isGroupCallOutboundRingEnabled: boolean; - maxGroupCallRingSize: number; me: MeType; notifyForCall: (title: string, isVideoCall: boolean) => unknown; openSystemPreferencesAction: () => unknown; @@ -116,7 +115,6 @@ const ActiveCallManager: React.FC = ({ keyChangeOk, getGroupCallVideoFrameSource, getPresentingSources, - maxGroupCallRingSize, me, openSystemPreferencesAction, renderDeviceSelection, @@ -234,7 +232,6 @@ const ActiveCallManager: React.FC = ({ isGroupCall={activeCall.callMode === CallMode.Group} isGroupCallOutboundRingEnabled={isGroupCallOutboundRingEnabled} isCallFull={isCallFull} - maxGroupCallRingSize={maxGroupCallRingSize} me={me} onCallCanceled={cancelActiveCall} onJoinCall={joinActiveCall} diff --git a/ts/components/CallingLobby.stories.tsx b/ts/components/CallingLobby.stories.tsx index d23ef75de..7707dcead 100644 --- a/ts/components/CallingLobby.stories.tsx +++ b/ts/components/CallingLobby.stories.tsx @@ -57,7 +57,6 @@ const createProps = (overrideProps: Partial = {}): PropsType => { isGroupCall, isGroupCallOutboundRingEnabled: true, isCallFull: boolean('isCallFull', overrideProps.isCallFull || false), - maxGroupCallRingSize: overrideProps.maxGroupCallRingSize || 16, me: overrideProps.me || { color: AvatarColors[0], id: generateUuid(), diff --git a/ts/components/CallingLobby.tsx b/ts/components/CallingLobby.tsx index 8b7a78219..82577c01f 100644 --- a/ts/components/CallingLobby.tsx +++ b/ts/components/CallingLobby.tsx @@ -20,6 +20,7 @@ import { import { AvatarColorType } from '../types/Colors'; import { LocalizerType } from '../types/Util'; import { ConversationType } from '../state/ducks/conversations'; +import { isConversationTooBigToRing } from '../conversations/isConversationTooBigToRing'; export type PropsType = { availableCameras: Array; @@ -29,6 +30,7 @@ export type PropsType = { | 'avatarPath' | 'color' | 'isMe' + | 'memberships' | 'name' | 'phoneNumber' | 'profileName' @@ -44,7 +46,6 @@ export type PropsType = { isGroupCall: boolean; isGroupCallOutboundRingEnabled: boolean; isCallFull?: boolean; - maxGroupCallRingSize: number; me: { avatarPath?: string; id: string; @@ -74,7 +75,6 @@ export const CallingLobby = ({ isGroupCall = false, isGroupCallOutboundRingEnabled, isCallFull = false, - maxGroupCallRingSize, me, onCallCanceled, onJoinCall, @@ -150,21 +150,30 @@ export const CallingLobby = ({ ? CallingButtonType.AUDIO_ON : CallingButtonType.AUDIO_OFF; + const isGroupTooLargeToRing = isConversationTooBigToRing(conversation); + const isRingButtonVisible: boolean = isGroupCall && isGroupCallOutboundRingEnabled && peekedParticipants.length === 0 && (groupMembers || []).length > 1; - const preCallInfoRingMode: RingMode = - isGroupCall && !outgoingRing ? RingMode.WillNotRing : RingMode.WillRing; + let preCallInfoRingMode: RingMode; + if (isGroupCall) { + preCallInfoRingMode = + outgoingRing && !isGroupTooLargeToRing + ? RingMode.WillRing + : RingMode.WillNotRing; + } else { + preCallInfoRingMode = RingMode.WillRing; + } let ringButtonType: | CallingButtonType.RING_DISABLED | CallingButtonType.RING_ON | CallingButtonType.RING_OFF; if (isRingButtonVisible) { - if ((groupMembers || []).length > maxGroupCallRingSize) { + if (isGroupTooLargeToRing) { ringButtonType = CallingButtonType.RING_DISABLED; } else if (outgoingRing) { ringButtonType = CallingButtonType.RING_ON; diff --git a/ts/conversations/isConversationTooBigToRing.ts b/ts/conversations/isConversationTooBigToRing.ts new file mode 100644 index 000000000..9cb9624e1 --- /dev/null +++ b/ts/conversations/isConversationTooBigToRing.ts @@ -0,0 +1,14 @@ +// Copyright 2021 Signal Messenger, LLC +// SPDX-License-Identifier: AGPL-3.0-only + +import { parseIntWithFallback } from '../util/parseIntWithFallback'; +import { getValue } from '../RemoteConfig'; +import type { ConversationType } from '../state/ducks/conversations'; + +const getMaxGroupCallRingSize = (): number => + parseIntWithFallback(getValue('global.calling.maxGroupCallRingSize'), 16); + +export const isConversationTooBigToRing = ( + conversation: Readonly> +): boolean => + (conversation.memberships?.length || 0) >= getMaxGroupCallRingSize(); diff --git a/ts/groups/limits.ts b/ts/groups/limits.ts index 5cb6cc85b..7004611ce 100644 --- a/ts/groups/limits.ts +++ b/ts/groups/limits.ts @@ -3,7 +3,6 @@ import { isNumber } from 'lodash'; import { parseIntOrThrow } from '../util/parseIntOrThrow'; -import { parseIntWithFallback } from '../util/parseIntWithFallback'; import { getValue, ConfigKeyType } from '../RemoteConfig'; function makeGetter(configKey: ConfigKeyType): (fallback?: number) => number { @@ -29,6 +28,3 @@ export const getGroupSizeRecommendedLimit = makeGetter( export const getGroupSizeHardLimit = makeGetter( 'global.groupsv2.groupSizeHardLimit' ); - -export const getMaxGroupCallRingSize = (): number => - parseIntWithFallback(getValue('global.calling.maxGroupCallRingSize'), 16); diff --git a/ts/services/calling.ts b/ts/services/calling.ts index a95e1291e..da8955389 100644 --- a/ts/services/calling.ts +++ b/ts/services/calling.ts @@ -40,6 +40,7 @@ import { GroupCallPeekInfoType, } from '../state/ducks/calling'; import { getConversationCallMode } from '../state/ducks/conversations'; +import { isConversationTooBigToRing } from '../conversations/isConversationTooBigToRing'; import { AudioDevice, AvailableIODevicesType, @@ -378,6 +379,9 @@ export class CallingClass { this.uxActions.showCallLobby({ callMode: CallMode.Group, conversationId: conversationProps.id, + isConversationTooBigToRing: isConversationTooBigToRing( + conversationProps + ), ...this.formatGroupCallForRedux(groupCall), }); break; diff --git a/ts/state/ducks/calling.ts b/ts/state/ducks/calling.ts index b644a3b0f..658219cd0 100644 --- a/ts/state/ducks/calling.ts +++ b/ts/state/ducks/calling.ts @@ -11,6 +11,7 @@ import { import { has, omit } from 'lodash'; import { getOwn } from '../../util/getOwn'; import { getPlatform } from '../selectors/user'; +import { isConversationTooBigToRing } from '../../conversations/isConversationTooBigToRing'; import { missingCaseError } from '../../util/missingCaseError'; import { calling } from '../../services/calling'; import { StateType as RootStateType } from '../reducer'; @@ -30,6 +31,7 @@ import { callingTones } from '../../util/callingTones'; import { requestCameraPermissions } from '../../util/callingPermissions'; import { sleep } from '../../util/sleep'; import { LatestQueue } from '../../util/LatestQueue'; +import type { ConversationChangedActionType } from './conversations'; // State @@ -228,6 +230,7 @@ export type ShowCallLobbyType = joinState: GroupCallJoinState; hasLocalAudio: boolean; hasLocalVideo: boolean; + isConversationTooBigToRing: boolean; peekInfo?: GroupCallPeekInfoType; remoteParticipants: Array; }; @@ -465,6 +468,7 @@ export type CallingActionType = | CallStateChangeFulfilledActionType | ChangeIODeviceFulfilledActionType | CloseNeedPermissionScreenActionType + | ConversationChangedActionType | DeclineCallActionType | GroupCallStateChangeActionType | HangUpActionType @@ -1069,9 +1073,22 @@ function startCall( }); break; case CallMode.Group: { - const outgoingRing = Boolean( - getState().calling.activeCallState?.outgoingRing - ); + let outgoingRing: boolean; + + const state = getState(); + const { activeCallState } = state.calling; + if (activeCallState?.outgoingRing) { + const conversation = getOwn( + state.conversations.conversationLookup, + activeCallState.conversationId + ); + outgoingRing = Boolean( + conversation && !isConversationTooBigToRing(conversation) + ); + } else { + outgoingRing = false; + } + calling.joinGroupCall( payload.conversationId, payload.hasLocalAudio, @@ -1220,6 +1237,7 @@ export function reducer( // We expect to be in this state briefly. The Calling service should update the // call state shortly. const existingCall = getGroupCall(conversationId, state); + const ringState = getGroupCallRingState(existingCall); call = { callMode: CallMode.Group, conversationId, @@ -1232,10 +1250,13 @@ export function reducer( deviceCount: action.payload.remoteParticipants.length, }, remoteParticipants: action.payload.remoteParticipants, - ...getGroupCallRingState(existingCall), + ...ringState, }; outgoingRing = - !call.peekInfo.uuids.length && !call.remoteParticipants.length; + !ringState.ringId && + !call.peekInfo.uuids.length && + !call.remoteParticipants.length && + !action.payload.isConversationTooBigToRing; break; } default: @@ -1352,6 +1373,25 @@ export function reducer( }; } + if (action.type === 'CONVERSATION_CHANGED') { + const activeCall = getActiveCall(state); + const { activeCallState } = state; + if ( + !activeCallState?.outgoingRing || + activeCallState.conversationId !== action.payload.id || + activeCall?.callMode !== CallMode.Group || + activeCall.joinState !== GroupCallJoinState.NotJoined || + !isConversationTooBigToRing(action.payload.data) + ) { + return state; + } + + return { + ...state, + activeCallState: { ...activeCallState, outgoingRing: false }, + }; + } + if (action.type === DECLINE_DIRECT_CALL) { return removeConversationFromState(state, action.payload.conversationId); } diff --git a/ts/state/ducks/conversations.ts b/ts/state/ducks/conversations.ts index 8d899ead6..072100033 100644 --- a/ts/state/ducks/conversations.ts +++ b/ts/state/ducks/conversations.ts @@ -443,7 +443,7 @@ type ConversationAddedActionType = { data: ConversationType; }; }; -type ConversationChangedActionType = { +export type ConversationChangedActionType = { type: 'CONVERSATION_CHANGED'; payload: { id: string; diff --git a/ts/state/smart/CallManager.tsx b/ts/state/smart/CallManager.tsx index d61feed86..8eb99028b 100644 --- a/ts/state/smart/CallManager.tsx +++ b/ts/state/smart/CallManager.tsx @@ -12,7 +12,6 @@ import { getMe, getConversationSelector } from '../selectors/conversations'; import { getActiveCall } from '../ducks/calling'; import { ConversationType } from '../ducks/conversations'; import { getIncomingCall } from '../selectors/calling'; -import { getMaxGroupCallRingSize } from '../../groups/limits'; import { isGroupCallOutboundRingEnabled } from '../../util/isGroupCallOutboundRingEnabled'; import { ActiveCallType, @@ -297,7 +296,6 @@ const mapStateToProps = (state: StateType) => ({ i18n: getIntl(state), isGroupCallOutboundRingEnabled: isGroupCallOutboundRingEnabled(), incomingCall: mapStateToIncomingCallProp(state), - maxGroupCallRingSize: getMaxGroupCallRingSize(), me: { ...getMe(state), // `getMe` returns a `ConversationType` which might not have a UUID, at least diff --git a/ts/test-both/conversations/isConversationTooBigToRing_test.ts b/ts/test-both/conversations/isConversationTooBigToRing_test.ts new file mode 100644 index 000000000..2dd42c0a2 --- /dev/null +++ b/ts/test-both/conversations/isConversationTooBigToRing_test.ts @@ -0,0 +1,61 @@ +// Copyright 2021 Signal Messenger, LLC +// SPDX-License-Identifier: AGPL-3.0-only + +import { assert } from 'chai'; +import * as sinon from 'sinon'; +import { times } from 'lodash'; +import { v4 as uuid } from 'uuid'; +import * as remoteConfig from '../../RemoteConfig'; + +import { isConversationTooBigToRing } from '../../conversations/isConversationTooBigToRing'; + +describe('isConversationTooBigToRing', () => { + let sinonSandbox: sinon.SinonSandbox; + let getMaxGroupCallRingSizeStub: sinon.SinonStub; + + beforeEach(() => { + sinonSandbox = sinon.createSandbox(); + + const getValueStub = sinonSandbox.stub(remoteConfig, 'getValue'); + getMaxGroupCallRingSizeStub = getValueStub.withArgs( + 'global.calling.maxGroupCallRingSize' + ); + }); + + const fakeMemberships = (count: number) => + times(count, () => ({ conversationId: uuid(), isAdmin: false })); + + afterEach(() => { + sinonSandbox.restore(); + }); + + it('returns false if there are no memberships (i.e., for a direct conversation)', () => { + assert.isFalse(isConversationTooBigToRing({})); + assert.isFalse(isConversationTooBigToRing({ memberships: [] })); + }); + + const textMaximum = (max: number): void => { + for (let count = 1; count < max; count += 1) { + const memberships = fakeMemberships(count); + assert.isFalse(isConversationTooBigToRing({ memberships })); + } + for (let count = max; count < max + 5; count += 1) { + const memberships = fakeMemberships(count); + assert.isTrue(isConversationTooBigToRing({ memberships })); + } + }; + + it('returns whether there are 16 or more people in the group, if there is nothing in remote config', () => { + textMaximum(16); + }); + + it('returns whether there are 16 or more people in the group, if the remote config value is bogus', () => { + getMaxGroupCallRingSizeStub.returns('uh oh'); + textMaximum(16); + }); + + it('returns whether there are 9 or more people in the group, if the remote config value is 9', () => { + getMaxGroupCallRingSizeStub.returns('9'); + textMaximum(9); + }); +}); diff --git a/ts/test-both/groups/limits_test.ts b/ts/test-both/groups/limits_test.ts index b38c15eee..77a79e704 100644 --- a/ts/test-both/groups/limits_test.ts +++ b/ts/test-both/groups/limits_test.ts @@ -8,14 +8,12 @@ import * as remoteConfig from '../../RemoteConfig'; import { getGroupSizeRecommendedLimit, getGroupSizeHardLimit, - getMaxGroupCallRingSize, } from '../../groups/limits'; describe('group limit utilities', () => { let sinonSandbox: sinon.SinonSandbox; let getRecommendedLimitStub: sinon.SinonStub; let getHardLimitStub: sinon.SinonStub; - let getMaxGroupCallRingSizeStub: sinon.SinonStub; beforeEach(() => { sinonSandbox = sinon.createSandbox(); @@ -27,9 +25,6 @@ describe('group limit utilities', () => { getHardLimitStub = getValueStub.withArgs( 'global.groupsv2.groupSizeHardLimit' ); - getMaxGroupCallRingSizeStub = getValueStub.withArgs( - 'global.calling.maxGroupCallRingSize' - ); }); afterEach(() => { @@ -69,21 +64,4 @@ describe('group limit utilities', () => { assert.strictEqual(getGroupSizeHardLimit(), 123); }); }); - - describe('getMaxGroupCallRingSize', () => { - it('returns 16 if the value in remote config is not defined', () => { - getMaxGroupCallRingSizeStub.returns(undefined); - assert.strictEqual(getMaxGroupCallRingSize(), 16); - }); - - it('returns 16 if the value in remote config is not a parseable integer', () => { - getMaxGroupCallRingSizeStub.returns('uh oh'); - assert.strictEqual(getMaxGroupCallRingSize(), 16); - }); - - it('returns the value in remote config, parsed as an integer', () => { - getMaxGroupCallRingSizeStub.returns('123'); - assert.strictEqual(getMaxGroupCallRingSize(), 123); - }); - }); }); diff --git a/ts/test-electron/state/ducks/calling_test.ts b/ts/test-electron/state/ducks/calling_test.ts index d218263e6..7dc764af3 100644 --- a/ts/test-electron/state/ducks/calling_test.ts +++ b/ts/test-electron/state/ducks/calling_test.ts @@ -1639,6 +1639,7 @@ describe('calling duck', () => { conversationId: 'fake-conversation-id', hasLocalAudio: true, hasLocalVideo: true, + isConversationTooBigToRing: false, connectionState: GroupCallConnectionState.Connected, joinState: GroupCallJoinState.NotJoined, peekInfo: { @@ -1701,6 +1702,7 @@ describe('calling duck', () => { conversationId: 'fake-group-call-conversation-id', hasLocalAudio: true, hasLocalVideo: true, + isConversationTooBigToRing: false, connectionState: GroupCallConnectionState.Connected, joinState: GroupCallJoinState.NotJoined, peekInfo: undefined, @@ -1725,6 +1727,7 @@ describe('calling duck', () => { conversationId: 'fake-group-call-conversation-id', hasLocalAudio: true, hasLocalVideo: true, + isConversationTooBigToRing: false, connectionState: GroupCallConnectionState.Connected, joinState: GroupCallJoinState.NotJoined, peekInfo: undefined, @@ -1761,6 +1764,7 @@ describe('calling duck', () => { conversationId: 'fake-group-call-conversation-id', hasLocalAudio: true, hasLocalVideo: true, + isConversationTooBigToRing: false, connectionState: GroupCallConnectionState.Connected, joinState: GroupCallJoinState.NotJoined, peekInfo: { @@ -1814,6 +1818,7 @@ describe('calling duck', () => { conversationId: 'fake-group-call-conversation-id', hasLocalAudio: true, hasLocalVideo: true, + isConversationTooBigToRing: false, connectionState: GroupCallConnectionState.Connected, joinState: GroupCallJoinState.NotJoined, peekInfo: undefined,