Merge pull request #7647 from betagouv/fix-dossier-transfer-without-email

fix: dossier_transfer requires a valid email

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
 

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

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) }

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.hidden_field :dossiers, value: dossier.id
+    = f.email_field :email, required: true
+    = f.hidden_field :dossier, value: dossier.id
     = f.submit "Envoyer la demande de transfert", class: 'button primary'

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'

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"

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"

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

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

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

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