Skip to content

Commit 6ecb53c

Browse files
committed
Don't enqueue jobs to process a preview image if no variant requires it
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.
1 parent 3efae44 commit 6ecb53c

File tree

6 files changed

+62
-13
lines changed

6 files changed

+62
-13
lines changed

activestorage/app/models/active_storage/attachment.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ def transform_variants_later
138138
end
139139
}
140140

141-
if blob.preview_image_needed_before_processing_variants?
141+
if blob.preview_image_needed_before_processing_variants? && preprocessed_variations.any?
142142
blob.create_preview_image_later(preprocessed_variations)
143143
else
144144
preprocessed_variations.each do |transformations|
Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
# frozen_string_literal: true
2+
3+
require "test_helper"
4+
require "database/setup"
5+
6+
class ActiveStorage::PreviewImageJobTest < ActiveJob::TestCase
7+
setup do
8+
@blob = create_file_blob(filename: "report.pdf", content_type: "application/pdf")
9+
@transformation = { resize_to_limit: [ 100, 100 ] }
10+
end
11+
12+
test "creates preview" do
13+
assert_changes -> { @blob.preview_image.attached? }, from: false, to: true do
14+
ActiveStorage::PreviewImageJob.perform_now @blob, [ @transformation ]
15+
end
16+
end
17+
18+
test "enqueues transform variant jobs" do
19+
assert_enqueued_with job: ActiveStorage::TransformJob, args: [ @blob, @transformation ] do
20+
ActiveStorage::PreviewImageJob.perform_now @blob, [ @transformation ]
21+
end
22+
end
23+
end

activestorage/test/models/attached/many_test.rb

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -925,7 +925,7 @@ def self.name; superclass.name; end
925925
end
926926

927927
test "transforms variants later conditionally via proc" do
928-
assert_no_enqueued_jobs only: ActiveStorage::TransformJob do
928+
assert_no_enqueued_jobs only: [ ActiveStorage::TransformJob, ActiveStorage::PreviewImageJob ] do
929929
@user.highlights_with_conditional_preprocessed.attach create_file_blob(filename: "racecar.jpg")
930930
end
931931

@@ -938,22 +938,24 @@ def self.name; superclass.name; end
938938
end
939939

940940
test "transforms variants later conditionally via method" do
941-
assert_no_enqueued_jobs only: ActiveStorage::TransformJob do
941+
assert_no_enqueued_jobs only: [ ActiveStorage::TransformJob, ActiveStorage::PreviewImageJob ] do
942942
@user.highlights_with_conditional_preprocessed.attach create_file_blob(filename: "racecar.jpg")
943943
end
944944

945945
blob = create_file_blob(filename: "racecar.jpg")
946946
@user.update(name: "transform via method")
947947

948948
assert_enqueued_with job: ActiveStorage::TransformJob, args: [blob, resize_to_limit: [3, 3]] do
949-
@user.highlights_with_conditional_preprocessed.attach blob
949+
assert_no_enqueued_jobs only: ActiveStorage::PreviewImageJob do
950+
@user.highlights_with_conditional_preprocessed.attach blob
951+
end
950952
end
951953
end
952954

953-
test "avoids enqueuing transform later job when blob is not representable" do
955+
test "avoids enqueuing transform later and create preview job job when blob is not representable" do
954956
unrepresentable_blob = create_blob(filename: "hello.txt")
955957

956-
assert_no_enqueued_jobs only: ActiveStorage::TransformJob do
958+
assert_no_enqueued_jobs only: [ ActiveStorage::TransformJob, ActiveStorage::PreviewImageJob ] do
957959
@user.highlights_with_preprocessed.attach unrepresentable_blob
958960
end
959961
end

activestorage/test/models/attached/one_test.rb

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -867,7 +867,7 @@ def self.name; superclass.name; end
867867
end
868868

869869
test "transforms variants later conditionally via proc" do
870-
assert_no_enqueued_jobs only: ActiveStorage::TransformJob do
870+
assert_no_enqueued_jobs only: [ ActiveStorage::TransformJob, ActiveStorage::PreviewImageJob ] do
871871
@user.avatar_with_conditional_preprocessed.attach create_file_blob(filename: "racecar.jpg")
872872
end
873873

@@ -880,7 +880,7 @@ def self.name; superclass.name; end
880880
end
881881

882882
test "transforms variants later conditionally via method" do
883-
assert_no_enqueued_jobs only: ActiveStorage::TransformJob do
883+
assert_no_enqueued_jobs only: [ ActiveStorage::TransformJob, ActiveStorage::PreviewImageJob ] do
884884
@user.avatar_with_conditional_preprocessed.attach create_file_blob(filename: "racecar.jpg")
885885
end
886886

@@ -892,11 +892,29 @@ def self.name; superclass.name; end
892892
end
893893
end
894894

895-
test "avoids enqueuing transform later job when blob is not representable" do
895+
test "avoids enqueuing transform later job or preview image job when blob is not representable" do
896896
unrepresentable_blob = create_blob(filename: "hello.txt")
897897

898-
assert_no_enqueued_jobs only: ActiveStorage::TransformJob do
898+
assert_no_enqueued_jobs only: [ ActiveStorage::TransformJob, ActiveStorage::PreviewImageJob ] do
899899
@user.avatar_with_preprocessed.attach unrepresentable_blob
900900
end
901901
end
902+
903+
test "avoids enqueuing transform later job or preview later job if there aren't any variants to preprocess" do
904+
blob = create_file_blob(filename: "report.pdf")
905+
906+
assert_no_enqueued_jobs only: [ ActiveStorage::TransformJob, ActiveStorage::PreviewImageJob ] do
907+
@user.resume.attach blob
908+
end
909+
end
910+
911+
test "creates preview later without transforming variants if required and there are variants to preprocess" do
912+
blob = create_file_blob(filename: "report.pdf")
913+
914+
assert_enqueued_with job: ActiveStorage::PreviewImageJob, args: [blob, [resize_to_fill: [400, 400]]] do
915+
assert_no_enqueued_jobs only: ActiveStorage::TransformJob do
916+
@user.resume_with_preprocessing.attach blob
917+
end
918+
end
919+
end
902920
end

activestorage/test/models/reflection_test.rb

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,8 @@ class ActiveStorage::ReflectionTest < ActiveSupport::TestCase
3939
test "reflecting on all attachments" do
4040
reflections = User.reflect_on_all_attachments.sort_by(&:name)
4141
assert_equal [ User ], reflections.collect(&:active_record).uniq
42-
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)
43-
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)
44-
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] }
42+
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)
43+
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)
44+
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] }
4545
end
4646
end

activestorage/test/test_helper.rb

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,12 @@ class User < ActiveRecord::Base
160160
attachable.variant :method, resize_to_limit: [3, 3],
161161
preprocessed: :should_preprocessed?
162162
end
163+
has_one_attached :resume do |attachable|
164+
attachable.variant :preview, resize_to_fill: [400, 400]
165+
end
166+
has_one_attached :resume_with_preprocessing do |attachable|
167+
attachable.variant :preview, resize_to_fill: [400, 400], preprocessed: true
168+
end
163169

164170
accepts_nested_attributes_for :highlights_attachments, allow_destroy: true
165171

0 commit comments

Comments
 (0)