From 5cd780251b7949b3d97c3fb503e3e1b998f75b06 Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Tue, 16 Apr 2024 11:12:32 +0200 Subject: [PATCH 1/5] refactor: mutualize email_expert logic --- app/controllers/experts/avis_controller.rb | 6 +--- .../instructeurs/dossiers_controller.rb | 12 ++------ app/models/expert.rb | 8 +++++ .../experts/avis_controller_spec.rb | 18 ++---------- spec/models/expert_spec.rb | 29 +++++++++++++++++++ 5 files changed, 42 insertions(+), 31 deletions(-) diff --git a/app/controllers/experts/avis_controller.rb b/app/controllers/experts/avis_controller.rb index 5c9ecb34c..a66dec92f 100644 --- a/app/controllers/experts/avis_controller.rb +++ b/app/controllers/experts/avis_controller.rb @@ -83,11 +83,7 @@ module Experts def avis_new @new_avis = Avis.new - if @dossier.procedure.experts_require_administrateur_invitation? - @experts_emails = @dossier.procedure.experts_procedures.where(revoked_at: nil).map { _1.expert.email }.sort - else - @experts_emails = @dossier.procedure.experts.map(&:email).sort - end + @experts_emails = Expert.autocomplete_mails(@dossier.procedure) end def create_avis diff --git a/app/controllers/instructeurs/dossiers_controller.rb b/app/controllers/instructeurs/dossiers_controller.rb index 340a919e3..c4d013794 100644 --- a/app/controllers/instructeurs/dossiers_controller.rb +++ b/app/controllers/instructeurs/dossiers_controller.rb @@ -64,21 +64,13 @@ module Instructeurs def avis @avis_seen_at = current_instructeur.follows.find_by(dossier: dossier)&.avis_seen_at @avis = Avis.new - if @dossier.procedure.experts_require_administrateur_invitation? - @experts_emails = dossier.procedure.experts_procedures.where(revoked_at: nil).map(&:expert).map(&:email).sort - else - @experts_emails = @dossier.procedure.experts.map(&:email).sort - end + @experts_emails = Expert.autocomplete_mails(@dossier.procedure) end def avis_new @avis_seen_at = current_instructeur.follows.find_by(dossier: dossier)&.avis_seen_at @avis = Avis.new - if @dossier.procedure.experts_require_administrateur_invitation? - @experts_emails = dossier.procedure.experts_procedures.where(revoked_at: nil).map(&:expert).map(&:email).sort - else - @experts_emails = @dossier.procedure.experts.map(&:email).sort - end + @experts_emails = Expert.autocomplete_mails(@dossier.procedure) end def personnes_impliquees diff --git a/app/models/expert.rb b/app/models/expert.rb index 3809a7e27..4de28eec3 100644 --- a/app/models/expert.rb +++ b/app/models/expert.rb @@ -29,6 +29,14 @@ class Expert < ApplicationRecord end end + def self.autocomplete_mails(procedure) + if procedure.experts_require_administrateur_invitation? + procedure.experts_procedures.where(revoked_at: nil).map(&:expert).map(&:email).sort + else + procedure.experts.map(&:email).sort + end + end + def merge(old_expert) return if old_expert.nil? diff --git a/spec/controllers/experts/avis_controller_spec.rb b/spec/controllers/experts/avis_controller_spec.rb index 946474bb3..5d5786ddf 100644 --- a/spec/controllers/experts/avis_controller_spec.rb +++ b/spec/controllers/experts/avis_controller_spec.rb @@ -355,26 +355,12 @@ describe Experts::AvisController, type: :controller do end describe '#avis_new' do - let!(:revoked_expert) { create(:experts_procedure, revoked_at: 2.days.ago, procedure: procedure, expert: create(:expert)).expert } before do + allow(Expert).to receive(:autocomplete_mails).and_return([]) get :avis_new, params: { procedure_id: procedure.id, id: avis_without_answer.id } end - context 'when procedure experts need administrateur invitation' do - let!(:procedure) { create(:procedure, experts_require_administrateur_invitation: true) } - it 'limit invited email list to not revoked experts' do - expect(assigns(:experts_emails)).to include(experts_procedure.expert.user.email) - expect(assigns(:experts_emails)).not_to include(revoked_expert.user.email) - end - end - context 'when procedure experts can be anyone' do - let!(:procedure) { create(:procedure, experts_require_administrateur_invitation: false) } - - it 'prefill autocomplete with all experts in the procedure' do - expect(assigns(:experts_emails)).to include(experts_procedure.expert.user.email) - expect(assigns(:experts_emails)).to include(revoked_expert.user.email) - end - end + it { expect(Expert).to have_received(:autocomplete_mails).with(procedure) } end describe '#create_avis' do diff --git a/spec/models/expert_spec.rb b/spec/models/expert_spec.rb index 8f187b5e1..0ec05616d 100644 --- a/spec/models/expert_spec.rb +++ b/spec/models/expert_spec.rb @@ -94,4 +94,33 @@ RSpec.describe Expert, type: :model do end end end + + describe '.autocomplete_mails' do + subject { Expert.autocomplete_mails(procedure) } + + let(:procedure) { create(:procedure, experts_require_administrateur_invitation: true) } + let(:expert) { create(:expert) } + let(:revoked_expert) { create(:expert) } + + before do + procedure.experts << expert << revoked_expert + ExpertsProcedure.find_by(expert: revoked_expert, procedure: procedure) + .update!(revoked_at: 1.day.ago) + end + + context 'when procedure experts need administrateur invitation' do + + it 'returns only not revoked experts' do + expect(subject).to eq([expert.user.email]) + end + end + + context 'when procedure experts can be anyone' do + let(:procedure) { create(:procedure, experts_require_administrateur_invitation: false) } + + it 'prefill autocomplete with all experts in the procedure' do + expect(subject).to eq([expert.user.email, revoked_expert.user.email]) + end + end + end end From 15026353277e911226d3844e560e1cf23f71af7b Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Tue, 16 Apr 2024 13:33:37 +0200 Subject: [PATCH 2/5] refactor: experts emails pluck --- app/models/expert.rb | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/app/models/expert.rb b/app/models/expert.rb index 4de28eec3..c0ee5cf8f 100644 --- a/app/models/expert.rb +++ b/app/models/expert.rb @@ -30,11 +30,18 @@ class Expert < ApplicationRecord end def self.autocomplete_mails(procedure) + experts = Expert + .joins(:experts_procedures, :user) + .where(experts_procedures: { procedure: procedure }) + if procedure.experts_require_administrateur_invitation? - procedure.experts_procedures.where(revoked_at: nil).map(&:expert).map(&:email).sort - else - procedure.experts.map(&:email).sort + experts = experts + .where(experts_procedures: { revoked_at: nil }) end + + experts + .pluck('users.email') + .sort end def merge(old_expert) From 8d95df225038817e8f0cc6014652b8316f1d9d56 Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Tue, 16 Apr 2024 15:26:06 +0200 Subject: [PATCH 3/5] feat: do not suggest unconfirmed experts --- app/models/expert.rb | 1 + spec/models/expert_spec.rb | 9 +++++---- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/app/models/expert.rb b/app/models/expert.rb index c0ee5cf8f..906e0e6c3 100644 --- a/app/models/expert.rb +++ b/app/models/expert.rb @@ -33,6 +33,7 @@ class Expert < ApplicationRecord experts = Expert .joins(:experts_procedures, :user) .where(experts_procedures: { procedure: procedure }) + .where.not(users: { confirmed_at: nil }) if procedure.experts_require_administrateur_invitation? experts = experts diff --git a/spec/models/expert_spec.rb b/spec/models/expert_spec.rb index 0ec05616d..89f53f82a 100644 --- a/spec/models/expert_spec.rb +++ b/spec/models/expert_spec.rb @@ -101,16 +101,17 @@ RSpec.describe Expert, type: :model do let(:procedure) { create(:procedure, experts_require_administrateur_invitation: true) } let(:expert) { create(:expert) } let(:revoked_expert) { create(:expert) } + let(:unconfirmed_expert) { create(:expert) } before do - procedure.experts << expert << revoked_expert + procedure.experts << expert << revoked_expert << unconfirmed_expert ExpertsProcedure.find_by(expert: revoked_expert, procedure: procedure) .update!(revoked_at: 1.day.ago) + unconfirmed_expert.user.update!(confirmed_at: nil) end context 'when procedure experts need administrateur invitation' do - - it 'returns only not revoked experts' do + it 'returns only confirmed not revoked experts' do expect(subject).to eq([expert.user.email]) end end @@ -118,7 +119,7 @@ RSpec.describe Expert, type: :model do context 'when procedure experts can be anyone' do let(:procedure) { create(:procedure, experts_require_administrateur_invitation: false) } - it 'prefill autocomplete with all experts in the procedure' do + it 'prefill autocomplete with all confirmed experts in the procedure' do expect(subject).to eq([expert.user.email, revoked_expert.user.email]) end end From d95ce505a890bef81ca2b69135fe58ba6d1546e4 Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Tue, 16 Apr 2024 15:47:13 +0200 Subject: [PATCH 4/5] feat: suggest recently added user even if its not confirmed --- app/models/expert.rb | 15 ++++++++++----- spec/models/expert_spec.rb | 10 ++++++---- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/app/models/expert.rb b/app/models/expert.rb index 906e0e6c3..a0bcc13a5 100644 --- a/app/models/expert.rb +++ b/app/models/expert.rb @@ -30,17 +30,22 @@ class Expert < ApplicationRecord end def self.autocomplete_mails(procedure) - experts = Expert + procedure_experts = Expert .joins(:experts_procedures, :user) .where(experts_procedures: { procedure: procedure }) - .where.not(users: { confirmed_at: nil }) - if procedure.experts_require_administrateur_invitation? - experts = experts + new_or_confirmed_experts = procedure_experts + .where.not(users: { confirmed_at: nil }) + .or(procedure_experts.where(users: { created_at: 1.day.ago.. })) + + suggested_expert = if procedure.experts_require_administrateur_invitation? + new_or_confirmed_experts .where(experts_procedures: { revoked_at: nil }) + else + new_or_confirmed_experts end - experts + suggested_expert .pluck('users.email') .sort end diff --git a/spec/models/expert_spec.rb b/spec/models/expert_spec.rb index 89f53f82a..98aef6b9d 100644 --- a/spec/models/expert_spec.rb +++ b/spec/models/expert_spec.rb @@ -102,17 +102,19 @@ RSpec.describe Expert, type: :model do let(:expert) { create(:expert) } let(:revoked_expert) { create(:expert) } let(:unconfirmed_expert) { create(:expert) } + let(:new_unconfirmed_expert) { create(:expert) } before do - procedure.experts << expert << revoked_expert << unconfirmed_expert + procedure.experts << expert << revoked_expert << unconfirmed_expert << new_unconfirmed_expert ExpertsProcedure.find_by(expert: revoked_expert, procedure: procedure) .update!(revoked_at: 1.day.ago) - unconfirmed_expert.user.update!(confirmed_at: nil) + unconfirmed_expert.user.update!(confirmed_at: nil, created_at: 2.days.ago) + new_unconfirmed_expert.user.update!(confirmed_at: nil) end context 'when procedure experts need administrateur invitation' do it 'returns only confirmed not revoked experts' do - expect(subject).to eq([expert.user.email]) + expect(subject).to eq([expert.user.email, new_unconfirmed_expert.user.email]) end end @@ -120,7 +122,7 @@ RSpec.describe Expert, type: :model do let(:procedure) { create(:procedure, experts_require_administrateur_invitation: false) } it 'prefill autocomplete with all confirmed experts in the procedure' do - expect(subject).to eq([expert.user.email, revoked_expert.user.email]) + expect(subject).to eq([expert.user.email, revoked_expert.user.email, new_unconfirmed_expert.user.email]) end end end From 981003975cfb41ad9cf69b56e5ee091895202fbb Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Tue, 16 Apr 2024 16:16:05 +0200 Subject: [PATCH 5/5] refactor: use last_sign_in_at as confirmed_at is not reliable --- app/models/expert.rb | 2 +- spec/models/expert_spec.rb | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/models/expert.rb b/app/models/expert.rb index a0bcc13a5..1bf87e7a8 100644 --- a/app/models/expert.rb +++ b/app/models/expert.rb @@ -35,7 +35,7 @@ class Expert < ApplicationRecord .where(experts_procedures: { procedure: procedure }) new_or_confirmed_experts = procedure_experts - .where.not(users: { confirmed_at: nil }) + .where.not(users: { last_sign_in_at: nil }) .or(procedure_experts.where(users: { created_at: 1.day.ago.. })) suggested_expert = if procedure.experts_require_administrateur_invitation? diff --git a/spec/models/expert_spec.rb b/spec/models/expert_spec.rb index 98aef6b9d..3ac083910 100644 --- a/spec/models/expert_spec.rb +++ b/spec/models/expert_spec.rb @@ -101,20 +101,20 @@ RSpec.describe Expert, type: :model do let(:procedure) { create(:procedure, experts_require_administrateur_invitation: true) } let(:expert) { create(:expert) } let(:revoked_expert) { create(:expert) } - let(:unconfirmed_expert) { create(:expert) } - let(:new_unconfirmed_expert) { create(:expert) } + let(:unsigned_expert) { create(:expert) } + let(:new_unsigned_expert) { create(:expert) } before do - procedure.experts << expert << revoked_expert << unconfirmed_expert << new_unconfirmed_expert + procedure.experts << expert << revoked_expert << unsigned_expert << new_unsigned_expert ExpertsProcedure.find_by(expert: revoked_expert, procedure: procedure) .update!(revoked_at: 1.day.ago) - unconfirmed_expert.user.update!(confirmed_at: nil, created_at: 2.days.ago) - new_unconfirmed_expert.user.update!(confirmed_at: nil) + unsigned_expert.user.update!(last_sign_in_at: nil, created_at: 2.days.ago) + new_unsigned_expert.user.update!(last_sign_in_at: nil) end context 'when procedure experts need administrateur invitation' do it 'returns only confirmed not revoked experts' do - expect(subject).to eq([expert.user.email, new_unconfirmed_expert.user.email]) + expect(subject).to eq([expert.user.email, new_unsigned_expert.user.email].sort) end end @@ -122,7 +122,7 @@ RSpec.describe Expert, type: :model do let(:procedure) { create(:procedure, experts_require_administrateur_invitation: false) } it 'prefill autocomplete with all confirmed experts in the procedure' do - expect(subject).to eq([expert.user.email, revoked_expert.user.email, new_unconfirmed_expert.user.email]) + expect(subject).to eq([expert.user.email, revoked_expert.user.email, new_unsigned_expert.user.email].sort) end end end