From de90d0acdfeb44d6b7e8bcdca532f7a8fe30fb18 Mon Sep 17 00:00:00 2001 From: Vector73 Date: Sat, 27 Apr 2024 20:21:14 +0530 Subject: [PATCH] message_delete: Update "first_message_id" on message deletion. We now "first_message_id" of the stream on the deletion of the first message that was sent to it. This results in 1 extra query when any stream message is deleted and 3 extra queries when the first message sent to any stream is deleted. Fixes #28877. --- api_docs/changelog.md | 5 +++++ version.py | 2 +- zerver/actions/message_delete.py | 33 ++++++++++++++++++++++++++++- zerver/openapi/zulip.yaml | 6 +++++- zerver/tests/test_message_delete.py | 26 +++++++++++++++++++++++ zerver/tests/test_retention.py | 2 +- 6 files changed, 70 insertions(+), 4 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 72d26fd06e..754f1e01e1 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,11 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 9.0 +**Feature level 256** + +* [`GET /events`](/api/get-events): Stream update events with a new + `first_message_id` may now be sent when messages are deleted. + **Feature level 255** * "Stream" was renamed to "Channel" across strings in the Zulip API diff --git a/version.py b/version.py index 3e38faf34d..cc775e758e 100644 --- a/version.py +++ b/version.py @@ -33,7 +33,7 @@ DESKTOP_WARNING_VERSION = "5.9.3" # Changes should be accompanied by documentation explaining what the # new level means in api_docs/changelog.md, as well as "**Changes**" # entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 255 +API_FEATURE_LEVEL = 256 # Bump the minor PROVISION_VERSION to indicate that folks should provision # only when going from an old version of the code to a newer version. Bump diff --git a/zerver/actions/message_delete.py b/zerver/actions/message_delete.py index 6aca536b0a..2281584a5e 100644 --- a/zerver/actions/message_delete.py +++ b/zerver/actions/message_delete.py @@ -3,7 +3,7 @@ from typing import Iterable, List, TypedDict from zerver.lib import retention from zerver.lib.retention import move_messages_to_archive from zerver.lib.stream_subscription import get_active_subscriptions_for_stream_id -from zerver.models import Message, Realm, UserMessage, UserProfile +from zerver.models import Message, Realm, Stream, UserMessage, UserProfile from zerver.tornado.django_api import send_event_on_commit @@ -15,6 +15,34 @@ class DeleteMessagesEvent(TypedDict, total=False): stream_id: int +def check_update_first_message_id( + realm: Realm, stream: Stream, message_ids: List[int], users_to_notify: Iterable[int] +) -> None: + # This will not update the `first_message_id` of streams where the + # first message was deleted prior to the implementation of this function. + assert stream.recipient_id is not None + if stream.first_message_id not in message_ids: + return + current_first_message_id = ( + Message.objects.filter(realm_id=realm.id, recipient_id=stream.recipient_id) + .values_list("id", flat=True) + .order_by("id") + .first() + ) + + stream.first_message_id = current_first_message_id + stream.save(update_fields=["first_message_id"]) + + stream_event = dict( + type="stream", + op="update", + property="first_message_id", + value=stream.first_message_id, + stream_id=stream.id, + ) + send_event_on_commit(realm, stream_event, users_to_notify) + + def do_delete_messages(realm: Realm, messages: Iterable[Message]) -> 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 @@ -52,6 +80,9 @@ def do_delete_messages(realm: Realm, messages: Iterable[Message]) -> None: archiving_chunk_size = retention.STREAM_MESSAGE_BATCH_SIZE 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) + check_update_first_message_id(realm, stream, message_ids, users_to_notify) event["message_type"] = message_type send_event_on_commit(realm, event, users_to_notify) diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index b5bf537a55..9b370b943d 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -18080,7 +18080,11 @@ paths: response. If all messages in the topic were deleted, then the success response will return `"complete": true`. - **Changes**: Before Zulip 8.0 (feature level 211), if the server's + **Changes**: Before Zulip 9.0 (feature level 256), the server never sent + events updating the `first_message_id` for a stream when the oldest + message that had been sent to it changed. + + Before Zulip 8.0 (feature level 211), if the server's processing was interrupted by a timeout, but some messages in the topic were deleted, then it would return `"result": "partially_completed"`, along with a `code` field for an error string, in the success response diff --git a/zerver/tests/test_message_delete.py b/zerver/tests/test_message_delete.py index 3f740ac2ee..7a2f381530 100644 --- a/zerver/tests/test_message_delete.py +++ b/zerver/tests/test_message_delete.py @@ -11,6 +11,7 @@ from zerver.actions.realm_settings import do_set_realm_property from zerver.lib.test_classes import ZulipTestCase from zerver.models import Message, Realm, UserProfile from zerver.models.realms import get_realm +from zerver.models.streams import get_stream if TYPE_CHECKING: from django.test.client import _MonkeyPatchedWSGIResponse as TestHttpResponse @@ -321,3 +322,28 @@ class DeleteMessageTest(ZulipTestCase): result = self.client_delete(f"/json/messages/{msg_id}") self.assert_json_success(result) self.assertFalse(Message.objects.filter(id=msg_id).exists()) + + def test_update_first_message_id_on_stream_message_deletion(self) -> None: + realm = get_realm("zulip") + stream_name = "test" + cordelia = self.example_user("cordelia") + self.make_stream(stream_name) + self.subscribe(cordelia, stream_name) + message_ids = [self.send_stream_message(cordelia, stream_name) for _ in range(5)] + first_message_id = message_ids[0] + + message = Message.objects.get(id=message_ids[3]) + do_delete_messages(realm, [message]) + 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]) + 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(23): + do_delete_messages(realm, all_messages) + stream = get_stream(stream_name, realm) + self.assertEqual(stream.first_message_id, None) diff --git a/zerver/tests/test_retention.py b/zerver/tests/test_retention.py index 2cb90a95a3..5e1536c61a 100644 --- a/zerver/tests/test_retention.py +++ b/zerver/tests/test_retention.py @@ -1138,7 +1138,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(20): + with self.assert_database_query_count(21): do_delete_messages(realm, messages) self.assertFalse(Message.objects.filter(id__in=message_ids).exists())