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

Dev Raj - Completed all tasks #7

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

Conversation

dev-raj-1729
Copy link

No description provided.

@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.

LGTM 🎉.
Dropped some reviews.
Also, look at commit conventions to improve further.

Comment on lines -16 to +36

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));
Copy link
Collaborator

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') {
Copy link
Collaborator

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
Copy link
Collaborator

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' &&
Copy link
Collaborator

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) {
Copy link
Collaborator

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)=>{
Copy link
Collaborator

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 = {
Copy link
Collaborator

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.

Comment on lines +29 to +31
return res.status(400).json({
error: "Invalid Id"
});
Copy link
Collaborator

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.

Comment on lines +8 to +34
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"
}),
Copy link
Collaborator

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.

Comment on lines +49 to +51
if(!(body.email && body.password && body.name && body.username)) {
return res.status(400).send({error: "Missing required fields"});
}
Copy link
Collaborator

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?

@K-Kumar-01
Copy link
Collaborator

Marks:
task-1: 95
Task-2: 99

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