email_notification: Include prior message context only when mentioned.

Earlier, email message notifications included prior messages sent
to the same topic for context. This is more confusing than helpful
for messages that the user is likely to have received notifications
for all the prior messages in the conversation already (or read them
in the Zulip UI).

Now, we include prior context only when the user is mentioned via
personal, group, stream or topic wildcard mention.

Fixes #27479.

zerver/lib/email_notifications.py

@@ -642,7 +642,7 @@ def handle_missedmessage_emails(
 
     for msg_list in messages_by_bucket.values():
         msg = min(msg_list, key=lambda msg: msg.date_sent)
-        if msg.is_stream_message():
+        if msg.is_stream_message() and UserMessage.has_any_mentions(user_profile_id, msg.id):
             context_messages = get_context_for_message(msg)
             filtered_context_messages = bulk_access_messages(user_profile, context_messages)
             msg_list.extend(filtered_context_messages)

zerver/models.py

@@ -3662,6 +3662,20 @@ class UserMessage(AbstractUserMessage):
         """
         return UserMessage.objects.select_for_update().order_by("message_id")
 
+    @staticmethod
+    def has_any_mentions(user_profile_id: int, message_id: int) -> bool:
+        # The query uses the 'zerver_usermessage_any_mentioned_message_id' index.
+        return UserMessage.objects.filter(
+            Q(
+                flags__andnz=UserMessage.flags.mentioned.mask
+                | UserMessage.flags.wildcard_mentioned.mask
+                | UserMessage.flags.topic_wildcard_mentioned.mask
+                | UserMessage.flags.group_mentioned.mask
+            ),
+            user_profile_id=user_profile_id,
+            message_id=message_id,
+        ).exists()
+
 
 def get_usermessage_by_message_id(
     user_profile: UserProfile, message_id: int

zerver/tests/test_message_notification_emails.py

@@ -216,18 +216,36 @@ class TestMessageNotificationEmails(ZulipTestCase):
         )
 
     def _extra_context_in_missed_stream_messages_topic_wildcard_mention_in_followed_topic(
-        self, show_message_content: bool = True
+        self,
+        show_message_content: bool = True,
+        *,
+        receiver_is_participant: bool,
     ) -> None:
-        for i in range(1, 6):
+        for i in range(1, 3):
             self.send_stream_message(self.example_user("othello"), "Denmark", content=str(i))
         self.send_stream_message(self.example_user("othello"), "Denmark", "11", topic_name="test2")
+
+        if receiver_is_participant:
+            self.send_stream_message(self.example_user("hamlet"), "Denmark", content="hello")
+
         msg_id = self.send_stream_message(self.example_user("othello"), "Denmark", "@**topic**")
+        trigger = NotificationTriggers.TOPIC_WILDCARD_MENTION_IN_FOLLOWED_TOPIC
+        if not receiver_is_participant:
+            trigger = NotificationTriggers.STREAM_EMAIL
 
         if show_message_content:
+            # If Hamlet (receiver) is not a topic participant, @topic doesn't mention him,
+            # so he won't receive added context (previous messages) in the email.
+            if receiver_is_participant:
                 verify_body_include = [
-                "Othello, the Moor of Venice: > 1 > 2 > 3 > 4 > 5 > @**topic** -- ",
+                    "Othello, the Moor of Venice: > 1 > 2 King Hamlet: > hello Othello, the Moor of Venice: > @**topic** -- ",
                     "You are receiving this because all topic participants were mentioned in #Denmark > test.",
                 ]
+            else:
+                verify_body_include = [
+                    "Othello, the Moor of Venice: > @**topic** -- ",
+                    "You are receiving this because you have email notifications enabled for #Denmark.",
+                ]
             email_subject = "#Denmark > test"
             verify_body_does_not_include: List[str] = []
         else:
@@ -252,7 +270,7 @@ class TestMessageNotificationEmails(ZulipTestCase):
             email_subject,
             show_message_content=show_message_content,
             verify_body_does_not_include=verify_body_does_not_include,
-            trigger=NotificationTriggers.TOPIC_WILDCARD_MENTION_IN_FOLLOWED_TOPIC,
+            trigger=trigger,
         )
 
     def _extra_context_in_missed_stream_messages_stream_wildcard_mention_in_followed_topic(
@@ -297,18 +315,36 @@ class TestMessageNotificationEmails(ZulipTestCase):
         )
 
     def _extra_context_in_missed_stream_messages_topic_wildcard_mention(
-        self, show_message_content: bool = True
+        self,
+        show_message_content: bool = True,
+        *,
+        receiver_is_participant: bool,
     ) -> None:
-        for i in range(1, 6):
+        for i in range(1, 3):
             self.send_stream_message(self.example_user("othello"), "Denmark", content=str(i))
         self.send_stream_message(self.example_user("othello"), "Denmark", "11", topic_name="test2")
+
+        if receiver_is_participant:
+            self.send_stream_message(self.example_user("hamlet"), "Denmark", content="hello")
+
         msg_id = self.send_stream_message(self.example_user("othello"), "denmark", "@**topic**")
+        trigger = NotificationTriggers.TOPIC_WILDCARD_MENTION
+        if not receiver_is_participant:
+            trigger = NotificationTriggers.STREAM_EMAIL
 
         if show_message_content:
+            # If Hamlet (receiver) is not a topic participant, @topic doesn't mention him,
+            # so he won't receive added context (previous messages) in the email.
+            if receiver_is_participant:
                 verify_body_include = [
-                "Othello, the Moor of Venice: > 1 > 2 > 3 > 4 > 5 > @**topic** -- ",
+                    "Othello, the Moor of Venice: > 1 > 2 King Hamlet: > hello Othello, the Moor of Venice: > @**topic** -- ",
                     "You are receiving this because all topic participants were mentioned in #Denmark > test.",
                 ]
+            else:
+                verify_body_include = [
+                    "Othello, the Moor of Venice: > @**topic** -- ",
+                    "You are receiving this because you have email notifications enabled for #Denmark.",
+                ]
             email_subject = "#Denmark > test"
             verify_body_does_not_include: List[str] = []
         else:
@@ -333,7 +369,7 @@ class TestMessageNotificationEmails(ZulipTestCase):
             email_subject,
             show_message_content=show_message_content,
             verify_body_does_not_include=verify_body_does_not_include,
-            trigger=NotificationTriggers.TOPIC_WILDCARD_MENTION,
+            trigger=trigger,
         )
 
     def _extra_context_in_missed_stream_messages_stream_wildcard_mention(
@@ -383,7 +419,7 @@ class TestMessageNotificationEmails(ZulipTestCase):
         self.send_stream_message(self.example_user("othello"), "Denmark", "11", topic_name="test2")
         msg_id = self.send_stream_message(self.example_user("othello"), "denmark", "12")
         verify_body_include = [
-            "Othello, the Moor of Venice: > 1 > 2 > 3 > 4 > 5 > 6 > 7 > 8 > 9 > 10 > 12 -- ",
+            "Othello, the Moor of Venice: > 12 -- ",
             "You are receiving this because you have email notifications enabled for #Denmark.",
         ]
         email_subject = "#Denmark > test"
@@ -432,7 +468,7 @@ class TestMessageNotificationEmails(ZulipTestCase):
         self.assert_json_success(self.resolve_topic_containing_message(othello_user, msg_id))
 
         verify_body_include = [
-            "Othello, the Moor of Venice: > 0 > 1 > 2 -- ",
+            "Othello, the Moor of Venice: > 2 -- ",
             "You are receiving this because you have email notifications enabled for #Denmark.",
         ]
         email_subject = "[resolved] #Denmark > threading and so forth"
@@ -987,7 +1023,8 @@ class TestMessageNotificationEmails(ZulipTestCase):
         self._extra_context_in_missed_stream_messages_mention(show_message_content=False)
         mail.outbox = []
         self._extra_context_in_missed_stream_messages_topic_wildcard_mention_in_followed_topic(
-            show_message_content=False
+            show_message_content=False,
+            receiver_is_participant=True,
         )
         mail.outbox = []
         self._extra_context_in_missed_stream_messages_stream_wildcard_mention_in_followed_topic(
@@ -995,7 +1032,8 @@ class TestMessageNotificationEmails(ZulipTestCase):
         )
         mail.outbox = []
         self._extra_context_in_missed_stream_messages_topic_wildcard_mention(
-            show_message_content=False
+            show_message_content=False,
+            receiver_is_participant=True,
         )
         mail.outbox = []
         self._extra_context_in_missed_stream_messages_stream_wildcard_mention(
@@ -1014,7 +1052,16 @@ class TestMessageNotificationEmails(ZulipTestCase):
     def test_extra_context_in_missed_stream_messages_topic_wildcard_in_followed_topic(
         self,
     ) -> None:
-        self._extra_context_in_missed_stream_messages_topic_wildcard_mention_in_followed_topic()
+        self._extra_context_in_missed_stream_messages_topic_wildcard_mention_in_followed_topic(
+            receiver_is_participant=True
+        )
+
+    def test_extra_context_in_missed_stream_messages_topic_wildcard_in_followed_topic_receiver_not_participant(
+        self,
+    ) -> None:
+        self._extra_context_in_missed_stream_messages_topic_wildcard_mention_in_followed_topic(
+            receiver_is_participant=False
+        )
 
     def test_extra_context_in_missed_stream_messages_stream_wildcard_in_followed_topic(
         self,
@@ -1022,7 +1069,16 @@ class TestMessageNotificationEmails(ZulipTestCase):
         self._extra_context_in_missed_stream_messages_stream_wildcard_mention_in_followed_topic()
 
     def test_extra_context_in_missed_stream_messages_topic_wildcard(self) -> None:
-        self._extra_context_in_missed_stream_messages_topic_wildcard_mention()
+        self._extra_context_in_missed_stream_messages_topic_wildcard_mention(
+            receiver_is_participant=True
+        )
+
+    def test_extra_context_in_missed_stream_messages_topic_wildcard_receiver_not_participant(
+        self,
+    ) -> None:
+        self._extra_context_in_missed_stream_messages_topic_wildcard_mention(
+            receiver_is_participant=False
+        )
 
     def test_extra_context_in_missed_stream_messages_stream_wildcard(self) -> None:
         self._extra_context_in_missed_stream_messages_stream_wildcard_mention()