FEATURE: Improve UX support for multiple email addresses (PR #9691)

GitHub

shouldn’t we do all of this this from inside setPrimaryEmail in models/user.js file given it’s all model logic ?

if possible please use @action also maybe a more descriptive name like: onChangeEmail

I think you could get the router with a service, that would be better

The title of this pull request changed from “FEATURE: Improve UX for user emails” to "FEATURE: Improve UX support for multiple email addresses

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/confused-by-error-message-from-reply-by-email/139841/21

Looks like we never used that HTTP status before. How does the UI handle that?

What’s the “type” of the type variable? Isn’t it a symbol? If it is, when are we converting it to a string instead of checking directly against other symbols?

    return if errors.present? || existing_user.present?

I wonder if we should add an entry in the staff logs about this operation?

I wonder if we should add an entry in the staff logs about this operation?

Sidekiq converts symbol arguments to strings, so it should be a string, unless the job is executed directly.

It should only happen if the user deletes the same email address. I will show a bootbox if this happens.

That sounds like a good idea.

That sounds like a good idea.

Shouldn’t this be done in the EmailUpdater class instead of the controller?

Also, we should not be logging the whole email address (cf. the moderators_view_emails site setting). I think just having a log that some staff member changed the email address of user X to be enough.

Can we do the return if errors.present? earlier (eg. right after we validate the email) so that we avoid doing any work if the email is not valid?

@tgxworld can you review this? With an “angle” on the security-side.

I’d like to merge it before the end of the week and I’d love another pair of eyes.

      UserHistory.create!(action: UserHistory.actions[:update_email], target_user_id: user.id, previous_value: old_primary.email, new_value: new_primary.email)

I feel like logging should be part of the same transaction as updating the primary status of the email address as well.