outbox: Fix some bugs from Narrow confusion; remove Narrow

src/__tests__/lib/exampleData.js

@@ -370,10 +370,9 @@ const outboxMessageBase: $Diff<Outbox, {| id: mixed, timestamp: mixed |}> = deep
 
   avatar_url: selfUser.avatar_url,
   content: '<p>Test.</p>',
-  display_recipient: 'test',
+  display_recipient: stream.name,
   // id: ...,
   markdownContent: 'Test.',
-  narrow: [{ operator: 'stream', operand: 'test' }],
   reactions: [],
   sender_email: selfUser.email,
   sender_full_name: selfUser.full_name,

src/chat/__tests__/narrowsSelectors-test.js

@@ -25,9 +25,7 @@ describe('getMessagesForNarrow', () => {
     // them to be numbers.
     [123]: message /* eslint-disable-line no-useless-computed-key */,
   };
-  const outboxMessage = eg.makeOutboxMessage({
-    narrow: HOME_NARROW,
-  });
+  const outboxMessage = eg.makeOutboxMessage({});
 
   test('if no outbox messages returns messages with no change', () => {
     const state = eg.reduxState({
@@ -36,6 +34,7 @@ describe('getMessagesForNarrow', () => {
       },
       messages,
       outbox: [],
+      realm: eg.realmState({ email: eg.selfUser.email }),
     });
 
     const result = getMessagesForNarrow(state, HOME_NARROW);
@@ -53,6 +52,7 @@ describe('getMessagesForNarrow', () => {
       caughtUp: {
         [HOME_NARROW_STR]: { older: false, newer: true },
       },
+      realm: eg.realmState({ email: eg.selfUser.email }),
     });
 
     const result = getMessagesForNarrow(state, HOME_NARROW);
@@ -67,6 +67,7 @@ describe('getMessagesForNarrow', () => {
       },
       messages,
       outbox: [outboxMessage],
+      realm: eg.realmState({ email: eg.selfUser.email }),
     });
 
     const result = getMessagesForNarrow(state, HOME_NARROW);
@@ -80,7 +81,8 @@ describe('getMessagesForNarrow', () => {
         [JSON.stringify(privateNarrow('[email protected]'))]: [123],
       },
       messages,
-      outbox: [{ ...outboxMessage, narrow: streamNarrow('denmark') }],
+      outbox: [outboxMessage],
+      realm: eg.realmState({ email: eg.selfUser.email }),
     });
 
     const result = getMessagesForNarrow(state, privateNarrow('[email protected]'));

src/chat/narrowsReducer.js

@@ -47,7 +47,11 @@ const eventNewMessage = (state, action) => {
   let stateChange = false;
   const newState: NarrowsState = {};
   Object.keys(state).forEach(key => {
-    const isInNarrow = isMessageInNarrow(action.message, JSON.parse(key), action.ownEmail);
+    const { flags } = action.message;
+    if (!flags) {
+      throw new Error('EVENT_NEW_MESSAGE message missing flags');
+    }
+    const isInNarrow = isMessageInNarrow(action.message, flags, JSON.parse(key), action.ownEmail);
     const isCaughtUp = action.caughtUp[key] && action.caughtUp[key].newer;
     const messageDoesNotExist = state[key].find(id => action.message.id === id) === undefined;
 

src/chat/narrowsSelectors.js

@@ -21,12 +21,12 @@ import {
   getOutbox,
 } from '../directSelectors';
 import { getCaughtUpForNarrow } from '../caughtup/caughtUpSelectors';
-import { getAllUsersByEmail } from '../users/userSelectors';
+import { getAllUsersByEmail, getOwnEmail } from '../users/userSelectors';
 import {
   isPrivateNarrow,
   isStreamOrTopicNarrow,
   emailsOfGroupNarrow,
-  narrowContains,
+  isMessageInNarrow,
 } from '../utils/narrow';
 import { shouldBeMuted } from '../utils/message';
 import { NULL_ARRAY, NULL_SUBSCRIPTION } from '../nullObjects';
@@ -35,11 +35,22 @@ export const outboxMessagesForNarrow: Selector<Outbox[], Narrow> = createSelecto
   (state, narrow) => narrow,
   getCaughtUpForNarrow,
   state => getOutbox(state),
-  (narrow, caughtUp, outboxMessages) => {
+  getOwnEmail,
+  (narrow, caughtUp, outboxMessages, ownEmail) => {
     if (!caughtUp.newer) {
       return NULL_ARRAY;
     }
-    const filtered = outboxMessages.filter(item => narrowContains(narrow, item.narrow));
+    // TODO?: Handle @-mention flags in outbox messages.  As is, if you
+    //   @-mention yourself (or a wildcard) and then go look at the
+    //   is:mentioned view while your message is still unsent, we wrongly
+    //   leave it out.  Pretty uncommon edge case, though.
+    //
+    // No other narrows rely on flags except the "starred" narrow.  Outbox
+    // messages can't be starred, so "no flags" gives that the right answer.
+    const fakeFlags = [];
+    const filtered = outboxMessages.filter(message =>
+      isMessageInNarrow(message, fakeFlags, narrow, ownEmail),
+    );
     return isEqual(filtered, outboxMessages) ? outboxMessages : filtered;
   },
 );

src/events/doEventActionSideEffects.js

@@ -35,7 +35,7 @@ const messageEvent = (state: GlobalState, message: Message): void => {
     activeAccount
     && narrow !== undefined // chat screen is not at top
     && !isHomeNarrow(narrow)
-    && isMessageInNarrow(message, narrow, activeAccount.email);
+    && isMessageInNarrow(message, flags, narrow, activeAccount.email);
   const isSenderSelf = getOwnEmail(state) === message.sender_email;
   if (!isUserInSameNarrow && !isSenderSelf) {
     playMessageSound();

src/outbox/outboxActions.js

@@ -9,7 +9,7 @@ import type {
   NamedUser,
   Narrow,
   Outbox,
-  User,
+  UserOrBot,
   Action,
 } from '../types';
 import {
@@ -20,11 +20,10 @@ import {
 } from '../actionConstants';
 import { getAuth } from '../selectors';
 import * as api from '../api';
-import { getSelfUserDetail, getUsersByEmail } from '../users/userSelectors';
+import { getAllUsersByEmail, getOwnUser } from '../users/userSelectors';
 import { getUsersAndWildcards } from '../users/userHelpers';
-import { isStreamNarrow, isPrivateOrGroupNarrow } from '../utils/narrow';
+import { caseNarrowPartial } from '../utils/narrow';
 import { BackoffMachine } from '../utils/async';
-import { NULL_USER } from '../nullObjects';
 
 export const messageSendStart = (outbox: Outbox): Action => ({
   type: MESSAGE_SEND_START,
@@ -64,17 +63,16 @@ export const trySendMessages = (dispatch: Dispatch, getState: GetState): boolean
         return; // i.e., continue
       }
 
-      const to = ((): string => {
-        const { narrow } = item;
-        // TODO: can this test be `if (item.type === private)`?
-        if (isPrivateOrGroupNarrow(narrow)) {
-          return narrow[0].operand;
-        } else {
-          // HACK: the server attempts to interpret this argument as JSON, then
-          // CSV, then a literal. To avoid misparsing, always use JSON.
-          return JSON.stringify([item.display_recipient]);
-        }
-      })();
+      // prettier-ignore
+      const to =
+        item.type === 'private'
+            // TODO(server-2.0): switch to numeric user IDs, not emails.
+          ? item.display_recipient.map(r => r.email).join(',')
+            // TODO(server-2.0): switch to numeric stream IDs, not names.
+            //   (This will require wiring the stream ID through to here.)
+            // HACK: the server attempts to interpret this argument as JSON, then
+            //   CSV, then a literal. To avoid misparsing, always use JSON.
+          : JSON.stringify([item.display_recipient]);
 
       await api.sendMessage(auth, {
         type: item.type,
@@ -106,14 +104,21 @@ export const sendOutbox = () => async (dispatch: Dispatch, getState: GetState) =
   dispatch(toggleOutboxSending(false));
 };
 
-const mapEmailsToUsers = (usersByEmail, narrow, selfDetail) =>
-  narrow[0].operand
-    .split(',')
-    .map(item => {
-      const user = usersByEmail.get(item) || NULL_USER;
-      return { email: item, id: user.user_id, full_name: user.full_name };
-    })
-    .concat({ email: selfDetail.email, id: selfDetail.user_id, full_name: selfDetail.full_name });
+// A valid display_recipient with all the thread's users, sorted by ID.
+const mapEmailsToUsers = (emails, allUsersByEmail, ownUser) => {
+  const result = emails.map(email => {
+    const user = allUsersByEmail.get(email);
+    if (!user) {
+      throw new Error('outbox: missing user when preparing to send PM');
+    }
+    return { email, id: user.user_id, full_name: user.full_name };
+  });
+  if (!result.some(r => r.id === ownUser.user_id)) {
+    result.push({ email: ownUser.email, id: ownUser.user_id, full_name: ownUser.full_name });
+  }
+  result.sort((r1, r2) => r1.id - r2.id);
+  return result;
+};
 
 // TODO type: `string | NamedUser[]` is a bit confusing.
 type DataFromNarrow = {|
@@ -124,19 +129,28 @@ type DataFromNarrow = {|
 
 const extractTypeToAndSubjectFromNarrow = (
   narrow: Narrow,
-  usersByEmail: Map<string, User>,
-  selfDetail: { email: string, user_id: number, full_name: string },
+  allUsersByEmail: Map<string, UserOrBot>,
+  ownUser: UserOrBot,
 ): DataFromNarrow => {
-  if (isPrivateOrGroupNarrow(narrow)) {
-    return {
-      type: 'private',
-      display_recipient: mapEmailsToUsers(usersByEmail, narrow, selfDetail),
-      subject: '',
-    };
-  } else if (isStreamNarrow(narrow)) {
-    return { type: 'stream', display_recipient: narrow[0].operand, subject: '(no topic)' };
-  }
-  return { type: 'stream', display_recipient: narrow[0].operand, subject: narrow[1].operand };
+  const forPm = emails => ({
+    type: 'private',
+    display_recipient: mapEmailsToUsers(emails, allUsersByEmail, ownUser),
+    subject: '',
+  });
+  return caseNarrowPartial(narrow, {
+    pm: email => forPm([email]),
+    groupPm: forPm,
+
+    // TODO: we shouldn't ever be passing a whole-stream narrow here;
+    //   ensure we don't, then remove this case
+    stream: name => ({ type: 'stream', display_recipient: name, subject: '(no topic)' }),
+
+    topic: (streamName, topic) => ({
+      type: 'stream',
+      display_recipient: streamName,
+      subject: topic,
+    }),
+  });
 };
 
 const getContentPreview = (content: string, state: GlobalState): string => {
@@ -159,21 +173,20 @@ export const addToOutbox = (narrow: Narrow, content: string) => async (
   getState: GetState,
 ) => {
   const state = getState();
-  const userDetail = getSelfUserDetail(state);
+  const ownUser = getOwnUser(state);
 
   const localTime = Math.round(new Date().getTime() / 1000);
   dispatch(
     messageSendStart({
-      narrow,
       isSent: false,
-      ...extractTypeToAndSubjectFromNarrow(narrow, getUsersByEmail(state), userDetail),
+      ...extractTypeToAndSubjectFromNarrow(narrow, getAllUsersByEmail(state), ownUser),
       markdownContent: content,
       content: getContentPreview(content, state),
       timestamp: localTime,
       id: localTime,
-      sender_full_name: userDetail.full_name,
-      sender_email: userDetail.email,
-      avatar_url: userDetail.avatar_url,
+      sender_full_name: ownUser.full_name,
+      sender_email: ownUser.email,
+      avatar_url: ownUser.avatar_url,
       isOutbox: true,
       reactions: [],
     }),

src/topics/__tests__/topicsSelectors-test.js

@@ -31,6 +31,7 @@ describe('getLastMessageTopic', () => {
   test('when no messages in narrow return an empty string', () => {
     const state = eg.reduxState({
       narrows: {},
+      realm: eg.realmState({ email: eg.selfUser.email }),
     });
 
     const topic = getLastMessageTopic(state, HOME_NARROW);
@@ -50,6 +51,7 @@ describe('getLastMessageTopic', () => {
         [message1.id]: message1,
         [message2.id]: message2,
       },
+      realm: eg.realmState({ email: eg.selfUser.email }),
     });
 
     const topic = getLastMessageTopic(state, narrow);

src/types.js

@@ -2,7 +2,7 @@
 import type { IntlShape } from 'react-intl';
 import type { DangerouslyImpreciseStyleProp } from 'react-native/Libraries/StyleSheet/StyleSheet';
 
-import type { Auth, Topic, Message, Reaction, ReactionType, Narrow } from './api/apiTypes';
+import type { Auth, Topic, Message, Reaction, ReactionType } from './api/apiTypes';
 import type { ZulipVersion } from './utils/zulipVersion';
 
 export type * from './generics';
@@ -171,10 +171,9 @@ export type Outbox = {|
    */
   isSent: boolean,
 
-  // These fields don't exist in `Message`.
-  // They're used for sending the message to the server.
+  // `markdownContent` doesn't exist in `Message`.
+  // It's used for sending the message to the server.
   markdownContent: string,
-  narrow: Narrow,
 
   // These fields are modeled on `Message`.
   avatar_url: string | null,

src/utils/__tests__/narrow-test.js

@@ -289,19 +289,12 @@ describe('isMessageInNarrow', () => {
       for (const [messageDescription, expected, message] of cases) {
         test(`${expected ? 'contains' : 'excludes'} ${messageDescription}`, () => {
           expect(
-            isMessageInNarrow({ ...message, flags: message.flags ?? [] }, narrow, ownEmail),
+            isMessageInNarrow(message, message.flags ?? [], narrow, ownEmail),
           ).toBe(expected);
         });
       }
     });
   }
-
-  test('message with flags absent throws an error', () => {
-    const message = eg.streamMessage({
-      // no flags
-    });
-    expect(() => isMessageInNarrow(message, MENTIONED_NARROW, ownEmail)).toThrow();
-  });
 });
 
 describe('getNarrowFromMessage', () => {

src/utils/narrow.js

@@ -248,8 +248,23 @@ export const isStreamOrTopicNarrow = (narrow?: Narrow): boolean =>
 export const isSearchNarrow = (narrow?: Narrow): boolean =>
   !!narrow && caseNarrowDefault(narrow, { search: () => true }, () => false);
 
-/** (For search narrows, just returns false.) */
-export const isMessageInNarrow = (message: Message, narrow: Narrow, ownEmail: string): boolean => {
+/**
+ * True just if the given message is part of the given narrow.
+ *
+ * This function does not support search narrows, and for them always
+ * returns false.
+ *
+ * The message's flags must be in `flags`; `message.flags` is ignored.  This
+ * makes it the caller's responsibility to deal with the ambiguity in our
+ * Message type of whether the message's flags live in a `flags` property or
+ * somewhere else.
+ */
+export const isMessageInNarrow = (
+  message: Message | Outbox,
+  flags: $ReadOnlyArray<string>,
+  narrow: Narrow,
+  ownEmail: string,
+): boolean => {
   const matchPmRecipients = (emails: string[]) => {
     if (message.type !== 'private') {
       return false;
@@ -262,11 +277,6 @@ export const isMessageInNarrow = (message: Message, narrow: Narrow, ownEmail: st
     );
   };
 
-  const { flags } = message;
-  if (!flags) {
-    throw new Error('`message.flags` should be defined.');
-  }
-
   return caseNarrow(narrow, {
     home: () => true,
     stream: name => name === message.display_recipient,
@@ -294,20 +304,6 @@ export const canSendToNarrow = (narrow: Narrow): boolean =>
     search: () => false,
   });
 
-/** True just if `haystack` contains all possible messages in `needle`. */
-export const narrowContains = (haystack: Narrow, needle: Narrow): boolean => {
-  if (isHomeNarrow(haystack)) {
-    return true;
-  }
-  if (isAllPrivateNarrow(haystack) && isPrivateOrGroupNarrow(needle)) {
-    return true;
-  }
-  if (isStreamNarrow(haystack) && needle[0].operand === haystack[0].operand) {
-    return true;
-  }
-  return JSON.stringify(needle) === JSON.stringify(haystack);
-};
-
 export const getNarrowFromMessage = (message: Message | Outbox, ownEmail: string) => {
   if (Array.isArray(message.display_recipient)) {
     const recipient =