From 4bb381fc80cbed33ba34fae0e2839bda215b1a60 Mon Sep 17 00:00:00 2001 From: Julia Bichler Date: Sun, 17 Jul 2022 13:34:04 +0200 Subject: [PATCH] message_edit: Support sending notifications with topic changes. Previously we did not send notification for topic-only edits. Now, we add backend support for sending notification to topic-only edits as well. We would add support for this in webapp in further commits since message edit UI will be updated as well. We just make sure that no notifications are sent when editing topic using pencil icon in message header. We also change the API default for moving a topic to only notify the new location, not the old one; this matches the current defaults in the web UI. Includes many tests. We also update the puppeteer tests to test only content edit as we are going to change the UI to not allow topic editing from message edit UI. Also fixing the existing tests to pass while doing topic edits is somewhat complex as notification message is also sent to new topic by default. Fixes #21712. Co-authored-by: Aman Agrawal Co-authored-by: Tim Abbott --- frontend_tests/puppeteer_tests/edit.ts | 6 +- static/js/message_edit.js | 2 + templates/zerver/api/changelog.md | 7 + version.py | 2 +- zerver/actions/message_edit.py | 30 ++- zerver/openapi/zulip.yaml | 19 +- zerver/tests/test_message_edit.py | 357 +++++++++++++++++++++++-- zerver/views/message_edit.py | 2 +- 8 files changed, 394 insertions(+), 31 deletions(-) diff --git a/frontend_tests/puppeteer_tests/edit.ts b/frontend_tests/puppeteer_tests/edit.ts index c069d156d6..947b20e4f1 100644 --- a/frontend_tests/puppeteer_tests/edit.ts +++ b/frontend_tests/puppeteer_tests/edit.ts @@ -37,9 +37,9 @@ async function test_stream_message_edit(page: Page): Promise { content: "test editing", }); - await edit_stream_message(page, "edited", "test edited"); + await edit_stream_message(page, "edits", "test edited"); - await common.check_messages_sent(page, "zhome", [["Verona > edited", ["test edited"]]]); + await common.check_messages_sent(page, "zhome", [["Verona > edits", ["test edited"]]]); } async function test_edit_message_with_slash_me(page: Page): Promise { @@ -61,7 +61,7 @@ async function test_edit_message_with_slash_me(page: Page): Promise { )} and normalize-space()="Desdemona"]`, ); - await edit_stream_message(page, "edited", "/me test edited a message with me"); + await edit_stream_message(page, "edits", "/me test edited a message with me"); await page.waitForSelector( `xpath/${last_message_xpath}//*[${common.has_class_x( diff --git a/static/js/message_edit.js b/static/js/message_edit.js index 6d60c8258c..7e66b15be7 100644 --- a/static/js/message_edit.js +++ b/static/js/message_edit.js @@ -844,6 +844,8 @@ export function save_inline_topic_edit($row) { message_id: message.id, topic: new_topic, propagate_mode: "change_later", + send_notification_to_old_thread: false, + send_notification_to_new_thread: false, }; channel.patch({ diff --git a/templates/zerver/api/changelog.md b/templates/zerver/api/changelog.md index 2e0e1100b7..775ec96fa3 100644 --- a/templates/zerver/api/changelog.md +++ b/templates/zerver/api/changelog.md @@ -20,6 +20,13 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 6.0 +**Feature level 152** + +* [`PATCH /messages/{message_id}`](/api/update-message): The + `send_notification_to_old_thread` and + `send_notification_to_new_thread` parameters are now respected when + moving a topic within a stream. + **Feature level 151** * [`POST /register`](/api/register-queue), [`GET /events`](/api/get-events), diff --git a/version.py b/version.py index a19b887247..e8b2e6036a 100644 --- a/version.py +++ b/version.py @@ -33,7 +33,7 @@ DESKTOP_WARNING_VERSION = "5.4.3" # Changes should be accompanied by documentation explaining what the # new level means in templates/zerver/api/changelog.md, as well as # "**Changes**" entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 151 +API_FEATURE_LEVEL = 152 # Bump the minor PROVISION_VERSION to indicate that folks should provision # only when going from an old version of the code to a newer version. Bump diff --git a/zerver/actions/message_edit.py b/zerver/actions/message_edit.py index f09bae74d7..0499a2f157 100644 --- a/zerver/actions/message_edit.py +++ b/zerver/actions/message_edit.py @@ -816,6 +816,7 @@ def do_update_message( send_event(user_profile.realm, event, users_to_be_notified) + sent_resolve_topic_notification = False if ( topic_name is not None and new_stream is None @@ -823,7 +824,7 @@ def do_update_message( and len(changed_messages) > 0 ): assert stream_being_edited is not None - maybe_send_resolve_topic_notifications( + sent_resolve_topic_notification = maybe_send_resolve_topic_notifications( user_profile=user_profile, stream=stream_being_edited, old_topic=orig_topic_name, @@ -831,7 +832,11 @@ def do_update_message( changed_messages=changed_messages, ) - if len(changed_messages) > 0 and new_stream is not None and stream_being_edited is not None: + if ( + len(changed_messages) > 0 + and (new_stream is not None or topic_name is not None) + and stream_being_edited is not None + ): # Notify users that the topic was moved. changed_messages_count = len(changed_messages) @@ -850,8 +855,25 @@ def do_update_message( "{changed_messages_count} messages were moved from this topic to {new_location} by {user}." ) + # 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: + if send_notification_to_new_thread and ( + new_stream is not None + or not sent_resolve_topic_notification + or ( + topic_name is not None + and orig_topic_name.lstrip(RESOLVED_TOPIC_PREFIX) + != topic_name.lstrip(RESOLVED_TOPIC_PREFIX) + ) + ): if moved_all_visible_messages: new_thread_notification_string = gettext_lazy( "This topic was moved here from {old_location} by {user}." @@ -870,7 +892,7 @@ def do_update_message( stream_being_edited, orig_topic_name, old_thread_notification_string, - new_stream, + new_stream if new_stream is not None else stream_being_edited, topic_name, new_thread_notification_string, changed_messages_count, diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 139ea55558..76c017b3aa 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -6259,21 +6259,30 @@ paths: - name: send_notification_to_old_thread in: query description: | - Whether to send an automated message to the old thread to + Whether to send an automated message to the old topic to notify users where the messages were moved to. - **Changes**: New in Zulip 3.0 (feature level 9). + **Changes**: Before Zulip 6.0 (feature level 152), this parameter + had a default of `true` and was ignored unless the stream was changed. + + New in Zulip 3.0 (feature level 9). schema: type: boolean - default: true + default: false example: true - name: send_notification_to_new_thread in: query description: | - Whether to send an automated message to the new thread to + Whether to send an automated message to the new topic to notify users where the messages came from. - **Changes**: New in Zulip 3.0 (feature level 9). + If the move is just [resolving/unresolving a topic](/help/resolve-a-topic), + this parameter will not trigger an additional notification. + + **Changes**: Before Zulip 6.0 (feature level 152), this parameter + was ignored unless the stream was changed. + + New in Zulip 3.0 (feature level 9). schema: type: boolean default: true diff --git a/zerver/tests/test_message_edit.py b/zerver/tests/test_message_edit.py index a4b41a6485..cb4bd15936 100644 --- a/zerver/tests/test_message_edit.py +++ b/zerver/tests/test_message_edit.py @@ -1305,9 +1305,9 @@ class EditMessageTest(EditMessageTestCase): send_notification_to_new_thread=False, content=None, ) - # This code path adds 9 (1 + 4/user with muted topics) to + # This code path adds 9 (1 + 4/user with muted topics) + 1 to # the number of database queries for moving a topic. - self.assert_length(queries, 18) + self.assert_length(queries, 19) for muting_user in get_users_muting_topic(stream.id, change_all_topic_name): for user in users_to_be_notified: @@ -1755,6 +1755,7 @@ class EditMessageTest(EditMessageTestCase): { "stream_id": new_stream.id, "propagate_mode": "change_all", + "send_notification_to_old_thread": "true", }, HTTP_ACCEPT_LANGUAGE="de", ) @@ -1903,6 +1904,7 @@ class EditMessageTest(EditMessageTestCase): { "stream_id": new_stream.id, "propagate_mode": "change_later", + "send_notification_to_old_thread": "true", }, ) self.assert_json_success(result) @@ -1933,6 +1935,7 @@ class EditMessageTest(EditMessageTestCase): { "stream_id": new_stream.id, "propagate_mode": "change_later", + "send_notification_to_old_thread": "true", }, ) self.assert_json_success(result) @@ -1962,6 +1965,7 @@ class EditMessageTest(EditMessageTestCase): { "stream_id": new_stream.id, "propagate_mode": "change_one", + "send_notification_to_old_thread": "true", }, ) self.assert_json_success(result) @@ -1992,6 +1996,7 @@ class EditMessageTest(EditMessageTestCase): { "stream_id": new_stream.id, "propagate_mode": "change_all", + "send_notification_to_old_thread": "true", }, ) self.assert_json_success(result) @@ -2036,7 +2041,7 @@ class EditMessageTest(EditMessageTestCase): else: self.assert_json_success(result) messages = get_topic_messages(user_profile, old_stream, "test") - self.assert_length(messages, 1) + self.assert_length(messages, 0) messages = get_topic_messages(user_profile, new_stream, "test") self.assert_length(messages, 4) @@ -2125,7 +2130,7 @@ class EditMessageTest(EditMessageTestCase): else: self.assert_json_success(result) messages = get_topic_messages(user_profile, old_stream, "test") - self.assert_length(messages, 1) + self.assert_length(messages, 0) messages = get_topic_messages(user_profile, new_stream, "test") self.assert_length(messages, 4) @@ -2222,7 +2227,7 @@ class EditMessageTest(EditMessageTestCase): ) self.assert_json_success(result) messages = get_topic_messages(user_profile, old_stream, "test") - self.assert_length(messages, 1) + self.assert_length(messages, 0) messages = get_topic_messages(user_profile, new_stream, "test") self.assert_length(messages, 4) @@ -2235,8 +2240,9 @@ class EditMessageTest(EditMessageTestCase): result = self.client_patch( f"/json/messages/{msg_id}", { - "stream_id": new_stream.id, "propagate_mode": "change_all", + "send_notification_to_old_thread": "true", + "stream_id": new_stream.id, "topic": "new topic", }, ) @@ -2448,6 +2454,290 @@ class EditMessageTest(EditMessageTestCase): messages = get_topic_messages(user_profile, new_stream, "test") self.assert_length(messages, 3) + def test_notify_new_topic(self) -> None: + user_profile = self.example_user("iago") + self.login("iago") + stream = self.make_stream("public stream") + self.subscribe(user_profile, stream.name) + msg_id = self.send_stream_message( + user_profile, stream.name, topic_name="test", content="First" + ) + self.send_stream_message(user_profile, stream.name, topic_name="test", content="Second") + self.send_stream_message(user_profile, stream.name, topic_name="test", content="third") + + result = self.client_patch( + "/json/messages/" + str(msg_id), + { + "message_id": msg_id, + "topic": "edited", + "propagate_mode": "change_all", + "send_notification_to_old_thread": "false", + "send_notification_to_new_thread": "true", + }, + ) + + self.assert_json_success(result) + + messages = get_topic_messages(user_profile, stream, "test") + self.assert_length(messages, 0) + + messages = get_topic_messages(user_profile, stream, "edited") + self.assert_length(messages, 4) + self.assertEqual( + messages[3].content, + f"This topic was moved here from #**public stream>test** by @_**Iago|{user_profile.id}**.", + ) + + def test_notify_old_topic(self) -> None: + user_profile = self.example_user("iago") + self.login("iago") + stream = self.make_stream("public stream") + self.subscribe(user_profile, stream.name) + msg_id = self.send_stream_message( + user_profile, stream.name, topic_name="test", content="First" + ) + self.send_stream_message(user_profile, stream.name, topic_name="test", content="Second") + self.send_stream_message(user_profile, stream.name, topic_name="test", content="third") + + result = self.client_patch( + "/json/messages/" + str(msg_id), + { + "message_id": msg_id, + "topic": "edited", + "propagate_mode": "change_all", + "send_notification_to_old_thread": "true", + "send_notification_to_new_thread": "false", + }, + ) + + self.assert_json_success(result) + + messages = get_topic_messages(user_profile, stream, "test") + self.assert_length(messages, 1) + self.assertEqual( + messages[0].content, + f"This topic was moved to #**public stream>edited** by @_**Iago|{user_profile.id}**.", + ) + + messages = get_topic_messages(user_profile, stream, "edited") + self.assert_length(messages, 3) + + def test_notify_both_topics(self) -> None: + user_profile = self.example_user("iago") + self.login("iago") + stream = self.make_stream("public stream") + self.subscribe(user_profile, stream.name) + msg_id = self.send_stream_message( + user_profile, stream.name, topic_name="test", content="First" + ) + self.send_stream_message(user_profile, stream.name, topic_name="test", content="Second") + self.send_stream_message(user_profile, stream.name, topic_name="test", content="third") + + result = self.client_patch( + "/json/messages/" + str(msg_id), + { + "message_id": msg_id, + "topic": "edited", + "propagate_mode": "change_all", + "send_notification_to_old_thread": "true", + "send_notification_to_new_thread": "true", + }, + ) + + self.assert_json_success(result) + + messages = get_topic_messages(user_profile, stream, "test") + self.assert_length(messages, 1) + self.assertEqual( + messages[0].content, + f"This topic was moved to #**public stream>edited** by @_**Iago|{user_profile.id}**.", + ) + + messages = get_topic_messages(user_profile, stream, "edited") + self.assert_length(messages, 4) + self.assertEqual( + messages[3].content, + f"This topic was moved here from #**public stream>test** by @_**Iago|{user_profile.id}**.", + ) + + def test_notify_no_topic(self) -> None: + user_profile = self.example_user("iago") + self.login("iago") + stream = self.make_stream("public stream") + self.subscribe(user_profile, stream.name) + msg_id = self.send_stream_message( + user_profile, stream.name, topic_name="test", content="First" + ) + self.send_stream_message(user_profile, stream.name, topic_name="test", content="Second") + self.send_stream_message(user_profile, stream.name, topic_name="test", content="third") + + result = self.client_patch( + "/json/messages/" + str(msg_id), + { + "message_id": msg_id, + "topic": "edited", + "propagate_mode": "change_all", + "send_notification_to_old_thread": "false", + "send_notification_to_new_thread": "false", + }, + ) + + self.assert_json_success(result) + + messages = get_topic_messages(user_profile, stream, "test") + self.assert_length(messages, 0) + + messages = get_topic_messages(user_profile, stream, "edited") + self.assert_length(messages, 3) + + def test_notify_new_topics_after_message_move(self) -> None: + user_profile = self.example_user("iago") + self.login("iago") + stream = self.make_stream("public stream") + self.subscribe(user_profile, stream.name) + msg_id = self.send_stream_message( + user_profile, stream.name, topic_name="test", content="First" + ) + self.send_stream_message(user_profile, stream.name, topic_name="test", content="Second") + self.send_stream_message(user_profile, stream.name, topic_name="test", content="Third") + + result = self.client_patch( + "/json/messages/" + str(msg_id), + { + "message_id": msg_id, + "topic": "edited", + "propagate_mode": "change_one", + "send_notification_to_old_thread": "false", + "send_notification_to_new_thread": "true", + }, + ) + + self.assert_json_success(result) + + messages = get_topic_messages(user_profile, stream, "test") + self.assert_length(messages, 2) + self.assertEqual(messages[0].content, "Second") + self.assertEqual(messages[1].content, "Third") + + messages = get_topic_messages(user_profile, stream, "edited") + self.assert_length(messages, 2) + self.assertEqual(messages[0].content, "First") + self.assertEqual( + messages[1].content, + f"A message was moved here from #**public stream>test** by @_**Iago|{user_profile.id}**.", + ) + + def test_notify_old_topics_after_message_move(self) -> None: + user_profile = self.example_user("iago") + self.login("iago") + stream = self.make_stream("public stream") + self.subscribe(user_profile, stream.name) + msg_id = self.send_stream_message( + user_profile, stream.name, topic_name="test", content="First" + ) + self.send_stream_message(user_profile, stream.name, topic_name="test", content="Second") + self.send_stream_message(user_profile, stream.name, topic_name="test", content="Third") + + result = self.client_patch( + "/json/messages/" + str(msg_id), + { + "message_id": msg_id, + "topic": "edited", + "propagate_mode": "change_one", + "send_notification_to_old_thread": "true", + "send_notification_to_new_thread": "false", + }, + ) + + self.assert_json_success(result) + + messages = get_topic_messages(user_profile, stream, "test") + self.assert_length(messages, 3) + self.assertEqual(messages[0].content, "Second") + self.assertEqual(messages[1].content, "Third") + self.assertEqual( + messages[2].content, + f"A message was moved from this topic to #**public stream>edited** by @_**Iago|{user_profile.id}**.", + ) + + messages = get_topic_messages(user_profile, stream, "edited") + self.assert_length(messages, 1) + self.assertEqual(messages[0].content, "First") + + def test_notify_both_topics_after_message_move(self) -> None: + user_profile = self.example_user("iago") + self.login("iago") + stream = self.make_stream("public stream") + self.subscribe(user_profile, stream.name) + msg_id = self.send_stream_message( + user_profile, stream.name, topic_name="test", content="First" + ) + self.send_stream_message(user_profile, stream.name, topic_name="test", content="Second") + self.send_stream_message(user_profile, stream.name, topic_name="test", content="Third") + + result = self.client_patch( + "/json/messages/" + str(msg_id), + { + "message_id": msg_id, + "topic": "edited", + "propagate_mode": "change_one", + "send_notification_to_old_thread": "true", + "send_notification_to_new_thread": "true", + }, + ) + + self.assert_json_success(result) + + messages = get_topic_messages(user_profile, stream, "test") + self.assert_length(messages, 3) + self.assertEqual(messages[0].content, "Second") + self.assertEqual(messages[1].content, "Third") + self.assertEqual( + messages[2].content, + f"A message was moved from this topic to #**public stream>edited** by @_**Iago|{user_profile.id}**.", + ) + + messages = get_topic_messages(user_profile, stream, "edited") + self.assert_length(messages, 2) + self.assertEqual(messages[0].content, "First") + self.assertEqual( + messages[1].content, + f"A message was moved here from #**public stream>test** by @_**Iago|{user_profile.id}**.", + ) + + def test_notify_no_topic_after_message_move(self) -> None: + user_profile = self.example_user("iago") + self.login("iago") + stream = self.make_stream("public stream") + self.subscribe(user_profile, stream.name) + msg_id = self.send_stream_message( + user_profile, stream.name, topic_name="test", content="First" + ) + self.send_stream_message(user_profile, stream.name, topic_name="test", content="Second") + self.send_stream_message(user_profile, stream.name, topic_name="test", content="Third") + + result = self.client_patch( + "/json/messages/" + str(msg_id), + { + "message_id": msg_id, + "topic": "edited", + "propagate_mode": "change_one", + "send_notification_to_old_thread": "false", + "send_notification_to_new_thread": "false", + }, + ) + + self.assert_json_success(result) + + messages = get_topic_messages(user_profile, stream, "test") + self.assert_length(messages, 2) + self.assertEqual(messages[0].content, "Second") + self.assertEqual(messages[1].content, "Third") + + messages = get_topic_messages(user_profile, stream, "edited") + self.assert_length(messages, 1) + self.assertEqual(messages[0].content, "First") + def parameterized_test_move_message_involving_private_stream( self, from_invite_only: bool, @@ -2503,11 +2793,7 @@ class EditMessageTest(EditMessageTestCase): self.assert_json_success(result) messages = get_topic_messages(admin_user, old_stream, "test") - self.assert_length(messages, 1) - self.assertEqual( - messages[0].content, - f"This topic was moved to #**new stream>test** by @_**Iago|{admin_user.id}**.", - ) + self.assert_length(messages, 0) messages = get_topic_messages(admin_user, new_stream, "test") self.assert_length(messages, 3) @@ -2649,7 +2935,7 @@ class EditMessageTest(EditMessageTestCase): == 0 ) - # Now move to a weird state and confirm no new messages + # Now move to a weird state and confirm we get the normal topic moved message. weird_topic = "✔ ✔✔" + original_topic result = self.client_patch( "/json/messages/" + str(id1), @@ -2668,11 +2954,15 @@ class EditMessageTest(EditMessageTestCase): ) messages = get_topic_messages(admin_user, stream, weird_topic) - self.assert_length(messages, 3) + self.assert_length(messages, 4) self.assertEqual( messages[2].content, f"@_**Iago|{admin_user.id}** has marked this topic as resolved.", ) + self.assertEqual( + messages[3].content, + f"This topic was moved here from #**new>✔ topic 1** by @_**Iago|{admin_user.id}**.", + ) unresolved_topic = original_topic result = self.client_patch( @@ -2692,16 +2982,19 @@ class EditMessageTest(EditMessageTestCase): ) messages = get_topic_messages(admin_user, stream, unresolved_topic) - self.assert_length(messages, 4) + self.assert_length(messages, 5) self.assertEqual( - messages[3].content, + messages[2].content, f"@_**Iago|{admin_user.id}** has marked this topic as resolved." + ) + self.assertEqual( + messages[4].content, f"@_**Iago|{admin_user.id}** has marked this topic as unresolved.", ) # Check topic unresolved notification message is only unread for participants. assert ( UserMessage.objects.filter( - user_profile__in=[admin_user, hamlet, aaron], message__id=messages[3].id + user_profile__in=[admin_user, hamlet, aaron], message__id=messages[4].id ) .extra(where=[UserMessage.where_unread()]) .count() @@ -2709,12 +3002,42 @@ class EditMessageTest(EditMessageTestCase): ) assert ( - UserMessage.objects.filter(user_profile=cordelia, message__id=messages[3].id) + UserMessage.objects.filter(user_profile=cordelia, message__id=messages[4].id) .extra(where=[UserMessage.where_unread()]) .count() == 0 ) + # Now move to another stream while resolving the topic and + # check the notifications. + final_stream = self.make_stream("final") + self.subscribe(admin_user, final_stream.name) + result = self.client_patch( + "/json/messages/" + str(id1), + { + "topic": resolved_topic, + "stream_id": final_stream.id, + "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, final_stream, resolved_topic) + # TODO: This should be 7 -- but currently we never trigger + # resolve-topic notifications when moving the stream, even if + # the resolve-topic state is changed at that time. + self.assert_length(messages, 6) + self.assertEqual( + messages[5].content, + f"This topic was moved here from #**new>topic 1** by @_**Iago|{admin_user.id}**.", + ) + class DeleteMessageTest(ZulipTestCase): def test_delete_message_invalid_request_format(self) -> None: diff --git a/zerver/views/message_edit.py b/zerver/views/message_edit.py index 0bf6d2d857..91901dd02e 100644 --- a/zerver/views/message_edit.py +++ b/zerver/views/message_edit.py @@ -122,7 +122,7 @@ def update_message_backend( propagate_mode: str = REQ( default="change_one", str_validator=check_string_in(PROPAGATE_MODE_VALUES) ), - send_notification_to_old_thread: bool = REQ(default=True, json_validator=check_bool), + send_notification_to_old_thread: bool = REQ(default=False, json_validator=check_bool), send_notification_to_new_thread: bool = REQ(default=True, json_validator=check_bool), content: Optional[str] = REQ(default=None), ) -> HttpResponse: