bots: Change logic for notification bot after moved message.

Previously, when a user moves a message to another topic, the Notification
bot will post a message saying "This topic was moved here from..." This is
confusing when the topic already contains messages. The changes aims to make
the messages more clear by changing the logic for the Notification bot. When
there is already messages in the topic, the bot will post "A message was
moved here from..." or "N messages were moved here from...". The bot will
post "This topic was moved here from (somewhere) by (someone)." when the
topic is empty.

Fixes #23267.

zerver/actions/message_edit.py

@@ -813,7 +813,7 @@ def do_update_message(
 
     send_event(user_profile.realm, event, users_to_be_notified)
 
-    sent_resolve_topic_notification = None
+    resolved_topic_message_id = None
     if topic_name is not None and content is None and len(changed_messages) > 0:
         # When stream is changed and topic is marked as resolved or unresolved
         # in the same API request, resolved or unresolved notification should
@@ -824,7 +824,7 @@ def do_update_message(
             stream_to_send_resolve_topic_notification = new_stream
 
         assert stream_to_send_resolve_topic_notification is not None
-        sent_resolve_topic_notification = maybe_send_resolve_topic_notifications(
+        resolved_topic_message_id = maybe_send_resolve_topic_notifications(
             user_profile=user_profile,
             stream=stream_to_send_resolve_topic_notification,
             old_topic=orig_topic_name,
@@ -867,18 +867,50 @@ def do_update_message(
         new_thread_notification_string = None
         if send_notification_to_new_thread and (
             new_stream is not None
-            or not sent_resolve_topic_notification
+            or not resolved_topic_message_id
             or (
                 pre_truncation_topic_name is not None
                 and orig_topic_name.lstrip(RESOLVED_TOPIC_PREFIX)
                 != pre_truncation_topic_name.lstrip(RESOLVED_TOPIC_PREFIX)
             )
         ):
-            if moved_all_visible_messages:
+            stream_for_new_topic = new_stream if new_stream is not None else stream_being_edited
+            assert stream_for_new_topic.recipient_id is not None
+
+            new_topic = topic_name if topic_name is not None else orig_topic_name
+
+            changed_message_ids = [changed_message.id for changed_message in changed_messages]
+
+            # We calculate whether the user moved the entire topic
+            # using that user's own permissions, which is important to
+            # avoid leaking information about whether there are
+            # messages in the destination topic's deeper history that
+            # the acting user does not have permission to access.
+            #
+            # TODO: These queries are quite inefficient, in that we're
+            # fetching full copies of all the messages in the
+            # destination topic to answer the question of whether the
+            # current user has access to at least one such message.
+            #
+            # The main strength of the current implementation is that
+            # it reuses existing logic, which is good for keeping it
+            # correct as we maintain the codebase.
+            preexisting_topic_messages = messages_for_topic(
+                stream_for_new_topic.recipient_id, new_topic
+            ).exclude(id__in=[*changed_message_ids, resolved_topic_message_id])
+
+            visible_preexisting_messages = bulk_access_messages(
+                user_profile, preexisting_topic_messages, stream=stream_for_new_topic
+            )
+
+            no_visible_preexisting_messages = len(visible_preexisting_messages) == 0
+
+            if no_visible_preexisting_messages and moved_all_visible_messages:
                 new_thread_notification_string = gettext_lazy(
                     "This topic was moved here from {old_location} by {user}."
                 )
-            elif changed_messages_count == 1:
+            else:
+                if changed_messages_count == 1:
                     new_thread_notification_string = gettext_lazy(
                         "A message was moved here from {old_location} by {user}."
                     )

zerver/tests/test_message_edit.py

@@ -1822,6 +1822,50 @@ class EditMessageTest(EditMessageTestCase):
             f"This topic was moved here from #**test move stream>test** by @_**Iago|{user_profile.id}**.",
         )
 
+    def test_move_message_to_preexisting_topic(self) -> None:
+        (user_profile, old_stream, new_stream, msg_id, msg_id_lt) = self.prepare_move_topics(
+            "iago",
+            "test move stream",
+            "new stream",
+            "test",
+            # Set the user's translation language to German to test that
+            # it is overridden by the realm's default language.
+            "de",
+        )
+
+        self.send_stream_message(
+            sender=self.example_user("iago"),
+            stream_name="new stream",
+            topic_name="test",
+            content="Always here",
+        )
+
+        result = self.client_patch(
+            f"/json/messages/{msg_id}",
+            {
+                "stream_id": new_stream.id,
+                "propagate_mode": "change_all",
+                "send_notification_to_old_thread": "true",
+            },
+            HTTP_ACCEPT_LANGUAGE="de",
+        )
+
+        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, 5)
+        self.assertEqual(
+            messages[4].content,
+            f"3 messages were 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:
         user_profile = self.example_user("iago")
         self.assertEqual(user_profile.role, UserProfile.ROLE_REALM_ADMINISTRATOR)
@@ -1971,6 +2015,44 @@ class EditMessageTest(EditMessageTestCase):
             f"2 messages were moved here from #**test move stream>test** by @_**Iago|{user_profile.id}**.",
         )
 
+    def test_move_message_to_preexisting_topic_change_later(self) -> None:
+        (user_profile, old_stream, new_stream, msg_id, msg_id_later) = self.prepare_move_topics(
+            "iago", "test move stream", "new stream", "test"
+        )
+
+        self.send_stream_message(
+            sender=self.example_user("iago"),
+            stream_name="new stream",
+            topic_name="test",
+            content="Always here",
+        )
+
+        result = self.client_patch(
+            f"/json/messages/{msg_id_later}",
+            {
+                "stream_id": new_stream.id,
+                "propagate_mode": "change_later",
+                "send_notification_to_old_thread": "true",
+            },
+        )
+        self.assert_json_success(result)
+
+        messages = get_topic_messages(user_profile, old_stream, "test")
+        self.assert_length(messages, 2)
+        self.assertEqual(messages[0].id, msg_id)
+        self.assertEqual(
+            messages[1].content,
+            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")
+        self.assert_length(messages, 4)
+        self.assertEqual(messages[0].id, msg_id_later)
+        self.assertEqual(
+            messages[3].content,
+            f"2 messages were moved here from #**test move stream>test** by @_**Iago|{user_profile.id}**.",
+        )
+
     def test_move_message_to_stream_change_later_all_moved(self) -> None:
         (user_profile, old_stream, new_stream, msg_id, msg_id_later) = self.prepare_move_topics(
             "iago", "test move stream", "new stream", "test"
@@ -2001,6 +2083,43 @@ class EditMessageTest(EditMessageTestCase):
             f"This topic was moved here from #**test move stream>test** by @_**Iago|{user_profile.id}**.",
         )
 
+    def test_move_message_to_preexisting_topic_change_later_all_moved(self) -> None:
+        (user_profile, old_stream, new_stream, msg_id, msg_id_later) = self.prepare_move_topics(
+            "iago", "test move stream", "new stream", "test"
+        )
+
+        self.send_stream_message(
+            sender=self.example_user("iago"),
+            stream_name="new stream",
+            topic_name="test",
+            content="Always here",
+        )
+
+        result = self.client_patch(
+            f"/json/messages/{msg_id}",
+            {
+                "stream_id": new_stream.id,
+                "propagate_mode": "change_later",
+                "send_notification_to_old_thread": "true",
+            },
+        )
+        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, 5)
+        self.assertEqual(messages[0].id, msg_id)
+        self.assertEqual(
+            messages[4].content,
+            f"3 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"
@@ -2032,6 +2151,44 @@ class EditMessageTest(EditMessageTestCase):
             f"A message was moved here from #**test move stream>test** by @_**Iago|{user_profile.id}**.",
         )
 
+    def test_move_message_to_preexisting_topic_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"
+        )
+
+        self.send_stream_message(
+            sender=self.example_user("iago"),
+            stream_name="new stream",
+            topic_name="test",
+            content="Always here",
+        )
+
+        result = self.client_patch(
+            "/json/messages/" + str(msg_id_later),
+            {
+                "stream_id": new_stream.id,
+                "propagate_mode": "change_one",
+                "send_notification_to_old_thread": "true",
+            },
+        )
+        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, 3)
+        self.assertEqual(messages[0].id, msg_id_later)
+        self.assertEqual(
+            messages[2].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"
@@ -2062,6 +2219,43 @@ class EditMessageTest(EditMessageTestCase):
             f"This topic was moved here from #**test move stream>test** by @_**Iago|{user_profile.id}**.",
         )
 
+    def test_move_message_to_preexisting_topic_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"
+        )
+
+        self.send_stream_message(
+            sender=self.example_user("iago"),
+            stream_name="new stream",
+            topic_name="test",
+            content="Always here",
+        )
+
+        result = self.client_patch(
+            "/json/messages/" + str(msg_id_later),
+            {
+                "stream_id": new_stream.id,
+                "propagate_mode": "change_all",
+                "send_notification_to_old_thread": "true",
+            },
+        )
+        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, 5)
+        self.assertEqual(messages[0].id, msg_id)
+        self.assertEqual(
+            messages[4].content,
+            f"3 messages were moved here from #**test move stream>test** by @_**Iago|{user_profile.id}**.",
+        )
+
     def test_move_message_between_streams_policy_setting(self) -> None:
         (user_profile, old_stream, new_stream, msg_id, msg_id_later) = self.prepare_move_topics(
             "othello", "old_stream_1", "new_stream_1", "test"
@@ -2380,7 +2574,7 @@ class EditMessageTest(EditMessageTestCase):
             "iago", "test move stream", "new stream", "test"
         )
 
-        with self.assert_database_query_count(53), cache_tries_captured() as cache_tries:
+        with self.assert_database_query_count(55), cache_tries_captured() as cache_tries:
             result = self.client_patch(
                 f"/json/messages/{msg_id}",
                 {