From 53ae88c777c0a4df175f1be29ff8c5528478942b Mon Sep 17 00:00:00 2001 From: Jamie Kyle <113370520+jamiebuilds-signal@users.noreply.github.com> Date: Mon, 18 Mar 2024 16:31:42 -0700 Subject: [PATCH] Sort by recency then alphabetically everywhere --- ts/components/AddUserToAnotherGroupModal.tsx | 6 +- ts/components/CallsNewCall.tsx | 8 +- ts/components/ForwardMessagesModal.tsx | 6 +- ts/components/SendStoryModal.tsx | 10 +- ts/components/StoriesSettingsModal.tsx | 4 +- .../ChooseGroupMembersModal.tsx | 6 +- ts/state/ducks/search.ts | 13 +- ts/state/selectors/conversations.ts | 35 ++- ts/state/smart/CallsTab.tsx | 4 +- .../util/filterAndSortConversations_test.ts | 204 +++++++++++------- ts/util/filterAndSortConversations.ts | 76 +++---- 11 files changed, 195 insertions(+), 177 deletions(-) diff --git a/ts/components/AddUserToAnotherGroupModal.tsx b/ts/components/AddUserToAnotherGroupModal.tsx index d9184d207795..5dd37f8ce5da 100644 --- a/ts/components/AddUserToAnotherGroupModal.tsx +++ b/ts/components/AddUserToAnotherGroupModal.tsx @@ -8,7 +8,7 @@ import type { ListRowProps } from 'react-virtualized'; import type { ConversationType } from '../state/ducks/conversations'; import type { LocalizerType } from '../types/Util'; import { ToastType } from '../types/Toast'; -import { filterAndSortConversationsByRecent } from '../util/filterAndSortConversations'; +import { filterAndSortConversations } from '../util/filterAndSortConversations'; import { ConfirmationDialog } from './ConfirmationDialog'; import type { GroupListItemConversationType } from './conversationList/GroupListItem'; import { @@ -56,7 +56,7 @@ export function AddUserToAnotherGroupModal({ }: Props): JSX.Element | null { const [searchTerm, setSearchTerm] = React.useState(''); const [filteredConversations, setFilteredConversations] = React.useState( - filterAndSortConversationsByRecent(candidateConversations, '', undefined) + filterAndSortConversations(candidateConversations, '', undefined) ); const [selectedGroupId, setSelectedGroupId] = React.useState< @@ -78,7 +78,7 @@ export function AddUserToAnotherGroupModal({ React.useEffect(() => { const timeout = setTimeout(() => { setFilteredConversations( - filterAndSortConversationsByRecent( + filterAndSortConversations( candidateConversations, normalizedSearchTerm, regionCode diff --git a/ts/components/CallsNewCall.tsx b/ts/components/CallsNewCall.tsx index 1c29aa9ea22c..4820b67a0f8b 100644 --- a/ts/components/CallsNewCall.tsx +++ b/ts/components/CallsNewCall.tsx @@ -9,7 +9,7 @@ import { List } from 'react-virtualized'; import type { ConversationType } from '../state/ducks/conversations'; import type { LocalizerType } from '../types/I18N'; import { SearchInput } from './SearchInput'; -import { filterAndSortConversationsByRecent } from '../util/filterAndSortConversations'; +import { filterAndSortConversations } from '../util/filterAndSortConversations'; import { NavSidebarSearchHeader } from './NavSidebar'; import { ListTile } from './ListTile'; import { strictAssert } from '../util/assert'; @@ -89,11 +89,7 @@ export function CallsNewCall({ if (query === '') { return activeConversations; } - return filterAndSortConversationsByRecent( - activeConversations, - query, - regionCode - ); + return filterAndSortConversations(activeConversations, query, regionCode); }, [activeConversations, query, regionCode]); const [groupConversations, directConversations] = useMemo(() => { diff --git a/ts/components/ForwardMessagesModal.tsx b/ts/components/ForwardMessagesModal.tsx index a7b6e2cad0ea..459de6b5fee2 100644 --- a/ts/components/ForwardMessagesModal.tsx +++ b/ts/components/ForwardMessagesModal.tsx @@ -23,7 +23,7 @@ import type { LocalizerType, ThemeType } from '../types/Util'; import type { SmartCompositionTextAreaProps } from '../state/smart/CompositionTextArea'; import { SearchInput } from './SearchInput'; import { StagedLinkPreview } from './conversation/StagedLinkPreview'; -import { filterAndSortConversationsByRecent } from '../util/filterAndSortConversations'; +import { filterAndSortConversations } from '../util/filterAndSortConversations'; import { shouldNeverBeCalled, asyncShouldNeverBeCalled, @@ -96,7 +96,7 @@ export function ForwardMessagesModal({ >([]); const [searchTerm, setSearchTerm] = useState(''); const [filteredConversations, setFilteredConversations] = useState( - filterAndSortConversationsByRecent(candidateConversations, '', regionCode) + filterAndSortConversations(candidateConversations, '', regionCode) ); const [isEditingMessage, setIsEditingMessage] = useState(false); const [cannotMessage, setCannotMessage] = useState(false); @@ -169,7 +169,7 @@ export function ForwardMessagesModal({ useEffect(() => { const timeout = setTimeout(() => { setFilteredConversations( - filterAndSortConversationsByRecent( + filterAndSortConversations( candidateConversations, normalizedSearchTerm, regionCode diff --git a/ts/components/SendStoryModal.tsx b/ts/components/SendStoryModal.tsx index beed425dd977..bcbc3ede56a3 100644 --- a/ts/components/SendStoryModal.tsx +++ b/ts/components/SendStoryModal.tsx @@ -5,7 +5,7 @@ import React, { useEffect, useMemo, useState } from 'react'; import { noop, sortBy } from 'lodash'; import { SearchInput } from './SearchInput'; -import { filterAndSortConversationsByRecent } from '../util/filterAndSortConversations'; +import { filterAndSortConversations } from '../util/filterAndSortConversations'; import type { ConversationType } from '../state/ducks/conversations'; import type { ConversationWithStoriesType } from '../state/selectors/conversations'; @@ -175,11 +175,7 @@ export function SendStoryModal({ const [searchTerm, setSearchTerm] = useState(''); const [filteredConversations, setFilteredConversations] = useState( - filterAndSortConversationsByRecent( - groupConversations, - searchTerm, - undefined - ) + filterAndSortConversations(groupConversations, searchTerm, undefined) ); const normalizedSearchTerm = searchTerm.trim(); @@ -187,7 +183,7 @@ export function SendStoryModal({ useEffect(() => { const timeout = setTimeout(() => { setFilteredConversations( - filterAndSortConversationsByRecent( + filterAndSortConversations( groupConversations, normalizedSearchTerm, undefined diff --git a/ts/components/StoriesSettingsModal.tsx b/ts/components/StoriesSettingsModal.tsx index 70ebd06ceb64..b709e4b2a91d 100644 --- a/ts/components/StoriesSettingsModal.tsx +++ b/ts/components/StoriesSettingsModal.tsx @@ -27,7 +27,7 @@ import { MY_STORY_ID, getStoryDistributionListName } from '../types/Stories'; import { PagedModal, ModalPage } from './Modal'; import { SearchInput } from './SearchInput'; import { StoryDistributionListName } from './StoryDistributionListName'; -import { filterAndSortConversationsByRecent } from '../util/filterAndSortConversations'; +import { filterAndSortConversations } from '../util/filterAndSortConversations'; import { isNotNil } from '../util/isNotNil'; import { shouldNeverBeCalled, @@ -89,7 +89,7 @@ function filterConversations( conversations: ReadonlyArray, searchTerm: string ) { - return filterAndSortConversationsByRecent( + return filterAndSortConversations( conversations, searchTerm, undefined diff --git a/ts/components/conversation/conversation-details/AddGroupMembersModal/ChooseGroupMembersModal.tsx b/ts/components/conversation/conversation-details/AddGroupMembersModal/ChooseGroupMembersModal.tsx index e7b998970312..31a183783ed4 100644 --- a/ts/components/conversation/conversation-details/AddGroupMembersModal/ChooseGroupMembersModal.tsx +++ b/ts/components/conversation/conversation-details/AddGroupMembersModal/ChooseGroupMembersModal.tsx @@ -19,7 +19,7 @@ import { missingCaseError } from '../../../../util/missingCaseError'; import type { LookupConversationWithoutServiceIdActionsType } from '../../../../util/lookupConversationWithoutServiceId'; import { parseAndFormatPhoneNumber } from '../../../../util/libphonenumberInstance'; import type { ParsedE164Type } from '../../../../util/libphonenumberInstance'; -import { filterAndSortConversationsByRecent } from '../../../../util/filterAndSortConversations'; +import { filterAndSortConversations } from '../../../../util/filterAndSortConversations'; import type { ConversationType } from '../../../../state/ducks/conversations'; import type { UUIDFetchStateKeyType, @@ -140,13 +140,13 @@ export function ChooseGroupMembersModal({ const canContinue = Boolean(selectedContacts.length); const [filteredContacts, setFilteredContacts] = useState( - filterAndSortConversationsByRecent(candidateContacts, '', regionCode) + filterAndSortConversations(candidateContacts, '', regionCode) ); const normalizedSearchTerm = searchTerm.trim(); useEffect(() => { const timeout = setTimeout(() => { setFilteredContacts( - filterAndSortConversationsByRecent( + filterAndSortConversations( candidateContacts, normalizedSearchTerm, regionCode diff --git a/ts/state/ducks/search.ts b/ts/state/ducks/search.ts index 5af3ea269be5..e532c5501984 100644 --- a/ts/state/ducks/search.ts +++ b/ts/state/ducks/search.ts @@ -6,7 +6,7 @@ import { debounce, omit, reject } from 'lodash'; import type { ReadonlyDeep } from 'type-fest'; import type { StateType as RootStateType } from '../reducer'; -import { filterAndSortConversationsByRecent } from '../../util/filterAndSortConversations'; +import { filterAndSortConversations } from '../../util/filterAndSortConversations'; import type { ClientSearchResultMessageType, ClientInterface, @@ -337,12 +337,11 @@ async function queryConversationsAndContacts( } ); - const searchResults: Array = - filterAndSortConversationsByRecent( - visibleConversations, - normalizedQuery, - regionCode - ); + const searchResults: Array = filterAndSortConversations( + visibleConversations, + normalizedQuery, + regionCode + ); // Split into two groups - active conversations and items just from address book let conversationIds: Array = []; diff --git a/ts/state/selectors/conversations.ts b/ts/state/selectors/conversations.ts index e498027ad5a7..84028d4f98ba 100644 --- a/ts/state/selectors/conversations.ts +++ b/ts/state/selectors/conversations.ts @@ -30,10 +30,7 @@ import { deconstructLookup } from '../../util/deconstructLookup'; import type { PropsDataType as TimelinePropsType } from '../../components/conversation/Timeline'; import { assertDev } from '../../util/assert'; import { isConversationUnregistered } from '../../util/isConversationUnregistered'; -import { - filterAndSortConversationsAlphabetically, - filterAndSortConversationsByRecent, -} from '../../util/filterAndSortConversations'; +import { filterAndSortConversations } from '../../util/filterAndSortConversations'; import type { ContactNameColorType } from '../../types/Colors'; import { ContactNameColors } from '../../types/Colors'; import type { AvatarDataType } from '../../types/Avatar'; @@ -724,11 +721,7 @@ export const getFilteredComposeContacts = createSelector( contacts: ReadonlyArray, regionCode: string | undefined ): Array => { - return filterAndSortConversationsAlphabetically( - contacts, - searchTerm, - regionCode - ); + return filterAndSortConversations(contacts, searchTerm, regionCode); } ); @@ -750,18 +743,16 @@ export const getFilteredComposeGroups = createSelector( }>; } > => { - return filterAndSortConversationsAlphabetically( - groups, - searchTerm, - regionCode - ).map(group => ({ - ...group, - // we don't disable groups when composing, already filtered - disabledReason: undefined, - // should always be populated for a group - membersCount: group.membersCount ?? 0, - memberships: group.memberships ?? [], - })); + return filterAndSortConversations(groups, searchTerm, regionCode).map( + group => ({ + ...group, + // we don't disable groups when composing, already filtered + disabledReason: undefined, + // should always be populated for a group + membersCount: group.membersCount ?? 0, + memberships: group.memberships ?? [], + }) + ); } ); @@ -769,7 +760,7 @@ export const getFilteredCandidateContactsForNewGroup = createSelector( getCandidateContactsForNewGroup, getNormalizedComposerConversationSearchTerm, getRegionCode, - filterAndSortConversationsByRecent + filterAndSortConversations ); const getGroupCreationComposerState = createSelector( diff --git a/ts/state/smart/CallsTab.tsx b/ts/state/smart/CallsTab.tsx index 2776e74b4ace..d3c0bef9bef5 100644 --- a/ts/state/smart/CallsTab.tsx +++ b/ts/state/smart/CallsTab.tsx @@ -14,7 +14,7 @@ import { getAllConversations, getConversationSelector, } from '../selectors/conversations'; -import { filterAndSortConversationsByRecent } from '../../util/filterAndSortConversations'; +import { filterAndSortConversations } from '../../util/filterAndSortConversations'; import type { CallHistoryFilter, CallHistoryFilterOptions, @@ -44,7 +44,7 @@ function getCallHistoryFilter( return conversation.removalStage == null; }); - const filteredConversations = filterAndSortConversationsByRecent( + const filteredConversations = filterAndSortConversations( currentConversations, query, regionCode diff --git a/ts/test-both/util/filterAndSortConversations_test.ts b/ts/test-both/util/filterAndSortConversations_test.ts index 6cbee74d5fe8..ee92e1381afa 100644 --- a/ts/test-both/util/filterAndSortConversations_test.ts +++ b/ts/test-both/util/filterAndSortConversations_test.ts @@ -2,90 +2,138 @@ // SPDX-License-Identifier: AGPL-3.0-only import { assert } from 'chai'; +import { pick } from 'lodash'; import { getDefaultConversation } from '../helpers/getDefaultConversation'; +import { filterAndSortConversations } from '../../util/filterAndSortConversations'; +import type { ConversationType } from '../../state/ducks/conversations'; -import { - filterAndSortConversationsAlphabetically, - filterAndSortConversationsByRecent, -} from '../../util/filterAndSortConversations'; +type CheckProps = Pick; + +function check({ + searchTerm, + input, + expected, +}: { + searchTerm: string; + input: Array; + expected: Array; +}) { + const conversations = input.map(props => { + return getDefaultConversation(props); + }); + const results = filterAndSortConversations(conversations, searchTerm, 'US'); + const actual = results.map(convo => { + return pick(convo, 'title', 'activeAt'); + }); + assert.sameDeepMembers(actual, expected); +} describe('filterAndSortConversations', () => { - const conversations = [ - getDefaultConversation({ - title: '+16505551234', - activeAt: 1, - }), - getDefaultConversation({ - title: 'The Abraham Lincoln Club', - activeAt: 4, - }), - getDefaultConversation({ - title: 'Boxing Club', - activeAt: 3, - }), - getDefaultConversation({ - title: 'Not recent', - }), - getDefaultConversation({ - title: 'George Washington', - e164: '+16505559876', - activeAt: 2, - }), - getDefaultConversation({ - title: 'A long long long title ending with burrito', - }), - ]; - - it('filterAndSortConversationsByRecent sorts by recency when no search term is provided', () => { - const titles = filterAndSortConversationsByRecent( - conversations, - '', - 'US' - ).map(contact => contact.title); - assert.sameOrderedMembers(titles, [ - 'The Abraham Lincoln Club', - 'Boxing Club', - 'George Washington', - '+16505551234', - 'Not recent', - 'A long long long title ending with burrito', - ]); - }); - - it('filterAndSortConversationsAlphabetically sorts by title when no search term is provided', () => { - const titles = filterAndSortConversationsAlphabetically( - conversations, - '', - 'US' - ).map(contact => contact.title); - assert.sameOrderedMembers(titles, [ - 'A long long long title ending with burrito', - 'Boxing Club', - 'George Washington', - 'Not recent', - 'The Abraham Lincoln Club', - '+16505551234', - ]); - }); - - it('filterAndSortConversationsAlphabetically sorts by title when a search term is provided', () => { - const titles = filterAndSortConversationsAlphabetically( - conversations, - 'club', - 'US' - ).map(contact => contact.title); - assert.sameOrderedMembers(titles, [ - 'Boxing Club', - 'The Abraham Lincoln Club', - ]); + it('finds a conversation by title', () => { + check({ + searchTerm: 'yes', + input: [{ title: 'no' }, { title: 'yes' }, { title: 'no' }], + expected: [{ title: 'yes' }], + }); }); it('finds a conversation when the search term is at the end of a long title', () => { - const titles = filterAndSortConversationsByRecent( - conversations, - 'burrito', - 'US' - ).map(convo => convo.title); - assert.deepEqual(titles, ['A long long long title ending with burrito']); + check({ + searchTerm: 'burrito', + input: [ + { title: 'no' }, + { + title: 'A long long long title ending with burrito', + }, + { title: 'no' }, + ], + expected: [ + { + title: 'A long long long title ending with burrito', + }, + ], + }); + }); + + it('finds a conversation by phone number', () => { + check({ + searchTerm: '9876', + input: [ + { title: 'no' }, + { title: 'yes', e164: '+16505559876' }, + { title: 'no' }, + ], + expected: [{ title: 'yes' }], + }); + }); + + describe('no search term', () => { + it('sorts by recency first', () => { + check({ + searchTerm: '', + input: [ + { title: 'B', activeAt: 2 }, + { title: 'A', activeAt: 1 }, + { title: 'C', activeAt: 3 }, + ], + expected: [ + { title: 'C', activeAt: 3 }, + { title: 'B', activeAt: 2 }, + { title: 'A', activeAt: 1 }, + ], + }); + }); + + it('falls back to alphabetically', () => { + check({ + searchTerm: '', + input: [ + { title: 'B', activeAt: 2 }, + { title: 'A', activeAt: 2 }, + { title: 'C', activeAt: 3 }, + ], + expected: [ + { title: 'C', activeAt: 3 }, + { title: 'A', activeAt: 2 }, + { title: 'B', activeAt: 2 }, + ], + }); + }); + }); + + describe('with search term', () => { + it('sorts by recency first', () => { + check({ + searchTerm: 'yes', + input: [ + { title: 'no' }, + { title: 'yes B', activeAt: 2 }, + { title: 'yes A', activeAt: 1 }, + { title: 'yes C', activeAt: 3 }, + ], + expected: [ + { title: 'yes C', activeAt: 3 }, + { title: 'yes B', activeAt: 2 }, + { title: 'yes A', activeAt: 1 }, + ], + }); + }); + + it('falls back to alphabetically', () => { + check({ + searchTerm: 'yes', + input: [ + { title: 'no' }, + { title: 'yes B', activeAt: 2 }, + { title: 'yes A', activeAt: 2 }, + { title: 'yes C', activeAt: 3 }, + ], + expected: [ + { title: 'yes C', activeAt: 3 }, + { title: 'yes A', activeAt: 2 }, + { title: 'yes B', activeAt: 2 }, + ], + }); + }); }); }); diff --git a/ts/util/filterAndSortConversations.ts b/ts/util/filterAndSortConversations.ts index 7e453b904c4c..f51c347b894d 100644 --- a/ts/util/filterAndSortConversations.ts +++ b/ts/util/filterAndSortConversations.ts @@ -2,7 +2,6 @@ // SPDX-License-Identifier: AGPL-3.0-only import type Fuse from 'fuse.js'; - import type { ConversationType } from '../state/ducks/conversations'; import { parseAndFormatPhoneNumber } from './libphonenumberInstance'; import { WEEK } from './durations'; @@ -129,7 +128,27 @@ function searchConversations( return index.search(extendedSearchTerm); } -export function filterAndSortConversationsByRecent( +function startsWithLetter(title: string) { + // Uses \p, the unicode character class escape, to check if a the first character is a + // letter + return /^\p{Letter}/u.test(title); +} + +function sortAlphabetically(a: ConversationType, b: ConversationType) { + // Sort alphabetically with conversations starting with a letter first (and phone + // numbers last) + const aStartsWithLetter = startsWithLetter(a.title); + const bStartsWithLetter = startsWithLetter(b.title); + if (aStartsWithLetter && !bStartsWithLetter) { + return -1; + } + if (!aStartsWithLetter && bStartsWithLetter) { + return 1; + } + return a.title.localeCompare(b.title); +} + +export function filterAndSortConversations( conversations: ReadonlyArray, searchTerm: string, regionCode: string | undefined @@ -156,53 +175,22 @@ export function filterAndSortConversationsByRecent( (b.score ?? 0) + (bLeft ? LEFT_GROUP_PENALTY : 0); - return aScore - bScore; + const activeScore = aScore - bScore; + if (activeScore !== 0) { + return activeScore; + } + return sortAlphabetically(a.item, b.item); }) .map(result => result.item); } return conversations.concat().sort((a, b) => { - if (a.activeAt && b.activeAt) { - return a.activeAt > b.activeAt ? -1 : 1; + const aScore = a.activeAt ?? 0; + const bScore = b.activeAt ?? 0; + const score = bScore - aScore; + if (score !== 0) { + return score; } - - return a.activeAt && !b.activeAt ? -1 : 1; + return sortAlphabetically(a, b); }); } - -function startsWithLetter(title: string) { - // Uses \p, the unicode character class escape, to check if a the first character is a - // letter - return /^\p{Letter}/u.test(title); -} - -function sortAlphabetically(a: ConversationType, b: ConversationType) { - // Sort alphabetically with conversations starting with a letter first (and phone - // numbers last) - const aStartsWithLetter = startsWithLetter(a.title); - const bStartsWithLetter = startsWithLetter(b.title); - if (aStartsWithLetter && !bStartsWithLetter) { - return -1; - } - if (!aStartsWithLetter && bStartsWithLetter) { - return 1; - } - return a.title.localeCompare(b.title); -} - -export function filterAndSortConversationsAlphabetically( - conversations: ReadonlyArray, - searchTerm: string, - regionCode: string | undefined -): Array { - if (searchTerm.length) { - const withoutUnknown = conversations.filter(item => item.titleNoDefault); - - return searchConversations(withoutUnknown, searchTerm, regionCode) - .slice() - .map(result => result.item) - .sort(sortAlphabetically); - } - - return conversations.concat().sort(sortAlphabetically); -}