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

@@ -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/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(

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,38 +111,26 @@ 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 = (
   url: string,
   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]));

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.