From cbc6c2947949efacd7fc996e007376e098557ea3 Mon Sep 17 00:00:00 2001 From: Scott Nonnenberg Date: Wed, 6 Jan 2021 07:41:43 -0800 Subject: [PATCH] Finish in-redux conversation lookups, getPropsForSearchResult moved --- package.json | 2 +- test/setup-test-node.js | 15 ++ ts/background.ts | 12 + ts/components/MessageSearchResult.tsx | 10 +- ts/models/messages.ts | 21 -- ts/services/bounce.ts | 2 +- ts/shims/Whisper.ts | 9 - ts/sql/Client.ts | 66 +++--- ts/state/ducks/conversations.ts | 71 +++++- ts/state/ducks/search.ts | 2 +- ts/state/ducks/user.ts | 2 +- ts/state/selectors/conversations.ts | 94 ++++++-- ts/state/selectors/search.ts | 69 +++--- .../state/selectors/conversations_test.ts | 201 ++++++++++++++++- ts/test-both/state/selectors/search_test.ts | 212 ++++++++++++++++++ ts/test-both/util/makeLookup_test.ts | 75 +++++++ .../state/ducks/conversations_test.ts | 168 ++++++++++++-- ts/util/makeLookup.ts | 16 +- 18 files changed, 901 insertions(+), 146 deletions(-) create mode 100644 test/setup-test-node.js create mode 100644 ts/test-both/state/selectors/search_test.ts create mode 100644 ts/test-both/util/makeLookup_test.ts diff --git a/package.json b/package.json index 6ee4d2857f..1b4e5865cc 100644 --- a/package.json +++ b/package.json @@ -29,7 +29,7 @@ "publish-to-apt": "NAME=$npm_package_name VERSION=$npm_package_version ./aptly.sh", "test": "yarn test-node && yarn test-electron", "test-electron": "yarn grunt test", - "test-node": "electron-mocha --recursive test/app test/modules ts/test-node ts/test-both", + "test-node": "electron-mocha --require test/setup-test-node.js --recursive test/app test/modules ts/test-node ts/test-both", "test-node-coverage": "nyc --reporter=lcov --reporter=text mocha --recursive test/app test/modules ts/test-node ts/test-both", "eslint": "eslint .", "lint": "yarn format --list-different && yarn eslint", diff --git a/test/setup-test-node.js b/test/setup-test-node.js new file mode 100644 index 0000000000..fe199db1c3 --- /dev/null +++ b/test/setup-test-node.js @@ -0,0 +1,15 @@ +/* eslint-disable no-console */ + +// To replicate logic we have on the client side +global.window = { + log: { + info: (...args) => console.log(...args), + warn: (...args) => console.warn(...args), + error: (...args) => console.error(...args), + }, + i18n: key => `i18n(${key})`, +}; + +// For ducks/network.getEmptyState() +global.navigator = {}; +global.WebSocket = {}; diff --git a/ts/background.ts b/ts/background.ts index 5262834fcd..2776eb7c3f 100644 --- a/ts/background.ts +++ b/ts/background.ts @@ -625,6 +625,18 @@ type WhatIsThis = import('./window.d').WhatIsThis; const initialState = { conversations: { conversationLookup: window.Signal.Util.makeLookup(conversations, 'id'), + conversationsByE164: window.Signal.Util.makeLookup( + conversations, + 'e164' + ), + conversationsByUuid: window.Signal.Util.makeLookup( + conversations, + 'uuid' + ), + conversationsByGroupId: window.Signal.Util.makeLookup( + conversations, + 'groupId' + ), messagesByConversation: {}, messagesLookup: {}, selectedConversation: undefined, diff --git a/ts/components/MessageSearchResult.tsx b/ts/components/MessageSearchResult.tsx index 207fb7630a..800dc2196b 100644 --- a/ts/components/MessageSearchResult.tsx +++ b/ts/components/MessageSearchResult.tsx @@ -18,7 +18,7 @@ export type PropsDataType = { id: string; conversationId: string; - sentAt: number; + sentAt?: number; snippet: string; @@ -166,9 +166,11 @@ export class MessageSearchResult extends React.PureComponent {
{this.renderFrom()} -
- -
+ {sentAt ? ( +
+ +
+ ) : null}
diff --git a/ts/models/messages.ts b/ts/models/messages.ts index 4e3d4f1ebd..1401245737 100644 --- a/ts/models/messages.ts +++ b/ts/models/messages.ts @@ -288,27 +288,6 @@ export class MessageModel extends window.Backbone.Model { }; } - // Other top-level prop-generation - getPropsForSearchResult(): WhatIsThis { - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - const sourceId = this.getContactId()!; - const from = this.findAndFormatContact(sourceId); - const conversationId = this.get('conversationId'); - const to = this.findAndFormatContact(conversationId); - - return { - from, - to, - - isSelected: this.isSelected, - - id: this.id, - conversationId, - sentAt: this.get('sent_at'), - snippet: this.get('snippet'), - }; - } - getPropsForMessageDetail(): WhatIsThis { const newIdentity = window.i18n('newIdentity'); const OUTGOING_KEY_ERROR = 'OutgoingIdentityKeyError'; diff --git a/ts/services/bounce.ts b/ts/services/bounce.ts index ecf05b599c..a56d1170dd 100644 --- a/ts/services/bounce.ts +++ b/ts/services/bounce.ts @@ -1,4 +1,4 @@ -// Copyright 2020 Signal Messenger, LLC +// Copyright 2021 Signal Messenger, LLC // SPDX-License-Identifier: AGPL-3.0-only import { app, BrowserWindow, ipcMain } from 'electron'; diff --git a/ts/shims/Whisper.ts b/ts/shims/Whisper.ts index 45b3432949..9981488c29 100644 --- a/ts/shims/Whisper.ts +++ b/ts/shims/Whisper.ts @@ -1,15 +1,6 @@ // Copyright 2019-2020 Signal Messenger, LLC // SPDX-License-Identifier: AGPL-3.0-only -// Matching Whisper.Message API -// eslint-disable-next-line max-len -// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types -export function getSearchResultsProps(attributes: any): any { - const model = new window.Whisper.Message(attributes); - - return model.getPropsForSearchResult(); -} - // Matching Whisper.Message API // eslint-disable-next-line max-len // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types diff --git a/ts/sql/Client.ts b/ts/sql/Client.ts index 8ff36fd81a..8e6b2dfc39 100644 --- a/ts/sql/Client.ts +++ b/ts/sql/Client.ts @@ -55,7 +55,11 @@ import { ConversationModel } from '../models/conversations'; // We listen to a lot of events on ipcRenderer, often on the same channel. This prevents // any warnings that might be sent to the console in that case. -ipcRenderer.setMaxListeners(0); +if (ipcRenderer && ipcRenderer.setMaxListeners) { + ipcRenderer.setMaxListeners(0); +} else { + window.log.warn('sql/Client: ipcRenderer is not available!'); +} const DATABASE_UPDATE_TIMEOUT = 2 * 60 * 1000; // two minutes @@ -413,35 +417,39 @@ function _getJob(id: number) { return _jobs[id]; } -ipcRenderer.on( - `${SQL_CHANNEL_KEY}-done`, - (_, jobId, errorForDisplay, result) => { - const job = _getJob(jobId); - if (!job) { - throw new Error( - `Received SQL channel reply to job ${jobId}, but did not have it in our registry!` - ); +if (ipcRenderer && ipcRenderer.on) { + ipcRenderer.on( + `${SQL_CHANNEL_KEY}-done`, + (_, jobId, errorForDisplay, result) => { + const job = _getJob(jobId); + if (!job) { + throw new Error( + `Received SQL channel reply to job ${jobId}, but did not have it in our registry!` + ); + } + + const { resolve, reject, fnName } = job; + + if (!resolve || !reject) { + throw new Error( + `SQL channel job ${jobId} (${fnName}): didn't have a resolve or reject` + ); + } + + if (errorForDisplay) { + return reject( + new Error( + `Error received from SQL channel job ${jobId} (${fnName}): ${errorForDisplay}` + ) + ); + } + + return resolve(result); } - - const { resolve, reject, fnName } = job; - - if (!resolve || !reject) { - throw new Error( - `SQL channel job ${jobId} (${fnName}): didn't have a resolve or reject` - ); - } - - if (errorForDisplay) { - return reject( - new Error( - `Error received from SQL channel job ${jobId} (${fnName}): ${errorForDisplay}` - ) - ); - } - - return resolve(result); - } -); + ); +} else { + window.log.warn('sql/Client: ipcRenderer.on is not available!'); +} function makeChannel(fnName: string) { return async (...args: Array) => { diff --git a/ts/state/ducks/conversations.ts b/ts/state/ducks/conversations.ts index 989b3dffd4..162958373f 100644 --- a/ts/state/ducks/conversations.ts +++ b/ts/state/ducks/conversations.ts @@ -1,4 +1,4 @@ -// Copyright 2019-2020 Signal Messenger, LLC +// Copyright 2019-2021 Signal Messenger, LLC // SPDX-License-Identifier: AGPL-3.0-only /* eslint-disable camelcase */ @@ -112,7 +112,8 @@ export type ConversationLookupType = { export type MessageType = { id: string; conversationId: string; - source: string; + source?: string; + sourceUuid?: string; type: | 'incoming' | 'outgoing' @@ -185,11 +186,14 @@ export type ConversationMessageType = { }; export type MessagesByConversationType = { - [key: string]: ConversationMessageType | null; + [key: string]: ConversationMessageType | undefined; }; export type ConversationsStateType = { conversationLookup: ConversationLookupType; + conversationsByE164: ConversationLookupType; + conversationsByUuid: ConversationLookupType; + conversationsByGroupId: ConversationLookupType; selectedConversation?: string; selectedMessage?: string; selectedMessageCounter: number; @@ -731,6 +735,9 @@ function showArchivedConversations(): ShowArchivedConversationsActionType { export function getEmptyState(): ConversationsStateType { return { conversationLookup: {}, + conversationsByE164: {}, + conversationsByUuid: {}, + conversationsByGroupId: {}, messagesByConversation: {}, messagesLookup: {}, selectedMessageCounter: 0, @@ -811,6 +818,55 @@ function hasMessageHeightChanged( return false; } +export function updateConversationLookups( + added: ConversationType | undefined, + removed: ConversationType | undefined, + state: ConversationsStateType +): Pick< + ConversationsStateType, + 'conversationsByE164' | 'conversationsByUuid' | 'conversationsByGroupId' +> { + const result = { + conversationsByE164: state.conversationsByE164, + conversationsByUuid: state.conversationsByUuid, + conversationsByGroupId: state.conversationsByGroupId, + }; + + if (removed && removed.e164) { + result.conversationsByE164 = omit(result.conversationsByE164, removed.e164); + } + if (removed && removed.uuid) { + result.conversationsByUuid = omit(result.conversationsByUuid, removed.uuid); + } + if (removed && removed.groupId) { + result.conversationsByGroupId = omit( + result.conversationsByGroupId, + removed.groupId + ); + } + + if (added && added.e164) { + result.conversationsByE164 = { + ...result.conversationsByE164, + [added.e164]: added, + }; + } + if (added && added.uuid) { + result.conversationsByUuid = { + ...result.conversationsByUuid, + [added.uuid]: added, + }; + } + if (added && added.groupId) { + result.conversationsByGroupId = { + ...result.conversationsByGroupId, + [added.groupId]: added, + }; + } + + return result; +} + export function reducer( state: Readonly = getEmptyState(), action: Readonly @@ -826,6 +882,7 @@ export function reducer( ...conversationLookup, [id]: data, }, + ...updateConversationLookups(data, undefined, state), }; } if (action.type === 'CONVERSATION_CHANGED') { @@ -863,16 +920,24 @@ export function reducer( ...conversationLookup, [id]: data, }, + ...updateConversationLookups(data, existing, state), }; } if (action.type === 'CONVERSATION_REMOVED') { const { payload } = action; const { id } = payload; const { conversationLookup } = state; + const existing = getOwn(conversationLookup, id); + + // No need to make a change if we didn't have a record of this conversation! + if (!existing) { + return state; + } return { ...state, conversationLookup: omit(conversationLookup, [id]), + ...updateConversationLookups(undefined, existing, state), }; } if (action.type === 'CONVERSATION_UNLOADED') { diff --git a/ts/state/ducks/search.ts b/ts/state/ducks/search.ts index d74e09c71e..7cdab85821 100644 --- a/ts/state/ducks/search.ts +++ b/ts/state/ducks/search.ts @@ -323,7 +323,7 @@ async function queryConversationsAndContacts( // Reducer -function getEmptyState(): SearchStateType { +export function getEmptyState(): SearchStateType { return { startSearchCounter: 0, query: '', diff --git a/ts/state/ducks/user.ts b/ts/state/ducks/user.ts index 3e488f9630..e40d9db7fd 100644 --- a/ts/state/ducks/user.ts +++ b/ts/state/ducks/user.ts @@ -67,7 +67,7 @@ function manualReconnect(): NoopActionType { // Reducer -function getEmptyState(): UserStateType { +export function getEmptyState(): UserStateType { return { attachmentsPath: 'missing', stickersPath: 'missing', diff --git a/ts/state/selectors/conversations.ts b/ts/state/selectors/conversations.ts index 2c675db7ce..42cec012f9 100644 --- a/ts/state/selectors/conversations.ts +++ b/ts/state/selectors/conversations.ts @@ -1,4 +1,4 @@ -// Copyright 2019-2020 Signal Messenger, LLC +// Copyright 2019-2021 Signal Messenger, LLC // SPDX-License-Identifier: AGPL-3.0-only import memoizee from 'memoizee'; @@ -15,6 +15,7 @@ import { MessagesByConversationType, MessageType, } from '../ducks/conversations'; +import { getOwn } from '../../util/getOwn'; import type { CallsByConversationType } from '../ducks/calling'; import { getCallsByConversation } from './calling'; import { getBubbleProps } from '../../shims/Whisper'; @@ -30,6 +31,20 @@ import { } from './user'; import { getPinnedConversationIds } from './items'; +let placeholderContact: ConversationType; +export const getPlaceholderContact = (): ConversationType => { + if (placeholderContact) { + return placeholderContact; + } + + placeholderContact = { + id: 'placeholder-contact', + type: 'direct', + title: window.i18n('unknownContact'), + }; + return placeholderContact; +}; + export const getConversations = (state: StateType): ConversationsStateType => state.conversations; @@ -40,6 +55,27 @@ export const getConversationLookup = createSelector( } ); +export const getConversationsByUuid = createSelector( + getConversations, + (state: ConversationsStateType): ConversationLookupType => { + return state.conversationsByUuid; + } +); + +export const getConversationsByE164 = createSelector( + getConversations, + (state: ConversationsStateType): ConversationLookupType => { + return state.conversationsByE164; + } +); + +export const getConversationsByGroupId = createSelector( + getConversations, + (state: ConversationsStateType): ConversationLookupType => { + return state.conversationsByGroupId; + } +); + export const getSelectedConversation = createSelector( getConversations, (state: ConversationsStateType): string | undefined => { @@ -201,17 +237,21 @@ export const getMe = createSelector( // Backbone-based prop-generation functions expect to get Conversation information // directly via ConversationController export function _conversationSelector( - conversation: ConversationType + conversation?: ConversationType // regionCode: string, // userNumber: string ): ConversationType { - return conversation; + if (conversation) { + return conversation; + } + + return getPlaceholderContact(); } // A little optimization to reset our selector cache when high-level application data // changes: regionCode and userNumber. type CachedConversationSelectorType = ( - conversation: ConversationType + conversation?: ConversationType ) => ConversationType; export const getCachedSelectorForConversation = createSelector( getRegionCode, @@ -223,23 +263,51 @@ export const getCachedSelectorForConversation = createSelector( } ); -export type GetConversationByIdType = ( - id: string -) => ConversationType | undefined; +export type GetConversationByIdType = (id?: string) => ConversationType; export const getConversationSelector = createSelector( getCachedSelectorForConversation, getConversationLookup, + getConversationsByUuid, + getConversationsByE164, + getConversationsByGroupId, ( selector: CachedConversationSelectorType, - lookup: ConversationLookupType + byId: ConversationLookupType, + byUuid: ConversationLookupType, + byE164: ConversationLookupType, + byGroupId: ConversationLookupType ): GetConversationByIdType => { - return (id: string) => { - const conversation = lookup[id]; - if (!conversation) { - return undefined; + return (id?: string) => { + if (!id) { + window.log.warn( + `getConversationSelector: Called with a falsey id ${id}` + ); + // This will return a placeholder contact + return selector(undefined); } - return selector(conversation); + const onE164 = getOwn(byE164, id); + if (onE164) { + return selector(onE164); + } + const onUuid = getOwn(byUuid, id); + if (onUuid) { + return selector(onUuid); + } + const onGroupId = getOwn(byGroupId, id); + if (onGroupId) { + return selector(onGroupId); + } + const onId = getOwn(byId, id); + if (onId) { + return selector(onId); + } + + window.log.warn( + `getConversationSelector: No conversation found for id ${id}` + ); + // This will return a placeholder contact + return selector(undefined); }; } ); diff --git a/ts/state/selectors/search.ts b/ts/state/selectors/search.ts index 5a98ebf1c7..069a126dfe 100644 --- a/ts/state/selectors/search.ts +++ b/ts/state/selectors/search.ts @@ -3,7 +3,6 @@ import memoizee from 'memoizee'; import { createSelector } from 'reselect'; -import { getSearchResultsProps } from '../../shims/Whisper'; import { instance } from '../../util/libphonenumberInstance'; import { StateType } from '../reducer'; @@ -24,7 +23,7 @@ import { } from '../../components/SearchResults'; import { PropsDataType as MessageSearchResultPropsDataType } from '../../components/MessageSearchResult'; -import { getRegionCode, getUserNumber } from './user'; +import { getRegionCode, getUserConversationId } from './user'; import { getUserAgent } from './items'; import { GetConversationByIdType, @@ -221,18 +220,21 @@ export const getSearchResults = createSelector( export function _messageSearchResultSelector( message: MessageSearchResultType, - _ourNumber: string, - _regionCode: string, - _sender?: ConversationType, - _recipient?: ConversationType, + from: ConversationType, + to: ConversationType, searchConversationId?: string, selectedMessageId?: string ): MessageSearchResultPropsDataType { - // Note: We don't use all of those parameters here, but the shim we call does. - // We want to call this function again if any of those parameters change. return { - ...getSearchResultsProps(message), - isSelected: message.id === selectedMessageId, + from, + to, + + id: message.id, + conversationId: message.conversationId, + sentAt: message.sent_at, + snippet: message.snippet, + + isSelected: Boolean(selectedMessageId && message.id === selectedMessageId), isSearchingInConversation: Boolean(searchConversationId), }; } @@ -241,16 +243,13 @@ export function _messageSearchResultSelector( // changes: regionCode and userNumber. type CachedMessageSearchResultSelectorType = ( message: MessageSearchResultType, - ourNumber: string, - regionCode: string, - sender?: ConversationType, - recipient?: ConversationType, + from: ConversationType, + to: ConversationType, searchConversationId?: string, selectedMessageId?: string ) => MessageSearchResultPropsDataType; export const getCachedSelectorForMessageSearchResult = createSelector( - getRegionCode, - getUserNumber, + getUserConversationId, (): CachedMessageSearchResultSelectorType => { // Note: memoizee will check all parameters provided, and only run our selector // if any of them have changed. @@ -267,43 +266,47 @@ export const getMessageSearchResultSelector = createSelector( getSelectedMessage, getConversationSelector, getSearchConversationId, - getRegionCode, - getUserNumber, + getUserConversationId, ( messageSearchResultSelector: CachedMessageSearchResultSelectorType, messageSearchResultLookup: MessageSearchResultLookupType, - selectedMessage: string | undefined, + selectedMessageId: string | undefined, conversationSelector: GetConversationByIdType, searchConversationId: string | undefined, - regionCode: string, - ourNumber: string + ourConversationId: string ): GetMessageSearchResultByIdType => { return (id: string) => { const message = messageSearchResultLookup[id]; if (!message) { + window.log.warn( + `getMessageSearchResultSelector: messageSearchResultLookup was missing id ${id}` + ); return undefined; } - const { conversationId, source, type } = message; - let sender: ConversationType | undefined; - let recipient: ConversationType | undefined; + const { conversationId, source, sourceUuid, type } = message; + let from: ConversationType; + let to: ConversationType; if (type === 'incoming') { - sender = conversationSelector(source); - recipient = conversationSelector(ourNumber); + from = conversationSelector(sourceUuid || source); + to = conversationSelector(conversationId); } else if (type === 'outgoing') { - sender = conversationSelector(ourNumber); - recipient = conversationSelector(conversationId); + from = conversationSelector(ourConversationId); + to = conversationSelector(conversationId); + } else { + window.log.warn( + `getMessageSearchResultSelector: Got unexpected type ${type}` + ); + return undefined; } return messageSearchResultSelector( message, - ourNumber, - regionCode, - sender, - recipient, + from, + to, searchConversationId, - selectedMessage + selectedMessageId ); }; } diff --git a/ts/test-both/state/selectors/conversations_test.ts b/ts/test-both/state/selectors/conversations_test.ts index b673f91986..b7f9077217 100644 --- a/ts/test-both/state/selectors/conversations_test.ts +++ b/ts/test-both/state/selectors/conversations_test.ts @@ -3,13 +3,212 @@ import { assert } from 'chai'; -import { ConversationLookupType } from '../../../state/ducks/conversations'; +import { + ConversationLookupType, + ConversationType, + getEmptyState, +} from '../../../state/ducks/conversations'; import { _getConversationComparator, _getLeftPaneLists, + getConversationSelector, + getPlaceholderContact, } from '../../../state/selectors/conversations'; +import { noopAction } from '../../../state/ducks/noop'; +import { StateType, reducer as rootReducer } from '../../../state/reducer'; describe('both/state/selectors/conversations', () => { + const getEmptyRootState = (): StateType => { + return rootReducer(undefined, noopAction()); + }; + + function getDefaultConversation(id: string): ConversationType { + return { + id, + type: 'direct', + title: `${id} title`, + }; + } + + describe('#getConversationSelector', () => { + it('returns empty placeholder if falsey id provided', () => { + const state = getEmptyRootState(); + const selector = getConversationSelector(state); + + const actual = selector(undefined); + + assert.deepEqual(actual, getPlaceholderContact()); + }); + it('returns empty placeholder if no match', () => { + const state = { + ...getEmptyRootState(), + }; + const selector = getConversationSelector(state); + + const actual = selector('random-id'); + + assert.deepEqual(actual, getPlaceholderContact()); + }); + + it('returns conversation by e164 first', () => { + const id = 'id'; + + const conversation = getDefaultConversation(id); + const wrongConversation = getDefaultConversation('wrong'); + + const state = { + ...getEmptyRootState(), + conversations: { + ...getEmptyState(), + conversationLookup: { + [id]: wrongConversation, + }, + conversationsByE164: { + [id]: conversation, + }, + conversationsByUuid: { + [id]: wrongConversation, + }, + conversationsByGroupId: { + [id]: wrongConversation, + }, + }, + }; + + const selector = getConversationSelector(state); + + const actual = selector(id); + + assert.strictEqual(actual, conversation); + }); + it('returns conversation by uuid', () => { + const id = 'id'; + + const conversation = getDefaultConversation(id); + const wrongConversation = getDefaultConversation('wrong'); + + const state = { + ...getEmptyRootState(), + conversations: { + ...getEmptyState(), + conversationLookup: { + [id]: wrongConversation, + }, + conversationsByUuid: { + [id]: conversation, + }, + conversationsByGroupId: { + [id]: wrongConversation, + }, + }, + }; + + const selector = getConversationSelector(state); + + const actual = selector(id); + + assert.strictEqual(actual, conversation); + }); + it('returns conversation by groupId', () => { + const id = 'id'; + + const conversation = getDefaultConversation(id); + const wrongConversation = getDefaultConversation('wrong'); + + const state = { + ...getEmptyRootState(), + conversations: { + ...getEmptyState(), + conversationLookup: { + [id]: wrongConversation, + }, + conversationsByGroupId: { + [id]: conversation, + }, + }, + }; + + const selector = getConversationSelector(state); + + const actual = selector(id); + + assert.strictEqual(actual, conversation); + }); + it('returns conversation by conversationId', () => { + const id = 'id'; + + const conversation = getDefaultConversation(id); + + const state = { + ...getEmptyRootState(), + conversations: { + ...getEmptyState(), + conversationLookup: { + [id]: conversation, + }, + }, + }; + + const selector = getConversationSelector(state); + + const actual = selector(id); + + assert.strictEqual(actual, conversation); + }); + + // Less important now, given that all prop-generation for conversations is in + // models/conversation.getProps() and not here. + it('does proper caching of result', () => { + const id = 'id'; + + const conversation = getDefaultConversation(id); + + const state = { + ...getEmptyRootState(), + conversations: { + ...getEmptyState(), + conversationLookup: { + [id]: conversation, + }, + }, + }; + + const selector = getConversationSelector(state); + + const actual = selector(id); + + const secondState = { + ...state, + conversations: { + ...getEmptyState(), + conversationLookup: { + [id]: conversation, + }, + }, + }; + + const secondSelector = getConversationSelector(secondState); + const secondActual = secondSelector(id); + + assert.strictEqual(actual, secondActual); + + const thirdState = { + ...state, + conversations: { + ...getEmptyState(), + conversationLookup: { + [id]: getDefaultConversation('third'), + }, + }, + }; + + const thirdSelector = getConversationSelector(thirdState); + const thirdActual = thirdSelector(id); + + assert.notStrictEqual(actual, thirdActual); + }); + }); + describe('#getLeftPaneList', () => { it('sorts conversations based on timestamp then by intl-friendly title', () => { const data: ConversationLookupType = { diff --git a/ts/test-both/state/selectors/search_test.ts b/ts/test-both/state/selectors/search_test.ts new file mode 100644 index 0000000000..b0bdb4acaa --- /dev/null +++ b/ts/test-both/state/selectors/search_test.ts @@ -0,0 +1,212 @@ +// Copyright 2021 Signal Messenger, LLC +// SPDX-License-Identifier: AGPL-3.0-only + +import { assert } from 'chai'; + +import { + ConversationType, + getEmptyState as getEmptyConversationState, + MessageType, +} from '../../../state/ducks/conversations'; +import { noopAction } from '../../../state/ducks/noop'; +import { getEmptyState as getEmptySearchState } from '../../../state/ducks/search'; +import { getEmptyState as getEmptyUserState } from '../../../state/ducks/user'; +import { getMessageSearchResultSelector } from '../../../state/selectors/search'; + +import { StateType, reducer as rootReducer } from '../../../state/reducer'; + +describe('both/state/selectors/search', () => { + const getEmptyRootState = (): StateType => { + return rootReducer(undefined, noopAction()); + }; + + function getDefaultMessage(id: string): MessageType { + return { + id, + conversationId: 'conversationId', + source: 'source', + sourceUuid: 'sourceUuid', + type: 'incoming' as const, + received_at: Date.now(), + attachments: [], + sticker: {}, + unread: false, + }; + } + + function getDefaultConversation(id: string): ConversationType { + return { + id, + type: 'direct', + title: `${id} title`, + }; + } + + describe('#getMessageSearchResultSelector', () => { + it('returns undefined if message not found in lookup', () => { + const state = getEmptyRootState(); + const selector = getMessageSearchResultSelector(state); + + const actual = selector('random-id'); + + assert.strictEqual(actual, undefined); + }); + + it('returns undefined if type is unexpected', () => { + const id = 'message-id'; + const state = { + ...getEmptyRootState(), + search: { + ...getEmptySearchState(), + messageLookup: { + [id]: { + ...getDefaultMessage(id), + type: 'keychange' as const, + snippet: 'snippet', + }, + }, + }, + }; + const selector = getMessageSearchResultSelector(state); + + const actual = selector(id); + + assert.strictEqual(actual, undefined); + }); + + it('returns incoming message', () => { + const searchId = 'search-id'; + const fromId = 'from-id'; + const toId = 'to-id'; + + const from = getDefaultConversation(fromId); + const to = getDefaultConversation(toId); + + const state = { + ...getEmptyRootState(), + conversations: { + ...getEmptyConversationState(), + conversationLookup: { + [fromId]: from, + [toId]: to, + }, + }, + search: { + ...getEmptySearchState(), + messageLookup: { + [searchId]: { + ...getDefaultMessage(searchId), + type: 'incoming' as const, + sourceUuid: fromId, + conversationId: toId, + snippet: 'snippet', + }, + }, + }, + }; + const selector = getMessageSearchResultSelector(state); + + const actual = selector(searchId); + const expected = { + from, + to, + + id: searchId, + conversationId: toId, + sentAt: undefined, + snippet: 'snippet', + + isSelected: false, + isSearchingInConversation: false, + }; + + assert.deepEqual(actual, expected); + }); + it('returns outgoing message and caches appropriately', () => { + const searchId = 'search-id'; + const fromId = 'from-id'; + const toId = 'to-id'; + + const from = getDefaultConversation(fromId); + const to = getDefaultConversation(toId); + + const state = { + ...getEmptyRootState(), + user: { + ...getEmptyUserState(), + ourConversationId: fromId, + }, + conversations: { + ...getEmptyConversationState(), + conversationLookup: { + [fromId]: from, + [toId]: to, + }, + }, + search: { + ...getEmptySearchState(), + messageLookup: { + [searchId]: { + ...getDefaultMessage(searchId), + type: 'outgoing' as const, + conversationId: toId, + snippet: 'snippet', + }, + }, + }, + }; + const selector = getMessageSearchResultSelector(state); + + const actual = selector(searchId); + const expected = { + from, + to, + + id: searchId, + conversationId: toId, + sentAt: undefined, + snippet: 'snippet', + + isSelected: false, + isSearchingInConversation: false, + }; + + assert.deepEqual(actual, expected); + + // Update the conversation lookup, but not the conversations in question + const secondState = { + ...state, + conversations: { + ...state.conversations, + conversationLookup: { + ...state.conversations.conversationLookup, + }, + }, + }; + const secondSelector = getMessageSearchResultSelector(secondState); + const secondActual = secondSelector(searchId); + + assert.strictEqual(secondActual, actual); + + // Update a conversation involved in rendering this search result + const thirdState = { + ...state, + conversations: { + ...state.conversations, + conversationLookup: { + ...state.conversations.conversationLookup, + [fromId]: { + ...from, + name: 'new-name', + }, + }, + }, + }; + + const thirdSelector = getMessageSearchResultSelector(thirdState); + const thirdActual = thirdSelector(searchId); + + assert.notStrictEqual(actual, thirdActual); + }); + }); +}); diff --git a/ts/test-both/util/makeLookup_test.ts b/ts/test-both/util/makeLookup_test.ts new file mode 100644 index 0000000000..635081cb72 --- /dev/null +++ b/ts/test-both/util/makeLookup_test.ts @@ -0,0 +1,75 @@ +// Copyright 2021 Signal Messenger, LLC +// SPDX-License-Identifier: AGPL-3.0-only + +import { assert } from 'chai'; +import { makeLookup } from '../../util/makeLookup'; + +describe('makeLookup', () => { + it('returns an empty object if passed an empty array', () => { + const result = makeLookup([], 'foo'); + + assert.deepEqual(result, {}); + }); + + it('returns an object that lets you look up objects by string key', () => { + const arr = [{ foo: 'bar' }, { foo: 'baz' }, { foo: 'qux' }]; + const result = makeLookup(arr, 'foo'); + + assert.hasAllKeys(result, ['bar', 'baz', 'qux']); + assert.strictEqual(result.bar, arr[0]); + assert.strictEqual(result.baz, arr[1]); + assert.strictEqual(result.qux, arr[2]); + }); + + it('if there are duplicates, the last one wins', () => { + const arr = [ + { foo: 'bar', first: true }, + { foo: 'bar', first: false }, + ]; + const result = makeLookup(arr, 'foo'); + + assert.deepEqual(result, { + bar: { foo: 'bar', first: false }, + }); + }); + + it('ignores falsy properties', () => { + const arr = [{}, { foo: '' }, { foo: false }, { foo: null }]; + const result = makeLookup(arr, 'foo'); + + assert.deepEqual(result, {}); + }); + + it('converts the lookup to a string', () => { + const arr = [ + { foo: 'bar' }, + { foo: 123 }, + { foo: {} }, + { + foo: { + toString() { + return 'baz'; + }, + }, + }, + {}, + ]; + const result = makeLookup(arr, 'foo'); + + assert.hasAllKeys(result, ['bar', '123', '[object Object]', 'baz']); + assert.strictEqual(result.bar, arr[0]); + assert.strictEqual(result['123'], arr[1]); + assert.strictEqual(result['[object Object]'], arr[2]); + assert.strictEqual(result.baz, arr[3]); + }); + + it('looks up own and inherited properties', () => { + const prototype = { foo: 'baz' }; + + const arr = [{ foo: 'bar' }, Object.create(prototype)]; + const result = makeLookup(arr, 'foo'); + + assert.strictEqual(result.bar, arr[0]); + assert.strictEqual(result.baz, arr[1]); + }); +}); diff --git a/ts/test-electron/state/ducks/conversations_test.ts b/ts/test-electron/state/ducks/conversations_test.ts index 3a87303359..9f01edea0c 100644 --- a/ts/test-electron/state/ducks/conversations_test.ts +++ b/ts/test-electron/state/ducks/conversations_test.ts @@ -9,8 +9,10 @@ import { ConversationsStateType, ConversationType, getConversationCallMode, + getEmptyState, MessageType, reducer, + updateConversationLookups, } from '../../../state/ducks/conversations'; import { CallMode } from '../../../types/Calling'; @@ -126,6 +128,136 @@ describe('both/state/ducks/conversations', () => { ); }); }); + + describe('updateConversationLookups', () => { + function getDefaultConversation(id: string): ConversationType { + return { + id, + type: 'direct', + title: `${id} title`, + }; + } + + it('does not change lookups if no conversations provided', () => { + const state = getEmptyState(); + const result = updateConversationLookups(undefined, undefined, state); + + assert.strictEqual( + state.conversationsByE164, + result.conversationsByE164 + ); + assert.strictEqual( + state.conversationsByUuid, + result.conversationsByUuid + ); + assert.strictEqual( + state.conversationsByGroupId, + result.conversationsByGroupId + ); + }); + + it('adds and removes e164-only contact', () => { + const removed = { + ...getDefaultConversation('id-removed'), + e164: 'e164-removed', + }; + + const state = { + ...getEmptyState(), + conversationsByE164: { + [removed.e164]: removed, + }, + }; + const added = { + ...getDefaultConversation('id-added'), + e164: 'e164-added', + }; + + const expected = { + [added.e164]: added, + }; + + const actual = updateConversationLookups(added, removed, state); + + assert.deepEqual(actual.conversationsByE164, expected); + assert.strictEqual( + state.conversationsByUuid, + actual.conversationsByUuid + ); + assert.strictEqual( + state.conversationsByGroupId, + actual.conversationsByGroupId + ); + }); + + it('adds and removes uuid-only contact', () => { + const removed = { + ...getDefaultConversation('id-removed'), + uuid: 'uuid-removed', + }; + + const state = { + ...getEmptyState(), + conversationsByuuid: { + [removed.uuid]: removed, + }, + }; + const added = { + ...getDefaultConversation('id-added'), + uuid: 'uuid-added', + }; + + const expected = { + [added.uuid]: added, + }; + + const actual = updateConversationLookups(added, removed, state); + + assert.strictEqual( + state.conversationsByE164, + actual.conversationsByE164 + ); + assert.deepEqual(actual.conversationsByUuid, expected); + assert.strictEqual( + state.conversationsByGroupId, + actual.conversationsByGroupId + ); + }); + + it('adds and removes groupId-only contact', () => { + const removed = { + ...getDefaultConversation('id-removed'), + groupId: 'groupId-removed', + }; + + const state = { + ...getEmptyState(), + conversationsBygroupId: { + [removed.groupId]: removed, + }, + }; + const added = { + ...getDefaultConversation('id-added'), + groupId: 'groupId-added', + }; + + const expected = { + [added.groupId]: added, + }; + + const actual = updateConversationLookups(added, removed, state); + + assert.strictEqual( + state.conversationsByE164, + actual.conversationsByE164 + ); + assert.strictEqual( + state.conversationsByUuid, + actual.conversationsByUuid + ); + assert.deepEqual(actual.conversationsByGroupId, expected); + }); + }); }); describe('reducer', () => { @@ -135,22 +267,12 @@ describe('both/state/ducks/conversations', () => { const messageIdTwo = 'message-guid-2'; const messageIdThree = 'message-guid-3'; - function getDefaultState(): ConversationsStateType { - return { - conversationLookup: {}, - selectedMessageCounter: 0, - selectedConversationPanelDepth: 0, - showArchived: false, - messagesLookup: {}, - messagesByConversation: {}, - }; - } - function getDefaultMessage(id: string): MessageType { return { id, conversationId: 'conversationId', source: 'source', + sourceUuid: 'sourceUuid', type: 'incoming' as const, received_at: Date.now(), attachments: [], @@ -174,7 +296,7 @@ describe('both/state/ducks/conversations', () => { describe('MESSAGE_SIZE_CHANGED', () => { const stateWithActiveConversation = { - ...getDefaultState(), + ...getEmptyState(), messagesByConversation: { [conversationId]: { heightChangeMessageIds: [], @@ -192,7 +314,7 @@ describe('both/state/ducks/conversations', () => { }; it('does nothing if no conversation is active', () => { - const state = getDefaultState(); + const state = getEmptyState(); assert.strictEqual( reducer(state, messageSizeChanged('messageId', 'convoId')), @@ -246,7 +368,7 @@ describe('both/state/ducks/conversations', () => { it('updates newest', () => { const action = repairNewestMessage(conversationId); const state: ConversationsStateType = { - ...getDefaultState(), + ...getEmptyState(), messagesLookup: { [messageId]: { ...getDefaultMessage(messageId), @@ -265,7 +387,7 @@ describe('both/state/ducks/conversations', () => { }; const expected: ConversationsStateType = { - ...getDefaultState(), + ...getEmptyState(), messagesLookup: { [messageId]: { ...getDefaultMessage(messageId), @@ -294,7 +416,7 @@ describe('both/state/ducks/conversations', () => { it('clears newest', () => { const action = repairNewestMessage(conversationId); const state: ConversationsStateType = { - ...getDefaultState(), + ...getEmptyState(), messagesLookup: { [messageId]: { ...getDefaultMessage(messageId), @@ -317,7 +439,7 @@ describe('both/state/ducks/conversations', () => { }; const expected: ConversationsStateType = { - ...getDefaultState(), + ...getEmptyState(), messagesLookup: { [messageId]: { ...getDefaultMessage(messageId), @@ -342,7 +464,7 @@ describe('both/state/ducks/conversations', () => { it('returns state if conversation not present', () => { const action = repairNewestMessage(conversationId); - const state: ConversationsStateType = getDefaultState(); + const state: ConversationsStateType = getEmptyState(); const actual = reducer(state, action); assert.equal(actual, state); @@ -353,7 +475,7 @@ describe('both/state/ducks/conversations', () => { it('updates oldest', () => { const action = repairOldestMessage(conversationId); const state: ConversationsStateType = { - ...getDefaultState(), + ...getEmptyState(), messagesLookup: { [messageId]: { ...getDefaultMessage(messageId), @@ -372,7 +494,7 @@ describe('both/state/ducks/conversations', () => { }; const expected: ConversationsStateType = { - ...getDefaultState(), + ...getEmptyState(), messagesLookup: { [messageId]: { ...getDefaultMessage(messageId), @@ -401,7 +523,7 @@ describe('both/state/ducks/conversations', () => { it('clears oldest', () => { const action = repairOldestMessage(conversationId); const state: ConversationsStateType = { - ...getDefaultState(), + ...getEmptyState(), messagesLookup: { [messageId]: { ...getDefaultMessage(messageId), @@ -424,7 +546,7 @@ describe('both/state/ducks/conversations', () => { }; const expected: ConversationsStateType = { - ...getDefaultState(), + ...getEmptyState(), messagesLookup: { [messageId]: { ...getDefaultMessage(messageId), @@ -449,7 +571,7 @@ describe('both/state/ducks/conversations', () => { it('returns state if conversation not present', () => { const action = repairOldestMessage(conversationId); - const state: ConversationsStateType = getDefaultState(); + const state: ConversationsStateType = getEmptyState(); const actual = reducer(state, action); assert.equal(actual, state); diff --git a/ts/util/makeLookup.ts b/ts/util/makeLookup.ts index ac9cbdf4f6..c8df84c8f3 100644 --- a/ts/util/makeLookup.ts +++ b/ts/util/makeLookup.ts @@ -1,13 +1,17 @@ // Copyright 2019-2020 Signal Messenger, LLC // SPDX-License-Identifier: AGPL-3.0-only -import { fromPairs, map } from 'lodash'; - export function makeLookup( items: Array, key: keyof T -): { [key: string]: T } { - const pairs = map(items, item => [item[key], item]); - - return fromPairs(pairs); +): Record { + return (items || []).reduce((lookup, item) => { + if (item && item[key]) { + // The force cast is necessary if we want the keyof T above, and the flexibility + // to pass anything in. And of course we're modifying a parameter! + // eslint-disable-next-line no-param-reassign + lookup[String(item[key])] = item; + } + return lookup; + }, {} as Record); }