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

enter the username of user you want to add as collaborator and the id… #9

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

Conversation

Captain-Kirk83
Copy link

@Captain-Kirk83 Captain-Kirk83 commented May 16, 2020

… of task in which you want him to collaborate

CSoC Task 3 Submission

I have completed the following tasks

  • Basic endpoints
  • Collaborator feature

… of task in which you want him to collaborate
@krashish8
Copy link
Member

krashish8 commented May 23, 2020

Good work @Captain-Kirk83!
Here are some suggestions:

  • Your validation logic is wrong in the RegisterSerializer. To check whether a user exists, you should use User.objects.filter(username=username) instead. On creating another user with same username but different password, it receives 500 error.
  • You have added an extra /users/ endpoint through which all the users are publicly available. This isn't a good practice.
  • In /todo/create POST endpoint, you should also return the Todo details alongwith the 200 Response.
  • In /todo/ GET endpoint, you are returning complete user details like hashed value of password too, which isn't good. You should have used UserSerializer instead.
  • In /todo/ GET endpoint, when the todo has both creator and a collaborator, the details are returned multiple times. For every existing Todo, it should be displayed only once in the response.
  • In adding and removing collaborators, it returns 500 Error on giving wrong Todo id or giving wrong username.
  • 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.
  • As a good practice, validation should be done in serializers, not in the views.
  • Also, while returning a 400 response, you should give a custom error message too, telling the user where the error occurred. However, this is fine for this assignment.

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