From 1873211a236922684bb0e7f46437bef31fe180a5 Mon Sep 17 00:00:00 2001
From: Chris Bobbe <cbobbe@zulip.com>
Date: Tue, 5 Jul 2022 14:46:04 -0400
Subject: [PATCH 1/7] messagesReducer tests: Give events an edit_timestamp when
 appropriate

According to our types, the only case in which a message-update
event is missing an edit_timestamp is when it's a rendering-only
event that comes from a server pre-FL 114.

So, fix some events that we don't mean to be rendering-only but that
we haven't given an edit_timestamp.
---
 src/message/__tests__/messagesReducer-test.js | 29 +++++++++++++++----
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/src/message/__tests__/messagesReducer-test.js b/src/message/__tests__/messagesReducer-test.js
index 3e0dffbe0f3..f72b79d75dd 100644
--- a/src/message/__tests__/messagesReducer-test.js
+++ b/src/message/__tests__/messagesReducer-test.js
@@ -216,9 +216,9 @@ describe('messagesReducer', () => {
       test('edited topic', () => {
         const message = eg.streamMessage();
         const newTopic = `${message.subject}abc`;
-        const action = mkMoveAction({ message, subject: newTopic });
+        const action = mkMoveAction({ message, subject: newTopic, edit_timestamp: 1000 });
         expect(messagesReducer(eg.makeMessagesState([message]), action, eg.plusReduxState)).toEqual(
-          eg.makeMessagesState([{ ...message, subject: newTopic }]),
+          eg.makeMessagesState([{ ...message, subject: newTopic, last_edit_timestamp: 1000 }]),
         );
       });
 
@@ -232,6 +232,7 @@ describe('messagesReducer', () => {
           message: message1,
           message_ids: [message1.id, message2.id],
           subject: newTopic,
+          edit_timestamp: 1000,
         });
         expect(
           messagesReducer(
@@ -241,7 +242,7 @@ describe('messagesReducer', () => {
           ),
         ).toEqual(
           eg.makeMessagesState([
-            { ...message1, subject: newTopic },
+            { ...message1, subject: newTopic, last_edit_timestamp: 1000 },
             { ...message2, subject: newTopic },
             message3,
           ]),
@@ -250,13 +251,18 @@ describe('messagesReducer', () => {
 
       test('new stream', () => {
         const message = eg.streamMessage();
-        const action = mkMoveAction({ message, new_stream_id: eg.otherStream.stream_id });
+        const action = mkMoveAction({
+          message,
+          new_stream_id: eg.otherStream.stream_id,
+          edit_timestamp: 1000,
+        });
         expect(messagesReducer(eg.makeMessagesState([message]), action, eg.plusReduxState)).toEqual(
           eg.makeMessagesState([
             {
               ...message,
               stream_id: eg.otherStream.stream_id,
               display_recipient: eg.otherStream.name,
+              last_edit_timestamp: 1000,
             },
           ]),
         );
@@ -269,6 +275,7 @@ describe('messagesReducer', () => {
           message,
           new_stream_id: eg.otherStream.stream_id,
           subject: newTopic,
+          edit_timestamp: 1000,
         });
         expect(messagesReducer(eg.makeMessagesState([message]), action, eg.plusReduxState)).toEqual(
           eg.makeMessagesState([
@@ -277,6 +284,7 @@ describe('messagesReducer', () => {
               stream_id: eg.otherStream.stream_id,
               display_recipient: eg.otherStream.name,
               subject: newTopic,
+              last_edit_timestamp: 1000,
             },
           ]),
         );
@@ -285,10 +293,19 @@ describe('messagesReducer', () => {
       test('new, unknown stream', () => {
         const message = eg.streamMessage();
         const unknownStream = eg.makeStream();
-        const action = mkMoveAction({ message, new_stream_id: unknownStream.stream_id });
+        const action = mkMoveAction({
+          message,
+          new_stream_id: unknownStream.stream_id,
+          edit_timestamp: 1000,
+        });
         expect(messagesReducer(eg.makeMessagesState([message]), action, eg.plusReduxState)).toEqual(
           eg.makeMessagesState([
-            { ...message, stream_id: unknownStream.stream_id, display_recipient: 'unknown' },
+            {
+              ...message,
+              stream_id: unknownStream.stream_id,
+              display_recipient: 'unknown',
+              last_edit_timestamp: 1000,
+            },
           ]),
         );
       });

From 02b9924c6fffb2da14dde9b5d682e935dbf3a98c Mon Sep 17 00:00:00 2001
From: Chris Bobbe <cbobbe@zulip.com>
Date: Tue, 5 Jul 2022 13:59:33 -0400
Subject: [PATCH 2/7] messagesReducer: Don't mishandle new servers'
 rendering-only update events

In FL 114, servers stopped representing rendering-only events with
missing edit_timestamp, and started representing them with
`rendering_only: true`. Handle both representations, not just the
old one.
---
 src/message/messagesReducer.js | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/src/message/messagesReducer.js b/src/message/messagesReducer.js
index a712b69e123..3aa6ecf6588 100644
--- a/src/message/messagesReducer.js
+++ b/src/message/messagesReducer.js
@@ -150,7 +150,17 @@ export default (
         return {
           ...(oldMessage: M),
           content: event.rendered_content ?? oldMessage.content,
-          last_edit_timestamp: event.edit_timestamp ?? oldMessage.last_edit_timestamp,
+
+          // Don't update last_edit_timestamp if it's a rendering-only
+          // event. How do we know if it is? From newer servers,
+          // rendering_only will be true; from older servers, edit_timestamp
+          // will be missing.
+          // TODO(server-5.0): Simplify away edit_timestamp-missing condition
+          last_edit_timestamp:
+            event.rendering_only === true || event.edit_timestamp == null
+              ? oldMessage.last_edit_timestamp
+              : event.edit_timestamp,
+
           // TODO(#3408): Update edit_history, too.  This is OK for now
           //   because we don't actually have any UI to expose it: #4134.
         };

From dbe5e62df938e4b3aa3b0fa6b11e8016f6e4165e Mon Sep 17 00:00:00 2001
From: Chris Bobbe <cbobbe@zulip.com>
Date: Tue, 5 Jul 2022 13:50:09 -0400
Subject: [PATCH 3/7] messagesReducer tests: Use new representation for
 is-rendering-only

In particular, all the update-message events we simulate in this
file have rendering_only, user_id, and edit_timestamp.
---
 src/message/__tests__/messagesReducer-test.js | 22 +++++++++++--------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/src/message/__tests__/messagesReducer-test.js b/src/message/__tests__/messagesReducer-test.js
index f72b79d75dd..06d7dee454f 100644
--- a/src/message/__tests__/messagesReducer-test.js
+++ b/src/message/__tests__/messagesReducer-test.js
@@ -1,7 +1,7 @@
 /* @flow strict-local */
 import deepFreeze from 'deep-freeze';
 
-import type { Submessage, UserId } from '../../types';
+import type { Submessage } from '../../types';
 import messagesReducer from '../messagesReducer';
 import { FIRST_UNREAD_ANCHOR } from '../../anchor';
 import {
@@ -165,17 +165,20 @@ describe('messagesReducer', () => {
 
   describe('EVENT_UPDATE_MESSAGE', () => {
     const mkAction = args => {
-      const { message, ...restArgs } = args;
-      // Include a user_id just if there's an edit_timestamp.  (Actual
-      // update_message events have both for normal edits, and neither for
-      // stealth server-edits.)
-      const forEdit: { user_id?: UserId } =
-        restArgs.edit_timestamp != null ? { user_id: message.sender_id } : Object.freeze({});
+      const {
+        message,
+        rendering_only = false,
+        user_id = message.sender_id,
+        edit_timestamp = message.timestamp + 1,
+        ...restArgs
+      } = args;
       return eg.mkActionEventUpdateMessage({
-        ...forEdit,
+        rendering_only,
         message_id: message.id,
         message_ids: [message.id],
         propagate_mode: 'change_one',
+        user_id,
+        edit_timestamp,
         is_me_message: false,
         ...restArgs,
       });
@@ -403,7 +406,7 @@ describe('messagesReducer', () => {
         // edit_history still empty
       };
       const action = mkAction({
-        // no edit_timestamp, no user_id
+        rendering_only: true,
         message,
         orig_content: message.content,
         orig_rendered_content: message.content,
@@ -431,6 +434,7 @@ describe('messagesReducer', () => {
         // undefined from action.edit_timestamp
       };
       const action = mkAction({
+        rendering_only: true,
         message,
         orig_content: message.content,
         orig_rendered_content: message.content,

From 446edd460ef4fb5a20b86622b26cef83ab8fb21b Mon Sep 17 00:00:00 2001
From: Chris Bobbe <cbobbe@zulip.com>
Date: Tue, 5 Jul 2022 10:45:54 -0400
Subject: [PATCH 4/7] messagesReducer tests [nfc]: Have mkAction take
 message_ids, propagate_mode

---
 src/message/__tests__/messagesReducer-test.js | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/message/__tests__/messagesReducer-test.js b/src/message/__tests__/messagesReducer-test.js
index 06d7dee454f..57864e1b59f 100644
--- a/src/message/__tests__/messagesReducer-test.js
+++ b/src/message/__tests__/messagesReducer-test.js
@@ -168,6 +168,8 @@ describe('messagesReducer', () => {
       const {
         message,
         rendering_only = false,
+        message_ids = [message.id],
+        propagate_mode = 'change_one',
         user_id = message.sender_id,
         edit_timestamp = message.timestamp + 1,
         ...restArgs
@@ -175,8 +177,8 @@ describe('messagesReducer', () => {
       return eg.mkActionEventUpdateMessage({
         rendering_only,
         message_id: message.id,
-        message_ids: [message.id],
-        propagate_mode: 'change_one',
+        message_ids,
+        propagate_mode,
         user_id,
         edit_timestamp,
         is_me_message: false,

From 0abd1b822ec71da580057b55cec67b4fcdf544b1 Mon Sep 17 00:00:00 2001
From: Chris Bobbe <cbobbe@zulip.com>
Date: Tue, 5 Jul 2022 11:18:49 -0400
Subject: [PATCH 5/7] messagesReducer tests: Cover multiple messages moved in
 one update event

---
 src/message/__tests__/messagesReducer-test.js | 104 +++++++++++++++++-
 1 file changed, 102 insertions(+), 2 deletions(-)

diff --git a/src/message/__tests__/messagesReducer-test.js b/src/message/__tests__/messagesReducer-test.js
index 57864e1b59f..0f082cd6f09 100644
--- a/src/message/__tests__/messagesReducer-test.js
+++ b/src/message/__tests__/messagesReducer-test.js
@@ -315,9 +315,109 @@ describe('messagesReducer', () => {
         );
       });
 
-      test.todo('multiple messages moved in one event');
+      test('multiple messages moved in one event', () => {
+        const message0 = eg.streamMessage({ stream: eg.stream, subject: 'old topic' });
+        const message1 = eg.streamMessage({ stream: eg.stream, subject: 'old topic' });
+        const message2 = eg.streamMessage({ stream: eg.stream, subject: 'old topic' });
 
-      test.todo("edited one message's content + multiple messages moved in one event");
+        const action = mkMoveAction({
+          edit_timestamp: 1000,
+          message: message0,
+          message_ids: [message0.id, message1.id, message2.id],
+          propagate_mode: 'change_later',
+          new_stream_id: eg.otherStream.stream_id,
+          subject: 'new topic',
+        });
+
+        expect(
+          messagesReducer(
+            eg.makeMessagesState([message0, message1, message2]),
+            action,
+            eg.plusReduxState,
+          ),
+        ).toEqual(
+          eg.makeMessagesState([
+            {
+              ...message0,
+              stream_id: eg.otherStream.stream_id,
+              display_recipient: eg.otherStream.name,
+              subject: 'new topic',
+              last_edit_timestamp: 1000,
+            },
+            {
+              ...message1,
+              stream_id: eg.otherStream.stream_id,
+              display_recipient: eg.otherStream.name,
+              subject: 'new topic',
+            },
+            {
+              ...message2,
+              stream_id: eg.otherStream.stream_id,
+              display_recipient: eg.otherStream.name,
+              subject: 'new topic',
+            },
+          ]),
+        );
+      });
+
+      test("edited one message's content + multiple messages moved in one event", () => {
+        const message0 = eg.streamMessage({
+          stream: eg.stream,
+          subject: 'old topic',
+          content: '<p>Old content</p>',
+        });
+        const message1 = eg.streamMessage({ stream: eg.stream, subject: 'old topic' });
+        const message2 = eg.streamMessage({ stream: eg.stream, subject: 'old topic' });
+
+        const action = mkMoveAction({
+          edit_timestamp: 1000,
+
+          // content edit (to apply to just one message)
+          message: message0,
+          orig_content: 'Old content',
+          orig_rendered_content: '<p>Old content</p>',
+          prev_rendered_content_version: 1,
+          content: 'New content',
+          rendered_content: '<p>New content</p>',
+
+          // move (to apply to all messages in `message_ids`)
+          message_ids: [message0.id, message1.id, message2.id],
+          propagate_mode: 'change_later',
+          new_stream_id: eg.otherStream.stream_id,
+          subject: 'new topic',
+        });
+
+        expect(
+          messagesReducer(
+            eg.makeMessagesState([message0, message1, message2]),
+            action,
+            eg.plusReduxState,
+          ),
+        ).toEqual(
+          eg.makeMessagesState([
+            {
+              ...message0,
+              content: '<p>New content</p>',
+              stream_id: eg.otherStream.stream_id,
+              display_recipient: eg.otherStream.name,
+              subject: 'new topic',
+              last_edit_timestamp: 1000,
+            },
+            {
+              ...message1,
+              stream_id: eg.otherStream.stream_id,
+              display_recipient: eg.otherStream.name,
+              subject: 'new topic',
+            },
+            {
+              ...message2,
+              stream_id: eg.otherStream.stream_id,
+              display_recipient: eg.otherStream.name,
+              subject: 'new topic',
+            },
+          ]),
+        );
+      });
     });
 
     test('when a message exists in state, it is updated', () => {

From 37a2ee8b4f4f1ae14169a8a032c56d5d93c03b52 Mon Sep 17 00:00:00 2001
From: Chris Bobbe <cbobbe@zulip.com>
Date: Mon, 20 Jun 2022 11:17:29 -0400
Subject: [PATCH 6/7] messagesReducer: Maintain edit_history on message updates
 in FL 118+ format

---
 src/api/modelTypes.js                         |   3 -
 src/message/__tests__/messagesReducer-test.js | 298 ++++++++++++++++--
 src/message/messagesReducer.js                | 102 +++++-
 3 files changed, 364 insertions(+), 39 deletions(-)

diff --git a/src/api/modelTypes.js b/src/api/modelTypes.js
index 4e1b9d16ec9..efcd9b6fbf1 100644
--- a/src/api/modelTypes.js
+++ b/src/api/modelTypes.js
@@ -760,8 +760,6 @@ type MessageBase = $ReadOnly<{|
    *
    * 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
@@ -773,7 +771,6 @@ type MessageBase = $ReadOnly<{|
   //     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
   //
diff --git a/src/message/__tests__/messagesReducer-test.js b/src/message/__tests__/messagesReducer-test.js
index 0f082cd6f09..e5e7373c7f1 100644
--- a/src/message/__tests__/messagesReducer-test.js
+++ b/src/message/__tests__/messagesReducer-test.js
@@ -223,7 +223,21 @@ describe('messagesReducer', () => {
         const newTopic = `${message.subject}abc`;
         const action = mkMoveAction({ message, subject: newTopic, edit_timestamp: 1000 });
         expect(messagesReducer(eg.makeMessagesState([message]), action, eg.plusReduxState)).toEqual(
-          eg.makeMessagesState([{ ...message, subject: newTopic, last_edit_timestamp: 1000 }]),
+          eg.makeMessagesState([
+            {
+              ...message,
+              subject: newTopic,
+              edit_history: [
+                {
+                  user_id: message.sender_id,
+                  timestamp: 1000,
+                  prev_topic: 'example topic',
+                  topic: 'example topicabc',
+                },
+              ],
+              last_edit_timestamp: 1000,
+            },
+          ]),
         );
       });
 
@@ -247,8 +261,31 @@ describe('messagesReducer', () => {
           ),
         ).toEqual(
           eg.makeMessagesState([
-            { ...message1, subject: newTopic, last_edit_timestamp: 1000 },
-            { ...message2, subject: newTopic },
+            {
+              ...message1,
+              subject: newTopic,
+              edit_history: [
+                {
+                  user_id: message1.sender_id,
+                  timestamp: 1000,
+                  prev_topic: 'some topic',
+                  topic: 'some revised topic',
+                },
+              ],
+              last_edit_timestamp: 1000,
+            },
+            {
+              ...message2,
+              subject: newTopic,
+              edit_history: [
+                {
+                  user_id: message2.sender_id,
+                  timestamp: 1000,
+                  prev_topic: 'some topic',
+                  topic: 'some revised topic',
+                },
+              ],
+            },
             message3,
           ]),
         );
@@ -267,6 +304,14 @@ describe('messagesReducer', () => {
               ...message,
               stream_id: eg.otherStream.stream_id,
               display_recipient: eg.otherStream.name,
+              edit_history: [
+                {
+                  user_id: message.sender_id,
+                  timestamp: 1000,
+                  prev_stream: message.stream_id,
+                  stream: eg.otherStream.stream_id,
+                },
+              ],
               last_edit_timestamp: 1000,
             },
           ]),
@@ -289,6 +334,16 @@ describe('messagesReducer', () => {
               stream_id: eg.otherStream.stream_id,
               display_recipient: eg.otherStream.name,
               subject: newTopic,
+              edit_history: [
+                {
+                  user_id: message.sender_id,
+                  timestamp: 1000,
+                  prev_stream: message.stream_id,
+                  stream: eg.otherStream.stream_id,
+                  prev_topic: 'example topic',
+                  topic: 'example topicabc',
+                },
+              ],
               last_edit_timestamp: 1000,
             },
           ]),
@@ -309,6 +364,14 @@ describe('messagesReducer', () => {
               ...message,
               stream_id: unknownStream.stream_id,
               display_recipient: 'unknown',
+              edit_history: [
+                {
+                  user_id: message.sender_id,
+                  timestamp: 1000,
+                  prev_stream: message.stream_id,
+                  stream: unknownStream.stream_id,
+                },
+              ],
               last_edit_timestamp: 1000,
             },
           ]),
@@ -321,6 +384,7 @@ describe('messagesReducer', () => {
         const message2 = eg.streamMessage({ stream: eg.stream, subject: 'old topic' });
 
         const action = mkMoveAction({
+          user_id: eg.thirdUser.user_id,
           edit_timestamp: 1000,
           message: message0,
           message_ids: [message0.id, message1.id, message2.id],
@@ -336,27 +400,24 @@ describe('messagesReducer', () => {
             eg.plusReduxState,
           ),
         ).toEqual(
-          eg.makeMessagesState([
-            {
-              ...message0,
+          eg.makeMessagesState(
+            [{ ...message0, last_edit_timestamp: 1000 }, message1, message2].map(m => ({
+              ...m,
               stream_id: eg.otherStream.stream_id,
               display_recipient: eg.otherStream.name,
               subject: 'new topic',
-              last_edit_timestamp: 1000,
-            },
-            {
-              ...message1,
-              stream_id: eg.otherStream.stream_id,
-              display_recipient: eg.otherStream.name,
-              subject: 'new topic',
-            },
-            {
-              ...message2,
-              stream_id: eg.otherStream.stream_id,
-              display_recipient: eg.otherStream.name,
-              subject: 'new topic',
-            },
-          ]),
+              edit_history: [
+                {
+                  user_id: eg.thirdUser.user_id,
+                  timestamp: 1000,
+                  prev_stream: eg.stream.stream_id,
+                  stream: eg.otherStream.stream_id,
+                  prev_topic: 'old topic',
+                  topic: 'new topic',
+                },
+              ],
+            })),
+          ),
         );
       });
 
@@ -370,6 +431,7 @@ describe('messagesReducer', () => {
         const message2 = eg.streamMessage({ stream: eg.stream, subject: 'old topic' });
 
         const action = mkMoveAction({
+          user_id: eg.thirdUser.user_id,
           edit_timestamp: 1000,
 
           // content edit (to apply to just one message)
@@ -401,20 +463,37 @@ describe('messagesReducer', () => {
               stream_id: eg.otherStream.stream_id,
               display_recipient: eg.otherStream.name,
               subject: 'new topic',
+              edit_history: [
+                {
+                  user_id: eg.thirdUser.user_id,
+                  timestamp: 1000,
+                  prev_content: 'Old content',
+                  prev_rendered_content: '<p>Old content</p>',
+                  prev_rendered_content_version: 1,
+                  prev_stream: eg.stream.stream_id,
+                  stream: eg.otherStream.stream_id,
+                  prev_topic: 'old topic',
+                  topic: 'new topic',
+                },
+              ],
               last_edit_timestamp: 1000,
             },
-            {
-              ...message1,
+            ...[message1, message2].map(m => ({
+              ...m,
               stream_id: eg.otherStream.stream_id,
               display_recipient: eg.otherStream.name,
               subject: 'new topic',
-            },
-            {
-              ...message2,
-              stream_id: eg.otherStream.stream_id,
-              display_recipient: eg.otherStream.name,
-              subject: 'new topic',
-            },
+              edit_history: [
+                {
+                  user_id: eg.thirdUser.user_id,
+                  timestamp: 1000,
+                  prev_stream: eg.stream.stream_id,
+                  stream: eg.otherStream.stream_id,
+                  prev_topic: 'old topic',
+                  topic: 'new topic',
+                },
+              ],
+            })),
           ]),
         );
       });
@@ -428,6 +507,15 @@ describe('messagesReducer', () => {
         ...message3Old,
         content: '<p>New content</p>',
         last_edit_timestamp: 123,
+        edit_history: [
+          {
+            prev_content: message3Old.content,
+            prev_rendered_content: message3Old.content,
+            prev_rendered_content_version: 1,
+            timestamp: 123,
+            user_id: message3Old.sender_id,
+          },
+        ],
       };
 
       const prevState = eg.makeMessagesState([message1, message2, message3Old]);
@@ -456,6 +544,14 @@ describe('messagesReducer', () => {
         ...message1Old,
         subject: 'New topic',
         last_edit_timestamp: 123,
+        edit_history: [
+          {
+            prev_topic: message1Old.subject,
+            timestamp: 123,
+            topic: 'New topic',
+            user_id: message1Old.sender_id,
+          },
+        ],
       };
       const prevState = eg.makeMessagesState([message1Old]);
       const action = mkMoveAction({
@@ -479,6 +575,17 @@ describe('messagesReducer', () => {
         content: '<p>New content</p>',
         subject: 'New updated topic',
         last_edit_timestamp: 456,
+        edit_history: [
+          {
+            prev_content: message1Old.content,
+            prev_rendered_content: message1Old.content,
+            prev_rendered_content_version: 1,
+            prev_topic: message1Old.subject,
+            timestamp: 456,
+            topic: 'New updated topic',
+            user_id: message1Old.sender_id,
+          },
+        ],
       };
 
       const prevState = eg.makeMessagesState([message1Old]);
@@ -548,6 +655,137 @@ describe('messagesReducer', () => {
         eg.makeMessagesState([messageAfter]),
       );
     });
+
+    describe('edit_history not covered above', () => {
+      describe('sequential updates', () => {
+        const initialContent = 'Old content';
+        const initialRenderedContent = `<p>${initialContent}</p>`;
+        const initialTopic = 'Old topic';
+
+        const afterNoUpdates = eg.streamMessage({
+          content: initialRenderedContent,
+          subject: initialTopic,
+          edit_history: undefined,
+        });
+
+        const senderId = afterNoUpdates.sender_id;
+
+        const update1Content = 'New content';
+        const update1RenderedContent = `<p>${update1Content}</p>`;
+        const update1Topic = 'New updated topic';
+
+        const update1Action = mkMoveAction({
+          edit_timestamp: 456,
+          message: afterNoUpdates,
+          orig_content: initialContent,
+          orig_rendered_content: initialRenderedContent,
+          rendered_content: update1RenderedContent,
+          content: update1Content,
+          subject: update1Topic,
+          prev_rendered_content_version: 1,
+        });
+
+        const afterUpdate1 = {
+          ...afterNoUpdates,
+          content: update1RenderedContent,
+          subject: update1Topic,
+          last_edit_timestamp: 456,
+          edit_history: [
+            {
+              prev_content: initialContent,
+              prev_rendered_content: initialRenderedContent,
+              prev_rendered_content_version: 1,
+              prev_topic: initialTopic,
+              timestamp: 456,
+              topic: update1Topic,
+              user_id: senderId,
+            },
+          ],
+        };
+
+        const update2Content = 'New content 2';
+        const update2RenderedContent = `<p>${update2Content}</p>`;
+        const update2Topic = 'New updated topic 2';
+
+        const update2Action = mkMoveAction({
+          edit_timestamp: 567,
+          message: afterUpdate1,
+          orig_content: update1Content,
+          orig_rendered_content: update1RenderedContent,
+          rendered_content: update2RenderedContent,
+          content: update2Content,
+          subject: update2Topic,
+          prev_rendered_content_version: 1,
+        });
+
+        const afterUpdate2 = {
+          ...afterUpdate1,
+          content: update2RenderedContent,
+          subject: update2Topic,
+          last_edit_timestamp: 567,
+          edit_history: [
+            {
+              prev_content: update1Content,
+              prev_rendered_content: update1RenderedContent,
+              prev_rendered_content_version: 1,
+              prev_topic: update1Topic,
+              timestamp: 567,
+              topic: update2Topic,
+              user_id: senderId,
+            },
+            {
+              prev_content: initialContent,
+              prev_rendered_content: initialRenderedContent,
+              prev_rendered_content_version: 1,
+              prev_topic: initialTopic,
+              timestamp: 456,
+              topic: update1Topic,
+              user_id: senderId,
+            },
+          ],
+        };
+
+        test('on never-edited message, or one received with realm_allow_edit_history: false, creates one-item array', () => {
+          expect(
+            messagesReducer(
+              eg.makeMessagesState([afterNoUpdates]),
+              update1Action,
+              eg.plusReduxState,
+            ),
+          ).toEqual(eg.makeMessagesState([afterUpdate1]));
+        });
+
+        test('on once-edited message, adds item at start of array', () => {
+          expect(
+            messagesReducer(eg.makeMessagesState([afterUpdate1]), update2Action, eg.plusReduxState),
+          ).toEqual(eg.makeMessagesState([afterUpdate2]));
+        });
+      });
+
+      // TODO(server-5.0): Simplify away.
+      test("don't touch it if we dropped it because server was FL <118", () => {
+        const message1Old = eg.streamMessage({
+          content: '<p>Old content</p>',
+          subject: 'Old topic',
+          edit_history: null,
+        });
+        const prevState = eg.makeMessagesState([message1Old]);
+        const action = mkMoveAction({
+          edit_timestamp: 456,
+          message: message1Old,
+          orig_content: message1Old.content,
+          orig_rendered_content: message1Old.content,
+          rendered_content: '<p>New content</p>',
+          content: 'New content',
+          subject: 'New updated topic',
+          prev_rendered_content_version: 1,
+        });
+        const newState = messagesReducer(prevState, action, eg.plusReduxState);
+        const newMessage = newState.get(message1Old.id);
+        expect(newMessage).not.toBeUndefined();
+        expect(newState.get(message1Old.id)?.edit_history).toBe(null);
+      });
+    });
   });
 
   describe('EVENT_REACTION_ADD', () => {
diff --git a/src/message/messagesReducer.js b/src/message/messagesReducer.js
index 3aa6ecf6588..178d75d3fa3 100644
--- a/src/message/messagesReducer.js
+++ b/src/message/messagesReducer.js
@@ -3,7 +3,13 @@
 import omit from 'lodash.omit';
 import Immutable from 'immutable';
 
-import type { MessagesState, Message, PerAccountApplicableAction, PerAccountState } from '../types';
+import type {
+  MessagesState,
+  Message,
+  PerAccountApplicableAction,
+  PerAccountState,
+  UpdateMessageEvent,
+} from '../types';
 import {
   REGISTER_COMPLETE,
   LOGOUT,
@@ -20,6 +26,9 @@ import {
 import { getNarrowsForMessage, keyFromNarrow } from '../utils/narrow';
 import * as logging from '../utils/logging';
 import { getStreamsById } from '../selectors';
+import type { MessageEdit } from '../api/modelTypes';
+import type { ReadWrite } from '../generics';
+import type { MessageMove } from '../api/misc';
 
 const initialState: MessagesState = Immutable.Map([]);
 
@@ -64,6 +73,65 @@ const eventNewMessage = (state, action) => {
   return state.set(action.message.id, omit(action.message, 'flags'));
 };
 
+const editHistory = <M: Message>(args: {|
+  oldMessage: M,
+  event: UpdateMessageEvent,
+  move: MessageMove | null,
+  shouldApplyContentChanges: boolean,
+|}) => {
+  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.
+    return null;
+  }
+
+  if (
+    event.rendering_only === true
+    // TODO(server-5.0): Simplify away these two checks
+    || event.edit_timestamp === undefined
+    || event.user_id === undefined
+  ) {
+    // See doc:
+    //   https://zulip.com/api/get-events#update_message
+    // > […] the event does not reflect a user-generated edit and does not
+    // > modify the message history.
+    return oldMessage.edit_history;
+  }
+
+  const newEntry: ReadWrite<MessageEdit> = {
+    timestamp: event.edit_timestamp,
+    user_id: event.user_id,
+  };
+
+  if (
+    shouldApplyContentChanges
+    && event.message_id === oldMessage.id
+    && event.orig_content != null
+  ) {
+    newEntry.prev_content = event.orig_content;
+    newEntry.prev_rendered_content = event.orig_rendered_content;
+    newEntry.prev_rendered_content_version = event.prev_rendered_content_version;
+  }
+
+  if (move) {
+    if (move.orig_stream_id !== move.new_stream_id) {
+      newEntry.prev_stream = move.orig_stream_id;
+      newEntry.stream = move.new_stream_id;
+    }
+    if (move.orig_topic !== move.new_topic) {
+      newEntry.prev_topic = move.orig_topic;
+      newEntry.topic = move.new_topic;
+    }
+  }
+
+  return [newEntry, ...(oldMessage.edit_history ?? [])];
+};
+
 export default (
   state: MessagesState = initialState, // eslint-disable-line default-param-last
   action: PerAccountApplicableAction,
@@ -161,8 +229,12 @@ export default (
               ? oldMessage.last_edit_timestamp
               : event.edit_timestamp,
 
-          // TODO(#3408): Update edit_history, too.  This is OK for now
-          //   because we don't actually have any UI to expose it: #4134.
+          edit_history: editHistory<M>({
+            oldMessage,
+            event,
+            move,
+            shouldApplyContentChanges: true,
+          }),
         };
       });
 
@@ -200,9 +272,27 @@ export default (
                 return oldMessage;
               }
 
-              // TODO(#3408): Update edit_history, too.  This is OK for now
-              //   because we don't actually have any UI to expose it: #4134.
-              return { ...oldMessage, ...update };
+              return {
+                ...oldMessage,
+                ...update,
+
+                // We already processed the message with ID
+                // `event.message_id`, above; skip it here.
+                edit_history:
+                  id === event.message_id
+                    ? oldMessage.edit_history
+                    : editHistory<M>({
+                        oldMessage,
+                        event,
+                        move,
+
+                        // See doc:
+                        //   https://zulip.com/api/get-events#update_message
+                        // > Content changes should be applied only to the single message
+                        // > indicated by `message_id`.
+                        shouldApplyContentChanges: false,
+                      }),
+              };
             });
           }
         });

From 77e97fce55f3d351ebd0c1f31a02a8ee75a5af73 Mon Sep 17 00:00:00 2001
From: Chris Bobbe <cbobbe@zulip.com>
Date: Tue, 5 Jul 2022 17:28:22 -0400
Subject: [PATCH 7/7] messages state: Keep edit_history null if
 allowEditHistory is false

---
 .../messages/__tests__/migrateMessages-test.js | 12 +++++++++++-
 src/api/messages/getMessages.js                | 14 ++++++++++----
 src/api/modelTypes.js                          | 16 +++++-----------
 src/events/__tests__/eventToAction-test.js     | 18 +++++++++++++++++-
 src/events/eventToAction.js                    |  6 +++++-
 src/message/__tests__/fetchActions-test.js     |  4 ++++
 src/message/__tests__/messagesReducer-test.js  |  5 ++---
 src/message/fetchActions.js                    |  4 ++++
 src/message/messagesReducer.js                 | 17 +++++++++++------
 9 files changed, 69 insertions(+), 27 deletions(-)

diff --git a/src/api/messages/__tests__/migrateMessages-test.js b/src/api/messages/__tests__/migrateMessages-test.js
index 85fe4d9e0d0..1e3913598e8 100644
--- a/src/api/messages/__tests__/migrateMessages-test.js
+++ b/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]);
 });
diff --git a/src/api/messages/getMessages.js b/src/api/messages/getMessages.js
index be5ac66f725..048acb01916 100644
--- a/src/api/messages/getMessages.js
+++ b/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);
 };
diff --git a/src/api/modelTypes.js b/src/api/modelTypes.js
index efcd9b6fbf1..499b9496f2e 100644
--- a/src/api/modelTypes.js
+++ b/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,
diff --git a/src/events/__tests__/eventToAction-test.js b/src/events/__tests__/eventToAction-test.js
index c8035d899b9..485290683dd 100644
--- a/src/events/__tests__/eventToAction-test.js
+++ b/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', () => {
diff --git a/src/events/eventToAction.js b/src/events/eventToAction.js
index 3cbc6897fb0..d59b8de13fd 100644
--- a/src/events/eventToAction.js
+++ b/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,
         },
diff --git a/src/message/__tests__/fetchActions-test.js b/src/message/__tests__/fetchActions-test.js
index a58beda80e9..62d8d6e993e 100644
--- a/src/message/__tests__/fetchActions-test.js
+++ b/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', () => {
diff --git a/src/message/__tests__/messagesReducer-test.js b/src/message/__tests__/messagesReducer-test.js
index e5e7373c7f1..776f8e45405 100644
--- a/src/message/__tests__/messagesReducer-test.js
+++ b/src/message/__tests__/messagesReducer-test.js
@@ -745,7 +745,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]),
@@ -762,8 +762,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',
diff --git a/src/message/fetchActions.js b/src/message/fetchActions.js
index ebfd1f26866..2bc00923333 100644
--- a/src/message/fetchActions.js
+++ b/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({
diff --git a/src/message/messagesReducer.js b/src/message/messagesReducer.js
index 178d75d3fa3..b11b61bb357 100644
--- a/src/message/messagesReducer.js
+++ b/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;
   }