resolve topic: Add resolve topic undo grace period.

Currently we send a notification to the topic if it has been resolved
or unresolved even if there is an immediate event of resolving and
then unresolving or vice-versa. This adds a setting of
RESOLVE_TOPIC_UNDO_GRACE_PERIOD_SECONDS under which if a topic has
been unresolved after being resolved immediately and the last message
was the notification of resolving, then delete the last message and
don't send a new notification and vice-versa.

We use the new message.type field to precisely identify relevant
messages.

Fixes zulip#19181.

zerver/lib/actions.py

@@ -5735,6 +5735,26 @@ class MessageUpdateUserInfoResult(TypedDict):
     mention_user_ids: Set[int]
 
 
+def confirm_topic_resolve_grace_period(stream: Stream, topic: str) -> bool:
+    message_args = {"recipient": stream.recipient, TOPIC_NAME: topic}
+    last_message = Message.objects.filter(**message_args).last()
+
+    if last_message is None:
+        return False
+
+    if last_message.type != Message.MessageType.RESOLVE_TOPIC_NOTIFICATION:
+        return False
+
+    current_time = timezone_now()
+    time_difference = (current_time - last_message.date_sent).total_seconds()
+
+    if time_difference > settings.RESOLVE_TOPIC_UNDO_GRACE_PERIOD_SECONDS:
+        return False
+
+    do_delete_messages(stream.realm, [last_message])
+    return True
+
+
 def maybe_send_resolve_topic_notifications(
     *,
     user_profile: UserProfile,
@@ -5773,6 +5793,9 @@ def maybe_send_resolve_topic_notifications(
         # not a bug with the "resolve topics" feature.
         return
 
+    if confirm_topic_resolve_grace_period(stream, new_topic):
+        return
+
     sender = get_system_bot(settings.NOTIFICATION_BOT, user_profile.realm_id)
     user_mention = f"@_**{user_profile.full_name}|{user_profile.id}**"
     with override_language(stream.realm.default_language):

zerver/migrations/0337_add_message_type.py

@@ -16,6 +16,7 @@ class Migration(migrations.Migration):
             field=models.PositiveSmallIntegerField(
                 choices=[
                     (1, "Normal"),
+                    (2, "Resolve Topic Notification"),
                 ],
                 default=1,
             ),
@@ -26,6 +27,7 @@ class Migration(migrations.Migration):
             field=models.PositiveSmallIntegerField(
                 choices=[
                     (1, "Normal"),
+                    (2, "Resolve Topic Notification"),
                 ],
                 default=1,
             ),

zerver/tests/test_message_edit.py

@@ -6,6 +6,7 @@
 import orjson
 from django.db import IntegrityError
 from django.http import HttpResponse
+from django.test import override_settings
 from django.utils.timezone import now as timezone_now
 
 from zerver.lib.actions import (
@@ -16,6 +17,7 @@
     do_update_message,
     get_topic_messages,
     get_user_info_for_message_updates,
+    maybe_send_resolve_topic_notifications,
 )
 from zerver.lib.message import MessageDict, has_message_access, messages_for_ids
 from zerver.lib.test_classes import ZulipTestCase
@@ -1956,6 +1958,7 @@ def validation_func(user_profile: UserProfile) -> bool:
 
         self.check_has_permission_policies("move_messages_between_streams_policy", validation_func)
 
+    @override_settings(RESOLVE_TOPIC_UNDO_GRACE_PERIOD_SECONDS=60)
     def test_mark_topic_as_resolved(self) -> None:
         self.login("iago")
         admin_user = self.example_user("iago")
@@ -2031,15 +2034,91 @@ def test_mark_topic_as_resolved(self) -> None:
         )
 
         unresolved_topic = original_topic
+
+        # Now unresolve the topic after the grace period
+        with mock.patch(
+            "zerver.lib.actions.timezone_now",
+            return_value=messages[2].date_sent + datetime.timedelta(seconds=80),
+        ):
+            result = self.client_patch(
+                "/json/messages/" + str(id1),
+                {
+                    "message_id": id1,
+                    "topic": unresolved_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(
+                unresolved_topic,
+                msg.topic_name(),
+            )
+
+        messages = get_topic_messages(admin_user, stream, unresolved_topic)
+        self.assert_length(messages, 4)
+        self.assertEqual(
+            messages[3].content,
+            f"@_**Iago|{admin_user.id}** has marked this topic as unresolved.",
+        )
+
+    @override_settings(RESOLVE_TOPIC_UNDO_GRACE_PERIOD_SECONDS=60)
+    def test_mark_topic_as_resolved_within_grace_period(self) -> None:
+        self.login("iago")
+        admin_user = self.example_user("iago")
+        stream = self.make_stream("new")
+        self.subscribe(admin_user, stream.name)
+        original_topic = "topic 1"
+        id1 = self.send_stream_message(
+            self.example_user("hamlet"), "new", content="message 1", topic_name=original_topic
+        )
+        id2 = self.send_stream_message(
+            admin_user, "new", content="message 2", topic_name=original_topic
+        )
+
+        resolved_topic = RESOLVED_TOPIC_PREFIX + original_topic
         result = self.client_patch(
             "/json/messages/" + str(id1),
             {
                 "message_id": id1,
-                "topic": unresolved_topic,
+                "topic": resolved_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(
+                resolved_topic,
+                msg.topic_name(),
+            )
+
+        messages = get_topic_messages(admin_user, stream, resolved_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
+
+        # Now unresolve the topic within the grace period.
+        with mock.patch(
+            "zerver.lib.actions.timezone_now",
+            return_value=messages[2].date_sent + datetime.timedelta(seconds=4),
+        ):
+            result = self.client_patch(
+                "/json/messages/" + str(id1),
+                {
+                    "message_id": id1,
+                    "topic": unresolved_topic,
+                    "propagate_mode": "change_all",
+                },
+            )
+
         self.assert_json_success(result)
         for msg_id in [id1, id2]:
             msg = Message.objects.get(id=msg_id)
@@ -2049,10 +2128,38 @@ def test_mark_topic_as_resolved(self) -> None:
             )
 
         messages = get_topic_messages(admin_user, stream, unresolved_topic)
-        self.assert_length(messages, 4)
+        self.assert_length(messages, 2)
         self.assertEqual(
-            messages[3].content,
-            f"@_**Iago|{admin_user.id}** has marked this topic as unresolved.",
+            messages[1].content,
+            "message 2",
+        )
+
+    def test_send_resolve_topic_notification_with_no_topic_messages(self) -> None:
+        self.login("iago")
+        admin_user = self.example_user("iago")
+        stream = self.make_stream("new")
+        self.subscribe(admin_user, stream.name)
+        original_topic = "topic 1"
+        message_id = self.send_stream_message(
+            self.example_user("hamlet"), "new", content="message 1", topic_name=original_topic
+        )
+
+        message = Message.objects.get(id=message_id)
+        do_delete_messages(admin_user.realm, [message])
+
+        resolve_topic = RESOLVED_TOPIC_PREFIX + original_topic
+        maybe_send_resolve_topic_notifications(
+            user_profile=admin_user,
+            stream=stream,
+            old_topic=original_topic,
+            new_topic=resolve_topic,
+        )
+
+        topic_messages = get_topic_messages(admin_user, stream, resolve_topic)
+        self.assert_length(topic_messages, 1)
+        self.assertEqual(
+            topic_messages[0].content,
+            f"@_**Iago|{admin_user.id}** has marked this topic as resolved.",
         )
 
 

zproject/default_settings.py

@@ -455,3 +455,8 @@ class JwtAuthKey(TypedDict):
 # Any message content exceeding this limit will be truncated.
 # See: `_internal_prep_message` function in zerver/lib/actions.py.
 MAX_MESSAGE_LENGTH = 10000
+
+# Grace period before which we don't send a resolve/unresolve
+# notification to stream and also delete the previous counter
+# notification.
+RESOLVE_TOPIC_UNDO_GRACE_PERIOD_SECONDS = 60

zproject/dev_settings.py

@@ -176,3 +176,5 @@
 SOCIAL_AUTH_SAML_SP_ENTITY_ID = "http://localhost:9991"
 
 MEMCACHED_USERNAME = None
+
+RESOLVE_TOPIC_UNDO_GRACE_PERIOD_SECONDS = 5