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 <amanagr@zulip.com>
Co-authored-by: Tim Abbott <tabbott@zulip.com>
This commit is contained in:
Julia Bichler 2022-07-17 13:34:04 +02:00 committed by Tim Abbott
parent cebc63bf82
commit 4bb381fc80
8 changed files with 394 additions and 31 deletions

View File

@ -37,9 +37,9 @@ async function test_stream_message_edit(page: Page): Promise<void> {
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<void> {
@ -61,7 +61,7 @@ async function test_edit_message_with_slash_me(page: Page): Promise<void> {
)} 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(

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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