From 894e8fdd47f6e646ca67f4d4cf10d70da4b8993c Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Mon, 25 Oct 2021 16:57:21 +0200 Subject: [PATCH 1/8] move update_email check to before_action --- app/controllers/users/profil_controller.rb | 13 ++++++++++--- config/locales/views/users/profil/fr.yml | 2 +- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/app/controllers/users/profil_controller.rb b/app/controllers/users/profil_controller.rb index f55752c11..47558351f 100644 --- a/app/controllers/users/profil_controller.rb +++ b/app/controllers/users/profil_controller.rb @@ -1,5 +1,7 @@ module Users class ProfilController < UserController + before_action :ensure_update_email_is_authorized, only: :update_email + def show @waiting_transfers = current_user.dossiers.joins(:transfer).group('dossier_transfers.email').count.to_a end @@ -11,9 +13,7 @@ module Users end def update_email - if current_user.instructeur? && !target_email_allowed? - flash.alert = t('.email_not_allowed', contact_email: CONTACT_EMAIL, requested_email: requested_email) - elsif current_user.update(update_email_params) + if current_user.update(update_email_params) flash.notice = t('devise.registrations.update_needs_confirmation') elsif current_user.errors&.details&.dig(:email)&.any? { |e| e[:error] == :taken } UserMailer.account_already_taken(current_user, requested_email).deliver_later @@ -34,6 +34,13 @@ module Users private + def ensure_update_email_is_authorized + if current_user.instructeur? && !target_email_allowed? + flash.alert = t('users.profil.ensure_update_email_is_authorized.email_not_allowed', contact_email: CONTACT_EMAIL, requested_email: requested_email) + redirect_to profil_path + end + end + def update_email_params params.require(:user).permit(:email) end diff --git a/config/locales/views/users/profil/fr.yml b/config/locales/views/users/profil/fr.yml index 8062880ff..4c18b6d04 100644 --- a/config/locales/views/users/profil/fr.yml +++ b/config/locales/views/users/profil/fr.yml @@ -21,7 +21,7 @@ fr:
Si ce n'est pas votre cas, contactez le support : %{contact_email} - update_email: + ensure_update_email_is_authorized: email_not_allowed: "L’email %{requested_email} ne peut être utilisé, contactez le support : %{contact_email}" transfer_all_dossiers: new_transfer: From 6625c6bac3a2b10dfb82cd99cf6a88eeb9b2b427 Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Tue, 26 Oct 2021 12:54:50 +0200 Subject: [PATCH 2/8] add requested_merge_into column in user table --- app/models/user.rb | 2 ++ ...0211026082232_add_requested_merge_into_column_to_users.rb | 5 +++++ db/schema.rb | 3 +++ 3 files changed, 10 insertions(+) create mode 100644 db/migrate/20211026082232_add_requested_merge_into_column_to_users.rb diff --git a/app/models/user.rb b/app/models/user.rb index 24656f846..f3d990a26 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -28,6 +28,7 @@ # administrateur_id :bigint # expert_id :bigint # instructeur_id :bigint +# requested_merge_into_id :bigint # class User < ApplicationRecord include EmailSanitizableConcern @@ -52,6 +53,7 @@ class User < ApplicationRecord belongs_to :instructeur, optional: true, dependent: :destroy belongs_to :administrateur, optional: true, dependent: :destroy belongs_to :expert, optional: true, dependent: :destroy + belongs_to :requested_merge_into, class_name: 'User', optional: true accepts_nested_attributes_for :france_connect_information diff --git a/db/migrate/20211026082232_add_requested_merge_into_column_to_users.rb b/db/migrate/20211026082232_add_requested_merge_into_column_to_users.rb new file mode 100644 index 000000000..aec5a9326 --- /dev/null +++ b/db/migrate/20211026082232_add_requested_merge_into_column_to_users.rb @@ -0,0 +1,5 @@ +class AddRequestedMergeIntoColumnToUsers < ActiveRecord::Migration[6.1] + def change + add_reference :users, :requested_merge_into, foreign_key: { to_table: :users }, null: true, index: true + end +end diff --git a/db/schema.rb b/db/schema.rb index ede968b34..c992f0ae8 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -794,11 +794,13 @@ ActiveRecord::Schema.define(version: 2021_10_26_131800) do t.bigint "administrateur_id" t.bigint "expert_id" t.string "locale" + t.bigint "requested_merge_into_id" t.index ["administrateur_id"], name: "index_users_on_administrateur_id" t.index ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true t.index ["email"], name: "index_users_on_email", unique: true t.index ["expert_id"], name: "index_users_on_expert_id" t.index ["instructeur_id"], name: "index_users_on_instructeur_id" + t.index ["requested_merge_into_id"], name: "index_users_on_requested_merge_into_id" t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true t.index ["unlock_token"], name: "index_users_on_unlock_token", unique: true end @@ -868,5 +870,6 @@ ActiveRecord::Schema.define(version: 2021_10_26_131800) do add_foreign_key "users", "administrateurs" add_foreign_key "users", "experts" add_foreign_key "users", "instructeurs" + add_foreign_key "users", "users", column: "requested_merge_into_id" add_foreign_key "without_continuation_mails", "procedures" end From 652b8367be9ce61b276f608938ab4b2090c196a6 Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Tue, 26 Oct 2021 13:22:51 +0200 Subject: [PATCH 3/8] accept or refuse merge --- app/controllers/users/profil_controller.rb | 25 ++++++++++++++ app/views/users/profil/show.html.haml | 10 ++++++ config/routes.rb | 2 ++ .../users/profil_controller_spec.rb | 34 +++++++++++++++++++ 4 files changed, 71 insertions(+) diff --git a/app/controllers/users/profil_controller.rb b/app/controllers/users/profil_controller.rb index 47558351f..27db94721 100644 --- a/app/controllers/users/profil_controller.rb +++ b/app/controllers/users/profil_controller.rb @@ -3,6 +3,7 @@ module Users before_action :ensure_update_email_is_authorized, only: :update_email def show + @waiting_merge_emails = waiting_merge_emails @waiting_transfers = current_user.dossiers.joins(:transfer).group('dossier_transfers.email').count.to_a end @@ -32,8 +33,32 @@ module Users redirect_to profil_path end + def accept_merge + users_requesting_merge.each { |user| current_user.merge(user) } + users_requesting_merge.update_all(requested_merge_into_id: nil) + + flash.notice = "Vous avez absorbé le compte #{waiting_merge_emails.join(', ')}" + redirect_to profil_path + end + + def refuse_merge + users = users_requesting_merge + users.update_all(requested_merge_into_id: nil) + + flash.notice = 'La fusion a été refusé' + redirect_to profil_path + end + private + def waiting_merge_emails + users_requesting_merge.pluck(:email) + end + + def users_requesting_merge + @requesting_merge ||= User.where(requested_merge_into: current_user) + end + def ensure_update_email_is_authorized if current_user.instructeur? && !target_email_allowed? flash.alert = t('users.profil.ensure_update_email_is_authorized.email_not_allowed', contact_email: CONTACT_EMAIL, requested_email: requested_email) diff --git a/app/views/users/profil/show.html.haml b/app/views/users/profil/show.html.haml index 46f60a875..d0021a281 100644 --- a/app/views/users/profil/show.html.haml +++ b/app/views/users/profil/show.html.haml @@ -5,6 +5,16 @@ #profil-page.container %h1 Profil + - if @waiting_merge_emails.any? + .card + .card-title Demande de fusion de comptes + %p + Acceptez-vous d’absorber le compte de + %span.email-address= @waiting_merge_emails.join(', ') + + = link_to 'Refuser la fusion', refuse_merge_path, method: :post, class: 'button', data: { confirm: "Confirmez-vous le refus ?" } + = link_to 'Accepter la fusion', accept_merge_path, method: :post, class: 'button', data: { confirm: "Confirmez-vous la fusion des comptes ?" } + .card .card-title Coordonnées %p diff --git a/config/routes.rb b/config/routes.rb index 249faa38c..45cd42836 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -280,6 +280,8 @@ Rails.application.routes.draw do get 'renew-api-token' => redirect('/profil') patch 'update_email' => 'profil#update_email' post 'transfer_all_dossiers' => 'profil#transfer_all_dossiers' + post 'accept_merge' => 'profil#accept_merge' + post 'refuse_merge' => 'profil#refuse_merge' end # diff --git a/spec/controllers/users/profil_controller_spec.rb b/spec/controllers/users/profil_controller_spec.rb index bb8cf8fcc..19e8f96bc 100644 --- a/spec/controllers/users/profil_controller_spec.rb +++ b/spec/controllers/users/profil_controller_spec.rb @@ -126,4 +126,38 @@ describe Users::ProfilController, type: :controller do expect(flash.notice).to eq("Le transfert de 3 dossiers à #{next_owner} est en cours") end end + + context 'POST #accept_merge' do + let!(:requesting_user) { create(:user, requested_merge_into: user) } + + subject { post :accept_merge } + + it 'merges the account' do + expect_any_instance_of(User).to receive(:merge) + + subject + requesting_user.reload + + expect(requesting_user.requested_merge_into).to be_nil + expect(flash.notice).to include('Vous avez absorbé') + expect(response).to redirect_to(profil_path) + end + end + + context 'POST #refuse_merge' do + let!(:requesting_user) { create(:user, requested_merge_into: user) } + + subject { post :refuse_merge } + + it 'merges the account' do + expect_any_instance_of(User).not_to receive(:merge) + + subject + requesting_user.reload + + expect(requesting_user.requested_merge_into).to be_nil + expect(flash.notice).to include('La fusion a été refusé') + expect(response).to redirect_to(profil_path) + end + end end From b160086cc579fcfb4041b94a3e4d754568ae15de Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Tue, 26 Oct 2021 13:36:14 +0200 Subject: [PATCH 4/8] update update_email to allow merge --- app/controllers/users/profil_controller.rb | 9 ++++---- app/mailers/user_mailer.rb | 4 ++-- app/models/user.rb | 5 +++++ .../user_mailer/account_already_taken.haml | 20 ----------------- app/views/user_mailer/ask_for_merge.haml | 22 +++++++++++++++++++ .../users/profil_controller_spec.rb | 11 ++++++---- spec/mailers/previews/user_mailer_preview.rb | 4 ++-- spec/mailers/user_mailer_spec.rb | 4 ++-- 8 files changed, 45 insertions(+), 34 deletions(-) delete mode 100644 app/views/user_mailer/account_already_taken.haml create mode 100644 app/views/user_mailer/ask_for_merge.haml diff --git a/app/controllers/users/profil_controller.rb b/app/controllers/users/profil_controller.rb index 27db94721..3d68ccfe9 100644 --- a/app/controllers/users/profil_controller.rb +++ b/app/controllers/users/profil_controller.rb @@ -14,11 +14,12 @@ module Users end def update_email - if current_user.update(update_email_params) + requested_user = User.find_by(email: requested_email) + + if requested_user.present? + current_user.ask_for_merge(requested_user) flash.notice = t('devise.registrations.update_needs_confirmation') - elsif current_user.errors&.details&.dig(:email)&.any? { |e| e[:error] == :taken } - UserMailer.account_already_taken(current_user, requested_email).deliver_later - # avoid leaking information about whether an account with this email exists or not + elsif current_user.update(update_email_params) flash.notice = t('devise.registrations.update_needs_confirmation') else flash.alert = current_user.errors.full_messages diff --git a/app/mailers/user_mailer.rb b/app/mailers/user_mailer.rb index 6fe55a4a0..e5b143566 100644 --- a/app/mailers/user_mailer.rb +++ b/app/mailers/user_mailer.rb @@ -12,10 +12,10 @@ class UserMailer < ApplicationMailer mail(to: user.email, subject: @subject, procedure: @procedure) end - def account_already_taken(user, requested_email) + def ask_for_merge(user, requested_email) @user = user @requested_email = requested_email - @subject = "Changement d’adresse email" + @subject = "Fusion de compte" mail(to: requested_email, subject: @subject) end diff --git a/app/models/user.rb b/app/models/user.rb index f3d990a26..7636d5cfd 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -220,6 +220,11 @@ class User < ApplicationRecord end end + def ask_for_merge(requested_user) + update(requested_merge_into: requested_user) + UserMailer.ask_for_merge(self, requested_user.email).deliver_later + end + private def link_invites! diff --git a/app/views/user_mailer/account_already_taken.haml b/app/views/user_mailer/account_already_taken.haml deleted file mode 100644 index 826cdbb37..000000000 --- a/app/views/user_mailer/account_already_taken.haml +++ /dev/null @@ -1,20 +0,0 @@ -- content_for(:title, @subject) - -%p - Bonjour, - -%p - L’utilisateur « #{@user.email} » a demandé le changement de son adresse vers « #{@requested_email} ». - -%p - Malheureusement, votre compte « #{@requested_email} » existe déjà. Nous ne pouvons pas fusionner automatiquement vos comptes. - -%p - %strong Nous ne pouvons donc pas effectuer le changement d’adresse email. - -%p - Si vous n’êtes pas à l’origine de cette demande, vous pouvez ignorer ce message. Et si vous avez besoin d’assistance, n’hésitez pas à nous contacter à - = succeed '.' do - = mail_to CONTACT_EMAIL - -= render partial: "layouts/mailers/signature" diff --git a/app/views/user_mailer/ask_for_merge.haml b/app/views/user_mailer/ask_for_merge.haml new file mode 100644 index 000000000..d61072a89 --- /dev/null +++ b/app/views/user_mailer/ask_for_merge.haml @@ -0,0 +1,22 @@ +- content_for(:title, @subject) + +%p + Bonjour, + +%p + L’utilisateur « #{@user.email} » a demandé la fusion de son compte avec le votre « #{@requested_email} ». + +%p + Si vous désirez confirmer la fusion de ces comptes : + +%ol + %li connectez-vous avec le compte #{@requested_email} + %li allez sur votre page profil + %li acceptez la fusion + +%p + Si vous n’êtes pas à l’origine de cette demande, vous pouvez ignorer ce message. Et si vous avez besoin d’assistance, n’hésitez pas à nous contacter à + = succeed '.' do + = mail_to CONTACT_EMAIL + += render partial: "layouts/mailers/signature" diff --git a/spec/controllers/users/profil_controller_spec.rb b/spec/controllers/users/profil_controller_spec.rb index 19e8f96bc..89baac76e 100644 --- a/spec/controllers/users/profil_controller_spec.rb +++ b/spec/controllers/users/profil_controller_spec.rb @@ -63,16 +63,19 @@ describe Users::ProfilController, type: :controller do let(:existing_user) { create(:user) } before do + expect_any_instance_of(User).to receive(:ask_for_merge).with(existing_user) + perform_enqueued_jobs do patch :update_email, params: { user: { email: existing_user.email } } end user.reload end - it { expect(user.unconfirmed_email).to be_nil } - it { expect(ActionMailer::Base.deliveries.last.to).to eq([existing_user.email]) } - it { expect(response).to redirect_to(profil_path) } - it { expect(flash.notice).to eq(I18n.t('devise.registrations.update_needs_confirmation')) } + it 'launches the merge process' do + expect(user.unconfirmed_email).to be_nil + expect(response).to redirect_to(profil_path) + expect(flash.notice).to eq(I18n.t('devise.registrations.update_needs_confirmation')) + end end context 'when the mail is incorrect' do diff --git a/spec/mailers/previews/user_mailer_preview.rb b/spec/mailers/previews/user_mailer_preview.rb index bf20eec17..436ac435f 100644 --- a/spec/mailers/previews/user_mailer_preview.rb +++ b/spec/mailers/previews/user_mailer_preview.rb @@ -8,8 +8,8 @@ class UserMailerPreview < ActionMailer::Preview UserMailer.new_account_warning(user, procedure) end - def account_already_taken - UserMailer.account_already_taken(user, 'dircab@territoires.gouv.fr') + def ask_for_merge + UserMailer.ask_for_merge(user, 'dircab@territoires.gouv.fr') end private diff --git a/spec/mailers/user_mailer_spec.rb b/spec/mailers/user_mailer_spec.rb index 651dcc4e8..88f4fa2dc 100644 --- a/spec/mailers/user_mailer_spec.rb +++ b/spec/mailers/user_mailer_spec.rb @@ -17,10 +17,10 @@ RSpec.describe UserMailer, type: :mailer do end end - describe '.account_already_taken' do + describe '.ask_for_merge' do let(:requested_email) { 'new@exemple.fr' } - subject { described_class.account_already_taken(user, requested_email) } + subject { described_class.ask_for_merge(user, requested_email) } it { expect(subject.to).to eq([requested_email]) } it { expect(subject.body).to include(requested_email) } From adfac5fb7bff67938d99dfd8087c2b8e4dfe896d Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Tue, 26 Oct 2021 10:12:15 +0200 Subject: [PATCH 5/8] improve ui --- app/views/users/profil/show.html.haml | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/app/views/users/profil/show.html.haml b/app/views/users/profil/show.html.haml index d0021a281..4bb8e0932 100644 --- a/app/views/users/profil/show.html.haml +++ b/app/views/users/profil/show.html.haml @@ -20,13 +20,14 @@ %p Votre email est actuellement %span.email-address= current_user.email + - if current_user.unconfirmed_email.present? - .card.warning - .card-title - Changement en attente : - %span.email-address= current_user.unconfirmed_email - %p - Pour finaliser votre changement d’adresse, vérifiez vos emails et cliquez sur le lien de confirmation. + + %p.mb-4 + Changement en attente : + %span.email-address= current_user.unconfirmed_email + %br + Pour finaliser votre changement d’adresse, vérifiez vos emails et cliquez sur le lien de confirmation. - if current_user.instructeur? %p.mb-4 From 9041e201e89fad9db552881d70c7b0b52a23296f Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Tue, 26 Oct 2021 16:18:12 +0200 Subject: [PATCH 6/8] one merge at a time --- app/controllers/users/profil_controller.rb | 4 ++++ app/views/users/profil/show.html.haml | 5 +++-- spec/controllers/users/profil_controller_spec.rb | 6 ++++++ 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/app/controllers/users/profil_controller.rb b/app/controllers/users/profil_controller.rb index 3d68ccfe9..8af489871 100644 --- a/app/controllers/users/profil_controller.rb +++ b/app/controllers/users/profil_controller.rb @@ -18,8 +18,12 @@ module Users if requested_user.present? current_user.ask_for_merge(requested_user) + current_user.update(unconfirmed_email: nil) + flash.notice = t('devise.registrations.update_needs_confirmation') elsif current_user.update(update_email_params) + current_user.update(requested_merge_into: nil) + flash.notice = t('devise.registrations.update_needs_confirmation') else flash.alert = current_user.errors.full_messages diff --git a/app/views/users/profil/show.html.haml b/app/views/users/profil/show.html.haml index 4bb8e0932..d7946c721 100644 --- a/app/views/users/profil/show.html.haml +++ b/app/views/users/profil/show.html.haml @@ -21,11 +21,12 @@ Votre email est actuellement %span.email-address= current_user.email - - if current_user.unconfirmed_email.present? + - waiting_email = current_user.unconfirmed_email || current_user.requested_merge_into&.email + - if waiting_email.present? %p.mb-4 Changement en attente : - %span.email-address= current_user.unconfirmed_email + %span.email-address= waiting_email %br Pour finaliser votre changement d’adresse, vérifiez vos emails et cliquez sur le lien de confirmation. diff --git a/spec/controllers/users/profil_controller_spec.rb b/spec/controllers/users/profil_controller_spec.rb index 89baac76e..6e3bd3779 100644 --- a/spec/controllers/users/profil_controller_spec.rb +++ b/spec/controllers/users/profil_controller_spec.rb @@ -49,12 +49,16 @@ describe Users::ProfilController, type: :controller do describe 'PATCH #update_email' do context 'when everything is fine' do + let(:previous_request) { create(:user) } + before do + user.update(requested_merge_into: previous_request) patch :update_email, params: { user: { email: 'loulou@lou.com' } } user.reload end it { expect(user.unconfirmed_email).to eq('loulou@lou.com') } + it { expect(user.requested_merge_into).to be_nil } it { expect(response).to redirect_to(profil_path) } it { expect(flash.notice).to eq(I18n.t('devise.registrations.update_needs_confirmation')) } end @@ -63,6 +67,8 @@ describe Users::ProfilController, type: :controller do let(:existing_user) { create(:user) } before do + user.update(unconfirmed_email: 'unconfirmed@mail.com') + expect_any_instance_of(User).to receive(:ask_for_merge).with(existing_user) perform_enqueued_jobs do From d1162d74932740a53c756225cfe5e2d17761e79a Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Fri, 29 Oct 2021 10:10:30 +0200 Subject: [PATCH 7/8] add e2e spec --- spec/system/users/change_email_spec.rb | 33 ++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/spec/system/users/change_email_spec.rb b/spec/system/users/change_email_spec.rb index c2a8bfcf0..76ad23469 100644 --- a/spec/system/users/change_email_spec.rb +++ b/spec/system/users/change_email_spec.rb @@ -29,3 +29,36 @@ describe 'Changing an email' do expect(user.reload.email).to eq(new_email) end end + +describe 'Merging account' do + let(:old_user) { create(:user) } + let(:new_user) { create(:user) } + + before do + login_as old_user, scope: :user + end + + scenario 'is easy' do + visit '/profil' + + fill_in :user_email, with: new_user.email + + perform_enqueued_jobs do + click_button 'Changer mon adresse' + end + + expect(page).to have_content(I18n.t('devise.registrations.update_needs_confirmation')) + expect(page).to have_content(old_user.email) + expect(page).to have_content(new_user.email) + + login_as new_user, scope: :user + visit '/profil' + + expect(page).to have_content("Acceptez-vous d’absorber le compte de #{old_user.email}") + click_on 'Accepter la fusion' + + expect(page).not_to have_content(old_user.email) + expect(page).to have_content(new_user.email) + expect { old_user.reload }.to raise_error(ActiveRecord::RecordNotFound) + end +end From 17d131b3cc3f97db8391e512f0f630c61fb3b806 Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Thu, 4 Nov 2021 15:51:54 +0100 Subject: [PATCH 8/8] add has_may requested_merge_from --- app/controllers/users/profil_controller.rb | 2 +- app/models/user.rb | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/app/controllers/users/profil_controller.rb b/app/controllers/users/profil_controller.rb index 8af489871..2c9dd47cc 100644 --- a/app/controllers/users/profil_controller.rb +++ b/app/controllers/users/profil_controller.rb @@ -61,7 +61,7 @@ module Users end def users_requesting_merge - @requesting_merge ||= User.where(requested_merge_into: current_user) + @requesting_merge ||= current_user.requested_merge_from end def ensure_update_email_is_authorized diff --git a/app/models/user.rb b/app/models/user.rb index 7636d5cfd..fba5d1190 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -49,6 +49,8 @@ class User < ApplicationRecord has_many :dossiers_invites, through: :invites, source: :dossier has_many :deleted_dossiers has_many :merge_logs, dependent: :destroy + has_many :requested_merge_from, class_name: 'User', dependent: :nullify, inverse_of: :requested_merge_into, foreign_key: :requested_merge_into_id + has_one :france_connect_information, dependent: :destroy belongs_to :instructeur, optional: true, dependent: :destroy belongs_to :administrateur, optional: true, dependent: :destroy