mirror of
https://github.com/zulip/zulip.git
synced 2025-11-09 08:26:11 +00:00
push_notifications: Clear push notifications on message deletion.
This commit adds support to revoke mobile push notifications for messages when deleted. Fixes #26584. Signed-off-by: Prakhar Pratyush <prakhar@zulip.com>
This commit is contained in:
committed by
Tim Abbott
parent
8b11e88a91
commit
7260ba689f
@@ -2,6 +2,7 @@ from collections import defaultdict
|
|||||||
from collections.abc import Iterable
|
from collections.abc import Iterable
|
||||||
from typing import TypedDict
|
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 import retention
|
||||||
from zerver.lib.message import event_recipient_ids_for_action_on_messages
|
from zerver.lib.message import event_recipient_ids_for_action_on_messages
|
||||||
from zerver.lib.retention import move_messages_to_archive
|
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
|
When the Recipient.PERSONAL is no longer a case to consider, this
|
||||||
restriction can be deleted.
|
restriction can be deleted.
|
||||||
"""
|
"""
|
||||||
|
message_ids = []
|
||||||
private_messages_by_recipient: defaultdict[int, list[Message]] = defaultdict(list)
|
private_messages_by_recipient: defaultdict[int, list[Message]] = defaultdict(list)
|
||||||
stream_messages_by_recipient_and_topic: defaultdict[tuple[int, str], list[Message]] = (
|
stream_messages_by_recipient_and_topic: defaultdict[tuple[int, str], list[Message]] = (
|
||||||
defaultdict(list)
|
defaultdict(list)
|
||||||
)
|
)
|
||||||
stream_by_recipient_id = {}
|
stream_by_recipient_id = {}
|
||||||
for message in messages:
|
for message in messages:
|
||||||
|
message_ids.append(message.id)
|
||||||
if message.is_channel_message:
|
if message.is_channel_message:
|
||||||
recipient_id = message.recipient_id
|
recipient_id = message.recipient_id
|
||||||
# topics are case-insensitive.
|
# topics are case-insensitive.
|
||||||
@@ -120,6 +123,8 @@ def do_delete_messages(
|
|||||||
recipient_id = message.recipient.id
|
recipient_id = message.recipient.id
|
||||||
private_messages_by_recipient[recipient_id].append(message)
|
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()):
|
for recipient_id, grouped_messages in sorted(private_messages_by_recipient.items()):
|
||||||
_process_grouped_messages_deletion(
|
_process_grouped_messages_deletion(
|
||||||
realm, grouped_messages, stream=None, topic=None, acting_user=acting_user
|
realm, grouped_messages, stream=None, topic=None, acting_user=acting_user
|
||||||
|
|||||||
@@ -220,27 +220,46 @@ def do_update_mobile_push_notification(
|
|||||||
|
|
||||||
|
|
||||||
def do_clear_mobile_push_notifications_for_ids(
|
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:
|
) -> None:
|
||||||
if len(message_ids) == 0:
|
if len(message_ids) == 0:
|
||||||
return
|
return
|
||||||
|
|
||||||
# This function supports clearing notifications for several users
|
if user_profile_ids is not None:
|
||||||
# only for the message-edit use case where we'll have a single message_id.
|
# This block gets executed in the following cases:
|
||||||
assert len(user_profile_ids) == 1 or len(message_ids) == 1
|
# * 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)
|
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:
|
for user_id, message_id in notifications_to_update:
|
||||||
messages_by_user[user_id].append(message_id)
|
messages_by_user[user_id].append(message_id)
|
||||||
|
|
||||||
|
|||||||
@@ -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"),
|
||||||
|
]
|
||||||
@@ -642,6 +642,13 @@ class UserMessage(AbstractUserMessage):
|
|||||||
),
|
),
|
||||||
name="zerver_usermessage_active_mobile_push_notification_id",
|
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
|
@override
|
||||||
|
|||||||
@@ -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.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.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_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.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.groups import SystemGroups
|
||||||
from zerver.models.realms import get_realm
|
from zerver.models.realms import get_realm
|
||||||
from zerver.models.streams import get_stream
|
from zerver.models.streams import get_stream
|
||||||
@@ -914,7 +915,62 @@ class DeleteMessageTest(ZulipTestCase):
|
|||||||
self.assertEqual(stream.first_message_id, message_ids[1])
|
self.assertEqual(stream.first_message_id, message_ids[1])
|
||||||
|
|
||||||
all_messages = Message.objects.filter(id__in=message_ids)
|
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)
|
do_delete_messages(realm, all_messages, acting_user=None)
|
||||||
stream = get_stream(stream_name, realm)
|
stream = get_stream(stream_name, realm)
|
||||||
self.assertEqual(stream.first_message_id, None)
|
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),
|
||||||
|
[],
|
||||||
|
)
|
||||||
|
|||||||
@@ -1151,7 +1151,7 @@ class TestDoDeleteMessages(ZulipTestCase):
|
|||||||
message_ids = [self.send_stream_message(cordelia, "Verona", str(i)) for i in range(10)]
|
message_ids = [self.send_stream_message(cordelia, "Verona", str(i)) for i in range(10)]
|
||||||
messages = Message.objects.filter(id__in=message_ids)
|
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)
|
do_delete_messages(realm, messages, acting_user=None)
|
||||||
self.assertFalse(Message.objects.filter(id__in=message_ids).exists())
|
self.assertFalse(Message.objects.filter(id__in=message_ids).exists())
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user