[User Registration] Make email required on backend

[lpatmo]

Context

We need to make sure the email field is required when a new user is registered via the /auth/users API endpoint.

Acceptance Criteria

[ ] Change the line in the model to make sure email is required
[ ] Make migrations file and run migrations
[ ] Add models test
[ ] Make sure tests pass

Ticket sizing

1 (small)

[BethanyG]

@lpatmo This is not an API change and since we are using a custom model, it is an addition (I keep forgetting that too!)

Add email in users_user as a required (non-blank) field.

Since we are using a custom user model, this change will need to be made in the model.py file of the users app.

• Custom User Models (Django Docs)
• Referencing the Custom User Model (Django Docs)
• Considerations for a Custom User Model
• Related SO Post on Requiring Email for Custom User Model

[lpatmo]

Gah I was going to give this to @bkbuilt or someone else who expressed interest since it's a good beginner issue, but started playing around to test it out in a new branch and realized I might as well create a PR for it.

PR: https://github.com/chris48s/backend/pull/1/files

Note that it is based off of #109.

I documented my implementation steps in chris48s#1, so if something drastic changes in #109 the implementation steps should be easily re-created.

[lpatmo]

(Or... if #109 gets merged in first, I'll just re-submit the PR to point towards master instead of towards Chris' branch)

[BethanyG]

@lpatmo I hate to do this to you, but for clarities sake, I really think this should be a separate PR...just so we keep related PRs with their milestones??

[lpatmo]

No problem, I meant to do this as a separate PR, but realized this was dependent on the other PR being merged in 😅. Happy to resubmit the PR to master after that other PR is resolved!

[BethanyG]

🤔 ...yeah. We need to get our arses in gear around testing....aaaaand you'll need to add a model test for your change 😄 .

[lpatmo]

Haha, cool! I missed that in my acceptance criteria 😅

[lpatmo]

I think we can recreate what I did in https://github.com/chris48s/backend/pull/1/files, plus add some tests to the model. Need to research where that will live, though 🤔

[bengineerdavis]

I think we can recreate what I did in https://github.com/chris48s/backend/pull/1/files, plus add some tests to the model. Need to research where that will live, though thinking

Do you mind adding this and other things you find for our 'prep' in our public docs?

[BethanyG]

@lpatmo @bengineerdavis @tgrrr -- apologies for not replying to this earlier. This change is required for #113, #110, #113 which I am currently working on. I don't think that what I am doing will interfere with this -- or vice-versa, but there may be merge conflicts if we're both checking in code in the next few days.

[lpatmo]

Thanks for the heads up! We probably won't have a PR up sooner than next Tuesday (when https://codebuddies.org/hangout/nAadjwkpEsF9gsTGM is scheduled), fwiw.

In the meantime, added this as reading to the google doc: https://www.obeythetestinggoat.com/book/chapter_database_layer_validation.html

[BethanyG]

In investigating further the changes needed for email validation in registration, I've made a few discoveries that now (I believe) cancel out this change entirely.

Firstly, I didn't realize that cookiecutter-django (which we used to generate this project) pre-installed django-allauth in addition to implementing a custom user model. See Cookiecutter Settings Page for a little info on that.

Secondly, in looking at the DB and the allauth configuration docs and the allauth custom user models docs around configuration and custom user models, I noticed the following:

and

...which means that allauth - either configured for social OR configured for email uses a special app and set of tables under accounts to store emails for users for authentication -- not the users_user table. The flag ACCOUNT_EMAIL_REQUIRED=True is for the account app and the account_email table -- NOT the email field in the users app, or the users_user table!

Why is this important? Having django-allauth pre-installed means we don't have to create the user registration/validation/confirmation flow from scratch - much of it is handled by the allauth app. BUT. That also means that the email requirement needs to be applied through django-allauth -- not through the users model.

Apologies that I didn't see this before -- would probably have saved a lot of reading and work. We may still need to add an email field to the model...I am not quite clear on the interactions just yet.

[BethanyG]

Recommend that if you do go ahead and make this change that it not be merged (nor should the registrations changes I am making be merged) until we decide which direction we are going to go with registration.

[lpatmo]

D'oh and LOL SO GLAD YOU NOTICED THIS!!

I'll continue the discussion re: authentication in #178 :)

(In the meantime, I'll close this issue since it's not needed anymore)

[tgrrr]

Copied for reference here:

@BethanyG wrote on #173:

We closed #111, because the authentication scheme we are using doesn't trigger off an email on the custom user model. See comments there and on discussion number
#178.

[tgrrr]

I also found this branch from @lpatmo working on this issue, dropping the link here for reference: ff5c10b

[BethanyG]

@tgrrr - this issue has been closed by @lpatmo and is unlikely to be reopened. Please see the comments above. Unless we decide we are re-creating email validation and user registration flows from scratch (very unlikely), we will have no need for the change. @lpatmo work was never merged and is not present in main.

[bengineerdavis]

That makes sense that we don't want to reinvent the wheel if we don't have to. I got the impression that the settings.py change for the environmental variable you found hadn't been changed, or was that already pre-configured?

…

On Tue, Sep 8, 2020 at 1:56 AM BethanyG ***@***.***> wrote: @tgrrr <https://github.com/tgrrr> - this issue has been closed by @lpatmo <https://github.com/lpatmo> and is unlikely to be reopened. Please see the comments above. Unless we decide we are re-creating email validation and user registration flows from scratch (very unlikely), we will have no need for the change. @lpatmo <https://github.com/lpatmo> work was never merged and is not present in main. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#111 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AMQU4TVY5XLKH7KB2HUMDGDSEXBQLANCNFSM4LRM4YWA> .