From c0c30bc5f7fe04a1aa692a25b1a44e6f49bd8a5d Mon Sep 17 00:00:00 2001 From: Prakhar Pratyush Date: Wed, 31 May 2023 20:26:18 +0530 Subject: [PATCH] topic_mentions: Fetch users to be notified of @topic mentions. This commit adds the 'topic_wildcard_mention_user_ids' and 'topic_wildcard_mention_in_followed_topic_user_ids' attributes to the 'RecipientInfoResult' dataclass. Only topic participants are notified of @topic mentions. Topic participants are anyone who sent a message to a topic or reacted to a message on the topic. 'topic_wildcard_mention_in_followed_topic_user_ids' stores the ids of the topic participants who follow the topic and have enabled the wildcard mention notifications for followed topics. 'topic_wildcard_mention_user_ids' stores the ids of the topic participants for whom 'user_allows_notifications_in_StreamTopic' with setting 'wildcard_mentions_notify' returns True. --- zerver/actions/message_edit.py | 1 + zerver/actions/message_send.py | 55 +++++++++++++++++++++---- zerver/lib/stream_subscription.py | 2 + zerver/lib/topic.py | 25 ++++++++++-- zerver/tests/test_soft_deactivation.py | 2 + zerver/tests/test_users.py | 56 ++++++++++++++++++++++++++ 6 files changed, 130 insertions(+), 11 deletions(-) diff --git a/zerver/actions/message_edit.py b/zerver/actions/message_edit.py index bd795d5b8b..19f725e02a 100644 --- a/zerver/actions/message_edit.py +++ b/zerver/actions/message_edit.py @@ -466,6 +466,7 @@ def do_update_message( recipient=target_message.recipient, sender_id=target_message.sender_id, stream_topic=stream_topic, + possible_topic_wildcard_mention=mention_data.message_has_topic_wildcards(), possible_stream_wildcard_mention=mention_data.message_has_stream_wildcards(), ) diff --git a/zerver/actions/message_send.py b/zerver/actions/message_send.py index ed4b6bdf80..f9c996458d 100644 --- a/zerver/actions/message_send.py +++ b/zerver/actions/message_send.py @@ -69,7 +69,10 @@ from zerver.lib.stream_topic import StreamTopicTarget from zerver.lib.streams import access_stream_for_send_message, ensure_stream from zerver.lib.string_validation import check_stream_name from zerver.lib.timestamp import timestamp_to_datetime -from zerver.lib.topic import filter_by_exact_message_topic +from zerver.lib.topic import ( + filter_by_exact_message_topic, + participants_for_topic, +) from zerver.lib.url_preview.types import UrlEmbedData from zerver.lib.user_message import UserMessageLite, bulk_insert_ums from zerver.lib.validator import check_widget_content @@ -166,9 +169,11 @@ class RecipientInfoResult: pm_mention_push_disabled_user_ids: Set[int] stream_email_user_ids: Set[int] stream_push_user_ids: Set[int] + topic_wildcard_mention_user_ids: Set[int] stream_wildcard_mention_user_ids: Set[int] followed_topic_email_user_ids: Set[int] followed_topic_push_user_ids: Set[int] + topic_wildcard_mention_in_followed_topic_user_ids: Set[int] stream_wildcard_mention_in_followed_topic_user_ids: Set[int] muted_sender_user_ids: Set[int] um_eligible_user_ids: Set[int] @@ -195,13 +200,16 @@ def get_recipient_info( sender_id: int, stream_topic: Optional[StreamTopicTarget], possibly_mentioned_user_ids: AbstractSet[int] = set(), + possible_topic_wildcard_mention: bool = True, possible_stream_wildcard_mention: bool = True, ) -> RecipientInfoResult: stream_push_user_ids: Set[int] = set() stream_email_user_ids: Set[int] = set() + topic_wildcard_mention_user_ids: Set[int] = set() stream_wildcard_mention_user_ids: Set[int] = set() followed_topic_push_user_ids: Set[int] = set() followed_topic_email_user_ids: Set[int] = set() + topic_wildcard_mention_in_followed_topic_user_ids: Set[int] = set() stream_wildcard_mention_in_followed_topic_user_ids: Set[int] = set() muted_sender_user_ids: Set[int] = get_muting_users(sender_id) @@ -217,12 +225,23 @@ def get_recipient_info( # of this function for different message types. assert stream_topic is not None + topic_participant_user_ids: Set[int] = set() + if possible_topic_wildcard_mention: + # A topic participant is anyone who either sent or reacted to messages in the topic. + # It is expensive to call `participants_for_topic` if the topic has a large number + # of messages. But it is fine to call it here, as this gets called only if the message + # has syntax that might be a @topic mention without having confirmed the syntax isn't, say, + # in a code block. + topic_participant_user_ids = participants_for_topic( + recipient.id, stream_topic.topic_name + ) subscription_rows = ( get_subscriptions_for_send_message( realm_id=realm_id, stream_id=stream_topic.stream_id, topic_name=stream_topic.topic_name, possible_stream_wildcard_mention=possible_stream_wildcard_mention, + topic_participant_user_ids=topic_participant_user_ids, possibly_mentioned_user_ids=possibly_mentioned_user_ids, ) .annotate( @@ -293,16 +312,33 @@ def get_recipient_info( ) followed_topic_push_user_ids = followed_topic_notification_recipients("push_notifications") - if possible_stream_wildcard_mention: - # We calculate `stream_wildcard_mention_user_ids` and `followed_topic_wildcard_mention_user_ids` - # only if there's a possible stream wildcard mention in the message. This is important so as - # to avoid unnecessarily sending huge user ID lists with thousands of elements to the - # event queue (which can happen because these settings are `True` by default for new users.) - stream_wildcard_mention_user_ids = notification_recipients("wildcard_mentions_notify") - stream_wildcard_mention_in_followed_topic_user_ids = ( + if possible_stream_wildcard_mention or possible_topic_wildcard_mention: + # We calculate `wildcard_mentions_notify_user_ids` and `followed_topic_wildcard_mentions_notify_user_ids` + # only if there's a possible stream or topic wildcard mention in the message. + # This is important so as to avoid unnecessarily sending huge user ID lists with + # thousands of elements to the event queue (which can happen because these settings + # are `True` by default for new users.) + wildcard_mentions_notify_user_ids = notification_recipients("wildcard_mentions_notify") + followed_topic_wildcard_mentions_notify_user_ids = ( followed_topic_notification_recipients("wildcard_mentions_notify") ) + if possible_stream_wildcard_mention: + stream_wildcard_mention_user_ids = wildcard_mentions_notify_user_ids + stream_wildcard_mention_in_followed_topic_user_ids = ( + followed_topic_wildcard_mentions_notify_user_ids + ) + + if possible_topic_wildcard_mention: + topic_wildcard_mention_user_ids = topic_participant_user_ids.intersection( + wildcard_mentions_notify_user_ids + ) + topic_wildcard_mention_in_followed_topic_user_ids = ( + topic_participant_user_ids.intersection( + followed_topic_wildcard_mentions_notify_user_ids + ) + ) + elif recipient.type == Recipient.HUDDLE: message_to_user_ids = get_huddle_user_ids(recipient) @@ -418,9 +454,11 @@ def get_recipient_info( pm_mention_push_disabled_user_ids=pm_mention_push_disabled_user_ids, stream_push_user_ids=stream_push_user_ids, stream_email_user_ids=stream_email_user_ids, + topic_wildcard_mention_user_ids=topic_wildcard_mention_user_ids, stream_wildcard_mention_user_ids=stream_wildcard_mention_user_ids, followed_topic_push_user_ids=followed_topic_push_user_ids, followed_topic_email_user_ids=followed_topic_email_user_ids, + topic_wildcard_mention_in_followed_topic_user_ids=topic_wildcard_mention_in_followed_topic_user_ids, stream_wildcard_mention_in_followed_topic_user_ids=stream_wildcard_mention_in_followed_topic_user_ids, muted_sender_user_ids=muted_sender_user_ids, um_eligible_user_ids=um_eligible_user_ids, @@ -541,6 +579,7 @@ def build_message_send_dict( sender_id=message.sender_id, stream_topic=stream_topic, possibly_mentioned_user_ids=mention_data.get_user_ids(), + possible_topic_wildcard_mention=mention_data.message_has_topic_wildcards(), possible_stream_wildcard_mention=mention_data.message_has_stream_wildcards(), ) diff --git a/zerver/lib/stream_subscription.py b/zerver/lib/stream_subscription.py index 170c88eb39..6f70a6718d 100644 --- a/zerver/lib/stream_subscription.py +++ b/zerver/lib/stream_subscription.py @@ -307,6 +307,7 @@ def get_subscriptions_for_send_message( stream_id: int, topic_name: str, possible_stream_wildcard_mention: bool, + topic_participant_user_ids: AbstractSet[int], possibly_mentioned_user_ids: AbstractSet[int], ) -> QuerySet[Subscription]: """This function optimizes an important use case for large @@ -352,6 +353,7 @@ def get_subscriptions_for_send_message( | Q(email_notifications=True) | (Q(email_notifications=None) & Q(user_profile__enable_stream_email_notifications=True)) | Q(user_profile_id__in=possibly_mentioned_user_ids) + | Q(user_profile_id__in=topic_participant_user_ids) | Q( user_profile_id__in=AlertWord.objects.filter(realm_id=realm_id).values_list( "user_profile_id" diff --git a/zerver/lib/topic.py b/zerver/lib/topic.py index 5908e13080..da4c0404cc 100644 --- a/zerver/lib/topic.py +++ b/zerver/lib/topic.py @@ -1,15 +1,15 @@ from datetime import datetime -from typing import Any, Dict, List, Optional, Tuple +from typing import Any, Dict, List, Optional, Set, Tuple import orjson from django.db import connection -from django.db.models import Q, QuerySet +from django.db.models import Q, QuerySet, Subquery from sqlalchemy.sql import ColumnElement, column, func, literal from sqlalchemy.types import Boolean, Text from zerver.lib.request import REQ from zerver.lib.types import EditHistoryEvent -from zerver.models import Message, Stream, UserMessage, UserProfile +from zerver.models import Message, Reaction, Stream, UserMessage, UserProfile # Only use these constants for events. ORIG_TOPIC = "orig_subject" @@ -284,3 +284,22 @@ def get_topic_resolution_and_bare_name(stored_name: str) -> Tuple[bool, str]: return (True, stored_name[len(RESOLVED_TOPIC_PREFIX) :]) return (False, stored_name) + + +def participants_for_topic(recipient_id: int, topic_name: str) -> Set[int]: + """ + Users who either sent or reacted to the messages in the topic. + The function is expensive for large numbers of messages in the topic. + """ + messages = Message.objects.filter(recipient_id=recipient_id, subject__iexact=topic_name) + participants = set( + UserProfile.objects.filter( + Q(id__in=Subquery(messages.values("sender_id"))) + | Q( + id__in=Subquery( + Reaction.objects.filter(message__in=messages).values("user_profile_id") + ) + ) + ).values_list("id", flat=True) + ) + return participants diff --git a/zerver/tests/test_soft_deactivation.py b/zerver/tests/test_soft_deactivation.py index af8babc2ca..48ca22d22c 100644 --- a/zerver/tests/test_soft_deactivation.py +++ b/zerver/tests/test_soft_deactivation.py @@ -624,6 +624,7 @@ class SoftDeactivationMessageTest(ZulipTestCase): expected_count: int, *, possible_stream_wildcard_mention: bool = False, + topic_participant_user_ids: AbstractSet[int] = set(), possibly_mentioned_user_ids: AbstractSet[int] = set(), ) -> None: self.assertEqual( @@ -633,6 +634,7 @@ class SoftDeactivationMessageTest(ZulipTestCase): stream_id=stream_id, topic_name=topic_name, possible_stream_wildcard_mention=possible_stream_wildcard_mention, + topic_participant_user_ids=topic_participant_user_ids, possibly_mentioned_user_ids=possibly_mentioned_user_ids, ) ), diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py index 9bd6a0b01f..a872e32629 100644 --- a/zerver/tests/test_users.py +++ b/zerver/tests/test_users.py @@ -1755,6 +1755,7 @@ class RecipientInfoTest(ZulipTestCase): recipient=recipient, sender_id=hamlet.id, stream_topic=stream_topic, + possible_topic_wildcard_mention=False, possible_stream_wildcard_mention=False, ) @@ -1767,9 +1768,11 @@ class RecipientInfoTest(ZulipTestCase): pm_mention_push_disabled_user_ids=set(), stream_push_user_ids=set(), stream_email_user_ids=set(), + topic_wildcard_mention_user_ids=set(), stream_wildcard_mention_user_ids=set(), followed_topic_push_user_ids=set(), followed_topic_email_user_ids=set(), + topic_wildcard_mention_in_followed_topic_user_ids=set(), stream_wildcard_mention_in_followed_topic_user_ids=set(), muted_sender_user_ids=set(), um_eligible_user_ids=all_user_ids, @@ -1820,6 +1823,59 @@ class RecipientInfoTest(ZulipTestCase): ) self.assertEqual(info.stream_wildcard_mention_user_ids, {hamlet.id, othello.id}) + do_change_user_setting( + hamlet, + "wildcard_mentions_notify", + True, + acting_user=None, + ) + info = get_recipient_info( + realm_id=realm.id, + recipient=recipient, + sender_id=hamlet.id, + stream_topic=stream_topic, + possible_topic_wildcard_mention=True, + possible_stream_wildcard_mention=False, + ) + self.assertEqual(info.stream_wildcard_mention_user_ids, set()) + self.assertEqual(info.topic_wildcard_mention_user_ids, set()) + + # User who sent a message to the topic, or reacted to a message on the topic + # is only considered as a possible user to be notified for topic mention. + self.send_stream_message(hamlet, stream_name, content="test message", topic_name=topic_name) + info = get_recipient_info( + realm_id=realm.id, + recipient=recipient, + sender_id=hamlet.id, + stream_topic=stream_topic, + possible_topic_wildcard_mention=True, + possible_stream_wildcard_mention=False, + ) + self.assertEqual(info.stream_wildcard_mention_user_ids, set()) + self.assertEqual(info.topic_wildcard_mention_user_ids, {hamlet.id}) + + info = get_recipient_info( + realm_id=realm.id, + recipient=recipient, + sender_id=hamlet.id, + stream_topic=stream_topic, + possible_topic_wildcard_mention=False, + possible_stream_wildcard_mention=True, + ) + self.assertEqual(info.stream_wildcard_mention_user_ids, {hamlet.id, othello.id}) + self.assertEqual(info.topic_wildcard_mention_user_ids, set()) + + info = get_recipient_info( + realm_id=realm.id, + recipient=recipient, + sender_id=hamlet.id, + stream_topic=stream_topic, + possible_topic_wildcard_mention=True, + possible_stream_wildcard_mention=True, + ) + self.assertEqual(info.stream_wildcard_mention_user_ids, {hamlet.id, othello.id}) + self.assertEqual(info.topic_wildcard_mention_user_ids, {hamlet.id}) + sub = get_subscription(stream_name, hamlet) sub.push_notifications = False sub.save()