Skip to content
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

Email Addresses #57

Open
drewroberts opened this issue Mar 21, 2021 · 7 comments
Open

Email Addresses #57

drewroberts opened this issue Mar 21, 2021 · 7 comments
Assignees
Labels
model description Description of a model perpetual Never ending issues

Comments

@drewroberts
Copy link
Member

I want an Email Addresses model that allows Users (#20) to have multiple Email Addresses and to log in with any of the Email Addresses or with the username on the User model.

Email Addresses can only belong to 1 User and are unique in this table. The User relationship is nullable. There must also be a primary boolean that is false by default, but when a User is registered, their Email Address is marked as primary. It can be updated, but a User may only have 1 primary Email Address and they must have 1 primary Email Address.

Before saving the Email Address from tipoff/forms, tipoff/waivers, roberts/leads, or anywhere else an Email Address is saved without a User, the Email Address entry must verify not only that it is unique, but that it passes the standard verification for email address format. In places where a User can be registered such as when creating a Cart in tipoff/checkout or any other packages, the same verification must occur.

Email Addresses also have a validated_at field that will be moved away from the User model. In all of those packages, a custom Validation email should be sent to the Email Address to validate that we are dealing with someone who actually has access to the Email Address.

@drewroberts
Copy link
Member Author

@pdbreen Can you work on this issue in the next few days? This will have a big impact on the User model and change the way we are doing logins throughout the application. I want to make sure I don't miss something.

@drewroberts
Copy link
Member Author

There are way too many string fields throughout the other packages for email and I want to get rid of them and replace with a relationship to this model.

@jasonmccreary
Copy link
Contributor

@drewroberts I think the general architecture is good. My only note would be the interference with Laravel. Allowing additional for login is not a huge deal, really just overriding one method, or if necessary creating a UserServiceProvider.

However, we would definitely want to leave a "primary" email address field on the User model. As moving this would have serious implications for additional auth components (reset password, confirmation, verification, etc). So we'd want to be mindful to craft this as an "add-on" and not a "replacement".

@drewroberts
Copy link
Member Author

Thank you, JMac. Using the Email Address model as an "add-on" instead of a "replacement" adds a few additional headaches such as:

  • Ensuring that an Email Address model is created with every new User and that it is updated when the User is updated
  • Needing to check when a User is registered that the email address is not already registered as a secondary email for another user

Due to the addition of username, we already need to override the login method and create a UserServiceProvider. For confirmation & verification, we handle thing differently already in the following packages:

  • tipoff/checkout
  • tipoff/forms
  • tipoff/waivers
  • tipoff/feedback
  • roberts/leads

As a result, those auth components need to be adjusted. The final one is the password reset and in my opinion, the benefit of this approach outweighs the cost of also rewriting it and the difficulty any existing application will have in trying to implement one of the packages in this ecosystem.

@pdbreen Could you proceed when you have time this week with a PR to replace those auth components with our own version to achieve this desired result with the Email Address model?

@jasonmccreary
Copy link
Contributor

@drewroberts, for clarity, I wasn't saying "don't do it". My main point was that by leaving a primary email address (even if it were duplicate data) on the User model would likely avoid nearly all of the "interference" with Laravel I mentioned. I would think the only thing you would need to change would be the login logic (and that's more for username).

@pdbreen
Copy link
Contributor

pdbreen commented Mar 24, 2021

I disagree with the leave the fields behind strategy -- it leaves too much possibility of code seeming to work properly. I'd rather any code fail loudly during test by absence of the column(s) to ensure the appropriate data in the appropriate tables is being managed properly.

@jasonmccreary
Copy link
Contributor

@pdbreen sure. I was offering the path of minimal effort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
model description Description of a model perpetual Never ending issues
Projects
None yet
Development

No branches or pull requests

3 participants