Skip to content

Solutions to Assignment#18

Open
Somanath1786 wants to merge 1 commit intopce-uw-jscript400:masterfrom
Somanath1786:master
Open

Solutions to Assignment#18
Somanath1786 wants to merge 1 commit intopce-uw-jscript400:masterfrom
Somanath1786:master

Conversation

@Somanath1786
Copy link

No description provided.

Copy link
Collaborator

@bwreid bwreid left a comment

Choose a reason for hiding this comment

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

Added some minor comments. Overall, everything looks great!

var user = await User.findOne({username});
if (user)
{
status = 400;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since these are all the same, you could probably just define this at the top!

}

const isValid = await bcrypt.compare(password, guest.password)
if (!isValid) throw new Error('Password is invalid');
Copy link
Collaborator

Choose a reason for hiding this comment

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

You'll want to use the same error message for both of the above errors. Otherwise, it's easier for someone to "guess" another user's username and is more insecure.

error.status = 404
return next(error)
}
if(!(user.admin))
Copy link
Collaborator

Choose a reason for hiding this comment

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

You shouldn't need the ()s here. Just !user.admin is fine.

// You should only be able to reserve a book if a user is logged in
router.patch('/:id/reserve', async (req, res, next) => {
const { id } = req.params
var status;
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could use let instead!


/// If able to return successfully, set reserved status to false and clear the memberId
book.reserved.status = false
book.reserved.memberId = ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

null might be a better value here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants