From ae5937b22afaf6e2f85a8610141198098c6eeb6f Mon Sep 17 00:00:00 2001 From: Colin Darie Date: Wed, 12 Jun 2024 16:44:46 +0200 Subject: [PATCH 1/2] chore(sidekiq): concise transition and avoid typos by not (re)opening classes --- config/initializers/transition_to_sidekiq.rb | 121 +++++-------------- 1 file changed, 27 insertions(+), 94 deletions(-) diff --git a/config/initializers/transition_to_sidekiq.rb b/config/initializers/transition_to_sidekiq.rb index 3b5360b6a..bfc07449b 100644 --- a/config/initializers/transition_to_sidekiq.rb +++ b/config/initializers/transition_to_sidekiq.rb @@ -1,99 +1,32 @@ if Rails.env.production? && SIDEKIQ_ENABLED ActiveSupport.on_load(:after_initialize) do - class ActiveStorage::PurgeJob < ActiveStorage::BaseJob - self.queue_adapter = :sidekiq - end - - class ActiveStorage::AnalyzeJob < ActiveStorage::BaseJob - self.queue_adapter = :sidekiq - end - - class VirusScannerJob - self.queue_adapter = :sidekiq - end - - class DossierRebaseJob < ApplicationJob - self.queue_adapter = :sidekiq - end - - class ProcedureExternalURLCheckJob < ApplicationJob - self.queue_adapter = :sidekiq - end - - class MaintenanceTasks::TaskJob - self.queue_adapter = :sidekiq - end - - class PriorizedMailDeliveryJob < ActionMailer::MailDeliveryJob - self.queue_adapter = :sidekiq - end - - class ProcedureSVASVRProcessDossierJob < ApplicationJob - self.queue_adapter = :sidekiq - end - - class WebHookJob < ApplicationJob - self.queue_adapter = :sidekiq - end - - class DestroyRecordLaterJob < ApplicationJob - self.queue_adapter = :sidekiq - end - - class ChampFetchExternalDataJob < ApplicationJob - self.queue_adapter = :sidekiq - end - - class DossierIndexSearchTermsJob < ApplicationJob - self.queue_adapter = :sidekiq - end - - class Migrations::BackfillStableIdJob - self.queue_adapter = :sidekiq - end - - class Cron::CronJob < ApplicationJob - self.queue_adapter = :sidekiq - end - - class APIEntreprise::Job < ApplicationJob - self.queue_adapter = :sidekiq - end - - class DossierOperationLogMoveToColdStorageBatchJob < ApplicationJob - self.queue_adapter = :sidekiq - end - - class BatchOperationEnqueueAllJob < ApplicationJob - self.queue_adapter = :sidekiq - end - - class BatchOperationProcessOneJob < ApplicationJob - self.queue_adapter = :sidekiq - end - - class TitreIdentiteWatermarkJob < ApplicationJob - self.queue_adapter = :sidekiq - end - - class AdminUpdateDefaultZonesJob < ApplicationJob - self.queue_adapter = :sidekiq - end - - class ProcessStalledDeclarativeDossierJob < ApplicationJob - self.queue_adapter = :sidekiq - end - - class ResetExpiringDossiersJob < ApplicationJob - self.queue_adapter = :sidekiq - end - - class SendClosingNotificationJob < ApplicationJob - self.queue_adapter = :sidekiq - end - - class ImageProcessorJob < ApplicationJob - self.queue_adapter = :sidekiq + [ + ActiveStorage::AnalyzeJob, + ActiveStorage::PurgeJob, + AdminUpdateDefaultZonesJob, + APIEntreprise::Job, + AdminUpdateDefaultZonesJob, + BatchOperationEnqueueAllJob, + BatchOperationProcessOneJob, + ChampFetchExternalDataJob, + Cron::CronJob, + DestroyRecordLaterJob, + DossierIndexSearchTermsJob, + DossierOperationLogMoveToColdStorageBatchJob, + DossierRebaseJob, + ImageProcessorJob, + MaintenanceTasks::TaskJob, + Migrations::BackfillStableIdJob, + PriorizedMailDeliveryJob, + ProcedureExternalURLCheckJob, + ProcedureSVASVRProcessDossierJob, + ProcessStalledDeclarativeDossierJob, + ResetExpiringDossiersJob, + SendClosingNotificationJob, + VirusScannerJob, + WebHookJob + ].each do |job_class| + job_class.queue_adapter = :sidekiq end end end From a1469b04fe10d1537449c6785fb530ff69baa47b Mon Sep 17 00:00:00 2001 From: Colin Darie Date: Thu, 13 Jun 2024 13:27:32 +0200 Subject: [PATCH 2/2] refactor(support): create HS conversation in async and run virus scanner on attachments --- app/controllers/support_controller.rb | 41 ++++++------ app/jobs/helpscout_create_conversation_job.rb | 21 ++++++ app/lib/helpscout/api.rb | 14 ++-- app/lib/helpscout/form_adapter.rb | 4 +- config/initializers/transition_to_sidekiq.rb | 1 + spec/controllers/support_controller_spec.rb | 30 ++++++--- .../helpscout_create_conversation_job_spec.rb | 64 +++++++++++++++++++ spec/lib/helpscout/form_adapter_spec.rb | 2 +- 8 files changed, 138 insertions(+), 39 deletions(-) create mode 100644 app/jobs/helpscout_create_conversation_job.rb create mode 100644 spec/jobs/helpscout_create_conversation_job_spec.rb diff --git a/app/controllers/support_controller.rb b/app/controllers/support_controller.rb index 3951e2658..e2e536499 100644 --- a/app/controllers/support_controller.rb +++ b/app/controllers/support_controller.rb @@ -14,24 +14,16 @@ class SupportController < ApplicationController flash.notice = "Votre message a été envoyé sur la messagerie de votre dossier." redirect_to messagerie_dossier_path(dossier) - elsif create_conversation - flash.notice = "Votre message a été envoyé." + return + end - if params[:admin] - redirect_to root_path(formulaire_contact_admin_submitted: true) - else - redirect_to root_path(formulaire_contact_general_submitted: true) - end + create_conversation_later + flash.notice = "Votre message a été envoyé." + + if params[:admin] + redirect_to root_path(formulaire_contact_admin_submitted: true) else - flash.now.alert = "Une erreur est survenue. Vous pouvez nous contacter à #{helpers.mail_to(Current.contact_email)}." - - if params[:admin] - setup_context_admin - render :admin - else - setup_context - render :index - end + redirect_to root_path(formulaire_contact_general_submitted: true) end end @@ -48,17 +40,26 @@ class SupportController < ApplicationController @options = Helpscout::FormAdapter.admin_options end - def create_conversation - Helpscout::FormAdapter.new( + def create_conversation_later + if params[:piece_jointe] + blob = ActiveStorage::Blob.create_and_upload!( + io: params[:piece_jointe].tempfile, + filename: params[:piece_jointe].original_filename, + content_type: params[:piece_jointe].content_type, + identify: false + ).tap(&:scan_for_virus_later) + end + + HelpscoutCreateConversationJob.perform_later( + blob_id: blob&.id, subject: params[:subject], email: email, phone: params[:phone], text: params[:text], - file: params[:piece_jointe], dossier_id: dossier&.id, browser: browser_name, tags: tags - ).send_form + ) end def create_commentaire diff --git a/app/jobs/helpscout_create_conversation_job.rb b/app/jobs/helpscout_create_conversation_job.rb new file mode 100644 index 000000000..a8175d029 --- /dev/null +++ b/app/jobs/helpscout_create_conversation_job.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +class HelpscoutCreateConversationJob < ApplicationJob + queue_as :default + + class FileNotScannedYetError < StandardError + end + + retry_on FileNotScannedYetError, wait: :exponentially_longer, attempts: 10 + + def perform(blob_id: nil, **args) + if blob_id.present? + blob = ActiveStorage::Blob.find(blob_id) + raise FileNotScannedYetError if blob.virus_scanner.pending? + + blob = nil unless blob.virus_scanner.safe? + end + + Helpscout::FormAdapter.new(**args, blob:).send_form + end +end diff --git a/app/lib/helpscout/api.rb b/app/lib/helpscout/api.rb index c0538c7a9..b2f72d85c 100644 --- a/app/lib/helpscout/api.rb +++ b/app/lib/helpscout/api.rb @@ -22,7 +22,7 @@ class Helpscout::API }) end - def create_conversation(email, subject, text, file) + def create_conversation(email, subject, text, blob) body = { subject: subject, customer: customer(email), @@ -34,7 +34,7 @@ class Helpscout::API type: 'customer', customer: customer(email), text: text, - attachments: attachments(file) + attachments: attachments(blob) } ] }.compact @@ -76,13 +76,13 @@ class Helpscout::API private - def attachments(file) - if file.present? + def attachments(blob) + if blob.present? [ { - fileName: file.original_filename, - mimeType: file.content_type, - data: Base64.strict_encode64(file.read) + fileName: blob.filename, + mimeType: blob.content_type, + data: Base64.strict_encode64(blob.download) } ] else diff --git a/app/lib/helpscout/form_adapter.rb b/app/lib/helpscout/form_adapter.rb index 03c168f08..4127e9dc2 100644 --- a/app/lib/helpscout/form_adapter.rb +++ b/app/lib/helpscout/form_adapter.rb @@ -66,7 +66,7 @@ class Helpscout::FormAdapter params[:email], params[:subject], params[:text], - params[:file] + params[:blob] ) if response.success? @@ -74,6 +74,8 @@ class Helpscout::FormAdapter @api.add_phone_number(params[:email], params[:phone]) end response.headers['Resource-ID'] + else + raise StandardError, "Error while creating conversation: #{response.response_code} '#{response.body}'" end end end diff --git a/config/initializers/transition_to_sidekiq.rb b/config/initializers/transition_to_sidekiq.rb index bfc07449b..28e9075e7 100644 --- a/config/initializers/transition_to_sidekiq.rb +++ b/config/initializers/transition_to_sidekiq.rb @@ -14,6 +14,7 @@ if Rails.env.production? && SIDEKIQ_ENABLED DossierIndexSearchTermsJob, DossierOperationLogMoveToColdStorageBatchJob, DossierRebaseJob, + HelpscoutCreateConversationJob, ImageProcessorJob, MaintenanceTasks::TaskJob, Migrations::BackfillStableIdJob, diff --git a/spec/controllers/support_controller_spec.rb b/spec/controllers/support_controller_spec.rb index d7275991d..281c7e4a3 100644 --- a/spec/controllers/support_controller_spec.rb +++ b/spec/controllers/support_controller_spec.rb @@ -58,9 +58,9 @@ describe SupportController, type: :controller do let(:params) { { subject: 'bonjour', text: 'un message' } } it 'creates a conversation on HelpScout' do - expect_any_instance_of(Helpscout::FormAdapter).to receive(:send_form).and_return(true) - - expect { subject }.to change(Commentaire, :count).by(0) + expect { subject }.to \ + change(Commentaire, :count).by(0).and \ + have_enqueued_job(HelpscoutCreateConversationJob).with(hash_including(params)) expect(flash[:notice]).to match('Votre message a été envoyé.') expect(response).to redirect_to root_path(formulaire_contact_general_submitted: true) @@ -80,9 +80,9 @@ describe SupportController, type: :controller do end it 'creates a conversation on HelpScout' do - expect_any_instance_of(Helpscout::FormAdapter).to receive(:send_form).and_return(true) - - expect { subject }.to change(Commentaire, :count).by(0) + expect { subject }.to \ + change(Commentaire, :count).by(0).and \ + have_enqueued_job(HelpscoutCreateConversationJob).with(hash_including(subject: 'bonjour', dossier_id: dossier.id)) expect(flash[:notice]).to match('Votre message a été envoyé.') expect(response).to redirect_to root_path(formulaire_contact_general_submitted: true) @@ -103,9 +103,8 @@ describe SupportController, type: :controller do end it 'posts the message to the dossier messagerie' do - expect_any_instance_of(Helpscout::FormAdapter).not_to receive(:send_form) - expect { subject }.to change(Commentaire, :count).by(1) + assert_no_enqueued_jobs(only: HelpscoutCreateConversationJob) expect(Commentaire.last.email).to eq(user.email) expect(Commentaire.last.dossier).to eq(dossier) @@ -159,10 +158,21 @@ describe SupportController, type: :controller do describe "when form is filled" do it "creates a conversation on HelpScout" do - expect_any_instance_of(Helpscout::FormAdapter).to receive(:send_form).and_return(true) - subject + expect { subject }.to have_enqueued_job(HelpscoutCreateConversationJob).with(hash_including(params.except(:admin))) expect(flash[:notice]).to match('Votre message a été envoyé.') end + + context "with a piece justificative" do + let(:logo) { fixture_file_upload('spec/fixtures/files/white.png', 'image/png') } + let(:params) { super().merge(piece_jointe: logo) } + + it "create blob and pass it to conversation job" do + expect { subject }.to \ + change(ActiveStorage::Blob, :count).by(1).and \ + have_enqueued_job(HelpscoutCreateConversationJob).with(hash_including(blob_id: Integer)).and \ + have_enqueued_job(VirusScannerJob) + end + end end describe "when invisible captcha is filled" do diff --git a/spec/jobs/helpscout_create_conversation_job_spec.rb b/spec/jobs/helpscout_create_conversation_job_spec.rb new file mode 100644 index 000000000..1220e538d --- /dev/null +++ b/spec/jobs/helpscout_create_conversation_job_spec.rb @@ -0,0 +1,64 @@ +require 'rails_helper' + +RSpec.describe HelpscoutCreateConversationJob, type: :job do + let(:args) { { email: 'sender@email.com' } } + + describe '#perform' do + context 'when blob_id is not present' do + it 'sends the form without a file' do + form_adapter = double('Helpscout::FormAdapter') + allow(Helpscout::FormAdapter).to receive(:new).with(hash_including(args.merge(blob: nil))).and_return(form_adapter) + expect(form_adapter).to receive(:send_form) + + described_class.perform_now(**args) + end + end + + context 'when blob_id is present' do + let(:blob) { + ActiveStorage::Blob.create_and_upload!(io: StringIO.new("toto"), filename: "toto.png") + } + + before do + allow(blob).to receive(:virus_scanner).and_return(double('VirusScanner', pending?: pending, safe?: safe)) + end + + context 'when the file has not been scanned yet' do + let(:pending) { true } + let(:safe) { false } + + it 'reenqueue job' do + expect { + described_class.perform_now(blob_id: blob.id, **args) + }.to have_enqueued_job(described_class).with(blob_id: blob.id, **args) + end + end + + context 'when the file is safe' do + let(:pending) { false } + let(:safe) { true } + + it 'downloads the file and sends the form' do + form_adapter = double('Helpscout::FormAdapter') + allow(Helpscout::FormAdapter).to receive(:new).with(hash_including(args.merge(blob:))).and_return(form_adapter) + allow(form_adapter).to receive(:send_form) + + described_class.perform_now(blob_id: blob.id, **args) + end + end + + context 'when the file is not safe' do + let(:pending) { false } + let(:safe) { false } + + it 'downloads the file and sends the form' do + form_adapter = double('Helpscout::FormAdapter') + allow(Helpscout::FormAdapter).to receive(:new).with(hash_including(args.merge(blob: nil))).and_return(form_adapter) + allow(form_adapter).to receive(:send_form) + + described_class.perform_now(blob_id: blob.id, **args) + end + end + end + end +end diff --git a/spec/lib/helpscout/form_adapter_spec.rb b/spec/lib/helpscout/form_adapter_spec.rb index d2e146cab..8eaa085a6 100644 --- a/spec/lib/helpscout/form_adapter_spec.rb +++ b/spec/lib/helpscout/form_adapter_spec.rb @@ -5,7 +5,7 @@ describe Helpscout::FormAdapter do context 'create_conversation' do before do allow(api).to receive(:create_conversation) - .and_return(double(success?: false)) + .and_return(double(success?: true, headers: {})) described_class.new(params, api).send_form end