From 17f5fb4a518cc741cccf4a51558c2f181d724ed8 Mon Sep 17 00:00:00 2001 From: Colin Darie Date: Tue, 13 Jun 2023 16:23:40 +0200 Subject: [PATCH 1/2] refactor(declarative): process dossier in job isolation --- .../stalled_declarative_procedures_job.rb | 11 +- ...process_stalled_declarative_dossier_job.rb | 20 +++ app/models/procedure.rb | 17 --- ...stalled_declarative_procedures_job_spec.rb | 141 ++++-------------- ...ss_stalled_declarative_dossier_job_spec.rb | 105 +++++++++++++ 5 files changed, 160 insertions(+), 134 deletions(-) create mode 100644 app/jobs/process_stalled_declarative_dossier_job.rb create mode 100644 spec/jobs/process_stalled_declarative_dossier_job_spec.rb diff --git a/app/jobs/cron/stalled_declarative_procedures_job.rb b/app/jobs/cron/stalled_declarative_procedures_job.rb index c713bac0e..9e4e60882 100644 --- a/app/jobs/cron/stalled_declarative_procedures_job.rb +++ b/app/jobs/cron/stalled_declarative_procedures_job.rb @@ -1,13 +1,10 @@ class Cron::StalledDeclarativeProceduresJob < Cron::CronJob - self.schedule_expression = "every 10 minute" + self.schedule_expression = "every 10 minutes" - def perform(*args) + def perform Procedure.declarative.find_each do |procedure| - begin - procedure.process_stalled_dossiers! - rescue => e - Sentry.set_tags(procedure: procedure.id) - Sentry.capture_exception(e) + procedure.dossiers.state_en_construction.where(declarative_triggered_at: nil).find_each do |dossier| + ProcessStalledDeclarativeDossierJob.perform_later(dossier) end end end diff --git a/app/jobs/process_stalled_declarative_dossier_job.rb b/app/jobs/process_stalled_declarative_dossier_job.rb new file mode 100644 index 000000000..cead75d26 --- /dev/null +++ b/app/jobs/process_stalled_declarative_dossier_job.rb @@ -0,0 +1,20 @@ +class ProcessStalledDeclarativeDossierJob < ApplicationJob + def perform(dossier) + return if dossier.declarative_triggered_at.present? + + case dossier.procedure.declarative_with_state + when Procedure.declarative_with_states.fetch(:en_instruction) + if !dossier.en_instruction? + dossier.passer_automatiquement_en_instruction! + end + when Procedure.declarative_with_states.fetch(:accepte) + if dossier.may_accepter_automatiquement? + dossier.accepter_automatiquement! + end + end + end + + def max_attempts + 3 # this job is enqueued by a cron, so it's better to not retry too much + end +end diff --git a/app/models/procedure.rb b/app/models/procedure.rb index 99d90149e..17372010d 100644 --- a/app/models/procedure.rb +++ b/app/models/procedure.rb @@ -490,23 +490,6 @@ class Procedure < ApplicationRecord end end - def process_stalled_dossiers! - case declarative_with_state - when Procedure.declarative_with_states.fetch(:en_instruction) - dossiers - .state_en_construction - .where(declarative_triggered_at: nil) - .find_each(&:passer_automatiquement_en_instruction!) - when Procedure.declarative_with_states.fetch(:accepte) - dossiers - .state_en_construction - .where(declarative_triggered_at: nil) - .find_each do |dossier| - dossier.accepter_automatiquement! if dossier.can_accepter_automatiquement? - end - end - end - def feature_enabled?(feature) Flipper.enabled?(feature, self) end diff --git a/spec/jobs/cron/stalled_declarative_procedures_job_spec.rb b/spec/jobs/cron/stalled_declarative_procedures_job_spec.rb index 2d2f359aa..2b2a6e593 100644 --- a/spec/jobs/cron/stalled_declarative_procedures_job_spec.rb +++ b/spec/jobs/cron/stalled_declarative_procedures_job_spec.rb @@ -1,126 +1,47 @@ RSpec.describe Cron::StalledDeclarativeProceduresJob, type: :job do describe "perform" do - let(:date) { Time.utc(2017, 9, 1, 10, 5, 0) } - let(:instruction_date) { date + 120 } - let(:state) { nil } let(:procedure) { create(:procedure, :published, :for_individual, :with_instructeur, declarative_with_state: state) } - let(:nouveau_dossier1) { create(:dossier, :en_construction, :with_individual, :with_attestation, procedure: procedure) } - let(:nouveau_dossier2) { create(:dossier, :en_construction, :with_individual, :with_attestation, procedure: procedure) } - let(:dossier_recu) { create(:dossier, :en_instruction, :with_individual, procedure: procedure) } - let(:dossier_brouillon) { create(:dossier, procedure: procedure) } - let(:dossier_repasse_en_construction) { create(:dossier, :en_construction, :with_individual, procedure: procedure) } - - before do - Timecop.freeze(date) - dossier_repasse_en_construction&.touch(:declarative_triggered_at) - end + let!(:brouillon) { create(:dossier, :brouillon, procedure:) } + let!(:en_construction) { create(:dossier, :en_construction, :with_individual, :with_attestation, procedure:) } + let!(:en_construction_triggered) { create(:dossier, :en_construction, :with_individual, :with_attestation, procedure:, declarative_triggered_at: 1.minute.ago) } + let!(:en_instruction) { create(:dossier, :en_instruction, :with_individual, procedure:) } subject(:perform_job) do - dossiers = [ - nouveau_dossier1, - nouveau_dossier2, - dossier_recu, - dossier_brouillon, - dossier_repasse_en_construction - ].compact - - Cron::StalledDeclarativeProceduresJob.new.perform - - dossiers.each(&:reload) + described_class.perform_now end - after { Timecop.return } + context "declarative en instruction" do + let(:state) { Dossier.states.fetch(:en_instruction) } - context "with some dossiers" do - context "en_construction" do - let(:state) { Dossier.states.fetch(:en_instruction) } - let(:last_operation) { nouveau_dossier1.dossier_operation_logs.last } - - it { - perform_job - expect(nouveau_dossier1.en_instruction?).to be_truthy - expect(nouveau_dossier1.en_instruction_at).to eq(date) - expect(last_operation.operation).to eq('passer_en_instruction') - - expect(last_operation.automatic_operation?).to be_truthy - - expect(nouveau_dossier2.en_instruction?).to be_truthy - expect(nouveau_dossier2.en_instruction_at).to eq(date) - - expect(dossier_recu.en_instruction?).to be_truthy - expect(dossier_recu.en_instruction_at).to eq(instruction_date) - - expect(dossier_brouillon.brouillon?).to be_truthy - expect(dossier_brouillon.en_instruction_at).to eq(nil) - - expect(dossier_repasse_en_construction.en_construction?).to be_truthy - } - end - - context "accepte" do - let(:state) { Dossier.states.fetch(:accepte) } - let(:last_operation) { nouveau_dossier1.dossier_operation_logs.last } - - it { - perform_job - expect(nouveau_dossier1.accepte?).to be true - expect(nouveau_dossier1.en_instruction_at).to eq(date) - expect(nouveau_dossier1.processed_at).to eq(date) - expect(nouveau_dossier1.attestation).to be_present - expect(last_operation.operation).to eq('accepter') - expect(last_operation.automatic_operation?).to be_truthy - - expect(nouveau_dossier2.accepte?).to be true - expect(nouveau_dossier2.en_instruction_at).to eq(date) - expect(nouveau_dossier2.processed_at).to eq(date) - expect(nouveau_dossier2.attestation).to be_present - - expect(dossier_recu.en_instruction?).to be true - expect(dossier_recu.en_instruction_at).to eq(instruction_date) - expect(dossier_recu.processed_at).to eq(nil) - - expect(dossier_brouillon.brouillon?).to be true - expect(dossier_brouillon.en_instruction_at).to eq(nil) - expect(dossier_brouillon.processed_at).to eq(nil) - } - - context "having etablissement in degraded_mode" do - let(:procedure) { create(:procedure, :published, :with_instructeur, for_individual: false, declarative_with_state: state) } - - let(:nouveau_dossier1) { create(:dossier, :en_construction, :with_entreprise, :with_attestation, procedure: procedure, as_degraded_mode: false) } - let(:nouveau_dossier2) { create(:dossier, :en_construction, :with_entreprise, :with_attestation, procedure: procedure, as_degraded_mode: true) } - let(:dossier_recu) { nil } - let(:dossier_repasse_en_construction) { nil } - - before do - expect(nouveau_dossier2).to_not receive(:accepter_automatiquement!) - expect(Sentry).to_not receive(:capture_exception) - end - - it { - perform_job - expect(nouveau_dossier1).to be_accepte - expect(nouveau_dossier2).to be_en_construction - } - end - end + it { + perform_job + expect(ProcessStalledDeclarativeDossierJob).not_to have_been_enqueued.with(brouillon) + expect(ProcessStalledDeclarativeDossierJob).to have_been_enqueued.with(en_construction) + expect(ProcessStalledDeclarativeDossierJob).not_to have_been_enqueued.with(en_construction_triggered) + expect(ProcessStalledDeclarativeDossierJob).not_to have_been_enqueued.with(en_instruction) + } end - end - describe 'safer perform' do - let(:state) { Dossier.states.fetch(:en_instruction) } + context "declarative accepte" do + let(:state) { Dossier.states.fetch(:accepte) } - it 'works no matter if one raise' do - procedure_1 = instance_double("Procedure", id: 1) - expect(procedure_1).to receive(:process_stalled_dossiers!) - procedure_2 = instance_double("Procedure", id: 2) - expect(procedure_2).to receive(:process_stalled_dossiers!).and_raise("boom") - procedure_3 = double(process_stalled_dossiers!: true) - expect(procedure_3).to receive(:process_stalled_dossiers!) + it { + perform_job + expect(ProcessStalledDeclarativeDossierJob).not_to have_been_enqueued.with(brouillon) + expect(ProcessStalledDeclarativeDossierJob).to have_been_enqueued.with(en_construction) + expect(ProcessStalledDeclarativeDossierJob).not_to have_been_enqueued.with(en_construction_triggered) + expect(ProcessStalledDeclarativeDossierJob).not_to have_been_enqueued.with(en_instruction) + } + end - expect(Procedure).to receive_message_chain(:declarative, :find_each).and_yield(procedure_1).and_yield(procedure_2).and_yield(procedure_3) - Cron::StalledDeclarativeProceduresJob.perform_now + context "not declarative" do + let(:state) { nil } + + it { + perform_job + expect(ProcessStalledDeclarativeDossierJob).not_to have_been_enqueued + } end end end diff --git a/spec/jobs/process_stalled_declarative_dossier_job_spec.rb b/spec/jobs/process_stalled_declarative_dossier_job_spec.rb new file mode 100644 index 000000000..a5eeaa60c --- /dev/null +++ b/spec/jobs/process_stalled_declarative_dossier_job_spec.rb @@ -0,0 +1,105 @@ +RSpec.describe ProcessStalledDeclarativeDossierJob, type: :job do + include ActiveSupport::Testing::TimeHelpers + + describe "perform" do + let(:procedure) { create(:procedure, :published, :for_individual, :with_instructeur, declarative_with_state: state) } + let(:last_operation) { dossier.dossier_operation_logs.last } + + subject(:perform_job) do + described_class.perform_now(dossier) + end + + before do + freeze_time + perform_job + dossier.reload + end + + context 'declarative en instruction' do + let(:state) { Dossier.states.fetch(:en_instruction) } + + context 'dossier en_construction' do + let(:dossier) { create(:dossier, :en_construction, :with_individual, :with_attestation, procedure:) } + + it { + expect(dossier.state).to eq('en_instruction') + expect(dossier.en_instruction_at).to eq(Time.current) + expect(last_operation.operation).to eq('passer_en_instruction') + expect(last_operation.automatic_operation?).to be_truthy + } + + context 'dossier repasse en_construction' do + let(:dossier) { create(:dossier, :en_construction, :with_individual, procedure:, declarative_triggered_at: 1.day.ago) } + + it { expect(dossier.state).to eq('en_construction') } + end + end + + context 'dossier already en_instruction' do + let(:dossier) { create(:dossier, :en_instruction, :with_individual, procedure:, en_instruction_at: 2.days.ago) } + + it { + expect(dossier.state).to eq('en_instruction') + expect(dossier.en_instruction_at).to eq(2.days.ago) + expect(dossier.processed_at).to be_nil + } + end + end + + context "declarative accepte" do + let(:state) { Dossier.states.fetch(:accepte) } + + context 'dossier en_construction' do + let(:dossier) { create(:dossier, :en_construction, :with_individual, :with_attestation, procedure:) } + + it { + expect(dossier.state).to eq('accepte') + expect(dossier.en_instruction_at).to eq(Time.current) + expect(dossier.processed_at).to eq(Time.current) + expect(dossier.attestation).to be_present + expect(last_operation.operation).to eq('accepter') + expect(last_operation.automatic_operation?).to be_truthy + } + end + + context 'dossier en_instruction' do + let(:dossier) { create(:dossier, :en_instruction, :with_individual, procedure:, en_instruction_at: 2.days.ago) } + + it { + expect(dossier.state).to eq('en_instruction') + expect(dossier.en_instruction_at).to eq(2.days.ago) + expect(dossier.processed_at).to be_nil + } + end + + context 'dossier brouillon' do + let(:dossier) { create(:dossier, :brouillon) } + + it { + expect(dossier.state).to eq('brouillon') + expect(dossier.en_instruction_at).to be_nil + expect(dossier.processed_at).to be_nil + } + end + + context 'for entreprise' do + let(:procedure) { create(:procedure, :published, :with_instructeur, for_individual: false, declarative_with_state: state) } + + let(:dossier) { create(:dossier, :en_construction, :with_entreprise, :with_attestation, procedure:, as_degraded_mode: false) } + + it { expect(dossier).to be_accepte } + + context 'having etablissement in degraded_mode' do + let(:dossier) { create(:dossier, :en_construction, :with_entreprise, :with_attestation, procedure:, as_degraded_mode: true) } + + before do + expect(dossier).to_not receive(:accepter_automatiquement!) + expect(Sentry).to_not receive(:capture_exception) + end + + it { expect(dossier).to be_en_construction } + end + end + end + end +end From 3939f85860229a8b05f6f2f34de6cfcc05366317 Mon Sep 17 00:00:00 2001 From: Colin Darie Date: Tue, 13 Jun 2023 16:27:16 +0200 Subject: [PATCH 2/2] refactor(job): wrap sentry tags in a single before_perform --- app/jobs/application_job.rb | 11 +++++++++++ app/jobs/archive_creation_job.rb | 4 ---- app/jobs/export_job.rb | 4 ---- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/app/jobs/application_job.rb b/app/jobs/application_job.rb index cd19f09c6..f9c3a8b34 100644 --- a/app/jobs/application_job.rb +++ b/app/jobs/application_job.rb @@ -5,6 +5,17 @@ class ApplicationJob < ActiveJob::Base attr_writer :request_id + before_perform do |job| + arg = job.arguments.first + + case arg + when Dossier + Sentry.set_tags(dossier: arg.id, procedure: arg.procedure.id) + when Procedure + Sentry.set_tags(procedure: arg.id) + end + end + around_perform do |job, block| Rails.logger.info("#{job.class.name} started at #{Time.zone.now}") Current.set(request_id: job.request_id) do diff --git a/app/jobs/archive_creation_job.rb b/app/jobs/archive_creation_job.rb index 7c00a60f3..661996ffb 100644 --- a/app/jobs/archive_creation_job.rb +++ b/app/jobs/archive_creation_job.rb @@ -1,10 +1,6 @@ class ArchiveCreationJob < ApplicationJob queue_as :archives - before_perform do |job| - Sentry.set_tags(procedure: job.arguments.first.id) - end - def max_run_time Archive::MAX_DUREE_GENERATION end diff --git a/app/jobs/export_job.rb b/app/jobs/export_job.rb index 86ce5fefb..0ea91c844 100644 --- a/app/jobs/export_job.rb +++ b/app/jobs/export_job.rb @@ -3,10 +3,6 @@ class ExportJob < ApplicationJob discard_on ActiveRecord::RecordNotFound - before_perform do |job| - Sentry.set_tags(procedure: job.arguments.first.procedure.id) - end - def max_run_time Export::MAX_DUREE_GENERATION end