Mirror
/
zulip
mirror of https://github.com/zulip/zulip.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

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 <mateusz.mandera@zulip.com>
This commit is contained in:
Gaurav Pandey 2024-06-04 22:43:37 +02:00 committed by Tim Abbott
parent 0232d97ae9
commit 1c5007461a
5 changed files with 204 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

67
zerver/actions/message_edit.py
View File

@ -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

141
zerver/tests/test_message_move_topic.py
View File

@ -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.",
)

5
zproject/default_settings.py
View File

@ -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

4
zproject/dev_settings.py
View File

@ -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

6
zproject/test_extra_settings.py
View File

@ -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