diff --git a/app/assets/images/apercu-indisponible.png b/app/assets/images/apercu-indisponible.png index 6bebd9f59..e6c56638a 100644 Binary files a/app/assets/images/apercu-indisponible.png and b/app/assets/images/apercu-indisponible.png differ diff --git a/app/assets/images/pdf-placeholder.png b/app/assets/images/pdf-placeholder.png deleted file mode 100644 index 58da75f9f..000000000 Binary files a/app/assets/images/pdf-placeholder.png and /dev/null differ diff --git a/app/jobs/titre_identite_watermark_job.rb b/app/jobs/image_processor_job.rb similarity index 50% rename from app/jobs/titre_identite_watermark_job.rb rename to app/jobs/image_processor_job.rb index 1b261e291..8017d2b35 100644 --- a/app/jobs/titre_identite_watermark_job.rb +++ b/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| diff --git a/app/models/champs/piece_justificative_champ.rb b/app/models/champs/piece_justificative_champ.rb index b6dfb0bef..dcd8e56ce 100644 --- a/app/models/champs/piece_justificative_champ.rb +++ b/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| - attachable.variant :medium, resize: '400x400' - end + has_many_attached :piece_justificative_file # TODO: if: -> { validate_champ_value? || validation_context == :prefill } validates :piece_justificative_file, diff --git a/app/models/champs/titre_identite_champ.rb b/app/models/champs/titre_identite_champ.rb index 667feca7e..6d95f4d43 100644 --- a/app/models/champs/titre_identite_champ.rb +++ b/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| - attachable.variant :medium, resize: '400x400' - end + has_many_attached :piece_justificative_file # TODO: if: -> { validate_champ_value? || validation_context == :prefill } validates :piece_justificative_file, content_type: ACCEPTED_FORMATS, size: { less_than: FILE_MAX_SIZE } diff --git a/app/models/concerns/attachment_image_processor_concern.rb b/app/models/concerns/attachment_image_processor_concern.rb new file mode 100644 index 000000000..25d4a3a09 --- /dev/null +++ b/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 diff --git a/app/models/concerns/blob_titre_identite_watermark_concern.rb b/app/models/concerns/blob_image_processor_concern.rb similarity index 57% rename from app/models/concerns/blob_titre_identite_watermark_concern.rb rename to app/models/concerns/blob_image_processor_concern.rb index 152c512f9..3b92ad4e4 100644 --- a/app/models/concerns/blob_titre_identite_watermark_concern.rb +++ b/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 - if watermark_pending? - TitreIdentiteWatermarkJob.perform_later(self) - end + def representation_required? + attachments.any? { _1.record.class == Champs::TitreIdentiteChamp || _1.record.class == Champs::PieceJustificativeChamp } end private diff --git a/app/services/auto_rotate_service.rb b/app/services/auto_rotate_service.rb new file mode 100644 index 000000000..9cec29a95 --- /dev/null +++ b/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 diff --git a/app/views/instructeurs/dossiers/pieces_jointes.html.haml b/app/views/instructeurs/dossiers/pieces_jointes.html.haml index a0b1816a1..1d08707a3 100644 --- a/app/views/instructeurs/dossiers/pieces_jointes.html.haml +++ b/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 diff --git a/app/views/shared/champs/piece_justificative/_show.html.haml b/app/views/shared/champs/piece_justificative/_show.html.haml index 50218e54b..670e9e011 100644 --- a/app/views/shared/champs/piece_justificative/_show.html.haml +++ b/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 diff --git a/config/initializers/active_storage.rb b/config/initializers/active_storage.rb index 042333ae3..5598525ad 100644 --- a/config/initializers/active_storage.rb +++ b/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 diff --git a/spec/components/attachment/multiple_component_spec.rb b/spec/components/attachment/multiple_component_spec.rb index 9f4d02549..26dfacd2c 100644 --- a/spec/components/attachment/multiple_component_spec.rb +++ b/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! diff --git a/spec/controllers/instructeurs/dossiers_controller_spec.rb b/spec/controllers/instructeurs/dossiers_controller_spec.rb index f4f065948..27a02ae7f 100644 --- a/spec/controllers/instructeurs/dossiers_controller_spec.rb +++ b/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"), - filename: "image.jpeg", - content_type: "image/jpeg", - # we don't want to run virus scanner on this file + io: File.open(path), + filename: "logo_test_procedure.png", + content_type: "image/png", 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 diff --git a/spec/fixtures/files/image-no-exif.jpg b/spec/fixtures/files/image-no-exif.jpg new file mode 100644 index 000000000..4ec032c2b Binary files /dev/null and b/spec/fixtures/files/image-no-exif.jpg differ diff --git a/spec/fixtures/files/image-no-rotation.jpg b/spec/fixtures/files/image-no-rotation.jpg new file mode 100644 index 000000000..07b5159a9 Binary files /dev/null and b/spec/fixtures/files/image-no-rotation.jpg differ diff --git a/spec/fixtures/files/image-rotated.jpg b/spec/fixtures/files/image-rotated.jpg new file mode 100644 index 000000000..2f58be2c8 Binary files /dev/null and b/spec/fixtures/files/image-rotated.jpg differ diff --git a/spec/jobs/image_processor_job_spec.rb b/spec/jobs/image_processor_job_spec.rb new file mode 100644 index 000000000..e32999273 --- /dev/null +++ b/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 diff --git a/spec/jobs/titre_identite_watermark_job_spec.rb b/spec/jobs/titre_identite_watermark_job_spec.rb deleted file mode 100644 index ebb9497cc..000000000 --- a/spec/jobs/titre_identite_watermark_job_spec.rb +++ /dev/null @@ -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 diff --git a/spec/services/auto_rotate_service_spec.rb b/spec/services/auto_rotate_service_spec.rb new file mode 100644 index 000000000..f8a007f76 --- /dev/null +++ b/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