diff --git a/src/chat/__tests__/narrowsSelectors-test.js b/src/chat/__tests__/narrowsSelectors-test.js index f75f4d775ef..83e885fc886 100644 --- a/src/chat/__tests__/narrowsSelectors-test.js +++ b/src/chat/__tests__/narrowsSelectors-test.js @@ -37,7 +37,8 @@ describe('getMessagesForNarrow', () => { }), messages, outbox: [], - realm: eg.realmState({ email: eg.selfUser.email }), + users: [eg.selfUser], + realm: eg.realmState({ user_id: eg.selfUser.user_id, email: eg.selfUser.email }), }); const result = getMessagesForNarrow(state, HOME_NARROW); @@ -55,7 +56,8 @@ describe('getMessagesForNarrow', () => { caughtUp: { [HOME_NARROW_STR]: { older: false, newer: true }, }, - realm: eg.realmState({ email: eg.selfUser.email }), + users: [eg.selfUser], + realm: eg.realmState({ user_id: eg.selfUser.user_id, email: eg.selfUser.email }), }); const result = getMessagesForNarrow(state, HOME_NARROW); @@ -70,7 +72,8 @@ describe('getMessagesForNarrow', () => { }), messages, outbox: [outboxMessage], - realm: eg.realmState({ email: eg.selfUser.email }), + users: [eg.selfUser], + realm: eg.realmState({ user_id: eg.selfUser.user_id, email: eg.selfUser.email }), }); const result = getMessagesForNarrow(state, HOME_NARROW); @@ -85,7 +88,8 @@ describe('getMessagesForNarrow', () => { }), messages, outbox: [outboxMessage], - realm: eg.realmState({ email: eg.selfUser.email }), + users: [eg.selfUser], + realm: eg.realmState({ user_id: eg.selfUser.user_id, email: eg.selfUser.email }), }); const result = getMessagesForNarrow(state, pm1to1NarrowFromUser(eg.otherUser)); diff --git a/src/chat/narrowsSelectors.js b/src/chat/narrowsSelectors.js index b62d402011f..7e5600dc0f4 100644 --- a/src/chat/narrowsSelectors.js +++ b/src/chat/narrowsSelectors.js @@ -22,7 +22,7 @@ import { getOutbox, } from '../directSelectors'; import { getCaughtUpForNarrow } from '../caughtup/caughtUpSelectors'; -import { getAllUsersByEmail, getOwnEmail } from '../users/userSelectors'; +import { getAllUsersByEmail, getOwnUser } from '../users/userSelectors'; import { isStreamOrTopicNarrow, emailsOfGroupPmNarrow, @@ -38,8 +38,8 @@ export const outboxMessagesForNarrow: Selector = createSelecto (state, narrow) => narrow, getCaughtUpForNarrow, state => getOutbox(state), - getOwnEmail, - (narrow, caughtUp, outboxMessages, ownEmail) => { + getOwnUser, + (narrow, caughtUp, outboxMessages, ownUser) => { if (!caughtUp.newer) { return NULL_ARRAY; } @@ -52,7 +52,7 @@ export const outboxMessagesForNarrow: Selector = createSelecto // messages can't be starred, so "no flags" gives that the right answer. const fakeFlags = []; const filtered = outboxMessages.filter(message => - isMessageInNarrow(message, fakeFlags, narrow, ownEmail), + isMessageInNarrow(message, fakeFlags, narrow, ownUser), ); return isEqual(filtered, outboxMessages) ? outboxMessages : filtered; }, diff --git a/src/events/doEventActionSideEffects.js b/src/events/doEventActionSideEffects.js index 246eda54adf..121b8328cd5 100644 --- a/src/events/doEventActionSideEffects.js +++ b/src/events/doEventActionSideEffects.js @@ -10,6 +10,7 @@ import { getActiveAccount, getChatScreenParams, getOwnEmail } from '../selectors import { playMessageSound } from '../utils/sound'; import { NULL_ARRAY } from '../nullObjects'; import { ensureTypingStatusExpiryLoop } from '../typing/typingActions'; +import { getOwnUser } from '../users/userSelectors'; /** * React to incoming `MessageEvent`s. @@ -34,7 +35,7 @@ const messageEvent = (state: GlobalState, message: Message): void => { activeAccount && narrow !== undefined // chat screen is not at top && !isHomeNarrow(narrow) - && isMessageInNarrow(message, flags, narrow, activeAccount.email); + && isMessageInNarrow(message, flags, narrow, getOwnUser(state)); const isSenderSelf = getOwnEmail(state) === message.sender_email; if (!isUserInSameNarrow && !isSenderSelf) { playMessageSound(); diff --git a/src/topics/__tests__/topicsSelectors-test.js b/src/topics/__tests__/topicsSelectors-test.js index 7bc1ba47efc..35cd65eb9b7 100644 --- a/src/topics/__tests__/topicsSelectors-test.js +++ b/src/topics/__tests__/topicsSelectors-test.js @@ -33,7 +33,8 @@ describe('getLastMessageTopic', () => { test('when no messages in narrow return an empty string', () => { const state = eg.reduxState({ narrows: Immutable.Map({}), - realm: eg.realmState({ email: eg.selfUser.email }), + users: [eg.selfUser], + realm: eg.realmState({ user_id: eg.selfUser.user_id, email: eg.selfUser.email }), }); const topic = getLastMessageTopic(state, HOME_NARROW); @@ -53,7 +54,8 @@ describe('getLastMessageTopic', () => { [message1.id]: message1, [message2.id]: message2, }, - realm: eg.realmState({ email: eg.selfUser.email }), + users: [eg.selfUser], + realm: eg.realmState({ user_id: eg.selfUser.user_id, email: eg.selfUser.email }), }); const topic = getLastMessageTopic(state, narrow); diff --git a/src/unread/unreadSelectors.js b/src/unread/unreadSelectors.js index da982d94c89..03382992af2 100644 --- a/src/unread/unreadSelectors.js +++ b/src/unread/unreadSelectors.js @@ -11,11 +11,12 @@ import { getUnreadHuddles, getUnreadMentions, } from '../directSelectors'; -import { getOwnEmail, getAllUsersByEmail } from '../users/userSelectors'; +import { getOwnUserId } from '../users/userSelectors'; import { getSubscriptionsById } from '../subscriptions/subscriptionSelectors'; import { isTopicMuted } from '../utils/message'; import { caseNarrow } from '../utils/narrow'; -import { NULL_SUBSCRIPTION, NULL_USER } from '../nullObjects'; +import { NULL_SUBSCRIPTION } from '../nullObjects'; +import { pmUnreadsKeyFromPmKeyIds } from '../utils/recipient'; /** The number of unreads in each stream, excluding muted topics, by stream ID. */ export const getUnreadByStream: Selector<{ [number]: number }> = createSelector( @@ -221,24 +222,13 @@ export const getUnreadByHuddlesMentionsAndPMs: Selector = createSelector export const getUnreadCountForNarrow: Selector = createSelector( (state, narrow) => narrow, state => getStreams(state), - state => getAllUsersByEmail(state), - state => getOwnEmail(state), + state => getOwnUserId(state), state => getUnreadTotal(state), state => getUnreadStreams(state), state => getUnreadHuddles(state), state => getUnreadPms(state), state => getMute(state), - ( - narrow, - streams, - allUsersByEmail, - ownEmail, - unreadTotal, - unreadStreams, - unreadHuddles, - unreadPms, - mute, - ) => { + (narrow, streams, ownUserId, unreadTotal, unreadStreams, unreadHuddles, unreadPms, mute) => { const sumLengths = unreads => unreads.reduce((sum, x) => sum + x.unread_message_ids.length, 0); return caseNarrow(narrow, { @@ -266,20 +256,14 @@ export const getUnreadCountForNarrow: Selector = createSelector( ); }, - pm: emails => { - if (emails.length > 1) { - const userIds = [...emails, ownEmail] - .map(email => (allUsersByEmail.get(email) || NULL_USER).user_id) - .sort((a, b) => a - b) - .join(','); - const unread = unreadHuddles.find(x => x.user_ids_string === userIds); + pm: (emails, ids) => { + if (ids.length > 1) { + const unreadsKey = pmUnreadsKeyFromPmKeyIds(ids, ownUserId); + const unread = unreadHuddles.find(x => x.user_ids_string === unreadsKey); return unread ? unread.unread_message_ids.length : 0; } else { - const sender = allUsersByEmail.get(emails[0]); - if (!sender) { - return 0; - } - const unread = unreadPms.find(x => x.sender_id === sender.user_id); + const senderId = ids[0]; + const unread = unreadPms.find(x => x.sender_id === senderId); return unread ? unread.unread_message_ids.length : 0; } }, diff --git a/src/utils/__tests__/narrow-test.js b/src/utils/__tests__/narrow-test.js index a151d9ed740..245784232f5 100644 --- a/src/utils/__tests__/narrow-test.js +++ b/src/utils/__tests__/narrow-test.js @@ -119,7 +119,6 @@ describe('SEARCH_NARROW', () => { }); describe('isMessageInNarrow', () => { - const ownEmail = eg.selfUser.email; const otherStream = eg.makeStream(); // prettier-ignore @@ -191,7 +190,7 @@ describe('isMessageInNarrow', () => { for (const [messageDescription, expected, message] of cases) { test(`${expected ? 'contains' : 'excludes'} ${messageDescription}`, () => { expect( - isMessageInNarrow(message, message.flags ?? [], narrow, ownEmail), + isMessageInNarrow(message, message.flags ?? [], narrow, eg.selfUser), ).toBe(expected); }); } diff --git a/src/utils/__tests__/recipient-test.js b/src/utils/__tests__/recipient-test.js index 63cb57d3684..6037d38e61d 100644 --- a/src/utils/__tests__/recipient-test.js +++ b/src/utils/__tests__/recipient-test.js @@ -1,61 +1,8 @@ import { - normalizeRecipients, normalizeRecipientsAsUserIds, - normalizeRecipientsSansMe, normalizeRecipientsAsUserIdsSansMe, isSameRecipient, } from '../recipient'; -import * as logging from '../logging'; - -describe('normalizeRecipients', () => { - test('joins emails from recipients, sorted, trimmed, not including missing ones', () => { - const recipients = [ - { email: '' }, - { email: 'abc@example.com' }, - { email: 'xyz@example.com' }, - { email: ' def@example.com ' }, - ]; - const expectedResult = 'abc@example.com,def@example.com,xyz@example.com'; - - logging.error.mockReturnValue(); - - const normalized = normalizeRecipients(recipients); - expect(normalized).toEqual(expectedResult); - - expect(logging.error.mock.calls).toHaveLength(2); - logging.error.mockReset(); - }); -}); - -describe('normalizeRecipientsSansMe', () => { - test('if only self email provided return unmodified', () => { - const recipients = [{ email: 'me@example.com' }]; - const ownEmail = 'me@example.com'; - const expectedResult = 'me@example.com'; - - const normalized = normalizeRecipientsSansMe(recipients, ownEmail); - - expect(normalized).toEqual(expectedResult); - }); - - test('when more than one emails normalize but filter out self email', () => { - const recipients = [ - { email: 'abc@example.com' }, - { email: 'me@example.com' }, - { email: ' def@example.com ' }, - ]; - const ownEmail = 'me@example.com'; - const expectedResult = 'abc@example.com,def@example.com'; - - logging.error.mockReturnValue(); - - const normalized = normalizeRecipientsSansMe(recipients, ownEmail); - expect(normalized).toEqual(expectedResult); - - expect(logging.error.mock.calls).toHaveLength(1); - logging.error.mockReset(); - }); -}); describe('normalizeRecipientsAsUserIds', () => { test('joins user IDs from recipients, sorted', () => { diff --git a/src/utils/narrow.js b/src/utils/narrow.js index 03c6b8469eb..c57793bee02 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -3,7 +3,7 @@ import invariant from 'invariant'; import type { ApiNarrow, Message, Outbox, User, UserOrBot } from '../types'; import { - normalizeRecipientsSansMe, + normalizeRecipientsAsUserIdsSansMe, pmKeyRecipientsFromMessage, recipientsOfPrivateMessage, streamNameOfStreamMessage, @@ -468,7 +468,7 @@ export const isMessageInNarrow = ( message: Message | Outbox, flags: $ReadOnlyArray, narrow: Narrow, - ownEmail: string, + ownUser: User, ): boolean => caseNarrow(narrow, { home: () => true, @@ -477,15 +477,15 @@ export const isMessageInNarrow = ( message.type === 'stream' && streamName === streamNameOfStreamMessage(message) && topic === message.subject, - pm: emails => { + pm: (emails, ids) => { if (message.type !== 'private') { return false; } - const recipients = recipientsOfPrivateMessage(message); - const narrowAsRecipients = emails.map(email => ({ email })); + const recipients = recipientsOfPrivateMessage(message).map(r => r.id); + const narrowAsRecipients = ids; return ( - normalizeRecipientsSansMe(recipients, ownEmail) - === normalizeRecipientsSansMe(narrowAsRecipients, ownEmail) + normalizeRecipientsAsUserIdsSansMe(recipients, ownUser.user_id) + === normalizeRecipientsAsUserIdsSansMe(narrowAsRecipients, ownUser.user_id) ); }, starred: () => flags.includes('starred'), diff --git a/src/utils/recipient.js b/src/utils/recipient.js index 1df7a16c91c..4553a5914ac 100644 --- a/src/utils/recipient.js +++ b/src/utils/recipient.js @@ -1,8 +1,8 @@ /* @flow strict-local */ import invariant from 'invariant'; +import isEqual from 'lodash.isequal'; import type { PmRecipientUser, Message, Outbox, User, UserOrBot } from '../types'; -import * as logging from './logging'; /** The stream name a stream message was sent to. Throws if a PM. */ export const streamNameOfStreamMessage = (message: Message | Outbox): string => { @@ -97,63 +97,18 @@ const filterRecipientUsers = ( : recipients.filter(r => r.user_id !== ownUserId).sort((a, b) => a.user_id - b.user_id); // Like filterRecipients, but on user IDs directly. -const filterRecipientsAsUserIds = >( - recipients: T, +const filterRecipientsAsUserIds = ( + recipients: $ReadOnlyArray, ownUserId: number, -): T => +): number[] => + // prettier-ignore recipients.length === 1 - ? recipients + // The spread is so that we always return a fresh array. This allows + // us to take $ReadOnlyArray and return a plain array, so the caller + // can go on to sort the result. + ? [...recipients] : recipients.filter(r => r !== ownUserId).sort((a, b) => a - b); -// Like filterRecipients, but identifying users by email address. -// Prefer filterRecipients instead. -// No sort; caller must sort if needed. -const filterRecipientsByEmail = ( - recipients: $ReadOnlyArray, - ownEmail: string, -): $ReadOnlyArray => - recipients.length === 1 ? recipients : recipients.filter(r => r.email !== ownEmail); - -/** PRIVATE -- exported only for tests. */ -export const normalizeRecipients = (recipients: $ReadOnlyArray<{ +email: string, ... }>) => { - const emails = recipients.map(r => r.email); - - if (emails.some(e => e.trim() !== e)) { - // This should never happen -- it makes the email address invalid. If - // there's some user input that might be accepted like this, it should - // be turned into a valid email address long before this point. We - // include this defensive logic only out of an abundance of caution - // because we had it, with no logging, for a long time. - logging.error('normalizeRecipients: got email with whitespace', { emails }); - } - if (emails.some(e => !e)) { - // This should similarly never happen -- it means we got an - // unrecoverably bogus email address in here. We carry on hoping, or - // pretending, that it just shouldn't have been in the list at all. - logging.error('normalizeRecipients: got empty email', { emails }); - } - // Both of these fudge conditions should really go away. We can do that - // after we've had a release out in the wild with the above logging for at - // least a few weeks, and seen no reports of them actually happening. - // Until then, conservatively keep fudging like we have for a long time. - const massagedEmails = emails.map(e => e.trim()).filter(Boolean); - - return massagedEmails.sort().join(','); -}; - -/** - * The same list of users as pmKeyRecipientsFromMessage, in quirkier form. - * - * Prefer normalizeRecipientsAsUserIdsSansMe over this; see #3764. - * See that function for further discussion. - * - * Users are sorted by email address. - */ -export const normalizeRecipientsSansMe = ( - recipients: $ReadOnlyArray<{ +email: string, ... }>, - ownEmail: string, -) => normalizeRecipients(filterRecipientsByEmail(recipients, ownEmail)); - export const normalizeRecipientsAsUserIds = (recipients: number[]) => recipients.sort((a, b) => a - b).join(','); @@ -164,8 +119,10 @@ export const normalizeRecipientsAsUserIds = (recipients: number[]) => // (see comment on Message#display_recipient). Then for 1:1 PMs the // server's behavior is quirkier... but we keep only one user for those // anyway, so it doesn't matter. -export const normalizeRecipientsAsUserIdsSansMe = (recipients: number[], ownUserId: number) => - normalizeRecipientsAsUserIds(filterRecipientsAsUserIds(recipients, ownUserId)); +export const normalizeRecipientsAsUserIdsSansMe = ( + recipients: $ReadOnlyArray, + ownUserId: number, +) => normalizeRecipientsAsUserIds(filterRecipientsAsUserIds(recipients, ownUserId)); /** * The set of users to show in the UI to identify a PM conversation. @@ -282,6 +239,8 @@ export const pmKeyRecipientsFromUsers = ( * * See also: * * `pmKeyRecipientsFromMessage`, which we use for other data structures. + * * `pmUnreadsKeyFromPmKeyIds`, for getting one of these keys given what + * we use for other data structures. * * `UnreadState`, the type of `state.unread`, which is the data structure * these keys appear in. * @@ -316,6 +275,29 @@ export const pmUnreadsKeyFromMessage = (message: Message, ownUserId?: number): s } }; +/** + * The key for a PM thread in "unreads" data, given the key we use elsewhere. + * + * This produces the same key string that `pmUnreadsKeyFromMessage` would + * give, given the list of users that `pmKeyRecipientsFromMessage` would + * give and which we use in most of our other data structures. + */ +// See comment on pmUnreadsKeyFromMessage for details on this form. +export const pmUnreadsKeyFromPmKeyIds = ( + userIds: $ReadOnlyArray, + ownUserId: number, +): string => { + if (userIds.length === 1) { + // A 1:1 PM. Both forms include just one user: the other user if any, + // and self for a self-1:1. + return userIds[0].toString(); + } else { + // A group PM. Our main "key" form includes just the other users; + // this form includes all users. + return [...userIds, ownUserId].sort((a, b) => a - b).join(','); + } +}; + export const isSameRecipient = ( message1: Message | Outbox, message2: Message | Outbox, @@ -330,9 +312,9 @@ export const isSameRecipient = ( switch (message1.type) { case 'private': - return ( - normalizeRecipients(recipientsOfPrivateMessage(message1)).toLowerCase() - === normalizeRecipients(recipientsOfPrivateMessage(message2)).toLowerCase() + return isEqual( + recipientsOfPrivateMessage(message1).map(r => r.id), + recipientsOfPrivateMessage(message2).map(r => r.id), ); case 'stream': return (