From 0ce79c8c50613d3a01f2b8ac3ee0e56dc6c9c743 Mon Sep 17 00:00:00 2001 From: Prakhar Pratyush Date: Fri, 24 May 2024 17:38:41 +0530 Subject: [PATCH] message_edit: Update do_update_message codepath to send event on commit. Earlier, we were using 'send_event' in 'do_update_message' which can lead to a situation where we enqueue events but the transaction fails at a later stage. Events should not be sent until we know we're not rolling back. --- zerver/actions/message_edit.py | 8 +++++--- zerver/tests/test_message_edit.py | 8 ++++---- zerver/tests/test_message_edit_notifications.py | 3 ++- zerver/tests/test_message_move_topic.py | 2 +- zerver/tests/test_muted_users.py | 13 +++++++------ 5 files changed, 19 insertions(+), 15 deletions(-) diff --git a/zerver/actions/message_edit.py b/zerver/actions/message_edit.py index 21f832f990..cb53e51ab6 100644 --- a/zerver/actions/message_edit.py +++ b/zerver/actions/message_edit.py @@ -81,7 +81,7 @@ from zerver.models import ( ) from zerver.models.streams import get_stream_by_id_in_realm from zerver.models.users import get_system_bot -from zerver.tornado.django_api import send_event, send_event_on_commit +from zerver.tornado.django_api import send_event_on_commit def subscriber_info(user_id: int) -> Dict[str, Any]: @@ -730,7 +730,9 @@ def do_update_message( "stream_id": stream_being_edited.id, "topic": orig_topic_name, } - send_event(user_profile.realm, delete_event, [user.id for user in users_losing_access]) + send_event_on_commit( + user_profile.realm, delete_event, [user.id for user in users_losing_access] + ) # Reset the Attachment.is_*_public caches for all messages # moved to another stream with different access permissions. @@ -1003,7 +1005,7 @@ def do_update_message( visibility_policy=new_visibility_policy, ) - send_event(user_profile.realm, event, users_to_be_notified) + send_event_on_commit(user_profile.realm, event, users_to_be_notified) resolved_topic_message_id = None if topic_name is not None and content is None: diff --git a/zerver/tests/test_message_edit.py b/zerver/tests/test_message_edit.py index d10fc8020e..57b6eb04a8 100644 --- a/zerver/tests/test_message_edit.py +++ b/zerver/tests/test_message_edit.py @@ -1127,7 +1127,7 @@ class EditMessageTest(ZulipTestCase): do_edit_message_assert_success(id_, "G", "cordelia") do_edit_message_assert_success(id_, "H", "hamlet") - @mock.patch("zerver.actions.message_edit.send_event") + @mock.patch("zerver.actions.message_edit.send_event_on_commit") def test_topic_wildcard_mention_in_followed_topic( self, mock_send_event: mock.MagicMock ) -> None: @@ -1185,7 +1185,7 @@ class EditMessageTest(ZulipTestCase): called = True self.assertTrue(called) - @mock.patch("zerver.actions.message_edit.send_event") + @mock.patch("zerver.actions.message_edit.send_event_on_commit") def test_stream_wildcard_mention_in_followed_topic( self, mock_send_event: mock.MagicMock ) -> None: @@ -1243,7 +1243,7 @@ class EditMessageTest(ZulipTestCase): called = True self.assertTrue(called) - @mock.patch("zerver.actions.message_edit.send_event") + @mock.patch("zerver.actions.message_edit.send_event_on_commit") def test_topic_wildcard_mention(self, mock_send_event: mock.MagicMock) -> None: stream_name = "Macbeth" hamlet = self.example_user("hamlet") @@ -1349,7 +1349,7 @@ class EditMessageTest(ZulipTestCase): ) self.assert_json_success(result) - @mock.patch("zerver.actions.message_edit.send_event") + @mock.patch("zerver.actions.message_edit.send_event_on_commit") def test_stream_wildcard_mention(self, mock_send_event: mock.MagicMock) -> None: stream_name = "Macbeth" hamlet = self.example_user("hamlet") diff --git a/zerver/tests/test_message_edit_notifications.py b/zerver/tests/test_message_edit_notifications.py index 73026e0bab..1695ce32e5 100644 --- a/zerver/tests/test_message_edit_notifications.py +++ b/zerver/tests/test_message_edit_notifications.py @@ -100,7 +100,8 @@ class EditMessageSideEffectsTest(ZulipTestCase): ) with mock.patch("zerver.tornado.event_queue.maybe_enqueue_notifications") as m: - result = self.client_patch(url, request) + with self.captureOnCommitCallbacks(execute=True): + result = self.client_patch(url, request) cordelia = self.example_user("cordelia") cordelia_calls = [ diff --git a/zerver/tests/test_message_move_topic.py b/zerver/tests/test_message_move_topic.py index 76de752d3e..2a6b9ad965 100644 --- a/zerver/tests/test_message_move_topic.py +++ b/zerver/tests/test_message_move_topic.py @@ -111,7 +111,7 @@ class MessageMoveTopicTest(ZulipTestCase): ) self.assert_json_error(result, "Invalid character in topic, at position 8!") - @mock.patch("zerver.actions.message_edit.send_event") + @mock.patch("zerver.actions.message_edit.send_event_on_commit") def test_edit_topic_public_history_stream(self, mock_send_event: mock.MagicMock) -> None: stream_name = "Macbeth" hamlet = self.example_user("hamlet") diff --git a/zerver/tests/test_muted_users.py b/zerver/tests/test_muted_users.py index 42c3ac8b29..98d69d8bc8 100644 --- a/zerver/tests/test_muted_users.py +++ b/zerver/tests/test_muted_users.py @@ -333,12 +333,13 @@ class MutedUsersTests(ZulipTestCase): self.login("cordelia") with mock.patch("zerver.tornado.event_queue.maybe_enqueue_notifications") as m: - result = self.client_patch( - "/json/messages/" + str(message_id), - dict( - content="@**King Hamlet**", - ), - ) + with self.captureOnCommitCallbacks(execute=True): + result = self.client_patch( + "/json/messages/" + str(message_id), + dict( + content="@**King Hamlet**", + ), + ) self.assert_json_success(result) m.assert_called_once() # `maybe_enqueue_notifications` was called for Hamlet after message edit mentioned him.