codebuddies / backend

CodeBuddies back-end
https://codebuddies.org
GNU General Public License v3.0
20 stars 25 forks source link

Dj rest auth #187

Closed BethanyG closed 4 years ago

BethanyG commented 4 years ago

What type of PR is this? (check all applicable)

Context

We wanted to implement a user registration process that incorporated an email and verification of that email. See issue #110 and discussion #178 for additional context.

Closes #110, #113, #114, #118

Other Related Tickets & Documents (as needed)

Implementation Details

We had originally planned to use django-rest-auth, but it is no longer maintained, and doesn't play nice with django-allauth anymore. dj-rest-auth is the currently maintained fork of django-rest-auth. However, dj-rest-auth requires the django-simpleJWT library, so the current django-rest-framework-jwt library was replaced.

It was also decided that authorization and registration should live under the main API url instead of being "off to the side", so the URLs and code were refactored to support this. Additionally, settings changes were made to require a validated email upon user registration. Users will no longer be allowed to login without a "verified" email.

More refactor/changes may be needed as we refine user profiles and permissions.

Obtaining JWT tokens has moved to:

Registration/Login/Logout are now located at:

User Details and current_user are located at:

Since these two are redundant, one may be omitted or changed later in the project.

New Libraries/Dependancies Introduced (Fill out as needed)

If you have added libraries or other dependancies, please list them (and links to their repos) below:

djangorestframework-simplejwt dj-rest-auth==1.1.1 django-rest-authtoken==2.1.3

Any new migration files added?

Did you add tests?

Code added or changed without test coverage or good reason for exemption won't be approved.

Did you add documentation?

codecov[bot] commented 4 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (main@6f9a92f). Click here to learn what that means. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #187   +/-   ##
=======================================
  Coverage        ?   81.56%           
=======================================
  Files           ?       33           
  Lines           ?      548           
  Branches        ?        0           
=======================================
  Hits            ?      447           
  Misses          ?      101           
  Partials        ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data Powered by Codecov. Last update 6f9a92f...604bac8. Read the comment docs.

lpatmo commented 4 years ago

Yay!!! πŸ‘πŸ‘πŸ‘

Excellent work, Bethany! I started reviewing this morning but didn't get a chance to finish testing and looking at everything, but will try to finish up tonight/tomorrow.

One small detail that can be fixed right now: project/core/templates/account/email/email _confirmation_subject.txt has an extra space near the underscore.

BethanyG commented 4 years ago

Hey @lpatmo - thanks for the sharp-eyed copyedit! Could you make that suggestion in the PR directly, so we can just accept it here -- or give me a file and line to reference? Not sure where the URL is is mis-typed? Thanks!

EDIT: NM! Found it. Will push a renamed file.

oooofh. Not sure why, but changing that file causes 17 test failures and an endless recursion. Reverting.

BethanyG commented 4 years ago

OK. I think I've fixed both the intermittent test failure and the mis-named file. Test are passing.

BethanyG commented 4 years ago

OK. Mashing the big, green button. πŸ˜„