events: Send `delete_message` event to user who deleted the message.

Fixes #29826.

Co-authored-by: Mukul Goyal <goyal.mukul7689@gmail.com>
Co-authored-by: Aman Agrawal <amanagr@zulip.com>

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`

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

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)

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
 
 

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.
 

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()
 

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:

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.

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)

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)

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.

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)

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)

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})