From f52554b5a3f7ee2fb96e29e15a2a0c8f98312267 Mon Sep 17 00:00:00 2001 From: sebastiencarceles Date: Tue, 28 Feb 2023 13:55:52 +0100 Subject: [PATCH] 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 | 10 ++-- app/models/champ.rb | 4 ++ app/models/champs/annuaire_education_champ.rb | 11 ++++ ...refill_annuaire_education_type_de_champ.rb | 7 +-- .../champ_fetch_external_data_job_spec.rb | 55 +++++++++++++++++++ spec/models/champ_spec.rb | 8 +++ .../champs/annuaire_education_champ_spec.rb | 42 ++++++++++++++ spec/models/prefill_params_spec.rb | 12 ---- ...l_annuaire_education_type_de_champ_spec.rb | 26 +-------- spec/system/users/dossier_prefill_get_spec.rb | 3 - .../system/users/dossier_prefill_post_spec.rb | 3 - 11 files changed, 127 insertions(+), 54 deletions(-) create mode 100644 spec/jobs/champ_fetch_external_data_job_spec.rb create mode 100644 spec/models/champs/annuaire_education_champ_spec.rb diff --git a/app/jobs/champ_fetch_external_data_job.rb b/app/jobs/champ_fetch_external_data_job.rb index 366586dda..790052863 100644 --- a/app/jobs/champ_fetch_external_data_job.rb +++ b/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 diff --git a/app/models/champ.rb b/app/models/champ.rb index c82a3db8f..cadb9e395 100644 --- a/app/models/champ.rb +++ b/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] diff --git a/app/models/champs/annuaire_education_champ.rb b/app/models/champs/annuaire_education_champ.rb index ffd064d1f..46a3ee389 100644 --- a/app/models/champs/annuaire_education_champ.rb +++ b/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 diff --git a/app/models/types_de_champ/prefill_annuaire_education_type_de_champ.rb b/app/models/types_de_champ/prefill_annuaire_education_type_de_champ.rb index afd256e05..7790458d9 100644 --- a/app/models/types_de_champ/prefill_annuaire_education_type_de_champ.rb +++ b/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 diff --git a/spec/jobs/champ_fetch_external_data_job_spec.rb b/spec/jobs/champ_fetch_external_data_job_spec.rb new file mode 100644 index 000000000..6f5277727 --- /dev/null +++ b/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 diff --git a/spec/models/champ_spec.rb b/spec/models/champ_spec.rb index 2d63881f7..e20c55417 100644 --- a/spec/models/champ_spec.rb +++ b/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 diff --git a/spec/models/champs/annuaire_education_champ_spec.rb b/spec/models/champs/annuaire_education_champ_spec.rb new file mode 100644 index 000000000..8c8f58be5 --- /dev/null +++ b/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 diff --git a/spec/models/prefill_params_spec.rb b/spec/models/prefill_params_spec.rb index 4764a1255..ccea47657 100644 --- a/spec/models/prefill_params_spec.rb +++ b/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 diff --git a/spec/models/types_de_champ/prefill_annuaire_education_type_de_champ_spec.rb b/spec/models/types_de_champ/prefill_annuaire_education_type_de_champ_spec.rb index e7bba2828..e79629698 100644 --- a/spec/models/types_de_champ/prefill_annuaire_education_type_de_champ_spec.rb +++ b/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 diff --git a/spec/system/users/dossier_prefill_get_spec.rb b/spec/system/users/dossier_prefill_get_spec.rb index 3fe04c9f0..206ea1f87 100644 --- a/spec/system/users/dossier_prefill_get_spec.rb +++ b/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') diff --git a/spec/system/users/dossier_prefill_post_spec.rb b/spec/system/users/dossier_prefill_post_spec.rb index 9c2c21d1e..ee31b2641 100644 --- a/spec/system/users/dossier_prefill_post_spec.rb +++ b/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')