-
-
Notifications
You must be signed in to change notification settings - Fork 771
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
Add django auth framework #574
Conversation
Codecov Report
@@ Coverage Diff @@
## master #574 +/- ##
==========================================
+ Coverage 33.99% 34.36% +0.37%
==========================================
Files 101 99 -2
Lines 5775 5735 -40
Branches 672 664 -8
==========================================
+ Hits 1963 1971 +8
+ Misses 3733 3685 -48
Partials 79 79
Continue to review full report at Codecov.
|
Ouch. Authentication checks without tested views kills coverage! Adding some view tests in this PR to resolve the coverage issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have not tested but code LGTM. mind posting a little bit about how you regression tested this?
app/app/urls.py
Outdated
# Interests | ||
path('actions/bounty/<int:bounty_id>/interest/new/', dashboard.views.new_interest, name='express-interest'), | ||
path('actions/bounty/<int:bounty_id>/interest/remove/', dashboard.views.remove_interest, name='remove-interest'), | ||
path('actions/bounty/<int:bounty_id>/interest/', dashboard.views.interested_profiles, name='interested-profiles'), | ||
# Legacy Support | ||
path('legacy/', include('legacy.urls', namespace='legacy')), | ||
re_path(r'^logout/$', auth_views.logout, name='logout'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we may have to modify some of the cloudfront settings associated with this
app/dashboard/views.py
Outdated
@@ -518,7 +537,7 @@ def bounty_details(request, ghuser='', ghrepo='', ghissue=0): | |||
params['is_legacy'] = bounty.is_legacy # TODO: Remove this following legacy contract sunset. | |||
if profile_id: | |||
profile_ids = list(params['interested_profiles'].values_list('profile_id', flat=True)) | |||
params['profile_interested'] = request.session.get('profile_id') in profile_ids | |||
params['profile_interested'] = profile_id in profile_ids |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strangely enough it doesnt seem like profile_interested
is even being used at all in the template
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I noticed that but opted to not yank it out yet. I can remove it here though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
added a new link to the django github url route in #541 -- will need to make sure that is migrated in this PR! |
@owocki Faucet GH URL reverse updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the Python looks good! So does the @login_required mean that people need to have accounts now? Or is it still using the github account?
@@ -538,7 +544,21 @@ def bounty_details(request, ghuser='', ghrepo='', ghissue=0): | |||
|
|||
|
|||
def profile_helper(handle): | |||
"""Define the profile helper.""" | |||
"""Define the profile helper. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docstrings 👌
Is there any additional testing that needs to be done? I looked at the code but hard to get a feel for the new changes without running it through the bounty process. |
Will check for breadth tonight, will check for depth and test on the weekend :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
PS: the naming convention fixes :D
#impressed
app/dashboard/views.py
Outdated
bounty = Bounty.objects.get(pk=bounty_id) | ||
except Bounty.DoesNotExist: | ||
return JsonResponse({'errors': ['Bounty doesn\'t exist!']}, | ||
status=401) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor thing but if bounty doesn't exist this should be a 404, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure where/why this showed up here, but I didn't add that xD It was from existing code that changed lines. Agreed, though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mbeacom sure we believe you :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thelostone-mc hmm you believe this or not? Not sure if I'm convinced 😂🤣
app/dashboard/views.py
Outdated
return JsonResponse({ | ||
'errors': ['Party haven\'t expressed interest on this bounty.'], | ||
'success': False}, | ||
status=401) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, 404 here?
@mbeacom Also digging the naming convention 👍🏻raised very minor concerns over 401 vs 404 in a couple spots but otherwise LGTM :) |
@mbeacom This guy just go live on production? 😍 Just a brainstorming question on DashboardTokensTest section... @thelostone-mc do you think we hit everything we needed to hit as far as assertions go for this? |
@mkosowsk It's on stage, tested it out |
Description
The goal of this PR is to enable the Django authentication and user framework, refactoring our current auth flow to use
User
handling with GH, and remove custom Github middleware / views.Checklist
Affected core subsystem(s)
Auth, Sessions
Testing
This will need tested on staging before being merged live.
Refers/Fixes
Fixes: #312
Refs: #234