messages state: Maintain edit_history to be reasonably current

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,36 +750,27 @@ 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.
-   *
-   * Stale if the message was updated or moved after we added it to the state.
    */
-  // 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
   //     could mean any number of subtle changes in server behavior), and do
   //     a re-fetch of server data soon after it.
-  // - Handle update-message events, writing the FL 118+ shape.
   //
   // 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', () => {