Skip to content
This repository has been archived by the owner on Jun 16, 2021. It is now read-only.

Tasks Completed #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

davidgarg20
Copy link

@davidgarg20 davidgarg20 commented May 17, 2020

CSoC Task 3 Submission

I have completed the following tasks

  • Basic endpoints
  • Collaborator feature

@krashish8
Copy link
Member

Great work on the assignment @davidgarg20 !
Here are some suggestions:

  • In the register endpoint, you haven't validated whether the user with given username exists or not, so it gives 500 Error.
  • The /auth/profile/ endpoint should give a 401 Unauthorized status code, instead of 404. The user is not authorized and that's the reason his profile can't be viewed.
  • You should have used custom permissions - permissions.py (check whether the user is owner, or to check whether the user is collaborator) to keep the code DRY, instead of validating this everytime.
  • You could have used RetrieveUpdateDestroyAPIView in the TodoDetailView instead of writing the code separately for all. You can make use of these API views if the task is trivial (such as retrieving, updating or deleting etc) instead of using GenericAPIView and repeating the code.
  • Atleast the owner of the Todo should be able to view all the collaborators of a particular Todo.
  • You haven't used serializers effectively. One can easily avoid code repetition by making good use of serializers. Also, as a good practice, validation should be done in the serializers, not in the views.
  • And while committing, you should make sure not to commit the files generated locally which aren't of any use e.g. the .idea folder (created due to your editor). You can add these to .gitignore or else not add them to the staging area.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants