-
Notifications
You must be signed in to change notification settings - Fork 17
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
Dev Raj - Completed all tasks #7
base: master
Are you sure you want to change the base?
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.
LGTM 🎉.
Dropped some reviews.
Also, look at commit conventions to improve further.
|
||
app.use("/",(err,_req,res,next)=>{ | ||
try{ | ||
if (err) { | ||
if (err.statusCode){ | ||
res.status(err.statusCode).json({ | ||
type: err.type, | ||
}); | ||
} else { | ||
res.sendStatus(400); | ||
} | ||
} else { | ||
next(); | ||
} | ||
} catch(_) { | ||
res.sendStatus(500); | ||
} | ||
}); | ||
app.use("/api/auth", UserRoutes); | ||
app.use("/api/todo", ToDoRoutes); | ||
|
||
app.use("/api/todo",authenticate,ToDoRoutes); | ||
app.use("/",(_,res)=>res.sendStatus(404)); |
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.
guide me on the first and last app.use()
const mongoose = require('mongoose'); | ||
const isValidUsername = (username) =>{ | ||
const usernameRegex = /^[\w.@+-]{1,150}$/gm | ||
if (typeof(username) != 'string') { |
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.
Use ===
and !==
instead of ==
and !=
.
|
||
const isValidName = (name) => { | ||
|
||
return typeof(name) == 'string' && name.length >=1 |
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.
would fail " "
const authHeader = req.headers.authorization; | ||
|
||
if(authHeader && | ||
authHeader.split(' ')[0] == 'token' && |
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.
Generally we don't care about the first word. So you don't explicitly need to check. Depends on how you would like though.
authHeader.split(' ').length == 2) { | ||
const token = authHeader.split(' ')[1]; | ||
Token.findOne({token},'user',(err,tokenObject)=>{ | ||
if (tokenObject==null) { |
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.
prefer using ! in such cases. ex: !tokenObject
error: "Invalid Token" | ||
}); | ||
} | ||
User.findById(tokenObject.user,'_id name email username',(err,user)=>{ |
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.
Since you do not need the createdTodos and collabTodos which are fewer in number just use -createdTodos -collabTodods
instead.
error: "User with that token no longer exists" | ||
}); | ||
} | ||
req.user = { |
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.
I would prefer to have all the details of the user though.
return res.status(400).json({ | ||
error: "Invalid Id" | ||
}); |
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.
Be more descriptive with the message. Just assume that if it was in production, an API is called with the following validation and you get Invalid ID
. It would be unclear, which ID is being talked about.
validate.validateBody({ | ||
title: "string" | ||
}),ToDoController.createToDo); | ||
router.use('/:id/',validate.validateParams); | ||
router.get("/:id/", ToDoController.getParticularToDo); | ||
|
||
router.patch("/:id/", | ||
validate.validateBody({ | ||
title: "string" | ||
}),ToDoController.editToDoPatch); | ||
|
||
router.put("/:id/", | ||
validate.validateBody({ | ||
title: "string" | ||
}),ToDoController.editToDo); | ||
|
||
router.delete("/:id/",ToDoController.deleteToDo); | ||
|
||
router.patch("/:id/add-collaborator/", | ||
validate.validateBody({ | ||
collaborator: "string" | ||
}),ToDoController.addCollaborator); | ||
|
||
router.patch("/:id/remove-collaborator/", | ||
validate.validateBody({ | ||
collaborator: "string" | ||
}), |
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.
Destructure validateBody
also. Suppose if you change name of import from validate
to newName
, many places would require change.
if(!(body.email && body.password && body.name && body.username)) { | ||
return res.status(400).send({error: "Missing required fields"}); | ||
} |
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.
Why not validating using the middleware here?
Marks: |
No description provided.