messages state: Keep edit_history null if allowEditHistory is false

Author: chrisbobbe

src/api/messages/__tests__/migrateMessages-test.js

@@ -62,6 +62,7 @@ describe('recent server', () => {
     input,
     identityOfAuth(eg.selfAuth),
     eg.recentZulipFeatureLevel,
+    true,
   );
 
   test('In reactions, replace user object with `user_id`', () => {
@@ -72,9 +73,17 @@ describe('recent server', () => {
     expect(actualOutput.map(m => m.avatar_url)).toEqual(expectedOutput.map(m => m.avatar_url));
   });
 
-  test('Keeps edit_history', () => {
+  test('Keeps edit_history, if allowEditHistory is true', () => {
     expect(actualOutput.map(m => m.edit_history)).toEqual(expectedOutput.map(m => m.edit_history));
   });
+
+  test('Drops edit_history, if allowEditHistory is false', () => {
+    expect(
+      migrateMessages(input, identityOfAuth(eg.selfAuth), eg.recentZulipFeatureLevel, false).map(
+        m => m.edit_history,
+      ),
+    ).toEqual(expectedOutput.map(m => null));
+  });
 });
 
 describe('drops edit_history from pre-118 server', () => {
@@ -97,6 +106,7 @@ describe('drops edit_history from pre-118 server', () => {
       ],
       identityOfAuth(eg.selfAuth),
       117,
+      true,
     ).map(m => m.edit_history),
   ).toEqual([null]);
 });

src/api/messages/getMessages.js

@@ -79,6 +79,7 @@ export const migrateMessages = (
   messages: $ReadOnlyArray<ServerMessage>,
   identity: Identity,
   zulipFeatureLevel: number,
+  allowEditHistory: boolean,
 ): Message[] =>
   messages.map(<M: Message>(message: ServerMessageOf<M>): M => ({
     ...message,
@@ -95,20 +96,22 @@ export const migrateMessages = (
         user_id: user.id,
       };
     }),
+
+    // Why condition on allowEditHistory? See MessageBase['edit_history'].
     // Why FL 118 condition? See MessageEdit type.
     edit_history:
       /* eslint-disable operator-linebreak */
-      zulipFeatureLevel >= 118
+      allowEditHistory && zulipFeatureLevel >= 118
         ? // $FlowIgnore[incompatible-cast] - See MessageEdit type
           (message.edit_history: $ReadOnlyArray<MessageEdit> | void)
         : null,
   }));
 
-const migrateResponse = (response, identity: Identity, zulipFeatureLevel) => {
+const migrateResponse = (response, identity: Identity, zulipFeatureLevel, allowEditHistory) => {
   const { messages, ...restResponse } = response;
   return {
     ...restResponse,
-    messages: migrateMessages(messages, identity, zulipFeatureLevel),
+    messages: migrateMessages(messages, identity, zulipFeatureLevel, allowEditHistory),
   };
 };
 
@@ -127,6 +130,9 @@ export default async (
 
   // TODO(#4659): Don't get this from callers.
   zulipFeatureLevel: number,
+
+  // TODO(#4659): Don't get this from callers?
+  allowEditHistory: boolean,
 ): Promise<ApiResponseMessages> => {
   const { narrow, anchor, numBefore, numAfter, useFirstUnread = false } = args;
   const response: ServerApiResponseMessages = await apiGet(auth, 'messages', {
@@ -138,5 +144,5 @@ export default async (
     use_first_unread_anchor: useFirstUnread,
     client_gravatar: true,
   });
-  return migrateResponse(response, identityOfAuth(auth), zulipFeatureLevel);
+  return migrateResponse(response, identityOfAuth(auth), zulipFeatureLevel, allowEditHistory);
 };

src/api/modelTypes.js

@@ -750,22 +750,17 @@ type MessageBase = $ReadOnly<{|
   // display_recipient handled on PmMessage and StreamMessage separately
 
   /**
-   * A possibly incomplete view of the message's edit history.
+   * A view of the message's edit history, if available.
    *
    * This is null if it's coming from a server with FL <118; see comment on
    * MessageEdit.
    *
+   * Null if the realm doesn't allow viewing edit history.
+   *
    * Missing/undefined if the message had no edit history when we added it
    * to the state.
-   *
-   * Missing/undefined if, at the time we added it to the state, the realm
-   * didn't allow viewing edit history.
    */
-  // TODO: Keep reasonably current:
-  // - Use `null` for the `allow_edit_history`-false case, to distinguish it
-  //   from the message-never-edited case. (Handle everywhere we add/update
-  //   Messages in Redux: the get-messages response, new-message events,
-  //   update-message events.)
+  // TODO: Keep current:
   // - Handle changes to the allow_edit_history setting:
   //   - Treat an allow_edit_history change similar to a restart event (in
   //     particular, a restart event where the server version changed, which
@@ -775,8 +770,7 @@ type MessageBase = $ReadOnly<{|
   // TODO(server-5.0): Remove FL <118 condition
   //
   // (Why optional and `| void`? We convert missing to undefined at the
-  // edge, while doing the FL <118 condition, but that's incidental and
-  // could easily change.)
+  // edge, but that's incidental and could easily change.)
   edit_history?: $ReadOnlyArray<MessageEdit> | null | void,
 
   id: number,

src/events/__tests__/eventToAction-test.js

@@ -75,10 +75,26 @@ describe('eventToAction', () => {
         expect(action.message.avatar_url).toBeInstanceOf(AvatarURL);
       });
 
-      test('Keeps edit_history', () => {
+      test('Keeps edit_history if allowEditHistory is true', () => {
+        // eslint-disable-next-line no-shadow
+        const action = eventToAction(
+          eg.reduxStatePlus({ realm: { ...eg.plusReduxState.realm, allowEditHistory: true } }),
+          event,
+        );
+        invariant(actionMatchesType(action, EVENT_NEW_MESSAGE), 'EVENT_NEW_MESSAGE produced');
         expect(action.message.edit_history).not.toBeNull();
         expect(action.message.edit_history).toEqual(serverMessage.edit_history);
       });
+
+      test('Drops edit_history if allowEditHistory is false', () => {
+        // eslint-disable-next-line no-shadow
+        const action = eventToAction(
+          eg.reduxStatePlus({ realm: { ...eg.plusReduxState.realm, allowEditHistory: false } }),
+          event,
+        );
+        invariant(actionMatchesType(action, EVENT_NEW_MESSAGE), 'EVENT_NEW_MESSAGE produced');
+        expect(action.message.edit_history).toBeNull();
+      });
     });
 
     describe('edit_history for FL <118', () => {

src/events/eventToAction.js

@@ -31,6 +31,7 @@ import {
   EVENT_SUBSCRIPTION,
   EVENT,
 } from '../actionConstants';
+import { getRealm } from '../selectors';
 import { getOwnUserId, tryGetUserForId } from '../users/userSelectors';
 import { AvatarURL } from '../utils/avatar';
 import { getRealmUrl, getZulipFeatureLevel } from '../account/accountsSelectors';
@@ -91,6 +92,8 @@ const actionTypeOfEventType = {
 // assumptions about the events the server sends, and doesn't check them.
 export default (state: PerAccountState, event: $FlowFixMe): EventAction | null => {
   const zulipFeatureLevel = getZulipFeatureLevel(state);
+  const allowEditHistory = getRealm(state).allowEditHistory;
+
   const type = (event.type: EventType);
   switch (type) {
     // For reference on each type of event, see:
@@ -118,8 +121,9 @@ export default (state: PerAccountState, event: $FlowFixMe): EventAction | null =
             realm: getRealmUrl(state),
           }),
           edit_history:
+            // Why condition on allowEditHistory? See MessageBase['edit_history'].
             // Why FL 118 condition? See MessageEdit type.
-            zulipFeatureLevel >= 118
+            allowEditHistory && zulipFeatureLevel >= 118
               ? (event.message.edit_history: $ReadOnlyArray<MessageEdit> | void)
               : null,
         },

src/message/__tests__/fetchActions-test.js

@@ -254,6 +254,10 @@ describe('fetchActions', () => {
       narrows: Immutable.Map({
         [streamNarrowStr]: [message1.id],
       }),
+      realm: {
+        ...eg.plusReduxState.realm,
+        allowEditHistory: true, // TODO: test with this `false`
+      },
     });
 
     describe('success', () => {

src/message/__tests__/messagesReducer-test.js

@@ -792,7 +792,7 @@ describe('messagesReducer', () => {
           ],
         };
 
-        test('on never-edited message, or one received with realm_allow_edit_history: false, creates one-item array', () => {
+        test('on never-edited message, creates one-item array', () => {
           expect(
             messagesReducer(
               eg.makeMessagesState([afterNoUpdates]),
@@ -809,8 +809,7 @@ describe('messagesReducer', () => {
         });
       });
 
-      // TODO(server-5.0): Simplify away.
-      test("don't touch it if we dropped it because server was FL <118", () => {
+      test("don't touch it if we dropped it", () => {
         const message1Old = eg.streamMessage({
           content: '<p>Old content</p>',
           subject: 'Old topic',

src/message/fetchActions.js

@@ -14,6 +14,7 @@ import * as api from '../api';
 import { Server5xxError, NetworkError } from '../api/apiErrors';
 import {
   getAuth,
+  getRealm,
   getSession,
   getFirstMessageId,
   getLastMessageId,
@@ -113,6 +114,7 @@ export const fetchMessages =
               useFirstUnread: fetchArgs.anchor === FIRST_UNREAD_ANCHOR, // TODO: don't use this; see #4203
             },
             getZulipFeatureLevel(getState()),
+            getRealm(getState()).allowEditHistory,
           ),
         );
       dispatch(
@@ -223,6 +225,7 @@ export async function fetchSomeMessageIdForConversation(
       narrow: apiNarrowOfNarrow(topicNarrow(streamId, topic), new Map(), streamsById),
     },
     zulipFeatureLevel,
+    false, // chosen arbitrarily; irrelevant to this function's task
   );
   // FlowIssue: can be void because array can be empty
   const message: Message | void = data.messages[0];
@@ -308,6 +311,7 @@ export const fetchPrivateMessages =
         numAfter: 0,
       },
       getZulipFeatureLevel(getState()),
+      getRealm(getState()).allowEditHistory,
     );
     dispatch(
       messageFetchComplete({

src/message/messagesReducer.js

@@ -81,12 +81,17 @@ const editHistory = <M: Message>(args: {|
 |}) => {
   const { oldMessage, event, move, shouldApplyContentChanges } = args;
   if (oldMessage.edit_history === null) {
-    // We ignored a maybe-interesting `edit_history` when we learned about
-    // the message because the value wouldn't have been in a nice shape; see
-    // Message['edit_history']. Keep ignoring it; don't give it a partial
-    // value, such as a one-item array based on this edit, which would be
-    // corrupt.
-    // TODO(server-5.0): Simplify away.
+    // Either:
+    // - we dropped edit_history because the server was old and the value
+    //   wouldn't have been in a nice shape, or
+    // - the realm is set to not allow viewing edit history
+    //
+    // (See Message['edit_history'].) Keep maintaining nothing here; don't
+    // write a partial value, such as a one-item array based on this edit,
+    // which would be corrupt.
+    //
+    // TODO(server-5.0): Simplify away the FL condition; keep the
+    //   allowEditHistory condition.
     return null;
   }