Standardize callbacks for user secondary emails
What does this MR do?
This MR removes before_validation
callbacks for user's secondary emails: public email
and commit email
. Those callbacks kept the validations from kicking-in - user would not be shown an error message when trying to change secondary emails to a not verified email address.
Another before_validation
was introduced instead - reset_secondary_emails
- which is called when primary email address is changed, and makes sure, that secondary emails are either verified addresses or are not set at all.
This makes code cleaner, but there is still some awkwardness left resulting from user having a primary email in user.email
and also emails as user.emails
, while user.email
is not necessarily one of user.emails
.
Changing that requires another MR.
This MR also makes few implementation details methods, not used outside of class, private.
Notes:
- there are also
before_save
callbacks forpublic email
andcommit email
and I have left them in place, asUsers::UpdateService
enables skipping validations and that is used in few places in the codebase.
In general I would avoid skipping validations, but I don't have enough context here, maybe there is a good reason to do it in those places.
- the MR arose from investigating the bug #219745 (comment 647352981)
Does this MR meet the acceptance criteria?
Conformity
-
I have included changelog trailers, or none are needed. (Does this MR need a changelog?) -
I have added/updated documentation, or it's not needed. (Is documentation required?) -
I have properly separated EE content from FOSS, or this MR is FOSS only. (Where should EE code go?) -
I have added information for database reviewers in the MR description, or it's not needed. (Does this MR have database related changes?) -
I have self-reviewed this MR per code review guidelines. -
This MR does not harm performance, or I have asked a reviewer to help assess the performance impact. (Merge request performance guidelines) -
I have followed the style guides. -
This change is backwards compatible across updates, or this does not apply.
Availability and Testing
-
I have added/updated tests following the Testing Guide, or it's not needed. (Consider all test levels. See the Test Planning Process.) -
I have tested this MR in all supported browsers, or it's not needed. -
I have informed the Infrastructure department of a default or new setting change per definition of done, or it's not needed.