diff --git a/zerver/actions/message_delete.py b/zerver/actions/message_delete.py index 0e51cf05f2..c7f224f9a4 100644 --- a/zerver/actions/message_delete.py +++ b/zerver/actions/message_delete.py @@ -2,6 +2,7 @@ from collections import defaultdict from collections.abc import Iterable from typing import TypedDict +from zerver.actions.message_flags import do_clear_mobile_push_notifications_for_ids from zerver.lib import retention from zerver.lib.message import event_recipient_ids_for_action_on_messages from zerver.lib.retention import move_messages_to_archive @@ -105,12 +106,14 @@ def do_delete_messages( When the Recipient.PERSONAL is no longer a case to consider, this restriction can be deleted. """ + message_ids = [] private_messages_by_recipient: defaultdict[int, list[Message]] = defaultdict(list) stream_messages_by_recipient_and_topic: defaultdict[tuple[int, str], list[Message]] = ( defaultdict(list) ) stream_by_recipient_id = {} for message in messages: + message_ids.append(message.id) if message.is_channel_message: recipient_id = message.recipient_id # topics are case-insensitive. @@ -120,6 +123,8 @@ def do_delete_messages( recipient_id = message.recipient.id private_messages_by_recipient[recipient_id].append(message) + do_clear_mobile_push_notifications_for_ids(user_profile_ids=None, message_ids=message_ids) + for recipient_id, grouped_messages in sorted(private_messages_by_recipient.items()): _process_grouped_messages_deletion( realm, grouped_messages, stream=None, topic=None, acting_user=acting_user diff --git a/zerver/actions/message_flags.py b/zerver/actions/message_flags.py index 962091a2db..4a9f5f9848 100644 --- a/zerver/actions/message_flags.py +++ b/zerver/actions/message_flags.py @@ -220,27 +220,46 @@ def do_update_mobile_push_notification( def do_clear_mobile_push_notifications_for_ids( - user_profile_ids: list[int], message_ids: list[int] + user_profile_ids: list[int] | None, message_ids: list[int] ) -> None: if len(message_ids) == 0: return - # This function supports clearing notifications for several users - # only for the message-edit use case where we'll have a single message_id. - assert len(user_profile_ids) == 1 or len(message_ids) == 1 + if user_profile_ids is not None: + # This block gets executed in the following cases: + # * Message(s) marked as read by a user + # * A message edited to remove mention(s) + if len(user_profile_ids) == 0: + return + + # This supports clearing notifications for several users only for + # the message-edit use case where we'll have a single message_id. + assert len(user_profile_ids) == 1 or len(message_ids) == 1 + + notifications_to_update = ( + UserMessage.objects.filter( + message_id__in=message_ids, + user_profile_id__in=user_profile_ids, + ) + .extra( # noqa: S610 + where=[UserMessage.where_active_push_notification()], + ) + .values_list("user_profile_id", "message_id") + ) + else: + # This block handles clearing notifications when message(s) get deleted. + notifications_to_update = ( + # Uses index: zerver_usermessage_message_active_mobile_push_notification_idx + UserMessage.objects.filter( + message_id__in=message_ids, + ) + .extra( # noqa: S610 + where=[UserMessage.where_active_push_notification()], + ) + .values_list("user_profile_id", "message_id") + ) messages_by_user = defaultdict(list) - notifications_to_update = ( - UserMessage.objects.filter( - message_id__in=message_ids, - user_profile_id__in=user_profile_ids, - ) - .extra( # noqa: S610 - where=[UserMessage.where_active_push_notification()], - ) - .values_list("user_profile_id", "message_id") - ) - for user_id, message_id in notifications_to_update: messages_by_user[user_id].append(message_id) diff --git a/zerver/migrations/0755_usermessage_zerver_usermessage_message_active_mobile_push_notification_idx.py b/zerver/migrations/0755_usermessage_zerver_usermessage_message_active_mobile_push_notification_idx.py new file mode 100644 index 0000000000..86cda0d651 --- /dev/null +++ b/zerver/migrations/0755_usermessage_zerver_usermessage_message_active_mobile_push_notification_idx.py @@ -0,0 +1,23 @@ +# Generated by Django 5.2.7 on 2025-10-29 05:28 + +from django.contrib.postgres.operations import AddIndexConcurrently +from django.db import migrations, models + + +class Migration(migrations.Migration): + atomic = False + dependencies = [ + ("zerver", "0754_merge_20251014_1855"), + ] + + operations = [ + AddIndexConcurrently( + model_name="usermessage", + index=models.Index( + models.F("message"), + condition=models.Q(("flags__andnz", 4096)), + name="zerver_usermessage_message_active_mobile_push_notification_idx", + ), + ), + migrations.RunSQL("ANALYZE zerver_usermessage"), + ] diff --git a/zerver/models/messages.py b/zerver/models/messages.py index 121df7a765..51e9c6215c 100644 --- a/zerver/models/messages.py +++ b/zerver/models/messages.py @@ -642,6 +642,13 @@ class UserMessage(AbstractUserMessage): ), name="zerver_usermessage_active_mobile_push_notification_id", ), + models.Index( + "message", + condition=Q( + flags__andnz=AbstractUserMessage.flags.active_mobile_push_notification.mask + ), + name="zerver_usermessage_message_active_mobile_push_notification_idx", + ), ] @override diff --git a/zerver/tests/test_message_delete.py b/zerver/tests/test_message_delete.py index 73ef505151..6da9e7d91d 100644 --- a/zerver/tests/test_message_delete.py +++ b/zerver/tests/test_message_delete.py @@ -10,8 +10,9 @@ from zerver.actions.message_delete import do_delete_messages from zerver.actions.realm_settings import do_change_realm_permission_group_setting from zerver.actions.streams import do_change_stream_group_based_setting, do_deactivate_stream from zerver.actions.user_groups import check_add_user_group +from zerver.actions.user_settings import do_change_user_setting from zerver.lib.test_classes import ZulipTestCase -from zerver.models import Message, NamedUserGroup, UserProfile +from zerver.models import Message, NamedUserGroup, UserMessage, UserProfile from zerver.models.groups import SystemGroups from zerver.models.realms import get_realm from zerver.models.streams import get_stream @@ -914,7 +915,62 @@ class DeleteMessageTest(ZulipTestCase): self.assertEqual(stream.first_message_id, message_ids[1]) all_messages = Message.objects.filter(id__in=message_ids) - with self.assert_database_query_count(25): + with self.assert_database_query_count(26): do_delete_messages(realm, all_messages, acting_user=None) stream = get_stream(stream_name, realm) self.assertEqual(stream.first_message_id, None) + + @mock.patch("zerver.lib.push_notifications.send_push_notifications") + @mock.patch("zerver.lib.push_notifications.push_notifications_configured", return_value=True) + def test_clear_push_notifications_on_message_deletion( + self, + mock_push_notifications: mock.MagicMock, + mock_send_push_notifications: mock.MagicMock, + ) -> None: + realm = get_realm("zulip") + hamlet = self.example_user("hamlet") + cordelia = self.example_user("cordelia") + self.register_push_device(cordelia.id) + do_change_user_setting(cordelia, "enable_stream_push_notifications", True, acting_user=None) + + def get_message_ids_with_active_push_notification(user_profile: UserProfile) -> list[int]: + return list( + UserMessage.objects.filter( + user_profile=user_profile, + ) + .extra( # noqa: S610 + where=[UserMessage.where_active_push_notification()], + ) + .order_by("message_id") + .values_list("message_id", flat=True) + ) + + # Initially, no active push notifications. + self.assertEqual(get_message_ids_with_active_push_notification(cordelia), []) + + self.subscribe(cordelia, "Verona") + message_ids = [self.send_stream_message(hamlet, "Verona")] + + # Verify push notifications sent to `cordelia` for `message_ids` + self.assertEqual( + get_message_ids_with_active_push_notification(cordelia), + message_ids, + ) + + messages = Message.objects.filter(id__in=message_ids) + with ( + mock.patch( + "zerver.worker.missedmessage_mobile_notifications.handle_remove_push_notification" + ) as mock_handle_remove_push_notification, + self.captureOnCommitCallbacks(execute=True), + ): + do_delete_messages(realm, messages, acting_user=None) + + # Verify messages deleted, `handle_remove_push_notification` called + # to clear/revoke push notifications, usermessages deleted. + self.assertFalse(Message.objects.filter(id__in=message_ids).exists()) + mock_handle_remove_push_notification.assert_called_once() + self.assertEqual( + get_message_ids_with_active_push_notification(cordelia), + [], + ) diff --git a/zerver/tests/test_retention.py b/zerver/tests/test_retention.py index 06ecd19cbb..865a2f5ea9 100644 --- a/zerver/tests/test_retention.py +++ b/zerver/tests/test_retention.py @@ -1151,7 +1151,7 @@ class TestDoDeleteMessages(ZulipTestCase): message_ids = [self.send_stream_message(cordelia, "Verona", str(i)) for i in range(10)] messages = Message.objects.filter(id__in=message_ids) - with self.assert_database_query_count(23): + with self.assert_database_query_count(24): do_delete_messages(realm, messages, acting_user=None) self.assertFalse(Message.objects.filter(id__in=message_ids).exists())