From 8eb295d67e32af510d9658103b4eb6d6b51de3cf Mon Sep 17 00:00:00 2001
From: Pierre de La Morinerie <kemenaran@gmail.com>
Date: Thu, 19 Dec 2019 13:25:32 +0100
Subject: [PATCH] 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