recipient [nfc]: Clean up and document how we compute unread_msgs keys.

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],
       },

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<PmConversationData[]> = 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<PmConversationData[]> = 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. */

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) => {

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(',');
   }
 };