Skip to content

Commit

Permalink
Don't enqueue jobs to process a preview image if no variant requires it
Browse files Browse the repository at this point in the history
This follows up on rails#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.
  • Loading branch information
rosa committed Mar 20, 2024
1 parent 3efae44 commit 6ecb53c
Show file tree
Hide file tree
Showing 6 changed files with 62 additions and 13 deletions.
2 changes: 1 addition & 1 deletion activestorage/app/models/active_storage/attachment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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|
Expand Down
23 changes: 23 additions & 0 deletions activestorage/test/jobs/preview_image_job_test.rb
Original file line number Diff line number Diff line change
@@ -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
12 changes: 7 additions & 5 deletions activestorage/test/models/attached/many_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -938,22 +938,24 @@ 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

blob = create_file_blob(filename: "racecar.jpg")
@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
Expand Down
26 changes: 22 additions & 4 deletions activestorage/test/models/attached/one_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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

Expand All @@ -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
6 changes: 3 additions & 3 deletions activestorage/test/models/reflection_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
6 changes: 6 additions & 0 deletions activestorage/test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down

0 comments on commit 6ecb53c

Please sign in to comment.