-
Notifications
You must be signed in to change notification settings - Fork 4
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
made pennkeys lowercase and changed test case #680
Conversation
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.
Looks good! Left a comment for some edge cases
Would also be great to add 1-2 sentences on why this is needed! It's helpful to have more documentation on why we're making changes. |
Do you want me to add comments in the code or in some sort of documentation? |
Just within the PR itself would be sufficient. Something like this. |
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.
Looks great! Requested some small fixes to make the code more succinct.
backend/courses/views.py
Outdated
username = request.data.get("pennkey") | ||
if not username: | ||
res["message"] = "User not found" | ||
return Response(res, status=status.HTTP_404_NOT_FOUND) |
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.
Can make this more succinct and use raise Http404("User not found.")
.
backend/courses/views.py
Outdated
username = request.data.get("pennkey") | ||
if not username: | ||
res["message"] = "User not found." | ||
return Response(res, status=status.HTTP_404_NOT_FOUND) |
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 thing here.
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 – thanks Nikhil!
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.
looks great!
* made pennkeys lowercase and changed test case * Added handling for edge cases * fixed whitespace * Fixed more whitespace. All stylizing * changed the response schema * made http404 message more succinct * imports in alphabetical order I think * removed one res = {}
Made it so when people send friend requests on penn courses, it changes the username to all lowercase so that when people add friends and accidentally capitalize the first letter it still is able to find the friend they are looking for.