mirror of https://github.com/zulip/zulip.git
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.
This commit is contained in:
parent
31c7b2bfd7
commit
de90d0acdf
|
@ -20,6 +20,11 @@ format used by the Zulip server that they are interacting with.
|
||||||
|
|
||||||
## Changes in Zulip 9.0
|
## 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**
|
**Feature level 255**
|
||||||
|
|
||||||
* "Stream" was renamed to "Channel" across strings in the Zulip API
|
* "Stream" was renamed to "Channel" across strings in the Zulip API
|
||||||
|
|
|
@ -33,7 +33,7 @@ DESKTOP_WARNING_VERSION = "5.9.3"
|
||||||
# Changes should be accompanied by documentation explaining what the
|
# Changes should be accompanied by documentation explaining what the
|
||||||
# new level means in api_docs/changelog.md, as well as "**Changes**"
|
# new level means in api_docs/changelog.md, as well as "**Changes**"
|
||||||
# entries in the endpoint's documentation in `zulip.yaml`.
|
# 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
|
# 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
|
# only when going from an old version of the code to a newer version. Bump
|
||||||
|
|
|
@ -3,7 +3,7 @@ from typing import Iterable, List, TypedDict
|
||||||
from zerver.lib import retention
|
from zerver.lib import retention
|
||||||
from zerver.lib.retention import move_messages_to_archive
|
from zerver.lib.retention import move_messages_to_archive
|
||||||
from zerver.lib.stream_subscription import get_active_subscriptions_for_stream_id
|
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
|
from zerver.tornado.django_api import send_event_on_commit
|
||||||
|
|
||||||
|
|
||||||
|
@ -15,6 +15,34 @@ class DeleteMessagesEvent(TypedDict, total=False):
|
||||||
stream_id: int
|
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:
|
def do_delete_messages(realm: Realm, messages: Iterable[Message]) -> None:
|
||||||
# messages in delete_message event belong to the same topic
|
# messages in delete_message event belong to the same topic
|
||||||
# or is a single direct message, as any other behaviour is not possible with
|
# 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
|
archiving_chunk_size = retention.STREAM_MESSAGE_BATCH_SIZE
|
||||||
|
|
||||||
move_messages_to_archive(message_ids, realm=realm, chunk_size=archiving_chunk_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
|
event["message_type"] = message_type
|
||||||
send_event_on_commit(realm, event, users_to_notify)
|
send_event_on_commit(realm, event, users_to_notify)
|
||||||
|
|
|
@ -18080,7 +18080,11 @@ paths:
|
||||||
response. If all messages in the topic were deleted, then the success
|
response. If all messages in the topic were deleted, then the success
|
||||||
response will return `"complete": true`.
|
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
|
processing was interrupted by a timeout, but some messages in the topic
|
||||||
were deleted, then it would return `"result": "partially_completed"`,
|
were deleted, then it would return `"result": "partially_completed"`,
|
||||||
along with a `code` field for an error string, in the success response
|
along with a `code` field for an error string, in the success response
|
||||||
|
|
|
@ -11,6 +11,7 @@ from zerver.actions.realm_settings import do_set_realm_property
|
||||||
from zerver.lib.test_classes import ZulipTestCase
|
from zerver.lib.test_classes import ZulipTestCase
|
||||||
from zerver.models import Message, Realm, UserProfile
|
from zerver.models import Message, Realm, UserProfile
|
||||||
from zerver.models.realms import get_realm
|
from zerver.models.realms import get_realm
|
||||||
|
from zerver.models.streams import get_stream
|
||||||
|
|
||||||
if TYPE_CHECKING:
|
if TYPE_CHECKING:
|
||||||
from django.test.client import _MonkeyPatchedWSGIResponse as TestHttpResponse
|
from django.test.client import _MonkeyPatchedWSGIResponse as TestHttpResponse
|
||||||
|
@ -321,3 +322,28 @@ class DeleteMessageTest(ZulipTestCase):
|
||||||
result = self.client_delete(f"/json/messages/{msg_id}")
|
result = self.client_delete(f"/json/messages/{msg_id}")
|
||||||
self.assert_json_success(result)
|
self.assert_json_success(result)
|
||||||
self.assertFalse(Message.objects.filter(id=msg_id).exists())
|
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)
|
||||||
|
|
|
@ -1138,7 +1138,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(20):
|
with self.assert_database_query_count(21):
|
||||||
do_delete_messages(realm, messages)
|
do_delete_messages(realm, messages)
|
||||||
self.assertFalse(Message.objects.filter(id__in=message_ids).exists())
|
self.assertFalse(Message.objects.filter(id__in=message_ids).exists())
|
||||||
|
|
||||||
|
|
Loading…
Reference in New Issue