From e231a03eff6cce62243284ebcf7f06f798715a78 Mon Sep 17 00:00:00 2001
From: Tim Abbott <tabbott@zulip.com>
Date: Mon, 21 Jun 2021 12:27:26 -0700
Subject: [PATCH] message_edit: Fix non-alternating resolve topic
 notifications.

Previously, it was possible for an unusual series of topic-edit
actions to result in Notification Bot reporting that a topic was
marked as resolved that had already been marked as resolved, etc.
---
 zerver/lib/actions.py             | 28 ++++++++++++++++++++++++++--
 zerver/tests/test_message_edit.py | 26 ++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py
index c29b163768..2181252050 100644
--- a/zerver/lib/actions.py
+++ b/zerver/lib/actions.py
@@ -5643,13 +5643,37 @@ def maybe_send_resolve_topic_notifications(
     old_topic: str,
     new_topic: str,
 ) -> None:
+    # Note that topics will have already been stripped in check_update_message.
+    #
+    # This logic is designed to treat removing a weird "✔ ✔✔ "
+    # prefix as unresolving the topic.
     if old_topic.lstrip(RESOLVED_TOPIC_PREFIX) != new_topic.lstrip(RESOLVED_TOPIC_PREFIX):
         return
 
-    if new_topic.startswith(RESOLVED_TOPIC_PREFIX):
+    if new_topic.startswith(RESOLVED_TOPIC_PREFIX) and not old_topic.startswith(
+        RESOLVED_TOPIC_PREFIX
+    ):
         notification_string = _("{user} has marked this topic as resolved.")
-    else:
+    elif old_topic.startswith(RESOLVED_TOPIC_PREFIX) and not new_topic.startswith(
+        RESOLVED_TOPIC_PREFIX
+    ):
         notification_string = _("{user} has marked this topic as unresolved.")
+    else:
+        # If there's some other weird topic that does not toggle the
+        # state of "topic starts with RESOLVED_TOPIC_PREFIX", we do
+        # nothing. Any other logic could result in cases where we send
+        # these notifications in a non-alternating fashion.
+        #
+        # Note that it is still possible for an individual topic to
+        # have multiple "This topic was marked as resolved"
+        # notifications in a row: one can send new messages to the
+        # pre-resolve topic and then resolve the topic created that
+        # way to get multiple in the resolved topic. And then an
+        # administrator can the messages in between. We consider this
+        # to be a fundamental risk of irresponsible message deletion,
+        # not a bug with the "resolve topics" feature.
+        return
+
     sender = get_system_bot(settings.NOTIFICATION_BOT)
     user_mention = f"@_**{user_profile.full_name}|{user_profile.id}**"
     with override_language(stream.realm.default_language):
diff --git a/zerver/tests/test_message_edit.py b/zerver/tests/test_message_edit.py
index 268c378631..6d2f507abb 100644
--- a/zerver/tests/test_message_edit.py
+++ b/zerver/tests/test_message_edit.py
@@ -2005,6 +2005,32 @@ class EditMessageTest(EditMessageTestCase):
             f"@_**Iago|{admin_user.id}** has marked this topic as resolved.",
         )
 
+        # Now move to a weird state and confirm no new messages
+        weird_topic = "✔ ✔✔" + original_topic
+        result = self.client_patch(
+            "/json/messages/" + str(id1),
+            {
+                "message_id": id1,
+                "topic": weird_topic,
+                "propagate_mode": "change_all",
+            },
+        )
+
+        self.assert_json_success(result)
+        for msg_id in [id1, id2]:
+            msg = Message.objects.get(id=msg_id)
+            self.assertEqual(
+                weird_topic,
+                msg.topic_name(),
+            )
+
+        messages = get_topic_messages(admin_user, stream, weird_topic)
+        self.assert_length(messages, 3)
+        self.assertEqual(
+            messages[2].content,
+            f"@_**Iago|{admin_user.id}** has marked this topic as resolved.",
+        )
+
         unresolved_topic = original_topic
         result = self.client_patch(
             "/json/messages/" + str(id1),