edit_history: Store additional fields on edit history events.

We modify the message_edit_history marshalling code so that this
commit does not change the API, since we haven't backfilled the data
yet.

FormattedEditHistoryEvent, introduced in the previous commit, doesn't
directly inherit fields from EditHistoryEvent, so no changes are
required there.

zerver/lib/actions.py

@@ -6815,6 +6815,7 @@ def do_update_message(
         assert stream_being_edited is not None
 
         edit_history_event["prev_stream"] = stream_being_edited.id
+        edit_history_event["stream"] = new_stream.id
         event[ORIG_TOPIC] = orig_topic_name
         target_message.recipient_id = new_stream.recipient_id
 
@@ -6874,6 +6875,8 @@ def do_update_message(
         event[TOPIC_NAME] = topic_name
         event[TOPIC_LINKS] = topic_links(target_message.sender.realm_id, topic_name)
         edit_history_event["prev_subject"] = orig_topic_name
+        edit_history_event["prev_topic"] = orig_topic_name
+        edit_history_event["topic"] = topic_name
 
     update_edit_history(target_message, timestamp, edit_history_event)
 
@@ -6888,9 +6891,13 @@ def do_update_message(
             "timestamp": edit_history_event["timestamp"],
         }
         if topic_name is not None:
+            # For backwards-compatability, we include this legacy field name.
             topic_only_edit_history_event["prev_subject"] = edit_history_event["prev_subject"]
+            topic_only_edit_history_event["prev_topic"] = edit_history_event["prev_topic"]
+            topic_only_edit_history_event["topic"] = edit_history_event["topic"]
         if new_stream is not None:
             topic_only_edit_history_event["prev_stream"] = edit_history_event["prev_stream"]
+            topic_only_edit_history_event["stream"] = edit_history_event["stream"]
 
         messages_list = update_messages_for_topic_edit(
             acting_user=user_profile,

zerver/lib/message.py

@@ -38,7 +38,12 @@ from zerver.lib.streams import get_web_public_streams_queryset
 from zerver.lib.timestamp import datetime_to_timestamp
 from zerver.lib.topic import DB_TOPIC_NAME, MESSAGE__TOPIC, TOPIC_LINKS, TOPIC_NAME
 from zerver.lib.topic_mutes import build_topic_mute_checker, topic_is_muted
-from zerver.lib.types import DisplayRecipientT, EditHistoryEvent, UserDisplayRecipient
+from zerver.lib.types import (
+    APIEditHistoryEvent,
+    DisplayRecipientT,
+    EditHistoryEvent,
+    UserDisplayRecipient,
+)
 from zerver.models import (
     MAX_TOPIC_NAME_LENGTH,
     Message,
@@ -502,8 +507,17 @@ class MessageDict:
         if last_edit_time is not None:
             obj["last_edit_timestamp"] = datetime_to_timestamp(last_edit_time)
             assert edit_history_json is not None
-            # Here we assume EditHistoryEvent == APIEditHistoryEvent
+            raw_edit_history: List[EditHistoryEvent] = orjson.loads(edit_history_json)
-            edit_history: List[EditHistoryEvent] = orjson.loads(edit_history_json)
+            edit_history: List[APIEditHistoryEvent] = []
+            for edit_history_event in raw_edit_history:
+                # Drop fields we're not yet ready to have appear in the API
+                if "prev_topic" in edit_history_event:
+                    del edit_history_event["prev_topic"]
+                if "stream" in edit_history_event:
+                    del edit_history_event["stream"]
+                if "topic" in edit_history_event:
+                    del edit_history_event["topic"]
+                edit_history.append(edit_history_event)
             obj["edit_history"] = edit_history
 
         if Message.need_to_render_content(

zerver/lib/types.py

@@ -102,6 +102,7 @@ class APIEditHistoryEvent(TypedDict, total=False):
     timestamp: int
     prev_stream: int
     # stream: int
+    # TODO: Remove prev_subject from the API.
     prev_subject: str
     # prev_topic: str
     # topic: str
@@ -119,10 +120,10 @@ class EditHistoryEvent(TypedDict, total=False):
     user_id: Optional[int]
     timestamp: int
     prev_stream: int
-    # stream: int
+    stream: int
     prev_subject: str
-    # prev_topic: str
+    prev_topic: str
-    # topic: str
+    topic: str
     prev_content: str
     prev_rendered_content: Optional[str]
     prev_rendered_content_version: Optional[int]

zerver/tests/test_message_edit.py

@@ -73,11 +73,12 @@ class EditMessageTestCase(ZulipTestCase):
             msg.sender_id,
         )
 
-        if msg.edit_history:
+        # TODO: uncomment assertion when edit_history fields in API match fields in database.
-            self.assertEqual(
+        # if msg.edit_history:
-                fetch_message_dict["edit_history"],
+        #     self.assertEqual(
-                orjson.loads(msg.edit_history),
+        #         fetch_message_dict["edit_history"],
-            )
+        #         orjson.loads(msg.edit_history),
+        #     )
 
     def prepare_move_topics(
         self,
@@ -709,9 +710,16 @@ class EditMessageTest(EditMessageTestCase):
         history data structures."""
         self.login("hamlet")
         hamlet = self.example_user("hamlet")
+        stream_1 = self.make_stream("stream 1")
+        stream_2 = self.make_stream("stream 2")
+        stream_3 = self.make_stream("stream 3")
+        self.subscribe(hamlet, stream_1.name)
+        self.subscribe(hamlet, stream_2.name)
+        self.subscribe(hamlet, stream_3.name)
         msg_id = self.send_stream_message(
-            self.example_user("hamlet"), "Denmark", topic_name="topic 1", content="content 1"
+            self.example_user("hamlet"), "stream 1", topic_name="topic 1", content="content 1"
         )
+
         result = self.client_patch(
             f"/json/messages/{msg_id}",
             {
@@ -742,9 +750,29 @@ class EditMessageTest(EditMessageTestCase):
         self.assert_json_success(result)
         history = orjson.loads(Message.objects.get(id=msg_id).edit_history)
         self.assertEqual(history[0][LEGACY_PREV_TOPIC], "topic 1")
+        self.assertEqual(history[0]["prev_topic"], "topic 1")
+        self.assertEqual(history[0]["topic"], "topic 2")
         self.assertEqual(history[0]["user_id"], hamlet.id)
-        self.assertEqual(set(history[0].keys()), {"timestamp", LEGACY_PREV_TOPIC, "user_id"})
+        self.assertEqual(
+            set(history[0].keys()),
+            {"timestamp", LEGACY_PREV_TOPIC, "prev_topic", "topic", "user_id"},
+        )
 
+        self.login("iago")
+        result = self.client_patch(
+            f"/json/messages/{msg_id}",
+            {
+                "stream_id": stream_2.id,
+            },
+        )
+        self.assert_json_success(result)
+        history = orjson.loads(Message.objects.get(id=msg_id).edit_history)
+        self.assertEqual(history[0]["prev_stream"], stream_1.id)
+        self.assertEqual(history[0]["stream"], stream_2.id)
+        self.assertEqual(history[0]["user_id"], self.example_user("iago").id)
+        self.assertEqual(set(history[0].keys()), {"timestamp", "prev_stream", "stream", "user_id"})
+
+        self.login("hamlet")
         result = self.client_patch(
             f"/json/messages/{msg_id}",
             {
@@ -756,12 +784,16 @@ class EditMessageTest(EditMessageTestCase):
         history = orjson.loads(Message.objects.get(id=msg_id).edit_history)
         self.assertEqual(history[0]["prev_content"], "content 2")
         self.assertEqual(history[0][LEGACY_PREV_TOPIC], "topic 2")
+        self.assertEqual(history[0]["prev_topic"], "topic 2")
+        self.assertEqual(history[0]["topic"], "topic 3")
         self.assertEqual(history[0]["user_id"], hamlet.id)
         self.assertEqual(
             set(history[0].keys()),
             {
                 "timestamp",
                 LEGACY_PREV_TOPIC,
+                "prev_topic",
+                "topic",
                 "prev_content",
                 "user_id",
                 "prev_rendered_content",
@@ -785,20 +817,55 @@ class EditMessageTest(EditMessageTestCase):
             f"/json/messages/{msg_id}",
             {
                 "topic": "topic 4",
+                "stream_id": stream_3.id,
             },
         )
         self.assert_json_success(result)
         history = orjson.loads(Message.objects.get(id=msg_id).edit_history)
         self.assertEqual(history[0][LEGACY_PREV_TOPIC], "topic 3")
+        self.assertEqual(history[0]["prev_topic"], "topic 3")
+        self.assertEqual(history[0]["topic"], "topic 4")
+        self.assertEqual(history[0]["prev_stream"], stream_2.id)
+        self.assertEqual(history[0]["stream"], stream_3.id)
         self.assertEqual(history[0]["user_id"], self.example_user("iago").id)
+        self.assertEqual(
+            set(history[0].keys()),
+            {
+                "timestamp",
+                LEGACY_PREV_TOPIC,
+                "prev_topic",
+                "topic",
+                "prev_stream",
+                "stream",
+                "user_id",
+            },
+        )
 
+        # Now, we verify that all of the edits stored in the message.edit_history
+        # have the correct data structure
         history = orjson.loads(Message.objects.get(id=msg_id).edit_history)
         self.assertEqual(history[0][LEGACY_PREV_TOPIC], "topic 3")
         self.assertEqual(history[2][LEGACY_PREV_TOPIC], "topic 2")
-        self.assertEqual(history[3][LEGACY_PREV_TOPIC], "topic 1")
+        self.assertEqual(history[4][LEGACY_PREV_TOPIC], "topic 1")
+
+        self.assertEqual(history[0]["prev_topic"], "topic 3")
+        self.assertEqual(history[0]["topic"], "topic 4")
+        self.assertEqual(history[0]["stream"], stream_3.id)
+        self.assertEqual(history[0]["prev_stream"], stream_2.id)
+
         self.assertEqual(history[1]["prev_content"], "content 3")
+
+        self.assertEqual(history[2]["prev_topic"], "topic 2")
+        self.assertEqual(history[2]["topic"], "topic 3")
         self.assertEqual(history[2]["prev_content"], "content 2")
-        self.assertEqual(history[4]["prev_content"], "content 1")
+
+        self.assertEqual(history[3]["stream"], stream_2.id)
+        self.assertEqual(history[3]["prev_stream"], stream_1.id)
+
+        self.assertEqual(history[4]["prev_topic"], "topic 1")
+        self.assertEqual(history[4]["topic"], "topic 2")
+
+        self.assertEqual(history[5]["prev_content"], "content 1")
 
         # Now, we verify that the edit history data sent back has the
         # correct filled-out fields
@@ -811,35 +878,50 @@ class EditMessageTest(EditMessageTestCase):
         i = 0
         for entry in message_history:
             expected_entries = {"content", "rendered_content", "topic", "timestamp", "user_id"}
-            if i in {0, 2, 3}:
+            if i in {0, 2, 4}:
                 expected_entries.add("prev_topic")
-            if i in {1, 2, 4}:
+                expected_entries.add("topic")
+            if i in {1, 2, 5}:
                 expected_entries.add("prev_content")
                 expected_entries.add("prev_rendered_content")
                 expected_entries.add("content_html_diff")
+            if i in {0, 3}:
+                expected_entries.add("prev_stream")
+                # TODO: uncomment when stream field is added to API
+                # expected_entries.add("stream")
             i += 1
             self.assertEqual(expected_entries, set(entry.keys()))
-        self.assert_length(message_history, 6)
+        self.assert_length(message_history, 7)
-        self.assertEqual(message_history[0]["prev_topic"], "topic 3")
         self.assertEqual(message_history[0]["topic"], "topic 4")
-        self.assertEqual(message_history[1]["topic"], "topic 3")
+        self.assertEqual(message_history[0]["prev_topic"], "topic 3")
-        self.assertEqual(message_history[2]["topic"], "topic 3")
+        # self.assertEqual(message_history[0]["stream"], stream_3.id)
-        self.assertEqual(message_history[2]["prev_topic"], "topic 2")
+        self.assertEqual(message_history[0]["prev_stream"], stream_2.id)
-        self.assertEqual(message_history[3]["topic"], "topic 2")
-        self.assertEqual(message_history[3]["prev_topic"], "topic 1")
-        self.assertEqual(message_history[4]["topic"], "topic 1")
-
         self.assertEqual(message_history[0]["content"], "content 4")
+
+        self.assertEqual(message_history[1]["topic"], "topic 3")
         self.assertEqual(message_history[1]["content"], "content 4")
         self.assertEqual(message_history[1]["prev_content"], "content 3")
+
+        self.assertEqual(message_history[2]["topic"], "topic 3")
+        self.assertEqual(message_history[2]["prev_topic"], "topic 2")
         self.assertEqual(message_history[2]["content"], "content 3")
         self.assertEqual(message_history[2]["prev_content"], "content 2")
-        self.assertEqual(message_history[3]["content"], "content 2")
-        self.assertEqual(message_history[4]["content"], "content 2")
-        self.assertEqual(message_history[4]["prev_content"], "content 1")
 
-        self.assertEqual(message_history[5]["content"], "content 1")
+        self.assertEqual(message_history[3]["topic"], "topic 2")
+        # self.assertEqual(message_history[3]["stream"], stream_2.id)
+        self.assertEqual(message_history[3]["prev_stream"], stream_1.id)
+        self.assertEqual(message_history[3]["content"], "content 2")
+
+        self.assertEqual(message_history[4]["topic"], "topic 2")
+        self.assertEqual(message_history[4]["prev_topic"], "topic 1")
+        self.assertEqual(message_history[4]["content"], "content 2")
+
         self.assertEqual(message_history[5]["topic"], "topic 1")
+        self.assertEqual(message_history[5]["content"], "content 2")
+        self.assertEqual(message_history[5]["prev_content"], "content 1")
+
+        self.assertEqual(message_history[6]["content"], "content 1")
+        self.assertEqual(message_history[6]["topic"], "topic 1")
 
     def test_edit_message_content_limit(self) -> None:
         def set_message_editing_params(