mirror of https://github.com/zulip/zulip.git
message_edit: Fix resolve topic notifications.
When 'resolve|unresolve' and 'move stream' actions occurs in
the same api call, 'This topic was marked as resolved|unresolved'
notification is not sent.
Both 'topic moved' and 'topic resolved' notification should be generated.
This commit updates the logic of when and where to send
'topic resolve|unresolve' notification. Unlike previous logic, notification
may be sent even in the case 'new_stream' is not None.
In general, 'topic resolved|unresolved' notification is sent to
'stream_being_edited'. In this particular case ('new_stream' is not None),
notification is sent to the 'new_stream' after check.
Test case is included.
Fixes: #22973
This commit is contained in:
Prakhar Pratyush
Tim Abbott
parent
62437e6930
commit
9997131df3
|
|
@ -809,16 +809,19 @@ 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
|
||||
and content is None
|
||||
and len(changed_messages) > 0
|
||||
):
|
||||
assert stream_being_edited is not None
|
||||
if topic_name is not None and content is None and len(changed_messages) > 0:
|
||||
# When stream is changed and topic is marked as resolved or unresolved
|
||||
# in the same API request, resolved or unresolved notification should
|
||||
# be sent to "new_stream".
|
||||
# In general, it is sent to "stream_being_edited".
|
||||
stream_to_send_resolve_topic_notification = stream_being_edited
|
||||
if new_stream is not None:
|
||||
stream_to_send_resolve_topic_notification = new_stream
|
||||
|
||||
assert stream_to_send_resolve_topic_notification is not None
|
||||
sent_resolve_topic_notification = maybe_send_resolve_topic_notifications(
|
||||
user_profile=user_profile,
|
||||
stream=stream_being_edited,
|
||||
stream=stream_to_send_resolve_topic_notification,
|
||||
old_topic=orig_topic_name,
|
||||
new_topic=topic_name,
|
||||
changed_messages=changed_messages,
|
||||
|
|
|
|||
|
|
@ -2887,6 +2887,65 @@ class EditMessageTest(EditMessageTestCase):
|
|||
f"This topic was moved here from #**public stream>{new_topic}** by @_**{user_profile.full_name}|{user_profile.id}**.",
|
||||
)
|
||||
|
||||
def test_notify_resolve_topic_and_move_stream(self) -> None:
|
||||
(
|
||||
user_profile,
|
||||
first_stream,
|
||||
second_stream,
|
||||
msg_id,
|
||||
msg_id_later,
|
||||
) = self.prepare_move_topics("iago", "first stream", "second stream", "test")
|
||||
|
||||
# 'prepare_move_topics' sends 3 messages in the first_stream
|
||||
messages = get_topic_messages(user_profile, first_stream, "test")
|
||||
self.assert_length(messages, 3)
|
||||
|
||||
# Test resolving a topic (test -> ✔ test) while changing stream (first_stream -> second_stream)
|
||||
new_topic = "✔ test"
|
||||
new_stream = second_stream
|
||||
result = self.client_patch(
|
||||
"/json/messages/" + str(msg_id),
|
||||
{
|
||||
"stream_id": new_stream.id,
|
||||
"topic": new_topic,
|
||||
"propagate_mode": "change_all",
|
||||
},
|
||||
)
|
||||
self.assert_json_success(result)
|
||||
messages = get_topic_messages(user_profile, new_stream, new_topic)
|
||||
self.assert_length(messages, 5)
|
||||
self.assertEqual(
|
||||
messages[3].content,
|
||||
f"@_**{user_profile.full_name}|{user_profile.id}** has marked this topic as resolved.",
|
||||
)
|
||||
self.assertEqual(
|
||||
messages[4].content,
|
||||
f"This topic was moved here from #**{first_stream.name}>test** by @_**{user_profile.full_name}|{user_profile.id}**.",
|
||||
)
|
||||
|
||||
# Test unresolving a topic (✔ test -> test) while changing stream (second_stream -> first_stream)
|
||||
new_topic = "test"
|
||||
new_stream = first_stream
|
||||
result = self.client_patch(
|
||||
"/json/messages/" + str(msg_id),
|
||||
{
|
||||
"stream_id": new_stream.id,
|
||||
"topic": new_topic,
|
||||
"propagate_mode": "change_all",
|
||||
},
|
||||
)
|
||||
self.assert_json_success(result)
|
||||
messages = get_topic_messages(user_profile, new_stream, new_topic)
|
||||
self.assert_length(messages, 7)
|
||||
self.assertEqual(
|
||||
messages[5].content,
|
||||
f"@_**{user_profile.full_name}|{user_profile.id}** has marked this topic as unresolved.",
|
||||
)
|
||||
self.assertEqual(
|
||||
messages[6].content,
|
||||
f"This topic was moved here from #**{second_stream.name}>✔ test** by @_**{user_profile.full_name}|{user_profile.id}**.",
|
||||
)
|
||||
|
||||
def parameterized_test_move_message_involving_private_stream(
|
||||
self,
|
||||
from_invite_only: bool,
|
||||
|
|
@ -3154,36 +3213,6 @@ class EditMessageTest(EditMessageTestCase):
|
|||
== 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:
|
||||
|
|
|
|||
Loading…
Reference in New Issue