review: update value with async fetch

1 - spec cover the job which fetches external data

2 - refactor the job with guard clauses

3 - delegate update operation to the champ itself

4 - annuaire education: override the update operation
to let the value be populated by the fetched data

5 - prefilling: don't fetch data synchronously

app/jobs/champ_fetch_external_data_job.rb

@@ -1,11 +1,9 @@
 class ChampFetchExternalDataJob < ApplicationJob
   def perform(champ, external_id)
-    if champ.external_id == external_id && champ.data.nil?
-      data = champ.fetch_external_data
+    return if champ.external_id != external_id
+    return if champ.data.present?
+    return if (data = champ.fetch_external_data).blank?
 
-      if data.present?
-        champ.update!(data: data)
-      end
-    end
+    champ.update_with_external_data!(data: data)
   end
 end

app/models/champ.rb

@@ -217,6 +217,10 @@ class Champ < ApplicationRecord
     raise NotImplemented.new(:fetch_external_data)
   end
 
+  def update_with_external_data!(data:)
+    update!(data: data)
+  end
+
   def clone
     champ_attributes = [:parent_id, :private, :row_id, :type, :type_de_champ_id]
     value_attributes = private? ? [] : [:value, :value_json, :data, :external_id]

app/models/champs/annuaire_education_champ.rb

@@ -28,4 +28,15 @@ class Champs::AnnuaireEducationChamp < Champs::TextChamp
   def fetch_external_data
     APIEducation::AnnuaireEducationAdapter.new(external_id).to_params
   end
+
+  def update_with_external_data!(data:)
+    if data&.is_a?(Hash) && data['nom_etablissement'].present? && data['nom_commune'].present? && data['identifiant_de_l_etablissement'].present?
+      update!(
+        data: data,
+        value: "#{data['nom_etablissement']}, #{data['nom_commune']} (#{data['identifiant_de_l_etablissement']})"
+      )
+    else
+      update!(data: data)
+    end
+  end
 end

app/models/types_de_champ/prefill_annuaire_education_type_de_champ.rb

@@ -3,14 +3,11 @@
 class TypesDeChamp::PrefillAnnuaireEducationTypeDeChamp < TypesDeChamp::PrefillTypeDeChamp
   def to_assignable_attributes(champ, value)
     return nil if value.blank?
-    return nil unless (data = APIEducation::AnnuaireEducationAdapter.new(value.presence).to_params)
 
     {
       id: champ.id,
-      external_id: data['identifiant_de_l_etablissement'],
-      value: "#{data['nom_etablissement']}, #{data['nom_commune']} (#{data['identifiant_de_l_etablissement']})"
+      external_id: value,
+      value: value
     }
-  rescue APIEducation::AnnuaireEducationAdapter::InvalidSchemaError
-    nil
   end
 end

spec/jobs/champ_fetch_external_data_job_spec.rb

@@ -0,0 +1,55 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe ChampFetchExternalDataJob, type: :job do
+  let(:champ) { Struct.new(:external_id, :data).new(champ_external_id, data) }
+  let(:external_id) { "an ID" }
+  let(:champ_external_id) { "an ID" }
+  let(:data) { nil }
+  let(:fetched_data) { nil }
+
+  subject(:perform_job) { described_class.perform_now(champ, external_id) }
+
+  before do
+    allow(champ).to receive(:fetch_external_data).and_return(fetched_data)
+    allow(champ).to receive(:update_with_external_data!)
+  end
+
+  shared_examples "a champ non-updater" do
+    it 'does not update the champ' do
+      perform_job
+      expect(champ).not_to have_received(:update_with_external_data!)
+    end
+  end
+
+  context 'when external_id matches the champ external_id and the champ data is nil' do
+    it 'fetches external data' do
+      perform_job
+      expect(champ).to have_received(:fetch_external_data)
+    end
+
+    context 'when the fetched data is present' do
+      let(:fetched_data) { "data" }
+
+      it 'updates the champ' do
+        perform_job
+        expect(champ).to have_received(:update_with_external_data!).with(data: fetched_data)
+      end
+    end
+
+    context 'when the fetched data is blank' do
+      it_behaves_like "a champ non-updater"
+    end
+  end
+
+  context 'when external_id does not match the champ external_id' do
+    let(:champ_external_id) { "something else" }
+    it_behaves_like "a champ non-updater"
+  end
+
+  context 'when the champ data is present' do
+    let(:data) { "present" }
+    it_behaves_like "a champ non-updater"
+  end
+end

spec/models/champ_spec.rb

@@ -602,4 +602,12 @@ describe Champ do
       end
     end
   end
+
+  describe '#update_with_external_data!' do
+    let(:champ) { create(:champ_siret) }
+    let(:data) { "data" }
+    subject { champ.update_with_external_data!(data: data) }
+
+    it { expect { subject }.to change { champ.reload.data }.to(data) }
+  end
 end

spec/models/champs/annuaire_education_champ_spec.rb

@@ -0,0 +1,42 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe Champs::AnnuaireEducationChamp do
+  describe '#update_with_external_data!' do
+    let(:champ) { create(:champ_annuaire_education, data: "any data") }
+    subject { champ.update_with_external_data!(data: data) }
+
+    shared_examples "a data updater (without updating the value)" do |data|
+      it { expect { subject }.to change { champ.reload.data }.to(data) }
+      it { expect { subject }.not_to change { champ.reload.value } }
+    end
+
+    context 'when data is nil' do
+      let(:data) { nil }
+      it_behaves_like "a data updater (without updating the value)", nil
+    end
+
+    context 'when data is empty' do
+      let(:data) { '' }
+      it_behaves_like "a data updater (without updating the value)", ''
+    end
+
+    context 'when data is inconsistent' do
+      let(:data) { { 'yo' => 'lo' } }
+      it_behaves_like "a data updater (without updating the value)", { 'yo' => 'lo' }
+    end
+
+    context 'when data is consistent' do
+      let(:data) {
+        {
+          'nom_etablissement': "karrigel an ankou",
+          'nom_commune' => 'kumun',
+          'identifiant_de_l_etablissement' => '666667'
+        }.with_indifferent_access
+      }
+      it { expect { subject }.to change { champ.reload.data }.to(data) }
+      it { expect { subject }.to change { champ.reload.value }.to('karrigel an ankou, kumun (666667)') }
+    end
+  end
+end

spec/models/prefill_params_spec.rb

@@ -6,7 +6,6 @@ RSpec.describe PrefillParams do
     let(:dossier) { create(:dossier, :brouillon, procedure: procedure) }
     let(:types_de_champ_public) { [] }
     let(:types_de_champ_private) { [] }
-    let(:api_annuaire_education_body) { File.read('spec/fixtures/files/api_education/annuaire_education.json') }
 
     subject(:prefill_params_array) { described_class.new(dossier, params).to_a }
 
@@ -18,11 +17,6 @@ RSpec.describe PrefillParams do
       VCR.insert_cassette('api_geo_departements')
       VCR.insert_cassette('api_geo_communes')
       VCR.insert_cassette('api_geo_epcis')
-
-      stub_request(:get, /https:\/\/data.education.gouv.fr\/api\/records\/1.0/).to_return(
-        body: api_annuaire_education_body,
-        status: 200
-      )
     end
 
     after do
@@ -256,12 +250,6 @@ RSpec.describe PrefillParams do
         expect(prefill_params_array).to match([])
       end
     end
-
-    context "when the annuaire education can't find the school for the given value" do
-      let(:api_annuaire_education_body) { File.read('spec/fixtures/files/api_education/annuaire_education_empty.json') }
-
-      it_behaves_like "a champ public value that is unauthorized", :annuaire_education, "value"
-    end
   end
 
   private

spec/models/types_de_champ/prefill_annuaire_education_type_de_champ_spec.rb

@@ -16,41 +16,17 @@ RSpec.describe TypesDeChamp::PrefillAnnuaireEducationTypeDeChamp do
 
     context 'when the value is nil' do
       let(:value) { nil }
-
       it { is_expected.to eq(nil) }
     end
 
     context 'when the value is empty' do
       let(:value) { '' }
-
       it { is_expected.to eq(nil) }
     end
 
     context 'when the value is present' do
       let(:value) { '0050009H' }
-
-      before do
-        stub_request(:get, /https:\/\/data.education.gouv.fr\/api\/records\/1.0/)
-          .to_return(body: body, status: 200)
-      end
-
-      context 'when the annuaire education api responds with a valid schema' do
-        let(:body) { File.read('spec/fixtures/files/api_education/annuaire_education.json') }
-
-        it { is_expected.to match({ id: champ.id, external_id: '0050009H', value: 'Lycée professionnel Sévigné, Gap (0050009H)' }) }
-      end
-
-      context "when the annuaire education api responds with invalid schema" do
-        let(:body) { File.read('spec/fixtures/files/api_education/annuaire_education_invalid.json') }
-
-        it { is_expected.to eq(nil) }
-      end
-
-      context 'when the annuaire education api responds with empty schema' do
-        let(:body) { File.read('spec/fixtures/files/api_education/annuaire_education_empty.json') }
-
-        it { is_expected.to eq(nil) }
-      end
+      it { is_expected.to match({ id: champ.id, external_id: '0050009H', value: '0050009H' }) }
     end
   end
 end

spec/system/users/dossier_prefill_get_spec.rb

@@ -74,9 +74,6 @@ describe 'Prefilling a dossier (with a GET request):', js: true do
     stub_request(:get, /https:\/\/entreprise.api.gouv.fr\/v2\/associations\//)
       .to_return(status: 200, body: File.read('spec/fixtures/files/api_entreprise/associations.json'))
 
-    stub_request(:get, /https:\/\/data.education.gouv.fr\/api\/records\/1.0/)
-      .to_return(status: 200, body: File.read('spec/fixtures/files/api_education/annuaire_education.json'))
-
     VCR.insert_cassette('api_geo_departements')
     VCR.insert_cassette('api_geo_communes')
     VCR.insert_cassette('api_geo_epcis')

spec/system/users/dossier_prefill_post_spec.rb

@@ -52,9 +52,6 @@ describe 'Prefilling a dossier (with a POST request):', js: true do
     stub_request(:get, /https:\/\/entreprise.api.gouv.fr\/v2\/associations\//)
       .to_return(status: 200, body: File.read('spec/fixtures/files/api_entreprise/associations.json'))
 
-    stub_request(:get, /https:\/\/data.education.gouv.fr\/api\/records\/1.0/)
-      .to_return(status: 200, body: File.read('spec/fixtures/files/api_education/annuaire_education.json'))
-
     VCR.insert_cassette('api_geo_departements')
     VCR.insert_cassette('api_geo_communes')
     VCR.insert_cassette('api_geo_epcis')