[issue-4351] add logic to notify old user email of email change #4374
Conversation
![hound[bot]](https://avatars.githubusercontent.com/in/3598?s=60&v=4){kind=small}
| @user = user | ||
| @email_to = user.email | ||
| @recipient_emails = [user.email] | ||
| @recipient_emails << previous_email if previous_email.present? && info == 'email' |
There was a problem hiding this comment.
previous_email.present? can be shortened to just if previous_email"
Also I think this line can be moved to inside the when "email" block in the case statement below as they both have the same condition.
| super do |user| | ||
| if user.email_changed? | ||
| user.update(valid_email: true) | ||
| UserInfoChangedMailerWorker.perform_async(user.id, 'email', prev_email) |
There was a problem hiding this comment.
I think this will reintroduce the race condition that the tap/array-with-a-single-id thing seeks to avoid. The save in update happens after the block is yielded, so when the worker is called before the super block is finished, the user hasn't been updated in the db and the email will go to the previous address.
Moving the worker call outside of the super block should be sufficient, assuming that user.email_changed? is still accurate after the save. It's possible that "changed?" info is lost after the transaction, though, and perhaps that's why the update_email_user_ids.each exists. Either way, it's gotta be outside the super block in order to ensure that the new address is saved while you pass along the old one.
There was a problem hiding this comment.
I get now. The user.email_changed? is not very accurate outside the save so i'm passing it through a variable now
I captured this behaviour in the |
|
This behavior looks good. I think if you're introducing changes to the interfaces for both UserInfoChangedMailer and UserInfoChangedMailerWorker that you should add a test for each that includes the |
| @@ -21,10 +21,15 @@ | |||
| end | |||
|
|
|||
| context "email address was changed" do | |||
| let(:mail) { UserInfoChangedMailer.user_info_changed(user, "email")} | |||
| prev_email = '[email protected]' | |||
| let(:mail) { UserInfoChangedMailer.user_info_changed(user, "email", prev_email)} | |||
There was a problem hiding this comment.
RSpec/EmptyLineAfterFinalLet: Add an empty line after the last let.
RSpec/DescribedClass: Use described_class instead of UserInfoChangedMailer.
Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
| @@ -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 | |||
There was a problem hiding this comment.
a good idea, if just to shut the hound up later
| @@ -3,9 +3,18 @@ | |||
| RSpec.describe UserInfoChangedMailerWorker do | |||
| let(:project) { create(:project) } | |||
| let(:user) { create(:user) } | |||
| let(:prev_pass) { '[email protected]' } | |||
There was a problem hiding this comment.
I think this should this be prev_email here and below?
| @@ -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 | |||
There was a problem hiding this comment.
a good idea, if just to shut the hound up later
Describe your change here.
Review checklist
apiary.apibfile?