From 8dc966feb44c41b2de814e6cf3ff65f4826c0503 Mon Sep 17 00:00:00 2001 From: Colin Darie Date: Tue, 2 Aug 2022 12:37:29 +0200 Subject: [PATCH 1/5] fix(dossier_transfer): requires an actual email Closes #7621 --- app/models/dossier_transfer.rb | 2 ++ config/locales/models/dossier_transfer/en.yml | 9 +++++++ config/locales/models/dossier_transfer/fr.yml | 9 +++++++ spec/models/dossier_transfer_spec.rb | 26 +++++++++++++++++++ 4 files changed, 46 insertions(+) create mode 100644 config/locales/models/dossier_transfer/en.yml create mode 100644 config/locales/models/dossier_transfer/fr.yml diff --git a/app/models/dossier_transfer.rb b/app/models/dossier_transfer.rb index 4ae5f88b3..8fbf6ec1d 100644 --- a/app/models/dossier_transfer.rb +++ b/app/models/dossier_transfer.rb @@ -12,6 +12,8 @@ class DossierTransfer < ApplicationRecord EXPIRATION_LIMIT = 2.weeks + validates :email, format: { with: Devise.email_regexp } + scope :pending, -> { where('created_at > ?', (Time.zone.now - EXPIRATION_LIMIT)) } scope :stale, -> { where('created_at < ?', (Time.zone.now - EXPIRATION_LIMIT)) } scope :with_dossiers, -> { joins(:dossiers).merge(Dossier.visible_by_user) } diff --git a/config/locales/models/dossier_transfer/en.yml b/config/locales/models/dossier_transfer/en.yml new file mode 100644 index 000000000..77c95aa97 --- /dev/null +++ b/config/locales/models/dossier_transfer/en.yml @@ -0,0 +1,9 @@ +en: + activerecord: + errors: + models: + dossier_transfer: + attributes: + email: + format: "Email address %{message}" + invalid: "is invalid" diff --git a/config/locales/models/dossier_transfer/fr.yml b/config/locales/models/dossier_transfer/fr.yml new file mode 100644 index 000000000..751410cef --- /dev/null +++ b/config/locales/models/dossier_transfer/fr.yml @@ -0,0 +1,9 @@ +fr: + activerecord: + errors: + models: + dossier_transfer: + attributes: + email: + format: "L'adresse email %{message}" + invalid: "est invalide" diff --git a/spec/models/dossier_transfer_spec.rb b/spec/models/dossier_transfer_spec.rb index aa2b08a90..06cf2ff2c 100644 --- a/spec/models/dossier_transfer_spec.rb +++ b/spec/models/dossier_transfer_spec.rb @@ -78,4 +78,30 @@ RSpec.describe DossierTransfer, type: :model do expect(DossierTransfer.count).to eq(0) end end + + describe "validation" do + let(:email) { build(:dossier_transfer).email } + + subject { build(:dossier_transfer, email: email) } + + it "factory is valid" do + expect(subject).to be_valid + end + + context "when email is blank" do + let(:email) { "" } + + it "requires a valid email" do + expect(subject).to be_invalid + end + end + + context "when email is not an email" do + let(:email) { "test" } + + it "requires a valid email" do + expect(subject).to be_invalid + end + end + end end From a5d5f4307fb7588477a1ae0c6cc24cb65e0f1616 Mon Sep 17 00:00:00 2001 From: Colin Darie Date: Tue, 2 Aug 2022 13:01:06 +0200 Subject: [PATCH 2/5] fix(dossier_transfer): avoid all transfers without valid email Cf #7621 --- app/controllers/users/profil_controller.rb | 10 ++++++++-- spec/controllers/users/profil_controller_spec.rb | 15 +++++++++++++-- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/app/controllers/users/profil_controller.rb b/app/controllers/users/profil_controller.rb index fced4cd88..945718935 100644 --- a/app/controllers/users/profil_controller.rb +++ b/app/controllers/users/profil_controller.rb @@ -30,8 +30,14 @@ module Users end def transfer_all_dossiers - DossierTransfer.initiate(next_owner_email, current_user.dossiers) - flash.notice = t('.new_transfer', count: current_user.dossiers.count, email: next_owner_email) + transfer = DossierTransfer.initiate(next_owner_email, current_user.dossiers) + + if transfer.valid? + flash.notice = t('.new_transfer', count: current_user.dossiers.count, email: next_owner_email) + else + flash.alert = transfer.errors.full_messages + end + redirect_to profil_path end diff --git a/spec/controllers/users/profil_controller_spec.rb b/spec/controllers/users/profil_controller_spec.rb index 6bc1771a3..06e2961c7 100644 --- a/spec/controllers/users/profil_controller_spec.rb +++ b/spec/controllers/users/profil_controller_spec.rb @@ -133,15 +133,26 @@ describe Users::ProfilController, type: :controller do let(:next_owner) { 'loulou@lou.com' } let(:created_transfer) { DossierTransfer.first } - before do + subject { post :transfer_all_dossiers, params: { next_owner: next_owner } - end + } + + before { subject } it "transfer all dossiers" do expect(created_transfer.email).to eq(next_owner) expect(created_transfer.dossiers).to match_array(dossiers) expect(flash.notice).to eq("Le transfert de 3 dossiers à #{next_owner} est en cours") end + + context "next owner has an empty email" do + let(:next_owner) { '' } + + it "should not transfer to an empty email" do + expect { subject }.not_to change { DossierTransfer.count } + expect(flash.alert).to eq(["L'adresse email est invalide"]) + end + end end context 'POST #accept_merge' do From d53aba4d248e73b3f293fbb1ad33cddcfc5c7b1c Mon Sep 17 00:00:00 2001 From: Colin Darie Date: Tue, 2 Aug 2022 15:20:33 +0200 Subject: [PATCH 3/5] fix(dossier_transfer): require a valid email Cf #7621 --- app/controllers/users/transfers_controller.rb | 22 ++++++++++---- app/views/users/dossiers/transferer.html.haml | 2 +- .../users/transfers_controller_spec.rb | 29 +++++++++++++++++++ 3 files changed, 46 insertions(+), 7 deletions(-) diff --git a/app/controllers/users/transfers_controller.rb b/app/controllers/users/transfers_controller.rb index 285e0f7a3..8a82a90b8 100644 --- a/app/controllers/users/transfers_controller.rb +++ b/app/controllers/users/transfers_controller.rb @@ -2,8 +2,14 @@ module Users class TransfersController < UserController def create transfer = DossierTransfer.new(transfer_params) - transfer.save! - redirect_to dossiers_path + + if transfer.valid? + transfer.save! + redirect_to dossiers_path + else + flash.alert = transfer.errors.full_messages + redirect_to transferer_dossier_path(transfer_params[:dossiers].first) + end end def update @@ -23,12 +29,16 @@ module Users private def transfer_params - transfer_params = params.require(:dossier_transfer).permit(:email, :dossiers) - if transfer_params[:dossiers].present? - transfer_params.merge(dossiers: [current_user.dossiers.find(transfer_params[:dossiers])]) + transfer_params = params.require(:dossier_transfer).permit(:email, :dossier) + + dossier_id = transfer_params.delete(:dossier) + dossiers = if dossier_id.present? + [current_user.dossiers.find(dossier_id)] else - transfer_params.merge(dossiers: current_user.dossiers) + current_user.dossiers end + + transfer_params.merge(dossiers: dossiers) end end end diff --git a/app/views/users/dossiers/transferer.html.haml b/app/views/users/dossiers/transferer.html.haml index 6966f85c2..8b2e955cf 100644 --- a/app/views/users/dossiers/transferer.html.haml +++ b/app/views/users/dossiers/transferer.html.haml @@ -5,5 +5,5 @@ = form_for @transfer, url: transfers_path, html: { class: 'form mt-2' } do |f| = f.label :email, 'Email du compte destinataire' = f.email_field :email - = f.hidden_field :dossiers, value: dossier.id + = f.hidden_field :dossier, value: dossier.id = f.submit "Envoyer la demande de transfert", class: 'button primary' diff --git a/spec/controllers/users/transfers_controller_spec.rb b/spec/controllers/users/transfers_controller_spec.rb index dd13c32b6..9c8809063 100644 --- a/spec/controllers/users/transfers_controller_spec.rb +++ b/spec/controllers/users/transfers_controller_spec.rb @@ -13,4 +13,33 @@ describe Users::TransfersController, type: :controller do it { expect { dossier_transfert.reload }.to raise_error(ActiveRecord::RecordNotFound) } end + + describe "POST create" do + subject { post :create, params: { dossier_transfer: { email: email, dossier: dossier.id } } } + + before { subject } + + context "with valid email" do + let(:email) { "test@rspec.net" } + + it { expect(DossierTransfer.last.email).to eq(email) } + it { expect(DossierTransfer.last.dossiers).to eq([dossier]) } + end + + shared_examples 'email error' do + it { expect { subject }.not_to change { DossierTransfer.count } } + it { expect(flash.alert).to match([/invalide/]) } + it { is_expected.to redirect_to transferer_dossier_path(dossier.id) } + end + + context "when email is empty" do + let(:email) { "" } + it_behaves_like 'email error' + end + + context "when email is invalid" do + let(:email) { "not-an-email" } + it_behaves_like 'email error' + end + end end From 11a9a6f53f2972d8ebc61bbaf33c97cd5562b802 Mon Sep 17 00:00:00 2001 From: Colin Darie Date: Tue, 2 Aug 2022 15:27:47 +0200 Subject: [PATCH 4/5] feat(dossier_transfer/email): require email in browser side --- app/views/users/dossiers/transferer.html.haml | 2 +- app/views/users/dossiers/transferer_all.html.haml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/users/dossiers/transferer.html.haml b/app/views/users/dossiers/transferer.html.haml index 8b2e955cf..29d1c5280 100644 --- a/app/views/users/dossiers/transferer.html.haml +++ b/app/views/users/dossiers/transferer.html.haml @@ -4,6 +4,6 @@ = form_for @transfer, url: transfers_path, html: { class: 'form mt-2' } do |f| = f.label :email, 'Email du compte destinataire' - = f.email_field :email + = f.email_field :email, required: true = f.hidden_field :dossier, value: dossier.id = f.submit "Envoyer la demande de transfert", class: 'button primary' diff --git a/app/views/users/dossiers/transferer_all.html.haml b/app/views/users/dossiers/transferer_all.html.haml index 229a6255e..71598242f 100644 --- a/app/views/users/dossiers/transferer_all.html.haml +++ b/app/views/users/dossiers/transferer_all.html.haml @@ -3,5 +3,5 @@ = form_for @transfer, url: transfers_path, html: { class: 'form mt-2' } do |f| = f.label :email, 'Email du compte destinataire' - = f.email_field :email + = f.email_field :email, required: true = f.submit "Envoyer la demande de transfert", class: 'button primary' From db968a1ffceae6e1664428176f0a025262cb15e7 Mon Sep 17 00:00:00 2001 From: Colin Darie Date: Tue, 2 Aug 2022 15:45:00 +0200 Subject: [PATCH 5/5] fix(db): remove DossierTransfer without email --- ...estroy_dossier_transfer_without_email.rake | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 lib/tasks/deployment/20220802133502_destroy_dossier_transfer_without_email.rake diff --git a/lib/tasks/deployment/20220802133502_destroy_dossier_transfer_without_email.rake b/lib/tasks/deployment/20220802133502_destroy_dossier_transfer_without_email.rake new file mode 100644 index 000000000..68284f1a4 --- /dev/null +++ b/lib/tasks/deployment/20220802133502_destroy_dossier_transfer_without_email.rake @@ -0,0 +1,25 @@ +namespace :after_party do + desc 'Deployment task: destroy_dossier_transfer_without_email' + task destroy_dossier_transfer_without_email: :environment do + puts "Running deploy task 'destroy_dossier_transfer_without_email'" + + invalid_dossiers = DossierTransfer.where(email: "") + + progress = ProgressReport.new(invalid_dossiers.count) + + invalid_dossiers.find_each do |dossier_transfer| + puts "Destroy dossier transfer #{dossier_transfer.id}" + dossier_transfer.destroy_and_nullify + + job = Delayed::Job.where("handler LIKE ALL(ARRAY[?, ?])", "%ActionMailer::MailDeliveryJob%", "%aj_globalid: gid://tps/DossierTransfer/#{dossier_transfer.id}\n%").first + job.destroy if job + + progress.inc + end + + # Update task as completed. If you remove the line below, the task will + # run with every deploy (or every time you call after_party:run). + AfterParty::TaskRecord + .create version: AfterParty::TaskRecorder.new(__FILE__).timestamp + end +end