narrow: Stop storing emails!

src/boot/store.js

@@ -18,7 +18,6 @@ import ZulipAsyncStorage from './ZulipAsyncStorage';
 import createMigration from '../redux-persist-migrate/index';
 import { provideLoggingContext } from './loggingContext';
 import { tryGetActiveAccount } from '../account/accountsSelectors';
-import { keyFromNarrow } from '../utils/narrow';
 import { objectFromEntries } from '../jsBackport';
 
 if (process.env.NODE_ENV === 'development') {
@@ -226,29 +225,25 @@ const migrations: { [string]: (GlobalState) => GlobalState } = {
   // `AvatarURL`.
   '18': dropCache,
 
-  // Change format of keys representing narrows, from JSON to our format.
-  '19': state => ({
+  // Change format of keys representing narrows: from JSON to our format,
+  // then for PM narrows adding user IDs.
+  '21': state => ({
     ...dropCache(state),
-    drafts: objectFromEntries(
-      // Note this will migrate straight to our current format, even after
-      // that changes from when this migration was written!  That saves us
-      // from duplicating `keyFromNarrow` here... but calls for care in
-      // migrations for future changes to `keyFromNarrow`.
-      Object.keys(state.drafts).map(key => [keyFromNarrow(JSON.parse(key)), state.drafts[key]]),
-    ),
+    // The old format was a rather hairy format that we don't want to
+    // permanently keep around the code to parse.  For PMs, there's an
+    // extra wrinkle in that any conversion would require using additional
+    // information to look up the IDs.  Drafts are inherently short-term,
+    // and are already discarded whenever switching between accounts;
+    // so we just drop them here.
+    drafts: {},
   }),
 
-  // Change format of keys representing PM narrows, adding user IDs.
-  '20': state => ({
+  // Change format of keys representing PM narrows, dropping emails.
+  '22': 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.replace(/^pm:d:(.*?):.*/s, 'pm:$1'))
         .map(key => [key, state.drafts[key]]),
     ),
   }),

src/chat/narrowsSelectors.js

@@ -20,7 +20,7 @@ import {
   getOutbox,
 } from '../directSelectors';
 import { getCaughtUpForNarrow } from '../caughtup/caughtUpSelectors';
-import { getAllUsersByEmail, getAllUsersById, getOwnUser } from '../users/userSelectors';
+import { getAllUsersById, getOwnUser } from '../users/userSelectors';
 import {
   isStreamOrTopicNarrow,
   isMessageInNarrow,
@@ -134,17 +134,14 @@ export const getStreamInNarrow: Selector<Subscription | {| ...Stream, in_home_vi
 export const isNarrowValid: Selector<boolean, Narrow> = createSelector(
   (state, narrow) => narrow,
   state => getStreams(state),
-  state => getAllUsersByEmail(state),
   state => getAllUsersById(state),
-  (narrow, streams, allUsersByEmail, allUsersById) =>
+  (narrow, streams, allUsersById) =>
     caseNarrowDefault(
       narrow,
       {
         stream: streamName => streams.find(s => s.name === streamName) !== undefined,
         topic: streamName => streams.find(s => s.name === streamName) !== undefined,
-        pm: (emails, ids) =>
-          emails.every(email => allUsersByEmail.get(email) !== undefined)
-          && ids.every(id => allUsersById.get(id) !== undefined),
+        pm: ids => ids.every(id => allUsersById.get(id) !== undefined),
       },
       () => true,
     ),

src/compose/getComposeInputPlaceholder.js

@@ -10,7 +10,7 @@ export default (
   caseNarrowDefault(
     narrow,
     {
-      pm: (emails, ids) => {
+      pm: ids => {
         if (ids.length > 1) {
           return { text: 'Message group' };
         }

src/message/fetchActions.js

@@ -32,6 +32,7 @@ import { realmInit } from '../realm/realmActions';
 import { startEventPolling } from '../events/eventActions';
 import { logout } from '../account/accountActions';
 import { ZulipVersion } from '../utils/zulipVersion';
+import { getAllUsersById } from '../users/userSelectors';
 
 const messageFetchStart = (narrow: Narrow, numBefore: number, numAfter: number): Action => ({
   type: MESSAGE_FETCH_START,
@@ -88,7 +89,7 @@ export const fetchMessages = (fetchArgs: {|
   try {
     const { messages, found_newest, found_oldest } = await api.getMessages(getAuth(getState()), {
       ...fetchArgs,
-      narrow: apiNarrowOfNarrow(fetchArgs.narrow),
+      narrow: apiNarrowOfNarrow(fetchArgs.narrow, getAllUsersById(getState())),
       useFirstUnread: fetchArgs.anchor === FIRST_UNREAD_ANCHOR, // TODO: don't use this; see #4203
     });
     dispatch(
@@ -236,7 +237,7 @@ export const fetchMessagesInNarrow = (
 const fetchPrivateMessages = () => async (dispatch: Dispatch, getState: GetState) => {
   const auth = getAuth(getState());
   const { messages, found_newest, found_oldest } = await api.getMessages(auth, {
-    narrow: apiNarrowOfNarrow(ALL_PRIVATE_NARROW),
+    narrow: apiNarrowOfNarrow(ALL_PRIVATE_NARROW, getAllUsersById(getState())),
     anchor: LAST_MESSAGE_ANCHOR,
     numBefore: 100,
     numAfter: 0,

src/outbox/outboxActions.js

@@ -124,7 +124,7 @@ const extractTypeToAndSubjectFromNarrow = (
   ownUser: UserOrBot,
 ): DataFromNarrow =>
   caseNarrowPartial(narrow, {
-    pm: (emails, ids) => ({
+    pm: ids => ({
       type: 'private',
       display_recipient: recipientsFromIds(ids, allUsersById, ownUser),
       subject: '',

src/title-buttons/titleButtonFromNarrow.js

@@ -18,7 +18,7 @@ export const InfoButton: ComponentType<Props> = props =>
     {
       stream: streamName => <InfoNavButtonStream {...props} />,
       topic: (streamName, topic) => <InfoNavButtonStream {...props} />,
-      pm: (emails, ids) =>
+      pm: ids =>
         ids.length === 1 ? (
           <InfoNavButtonPrivate userId={ids[0]} color={props.color} />
         ) : (

src/title/Title.js

@@ -30,7 +30,7 @@ export default class Title extends PureComponent<Props> {
       allPrivate: () => <TitleSpecial code="private" color={color} />,
       stream: () => <TitleStream narrow={narrow} color={color} />,
       topic: () => <TitleStream narrow={narrow} color={color} />,
-      pm: (emails, ids) =>
+      pm: ids =>
         ids.length === 1 ? (
           <TitlePrivate userId={ids[0]} color={color} />
         ) : (

src/unread/unreadSelectors.js

@@ -256,7 +256,7 @@ export const getUnreadCountForNarrow: Selector<number, Narrow> = createSelector(
         );
       },
 
-      pm: (emails, ids) => {
+      pm: ids => {
         if (ids.length > 1) {
           const unreadsKey = pmUnreadsKeyFromPmKeyIds(ids, ownUserId);
           const unread = unreadHuddles.find(x => x.user_ids_string === unreadsKey);

src/utils/__tests__/narrow-test.js

@@ -40,7 +40,7 @@ describe('pm1to1NarrowFromUser', () => {
   test('produces a 1:1 narrow', () => {
     const narrow = pm1to1NarrowFromUser(eg.otherUser);
     expect(is1to1PmNarrow(narrow)).toBeTrue();
-    expect(caseNarrowPartial(narrow, { pm: (emails, ids) => ids })).toEqual([eg.otherUser.user_id]);
+    expect(caseNarrowPartial(narrow, { pm: ids => ids })).toEqual([eg.otherUser.user_id]);
   });
 
   test('if operator is "pm-with" and only one email, then it is a private narrow', () => {

src/utils/narrow.js

@@ -32,7 +32,7 @@ import {
 export opaque type Narrow =
   | {| type: 'stream', streamName: string |}
   | {| type: 'topic', streamName: string, topic: string |}
-  | {| type: 'pm', userIds: $ReadOnlyArray<number>, emails: $ReadOnlyArray<string> |}
+  | {| type: 'pm', userIds: $ReadOnlyArray<number> |}
   | {| type: 'search', query: string |}
   | {| type: 'all' | 'starred' | 'mentioned' | 'all-pm' |};
 
@@ -59,8 +59,8 @@ export const HOME_NARROW_STR: string = keyFromNarrow(HOME_NARROW);
  * accidentally disagreeing on whether to include the self-user, or on how
  * to sort the list (by user ID vs. email), or neglecting to sort it at all.
  */
-const pmNarrowInternal = (userIds: $ReadOnlyArray<number>, emails: string[]): Narrow =>
-  Object.freeze({ type: 'pm', userIds, emails });
+const pmNarrowInternal = (userIds: $ReadOnlyArray<number>): Narrow =>
+  Object.freeze({ type: 'pm', userIds });
 
 /**
  * A PM narrow, either 1:1 or group.
@@ -73,7 +73,7 @@ const pmNarrowInternal = (userIds: $ReadOnlyArray<number>, emails: string[]): Na
  * different form of input.
  */
 export const pmNarrowFromRecipients = (recipients: PmKeyRecipients): Narrow =>
-  pmNarrowInternal(recipients.map(r => r.id), recipients.map(r => r.email));
+  pmNarrowInternal(recipients);
 
 /**
  * A PM narrow, either 1:1 or group.
@@ -85,7 +85,7 @@ export const pmNarrowFromRecipients = (recipients: PmKeyRecipients): Narrow =>
  * single specific user.
  */
 export const pmNarrowFromUsers = (recipients: PmKeyUsers): Narrow =>
-  pmNarrowInternal(recipients.map(r => r.user_id), recipients.map(r => r.email));
+  pmNarrowInternal(recipients.map(r => r.user_id));
 
 /**
  * FOR TESTS ONLY.  Like pmNarrowFromUsers, but without validation.
@@ -105,7 +105,7 @@ export const pmNarrowFromUsers = (recipients: PmKeyUsers): Narrow =>
 // annoying thing is just that that requires an ownUserId value.
 export const pmNarrowFromUsersUnsafe = (recipients: UserOrBot[]): Narrow => {
   recipients.sort((a, b) => a.user_id - b.user_id);
-  return pmNarrowInternal(recipients.map(r => r.user_id), recipients.map(r => r.email));
+  return pmNarrowInternal(recipients.map(r => r.user_id));
 };
 
 /**
@@ -115,8 +115,7 @@ export const pmNarrowFromUsersUnsafe = (recipients: UserOrBot[]): Narrow => {
  * statically has just one other user it's a bit more convenient because it
  * doesn't require going through our `recipient` helpers.
  */
-export const pm1to1NarrowFromUser = (user: UserOrBot): Narrow =>
-  pmNarrowInternal([user.user_id], [user.email]);
+export const pm1to1NarrowFromUser = (user: UserOrBot): Narrow => pmNarrowInternal([user.user_id]);
 
 export const specialNarrow = (operand: string): Narrow => {
   if (operand === 'starred') {
@@ -153,7 +152,7 @@ export const SEARCH_NARROW = (query: string): Narrow => Object.freeze({ type: 's
 
 type NarrowCases<T> = {|
   home: () => T,
-  pm: (emails: $ReadOnlyArray<string>, ids: $ReadOnlyArray<number>) => T,
+  pm: (ids: $ReadOnlyArray<number>) => T,
   starred: () => T,
   mentioned: () => T,
   allPrivate: () => T,
@@ -171,7 +170,7 @@ export function caseNarrow<T>(narrow: Narrow, cases: NarrowCases<T>): T {
   switch (narrow.type) {
     case 'stream': return cases.stream(narrow.streamName);
     case 'topic': return cases.topic(narrow.streamName, narrow.topic);
-    case 'pm': return cases.pm(narrow.emails, narrow.userIds);
+    case 'pm': return cases.pm(narrow.userIds);
     case 'search': return cases.search(narrow.query);
     case 'all': return cases.home();
     case 'starred': return cases.starred();
@@ -237,8 +236,7 @@ export function caseNarrowDefault<T>(
  * something like `base64Utf8Encode` to encode into the permitted subset.
  */
 // The arbitrary Unicode codepoints come from (a) stream names and topics,
-// and (b) our use of `\x00` as a delimiter.  Also perhaps email addresses,
-// if it's possible for those to have exciting characters in them.
+// and (b) our use of `\x00` as a delimiter.
 export function keyFromNarrow(narrow: Narrow): string {
   // The ":s" (for "string") in several of these is to keep them disjoint,
   // out of an abundance of caution, from future keys that use numeric IDs.
@@ -254,7 +252,8 @@ export function keyFromNarrow(narrow: Narrow): string {
     topic: (streamName, topic) => `topic:s:${streamName}\x00${topic}`,
 
     // An earlier version had `pm:s:`.
-    pm: (emails, ids) => `pm:d:${ids.join(',')}:${emails.join(',')}`,
+    // Another had `pm:d:`.
+    pm: ids => `pm:${ids.join(',')}`,
 
     home: () => 'all',
     starred: () => 'starred',
@@ -295,17 +294,8 @@ export const parseNarrow = (narrowStr: string): Narrow => {
     }
 
     case 'pm:': {
-      // The `/s` regexp flag means the `.` patterns match absolutely
-      // anything.  By default they reject certain "newline" characters,
-      // which might be impossible in email addresses but it's simplest
-      // not to have to care.
-      const match = /^d:(.*?):(.*)/s.exec(rest);
-      if (!match) {
-        throw makeError();
-      }
-      const ids = match[1].split(',').map(s => Number.parseInt(s, 10));
-      const emails = match[2].split(',');
-      return pmNarrowInternal(ids, emails);
+      const ids = rest.split(',').map(s => Number.parseInt(s, 10));
+      return pmNarrowInternal(ids);
     }
 
     case 'search:': {
@@ -336,10 +326,10 @@ export const isHomeNarrow = (narrow?: Narrow): boolean =>
   !!narrow && caseNarrowDefault(narrow, { home: () => true }, () => false);
 
 export const is1to1PmNarrow = (narrow?: Narrow): boolean =>
-  !!narrow && caseNarrowDefault(narrow, { pm: (emails, ids) => ids.length === 1 }, () => false);
+  !!narrow && caseNarrowDefault(narrow, { pm: ids => ids.length === 1 }, () => false);
 
 export const isGroupPmNarrow = (narrow?: Narrow): boolean =>
-  !!narrow && caseNarrowDefault(narrow, { pm: (emails, ids) => ids.length > 1 }, () => false);
+  !!narrow && caseNarrowDefault(narrow, { pm: ids => ids.length > 1 }, () => false);
 
 /**
  * The "PM key recipients" IDs for a PM narrow; else error.
@@ -348,7 +338,7 @@ export const isGroupPmNarrow = (narrow?: Narrow): boolean =>
  * `PmKeyUsers`, but contains only their user IDs.
  */
 export const userIdsOfPmNarrow = (narrow: Narrow): $ReadOnlyArray<number> =>
-  caseNarrowPartial(narrow, { pm: (emails, ids) => ids });
+  caseNarrowPartial(narrow, { pm: ids => ids });
 
 /**
  * The stream name for a stream or topic narrow; else error.
@@ -399,14 +389,28 @@ export const isSearchNarrow = (narrow?: Narrow): boolean =>
 /**
  * Convert the narrow into the form used in the Zulip API at get-messages.
  */
-export const apiNarrowOfNarrow = (narrow: Narrow): ApiNarrow =>
+export const apiNarrowOfNarrow = (
+  narrow: Narrow,
+  allUsersById: Map<number, UserOrBot>,
+): ApiNarrow =>
   caseNarrow(narrow, {
     stream: streamName => [{ operator: 'stream', operand: streamName }],
     topic: (streamName, topic) => [
       { operator: 'stream', operand: streamName },
       { operator: 'topic', operand: topic },
     ],
-    pm: emails => [{ operator: 'pm-with', operand: emails.join(',') }],
+    pm: ids => {
+      const emails = [];
+      for (const id of ids) {
+        const email = allUsersById.get(id)?.email;
+        if (email === undefined) {
+          throw new Error('apiNarrowOfNarrow: missing user');
+        }
+        emails.push(email);
+      }
+      // TODO(server-2.1): just send IDs instead
+      return [{ operator: 'pm-with', operand: emails.join(',') }];
+    },
     search: query => [{ operator: 'search', operand: query }],
     home: () => [],
     starred: () => [{ operator: 'is', operand: 'starred' }],
@@ -441,7 +445,7 @@ export const isMessageInNarrow = (
       message.type === 'stream'
       && streamName === streamNameOfStreamMessage(message)
       && topic === message.subject,
-    pm: (emails, ids) => {
+    pm: ids => {
       if (message.type !== 'private') {
         return false;
       }

src/utils/recipient.js

@@ -59,7 +59,7 @@ export const recipientsOfPrivateMessage = (
  *
  * See also `pmNarrowFromRecipients`, which requires a value of this type.
  */
-export opaque type PmKeyRecipients: $ReadOnlyArray<PmRecipientUser> = $ReadOnlyArray<PmRecipientUser>;
+export opaque type PmKeyRecipients: $ReadOnlyArray<number> = $ReadOnlyArray<number>;
 
 /**
  * A list of users identifying a PM conversation, as per pmKeyRecipientsFromMessage.
@@ -180,7 +180,10 @@ export const pmKeyRecipientsFromMessage = (
   if (message.type !== 'private') {
     throw new Error('pmKeyRecipientsFromMessage: expected PM, got stream message');
   }
-  return filterRecipients(recipientsOfPrivateMessage(message), ownUser.user_id);
+  return filterRecipientsAsUserIds(
+    recipientsOfPrivateMessage(message).map(r => r.id),
+    ownUser.user_id,
+  );
 };
 
 /**