From 68e93d24354ec4fca32a3d1b2bfbcf2f8ce5fdb9 Mon Sep 17 00:00:00 2001 From: Rohitt Vashishtha Date: Thu, 28 Nov 2019 10:26:57 +0000 Subject: [PATCH] update-message: Use MentionData in the update_message_backend code. This is a performance optimization, since we can avoid doing work related to wildcard mentions in the common case that the message can't have any. We also add a unit test for adding wildcard mentions in a message edit. --- zerver/lib/actions.py | 7 +++++-- zerver/tests/test_events.py | 7 ++++++- zerver/tests/test_messages.py | 39 ++++++++++++++++++++++++++++++++++- zerver/views/messages.py | 10 +++++++-- 4 files changed, 57 insertions(+), 6 deletions(-) diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 3110b49067..acf91a43ed 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -4436,7 +4436,7 @@ def do_update_embedded_data(user_profile: UserProfile, def do_update_message(user_profile: UserProfile, message: Message, topic_name: Optional[str], propagate_mode: str, content: Optional[str], rendered_content: Optional[str], prior_mention_user_ids: Set[int], - mention_user_ids: Set[int]) -> int: + mention_user_ids: Set[int], mention_data: Optional[bugdown.MentionData]=None) -> int: """ The main function for message editing. A message edit event can modify: @@ -4471,6 +4471,9 @@ def do_update_message(user_profile: UserProfile, message: Message, topic_name: O assert rendered_content is not None update_user_message_flags(message, ums) + # mention_data is required if there's a content edit. + assert mention_data is not None + # One could imagine checking realm.allow_edit_history here and # modifying the events based on that setting, but doing so # doesn't really make sense. We need to send the edit event @@ -4513,7 +4516,7 @@ def do_update_message(user_profile: UserProfile, message: Message, topic_name: O recipient=message.recipient, sender_id=message.sender_id, stream_topic=stream_topic, - possible_wildcard_mention=True, + possible_wildcard_mention=mention_data.message_has_wildcards(), ) event['push_notify_user_ids'] = list(info['push_notify_user_ids']) diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index b5d98b75aa..9b56be72d2 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -104,6 +104,7 @@ from zerver.lib.actions import ( check_delete_user_group, do_update_user_custom_profile_data_if_changed, ) +from zerver.lib.bugdown import MentionData from zerver.lib.events import ( apply_events, fetch_initial_state_data, @@ -733,12 +734,16 @@ class EventsRegisterTest(ZulipTestCase): rendered_content = render_markdown(message, content) prior_mention_user_ids = set() # type: Set[int] mentioned_user_ids = set() # type: Set[int] + mention_data = MentionData( + realm_id=self.user_profile.realm_id, + content=content, + ) events = self.do_test( lambda: do_update_message(self.user_profile, message, topic, propagate_mode, content, rendered_content, prior_mention_user_ids, - mentioned_user_ids), + mentioned_user_ids, mention_data), state_change_expected=True, ) error = schema_checker('events[0]', events[0]) diff --git a/zerver/tests/test_messages.py b/zerver/tests/test_messages.py index 7fe60f3cb6..49bfef1f15 100644 --- a/zerver/tests/test_messages.py +++ b/zerver/tests/test_messages.py @@ -2925,7 +2925,8 @@ class EditMessageTest(ZulipTestCase): content=None, rendered_content=None, prior_mention_user_ids=set(), - mention_user_ids=set() + mention_user_ids=set(), + mention_data=None, ) mock_send_event.assert_called_with(mock.ANY, mock.ANY, users_to_be_notified) @@ -2972,6 +2973,42 @@ class EditMessageTest(ZulipTestCase): users_to_be_notified = list(map(notify, [hamlet.id])) do_update_message_topic_success(hamlet, message, "Change again", users_to_be_notified) + @mock.patch("zerver.lib.actions.send_event") + def test_wildcard_mention(self, mock_send_event: mock.MagicMock) -> None: + stream_name = "Macbeth" + hamlet = self.example_user("hamlet") + cordelia = self.example_user("cordelia") + self.make_stream(stream_name, history_public_to_subscribers=True) + self.subscribe(hamlet, stream_name) + self.subscribe(cordelia, stream_name) + self.login(hamlet.email) + message_id = self.send_stream_message(hamlet.email, stream_name, "Hello everyone") + + def notify(user_id: int) -> Dict[str, Any]: + return { + "id": user_id, + "flags": ["wildcard_mentioned"] + } + + users_to_be_notified = list(map(notify, [cordelia.id, hamlet.id])) + result = self.client_patch("/json/messages/" + str(message_id), { + 'message_id': message_id, + 'content': 'Hello @**everyone**', + }) + self.assert_json_success(result) + + # Extract the send_event call where event type is 'update_message'. + # Here we assert wildcard_mention_user_ids has been set properly. + called = False + for call_args in mock_send_event.call_args_list: + (arg_realm, arg_event, arg_notified_users) = call_args[0] + if arg_event['type'] == 'update_message': + self.assertEqual(arg_event['type'], 'update_message') + self.assertEqual(arg_event['wildcard_mention_user_ids'], [cordelia.id, hamlet.id]) + self.assertEqual(arg_notified_users, users_to_be_notified) + called = True + self.assertTrue(called) + def test_propagate_topic_forward(self) -> None: self.login(self.example_email("hamlet")) id1 = self.send_stream_message(self.example_email("hamlet"), "Scotland", diff --git a/zerver/views/messages.py b/zerver/views/messages.py index 0e7f9df721..7b93857473 100644 --- a/zerver/views/messages.py +++ b/zerver/views/messages.py @@ -1499,12 +1499,17 @@ def update_message_backend(request: HttpRequest, user_profile: UserMessage, links_for_embed = set() # type: Set[str] prior_mention_user_ids = set() # type: Set[int] mention_user_ids = set() # type: Set[int] + mention_data = None # type: Optional[bugdown.MentionData] if content is not None: content = content.strip() if content == "": content = "(deleted)" content = truncate_body(content) + mention_data = bugdown.MentionData( + realm_id=user_profile.realm.id, + content=content, + ) user_info = get_user_info_for_message_updates(message.id) prior_mention_user_ids = user_info['mention_user_ids'] @@ -1515,7 +1520,8 @@ def update_message_backend(request: HttpRequest, user_profile: UserMessage, rendered_content = render_incoming_message(message, content, user_info['message_user_ids'], - user_profile.realm) + user_profile.realm, + mention_data=mention_data) links_for_embed |= message.links_for_preview mention_user_ids = message.mentions_user_ids @@ -1523,7 +1529,7 @@ def update_message_backend(request: HttpRequest, user_profile: UserMessage, number_changed = do_update_message(user_profile, message, topic_name, propagate_mode, content, rendered_content, prior_mention_user_ids, - mention_user_ids) + mention_user_ids, mention_data) # Include the number of messages changed in the logs request._log_data['extra'] = "[%s]" % (number_changed,)