From 0c07c6531c75649045d6bf8a4f0bc6f4010c5e21 Mon Sep 17 00:00:00 2001 From: Alex Vandiver Date: Wed, 28 Aug 2024 14:45:35 +0000 Subject: [PATCH] thumbnail: Enqueue thumbnails when we render a spinner. Thumbnails are usually enqueued in the worker when the image is uploaded. However, for images which were uploaded before the existence of the thumbnailing worker, and whose metadata was backfilled (see previous commit) this leaves a permanent spinner, since nothing triggers the thumbnail worker for them. Enqueue a thumbnail worker for every spinner which we render into Markdown. This ensures that _something_ is attempting to resolve the spinner which the user sees. In the case of freshly-uploaded images which are still in the queue, this results in a duplicate entry in the thumbnailing queue -- this is harmless, since the worker determines that all of the thumbnails we need have already been generated, and it does no further work. However, in the case of historical uploads, it properly kicks off the thumbnailing process and results in a subsequent message update to include the freshly-generated thumbnail. While specifically useful for backfilled uploads, this is also generally a good safety step for a good user experience, as it also prevents dropped events in the queue from unknown causes from leaving perpetual spinners in the message feed. Because `get_user_upload_previews` is potentially called twice for every message with spinners (see 6f20c15ae9e5), we add an additional flag to `get_user_upload_previews` to suppress a _second_ event from being enqueued for every spinner generated. --- zerver/actions/message_send.py | 1 + zerver/lib/thumbnail.py | 10 +++++++++ zerver/tests/test_events.py | 4 +++- zerver/tests/test_markdown_thumbnail.py | 30 +++++++++++++++++++++++++ 4 files changed, 44 insertions(+), 1 deletion(-) diff --git a/zerver/actions/message_send.py b/zerver/actions/message_send.py index 81fd36c781..14422b6919 100644 --- a/zerver/actions/message_send.py +++ b/zerver/actions/message_send.py @@ -879,6 +879,7 @@ def do_send_messages( send_request.message.realm_id, send_request.message.content, lock=True, + enqueue=False, path_ids=list(send_request.rendering_result.thumbnail_spinners), ) new_rendered_content = rewrite_thumbnailed_images( diff --git a/zerver/lib/thumbnail.py b/zerver/lib/thumbnail.py index 0bd945208a..9e16a47d4f 100644 --- a/zerver/lib/thumbnail.py +++ b/zerver/lib/thumbnail.py @@ -336,6 +336,7 @@ def get_user_upload_previews( realm_id: int, content: str, lock: bool = False, + enqueue: bool = True, path_ids: list[str] | None = None, ) -> dict[str, MarkdownImageMetadata | None]: if path_ids is None: @@ -354,6 +355,15 @@ def get_user_upload_previews( # but has not been thumbnailed yet; we will render a # spinner. upload_preview_data[image_attachment.path_id] = None + + # We re-queue the row for thumbnailing to make sure that + # we do eventually thumbnail it (e.g. if this is a + # historical upload from before this system, which we + # backfilled ImageAttachment rows for); this is a no-op in + # the worker if all of the currently-configured thumbnail + # formats have already been generated. + if enqueue: + queue_event_on_commit("thumbnail", {"id": image_attachment.id}) else: url, is_animated = get_default_thumbnail_url(image_attachment) upload_preview_data[image_attachment.path_id] = MarkdownImageMetadata( diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index bf94f90bc8..0680d16e0f 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -1038,7 +1038,9 @@ class NormalActionsTest(BaseAction): "img.png", "image/png", read_test_image_file("img.png"), self.example_user("iago") ) path_id = url[len("/user_upload/") + 1 :] - self.send_stream_message(iago, "Verona", f"[img.png]({url})") + self.send_stream_message( + iago, "Verona", f"[img.png]({url})", skip_capture_on_commit_callbacks=True + ) # Generating a thumbnail for an image sends a message update event with self.verify_action(state_change_expected=False) as events: diff --git a/zerver/tests/test_markdown_thumbnail.py b/zerver/tests/test_markdown_thumbnail.py index 59ca8e1d26..859089a746 100644 --- a/zerver/tests/test_markdown_thumbnail.py +++ b/zerver/tests/test_markdown_thumbnail.py @@ -300,6 +300,7 @@ class MarkdownThumbnailTest(ZulipTestCase): from_user=sender_user_profile, to_user=self.example_user("hamlet"), content=f"This [image](/user_uploads/{path_id}) is private", + skip_capture_on_commit_callbacks=True, ) placeholder = ( f'
' @@ -382,3 +383,32 @@ class MarkdownThumbnailTest(ZulipTestCase): self.assert_message_content_is( message_id, f'

image

\n{rendered_thumb}' ) + + def test_thumbnail_historical_image(self) -> None: + # Note that this is outside the captureOnCommitCallbacks, so + # we don't actually run thumbnailing for it. This results in + # a ImageAttachment row but no thumbnails, which matches the + # state of backfilled previously-uploaded images. + path_id = self.upload_image("img.png") + + with self.captureOnCommitCallbacks(execute=True): + message_id = self.send_message_content(f"An [image](/user_uploads/{path_id})") + + content = f"[image](/user_uploads/{path_id})" + expected = ( + f'

image

\n' + f'
' + '
' + ) + + message_id = self.send_message_content(content) + self.assert_message_content_is(message_id, expected) + + # Exiting the block should have run the thumbnailing that was + # enqueued when rendering the message. + expected = ( + f'

image

\n' + f'
' + f'
' + ) + self.assert_message_content_is(message_id, expected)