narrow: Switch to a reasonable representation, and store user IDs

src/boot/store.js

@@ -239,6 +239,21 @@ const migrations: { [string]: (GlobalState) => GlobalState } = {
     ),
   }),
 
+  // Change format of keys representing PM narrows, adding user IDs.
+  '20': state => ({
+    ...dropCache(state),
+    drafts: objectFromEntries(
+      Object.keys(state.drafts)
+        // Just drop any old-style, email-only PM keys.  Converting them
+        // would require using additional information to look up the IDs,
+        // which would make this more complex than any of our other
+        // migrations.  Drafts are inherently short-term, and are already
+        // discarded whenever switching between accounts.
+        .filter(key => !key.startsWith('pm:s:'))
+        .map(key => [key, state.drafts[key]]),
+    ),
+  }),
+
   // TIP: When adding a migration, consider just using `dropCache`.
 };
 

src/chat/__tests__/narrowsSelectors-test.js

@@ -11,7 +11,7 @@ import {
 import {
   HOME_NARROW,
   HOME_NARROW_STR,
-  pmNarrowFromEmail,
+  pm1to1NarrowFromUser,
   streamNarrow,
   topicNarrow,
   STARRED_NARROW,
@@ -81,14 +81,14 @@ describe('getMessagesForNarrow', () => {
   test('do not combine messages and outbox in different narrow', () => {
     const state = eg.reduxState({
       narrows: Immutable.Map({
-        [keyFromNarrow(pmNarrowFromEmail('john@example.com'))]: [123],
+        [keyFromNarrow(pm1to1NarrowFromUser(eg.otherUser))]: [123],
       }),
       messages,
       outbox: [outboxMessage],
       realm: eg.realmState({ email: eg.selfUser.email }),
     });
 
-    const result = getMessagesForNarrow(state, pmNarrowFromEmail('john@example.com'));
+    const result = getMessagesForNarrow(state, pm1to1NarrowFromUser(eg.otherUser));
 
     expect(result).toEqual([message]);
   });
@@ -193,7 +193,7 @@ describe('getStreamInNarrow', () => {
   });
 
   test('return NULL_SUBSCRIPTION is narrow is not topic or stream', () => {
-    expect(getStreamInNarrow(state, pmNarrowFromEmail('abc@zulip.com'))).toEqual(NULL_SUBSCRIPTION);
+    expect(getStreamInNarrow(state, pm1to1NarrowFromUser(eg.otherUser))).toEqual(NULL_SUBSCRIPTION);
     expect(getStreamInNarrow(state, topicNarrow(stream4.name, 'topic'))).toEqual(NULL_SUBSCRIPTION);
   });
 });
@@ -256,7 +256,7 @@ describe('isNarrowValid', () => {
       streams: [],
       users: [user],
     });
-    const narrow = pmNarrowFromEmail(user.email);
+    const narrow = pm1to1NarrowFromUser(user);
 
     const result = isNarrowValid(state, narrow);
 
@@ -274,7 +274,7 @@ describe('isNarrowValid', () => {
       streams: [],
       users: [],
     });
-    const narrow = pmNarrowFromEmail(user.email);
+    const narrow = pm1to1NarrowFromUser(user);
 
     const result = isNarrowValid(state, narrow);
 
@@ -331,7 +331,7 @@ describe('isNarrowValid', () => {
       streams: [],
       users: [],
     });
-    const narrow = pmNarrowFromEmail(bot.email);
+    const narrow = pm1to1NarrowFromUser(bot);
 
     const result = isNarrowValid(state, narrow);
 
@@ -349,7 +349,7 @@ describe('isNarrowValid', () => {
       streams: [],
       users: [],
     });
-    const narrow = pmNarrowFromEmail(notActiveUser.email);
+    const narrow = pm1to1NarrowFromUser(notActiveUser);
 
     const result = isNarrowValid(state, narrow);
 

src/compose/__tests__/getComposeInputPlaceholder-test.js

@@ -3,7 +3,7 @@ import deepFreeze from 'deep-freeze';
 
 import getComposeInputPlaceholder from '../getComposeInputPlaceholder';
 import {
-  pmNarrowFromEmail,
+  pm1to1NarrowFromUser,
   streamNarrow,
   topicNarrow,
   pmNarrowFromUsersUnsafe,
@@ -15,7 +15,7 @@ describe('getComposeInputPlaceholder', () => {
   const ownEmail = eg.selfUser.email;
 
   test('returns "Message @ThisPerson" object for person narrow', () => {
-    const narrow = deepFreeze(pmNarrowFromEmail(eg.otherUser.email));
+    const narrow = deepFreeze(pm1to1NarrowFromUser(eg.otherUser));
     const placeholder = getComposeInputPlaceholder(narrow, ownEmail, usersByEmail);
     expect(placeholder).toEqual({
       text: 'Message {recipient}',
@@ -24,7 +24,7 @@ describe('getComposeInputPlaceholder', () => {
   });
 
   test('returns "Jot down something" object for self narrow', () => {
-    const narrow = deepFreeze(pmNarrowFromEmail(eg.selfUser.email));
+    const narrow = deepFreeze(pm1to1NarrowFromUser(eg.selfUser));
     const placeholder = getComposeInputPlaceholder(narrow, ownEmail, usersByEmail);
     expect(placeholder).toEqual({ text: 'Jot down something' });
   });

src/notification/__tests__/notification-test.js

@@ -4,7 +4,7 @@ import deepFreeze from 'deep-freeze';
 import type { UserOrBot } from '../../api/modelTypes';
 import type { JSONableDict } from '../../utils/jsonable';
 import { getNarrowFromNotificationData } from '..';
-import { topicNarrow, pmNarrowFromEmail, pmNarrowFromUsersUnsafe } from '../../utils/narrow';
+import { topicNarrow, pm1to1NarrowFromUser, pmNarrowFromUsersUnsafe } from '../../utils/narrow';
 
 import * as eg from '../../__tests__/lib/exampleData';
 import { fromAPNsImpl as extractIosNotificationData } from '../extract';
@@ -17,7 +17,7 @@ describe('getNarrowFromNotificationData', () => {
   test('unknown notification data returns null', () => {
     // $FlowFixMe: actually validate APNs messages
     const notification: Notification = {};
-    const narrow = getNarrowFromNotificationData(notification, DEFAULT_MAP, ownUserId);
+    const narrow = getNarrowFromNotificationData(notification, DEFAULT_MAP, new Map(), ownUserId);
     expect(narrow).toBe(null);
   });
 
@@ -27,22 +27,31 @@ describe('getNarrowFromNotificationData', () => {
       stream: 'some stream',
       topic: 'some topic',
     };
-    const narrow = getNarrowFromNotificationData(notification, DEFAULT_MAP, ownUserId);
+    const narrow = getNarrowFromNotificationData(notification, DEFAULT_MAP, new Map(), ownUserId);
     expect(narrow).toEqual(topicNarrow('some stream', 'some topic'));
   });
 
   test('on notification for a private message returns a PM narrow', () => {
+    const users = [eg.selfUser, eg.otherUser];
+    const allUsersById: Map<number, UserOrBot> = new Map(users.map(u => [u.user_id, u]));
+    const allUsersByEmail: Map<string, UserOrBot> = new Map(users.map(u => [u.email, u]));
     const notification = {
       recipient_type: 'private',
-      sender_email: 'mark@example.com',
+      sender_email: eg.otherUser.email,
     };
-    const narrow = getNarrowFromNotificationData(notification, DEFAULT_MAP, ownUserId);
-    expect(narrow).toEqual(pmNarrowFromEmail('mark@example.com'));
+    const narrow = getNarrowFromNotificationData(
+      notification,
+      allUsersById,
+      allUsersByEmail,
+      ownUserId,
+    );
+    expect(narrow).toEqual(pm1to1NarrowFromUser(eg.otherUser));
   });
 
   test('on notification for a group message returns a group narrow', () => {
     const users = [eg.selfUser, eg.makeUser(), eg.makeUser(), eg.makeUser()];
     const allUsersById: Map<number, UserOrBot> = new Map(users.map(u => [u.user_id, u]));
+    const allUsersByEmail: Map<string, UserOrBot> = new Map(users.map(u => [u.email, u]));
 
     const notification = {
       recipient_type: 'private',
@@ -51,7 +60,12 @@ describe('getNarrowFromNotificationData', () => {
 
     const expectedNarrow = pmNarrowFromUsersUnsafe(users.slice(1));
 
-    const narrow = getNarrowFromNotificationData(notification, allUsersById, ownUserId);
+    const narrow = getNarrowFromNotificationData(
+      notification,
+      allUsersById,
+      allUsersByEmail,
+      ownUserId,
+    );
 
     expect(narrow).toEqual(expectedNarrow);
   });
@@ -63,7 +77,7 @@ describe('getNarrowFromNotificationData', () => {
     };
     const allUsersById = new Map();
 
-    const narrow = getNarrowFromNotificationData(notification, allUsersById, ownUserId);
+    const narrow = getNarrowFromNotificationData(notification, allUsersById, new Map(), ownUserId);
 
     expect(narrow).toBe(null);
   });

src/notification/index.js

@@ -4,7 +4,7 @@ import NotificationsIOS from 'react-native-notifications';
 
 import type { Notification } from './types';
 import type { Auth, Dispatch, Identity, Narrow, UserOrBot } from '../types';
-import { topicNarrow, pmNarrowFromEmail, pmNarrowFromUsers } from '../utils/narrow';
+import { topicNarrow, pmNarrowFromUsers, pm1to1NarrowFromUser } from '../utils/narrow';
 import type { JSONable, JSONableDict } from '../utils/jsonable';
 import * as api from '../api';
 import * as logging from '../utils/logging';
@@ -86,6 +86,7 @@ export const getAccountFromNotificationData = (
 export const getNarrowFromNotificationData = (
   data: Notification,
   allUsersById: Map<number, UserOrBot>,
+  allUsersByEmail: Map<string, UserOrBot>,
   ownUserId: number,
 ): Narrow | null => {
   if (!data.recipient_type) {
@@ -102,7 +103,8 @@ export const getNarrowFromNotificationData = (
   }
 
   if (data.pm_users === undefined) {
-    return pmNarrowFromEmail(data.sender_email);
+    const user = allUsersByEmail.get(data.sender_email);
+    return (user && pm1to1NarrowFromUser(user)) ?? null;
   }
 
   const ids = data.pm_users.split(',').map(s => parseInt(s, 10));

src/notification/notificationActions.js

@@ -14,7 +14,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 { getAllUsersById, getOwnUserId } from '../users/userSelectors';
+import { getAllUsersByEmail, getAllUsersById, getOwnUserId } from '../users/userSelectors';
 import { doNarrow } from '../message/messagesActions';
 import { accountSwitch } from '../account/accountActions';
 import { getIdentities } from '../account/accountsSelectors';
@@ -52,7 +52,12 @@ export const narrowToNotification = (data: ?Notification) => (
     return;
   }
 
-  const narrow = getNarrowFromNotificationData(data, getAllUsersById(state), getOwnUserId(state));
+  const narrow = getNarrowFromNotificationData(
+    data,
+    getAllUsersById(state),
+    getAllUsersByEmail(state),
+    getOwnUserId(state),
+  );
   if (narrow) {
     dispatch(doNarrow(narrow));
   }

src/title/__tests__/titleSelectors-test.js

@@ -1,7 +1,7 @@
 /* @flow strict-local */
 
 import { DEFAULT_TITLE_BACKGROUND_COLOR, getTitleBackgroundColor } from '../titleSelectors';
-import { pmNarrowFromUsersUnsafe, streamNarrow, pmNarrowFromEmail } from '../../utils/narrow';
+import { pmNarrowFromUsersUnsafe, streamNarrow, pm1to1NarrowFromUser } from '../../utils/narrow';
 import * as eg from '../../__tests__/lib/exampleData';
 
 describe('getTitleBackgroundColor', () => {
@@ -24,7 +24,7 @@ describe('getTitleBackgroundColor', () => {
   });
 
   test('return default for non topic/stream narrow', () => {
-    expect(getTitleBackgroundColor(state, pmNarrowFromEmail(eg.otherUser.email))).toEqual(
+    expect(getTitleBackgroundColor(state, pm1to1NarrowFromUser(eg.otherUser))).toEqual(
       DEFAULT_TITLE_BACKGROUND_COLOR,
     );
     expect(

src/utils/__tests__/narrow-test.js

@@ -3,10 +3,9 @@
 import {
   HOME_NARROW,
   isHomeNarrow,
-  pmNarrowFromEmail,
+  pm1to1NarrowFromUser,
   is1to1PmNarrow,
   pmNarrowFromUsersUnsafe,
-  specialNarrow,
   isSpecialNarrow,
   ALL_PRIVATE_NARROW,
   streamNarrow,
@@ -17,14 +16,12 @@ import {
   isSearchNarrow,
   isPmNarrow,
   isMessageInNarrow,
-  isSameNarrow,
   isStreamOrTopicNarrow,
   getNarrowsForMessage,
   getNarrowForReply,
   parseNarrow,
   STARRED_NARROW,
   MENTIONED_NARROW,
-  pm1to1NarrowFromUser,
   keyFromNarrow,
   streamNameOfNarrow,
   topicOfNarrow,
@@ -39,24 +36,24 @@ describe('HOME_NARROW', () => {
   });
 });
 
-describe('pmNarrowFromEmail', () => {
+describe('pm1to1NarrowFromUser', () => {
   test('produces a 1:1 narrow', () => {
-    const narrow = pmNarrowFromEmail(eg.otherUser.email);
+    const narrow = pm1to1NarrowFromUser(eg.otherUser);
     expect(is1to1PmNarrow(narrow)).toBeTrue();
     expect(emailOfPm1to1Narrow(narrow)).toEqual(eg.otherUser.email);
   });
 
   test('if operator is "pm-with" and only one email, then it is a private narrow', () => {
     expect(is1to1PmNarrow(HOME_NARROW)).toBe(false);
-    expect(is1to1PmNarrow(pmNarrowFromEmail(eg.otherUser.email))).toBe(true);
+    expect(is1to1PmNarrow(pm1to1NarrowFromUser(eg.otherUser))).toBe(true);
   });
 });
 
 describe('isPmNarrow', () => {
   test('a private or group narrow is any "pm-with" narrow', () => {
     expect(isPmNarrow(undefined)).toBe(false);
     expect(isPmNarrow(HOME_NARROW)).toBe(false);
-    expect(isPmNarrow(pmNarrowFromEmail(eg.otherUser.email))).toBe(true);
+    expect(isPmNarrow(pm1to1NarrowFromUser(eg.otherUser))).toBe(true);
     expect(isPmNarrow(pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser]))).toBe(true);
   });
 });
@@ -67,7 +64,7 @@ describe('isStreamOrTopicNarrow', () => {
     expect(isStreamOrTopicNarrow(streamNarrow('some stream'))).toBe(true);
     expect(isStreamOrTopicNarrow(topicNarrow('some stream', 'some topic'))).toBe(true);
     expect(isStreamOrTopicNarrow(HOME_NARROW)).toBe(false);
-    expect(isStreamOrTopicNarrow(pmNarrowFromEmail(eg.otherUser.email))).toBe(false);
+    expect(isStreamOrTopicNarrow(pm1to1NarrowFromUser(eg.otherUser))).toBe(false);
     expect(isStreamOrTopicNarrow(pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser]))).toBe(
       false,
     );
@@ -143,15 +140,15 @@ describe('isMessageInNarrow', () => {
       ['PM', false, eg.pmMessage()],
     ]],
 
-    ['1:1 PM conversation, non-self', pmNarrowFromEmail(eg.otherUser.email), [
+    ['1:1 PM conversation, non-self', pm1to1NarrowFromUser(eg.otherUser), [
       ['matching PM, inbound', true, eg.pmMessage()],
       ['matching PM, outbound', true, eg.pmMessage({ sender: 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()],
     ]],
-    ['self-1:1 conversation', pmNarrowFromEmail(eg.selfUser.email), [
+    ['self-1:1 conversation', pm1to1NarrowFromUser(eg.selfUser), [
       ['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 })],
@@ -281,7 +278,7 @@ describe('getNarrowsForMessage', () => {
     {
       label: 'PM message with one person',
       message: eg.pmMessage({ sender: eg.otherUser }),
-      expectedNarrows: [HOME_NARROW, ALL_PRIVATE_NARROW, pmNarrowFromEmail(eg.otherUser.email)],
+      expectedNarrows: [HOME_NARROW, ALL_PRIVATE_NARROW, pm1to1NarrowFromUser(eg.otherUser)],
     },
     {
       label: 'Group PM message',
@@ -309,12 +306,12 @@ describe('getNarrowForReply', () => {
         eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser] }),
         eg.selfUser,
       ),
-    ).toEqual(pmNarrowFromEmail(eg.selfUser.email));
+    ).toEqual(pm1to1NarrowFromUser(eg.selfUser));
   });
 
   test('for 1:1 PM, returns a 1:1 PM narrow', () => {
     const message = eg.pmMessage();
-    const expectedNarrow = pmNarrowFromEmail(eg.otherUser.email);
+    const expectedNarrow = pm1to1NarrowFromUser(eg.otherUser);
 
     const actualNarrow = getNarrowForReply(message, eg.selfUser);
 
@@ -342,20 +339,6 @@ describe('getNarrowForReply', () => {
   });
 });
 
-describe('isSameNarrow', () => {
-  test('Return true if two narrows are same', () => {
-    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);
-    expect(isSameNarrow(topicNarrow('stream', 'topic'), topicNarrow('stream', 'topic'))).toBe(true);
-    expect(isSameNarrow(topicNarrow('stream', 'topic'), topicNarrow('stream', 'topic1'))).toBe(
-      false,
-    );
-    expect(isSameNarrow(HOME_NARROW, specialNarrow('private'))).toBe(false);
-    expect(isSameNarrow(HOME_NARROW, HOME_NARROW)).toBe(true);
-  });
-});
-
 describe('keyFromNarrow+parseNarrow', () => {
   const baseNarrows = [
     ['whole stream', streamNarrow(eg.stream.name)],