Fix some "is message in narrow" bugs, and test much more thoroughly

src/__tests__/lib/exampleData.js

@@ -179,6 +179,7 @@ export const selfAccount: Account = makeAccount({
 export const selfAuth: Auth = deepFreeze(authOfAccount(selfAccount));
 
 export const otherUser: User = makeUser({ name: 'other' });
+export const thirdUser: User = makeUser({ name: 'third' });
 
 export const crossRealmBot: CrossRealmBot = makeCrossRealmBot({ name: 'bot' });
 
@@ -293,14 +294,23 @@ const randMessageId: () => number = makeUniqueRandInt('message ID', 10000000);
  *
  * Beware! These values may not be representative.
  */
-export const pmMessage = (extra?: $Rest<Message, {}>): Message => {
+export const pmMessage = (args?: {|
+  ...$Rest<Message, {}>,
+  sender?: User,
+  recipients?: User[],
+|}): Message => {
+  // The `Object.freeze` is to work around a Flow issue:
+  //   https://github.com/facebook/flow/issues/2386#issuecomment-695064325
+  const { sender = otherUser, recipients = [otherUser, selfUser], ...extra } =
+    args ?? Object.freeze({});
+
   const baseMessage: Message = {
     ...messagePropertiesBase,
-    ...messagePropertiesFromSender(otherUser),
+    ...messagePropertiesFromSender(sender),
 
     content: 'This is an example PM message.',
     content_type: 'text/markdown',
-    display_recipient: [displayRecipientFromUser(selfUser)],
+    display_recipient: recipients.map(displayRecipientFromUser),
     id: randMessageId(),
     recipient_id: 2342,
     stream_id: -1,
@@ -327,9 +337,9 @@ const messagePropertiesFromStream = (stream1: Stream) => {
  * Beware! These values may not be representative.
  */
 export const streamMessage = (args?: {| ...$Rest<Message, {}>, stream?: Stream |}): Message => {
-  // The redundant `stream` in the ?? case avoids a Flow issue:
-  // https://github.com/facebook/flow/issues/2386
-  const { stream: streamInner = stream, ...extra } = args ?? { stream };
+  // The `Object.freeze` is to work around a Flow issue:
+  //   https://github.com/facebook/flow/issues/2386#issuecomment-695064325
+  const { stream: streamInner = stream, ...extra } = args ?? Object.freeze({});
 
   const baseMessage: Message = {
     ...messagePropertiesBase,

src/chat/__tests__/narrowsReducer-test.js

@@ -286,16 +286,19 @@ describe('narrowsReducer', () => {
       [groupNarrowStr]: [2, 4],
     });
 
-    const message = eg.pmMessage({ id: 5, flags: [] });
+    const message = eg.pmMessage({
+      id: 5,
+      flags: [],
+      sender: eg.selfUser,
+      display_recipient: [eg.displayRecipientFromUser(eg.selfUser), { email: '[email protected]' }],
+    });
 
     const action = deepFreeze({
       ...eg.eventNewMessageActionBase,
       message,
-      caughtUp: {
-        [HOME_NARROW_STR]: { older: false, newer: true },
-        [ALL_PRIVATE_NARROW_STR]: { older: false, newer: true },
-        [privateNarrowStr]: { older: false, newer: true },
-      },
+      caughtUp: Object.fromEntries(
+        Object.keys(initialState).map(key => [key, { older: false, newer: true }]),
+      ),
     });
 
     const expectedState = {

src/message/messagesActions.js

@@ -8,6 +8,7 @@ import { FIRST_UNREAD_ANCHOR } from '../anchor';
 import { getStreamsById } from '../subscriptions/subscriptionSelectors';
 import * as api from '../api';
 import { isUrlOnRealm } from '../utils/url';
+import { getOwnUserId } from '../users/userSelectors';
 
 /**
  * Navigate to the given narrow.
@@ -28,7 +29,8 @@ export const messageLinkPress = (href: string) => async (
   const auth = getAuth(state);
   const usersById = getUsersById(state);
   const streamsById = getStreamsById(state);
-  const narrow = getNarrowFromLink(href, auth.realm, usersById, streamsById);
+  const ownUserId = getOwnUserId(state);
+  const narrow = getNarrowFromLink(href, auth.realm, usersById, streamsById, ownUserId);
   if (narrow) {
     const anchor = getMessageIdFromLink(href, auth.realm);
     dispatch(doNarrow(narrow, anchor));

src/notification/__tests__/notification-test.js

@@ -12,11 +12,12 @@ import objectEntries from '../../utils/objectEntries';
 
 describe('getNarrowFromNotificationData', () => {
   const DEFAULT_MAP = new Map<number, User>();
+  const ownUserId = eg.selfUser.user_id;
 
   test('unknown notification data returns null', () => {
     // $FlowFixMe: actually validate APNs messages
     const notification: Notification = {};
-    const narrow = getNarrowFromNotificationData(notification, DEFAULT_MAP);
+    const narrow = getNarrowFromNotificationData(notification, DEFAULT_MAP, ownUserId);
     expect(narrow).toBe(null);
   });
 
@@ -26,7 +27,7 @@ describe('getNarrowFromNotificationData', () => {
       stream: 'some stream',
       topic: 'some topic',
     };
-    const narrow = getNarrowFromNotificationData(notification, DEFAULT_MAP);
+    const narrow = getNarrowFromNotificationData(notification, DEFAULT_MAP, ownUserId);
     expect(narrow).toEqual(topicNarrow('some stream', 'some topic'));
   });
 
@@ -35,22 +36,22 @@ describe('getNarrowFromNotificationData', () => {
       recipient_type: 'private',
       sender_email: '[email protected]',
     };
-    const narrow = getNarrowFromNotificationData(notification, DEFAULT_MAP);
+    const narrow = getNarrowFromNotificationData(notification, DEFAULT_MAP, ownUserId);
     expect(narrow).toEqual(privateNarrow('[email protected]'));
   });
 
   test('on notification for a group message returns a group narrow', () => {
-    const users = [eg.makeUser(), eg.makeUser(), eg.makeUser(), eg.makeUser()];
+    const users = [eg.selfUser, eg.makeUser(), eg.makeUser(), eg.makeUser()];
     const usersById: Map<number, User> = new Map(users.map(u => [u.user_id, u]));
 
     const notification = {
       recipient_type: 'private',
       pm_users: users.map(u => u.user_id).join(','),
     };
 
-    const expectedNarrow = groupNarrow(users.map(u => u.email));
+    const expectedNarrow = groupNarrow(users.slice(1).map(u => u.email));
 
-    const narrow = getNarrowFromNotificationData(notification, usersById);
+    const narrow = getNarrowFromNotificationData(notification, usersById, ownUserId);
 
     expect(narrow).toEqual(expectedNarrow);
   });
@@ -62,7 +63,7 @@ describe('getNarrowFromNotificationData', () => {
     };
     const usersById = new Map();
 
-    const narrow = getNarrowFromNotificationData(notification, usersById);
+    const narrow = getNarrowFromNotificationData(notification, usersById, ownUserId);
 
     expect(narrow).toBe(null);
   });

src/notification/extract.js

@@ -182,10 +182,11 @@ export const fromAPNsImpl = (rawData: JSONableDict): Notification | void => {
       if (typeof pm_users !== 'string') {
         throw err('invalid');
       }
-      if (pm_users.split(',').some(s => Number.isNaN(parseInt(s, 10)))) {
+      const ids = pm_users.split(',').map(s => parseInt(s, 10));
+      if (ids.some(id => Number.isNaN(id))) {
         throw err('invalid');
       }
-      return { recipient_type: 'private', pm_users, ...realm_uri_obj };
+      return { recipient_type: 'private', pm_users: ids.sort().join(','), ...realm_uri_obj };
     }
 
     if (typeof sender_email !== 'string') {

src/notification/index.js

@@ -17,6 +17,7 @@ import {
 import { identityOfAuth } from '../account/accountMisc';
 import { fromAPNs } from './extract';
 import { tryParseUrl } from '../utils/url';
+import { pmKeyRecipientsFromIds } from '../utils/recipient';
 
 /**
  * Identify the account the notification is for, if possible.
@@ -85,6 +86,7 @@ export const getAccountFromNotificationData = (
 export const getNarrowFromNotificationData = (
   data: Notification,
   usersById: Map<number, User>,
+  ownUserId: number,
 ): Narrow | null => {
   if (!data.recipient_type) {
     // This condition is impossible if the value is rightly-typed; but in
@@ -103,17 +105,9 @@ export const getNarrowFromNotificationData = (
     return privateNarrow(data.sender_email);
   }
 
-  const emails = [];
-  const idStrs = data.pm_users.split(',');
-  for (let i = 0; i < idStrs.length; ++i) {
-    const id = parseInt(idStrs[i], 10);
-    const user = usersById.get(id);
-    if (!user) {
-      return null;
-    }
-    emails.push(user.email);
-  }
-  return groupNarrow(emails);
+  const ids = data.pm_users.split(',').map(s => parseInt(s, 10));
+  const users = pmKeyRecipientsFromIds(ids, usersById, ownUserId);
+  return users && groupNarrow(users.map(u => u.email));
 };
 
 const getInitialNotification = async (): Promise<Notification | null> => {

src/notification/notificationActions.js

@@ -13,7 +13,7 @@ import { getAuth, getActiveAccount } from '../selectors';
 import { getSession, getAccounts } from '../directSelectors';
 import { GOT_PUSH_TOKEN, ACK_PUSH_TOKEN, UNACK_PUSH_TOKEN } from '../actionConstants';
 import { identityOfAccount, authOfAccount } from '../account/accountMisc';
-import { getUsersById } from '../users/userSelectors';
+import { getOwnUserId, getUsersById } from '../users/userSelectors';
 import { doNarrow } from '../message/messagesActions';
 import { accountSwitch } from '../account/accountActions';
 import { getIdentities } from '../account/accountsSelectors';
@@ -51,7 +51,7 @@ export const narrowToNotification = (data: ?Notification) => (
     return;
   }
 
-  const narrow = getNarrowFromNotificationData(data, getUsersById(state));
+  const narrow = getNarrowFromNotificationData(data, getUsersById(state), getOwnUserId(state));
   if (narrow) {
     dispatch(doNarrow(narrow));
   }

src/notification/types.js

@@ -14,7 +14,7 @@
 // NOTE: Keep the Android-side code in sync with this type definition.
 export type Notification =
   | {| recipient_type: 'stream', stream: string, topic: string, realm_uri?: string |}
-  // Group PM messages have `pm_users`, which is comma-separated IDs.
+  // Group PM messages have `pm_users`, which is sorted, comma-separated IDs.
   | {| recipient_type: 'private', pm_users: string, realm_uri?: string |}
   // 1:1 PM messages lack `pm_users`.
   | {| recipient_type: 'private', sender_email: string, realm_uri?: string |};

src/pm-conversations/__tests__/pmConversationsSelectors-test.js

@@ -27,43 +27,11 @@ describe('getRecentConversations', () => {
   });
 
   test('returns unique list of recipients, includes conversations with self', () => {
-    const meAndJohnPm1 = eg.pmMessage({
-      id: 1,
-      display_recipient: [
-        eg.displayRecipientFromUser(eg.selfUser),
-        eg.displayRecipientFromUser(userJohn),
-      ],
-    });
-
-    const meAndMarkPm = eg.pmMessage({
-      id: 2,
-      display_recipient: [
-        eg.displayRecipientFromUser(eg.selfUser),
-        eg.displayRecipientFromUser(userMark),
-      ],
-    });
-
-    const meAndJohnPm2 = eg.pmMessage({
-      id: 3,
-      display_recipient: [
-        eg.displayRecipientFromUser(eg.selfUser),
-        eg.displayRecipientFromUser(userJohn),
-      ],
-    });
-
-    const meOnlyPm = eg.pmMessage({
-      id: 4,
-      display_recipient: [eg.displayRecipientFromUser(eg.selfUser)],
-    });
-
-    const meJohnAndMarkPm = eg.pmMessage({
-      id: 0,
-      display_recipient: [
-        eg.displayRecipientFromUser(eg.selfUser),
-        eg.displayRecipientFromUser(userMark),
-        eg.displayRecipientFromUser(userJohn),
-      ],
-    });
+    const meAndJohnPm1 = eg.pmMessage({ id: 1, recipients: [eg.selfUser, userJohn] });
+    const meAndMarkPm = eg.pmMessage({ id: 2, recipients: [eg.selfUser, userMark] });
+    const meAndJohnPm2 = eg.pmMessage({ id: 3, recipients: [eg.selfUser, userJohn] });
+    const meOnlyPm = eg.pmMessage({ id: 4, recipients: [eg.selfUser] });
+    const meJohnAndMarkPm = eg.pmMessage({ id: 0, recipients: [eg.selfUser, userMark, userJohn] });
 
     const state = eg.reduxState({
       realm: eg.realmState({ email: eg.selfUser.email }),
@@ -150,52 +118,12 @@ describe('getRecentConversations', () => {
   test('returns recipients sorted by last activity', () => {
     // Maybe we can share these definitions with the above test;
     // first, we have to sort out why the IDs are different.
-
-    const meAndMarkPm1 = eg.pmMessage({
-      id: 1,
-      display_recipient: [
-        eg.displayRecipientFromUser(eg.selfUser),
-        eg.displayRecipientFromUser(userMark),
-      ],
-    });
-
-    const meAndJohnPm1 = eg.pmMessage({
-      id: 2,
-      display_recipient: [
-        eg.displayRecipientFromUser(eg.selfUser),
-        eg.displayRecipientFromUser(userJohn),
-      ],
-    });
-
-    const meAndMarkPm2 = eg.pmMessage({
-      id: 3,
-      display_recipient: [
-        eg.displayRecipientFromUser(eg.selfUser),
-        eg.displayRecipientFromUser(userMark),
-      ],
-    });
-
-    const meAndJohnPm2 = eg.pmMessage({
-      id: 4,
-      display_recipient: [
-        eg.displayRecipientFromUser(eg.selfUser),
-        eg.displayRecipientFromUser(userJohn),
-      ],
-    });
-
-    const meJohnAndMarkPm = eg.pmMessage({
-      id: 5,
-      display_recipient: [
-        eg.displayRecipientFromUser(eg.selfUser),
-        eg.displayRecipientFromUser(userJohn),
-        eg.displayRecipientFromUser(userMark),
-      ],
-    });
-
-    const meOnlyPm = eg.pmMessage({
-      id: 6,
-      display_recipient: [eg.displayRecipientFromUser(eg.selfUser)],
-    });
+    const meAndMarkPm1 = eg.pmMessage({ id: 1, recipients: [eg.selfUser, userMark] });
+    const meAndJohnPm1 = eg.pmMessage({ id: 2, recipients: [eg.selfUser, userJohn] });
+    const meAndMarkPm2 = eg.pmMessage({ id: 3, recipients: [eg.selfUser, userMark] });
+    const meAndJohnPm2 = eg.pmMessage({ id: 4, recipients: [eg.selfUser, userJohn] });
+    const meJohnAndMarkPm = eg.pmMessage({ id: 5, recipients: [eg.selfUser, userJohn, userMark] });
+    const meOnlyPm = eg.pmMessage({ id: 6, recipients: [eg.selfUser] });
 
     const state = eg.reduxState({
       realm: eg.realmState({ email: eg.selfUser.email }),

src/pm-conversations/pmConversationsSelectors.js

@@ -19,8 +19,12 @@ export const getRecentConversations: Selector<PmConversationData[]> = createSele
     unreadHuddles: { [string]: number },
   ): PmConversationData[] => {
     const recipients = messages.map(msg => ({
+      // Note this can be a different set of users from those in `emails` below.
       ids: pmUnreadsKeyFromMessage(msg, ownUser.user_id),
+
+      // The users represented in this `emails` string are sorted by email address.
       emails: normalizeRecipientsSansMe(msg.display_recipient, ownUser.email),
+
       msgId: msg.id,
     }));
 

src/reduxTypes.js

@@ -74,13 +74,15 @@ export type CaughtUp = {|
  *
  * See `CaughtUp` for details on what each value means.
  */
-export type CaughtUpState = {|
+export type CaughtUpState = {
+  // TODO(flow-v0.126): Should be exact. See note in src/utils/jsonable.js.
   [narrow: string]: CaughtUp,
-|};
+};
 
-export type DraftsState = {|
+export type DraftsState = {
+  // TODO(flow-v0.126): Should be exact. See note in src/utils/jsonable.js.
   [narrow: string]: string,
-|};
+};
 
 export type Fetching = {|
   older: boolean,
@@ -93,6 +95,7 @@ export type Fetching = {|
  * See also: `CaughtUpState`, `NarrowsState`.
  */
 export type FetchingState = {
+  // TODO(flow-v0.126): Should be exact. See note in src/utils/jsonable.js.
   [narrow: string]: Fetching,
 };
 
@@ -154,8 +157,7 @@ export type FlagName = $Keys<FlagsState>;
  * messages belonging to a given narrow.
  */
 export type MessagesState = {
-  // MessagesState should be exact; we're waiting for Flow v0.126.0. See note
-  // in src/utils/jsonable.js.
+  // TODO(flow-v0.126): Should be exact. See note in src/utils/jsonable.js.
   [id: number]: $Exact<Message>,
 };