From c7a307553c2c334904fa2caedf387bf9a1a000eb Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Thu, 19 Dec 2019 13:22:40 +0100 Subject: [PATCH 1/2] helpers: allow `dossier_display_state` to take a state as input This allow to use either: - `dossier_display_state(dossier)` - `dossier_display_state(:en_construction)` --- app/helpers/dossier_helper.rb | 7 ++++--- spec/helpers/dossier_helper_spec.rb | 31 +++++++---------------------- 2 files changed, 11 insertions(+), 27 deletions(-) diff --git a/app/helpers/dossier_helper.rb b/app/helpers/dossier_helper.rb index e279f199b..f4dabce51 100644 --- a/app/helpers/dossier_helper.rb +++ b/app/helpers/dossier_helper.rb @@ -47,9 +47,10 @@ module DossierHelper dossier.brouillon? && dossier.procedure.close? end - def dossier_display_state(dossier, lower: false) - state = I18n.t(dossier.state, scope: [:activerecord, :attributes, :dossier, :state]) - lower ? state.downcase : state + def dossier_display_state(dossier_or_state, lower: false) + state = dossier_or_state.is_a?(Dossier) ? dossier_or_state.state : dossier_or_state + display_state = I18n.t(state, scope: [:activerecord, :attributes, :dossier, :state]) + lower ? display_state.downcase : display_state end def dossier_legacy_state(dossier) diff --git a/spec/helpers/dossier_helper_spec.rb b/spec/helpers/dossier_helper_spec.rb index e327ca095..2296235f9 100644 --- a/spec/helpers/dossier_helper_spec.rb +++ b/spec/helpers/dossier_helper_spec.rb @@ -132,37 +132,20 @@ RSpec.describe DossierHelper, type: :helper do expect(subject).to eq('Refusé') end - context "lower: true" do + context 'when requesting lowercase' do subject { dossier_display_state(dossier, lower: true) } - it 'brouillon is brouillon' do + it 'lowercases the display name' do dossier.brouillon! expect(subject).to eq('brouillon') end + end - it 'en_construction is En construction' do - dossier.en_construction! - expect(subject).to eq('en construction') - end + context 'when providing directly a state name' do + subject { dossier_display_state(:brouillon) } - it 'accepte is traité' do - dossier.accepte! - expect(subject).to eq('accepté') - end - - it 'en_instruction is reçu' do - dossier.en_instruction! - expect(subject).to eq('en instruction') - end - - it 'sans_suite is traité' do - dossier.sans_suite! - expect(subject).to eq('sans suite') - end - - it 'refuse is traité' do - dossier.refuse! - expect(subject).to eq('refusé') + it 'generates a display name for the given state' do + expect(subject).to eq('Brouillon') end end end From 8eb295d67e32af510d9658103b4eb6d6b51de3cf Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Thu, 19 Dec 2019 13:25:32 +0100 Subject: [PATCH 2/2] dossiers: avoid exceptions on invalid AASM transitions When attempting an invalid transition on a dossier, provide a meaningful error message (instead of letting an `AASM::InvalidTransition` exception propagate). This handles not only the case where the same state is applied twice (which was already handled manually), but all cases where the transition is invalid. --- .../instructeurs/dossiers_controller.rb | 35 ++++++++++++------- .../instructeurs/dossiers_controller_spec.rb | 29 ++++++++++++--- 2 files changed, 47 insertions(+), 17 deletions(-) diff --git a/app/controllers/instructeurs/dossiers_controller.rb b/app/controllers/instructeurs/dossiers_controller.rb index e02d91a49..caa08f48a 100644 --- a/app/controllers/instructeurs/dossiers_controller.rb +++ b/app/controllers/instructeurs/dossiers_controller.rb @@ -88,33 +88,33 @@ module Instructeurs end def passer_en_instruction - if dossier.en_instruction? - flash.notice = 'Le dossier est déjà en instruction.' - else + begin dossier.passer_en_instruction!(current_instructeur) flash.notice = 'Dossier passé en instruction.' + rescue AASM::InvalidTransition => e + flash.alert = aasm_error_message(e, target_state: :en_instruction) end render partial: 'state_button_refresh', locals: { dossier: dossier } end def repasser_en_construction - if dossier.en_construction? - flash.notice = 'Le dossier est déjà en construction.' - else + begin dossier.repasser_en_construction!(current_instructeur) flash.notice = 'Dossier repassé en construction.' + rescue AASM::InvalidTransition => e + flash.alert = aasm_error_message(e, target_state: :en_construction) end render partial: 'state_button_refresh', locals: { dossier: dossier } end def repasser_en_instruction - if dossier.en_instruction? - flash.notice = 'Le dossier est déjà en instruction.' - else + begin flash.notice = "Le dossier #{dossier.id} a été repassé en instruction." dossier.repasser_en_instruction!(current_instructeur) + rescue AASM::InvalidTransition => e + flash.alert = aasm_error_message(e, target_state: :en_instruction) end render partial: 'state_button_refresh', locals: { dossier: dossier } @@ -124,20 +124,23 @@ module Instructeurs motivation = params[:dossier] && params[:dossier][:motivation] justificatif = params[:dossier] && params[:dossier][:justificatif_motivation] - if dossier.termine? - flash.notice = "Le dossier est déjà #{dossier_display_state(dossier, lower: true)}" - else + begin case params[:process_action] when "refuser" + target_state = :refuse dossier.refuser!(current_instructeur, motivation, justificatif) flash.notice = "Dossier considéré comme refusé." when "classer_sans_suite" + target_state = :sans_suite dossier.classer_sans_suite!(current_instructeur, motivation, justificatif) flash.notice = "Dossier considéré comme sans suite." when "accepter" + target_state = :accepte dossier.accepter!(current_instructeur, motivation, justificatif) flash.notice = "Dossier traité avec succès." end + rescue AASM::InvalidTransition => e + flash.alert = aasm_error_message(e, target_state: target_state) end render partial: 'state_button_refresh', locals: { dossier: dossier } @@ -219,5 +222,13 @@ module Instructeurs def mark_annotations_privees_as_read current_instructeur.mark_tab_as_seen(dossier, :annotations_privees) end + + def aasm_error_message(exception, target_state:) + if exception.originating_state == target_state + "Le dossier est déjà #{dossier_display_state(target_state, lower: true)}." + else + "Le dossier est en ce moment #{dossier_display_state(exception.originating_state, lower: true)} : il n’est pas possible de le passer #{dossier_display_state(target_state, lower: true)}." + end + end end end diff --git a/spec/controllers/instructeurs/dossiers_controller_spec.rb b/spec/controllers/instructeurs/dossiers_controller_spec.rb index 71e808cb1..db3e30ef0 100644 --- a/spec/controllers/instructeurs/dossiers_controller_spec.rb +++ b/spec/controllers/instructeurs/dossiers_controller_spec.rb @@ -112,9 +112,23 @@ describe Instructeurs::DossiersController, type: :controller do context 'when the dossier has already been put en_instruction' do let(:dossier) { create(:dossier, :en_instruction, procedure: procedure) } - it 'warns about the error, but doesn’t raise' do + it 'warns about the error' do expect(dossier.reload.state).to eq(Dossier.states.fetch(:en_instruction)) expect(response).to have_http_status(:ok) + expect(response.body).to have_text('Le dossier est déjà en instruction.') + end + end + + context 'when the dossier has already been closed' do + let(:dossier) { create(:dossier, :accepte, procedure: procedure) } + + it 'doesn’t change the dossier state' do + expect(dossier.reload.state).to eq(Dossier.states.fetch(:accepte)) + end + + it 'warns about the error' do + expect(response).to have_http_status(:ok) + expect(response.body).to have_text('Le dossier est en ce moment accepté : il n’est pas possible de le passer en instruction.') end end end @@ -136,9 +150,10 @@ describe Instructeurs::DossiersController, type: :controller do context 'when the dossier has already been put en_construction' do let(:dossier) { create(:dossier, :en_construction, procedure: procedure) } - it 'warns about the error, but doesn’t raise' do + it 'warns about the error' do expect(dossier.reload.state).to eq(Dossier.states.fetch(:en_construction)) expect(response).to have_http_status(:ok) + expect(response.body).to have_text('Le dossier est déjà en construction.') end end end @@ -161,9 +176,10 @@ describe Instructeurs::DossiersController, type: :controller do context 'when the dossier has already been put en_instruction' do let(:dossier) { create(:dossier, :en_instruction, procedure: procedure) } - it 'warns about the error, but doesn’t raise' do + it 'warns about the error' do expect(dossier.reload.state).to eq(Dossier.states.fetch(:en_instruction)) expect(response).to have_http_status(:ok) + expect(response.body).to have_text('Le dossier est déjà en instruction.') end end @@ -355,15 +371,18 @@ describe Instructeurs::DossiersController, type: :controller do context 'when a dossier is already closed' do let(:dossier) { create(:dossier, :accepte, procedure: procedure) } - before { allow(dossier).to receive(:accepter!) } + before { allow(dossier).to receive(:after_accepter) } subject { post :terminer, params: { process_action: "accepter", procedure_id: procedure.id, dossier_id: dossier.id, dossier: { justificatif_motivation: fake_justificatif } }, format: 'js' } it 'does not close it again' do subject - expect(dossier).not_to have_received(:accepter!) + expect(dossier).not_to have_received(:after_accepter) + expect(dossier.state).to eq(Dossier.states.fetch(:accepte)) + expect(response).to have_http_status(:ok) + expect(response.body).to have_text('Le dossier est déjà accepté.') end end end