From a8346c490e5569b401e729a7289e4b37a82cc5c5 Mon Sep 17 00:00:00 2001 From: Evan Hahn <69474926+EvanHahn-Signal@users.noreply.github.com> Date: Thu, 13 May 2021 09:47:30 -0500 Subject: [PATCH] Fix inaccurate numbers on group details screen --- .../ConversationDetails.stories.tsx | 7 + .../ConversationDetails.tsx | 12 +- .../ConversationDetailsHeader.stories.tsx | 2 +- .../ConversationDetailsHeader.tsx | 5 +- ts/state/smart/ConversationDetails.tsx | 16 +- ts/state/smart/PendingInvites.tsx | 34 +-- ts/test-both/util/getGroupMemberships_test.ts | 235 ++++++++++++++++++ ts/util/getGroupMemberships.ts | 65 +++++ 8 files changed, 324 insertions(+), 52 deletions(-) create mode 100644 ts/test-both/util/getGroupMemberships_test.ts create mode 100644 ts/util/getGroupMemberships.ts diff --git a/ts/components/conversation/conversation-details/ConversationDetails.stories.tsx b/ts/components/conversation/conversation-details/ConversationDetails.stories.tsx index 5cbf07930..b595d0dde 100644 --- a/ts/components/conversation/conversation-details/ConversationDetails.stories.tsx +++ b/ts/components/conversation/conversation-details/ConversationDetails.stories.tsx @@ -45,6 +45,13 @@ const createProps = (hasGroupLink = false): Props => ({ isMe: i === 2, }), })), + pendingApprovalMemberships: times(8, () => ({ + member: getDefaultConversation(), + })), + pendingMemberships: times(5, () => ({ + metadata: {}, + member: getDefaultConversation(), + })), setDisappearingMessages: action('setDisappearingMessages'), showAllMedia: action('showAllMedia'), showContactModal: action('showContactModal'), diff --git a/ts/components/conversation/conversation-details/ConversationDetails.tsx b/ts/components/conversation/conversation-details/ConversationDetails.tsx index fb2c670fd..3987ef56a 100644 --- a/ts/components/conversation/conversation-details/ConversationDetails.tsx +++ b/ts/components/conversation/conversation-details/ConversationDetails.tsx @@ -22,6 +22,10 @@ import { ConversationDetailsMembershipList, GroupV2Membership, } from './ConversationDetailsMembershipList'; +import { + GroupV2PendingMembership, + GroupV2RequestingMembership, +} from './PendingInvites'; import { EditConversationAttributesModal } from './EditConversationAttributesModal'; import { RequestState } from './util'; @@ -41,6 +45,8 @@ export type StateProps = { isAdmin: boolean; loadRecentMediaItems: (limit: number) => void; memberships: Array; + pendingApprovalMemberships: ReadonlyArray; + pendingMemberships: ReadonlyArray; setDisappearingMessages: (seconds: number) => void; showAllMedia: () => void; showContactModal: (conversationId: string) => void; @@ -77,6 +83,8 @@ export const ConversationDetails: React.ComponentType = ({ isAdmin, loadRecentMediaItems, memberships, + pendingApprovalMemberships, + pendingMemberships, setDisappearingMessages, showAllMedia, showContactModal, @@ -108,9 +116,6 @@ export const ConversationDetails: React.ComponentType = ({ throw new Error('ConversationDetails rendered without a conversation'); } - const pendingMemberships = conversation.pendingMemberships || []; - const pendingApprovalMemberships = - conversation.pendingApprovalMemberships || []; const invitesCount = pendingMemberships.length + pendingApprovalMemberships.length; @@ -213,6 +218,7 @@ export const ConversationDetails: React.ComponentType = ({ canEdit={canEditGroupInfo} conversation={conversation} i18n={i18n} + memberships={memberships} startEditing={() => { setModalState(ModalState.EditingGroupAttributes); }} diff --git a/ts/components/conversation/conversation-details/ConversationDetailsHeader.stories.tsx b/ts/components/conversation/conversation-details/ConversationDetailsHeader.stories.tsx index 1e9b3ac8b..b31d529f4 100644 --- a/ts/components/conversation/conversation-details/ConversationDetailsHeader.stories.tsx +++ b/ts/components/conversation/conversation-details/ConversationDetailsHeader.stories.tsx @@ -27,7 +27,6 @@ const createConversation = (): ConversationType => type: 'group', lastUpdated: 0, title: text('conversation title', 'Some Conversation'), - memberships: new Array(number('conversation members length', 0)), }); const createProps = (overrideProps: Partial = {}): Props => ({ @@ -35,6 +34,7 @@ const createProps = (overrideProps: Partial = {}): Props => ({ i18n, canEdit: false, startEditing: action('startEditing'), + memberships: new Array(number('conversation members length', 0)), ...overrideProps, }); diff --git a/ts/components/conversation/conversation-details/ConversationDetailsHeader.tsx b/ts/components/conversation/conversation-details/ConversationDetailsHeader.tsx index 5deb85a66..bdf54f6dd 100644 --- a/ts/components/conversation/conversation-details/ConversationDetailsHeader.tsx +++ b/ts/components/conversation/conversation-details/ConversationDetailsHeader.tsx @@ -7,12 +7,14 @@ import { Avatar } from '../../Avatar'; import { Emojify } from '../Emojify'; import { LocalizerType } from '../../../types/Util'; import { ConversationType } from '../../../state/ducks/conversations'; +import { GroupV2Membership } from './ConversationDetailsMembershipList'; import { bemGenerator } from './util'; export type Props = { canEdit: boolean; conversation: ConversationType; i18n: LocalizerType; + memberships: Array; startEditing: () => void; }; @@ -22,10 +24,9 @@ export const ConversationDetailsHeader: React.ComponentType = ({ canEdit, conversation, i18n, + memberships, startEditing, }) => { - const memberships = conversation.memberships || []; - const contents = ( <> , membership) => { - const member = conversationSelector(membership.conversationId); - if (!member || isConversationUnregistered(member)) { - return result; - } - return [...result, { isAdmin: membership.isAdmin, member }]; - }, - [] - ); - const isAdmin = Boolean(conversation?.areWeAdmin); const candidateContactsToAdd = getCandidateContactsForNewGroup(state); @@ -80,7 +68,7 @@ const mapStateToProps = ( conversation, i18n: getIntl(state), isAdmin, - memberships, + ...getGroupMemberships(conversation, conversationSelector), }; }; diff --git a/ts/state/smart/PendingInvites.tsx b/ts/state/smart/PendingInvites.tsx index 366d6d95f..38a479fca 100644 --- a/ts/state/smart/PendingInvites.tsx +++ b/ts/state/smart/PendingInvites.tsx @@ -4,8 +4,6 @@ import { connect } from 'react-redux'; import { mapDispatchToProps } from '../actions'; import { - GroupV2PendingMembership, - GroupV2RequestingMembership, PendingInvites, PropsType, } from '../../components/conversation/conversation-details/PendingInvites'; @@ -13,7 +11,7 @@ import { StateType } from '../reducer'; import { getIntl } from '../selectors/user'; import { getConversationByIdSelector } from '../selectors/conversations'; -import { isConversationUnregistered } from '../../util/isConversationUnregistered'; +import { getGroupMemberships } from '../../util/getGroupMemberships'; import { assert } from '../../util/assert'; export type SmartPendingInvitesProps = { @@ -35,39 +33,11 @@ const mapStateToProps = ( ' expected a conversation to be found' ); - const pendingApprovalMemberships = ( - conversation.pendingApprovalMemberships || [] - ).reduce((result: Array, membership) => { - const member = conversationSelector(membership.conversationId); - if (!member || isConversationUnregistered(member)) { - return result; - } - return [...result, { member }]; - }, []); - - const pendingMemberships = (conversation.pendingMemberships || []).reduce( - (result: Array, membership) => { - const member = conversationSelector(membership.conversationId); - if (!member || isConversationUnregistered(member)) { - return result; - } - return [ - ...result, - { - member, - metadata: { addedByUserId: membership.addedByUserId }, - }, - ]; - }, - [] - ); - return { ...props, + ...getGroupMemberships(conversation, conversationSelector), conversation, i18n: getIntl(state), - pendingApprovalMemberships, - pendingMemberships, }; }; diff --git a/ts/test-both/util/getGroupMemberships_test.ts b/ts/test-both/util/getGroupMemberships_test.ts new file mode 100644 index 000000000..fab726ff8 --- /dev/null +++ b/ts/test-both/util/getGroupMemberships_test.ts @@ -0,0 +1,235 @@ +// Copyright 2021 Signal Messenger, LLC +// SPDX-License-Identifier: AGPL-3.0-only + +import { assert } from 'chai'; +import { ConversationType } from '../../state/ducks/conversations'; +import { getDefaultConversation } from '../helpers/getDefaultConversation'; + +import { getGroupMemberships } from '../../util/getGroupMemberships'; + +describe('getGroupMemberships', () => { + const normalConversation1 = getDefaultConversation(); + const normalConversation2 = getDefaultConversation(); + const unregisteredConversation = getDefaultConversation({ + discoveredUnregisteredAt: Date.now(), + }); + + function getConversationById(id: string): undefined | ConversationType { + return [ + normalConversation1, + normalConversation2, + unregisteredConversation, + ].find(conversation => conversation.id === id); + } + + describe('memberships', () => { + it('returns an empty array if passed undefined', () => { + const conversation = {}; + + const result = getGroupMemberships(conversation, getConversationById) + .memberships; + + assert.isEmpty(result); + }); + + it('returns an empty array if passed an empty array', () => { + const conversation = { memberships: [] }; + + const result = getGroupMemberships(conversation, getConversationById) + .memberships; + + assert.isEmpty(result); + }); + + it("filters out conversation IDs that don't exist", () => { + const conversation = { + memberships: [ + { + conversationId: 'garbage', + isAdmin: true, + }, + ], + }; + + const result = getGroupMemberships(conversation, getConversationById) + .memberships; + + assert.isEmpty(result); + }); + + it('filters out unregistered conversations', () => { + const conversation = { + memberships: [ + { + conversationId: unregisteredConversation.id, + isAdmin: true, + }, + ], + }; + + const result = getGroupMemberships(conversation, getConversationById) + .memberships; + + assert.isEmpty(result); + }); + + it('hydrates memberships', () => { + const conversation = { + memberships: [ + { + conversationId: normalConversation2.id, + isAdmin: false, + }, + { + conversationId: normalConversation1.id, + isAdmin: true, + }, + ], + }; + + const result = getGroupMemberships(conversation, getConversationById) + .memberships; + + assert.lengthOf(result, 2); + assert.deepEqual(result[0], { + isAdmin: false, + member: normalConversation2, + }); + assert.deepEqual(result[1], { + isAdmin: true, + member: normalConversation1, + }); + }); + }); + + describe('pendingApprovalMemberships', () => { + it('returns an empty array if passed undefined', () => { + const conversation = {}; + + const result = getGroupMemberships(conversation, getConversationById) + .pendingApprovalMemberships; + + assert.isEmpty(result); + }); + + it('returns an empty array if passed an empty array', () => { + const conversation = { pendingApprovalMemberships: [] }; + + const result = getGroupMemberships(conversation, getConversationById) + .pendingApprovalMemberships; + + assert.isEmpty(result); + }); + + it("filters out conversation IDs that don't exist", () => { + const conversation = { + pendingApprovalMemberships: [{ conversationId: 'garbage' }], + }; + + const result = getGroupMemberships(conversation, getConversationById) + .pendingApprovalMemberships; + + assert.isEmpty(result); + }); + + it('filters out unregistered conversations', () => { + const conversation = { + pendingApprovalMemberships: [ + { conversationId: unregisteredConversation.id }, + ], + }; + + const result = getGroupMemberships(conversation, getConversationById) + .pendingApprovalMemberships; + + assert.isEmpty(result); + }); + + it('hydrates pending-approval memberships', () => { + const conversation = { + pendingApprovalMemberships: [ + { conversationId: normalConversation2.id }, + { conversationId: normalConversation1.id }, + ], + }; + + const result = getGroupMemberships(conversation, getConversationById) + .pendingApprovalMemberships; + + assert.lengthOf(result, 2); + assert.deepEqual(result[0], { member: normalConversation2 }); + assert.deepEqual(result[1], { member: normalConversation1 }); + }); + }); + + describe('pendingMemberships', () => { + it('returns an empty array if passed undefined', () => { + const conversation = {}; + + const result = getGroupMemberships(conversation, getConversationById) + .pendingMemberships; + + assert.isEmpty(result); + }); + + it('returns an empty array if passed an empty array', () => { + const conversation = { pendingMemberships: [] }; + + const result = getGroupMemberships(conversation, getConversationById) + .pendingMemberships; + + assert.isEmpty(result); + }); + + it("filters out conversation IDs that don't exist", () => { + const conversation = { + pendingMemberships: [ + { conversationId: 'garbage', addedByUserId: normalConversation1.id }, + ], + }; + + const result = getGroupMemberships(conversation, getConversationById) + .pendingMemberships; + + assert.isEmpty(result); + }); + + it('filters out unregistered conversations', () => { + const conversation = { + pendingMemberships: [ + { + conversationId: unregisteredConversation.id, + addedByUserId: normalConversation1.id, + }, + ], + }; + + const result = getGroupMemberships(conversation, getConversationById) + .pendingMemberships; + + assert.isEmpty(result); + }); + + it('hydrates pending memberships', () => { + const conversation = { + pendingMemberships: [ + { conversationId: normalConversation2.id, addedByUserId: 'abc' }, + { conversationId: normalConversation1.id, addedByUserId: 'xyz' }, + ], + }; + + const result = getGroupMemberships(conversation, getConversationById) + .pendingMemberships; + + assert.lengthOf(result, 2); + assert.deepEqual(result[0], { + member: normalConversation2, + metadata: { addedByUserId: 'abc' }, + }); + assert.deepEqual(result[1], { + member: normalConversation1, + metadata: { addedByUserId: 'xyz' }, + }); + }); + }); +}); diff --git a/ts/util/getGroupMemberships.ts b/ts/util/getGroupMemberships.ts new file mode 100644 index 000000000..0d0362022 --- /dev/null +++ b/ts/util/getGroupMemberships.ts @@ -0,0 +1,65 @@ +// Copyright 2021 Signal Messenger, LLC +// SPDX-License-Identifier: AGPL-3.0-only + +import { GroupV2Membership } from '../components/conversation/conversation-details/ConversationDetailsMembershipList'; +import { + GroupV2PendingMembership, + GroupV2RequestingMembership, +} from '../components/conversation/conversation-details/PendingInvites'; +import { ConversationType } from '../state/ducks/conversations'; +import { isConversationUnregistered } from './isConversationUnregistered'; + +export const getGroupMemberships = ( + { + memberships = [], + pendingApprovalMemberships = [], + pendingMemberships = [], + }: Readonly< + Pick< + ConversationType, + 'memberships' | 'pendingApprovalMemberships' | 'pendingMemberships' + > + >, + getConversationById: (conversationId: string) => undefined | ConversationType +): { + memberships: Array; + pendingApprovalMemberships: Array; + pendingMemberships: Array; +} => ({ + memberships: memberships.reduce( + (result: Array, membership) => { + const member = getConversationById(membership.conversationId); + if (!member || isConversationUnregistered(member)) { + return result; + } + return [...result, { isAdmin: membership.isAdmin, member }]; + }, + [] + ), + pendingApprovalMemberships: pendingApprovalMemberships.reduce( + (result: Array, membership) => { + const member = getConversationById(membership.conversationId); + if (!member || isConversationUnregistered(member)) { + return result; + } + return [...result, { member }]; + }, + [] + ), + pendingMemberships: pendingMemberships.reduce( + (result: Array, membership) => { + const member = getConversationById(membership.conversationId); + if (!member || isConversationUnregistered(member)) { + return result; + } + return [ + ...result, + { + member, + metadata: { addedByUserId: membership.addedByUserId }, + }, + ]; + }, + [] + ), +});