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: