From 7a80fcf042cb18d2d8dd86ffb82f0c7eff59ad61 Mon Sep 17 00:00:00 2001 From: Vector73 Date: Wed, 17 Jul 2024 08:41:28 +0530 Subject: [PATCH] events: Send `delete_message` event to user who deleted the message. Fixes #29826. Co-authored-by: Mukul Goyal Co-authored-by: Aman Agrawal --- api_docs/changelog.md | 6 ++++++ version.py | 2 +- zerver/actions/message_delete.py | 14 +++++++++---- zerver/actions/message_edit.py | 8 +++++--- zerver/openapi/zulip.yaml | 16 +++++++++++++-- .../test_delete_unclaimed_attachments.py | 20 +++++++++++++------ zerver/tests/test_events.py | 14 ++++++------- zerver/tests/test_link_embed.py | 2 +- zerver/tests/test_message_delete.py | 8 ++++---- zerver/tests/test_message_move_topic.py | 2 +- zerver/tests/test_push_notifications.py | 2 +- zerver/tests/test_retention.py | 2 +- zerver/views/message_edit.py | 2 +- zerver/views/streams.py | 2 +- 14 files changed, 67 insertions(+), 33 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 07313941e5..3cdf94ce14 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,12 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 9.0 +**Feature level 274** + +* [`GET /events`](/api/get-events): `delete_message` events are now + always sent to the user who deletes the message, even if the message + was in a channel that the user was not subscribed to. + **Feature level 273** * [`POST /register`](/api/register-queue): Added `server_thumbnail_formats` diff --git a/version.py b/version.py index b6cf0e4ce1..2a34fe9d83 100644 --- a/version.py +++ b/version.py @@ -34,7 +34,7 @@ DESKTOP_WARNING_VERSION = "5.9.3" # new level means in api_docs/changelog.md, as well as "**Changes**" # entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 273 # Last bumped for server_thumbnail_formats +API_FEATURE_LEVEL = 274 # Last bumped for `delete_message` event. # Bump the minor PROVISION_VERSION to indicate that folks should provision diff --git a/zerver/actions/message_delete.py b/zerver/actions/message_delete.py index 7ccc754a27..a9fd12d205 100644 --- a/zerver/actions/message_delete.py +++ b/zerver/actions/message_delete.py @@ -45,7 +45,9 @@ def check_update_first_message_id( send_event_on_commit(realm, stream_event, users_to_notify) -def do_delete_messages(realm: Realm, messages: Iterable[Message]) -> None: +def do_delete_messages( + realm: Realm, messages: Iterable[Message], *, acting_user: UserProfile | None +) -> None: # messages in delete_message event belong to the same topic # or is a single direct message, as any other behaviour is not possible with # the current callers to this method. @@ -61,12 +63,12 @@ def do_delete_messages(realm: Realm, messages: Iterable[Message]) -> None: sample_message = messages[0] message_type = "stream" - users_to_notify = [] + users_to_notify = set() if not sample_message.is_stream_message(): assert len(messages) == 1 message_type = "private" ums = UserMessage.objects.filter(message_id__in=message_ids) - users_to_notify = [um.user_profile_id for um in ums] + users_to_notify = set(ums.values_list("user_profile_id", flat=True)) archiving_chunk_size = retention.MESSAGE_BATCH_SIZE if message_type == "stream": @@ -78,9 +80,13 @@ def do_delete_messages(realm: Realm, messages: Iterable[Message]) -> None: ) # We exclude long-term idle users, since they by definition have no active clients. subscriptions = subscriptions.exclude(user_profile__long_term_idle=True) - users_to_notify = list(subscriptions.values_list("user_profile_id", flat=True)) + users_to_notify = set(subscriptions.values_list("user_profile_id", flat=True)) archiving_chunk_size = retention.STREAM_MESSAGE_BATCH_SIZE + if acting_user is not None: + # Always send event to the user who deleted the message. + users_to_notify.add(acting_user.id) + move_messages_to_archive(message_ids, realm=realm, chunk_size=archiving_chunk_size) if message_type == "stream": stream = Stream.objects.get(id=sample_message.recipient.type_id) diff --git a/zerver/actions/message_edit.py b/zerver/actions/message_edit.py index 4002372680..3597e33ce6 100644 --- a/zerver/actions/message_edit.py +++ b/zerver/actions/message_edit.py @@ -187,7 +187,7 @@ def maybe_send_resolve_topic_notifications( # For that reason, we apply a short grace period during which # such an undo action will just delete the previous notification # message instead. - if maybe_delete_previous_resolve_topic_notification(stream, new_topic_name): + if maybe_delete_previous_resolve_topic_notification(user_profile, stream, new_topic_name): return None, True # Compute the users who either sent or reacted to messages that @@ -222,7 +222,9 @@ def maybe_send_resolve_topic_notifications( return resolved_topic_message_id, False -def maybe_delete_previous_resolve_topic_notification(stream: Stream, topic: str) -> bool: +def maybe_delete_previous_resolve_topic_notification( + user_profile: UserProfile, stream: Stream, topic: str +) -> bool: assert stream.recipient_id is not None last_message = messages_for_topic(stream.realm_id, stream.recipient_id, topic).last() @@ -238,7 +240,7 @@ def maybe_delete_previous_resolve_topic_notification(stream: Stream, topic: str) if time_difference > settings.RESOLVE_TOPIC_UNDO_GRACE_PERIOD_SECONDS: return False - do_delete_messages(stream.realm, [last_message]) + do_delete_messages(stream.realm, [last_message], acting_user=user_profile) return True diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 1dba7b7364..9522454bf8 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -2071,13 +2071,25 @@ paths: additionalProperties: false description: | Event sent when a message has been deleted. - Sent to all users who received the message. + + Sent to all users who currently are subscribed to the + messages' recipient. May also be sent to additional users + who had access to it, including, in particular, an + administrator user deleting messages in a stream that they + are not subscribed to. + + This means that clients can assume that they will always + receive an event of this type for deletions that the + client itself initiated. This event is also sent when the user loses access to a message, such as when it is [moved to a channel][message-move-channel] that the user does not [have permission to access][channel-access]. - **Changes**: Before Zulip 5.0 (feature level 77), events + **Changes**: Before Zulip 9.0 (feature level 274), this + event was only sent to subscribers of the message's recipient. + + Before Zulip 5.0 (feature level 77), events for direct messages contained additional `sender_id` and `recipient_id` fields. diff --git a/zerver/tests/test_delete_unclaimed_attachments.py b/zerver/tests/test_delete_unclaimed_attachments.py index c23eb0c27b..31f6fc15ee 100644 --- a/zerver/tests/test_delete_unclaimed_attachments.py +++ b/zerver/tests/test_delete_unclaimed_attachments.py @@ -151,7 +151,7 @@ class UnclaimedAttachmentTest(UploadSerializeMixin, ZulipTestCase): message_id = self.send_stream_message(hamlet, "Denmark", body, "test") # Delete that message; this moves it to ArchivedAttachment but leaves the file on disk - do_delete_messages(hamlet.realm, [Message.objects.get(id=message_id)]) + do_delete_messages(hamlet.realm, [Message.objects.get(id=message_id)], acting_user=None) self.assert_exists( attachment, has_file=True, has_attachment=False, has_archived_attachment=True ) @@ -190,7 +190,9 @@ class UnclaimedAttachmentTest(UploadSerializeMixin, ZulipTestCase): # Delete the second message; this leaves an Attachment and an # ArchivedAttachment, both associated with a message - do_delete_messages(hamlet.realm, [Message.objects.get(id=first_message_id)]) + do_delete_messages( + hamlet.realm, [Message.objects.get(id=first_message_id)], acting_user=None + ) self.assert_exists( attachment, has_file=True, has_attachment=True, has_archived_attachment=True ) @@ -220,7 +222,9 @@ class UnclaimedAttachmentTest(UploadSerializeMixin, ZulipTestCase): ) # Deleting the other message now leaves just an ArchivedAttachment - do_delete_messages(hamlet.realm, [Message.objects.get(id=second_message_id)]) + do_delete_messages( + hamlet.realm, [Message.objects.get(id=second_message_id)], acting_user=None + ) self.assert_exists( attachment, has_file=True, has_attachment=False, has_archived_attachment=True ) @@ -298,7 +302,9 @@ class UnclaimedAttachmentTest(UploadSerializeMixin, ZulipTestCase): # Deleting the sent message leaves us with an Attachment # attached to the scheduled message, and an archived # attachment with an archived message - do_delete_messages(hamlet.realm, [Message.objects.get(id=sent_message_id)]) + do_delete_messages( + hamlet.realm, [Message.objects.get(id=sent_message_id)], acting_user=None + ) self.assert_exists( attachment, has_file=True, has_attachment=True, has_archived_attachment=True ) @@ -361,7 +367,9 @@ class UnclaimedAttachmentTest(UploadSerializeMixin, ZulipTestCase): # Delete the message and then unschedule the scheduled message # before expiring the ArchivedMessages. - do_delete_messages(hamlet.realm, [Message.objects.get(id=sent_message_id)]) + do_delete_messages( + hamlet.realm, [Message.objects.get(id=sent_message_id)], acting_user=None + ) delete_scheduled_message(hamlet, scheduled_message_id) self.assert_exists( attachment, has_file=True, has_attachment=True, has_archived_attachment=True @@ -425,7 +433,7 @@ class UnclaimedAttachmentTest(UploadSerializeMixin, ZulipTestCase): message_id = self.send_stream_message(hamlet, "Denmark", body, "test") # Delete and purge the message, leaving both the ArchivedAttachments dangling - do_delete_messages(hamlet.realm, [Message.objects.get(id=message_id)]) + do_delete_messages(hamlet.realm, [Message.objects.get(id=message_id)], acting_user=None) with self.settings(ARCHIVED_DATA_VACUUMING_DELAY_DAYS=0): clean_archived_data() diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index eb06698de5..947eb7c463 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -3117,7 +3117,7 @@ class NormalActionsTest(BaseAction): msg_id_2 = self.send_stream_message(hamlet, "Verona") messages = [Message.objects.get(id=msg_id), Message.objects.get(id=msg_id_2)] with self.verify_action(state_change_expected=True) as events: - do_delete_messages(self.user_profile.realm, messages) + do_delete_messages(self.user_profile.realm, messages, acting_user=None) check_delete_message( "events[0]", events[0], @@ -3138,7 +3138,7 @@ class NormalActionsTest(BaseAction): with self.verify_action( state_change_expected=True, bulk_message_deletion=False, num_events=2 ) as events: - do_delete_messages(self.user_profile.realm, messages) + do_delete_messages(self.user_profile.realm, messages, acting_user=None) check_delete_message( "events[0]", events[0], @@ -3154,7 +3154,7 @@ class NormalActionsTest(BaseAction): msg_id_2 = self.send_stream_message(hamlet, "test_stream1") message = Message.objects.get(id=msg_id) with self.verify_action(state_change_expected=True, num_events=2) as events: - do_delete_messages(self.user_profile.realm, [message]) + do_delete_messages(self.user_profile.realm, [message], acting_user=None) check_stream_update("events[0]", events[0]) self.assertEqual(events[0]["property"], "first_message_id") @@ -3176,7 +3176,7 @@ class NormalActionsTest(BaseAction): ) message = Message.objects.get(id=msg_id) with self.verify_action(state_change_expected=True) as events: - do_delete_messages(self.user_profile.realm, [message]) + do_delete_messages(self.user_profile.realm, [message], acting_user=None) check_delete_message( "events[0]", events[0], @@ -3193,7 +3193,7 @@ class NormalActionsTest(BaseAction): ) message = Message.objects.get(id=msg_id) with self.verify_action(state_change_expected=True, bulk_message_deletion=False) as events: - do_delete_messages(self.user_profile.realm, [message]) + do_delete_messages(self.user_profile.realm, [message], acting_user=None) check_delete_message( "events[0]", events[0], @@ -3210,13 +3210,13 @@ class NormalActionsTest(BaseAction): msg_id = self.send_stream_message(user_profile, "Verona") message = Message.objects.get(id=msg_id) with self.verify_action(state_change_expected=True): - do_delete_messages(self.user_profile.realm, [message]) + do_delete_messages(self.user_profile.realm, [message], acting_user=None) result = fetch_initial_state_data(user_profile, realm=user_profile.realm) self.assertEqual(result["max_message_id"], -1) def test_do_delete_message_with_no_messages(self) -> None: with self.verify_action(num_events=0, state_change_expected=False) as events: - do_delete_messages(self.user_profile.realm, []) + do_delete_messages(self.user_profile.realm, [], acting_user=None) self.assertEqual(events, []) def test_add_attachment(self) -> None: diff --git a/zerver/tests/test_link_embed.py b/zerver/tests/test_link_embed.py index 2a472882ac..9045f6a83b 100644 --- a/zerver/tests/test_link_embed.py +++ b/zerver/tests/test_link_embed.py @@ -496,7 +496,7 @@ class PreviewTestCase(ZulipTestCase): event = patched.call_args[0][1] msg = Message.objects.select_related("sender").get(id=msg_id) - do_delete_messages(msg.realm, [msg]) + do_delete_messages(msg.realm, [msg], acting_user=None) # We do still fetch the URL, as we don't want to incur the # cost of locking the row while we do the HTTP fetches. diff --git a/zerver/tests/test_message_delete.py b/zerver/tests/test_message_delete.py index 1a1fdd55e5..3343b8ba8d 100644 --- a/zerver/tests/test_message_delete.py +++ b/zerver/tests/test_message_delete.py @@ -308,7 +308,7 @@ class DeleteMessageTest(ZulipTestCase): m.side_effect = AssertionError( "Events should be sent only after the transaction commits." ) - do_delete_messages(hamlet.realm, [message]) + do_delete_messages(hamlet.realm, [message], acting_user=None) def test_delete_message_in_unsubscribed_private_stream(self) -> None: hamlet = self.example_user("hamlet") @@ -347,17 +347,17 @@ class DeleteMessageTest(ZulipTestCase): first_message_id = message_ids[0] message = Message.objects.get(id=message_ids[3]) - do_delete_messages(realm, [message]) + do_delete_messages(realm, [message], acting_user=None) stream = get_stream(stream_name, realm) self.assertEqual(stream.first_message_id, first_message_id) first_message = Message.objects.get(id=first_message_id) - do_delete_messages(realm, [first_message]) + do_delete_messages(realm, [first_message], acting_user=None) stream = get_stream(stream_name, realm) self.assertEqual(stream.first_message_id, message_ids[1]) all_messages = Message.objects.filter(id__in=message_ids) with self.assert_database_query_count(24): - do_delete_messages(realm, all_messages) + do_delete_messages(realm, all_messages, acting_user=None) stream = get_stream(stream_name, realm) self.assertEqual(stream.first_message_id, None) diff --git a/zerver/tests/test_message_move_topic.py b/zerver/tests/test_message_move_topic.py index 2829821f66..d334c4a0f5 100644 --- a/zerver/tests/test_message_move_topic.py +++ b/zerver/tests/test_message_move_topic.py @@ -1733,7 +1733,7 @@ class MessageMoveTopicTest(ZulipTestCase): ) message = Message.objects.get(id=message_id) - do_delete_messages(admin_user.realm, [message]) + do_delete_messages(admin_user.realm, [message], acting_user=None) assert stream.recipient_id is not None changed_messages = messages_for_topic(stream.realm_id, stream.recipient_id, original_topic) diff --git a/zerver/tests/test_push_notifications.py b/zerver/tests/test_push_notifications.py index 63836f576c..76ea61f8b5 100644 --- a/zerver/tests/test_push_notifications.py +++ b/zerver/tests/test_push_notifications.py @@ -3263,7 +3263,7 @@ class HandlePushNotificationTest(PushNotificationTest): "trigger": NotificationTriggers.DIRECT_MESSAGE, } # Now, delete the message the normal way - do_delete_messages(user_profile.realm, [message]) + do_delete_messages(user_profile.realm, [message], acting_user=None) # This mock.patch() should be assertNoLogs once that feature # is added to Python. diff --git a/zerver/tests/test_retention.py b/zerver/tests/test_retention.py index 0c5928767a..cc19bb6b44 100644 --- a/zerver/tests/test_retention.py +++ b/zerver/tests/test_retention.py @@ -1146,7 +1146,7 @@ class TestDoDeleteMessages(ZulipTestCase): messages = Message.objects.filter(id__in=message_ids) with self.assert_database_query_count(22): - do_delete_messages(realm, messages) + do_delete_messages(realm, messages, acting_user=None) self.assertFalse(Message.objects.filter(id__in=message_ids).exists()) archived_messages = ArchivedMessage.objects.filter(id__in=message_ids) diff --git a/zerver/views/message_edit.py b/zerver/views/message_edit.py index e300d04d56..2e70b3a369 100644 --- a/zerver/views/message_edit.py +++ b/zerver/views/message_edit.py @@ -183,7 +183,7 @@ def delete_message_backend( message = access_message(user_profile, message_id, lock_message=True) validate_can_delete_message(user_profile, message) try: - do_delete_messages(user_profile.realm, [message]) + do_delete_messages(user_profile.realm, [message], acting_user=user_profile) except (Message.DoesNotExist, IntegrityError): raise JsonableError(_("Message already deleted")) return json_success(request) diff --git a/zerver/views/streams.py b/zerver/views/streams.py index 2deb5c5233..609bbd6f06 100644 --- a/zerver/views/streams.py +++ b/zerver/views/streams.py @@ -938,7 +938,7 @@ def delete_in_topic( ) if not messages_to_delete: break - do_delete_messages(user_profile.realm, messages_to_delete) + do_delete_messages(user_profile.realm, messages_to_delete, acting_user=user_profile) return json_success(request, data={"complete": True})