From b022e9c1d21041ced6880ed41c668fc70b793941 Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Fri, 23 Oct 2020 17:02:53 -0700
Subject: [PATCH 01/17] notif: Always sort user IDs in pm_users.

We already ensure this in the Android case (in FcmMessage.kt);
do so in the iOS case too, and document it in the type.

In practice the list should already have always been sorted: the
server sends it in that form, and has always done so since the
pm_users field was introduced in server commit 1.7.0-2360-g693a9a5e7.
(To see this in the history, try the following Git commands:
   git log -L :get_message_payload:zerver/lib/push_notifications.py
   git log -L :huddle_users:zerver/lib/message.py
.)  So the only way this could have gone wrong is if a rogue server
changed that behavior for some reason; and the main effect of this
commit is really just to document this invariant.
---
 src/notification/extract.js | 5 +++--
 src/notification/types.js   | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/notification/extract.js b/src/notification/extract.js
index 3f14165748f..1e598d123a0 100644
--- a/src/notification/extract.js
+++ b/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') {
diff --git a/src/notification/types.js b/src/notification/types.js
index a1f76331a99..9ac56660d97 100644
--- a/src/notification/types.js
+++ b/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 |};

From 7727e2999bde851fea268ebd602417d4656615ef Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Fri, 23 Oct 2020 17:12:44 -0700
Subject: [PATCH 02/17] narrow [nfc]: Document more details on identifying
 group PMs.

Which turned up a couple of bugs!  We'll fix those later in
this series.
---
 .../pmConversationsSelectors.js               |  4 ++
 src/utils/internalLinks.js                    |  3 ++
 src/utils/narrow.js                           | 46 +++++++++++++++++++
 src/utils/recipient.js                        | 21 +++++++++
 4 files changed, 74 insertions(+)

diff --git a/src/pm-conversations/pmConversationsSelectors.js b/src/pm-conversations/pmConversationsSelectors.js
index f8efbd8ba21..b293321b023 100644
--- a/src/pm-conversations/pmConversationsSelectors.js
+++ b/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,
     }));
 
diff --git a/src/utils/internalLinks.js b/src/utils/internalLinks.js
index 8d2c4c00f83..5800efe1122 100644
--- a/src/utils/internalLinks.js
+++ b/src/utils/internalLinks.js
@@ -138,6 +138,9 @@ export const getNarrowFromLink = (
       if (recipientEmails === null) {
         return null;
       }
+      // BUG: should normalize recipients; see comment on groupNarrow.
+      // (We're parsing a link someone wrote in a message, so the server
+      // gives us no guarantees here.)
       return groupNarrow(recipientEmails);
     }
     case 'topic':
diff --git a/src/utils/narrow.js b/src/utils/narrow.js
index 0ea5e25c45c..6fe751be5f8 100644
--- a/src/utils/narrow.js
+++ b/src/utils/narrow.js
@@ -21,6 +21,52 @@ export const privateNarrow = (email: string): Narrow => [
   },
 ];
 
+/**
+ * A group PM narrow.
+ *
+ * The users represented in `emails` should agree, as a (multi)set, with
+ * `pmKeyRecipientsFromMessage`.
+ *
+ * They might not have a consistent sorting.  (This would be good to fix.)
+ * Consumers of this data should sort for themselves when making comparisons.
+ */
+// Ideally, all callers should agree on how they're sorted, too.  Because
+// they don't, we have latent bugs (possibly a live one somewhere) where we
+// can wind up with several distinct narrows that are actually the same
+// group PM conversation.
+//
+// For example this happens if you have a group PM conversation where email
+// and ID sorting don't happen to coincide; visit a group PM conversation
+// from the main nav (either the unreads or PMs screen) -- which sorts by
+// email; and then visit the same conversation from a recipient bar on the
+// "all messages" narrow -- which sorts by ID.  The Redux logs in the
+// debugger will show two different entries in `state.narrows`.  This bug is
+// merely latent only because it doesn't (as far as we know) have any
+// user-visible effect.
+//
+// But we also have some callers that don't even ensure the set is the right
+// one, with the self-user properly there or not.  Known call stacks:
+//  * BUG #4293: getNarrowFromNotificationData: comes from notification's
+//      pm_users... which is sorted but not filtered.  This means if you
+//      follow a group PM notif, then get another message in that
+//      conversation, it won't appear.  (And if you send, it'll promptly
+//      disappear.)
+//  * BUG, ish: getNarrowFromLink doesn't ensure this precondition is met.
+//      And... there's basically a bug in the webapp, where the URL that
+//      appears in the location bar for a group PM conversation excludes
+//      self -- so it's unusable if you try to give someone else in it a
+//      link to a particular message, say.  But conversely I guess it means
+//      that the mobile app actually works just as well as the webapp on the
+//      links people generate from the webapp.
+//  * OK, perilously, unsorted: CreateGroupScreen: the self user isn't
+//      offered in the UI, so effectively the list is filtered; can call
+//      with just one email, but happily this works out the same as pmNarrow
+//  * OK, email: PmConversationList < PmConversationCard: the data comes
+//      from `getRecentConversations`, which filters and sorts by email
+//  * OK, email: PmConversationList < UnreadCards: ditto
+//  * OK, unsorted: getNarrowFromMessage
+//  * Good: messageHeaderAsHtml: comes from pmKeyRecipientsFromMessage,
+//      which filters and sorts by ID
 export const groupNarrow = (emails: string[]): Narrow => [
   {
     operator: 'pm-with',
diff --git a/src/utils/recipient.js b/src/utils/recipient.js
index b6f8ee456c2..ff9a3eacbc7 100644
--- a/src/utils/recipient.js
+++ b/src/utils/recipient.js
@@ -22,6 +22,14 @@ export const normalizeRecipients = (recipients: $ReadOnlyArray<{ email: string,
         .sort()
         .join(',');
 
+/**
+ * The same set 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,
@@ -38,6 +46,15 @@ export const normalizeRecipientsAsUserIds = (
     .sort()
     .join(',');
 
+/**
+ * The same set of users as pmKeyRecipientsFromMessage, in quirkier form.
+ *
+ * Sorted by user ID.
+ */
+// Note that sorting by user ID is the same as the server does for group PMs
+// (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: $ReadOnlyArray<{ user_id: number, ... }>,
   ownUserId: number,
@@ -87,6 +104,10 @@ export const pmUiRecipientsFromMessage = (
  *    It would be great to unify on a single version, as the variation is a
  *    possible source of bugs.
  */
+// The resulting users are sorted by user ID.  That's because:
+//  * For group PMs, the server provides them in that order; see comment
+//    on Message#display_recipient.
+//  * For 1:1 PMs, we only keep one user in the list.
 export const pmKeyRecipientsFromMessage = (
   message: Message | Outbox,
   ownUser: User,

From fbf0423a07a561e767a9424756d686e877af5bf9 Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Fri, 23 Oct 2020 15:56:55 -0700
Subject: [PATCH 03/17] example data: Take sender and recipients as pmMessage
 arguments.

As demonstrated, this allows callers to customize these a lot more
cleanly than they can by overriding the actual message properties
directly.

There are a few call sites we don't update here, in
narrowsReducer-test.js; that file hasn't yet been upgraded to be
well-typed, and so those call sites don't have real User objects
to provide.
---
 src/__tests__/lib/exampleData.js              | 14 ++-
 .../pmConversationsSelectors-test.js          | 94 +++----------------
 2 files changed, 22 insertions(+), 86 deletions(-)

diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js
index 5f2885600fb..ce6ae6e50fb 100644
--- a/src/__tests__/lib/exampleData.js
+++ b/src/__tests__/lib/exampleData.js
@@ -293,14 +293,22 @@ 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 = [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,
diff --git a/src/pm-conversations/__tests__/pmConversationsSelectors-test.js b/src/pm-conversations/__tests__/pmConversationsSelectors-test.js
index 0ca8a09fd42..e9ea2d9ef21 100644
--- a/src/pm-conversations/__tests__/pmConversationsSelectors-test.js
+++ b/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 }),

From 5fc08408bfd56b3fb33d2f915acdf2b1d4b5ed9f Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Mon, 2 Nov 2020 14:57:38 -0800
Subject: [PATCH 04/17] example data [nfc]: Use cleaner workaround for Flow
 "unsealed" issue.

We discovered this nicer one after having used the other one here.
Reminded of the contrast in discussion on other changes in this file:
  https://github.com/zulip/zulip-mobile/pull/4294#discussion_r516179857
---
 src/__tests__/lib/exampleData.js | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js
index ce6ae6e50fb..339e4319e3b 100644
--- a/src/__tests__/lib/exampleData.js
+++ b/src/__tests__/lib/exampleData.js
@@ -335,9 +335,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,

From 10ef7c0b1f5d1802b04ff578eb5fb7d04c5b98aa Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Wed, 28 Oct 2020 13:17:11 -0700
Subject: [PATCH 05/17] types: Make some more indexer-using object types
 inexact.

I just ran into this issue with CaughtUpState when making another
change.  Apply the workaround there and on the remaining example
in this file, and mark all instances with a conditional TODO.
---
 src/reduxTypes.js | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/src/reduxTypes.js b/src/reduxTypes.js
index 58f088ebbac..2710a95ce76 100644
--- a/src/reduxTypes.js
+++ b/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>,
 };
 

From 968acc352908c3b262962758f1625d40de11fedd Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Wed, 28 Oct 2020 13:21:03 -0700
Subject: [PATCH 06/17] narrowsReducer tests: Fix a tangled-up test case.

The data in this test case has never made much sense since 7a5da9587,
shortly after the test was added -- testing a PM with a sender not in
the display_recipient, which never happens.  With e09648c68 it grew
another kind of testing mistake which helped mask that problem: the
`caughtUp` state was false for the narrows we didn't expect the
message to get added to, so we didn't actually test whether the logic
believed the message belonged to them.

Then a bug in eg.pmMessage made the message differently malformed,
after 4077e8880 had this test start using that function; the message
would match all PM narrows, and the test kept passing only because of
that `caughtUp` masking.

We're about to fix that bug in eg.pmMessage.  That will cause this
test to fail (with the message matching no conversation narrows)
unless adjusted.  Just fix the whole test to make more sense.
---
 src/chat/__tests__/narrowsReducer-test.js | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/src/chat/__tests__/narrowsReducer-test.js b/src/chat/__tests__/narrowsReducer-test.js
index ec5768729c0..29717049f5e 100644
--- a/src/chat/__tests__/narrowsReducer-test.js
+++ b/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: 'mark@example.com' }],
+    });
 
     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 = {

From dfe8e12fcc7ee694a2004741a47d9bf6d8727f84 Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Tue, 27 Oct 2020 18:11:33 -0700
Subject: [PATCH 07/17] example data: Fix recipients in pmMessage defaults.

The display_recipient of a PM always contains all users involved,
including the self user.  See comment on the Message type.
---
 src/__tests__/lib/exampleData.js | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js
index 339e4319e3b..2c1fe5bad3d 100644
--- a/src/__tests__/lib/exampleData.js
+++ b/src/__tests__/lib/exampleData.js
@@ -300,7 +300,8 @@ export const pmMessage = (args?: {|
 |}): Message => {
   // The `Object.freeze` is to work around a Flow issue:
   //   https://github.com/facebook/flow/issues/2386#issuecomment-695064325
-  const { sender = otherUser, recipients = [selfUser], ...extra } = args ?? Object.freeze({});
+  const { sender = otherUser, recipients = [otherUser, selfUser], ...extra } =
+    args ?? Object.freeze({});
 
   const baseMessage: Message = {
     ...messagePropertiesBase,

From 6d9f4da020c863d0539d69050f503a6e06a032ad Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Thu, 22 Oct 2020 17:55:26 -0700
Subject: [PATCH 08/17] example data: Add `thirdUser`, to go with `otherUser`
 and `selfUser`.

We have a fair number of codepaths specific to group PM conversations
and messages.  When testing those, it's essential to have at least
three distinct users.  So this is a pretty widespread need.
---
 src/__tests__/lib/exampleData.js | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js
index 2c1fe5bad3d..00679e75174 100644
--- a/src/__tests__/lib/exampleData.js
+++ b/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' });
 

From e0d065280fe9e5a781eac0b7524e0b63af87d0d0 Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Thu, 22 Oct 2020 17:31:19 -0700
Subject: [PATCH 09/17] narrow tests: Make tests well-typed.

Mostly this involves using real Message objects instead of objects
with a handful of the same properties, and passing the full list of
arguments to some function calls where the tests were getting away
without the last one because the control path we're exercising
happens not to use it.

We also discard some test fragments that just check behavior on
ill-typed data that doesn't particularly correspond to any data we
believe our actual code produces, like `[{}, {}]` to the isFooNarrow
functions or `undefined` to `isSameNarrow`.

There's a lot more that could be done to simplify these tests and
further improve them, but this gives us a basis for any refactoring.
---
 src/utils/__tests__/narrow-test.js | 184 ++++++++++++-----------------
 1 file changed, 76 insertions(+), 108 deletions(-)

diff --git a/src/utils/__tests__/narrow-test.js b/src/utils/__tests__/narrow-test.js
index b3ba527b9a8..909e72e628e 100644
--- a/src/utils/__tests__/narrow-test.js
+++ b/src/utils/__tests__/narrow-test.js
@@ -1,3 +1,5 @@
+/* @flow strict-local */
+
 import {
   HOME_NARROW,
   isHomeNarrow,
@@ -24,6 +26,8 @@ import {
   MENTIONED_NARROW,
 } from '../narrow';
 
+import * as eg from '../../__tests__/lib/exampleData';
+
 describe('HOME_NARROW', () => {
   test('produces an empty list', () => {
     expect(HOME_NARROW).toEqual([]);
@@ -45,8 +49,7 @@ describe('privateNarrow', () => {
   });
 
   test('if operator is "pm-with" and only one email, then it is a private narrow', () => {
-    expect(isPrivateNarrow([])).toBe(false);
-    expect(isPrivateNarrow([{}, {}])).toBe(false);
+    expect(isPrivateNarrow(HOME_NARROW)).toBe(false);
     expect(isPrivateNarrow(privateNarrow('bob@example.com'))).toBe(true);
     expect(
       isPrivateNarrow([
@@ -70,8 +73,7 @@ describe('groupNarrow', () => {
   });
 
   test('a group narrow is only private chat with more than one recipients', () => {
-    expect(isGroupNarrow([])).toBe(false);
-    expect(isGroupNarrow([{}, {}])).toBe(false);
+    expect(isGroupNarrow(HOME_NARROW)).toBe(false);
     expect(isGroupNarrow(privateNarrow('bob@example.com'))).toBe(false);
     expect(isGroupNarrow(groupNarrow(['bob@example.com', 'john@example.com']))).toBe(true);
     expect(
@@ -96,8 +98,7 @@ describe('groupNarrow', () => {
 describe('isPrivateOrGroupNarrow', () => {
   test('a private or group narrow is any "pm-with" narrow', () => {
     expect(isPrivateOrGroupNarrow(undefined)).toBe(false);
-    expect(isPrivateOrGroupNarrow([])).toBe(false);
-    expect(isPrivateOrGroupNarrow([{}, {}])).toBe(false);
+    expect(isPrivateOrGroupNarrow(HOME_NARROW)).toBe(false);
     expect(isPrivateOrGroupNarrow(privateNarrow('bob@example.com'))).toBe(true);
     expect(isPrivateOrGroupNarrow(groupNarrow(['bob@example.com', 'john@example.com']))).toBe(true);
     expect(
@@ -145,8 +146,7 @@ describe('specialNarrow', () => {
 
   test('only narrowing with the "is" operator is special narrow', () => {
     expect(isSpecialNarrow(undefined)).toBe(false);
-    expect(isSpecialNarrow([])).toBe(false);
-    expect(isSearchNarrow([{}, {}])).toBe(false);
+    expect(isSpecialNarrow(HOME_NARROW)).toBe(false);
     expect(isSpecialNarrow(streamNarrow('some stream'))).toBe(false);
     expect(isSpecialNarrow(STARRED_NARROW)).toBe(true);
     expect(isSpecialNarrow([{ operator: 'stream', operand: 'some stream' }])).toBe(false);
@@ -166,8 +166,7 @@ describe('streamNarrow', () => {
 
   test('only narrow with operator of "stream" is a stream narrow', () => {
     expect(isStreamNarrow(undefined)).toBe(false);
-    expect(isStreamNarrow([])).toBe(false);
-    expect(isSearchNarrow([{}, {}])).toBe(false);
+    expect(isStreamNarrow(HOME_NARROW)).toBe(false);
     expect(isStreamNarrow(streamNarrow('some stream'))).toBe(true);
     expect(isStreamNarrow([{ operator: 'stream', operand: 'some stream' }])).toBe(true);
   });
@@ -183,7 +182,7 @@ describe('topicNarrow', () => {
 
   test('only narrow with two items, one for stream, one for topic is a topic narrow', () => {
     expect(isTopicNarrow(undefined)).toBe(false);
-    expect(isTopicNarrow([])).toBe(false);
+    expect(isTopicNarrow(HOME_NARROW)).toBe(false);
     expect(isTopicNarrow(topicNarrow('some stream', 'some topic'))).toBe(true);
     expect(
       isTopicNarrow([
@@ -212,142 +211,119 @@ describe('SEARCH_NARROW', () => {
 
   test('narrow with "search" operand is a search narrow', () => {
     expect(isSearchNarrow(undefined)).toBe(false);
-    expect(isSearchNarrow([])).toBe(false);
-    expect(isSearchNarrow([{}, {}])).toBe(false);
-    expect(isSearchNarrow([{ operator: 'search' }])).toBe(true);
+    expect(isSearchNarrow(HOME_NARROW)).toBe(false);
+    expect(isSearchNarrow(SEARCH_NARROW('some query'))).toBe(true);
   });
 });
 
 describe('isMessageInNarrow', () => {
+  const ownEmail = eg.selfUser.email;
+
   test('any message is in "Home"', () => {
-    const message = {
-      flags: [],
-    };
+    const message = eg.streamMessage({ flags: [] });
     const narrow = HOME_NARROW;
-    expect(isMessageInNarrow(message, narrow)).toBe(true);
+    expect(isMessageInNarrow(message, narrow, ownEmail)).toBe(true);
   });
 
   test('message with type "private" is in private narrow if recipient matches', () => {
-    const message = {
-      flags: [],
-      type: 'private',
-      display_recipient: [{ email: 'me@example.com' }, { email: 'john@example.com' }],
-    };
-    const narrow = privateNarrow('john@example.com');
-
-    expect(isMessageInNarrow(message, narrow, 'me@example.com')).toBe(true);
+    const message = eg.pmMessage({ flags: [] });
+    const narrow = privateNarrow(eg.otherUser.email);
+    expect(isMessageInNarrow(message, narrow, ownEmail)).toBe(true);
   });
 
   test('message to self is in "private" narrow with self', () => {
-    const message = {
+    const message = eg.pmMessage({
       flags: [],
-      type: 'private',
-      display_recipient: [{ email: 'me@example.com' }],
-    };
-    const narrow = privateNarrow('me@example.com');
-
-    expect(isMessageInNarrow(message, narrow, 'me@example.com')).toBe(true);
+      display_recipient: [eg.displayRecipientFromUser(eg.selfUser)],
+    });
+    const narrow = privateNarrow(eg.selfUser.email);
+    expect(isMessageInNarrow(message, narrow, ownEmail)).toBe(true);
   });
 
   test('message with type "private" is in group narrow if all recipients match ', () => {
-    const message = {
-      type: 'private',
+    const message = eg.pmMessage({
       flags: [],
-      display_recipient: [
-        { email: 'me@example.com' },
-        { email: 'john@example.com' },
-        { email: 'mark@example.com' },
-      ],
-    };
-    const ownEmail = 'me@example.com';
-    const narrow = groupNarrow(['john@example.com', 'mark@example.com']);
-
+      display_recipient: [eg.selfUser, eg.otherUser, eg.thirdUser].map(eg.displayRecipientFromUser),
+    });
+    const narrow = groupNarrow([eg.otherUser.email, eg.thirdUser.email]);
     expect(isMessageInNarrow(message, narrow, ownEmail)).toBe(true);
   });
 
   test('message with type "private" is always in "private messages" narrow', () => {
-    const message = {
+    const message = eg.pmMessage({
       flags: [],
-      type: 'private',
-      display_recipient: [{ email: 'me@example.com' }, { email: 'john@example.com' }],
-    };
-    expect(isMessageInNarrow(message, ALL_PRIVATE_NARROW)).toBe(true);
+      display_recipient: [eg.selfUser, eg.otherUser].map(eg.displayRecipientFromUser),
+    });
+    expect(isMessageInNarrow(message, ALL_PRIVATE_NARROW, ownEmail)).toBe(true);
   });
 
   test('message with type "stream" is in narrow if recipient and current stream match', () => {
-    const message = {
+    const message = eg.streamMessage({
       flags: [],
-      type: 'stream',
-      display_recipient: 'some stream',
-    };
-    const narrow = streamNarrow('some stream');
-
-    expect(isMessageInNarrow(message, narrow)).toBe(true);
+    });
+    const narrow = streamNarrow(eg.stream.name);
+    expect(isMessageInNarrow(message, narrow, ownEmail)).toBe(true);
   });
 
-  test('message with flags undefined throws an error', () => {
-    const message = {
-      // no flags key
-    };
-    expect(() => isMessageInNarrow(message, MENTIONED_NARROW)).toThrow();
+  test('message with flags absent throws an error', () => {
+    const message = eg.streamMessage({
+      // no flags
+    });
+    expect(() => isMessageInNarrow(message, MENTIONED_NARROW, ownEmail)).toThrow();
   });
 
   test('message with flag "mentioned" is in is:mentioned narrow', () => {
-    const message = {
+    const message = eg.streamMessage({
       flags: ['mentioned'],
-    };
-    expect(isMessageInNarrow(message, MENTIONED_NARROW)).toBe(true);
+    });
+    expect(isMessageInNarrow(message, MENTIONED_NARROW, ownEmail)).toBe(true);
   });
 
   test('message with flag "wildcard_mentioned" is in is:mentioned narrow', () => {
-    const message = {
+    const message = eg.streamMessage({
       flags: ['wildcard_mentioned'],
-    };
-    expect(isMessageInNarrow(message, MENTIONED_NARROW)).toBe(true);
+    });
+    expect(isMessageInNarrow(message, MENTIONED_NARROW, ownEmail)).toBe(true);
   });
 
   test('message without flag "mentioned" or "wildcard_mentioned" is not in is:mentioned narrow', () => {
-    const message = {
+    const message = eg.streamMessage({
       flags: [],
-    };
-    expect(isMessageInNarrow(message, MENTIONED_NARROW)).toBe(false);
+    });
+    expect(isMessageInNarrow(message, MENTIONED_NARROW, ownEmail)).toBe(false);
   });
 
   test('message with flag "starred" is in is:starred narrow', () => {
-    const message = {
+    const message = eg.streamMessage({
       flags: ['starred'],
-    };
-    expect(isMessageInNarrow(message, STARRED_NARROW)).toBe(true);
+    });
+    expect(isMessageInNarrow(message, STARRED_NARROW, ownEmail)).toBe(true);
   });
 
   test('message without flag "starred" is not in is:starred narrow', () => {
-    const message = {
+    const message = eg.streamMessage({
       flags: [],
-    };
-    expect(isMessageInNarrow(message, STARRED_NARROW)).toBe(false);
+    });
+    expect(isMessageInNarrow(message, STARRED_NARROW, ownEmail)).toBe(false);
   });
 
   test('message with type stream is in topic narrow if current stream and topic match with its own', () => {
-    const message = {
-      type: 'stream',
-      subject: 'some topic',
-      display_recipient: 'some stream',
+    const message = eg.streamMessage({
       flags: [],
-    };
-    const narrow = topicNarrow('some stream', 'some topic');
-
-    expect(isMessageInNarrow(message, narrow)).toBe(true);
+    });
+    const narrow = topicNarrow(eg.stream.name, message.subject);
+    expect(isMessageInNarrow(message, narrow, ownEmail)).toBe(true);
   });
 });
 
 describe('getNarrowFromMessage', () => {
-  test('message with single recipient, returns a private narrow', () => {
-    const message = {
-      display_recipient: [{ email: 'bob@example.com' }],
-    };
-    const ownEmail = 'hamlet@zulip.com';
+  const ownEmail = eg.selfUser.email;
 
-    const expectedNarrow = privateNarrow('bob@example.com');
+  test('message with single recipient, returns a private narrow', () => {
+    const message = eg.pmMessage({
+      display_recipient: [eg.displayRecipientFromUser(eg.otherUser)],
+    });
+    const expectedNarrow = privateNarrow(eg.otherUser.email);
 
     const actualNarrow = getNarrowFromMessage(message, ownEmail);
 
@@ -355,11 +331,10 @@ describe('getNarrowFromMessage', () => {
   });
 
   test('for message with multiple recipients, return a group narrow', () => {
-    const message = {
-      display_recipient: [{ email: 'bob@example.com' }, { email: 'john@example.com' }],
-    };
-    const ownEmail = 'hamlet@zulip.com';
-    const expectedNarrow = groupNarrow(['bob@example.com', 'john@example.com']);
+    const message = eg.pmMessage({
+      display_recipient: [eg.otherUser, eg.thirdUser].map(eg.displayRecipientFromUser),
+    });
+    const expectedNarrow = groupNarrow([eg.otherUser.email, eg.thirdUser.email]);
 
     const actualNarrow = getNarrowFromMessage(message, ownEmail);
 
@@ -367,24 +342,19 @@ describe('getNarrowFromMessage', () => {
   });
 
   test('if recipient of a message is string, returns a stream narrow', () => {
-    const message = {
-      display_recipient: 'stream',
-    };
-    const expectedNarrow = streamNarrow('stream');
+    const message = eg.streamMessage({ subject: '' });
+    const expectedNarrow = streamNarrow(eg.stream.name);
 
-    const actualNarrow = getNarrowFromMessage(message);
+    const actualNarrow = getNarrowFromMessage(message, ownEmail);
 
     expect(actualNarrow).toEqual(expectedNarrow);
   });
 
   test('if recipient is a string and there is a subject returns a topic narrow', () => {
-    const message = {
-      display_recipient: 'stream',
-      subject: 'subject',
-    };
-    const expectedNarrow = topicNarrow('stream', 'subject');
+    const message = eg.streamMessage();
+    const expectedNarrow = topicNarrow(eg.stream.name, message.subject);
 
-    const actualNarrow = getNarrowFromMessage(message);
+    const actualNarrow = getNarrowFromMessage(message, ownEmail);
 
     expect(actualNarrow).toEqual(expectedNarrow);
   });
@@ -392,8 +362,6 @@ describe('getNarrowFromMessage', () => {
 
 describe('isSameNarrow', () => {
   test('Return true if two narrows are same', () => {
-    expect(isSameNarrow(undefined, undefined)).toBe(false);
-    expect(isSameNarrow(streamNarrow('stream'), undefined)).toBe(false);
     expect(isSameNarrow(streamNarrow('stream'), streamNarrow('stream'))).toBe(true);
     expect(isSameNarrow(streamNarrow('stream'), streamNarrow('stream1'))).toBe(false);
     expect(isSameNarrow(streamNarrow('stream'), topicNarrow('stream', 'topic'))).toBe(false);

From fe6f8866e57e2db9c6eb4f6016367da8ab0f4193 Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Fri, 23 Oct 2020 14:51:28 -0700
Subject: [PATCH 10/17] narrow tests [nfc]: Make isMessageInNarrow tests
 table-driven.

This will make it a lot easier to see what cases aren't covered,
and to add more cases.
---
 src/utils/__tests__/narrow-test.js | 132 ++++++++++-------------------
 1 file changed, 45 insertions(+), 87 deletions(-)

diff --git a/src/utils/__tests__/narrow-test.js b/src/utils/__tests__/narrow-test.js
index 909e72e628e..01497eb6a33 100644
--- a/src/utils/__tests__/narrow-test.js
+++ b/src/utils/__tests__/narrow-test.js
@@ -219,51 +219,52 @@ describe('SEARCH_NARROW', () => {
 describe('isMessageInNarrow', () => {
   const ownEmail = eg.selfUser.email;
 
-  test('any message is in "Home"', () => {
-    const message = eg.streamMessage({ flags: [] });
-    const narrow = HOME_NARROW;
-    expect(isMessageInNarrow(message, narrow, ownEmail)).toBe(true);
-  });
-
-  test('message with type "private" is in private narrow if recipient matches', () => {
-    const message = eg.pmMessage({ flags: [] });
-    const narrow = privateNarrow(eg.otherUser.email);
-    expect(isMessageInNarrow(message, narrow, ownEmail)).toBe(true);
-  });
-
-  test('message to self is in "private" narrow with self', () => {
-    const message = eg.pmMessage({
-      flags: [],
-      display_recipient: [eg.displayRecipientFromUser(eg.selfUser)],
+  // prettier-ignore
+  for (const [narrowDescription, narrow, cases] of [
+    ['all-messages ("home") narrow', HOME_NARROW, [
+      ['a message', true, eg.streamMessage()],
+    ]],
+
+    ['whole-stream narrow', streamNarrow(eg.stream.name), [
+      ['matching stream message', true, eg.streamMessage()],
+    ]],
+    ['stream conversation', topicNarrow(eg.stream.name, 'cabbages'), [
+      ['matching message', true, eg.streamMessage({ subject: 'cabbages' })],
+    ]],
+
+    ['1:1 PM conversation, non-self', privateNarrow(eg.otherUser.email), [
+      ['matching PM', true, eg.pmMessage()],
+    ]],
+    ['self-1:1 conversation', privateNarrow(eg.selfUser.email), [
+      ['self-1:1 message', true, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser] })],
+    ]],
+    ['group-PM conversation', groupNarrow([eg.otherUser.email, eg.thirdUser.email]), [
+      ['matching group-PM', true, eg.pmMessage({ recipients: [eg.selfUser, eg.otherUser, eg.thirdUser] })],
+    ]],
+    ['all-PMs narrow', ALL_PRIVATE_NARROW, [
+      ['a PM', true, eg.pmMessage()],
+    ]],
+
+    ['is:mentioned', MENTIONED_NARROW, [
+      ['w/ mentioned flag', true, eg.streamMessage({ flags: ['mentioned'] })],
+      ['w/ wildcard_mentioned flag', true, eg.streamMessage({ flags: ['wildcard_mentioned'] })],
+      ['w/o flags', false, eg.streamMessage()],
+    ]],
+    ['is:starred', STARRED_NARROW, [
+      ['w/ starred flag', true, eg.streamMessage({ flags: ['starred'] })],
+      ['w/o flags', false, eg.streamMessage()],
+    ]],
+  ]) {
+    describe(narrowDescription, () => {
+      for (const [messageDescription, expected, message] of cases) {
+        test(`${expected ? 'contains' : 'excludes'} ${messageDescription}`, () => {
+          expect(
+            isMessageInNarrow({ ...message, flags: message.flags ?? [] }, narrow, ownEmail),
+          ).toBe(expected);
+        });
+      }
     });
-    const narrow = privateNarrow(eg.selfUser.email);
-    expect(isMessageInNarrow(message, narrow, ownEmail)).toBe(true);
-  });
-
-  test('message with type "private" is in group narrow if all recipients match ', () => {
-    const message = eg.pmMessage({
-      flags: [],
-      display_recipient: [eg.selfUser, eg.otherUser, eg.thirdUser].map(eg.displayRecipientFromUser),
-    });
-    const narrow = groupNarrow([eg.otherUser.email, eg.thirdUser.email]);
-    expect(isMessageInNarrow(message, narrow, ownEmail)).toBe(true);
-  });
-
-  test('message with type "private" is always in "private messages" narrow', () => {
-    const message = eg.pmMessage({
-      flags: [],
-      display_recipient: [eg.selfUser, eg.otherUser].map(eg.displayRecipientFromUser),
-    });
-    expect(isMessageInNarrow(message, ALL_PRIVATE_NARROW, ownEmail)).toBe(true);
-  });
-
-  test('message with type "stream" is in narrow if recipient and current stream match', () => {
-    const message = eg.streamMessage({
-      flags: [],
-    });
-    const narrow = streamNarrow(eg.stream.name);
-    expect(isMessageInNarrow(message, narrow, ownEmail)).toBe(true);
-  });
+  }
 
   test('message with flags absent throws an error', () => {
     const message = eg.streamMessage({
@@ -271,49 +272,6 @@ describe('isMessageInNarrow', () => {
     });
     expect(() => isMessageInNarrow(message, MENTIONED_NARROW, ownEmail)).toThrow();
   });
-
-  test('message with flag "mentioned" is in is:mentioned narrow', () => {
-    const message = eg.streamMessage({
-      flags: ['mentioned'],
-    });
-    expect(isMessageInNarrow(message, MENTIONED_NARROW, ownEmail)).toBe(true);
-  });
-
-  test('message with flag "wildcard_mentioned" is in is:mentioned narrow', () => {
-    const message = eg.streamMessage({
-      flags: ['wildcard_mentioned'],
-    });
-    expect(isMessageInNarrow(message, MENTIONED_NARROW, ownEmail)).toBe(true);
-  });
-
-  test('message without flag "mentioned" or "wildcard_mentioned" is not in is:mentioned narrow', () => {
-    const message = eg.streamMessage({
-      flags: [],
-    });
-    expect(isMessageInNarrow(message, MENTIONED_NARROW, ownEmail)).toBe(false);
-  });
-
-  test('message with flag "starred" is in is:starred narrow', () => {
-    const message = eg.streamMessage({
-      flags: ['starred'],
-    });
-    expect(isMessageInNarrow(message, STARRED_NARROW, ownEmail)).toBe(true);
-  });
-
-  test('message without flag "starred" is not in is:starred narrow', () => {
-    const message = eg.streamMessage({
-      flags: [],
-    });
-    expect(isMessageInNarrow(message, STARRED_NARROW, ownEmail)).toBe(false);
-  });
-
-  test('message with type stream is in topic narrow if current stream and topic match with its own', () => {
-    const message = eg.streamMessage({
-      flags: [],
-    });
-    const narrow = topicNarrow(eg.stream.name, message.subject);
-    expect(isMessageInNarrow(message, narrow, ownEmail)).toBe(true);
-  });
 });
 
 describe('getNarrowFromMessage', () => {

From 891db43792ef0c7d0ac69edd0159bef16e314a71 Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Fri, 23 Oct 2020 15:48:07 -0700
Subject: [PATCH 11/17] narrow tests: Add more test cases for
 isMessageInNarrow.

Including two failing tests!  Commented out.  We'll fix the bug
those uncovered shortly.
---
 src/utils/__tests__/narrow-test.js | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/src/utils/__tests__/narrow-test.js b/src/utils/__tests__/narrow-test.js
index 01497eb6a33..850bee619c6 100644
--- a/src/utils/__tests__/narrow-test.js
+++ b/src/utils/__tests__/narrow-test.js
@@ -218,6 +218,7 @@ describe('SEARCH_NARROW', () => {
 
 describe('isMessageInNarrow', () => {
   const ownEmail = eg.selfUser.email;
+  const otherStream = eg.makeStream();
 
   // prettier-ignore
   for (const [narrowDescription, narrow, cases] of [
@@ -227,22 +228,43 @@ describe('isMessageInNarrow', () => {
 
     ['whole-stream narrow', streamNarrow(eg.stream.name), [
       ['matching stream message', true, eg.streamMessage()],
+      ['other-stream message', false, eg.streamMessage({ stream: otherStream })],
+      ['PM', false, eg.pmMessage()],
     ]],
     ['stream conversation', topicNarrow(eg.stream.name, 'cabbages'), [
       ['matching message', true, eg.streamMessage({ subject: 'cabbages' })],
+      ['message in same stream but other topic', false, eg.streamMessage({ subject: 'kings' })],
+      ['other-stream message', false, eg.streamMessage({ stream: otherStream })],
+      ['PM', false, eg.pmMessage()],
     ]],
 
     ['1:1 PM conversation, non-self', privateNarrow(eg.otherUser.email), [
-      ['matching PM', true, eg.pmMessage()],
+      ['matching PM, inbound', true, eg.pmMessage()],
+      ['matching PM, outbound', true, eg.pmMessage({ sender: eg.selfUser })],
+      // FAILING: ['self-1:1 message', false, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser] })],
+      ['group-PM including this user, inbound', false, eg.pmMessage({ recipients: [eg.selfUser, eg.otherUser, eg.thirdUser] })],
+      ['group-PM including this user, outbound', false, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser, eg.otherUser, eg.thirdUser] })],
+      ['stream message', false, eg.streamMessage()],
     ]],
     ['self-1:1 conversation', privateNarrow(eg.selfUser.email), [
       ['self-1:1 message', true, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser] })],
+      ['other 1:1 message, inbound', false, eg.pmMessage()],
+      ['other 1:1 message, outbound', false, eg.pmMessage({ sender: eg.selfUser })],
+      ['group-PM, inbound', false, eg.pmMessage({ recipients: [eg.selfUser, eg.otherUser, eg.thirdUser] })],
+      ['group-PM, outbound', false, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser, eg.otherUser, eg.thirdUser] })],
+      ['stream message', false, eg.streamMessage()],
     ]],
     ['group-PM conversation', groupNarrow([eg.otherUser.email, eg.thirdUser.email]), [
-      ['matching group-PM', true, eg.pmMessage({ recipients: [eg.selfUser, eg.otherUser, eg.thirdUser] })],
+      ['matching group-PM, inbound', true, eg.pmMessage({ recipients: [eg.selfUser, eg.otherUser, eg.thirdUser] })],
+      ['matching group-PM, outbound', true, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser, eg.otherUser, eg.thirdUser] })],
+      ['1:1 within group, inbound', false, eg.pmMessage()],
+      ['1:1 within group, outbound', false, eg.pmMessage({ sender: eg.selfUser })],
+      // FAILING: ['self-1:1 message', false, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser] })],
+      ['stream message', false, eg.streamMessage()],
     ]],
     ['all-PMs narrow', ALL_PRIVATE_NARROW, [
       ['a PM', true, eg.pmMessage()],
+      ['stream message', false, eg.streamMessage()],
     ]],
 
     ['is:mentioned', MENTIONED_NARROW, [

From 5b8d26acb835cc0caaa2170b257e070aad7ec382 Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Tue, 27 Oct 2020 19:37:33 -0700
Subject: [PATCH 12/17] narrow: Fix bug where self-1:1 "is in" every PM
 conversation.

We had a hilarious bug: if you've already visited some group PM
conversation (so that we're following it in `state.narrows`), and
then you send yourself a 1:1 PM -- either from some other client,
or from this same client -- then the 1:1 will appear to show up in
that group conversation.

This doesn't affect what messages exist on the server, or what
messages anyone else sees.  And it can only happen with a message you
yourself sent, because it's limited to self-PMs.  Still -- it could
potentially *look* to you like something quite awkward had happened!
Which would, itself, be awkward.

The root of the bug is a funny `normalizedRecipients === ownEmail`
case in the `isMessageInNarrow` function.  That, in turn, seems to be
there (it was added in d8faae9d7, which introduced this bug) to try to
correct for where we add `ownEmail` to the list of emails on the line
before... which in turn is to try to correct for how in many places we
leave out the self-user from describing certain PM narrows.

We can fix the bug by consistently using that latter normalization,
quirky though it is, within that function.  The email list
representing a group PM narrow should already be normalized in that
way, leaving out the self user; and the old code here was already
relying on that.

Fixes: #3324
---
 src/utils/__tests__/narrow-test.js |  4 ++--
 src/utils/narrow.js                | 18 ++++++++++--------
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/src/utils/__tests__/narrow-test.js b/src/utils/__tests__/narrow-test.js
index 850bee619c6..ff1013486d6 100644
--- a/src/utils/__tests__/narrow-test.js
+++ b/src/utils/__tests__/narrow-test.js
@@ -241,7 +241,7 @@ describe('isMessageInNarrow', () => {
     ['1:1 PM conversation, non-self', privateNarrow(eg.otherUser.email), [
       ['matching PM, inbound', true, eg.pmMessage()],
       ['matching PM, outbound', true, eg.pmMessage({ sender: eg.selfUser })],
-      // FAILING: ['self-1:1 message', false, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser] })],
+      ['self-1:1 message', false, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser] })],
       ['group-PM including this user, inbound', false, eg.pmMessage({ recipients: [eg.selfUser, eg.otherUser, eg.thirdUser] })],
       ['group-PM including this user, outbound', false, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser, eg.otherUser, eg.thirdUser] })],
       ['stream message', false, eg.streamMessage()],
@@ -259,7 +259,7 @@ describe('isMessageInNarrow', () => {
       ['matching group-PM, outbound', true, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser, eg.otherUser, eg.thirdUser] })],
       ['1:1 within group, inbound', false, eg.pmMessage()],
       ['1:1 within group, outbound', false, eg.pmMessage({ sender: eg.selfUser })],
-      // FAILING: ['self-1:1 message', false, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser] })],
+      ['self-1:1 message', false, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser] })],
       ['stream message', false, eg.streamMessage()],
     ]],
     ['all-PMs narrow', ALL_PRIVATE_NARROW, [
diff --git a/src/utils/narrow.js b/src/utils/narrow.js
index 6fe751be5f8..5f035f7e5bc 100644
--- a/src/utils/narrow.js
+++ b/src/utils/narrow.js
@@ -2,8 +2,8 @@
 import isEqual from 'lodash.isequal';
 import unescape from 'lodash.unescape';
 
-import type { Narrow, Message, Outbox } from '../types';
-import { normalizeRecipients } from './recipient';
+import type { Narrow, Message, Outbox, PmRecipientUser } from '../types';
+import { normalizeRecipientsSansMe } from './recipient';
 
 export const isSameNarrow = (narrow1: Narrow, narrow2: Narrow): boolean =>
   Array.isArray(narrow1) && Array.isArray(narrow2) && isEqual(narrow1, narrow2);
@@ -256,10 +256,12 @@ export const isSearchNarrow = (narrow?: Narrow): boolean =>
 
 /** (For search narrows, just returns false.) */
 export const isMessageInNarrow = (message: Message, narrow: Narrow, ownEmail: string): boolean => {
-  const matchRecipients = (emails: string[]) => {
-    const normalizedRecipients = normalizeRecipients(message.display_recipient);
-    const normalizedNarrow = [...emails, ownEmail].sort().join(',');
-    return normalizedRecipients === ownEmail || normalizedRecipients === normalizedNarrow;
+  const matchPmRecipients = (emails: string[]) => {
+    if (message.type !== 'private') {
+      return false;
+    }
+    const recipients: PmRecipientUser[] = message.display_recipient;
+    return normalizeRecipientsSansMe(recipients, ownEmail) === emails.sort().join(',');
   };
 
   const { flags } = message;
@@ -272,8 +274,8 @@ export const isMessageInNarrow = (message: Message, narrow: Narrow, ownEmail: st
     stream: name => name === message.display_recipient,
     topic: (streamName, topic) =>
       streamName === message.display_recipient && topic === message.subject,
-    pm: email => matchRecipients([email]),
-    groupPm: matchRecipients,
+    pm: email => matchPmRecipients([email]),
+    groupPm: matchPmRecipients,
     starred: () => flags.includes('starred'),
     mentioned: () => flags.includes('mentioned') || flags.includes('wildcard_mentioned'),
     allPrivate: () => message.type === 'private',

From e99b60f8f9c44e5fd3c6436aa32a10327165a663 Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Thu, 29 Oct 2020 15:18:30 -0700
Subject: [PATCH 13/17] narrow: Re-normalize group PM user-set in
 isMessageInNarrow.

In most places in the app, we represent a group PM conversation by
just the users in it *other* than self.  This aligns with how the
webapp represents the narrow in the URL: the fragment may look like
`#narrow/pm-with/101,104-group` for a conversation with a total of
3 users in it.  In particular, most of our code consistently
generates group-narrow values following this convention, and in some
places our code relies on that assumption.  But we don't always
fulfill it.

One place we don't is when you open a notification for a group PM:
the server represents the conversation there with the full list of
users, and we don't normalize it to the other convention.  Then one
place we rely on the assumption is here in `isMessageInNarrow`...
which we use to decide whether a new message should be shown in the
conversation.  The result is #4293, that the narrow doesn't see any
replies you send or any further messages that arrive.

The ultimate fix here is an overhaul of how we represent narrows,
to something more structured, with more explicit, checkable,
checked invariants.  Pending that, we'll fix the issue at hand at
both ends -- either one suffices to fix the issue, and each one
potentially fixes other, unknown issues.  In this commit, we fix
the consumer of the data, to no longer rely on this assumption.

Fixes: #4293
---
 src/utils/__tests__/narrow-test.js |  8 ++++++++
 src/utils/narrow.js                | 16 +++++++++-------
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/src/utils/__tests__/narrow-test.js b/src/utils/__tests__/narrow-test.js
index ff1013486d6..d9f54001801 100644
--- a/src/utils/__tests__/narrow-test.js
+++ b/src/utils/__tests__/narrow-test.js
@@ -262,6 +262,14 @@ describe('isMessageInNarrow', () => {
       ['self-1:1 message', false, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser] })],
       ['stream message', false, eg.streamMessage()],
     ]],
+    ['group-PM conversation, including self', groupNarrow([eg.selfUser.email, eg.otherUser.email, eg.thirdUser.email]), [
+      ['matching group-PM, inbound', true, eg.pmMessage({ recipients: [eg.selfUser, eg.otherUser, eg.thirdUser] })],
+      ['matching group-PM, outbound', true, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser, eg.otherUser, eg.thirdUser] })],
+      ['1:1 within group, inbound', false, eg.pmMessage()],
+      ['1:1 within group, outbound', false, eg.pmMessage({ sender: eg.selfUser })],
+      ['self-1:1 message', false, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser] })],
+      ['stream message', false, eg.streamMessage()],
+    ]],
     ['all-PMs narrow', ALL_PRIVATE_NARROW, [
       ['a PM', true, eg.pmMessage()],
       ['stream message', false, eg.streamMessage()],
diff --git a/src/utils/narrow.js b/src/utils/narrow.js
index 5f035f7e5bc..1216aeb6ffd 100644
--- a/src/utils/narrow.js
+++ b/src/utils/narrow.js
@@ -25,7 +25,8 @@ export const privateNarrow = (email: string): Narrow => [
  * A group PM narrow.
  *
  * The users represented in `emails` should agree, as a (multi)set, with
- * `pmKeyRecipientsFromMessage`.
+ * `pmKeyRecipientsFromMessage`.  But this isn't checked, and there may be
+ * bugs where they don't; some consumers of this data re-normalize to be sure.
  *
  * They might not have a consistent sorting.  (This would be good to fix.)
  * Consumers of this data should sort for themselves when making comparisons.
@@ -46,11 +47,8 @@ export const privateNarrow = (email: string): Narrow => [
 //
 // But we also have some callers that don't even ensure the set is the right
 // one, with the self-user properly there or not.  Known call stacks:
-//  * BUG #4293: getNarrowFromNotificationData: comes from notification's
-//      pm_users... which is sorted but not filtered.  This means if you
-//      follow a group PM notif, then get another message in that
-//      conversation, it won't appear.  (And if you send, it'll promptly
-//      disappear.)
+//  * BUG, at least latent: getNarrowFromNotificationData: comes from
+//      notification's pm_users... which is sorted but not filtered.
 //  * BUG, ish: getNarrowFromLink doesn't ensure this precondition is met.
 //      And... there's basically a bug in the webapp, where the URL that
 //      appears in the location bar for a group PM conversation excludes
@@ -261,7 +259,11 @@ export const isMessageInNarrow = (message: Message, narrow: Narrow, ownEmail: st
       return false;
     }
     const recipients: PmRecipientUser[] = message.display_recipient;
-    return normalizeRecipientsSansMe(recipients, ownEmail) === emails.sort().join(',');
+    const narrowAsRecipients = emails.map(email => ({ email }));
+    return (
+      normalizeRecipientsSansMe(recipients, ownEmail)
+      === normalizeRecipientsSansMe(narrowAsRecipients, ownEmail)
+    );
   };
 
   const { flags } = message;

From f95ea039ea0c5a60ecddc5d7e143b9b6e3ac68f3 Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Thu, 29 Oct 2020 15:34:52 -0700
Subject: [PATCH 14/17] notif: Normalize set of users to represent a group PM
 thread.

This fixes #4293 from the other direction as we did in the previous
commit.  As a result we avoid the at-least-latent bug where we can
end up with duplicate narrows in our Redux state; and we also avoid
any unknown other bug where, like with #4293, we're relying on the
set of users having been filtered this way.
---
 src/notification/__tests__/notification-test.js | 15 ++++++++-------
 src/notification/index.js                       |  5 +++++
 src/notification/notificationActions.js         |  4 ++--
 src/utils/narrow.js                             |  4 ++--
 4 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/src/notification/__tests__/notification-test.js b/src/notification/__tests__/notification-test.js
index 6c197106628..849bafb1200 100644
--- a/src/notification/__tests__/notification-test.js
+++ b/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,12 +36,12 @@ describe('getNarrowFromNotificationData', () => {
       recipient_type: 'private',
       sender_email: 'mark@example.com',
     };
-    const narrow = getNarrowFromNotificationData(notification, DEFAULT_MAP);
+    const narrow = getNarrowFromNotificationData(notification, DEFAULT_MAP, ownUserId);
     expect(narrow).toEqual(privateNarrow('mark@example.com'));
   });
 
   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 = {
@@ -48,9 +49,9 @@ describe('getNarrowFromNotificationData', () => {
       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);
   });
diff --git a/src/notification/index.js b/src/notification/index.js
index 59826da1ad2..c9941deaa11 100644
--- a/src/notification/index.js
+++ b/src/notification/index.js
@@ -85,6 +85,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
@@ -107,6 +108,10 @@ export const getNarrowFromNotificationData = (
   const idStrs = data.pm_users.split(',');
   for (let i = 0; i < idStrs.length; ++i) {
     const id = parseInt(idStrs[i], 10);
+    if (id === ownUserId) {
+      continue;
+    }
+
     const user = usersById.get(id);
     if (!user) {
       return null;
diff --git a/src/notification/notificationActions.js b/src/notification/notificationActions.js
index 5cfc754263c..8ec47feb406 100644
--- a/src/notification/notificationActions.js
+++ b/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));
   }
diff --git a/src/utils/narrow.js b/src/utils/narrow.js
index 1216aeb6ffd..80231050bac 100644
--- a/src/utils/narrow.js
+++ b/src/utils/narrow.js
@@ -47,8 +47,6 @@ export const privateNarrow = (email: string): Narrow => [
 //
 // But we also have some callers that don't even ensure the set is the right
 // one, with the self-user properly there or not.  Known call stacks:
-//  * BUG, at least latent: getNarrowFromNotificationData: comes from
-//      notification's pm_users... which is sorted but not filtered.
 //  * BUG, ish: getNarrowFromLink doesn't ensure this precondition is met.
 //      And... there's basically a bug in the webapp, where the URL that
 //      appears in the location bar for a group PM conversation excludes
@@ -63,6 +61,8 @@ export const privateNarrow = (email: string): Narrow => [
 //      from `getRecentConversations`, which filters and sorts by email
 //  * OK, email: PmConversationList < UnreadCards: ditto
 //  * OK, unsorted: getNarrowFromMessage
+//  * Good: getNarrowFromNotificationData: filters, and starts from
+//      notification's pm_users, which is sorted.
 //  * Good: messageHeaderAsHtml: comes from pmKeyRecipientsFromMessage,
 //      which filters and sorts by ID
 export const groupNarrow = (emails: string[]): Narrow => [

From 63bf00c900b8073b707bc8ef71fa9c560c96d002 Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Thu, 29 Oct 2020 15:51:47 -0700
Subject: [PATCH 15/17] recipient: Factor out helper pmKeyRecipientsFromIds.

Does much the same job as pmKeyRecipientsFromMessage, but starting
from a list of user IDs.  We have this logic already to handle
notifications for group PMs, which come with such a list encoded as
comma-separated strings.  We'll want similar logic for group-PM narrow
URLs, which do the same thing; so factor it out to be shared.
---
 src/notification/index.js | 19 ++++---------------
 src/utils/recipient.js    | 28 ++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/src/notification/index.js b/src/notification/index.js
index c9941deaa11..5bc92470073 100644
--- a/src/notification/index.js
+++ b/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.
@@ -104,21 +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);
-    if (id === ownUserId) {
-      continue;
-    }
-
-    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> => {
diff --git a/src/utils/recipient.js b/src/utils/recipient.js
index ff9a3eacbc7..71e76ca09cc 100644
--- a/src/utils/recipient.js
+++ b/src/utils/recipient.js
@@ -118,6 +118,34 @@ export const pmKeyRecipientsFromMessage = (
   return filterRecipients(message.display_recipient, ownUser.user_id);
 };
 
+/**
+ * The set of users to identify a PM conversation by in our data structures.
+ *
+ * This produces the same set of users as `pmKeyRecipientsFromMessage`, just
+ * from a different form of input.
+ *
+ * The input may either include or exclude self, without affecting the
+ * result.
+ */
+export const pmKeyRecipientsFromIds = (
+  userIds: number[],
+  usersById: Map<number, User>,
+  ownUserId: number,
+): User[] | null => {
+  const users = [];
+  for (const id of userIds) {
+    if (id === ownUserId && userIds.length > 1) {
+      continue;
+    }
+    const user = usersById.get(id);
+    if (!user) {
+      return null;
+    }
+    users.push(user);
+  }
+  return users;
+};
+
 /**
  * The key this PM is filed under in the "unread messages" data structure.
  *

From 67919dad631720ed0ece7805d19311155484a186 Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Mon, 2 Nov 2020 15:24:11 -0800
Subject: [PATCH 16/17] internalLinks tests [nfc]: Clean up slightly around
 usersById.

This makes it just a bit more explicit what each of these bits
are doing, before we make some other changes in the next commit.
---
 src/utils/__tests__/internalLinks-test.js | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/src/utils/__tests__/internalLinks-test.js b/src/utils/__tests__/internalLinks-test.js
index 80cd605e905..6513cca5b02 100644
--- a/src/utils/__tests__/internalLinks-test.js
+++ b/src/utils/__tests__/internalLinks-test.js
@@ -125,11 +125,7 @@ describe('decodeHashComponent', () => {
 
 describe('getNarrowFromLink', () => {
   const [userA, userB, userC] = [eg.makeUser(), eg.makeUser(), eg.makeUser()];
-  const usersById: Map<number, User> = new Map([
-    [userA.user_id, userA],
-    [userB.user_id, userB],
-    [userC.user_id, userC],
-  ]);
+  const usersById: Map<number, User> = new Map([userA, userB, userC].map(u => [u.user_id, u]));
 
   const streamGeneral = eg.makeStream({ name: 'general' });
 
@@ -263,7 +259,7 @@ describe('getNarrowFromLink', () => {
   });
 
   test('if any of the user ids are not found: return null', () => {
-    const otherId = 1 + Math.max(userA.user_id, userB.user_id, userC.user_id);
+    const otherId = 1 + Math.max(...usersById.keys());
     const ids = `${userA.user_id},${userB.user_id},${otherId}`;
     expect(get(`https://example.com/#narrow/pm-with/${ids}-group`)).toEqual(null);
   });

From ff872f600383ff0d6ffe1659a70ec3b14773f029 Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Thu, 29 Oct 2020 16:06:17 -0700
Subject: [PATCH 17/17] narrow: Normalize set of users in parsing group-PM
 links.

In the tests, we replace `userA` with `eg.selfUser`, to make it
explicit which one is self now that that affects the logic.
---
 src/message/messagesActions.js            |  4 +++-
 src/utils/__tests__/internalLinks-test.js | 25 +++++++++++++++------
 src/utils/internalLinks.js                | 27 +++++++----------------
 src/utils/narrow.js                       | 18 ++++++---------
 4 files changed, 36 insertions(+), 38 deletions(-)

diff --git a/src/message/messagesActions.js b/src/message/messagesActions.js
index 1febbe3f225..3a62c927d23 100644
--- a/src/message/messagesActions.js
+++ b/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));
diff --git a/src/utils/__tests__/internalLinks-test.js b/src/utils/__tests__/internalLinks-test.js
index 6513cca5b02..865fc783bc2 100644
--- a/src/utils/__tests__/internalLinks-test.js
+++ b/src/utils/__tests__/internalLinks-test.js
@@ -124,8 +124,10 @@ describe('decodeHashComponent', () => {
 });
 
 describe('getNarrowFromLink', () => {
-  const [userA, userB, userC] = [eg.makeUser(), eg.makeUser(), eg.makeUser()];
-  const usersById: Map<number, User> = new Map([userA, userB, userC].map(u => [u.user_id, u]));
+  const [userB, userC] = [eg.makeUser(), eg.makeUser()];
+  const usersById: Map<number, User> = new Map(
+    [eg.selfUser, userB, userC].map(u => [u.user_id, u]),
+  );
 
   const streamGeneral = eg.makeStream({ name: 'general' });
 
@@ -135,6 +137,7 @@ describe('getNarrowFromLink', () => {
       new URL('https://example.com'),
       usersById,
       new Map(streams.map(s => [s.stream_id, s])),
+      eg.selfUser.user_id,
     );
 
   test('on link to realm domain but not narrow: return null', () => {
@@ -252,15 +255,23 @@ describe('getNarrowFromLink', () => {
   });
 
   test('on group PM link', () => {
-    const ids = `${userA.user_id},${userB.user_id},${userC.user_id}`;
+    const ids = `${userB.user_id},${userC.user_id}`;
     expect(get(`https://example.com/#narrow/pm-with/${ids}-group`)).toEqual(
-      groupNarrow([userA.email, userB.email, userC.email]),
+      groupNarrow([userB.email, userC.email]),
+    );
+  });
+
+  test('on group PM link including self', () => {
+    // The webapp doesn't generate these, but best to handle them anyway.
+    const ids = `${eg.selfUser.user_id},${userB.user_id},${userC.user_id}`;
+    expect(get(`https://example.com/#narrow/pm-with/${ids}-group`)).toEqual(
+      groupNarrow([userB.email, userC.email]),
     );
   });
 
   test('if any of the user ids are not found: return null', () => {
     const otherId = 1 + Math.max(...usersById.keys());
-    const ids = `${userA.user_id},${userB.user_id},${otherId}`;
+    const ids = `${userB.user_id},${otherId}`;
     expect(get(`https://example.com/#narrow/pm-with/${ids}-group`)).toEqual(null);
   });
 
@@ -269,9 +280,9 @@ describe('getNarrowFromLink', () => {
   });
 
   test('on a message link', () => {
-    const ids = `${userA.user_id},${userC.user_id}`;
+    const ids = `${userB.user_id},${userC.user_id}`;
     expect(get(`https://example.com/#narrow/pm-with/${ids}-group/near/2`)).toEqual(
-      groupNarrow([userA.email, userC.email]),
+      groupNarrow([userB.email, userC.email]),
     );
 
     expect(get('https://example.com/#narrow/stream/jest/topic/test/near/1')).toEqual(
diff --git a/src/utils/internalLinks.js b/src/utils/internalLinks.js
index 5800efe1122..d81097de323 100644
--- a/src/utils/internalLinks.js
+++ b/src/utils/internalLinks.js
@@ -2,6 +2,7 @@
 import { addBreadcrumb } from '@sentry/react-native';
 import type { Narrow, Stream, User } from '../types';
 import { topicNarrow, streamNarrow, groupNarrow, specialNarrow } from './narrow';
+import { pmKeyRecipientsFromIds } from './recipient';
 import { isUrlOnRealm } from './url';
 
 // TODO: Work out what this does, write a jsdoc for its interface, and
@@ -110,17 +111,9 @@ const parseStreamOperand = (operand, streamsById): string => {
 const parseTopicOperand = operand => decodeHashComponent(operand);
 
 /** Parse the operand of a `pm-with` operator. */
-const parsePmOperand = (operand, usersById) => {
-  const recipientIds = operand.split('-')[0].split(',');
-  const recipientEmails = [];
-  for (let i = 0; i < recipientIds.length; ++i) {
-    const user = usersById.get(parseInt(recipientIds[i], 10));
-    if (user === undefined) {
-      return null;
-    }
-    recipientEmails.push(user.email);
-  }
-  return recipientEmails;
+const parsePmOperand = (operand, usersById, ownUserId) => {
+  const idStrs = operand.split('-')[0].split(',');
+  return idStrs.map(s => parseInt(s, 10));
 };
 
 export const getNarrowFromLink = (
@@ -128,20 +121,16 @@ export const getNarrowFromLink = (
   realm: URL,
   usersById: Map<number, User>,
   streamsById: Map<number, Stream>,
+  ownUserId: number,
 ): Narrow | null => {
   const type = getLinkType(url, realm);
   const paths = getPathsFromUrl(url, realm);
 
   switch (type) {
     case 'pm': {
-      const recipientEmails = parsePmOperand(paths[1], usersById);
-      if (recipientEmails === null) {
-        return null;
-      }
-      // BUG: should normalize recipients; see comment on groupNarrow.
-      // (We're parsing a link someone wrote in a message, so the server
-      // gives us no guarantees here.)
-      return groupNarrow(recipientEmails);
+      const ids = parsePmOperand(paths[1], usersById, ownUserId);
+      const users = pmKeyRecipientsFromIds(ids, usersById, ownUserId);
+      return users && groupNarrow(users.map(u => u.email));
     }
     case 'topic':
       return topicNarrow(parseStreamOperand(paths[1], streamsById), parseTopicOperand(paths[3]));
diff --git a/src/utils/narrow.js b/src/utils/narrow.js
index 80231050bac..a6f92090e59 100644
--- a/src/utils/narrow.js
+++ b/src/utils/narrow.js
@@ -25,8 +25,8 @@ export const privateNarrow = (email: string): Narrow => [
  * A group PM narrow.
  *
  * The users represented in `emails` should agree, as a (multi)set, with
- * `pmKeyRecipientsFromMessage`.  But this isn't checked, and there may be
- * bugs where they don't; some consumers of this data re-normalize to be sure.
+ * `pmKeyRecipientsFromMessage`.  But this isn't checked, and we've had bugs
+ * where they don't; some consumers of this data re-normalize to be sure.
  *
  * They might not have a consistent sorting.  (This would be good to fix.)
  * Consumers of this data should sort for themselves when making comparisons.
@@ -45,21 +45,17 @@ export const privateNarrow = (email: string): Narrow => [
 // merely latent only because it doesn't (as far as we know) have any
 // user-visible effect.
 //
-// But we also have some callers that don't even ensure the set is the right
-// one, with the self-user properly there or not.  Known call stacks:
-//  * BUG, ish: getNarrowFromLink doesn't ensure this precondition is met.
-//      And... there's basically a bug in the webapp, where the URL that
-//      appears in the location bar for a group PM conversation excludes
-//      self -- so it's unusable if you try to give someone else in it a
-//      link to a particular message, say.  But conversely I guess it means
-//      that the mobile app actually works just as well as the webapp on the
-//      links people generate from the webapp.
+// Known call stacks:
 //  * OK, perilously, unsorted: CreateGroupScreen: the self user isn't
 //      offered in the UI, so effectively the list is filtered; can call
 //      with just one email, but happily this works out the same as pmNarrow
 //  * OK, email: PmConversationList < PmConversationCard: the data comes
 //      from `getRecentConversations`, which filters and sorts by email
 //  * OK, email: PmConversationList < UnreadCards: ditto
+//  * OK, unsorted: getNarrowFromLink.  Though there's basically a bug in
+//      the webapp, where the URL that appears in the location bar for a
+//      group PM conversation excludes self -- so it's unusable if you try
+//      to give someone else in it a link to a particular message, say.
 //  * OK, unsorted: getNarrowFromMessage
 //  * Good: getNarrowFromNotificationData: filters, and starts from
 //      notification's pm_users, which is sorted.