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

Complete all the basic end points and add collaborator feature. #3

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

Conversation

try-catch-stack
Copy link

@try-catch-stack try-catch-stack commented Aug 6, 2021

CSOC Task 5 Submission

I have completed the following tasks

  • Task 1: Completed all the basic endpoints- Auth - Login, Register, Get Profile and Todo - Get Detail, Get List, Create, Edit(PUT and PATCH) and Delete.

  • Task 2: Implemented collaborator feature- The creator of a todo can add or remove the collaborators to a todo.Collaborators have the permissions to delete or edit the todo they have been added to as a collaborator. Any todo can have multiple collaborators but a collaborator cannot add or remove a collaborator from the todo.
    Note: Collaborators can edit a todo only by sending PATCH requests and not PUT requests as PUT will replace the existing document with the updated one which will also remove the existing collaborators from the todo.

End points for collaborator feature:

Both the end points take user token as the authorization header with the prefix Token and User id in the request body.

Add collaborator : POST /todo/:id/add-collaborators/
Remove collaborator : POST /todo/:id/remove-collaborators/

Request Body (Sample):

{
  "userid":"610bfb7f1d4fbe7cc65304b2"
}

Response Body (Sample):

{    
  "Message":"User with the given id successfully added as collaborator.",
  "TodoId":"610c0e381e7db589c1dfebe2",
  "TodoTitle":"Sample TODO"
 }

Response Code: 200

API Documentation

Deployed API URL: https://rest-api-todoapp.herokuapp.com/api/

@K-Kumar-01 K-Kumar-01 self-assigned this Aug 23, 2021
Copy link
Collaborator

@K-Kumar-01 K-Kumar-01 left a comment

Choose a reason for hiding this comment

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

@try-catch-stack
Some general changes which I would suggest.

  1. Try formatting the code with prettier or any appropriate formatter.
  2. Remove the comments which were added for explanation to you or when work gets done. If something needs explanation, then only comment should remain there.
  3. The commit messages made were not good. Please see on how to write good commits. They are also essential.

I didn't check for the collaborator feature yet as it is dependent on userId which I am unsure of how to get(can use mongodb compass but think of me as someone who doesn't know about that).
If you can, please change the logic, otherwise, I will see to it later.

Comment on lines +17 to +19
todosOfUser.push({"id":todo._id,"title":todo.title})
}else{
todosCollaboratedTo.push({"id":todo._id,"title":todo.title})
Copy link
Collaborator

Choose a reason for hiding this comment

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

@try-catch-stack
Try using spread operator. It is faster. See here for more details.

const {title} = req.body
const user = req.user
console.log
if(title.trim()==""){ //Checking if title is empty string(Only white space)
Copy link
Collaborator

Choose a reason for hiding this comment

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

"" is treated as false value in javascript. So, there is no need for equal sign.
Better would be

if(!title.trim()){
//logic here
}

Comment on lines +48 to +52
if(savedTodo){
res.status(200).json({
"id":todo._id,
"title":todo.title})}else{
res.status(400).send("Title field cannot be empty.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

what will happen if an error occurs here? Nothing. Handle the case for that.

const user=req.user

ToDo.findOne({$or: [{_id:id,createdBy:user._id},{_id:id,collaborators:user._id}]},function(err,foundTodo){
if(err) { console.log(err)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of consoling the error, return the error with proper codes.
Here it is an internal error, so a 500 code can be sent.

Comment on lines +141 to +148
if(todo.collaborators.includes(userid)){return res.status(200).send("This user is already a collaborator for the TODO.")}
todo.collaborators.push(userid)
todo.save()
res.status(200).json({"Message":"User with the given id successfully added as collaborator.",
"TodoId":todo._id,
"TodoTitle":todo.title})
}else{
res.status(400).send("Error in adding collaborator.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are adding a collaborator by userID. How am I supposed to get that user ID?
Ids are generally not meant for disclosure. Since username or email is kept unique, try using them for addition.

Basically, what I mean is one will provide a field userName: <value> . Find user, get that ID and use the above logic. But don't expect ID from the body.

if(!err && todo){

if(!todo.collaborators.includes(userid)){return res.status(200).send("The user you are tying to remove is already not a collaborator.")}
ToDo.findOneAndUpdate({_id:id,createdBy:user._id},{$pull:{collaborators:userid}},function(err,updatedTodo){
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any specific reason as to why using $pull here but .push in the addition of collaborators?

title: { type: String, required: true },
createdBy: { type: Schema.Types.ObjectId, ref: "User" },
collaborators: [{ type: Schema.Types.ObjectId}]
Copy link
Collaborator

Choose a reason for hiding this comment

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

A ref is not created here. I don't think it will work.

const router = Router();

router.get("/", ToDoController.getAllToDo);
router.post("/:id/", ToDoController.createToDo);
router.get("/",middleware.checkAuth, ToDoController.getAllToDo);
Copy link
Collaborator

Choose a reason for hiding this comment

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

use router.use() for middlewares.

Copy link
Collaborator

@K-Kumar-01 K-Kumar-01 left a comment

Choose a reason for hiding this comment

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

Checked for todos and they were working nice. Good job

"TodoId":todo._id,
"TodoTitle":todo.title})
}else{
res.status(400).send("Error in adding collaborator.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

When I add a collab via a different token, I get an error in adding collab. Explain the error. A better way would be to explain whether I am unauthorized or there is internal error.

@K-Kumar-01
Copy link
Collaborator

K-Kumar-01 commented Aug 23, 2021

Marks:
task-1: 95
task-2: 97

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

Successfully merging this pull request may close these issues.

2 participants