From 2ad4e4c01f02c3f17449580a82a8b2040e3d8bdd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=A9bastien=20Carceles?= <sebastien.carceles@gmail.com>
Date: Sat, 14 Jan 2023 10:13:26 +0100
Subject: [PATCH] refacto: better error management

---
 app/controllers/champs/siret_controller.rb    |  7 +++-
 ...t_champ_etablissement_fetchable_concern.rb | 32 +++++++++++--------
 ...mp_etablissement_fetchable_concern_spec.rb | 18 ++++++++---
 3 files changed, 38 insertions(+), 19 deletions(-)

diff --git a/app/controllers/champs/siret_controller.rb b/app/controllers/champs/siret_controller.rb
index f8112bf6b..23e4ab2ee 100644
--- a/app/controllers/champs/siret_controller.rb
+++ b/app/controllers/champs/siret_controller.rb
@@ -3,6 +3,11 @@ class Champs::SiretController < ApplicationController
 
   def show
     @champ = policy_scope(Champ).find(params[:champ_id])
-    @siret = @champ.fetch_etablissement!(read_param_value(@champ.input_name, 'value'), current_user)
+
+    if @champ.fetch_etablissement!(read_param_value(@champ.input_name, 'value'), current_user)
+      @siret = @champ.etablissement.siret
+    else
+      @siret = @champ.etablissement_fetch_error_key
+    end
   end
 end
diff --git a/app/models/concerns/siret_champ_etablissement_fetchable_concern.rb b/app/models/concerns/siret_champ_etablissement_fetchable_concern.rb
index 3beb7b652..cdc629372 100644
--- a/app/models/concerns/siret_champ_etablissement_fetchable_concern.rb
+++ b/app/models/concerns/siret_champ_etablissement_fetchable_concern.rb
@@ -1,34 +1,38 @@
 module SiretChampEtablissementFetchableConcern
   extend ActiveSupport::Concern
 
-  def fetch_etablissement!(siret, user)
-    return clear if siret.empty?
-    return clear(error: :invalid) unless Siret.new(siret: siret).valid? # i18n-tasks-use t('errors.messages.invalid_siret')
-    return clear(error: :not_found) unless (etablissement = APIEntrepriseService.create_etablissement(self, siret, user.id)) # i18n-tasks-use t('errors.messages.siret_not_found')
+  attr_reader :etablissement_fetch_error_key
 
-    return update_etablissement!(etablissement)
+  def fetch_etablissement!(siret, user)
+    return clear_etablissement!(:empty) if siret.empty?
+    return clear_etablissement!(:invalid) unless Siret.new(siret: siret).valid? # i18n-tasks-use t('errors.messages.invalid_siret')
+    return clear_etablissement!(:not_found) unless (etablissement = APIEntrepriseService.create_etablissement(self, siret, user&.id)) # i18n-tasks-use t('errors.messages.siret_not_found')
+
+    update!(value: siret, etablissement: etablissement)
   rescue => error
     if error.try(:network_error?) && !APIEntrepriseService.api_up?
       # TODO: notify ops
-      etablissement = APIEntrepriseService.create_etablissement_as_degraded_mode(self, siret, user.id)
-      return update_etablissement!(etablissement, error: :api_entreprise_down)
+      update!(
+        value: siret,
+        etablissement: APIEntrepriseService.create_etablissement_as_degraded_mode(self, siret, user.id)
+      )
+      @etablissement_fetch_error_key = :api_entreprise_down
+      false
     else
       Sentry.capture_exception(error, extra: { dossier_id: dossier_id, siret: siret })
-      return clear(error: :network_error) # i18n-tasks-use t('errors.messages.siret_network_error')
+      clear_etablissement!(:network_error) # i18n-tasks-use t('errors.messages.siret_network_error')
     end
   end
 
   private
 
-  def update_etablissement!(etablissement, error: nil)
-    update!(value: etablissement.siret, etablissement: etablissement)
-    error.presence || etablissement.siret
-  end
+  def clear_etablissement!(error_key)
+    @etablissement_fetch_error_key = error_key
 
-  def clear(error: nil)
     etablissement_to_destroy = etablissement
     update!(etablissement: nil)
     etablissement_to_destroy&.destroy
-    error.presence
+
+    false
   end
 end
diff --git a/spec/models/concern/siret_champ_etablissement_fetchable_concern_spec.rb b/spec/models/concern/siret_champ_etablissement_fetchable_concern_spec.rb
index c916bc7b9..a69ef2a3b 100644
--- a/spec/models/concern/siret_champ_etablissement_fetchable_concern_spec.rb
+++ b/spec/models/concern/siret_champ_etablissement_fetchable_concern_spec.rb
@@ -20,13 +20,18 @@ RSpec.describe SiretChampEtablissementFetchableConcern do
 
       it { expect { fetch_etablissement! }.to change { Etablissement.count }.by(-1) }
 
-      it { expect(fetch_etablissement!).to eq(error) }
+      it { expect(fetch_etablissement!).to eq(false) }
+
+      it 'populates the etablissement_fetch_error_key' do
+        fetch_etablissement!
+        expect(champ.etablissement_fetch_error_key).to eq(error)
+      end
     end
 
     context 'when the SIRET is empty' do
       let(:siret) { '' }
 
-      it_behaves_like 'an error occured', nil
+      it_behaves_like 'an error occured', :empty
     end
 
     context 'when the SIRET is invalid' do
@@ -63,7 +68,12 @@ RSpec.describe SiretChampEtablissementFetchableConcern do
 
       it { expect { fetch_etablissement! }.to change { Etablissement.count }.by(1) }
 
-      it { expect(fetch_etablissement!).to eq(:api_entreprise_down) }
+      it { expect(fetch_etablissement!).to eq(false) }
+
+      it 'populates the etablissement_fetch_error_key' do
+        fetch_etablissement!
+        expect(champ.etablissement_fetch_error_key).to eq(:api_entreprise_down)
+      end
     end
 
     context 'when the SIRET is valid but unknown' do
@@ -86,7 +96,7 @@ RSpec.describe SiretChampEtablissementFetchableConcern do
 
       it { expect { fetch_etablissement! }.to change { Etablissement.count }.by(1) }
 
-      it { expect(fetch_etablissement!).to eq(siret) }
+      it { expect(fetch_etablissement!).to eq(true) }
     end
   end
 end