Merge pull request #51351 from rosa/dont-enqueue-unnecessary-preview-…

…jobs

Don't enqueue jobs to process a preview image if no variant requires it

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|

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

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,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

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

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

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