Skip to content

Django Allauth (#201)#274

Merged
scragly merged 35 commits into
masterfrom
#201-django-allauth
Oct 14, 2019
Merged

Django Allauth (#201)#274
scragly merged 35 commits into
masterfrom
#201-django-allauth

Conversation

@gdude2002
Copy link
Copy Markdown
Contributor

@gdude2002 gdude2002 commented Oct 3, 2019

This pull request implements Django Allauth as described in #201.

Fixes #201.


This is a fairly big PR which, I think, may have had a little scope creep. Here's what it changes:

  • Add django-allauth dependency
  • Implements Discord OAuth logins using django-allauth
  • Exposes messages from the Django messages framework as slide-in Bulma notifications at the top right of the page, positioned just below the navbar
  • Re-styles Bulma notifications when used in the context of the Django messages framework
  • Redefines Bulma's bundled colours, in order to make them less loud and make Bulma calculate colours based on the primary blurple better (by redefining turquoise to blurple)
  • Implements some logic to synchronise Discord roles and Django permissions groups based on signals emitted by various events

I would super love some reviews for this, particularly when it comes to the tests I wrote for the signal handlers, and the signal handling itself.

I'm not super happy with the signal handlers test - it's more setup than test! If anyone has any ideas on how to clean that up, I'd appreciate it.


Old TODO:

  • Set up signal listeners for Allauth events
  • Set up signal listeners for relevant Model save events
  • Document the signal system in comments/docstrings
  • Fix up message system
    • Move messages to a corner
    • Restyle messages
      • Discord embed-style messages with the left border?
      • More muted colours? Perhaps shades of the primary blurple.
  • Profile page with GH connect/disconnect and account delete
  • Create templates for the Allauth-provided account pages
  • Review the Allauth URLs to ensure we're only using what we need
    • Ensure login page explains the data we're collecting and collects explicit consent
  • Create a user profile page (or just send them back to the homepage)
  • Figure out how to signpost/link to this system from the rest of the site

@gdude2002 gdude2002 added area: backend Related to internal functionality and utilities enhancement area: frontend Related to site content and user interaction language: python Involves Python code labels Oct 6, 2019
@gdude2002 gdude2002 marked this pull request as ready for review October 6, 2019 20:51
@gdude2002
Copy link
Copy Markdown
Contributor Author

gdude2002 commented Oct 6, 2019

Super happy to say that this PR is now ready for review! Here are some screenshots and videos that showcase the changes I've made.

Login link in dropdown

Login link in dropdown

New Bulma colours, shown here as notification styles

Bulma Colours

Django admin, showing a role mapping

Role mapping

Django admin, showing my Discord account, sans email

image

MP4 showing the login flow

Comment thread pydis_site/apps/home/apps.py
Comment thread pydis_site/apps/home/signals.py
Comment thread pydis_site/apps/home/signals.py Outdated
Co-Authored-By: Johannes Christ <jc@jchri.st>
Comment thread pydis_site/apps/home/signals.py Outdated
Comment thread pydis_site/apps/home/signals.py Outdated
Copy link
Copy Markdown
Contributor

@jchristgit jchristgit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty solid so I will have to resort to notpicking :(

The role mapping model is just about the cleanest way I have seen to accomplish this. I think in my last implementation of this I just hardcoded that, but a nice admin frontend for it is pretty brilliant.

That said, if the role mappings update, shouldn't the signal handler fire?

Comment thread pydis_site/apps/home/tests/test_signal_listener.py Outdated
Comment thread pydis_site/apps/home/tests/test_signal_listener.py Outdated
Comment thread pydis_site/apps/home/tests/test_signal_listener.py Outdated
Comment thread pydis_site/templates/base/navbar.html
@jchristgit jchristgit self-assigned this Oct 11, 2019
@gdude2002
Copy link
Copy Markdown
Contributor Author

gdude2002 commented Oct 11, 2019

Good catch on the missing signals. I'm leaving a short TODO list in this comment so I don't forget:

  • Handle RoleMapping deletions
  • Handle RoleMapping updates (use pre_save and check against object already in DB if possible, otherwise try to disallow updates)

This also enforces unique values for both attributes on the RoleMapping model. Supporting configurations where this isn't the case would introduce quite a lot of added complexity.
@gdude2002
Copy link
Copy Markdown
Contributor Author

Alright, I think that's all of your review addressed, @jchristgit!

Copy link
Copy Markdown
Contributor

@jchristgit jchristgit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very solid work

Copy link
Copy Markdown
Contributor

@lemonsaurus lemonsaurus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very high quality PR, but I do have a few requests before I can approve.

Comment thread pydis_site/apps/home/signals.py Outdated
Comment thread pydis_site/apps/home/signals.py Outdated
Comment thread pydis_site/apps/home/signals.py
Comment thread pydis_site/apps/home/tests/test_signal_listener.py Outdated
Comment thread pydis_site/apps/home/tests/test_signal_listener.py Outdated
Comment thread pydis_site/apps/home/tests/test_signal_listener.py Outdated
Comment thread pydis_site/apps/home/tests/test_signal_listener.py Outdated
Comment thread pydis_site/apps/home/tests/test_views.py
Comment thread pydis_site/apps/home/urls.py Outdated
Comment thread pydis_site/settings.py Outdated
Copy link
Copy Markdown
Contributor

@scragly scragly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely sexy. LGTM

@scragly scragly dismissed lemonsaurus’s stale review October 14, 2019 11:51

Comments all addressed

@scragly scragly merged commit 03d5a4c into master Oct 14, 2019
@scragly scragly deleted the #201-django-allauth branch October 14, 2019 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: backend Related to internal functionality and utilities area: frontend Related to site content and user interaction language: python Involves Python code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Django: Oauth integration

4 participants