diff --git a/src/pm-conversations/__tests__/pmConversationsSelectors-test.js b/src/pm-conversations/__tests__/pmConversationsSelectors-test.js index 52fa55a8851..f665209a1ac 100644 --- a/src/pm-conversations/__tests__/pmConversationsSelectors-test.js +++ b/src/pm-conversations/__tests__/pmConversationsSelectors-test.js @@ -7,6 +7,7 @@ describe('getRecentConversations', () => { test('when no messages, return no conversations', () => { const state = deepFreeze({ realm: { email: 'me@example.com' }, + users: [{ user_id: 0, email: 'me@example.com' }], narrows: { [ALL_PRIVATE_NARROW_STR]: [], }, @@ -24,6 +25,11 @@ describe('getRecentConversations', () => { test('returns unique list of recipients, includes conversations with self', () => { const state = deepFreeze({ realm: { email: 'me@example.com' }, + users: [ + { user_id: 0, email: 'me@example.com' }, + { user_id: 1, email: 'john@example.com' }, + { user_id: 2, email: 'mark@example.com' }, + ], narrows: { [ALL_PRIVATE_NARROW_STR]: [0, 1, 2, 3, 4], }, @@ -126,6 +132,11 @@ describe('getRecentConversations', () => { test('returns recipients sorted by last activity', () => { const state = deepFreeze({ realm: { email: 'me@example.com' }, + users: [ + { user_id: 0, email: 'me@example.com' }, + { user_id: 1, email: 'john@example.com' }, + { user_id: 2, email: 'mark@example.com' }, + ], narrows: { [ALL_PRIVATE_NARROW_STR]: [1, 2, 3, 4, 5, 6], }, diff --git a/src/pm-conversations/pmConversationsSelectors.js b/src/pm-conversations/pmConversationsSelectors.js index 09b251caef3..f8efbd8ba21 100644 --- a/src/pm-conversations/pmConversationsSelectors.js +++ b/src/pm-conversations/pmConversationsSelectors.js @@ -1,26 +1,26 @@ /* @flow strict-local */ import { createSelector } from 'reselect'; -import type { Message, PmConversationData, Selector } from '../types'; +import type { Message, PmConversationData, Selector, User } from '../types'; import { getPrivateMessages } from '../message/messageSelectors'; -import { getOwnEmail } from '../users/userSelectors'; +import { getOwnUser } from '../users/userSelectors'; import { getUnreadByPms, getUnreadByHuddles } from '../unread/unreadSelectors'; -import { normalizeRecipientsSansMe, getRecipientsIds } from '../utils/recipient'; +import { normalizeRecipientsSansMe, pmUnreadsKeyFromMessage } from '../utils/recipient'; export const getRecentConversations: Selector = createSelector( - getOwnEmail, + getOwnUser, getPrivateMessages, getUnreadByPms, getUnreadByHuddles, ( - ownEmail: string, + ownUser: User, messages: Message[], unreadPms: { [number]: number }, unreadHuddles: { [string]: number }, ): PmConversationData[] => { const recipients = messages.map(msg => ({ - ids: getRecipientsIds(msg, ownEmail), - emails: normalizeRecipientsSansMe(msg.display_recipient, ownEmail), + ids: pmUnreadsKeyFromMessage(msg, ownUser.user_id), + emails: normalizeRecipientsSansMe(msg.display_recipient, ownUser.email), msgId: msg.id, })); @@ -43,6 +43,10 @@ export const getRecentConversations: Selector = createSele return sortedByMostRecent.map(recipient => ({ ...recipient, unread: + // This business of looking in one place and then the other is kind + // of messy. Fortunately it always works, because the key spaces + // are disjoint: all `unreadHuddles` keys contain a comma, and all + // `unreadPms` keys don't. /* $FlowFixMe: The keys of unreadPms are logically numbers, but because it's an object they end up converted to strings, so this access with string keys works. We should probably use a Map for this and similar maps. */ diff --git a/src/unread/unreadHuddlesReducer.js b/src/unread/unreadHuddlesReducer.js index f7bf8977ecd..64b7da25db0 100644 --- a/src/unread/unreadHuddlesReducer.js +++ b/src/unread/unreadHuddlesReducer.js @@ -8,7 +8,7 @@ import { EVENT_MESSAGE_DELETE, EVENT_UPDATE_MESSAGE_FLAGS, } from '../actionConstants'; -import { getRecipientsIds } from '../utils/recipient'; +import { pmUnreadsKeyFromMessage } from '../utils/recipient'; import { addItemsToHuddleArray, removeItemsDeeply } from './unreadHelpers'; import { NULL_ARRAY } from '../nullObjects'; @@ -27,7 +27,7 @@ const eventNewMessage = (state, action) => { return state; } - return addItemsToHuddleArray(state, [action.message.id], getRecipientsIds(action.message)); + return addItemsToHuddleArray(state, [action.message.id], pmUnreadsKeyFromMessage(action.message)); }; const eventUpdateMessageFlags = (state, action) => { diff --git a/src/utils/recipient.js b/src/utils/recipient.js index c7a59241816..fc371dc7a54 100644 --- a/src/utils/recipient.js +++ b/src/utils/recipient.js @@ -64,8 +64,9 @@ export const pmUiRecipientsFromMessage = ( * including stream and topic narrows. * * * `normalizeRecipients`, `normalizeRecipientsSansMe`, and - * `getRecipientsIds`, which do the same job as this function with slight - * variations, and which we variously use in different places in the app. + * `pmUnreadsKeyFromMessage`, which do the same job as this function with + * slight variations, and which we variously use in different places in + * the app. * * It would be great to unify on a single version, as the variation is a * possible source of bugs. @@ -80,21 +81,44 @@ export const pmKeyRecipientsFromMessage = ( return filterRecipients(message.display_recipient, ownUser.user_id); }; -export const getRecipientsIds = (message: Message, ownEmail?: string): string => { +/** + * The key this PM is filed under in the "unread messages" data structure. + * + * Note this diverges slightly from pmKeyRecipientsFromMessage in its + * behavior -- it encodes a different set of users. + * + * See also: + * * `pmKeyRecipientsFromMessage`, which we use for other data structures. + * * `UnreadState`, the type of `state.unread`, which is the data structure + * these keys appear in. + * + * @param ownUserId - Required if the message could be a 1:1 PM; optional if + * it is definitely a group PM. + */ +// Specifically, this includes all user IDs for group PMs and self-PMs, +// and just the other user ID for non-self 1:1s; and in each case the list +// is sorted numerically and encoded in ASCII-decimal, comma-separated. +// See the `unread_msgs` data structure in `src/api/initialDataTypes.js`. +export const pmUnreadsKeyFromMessage = (message: Message, ownUserId?: number): string => { if (message.type !== 'private') { - throw new Error('getRecipientsIds: expected PM, got stream message'); + throw new Error('pmUnreadsKeyFromMessage: expected PM, got stream message'); } - const recipients = message.display_recipient; - if (recipients.length === 2) { - if (ownEmail === undefined) { - throw new Error('getRecipientsIds: got 1:1 PM, but ownEmail omitted'); + const recipients: PmRecipientUser[] = message.display_recipient; + // This includes all users in the thread; see `Message#display_recipient`. + const userIds = recipients.map(r => r.id); + + if (userIds.length === 1) { + // Self-PM. + return userIds[0].toString(); + } else if (userIds.length === 2) { + // Non-self 1:1 PM. Unlike display_recipient, leave out the self user. + if (ownUserId === undefined) { + throw new Error('getRecipientsIds: got 1:1 PM, but ownUserId omitted'); } - return recipients.filter(r => r.email !== ownEmail)[0].id.toString(); + return userIds.filter(userId => userId !== ownUserId)[0].toString(); } else { - return recipients - .map(s => s.id) - .sort((a, b) => a - b) - .join(','); + // Group PM. + return userIds.sort((a, b) => a - b).join(','); } };