From 2e763d5e92fc25fb47e3f48bdbd01898238cf546 Mon Sep 17 00:00:00 2001 From: Colin Darie Date: Wed, 3 Apr 2024 10:21:50 +0200 Subject: [PATCH] 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