api: Stop assuming fetched messages' reactions have deprecated user…

… field

Our code at the edge for transforming the old format of reactions,
with `user`, into the new format, with `user_id`, was depending on
`user` always being present.

Since `user_id` is documented as being added in Zulip 3.0:
  https://zulip.com/api/get-messages#response
, and since zulip#5747 we've been refusing to connect to servers pre-3.0,
we can assume `user_id` is always present and stop reading `user`.

Fixes: zulip#4072

src/api/__tests__/rawModelTypes-test.js

@@ -1,31 +1,14 @@
 /* @flow strict-local */
-import {
-  transformFetchedMessage,
-  type FetchedMessage,
-  type FetchedReaction,
-} from '../rawModelTypes';
+import { transformFetchedMessage, type FetchedMessage } from '../rawModelTypes';
 import { identityOfAuth } from '../../account/accountMisc';
 import * as eg from '../../__tests__/lib/exampleData';
 import type { Message } from '../modelTypes';
 import { GravatarURL } from '../../utils/avatar';
 
 describe('transformFetchedMessage', () => {
-  const reactingUser = eg.makeUser();
-
-  const serverReaction: FetchedReaction = {
-    emoji_name: '+1',
-    reaction_type: 'unicode_emoji',
-    emoji_code: '1f44d',
-    user: {
-      email: reactingUser.email,
-      full_name: reactingUser.full_name,
-      id: reactingUser.user_id,
-    },
-  };
-
   const fetchedMessage: FetchedMessage = {
     ...eg.streamMessage(),
-    reactions: [serverReaction],
+    reactions: [eg.unicodeEmojiReaction],
     avatar_url: null,
     edit_history: [
       {
@@ -46,14 +29,6 @@ describe('transformFetchedMessage', () => {
 
     const expectedOutput: Message = {
       ...fetchedMessage,
-      reactions: [
-        {
-          user_id: reactingUser.user_id,
-          emoji_name: serverReaction.emoji_name,
-          reaction_type: serverReaction.reaction_type,
-          emoji_code: serverReaction.emoji_code,
-        },
-      ],
       avatar_url: GravatarURL.validateAndConstructInstance({ email: fetchedMessage.sender_email }),
       edit_history:
         // $FlowIgnore[incompatible-cast] - See MessageEdit type
@@ -67,10 +42,6 @@ describe('transformFetchedMessage', () => {
       true,
     );
 
-    test('In reactions, replace user object with `user_id`', () => {
-      expect(actualOutput.reactions).toEqual(expectedOutput.reactions);
-    });
-
     test('Converts avatar_url correctly', () => {
       expect(actualOutput.avatar_url).toEqual(expectedOutput.avatar_url);
     });

src/api/rawModelTypes.js

@@ -1,35 +1,9 @@
 /* @flow strict-local */
 
 import type { Identity } from '../types';
-import type {
-  PmMessage,
-  StreamMessage,
-  Message,
-  MessageEdit,
-  Reaction,
-  UserId,
-} from './modelTypes';
+import type { PmMessage, StreamMessage, Message, MessageEdit, UserId } from './modelTypes';
 import { AvatarURL } from '../utils/avatar';
 
-/**
- * The variant of `Reaction` found in a fetch-message(s) response.
- *
- * Note that reaction events have a *different* variation; see their
- * handling in `eventToAction`.
- */
-// We shouldn't have to rely on this format on servers at feature
-// level 2+; those newer servers include a top-level `user_id` field
-// in addition to the `user` object. See #4072.
-// TODO(server-3.0): Simplify this away.
-export type FetchedReaction = $ReadOnly<{|
-  ...$Diff<Reaction, {| user_id: mixed |}>,
-  user: $ReadOnly<{|
-    email: string,
-    full_name: string,
-    id: UserId,
-  |}>,
-|}>;
-
 /**
  * The elements of Message.edit_history found in a fetch-message(s) response.
  *
@@ -56,7 +30,6 @@ export type FetchedMessageEdit = $ReadOnly<{|
 type FetchedMessageOf<M: Message> = $ReadOnly<{|
   ...$Exact<M>,
   avatar_url: string | null,
-  reactions: $ReadOnlyArray<FetchedReaction>,
 
   // Unlike Message['edit_history'], this can't be `null`.
   edit_history?: $ReadOnlyArray<FetchedMessageEdit>,
@@ -80,13 +53,6 @@ export const transformFetchedMessage = <M: Message>(
     userId: message.sender_id,
     realm: identity.realm,
   }),
-  reactions: message.reactions.map(reaction => {
-    const { user, ...restReaction } = reaction;
-    return {
-      ...restReaction,
-      user_id: user.id,
-    };
-  }),
 
   // Why condition on allowEditHistory? See MessageBase['edit_history'].
   // Why FL 118 condition? See MessageEdit type.

src/message/__tests__/fetchActions-test.js

@@ -340,48 +340,39 @@ describe('fetchActions', () => {
       });
 
       test("rejects when validation-at-the-edge can't handle data, dispatches MESSAGE_FETCH_ERROR", async () => {
+        // Regression test for #4156.  There was a server bug that caused
+        // message data to be malformed, and though it was only briefly in
+        // main, a user did run into it in real life:
+        //   https://github.com/zulip/zulip-mobile/issues/4156#issuecomment-655905093
+        //
         // This validation is done in transformFetchedMessages in
         // rawModelTypes.
-        //
-        // Simulate #4156, a real-life problem that a user at server commit
-        // 0af2f9d838 ran into [1], by having `user` be missing on reactions
-        // on a message:
-        //   https://github.com/zulip/zulip-mobile/issues/4156#issuecomment-655905093
-        const store = mockStore<GlobalState, Action>(baseState);
 
-        // Missing `user` (and `user_id`, for good measure), to
-        // simulate #4156.
-        const faultyReaction = {
-          reaction_type: 'unicode_emoji',
-          emoji_code: '1f44d',
-          emoji_name: 'thumbs_up',
-        };
+        const store = mockStore<GlobalState, Action>(baseState);
 
         const response = {
-          // Flow would complain at `faultyReaction` if it
+          // Flow would complain at `sender_email` if it
           // type-checked `response`, but we should ignore it if that
-          // day comes. It's badly shaped on purpose.
-          messages: [{ ...fetchedMessage1, reactions: [faultyReaction] }],
+          // day comes. It's badly typed on purpose.
+          messages: [{ ...fetchedMessage1, avatar_url: null, sender_email: undefined }],
           result: 'success',
         };
         // $FlowFixMe[prop-missing]: See mock in jest/globalFetch.js.
         fetch.mockResponseSuccess(JSON.stringify(response));
 
+        const expectedError = Error("Cannot read properties of undefined (reading 'toLowerCase')");
+
         await expect(
           store.dispatch(
             fetchMessages({ narrow: HOME_NARROW, anchor: 0, numBefore: 1, numAfter: 1 }),
           ),
-        ).rejects.toThrow(
-          // Update this with changes to the error type.
-          TypeError,
-        );
+        ).rejects.toThrow(expectedError);
 
         const actions = store.getActions();
 
         expect(actions[actions.length - 1]).toMatchObject({
           type: 'MESSAGE_FETCH_ERROR',
-          // Update this with changes to the error type.
-          error: expect.any(TypeError),
+          error: expectedError,
         });
       });