From af84897a512b9fa0fa1b6ebe8b43fd1732a7c1f2 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 11 Dec 2020 22:08:13 -0800 Subject: [PATCH 1/6] narrow [nfc]: Take whole own-user in isMessageInNarrow, to have ID. This sets us up to stop using emails here entirely in favor of user IDs; we'll do that in the next commit. --- src/chat/__tests__/narrowsSelectors-test.js | 12 ++++++++---- src/chat/narrowsSelectors.js | 8 ++++---- src/events/doEventActionSideEffects.js | 3 ++- src/topics/__tests__/topicsSelectors-test.js | 6 ++++-- src/utils/__tests__/narrow-test.js | 3 +-- src/utils/narrow.js | 6 +++--- 6 files changed, 22 insertions(+), 16 deletions(-) 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/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/narrow.js b/src/utils/narrow.js index 03c6b8469eb..6ef3d408d20 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -468,7 +468,7 @@ export const isMessageInNarrow = ( message: Message | Outbox, flags: $ReadOnlyArray, narrow: Narrow, - ownEmail: string, + ownUser: User, ): boolean => caseNarrow(narrow, { home: () => true, @@ -484,8 +484,8 @@ export const isMessageInNarrow = ( const recipients = recipientsOfPrivateMessage(message); const narrowAsRecipients = emails.map(email => ({ email })); return ( - normalizeRecipientsSansMe(recipients, ownEmail) - === normalizeRecipientsSansMe(narrowAsRecipients, ownEmail) + normalizeRecipientsSansMe(recipients, ownUser.email) + === normalizeRecipientsSansMe(narrowAsRecipients, ownUser.email) ); }, starred: () => flags.includes('starred'), From aee9c5eb96cd8bb4c8b7c7186a893b431182e4ab Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 11 Dec 2020 22:15:00 -0800 Subject: [PATCH 2/6] narrow: Use user IDs in isMessageInNarrow, instead of emails. The way this change might affect behavior is if we receive a message where the email for some user doesn't agree (e.g. because the user changed their email) with one we've seen before, in particular one we've previously used to identify that narrow in `state.narrows`. The old code would fail to make the connection, and so wouldn't put the new message in the existing narrow, which could make it fail to show up in the message list for that conversation. After this change, we'll correctly place the new message in the narrow. --- src/utils/narrow.js | 12 ++++++------ src/utils/recipient.js | 20 +++++++++++++------- 2 files changed, 19 insertions(+), 13 deletions(-) diff --git a/src/utils/narrow.js b/src/utils/narrow.js index 6ef3d408d20..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, @@ -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, ownUser.email) - === normalizeRecipientsSansMe(narrowAsRecipients, ownUser.email) + 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..f883fff140a 100644 --- a/src/utils/recipient.js +++ b/src/utils/recipient.js @@ -97,12 +97,16 @@ 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. @@ -150,7 +154,7 @@ export const normalizeRecipients = (recipients: $ReadOnlyArray<{ +email: string, * Users are sorted by email address. */ export const normalizeRecipientsSansMe = ( - recipients: $ReadOnlyArray<{ +email: string, ... }>, + recipients: $ReadOnlyArray, ownEmail: string, ) => normalizeRecipients(filterRecipientsByEmail(recipients, ownEmail)); @@ -164,8 +168,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. From 7e769af60d30792e9417648681b609faad7d4688 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sat, 12 Dec 2020 01:31:28 -0800 Subject: [PATCH 3/6] recipient [nfc]: Delete normalizeRecipientsSansMe! We deleted the last use of this function in the preceding commit. This in turn was one of two remaining callers of the quite hairy normalizeRecipients -- so deleting it gets us close to being able to delete that function too. --- src/utils/__tests__/recipient-test.js | 31 --------------------------- src/utils/recipient.js | 22 ------------------- 2 files changed, 53 deletions(-) diff --git a/src/utils/__tests__/recipient-test.js b/src/utils/__tests__/recipient-test.js index 63cb57d3684..7d6009b7281 100644 --- a/src/utils/__tests__/recipient-test.js +++ b/src/utils/__tests__/recipient-test.js @@ -1,7 +1,6 @@ import { normalizeRecipients, normalizeRecipientsAsUserIds, - normalizeRecipientsSansMe, normalizeRecipientsAsUserIdsSansMe, isSameRecipient, } from '../recipient'; @@ -27,36 +26,6 @@ describe('normalizeRecipients', () => { }); }); -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', () => { const recipients = [22, 1, 5, 3, 4]; diff --git a/src/utils/recipient.js b/src/utils/recipient.js index f883fff140a..802fc8324ae 100644 --- a/src/utils/recipient.js +++ b/src/utils/recipient.js @@ -109,15 +109,6 @@ const filterRecipientsAsUserIds = ( ? [...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); @@ -145,19 +136,6 @@ export const normalizeRecipients = (recipients: $ReadOnlyArray<{ +email: string, 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, - ownEmail: string, -) => normalizeRecipients(filterRecipientsByEmail(recipients, ownEmail)); - export const normalizeRecipientsAsUserIds = (recipients: number[]) => recipients.sort((a, b) => a - b).join(','); From 723635b5d2fda2680605454c7652ccc73601ca7e Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sat, 12 Dec 2020 01:58:33 -0800 Subject: [PATCH 4/6] recipient: Use IDs in isSameRecipient; cut out normalizeRecipients! The hairy logic in normalizeRecipients was one of the last few cases of the confusion around PM recipients we're seeking to remove for #4035. It also represents one of the many places we're in the process of converting from using emails to using numeric user IDs. The case where this could potentially change behavior is where we have several messages that are in the same PM conversation, but the `display_recipient` property on different messages disagrees about the email of some participant (e.g. because that user changed their email between when we learned about one message and when we learned about the other.) The old code would fail to notice the messages were in the same conversation, and so when showing them consecutively in an interleaved narrow (like all-messages or a search result), we'd show a new recipient bar between them. With this fix, we correctly identify the messages as belonging to the same conversation. --- src/utils/__tests__/recipient-test.js | 22 ----------------- src/utils/recipient.js | 35 +++------------------------ 2 files changed, 4 insertions(+), 53 deletions(-) diff --git a/src/utils/__tests__/recipient-test.js b/src/utils/__tests__/recipient-test.js index 7d6009b7281..6037d38e61d 100644 --- a/src/utils/__tests__/recipient-test.js +++ b/src/utils/__tests__/recipient-test.js @@ -1,30 +1,8 @@ import { - normalizeRecipients, normalizeRecipientsAsUserIds, 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('normalizeRecipientsAsUserIds', () => { test('joins user IDs from recipients, sorted', () => { diff --git a/src/utils/recipient.js b/src/utils/recipient.js index 802fc8324ae..cd140995166 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 => { @@ -109,33 +109,6 @@ const filterRecipientsAsUserIds = ( ? [...recipients] : recipients.filter(r => r !== ownUserId).sort((a, b) => a - b); -/** 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(','); -}; - export const normalizeRecipientsAsUserIds = (recipients: number[]) => recipients.sort((a, b) => a - b).join(','); @@ -314,9 +287,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 ( From fe0a2e9feb88ce298127e9f89516fd4013e3246e Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 11 Dec 2020 22:25:44 -0800 Subject: [PATCH 5/6] unread: Use IDs, not emails, in getUnreadCountForNarrow. As a bonus this eliminates one of the places using NULL_USER. The case where that could have affected behavior was... if looking at a PM conversation where one of the emails didn't correspond to any user we knew, perhaps because the user had changed their email. With this fix, we'll correctly show an UnreadNotice "N unread messages" banner if applicable. With the old code, this selector would return 0 and so we wouldn't show the banner. --- src/unread/unreadSelectors.js | 36 ++++++++++------------------------- 1 file changed, 10 insertions(+), 26 deletions(-) diff --git a/src/unread/unreadSelectors.js b/src/unread/unreadSelectors.js index da982d94c89..be01167ac0e 100644 --- a/src/unread/unreadSelectors.js +++ b/src/unread/unreadSelectors.js @@ -11,11 +11,11 @@ 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'; /** The number of unreads in each stream, excluding muted topics, by stream ID. */ export const getUnreadByStream: Selector<{ [number]: number }> = createSelector( @@ -221,24 +221,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 +255,15 @@ 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(','); + pm: (emails, ids) => { + if (ids.length > 1) { + // TODO this should go somewhere central like recipient.js + const userIds = [...ids, ownUserId].sort((a, b) => a - b).join(','); const unread = unreadHuddles.find(x => x.user_ids_string === userIds); 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; } }, From e7315cd2dce27897766b3bd9d83f6878a26ca807 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 16 Dec 2020 10:58:24 -0800 Subject: [PATCH 6/6] recipient [nfc]: Add explicit helper pmUnreadsKeyFromPmKeyIds. This lets us take out one more small bit of ad-hoc implementation of this sort of logic. In fact, after all the previous work we've done in this direction, this one appears to have been the last remaining such ad-hoc implementation! So this completes #4035. Fixes: #4035 --- src/unread/unreadSelectors.js | 6 +++--- src/utils/recipient.js | 25 +++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/src/unread/unreadSelectors.js b/src/unread/unreadSelectors.js index be01167ac0e..03382992af2 100644 --- a/src/unread/unreadSelectors.js +++ b/src/unread/unreadSelectors.js @@ -16,6 +16,7 @@ import { getSubscriptionsById } from '../subscriptions/subscriptionSelectors'; import { isTopicMuted } from '../utils/message'; import { caseNarrow } from '../utils/narrow'; 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( @@ -257,9 +258,8 @@ export const getUnreadCountForNarrow: Selector = createSelector( pm: (emails, ids) => { if (ids.length > 1) { - // TODO this should go somewhere central like recipient.js - const userIds = [...ids, ownUserId].sort((a, b) => a - b).join(','); - const unread = unreadHuddles.find(x => x.user_ids_string === userIds); + 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 senderId = ids[0]; diff --git a/src/utils/recipient.js b/src/utils/recipient.js index cd140995166..4553a5914ac 100644 --- a/src/utils/recipient.js +++ b/src/utils/recipient.js @@ -239,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. * @@ -273,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,