From 022f1de8098fabe27bd3c3ca3d71f84ead3ffafc Mon Sep 17 00:00:00 2001 From: Oluwatoyosi Oyegoke <34948675+Tooyosi@users.noreply.github.com> Date: Wed, 4 Sep 2024 18:01:28 +0100 Subject: [PATCH] [issue-4351] add logic to notify old user email of email change (#4374) * [issue-4351] add logic to notify old user email of email change * [issue-4351] hound cleanups * [issue-4351] move UserInfoChangedMailer call outside super to prevent race condition * [issue-4351] edit test case to reflect old mail sent * [issue-4351] add test cases to both UserInfoChangedMailerWorker and UserInfoChangedMailer * [issue-4351] hound cleanups * [issue-4351] hound cleanups * [issue-4351] renaming test variables --- app/controllers/api/v1/users_controller.rb | 19 ++++++++----------- app/mailers/user_info_changed_mailer.rb | 7 ++++--- .../user_info_changed_mailer_worker.rb | 4 ++-- .../api/v1/users_controller_spec.rb | 5 +++-- spec/mailers/user_info_changed_mailer_spec.rb | 8 +++++++- .../user_info_changed_mailer_worker_spec.rb | 13 +++++++++++-- 6 files changed, 35 insertions(+), 21 deletions(-) diff --git a/app/controllers/api/v1/users_controller.rb b/app/controllers/api/v1/users_controller.rb index 5ea2507f1..78ff809d1 100644 --- a/app/controllers/api/v1/users_controller.rb +++ b/app/controllers/api/v1/users_controller.rb @@ -47,18 +47,15 @@ def me end def update - [].tap do |update_email_user_ids| - - super do |user| - if user.email_changed? - update_email_user_ids << user.id - user.update_attribute(:valid_email, true) - end - end + prev_email = user.email + email_changed = false + super do |user| + email_changed = user.email_changed? + end - update_email_user_ids.each do |user_id| - UserInfoChangedMailerWorker.perform_async(user_id, "email") - end + if email_changed + user.update(valid_email: true) + UserInfoChangedMailerWorker.perform_async(user.id, 'email', prev_email) end end diff --git a/app/mailers/user_info_changed_mailer.rb b/app/mailers/user_info_changed_mailer.rb index 38fcfa1ab..da644cfb7 100644 --- a/app/mailers/user_info_changed_mailer.rb +++ b/app/mailers/user_info_changed_mailer.rb @@ -1,19 +1,20 @@ class UserInfoChangedMailer < ApplicationMailer layout false - def user_info_changed(user, info) + def user_info_changed(user, info, previous_email=nil) @user = user - @email_to = user.email + @recipient_emails = [user.email] case info when "email" subject = "Your Zooniverse email address has been changed" template = "email_changed" + @recipient_emails << previous_email if previous_email when "password" subject = "Your Zooniverse password has been changed" template = "password_changed" end - mail(to: @email_to, subject: subject, :template_name => template ) + mail(to: @recipient_emails, subject: subject, template_name: template) end end diff --git a/app/workers/user_info_changed_mailer_worker.rb b/app/workers/user_info_changed_mailer_worker.rb index f6409896f..cf1b66582 100644 --- a/app/workers/user_info_changed_mailer_worker.rb +++ b/app/workers/user_info_changed_mailer_worker.rb @@ -5,10 +5,10 @@ class UserInfoChangedMailerWorker attr_reader :user, :project - def perform(user_id, info) + def perform(user_id, info, previous_email=nil) @user = User.find(user_id) if @user && @user.receives_email? - UserInfoChangedMailer.user_info_changed(user, info).deliver + UserInfoChangedMailer.user_info_changed(user, info, previous_email).deliver end rescue ActiveRecord::RecordNotFound nil diff --git a/spec/controllers/api/v1/users_controller_spec.rb b/spec/controllers/api/v1/users_controller_spec.rb index d28cb8585..a8ffb0a1e 100644 --- a/spec/controllers/api/v1/users_controller_spec.rb +++ b/spec/controllers/api/v1/users_controller_spec.rb @@ -473,13 +473,14 @@ def update_request context "when changing email" do let(:put_operations) { {users: {email: "test@example.com"}} } + let!(:user_email) { user.email } after(:each) do update_request end - it "sends an email to the new address if user is valid" do - expect(UserInfoChangedMailerWorker).to receive(:perform_async).with(user.id, "email") + it "sends an email to the new and old address if user is valid" do + expect(UserInfoChangedMailerWorker).to receive(:perform_async).with(user.id, "email", user_email) end it "sets valid_email parameter to true" do diff --git a/spec/mailers/user_info_changed_mailer_spec.rb b/spec/mailers/user_info_changed_mailer_spec.rb index 614a28ccd..79ffb2cb9 100644 --- a/spec/mailers/user_info_changed_mailer_spec.rb +++ b/spec/mailers/user_info_changed_mailer_spec.rb @@ -21,10 +21,16 @@ end context "email address was changed" do - let(:mail) { UserInfoChangedMailer.user_info_changed(user, "email")} + prev_email = 'prev@example.com' + let(:mail) { UserInfoChangedMailer.user_info_changed(user, 'email', prev_email)} + it 'should have the correct subject' do expect(mail.subject).to eq("Your Zooniverse email address has been changed") end + + it 'notifies the previous mail' do + expect(mail.to).to include(prev_email) + end end end end diff --git a/spec/workers/user_info_changed_mailer_worker_spec.rb b/spec/workers/user_info_changed_mailer_worker_spec.rb index 7643ad8e9..0cc7f72c8 100644 --- a/spec/workers/user_info_changed_mailer_worker_spec.rb +++ b/spec/workers/user_info_changed_mailer_worker_spec.rb @@ -3,9 +3,18 @@ RSpec.describe UserInfoChangedMailerWorker do let(:project) { create(:project) } let(:user) { create(:user) } + let(:prev_email) { 'oldemaild@example.com' } - it 'should deliver the mail' do - expect{ subject.perform(user.id, "password") }.to change{ ActionMailer::Base.deliveries.count }.by(1) + context 'when delivering an email' do + it 'delivers the mail' do + expect { subject.perform(user.id, 'password') }.to change { ActionMailer::Base.deliveries.count }.by(1) + end + + it 'delivers to the right recipients' do + subject.perform(user.id, 'email', prev_email) + mail = ActionMailer::Base.deliveries.last + expect(mail.to).to eq([user.email, prev_email]) + end end context "without a user" do