From e9b7a74b32c120cf4c183f59d9f99dc516fa395c Mon Sep 17 00:00:00 2001 From: Scott Nonnenberg Date: Tue, 6 Dec 2022 13:12:57 -0800 Subject: [PATCH] GroupV2: Show summary of change details on re-join of group --- _locales/en/messages.json | 4 + stylesheets/components/SystemMessage.scss | 7 + .../conversation/GroupV2Change.stories.tsx | 85 +++++++++---- ts/components/conversation/GroupV2Change.tsx | 4 + ts/groupChange.ts | 3 + ts/groups.ts | 120 +++++++++++++++--- 6 files changed, 181 insertions(+), 42 deletions(-) diff --git a/_locales/en/messages.json b/_locales/en/messages.json index f395b9fe7876..5232a90adcd1 100644 --- a/_locales/en/messages.json +++ b/_locales/en/messages.json @@ -4099,6 +4099,10 @@ "message": "The group was changed to allow all members to send messages.", "description": "Shown in timeline or conversation preview when v2 group changes" }, + "icu:GroupV2--summary": { + "messageformat": "This group's members or settings have changed.", + "description": "When rejoining a group, any detected changes are collapsed down into this summary" + }, "GroupV1--Migration--disabled": { "message": "Upgrade this group to activate new features like @mentions and admins. Members who have not shared their name or photo in this group will be invited to join. $learnMore$", "description": "Shown instead of composition area when user is forced to migrate a legacy group (GV1)." diff --git a/stylesheets/components/SystemMessage.scss b/stylesheets/components/SystemMessage.scss index 6706d5f8ea85..54b5d98d60dd 100644 --- a/stylesheets/components/SystemMessage.scss +++ b/stylesheets/components/SystemMessage.scss @@ -156,6 +156,13 @@ ); } + &--icon-group-summary::before { + @include system-message-icon( + '../images/icons/v2/info-16.svg', + '../images/icons/v2/info-16.svg' + ); + } + &--icon-info::before { @include system-message-icon( '../images/icons/v2/info-16.svg', diff --git a/ts/components/conversation/GroupV2Change.stories.tsx b/ts/components/conversation/GroupV2Change.stories.tsx index 693220e6e5fd..0ea3db2d58f2 100644 --- a/ts/components/conversation/GroupV2Change.stories.tsx +++ b/ts/components/conversation/GroupV2Change.stories.tsx @@ -1278,14 +1278,45 @@ export function AdminApprovalRemove(): JSX.Element { }, ], })} + + ); +} + +AdminApprovalRemove.story = { + name: 'Admin Approval (Remove)', +}; + +export function AdminApprovalBounce(): JSX.Element { + return ( + <> Should show button: {renderChange( { + // From Joiner from: CONTACT_A, details: [ { - type: 'admin-approval-remove-one', + type: 'admin-approval-bounce', uuid: CONTACT_A, + times: 1, + isApprovalPending: false, + }, + ], + }, + { + groupMemberships: [{ uuid: CONTACT_C, isAdmin: false }], + groupBannedMemberships: [CONTACT_B], + } + )} + {renderChange( + { + // From nobody + details: [ + { + type: 'admin-approval-bounce', + uuid: CONTACT_A, + times: 1, + isApprovalPending: false, }, ], }, @@ -1295,37 +1326,26 @@ export function AdminApprovalRemove(): JSX.Element { } )} {renderChange({ - from: ADMIN_A, details: [ { - type: 'admin-approval-remove-one', + type: 'admin-approval-bounce', uuid: CONTACT_A, + times: 1, + isApprovalPending: false, }, + // No group membership info ], })} - Should show button: - {renderChange( - { - details: [ - { - type: 'admin-approval-remove-one', - uuid: CONTACT_A, - }, - ], - }, - { - groupMemberships: [{ uuid: CONTACT_C, isAdmin: false }], - groupBannedMemberships: [CONTACT_B], - } - )} Would show button, but we're not admin: {renderChange( { from: CONTACT_A, details: [ { - type: 'admin-approval-remove-one', + type: 'admin-approval-bounce', uuid: CONTACT_A, + times: 1, + isApprovalPending: false, }, ], }, @@ -1338,8 +1358,10 @@ export function AdminApprovalRemove(): JSX.Element { from: CONTACT_A, details: [ { - type: 'admin-approval-remove-one', + type: 'admin-approval-bounce', uuid: CONTACT_A, + times: 1, + isApprovalPending: false, }, ], }, @@ -1351,8 +1373,10 @@ export function AdminApprovalRemove(): JSX.Element { from: CONTACT_A, details: [ { - type: 'admin-approval-remove-one', + type: 'admin-approval-bounce', uuid: CONTACT_A, + times: 1, + isApprovalPending: false, }, ], }, @@ -1363,8 +1387,8 @@ export function AdminApprovalRemove(): JSX.Element { ); } -AdminApprovalRemove.story = { - name: 'Admin Approval (Remove)', +AdminApprovalBounce.story = { + name: 'Admin Approval (Bounce)', }; export function GroupLinkAdd(): JSX.Element { @@ -1646,3 +1670,18 @@ export function AnnouncementGroupChange(): JSX.Element { AnnouncementGroupChange.story = { name: 'Announcement Group (Change)', }; + +export function Summary(): JSX.Element { + return ( + <> + {renderChange({ + from: OUR_ACI, + details: [ + { + type: 'summary', + }, + ], + })} + + ); +} diff --git a/ts/components/conversation/GroupV2Change.tsx b/ts/components/conversation/GroupV2Change.tsx index 0593cf64e8d1..89d226b3a1aa 100644 --- a/ts/components/conversation/GroupV2Change.tsx +++ b/ts/components/conversation/GroupV2Change.tsx @@ -70,6 +70,7 @@ type GroupIconType = | 'group-avatar' | 'group-decline' | 'group-edit' + | 'group-summary' | 'group-leave' | 'group-remove'; @@ -120,6 +121,9 @@ function getIcon( if (changeType === 'admin-approval-bounce' && isLastText) { possibleIcon = undefined; } + if (changeType === 'summary') { + possibleIcon = 'group-summary'; + } return possibleIcon || 'group'; } diff --git a/ts/groupChange.ts b/ts/groupChange.ts index 0dc2d9e89db2..30dc1dc69c3c 100644 --- a/ts/groupChange.ts +++ b/ts/groupChange.ts @@ -925,6 +925,9 @@ export function renderChangeDetail( } return renderString('GroupV2--announcements--member--unknown', i18n); } + if (detail.type === 'summary') { + return renderString('icu:GroupV2--summary', i18n); + } throw missingCaseError(detail); } diff --git a/ts/groups.ts b/ts/groups.ts index 3f0b9e63ba1c..d58a287ee0d6 100644 --- a/ts/groups.ts +++ b/ts/groups.ts @@ -202,6 +202,9 @@ export type GroupV2DescriptionChangeType = { // Adding this field; cannot remove previous field for backwards compatibility description?: string; }; +export type GroupV2SummaryType = { + type: 'summary'; +}; export type GroupV2ChangeDetailType = | GroupV2AccessAttributesChangeType @@ -227,6 +230,7 @@ export type GroupV2ChangeDetailType = | GroupV2PendingAddOneChangeType | GroupV2PendingRemoveManyChangeType | GroupV2PendingRemoveOneChangeType + | GroupV2SummaryType | GroupV2TitleChangeType; export type GroupV2ChangeType = { @@ -3658,32 +3662,62 @@ async function updateGroupViaSingleChange({ groupChange: Proto.IGroupChange; newRevision: number; }): Promise { + const previouslyKnewAboutThisGroup = + isNumber(group.revision) && group.membersV2?.length; const wasInGroup = !group.left; - const result: UpdatesResultType = await integrateGroupChange({ + const singleChangeResult: UpdatesResultType = await integrateGroupChange({ group, groupChange, newRevision, }); - const nowInGroup = !result.newAttributes.left; + const nowInGroup = !singleChangeResult.newAttributes.left; // If we were just added to the group (for example, via a join link), we go fetch the // entire group state to make sure we're up to date. if (!wasInGroup && nowInGroup) { - const { newAttributes, members } = await updateGroupViaState({ - group: result.newAttributes, + const { + newAttributes, + members, + groupChangeMessages: catchupMessages, + } = await updateGroupViaState({ + group: singleChangeResult.newAttributes, }); + const groupChangeMessages = [...singleChangeResult.groupChangeMessages]; + // If we've just been added to a group we were previously in, we do want to show + // a summary instead of nothing. + if ( + groupChangeMessages.length > 0 && + previouslyKnewAboutThisGroup && + catchupMessages.length > 0 + ) { + groupChangeMessages.push({ + ...generateBasicMessage(), + type: 'group-v2-change', + groupV2Change: { + details: [ + { + type: 'summary', + }, + ], + }, + readStatus: ReadStatus.Read, + // For simplicity, since we don't know who this change is from here, always Seen + seenStatus: SeenStatus.Seen, + }); + } + // We discard any change events that come out of this full group fetch, but we do // keep the final group attributes generated, as well as any new members. return { - ...result, - members: [...result.members, ...members], + groupChangeMessages, + members: [...singleChangeResult.members, ...members], newAttributes, }; } - return result; + return singleChangeResult; } async function updateGroupViaLogs({ @@ -4170,6 +4204,21 @@ function extractDiffs({ let areWePendingApproval = false; let whoInvitedUsUserId = null; + function isUs(uuid: UUIDStringType): boolean { + return uuid === ourACI.toString() || uuid === ourPNI?.toString(); + } + function keepOnlyOurAdds( + list: Array + ): Array { + return list.filter( + item => + (item.type === 'member-add-from-invite' && isUs(item.uuid)) || + (item.type === 'member-add-from-link' && isUs(item.uuid)) || + (item.type === 'member-add-from-admin-approval' && isUs(item.uuid)) || + (item.type === 'member-add' && isUs(item.uuid)) + ); + } + // access control if ( @@ -4274,6 +4323,10 @@ function extractDiffs({ const oldMemberLookup = new Map( (old.membersV2 || []).map(member => [member.uuid, member]) ); + const didWeStartInGroup = + (ourACI && oldMemberLookup.get(ourACI.toString())) || + (ourPNI && oldMemberLookup.get(ourPNI.toString())); + const oldPendingMemberLookup = new Map< UUIDStringType, GroupV2PendingMemberType @@ -4288,16 +4341,16 @@ function extractDiffs({ (current.membersV2 || []).forEach(currentMember => { const { uuid } = currentMember; - const isUs = uuid === ourACI.toString(); + const uuidIsUs = isUs(uuid); - if (isUs) { + if (uuidIsUs) { areWeInGroup = true; } const oldMember = oldMemberLookup.get(uuid); if (!oldMember) { let pendingMember = oldPendingMemberLookup.get(uuid); - if (isUs && ourPNI && !pendingMember) { + if (uuidIsUs && ourPNI && !pendingMember) { pendingMember = oldPendingMemberLookup.get(ourPNI.toString()); } if (pendingMember) { @@ -4347,7 +4400,7 @@ function extractDiffs({ // pretend that the PNI wasn't pending so that we won't generate a // pending-add-one notification below. if ( - isUs && + uuidIsUs && ourPNI && !oldMember && oldPendingMemberLookup.has(ourPNI.toString()) && @@ -4373,7 +4426,7 @@ function extractDiffs({ const { uuid } = currentPendingMember; const oldPendingMember = oldPendingMemberLookup.get(uuid); - if (uuid === ourACI.toString() || uuid === ourPNI?.toString()) { + if (isUs(uuid)) { if (uuid === ourACI.toString()) { uuidKindInvitedToGroup = UUIDKind.ACI; } else if (uuidKindInvitedToGroup === undefined) { @@ -4494,8 +4547,9 @@ function extractDiffs({ const firstUpdate = !isNumber(old.revision); const isFromUs = ourACI.toString() === sourceUuid; + const justJoinedGroup = !firstUpdate && !didWeStartInGroup && areWeInGroup; - // Here we hardcode initial messages if this is our first time processing data this + // Here we hardcode initial messages if this is our first time processing data for this // group. Ideally we can collapse it down to just one of: 'you were added', // 'you were invited', or 'you created.' if (firstUpdate && uuidKindInvitedToGroup !== undefined) { @@ -4554,17 +4608,19 @@ function extractDiffs({ seenStatus: isFromUs ? SeenStatus.Seen : SeenStatus.Unseen, }; } else if (firstUpdate && areWeInGroup) { + const filteredDetails = keepOnlyOurAdds(details); + + strictAssert( + filteredDetails.length === 1, + 'extractDiffs/firstUpdate: Should be only one self-add!' + ); + message = { ...generateBasicMessage(), type: 'group-v2-change', groupV2Change: { from: sourceUuid, - details: [ - { - type: 'member-add', - uuid: ourACI.toString(), - }, - ], + details: filteredDetails, }, readStatus: ReadStatus.Read, seenStatus: isFromUs ? SeenStatus.Seen : SeenStatus.Unseen, @@ -4584,6 +4640,32 @@ function extractDiffs({ readStatus: ReadStatus.Read, seenStatus: isFromUs ? SeenStatus.Seen : SeenStatus.Unseen, }; + } else if (justJoinedGroup) { + const filteredDetails = keepOnlyOurAdds(details); + + strictAssert( + filteredDetails.length === 1, + 'extractDiffs/justJoinedGroup: Should be only one self-add!' + ); + + // If we've dropped other changes, we collapse them into a single summary + if (details.length > 1) { + filteredDetails.push({ + type: 'summary', + }); + } + + message = { + ...generateBasicMessage(), + type: 'group-v2-change', + sourceUuid, + groupV2Change: { + from: sourceUuid, + details: filteredDetails, + }, + readStatus: ReadStatus.Read, + seenStatus: isFromUs ? SeenStatus.Seen : SeenStatus.Unseen, + }; } else if (details.length > 0) { message = { ...generateBasicMessage(),