Make PmMessage and StreamMessage types.

src/__tests__/lib/exampleData.js

@@ -6,6 +6,8 @@ import Immutable from 'immutable';
 import type {
   CrossRealmBot,
   Message,
+  PmMessage,
+  StreamMessage,
   PmRecipientUser,
   Reaction,
   Stream,
@@ -296,7 +298,6 @@ const messagePropertiesBase = deepFreeze({
   is_me_message: false,
   // last_edit_timestamp omitted
   reactions: [],
-  subject_links: [],
   submessages: [],
 });
 
@@ -324,11 +325,11 @@ const randMessageId: () => number = makeUniqueRandInt('message ID', 10000000);
  * Beware! These values may not be representative.
  */
 export const pmMessage = (args?: {|
-  ...$Rest<Message, { ... }>,
+  ...$Rest<PmMessage, { ... }>,
   sender?: User,
   recipients?: User[],
   sender_id?: number, // accept a plain number, for convenience in tests
-|}): Message => {
+|}): PmMessage => {
   // The `Object.freeze` is to work around a Flow issue:
   //   https://github.com/facebook/flow/issues/2386#issuecomment-695064325
   const {
@@ -338,7 +339,7 @@ export const pmMessage = (args?: {|
     ...extra
   } = args ?? Object.freeze({});
 
-  const baseMessage: Message = {
+  const baseMessage: PmMessage = {
     ...messagePropertiesBase,
     ...messagePropertiesFromSender(sender),
 
@@ -348,8 +349,6 @@ export const pmMessage = (args?: {|
     // in real messages.  (See comments on the Message type.)
     display_recipient: recipients.map(displayRecipientFromUser),
     id: randMessageId(),
-    recipient_id: 2342,
-    stream_id: -1,
     subject: '',
     timestamp: 1556579160,
     type: 'private',
@@ -361,13 +360,15 @@ export const pmMessage = (args?: {|
   });
 };
 
-export const pmMessageFromTo = (from: User, to: User[], extra?: $Rest<Message, { ... }>): Message =>
-  pmMessage({ sender: from, recipients: [from, ...to], ...extra });
+export const pmMessageFromTo = (
+  from: User,
+  to: User[],
+  extra?: $Rest<PmMessage, { ... }>,
+): PmMessage => pmMessage({ sender: from, recipients: [from, ...to], ...extra });
 
 const messagePropertiesFromStream = (stream1: Stream) => {
   const { stream_id, name: display_recipient } = stream1;
   return deepFreeze({
-    recipient_id: 2567,
     display_recipient,
     stream_id,
   });
@@ -379,15 +380,15 @@ const messagePropertiesFromStream = (stream1: Stream) => {
  * Beware! These values may not be representative.
  */
 export const streamMessage = (args?: {|
-  ...$Rest<Message, { ... }>,
+  ...$Rest<StreamMessage, { ... }>,
   stream?: Stream,
   sender?: User,
-|}): Message => {
+|}): StreamMessage => {
   // The `Object.freeze` is to work around a Flow issue:
   //   https://github.com/facebook/flow/issues/2386#issuecomment-695064325
   const { stream: streamInner = stream, sender = otherUser, ...extra } = args ?? Object.freeze({});
 
-  const baseMessage: Message = {
+  const baseMessage: StreamMessage = {
     ...messagePropertiesBase,
     ...messagePropertiesFromSender(sender),
     ...messagePropertiesFromStream(streamInner),
@@ -396,6 +397,7 @@ export const streamMessage = (args?: {|
     content_type: 'text/markdown',
     id: randMessageId(),
     subject: 'example topic',
+    subject_links: [],
     timestamp: 1556579727,
     type: 'stream',
   };

src/api/messages/getMessages.js

@@ -2,7 +2,7 @@
 import type { Auth, ApiResponseSuccess } from '../transportTypes';
 import type { Identity } from '../../types';
 import type { Message, ApiNarrow } from '../apiTypes';
-import type { Reaction, UserId } from '../modelTypes';
+import type { PmMessage, StreamMessage, Reaction, UserId } from '../modelTypes';
 import { apiGet } from '../apiFetch';
 import { identityOfAuth } from '../../account/accountMisc';
 import { AvatarURL } from '../../utils/avatar';
@@ -31,12 +31,16 @@ export type ServerReaction = $ReadOnly<{|
   |}>,
 |}>;
 
-export type ServerMessage = $ReadOnly<{|
-  ...$Exact<Message>,
+// How `ServerMessage` relates to `Message`, in a way that applies
+// uniformly to `Message`'s subtypes.
+type ServerMessageOf<M: Message> = $ReadOnly<{|
+  ...$Exact<M>,
   avatar_url: string | null,
   reactions: $ReadOnlyArray<ServerReaction>,
 |}>;
 
+export type ServerMessage = ServerMessageOf<PmMessage> | ServerMessageOf<StreamMessage>;
+
 // The actual response from the server.  We convert the data from this to
 // `ApiResponseMessages` before returning it to application code.
 type ServerApiResponseMessages = {|
@@ -45,27 +49,26 @@ type ServerApiResponseMessages = {|
 |};
 
 /** Exported for tests only. */
-export const migrateMessages = (messages: ServerMessage[], identity: Identity): Message[] =>
-  messages.map(message => {
-    const { reactions, avatar_url: rawAvatarUrl, ...restMessage } = message;
-
-    return {
-      ...restMessage,
-      avatar_url: AvatarURL.fromUserOrBotData({
-        rawAvatarUrl,
-        email: message.sender_email,
-        userId: message.sender_id,
-        realm: identity.realm,
-      }),
-      reactions: reactions.map(reaction => {
-        const { user, ...restReaction } = reaction;
-        return {
-          ...restReaction,
-          user_id: user.id,
-        };
-      }),
-    };
-  });
+export const migrateMessages = (
+  messages: $ReadOnlyArray<ServerMessage>,
+  identity: Identity,
+): Message[] =>
+  messages.map(<M: Message>(message: ServerMessageOf<M>): M => ({
+    ...message,
+    avatar_url: AvatarURL.fromUserOrBotData({
+      rawAvatarUrl: message.avatar_url,
+      email: message.sender_email,
+      userId: message.sender_id,
+      realm: identity.realm,
+    }),
+    reactions: message.reactions.map(reaction => {
+      const { user, ...restReaction } = reaction;
+      return {
+        ...restReaction,
+        user_id: user.id,
+      };
+    }),
+  }));
 
 const migrateResponse = (response, identity: Identity) => {
   const { messages, ...restResponse } = response;

src/api/modelTypes.js

@@ -445,48 +445,10 @@ export type Submessage = $ReadOnly<{|
 |}>;
 
 /**
- * A Zulip message.
- *
- * This type is mainly intended to represent the data the server sends as
- * the `message` property of an event of type `message`.  Caveat lector: we
- * pass these around to a lot of places, and do a lot of further munging, so
- * this type may not quite represent that.  Any differences should
- * definitely be commented, and perhaps refactored.
- *
- * The server's behavior here is undocumented and the source is very
- * complex; this is naturally a place where a large fraction of all the
- * features of Zulip have to appear.
- *
- * Major appearances of this type include
- *  * `message: Message` on a server event of type `message`, and our
- *    `EVENT_NEW_MESSAGE` Redux action for the event;
- *  * `messages: Message[]` in a `/messages` (our `getMessages`) response,
- *    and our resulting `MESSAGE_FETCH_COMPLETE` Redux action;
- *  * `messages: {| [id]: Message |}` in our global Redux state.
- *
- * References include:
- *  * the two example events at https://zulip.com/api/get-events-from-queue
- *  * `process_message_event` in zerver/tornado/event_queue.py; the call
- *    `client.add_event(user_event)` makes the final determination of what
- *    goes into the event, so `message_dict` is the final value of `message`
- *  * `MessageDict.wide_dict` and its helpers in zerver/lib/message.py;
- *    via `do_send_messages` in `zerver/lib/actions.py`, these supply most
- *    of the data ultimately handled by `process_message_event`
- *  * `messages_for_ids` and its helpers in zerver/lib/message.py; via
- *    `get_messages_backend`, these supply the data ultimately returned by
- *    `/messages`
- *  * the `Message` and `AbstractMessage` models in zerver/models.py, but
- *    with caution; many fields are adjusted between the DB row and the event
- *  * empirical study looking at Redux events logged [to the
- *    console](docs/howto/debugging.md).
- *
- * See also `Outbox`, which is deliberately similar so that we can use
- * the type `Message | Outbox` in many places.
- *
- * See also `MessagesState` for discussion of how we fetch and store message
- * data.
+ * Properties in common among the two different flavors of a
+ * `Message`: `PmMessage` and `StreamMessage`.
  */
-export type Message = $ReadOnly<{|
+type MessageBase = $ReadOnly<{|
   /** Our own flag; if true, really type `Outbox`. */
   isOutbox: false,
 
@@ -544,10 +506,14 @@ export type Message = $ReadOnly<{|
 
   timestamp: number,
 
-  //
-  // Properties that behave differently for stream vs. private messages.
+  /** Deprecated; a server implementation detail not useful in a client. */
+  // recipient_id: number,
+|}>;
 
-  type: 'stream' | 'private',
+export type PmMessage = $ReadOnly<{|
+  ...MessageBase,
+
+  type: 'private',
 
   // Notes from studying the server code:
   //  * Notes are primarily from the server as of 2020-04 at cb85763c7, but
@@ -567,27 +533,81 @@ export type Message = $ReadOnly<{|
   //      it sorted by user ID; so, best not to assume current behavior.
   //
   /**
-   * The set of all users in the thread, for a PM; else the stream name.
+   * The set of all users in the thread.
    *
-   * For a private message, this lists the sender as well as all (other)
-   * recipients, and it lists each user just once.  In particular the
-   * self-user is always included.
+   * This lists the sender as well as all (other) recipients, and it
+   * lists each user just once.  In particular the self-user is always
+   * included.
    *
    * The ordering is less well specified; if it matters, sort first.
-   *
-   * For stream messages, prefer `stream_id`; see #3918.
    */
-  display_recipient: string | $ReadOnlyArray<PmRecipientUser>, // `string` for type stream, else PmRecipientUser[]
+  display_recipient: $ReadOnlyArray<PmRecipientUser>,
 
-  /** Deprecated; a server implementation detail not useful in a client. */
-  recipient_id: number,
+  subject: '',
+|}>;
+
+export type StreamMessage = $ReadOnly<{|
+  ...MessageBase,
 
-  stream_id: number, // FixMe: actually only for type `stream`, else absent.
+  type: 'stream',
+
+  /**
+   * The stream name.
+   *
+   * Prefer `stream_id`; see #3918.
+   */
+  display_recipient: string,
+
+  stream_id: number,
 
   subject: string,
   subject_links: $ReadOnlyArray<string>,
 |}>;
 
+/**
+ * A Zulip message.
+ *
+ * This type is mainly intended to represent the data the server sends as
+ * the `message` property of an event of type `message`.  Caveat lector: we
+ * pass these around to a lot of places, and do a lot of further munging, so
+ * this type may not quite represent that.  Any differences should
+ * definitely be commented, and perhaps refactored.
+ *
+ * The server's behavior here is undocumented and the source is very
+ * complex; this is naturally a place where a large fraction of all the
+ * features of Zulip have to appear.
+ *
+ * Major appearances of this type include
+ *  * `message: Message` on a server event of type `message`, and our
+ *    `EVENT_NEW_MESSAGE` Redux action for the event;
+ *  * `messages: Message[]` in a `/messages` (our `getMessages`) response,
+ *    and our resulting `MESSAGE_FETCH_COMPLETE` Redux action;
+ *  * `messages: {| [id]: Message |}` in our global Redux state.
+ *
+ * References include:
+ *  * the two example events at https://zulip.com/api/get-events-from-queue
+ *  * `process_message_event` in zerver/tornado/event_queue.py; the call
+ *    `client.add_event(user_event)` makes the final determination of what
+ *    goes into the event, so `message_dict` is the final value of `message`
+ *  * `MessageDict.wide_dict` and its helpers in zerver/lib/message.py;
+ *    via `do_send_messages` in `zerver/lib/actions.py`, these supply most
+ *    of the data ultimately handled by `process_message_event`
+ *  * `messages_for_ids` and its helpers in zerver/lib/message.py; via
+ *    `get_messages_backend`, these supply the data ultimately returned by
+ *    `/messages`
+ *  * the `Message` and `AbstractMessage` models in zerver/models.py, but
+ *    with caution; many fields are adjusted between the DB row and the event
+ *  * empirical study looking at Redux events logged [to the
+ *    console](docs/howto/debugging.md).
+ *
+ * See also `Outbox`, which is deliberately similar so that we can use
+ * the type `Message | Outbox` in many places.
+ *
+ * See also `MessagesState` for discussion of how we fetch and store message
+ * data.
+ */
+export type Message = PmMessage | StreamMessage;
+
 //
 //
 //

src/message/__tests__/fetchActions-test.js

@@ -2,7 +2,6 @@
 import invariant from 'invariant';
 import configureStore from 'redux-mock-store';
 import thunk from 'redux-thunk';
-import omit from 'lodash.omit';
 import Immutable from 'immutable';
 
 import type { GlobalState } from '../../reduxTypes';
@@ -15,7 +14,6 @@ import {
   tryFetch,
 } from '../fetchActions';
 import { FIRST_UNREAD_ANCHOR } from '../../anchor';
-import type { Message } from '../../api/modelTypes';
 import type { ServerMessage } from '../../api/messages/getMessages';
 import { streamNarrow, HOME_NARROW, HOME_NARROW_STR, keyFromNarrow } from '../../utils/narrow';
 import { GravatarURL } from '../../utils/avatar';
@@ -118,15 +116,13 @@ describe('fetchActions', () => {
     };
     const message1 = eg.streamMessage({ id: 1, sender });
 
-    type CommonFields = $Diff<Message, {| reactions: mixed, avatar_url: mixed |}>;
-
     // message1 exactly as we receive it from the server, before our
     // own transformations.
     //
     // TODO: Deduplicate this logic with similar logic in
     // migrateMessages-test.js.
     const serverMessage1: ServerMessage = {
-      ...(omit(message1, 'reactions', 'avatar_url'): CommonFields),
+      ...message1,
       reactions: [],
       avatar_url: null, // Null in server data will be transformed to a GravatarURL
     };