From 1c5007461a8be6cb650f822f77e83fa00a83b61f Mon Sep 17 00:00:00 2001 From: Gaurav Pandey Date: Tue, 4 Jun 2024 22:43:37 +0200 Subject: [PATCH] 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 #19181. Co-authored-by: Mateusz Mandera --- zerver/actions/message_edit.py | 67 ++++++++--- zerver/tests/test_message_move_topic.py | 141 +++++++++++++++++++++++- zproject/default_settings.py | 5 + zproject/dev_settings.py | 4 + zproject/test_extra_settings.py | 6 + 5 files changed, 204 insertions(+), 19 deletions(-) diff --git a/zerver/actions/message_edit.py b/zerver/actions/message_edit.py index 528f8a2522..77f82c2849 100644 --- a/zerver/actions/message_edit.py +++ b/zerver/actions/message_edit.py @@ -12,7 +12,7 @@ from django.utils.translation import gettext_lazy from django.utils.translation import override as override_language from django_stubs_ext import StrPromise -from zerver.actions.message_delete import DeleteMessagesEvent +from zerver.actions.message_delete import DeleteMessagesEvent, do_delete_messages from zerver.actions.message_flags import do_update_mobile_push_notification from zerver.actions.message_send import ( filter_presence_idle_user_ids, @@ -147,7 +147,7 @@ def maybe_send_resolve_topic_notifications( old_topic_name: str, new_topic_name: str, changed_messages: QuerySet[Message], -) -> Optional[int]: +) -> Tuple[Optional[int], bool]: """Returns resolved_topic_message_id if resolve topic notifications were in fact sent.""" # Note that topics will have already been stripped in check_update_message. # @@ -174,7 +174,16 @@ def maybe_send_resolve_topic_notifications( # 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 None + return None, False + + # Sometimes a user might accidentally resolve a topic, and then + # have to undo the action. We don't want to spam "resolved", + # "unresolved" messages one after another in such a situation. + # For that reason, we apply a short grace period during which + # such an undo action will just delete the previous notification + # message instead. + if maybe_delete_previous_resolve_topic_notification(stream, new_topic_name): + return None, True # Compute the users who either sent or reacted to messages that # were moved via the "resolve topic' action. Only those users @@ -205,7 +214,27 @@ def maybe_send_resolve_topic_notifications( limit_unread_user_ids=affected_participant_ids, ) - return resolved_topic_message_id + return resolved_topic_message_id, False + + +def maybe_delete_previous_resolve_topic_notification(stream: Stream, topic: str) -> bool: + assert stream.recipient_id is not None + last_message = messages_for_topic(stream.realm_id, stream.recipient_id, topic).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 send_message_moved_breadcrumbs( @@ -1009,6 +1038,7 @@ def do_update_message( send_event_on_commit(user_profile.realm, event, users_to_be_notified) resolved_topic_message_id = None + resolved_topic_message_deleted = False if topic_name is not None and content is None: # When stream is changed and topic is marked as resolved or unresolved # in the same API request, resolved or unresolved notification should @@ -1019,12 +1049,14 @@ def do_update_message( stream_to_send_resolve_topic_notification = new_stream assert stream_to_send_resolve_topic_notification is not None - resolved_topic_message_id = maybe_send_resolve_topic_notifications( - user_profile=user_profile, - stream=stream_to_send_resolve_topic_notification, - old_topic_name=orig_topic_name, - new_topic_name=topic_name, - changed_messages=changed_messages, + resolved_topic_message_id, resolved_topic_message_deleted = ( + maybe_send_resolve_topic_notifications( + user_profile=user_profile, + stream=stream_to_send_resolve_topic_notification, + old_topic_name=orig_topic_name, + new_topic_name=topic_name, + changed_messages=changed_messages, + ) ) if (new_stream is not None or topic_name is not None) and stream_being_edited is not None: @@ -1047,21 +1079,22 @@ def do_update_message( # The new thread notification code path is a bit subtle. We # don't want every resolve-topic action to also annoyingly # send an extra notification that the topic was moved! - # - # Since one can resolve/unresolve a topic at the same time - # you're moving it, we need to carefully treat the resolve - # topic notification as satisfying our obligation to send a - # notification to the new topic only if the only thing this - # request did is mark the topic as resolved. new_thread_notification_string = None if send_notification_to_new_thread and ( + # The stream changed -> eligible to notify. new_stream is not None - or not resolved_topic_message_id + # The topic changed -> eligible to notify. or ( pre_truncation_topic_name is not None and orig_topic_name.lstrip(RESOLVED_TOPIC_PREFIX) != pre_truncation_topic_name.lstrip(RESOLVED_TOPIC_PREFIX) ) + or not ( + # We have not completed our obligation to notify about a + # resolve topic, which happens if either we sent a notification or + # deleted a very recent previous notification. + resolved_topic_message_id or resolved_topic_message_deleted + ) ): 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 diff --git a/zerver/tests/test_message_move_topic.py b/zerver/tests/test_message_move_topic.py index 2a6b9ad965..f49575e79d 100644 --- a/zerver/tests/test_message_move_topic.py +++ b/zerver/tests/test_message_move_topic.py @@ -3,14 +3,22 @@ from typing import Any, Dict, List from unittest import mock import orjson +import time_machine +from django.test import override_settings +from django.utils.timezone import now as timezone_now -from zerver.actions.message_edit import check_update_message, do_update_message +from zerver.actions.message_delete import do_delete_messages +from zerver.actions.message_edit import ( + check_update_message, + do_update_message, + maybe_send_resolve_topic_notifications, +) from zerver.actions.reactions import do_add_reaction from zerver.actions.realm_settings import do_set_realm_property from zerver.actions.user_topics import do_set_user_topic_visibility_policy from zerver.lib.message import truncate_topic from zerver.lib.test_classes import ZulipTestCase, get_topic_messages -from zerver.lib.topic import RESOLVED_TOPIC_PREFIX +from zerver.lib.topic import RESOLVED_TOPIC_PREFIX, messages_for_topic from zerver.lib.user_topics import ( get_users_with_user_topic_visibility_policy, set_topic_visibility_policy, @@ -1623,3 +1631,132 @@ class MessageMoveTopicTest(ZulipTestCase): .extra(where=[UserMessage.where_unread()]) .exists() ) + + @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") + hamlet = self.example_user("hamlet") + stream = self.make_stream("new") + self.subscribe(admin_user, stream.name) + self.subscribe(hamlet, stream.name) + original_topic = "topic 1" + id1 = self.send_stream_message( + 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 + start_time = timezone_now() + with time_machine.travel(start_time, tick=False): + result = self.client_patch( + "/json/messages/" + str(id1), + { + "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 time_machine.travel(start_time + timedelta(seconds=30), tick=False): + result = self.client_patch( + "/json/messages/" + str(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) + # The message about the topic having been resolved is gone. + self.assert_length(messages, 2) + self.assertEqual( + messages[1].content, + "message 2", + ) + self.assertEqual(messages[0].content, "message 1") + + # Now resolve the topic again after the grace period + with time_machine.travel(start_time + timedelta(seconds=61), tick=False): + result = self.client_patch( + "/json/messages/" + str(id1), + { + "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.", + ) + + def test_send_resolve_topic_notification_with_no_topic_messages(self) -> None: + self.login("iago") + admin_user = self.example_user("iago") + hamlet = self.example_user("hamlet") + stream = self.make_stream("new") + self.subscribe(admin_user, stream.name) + self.subscribe(hamlet, stream.name) + original_topic = "topic 1" + message_id = self.send_stream_message( + hamlet, "new", content="message 1", topic_name=original_topic + ) + + message = Message.objects.get(id=message_id) + do_delete_messages(admin_user.realm, [message]) + + assert stream.recipient_id is not None + changed_messages = messages_for_topic(stream.realm_id, stream.recipient_id, original_topic) + resolve_topic = RESOLVED_TOPIC_PREFIX + original_topic + maybe_send_resolve_topic_notifications( + user_profile=admin_user, + stream=stream, + old_topic_name=original_topic, + new_topic_name=resolve_topic, + changed_messages=changed_messages, + ) + + 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.", + ) diff --git a/zproject/default_settings.py b/zproject/default_settings.py index acadf16a5b..c0109c7014 100644 --- a/zproject/default_settings.py +++ b/zproject/default_settings.py @@ -641,3 +641,8 @@ CUSTOM_AUTHENTICATION_WRAPPER_FUNCTION: Optional[Callable[..., Any]] = None # False in production, as we can only handle named user groups in the # web app settings UI. ALLOW_ANONYMOUS_GROUP_VALUED_SETTINGS = False + +# Grace period during which we don't send a resolve/unresolve +# notification to a stream and also delete the previous counter +# notification. +RESOLVE_TOPIC_UNDO_GRACE_PERIOD_SECONDS = 60 diff --git a/zproject/dev_settings.py b/zproject/dev_settings.py index c35015b60b..9ae2b0434e 100644 --- a/zproject/dev_settings.py +++ b/zproject/dev_settings.py @@ -210,3 +210,7 @@ PUSH_NOTIFICATION_BOUNCER_URL = f"http://push.{EXTERNAL_HOST}" # Breaks the UI if used, but enabled for development environment testing. ALLOW_ANONYMOUS_GROUP_VALUED_SETTINGS = True + +# This value needs to be lower in development than usual to allow +# for quicker testing of the feature. +RESOLVE_TOPIC_UNDO_GRACE_PERIOD_SECONDS = 5 diff --git a/zproject/test_extra_settings.py b/zproject/test_extra_settings.py index 81936e13bd..be79da7f1d 100644 --- a/zproject/test_extra_settings.py +++ b/zproject/test_extra_settings.py @@ -270,3 +270,9 @@ SCIM_CONFIG: Dict[str, SCIMConfigDict] = { } ALLOW_ANONYMOUS_GROUP_VALUED_SETTINGS = True + +# This override disables the grace period for undoing resolving/unresolving +# a topic in tests. +# This allows tests to not worry about the special behavior during the grace period. +# Otherwise they would have to do lots of mocking of the timer to work around this. +RESOLVE_TOPIC_UNDO_GRACE_PERIOD_SECONDS = 0