Merge pull request #10449 from demarches-simplifiees/fix-gallery-with…

…-pdf-previews-and-variants-for-exotic-formats

ETQ instructeur, dans la galerie, je peux prévisualiser des pdfs et voir des images au format rare

.github/workflows/ci.yml

@@ -81,7 +81,8 @@ jobs:
       - name: Install build dependancies
         # - fonts pickable by ImageMagick
         # - rust for YJIT support
-        run: sudo apt-get update && sudo apt-get install -y gsfonts rustc redis-server
+        # - poppler-utils for pdf previews
+        run: sudo apt-get update && sudo apt-get install -y gsfonts rustc redis-server poppler-utils
 
       - name: Setup the app runtime and dependencies
         uses: ./.github/actions/ci-setup-rails

app/helpers/gallery_helper.rb

@@ -0,0 +1,27 @@
+module GalleryHelper
+  def displayable_pdf?(blob)
+    blob.previewable? && blob.content_type.in?(AUTHORIZED_PDF_TYPES)
+  end
+
+  def displayable_image?(blob)
+    blob.variable? && blob.content_type.in?(AUTHORIZED_IMAGE_TYPES)
+  end
+
+  def preview_url_for(attachment)
+    attachment.preview(resize_to_limit: [400, 400]).processed.url
+  rescue ActiveStorage::Error
+    'pdf-placeholder.png'
+  end
+
+  def variant_url_for(attachment)
+    attachment.variant(resize_to_limit: [400, 400]).processed.url
+  rescue ActiveStorage::Error
+    'apercu-indisponible.png'
+  end
+
+  def blob_url(attachment)
+    attachment.blob.content_type.in?(RARE_IMAGE_TYPES) ? attachment.variant(resize_to_limit: [2000, 2000]).processed.url : attachment.blob.url
+  rescue ActiveStorage::Error
+    attachment.blob.url
+  end
+end

app/jobs/image_processor_job.rb

@@ -10,6 +10,10 @@ class FileNotScannedYetError < StandardError
   # (to avoid modifying the file while it is being scanned).
   retry_on FileNotScannedYetError, wait: :exponentially_longer, attempts: 10
 
+  rescue_from ActiveStorage::PreviewError do
+    retry_or_discard
+  end
+
   def perform(blob)
     return if blob.nil?
     raise FileNotScannedYetError if blob.virus_scanner.pending?
@@ -38,6 +42,9 @@ def create_representations(blob)
     blob.attachments.each do |attachment|
       next unless attachment&.representable?
       attachment.representation(resize_to_limit: [400, 400]).processed
+      if attachment.blob.content_type.in?(RARE_IMAGE_TYPES)
+        attachment.variant(resize_to_limit: [2000, 2000]).processed
+      end
     end
   end
 
@@ -55,4 +62,14 @@ def add_watermark(blob)
       end
     end
   end
+
+  def retry_or_discard
+    if executions < max_attempts
+      retry_job wait: 5.minutes
+    end
+  end
+
+  def max_attempts
+    3
+  end
 end

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

@@ -8,20 +8,20 @@
       - champ.piece_justificative_file.with_all_variant_records.each do |attachment|
         .gallery-item
           - blob = attachment.blob
-          - if blob.content_type.in?(AUTHORIZED_PDF_TYPES)
+          - if displayable_pdf?(blob)
             = 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(preview_url_for(attachment), loading: :lazy)
                 .fr-btn.fr-btn--tertiary.fr-btn--icon-left.fr-icon-eye{ role: :button }
                   Visualiser
             .champ-libelle
               = champ.libelle.truncate(25)
             = render Attachment::ShowComponent.new(attachment: attachment, truncate: true)
 
-          - 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
+          - elsif displayable_image?(blob)
+            = link_to image_url(blob_url(attachment)), title: "#{champ.libelle} -- #{blob.filename}", data: { src: blob.url }, class: 'gallery-link' do
               .thumbnail
-                = image_tag(attachment.variant(resize_to_limit: [400, 400]).processed.url, loading: :lazy)
+                = image_tag(variant_url_for(attachment), 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

@@ -8,17 +8,17 @@
       - 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)
+          - if displayable_pdf?(blob)
             = 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(preview_url_for(attachment), 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
+          - elsif displayable_image?(blob)
+            = link_to image_url(blob_url(attachment)), title: "#{champ.libelle} -- #{blob.filename}", data: { src: blob.url }, class: 'gallery-link' do
               .thumbnail
-                = image_tag(attachment.variant(resize_to_limit: [400, 400]).processed.url, loading: :lazy)
+                = image_tag(variant_url_for(attachment), loading: :lazy)
                 .fr-btn.fr-btn--tertiary.fr-btn--icon-left.fr-icon-eye{ role: :button }
                   = 'Visualiser'
           - else

config/initializers/authorized_content_types.rb

@@ -15,6 +15,10 @@
   'image/vnd.dwg' # multimedia x 137 auto desk
 ]
 
+RARE_IMAGE_TYPES = [
+  'image/tiff' # multimedia x 3985
+]
+
 AUTHORIZED_CONTENT_TYPES = AUTHORIZED_IMAGE_TYPES + AUTHORIZED_PDF_TYPES + [
   # multimedia
   'video/mp4', # multimedia x 2075

spec/helpers/gallery_helper_spec.rb

@@ -0,0 +1,57 @@
+RSpec.describe GalleryHelper, type: :helper do
+  let(:procedure) { create(:procedure_with_dossiers) }
+  let(:type_de_champ_pj) { create(:type_de_champ_piece_justificative, stable_id: 3, libelle: 'Justificatif de domicile', procedure:) }
+  let(:champ_pj) { create(:champ_piece_justificative, type_de_champ: type_de_champ_pj) }
+  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
+  let(:attachment) { ActiveStorage::Attachment.create(name: "test", blob: blob, record: champ_pj) }
+
+  describe ".variant_url_for" do
+    subject { variant_url_for(attachment) }
+
+    context "when attachment can be represented with a variant" do
+      let(:file) { fixture_file_upload('spec/fixtures/files/logo_test_procedure.png', 'image/png') }
+
+      it { expect { subject }.to change { ActiveStorage::VariantRecord.count }.by(1) }
+      it { is_expected.not_to eq("apercu-indisponible.png") }
+    end
+
+    context "when attachment cannot be represented with a variant" do
+      let(:file) { fixture_file_upload('spec/fixtures/files/instructeurs-file.csv', 'text/csv') }
+
+      it { expect { subject }.not_to change { ActiveStorage::VariantRecord.count } }
+      it { is_expected.to eq("apercu-indisponible.png") }
+    end
+  end
+
+  describe ".preview_url_for" do
+    subject { preview_url_for(attachment) }
+
+    context "when attachment can be represented with a preview" do
+      let(:file) { fixture_file_upload('spec/fixtures/files/RIB.pdf', 'application/pdf') }
+
+      it { expect { subject }.to change { ActiveStorage::VariantRecord.count }.by(1) }
+      it { is_expected.not_to eq("pdf-placeholder.png") }
+    end
+
+    context "when attachment cannot be represented with a preview" do
+      let(:file) { fixture_file_upload('spec/fixtures/files/instructeurs-file.csv', 'text/csv') }
+
+      it { expect { subject }.not_to change { ActiveStorage::VariantRecord.count } }
+      it { is_expected.to eq("pdf-placeholder.png") }
+    end
+  end
+end

spec/jobs/image_processor_job_spec.rb

@@ -63,7 +63,6 @@
   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,
@@ -81,19 +80,35 @@
       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 }
+    context "when type image is usual" do
+      let(:file) { fixture_file_upload('spec/fixtures/files/logo_test_procedure.png', 'image/png') }
+
+      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
 
-    context "when representation is required" do
+    context "when type image is rare" do
+      let(:file) { fixture_file_upload('spec/fixtures/files/pencil.tiff', 'image/tiff') }
+
       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)
+      it "creates a second variant" do
+        expect { described_class.perform_now(blob) }.to change { ActiveStorage::VariantRecord.count }.by(2)
       end
     end
   end