From 00c905a615a61474467426e8ad30425c0f034002 Mon Sep 17 00:00:00 2001 From: Eric Leroy-Terquem Date: Wed, 3 Jul 2024 18:07:33 +0200 Subject: [PATCH 1/4] chore: generate variant for procedure logo in background --- app/controllers/procedures_controller.rb | 7 ++++++- app/models/attestation_template.rb | 3 ++- app/models/concerns/blob_image_processor_concern.rb | 6 +++++- app/models/procedure.rb | 7 +++---- 4 files changed, 16 insertions(+), 7 deletions(-) diff --git a/app/controllers/procedures_controller.rb b/app/controllers/procedures_controller.rb index 60fd044a0..b4037a4d8 100644 --- a/app/controllers/procedures_controller.rb +++ b/app/controllers/procedures_controller.rb @@ -5,7 +5,12 @@ class ProceduresController < ApplicationController def logo if @procedure.logo.attached? - redirect_to url_for(@procedure.logo.variant(:email)) + logo_variant = @procedure.logo.variant(resize_to_limit: [400, 400]) + if logo_variant.key.present? + redirect_to url_for(logo_variant.processed) + else + redirect_to url_for(@procedure.logo) + end else redirect_to ActionController::Base.helpers.image_url(PROCEDURE_DEFAULT_LOGO_SRC) end diff --git a/app/models/attestation_template.rb b/app/models/attestation_template.rb index 8d9fdfca4..8aba5d352 100644 --- a/app/models/attestation_template.rb +++ b/app/models/attestation_template.rb @@ -106,7 +106,8 @@ class AttestationTemplate < ApplicationRecord def logo_url if logo.attached? - Rails.application.routes.url_helpers.url_for(logo) + logo_variant = logo.variant(resize_to_limit: [400, 400]) + logo_variant.key.present? ? logo_variant.processed.url : Rails.application.routes.url_helpers.url_for(logo) end end diff --git a/app/models/concerns/blob_image_processor_concern.rb b/app/models/concerns/blob_image_processor_concern.rb index 8a7870371..63fac2365 100644 --- a/app/models/concerns/blob_image_processor_concern.rb +++ b/app/models/concerns/blob_image_processor_concern.rb @@ -10,7 +10,7 @@ module BlobImageProcessorConcern end def representation_required? - from_champ? || from_messagerie? + from_champ? || from_messagerie? || is_a_logo? end private @@ -23,6 +23,10 @@ module BlobImageProcessorConcern attachments.any? { _1.record.class == Commentaire } end + def logo? + attachments.any? { _1.name == 'logo' } + end + def watermark_required? attachments.any? { _1.record.class == Champs::TitreIdentiteChamp } end diff --git a/app/models/procedure.rb b/app/models/procedure.rb index ae167b266..9e8072fd4 100644 --- a/app/models/procedure.rb +++ b/app/models/procedure.rb @@ -176,9 +176,7 @@ class Procedure < ApplicationRecord belongs_to :defaut_groupe_instructeur, class_name: 'GroupeInstructeur', inverse_of: false, optional: true - has_one_attached :logo do |attachable| - attachable.variant :email, resize_to_limit: [450, 450] - end + has_one_attached :logo has_one_attached :notice has_one_attached :deliberation @@ -705,7 +703,8 @@ class Procedure < ApplicationRecord def logo_url if logo.attached? - Rails.application.routes.url_helpers.url_for(logo) + logo_variant = logo.variant(resize_to_limit: [400, 400]) + logo_variant.key.present? ? logo_variant.processed.url : Rails.application.routes.url_helpers.url_for(logo) else ActionController::Base.helpers.image_url(PROCEDURE_DEFAULT_LOGO_SRC) end From 0511a84abfde9be953a065169afa47d3a6ce5b2a Mon Sep 17 00:00:00 2001 From: Eric Leroy-Terquem Date: Thu, 4 Jul 2024 17:16:52 +0200 Subject: [PATCH 2/4] chore: move uninterlace to image processor job --- app/jobs/image_processor_job.rb | 11 ++++ app/services/uninterlace_service.rb | 23 ++++++++ spec/jobs/image_processor_job_spec.rb | 80 ++++++++++++++++++++++----- 3 files changed, 101 insertions(+), 13 deletions(-) create mode 100644 app/services/uninterlace_service.rb diff --git a/app/jobs/image_processor_job.rb b/app/jobs/image_processor_job.rb index 9efc16ec1..230db3344 100644 --- a/app/jobs/image_processor_job.rb +++ b/app/jobs/image_processor_job.rb @@ -22,6 +22,7 @@ class ImageProcessorJob < ApplicationJob 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) + uninterlace(blob) if blob.content_type == "image/png" create_representations(blob) if blob.representation_required? add_watermark(blob) if blob.watermark_pending? end @@ -40,6 +41,16 @@ class ImageProcessorJob < ApplicationJob end end + def uninterlace(blob) + blob.open do |file| + processed = UninterlaceService.new.process(file) + return if processed.blank? + + blob.upload(processed) + blob.save! + end + end + def create_representations(blob) blob.attachments.each do |attachment| next unless attachment&.representable? diff --git a/app/services/uninterlace_service.rb b/app/services/uninterlace_service.rb new file mode 100644 index 000000000..d4cef3aaf --- /dev/null +++ b/app/services/uninterlace_service.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +class UninterlaceService + def process(file) + uninterlace_png(file) + end + + private + + def uninterlace_png(uploaded_file) + if interlaced?(uploaded_file.to_path) + chunky_img = ChunkyPNG::Image.from_io(uploaded_file.to_io) + chunky_img.save(uploaded_file.to_path, interlace: false) + uploaded_file.reopen(uploaded_file.to_path, 'rb') + end + uploaded_file + end + + def interlaced?(png_path) + png = MiniMagick::Image.open(png_path) + png.data["interlace"] != "None" + end +end diff --git a/spec/jobs/image_processor_job_spec.rb b/spec/jobs/image_processor_job_spec.rb index cc6d4fd9a..78c5702a0 100644 --- a/spec/jobs/image_processor_job_spec.rb +++ b/spec/jobs/image_processor_job_spec.rb @@ -13,6 +13,7 @@ describe ImageProcessorJob, type: :job do let(:antivirus_pending) { false } let(:watermark_service) { instance_double("WatermarkService") } let(:auto_rotate_service) { instance_double("AutoRotateService") } + let(:uninterlace_service) { instance_double("UninterlaceService") } before do virus_scanner_mock = instance_double("ActiveStorage::VirusScanner", pending?: antivirus_pending) @@ -23,9 +24,6 @@ describe ImageProcessorJob, type: :job do 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 @@ -37,11 +35,33 @@ describe ImageProcessorJob, type: :job do end describe 'autorotate' do + 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 + + before do + allow(AutoRotateService).to receive(:new).and_return(auto_rotate_service) + allow(auto_rotate_service).to receive(:process).and_return(true) + end + context "when image is not a jpg" do - let(:rotated_file) { Tempfile.new("rotated.png") } + let(:file) { fixture_file_upload('spec/fixtures/files/uninterlaced-black.png', 'image/png') } before do - allow(rotated_file).to receive(:size).and_return(100) + allow(file).to receive(:size).and_return(100) end it "it does not process autorotate" do @@ -52,6 +72,7 @@ describe ImageProcessorJob, type: :job do context "when image is a jpg " do let(:rotated_file) { Tempfile.new("rotated.jpg") } + let(:file) { fixture_file_upload('spec/fixtures/files/image-rotated.jpg', 'image/jpeg') } before do allow(rotated_file).to receive(:size).and_return(100) @@ -118,34 +139,67 @@ describe ImageProcessorJob, type: :job do describe 'watermark' do context "when watermark is already done" do before do - allow(blob).to receive(:watermark_done?).and_return(true) + allow(blob_jpg).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) + described_class.perform_now(blob_jpg) end end context "when the blob is ready to be watermarked" do - let(:watermarked_file) { Tempfile.new("watermarked.png") } + let(:watermarked_file) { Tempfile.new("watermarked.jpg") } before do allow(watermarked_file).to receive(:size).and_return(100) - allow(blob).to receive(:watermark_pending?).and_return(true) + allow(blob_jpg).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) + described_class.perform_now(blob_jpg) }.to change { - blob.reload.checksum + blob_jpg.reload.checksum } - expect(blob.byte_size).to eq(100) - expect(blob.watermarked_at).to be_present + expect(blob_jpg.byte_size).to eq(100) + expect(blob_jpg.watermarked_at).to be_present + end + end + end + + describe 'uninterlace' do + 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 + + before do + allow(UninterlaceService).to receive(:new).and_return(uninterlace_service) + end + + context "when file is interlaced" do + let(:file) { fixture_file_upload('spec/fixtures/files/interlaced-black.png', 'image/png') } + let(:uninterlaced_file) { fixture_file_upload('spec/fixtures/files/uninterlaced-black.png', 'image/png') } + + it "it process uninterlace" do + expect(uninterlace_service).to receive(:process).and_return(uninterlaced_file) + described_class.perform_now(blob) end end end From 4d0961d3ec925993f1441d98a3c1ce276b015568 Mon Sep 17 00:00:00 2001 From: Eric Leroy-Terquem Date: Wed, 24 Jul 2024 11:10:24 +0200 Subject: [PATCH 3/4] chore: remove uninterlacing from web machines --- .../attestation_template_v2s_controller.rb | 13 ------------ .../attestation_templates_controller.rb | 12 ----------- .../groupe_instructeurs_controller.rb | 1 - .../groupe_instructeurs_signature_concern.rb | 4 +--- .../concerns/uninterlace_png_concern.rb | 21 ------------------- .../groupe_instructeurs_controller.rb | 1 - .../attestation_templates_controller_spec.rb | 2 -- 7 files changed, 1 insertion(+), 53 deletions(-) delete mode 100644 app/controllers/concerns/uninterlace_png_concern.rb diff --git a/app/controllers/administrateurs/attestation_template_v2s_controller.rb b/app/controllers/administrateurs/attestation_template_v2s_controller.rb index ce926e84b..e44198380 100644 --- a/app/controllers/administrateurs/attestation_template_v2s_controller.rb +++ b/app/controllers/administrateurs/attestation_template_v2s_controller.rb @@ -2,8 +2,6 @@ module Administrateurs class AttestationTemplateV2sController < AdministrateurController - include UninterlacePngConcern - before_action :retrieve_procedure before_action :ensure_feature_active before_action :retrieve_attestation_template @@ -74,17 +72,6 @@ module Administrateurs @attestation_template.procedure = @procedure end - logo_file = attestation_params.delete(:logo) - signature_file = attestation_params.delete(:signature) - - if logo_file - attestation_params[:logo] = uninterlace_png(logo_file) - end - - if signature_file - attestation_params[:signature] = uninterlace_png(signature_file) - end - @attestation_template.assign_attributes(attestation_params) if @attestation_template.invalid? diff --git a/app/controllers/administrateurs/attestation_templates_controller.rb b/app/controllers/administrateurs/attestation_templates_controller.rb index d703b99b2..96f611f54 100644 --- a/app/controllers/administrateurs/attestation_templates_controller.rb +++ b/app/controllers/administrateurs/attestation_templates_controller.rb @@ -2,8 +2,6 @@ module Administrateurs class AttestationTemplatesController < AdministrateurController - include UninterlacePngConcern - before_action :retrieve_procedure before_action :preload_revisions @@ -63,16 +61,6 @@ module Administrateurs if @activated_attestation_params.nil? @activated_attestation_params = params.require(:attestation_template) .permit(:title, :body, :footer, :activated, :logo, :signature) - - logo_file = params['attestation_template'].delete('logo') - signature_file = params['attestation_template'].delete('signature') - - if logo_file.present? - @activated_attestation_params[:logo] = uninterlace_png(logo_file) - end - if signature_file.present? - @activated_attestation_params[:signature] = uninterlace_png(signature_file) - end end @activated_attestation_params diff --git a/app/controllers/administrateurs/groupe_instructeurs_controller.rb b/app/controllers/administrateurs/groupe_instructeurs_controller.rb index 61225de77..b0fd71ff5 100644 --- a/app/controllers/administrateurs/groupe_instructeurs_controller.rb +++ b/app/controllers/administrateurs/groupe_instructeurs_controller.rb @@ -5,7 +5,6 @@ module Administrateurs include ActiveSupport::NumberHelper include EmailSanitizableConcern include Logic - include UninterlacePngConcern include GroupeInstructeursSignatureConcern before_action :ensure_not_super_admin!, only: [:add_instructeur] diff --git a/app/controllers/concerns/groupe_instructeurs_signature_concern.rb b/app/controllers/concerns/groupe_instructeurs_signature_concern.rb index e7c843a4a..4e2fee3c0 100644 --- a/app/controllers/concerns/groupe_instructeurs_signature_concern.rb +++ b/app/controllers/concerns/groupe_instructeurs_signature_concern.rb @@ -19,9 +19,7 @@ module GroupeInstructeursSignatureConcern flash[:alert] = "Aucun fichier joint pour le tampon de l'attestation" render :show else - signature = uninterlace_png(signature_file) - - if @groupe_instructeur.signature.attach(signature) + if @groupe_instructeur.signature.attach(signature_file) handle_redirect :success else handle_redirect :alert diff --git a/app/controllers/concerns/uninterlace_png_concern.rb b/app/controllers/concerns/uninterlace_png_concern.rb deleted file mode 100644 index ff82fc7ba..000000000 --- a/app/controllers/concerns/uninterlace_png_concern.rb +++ /dev/null @@ -1,21 +0,0 @@ -# frozen_string_literal: true - -module UninterlacePngConcern - extend ActiveSupport::Concern - - private - - def uninterlace_png(uploaded_file) - if uploaded_file&.content_type == 'image/png' && interlaced?(uploaded_file.tempfile.to_path) - chunky_img = ChunkyPNG::Image.from_io(uploaded_file.to_io) - chunky_img.save(uploaded_file.tempfile.to_path, interlace: false) - uploaded_file.tempfile.reopen(uploaded_file.tempfile.to_path, 'rb') - end - uploaded_file - end - - def interlaced?(png_path) - png = MiniMagick::Image.open(png_path) - png.data["interlace"] != "None" - end -end diff --git a/app/controllers/instructeurs/groupe_instructeurs_controller.rb b/app/controllers/instructeurs/groupe_instructeurs_controller.rb index 7fbf5dbd2..0c5a0671d 100644 --- a/app/controllers/instructeurs/groupe_instructeurs_controller.rb +++ b/app/controllers/instructeurs/groupe_instructeurs_controller.rb @@ -3,7 +3,6 @@ module Instructeurs class GroupeInstructeursController < InstructeurController include EmailSanitizableConcern - include UninterlacePngConcern include GroupeInstructeursSignatureConcern before_action :ensure_allowed! diff --git a/spec/controllers/administrateurs/attestation_templates_controller_spec.rb b/spec/controllers/administrateurs/attestation_templates_controller_spec.rb index f38e2fd06..1f6ef9243 100644 --- a/spec/controllers/administrateurs/attestation_templates_controller_spec.rb +++ b/spec/controllers/administrateurs/attestation_templates_controller_spec.rb @@ -8,8 +8,6 @@ describe Administrateurs::AttestationTemplatesController, type: :controller do let(:logo2) { fixture_file_upload('spec/fixtures/files/white.png', 'image/png') } let(:signature) { fixture_file_upload('spec/fixtures/files/black.png', 'image/png') } let(:signature2) { fixture_file_upload('spec/fixtures/files/black.png', 'image/png') } - let(:interlaced_logo) { fixture_file_upload('spec/fixtures/files/interlaced-black.png', 'image/png') } - let(:uninterlaced_logo) { fixture_file_upload('spec/fixtures/files/uninterlaced-black.png', 'image/png') } let(:invalid_logo) { fixture_file_upload('spec/fixtures/files/invalid_file_format.json', 'application/json') } before do From 37a018880b4d4add48e18ee7201e989ba907c515 Mon Sep 17 00:00:00 2001 From: Eric Leroy-Terquem Date: Thu, 29 Aug 2024 17:30:23 +0200 Subject: [PATCH 4/4] chore: generate representations coming from ActionText in background --- app/jobs/image_processor_job.rb | 3 ++ .../concerns/blob_image_processor_concern.rb | 6 +++- app/views/active_storage/blobs/_blob.html.erb | 29 +++++++++++-------- 3 files changed, 25 insertions(+), 13 deletions(-) diff --git a/app/jobs/image_processor_job.rb b/app/jobs/image_processor_job.rb index 230db3344..16b76f32d 100644 --- a/app/jobs/image_processor_job.rb +++ b/app/jobs/image_processor_job.rb @@ -58,6 +58,9 @@ class ImageProcessorJob < ApplicationJob if attachment.blob.content_type.in?(RARE_IMAGE_TYPES) attachment.variant(resize_to_limit: [2000, 2000]).processed end + if attachment.record.class == ActionText::RichText + attachment.variant(resize_to_limit: [1024, 768]).processed + end end end diff --git a/app/models/concerns/blob_image_processor_concern.rb b/app/models/concerns/blob_image_processor_concern.rb index 63fac2365..08d6f1067 100644 --- a/app/models/concerns/blob_image_processor_concern.rb +++ b/app/models/concerns/blob_image_processor_concern.rb @@ -10,7 +10,7 @@ module BlobImageProcessorConcern end def representation_required? - from_champ? || from_messagerie? || is_a_logo? + from_champ? || from_messagerie? || logo? || from_action_text? end private @@ -27,6 +27,10 @@ module BlobImageProcessorConcern attachments.any? { _1.name == 'logo' } end + def from_action_text? + attachments.any? { _1.record.class == ActionText::RichText } + end + def watermark_required? attachments.any? { _1.record.class == Champs::TitreIdentiteChamp } end diff --git a/app/views/active_storage/blobs/_blob.html.erb b/app/views/active_storage/blobs/_blob.html.erb index 49ba357dd..8e95058a4 100644 --- a/app/views/active_storage/blobs/_blob.html.erb +++ b/app/views/active_storage/blobs/_blob.html.erb @@ -1,14 +1,19 @@ -
attachment--<%= blob.filename.extension %>"> - <% if blob.representable? %> - <%= image_tag blob.representation(resize_to_limit: local_assigns[:in_gallery] ? [ 800, 600 ] : [ 1024, 768 ]) %> - <% end %> - -
- <% if caption = blob.try(:caption) %> - <%= caption %> +<% if blob.representable? %> + <% representation = blob.representation(resize_to_limit: local_assigns[:in_gallery] ? [ 800, 600 ] : [ 1024, 768 ]) %> + <% if representation.image&.attached? || representation.key.present? %> +
+ <%= image_tag representation %> <% else %> - <%= blob.filename %> - <%= number_to_human_size blob.byte_size %> +
+ <%= image_tag blob %> <% end %> -
-
+ +
+ <% if caption = blob.try(:caption) %> + <%= caption %> + <% else %> + <%= blob.filename %> + <%= number_to_human_size blob.byte_size %> + <% end %> +
+<% end %>