notification_data: Rename sender_id -> acting_user_id.

This better shows the situation for message edits, where we use the same
class.

zerver/lib/notification_data.py

@@ -48,21 +48,24 @@ class UserMessageNotificationsData:
     # `enable_offline_email_notifications` settings (for PMs and mentions), but currently they
     # don't.
 
-    def is_notifiable(self, private_message: bool, sender_id: int, idle: bool) -> bool:
+    # For these functions, acting_user_id is the user sent a message
+    # (or edited a message) triggering the event for which we need to
+    # determine notifiability.
+    def is_notifiable(self, private_message: bool, acting_user_id: int, idle: bool) -> bool:
         return self.is_email_notifiable(
-            private_message, sender_id, idle
-        ) or self.is_push_notifiable(private_message, sender_id, idle)
+            private_message, acting_user_id, idle
+        ) or self.is_push_notifiable(private_message, acting_user_id, idle)
 
-    def is_push_notifiable(self, private_message: bool, sender_id: int, idle: bool) -> bool:
-        return self.get_push_notification_trigger(private_message, sender_id, idle) is not None
+    def is_push_notifiable(self, private_message: bool, acting_user_id: int, idle: bool) -> bool:
+        return self.get_push_notification_trigger(private_message, acting_user_id, idle) is not None
 
     def get_push_notification_trigger(
-        self, private_message: bool, sender_id: int, idle: bool
+        self, private_message: bool, acting_user_id: int, idle: bool
     ) -> Optional[str]:
         if not idle and not self.online_push_enabled:
             return None
 
-        if self.user_id == sender_id:
+        if self.user_id == acting_user_id:
             return None
 
         if self.sender_is_muted:
@@ -79,16 +82,18 @@ class UserMessageNotificationsData:
         else:
             return None
 
-    def is_email_notifiable(self, private_message: bool, sender_id: int, idle: bool) -> bool:
-        return self.get_email_notification_trigger(private_message, sender_id, idle) is not None
+    def is_email_notifiable(self, private_message: bool, acting_user_id: int, idle: bool) -> bool:
+        return (
+            self.get_email_notification_trigger(private_message, acting_user_id, idle) is not None
+        )
 
     def get_email_notification_trigger(
-        self, private_message: bool, sender_id: int, idle: bool
+        self, private_message: bool, acting_user_id: int, idle: bool
     ) -> Optional[str]:
         if not idle:
             return None
 
-        if self.user_id == sender_id:
+        if self.user_id == acting_user_id:
             return None
 
         if self.sender_is_muted:

zerver/lib/test_classes.py

@@ -1339,7 +1339,7 @@ Output:
         )
 
     def get_maybe_enqueue_notifications_parameters(
-        self, *, message_id: int, user_id: int, sender_id: int, **kwargs: Any
+        self, *, message_id: int, user_id: int, acting_user_id: int, **kwargs: Any
     ) -> Dict[str, Any]:
         """
         Returns a dictionary with the passed parameters, after filling up the
@@ -1352,7 +1352,7 @@ Output:
         return dict(
             user_data=user_notifications_data,
             message_id=message_id,
-            sender_id=sender_id,
+            acting_user_id=acting_user_id,
             private_message=kwargs.get("private_message", False),
             stream_name=kwargs.get("stream_name", None),
             idle=kwargs.get("idle", True),

zerver/tests/test_event_queue.py

@@ -31,7 +31,7 @@ class MissedMessageNotificationsTest(ZulipTestCase):
         # This test is for verifying whether `maybe_enqueue_notifications` returns the
         # `already_notified` data correctly.
         params = self.get_maybe_enqueue_notifications_parameters(
-            message_id=1, user_id=1, sender_id=2
+            message_id=1, user_id=1, acting_user_id=2
         )
 
         with mock_queue_publish(
@@ -160,7 +160,7 @@ class MissedMessageNotificationsTest(ZulipTestCase):
         ) -> None:
             expected_args_dict = self.get_maybe_enqueue_notifications_parameters(
                 user_id=user_profile.id,
-                sender_id=iago.id,
+                acting_user_id=iago.id,
                 message_id=message_id,
                 **kwargs,
             )

zerver/tests/test_message_edit_notifications.py

@@ -187,7 +187,7 @@ class EditMessageSideEffectsTest(ZulipTestCase):
         hamlet = self.example_user("hamlet")
         expected_enqueue_kwargs = self.get_maybe_enqueue_notifications_parameters(
             user_id=cordelia.id,
-            sender_id=hamlet.id,
+            acting_user_id=hamlet.id,
             message_id=message_id,
             mentioned=True,
             flags=["mentioned"],
@@ -317,7 +317,7 @@ class EditMessageSideEffectsTest(ZulipTestCase):
 
         expected_enqueue_kwargs = self.get_maybe_enqueue_notifications_parameters(
             user_id=cordelia.id,
-            sender_id=hamlet.id,
+            acting_user_id=hamlet.id,
             message_id=message_id,
             mentioned=True,
             stream_name="Scotland",
@@ -354,7 +354,7 @@ class EditMessageSideEffectsTest(ZulipTestCase):
 
         expected_enqueue_kwargs = self.get_maybe_enqueue_notifications_parameters(
             user_id=cordelia.id,
-            sender_id=hamlet.id,
+            acting_user_id=hamlet.id,
             message_id=message_id,
             stream_name="Scotland",
             online_push_enabled=True,
@@ -391,7 +391,7 @@ class EditMessageSideEffectsTest(ZulipTestCase):
         expected_enqueue_kwargs = self.get_maybe_enqueue_notifications_parameters(
             user_id=cordelia.id,
             message_id=message_id,
-            sender_id=self.example_user("hamlet").id,
+            acting_user_id=self.example_user("hamlet").id,
             mentioned=True,
             flags=["mentioned"],
             stream_name="Scotland",
@@ -423,7 +423,7 @@ class EditMessageSideEffectsTest(ZulipTestCase):
 
         expected_enqueue_kwargs = self.get_maybe_enqueue_notifications_parameters(
             user_id=cordelia.id,
-            sender_id=hamlet.id,
+            acting_user_id=hamlet.id,
             message_id=message_id,
             wildcard_mention_notify=True,
             flags=["wildcard_mentioned"],
@@ -481,7 +481,7 @@ class EditMessageSideEffectsTest(ZulipTestCase):
 
         expected_enqueue_kwargs = self.get_maybe_enqueue_notifications_parameters(
             user_id=cordelia.id,
-            sender_id=hamlet.id,
+            acting_user_id=hamlet.id,
             message_id=message_id,
             mentioned=True,
             flags=["mentioned"],

zerver/tests/test_notification_data.py

@@ -4,30 +4,34 @@ from zerver.lib.test_classes import ZulipTestCase
 class TestNotificationData(ZulipTestCase):
     def test_is_push_notifiable(self) -> None:
         user_id = self.example_user("hamlet").id
-        sender_id = self.example_user("cordelia").id
+        acting_user_id = self.example_user("cordelia").id
 
         # Boring case
         user_data = self.create_user_notifications_data_object(user_id=user_id)
         self.assertEqual(
             user_data.get_push_notification_trigger(
-                private_message=False, sender_id=sender_id, idle=True
+                private_message=False, acting_user_id=acting_user_id, idle=True
             ),
             None,
         )
         self.assertFalse(
-            user_data.is_push_notifiable(private_message=False, sender_id=sender_id, idle=True)
+            user_data.is_push_notifiable(
+                private_message=False, acting_user_id=acting_user_id, idle=True
+            )
         )
 
         # Private message
         user_data = self.create_user_notifications_data_object(user_id=user_id)
         self.assertEqual(
             user_data.get_push_notification_trigger(
-                private_message=True, sender_id=sender_id, idle=True
+                private_message=True, acting_user_id=acting_user_id, idle=True
             ),
             "private_message",
         )
         self.assertTrue(
-            user_data.is_push_notifiable(private_message=True, sender_id=sender_id, idle=True)
+            user_data.is_push_notifiable(
+                private_message=True, acting_user_id=acting_user_id, idle=True
+            )
         )
 
         # Mention
@@ -36,12 +40,14 @@ class TestNotificationData(ZulipTestCase):
         )
         self.assertEqual(
             user_data.get_push_notification_trigger(
-                private_message=False, sender_id=sender_id, idle=True
+                private_message=False, acting_user_id=acting_user_id, idle=True
             ),
             "mentioned",
         )
         self.assertTrue(
-            user_data.is_push_notifiable(private_message=False, sender_id=sender_id, idle=True)
+            user_data.is_push_notifiable(
+                private_message=False, acting_user_id=acting_user_id, idle=True
+            )
         )
 
         # Wildcard mention
@@ -50,12 +56,14 @@ class TestNotificationData(ZulipTestCase):
         )
         self.assertEqual(
             user_data.get_push_notification_trigger(
-                private_message=False, sender_id=sender_id, idle=True
+                private_message=False, acting_user_id=acting_user_id, idle=True
             ),
             "wildcard_mentioned",
         )
         self.assertTrue(
-            user_data.is_push_notifiable(private_message=False, sender_id=sender_id, idle=True)
+            user_data.is_push_notifiable(
+                private_message=False, acting_user_id=acting_user_id, idle=True
+            )
         )
 
         # Stream notification
@@ -64,12 +72,14 @@ class TestNotificationData(ZulipTestCase):
         )
         self.assertEqual(
             user_data.get_push_notification_trigger(
-                private_message=False, sender_id=sender_id, idle=True
+                private_message=False, acting_user_id=acting_user_id, idle=True
             ),
             "stream_push_notify",
         )
         self.assertTrue(
-            user_data.is_push_notifiable(private_message=False, sender_id=sender_id, idle=True)
+            user_data.is_push_notifiable(
+                private_message=False, acting_user_id=acting_user_id, idle=True
+            )
         )
 
         # Now, test the `online_push_enabled` property
@@ -77,12 +87,14 @@ class TestNotificationData(ZulipTestCase):
         user_data = self.create_user_notifications_data_object(user_id=user_id)
         self.assertEqual(
             user_data.get_push_notification_trigger(
-                private_message=True, sender_id=sender_id, idle=False
+                private_message=True, acting_user_id=acting_user_id, idle=False
             ),
             None,
         )
         self.assertFalse(
-            user_data.is_push_notifiable(private_message=True, sender_id=sender_id, idle=False)
+            user_data.is_push_notifiable(
+                private_message=True, acting_user_id=acting_user_id, idle=False
+            )
         )
 
         # Test notifications are sent when not idle but `online_push_enabled = True`
@@ -91,12 +103,14 @@ class TestNotificationData(ZulipTestCase):
         )
         self.assertEqual(
             user_data.get_push_notification_trigger(
-                private_message=True, sender_id=sender_id, idle=False
+                private_message=True, acting_user_id=acting_user_id, idle=False
             ),
             "private_message",
         )
         self.assertTrue(
-            user_data.is_push_notifiable(private_message=True, sender_id=sender_id, idle=False)
+            user_data.is_push_notifiable(
+                private_message=True, acting_user_id=acting_user_id, idle=False
+            )
         )
 
         # The following are hypothetical cases, since a private message can never have `stream_push_notify = True`.
@@ -113,17 +127,19 @@ class TestNotificationData(ZulipTestCase):
         )
         self.assertEqual(
             user_data.get_push_notification_trigger(
-                private_message=True, sender_id=sender_id, idle=True
+                private_message=True, acting_user_id=acting_user_id, idle=True
             ),
             None,
         )
         self.assertFalse(
-            user_data.is_push_notifiable(private_message=True, sender_id=sender_id, idle=True)
+            user_data.is_push_notifiable(
+                private_message=True, acting_user_id=acting_user_id, idle=True
+            )
         )
 
         # Message sender is the user the object corresponds to.
         user_data = self.create_user_notifications_data_object(
-            user_id=sender_id,
+            user_id=acting_user_id,
             sender_is_muted=False,
             flags=["mentioned", "wildcard_mentioned"],
             wildcard_mention_notify=True,
@@ -133,40 +149,46 @@ class TestNotificationData(ZulipTestCase):
         )
         self.assertEqual(
             user_data.get_push_notification_trigger(
-                private_message=True, sender_id=sender_id, idle=True
+                private_message=True, acting_user_id=acting_user_id, idle=True
             ),
             None,
         )
         self.assertFalse(
-            user_data.is_push_notifiable(private_message=True, sender_id=sender_id, idle=True)
+            user_data.is_push_notifiable(
+                private_message=True, acting_user_id=acting_user_id, idle=True
+            )
         )
 
     def test_is_email_notifiable(self) -> None:
         user_id = self.example_user("hamlet").id
-        sender_id = self.example_user("cordelia").id
+        acting_user_id = self.example_user("cordelia").id
 
         # Boring case
         user_data = self.create_user_notifications_data_object(user_id=user_id)
         self.assertEqual(
             user_data.get_email_notification_trigger(
-                private_message=False, sender_id=sender_id, idle=True
+                private_message=False, acting_user_id=acting_user_id, idle=True
             ),
             None,
         )
         self.assertFalse(
-            user_data.is_email_notifiable(private_message=False, sender_id=sender_id, idle=True)
+            user_data.is_email_notifiable(
+                private_message=False, acting_user_id=acting_user_id, idle=True
+            )
         )
 
         # Private message
         user_data = self.create_user_notifications_data_object(user_id=user_id)
         self.assertEqual(
             user_data.get_email_notification_trigger(
-                private_message=True, sender_id=sender_id, idle=True
+                private_message=True, acting_user_id=acting_user_id, idle=True
             ),
             "private_message",
         )
         self.assertTrue(
-            user_data.is_email_notifiable(private_message=True, sender_id=sender_id, idle=True)
+            user_data.is_email_notifiable(
+                private_message=True, acting_user_id=acting_user_id, idle=True
+            )
         )
 
         # Mention
@@ -175,12 +197,14 @@ class TestNotificationData(ZulipTestCase):
         )
         self.assertEqual(
             user_data.get_email_notification_trigger(
-                private_message=False, sender_id=sender_id, idle=True
+                private_message=False, acting_user_id=acting_user_id, idle=True
             ),
             "mentioned",
         )
         self.assertTrue(
-            user_data.is_email_notifiable(private_message=False, sender_id=sender_id, idle=True)
+            user_data.is_email_notifiable(
+                private_message=False, acting_user_id=acting_user_id, idle=True
+            )
         )
 
         # Wildcard mention
@@ -189,12 +213,14 @@ class TestNotificationData(ZulipTestCase):
         )
         self.assertEqual(
             user_data.get_email_notification_trigger(
-                private_message=False, sender_id=sender_id, idle=True
+                private_message=False, acting_user_id=acting_user_id, idle=True
             ),
             "wildcard_mentioned",
         )
         self.assertTrue(
-            user_data.is_email_notifiable(private_message=False, sender_id=sender_id, idle=True)
+            user_data.is_email_notifiable(
+                private_message=False, acting_user_id=acting_user_id, idle=True
+            )
         )
 
         # Stream notification
@@ -203,24 +229,28 @@ class TestNotificationData(ZulipTestCase):
         )
         self.assertEqual(
             user_data.get_email_notification_trigger(
-                private_message=False, sender_id=sender_id, idle=True
+                private_message=False, acting_user_id=acting_user_id, idle=True
             ),
             "stream_email_notify",
         )
         self.assertTrue(
-            user_data.is_email_notifiable(private_message=False, sender_id=sender_id, idle=True)
+            user_data.is_email_notifiable(
+                private_message=False, acting_user_id=acting_user_id, idle=True
+            )
         )
 
         # Test no notifications when not idle
         user_data = self.create_user_notifications_data_object(user_id=user_id)
         self.assertEqual(
             user_data.get_email_notification_trigger(
-                private_message=True, sender_id=sender_id, idle=False
+                private_message=True, acting_user_id=acting_user_id, idle=False
             ),
             None,
         )
         self.assertFalse(
-            user_data.is_email_notifiable(private_message=True, sender_id=sender_id, idle=False)
+            user_data.is_email_notifiable(
+                private_message=True, acting_user_id=acting_user_id, idle=False
+            )
         )
 
         # The following are hypothetical cases, since a private message can never have `stream_email_notify = True`.
@@ -237,17 +267,19 @@ class TestNotificationData(ZulipTestCase):
         )
         self.assertEqual(
             user_data.get_email_notification_trigger(
-                private_message=True, sender_id=sender_id, idle=True
+                private_message=True, acting_user_id=acting_user_id, idle=True
             ),
             None,
         )
         self.assertFalse(
-            user_data.is_email_notifiable(private_message=True, sender_id=sender_id, idle=True)
+            user_data.is_email_notifiable(
+                private_message=True, acting_user_id=acting_user_id, idle=True
+            )
         )
 
         # Message sender is the user the object corresponds to.
         user_data = self.create_user_notifications_data_object(
-            user_id=sender_id,
+            user_id=acting_user_id,
             sender_is_muted=False,
             flags=["mentioned", "wildcard_mentioned"],
             wildcard_mention_notify=True,
@@ -257,20 +289,22 @@ class TestNotificationData(ZulipTestCase):
         )
         self.assertEqual(
             user_data.get_email_notification_trigger(
-                private_message=True, sender_id=sender_id, idle=True
+                private_message=True, acting_user_id=acting_user_id, idle=True
             ),
             None,
         )
         self.assertFalse(
-            user_data.is_email_notifiable(private_message=True, sender_id=sender_id, idle=True)
+            user_data.is_email_notifiable(
+                private_message=True, acting_user_id=acting_user_id, idle=True
+            )
         )
 
     def test_is_notifiable(self) -> None:
         # This is just for coverage purposes. We've already tested all scenarios above,
         # and `is_notifiable` is a simple OR of the email and push functions.
         user_id = self.example_user("hamlet").id
-        sender_id = self.example_user("cordelia").id
+        acting_user_id = self.example_user("cordelia").id
         user_data = self.create_user_notifications_data_object(user_id=user_id)
         self.assertTrue(
-            user_data.is_notifiable(private_message=True, sender_id=sender_id, idle=True)
+            user_data.is_notifiable(private_message=True, acting_user_id=acting_user_id, idle=True)
         )

zerver/tornado/event_queue.py

@@ -768,7 +768,7 @@ def missedmessage_hook(
         )
         maybe_enqueue_notifications(
             user_data=user_notifications_data,
-            sender_id=sender_id,
+            acting_user_id=sender_id,
             message_id=message_id,
             private_message=private_message,
             stream_name=stream_name,
@@ -791,7 +791,7 @@ def receiver_is_off_zulip(user_profile_id: int) -> bool:
 def maybe_enqueue_notifications(
     *,
     user_data: UserMessageNotificationsData,
-    sender_id: int,
+    acting_user_id: int,
     message_id: int,
     private_message: bool,
     stream_name: Optional[str],
@@ -807,10 +807,10 @@ def maybe_enqueue_notifications(
     """
     notified: Dict[str, bool] = {}
 
-    if user_data.is_push_notifiable(private_message, sender_id, idle):
+    if user_data.is_push_notifiable(private_message, acting_user_id, idle):
         notice = build_offline_notification(user_data.user_id, message_id)
         notice["trigger"] = user_data.get_push_notification_trigger(
-            private_message, sender_id, idle
+            private_message, acting_user_id, idle
         )
         notice["stream_name"] = stream_name
         if not already_notified.get("push_notified"):
@@ -821,10 +821,10 @@ def maybe_enqueue_notifications(
     # mention.  Eventually, we'll add settings to allow email
     # notifications to match the model of push notifications
     # above.
-    if user_data.is_email_notifiable(private_message, sender_id, idle):
+    if user_data.is_email_notifiable(private_message, acting_user_id, idle):
         notice = build_offline_notification(user_data.user_id, message_id)
         notice["trigger"] = user_data.get_email_notification_trigger(
-            private_message, sender_id, idle
+            private_message, acting_user_id, idle
         )
         notice["stream_name"] = stream_name
         if not already_notified.get("email_notified"):
@@ -952,7 +952,9 @@ def process_message_event(
         # shouldn't receive notifications even if they were online. In that case we can
         # avoid the more expensive `receiver_is_off_zulip` call, and move on to process
         # the next user.
-        if not user_notifications_data.is_notifiable(private_message, sender_id, idle=True):
+        if not user_notifications_data.is_notifiable(
+            private_message, acting_user_id=sender_id, idle=True
+        ):
             continue
 
         idle = receiver_is_off_zulip(user_profile_id) or (user_profile_id in presence_idle_user_ids)
@@ -962,7 +964,7 @@ def process_message_event(
         extra_user_data[user_profile_id]["internal_data"].update(
             maybe_enqueue_notifications(
                 user_data=user_notifications_data,
-                sender_id=sender_id,
+                acting_user_id=sender_id,
                 message_id=message_id,
                 private_message=private_message,
                 stream_name=stream_name,
@@ -1100,22 +1102,17 @@ def process_message_update_event(
         user_profile_id = user_data["id"]
 
         if "user_id" in event_template:
-            # This is inaccurate: the user we'll get here will be the
-            # sender if the message's content was edited, which is
-            # typically where we might send new notifications.
-            # However, for topic/stream edits, it could be another
-            # user.  We may need to adjust the format for
-            # update_message events to address this issue.
-            message_sender_id = event_template["user_id"]
+            # The user we'll get here will be the sender if the message's
+            # content was edited, and the editor for topic edits. That's
+            # the correct "acting_user" for both cases.
+            acting_user_id = event_template["user_id"]
         else:
-            # This is also inaccurate, but usefully so.  Events
-            # without a `user_id` field come from the
-            # do_update_embedded_data code path, and represent not an
-            # update to the raw content, but instead just rendering
-            # previews. Setting the current user at the sender is a
-            # hack to simplify notifications logic for this code
-            # path. TODO: Change this to short-circuit more directly.
-            message_sender_id = user_profile_id
+            # Events without a `user_id` field come from the do_update_embedded_data
+            # code path, and represent just rendering previews; there should be no
+            # real content changes.
+            # It doesn't really matter what we set `acting_user_id` in this case,
+            # becuase we know this event isn't meant to send notifications.
+            acting_user_id = user_profile_id
 
         user_event = dict(event_template)  # shallow copy, but deep enough for our needs
         for key in user_data.keys():
@@ -1136,7 +1133,7 @@ def process_message_update_event(
         maybe_enqueue_notifications_for_message_update(
             user_data=user_notifications_data,
             message_id=message_id,
-            sender_id=message_sender_id,
+            acting_user_id=acting_user_id,
             private_message=(stream_name is None),
             stream_name=stream_name,
             presence_idle=(user_profile_id in presence_idle_user_ids),
@@ -1153,7 +1150,7 @@ def process_message_update_event(
 def maybe_enqueue_notifications_for_message_update(
     user_data: UserMessageNotificationsData,
     message_id: int,
-    sender_id: int,
+    acting_user_id: int,
     private_message: bool,
     stream_name: Optional[str],
     presence_idle: bool,
@@ -1197,7 +1194,7 @@ def maybe_enqueue_notifications_for_message_update(
     maybe_enqueue_notifications(
         user_data=user_data,
         message_id=message_id,
-        sender_id=sender_id,
+        acting_user_id=acting_user_id,
         private_message=private_message,
         stream_name=stream_name,
         idle=idle,