From 6ecb53cf9288a6fea468225eceb2a34b18f6e1e8 Mon Sep 17 00:00:00 2001 From: Rosa Gutierrez Date: Tue, 19 Mar 2024 14:23:36 +0100 Subject: [PATCH] Don't enqueue jobs to process a preview image if no variant requires it This follows up on https://github.com/rails/rails/pull/51030, that introduces a behaviour change for previewable attachments that don't specify any variants at all or no variants that require pre-processing via `TransformJob`. Before, when no variant required pre-processing, Attachment#transform_variants_later didn't do anything. Now, a `ActiveStorage::PreviewImageJob` is enqueued with the attachment's blob and a empty array. --- .../app/models/active_storage/attachment.rb | 2 +- .../test/jobs/preview_image_job_test.rb | 23 ++++++++++++++++ .../test/models/attached/many_test.rb | 12 +++++---- .../test/models/attached/one_test.rb | 26 ++++++++++++++++--- activestorage/test/models/reflection_test.rb | 6 ++--- activestorage/test/test_helper.rb | 6 +++++ 6 files changed, 62 insertions(+), 13 deletions(-) create mode 100644 activestorage/test/jobs/preview_image_job_test.rb diff --git a/activestorage/app/models/active_storage/attachment.rb b/activestorage/app/models/active_storage/attachment.rb index ce439f6fd21e6..6b697dafb45ec 100644 --- a/activestorage/app/models/active_storage/attachment.rb +++ b/activestorage/app/models/active_storage/attachment.rb @@ -138,7 +138,7 @@ def transform_variants_later end } - if blob.preview_image_needed_before_processing_variants? + if blob.preview_image_needed_before_processing_variants? && preprocessed_variations.any? blob.create_preview_image_later(preprocessed_variations) else preprocessed_variations.each do |transformations| diff --git a/activestorage/test/jobs/preview_image_job_test.rb b/activestorage/test/jobs/preview_image_job_test.rb new file mode 100644 index 0000000000000..ea8040f18f43a --- /dev/null +++ b/activestorage/test/jobs/preview_image_job_test.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +require "test_helper" +require "database/setup" + +class ActiveStorage::PreviewImageJobTest < ActiveJob::TestCase + setup do + @blob = create_file_blob(filename: "report.pdf", content_type: "application/pdf") + @transformation = { resize_to_limit: [ 100, 100 ] } + end + + test "creates preview" do + assert_changes -> { @blob.preview_image.attached? }, from: false, to: true do + ActiveStorage::PreviewImageJob.perform_now @blob, [ @transformation ] + end + end + + test "enqueues transform variant jobs" do + assert_enqueued_with job: ActiveStorage::TransformJob, args: [ @blob, @transformation ] do + ActiveStorage::PreviewImageJob.perform_now @blob, [ @transformation ] + end + end +end diff --git a/activestorage/test/models/attached/many_test.rb b/activestorage/test/models/attached/many_test.rb index 58785f344460f..e9655037ef502 100644 --- a/activestorage/test/models/attached/many_test.rb +++ b/activestorage/test/models/attached/many_test.rb @@ -925,7 +925,7 @@ def self.name; superclass.name; end end test "transforms variants later conditionally via proc" do - assert_no_enqueued_jobs only: ActiveStorage::TransformJob do + assert_no_enqueued_jobs only: [ ActiveStorage::TransformJob, ActiveStorage::PreviewImageJob ] do @user.highlights_with_conditional_preprocessed.attach create_file_blob(filename: "racecar.jpg") end @@ -938,7 +938,7 @@ def self.name; superclass.name; end end test "transforms variants later conditionally via method" do - assert_no_enqueued_jobs only: ActiveStorage::TransformJob do + assert_no_enqueued_jobs only: [ ActiveStorage::TransformJob, ActiveStorage::PreviewImageJob ] do @user.highlights_with_conditional_preprocessed.attach create_file_blob(filename: "racecar.jpg") end @@ -946,14 +946,16 @@ def self.name; superclass.name; end @user.update(name: "transform via method") assert_enqueued_with job: ActiveStorage::TransformJob, args: [blob, resize_to_limit: [3, 3]] do - @user.highlights_with_conditional_preprocessed.attach blob + assert_no_enqueued_jobs only: ActiveStorage::PreviewImageJob do + @user.highlights_with_conditional_preprocessed.attach blob + end end end - test "avoids enqueuing transform later job when blob is not representable" do + test "avoids enqueuing transform later and create preview job job when blob is not representable" do unrepresentable_blob = create_blob(filename: "hello.txt") - assert_no_enqueued_jobs only: ActiveStorage::TransformJob do + assert_no_enqueued_jobs only: [ ActiveStorage::TransformJob, ActiveStorage::PreviewImageJob ] do @user.highlights_with_preprocessed.attach unrepresentable_blob end end diff --git a/activestorage/test/models/attached/one_test.rb b/activestorage/test/models/attached/one_test.rb index 9c86acac5754e..9755b6910767e 100644 --- a/activestorage/test/models/attached/one_test.rb +++ b/activestorage/test/models/attached/one_test.rb @@ -867,7 +867,7 @@ def self.name; superclass.name; end end test "transforms variants later conditionally via proc" do - assert_no_enqueued_jobs only: ActiveStorage::TransformJob do + assert_no_enqueued_jobs only: [ ActiveStorage::TransformJob, ActiveStorage::PreviewImageJob ] do @user.avatar_with_conditional_preprocessed.attach create_file_blob(filename: "racecar.jpg") end @@ -880,7 +880,7 @@ def self.name; superclass.name; end end test "transforms variants later conditionally via method" do - assert_no_enqueued_jobs only: ActiveStorage::TransformJob do + assert_no_enqueued_jobs only: [ ActiveStorage::TransformJob, ActiveStorage::PreviewImageJob ] do @user.avatar_with_conditional_preprocessed.attach create_file_blob(filename: "racecar.jpg") end @@ -892,11 +892,29 @@ def self.name; superclass.name; end end end - test "avoids enqueuing transform later job when blob is not representable" do + test "avoids enqueuing transform later job or preview image job when blob is not representable" do unrepresentable_blob = create_blob(filename: "hello.txt") - assert_no_enqueued_jobs only: ActiveStorage::TransformJob do + assert_no_enqueued_jobs only: [ ActiveStorage::TransformJob, ActiveStorage::PreviewImageJob ] do @user.avatar_with_preprocessed.attach unrepresentable_blob end end + + test "avoids enqueuing transform later job or preview later job if there aren't any variants to preprocess" do + blob = create_file_blob(filename: "report.pdf") + + assert_no_enqueued_jobs only: [ ActiveStorage::TransformJob, ActiveStorage::PreviewImageJob ] do + @user.resume.attach blob + end + end + + test "creates preview later without transforming variants if required and there are variants to preprocess" do + blob = create_file_blob(filename: "report.pdf") + + assert_enqueued_with job: ActiveStorage::PreviewImageJob, args: [blob, [resize_to_fill: [400, 400]]] do + assert_no_enqueued_jobs only: ActiveStorage::TransformJob do + @user.resume_with_preprocessing.attach blob + end + end + end end diff --git a/activestorage/test/models/reflection_test.rb b/activestorage/test/models/reflection_test.rb index a062bf12b7acf..2303212ff1276 100644 --- a/activestorage/test/models/reflection_test.rb +++ b/activestorage/test/models/reflection_test.rb @@ -39,8 +39,8 @@ class ActiveStorage::ReflectionTest < ActiveSupport::TestCase test "reflecting on all attachments" do reflections = User.reflect_on_all_attachments.sort_by(&:name) assert_equal [ User ], reflections.collect(&:active_record).uniq - assert_equal %i[ avatar avatar_with_conditional_preprocessed avatar_with_preprocessed avatar_with_variants cover_photo highlights highlights_with_conditional_preprocessed highlights_with_preprocessed highlights_with_variants intro_video name_pronunciation_audio vlogs ], reflections.collect(&:name) - assert_equal %i[ has_one_attached has_one_attached has_one_attached has_one_attached has_one_attached has_many_attached has_many_attached has_many_attached has_many_attached has_one_attached has_one_attached has_many_attached ], reflections.collect(&:macro) - assert_equal [ :purge_later, :purge_later, :purge_later, :purge_later, false, :purge_later, :purge_later, :purge_later, :purge_later, :purge_later, :purge_later, false ], reflections.collect { |reflection| reflection.options[:dependent] } + assert_equal %i[ avatar avatar_with_conditional_preprocessed avatar_with_preprocessed avatar_with_variants cover_photo highlights highlights_with_conditional_preprocessed highlights_with_preprocessed highlights_with_variants intro_video name_pronunciation_audio resume resume_with_preprocessing vlogs ], reflections.collect(&:name) + assert_equal %i[ has_one_attached has_one_attached has_one_attached has_one_attached has_one_attached has_many_attached has_many_attached has_many_attached has_many_attached has_one_attached has_one_attached has_one_attached has_one_attached has_many_attached ], reflections.collect(&:macro) + assert_equal [ :purge_later, :purge_later, :purge_later, :purge_later, false, :purge_later, :purge_later, :purge_later, :purge_later, :purge_later, :purge_later, :purge_later, :purge_later, false ], reflections.collect { |reflection| reflection.options[:dependent] } end end diff --git a/activestorage/test/test_helper.rb b/activestorage/test/test_helper.rb index 8a3c8a5fd82ce..48fe7dfc72d1e 100644 --- a/activestorage/test/test_helper.rb +++ b/activestorage/test/test_helper.rb @@ -160,6 +160,12 @@ class User < ActiveRecord::Base attachable.variant :method, resize_to_limit: [3, 3], preprocessed: :should_preprocessed? end + has_one_attached :resume do |attachable| + attachable.variant :preview, resize_to_fill: [400, 400] + end + has_one_attached :resume_with_preprocessing do |attachable| + attachable.variant :preview, resize_to_fill: [400, 400], preprocessed: true + end accepts_nested_attributes_for :highlights_attachments, allow_destroy: true