messages reducer: Sketch a plan for keeping Message.edit_history current

src/api/eventTypes.js

@@ -298,17 +298,19 @@ export type UserSettingsUpdateEvent = $ReadOnly<{|
  *
  * See also `messageMoved` in `misc.js`.
  */
-// This is current to feature level 109.
+// This is current to feature level 132.
 // NB if this ever gains a feature of moving PMs, `messageMoved` needs updating.
 export type UpdateMessageEvent = $ReadOnly<{|
   ...EventCommon,
   type: typeof EventTypes.update_message,
 
-  // Future servers might send `null` here:
-  //   https://chat.zulip.org/#narrow/stream/378-api-design/topic/.60update_message.60.20event/near/1309241
-  // TODO(server-5.0): Update this and/or simplify.
+  // Before FL 114, can be absent with the same meaning as null.
+  // TODO(server-5.0): Make required.
   user_id?: UserId | null,
 
+  // TODO(server-5.0): New in FL 114. On old servers, infer from `user_id`.
+  rendering_only?: boolean,
+
   // Any content changes apply to just message_id.
   message_id: number,
 
@@ -320,9 +322,16 @@ export type UpdateMessageEvent = $ReadOnly<{|
   message_ids: $ReadOnlyArray<number>,
 
   flags: $ReadOnlyArray<string>,
+
+  // TODO(server-5.0): Always present as of FL 114; make required.
   edit_timestamp?: number,
+
   stream_name?: string,
+
+  // As of FL 112, present for all stream-message updates.
+  // TODO(server-5.0): Remove comment but keep optional; absent for PMs.
   stream_id?: number,
+
   new_stream_id?: number,
   propagate_mode?: PropagateMode,
   orig_subject?: string,

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 { PmMessage, StreamMessage, Reaction, UserId } from '../modelTypes';
+import type { PmMessage, StreamMessage, Reaction, UserId, MessageEdit } from '../modelTypes';
 import { apiGet } from '../apiFetch';
 import { identityOfAuth } from '../../account/accountMisc';
 import { AvatarURL } from '../../utils/avatar';
@@ -62,7 +62,7 @@ type ServerMessageOf<M: Message> = $ReadOnly<{|
   ...$Exact<M>,
   avatar_url: string | null,
   reactions: $ReadOnlyArray<ServerReaction>,
-  edit_history: $ReadOnlyArray<ServerMessageEdit>,
+  edit_history?: $ReadOnlyArray<ServerMessageEdit>,
 |}>;
 
 export type ServerMessage = ServerMessageOf<PmMessage> | ServerMessageOf<StreamMessage>;
@@ -100,7 +100,7 @@ export const migrateMessages = (
       /* eslint-disable operator-linebreak */
       zulipFeatureLevel >= 118
         ? // $FlowIgnore[incompatible-cast] - See MessageEdit type
-          (message.edit_history: Message['edit_history'])
+          (message.edit_history: $ReadOnlyArray<MessageEdit> | void)
         : null,
   }));
 

src/api/modelTypes.js

@@ -749,9 +749,38 @@ type MessageBase = $ReadOnly<{|
   content_type: 'text/html',
   // display_recipient handled on PmMessage and StreamMessage separately
 
-  // Optional because we only store it in Redux if it's coming from a server
-  // with FL >=118; see comment on MessageEdit.
-  edit_history: $ReadOnlyArray<MessageEdit> | null,
+  /**
+   * A possibly incomplete view of the message's edit history.
+   *
+   * This is null if it's coming from a server with FL <118; see comment on
+   * MessageEdit.
+   *
+   * 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.)
+  // - 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.)
+  edit_history?: $ReadOnlyArray<MessageEdit> | null | void,
 
   id: number,
   is_me_message: boolean,

src/events/eventToAction.js

@@ -1,7 +1,7 @@
 /* @flow strict-local */
 import { EventTypes, type EventType, type RealmUserUpdateEventRaw } from '../api/eventTypes';
 import * as logging from '../utils/logging';
-import type { PerAccountState, EventAction, Message } from '../types';
+import type { PerAccountState, EventAction, MessageEdit } from '../types';
 import {
   EVENT_ALERT_WORDS,
   EVENT_NEW_MESSAGE,
@@ -120,7 +120,7 @@ export default (state: PerAccountState, event: $FlowFixMe): EventAction | null =
           edit_history:
             // Why FL 118 condition? See MessageEdit type.
             zulipFeatureLevel >= 118
-              ? (event.message.edit_history: $NonMaybeType<Message['edit_history']>)
+              ? (event.message.edit_history: $ReadOnlyArray<MessageEdit> | void)
               : null,
         },
         local_message_id: event.local_message_id,

src/message/__tests__/messagesReducer-test.js

@@ -292,6 +292,10 @@ describe('messagesReducer', () => {
           ]),
         );
       });
+
+      test.todo('multiple messages moved in one event');
+
+      test.todo("edited one message's content + multiple messages moved in one event");
     });
 
     test('when a message exists in state, it is updated', () => {

src/realm/__tests__/realmReducer-test.js

@@ -57,6 +57,7 @@ describe('realmReducer', () => {
         createWebPublicStreamPolicy: action.data.realm_create_web_public_stream_policy,
         enableSpectatorAccess: action.data.realm_enable_spectator_access,
         waitingPeriodThreshold: action.data.realm_waiting_period_threshold,
+        allowEditHistory: action.data.realm_allow_edit_history,
 
         //
         // InitialDataRealmUser
@@ -457,6 +458,14 @@ describe('realmReducer', () => {
         check(90, 90);
         check(90, 30);
       });
+
+      describe('allowEditHistory / allow_edit_history', () => {
+        const check = mkCheck('allowEditHistory', 'allow_edit_history');
+        check(true, true);
+        check(true, false);
+        check(false, true);
+        check(false, false);
+      });
     });
   });
 });

src/realm/realmReducer.js

@@ -51,6 +51,7 @@ const initialState = {
   createWebPublicStreamPolicy: CreateWebPublicStreamPolicy.Nobody,
   enableSpectatorAccess: false,
   waitingPeriodThreshold: 90,
+  allowEditHistory: false,
 
   //
   // InitialDataRealmUser
@@ -149,6 +150,7 @@ export default (
           action.data.realm_create_web_public_stream_policy ?? CreateWebPublicStreamPolicy.Nobody,
         enableSpectatorAccess: action.data.realm_enable_spectator_access ?? false,
         waitingPeriodThreshold: action.data.realm_waiting_period_threshold,
+        allowEditHistory: action.data.realm_allow_edit_history,
 
         //
         // InitialDataRealmUser
@@ -240,6 +242,9 @@ export default (
             if (data.waiting_period_threshold !== undefined) {
               result.waitingPeriodThreshold = data.waiting_period_threshold;
             }
+            if (data.allow_edit_history !== undefined) {
+              result.allowEditHistory = data.allow_edit_history;
+            }
 
             return result;
           }

src/reduxTypes.js

@@ -303,6 +303,7 @@ export type RealmState = {|
   +createWebPublicStreamPolicy: CreateWebPublicStreamPolicyT,
   +enableSpectatorAccess: boolean,
   +waitingPeriodThreshold: number,
+  +allowEditHistory: boolean,
 
   //
   // InitialDataRealmUser

src/storage/__tests__/migrations-test.js

@@ -87,7 +87,7 @@ describe('migrations', () => {
   // What `base` becomes after all migrations.
   const endBase = {
     ...base37,
-    migrations: { version: 49 },
+    migrations: { version: 50 },
   };
 
   for (const [desc, before, after] of [
@@ -110,8 +110,8 @@ describe('migrations', () => {
     // redundant with this one, because none of the migration steps notice
     // whether any properties outside `storeKeys` are present or not.
     [
-      'check dropCache at 49',
-      { ...endBase, migrations: { version: 48 }, mute: [], nonsense: [1, 2, 3] },
+      'check dropCache at 50',
+      { ...endBase, migrations: { version: 49 }, mute: [], nonsense: [1, 2, 3] },
       endBase,
     ],
 

src/storage/migrations.js

@@ -437,6 +437,9 @@ const migrationsInner: {| [string]: (LessPartialState) => LessPartialState |} =
   // Add defaultExternalAccounts to state.realm.
   '49': dropCache,
 
+  // Add allowEditHistory to state.realm.
+  '50': dropCache,
+
   // TIP: When adding a migration, consider just using `dropCache`.
   //   (See its jsdoc for guidance on when that's the right answer.)
 };