From 2e763d5e92fc25fb47e3f48bdbd01898238cf546 Mon Sep 17 00:00:00 2001 From: Colin Darie Date: Wed, 3 Apr 2024 10:21:50 +0200 Subject: [PATCH 1/9] perf(search): ignore search_terms columns, use raw sql instead --- app/jobs/dossier_update_search_terms_job.rb | 1 - app/models/concerns/dossier_clone_concern.rb | 1 - .../concerns/dossier_searchable_concern.rb | 10 ++-- app/models/dossier.rb | 2 +- spec/controllers/recherche_controller_spec.rb | 2 + .../dossier_update_search_terms_job_spec.rb | 22 +++++--- .../concerns/dossier_clone_concern_spec.rb | 12 ++++- .../dossier_searchable_concern_spec.rb | 21 +++++--- spec/models/dossier_spec.rb | 4 +- spec/services/dossier_search_service_spec.rb | 54 +++++++++++++------ .../routing/rules_full_scenario_spec.rb | 2 + spec/system/users/invite_spec.rb | 1 + spec/system/users/list_dossiers_spec.rb | 1 + 13 files changed, 92 insertions(+), 41 deletions(-) diff --git a/app/jobs/dossier_update_search_terms_job.rb b/app/jobs/dossier_update_search_terms_job.rb index 8b997e3f5..5de7c61c8 100644 --- a/app/jobs/dossier_update_search_terms_job.rb +++ b/app/jobs/dossier_update_search_terms_job.rb @@ -3,6 +3,5 @@ class DossierUpdateSearchTermsJob < ApplicationJob def perform(dossier) dossier.update_search_terms - dossier.save!(touch: false) end end diff --git a/app/models/concerns/dossier_clone_concern.rb b/app/models/concerns/dossier_clone_concern.rb index 537afd8d4..86a135ae7 100644 --- a/app/models/concerns/dossier_clone_concern.rb +++ b/app/models/concerns/dossier_clone_concern.rb @@ -71,7 +71,6 @@ module DossierCloneConcern touch(:last_champ_updated_at) end reload - update_search_terms_later editing_fork.destroy_editing_fork! end diff --git a/app/models/concerns/dossier_searchable_concern.rb b/app/models/concerns/dossier_searchable_concern.rb index 1bb9ef424..f810e456c 100644 --- a/app/models/concerns/dossier_searchable_concern.rb +++ b/app/models/concerns/dossier_searchable_concern.rb @@ -2,10 +2,10 @@ module DossierSearchableConcern extend ActiveSupport::Concern included do - before_save :update_search_terms + after_commit :update_search_terms_later def update_search_terms - self.search_terms = [ + search_terms = [ user&.email, *champs_public.flat_map(&:search_terms), *etablissement&.search_terms, @@ -13,7 +13,11 @@ module DossierSearchableConcern individual&.prenom ].compact_blank.join(' ') - self.private_search_terms = champs_private.flat_map(&:search_terms).compact_blank.join(' ') + private_search_terms = champs_private.flat_map(&:search_terms).compact_blank.join(' ') + + sql = "UPDATE dossiers SET search_terms = :search_terms, private_search_terms = :private_search_terms WHERE id = :id" + sanitized_sql = self.class.sanitize_sql_array([sql, search_terms:, private_search_terms:, id:]) + self.class.connection.execute(sanitized_sql) end def update_search_terms_later diff --git a/app/models/dossier.rb b/app/models/dossier.rb index 3212a4f80..5d3c1a810 100644 --- a/app/models/dossier.rb +++ b/app/models/dossier.rb @@ -1,5 +1,5 @@ class Dossier < ApplicationRecord - self.ignored_columns += [:re_instructed_at] + self.ignored_columns += [:re_instructed_at, :search_terms, :private_search_terms] include DossierCloneConcern include DossierCorrectableConcern diff --git a/spec/controllers/recherche_controller_spec.rb b/spec/controllers/recherche_controller_spec.rb index 7048c2ccb..91bfe25fe 100644 --- a/spec/controllers/recherche_controller_spec.rb +++ b/spec/controllers/recherche_controller_spec.rb @@ -27,6 +27,8 @@ describe RechercheController, type: :controller do dossier_with_expert.champs_private[0].value = "Dossier B is incomplete" dossier_with_expert.champs_private[1].value = "Dossier B is invalid" dossier_with_expert.save! + + perform_enqueued_jobs(only: DossierUpdateSearchTermsJob) end describe 'GET #index' do diff --git a/spec/jobs/dossier_update_search_terms_job_spec.rb b/spec/jobs/dossier_update_search_terms_job_spec.rb index 822826c88..3dfda131f 100644 --- a/spec/jobs/dossier_update_search_terms_job_spec.rb +++ b/spec/jobs/dossier_update_search_terms_job_spec.rb @@ -1,15 +1,21 @@ RSpec.describe DossierUpdateSearchTermsJob, type: :job do let(:dossier) { create(:dossier) } - let(:champ_public) { dossier.champs_public.first } - let(:champ_private) { dossier.champs_private.first } - subject(:perform_job) { described_class.perform_now(dossier) } + subject(:perform_job) { described_class.perform_now(dossier.reload) } - context 'with an update' do - before do - create(:champ_text, dossier: dossier, value: "un nouveau champ") - end + before do + create(:champ_text, dossier:, value: "un nouveau champ") + create(:champ_text, dossier:, value: "private champ", private: true) + end - it { expect { perform_job }.to change { dossier.reload.search_terms }.to(/un nouveau champ/) } + it "update search terms columns" do + perform_job + + sql = "SELECT search_terms, private_search_terms FROM dossiers WHERE id = :id" + sanitized_sql = Dossier.sanitize_sql_array([sql, id: dossier.id]) + result = Dossier.connection.execute(sanitized_sql).first + + expect(result['search_terms']).to match(/un nouveau champ/) + expect(result['private_search_terms']).to match(/private champ/) end end diff --git a/spec/models/concerns/dossier_clone_concern_spec.rb b/spec/models/concerns/dossier_clone_concern_spec.rb index 064d14284..f3d5ffa1c 100644 --- a/spec/models/concerns/dossier_clone_concern_spec.rb +++ b/spec/models/concerns/dossier_clone_concern_spec.rb @@ -45,11 +45,19 @@ RSpec.describe DossierCloneConcern do it { expect(new_dossier.last_champ_updated_at).to be_nil } it { expect(new_dossier.last_commentaire_updated_at).to be_nil } it { expect(new_dossier.motivation).to be_nil } - it { expect(new_dossier.private_search_terms).to eq("") } it { expect(new_dossier.processed_at).to be_nil } - it { expect(new_dossier.search_terms).to match(dossier.user.email) } it { expect(new_dossier.termine_close_to_expiration_notice_sent_at).to be_nil } it { expect(new_dossier.dossier_transfer_id).to be_nil } + + it "update search terms" do + new_dossier + perform_enqueued_jobs(only: DossierUpdateSearchTermsJob) + sql = "SELECT search_terms, private_search_terms FROM dossiers where id = :id" + result = Dossier.connection.execute(Dossier.sanitize_sql_array([sql, id: new_dossier.id])).first + + expect(result["search_terms"]).to match(dossier.user.email) + expect(result["private_search_terms"]).to eq("") + end end context 'copies some attributes' do diff --git a/spec/models/concerns/dossier_searchable_concern_spec.rb b/spec/models/concerns/dossier_searchable_concern_spec.rb index a13f78d8a..58b6fc25e 100644 --- a/spec/models/concerns/dossier_searchable_concern_spec.rb +++ b/spec/models/concerns/dossier_searchable_concern_spec.rb @@ -13,15 +13,23 @@ describe DossierSearchableConcern do let(:france_connect_information) { build(:france_connect_information, given_name: 'Chris', family_name: 'Harrisson') } let(:user) { build(:user, france_connect_informations: [france_connect_information]) } + let(:result) do + Dossier.connection.execute( + Dossier.sanitize_sql_array(["SELECT search_terms, private_search_terms FROM dossiers WHERE id = :id", id: dossier.id]) + ).first + end + before do champ_public.update_attribute(:value, "champ public") champ_private.update_attribute(:value, "champ privé") - dossier.update_search_terms + perform_enqueued_jobs(only: DossierUpdateSearchTermsJob) end - it { expect(dossier.search_terms).to eq("#{user.email} champ public #{etablissement.entreprise_siren} #{etablissement.entreprise_numero_tva_intracommunautaire} #{etablissement.entreprise_forme_juridique} #{etablissement.entreprise_forme_juridique_code} #{etablissement.entreprise_nom_commercial} #{etablissement.entreprise_raison_sociale} #{etablissement.entreprise_siret_siege_social} #{etablissement.entreprise_nom} #{etablissement.entreprise_prenom} #{etablissement.association_rna} #{etablissement.association_titre} #{etablissement.association_objet} #{etablissement.siret} #{etablissement.naf} #{etablissement.libelle_naf} #{etablissement.adresse} #{etablissement.code_postal} #{etablissement.localite} #{etablissement.code_insee_localite}") } - it { expect(dossier.private_search_terms).to eq('champ privé') } + it "update columns" do + expect(result["search_terms"]).to eq("#{user.email} champ public #{etablissement.entreprise_siren} #{etablissement.entreprise_numero_tva_intracommunautaire} #{etablissement.entreprise_forme_juridique} #{etablissement.entreprise_forme_juridique_code} #{etablissement.entreprise_nom_commercial} #{etablissement.entreprise_raison_sociale} #{etablissement.entreprise_siret_siege_social} #{etablissement.entreprise_nom} #{etablissement.entreprise_prenom} #{etablissement.association_rna} #{etablissement.association_titre} #{etablissement.association_objet} #{etablissement.siret} #{etablissement.naf} #{etablissement.libelle_naf} #{etablissement.adresse} #{etablissement.code_postal} #{etablissement.localite} #{etablissement.code_insee_localite}") + expect(result["private_search_terms"]).to eq('champ privé') + end context 'with an update' do before do @@ -31,11 +39,12 @@ describe DossierSearchableConcern do ) perform_enqueued_jobs(only: DossierUpdateSearchTermsJob) - dossier.reload end - it { expect(dossier.search_terms).to include('nouvelle valeur publique') } - it { expect(dossier.private_search_terms).to include('nouvelle valeur privee') } + it "update columns" do + expect(result["search_terms"]).to include('nouvelle valeur publique') + expect(result["private_search_terms"]).to include('nouvelle valeur privee') + end end end end diff --git a/spec/models/dossier_spec.rb b/spec/models/dossier_spec.rb index f1fbe3ab5..8af8b7891 100644 --- a/spec/models/dossier_spec.rb +++ b/spec/models/dossier_spec.rb @@ -899,7 +899,7 @@ describe Dossier, type: :model do dossier.procedure.update_column(:web_hook_url, '/webhook.json') expect { - dossier.update_column(:search_terms, 'bonjour') + dossier.update_column(:conservation_extension, 'P1W') }.to_not have_enqueued_job(WebHookJob) expect { @@ -907,7 +907,7 @@ describe Dossier, type: :model do }.to have_enqueued_job(WebHookJob).with(dossier.procedure.id, dossier.id, 'en_construction', anything) expect { - dossier.update_column(:search_terms, 'bonjour2') + dossier.update_column(:conservation_extension, 'P2W') }.to_not have_enqueued_job(WebHookJob) expect { diff --git a/spec/services/dossier_search_service_spec.rb b/spec/services/dossier_search_service_spec.rb index 6528847fe..0f2baeec0 100644 --- a/spec/services/dossier_search_service_spec.rb +++ b/spec/services/dossier_search_service_spec.rb @@ -15,23 +15,33 @@ describe DossierSearchService do before do instructeur_1.assign_to_procedure(procedure_1) instructeur_2.assign_to_procedure(procedure_2) + + # create dossier before performing jobs + # because let!() syntax is executed after "before" callback + dossier_0 + dossier_1 + dossier_2 + dossier_3 + dossier_archived + + perform_enqueued_jobs(only: DossierUpdateSearchTermsJob) end let(:procedure_1) { create(:procedure, :published, administrateur: administrateur_1) } let(:procedure_2) { create(:procedure, :published, administrateur: administrateur_2) } - let!(:dossier_0) { create(:dossier, state: Dossier.states.fetch(:brouillon), procedure: procedure_1, user: create(:user, email: 'brouillon@clap.fr')) } + let(:dossier_0) { create(:dossier, state: Dossier.states.fetch(:brouillon), procedure: procedure_1, user: create(:user, email: 'brouillon@clap.fr')) } - let!(:etablissement_1) { create(:etablissement, entreprise_raison_sociale: 'OCTO Academy', siret: '41636169600051') } - let!(:dossier_1) { create(:dossier, :en_construction, procedure: procedure_1, user: create(:user, email: 'contact@test.com'), etablissement: etablissement_1) } + let(:etablissement_1) { create(:etablissement, entreprise_raison_sociale: 'OCTO Academy', siret: '41636169600051') } + let(:dossier_1) { create(:dossier, :en_construction, procedure: procedure_1, user: create(:user, email: 'contact@test.com'), etablissement: etablissement_1) } - let!(:etablissement_2) { create(:etablissement, entreprise_raison_sociale: 'Plop octo', siret: '41816602300012') } - let!(:dossier_2) { create(:dossier, :en_construction, procedure: procedure_1, user: create(:user, email: 'plop@gmail.com'), etablissement: etablissement_2) } + let(:etablissement_2) { create(:etablissement, entreprise_raison_sociale: 'Plop octo', siret: '41816602300012') } + let(:dossier_2) { create(:dossier, :en_construction, procedure: procedure_1, user: create(:user, email: 'plop@gmail.com'), etablissement: etablissement_2) } - let!(:etablissement_3) { create(:etablissement, entreprise_raison_sociale: 'OCTO Technology', siret: '41816609600051') } - let!(:dossier_3) { create(:dossier, :en_construction, procedure: procedure_2, user: create(:user, email: 'peace@clap.fr'), etablissement: etablissement_3) } + let(:etablissement_3) { create(:etablissement, entreprise_raison_sociale: 'OCTO Technology', siret: '41816609600051') } + let(:dossier_3) { create(:dossier, :en_construction, procedure: procedure_2, user: create(:user, email: 'peace@clap.fr'), etablissement: etablissement_3) } - let!(:dossier_archived) { create(:dossier, :en_construction, procedure: procedure_1, archived: true, user: create(:user, email: 'archived@clap.fr')) } + let(:dossier_archived) { create(:dossier, :en_construction, procedure: procedure_1, archived: true, user: create(:user, email: 'archived@clap.fr')) } describe 'search is empty' do let(:terms) { '' } @@ -99,6 +109,16 @@ describe DossierSearchService do describe '#matching_dossiers_for_user' do subject { liste_dossiers } + before do + dossier_0 + dossier_0b + dossier_1 + dossier_2 + dossier_3 + dossier_archived + perform_enqueued_jobs(only: DossierUpdateSearchTermsJob) + end + let(:liste_dossiers) do described_class.matching_dossiers_for_user(terms, user_1) end @@ -109,19 +129,19 @@ describe DossierSearchService do let(:procedure_1) { create(:procedure, :published) } let(:procedure_2) { create(:procedure, :published) } - let!(:dossier_0) { create(:dossier, state: Dossier.states.fetch(:brouillon), procedure: procedure_1, user: user_1) } - let!(:dossier_0b) { create(:dossier, state: Dossier.states.fetch(:brouillon), procedure: procedure_1, user: user_2) } + let(:dossier_0) { create(:dossier, state: Dossier.states.fetch(:brouillon), procedure: procedure_1, user: user_1) } + let(:dossier_0b) { create(:dossier, state: Dossier.states.fetch(:brouillon), procedure: procedure_1, user: user_2) } - let!(:etablissement_1) { create(:etablissement, entreprise_raison_sociale: 'OCTO Academy', siret: '41636169600051') } - let!(:dossier_1) { create(:dossier, state: Dossier.states.fetch(:en_construction), procedure: procedure_1, user: user_1, etablissement: etablissement_1) } + let(:etablissement_1) { create(:etablissement, entreprise_raison_sociale: 'OCTO Academy', siret: '41636169600051') } + let(:dossier_1) { create(:dossier, state: Dossier.states.fetch(:en_construction), procedure: procedure_1, user: user_1, etablissement: etablissement_1) } - let!(:etablissement_2) { create(:etablissement, entreprise_raison_sociale: 'Plop octo', siret: '41816602300012') } - let!(:dossier_2) { create(:dossier, state: Dossier.states.fetch(:en_construction), procedure: procedure_1, user: user_1, etablissement: etablissement_2) } + let(:etablissement_2) { create(:etablissement, entreprise_raison_sociale: 'Plop octo', siret: '41816602300012') } + let(:dossier_2) { create(:dossier, state: Dossier.states.fetch(:en_construction), procedure: procedure_1, user: user_1, etablissement: etablissement_2) } - let!(:etablissement_3) { create(:etablissement, entreprise_raison_sociale: 'OCTO Technology', siret: '41816609600051') } - let!(:dossier_3) { create(:dossier, state: Dossier.states.fetch(:en_construction), procedure: procedure_2, user: user_1, etablissement: etablissement_3) } + let(:etablissement_3) { create(:etablissement, entreprise_raison_sociale: 'OCTO Technology', siret: '41816609600051') } + let(:dossier_3) { create(:dossier, state: Dossier.states.fetch(:en_construction), procedure: procedure_2, user: user_1, etablissement: etablissement_3) } - let!(:dossier_archived) { create(:dossier, state: Dossier.states.fetch(:en_construction), procedure: procedure_1, archived: true, user: user_1) } + let(:dossier_archived) { create(:dossier, state: Dossier.states.fetch(:en_construction), procedure: procedure_1, archived: true, user: user_1) } describe 'search is empty' do let(:terms) { '' } diff --git a/spec/system/routing/rules_full_scenario_spec.rb b/spec/system/routing/rules_full_scenario_spec.rb index 4163ca067..d72fd0c46 100644 --- a/spec/system/routing/rules_full_scenario_spec.rb +++ b/spec/system/routing/rules_full_scenario_spec.rb @@ -134,6 +134,8 @@ describe 'The routing with rules', js: true do user_send_dossier(litteraire_user, 'littéraire') user_send_dossier(artistique_user, 'artistique') + perform_enqueued_jobs(only: DossierUpdateSearchTermsJob) + # the litteraires instructeurs only manage the litteraires dossiers register_instructeur_and_log_in(victor.email) click_on procedure.libelle diff --git a/spec/system/users/invite_spec.rb b/spec/system/users/invite_spec.rb index ea83d9789..2d681a49b 100644 --- a/spec/system/users/invite_spec.rb +++ b/spec/system/users/invite_spec.rb @@ -162,6 +162,7 @@ describe 'Invitations' do before do navigate_to_invited_dossier(invite) visit dossiers_path + perform_enqueued_jobs(only: DossierUpdateSearchTermsJob) end it "can search by id and it displays the dossier" do diff --git a/spec/system/users/list_dossiers_spec.rb b/spec/system/users/list_dossiers_spec.rb index 5325c2cc0..6fc525e04 100644 --- a/spec/system/users/list_dossiers_spec.rb +++ b/spec/system/users/list_dossiers_spec.rb @@ -268,6 +268,7 @@ describe 'user access to the list of their dossiers', js: true do context 'when it matches multiple dossiers' do let!(:dossier_with_champs) { create(:dossier, :with_populated_champs, :en_construction, user: user) } before do + perform_enqueued_jobs(only: DossierUpdateSearchTermsJob) find('.fr-search-bar .fr-btn').click end From 5a2ddcb471003238aa064bf4cabd80776a803f42 Mon Sep 17 00:00:00 2001 From: Colin Darie Date: Wed, 3 Apr 2024 16:34:24 +0200 Subject: [PATCH 2/9] test: faster dossier clone spec by merging similar `it` --- .../concerns/dossier_clone_concern_spec.rb | 161 ++++++++++-------- spec/models/dossier_spec.rb | 62 +++---- 2 files changed, 120 insertions(+), 103 deletions(-) diff --git a/spec/models/concerns/dossier_clone_concern_spec.rb b/spec/models/concerns/dossier_clone_concern_spec.rb index f3d5ffa1c..2763d3bf7 100644 --- a/spec/models/concerns/dossier_clone_concern_spec.rb +++ b/spec/models/concerns/dossier_clone_concern_spec.rb @@ -19,45 +19,44 @@ RSpec.describe DossierCloneConcern do let(:types_de_champ_public) { [{}] } let(:types_de_champ_private) { [{}] } let(:fork) { false } - let(:new_dossier) { dossier.clone(fork:) } + subject(:new_dossier) { dossier.clone(fork:) } - context 'reset most attributes' do - it { expect(new_dossier.id).not_to eq(dossier.id) } - it { expect(new_dossier.api_entreprise_job_exceptions).to be_nil } - it { expect(new_dossier.archived).to be_falsey } - it { expect(new_dossier.brouillon_close_to_expiration_notice_sent_at).to be_nil } - it { expect(new_dossier.conservation_extension).to eq(0.seconds) } - it { expect(new_dossier.declarative_triggered_at).to be_nil } - it { expect(new_dossier.deleted_user_email_never_send).to be_nil } - it { expect(new_dossier.depose_at).to be_nil } - it { expect(new_dossier.en_construction_at).to be_nil } - it { expect(new_dossier.en_construction_close_to_expiration_notice_sent_at).to be_nil } - it { expect(new_dossier.en_instruction_at).to be_nil } - it { expect(new_dossier.for_procedure_preview).to be_falsey } - it { expect(new_dossier.groupe_instructeur_updated_at).to be_nil } - it { expect(new_dossier.hidden_at).to be_nil } - it { expect(new_dossier.hidden_by_administration_at).to be_nil } - it { expect(new_dossier.hidden_by_reason).to be_nil } - it { expect(new_dossier.hidden_by_user_at).to be_nil } - it { expect(new_dossier.identity_updated_at).to be_nil } - it { expect(new_dossier.last_avis_updated_at).to be_nil } - it { expect(new_dossier.last_champ_private_updated_at).to be_nil } - it { expect(new_dossier.last_champ_updated_at).to be_nil } - it { expect(new_dossier.last_commentaire_updated_at).to be_nil } - it { expect(new_dossier.motivation).to be_nil } - it { expect(new_dossier.processed_at).to be_nil } - it { expect(new_dossier.termine_close_to_expiration_notice_sent_at).to be_nil } - it { expect(new_dossier.dossier_transfer_id).to be_nil } + it 'resets most of the attributes for the cloned dossier' do + expect(new_dossier.id).not_to eq(dossier.id) + expect(new_dossier.api_entreprise_job_exceptions).to be_nil + expect(new_dossier.archived).to be_falsey + expect(new_dossier.brouillon_close_to_expiration_notice_sent_at).to be_nil + expect(new_dossier.conservation_extension).to eq(0.seconds) + expect(new_dossier.declarative_triggered_at).to be_nil + expect(new_dossier.deleted_user_email_never_send).to be_nil + expect(new_dossier.depose_at).to be_nil + expect(new_dossier.en_construction_at).to be_nil + expect(new_dossier.en_construction_close_to_expiration_notice_sent_at).to be_nil + expect(new_dossier.en_instruction_at).to be_nil + expect(new_dossier.for_procedure_preview).to be_falsey + expect(new_dossier.groupe_instructeur_updated_at).to be_nil + expect(new_dossier.hidden_at).to be_nil + expect(new_dossier.hidden_by_administration_at).to be_nil + expect(new_dossier.hidden_by_reason).to be_nil + expect(new_dossier.hidden_by_user_at).to be_nil + expect(new_dossier.identity_updated_at).to be_nil + expect(new_dossier.last_avis_updated_at).to be_nil + expect(new_dossier.last_champ_private_updated_at).to be_nil + expect(new_dossier.last_champ_updated_at).to be_nil + expect(new_dossier.last_commentaire_updated_at).to be_nil + expect(new_dossier.motivation).to be_nil + expect(new_dossier.processed_at).to be_nil + end - it "update search terms" do - new_dossier - perform_enqueued_jobs(only: DossierUpdateSearchTermsJob) - sql = "SELECT search_terms, private_search_terms FROM dossiers where id = :id" - result = Dossier.connection.execute(Dossier.sanitize_sql_array([sql, id: new_dossier.id])).first + it "updates search terms" do + subject - expect(result["search_terms"]).to match(dossier.user.email) - expect(result["private_search_terms"]).to eq("") - end + perform_enqueued_jobs(only: DossierUpdateSearchTermsJob) + sql = "SELECT search_terms, private_search_terms FROM dossiers where id = :id" + result = Dossier.connection.execute(Dossier.sanitize_sql_array([sql, id: new_dossier.id])).first + + expect(result["search_terms"]).to match(dossier.user.email) + expect(result["private_search_terms"]).to eq("") end context 'copies some attributes' do @@ -67,18 +66,22 @@ RSpec.describe DossierCloneConcern do end context 'when not forked' do - it { expect(new_dossier.groupe_instructeur).to be_nil } + it "copies or reset attributes" do + expect(new_dossier.groupe_instructeur).to be_nil + expect(new_dossier.autorisation_donnees).to eq(dossier.autorisation_donnees) + expect(new_dossier.revision_id).to eq(dossier.revision_id) + expect(new_dossier.user_id).to eq(dossier.user_id) + end end - it { expect(new_dossier.autorisation_donnees).to eq(dossier.autorisation_donnees) } - it { expect(new_dossier.revision_id).to eq(dossier.revision_id) } - it { expect(new_dossier.user_id).to eq(dossier.user_id) } end context 'forces some attributes' do let(:dossier) { create(:dossier, :accepte) } - it { expect(new_dossier.brouillon?).to eq(true) } - it { expect(new_dossier.parent_dossier).to eq(dossier) } + it do + expect(new_dossier.brouillon?).to eq(true) + expect(new_dossier.parent_dossier).to eq(dossier) + end context 'destroy parent' do before { new_dossier } @@ -91,22 +94,28 @@ RSpec.describe DossierCloneConcern do context 'procedure with_individual' do let(:procedure) { create(:procedure, :for_individual) } - it { expect(new_dossier.individual.slice(:nom, :prenom, :gender)).to eq(dossier.individual.slice(:nom, :prenom, :gender)) } - it { expect(new_dossier.individual.id).not_to eq(dossier.individual.id) } + it do + expect(new_dossier.individual.slice(:nom, :prenom, :gender)).to eq(dossier.individual.slice(:nom, :prenom, :gender)) + expect(new_dossier.individual.id).not_to eq(dossier.individual.id) + end end context 'procedure with etablissement' do let(:dossier) { create(:dossier, :with_entreprise) } - it { expect(new_dossier.etablissement.slice(:siret)).to eq(dossier.etablissement.slice(:siret)) } - it { expect(new_dossier.etablissement.id).not_to eq(dossier.etablissement.id) } + it do + expect(new_dossier.etablissement.slice(:siret)).to eq(dossier.etablissement.slice(:siret)) + expect(new_dossier.etablissement.id).not_to eq(dossier.etablissement.id) + end end describe 'champs' do it { expect(new_dossier.id).not_to eq(dossier.id) } context 'public are duplicated' do - it { expect(new_dossier.champs_public.count).to eq(dossier.champs_public.count) } - it { expect(new_dossier.champs_public.ids).not_to eq(dossier.champs_public.ids) } + it do + expect(new_dossier.champs_public.count).to eq(dossier.champs_public.count) + expect(new_dossier.champs_public.ids).not_to eq(dossier.champs_public.ids) + end it 'keeps champs.values' do original_first_champ = dossier.champs_public.first @@ -121,8 +130,10 @@ RSpec.describe DossierCloneConcern do let(:champ_repetition) { create(:champ_repetition, type_de_champ: type_de_champ_repetition, dossier: dossier) } before { dossier.champs_public << champ_repetition } - it { expect(Champs::RepetitionChamp.where(dossier: new_dossier).first.champs.count).to eq(4) } - it { expect(Champs::RepetitionChamp.where(dossier: new_dossier).first.champs.ids).not_to eq(champ_repetition.champs.ids) } + it do + expect(Champs::RepetitionChamp.where(dossier: new_dossier).first.champs.count).to eq(4) + expect(Champs::RepetitionChamp.where(dossier: new_dossier).first.champs.ids).not_to eq(champ_repetition.champs.ids) + end end context 'for Champs::CarteChamp with geo areas, original_champ.geo_areas are duped' do @@ -132,8 +143,10 @@ RSpec.describe DossierCloneConcern do let(:champ_carte) { create(:champ_carte, type_de_champ: type_de_champ_carte, geo_areas: [geo_area]) } before { dossier.champs_public << champ_carte } - it { expect(Champs::CarteChamp.where(dossier: new_dossier).first.geo_areas.count).to eq(1) } - it { expect(Champs::CarteChamp.where(dossier: new_dossier).first.geo_areas.ids).not_to eq(champ_carte.geo_areas.ids) } + it do + expect(Champs::CarteChamp.where(dossier: new_dossier).first.geo_areas.count).to eq(1) + expect(Champs::CarteChamp.where(dossier: new_dossier).first.geo_areas.ids).not_to eq(champ_carte.geo_areas.ids) + end end context 'for Champs::SiretChamp, original_champ.etablissement is duped' do @@ -143,8 +156,10 @@ RSpec.describe DossierCloneConcern do let(:champ_siret) { create(:champ_siret, type_de_champ: type_de_champs_siret, etablissement: create(:etablissement)) } before { dossier.champs_public << champ_siret } - it { expect(Champs::SiretChamp.where(dossier: dossier).first.etablissement).not_to be_nil } - it { expect(Champs::SiretChamp.where(dossier: new_dossier).first.etablissement.id).not_to eq(champ_siret.etablissement.id) } + it do + expect(Champs::SiretChamp.where(dossier: dossier).first.etablissement).not_to be_nil + expect(Champs::SiretChamp.where(dossier: new_dossier).first.etablissement.id).not_to eq(champ_siret.etablissement.id) + end end context 'for Champs::PieceJustificative, original_champ.piece_justificative_file is duped' do @@ -161,18 +176,19 @@ RSpec.describe DossierCloneConcern do let(:champ_address) { create(:champ_address, type_de_champ: type_de_champs_adress, external_id: 'Address', data: { city_code: '75019' }) } before { dossier.champs_public << champ_address } - it { expect(Champs::AddressChamp.where(dossier: dossier).first.data).not_to be_nil } - it { expect(Champs::AddressChamp.where(dossier: dossier).first.external_id).not_to be_nil } - it { expect(Champs::AddressChamp.where(dossier: new_dossier).first.external_id).to eq(champ_address.external_id) } - it { expect(Champs::AddressChamp.where(dossier: new_dossier).first.data).to eq(champ_address.data) } + it do + expect(Champs::AddressChamp.where(dossier: dossier).first.data).not_to be_nil + expect(Champs::AddressChamp.where(dossier: dossier).first.external_id).not_to be_nil + expect(Champs::AddressChamp.where(dossier: new_dossier).first.external_id).to eq(champ_address.external_id) + expect(Champs::AddressChamp.where(dossier: new_dossier).first.data).to eq(champ_address.data) + end end end context 'private are renewd' do - it { expect(new_dossier.champs_private.count).to eq(dossier.champs_private.count) } - it { expect(new_dossier.champs_private.ids).not_to eq(dossier.champs_private.ids) } - it 'reset champs private values' do + expect(new_dossier.champs_private.count).to eq(dossier.champs_private.count) + expect(new_dossier.champs_private.ids).not_to eq(dossier.champs_private.ids) original_first_champs_private = dossier.champs_private.first original_first_champs_private.update!(value: 'kthxbye') @@ -186,10 +202,12 @@ RSpec.describe DossierCloneConcern do let(:new_dossier) { dossier.clone(fork: true) } before { dossier.champs_public.reload } # we compare timestamps so we have to get the precision limit from the db } - it { expect(new_dossier.editing_fork_origin).to eq(dossier) } - it { expect(new_dossier.champs_public[0].id).not_to eq(dossier.champs_public[0].id) } - it { expect(new_dossier.champs_public[0].created_at).to eq(dossier.champs_public[0].created_at) } - it { expect(new_dossier.champs_public[0].updated_at).to eq(dossier.champs_public[0].updated_at) } + it do + expect(new_dossier.editing_fork_origin).to eq(dossier) + expect(new_dossier.champs_public[0].id).not_to eq(dossier.champs_public[0].id) + expect(new_dossier.champs_public[0].created_at).to eq(dossier.champs_public[0].created_at) + expect(new_dossier.champs_public[0].updated_at).to eq(dossier.champs_public[0].updated_at) + end context "piece justificative champ" do let(:types_de_champ_public) { [{ type: :piece_justificative }] } @@ -261,8 +279,10 @@ RSpec.describe DossierCloneConcern do forked_dossier.assign_to_groupe_instructeur(dossier.procedure.defaut_groupe_instructeur, DossierAssignment.modes.fetch(:manual)) } - it { is_expected.to eq(added: [], updated: [], removed: []) } - it { expect(forked_dossier.forked_with_changes?).to be_truthy } + it do + expect(subject).to eq(added: [], updated: [], removed: []) + expect(forked_dossier.forked_with_changes?).to be_truthy + end end context 'with updated champ' do @@ -270,8 +290,8 @@ RSpec.describe DossierCloneConcern do before { updated_champ.update(value: 'new value') } - it { is_expected.to eq(added: [], updated: [updated_champ], removed: []) } it 'forked_with_changes? should reflect dossier state' do + expect(subject).to eq(added: [], updated: [updated_champ], removed: []) expect(dossier.forked_with_changes?).to be_falsey expect(forked_dossier.forked_with_changes?).to be_truthy expect(updated_champ.forked_with_changes?).to be_truthy @@ -318,13 +338,10 @@ RSpec.describe DossierCloneConcern do it { expect { subject }.to change { dossier.reload.champs.size }.by(0) } it { expect { subject }.not_to change { dossier.reload.champs.order(:created_at).reject { _1.stable_id.in?([99, 994]) }.map(&:value) } } + it { expect { subject }.to have_enqueued_job(DossierUpdateSearchTermsJob).with(dossier) } it { expect { subject }.to change { dossier.reload.champs.find { _1.stable_id == 99 }.value }.from('old value').to('new value') } it { expect { subject }.to change { dossier.reload.champs.find { _1.stable_id == 994 }.value }.from('old value').to('new value in repetition') } - it 'update dossier search terms' do - expect { subject }.to have_enqueued_job(DossierUpdateSearchTermsJob).with(dossier) - end - it 'fork is hidden after merge' do subject expect(forked_dossier.reload.hidden_by_reason).to eq("stale_fork") diff --git a/spec/models/dossier_spec.rb b/spec/models/dossier_spec.rb index 8af8b7891..68c7df9e5 100644 --- a/spec/models/dossier_spec.rb +++ b/spec/models/dossier_spec.rb @@ -995,28 +995,28 @@ describe Dossier, type: :model do allow(NotificationMailer).to receive(:send_accepte_notification).and_return(double(deliver_later: true)) allow(dossier).to receive(:build_attestation).and_return(attestation) - Timecop.freeze(now) + travel_to now dossier.accepter!(instructeur: instructeur, motivation: 'motivation') dossier.reload end - after { Timecop.return } - - it { expect(dossier.traitements.last.motivation).to eq('motivation') } - it { expect(dossier.motivation).to eq('motivation') } - it { expect(dossier.traitements.last.instructeur_email).to eq(instructeur.email) } - it { expect(dossier.en_instruction_at).to eq(dossier.en_instruction_at) } - it { expect(dossier.traitements.last.processed_at).to eq(now) } - it { expect(dossier.processed_at).to eq(now) } - it { expect(dossier.state).to eq('accepte') } - it { expect(last_operation.operation).to eq('accepter') } - it { expect(last_operation.automatic_operation?).to be_falsey } - it { expect(operation_serialized['operation']).to eq('accepter') } - it { expect(operation_serialized['dossier_id']).to eq(dossier.id) } - it { expect(operation_serialized['executed_at']).to eq(last_operation.executed_at.iso8601) } - it { expect(NotificationMailer).to have_received(:send_accepte_notification).with(dossier) } - it { expect(dossier.attestation).to eq(attestation) } - it { expect(dossier.commentaires.count).to eq(1) } + it "update attributes" do + expect(dossier.traitements.last.motivation).to eq('motivation') + expect(dossier.motivation).to eq('motivation') + expect(dossier.traitements.last.instructeur_email).to eq(instructeur.email) + expect(dossier.en_instruction_at).to eq(dossier.en_instruction_at) + expect(dossier.traitements.last.processed_at).to eq(now) + expect(dossier.processed_at).to eq(now) + expect(dossier.state).to eq('accepte') + expect(last_operation.operation).to eq('accepter') + expect(last_operation.automatic_operation?).to be_falsey + expect(operation_serialized['operation']).to eq('accepter') + expect(operation_serialized['dossier_id']).to eq(dossier.id) + expect(operation_serialized['executed_at']).to eq(last_operation.executed_at.iso8601) + expect(NotificationMailer).to have_received(:send_accepte_notification).with(dossier) + expect(dossier.attestation).to eq(attestation) + expect(dossier.commentaires.count).to eq(1) + end end describe '#accepter_automatiquement!' do @@ -1642,24 +1642,24 @@ describe Dossier, type: :model do let(:last_operation) { dossier.dossier_operation_logs.last } before do - Timecop.freeze + freeze_time allow(NotificationMailer).to receive(:send_repasser_en_instruction_notification).and_return(double(deliver_later: true)) dossier.repasser_en_instruction!(instructeur: instructeur) dossier.reload end - it { expect(dossier.state).to eq('en_instruction') } - it { expect(dossier.archived).to be_falsey } - it { expect(dossier.motivation).to be_nil } - it { expect(dossier.justificatif_motivation.attached?).to be_falsey } - it { expect(dossier.attestation).to be_nil } - it { expect(dossier.sva_svr_decision_on).to be_nil } - it { expect(dossier.termine_close_to_expiration_notice_sent_at).to be_nil } - it { expect(last_operation.operation).to eq('repasser_en_instruction') } - it { expect(last_operation.data['author']['email']).to eq(instructeur.email) } - it { expect(NotificationMailer).to have_received(:send_repasser_en_instruction_notification).with(dossier) } - - after { Timecop.return } + it "update attributes" do + expect(dossier.state).to eq('en_instruction') + expect(dossier.archived).to be_falsey + expect(dossier.motivation).to be_nil + expect(dossier.justificatif_motivation.attached?).to be_falsey + expect(dossier.attestation).to be_nil + expect(dossier.sva_svr_decision_on).to be_nil + expect(dossier.termine_close_to_expiration_notice_sent_at).to be_nil + expect(last_operation.operation).to eq('repasser_en_instruction') + expect(last_operation.data['author']['email']).to eq(instructeur.email) + expect(NotificationMailer).to have_received(:send_repasser_en_instruction_notification).with(dossier) + end end describe '#notify_draft_not_submitted' do From ee465b38ffe50e305bcc5dd92c301d4a22375e4d Mon Sep 17 00:00:00 2001 From: Colin Darie Date: Thu, 25 Apr 2024 17:44:05 +0200 Subject: [PATCH 3/9] feat(search): debounce update search terms --- .../concerns/dossier_searchable_concern.rb | 11 +++++++- .../dossier_searchable_concern_spec.rb | 26 +++++++++++++------ 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/app/models/concerns/dossier_searchable_concern.rb b/app/models/concerns/dossier_searchable_concern.rb index f810e456c..9adec58fe 100644 --- a/app/models/concerns/dossier_searchable_concern.rb +++ b/app/models/concerns/dossier_searchable_concern.rb @@ -1,9 +1,15 @@ +# frozen_string_literal: true + module DossierSearchableConcern extend ActiveSupport::Concern included do after_commit :update_search_terms_later + SEARCH_TERMS_DEBOUNCE = 30.seconds + + kredis_flag :debounce_update_search_terms_flag + def update_search_terms search_terms = [ user&.email, @@ -21,7 +27,10 @@ module DossierSearchableConcern end def update_search_terms_later - DossierUpdateSearchTermsJob.perform_later(self) + return if debounce_update_search_terms_flag.marked? + + debounce_update_search_terms_flag.mark(expires_in: SEARCH_TERMS_DEBOUNCE) + DossierUpdateSearchTermsJob.set(wait: SEARCH_TERMS_DEBOUNCE).perform_later(self) end end end diff --git a/spec/models/concerns/dossier_searchable_concern_spec.rb b/spec/models/concerns/dossier_searchable_concern_spec.rb index 58b6fc25e..343f31795 100644 --- a/spec/models/concerns/dossier_searchable_concern_spec.rb +++ b/spec/models/concerns/dossier_searchable_concern_spec.rb @@ -2,8 +2,6 @@ describe DossierSearchableConcern do let(:champ_public) { dossier.champs_public.first } let(:champ_private) { dossier.champs_private.first } - subject { dossier } - describe '#update_search_terms' do let(:etablissement) { dossier.etablissement } let(:dossier) { create(:dossier, :with_entreprise, user: user) } @@ -19,32 +17,44 @@ describe DossierSearchableConcern do ).first end - before do + it "update columns" do champ_public.update_attribute(:value, "champ public") champ_private.update_attribute(:value, "champ privé") - perform_enqueued_jobs(only: DossierUpdateSearchTermsJob) - end - it "update columns" do expect(result["search_terms"]).to eq("#{user.email} champ public #{etablissement.entreprise_siren} #{etablissement.entreprise_numero_tva_intracommunautaire} #{etablissement.entreprise_forme_juridique} #{etablissement.entreprise_forme_juridique_code} #{etablissement.entreprise_nom_commercial} #{etablissement.entreprise_raison_sociale} #{etablissement.entreprise_siret_siege_social} #{etablissement.entreprise_nom} #{etablissement.entreprise_prenom} #{etablissement.association_rna} #{etablissement.association_titre} #{etablissement.association_objet} #{etablissement.siret} #{etablissement.naf} #{etablissement.libelle_naf} #{etablissement.adresse} #{etablissement.code_postal} #{etablissement.localite} #{etablissement.code_insee_localite}") expect(result["private_search_terms"]).to eq('champ privé') end context 'with an update' do before do + stub_const("DossierSearchableConcern::SEARCH_TERMS_DEBOUNCE", 1.second) + end + + it "update columns" do dossier.update( champs_public_attributes: [{ id: champ_public.id, value: 'nouvelle valeur publique' }], champs_private_attributes: [{ id: champ_private.id, value: 'nouvelle valeur privee' }] ) perform_enqueued_jobs(only: DossierUpdateSearchTermsJob) - end - it "update columns" do expect(result["search_terms"]).to include('nouvelle valeur publique') expect(result["private_search_terms"]).to include('nouvelle valeur privee') end + + it "debounce jobs" do + assert_enqueued_jobs(1, only: DossierUpdateSearchTermsJob) do + 3.times { dossier.index_search_terms_later } + end + + # wait redis key expiration + sleep 1.01.seconds + + assert_enqueued_jobs(1, only: DossierUpdateSearchTermsJob) do + dossier.update(champs_public_attributes: [{ id: champ_public.id, value: rand(10).to_s }]) + end + end end end end From 44088248820633162f2b17c0f6f582e9601e7570 Mon Sep 17 00:00:00 2001 From: Colin Darie Date: Thu, 25 Apr 2024 17:44:28 +0200 Subject: [PATCH 4/9] fix(dossier): update search terms when etablissement or individual changed --- app/models/etablissement.rb | 2 ++ app/models/individual.rb | 2 ++ spec/models/etablissement_spec.rb | 10 ++++++++++ spec/models/individual_spec.rb | 32 +++++++++++++++++++------------ 4 files changed, 34 insertions(+), 12 deletions(-) diff --git a/app/models/etablissement.rb b/app/models/etablissement.rb index 811a0c0a9..d57bc08b2 100644 --- a/app/models/etablissement.rb +++ b/app/models/etablissement.rb @@ -17,6 +17,8 @@ class Etablissement < ApplicationRecord fermé: "fermé" }, _prefix: true + after_commit -> { dossier&.debounce_update_search_terms } + def entreprise_raison_sociale read_attribute(:entreprise_raison_sociale).presence || raison_sociale_for_ei end diff --git a/app/models/individual.rb b/app/models/individual.rb index c3d92b9eb..6e52045b4 100644 --- a/app/models/individual.rb +++ b/app/models/individual.rb @@ -17,6 +17,8 @@ class Individual < ApplicationRecord validates :email, presence: true, if: -> { dossier.for_tiers? && self.email? }, on: :update + after_commit -> { dossier.debounce_update_search_terms }, if: -> { nom_previously_changed? || prenom_previously_changed? } + GENDER_MALE = "M." GENDER_FEMALE = 'Mme' diff --git a/spec/models/etablissement_spec.rb b/spec/models/etablissement_spec.rb index a7f38d6cd..7653c79cd 100644 --- a/spec/models/etablissement_spec.rb +++ b/spec/models/etablissement_spec.rb @@ -115,6 +115,16 @@ describe Etablissement do end end + describe 'update search terms' do + let(:etablissement) { create(:etablissement, dossier: build(:dossier)) } + + it "schedule update search terms" do + assert_enqueued_jobs(1, only: DossierUpdateSearchTermsJob) do + etablissement.update(entreprise_nom: "nom") + end + end + end + private def csv_to_array_of_hash(lines) diff --git a/spec/models/individual_spec.rb b/spec/models/individual_spec.rb index 64820f03d..a37d1e15b 100644 --- a/spec/models/individual_spec.rb +++ b/spec/models/individual_spec.rb @@ -7,41 +7,49 @@ describe Individual do describe "#save" do let(:individual) { build(:individual) } - subject { individual.save } + subject do + individual.save + individual + end context "with birthdate" do before do individual.birthdate = birthdate_from_user - subject end context "and the format is dd/mm/yyy " do let(:birthdate_from_user) { "12/11/1980" } - it { expect(individual.birthdate).to eq(Date.new(1980, 11, 12)) } + it { expect(subject.birthdate).to eq(Date.new(1980, 11, 12)) } end context "and the format is ISO" do let(:birthdate_from_user) { "1980-11-12" } - it { expect(individual.birthdate).to eq(Date.new(1980, 11, 12)) } + it { expect(subject.birthdate).to eq(Date.new(1980, 11, 12)) } end context "and the format is WTF" do let(:birthdate_from_user) { "1980 1 12" } - it { expect(individual.birthdate).to be_nil } + it { expect(subject.birthdate).to be_nil } end end - context 'when an individual has an invalid notification_method' do - let(:invalid_individual) { build(:individual, notification_method: 'invalid_method') } - - it 'raises an ArgumentError' do - expect { - invalid_individual.valid? - }.to raise_error(ArgumentError, "'invalid_method' is not a valid notification_method") + it "schedule update search terms" do + assert_enqueued_jobs(1, only: DossierUpdateSearchTermsJob) do + subject end end end + + context 'when an individual has an invalid notification_method' do + let(:invalid_individual) { build(:individual, notification_method: 'invalid_method') } + + it 'raises an ArgumentError' do + expect { + invalid_individual.valid? + }.to raise_error(ArgumentError, "'invalid_method' is not a valid notification_method") + end + end end From 797bd6b94bbb5b8c6671ef522d7e896b7e4bc10e Mon Sep 17 00:00:00 2001 From: Colin Darie Date: Thu, 25 Apr 2024 17:51:54 +0200 Subject: [PATCH 5/9] fix(search): preload before updating search terms because we access all champs --- app/models/concerns/dossier_searchable_concern.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/models/concerns/dossier_searchable_concern.rb b/app/models/concerns/dossier_searchable_concern.rb index 9adec58fe..e26344115 100644 --- a/app/models/concerns/dossier_searchable_concern.rb +++ b/app/models/concerns/dossier_searchable_concern.rb @@ -11,6 +11,8 @@ module DossierSearchableConcern kredis_flag :debounce_update_search_terms_flag def update_search_terms + DossierPreloader.load_one(self) + search_terms = [ user&.email, *champs_public.flat_map(&:search_terms), From 39b03272373cbe209cbe7faf33f6ed8f85b5a820 Mon Sep 17 00:00:00 2001 From: Colin Darie Date: Thu, 25 Apr 2024 17:57:55 +0200 Subject: [PATCH 6/9] refactor(search): rename update search terms => index search terms --- app/jobs/dossier_index_search_terms_job.rb | 7 +++++++ app/jobs/dossier_update_search_terms_job.rb | 7 ------- app/models/concerns/dossier_searchable_concern.rb | 14 +++++++------- app/models/etablissement.rb | 2 +- app/models/individual.rb | 2 +- config/initializers/transition_to_sidekiq.rb | 2 +- spec/controllers/recherche_controller_spec.rb | 2 +- ...c.rb => dossier_index_search_terms_job_spec.rb} | 2 +- spec/models/concerns/dossier_clone_concern_spec.rb | 4 ++-- .../concerns/dossier_searchable_concern_spec.rb | 10 +++++----- spec/models/etablissement_spec.rb | 2 +- spec/models/individual_spec.rb | 7 ++++--- spec/services/dossier_search_service_spec.rb | 4 ++-- spec/system/routing/rules_full_scenario_spec.rb | 2 +- spec/system/users/invite_spec.rb | 2 +- spec/system/users/list_dossiers_spec.rb | 2 +- 16 files changed, 36 insertions(+), 35 deletions(-) create mode 100644 app/jobs/dossier_index_search_terms_job.rb delete mode 100644 app/jobs/dossier_update_search_terms_job.rb rename spec/jobs/{dossier_update_search_terms_job_spec.rb => dossier_index_search_terms_job_spec.rb} (92%) diff --git a/app/jobs/dossier_index_search_terms_job.rb b/app/jobs/dossier_index_search_terms_job.rb new file mode 100644 index 000000000..688171472 --- /dev/null +++ b/app/jobs/dossier_index_search_terms_job.rb @@ -0,0 +1,7 @@ +class DossierIndexSearchTermsJob < ApplicationJob + discard_on ActiveRecord::RecordNotFound + + def perform(dossier) + dossier.index_search_terms + end +end diff --git a/app/jobs/dossier_update_search_terms_job.rb b/app/jobs/dossier_update_search_terms_job.rb deleted file mode 100644 index 5de7c61c8..000000000 --- a/app/jobs/dossier_update_search_terms_job.rb +++ /dev/null @@ -1,7 +0,0 @@ -class DossierUpdateSearchTermsJob < ApplicationJob - discard_on ActiveRecord::RecordNotFound - - def perform(dossier) - dossier.update_search_terms - end -end diff --git a/app/models/concerns/dossier_searchable_concern.rb b/app/models/concerns/dossier_searchable_concern.rb index e26344115..78eb5dd02 100644 --- a/app/models/concerns/dossier_searchable_concern.rb +++ b/app/models/concerns/dossier_searchable_concern.rb @@ -4,13 +4,13 @@ module DossierSearchableConcern extend ActiveSupport::Concern included do - after_commit :update_search_terms_later + after_commit :index_search_terms_later SEARCH_TERMS_DEBOUNCE = 30.seconds - kredis_flag :debounce_update_search_terms_flag + kredis_flag :debounce_index_search_terms_flag - def update_search_terms + def index_search_terms DossierPreloader.load_one(self) search_terms = [ @@ -28,11 +28,11 @@ module DossierSearchableConcern self.class.connection.execute(sanitized_sql) end - def update_search_terms_later - return if debounce_update_search_terms_flag.marked? + def index_search_terms_later + return if debounce_index_search_terms_flag.marked? - debounce_update_search_terms_flag.mark(expires_in: SEARCH_TERMS_DEBOUNCE) - DossierUpdateSearchTermsJob.set(wait: SEARCH_TERMS_DEBOUNCE).perform_later(self) + debounce_index_search_terms_flag.mark(expires_in: SEARCH_TERMS_DEBOUNCE) + DossierIndexSearchTermsJob.set(wait: SEARCH_TERMS_DEBOUNCE).perform_later(self) end end end diff --git a/app/models/etablissement.rb b/app/models/etablissement.rb index d57bc08b2..4987c4785 100644 --- a/app/models/etablissement.rb +++ b/app/models/etablissement.rb @@ -17,7 +17,7 @@ class Etablissement < ApplicationRecord fermé: "fermé" }, _prefix: true - after_commit -> { dossier&.debounce_update_search_terms } + after_commit -> { dossier&.index_search_terms_later } def entreprise_raison_sociale read_attribute(:entreprise_raison_sociale).presence || raison_sociale_for_ei diff --git a/app/models/individual.rb b/app/models/individual.rb index 6e52045b4..47e07af22 100644 --- a/app/models/individual.rb +++ b/app/models/individual.rb @@ -17,7 +17,7 @@ class Individual < ApplicationRecord validates :email, presence: true, if: -> { dossier.for_tiers? && self.email? }, on: :update - after_commit -> { dossier.debounce_update_search_terms }, if: -> { nom_previously_changed? || prenom_previously_changed? } + after_commit -> { dossier.index_search_terms_later }, if: -> { nom_previously_changed? || prenom_previously_changed? } GENDER_MALE = "M." GENDER_FEMALE = 'Mme' diff --git a/config/initializers/transition_to_sidekiq.rb b/config/initializers/transition_to_sidekiq.rb index 419385827..a4c8e961a 100644 --- a/config/initializers/transition_to_sidekiq.rb +++ b/config/initializers/transition_to_sidekiq.rb @@ -44,7 +44,7 @@ if Rails.env.production? && SIDEKIQ_ENABLED self.queue_adapter = :sidekiq end - class DossierUpdateSearchTermsJob < ApplicationJob + class DossierIndexSearchTermsJob < ApplicationJob self.queue_adapter = :sidekiq end diff --git a/spec/controllers/recherche_controller_spec.rb b/spec/controllers/recherche_controller_spec.rb index 91bfe25fe..b8d87a0ee 100644 --- a/spec/controllers/recherche_controller_spec.rb +++ b/spec/controllers/recherche_controller_spec.rb @@ -28,7 +28,7 @@ describe RechercheController, type: :controller do dossier_with_expert.champs_private[1].value = "Dossier B is invalid" dossier_with_expert.save! - perform_enqueued_jobs(only: DossierUpdateSearchTermsJob) + perform_enqueued_jobs(only: DossierIndexSearchTermsJob) end describe 'GET #index' do diff --git a/spec/jobs/dossier_update_search_terms_job_spec.rb b/spec/jobs/dossier_index_search_terms_job_spec.rb similarity index 92% rename from spec/jobs/dossier_update_search_terms_job_spec.rb rename to spec/jobs/dossier_index_search_terms_job_spec.rb index 3dfda131f..9cb016ec9 100644 --- a/spec/jobs/dossier_update_search_terms_job_spec.rb +++ b/spec/jobs/dossier_index_search_terms_job_spec.rb @@ -1,4 +1,4 @@ -RSpec.describe DossierUpdateSearchTermsJob, type: :job do +RSpec.describe DossierIndexSearchTermsJob, type: :job do let(:dossier) { create(:dossier) } subject(:perform_job) { described_class.perform_now(dossier.reload) } diff --git a/spec/models/concerns/dossier_clone_concern_spec.rb b/spec/models/concerns/dossier_clone_concern_spec.rb index 2763d3bf7..6f182b1c3 100644 --- a/spec/models/concerns/dossier_clone_concern_spec.rb +++ b/spec/models/concerns/dossier_clone_concern_spec.rb @@ -51,7 +51,7 @@ RSpec.describe DossierCloneConcern do it "updates search terms" do subject - perform_enqueued_jobs(only: DossierUpdateSearchTermsJob) + perform_enqueued_jobs(only: DossierIndexSearchTermsJob) sql = "SELECT search_terms, private_search_terms FROM dossiers where id = :id" result = Dossier.connection.execute(Dossier.sanitize_sql_array([sql, id: new_dossier.id])).first @@ -338,7 +338,7 @@ RSpec.describe DossierCloneConcern do it { expect { subject }.to change { dossier.reload.champs.size }.by(0) } it { expect { subject }.not_to change { dossier.reload.champs.order(:created_at).reject { _1.stable_id.in?([99, 994]) }.map(&:value) } } - it { expect { subject }.to have_enqueued_job(DossierUpdateSearchTermsJob).with(dossier) } + it { expect { subject }.to have_enqueued_job(DossierIndexSearchTermsJob).with(dossier) } it { expect { subject }.to change { dossier.reload.champs.find { _1.stable_id == 99 }.value }.from('old value').to('new value') } it { expect { subject }.to change { dossier.reload.champs.find { _1.stable_id == 994 }.value }.from('old value').to('new value in repetition') } diff --git a/spec/models/concerns/dossier_searchable_concern_spec.rb b/spec/models/concerns/dossier_searchable_concern_spec.rb index 343f31795..ae9325d7d 100644 --- a/spec/models/concerns/dossier_searchable_concern_spec.rb +++ b/spec/models/concerns/dossier_searchable_concern_spec.rb @@ -2,7 +2,7 @@ describe DossierSearchableConcern do let(:champ_public) { dossier.champs_public.first } let(:champ_private) { dossier.champs_private.first } - describe '#update_search_terms' do + describe '#index_search_terms' do let(:etablissement) { dossier.etablissement } let(:dossier) { create(:dossier, :with_entreprise, user: user) } let(:etablissement) { build(:etablissement, entreprise_nom: 'Dupont', entreprise_prenom: 'Thomas', association_rna: '12345', association_titre: 'asso de test', association_objet: 'tests unitaires') } @@ -20,7 +20,7 @@ describe DossierSearchableConcern do it "update columns" do champ_public.update_attribute(:value, "champ public") champ_private.update_attribute(:value, "champ privé") - perform_enqueued_jobs(only: DossierUpdateSearchTermsJob) + perform_enqueued_jobs(only: DossierIndexSearchTermsJob) expect(result["search_terms"]).to eq("#{user.email} champ public #{etablissement.entreprise_siren} #{etablissement.entreprise_numero_tva_intracommunautaire} #{etablissement.entreprise_forme_juridique} #{etablissement.entreprise_forme_juridique_code} #{etablissement.entreprise_nom_commercial} #{etablissement.entreprise_raison_sociale} #{etablissement.entreprise_siret_siege_social} #{etablissement.entreprise_nom} #{etablissement.entreprise_prenom} #{etablissement.association_rna} #{etablissement.association_titre} #{etablissement.association_objet} #{etablissement.siret} #{etablissement.naf} #{etablissement.libelle_naf} #{etablissement.adresse} #{etablissement.code_postal} #{etablissement.localite} #{etablissement.code_insee_localite}") expect(result["private_search_terms"]).to eq('champ privé') @@ -37,21 +37,21 @@ describe DossierSearchableConcern do champs_private_attributes: [{ id: champ_private.id, value: 'nouvelle valeur privee' }] ) - perform_enqueued_jobs(only: DossierUpdateSearchTermsJob) + perform_enqueued_jobs(only: DossierIndexSearchTermsJob) expect(result["search_terms"]).to include('nouvelle valeur publique') expect(result["private_search_terms"]).to include('nouvelle valeur privee') end it "debounce jobs" do - assert_enqueued_jobs(1, only: DossierUpdateSearchTermsJob) do + assert_enqueued_jobs(1, only: DossierIndexSearchTermsJob) do 3.times { dossier.index_search_terms_later } end # wait redis key expiration sleep 1.01.seconds - assert_enqueued_jobs(1, only: DossierUpdateSearchTermsJob) do + assert_enqueued_jobs(1, only: DossierIndexSearchTermsJob) do dossier.update(champs_public_attributes: [{ id: champ_public.id, value: rand(10).to_s }]) end end diff --git a/spec/models/etablissement_spec.rb b/spec/models/etablissement_spec.rb index 7653c79cd..3b46462f2 100644 --- a/spec/models/etablissement_spec.rb +++ b/spec/models/etablissement_spec.rb @@ -119,7 +119,7 @@ describe Etablissement do let(:etablissement) { create(:etablissement, dossier: build(:dossier)) } it "schedule update search terms" do - assert_enqueued_jobs(1, only: DossierUpdateSearchTermsJob) do + assert_enqueued_jobs(1, only: DossierIndexSearchTermsJob) do etablissement.update(entreprise_nom: "nom") end end diff --git a/spec/models/individual_spec.rb b/spec/models/individual_spec.rb index a37d1e15b..6f8fb7c6f 100644 --- a/spec/models/individual_spec.rb +++ b/spec/models/individual_spec.rb @@ -36,9 +36,10 @@ describe Individual do end end - it "schedule update search terms" do - assert_enqueued_jobs(1, only: DossierUpdateSearchTermsJob) do - subject + it "schedule index search terms" do + subject.dossier.debounce_index_search_terms_flag.remove + assert_enqueued_jobs(1, only: DossierIndexSearchTermsJob) do + individual.update(nom: "new name") end end end diff --git a/spec/services/dossier_search_service_spec.rb b/spec/services/dossier_search_service_spec.rb index 0f2baeec0..759f7b60b 100644 --- a/spec/services/dossier_search_service_spec.rb +++ b/spec/services/dossier_search_service_spec.rb @@ -24,7 +24,7 @@ describe DossierSearchService do dossier_3 dossier_archived - perform_enqueued_jobs(only: DossierUpdateSearchTermsJob) + perform_enqueued_jobs(only: DossierIndexSearchTermsJob) end let(:procedure_1) { create(:procedure, :published, administrateur: administrateur_1) } @@ -116,7 +116,7 @@ describe DossierSearchService do dossier_2 dossier_3 dossier_archived - perform_enqueued_jobs(only: DossierUpdateSearchTermsJob) + perform_enqueued_jobs(only: DossierIndexSearchTermsJob) end let(:liste_dossiers) do diff --git a/spec/system/routing/rules_full_scenario_spec.rb b/spec/system/routing/rules_full_scenario_spec.rb index d72fd0c46..2729a71ce 100644 --- a/spec/system/routing/rules_full_scenario_spec.rb +++ b/spec/system/routing/rules_full_scenario_spec.rb @@ -134,7 +134,7 @@ describe 'The routing with rules', js: true do user_send_dossier(litteraire_user, 'littéraire') user_send_dossier(artistique_user, 'artistique') - perform_enqueued_jobs(only: DossierUpdateSearchTermsJob) + perform_enqueued_jobs(only: DossierIndexSearchTermsJob) # the litteraires instructeurs only manage the litteraires dossiers register_instructeur_and_log_in(victor.email) diff --git a/spec/system/users/invite_spec.rb b/spec/system/users/invite_spec.rb index 2d681a49b..15c8a466b 100644 --- a/spec/system/users/invite_spec.rb +++ b/spec/system/users/invite_spec.rb @@ -162,7 +162,7 @@ describe 'Invitations' do before do navigate_to_invited_dossier(invite) visit dossiers_path - perform_enqueued_jobs(only: DossierUpdateSearchTermsJob) + perform_enqueued_jobs(only: DossierIndexSearchTermsJob) end it "can search by id and it displays the dossier" do diff --git a/spec/system/users/list_dossiers_spec.rb b/spec/system/users/list_dossiers_spec.rb index 6fc525e04..eaa32c284 100644 --- a/spec/system/users/list_dossiers_spec.rb +++ b/spec/system/users/list_dossiers_spec.rb @@ -268,7 +268,7 @@ describe 'user access to the list of their dossiers', js: true do context 'when it matches multiple dossiers' do let!(:dossier_with_champs) { create(:dossier, :with_populated_champs, :en_construction, user: user) } before do - perform_enqueued_jobs(only: DossierUpdateSearchTermsJob) + perform_enqueued_jobs(only: DossierIndexSearchTermsJob) find('.fr-search-bar .fr-btn').click end From 6733b2884f5af7902ff6fc24c005e847ad1b652c Mon Sep 17 00:00:00 2001 From: Colin Darie Date: Thu, 25 Apr 2024 17:46:21 +0200 Subject: [PATCH 7/9] feat(search): index mandataire name --- app/models/concerns/dossier_searchable_concern.rb | 4 +++- spec/models/concerns/dossier_searchable_concern_spec.rb | 8 ++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/app/models/concerns/dossier_searchable_concern.rb b/app/models/concerns/dossier_searchable_concern.rb index 78eb5dd02..2b52fe668 100644 --- a/app/models/concerns/dossier_searchable_concern.rb +++ b/app/models/concerns/dossier_searchable_concern.rb @@ -18,7 +18,9 @@ module DossierSearchableConcern *champs_public.flat_map(&:search_terms), *etablissement&.search_terms, individual&.nom, - individual&.prenom + individual&.prenom, + mandataire_first_name, + mandataire_last_name ].compact_blank.join(' ') private_search_terms = champs_private.flat_map(&:search_terms).compact_blank.join(' ') diff --git a/spec/models/concerns/dossier_searchable_concern_spec.rb b/spec/models/concerns/dossier_searchable_concern_spec.rb index ae9325d7d..069a6696e 100644 --- a/spec/models/concerns/dossier_searchable_concern_spec.rb +++ b/spec/models/concerns/dossier_searchable_concern_spec.rb @@ -56,5 +56,13 @@ describe DossierSearchableConcern do end end end + + context 'mandataire' do + it "update columns" do + dossier.update(mandataire_first_name: "Chris") + perform_enqueued_jobs(only: DossierIndexSearchTermsJob) + expect(result["search_terms"]).to include("Chris") + end + end end end From e01f5b7993b44e1eccdf651a7d959e347e12258b Mon Sep 17 00:00:00 2001 From: Colin Darie Date: Thu, 25 Apr 2024 18:48:14 +0200 Subject: [PATCH 8/9] refactor(search): index search terms only when necessary --- .../instructeurs/dossiers_controller.rb | 1 + app/models/concerns/dossier_clone_concern.rb | 2 ++ .../concerns/dossier_searchable_concern.rb | 2 +- app/models/concerns/dossier_state_concern.rb | 2 ++ .../instructeurs/dossiers_controller_spec.rb | 1 + .../concerns/dossier_clone_concern_spec.rb | 11 ++++++++-- .../dossier_searchable_concern_spec.rb | 20 ++++++++++++++++--- 7 files changed, 33 insertions(+), 6 deletions(-) diff --git a/app/controllers/instructeurs/dossiers_controller.rb b/app/controllers/instructeurs/dossiers_controller.rb index 8063e0438..b8a40d673 100644 --- a/app/controllers/instructeurs/dossiers_controller.rb +++ b/app/controllers/instructeurs/dossiers_controller.rb @@ -280,6 +280,7 @@ module Instructeurs end dossier.save(context: :champs_private_value) + dossier.index_search_terms_later respond_to do |format| format.turbo_stream do diff --git a/app/models/concerns/dossier_clone_concern.rb b/app/models/concerns/dossier_clone_concern.rb index 86a135ae7..913af3e45 100644 --- a/app/models/concerns/dossier_clone_concern.rb +++ b/app/models/concerns/dossier_clone_concern.rb @@ -71,6 +71,7 @@ module DossierCloneConcern touch(:last_champ_updated_at) end reload + index_search_terms_later editing_fork.destroy_editing_fork! end @@ -119,6 +120,7 @@ module DossierCloneConcern end end + cloned_dossier.index_search_terms_later if !fork cloned_dossier.reload end diff --git a/app/models/concerns/dossier_searchable_concern.rb b/app/models/concerns/dossier_searchable_concern.rb index 2b52fe668..24457b68b 100644 --- a/app/models/concerns/dossier_searchable_concern.rb +++ b/app/models/concerns/dossier_searchable_concern.rb @@ -4,7 +4,7 @@ module DossierSearchableConcern extend ActiveSupport::Concern included do - after_commit :index_search_terms_later + after_commit :index_search_terms_later, if: -> { previously_new_record? || user_previously_changed? || mandataire_first_name_previously_changed? || mandataire_last_name_previously_changed? } SEARCH_TERMS_DEBOUNCE = 30.seconds diff --git a/app/models/concerns/dossier_state_concern.rb b/app/models/concerns/dossier_state_concern.rb index 1c99ffb65..e90902066 100644 --- a/app/models/concerns/dossier_state_concern.rb +++ b/app/models/concerns/dossier_state_concern.rb @@ -13,6 +13,8 @@ module DossierStateConcern MailTemplatePresenterService.create_commentaire_for_state(self, Dossier.states.fetch(:en_construction)) procedure.compute_dossiers_count + + index_search_terms_later end def after_commit_passer_en_construction diff --git a/spec/controllers/instructeurs/dossiers_controller_spec.rb b/spec/controllers/instructeurs/dossiers_controller_spec.rb index f4f065948..458654a6f 100644 --- a/spec/controllers/instructeurs/dossiers_controller_spec.rb +++ b/spec/controllers/instructeurs/dossiers_controller_spec.rb @@ -1041,6 +1041,7 @@ describe Instructeurs::DossiersController, type: :controller do expect(champ_drop_down_list.value).to eq('other value') expect(dossier.reload.last_champ_private_updated_at).to eq(now) expect(response).to have_http_status(200) + assert_enqueued_jobs(1, only: DossierIndexSearchTermsJob) } it 'updates the annotations' do diff --git a/spec/models/concerns/dossier_clone_concern_spec.rb b/spec/models/concerns/dossier_clone_concern_spec.rb index 6f182b1c3..234f4de53 100644 --- a/spec/models/concerns/dossier_clone_concern_spec.rb +++ b/spec/models/concerns/dossier_clone_concern_spec.rb @@ -49,9 +49,15 @@ RSpec.describe DossierCloneConcern do end it "updates search terms" do - subject + # In spec, dossier and flag reference are created just before deep clone, + # which keep the flag reference from the original, pointing to the original id. + # We have to remove the flag reference before the clone + dossier.remove_instance_variable(:@debounce_index_search_terms_flag_kredis_flag) + + perform_enqueued_jobs(only: DossierIndexSearchTermsJob) do + subject + end - perform_enqueued_jobs(only: DossierIndexSearchTermsJob) sql = "SELECT search_terms, private_search_terms FROM dossiers where id = :id" result = Dossier.connection.execute(Dossier.sanitize_sql_array([sql, id: new_dossier.id])).first @@ -334,6 +340,7 @@ RSpec.describe DossierCloneConcern do end updated_champ.update(value: 'new value') updated_repetition_champ.update(value: 'new value in repetition') + dossier.debounce_index_search_terms_flag.remove end it { expect { subject }.to change { dossier.reload.champs.size }.by(0) } diff --git a/spec/models/concerns/dossier_searchable_concern_spec.rb b/spec/models/concerns/dossier_searchable_concern_spec.rb index 069a6696e..273a2e3fe 100644 --- a/spec/models/concerns/dossier_searchable_concern_spec.rb +++ b/spec/models/concerns/dossier_searchable_concern_spec.rb @@ -29,14 +29,22 @@ describe DossierSearchableConcern do context 'with an update' do before do stub_const("DossierSearchableConcern::SEARCH_TERMS_DEBOUNCE", 1.second) + + # dossier creation trigger a first indexation and flag, + # so we we have to remove this flag + dossier.debounce_index_search_terms_flag.remove end - it "update columns" do + it "update columns en construction" do dossier.update( champs_public_attributes: [{ id: champ_public.id, value: 'nouvelle valeur publique' }], champs_private_attributes: [{ id: champ_private.id, value: 'nouvelle valeur privee' }] ) + assert_enqueued_jobs(1, only: DossierIndexSearchTermsJob) do + dossier.passer_en_construction + end + perform_enqueued_jobs(only: DossierIndexSearchTermsJob) expect(result["search_terms"]).to include('nouvelle valeur publique') @@ -52,15 +60,21 @@ describe DossierSearchableConcern do sleep 1.01.seconds assert_enqueued_jobs(1, only: DossierIndexSearchTermsJob) do - dossier.update(champs_public_attributes: [{ id: champ_public.id, value: rand(10).to_s }]) + dossier.index_search_terms_later end end end context 'mandataire' do it "update columns" do - dossier.update(mandataire_first_name: "Chris") + dossier.debounce_index_search_terms_flag.remove + + assert_enqueued_jobs(1, only: DossierIndexSearchTermsJob) do + dossier.update!(mandataire_first_name: "Chris") + end + perform_enqueued_jobs(only: DossierIndexSearchTermsJob) + expect(result["search_terms"]).to include("Chris") end end From 71c1d7ac9ac0a3a08622c0e2c7114174cbe8ef07 Mon Sep 17 00:00:00 2001 From: Colin Darie Date: Mon, 13 May 2024 15:55:39 +0200 Subject: [PATCH 9/9] feat(brouillon): index search terms debounced when updating brouillon --- app/controllers/users/dossiers_controller.rb | 2 ++ spec/controllers/users/dossiers_controller_spec.rb | 10 ++++++++++ 2 files changed, 12 insertions(+) diff --git a/app/controllers/users/dossiers_controller.rb b/app/controllers/users/dossiers_controller.rb index 92f4d00a8..1b9e1c05c 100644 --- a/app/controllers/users/dossiers_controller.rb +++ b/app/controllers/users/dossiers_controller.rb @@ -296,6 +296,8 @@ module Users @dossier = dossier_with_champs(pj_template: false) @errors = update_dossier_and_compute_errors + @dossier.index_search_terms_later if @errors.empty? + respond_to do |format| format.turbo_stream do @to_show, @to_hide, @to_update = champs_to_turbo_update(champs_public_attributes_params, dossier.champs.filter(&:public?)) diff --git a/spec/controllers/users/dossiers_controller_spec.rb b/spec/controllers/users/dossiers_controller_spec.rb index ad99422a4..e0fd3501e 100644 --- a/spec/controllers/users/dossiers_controller_spec.rb +++ b/spec/controllers/users/dossiers_controller_spec.rb @@ -770,6 +770,16 @@ describe Users::DossiersController, type: :controller do end end end + + it "debounce search terms indexation" do + # dossier creation trigger a first indexation and flag, + # so we we have to remove this flag + dossier.debounce_index_search_terms_flag.remove + + assert_enqueued_jobs(1, only: DossierIndexSearchTermsJob) do + 3.times { patch :update, params: payload, format: :turbo_stream } + end + end end describe '#update en_construction' do