From ce5d13f9b289d0e8b82655fae28ed0748aa54e3f Mon Sep 17 00:00:00 2001 From: Kartik Srivastava Date: Sat, 4 Feb 2023 03:03:37 +0530 Subject: [PATCH] message_send: Handle notifications for UNMUTED topic in a muted stream. This commit adds 'visibility_policy' as a parameter to user_allows_notifications_in_StreamTopic function. This adds logic inside the user_allows_notifications_in_StreamTopic function, to not return False when a stream is muted but the topic is UNMUTED. Adds a method `user_id_to_visibility_policy_dict` to 'StreamTopicTarget' class to fetch (user_id => visibility_policy) in single db query. Co-authored-by: Kartik Srivastava Co-authored-by: Prakhar Pratyush --- zerver/actions/message_send.py | 6 +- zerver/lib/notification_data.py | 13 ++-- zerver/lib/stream_topic.py | 15 +++- zerver/tests/test_event_queue.py | 127 +++++++++++++++++++++++++++++++ zerver/tests/test_users.py | 27 +++++++ 5 files changed, 180 insertions(+), 8 deletions(-) diff --git a/zerver/actions/message_send.py b/zerver/actions/message_send.py index 2e10948e07..bdc355b0c3 100644 --- a/zerver/actions/message_send.py +++ b/zerver/actions/message_send.py @@ -210,7 +210,6 @@ def get_recipient_info( # stream_topic. We may eventually want to have different versions # of this function for different message types. assert stream_topic is not None - user_ids_muting_topic = stream_topic.user_ids_with_visibility_policy(UserTopic.MUTED) subscription_rows = ( get_subscriptions_for_send_message( @@ -240,6 +239,7 @@ def get_recipient_info( ) message_to_user_ids = [row["user_profile_id"] for row in subscription_rows] + user_id_to_visibility_policy = stream_topic.user_id_to_visibility_policy_dict() def notification_recipients(setting: str) -> Set[int]: return { @@ -247,7 +247,9 @@ def get_recipient_info( for row in subscription_rows if user_allows_notifications_in_StreamTopic( row["is_muted"], - row["user_profile_id"] in user_ids_muting_topic, + user_id_to_visibility_policy.get( + row["user_profile_id"], UserTopic.VISIBILITY_POLICY_INHERIT + ), row[setting], row["user_profile_" + setting], ) diff --git a/zerver/lib/notification_data.py b/zerver/lib/notification_data.py index e3cc68c8be..de565bde08 100644 --- a/zerver/lib/notification_data.py +++ b/zerver/lib/notification_data.py @@ -4,7 +4,7 @@ from typing import Any, Collection, Dict, List, Optional, Set from zerver.lib.mention import MentionData from zerver.lib.user_groups import get_user_group_direct_member_ids -from zerver.models import NotificationTriggers, UserGroup, UserProfile +from zerver.models import NotificationTriggers, UserGroup, UserProfile, UserTopic @dataclass @@ -176,15 +176,18 @@ class UserMessageNotificationsData: def user_allows_notifications_in_StreamTopic( stream_is_muted: bool, - topic_is_muted: bool, + visibility_policy: int, stream_specific_setting: Optional[bool], global_setting: bool, ) -> bool: """ - Captures the hierarchy of notification settings, where muting is considered first, followed - by stream-specific settings, and the global-setting in the UserProfile is the fallback. + Captures the hierarchy of notification settings, where visibility policy is considered first, + followed by stream-specific settings, and the global-setting in the UserProfile is the fallback. """ - if stream_is_muted or topic_is_muted: + if stream_is_muted and visibility_policy != UserTopic.UNMUTED: + return False + + if visibility_policy == UserTopic.MUTED: return False if stream_specific_setting is not None: diff --git a/zerver/lib/stream_topic.py b/zerver/lib/stream_topic.py index a8dbab123e..264791b6c6 100644 --- a/zerver/lib/stream_topic.py +++ b/zerver/lib/stream_topic.py @@ -1,4 +1,4 @@ -from typing import Set +from typing import Dict, Set from zerver.models import UserTopic @@ -24,3 +24,16 @@ class StreamTopicTarget: "user_profile_id", ) return {row["user_profile_id"] for row in query} + + def user_id_to_visibility_policy_dict(self) -> Dict[int, int]: + user_id_to_visibility_policy: Dict[int, int] = {} + + query = UserTopic.objects.filter( + stream_id=self.stream_id, topic_name__iexact=self.topic_name + ).values( + "visibility_policy", + "user_profile_id", + ) + for row in query: + user_id_to_visibility_policy[row["user_profile_id"]] = row["visibility_policy"] + return user_id_to_visibility_policy diff --git a/zerver/tests/test_event_queue.py b/zerver/tests/test_event_queue.py index c118211dca..a523c42c38 100644 --- a/zerver/tests/test_event_queue.py +++ b/zerver/tests/test_event_queue.py @@ -633,6 +633,133 @@ class MissedMessageHookTest(ZulipTestCase): already_notified={"email_notified": False, "push_notified": False}, ) + def test_stream_email_and_push_notify_unmuted_topic_muted_stream_with_all_notifications_turned_off( + self, + ) -> None: + # Both push and email notifications should not be sent + self.change_subscription_properties({"is_muted": True}) + do_set_user_topic_visibility_policy( + self.user_profile, + get_stream("Denmark", self.user_profile.realm), + "unmutingtest", + visibility_policy=UserTopic.UNMUTED, + ) + msg_id = self.send_stream_message( + self.iago, + "Denmark", + content="what's up everyone?", + topic_name="unmutingtest", + ) + with mock.patch("zerver.tornado.event_queue.maybe_enqueue_notifications") as mock_enqueue: + missedmessage_hook(self.user_profile.id, self.client_descriptor, True) + mock_enqueue.assert_called_once() + args_dict = mock_enqueue.call_args_list[0][1] + + self.assert_maybe_enqueue_notifications_call_args( + args_dict=args_dict, + message_id=msg_id, + user_id=self.user_profile.id, + already_notified={"email_notified": False, "push_notified": False}, + ) + + def test_stream_email_and_push_notify_unmuted_topic_muted_stream_with_global_setting_turned_on( + self, + ) -> None: + # Both push and email notifications should be sent + do_change_user_setting( + self.user_profile, "enable_stream_push_notifications", True, acting_user=None + ) + do_change_user_setting( + self.user_profile, "enable_stream_email_notifications", True, acting_user=None + ) + self.change_subscription_properties({"is_muted": True}) + do_set_user_topic_visibility_policy( + self.user_profile, + get_stream("Denmark", self.user_profile.realm), + "unmutingtest", + visibility_policy=UserTopic.UNMUTED, + ) + msg_id = self.send_stream_message( + self.iago, + "Denmark", + content="what's up everyone?", + topic_name="unmutingtest", + ) + with mock.patch("zerver.tornado.event_queue.maybe_enqueue_notifications") as mock_enqueue: + missedmessage_hook(self.user_profile.id, self.client_descriptor, True) + mock_enqueue.assert_called_once() + args_dict = mock_enqueue.call_args_list[0][1] + + self.assert_maybe_enqueue_notifications_call_args( + args_dict=args_dict, + message_id=msg_id, + user_id=self.user_profile.id, + stream_push_notify=True, + stream_email_notify=True, + already_notified={"email_notified": True, "push_notified": True}, + ) + + def test_stream_email_and_push_notify_unmuted_topic_muted_stream_with_stream_setting_turned_on( + self, + ) -> None: + # Both push and email notifications should be sent + self.change_subscription_properties( + {"push_notifications": True, "email_notifications": True, "is_muted": True} + ) + do_set_user_topic_visibility_policy( + self.user_profile, + get_stream("Denmark", self.user_profile.realm), + "unmutingtest", + visibility_policy=UserTopic.UNMUTED, + ) + msg_id = self.send_stream_message( + self.iago, + "Denmark", + content="what's up everyone?", + topic_name="unmutingtest", + ) + with mock.patch("zerver.tornado.event_queue.maybe_enqueue_notifications") as mock_enqueue: + missedmessage_hook(self.user_profile.id, self.client_descriptor, True) + mock_enqueue.assert_called_once() + args_dict = mock_enqueue.call_args_list[0][1] + + self.assert_maybe_enqueue_notifications_call_args( + args_dict=args_dict, + message_id=msg_id, + user_id=self.user_profile.id, + stream_push_notify=True, + stream_email_notify=True, + already_notified={"email_notified": True, "push_notified": True}, + ) + + def test_stream_email_and_push_notify_unmuted_topic_and_unmuted_stream( + self, + ) -> None: + # Both push and email notifications should be not sent + do_set_user_topic_visibility_policy( + self.user_profile, + get_stream("Denmark", self.user_profile.realm), + "unmutingtest", + visibility_policy=UserTopic.UNMUTED, + ) + msg_id = self.send_stream_message( + self.iago, + "Denmark", + content="what's up everyone?", + topic_name="unmutingtest", + ) + with mock.patch("zerver.tornado.event_queue.maybe_enqueue_notifications") as mock_enqueue: + missedmessage_hook(self.user_profile.id, self.client_descriptor, True) + mock_enqueue.assert_called_once() + args_dict = mock_enqueue.call_args_list[0][1] + + self.assert_maybe_enqueue_notifications_call_args( + args_dict=args_dict, + message_id=msg_id, + user_id=self.user_profile.id, + already_notified={"email_notified": False, "push_notified": False}, + ) + def test_muted_sender(self) -> None: do_mute_user(self.user_profile, self.iago) msg_id = self.send_personal_message(self.iago, self.user_profile) diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py index b0447612d8..61d003f60c 100644 --- a/zerver/tests/test_users.py +++ b/zerver/tests/test_users.py @@ -1841,6 +1841,33 @@ class RecipientInfoTest(ZulipTestCase): ) self.assertEqual(info.stream_push_user_ids, {hamlet.id}) + # Now have Hamlet mute the stream and unmute the topic, + # which shouldn't omit him from stream_push_user_ids. + sub.is_muted = True + sub.save() + + do_set_user_topic_visibility_policy( + hamlet, + stream, + topic_name, + visibility_policy=UserTopic.UNMUTED, + ) + + info = get_recipient_info( + realm_id=realm.id, + recipient=recipient, + sender_id=hamlet.id, + stream_topic=stream_topic, + ) + self.assertEqual(info.stream_push_user_ids, {hamlet.id}) + + # Now unmute the stream and remove topic visibility_policy. + sub.is_muted = False + sub.save() + do_set_user_topic_visibility_policy( + hamlet, stream, topic_name, visibility_policy=UserTopic.VISIBILITY_POLICY_INHERIT + ) + # Now have Hamlet mute the topic to omit him from stream_push_user_ids. do_set_user_topic_visibility_policy( hamlet,