diff --git a/ts/state/selectors/conversations.ts b/ts/state/selectors/conversations.ts index 9856c0d9fd..d36e26cba4 100644 --- a/ts/state/selectors/conversations.ts +++ b/ts/state/selectors/conversations.ts @@ -252,18 +252,21 @@ export const _getLeftPaneLists = ( const max = values.length; for (let i = 0; i < max; i += 1) { let conversation = values[i]; - if (conversation.activeAt) { - if (selectedConversation === conversation.id) { - conversation = { - ...conversation, - isSelected: true, - }; - } + if (selectedConversation === conversation.id) { + conversation = { + ...conversation, + isSelected: true, + }; + } + // We always show pinned conversations + if (conversation.isPinned) { + pinnedConversations.push(conversation); + } + + if (conversation.activeAt) { if (conversation.isArchived) { archivedConversations.push(conversation); - } else if (conversation.isPinned) { - pinnedConversations.push(conversation); } else { conversations.push(conversation); } @@ -352,12 +355,37 @@ export const getComposerConversationSearchTerm = createSelector( } ); +function isTrusted(conversation: ConversationType): boolean { + if (conversation.type === 'group') { + return true; + } + + return Boolean( + isString(conversation.name) || + conversation.profileSharing || + conversation.isMe + ); +} + +function hasDisplayInfo(conversation: ConversationType): boolean { + if (conversation.type === 'group') { + return Boolean(conversation.name); + } + + return Boolean( + conversation.name || + conversation.profileName || + conversation.phoneNumber || + conversation.isMe + ); +} + function canComposeConversation(conversation: ConversationType): boolean { return Boolean( - !conversation.isMe && - !conversation.isBlocked && + !conversation.isBlocked && !isConversationUnregistered(conversation) && - (isString(conversation.name) || conversation.profileSharing) + hasDisplayInfo(conversation) && + isTrusted(conversation) ); } @@ -371,28 +399,22 @@ export const getAllComposableConversations = createSelector( !isConversationUnregistered(conversation) && // All conversation should have a title except in weird cases where // they don't, in that case we don't want to show these for Forwarding. - conversation.title - ) -); - -const getContactsAndMe = createSelector( - getConversationLookup, - (conversationLookup: ConversationLookupType): Array => - Object.values(conversationLookup).filter( - contact => - contact.type === 'direct' && - !contact.isBlocked && - !isConversationUnregistered(contact) && - (isString(contact.name) || contact.profileSharing) + conversation.title && + hasDisplayInfo(conversation) ) ); /** - * This returns contacts for the composer and group members, which isn't just your primary - * system contacts. It may include false positives, which is better than missing contacts. + * getComposableContacts/getCandidateContactsForNewGroup both return contacts for the + * composer and group members, a different list from your primary system contacts. + * This list may include false positives, which is better than missing contacts. * - * Because it filters unregistered contacts and that's (partially) determined by the - * current time, it's possible for this to return stale contacts that have unregistered + * Note: the key difference between them: + * getComposableContacts includes Note to Self + * getCandidateContactsForNewGroup does not include Note to Self + * + * Because they filter unregistered contacts and that's (partially) determined by the + * current time, it's possible for them to return stale contacts that have unregistered * if no other conversations change. This should be a rare false positive. */ export const getComposableContacts = createSelector( @@ -404,6 +426,17 @@ export const getComposableContacts = createSelector( ) ); +export const getCandidateContactsForNewGroup = createSelector( + getConversationLookup, + (conversationLookup: ConversationLookupType): Array => + Object.values(conversationLookup).filter( + conversation => + conversation.type === 'direct' && + !conversation.isMe && + canComposeConversation(conversation) + ) +); + export const getComposableGroups = createSelector( getConversationLookup, (conversationLookup: ConversationLookupType): Array => @@ -418,9 +451,9 @@ const getNormalizedComposerConversationSearchTerm = createSelector( (searchTerm: string): string => searchTerm.trim() ); -export const getComposeContacts = createSelector( +export const getFilteredComposeContacts = createSelector( getNormalizedComposerConversationSearchTerm, - getContactsAndMe, + getComposableContacts, ( searchTerm: string, contacts: Array @@ -429,7 +462,7 @@ export const getComposeContacts = createSelector( } ); -export const getComposeGroups = createSelector( +export const getFilteredComposeGroups = createSelector( getNormalizedComposerConversationSearchTerm, getComposableGroups, ( @@ -440,8 +473,8 @@ export const getComposeGroups = createSelector( } ); -export const getCandidateContactsForNewGroup = createSelector( - getComposableContacts, +export const getFilteredCandidateContactsForNewGroup = createSelector( + getCandidateContactsForNewGroup, getNormalizedComposerConversationSearchTerm, filterAndSortConversationsByTitle ); diff --git a/ts/state/smart/ConversationDetails.tsx b/ts/state/smart/ConversationDetails.tsx index 0ee98edb3f..aa9997e79b 100644 --- a/ts/state/smart/ConversationDetails.tsx +++ b/ts/state/smart/ConversationDetails.tsx @@ -9,7 +9,7 @@ import { StateProps, } from '../../components/conversation/conversation-details/ConversationDetails'; import { - getComposableContacts, + getCandidateContactsForNewGroup, getConversationByIdSelector, } from '../selectors/conversations'; import { GroupV2Membership } from '../../components/conversation/conversation-details/ConversationDetailsMembershipList'; @@ -71,7 +71,7 @@ const mapStateToProps = ( ); const isAdmin = Boolean(conversation?.areWeAdmin); - const candidateContactsToAdd = getComposableContacts(state); + const candidateContactsToAdd = getCandidateContactsForNewGroup(state); return { ...props, diff --git a/ts/state/smart/LeftPane.tsx b/ts/state/smart/LeftPane.tsx index a4b1af24f3..84481a6f25 100644 --- a/ts/state/smart/LeftPane.tsx +++ b/ts/state/smart/LeftPane.tsx @@ -16,10 +16,10 @@ import { ComposerStep, OneTimeModalState } from '../ducks/conversations'; import { getSearchResults, isSearching } from '../selectors/search'; import { getIntl, getRegionCode } from '../selectors/user'; import { - getCandidateContactsForNewGroup, + getFilteredCandidateContactsForNewGroup, getCantAddContactForModal, - getComposeContacts, - getComposeGroups, + getFilteredComposeContacts, + getFilteredComposeGroups, getComposeGroupAvatar, getComposeGroupName, getComposeSelectedContacts, @@ -96,15 +96,15 @@ const getModeSpecificProps = ( case ComposerStep.StartDirectConversation: return { mode: LeftPaneMode.Compose, - composeContacts: getComposeContacts(state), - composeGroups: getComposeGroups(state), + composeContacts: getFilteredComposeContacts(state), + composeGroups: getFilteredComposeGroups(state), regionCode: getRegionCode(state), searchTerm: getComposerConversationSearchTerm(state), }; case ComposerStep.ChooseGroupMembers: return { mode: LeftPaneMode.ChooseGroupMembers, - candidateContacts: getCandidateContactsForNewGroup(state), + candidateContacts: getFilteredCandidateContactsForNewGroup(state), cantAddContactForModal: getCantAddContactForModal(state), isShowingRecommendedGroupSizeModal: getRecommendedGroupSizeModalState(state) === diff --git a/ts/test-both/state/selectors/conversations_test.ts b/ts/test-both/state/selectors/conversations_test.ts index b03f4c9d2a..4f70aaac86 100644 --- a/ts/test-both/state/selectors/conversations_test.ts +++ b/ts/test-both/state/selectors/conversations_test.ts @@ -16,16 +16,19 @@ import { getAllComposableConversations, getCandidateContactsForNewGroup, getCantAddContactForModal, - getComposeContacts, - getComposeGroups, + getComposableContacts, + getComposableGroups, getComposeGroupAvatar, getComposeGroupName, - getComposeSelectedContacts, getComposerConversationSearchTerm, getComposerStep, + getComposeSelectedContacts, getConversationByIdSelector, - getConversationSelector, getConversationsByTitleSelector, + getConversationSelector, + getFilteredCandidateContactsForNewGroup, + getFilteredComposeContacts, + getFilteredComposeGroups, getInvitedContactsForNewlyCreatedGroup, getMaximumGroupSizeModalState, getPlaceholderContact, @@ -486,6 +489,7 @@ describe('both/state/selectors/conversations', () => { 'our-conversation-id': { ...getDefaultConversation('our-conversation-id'), isMe: true, + profileName: 'My own name', }, }, }, @@ -502,27 +506,33 @@ describe('both/state/selectors/conversations', () => { Object.assign(result.conversations.conversationLookup, { 'convo-1': { ...getDefaultConversation('convo-1'), + type: 'direct', + profileName: 'A', title: 'A', }, 'convo-2': { ...getDefaultConversation('convo-2'), type: 'group', isGroupV1AndDisabled: true, + name: '2', title: 'Should Be Dropped (GV1)', }, 'convo-3': { ...getDefaultConversation('convo-3'), type: 'group', + name: 'B', title: 'B', }, 'convo-4': { ...getDefaultConversation('convo-4'), isBlocked: true, + name: '4', title: 'Should Be Dropped (blocked)', }, 'convo-5': { ...getDefaultConversation('convo-5'), discoveredUnregisteredAt: new Date(1999, 3, 20).getTime(), + name: 'C', title: 'C', }, 'convo-6': { @@ -534,6 +544,7 @@ describe('both/state/selectors/conversations', () => { 'convo-7': { ...getDefaultConversation('convo-7'), discoveredUnregisteredAt: Date.now(), + name: '7', title: 'Should Be Dropped (unregistered)', }, }); @@ -554,7 +565,254 @@ describe('both/state/selectors/conversations', () => { }); }); - describe('#getComposeContacts', () => { + describe('#getComposableContacts', () => { + const getRootState = (): StateType => { + const rootState = getEmptyRootState(); + return { + ...rootState, + conversations: { + ...getEmptyState(), + conversationLookup: { + 'our-conversation-id': { + ...getDefaultConversation('our-conversation-id'), + isMe: true, + }, + }, + }, + user: { + ...rootState.user, + ourConversationId: 'our-conversation-id', + i18n, + }, + }; + }; + + it('returns only direct contacts, including me', () => { + const state = { + ...getRootState(), + conversations: { + ...getEmptyState(), + conversationLookup: { + 'convo-0': { + ...getDefaultConversation('convo-0'), + isMe: true, + profileSharing: false, + }, + 'convo-1': { + ...getDefaultConversation('convo-1'), + type: 'group' as const, + name: 'Friends!', + }, + 'convo-2': { + ...getDefaultConversation('convo-2'), + name: 'Alice', + }, + }, + }, + }; + + const result = getComposableContacts(state); + + const ids = result.map(group => group.id); + assert.deepEqual(ids, ['convo-0', 'convo-2']); + }); + it('excludes blocked, unregistered, and missing name/profileSharing', () => { + const state = { + ...getRootState(), + conversations: { + ...getEmptyState(), + conversationLookup: { + 'convo-0': { + ...getDefaultConversation('convo-0'), + name: 'Ex', + isBlocked: true, + }, + 'convo-1': { + ...getDefaultConversation('convo-1'), + name: 'Bob', + discoveredUnregisteredAt: Date.now(), + }, + 'convo-2': { + ...getDefaultConversation('convo-2'), + name: 'Charlie', + }, + }, + }, + }; + + const result = getComposableContacts(state); + + const ids = result.map(group => group.id); + assert.deepEqual(ids, ['convo-2']); + }); + }); + + describe('#getCandidateContactsForNewGroup', () => { + const getRootState = (): StateType => { + const rootState = getEmptyRootState(); + return { + ...rootState, + conversations: { + ...getEmptyState(), + conversationLookup: { + 'our-conversation-id': { + ...getDefaultConversation('our-conversation-id'), + isMe: true, + }, + }, + }, + user: { + ...rootState.user, + ourConversationId: 'our-conversation-id', + i18n, + }, + }; + }; + + it('returns only direct contacts, without me', () => { + const state = { + ...getRootState(), + conversations: { + ...getEmptyState(), + conversationLookup: { + 'convo-0': { + ...getDefaultConversation('convo-0'), + isMe: true, + name: 'Me!', + }, + 'convo-1': { + ...getDefaultConversation('convo-1'), + type: 'group' as const, + name: 'Friends!', + }, + 'convo-2': { + ...getDefaultConversation('convo-2'), + name: 'Alice', + }, + }, + }, + }; + + const result = getCandidateContactsForNewGroup(state); + + const ids = result.map(group => group.id); + assert.deepEqual(ids, ['convo-2']); + }); + it('excludes blocked, unregistered, and missing name/profileSharing', () => { + const state = { + ...getRootState(), + conversations: { + ...getEmptyState(), + conversationLookup: { + 'convo-0': { + ...getDefaultConversation('convo-0'), + name: 'Ex', + isBlocked: true, + }, + 'convo-1': { + ...getDefaultConversation('convo-1'), + name: 'Bob', + discoveredUnregisteredAt: Date.now(), + }, + 'convo-2': { + ...getDefaultConversation('convo-2'), + name: 'Charlie', + }, + }, + }, + }; + + const result = getCandidateContactsForNewGroup(state); + + const ids = result.map(group => group.id); + assert.deepEqual(ids, ['convo-2']); + }); + }); + + describe('#getComposableGroups', () => { + const getRootState = (): StateType => { + const rootState = getEmptyRootState(); + return { + ...rootState, + conversations: { + ...getEmptyState(), + conversationLookup: { + 'our-conversation-id': { + ...getDefaultConversation('our-conversation-id'), + isMe: true, + }, + }, + }, + user: { + ...rootState.user, + ourConversationId: 'our-conversation-id', + i18n, + }, + }; + }; + + it('returns only groups with name', () => { + const state = { + ...getRootState(), + conversations: { + ...getEmptyState(), + conversationLookup: { + 'convo-0': { + ...getDefaultConversation('convo-0'), + isMe: true, + name: 'Me!', + }, + 'convo-1': { + ...getDefaultConversation('convo-1'), + type: 'group' as const, + name: 'Friends!', + }, + 'convo-2': { + ...getDefaultConversation('convo-2'), + type: 'group' as const, + }, + }, + }, + }; + + const result = getComposableGroups(state); + + const ids = result.map(group => group.id); + assert.deepEqual(ids, ['convo-1']); + }); + it('excludes blocked, and missing name/profileSharing', () => { + const state = { + ...getRootState(), + conversations: { + ...getEmptyState(), + conversationLookup: { + 'convo-0': { + ...getDefaultConversation('convo-0'), + type: 'group' as const, + name: 'Family!', + isBlocked: true, + }, + 'convo-1': { + ...getDefaultConversation('convo-1'), + type: 'group' as const, + name: 'Friends!', + }, + 'convo-2': { + ...getDefaultConversation('convo-2'), + type: 'group' as const, + }, + }, + }, + }; + + const result = getComposableGroups(state); + + const ids = result.map(group => group.id); + assert.deepEqual(ids, ['convo-1']); + }); + }); + + describe('#getFilteredComposeContacts', () => { const getRootState = (searchTerm = ''): StateType => { const rootState = getEmptyRootState(); return { @@ -564,6 +822,9 @@ describe('both/state/selectors/conversations', () => { conversationLookup: { 'our-conversation-id': { ...getDefaultConversation('our-conversation-id'), + name: 'Me, Myself, and I', + title: 'Me, Myself, and I', + searchableTitle: 'Note to Self', isMe: true, }, }, @@ -583,13 +844,6 @@ describe('both/state/selectors/conversations', () => { const getRootStateWithConversations = (searchTerm = ''): StateType => { const result = getRootState(searchTerm); Object.assign(result.conversations.conversationLookup, { - 'convo-0': { - ...getDefaultConversation('convo-0'), - name: 'Me, Myself, and I', - title: 'Me, Myself, and I', - searchableTitle: 'Note to Self', - isMe: true, - }, 'convo-1': { ...getDefaultConversation('convo-1'), name: 'In System Contacts', @@ -618,6 +872,7 @@ describe('both/state/selectors/conversations', () => { 'convo-6': { ...getDefaultConversation('convo-6'), profileSharing: true, + profileName: 'C. Has Profile Sharing', title: 'C. Has Profile Sharing', }, 'convo-7': { @@ -631,23 +886,27 @@ describe('both/state/selectors/conversations', () => { it('returns no results when there are no contacts', () => { const state = getRootState('foo bar baz'); - const result = getComposeContacts(state); + const result = getFilteredComposeContacts(state); assert.isEmpty(result); }); - it('includes Note to Self', () => { + it('includes Note to Self with no search term', () => { const state = getRootStateWithConversations(); - const result = getComposeContacts(state); + const result = getFilteredComposeContacts(state); const ids = result.map(contact => contact.id); - // convo-6 is sorted last because it doesn't have a name - assert.deepEqual(ids, ['convo-1', 'convo-5', 'convo-0', 'convo-6']); + assert.deepEqual(ids, [ + 'convo-1', + 'convo-5', + 'convo-6', + 'our-conversation-id', + ]); }); it('can search for contacts', () => { const state = getRootStateWithConversations('in system'); - const result = getComposeContacts(state); + const result = getFilteredComposeContacts(state); const ids = result.map(contact => contact.id); // NOTE: convo-6 matches because you can't write "Sharing" without "in" @@ -656,22 +915,22 @@ describe('both/state/selectors/conversations', () => { it('can search for note to self', () => { const state = getRootStateWithConversations('note'); - const result = getComposeContacts(state); + const result = getFilteredComposeContacts(state); const ids = result.map(contact => contact.id); - assert.deepEqual(ids, ['convo-0']); + assert.deepEqual(ids, ['our-conversation-id']); }); it('returns note to self when searching for your own name', () => { const state = getRootStateWithConversations('Myself'); - const result = getComposeContacts(state); + const result = getFilteredComposeContacts(state); const ids = result.map(contact => contact.id); - assert.deepEqual(ids, ['convo-0']); + assert.deepEqual(ids, ['our-conversation-id']); }); }); - describe('#getComposeGroups', () => { + describe('#getFilteredComposeGroups', () => { const getState = (searchTerm = ''): StateType => { const rootState = getEmptyRootState(); return { @@ -738,7 +997,7 @@ describe('both/state/selectors/conversations', () => { it('can search for groups', () => { const state = getState('hello'); - const result = getComposeGroups(state); + const result = getFilteredComposeGroups(state); const ids = result.map(group => group.id); assert.deepEqual(ids, ['convo-3']); @@ -746,14 +1005,14 @@ describe('both/state/selectors/conversations', () => { it('does not return unknown groups when getting all groups (no search term)', () => { const state = getState(); - const result = getComposeGroups(state); + const result = getFilteredComposeGroups(state); const ids = result.map(group => group.id); assert.deepEqual(ids, ['convo-3', 'convo-6', 'convo-7']); }); }); - describe('#getCandidateContactsForNewGroup', () => { + describe('#getFilteredCandidateContactsForNewGroup', () => { const getRootState = (searchTerm = ''): StateType => { const rootState = getEmptyRootState(); return { @@ -819,7 +1078,7 @@ describe('both/state/selectors/conversations', () => { it('returns sorted contacts when there is no search term', () => { const state = getRootState(); - const result = getCandidateContactsForNewGroup(state); + const result = getFilteredCandidateContactsForNewGroup(state); const ids = result.map(contact => contact.id); assert.deepEqual(ids, ['convo-1', 'convo-5']); @@ -827,7 +1086,7 @@ describe('both/state/selectors/conversations', () => { it('can search for contacts', () => { const state = getRootState('system contacts'); - const result = getCandidateContactsForNewGroup(state); + const result = getFilteredCandidateContactsForNewGroup(state); const ids = result.map(contact => contact.id); assert.deepEqual(ids, ['convo-1', 'convo-5']); @@ -917,7 +1176,7 @@ describe('both/state/selectors/conversations', () => { }); }); - describe('#getLeftPaneList', () => { + describe('#_getLeftPaneLists', () => { it('sorts conversations based on timestamp then by intl-friendly title', () => { const data: ConversationLookupType = { id1: { @@ -1152,6 +1411,157 @@ describe('both/state/selectors/conversations', () => { assert.strictEqual(pinnedConversations[1].name, 'Pin Two'); assert.strictEqual(pinnedConversations[2].name, 'Pin Three'); }); + + it('includes archived and pinned conversations with no active_at', () => { + const data: ConversationLookupType = { + pin2: { + id: 'pin2', + e164: '+18005551111', + name: 'Pin Two', + timestamp: 30, + inboxPosition: 30, + phoneNumber: 'notused', + isArchived: false, + isPinned: true, + markedUnread: false, + + type: 'direct', + isMe: false, + lastUpdated: Date.now(), + title: 'Pin Two', + unreadCount: 1, + isSelected: false, + typingContact: { + name: 'Someone There', + color: 'blue', + phoneNumber: '+18005551111', + }, + + acceptedMessageRequest: true, + }, + pin3: { + id: 'pin3', + e164: '+18005551111', + name: 'Pin Three', + timestamp: 30, + inboxPosition: 30, + phoneNumber: 'notused', + isArchived: false, + isPinned: true, + markedUnread: false, + + type: 'direct', + isMe: false, + lastUpdated: Date.now(), + title: 'Pin Three', + unreadCount: 1, + isSelected: false, + typingContact: { + name: 'Someone There', + color: 'blue', + phoneNumber: '+18005551111', + }, + + acceptedMessageRequest: true, + }, + pin1: { + id: 'pin1', + e164: '+18005551111', + name: 'Pin One', + timestamp: 30, + inboxPosition: 30, + phoneNumber: 'notused', + isArchived: true, + isPinned: true, + markedUnread: false, + + type: 'direct', + isMe: false, + lastUpdated: Date.now(), + title: 'Pin One', + unreadCount: 1, + isSelected: false, + typingContact: { + name: 'Someone There', + color: 'blue', + phoneNumber: '+18005551111', + }, + + acceptedMessageRequest: true, + }, + pin4: { + id: 'pin1', + e164: '+18005551111', + name: 'Pin Four', + timestamp: 30, + inboxPosition: 30, + phoneNumber: 'notused', + activeAt: Date.now(), + isArchived: true, + isPinned: false, + markedUnread: false, + + type: 'direct', + isMe: false, + lastUpdated: Date.now(), + title: 'Pin One', + unreadCount: 1, + isSelected: false, + typingContact: { + name: 'Someone There', + color: 'blue', + phoneNumber: '+18005551111', + }, + + acceptedMessageRequest: true, + }, + pin5: { + id: 'pin1', + e164: '+18005551111', + name: 'Pin Five', + timestamp: 30, + inboxPosition: 30, + phoneNumber: 'notused', + isArchived: false, + isPinned: false, + markedUnread: false, + + type: 'direct', + isMe: false, + lastUpdated: Date.now(), + title: 'Pin One', + unreadCount: 1, + isSelected: false, + typingContact: { + name: 'Someone There', + color: 'blue', + phoneNumber: '+18005551111', + }, + + acceptedMessageRequest: true, + }, + }; + + const pinnedConversationIds = ['pin1', 'pin2', 'pin3']; + const comparator = _getConversationComparator(); + const { + pinnedConversations, + archivedConversations, + } = _getLeftPaneLists( + data, + comparator, + undefined, + pinnedConversationIds + ); + + assert.strictEqual(pinnedConversations[0].name, 'Pin One'); + assert.strictEqual(pinnedConversations[1].name, 'Pin Two'); + assert.strictEqual(pinnedConversations[2].name, 'Pin Three'); + assert.strictEqual(pinnedConversations.length, 3); + + assert.strictEqual(archivedConversations[0].name, 'Pin Four'); + assert.strictEqual(archivedConversations.length, 1); + }); }); });