Additional email support

planned

(Leo McArdle) #1

After some chat with the @team, we’re moving forward with this feature request cautiously together, and in phases.

Phase 1

Phase 1 won’t change anything but the architecture of how a user’s email address(es) are stored. No new features, just some well executed database migrations:

  • Create a new user_email table, with:
    • email column, unique with special handling to support case insensitivity (treating bob@ and BOB@ as the same)
    • user_id column
    • timestamps
  • Migrate the email column from the user table into the new user_email table
    • Add email_id column to user table, not null to enforce primary email as user.email

The associations would therefore be:

  • User
    • belongs_to :email, class_name: 'UserEmail'
    • has_many :user_emails
  • UserEmail
    • belongs_to :user

@sam this is a little different from what we discussed, but I think it makes migration even easier (keeping user.email pointing to the primary email). Thoughts?

Phase 2

Phase 2 will involve adding server side handling of multiple email addresses. At this point this is less clearly defined, and still requires some discussion and planning. A feature almost certainly in this phase will be supporting email-in from alternative addresses.

Phase 3

Phase 3 is even less clearly defined than phase 2, and requires even more discussion and planning, but will focus on adding the UI and user flows to make all of the work done in phases 1&2 useful.


Coding will start on Monday, so I’ll update the topic then with the crucial parts of phase 1 I’ve forgotten to think through.


Discourse Patreon Integration
Two emails for one user
Can I start a new topic by sending an email message?
(Sam Saffron) #2

Well that is not going to work :slight_smile: cause the way you have it wired it would be User.find(1).email.email which is very confusing.

I don’t mind if we wire up a custom method that looks up primary email address but the belongs_to should be belongs_to :primary_user_email, class_name: 'UserEmail'

Also, phase 1 ensures this does not introduce and N+1 queries when looking at user lists (in particular admin can very easily regress here to N+1)


(Leo McArdle) #3

Duh, of course. That’s definitely not what I was going for.

Am I right in thinking there’s nothing inherent in the architecture I’ve proposed which avoids N+1 queries, but instead phase 1 gives us a chance to catch any that are happening? And the way to avoid them is eager loading in these instances, right?

Are you opposed to calling that method email to avoid having to go through each and every .email and replacing it, or is that a necessary part of avoiding N + 1?


(Leo McArdle) #4

After recovering from eating horrendous quantities of chocolate, I’ve finally got back to working on this.

This is what I’ve come up with for phase 1:


(Alan Tan) #5

I’m working on getting this merged but on a closer look I realized that the proposed associations would not work. The UserEmail table needs to have a constraint that the user_id column is not null. Note that the constraint is essential as we don’t want to have an UserEmail record that belongs to no one. At the same time, we’re adding a primary_email_id column which makes sure that a user will always have an email. With those two constraints in place, it isn’t possible to create either an User or an UserEmail record.

Instead we should just add a primary column to the UserEmail table and just make primary_email a scope.


(Alan Tan) #8

Thank you for working on this @LeoMcA :slight_smile: I’ve merged in your PR in


(Leo McArdle) #9

Happy to help, thanks for your and @sam’s work to ensure I didn’t break everything!

With phase 1 merged, I think it’s time to crack on with phase 2!

I’ve been through the changes as a result of phase 1 and found a few areas where we might want to add support for alternative email addresses:

  • email receiver
  • user email address updater
  • topic importers
  • authentication
  • sso

That being said, I think it makes more sense to add each of these as features in their own right, rather than all grouped together as one phase. After all, if phase 1 has been implemented as intended, none of these are broken at the moment, they’re just not as feature rich as they could be.

Adding support for receiving emails from alternative email addresses is the reason why we wanted to see support for alternative emails in Discourse within Mozilla, and in previous discussions it was an oft-mentioned use-case, so I’ll be getting started on that early next week - unless a member of the @team pickets my house or something.


(Jeff Atwood) #10

Any updates on where we are with this? I believe all the groundwork is in place and tested as we have a one to many relationship between accounts and emails now in 1.9?


(Leo McArdle) #11

Yeah, this dropped down the priority list as a result of a lot of work needing to happen internally on our new authentication system (and because of a dramatic bandwidth reduction on my end due to starting at university).

Most of that authentication work is done now, so this is pretty much next on the list.

Latest I should end up doing it is around Christmas.


(Jeff Atwood) #12

Going slow is fine, as absorbing the initial one-to-one → one-to-many change was quite a bit of risky work!


(Leo McArdle) #13

I have a few PRs in the pipeline to finish this work off: one to allow multiple additional emails, another to add specs which show all aspects of email receiving working with additional emails, and one I’m currently working on around the UI.

I have a couple of questions:

First, what do we actually want to refer to these non-primary emails as? I’ve been using a number of different words (in my brain, and as a result in my code). There’s:

  • alternate (which screws with my poor british brain)
  • alternative
  • additional
  • secondary (which makes the most sense to me)

I don’t have particularly strong opinions about it, so if someone wants to tell me which to use I’ll start being consistent.

Secondly, is it going to feel like I’ve acted in bad faith if I don’t implement the user-facing UI for this? I ask because we’ll be implementing our own in our own authentication plugin, so any code I write in core implementing this is completely useless to Mozilla.

The PR I have lined up implements these things in core:

  • secondary emails in the user serializer
  • secondary emails returned when an admin clicks “show email”
  • secondary emails shown in the user admin page
  • a plugin outlet right below the existing email section on my/preferences/account

This allows a plugin developer to implement something like this very easily, with just a little ember:

Screenshot from 2018-01-05 13-04-26

I feel like this PR would be a fine place to end my work on this - as it adds everything necessary to core to allow an authentication plugin to manage the addition and removal of secondary emails - but as I said I don’t want to lie awake at night worrying the core team don’t like me because I haven’t followed through on the full implementation of this feature. :slight_smile:


(Carson) #14

Is this live in Discourse? A great feature, amazing work from the community!


(Sam Saffron) #15

Its a in-progress to be merged PR, adding this to @tgxworld list after he is done with 2fa.


(Stephen Chung) #17

Once we have this, then we really need the feature to merge users…