From e566e985e4d2e824ecbafef74f171883e3a77b2a Mon Sep 17 00:00:00 2001 From: Aman Agrawal Date: Mon, 28 Dec 2020 10:30:07 +0000 Subject: [PATCH] topic_edit: Store edit history in all the message affected. Instead of just storing the edit history in the message which triggered the topic edit, we store the edit history in all the messages that changed. This helps users track the edit history of a message more reliably. --- zerver/lib/actions.py | 2 ++ zerver/lib/topic.py | 24 ++++++++++--- zerver/tests/test_message_edit.py | 56 +++++++++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 5 deletions(-) diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 7a77a4aa37..ed47e394da 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -4734,6 +4734,8 @@ def do_update_message(user_profile: UserProfile, message: Message, orig_topic_name=orig_topic_name, topic_name=topic_name, new_stream=new_stream, + edit_history_event=edit_history_event, + last_edit_time=timestamp ) changed_messages += messages_list diff --git a/zerver/lib/topic.py b/zerver/lib/topic.py index cf63bec035..79df3b079a 100644 --- a/zerver/lib/topic.py +++ b/zerver/lib/topic.py @@ -1,5 +1,7 @@ +from datetime import datetime from typing import Any, Dict, List, Optional, Tuple +import orjson from django.db import connection from django.db.models.query import Q, QuerySet from sqlalchemy import Text @@ -108,7 +110,10 @@ def update_messages_for_topic_edit(message: Message, propagate_mode: str, orig_topic_name: str, topic_name: Optional[str], - new_stream: Optional[Stream]) -> List[Message]: + new_stream: Optional[Stream], + edit_history_event: Dict[str, Any], + last_edit_time: datetime) -> List[Message]: + propagate_query = Q(recipient = message.recipient, subject__iexact = orig_topic_name) if propagate_mode == 'change_all': propagate_query = propagate_query & ~Q(id = message.id) @@ -117,7 +122,7 @@ def update_messages_for_topic_edit(message: Message, messages = Message.objects.filter(propagate_query).select_related() - update_fields: Dict[str, object] = {} + update_fields = ['edit_history', 'last_edit_time'] # Evaluate the query before running the update messages_list = list(messages) @@ -127,15 +132,24 @@ def update_messages_for_topic_edit(message: Message, # caller) requires the new value, so we manually update the # objects in addition to sending a bulk query to the database. if new_stream is not None: - update_fields["recipient"] = new_stream.recipient + update_fields.append("recipient") for m in messages_list: m.recipient = new_stream.recipient if topic_name is not None: - update_fields["subject"] = topic_name + update_fields.append("subject") for m in messages_list: m.set_topic_name(topic_name) - messages.update(**update_fields) + for message in messages_list: + message.last_edit_time = last_edit_time + if message.edit_history is not None: + edit_history = orjson.loads(message.edit_history) + edit_history.insert(0, edit_history_event) + else: + edit_history = [edit_history_event] + message.edit_history = orjson.dumps(edit_history).decode() + + Message.objects.bulk_update(messages_list, update_fields) return messages_list diff --git a/zerver/tests/test_message_edit.py b/zerver/tests/test_message_edit.py index 9fbc13a106..999c54cc86 100644 --- a/zerver/tests/test_message_edit.py +++ b/zerver/tests/test_message_edit.py @@ -759,6 +759,62 @@ class EditMessageTest(ZulipTestCase): called = True self.assertTrue(called) + def test_topic_edit_history_saved_in_all_message(self) -> None: + self.login('hamlet') + id1 = self.send_stream_message(self.example_user("hamlet"), "Scotland", + topic_name="topic1") + id2 = self.send_stream_message(self.example_user("iago"), "Scotland", + topic_name="topic1") + id3 = self.send_stream_message(self.example_user("iago"), "Rome", + topic_name="topic1") + id4 = self.send_stream_message(self.example_user("hamlet"), "Scotland", + topic_name="topic2") + id5 = self.send_stream_message(self.example_user("iago"), "Scotland", + topic_name="topic1") + + def verify_edit_history(new_topic: str, len_edit_history: int) -> None: + for msg_id in [id1, id2, id5]: + msg = Message.objects.get(id=msg_id) + + self.assertEqual( + new_topic, + msg.topic_name(), + ) + # Since edit history is being generated by do_update_message, + # it's contents can vary over time; So, to keep this test + # future proof, we only verify it's length. + self.assertEqual( + len(orjson.loads(msg.edit_history)), + len_edit_history + ) + + for msg_id in [id3, id4]: + msg = Message.objects.get(id=msg_id) + self.assertEqual( + msg.edit_history, + None + ) + + new_topic = 'edited' + result = self.client_patch("/json/messages/" + str(id1), { + 'message_id': id1, + 'topic': new_topic, + 'propagate_mode': 'change_later', + }) + + self.assert_json_success(result) + verify_edit_history(new_topic, 1) + + new_topic = 'edited2' + result = self.client_patch("/json/messages/" + str(id1), { + 'message_id': id1, + 'topic': new_topic, + 'propagate_mode': 'change_later', + }) + + self.assert_json_success(result) + verify_edit_history(new_topic, 2) + def test_propagate_topic_forward(self) -> None: self.login('hamlet') id1 = self.send_stream_message(self.example_user("hamlet"), "Scotland",