-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
set valid_email to true on user email change #4292
Conversation
@@ -52,6 +52,7 @@ def update | |||
super do |user| | |||
if user.email_changed? | |||
update_email_user_ids << user.id | |||
user.update_attribute(:valid_email, true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rails/SkipsModelValidations: Avoid using update_attribute because it skips validations.
@@ -482,6 +482,10 @@ def update_request | |||
expect(UserInfoChangedMailerWorker).to receive(:perform_async).with(user.id, "email") | |||
end | |||
|
|||
it "sets valid_email parameter to true" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
Closes #4278 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spec needs to be modified a bit to test this behavior correctly. valid_email
is not set in the user factory, so the database default of 'true' is used when a new user is instantiated. This is why the included test passes even though the user wasn't reloaded from the database: the valid_email
attribute was true to begin with.
You need to create a new user with an invalid email in your spec:
let(:invalid_user) { create(:user, valid_email: false) }
Then, when you check that the attribute was changed in your spec:
expect(user.reload.valid_email).to be_truthy
The controller logic looks good, so this way you know that you started with an invalid email and can confirm that it was successfully changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! For future reference, this is being done instead of using Devise's Reconfirmable module to avoid requiring users who have fixed or changed their email address to confirm. Since the initial confirmation is specifically to weed out spam, this is sufficient.
Describe your change here.
Review checklist
apiary.apib
file?