diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index b72d78df25..6c0b1afb91 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -1153,6 +1153,20 @@ def do_send_messages(messages_maybe_none): presence_idle_user_ids=presence_idle_user_ids, ) + ''' + TODO: We may want to limit user_ids to only those users who have + UserMessage rows, if only for minor performance reasons. + + For now we queue events for all subscribers/sendees of the + message, since downstream code may still do notifications + that don't require UserMessage rows. + + Our automated tests have gotten better on this codepath, + but we may have coverage gaps, so we should be careful + about changing the next line. + ''' + user_ids = message['active_user_ids'] | set(user_flags.keys()) + users = [ dict( id=user_id, @@ -1160,7 +1174,7 @@ def do_send_messages(messages_maybe_none): always_push_notify=(user_id in message['push_notify_user_ids']), stream_push_notify=(user_id in message['stream_push_user_ids']), ) - for user_id in message['active_user_ids'] + for user_id in user_ids ] if message['message'].is_stream_message(): diff --git a/zerver/tests/test_messages.py b/zerver/tests/test_messages.py index adda4dc8d0..f5632d1acf 100644 --- a/zerver/tests/test_messages.py +++ b/zerver/tests/test_messages.py @@ -68,7 +68,7 @@ import DNS import mock import time import ujson -from typing import Any, Dict, List, Optional, Text +from typing import Any, Dict, List, Optional, Set, Text from collections import namedtuple @@ -593,6 +593,19 @@ class StreamMessagesTest(ZulipTestCase): message = most_recent_message(user_profile) assert(UserMessage.objects.get(user_profile=user_profile, message=message).flags.mentioned.is_set) + def _send_stream_message(self, email, stream_name, content): + # type: (Text, Text, Text) -> Set[int] + with mock.patch('zerver.lib.actions.send_event') as m: + self.send_stream_message( + email, + stream_name, + content=content + ) + self.assertEqual(m.call_count, 1) + users = m.call_args[0][1] + user_ids = {u['id'] for u in users} + return user_ids + def test_unsub_mention(self): # type: () -> None cordelia = self.example_user('cordelia') @@ -607,14 +620,15 @@ class StreamMessagesTest(ZulipTestCase): ).delete() def mention_cordelia(): - # type: () -> None + # type: () -> Set[int] content = 'test @**Cordelia Lear** rules' - self.send_stream_message( - hamlet.email, - stream_name, - content=content, + user_ids = self._send_stream_message( + email=hamlet.email, + stream_name=stream_name, + content=content ) + return user_ids def num_cordelia_messages(): # type: () -> int @@ -622,15 +636,16 @@ class StreamMessagesTest(ZulipTestCase): user_profile=cordelia ).count() - mention_cordelia() - + user_ids = mention_cordelia() self.assertEqual(0, num_cordelia_messages()) + self.assertNotIn(cordelia.id, user_ids) # Make sure test isn't too brittle-subscribing # Cordelia and mentioning her should give her a # message. self.subscribe(cordelia, stream_name) - mention_cordelia() + user_ids = mention_cordelia() + self.assertIn(cordelia.id, user_ids) self.assertEqual(1, num_cordelia_messages()) def test_message_bot_mentions(self): @@ -655,12 +670,13 @@ class StreamMessagesTest(ZulipTestCase): content = 'test @**Normal Bot** rules' - self.send_stream_message( - hamlet.email, - stream_name, + user_ids = self._send_stream_message( + email=hamlet.email, + stream_name=stream_name, content=content ) + self.assertIn(normal_bot.id, user_ids) user_message = most_recent_usermessage(normal_bot) self.assertEqual(user_message.message.content, content) self.assertTrue(user_message.flags.mentioned)