Merge pull request #2893 from betagouv/frederic/fix_2604-robust_filtre_instructeur

Robustesse filtre instructeur

app/controllers/new_gestionnaire/procedures_controller.rb

@@ -208,7 +208,15 @@ module NewGestionnaire
     end
 
     def procedure_presentation
-      @procedure_presentation ||= current_gestionnaire.procedure_presentation_for_procedure_id(params[:procedure_id])
+      @procedure_presentation ||= get_procedure_presentation
+    end
+
+    def get_procedure_presentation
+      procedure_presentation, errors = current_gestionnaire.procedure_presentation_and_errors_for_procedure_id(params[:procedure_id])
+      if errors.present?
+        flash[:alert] = "Votre affichage a dû être réinitialisé en raison du problème suivant : " + errors.full_messages.join(', ')
+      end
+      procedure_presentation
     end
 
     def displayed_fields_values

app/models/assign_to.rb

@@ -3,7 +3,26 @@ class AssignTo < ApplicationRecord
   belongs_to :gestionnaire
   has_one :procedure_presentation, dependent: :destroy
 
-  def procedure_presentation_or_default
-    self.procedure_presentation || build_procedure_presentation
+  def procedure_presentation_or_default_and_errors
+    errors = reset_procedure_presentation_if_invalid
+    [procedure_presentation || build_procedure_presentation, errors]
+  end
+
+  private
+
+  def reset_procedure_presentation_if_invalid
+    if procedure_presentation&.invalid?
+      # This is a last defense against invalid `ProcedurePresentation`s persistently
+      # hindering instructeurs. Whenever this gets triggered, it means that there is
+      # a bug somewhere else that we need to fix.
+
+      errors = procedure_presentation.errors
+      Raven.capture_message(
+        "Destroying invalid ProcedurePresentation",
+        extra: { procedure_presentation: procedure_presentation.as_json }
+      )
+      self.procedure_presentation = nil
+      errors
+    end
   end
 end

app/models/gestionnaire.rb

@@ -81,8 +81,8 @@ class Gestionnaire < ApplicationRecord
     end
   end
 
-  def procedure_presentation_for_procedure_id(procedure_id)
-    assign_to.find_by(procedure_id: procedure_id).procedure_presentation_or_default
+  def procedure_presentation_and_errors_for_procedure_id(procedure_id)
+    assign_to.find_by(procedure_id: procedure_id).procedure_presentation_or_default_and_errors
   end
 
   def notifications_for_dossier(dossier)

spec/models/assign_to_spec.rb

@@ -1,17 +1,40 @@
 describe AssignTo, type: :model do
-  describe '#procedure_presentation_or_default' do
-    context "without a procedure_presentation" do
-      let!(:assign_to) { AssignTo.create }
+  describe '#procedure_presentation_or_default_and_errors' do
+    let(:procedure) { create(:procedure) }
+    let(:assign_to) { AssignTo.create(procedure: procedure) }
 
-      it { expect(assign_to.procedure_presentation_or_default.persisted?).to be_falsey }
+    let(:procedure_presentation_and_errors) { assign_to.procedure_presentation_or_default_and_errors }
+    let(:procedure_presentation_or_default) { procedure_presentation_and_errors.first }
+    let(:errors) { procedure_presentation_and_errors.second }
+
+    context "without a procedure_presentation" do
+      it { expect(procedure_presentation_or_default).not_to be_persisted }
+      it { expect(procedure_presentation_or_default).to be_valid }
+      it { expect(errors).to be_nil }
     end
 
     context "with a procedure_presentation" do
-      let(:procedure) { create(:procedure) }
-      let!(:assign_to) { AssignTo.create(procedure: procedure) }
       let!(:procedure_presentation) { ProcedurePresentation.create(assign_to: assign_to) }
 
-      it { expect(assign_to.procedure_presentation_or_default).to eq(procedure_presentation) }
+      it { expect(procedure_presentation_or_default).to eq(procedure_presentation) }
+      it { expect(procedure_presentation_or_default).to be_valid }
+      it { expect(errors).to be_nil }
+    end
+
+    context "with an invalid procedure_presentation" do
+      let!(:procedure_presentation) do
+        pp = ProcedurePresentation.new(assign_to: assign_to, displayed_fields: [{ 'table' => 'invalid', 'column' => 'random' }])
+        pp.save(validate: false)
+        pp
+      end
+
+      it { expect(procedure_presentation_or_default).not_to be_persisted }
+      it { expect(procedure_presentation_or_default).to be_valid }
+      it { expect(errors).to be_present }
+      it do
+        procedure_presentation_or_default
+        expect(assign_to.procedure_presentation).not_to be(procedure_presentation)
+      end
     end
   end
 end

spec/models/gestionnaire_spec.rb

@@ -197,11 +197,36 @@ describe Gestionnaire, type: :model do
     end
   end
 
-  describe "procedure_presentation_for_procedure_id" do
+  describe "procedure_presentation_and_errors_for_procedure_id" do
+    let(:procedure_presentation_and_errors) { gestionnaire.procedure_presentation_and_errors_for_procedure_id(procedure_id) }
+    let(:procedure_presentation) { procedure_presentation_and_errors.first }
+    let(:errors) { procedure_presentation_and_errors.second }
+
+    context 'with explicit presentation' do
+      let(:procedure_id) { procedure.id }
       let!(:pp) { ProcedurePresentation.create(assign_to: procedure_assign) }
 
-    it { expect(gestionnaire.procedure_presentation_for_procedure_id(procedure.id)).to eq(pp) }
-    it { expect(gestionnaire.procedure_presentation_for_procedure_id(procedure_2.id).persisted?).to be_falsey }
+      it { expect(procedure_presentation).to eq(pp) }
+      it { expect(errors).to be_nil }
+    end
+
+    context 'with invalid presentation' do
+      let(:procedure_id) { procedure.id }
+      before do
+        pp = ProcedurePresentation.create(assign_to: procedure_assign, displayed_fields: [{ 'table' => 'invalid', 'column' => 'random' }])
+        pp.save(:validate => false)
+      end
+
+      it { expect(procedure_presentation).not_to be_persisted }
+      it { expect(errors).to be_present }
+    end
+
+    context 'with default presentation' do
+      let(:procedure_id) { procedure_2.id }
+
+      it { expect(procedure_presentation).not_to be_persisted }
+      it { expect(errors).to be_nil }
+    end
   end
 
   describe '#notifications_for_dossier' do