From c727b36e9c188abfb4adebd6efd60dc5f7d18d38 Mon Sep 17 00:00:00 2001 From: Prakhar Pratyush Date: Sat, 9 Dec 2023 01:23:31 +0530 Subject: [PATCH] soft_reactivate: Soft reactivate if group mention has < 12 members. Earlier, we didn't soft-reactivate users for group mentions at all because it wasn't easy to calculate group size. Now, we will soft reactivate if the user group mentions has less than 12 members. We don't reactivate all users because a user group can have a very large size, which can lead to large backlogs in the deferred-work queue. Fixes part of #27586. --- zerver/lib/email_notifications.py | 4 +- zerver/lib/push_notifications.py | 6 ++- zerver/lib/soft_deactivation.py | 35 ++++++++++----- .../tests/test_message_notification_emails.py | 42 ++++++++++++++--- zerver/tests/test_push_notifications.py | 45 +++++++++++++++---- zproject/default_settings.py | 4 ++ 6 files changed, 109 insertions(+), 27 deletions(-) diff --git a/zerver/lib/email_notifications.py b/zerver/lib/email_notifications.py index 1202faf633..52f1df0852 100644 --- a/zerver/lib/email_notifications.py +++ b/zerver/lib/email_notifications.py @@ -414,9 +414,11 @@ def do_send_missedmessage_events_reply_in_zulip( ) mentioned_user_group_name = None + mentioned_user_group_members_count = None mentioned_user_group = get_mentioned_user_group(missed_messages, user_profile) if mentioned_user_group is not None: mentioned_user_group_name = mentioned_user_group.name + mentioned_user_group_members_count = mentioned_user_group.members_count triggers = [message["trigger"] for message in missed_messages] unique_triggers = set(triggers) @@ -564,7 +566,7 @@ def do_send_missedmessage_events_reply_in_zulip( # Soft reactivate the long_term_idle user personally mentioned soft_reactivate_if_personal_notification( - user_profile, unique_triggers, mentioned_user_group_name + user_profile, unique_triggers, mentioned_user_group_members_count ) with override_language(user_profile.default_language): diff --git a/zerver/lib/push_notifications.py b/zerver/lib/push_notifications.py index 58e08210ef..970fd32c4c 100644 --- a/zerver/lib/push_notifications.py +++ b/zerver/lib/push_notifications.py @@ -1356,13 +1356,17 @@ def handle_push_notification(user_profile_id: int, missed_message: Dict[str, Any # message or not. mentioned_user_group_id = None mentioned_user_group_name = None + mentioned_user_group_members_count = None mentioned_user_group = get_mentioned_user_group([missed_message], user_profile) if mentioned_user_group is not None: mentioned_user_group_id = mentioned_user_group.id mentioned_user_group_name = mentioned_user_group.name + mentioned_user_group_members_count = mentioned_user_group.members_count # Soft reactivate if pushing to a long_term_idle user that is personally mentioned - soft_reactivate_if_personal_notification(user_profile, {trigger}, mentioned_user_group_name) + soft_reactivate_if_personal_notification( + user_profile, {trigger}, mentioned_user_group_members_count + ) if message.is_stream_message(): # This will almost always be True. The corner case where you diff --git a/zerver/lib/soft_deactivation.py b/zerver/lib/soft_deactivation.py index 8d7d40256c..0a4c45c9c5 100644 --- a/zerver/lib/soft_deactivation.py +++ b/zerver/lib/soft_deactivation.py @@ -400,27 +400,35 @@ def queue_soft_reactivation(user_profile_id: int) -> None: def soft_reactivate_if_personal_notification( - user_profile: UserProfile, unique_triggers: Set[str], mentioned_user_group_name: Optional[str] + user_profile: UserProfile, + unique_triggers: Set[str], + mentioned_user_group_members_count: Optional[int], ) -> None: """When we're about to send an email/push notification to a long_term_idle user, it's very likely that the user will try to return to Zulip. As a result, it makes sense to optimistically soft-reactivate that user, to give them a good return experience. - It's important that we do nothing for stream wildcard or group mentions, - because soft-reactivating an entire realm would be very expensive - (and we can't easily check the group's size). The caller is - responsible for passing a mentioned_user_group_name that is None - for messages that contain both a personal mention and a group - mention. + It's important that we do nothing for stream wildcard or large + group mentions (size > 'settings.MAX_GROUP_SIZE_FOR_MENTION_REACTIVATION'), + because soft-reactivating an entire realm or a large group would be + very expensive. The caller is responsible for passing a + mentioned_user_group_members_count that is None for messages that + contain both a personal mention and a group mention. """ if not user_profile.long_term_idle: return direct_message = NotificationTriggers.DIRECT_MESSAGE in unique_triggers - personal_mention = ( - NotificationTriggers.MENTION in unique_triggers and mentioned_user_group_name is None - ) + + personal_mention = False + small_group_mention = False + if NotificationTriggers.MENTION in unique_triggers: + if mentioned_user_group_members_count is None: + personal_mention = True + elif mentioned_user_group_members_count <= settings.MAX_GROUP_SIZE_FOR_MENTION_REACTIVATION: + small_group_mention = True + topic_wildcard_mention = any( trigger in unique_triggers for trigger in [ @@ -428,7 +436,12 @@ def soft_reactivate_if_personal_notification( NotificationTriggers.TOPIC_WILDCARD_MENTION_IN_FOLLOWED_TOPIC, ] ) - if not direct_message and not personal_mention and not topic_wildcard_mention: + if ( + not direct_message + and not personal_mention + and not small_group_mention + and not topic_wildcard_mention + ): return queue_soft_reactivation(user_profile.id) diff --git a/zerver/tests/test_message_notification_emails.py b/zerver/tests/test_message_notification_emails.py index c2467d073f..976f5df7e5 100644 --- a/zerver/tests/test_message_notification_emails.py +++ b/zerver/tests/test_message_notification_emails.py @@ -14,7 +14,7 @@ from django.test import override_settings from django_stubs_ext import StrPromise from zerver.actions.create_user import do_create_user -from zerver.actions.user_groups import check_add_user_group +from zerver.actions.user_groups import add_subgroups_to_user_group, check_add_user_group from zerver.actions.user_settings import do_change_user_setting from zerver.actions.user_topics import do_set_user_topic_visibility_policy from zerver.lib.email_notifications import ( @@ -1615,14 +1615,27 @@ class TestMessageNotificationEmails(ZulipTestCase): email_subject = "DMs with Othello, the Moor of Venice" self._test_cases(msg_id, verify_body_include, email_subject, verify_html_body=True) + @override_settings(MAX_GROUP_SIZE_FOR_MENTION_REACTIVATION=2) def test_long_term_idle_user_missed_message(self) -> None: hamlet = self.example_user("hamlet") othello = self.example_user("othello") cordelia = self.example_user("cordelia") - large_user_group = check_add_user_group( - get_realm("zulip"), "large_user_group", [hamlet, othello, cordelia], acting_user=None + zulip_realm = get_realm("zulip") + + # user groups having upto 'MAX_GROUP_SIZE_FOR_MENTION_REACTIVATION' + # members are small user groups. + small_user_group = check_add_user_group( + zulip_realm, "small_user_group", [hamlet, othello], acting_user=None ) + large_user_group = check_add_user_group( + zulip_realm, "large_user_group", [hamlet], acting_user=None + ) + subgroup = check_add_user_group( + zulip_realm, "subgroup", [othello, cordelia], acting_user=None + ) + add_subgroups_to_user_group(large_user_group, [subgroup], acting_user=None) + def reset_hamlet_as_soft_deactivated_user() -> None: nonlocal hamlet hamlet = self.example_user("hamlet") @@ -1727,8 +1740,25 @@ class TestMessageNotificationEmails(ZulipTestCase): reset_hamlet_as_soft_deactivated_user() self.expect_to_stay_long_term_idle(hamlet, send_stream_wildcard_mention) - # Group mention should NOT soft reactivate the user - def send_group_mention() -> None: + # Small group mention should soft reactivate the user + def send_small_group_mention() -> None: + mention = "@*small_user_group*" + stream_mentioned_message_id = self.send_stream_message(othello, "Denmark", mention) + handle_missedmessage_emails( + hamlet.id, + { + stream_mentioned_message_id: MissedMessageData( + trigger=NotificationTriggers.MENTION, + mentioned_user_group_id=small_user_group.id, + ), + }, + ) + + reset_hamlet_as_soft_deactivated_user() + self.expect_soft_reactivation(hamlet, send_small_group_mention) + + # Large group mention should NOT soft reactivate the user + def send_large_group_mention() -> None: mention = "@*large_user_group*" stream_mentioned_message_id = self.send_stream_message(othello, "Denmark", mention) handle_missedmessage_emails( @@ -1742,7 +1772,7 @@ class TestMessageNotificationEmails(ZulipTestCase): ) reset_hamlet_as_soft_deactivated_user() - self.expect_to_stay_long_term_idle(hamlet, send_group_mention) + self.expect_to_stay_long_term_idle(hamlet, send_large_group_mention) def test_followed_topic_missed_message(self) -> None: hamlet = self.example_user("hamlet") diff --git a/zerver/tests/test_push_notifications.py b/zerver/tests/test_push_notifications.py index 2ed0b73e0f..326c321b9a 100644 --- a/zerver/tests/test_push_notifications.py +++ b/zerver/tests/test_push_notifications.py @@ -36,7 +36,7 @@ from zerver.actions.realm_settings import ( do_deactivate_realm, do_set_realm_authentication_methods, ) -from zerver.actions.user_groups import check_add_user_group +from zerver.actions.user_groups import add_subgroups_to_user_group, check_add_user_group from zerver.actions.user_settings import do_change_user_setting, do_regenerate_api_key from zerver.actions.user_topics import do_set_user_topic_visibility_policy from zerver.lib import redis_utils @@ -3563,19 +3563,32 @@ class HandlePushNotificationTest(PushNotificationTest): mock_send_android.assert_called_with(user_identity, android_devices, {"gcm": True}, {}) mock_push_notifications.assert_called_once() + @override_settings(MAX_GROUP_SIZE_FOR_MENTION_REACTIVATION=2) @mock.patch("zerver.lib.push_notifications.push_notifications_configured", return_value=True) def test_user_push_soft_reactivate_soft_deactivated_user( self, mock_push_notifications: mock.MagicMock ) -> None: othello = self.example_user("othello") cordelia = self.example_user("cordelia") - large_user_group = check_add_user_group( - get_realm("zulip"), - "large_user_group", - [self.user_profile, othello, cordelia], + zulip_realm = get_realm("zulip") + + # user groups having upto 'MAX_GROUP_SIZE_FOR_MENTION_REACTIVATION' + # members are small user groups. + small_user_group = check_add_user_group( + zulip_realm, + "small_user_group", + [self.user_profile, othello], acting_user=None, ) + large_user_group = check_add_user_group( + zulip_realm, "large_user_group", [self.user_profile], acting_user=None + ) + subgroup = check_add_user_group( + zulip_realm, "subgroup", [othello, cordelia], acting_user=None + ) + add_subgroups_to_user_group(large_user_group, [subgroup], acting_user=None) + # Personal mention in a stream message should soft reactivate the user def mention_in_stream() -> None: mention = f"@**{self.user_profile.full_name}**" @@ -3670,8 +3683,24 @@ class HandlePushNotificationTest(PushNotificationTest): self.soft_deactivate_main_user() self.expect_to_stay_long_term_idle(self.user_profile, send_stream_wildcard_mention) - # Group mention should NOT soft reactivate the user - def send_group_mention() -> None: + # Small group mention should soft reactivate the user + def send_small_group_mention() -> None: + mention = "@*small_user_group*" + stream_mentioned_message_id = self.send_stream_message(othello, "Denmark", mention) + handle_push_notification( + self.user_profile.id, + { + "message_id": stream_mentioned_message_id, + "trigger": NotificationTriggers.MENTION, + "mentioned_user_group_id": small_user_group.id, + }, + ) + + self.soft_deactivate_main_user() + self.expect_soft_reactivation(self.user_profile, send_small_group_mention) + + # Large group mention should NOT soft reactivate the user + def send_large_group_mention() -> None: mention = "@*large_user_group*" stream_mentioned_message_id = self.send_stream_message(othello, "Denmark", mention) handle_push_notification( @@ -3684,7 +3713,7 @@ class HandlePushNotificationTest(PushNotificationTest): ) self.soft_deactivate_main_user() - self.expect_to_stay_long_term_idle(self.user_profile, send_group_mention) + self.expect_to_stay_long_term_idle(self.user_profile, send_large_group_mention) @mock.patch("zerver.lib.push_notifications.logger.info") @mock.patch("zerver.lib.push_notifications.push_notifications_configured", return_value=True) diff --git a/zproject/default_settings.py b/zproject/default_settings.py index f75ef9dd10..8df9be7bb2 100644 --- a/zproject/default_settings.py +++ b/zproject/default_settings.py @@ -621,6 +621,10 @@ TYPING_STARTED_WAIT_PERIOD_MILLISECONDS = 30000 # load in large organizations. MAX_STREAM_SIZE_FOR_TYPING_NOTIFICATIONS = 100 +# The maximum user-group size value upto which members should +# be soft-reactivated in the case of user group mention. +MAX_GROUP_SIZE_FOR_MENTION_REACTIVATION = 11 + # Limiting guest access to other users via the # can_access_all_users_group setting makes presence queries much more # expensive. This can be a significant performance problem for