From 79069b5dec382e6a1c25d8aee55db011af239daf Mon Sep 17 00:00:00 2001 From: Shubh Gupta Date: Sun, 16 Jan 2022 03:03:32 +0530 Subject: [PATCH] message edit: Indicate how many messages were moved in notificactions. When moving only part of a topic, it's useful to display that information to users in these notifications so that it's clear what's happening. The most important consequence is actually just increasing confidence that when you see that the whole topic was moved, that's accurate. Substantially modified by tabbott. Fixes #20575. --- zerver/lib/actions.py | 36 +++++++++++--- zerver/tests/test_message_edit.py | 79 +++++++++++++++++++++++++++---- 2 files changed, 100 insertions(+), 15 deletions(-) diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 88bba79e1d..703a9b99c2 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -6488,6 +6488,7 @@ def send_message_moved_breadcrumbs( new_stream: Stream, new_topic: Optional[str], new_thread_notification_string: Optional[str], + changed_messages_count: int, ) -> None: # Since moving content between streams is highly disruptive, # it's worth adding a couple tombstone messages showing what @@ -6510,6 +6511,7 @@ def send_message_moved_breadcrumbs( new_thread_notification_string.format( old_location=old_topic_link, user=user_mention, + changed_messages_count=changed_messages_count, ), ) @@ -6523,6 +6525,7 @@ def send_message_moved_breadcrumbs( old_thread_notification_string.format( user=user_mention, new_location=new_topic_link, + changed_messages_count=changed_messages_count, ), ) @@ -7017,17 +7020,37 @@ def do_update_message( if len(changed_messages) > 0 and new_stream is not None and stream_being_edited is not None: # Notify users that the topic was moved. + changed_messages_count = len(changed_messages) + old_thread_notification_string = None if send_notification_to_old_thread: - old_thread_notification_string = gettext_lazy( - "This topic was moved by {user} to {new_location}" - ) + if propagate_mode == "change_all": + old_thread_notification_string = gettext_lazy( + "This topic was moved to {new_location} by {user}." + ) + elif propagate_mode == "change_one": + old_thread_notification_string = gettext_lazy( + "A message was moved from this topic to {new_location} by {user}." + ) + else: + old_thread_notification_string = gettext_lazy( + "{changed_messages_count} messages were moved from this topic to {new_location} by {user}." + ) new_thread_notification_string = None if send_notification_to_new_thread: - new_thread_notification_string = gettext_lazy( - "This topic was moved here from {old_location} by {user}" - ) + if propagate_mode == "change_all": + new_thread_notification_string = gettext_lazy( + "This topic was moved here from {old_location} by {user}." + ) + elif propagate_mode == "change_one": + new_thread_notification_string = gettext_lazy( + "A message was moved here from {old_location} by {user}." + ) + else: + new_thread_notification_string = gettext_lazy( + "{changed_messages_count} messages were moved here from {old_location} by {user}." + ) send_message_moved_breadcrumbs( user_profile, @@ -7037,6 +7060,7 @@ def do_update_message( new_stream, topic_name, new_thread_notification_string, + changed_messages_count, ) if ( diff --git a/zerver/tests/test_message_edit.py b/zerver/tests/test_message_edit.py index 1e5a60675f..ecca4fb420 100644 --- a/zerver/tests/test_message_edit.py +++ b/zerver/tests/test_message_edit.py @@ -1428,14 +1428,14 @@ class EditMessageTest(EditMessageTestCase): self.assert_length(messages, 1) self.assertEqual( messages[0].content, - f"This topic was moved by @_**Iago|{user_profile.id}** to #**new stream>test**", + f"This topic was moved to #**new stream>test** by @_**Iago|{user_profile.id}**.", ) messages = get_topic_messages(user_profile, new_stream, "test") self.assert_length(messages, 4) self.assertEqual( messages[3].content, - f"This topic was moved here from #**test move stream>test** by @_**Iago|{user_profile.id}**", + f"This topic was moved here from #**test move stream>test** by @_**Iago|{user_profile.id}**.", ) def test_move_message_realm_admin_cant_move_to_another_realm(self) -> None: @@ -1580,7 +1580,7 @@ class EditMessageTest(EditMessageTestCase): self.assertEqual(messages[0].id, msg_id) self.assertEqual( messages[1].content, - f"This topic was moved by @_**Iago|{user_profile.id}** to #**new stream>test**", + f"2 messages were moved from this topic to #**new stream>test** by @_**Iago|{user_profile.id}**.", ) messages = get_topic_messages(user_profile, new_stream, "test") @@ -1588,7 +1588,68 @@ class EditMessageTest(EditMessageTestCase): self.assertEqual(messages[0].id, msg_id_later) self.assertEqual( messages[2].content, - f"This topic was moved here from #**test move stream>test** by @_**Iago|{user_profile.id}**", + f"2 messages were moved here from #**test move stream>test** by @_**Iago|{user_profile.id}**.", + ) + + def test_move_message_to_stream_change_one(self) -> None: + (user_profile, old_stream, new_stream, msg_id, msg_id_later) = self.prepare_move_topics( + "iago", "test move stream", "new stream", "test" + ) + + result = self.client_patch( + "/json/messages/" + str(msg_id_later), + { + "message_id": msg_id_later, + "stream_id": new_stream.id, + "propagate_mode": "change_one", + }, + ) + self.assert_json_success(result) + + messages = get_topic_messages(user_profile, old_stream, "test") + self.assert_length(messages, 3) + self.assertEqual(messages[0].id, msg_id) + self.assertEqual( + messages[2].content, + f"A message was moved from this topic to #**new stream>test** by @_**Iago|{user_profile.id}**.", + ) + + messages = get_topic_messages(user_profile, new_stream, "test") + self.assert_length(messages, 2) + self.assertEqual(messages[0].id, msg_id_later) + self.assertEqual( + messages[1].content, + f"A message was moved here from #**test move stream>test** by @_**Iago|{user_profile.id}**.", + ) + + def test_move_message_to_stream_change_all(self) -> None: + (user_profile, old_stream, new_stream, msg_id, msg_id_later) = self.prepare_move_topics( + "iago", "test move stream", "new stream", "test" + ) + + result = self.client_patch( + "/json/messages/" + str(msg_id_later), + { + "message_id": msg_id_later, + "stream_id": new_stream.id, + "propagate_mode": "change_all", + }, + ) + self.assert_json_success(result) + + messages = get_topic_messages(user_profile, old_stream, "test") + self.assert_length(messages, 1) + self.assertEqual( + messages[0].content, + f"This topic was moved to #**new stream>test** by @_**Iago|{user_profile.id}**.", + ) + + messages = get_topic_messages(user_profile, new_stream, "test") + self.assert_length(messages, 4) + self.assertEqual(messages[0].id, msg_id) + self.assertEqual( + messages[3].content, + f"This topic was moved here from #**test move stream>test** by @_**Iago|{user_profile.id}**.", ) def test_move_message_between_streams_policy_setting(self) -> None: @@ -1832,14 +1893,14 @@ class EditMessageTest(EditMessageTestCase): self.assert_length(messages, 1) self.assertEqual( messages[0].content, - f"This topic was moved by @_**Iago|{user_profile.id}** to #**new stream>new topic**", + f"This topic was moved to #**new stream>new topic** by @_**Iago|{user_profile.id}**.", ) messages = get_topic_messages(user_profile, new_stream, "new topic") self.assert_length(messages, 4) self.assertEqual( messages[3].content, - f"This topic was moved here from #**test move stream>test** by @_**Iago|{user_profile.id}**", + f"This topic was moved here from #**test move stream>test** by @_**Iago|{user_profile.id}**.", ) self.assert_json_success(result) @@ -2006,7 +2067,7 @@ class EditMessageTest(EditMessageTestCase): self.assert_length(messages, 4) self.assertEqual( messages[3].content, - f"This topic was moved here from #**test move stream>test** by @_**Iago|{user_profile.id}**", + f"This topic was moved here from #**test move stream>test** by @_**Iago|{user_profile.id}**.", ) def test_notify_old_thread_move_message_to_stream(self) -> None: @@ -2031,7 +2092,7 @@ class EditMessageTest(EditMessageTestCase): self.assert_length(messages, 1) self.assertEqual( messages[0].content, - f"This topic was moved by @_**Iago|{user_profile.id}** to #**new stream>test**", + f"This topic was moved to #**new stream>test** by @_**Iago|{user_profile.id}**.", ) messages = get_topic_messages(user_profile, new_stream, "test") @@ -2096,7 +2157,7 @@ class EditMessageTest(EditMessageTestCase): self.assert_length(messages, 1) self.assertEqual( messages[0].content, - f"This topic was moved by @_**Iago|{admin_user.id}** to #**new stream>test**", + f"This topic was moved to #**new stream>test** by @_**Iago|{admin_user.id}**.", ) messages = get_topic_messages(admin_user, new_stream, "test")