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

Add JWT authentication support on the backend #67

Closed
wants to merge 7 commits into from
Closed

Conversation

71c
Copy link
Collaborator

@71c 71c commented Feb 27, 2020

Closes #58
Followed a tutorial for adding JWT authentication support on the backend. The user view was changed to use a view instead of a viewset to follow the tutorial (but I kept the viewset commented out for now).

Copy link
Collaborator

@duci9y duci9y left a comment

Choose a reason for hiding this comment

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

I think everything looks good but we should discuss trying to use the newer library before merging.

@@ -12,6 +12,8 @@ gunicorn = "*"
django-heroku = "*"
djangorestframework = "*"
pillow = "*"
djangorestframework-jwt = "*"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Based on the update here, I think we should pivot to using a newer, active library.

return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)


"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

You probably don't need this comment to be a docstring.

controversial
controversial previously approved these changes Feb 27, 2020
Copy link
Collaborator

@controversial controversial left a comment

Choose a reason for hiding this comment

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

Left a few minor comments that you could address (or not). Looks good overall

from .models import *

# Serializers to support seralizing User and Group objects
class UserSerializer(serializers.HyperlinkedModelSerializer):
class Meta:
model = User
fields = ['email', 'first_name', 'last_name', 'groups', 'is_staff']
# Should it have the 'username' field?
fields = ['email', 'first_name', 'last_name', 'groups', 'is_staff', 'username']
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we necessarily need username in addition to email/first/last, but up to you

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the "username" field is required?

"""
API endpoint that allows users to be viewed or edited.
Create a new user. It's called 'UserList' because normally we'd have a get
method here too, for retrieving a list of all User objects.
Copy link
Collaborator

@controversial controversial Feb 27, 2020

Choose a reason for hiding this comment

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

The official convention (just for the future) is that Python docstrings like this one should contain only client-facing descriptions of the functionality, and not include implementation-specific notes that consumers of your class/function won't need to know about. This is the same philosophy behind the "function contracts" we're being asked to write in Comp 15. You don't necessarily need to change it this time, but you should keep this in mind for the future as a best practice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just used the same code that they have in the tutorial.

return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)


"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can go ahead and remove code from previous git revisions instead of commenting it out. If it's needed in the future it will still be in git history

@duci9y duci9y temporarily deployed to tutv-issue-58-q7e8ocv81nza9qke February 28, 2020 18:16 Inactive
@duci9y
Copy link
Collaborator

duci9y commented Mar 3, 2020

Closing in favor of #73

@duci9y duci9y closed this Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add JWT authentication support on the backend
3 participants