Merge pull request #10364 from demarches-simplifiees/rotate-image-auto-ldu

ETQ instructeur, les images sont tournées dans le bon sens et je peux prévisualiser les PDF

app/jobs/image_processor_job.rb

@@ -1,4 +1,4 @@
-class TitreIdentiteWatermarkJob < ApplicationJob
+class ImageProcessorJob < ApplicationJob
   class FileNotScannedYetError < StandardError
   end
 
@@ -11,8 +11,38 @@ class TitreIdentiteWatermarkJob < ApplicationJob
   retry_on FileNotScannedYetError, wait: :exponentially_longer, attempts: 10
 
   def perform(blob)
-    return if blob.watermark_done?
+    return if blob.nil?
     raise FileNotScannedYetError if blob.virus_scanner.pending?
+    return if ActiveStorage::Attachment.find_by(blob_id: blob.id)&.record_type == "ActiveStorage::VariantRecord"
+
+    auto_rotate(blob) if ["image/jpeg", "image/jpg"].include?(blob.content_type)
+    create_representations(blob) if blob.representation_required?
+    add_watermark(blob) if blob.watermark_pending?
+  end
+
+  private
+
+  def auto_rotate(blob)
+    blob.open do |file|
+      Tempfile.create(["rotated", File.extname(file)]) do |output|
+        processed = AutoRotateService.new.process(file, output)
+        return if processed.blank?
+
+        blob.upload(processed) # also update checksum & byte_size accordingly
+        blob.save!
+      end
+    end
+  end
+
+  def create_representations(blob)
+    blob.attachments.each do |attachment|
+      next unless attachment&.representable?
+      attachment.representation(resize_to_limit: [400, 400]).processed
+    end
+  end
+
+  def add_watermark(blob)
+    return if blob.watermark_done?
 
     blob.open do |file|
       Tempfile.create(["watermarked", File.extname(file)]) do |output|

app/models/champs/piece_justificative_champ.rb

@@ -1,9 +1,7 @@
 class Champs::PieceJustificativeChamp < Champ
   FILE_MAX_SIZE = 200.megabytes
 
-  has_many_attached :piece_justificative_file do |attachable|
+  has_many_attached :piece_justificative_file
-    attachable.variant :medium, resize: '400x400'
-  end
 
   # TODO: if: -> { validate_champ_value? || validation_context == :prefill }
   validates :piece_justificative_file,

app/models/champs/titre_identite_champ.rb

@@ -2,9 +2,7 @@ class Champs::TitreIdentiteChamp < Champ
   FILE_MAX_SIZE = 20.megabytes
   ACCEPTED_FORMATS = ['image/png', 'image/jpeg']
 
-  has_many_attached :piece_justificative_file do |attachable|
+  has_many_attached :piece_justificative_file
-    attachable.variant :medium, resize: '400x400'
-  end
 
   # TODO: if: -> { validate_champ_value? || validation_context == :prefill }
   validates :piece_justificative_file, content_type: ACCEPTED_FORMATS, size: { less_than: FILE_MAX_SIZE }

app/models/concerns/attachment_image_processor_concern.rb

@@ -0,0 +1,24 @@
+# Run a virus scan on all attachments after they are analyzed.
+#
+# We're using a class extension to ensure that all attachments get scanned,
+# regardless on how they were created. This could be an ActiveStorage::Analyzer,
+# but as of Rails 6.1 only the first matching analyzer is ever run on
+# a blob (and we may want to analyze the dimension of a picture as well
+# as scanning it).
+module AttachmentImageProcessorConcern
+  extend ActiveSupport::Concern
+
+  included do
+    after_create_commit :process_image
+  end
+
+  private
+
+  def process_image
+    return if blob.nil?
+    return if blob.attachments.size > 1
+    return if blob.attachments.last.record_type == "Export"
+
+    ImageProcessorJob.perform_later(blob)
+  end
+end

app/models/concerns/blob_image_processor_concern.rb

@@ -1,4 +1,4 @@
-module BlobTitreIdentiteWatermarkConcern
+module BlobImageProcessorConcern
   def watermark_pending?
     watermark_required? && !watermark_done?
   end
@@ -7,10 +7,8 @@ module BlobTitreIdentiteWatermarkConcern
     watermarked_at.present?
   end
 
-  def watermark_later
+  def representation_required?
-    if watermark_pending?
+    attachments.any? { _1.record.class == Champs::TitreIdentiteChamp || _1.record.class == Champs::PieceJustificativeChamp }
-      TitreIdentiteWatermarkJob.perform_later(self)
-    end
   end
 
   private

app/services/auto_rotate_service.rb

@@ -0,0 +1,34 @@
+class AutoRotateService
+  def process(file, output)
+    auto_rotate_image(file, output)
+  end
+
+  private
+
+  def auto_rotate_image(file, output)
+    image = MiniMagick::Image.new(file.to_path)
+
+    return nil if !image.valid?
+
+    case image["%[orientation]"]
+    when 'LeftBottom'
+      rotate_image(file, output, 90)
+    when 'BottomRight'
+      rotate_image(file, output, 180)
+    when 'RightTop'
+      rotate_image(file, output, 270)
+    else
+      nil
+    end
+  end
+
+  def rotate_image(file, output, degree)
+    MiniMagick::Tool::Convert.new do |convert|
+      convert << file.to_path
+      convert.rotate(degree)
+      convert.auto_orient
+      convert << output.to_path
+    end
+    output
+  end
+end

app/views/instructeurs/dossiers/pieces_jointes.html.haml

@@ -5,13 +5,13 @@
 .fr-container
   .gallery.gallery-pieces-jointes{ "data-controller": "lightbox" }
     - @champs_with_pieces_jointes.each do |champ|
-      - champ.piece_justificative_file.each do |attachment|
+      - champ.piece_justificative_file.with_all_variant_records.each do |attachment|
         .gallery-item
           - blob = attachment.blob
           - if blob.content_type.in?(AUTHORIZED_PDF_TYPES)
             = link_to blob.url, id: blob.id, data: { iframe: true, src: blob.url }, class: 'gallery-link', type: blob.content_type, title: "#{champ.libelle} -- #{blob.filename}" do
               .thumbnail
-                = image_tag("pdf-placeholder.png")
+                = image_tag(attachment.representation(resize_to_limit: [400, 400]).processed.url, loading: :lazy)
                 .fr-btn.fr-btn--tertiary.fr-btn--icon-left.fr-icon-eye{ role: :button }
                   Visualiser
             .champ-libelle
@@ -21,7 +21,7 @@
           - elsif blob.content_type.in?(AUTHORIZED_IMAGE_TYPES)
             = link_to image_url(blob.url), title: "#{champ.libelle} -- #{blob.filename}", data: { src: blob.url }, class: 'gallery-link' do
               .thumbnail
-                = image_tag(attachment.variant(:medium), loading: :lazy)
+                = image_tag(attachment.representation(resize_to_limit: [400, 400]).processed.url, loading: :lazy)
                 .fr-btn.fr-btn--tertiary.fr-btn--icon-left.fr-icon-eye{ role: :button }
                   Visualiser
             .champ-libelle

app/views/shared/champs/piece_justificative/_show.html.haml

@@ -5,20 +5,20 @@
         %li= render Attachment::ShowComponent.new(attachment:, new_tab: true)
   - else
     .gallery-items-list
-      - champ.piece_justificative_file.attachments.each do |attachment|
+      - champ.piece_justificative_file.attachments.with_all_variant_records.each do |attachment|
         .gallery-item
           - blob = attachment.blob
           - if blob.content_type.in?(AUTHORIZED_PDF_TYPES)
             = link_to blob.url, id: blob.id, data: { iframe: true, src: blob.url }, class: 'gallery-link', type: blob.content_type, title: "#{champ.libelle} -- #{blob.filename}" do
               .thumbnail
-                = image_tag("pdf-placeholder.png")
+                = image_tag(attachment.representation(resize_to_limit: [400, 400]).processed.url, loading: :lazy)
                 .fr-btn.fr-btn--tertiary.fr-btn--icon-left.fr-icon-eye{ role: :button }
                   = 'Visualiser'
 
           - elsif blob.content_type.in?(AUTHORIZED_IMAGE_TYPES)
             = link_to image_url(blob.url), title: "#{champ.libelle} -- #{blob.filename}", data: { src: blob.url }, class: 'gallery-link' do
               .thumbnail
-                = image_tag(attachment.variant(:medium), loading: :lazy)
+                = image_tag(attachment.representation(resize_to_limit: [400, 400]).processed.url, loading: :lazy)
                 .fr-btn.fr-btn--tertiary.fr-btn--icon-left.fr-icon-eye{ role: :button }
                   = 'Visualiser'
           - else

config/initializers/active_storage.rb

@@ -4,7 +4,7 @@ Rails.application.config.active_storage.analyzers.delete ActiveStorage::Analyzer
 Rails.application.config.active_storage.analyzers.delete ActiveStorage::Analyzer::VideoAnalyzer
 
 ActiveSupport.on_load(:active_storage_blob) do
-  include BlobTitreIdentiteWatermarkConcern
+  include BlobImageProcessorConcern
   include BlobVirusScannerConcern
   include BlobSignedIdConcern
 
@@ -15,7 +15,7 @@ ActiveSupport.on_load(:active_storage_blob) do
 end
 
 ActiveSupport.on_load(:active_storage_attachment) do
-  include AttachmentTitreIdentiteWatermarkConcern
+  include AttachmentImageProcessorConcern
   include AttachmentVirusScannerConcern
 end
 

spec/components/attachment/multiple_component_spec.rb

@@ -105,7 +105,7 @@ RSpec.describe Attachment::MultipleComponent, type: :component do
     attached_file.attach(
       io: StringIO.new("x" * 2),
       filename: "me.jpg",
-      content_type: "image/jpeg",
+      content_type: "image/png",
       metadata: { virus_scan_result: ActiveStorage::VirusScanner::SAFE }
     )
     champ.save!

spec/controllers/instructeurs/dossiers_controller_spec.rb

@@ -1429,13 +1429,13 @@ describe Instructeurs::DossiersController, type: :controller do
   describe '#pieces_jointes' do
     let(:procedure) { create(:procedure, :published, types_de_champ_public: [{ type: :piece_justificative }], instructeurs:) }
     let(:dossier) { create(:dossier, :en_construction, :with_populated_champs, procedure: procedure) }
+    let(:path) { 'spec/fixtures/files/logo_test_procedure.png' }
 
     before do
       dossier.champs.first.piece_justificative_file.attach(
-        io: StringIO.new("image file"),
+        io: File.open(path),
-        filename: "image.jpeg",
+        filename: "logo_test_procedure.png",
-        content_type: "image/jpeg",
+        content_type: "image/png",
-        # we don't want to run virus scanner on this file
         metadata: { virus_scan_result: ActiveStorage::VirusScanner::SAFE }
       )
       get :pieces_jointes, params: {
@@ -1446,7 +1446,7 @@ describe Instructeurs::DossiersController, type: :controller do
 
     it do
       expect(response.body).to include('Télécharger le fichier toto.txt')
-      expect(response.body).to include('Télécharger le fichier image.jpeg')
+      expect(response.body).to include('Télécharger le fichier logo_test_procedure.png')
       expect(response.body).to include('Visualiser')
     end
   end

spec/jobs/image_processor_job_spec.rb

@@ -0,0 +1,135 @@
+describe ImageProcessorJob, type: :job do
+  let(:blob) do
+    ActiveStorage::Blob.create_and_upload!(io: StringIO.new("toto"), filename: "toto.png")
+  end
+
+  let(:blob_jpg) do
+    ActiveStorage::Blob.create_and_upload!(io: StringIO.new("toto"), filename: "toto.jpg")
+  end
+
+  let(:attachment) { ActiveStorage::Attachment.new(name: "test", blob: blob) }
+  let(:antivirus_pending) { false }
+  let(:watermark_service) { instance_double("WatermarkService") }
+  let(:auto_rotate_service) { instance_double("AutoRotateService") }
+
+  before do
+    virus_scanner_mock = instance_double("ActiveStorage::VirusScanner", pending?: antivirus_pending)
+    allow(blob).to receive(:attachments).and_return([attachment])
+
+    allow(blob).to receive(:virus_scanner).and_return(virus_scanner_mock)
+    allow(blob_jpg).to receive(:virus_scanner).and_return(virus_scanner_mock)
+
+    allow(WatermarkService).to receive(:new).and_return(watermark_service)
+    allow(watermark_service).to receive(:process).and_return(true)
+
+    allow(AutoRotateService).to receive(:new).and_return(auto_rotate_service)
+    allow(auto_rotate_service).to receive(:process).and_return(true)
+  end
+
+  context "when the blob is not scanned yet" do
+    let(:antivirus_pending) { true }
+
+    it "raises a FileNotScannedYetError" do
+      expect { described_class.perform_now(blob) }.to have_enqueued_job(described_class).with(blob)
+    end
+  end
+
+  describe 'autorotate' do
+    context "when image is not a jpg" do
+      let(:rotated_file) { Tempfile.new("rotated.png") }
+
+      before do
+        allow(rotated_file).to receive(:size).and_return(100)
+      end
+
+      it "it does not process autorotate" do
+        expect(auto_rotate_service).not_to receive(:process)
+        described_class.perform_now(blob)
+      end
+    end
+
+    context "when image is a jpg " do
+      let(:rotated_file) { Tempfile.new("rotated.jpg") }
+
+      before do
+        allow(rotated_file).to receive(:size).and_return(100)
+      end
+
+      it "it processes autorotate" do
+        expect(auto_rotate_service).to receive(:process).and_return(rotated_file)
+        described_class.perform_now(blob_jpg)
+      end
+    end
+  end
+
+  describe 'create representation' do
+    let(:file) { fixture_file_upload('spec/fixtures/files/logo_test_procedure.png', 'image/png') }
+    let(:blob_info) do
+      {
+        filename: file.original_filename,
+        byte_size: file.size,
+        checksum: Digest::SHA256.file(file.path),
+        content_type: file.content_type,
+        # we don't want to run virus scanner on this file
+        metadata: { virus_scan_result: ActiveStorage::VirusScanner::SAFE }
+      }
+    end
+
+    let(:blob) do
+      blob = ActiveStorage::Blob.create_before_direct_upload!(**blob_info)
+      blob.upload(file)
+      blob
+    end
+
+    context "when representation is not required" do
+      it "it does not create blob representation" do
+        expect { described_class.perform_now(blob) }.not_to change { ActiveStorage::VariantRecord.count }
+      end
+    end
+
+    context "when representation is required" do
+      before do
+        allow(blob).to receive(:representation_required?).and_return(true)
+      end
+
+      it "it creates blob representation" do
+        expect { described_class.perform_now(blob) }.to change { ActiveStorage::VariantRecord.count }.by(1)
+      end
+    end
+  end
+
+  describe 'watermark' do
+    context "when watermark is already done" do
+      before do
+        allow(blob).to receive(:watermark_done?).and_return(true)
+      end
+
+      it "does not process the watermark" do
+        expect(watermark_service).not_to receive(:process)
+        described_class.perform_now(blob)
+      end
+    end
+
+    context "when the blob is ready to be watermarked" do
+      let(:watermarked_file) { Tempfile.new("watermarked.png") }
+
+      before do
+        allow(watermarked_file).to receive(:size).and_return(100)
+        allow(blob).to receive(:watermark_pending?).and_return(true)
+      end
+
+      it "processes the blob with watermark" do
+        expect(watermark_service).to receive(:process).and_return(watermarked_file)
+
+        expect {
+          described_class.perform_now(blob)
+        }.to change {
+          blob.reload.checksum
+        }
+
+        expect(blob.byte_size).to eq(100)
+        expect(blob.watermarked_at).to be_present
+      end
+    end
+  end
+end

spec/jobs/titre_identite_watermark_job_spec.rb

@@ -1,56 +0,0 @@
-describe TitreIdentiteWatermarkJob, type: :job do
-  let(:blob) do
-    ActiveStorage::Blob.create_and_upload!(io: StringIO.new("toto"), filename: "toto.png")
-  end
-
-  let(:antivirus_pending) { false }
-  let(:watermark_service) { instance_double("WatermarkService") }
-
-  before do
-    virus_scanner_mock = instance_double("ActiveStorage::VirusScanner", pending?: antivirus_pending)
-    allow(blob).to receive(:virus_scanner).and_return(virus_scanner_mock)
-
-    allow(WatermarkService).to receive(:new).and_return(watermark_service)
-    allow(watermark_service).to receive(:process).and_return(true)
-  end
-
-  context "when watermark is already done" do
-    before do
-      allow(blob).to receive(:watermark_done?).and_return(true)
-    end
-
-    it "does not process the blob" do
-      expect(watermark_service).not_to receive(:process)
-      described_class.perform_now(blob)
-    end
-  end
-
-  context "when the blob is not scanned yet" do
-    let(:antivirus_pending) { true }
-
-    it "raises a FileNotScannedYetError" do
-      expect { described_class.perform_now(blob) }.to have_enqueued_job(described_class).with(blob)
-    end
-  end
-
-  context "when the blob is ready to be watermarked" do
-    let(:watermarked_file) { Tempfile.new("watermarked.png") }
-
-    before do
-      allow(watermarked_file).to receive(:size).and_return(100)
-    end
-
-    it "processes the blob with watermark" do
-      expect(watermark_service).to receive(:process).and_return(watermarked_file)
-
-      expect {
-        described_class.perform_now(blob)
-      }.to change {
-        blob.reload.checksum
-      }
-
-      expect(blob.byte_size).to eq(100)
-      expect(blob.watermarked_at).to be_present
-    end
-  end
-end

spec/services/auto_rotate_service_spec.rb

@@ -0,0 +1,33 @@
+RSpec.describe AutoRotateService do
+  let(:image) { file_fixture("image-rotated.jpg") }
+  let(:image_no_exif) { file_fixture("image-no-exif.jpg") }
+  let(:image_no_rotation) { file_fixture("image-no-rotation.jpg") }
+  let(:auto_rotate_service) { AutoRotateService.new }
+
+  describe '#process' do
+    it 'returns a tempfile if auto_rotate succeeds' do
+      Tempfile.create do |output|
+        result = auto_rotate_service.process(image, output)
+        expect(MiniMagick::Image.new(image.to_path)["%[orientation]"]).to eq('LeftBottom')
+        expect(MiniMagick::Image.new(output.to_path)["%[orientation]"]).to eq('TopLeft')
+        expect(result.size).to be_between(image.size / 1.2, image.size)
+      end
+    end
+
+    it 'returns nil if image does not need to be return' do
+      Tempfile.create do |output|
+        result = auto_rotate_service.process(image_no_rotation, output)
+        expect(MiniMagick::Image.new(image_no_rotation.to_path)["%[orientation]"]).to eq('TopLeft')
+        expect(result).to eq nil
+      end
+    end
+
+    it 'returns nil if no exif info on image' do
+      Tempfile.create do |output|
+        result = auto_rotate_service.process(image_no_exif, output)
+        expect(MiniMagick::Image.new(image_no_exif.to_path)["%[orientation]"]).to eq('Undefined')
+        expect(result).to eq nil
+      end
+    end
+  end
+end