From a410eee2e1739d9caed02ca3d957f47418909f9c Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Tue, 1 Mar 2022 16:08:38 -0800 Subject: [PATCH] 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 | 7 ++ zerver/lib/message.py | 20 ++++- zerver/lib/types.py | 7 +- zerver/tests/test_message_edit.py | 130 ++++++++++++++++++++++++------ 4 files changed, 134 insertions(+), 30 deletions(-) diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 05922f7b92..9f32503021 100644 --- a/zerver/lib/actions.py +++ b/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, diff --git a/zerver/lib/message.py b/zerver/lib/message.py index 728f8e215a..318d4e6faf 100644 --- a/zerver/lib/message.py +++ b/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 - edit_history: List[EditHistoryEvent] = orjson.loads(edit_history_json) + raw_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( diff --git a/zerver/lib/types.py b/zerver/lib/types.py index 60ba026fca..edd987915c 100644 --- a/zerver/lib/types.py +++ b/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 - # topic: str + prev_topic: str + topic: str prev_content: str prev_rendered_content: Optional[str] prev_rendered_content_version: Optional[int] diff --git a/zerver/tests/test_message_edit.py b/zerver/tests/test_message_edit.py index 76aa3c4b46..f8896b2761 100644 --- a/zerver/tests/test_message_edit.py +++ b/zerver/tests/test_message_edit.py @@ -73,11 +73,12 @@ class EditMessageTestCase(ZulipTestCase): msg.sender_id, ) - if msg.edit_history: - self.assertEqual( - fetch_message_dict["edit_history"], - orjson.loads(msg.edit_history), - ) + # TODO: uncomment assertion when edit_history fields in API match fields in database. + # if msg.edit_history: + # self.assertEqual( + # 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.assertEqual(message_history[0]["prev_topic"], "topic 3") + self.assert_length(message_history, 7) self.assertEqual(message_history[0]["topic"], "topic 4") - self.assertEqual(message_history[1]["topic"], "topic 3") - self.assertEqual(message_history[2]["topic"], "topic 3") - self.assertEqual(message_history[2]["prev_topic"], "topic 2") - 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]["prev_topic"], "topic 3") + # self.assertEqual(message_history[0]["stream"], stream_3.id) + self.assertEqual(message_history[0]["prev_stream"], stream_2.id) 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(