From 8a0e24fe92a843eef03fe8f21372198db9ae39df Mon Sep 17 00:00:00 2001 From: tooyosi Date: Wed, 7 Aug 2024 17:59:41 +0100 Subject: [PATCH 1/8] [issue-4351] add logic to notify old user email of email change --- app/controllers/api/v1/users_controller.rb | 16 +++++----------- app/mailers/user_info_changed_mailer.rb | 7 ++++--- app/workers/user_info_changed_mailer_worker.rb | 4 ++-- spec/controllers/api/v1/users_controller_spec.rb | 5 +++-- 4 files changed, 14 insertions(+), 18 deletions(-) diff --git a/app/controllers/api/v1/users_controller.rb b/app/controllers/api/v1/users_controller.rb index 5ea2507f1..72ad51e38 100644 --- a/app/controllers/api/v1/users_controller.rb +++ b/app/controllers/api/v1/users_controller.rb @@ -47,17 +47,11 @@ 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 - - update_email_user_ids.each do |user_id| - UserInfoChangedMailerWorker.perform_async(user_id, "email") + prev_email = user.email + super do |user| + if user.email_changed? + user.update_attribute(:valid_email, true) + UserInfoChangedMailerWorker.perform_async(user.id, "email", prev_email) end end end diff --git a/app/mailers/user_info_changed_mailer.rb b/app/mailers/user_info_changed_mailer.rb index 38fcfa1ab..e57cae587 100644 --- a/app/mailers/user_info_changed_mailer.rb +++ b/app/mailers/user_info_changed_mailer.rb @@ -1,9 +1,10 @@ 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] + @recipient_emails << previous_email if previous_email.present? && info == "email" case info when "email" @@ -14,6 +15,6 @@ def user_info_changed(user, info) template = "password_changed" end - mail(to: @email_to, subject: subject, :template_name => template ) + mail(to: @recipient_emails, subject: 'Limited 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..110c26cdd 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 address if user is valid", focus: true do + expect(UserInfoChangedMailerWorker).to receive(:perform_async).with(user.id, "email", user_email) end it "sets valid_email parameter to true" do From 883d2e53cf02570268e3cc2ef2e833209426270e Mon Sep 17 00:00:00 2001 From: tooyosi Date: Wed, 7 Aug 2024 18:21:59 +0100 Subject: [PATCH 2/8] [issue-4351] hound cleanups --- app/controllers/api/v1/users_controller.rb | 4 ++-- app/mailers/user_info_changed_mailer.rb | 4 ++-- spec/controllers/api/v1/users_controller_spec.rb | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/controllers/api/v1/users_controller.rb b/app/controllers/api/v1/users_controller.rb index 72ad51e38..1768e7056 100644 --- a/app/controllers/api/v1/users_controller.rb +++ b/app/controllers/api/v1/users_controller.rb @@ -50,8 +50,8 @@ def update prev_email = user.email super do |user| if user.email_changed? - user.update_attribute(:valid_email, true) - UserInfoChangedMailerWorker.perform_async(user.id, "email", prev_email) + user.update(valid_email: true) + UserInfoChangedMailerWorker.perform_async(user.id, 'email', prev_email) end end end diff --git a/app/mailers/user_info_changed_mailer.rb b/app/mailers/user_info_changed_mailer.rb index e57cae587..92937bd5d 100644 --- a/app/mailers/user_info_changed_mailer.rb +++ b/app/mailers/user_info_changed_mailer.rb @@ -4,7 +4,7 @@ class UserInfoChangedMailer < ApplicationMailer def user_info_changed(user, info, previous_email=nil) @user = user @recipient_emails = [user.email] - @recipient_emails << previous_email if previous_email.present? && info == "email" + @recipient_emails << previous_email if previous_email.present? && info == 'email' case info when "email" @@ -15,6 +15,6 @@ def user_info_changed(user, info, previous_email=nil) template = "password_changed" end - mail(to: @recipient_emails, subject: 'Limited subject', :template_name => template ) + mail(to: @recipient_emails, subject: subject, template_name: template ) end end diff --git a/spec/controllers/api/v1/users_controller_spec.rb b/spec/controllers/api/v1/users_controller_spec.rb index 110c26cdd..cba0737cb 100644 --- a/spec/controllers/api/v1/users_controller_spec.rb +++ b/spec/controllers/api/v1/users_controller_spec.rb @@ -473,13 +473,13 @@ def update_request context "when changing email" do let(:put_operations) { {users: {email: "test@example.com"}} } - let!(:user_email){ user.email } + let!(:user_email) { user.email } after(:each) do update_request end - it "sends an email to the new address if user is valid", focus: true do + it "sends an email to the new address if user is valid" do expect(UserInfoChangedMailerWorker).to receive(:perform_async).with(user.id, "email", user_email) end From aae5b02df5152e41329431a395ad86f0439164a3 Mon Sep 17 00:00:00 2001 From: tooyosi Date: Fri, 16 Aug 2024 17:10:51 +0100 Subject: [PATCH 3/8] [issue-4351] move UserInfoChangedMailer call outside super to prevent race condition --- app/controllers/api/v1/users_controller.rb | 12 ++++++++---- app/mailers/user_info_changed_mailer.rb | 2 +- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/app/controllers/api/v1/users_controller.rb b/app/controllers/api/v1/users_controller.rb index 1768e7056..1fdfedb99 100644 --- a/app/controllers/api/v1/users_controller.rb +++ b/app/controllers/api/v1/users_controller.rb @@ -48,12 +48,16 @@ def me def update prev_email = user.email + email_changed = false super do |user| - if user.email_changed? - user.update(valid_email: true) - UserInfoChangedMailerWorker.perform_async(user.id, 'email', prev_email) - end + email_changed = user.email_changed? + end + + if email_changed + user.update(valid_email: true) + UserInfoChangedMailerWorker.perform_async(user.id, 'email', prev_email) end + end def destroy diff --git a/app/mailers/user_info_changed_mailer.rb b/app/mailers/user_info_changed_mailer.rb index 92937bd5d..849bb89a6 100644 --- a/app/mailers/user_info_changed_mailer.rb +++ b/app/mailers/user_info_changed_mailer.rb @@ -4,12 +4,12 @@ class UserInfoChangedMailer < ApplicationMailer def user_info_changed(user, info, previous_email=nil) @user = user @recipient_emails = [user.email] - @recipient_emails << previous_email if previous_email.present? && info == '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" From d015acf560e0f9346a0bc700a1a636c7b2652881 Mon Sep 17 00:00:00 2001 From: tooyosi Date: Fri, 16 Aug 2024 19:01:07 +0100 Subject: [PATCH 4/8] [issue-4351] edit test case to reflect old mail sent --- app/controllers/api/v1/users_controller.rb | 3 +-- app/mailers/user_info_changed_mailer.rb | 2 +- spec/controllers/api/v1/users_controller_spec.rb | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/app/controllers/api/v1/users_controller.rb b/app/controllers/api/v1/users_controller.rb index 1fdfedb99..c4b5632bb 100644 --- a/app/controllers/api/v1/users_controller.rb +++ b/app/controllers/api/v1/users_controller.rb @@ -57,7 +57,6 @@ def update user.update(valid_email: true) UserInfoChangedMailerWorker.perform_async(user.id, 'email', prev_email) end - end def destroy @@ -120,4 +119,4 @@ def filter_by_params def downcase_filter_params(param_value) param_value.split(',').map(&:downcase) end -end +end \ No newline at end of file diff --git a/app/mailers/user_info_changed_mailer.rb b/app/mailers/user_info_changed_mailer.rb index 849bb89a6..da644cfb7 100644 --- a/app/mailers/user_info_changed_mailer.rb +++ b/app/mailers/user_info_changed_mailer.rb @@ -15,6 +15,6 @@ def user_info_changed(user, info, previous_email=nil) template = "password_changed" end - mail(to: @recipient_emails, subject: subject, template_name: template ) + mail(to: @recipient_emails, subject: subject, template_name: template) end end diff --git a/spec/controllers/api/v1/users_controller_spec.rb b/spec/controllers/api/v1/users_controller_spec.rb index cba0737cb..a8ffb0a1e 100644 --- a/spec/controllers/api/v1/users_controller_spec.rb +++ b/spec/controllers/api/v1/users_controller_spec.rb @@ -479,7 +479,7 @@ def update_request update_request end - it "sends an email to the new address if user is valid" do + 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 From 16e2a9ceaf46539d4a33b8d0ffaa1b189b7a7d79 Mon Sep 17 00:00:00 2001 From: tooyosi Date: Mon, 2 Sep 2024 10:23:21 +0100 Subject: [PATCH 5/8] [issue-4351] add test cases to both UserInfoChangedMailerWorker and UserInfoChangedMailer --- spec/mailers/user_info_changed_mailer_spec.rb | 7 ++++++- .../workers/user_info_changed_mailer_worker_spec.rb | 13 +++++++++++-- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/spec/mailers/user_info_changed_mailer_spec.rb b/spec/mailers/user_info_changed_mailer_spec.rb index 614a28ccd..420e1db33 100644 --- a/spec/mailers/user_info_changed_mailer_spec.rb +++ b/spec/mailers/user_info_changed_mailer_spec.rb @@ -21,10 +21,15 @@ 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 'should notify 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..c06667649 100644 --- a/spec/workers/user_info_changed_mailer_worker_spec.rb +++ b/spec/workers/user_info_changed_mailer_worker_spec.rb @@ -4,8 +4,17 @@ let(:project) { create(:project) } let(:user) { create(:user) } - it 'should deliver the mail' do - expect{ subject.perform(user.id, "password") }.to change{ ActionMailer::Base.deliveries.count }.by(1) + context 'email delivery' do + it 'should deliver the mail' do + expect{ subject.perform(user.id, "password") }.to change{ ActionMailer::Base.deliveries.count }.by(1) + end + + it 'should deliver to the right recipients' do + prev_pass = 'oldpassword@example.com' + subject.perform(user.id, "email", prev_pass) + mail = ActionMailer::Base.deliveries.last + expect(mail.to).to eq([user.email, prev_pass]) + end end context "without a user" do From dedc60decbc1bad8325df3752365cb85ea47b470 Mon Sep 17 00:00:00 2001 From: tooyosi Date: Mon, 2 Sep 2024 11:35:46 +0100 Subject: [PATCH 6/8] [issue-4351] hound cleanups --- spec/mailers/user_info_changed_mailer_spec.rb | 5 +++-- spec/workers/user_info_changed_mailer_worker_spec.rb | 10 +++++----- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/spec/mailers/user_info_changed_mailer_spec.rb b/spec/mailers/user_info_changed_mailer_spec.rb index 420e1db33..79ffb2cb9 100644 --- a/spec/mailers/user_info_changed_mailer_spec.rb +++ b/spec/mailers/user_info_changed_mailer_spec.rb @@ -22,12 +22,13 @@ context "email address was changed" do prev_email = 'prev@example.com' - let(:mail) { UserInfoChangedMailer.user_info_changed(user, "email", prev_email)} + 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 'should notify the previous mail' do + it 'notifies the previous mail' do expect(mail.to).to include(prev_email) 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 c06667649..64fed9309 100644 --- a/spec/workers/user_info_changed_mailer_worker_spec.rb +++ b/spec/workers/user_info_changed_mailer_worker_spec.rb @@ -5,13 +5,13 @@ let(:user) { create(:user) } context 'email delivery' do - it 'should deliver the mail' do - expect{ subject.perform(user.id, "password") }.to change{ ActionMailer::Base.deliveries.count }.by(1) + it 'delivers the mail' do + expect { subject.perform(user.id, 'password') }.to change { ActionMailer::Base.deliveries.count }.by(1) end - it 'should deliver to the right recipients' do - prev_pass = 'oldpassword@example.com' - subject.perform(user.id, "email", prev_pass) + it 'delivers to the right recipients' do + let(:prev_pass) { 'oldpassword@example.com' } + subject.perform(user.id, 'email', prev_pass) mail = ActionMailer::Base.deliveries.last expect(mail.to).to eq([user.email, prev_pass]) end From 554d4a7024d66acc3a5cd3a15eec826f8e982ff4 Mon Sep 17 00:00:00 2001 From: tooyosi Date: Mon, 2 Sep 2024 11:49:50 +0100 Subject: [PATCH 7/8] [issue-4351] hound cleanups --- app/controllers/api/v1/users_controller.rb | 2 +- spec/workers/user_info_changed_mailer_worker_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/api/v1/users_controller.rb b/app/controllers/api/v1/users_controller.rb index c4b5632bb..78ff809d1 100644 --- a/app/controllers/api/v1/users_controller.rb +++ b/app/controllers/api/v1/users_controller.rb @@ -119,4 +119,4 @@ def filter_by_params def downcase_filter_params(param_value) param_value.split(',').map(&:downcase) end -end \ No newline at end of file +end diff --git a/spec/workers/user_info_changed_mailer_worker_spec.rb b/spec/workers/user_info_changed_mailer_worker_spec.rb index 64fed9309..39bc6f02e 100644 --- a/spec/workers/user_info_changed_mailer_worker_spec.rb +++ b/spec/workers/user_info_changed_mailer_worker_spec.rb @@ -3,6 +3,7 @@ RSpec.describe UserInfoChangedMailerWorker do let(:project) { create(:project) } let(:user) { create(:user) } + let(:prev_pass) { 'oldpassword@example.com' } context 'email delivery' do it 'delivers the mail' do @@ -10,7 +11,6 @@ end it 'delivers to the right recipients' do - let(:prev_pass) { 'oldpassword@example.com' } subject.perform(user.id, 'email', prev_pass) mail = ActionMailer::Base.deliveries.last expect(mail.to).to eq([user.email, prev_pass]) From 7506705c96eb23ccd3cd65ae1a9246c362d8f009 Mon Sep 17 00:00:00 2001 From: tooyosi Date: Wed, 4 Sep 2024 17:54:36 +0100 Subject: [PATCH 8/8] [issue-4351] renaming test variables --- spec/workers/user_info_changed_mailer_worker_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/workers/user_info_changed_mailer_worker_spec.rb b/spec/workers/user_info_changed_mailer_worker_spec.rb index 39bc6f02e..0cc7f72c8 100644 --- a/spec/workers/user_info_changed_mailer_worker_spec.rb +++ b/spec/workers/user_info_changed_mailer_worker_spec.rb @@ -3,17 +3,17 @@ RSpec.describe UserInfoChangedMailerWorker do let(:project) { create(:project) } let(:user) { create(:user) } - let(:prev_pass) { 'oldpassword@example.com' } + let(:prev_email) { 'oldemaild@example.com' } - context 'email delivery' do + 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_pass) + subject.perform(user.id, 'email', prev_email) mail = ActionMailer::Base.deliveries.last - expect(mail.to).to eq([user.email, prev_pass]) + expect(mail.to).to eq([user.email, prev_email]) end end