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

Initial submission for week 3 Dev #11

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

Satendra124
Copy link

CSoC Task 3 Submission

I have completed the following tasks

  • Basic endpoints
  • Collaborator feature

@Satendra124 Satendra124 marked this pull request as ready for review May 18, 2020 02:54
@Satendra124
Copy link
Author

Everything working locally and is ready for review.

@krashish8
Copy link
Member

Good work @Satendra124!
Here are some suggestions:

  • In /todo/create/ POST endpoint, if multiple todos have the same title, it gives 500 error in getting the Todo.
  • In /todo/create/ POST endpoint, while adding a collaborator, you haven't validated whether the user exists. It given 500 error response.
  • You haven't added any validation in the Todo GET view, anyone can view others' todo.
  • You have created an extra /todo/edit/{id} endpoint (maybe due to the RetrieveAPIView). UpdateAPIView shall be used in PUT and PATCH.
  • Again, in SpecificTodoView, you have used RetrieveAPIView but have included the delete function. Although this works, but this isn't a good practice. Also, you haven't added validations here. Anyone can delete others' todo.
  • You could have used RetreiveUpdateDestroyAPIView 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 or deleting etc) instead of using GenericAPIView everytime.
  • 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 haven't used serializers effectively. As a good practice, validation, user creation, etc. should be done in the serializers, not in the views.
  • In the UserProfileView, you can directly get the user from request.user instead of filtering using the token.

@Satendra124
Copy link
Author

@krashish8 yes i knew there were a lot of unhandled errors . i could have fixed them but learning alot of concept took too much time for me . This code was completed in like 2 days before that i was just trying to figure out how things work .
Will take care of those points in future .
Thanks 😊

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