get_active_presence_idle_user_ids: Check notifiability more thoroughly.

* Have the `get_active_presence_idle_user_ids` function look at all the
user data, not just `private_message` and `mentioned`.
* Fix a couple of incorrect `missedmessage_hook` tests, which did not
catch the earlier behaviour.
* Add some comments to the tests for this function for clarity.
* Add a helper to create `UserMessageNotificationsData` objects from the
user ID lists. This will later help us deduplicate code in the event_queue
logic.

This fixes a bug which earlier existed, that if a user turned on stream
notifications, and received a message in that stream which did not mention
them, they wouldn't be in the `presence_idle_users` list, and hence would
never get notifications for that message.

Note that, after this commit, users might still not get notifications in
the above scenarios in some cases, because the downstream logic in the
notification queue consumers sometimes erroneously skips sending
notifications for stream messages.

zerver/lib/actions.py

@@ -110,6 +110,7 @@ from zerver.lib.message import (
     update_first_visible_message_id,
     wildcard_mention_allowed,
 )
+from zerver.lib.notification_data import UserMessageNotificationsData
 from zerver.lib.pysa import mark_sanitized
 from zerver.lib.queue import queue_json_publish
 from zerver.lib.realm_icon import realm_icon_url
@@ -1962,13 +1963,24 @@ def do_send_messages(
 
         sender = send_request.message.sender
         message_type = wide_message_dict["type"]
+        active_users_data = [
+            UserMessageNotificationsData.from_user_id_sets(
+                user_id=user_id,
+                flags=user_flags.get(user_id, []),
+                online_push_user_ids=send_request.online_push_user_ids,
+                stream_push_user_ids=send_request.stream_push_user_ids,
+                stream_email_user_ids=send_request.stream_email_user_ids,
+                wildcard_mention_user_ids=send_request.wildcard_mention_user_ids,
+                muted_sender_user_ids=send_request.muted_sender_user_ids,
+            )
+            for user_id in send_request.active_user_ids
+        ]
 
         presence_idle_user_ids = get_active_presence_idle_user_ids(
             realm=sender.realm,
             sender_id=sender.id,
             message_type=message_type,
-            active_user_ids=send_request.active_user_ids,
-            user_flags=user_flags,
+            active_users_data=active_users_data,
         )
 
         event = dict(
@@ -6557,15 +6569,13 @@ def get_active_presence_idle_user_ids(
     realm: Realm,
     sender_id: int,
     message_type: str,
-    active_user_ids: Set[int],
-    user_flags: Dict[int, List[str]],
+    active_users_data: List[UserMessageNotificationsData],
 ) -> List[int]:
     """
     Given a list of active_user_ids, we build up a subset
     of those users who fit these criteria:
 
-        * They are likely to need notifications (either due
-          to mentions, alert words, or being PM'ed).
+        * They are likely to need notifications.
         * They are no longer "present" according to the
           UserPresence table.
     """
@@ -6573,16 +6583,17 @@ def get_active_presence_idle_user_ids(
     if realm.presence_disabled:
         return []
 
-    is_pm = message_type == "private"
+    is_private_message = message_type == "private"
 
     user_ids = set()
-    for user_id in active_user_ids:
-        flags = user_flags.get(user_id, [])
-        mentioned = "mentioned" in flags or "wildcard_mentioned" in flags
-        private_message = is_pm and user_id != sender_id
-        alerted = "has_alert_word" in flags
-        if mentioned or private_message or alerted:
-            user_ids.add(user_id)
+    for user_data in active_users_data:
+        alerted = "has_alert_word" in user_data.flags
+
+        # We only need to know the presence idle state for a user if this message would be notifiable
+        # for them if they were indeed idle. Only including those users in the calculation below is a
+        # very important optimization for open communities with many inactive users.
+        if user_data.is_notifiable(is_private_message, sender_id, idle=True) or alerted:
+            user_ids.add(user_data.id)
 
     return filter_presence_idle_user_ids(user_ids)
 

zerver/lib/notification_data.py

@@ -1,5 +1,5 @@
 from dataclasses import dataclass
-from typing import Collection
+from typing import Collection, Set
 
 
 @dataclass
@@ -19,6 +19,31 @@ class UserMessageNotificationsData:
         if self.wildcard_mention_notify:
             assert "wildcard_mentioned" in self.flags
 
+    @classmethod
+    def from_user_id_sets(
+        cls,
+        user_id: int,
+        flags: Collection[str],
+        online_push_user_ids: Set[int],
+        stream_push_user_ids: Set[int],
+        stream_email_user_ids: Set[int],
+        wildcard_mention_user_ids: Set[int],
+        muted_sender_user_ids: Set[int],
+    ) -> "UserMessageNotificationsData":
+        wildcard_mention_notify = (
+            user_id in wildcard_mention_user_ids and "wildcard_mentioned" in flags
+        )
+        return cls(
+            id=user_id,
+            flags=flags,
+            mentioned=("mentioned" in flags),
+            online_push_enabled=(user_id in online_push_user_ids),
+            stream_push_notify=(user_id in stream_push_user_ids),
+            stream_email_notify=(user_id in stream_email_user_ids),
+            wildcard_mention_notify=wildcard_mention_notify,
+            sender_is_muted=(user_id in muted_sender_user_ids),
+        )
+
     # TODO: The following functions should also look at the `enable_offline_push_notifications` and
     # `enable_offline_email_notifications` settings (for PMs and mentions), but currently they
     # don't.

zerver/tests/test_event_queue.py

@@ -439,7 +439,7 @@ class MissedMessageNotificationsTest(ZulipTestCase):
                 stream_push_notify=True,
                 stream_email_notify=False,
                 stream_name="Denmark",
-                already_notified={"email_notified": False, "push_notified": False},
+                already_notified={"email_notified": False, "push_notified": True},
             )
         destroy_event_queue(client_descriptor.event_queue.id)
 
@@ -462,7 +462,7 @@ class MissedMessageNotificationsTest(ZulipTestCase):
                 stream_push_notify=False,
                 stream_email_notify=True,
                 stream_name="Denmark",
-                already_notified={"email_notified": False, "push_notified": False},
+                already_notified={"email_notified": True, "push_notified": False},
             )
         destroy_event_queue(client_descriptor.event_queue.id)
 

zerver/tests/test_messages.py

@@ -1,5 +1,5 @@
 import datetime
-from typing import Dict, List
+from typing import List
 
 from django.utils.timezone import now as timezone_now
 
@@ -22,17 +22,18 @@ class MissedMessageTest(ZulipTestCase):
         realm = sender.realm
         hamlet = self.example_user("hamlet")
         othello = self.example_user("othello")
-        recipient_ids = {hamlet.id, othello.id}
-        message_type = "stream"
-        user_flags: Dict[int, List[str]] = {}
+        user_data_objects = [
+            self.create_user_notifications_data_object(id=hamlet.id),
+            self.create_user_notifications_data_object(id=othello.id),
+        ]
+        message_type = "private"
 
         def assert_missing(user_ids: List[int]) -> None:
             presence_idle_user_ids = get_active_presence_idle_user_ids(
                 realm=realm,
                 sender_id=sender.id,
                 message_type=message_type,
-                active_user_ids=recipient_ids,
-                user_flags=user_flags,
+                active_users_data=user_data_objects,
             )
             self.assertEqual(sorted(user_ids), sorted(presence_idle_user_ids))
 
@@ -48,16 +49,34 @@ class MissedMessageTest(ZulipTestCase):
         message_type = "private"
         assert_missing([hamlet.id, othello.id])
 
+        # We have already thoroughly tested the `is_notifiable` function elsewhere,
+        # so we needn't test all cases here. This test exists mainly to avoid a bug
+        # which existed earlier with `get_active_presence_idle_user_ids` only looking
+        # at `private_message` and the `mentioned` flag, not stream level notifications.
+        # Simulate Hamlet has turned on notifications for the stream, and test that he's
+        # in the list.
         message_type = "stream"
-        user_flags[hamlet.id] = ["mentioned"]
+        user_data_objects[0].stream_email_notify = True
         assert_missing([hamlet.id])
 
+        # We don't currently send push or email notifications for alert words -- only
+        # desktop notifications, so `is_notifiable` will return False even if the flags contain
+        # alert words. Test that `get_active_presence_idle_user_ids` correctly includes even
+        # the alert word case in the list.
+        user_data_objects[0].stream_email_notify = False
+        user_data_objects[0].flags = ["has_alert_word"]
+        assert_missing([hamlet.id])
+
+        # Hamlet is idle (and the message has an alert word), so he should be in the list.
         set_presence(hamlet, "iPhone", ago=5000)
         assert_missing([hamlet.id])
 
+        # If Hamlet is active, don't include him in the `presence_idle` list.
         set_presence(hamlet, "website", ago=15)
         assert_missing([])
 
+        # Hamlet is active now, so only Othello should be in the list for a huddle
+        # message.
         message_type = "private"
         assert_missing([othello.id])