From 71abbbdd7a03f6c7c2f6ab88f1e7b45ba53943a6 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Sat, 12 Nov 2022 17:49:11 -0500 Subject: [PATCH] message_edit: Handle truncated topic resolution. This solves the problem that resolving a topic with a long name (>60 characters) will cause the topic name to be truncated, and thus the edit message code path thinks that the topic is being moved in addition to being resolved. We store the pre-truncation topic and use it to check against the original topic when determining whether a topic is being moved while getting (un)resovled or not. Fixes #23482 Signed-off-by: Zixuan James Li --- zerver/actions/message_edit.py | 8 +++- zerver/tests/test_message_edit.py | 64 ++++++++++++++++++++++++++++++- 2 files changed, 68 insertions(+), 4 deletions(-) diff --git a/zerver/actions/message_edit.py b/zerver/actions/message_edit.py index f243b8fb14..03592da5ca 100644 --- a/zerver/actions/message_edit.py +++ b/zerver/actions/message_edit.py @@ -538,6 +538,10 @@ def do_update_message( user_id for user_id in new_stream_sub_ids if user_id not in old_stream_sub_ids ] + # We save the full topic name so that checks that require comparison + # between the original topic and the topic name passed into this function + # will not be affected by the potential truncation of topic_name below. + pre_truncation_topic_name = topic_name if topic_name is not None: topic_name = truncate_topic(topic_name) target_message.set_topic_name(topic_name) @@ -865,9 +869,9 @@ def do_update_message( new_stream is not None or not sent_resolve_topic_notification or ( - topic_name is not None + pre_truncation_topic_name is not None and orig_topic_name.lstrip(RESOLVED_TOPIC_PREFIX) - != topic_name.lstrip(RESOLVED_TOPIC_PREFIX) + != pre_truncation_topic_name.lstrip(RESOLVED_TOPIC_PREFIX) ) ): if moved_all_visible_messages: diff --git a/zerver/tests/test_message_edit.py b/zerver/tests/test_message_edit.py index 03bf6ffa7d..87621ce598 100644 --- a/zerver/tests/test_message_edit.py +++ b/zerver/tests/test_message_edit.py @@ -17,7 +17,7 @@ from zerver.actions.reactions import do_add_reaction from zerver.actions.realm_settings import do_change_realm_plan_type, do_set_realm_property from zerver.actions.streams import do_change_stream_post_policy, do_deactivate_stream from zerver.actions.users import do_change_user_role -from zerver.lib.message import MessageDict, has_message_access, messages_for_ids +from zerver.lib.message import MessageDict, has_message_access, messages_for_ids, truncate_topic from zerver.lib.test_classes import ZulipTestCase, get_topic_messages from zerver.lib.test_helpers import cache_tries_captured, queries_captured from zerver.lib.topic import RESOLVED_TOPIC_PREFIX, TOPIC_NAME @@ -28,7 +28,16 @@ from zerver.lib.user_topics import ( topic_is_muted, ) from zerver.lib.utils import assert_is_not_none -from zerver.models import Message, Realm, Stream, UserMessage, UserProfile, get_realm, get_stream +from zerver.models import ( + MAX_TOPIC_NAME_LENGTH, + Message, + Realm, + Stream, + UserMessage, + UserProfile, + get_realm, + get_stream, +) if TYPE_CHECKING: from django.test.client import _MonkeyPatchedWSGIResponse as TestHttpResponse @@ -2722,6 +2731,57 @@ class EditMessageTest(EditMessageTestCase): self.assert_length(messages, 1) self.assertEqual(messages[0].content, "First") + def test_notify_resolve_topic_long_name(self) -> None: + user_profile = self.example_user("hamlet") + self.login("hamlet") + stream = self.make_stream("public stream") + self.subscribe(user_profile, stream.name) + # Marking topics with a long name as resolved causes the new topic name to be truncated. + # We want to avoid having code paths believing that the topic is "moved" instead of + # "resolved" in this edge case. + topic_name = "a" * MAX_TOPIC_NAME_LENGTH + msg_id = self.send_stream_message( + user_profile, stream.name, topic_name=topic_name, content="First" + ) + + resolved_topic = RESOLVED_TOPIC_PREFIX + topic_name + result = self.client_patch( + "/json/messages/" + str(msg_id), + { + "topic": resolved_topic, + "propagate_mode": "change_all", + }, + ) + self.assert_json_success(result) + + new_topic_name = truncate_topic(resolved_topic) + messages = get_topic_messages(user_profile, stream, new_topic_name) + self.assert_length(messages, 2) + self.assertEqual(messages[0].content, "First") + self.assertEqual( + messages[1].content, + f"@_**{user_profile.full_name}|{user_profile.id}** has marked this topic as resolved.", + ) + + # Note that we are removing the prefix from the already truncated topic, + # so unresolved_topic_name will not be the same as the original topic_name + unresolved_topic_name = new_topic_name.replace(RESOLVED_TOPIC_PREFIX, "") + result = self.client_patch( + "/json/messages/" + str(msg_id), + { + "topic": unresolved_topic_name, + "propagate_mode": "change_all", + }, + ) + self.assert_json_success(result) + + messages = get_topic_messages(user_profile, stream, unresolved_topic_name) + self.assert_length(messages, 3) + self.assertEqual( + messages[2].content, + f"@_**{user_profile.full_name}|{user_profile.id}** has marked this topic as unresolved.", + ) + def test_notify_resolve_and_move_topic(self) -> None: user_profile = self.example_user("hamlet") self.login("hamlet")