-
Notifications
You must be signed in to change notification settings - Fork 60
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
MVP submission #49
base: master
Are you sure you want to change the base?
MVP submission #49
Conversation
Suggestions
|
routes/routes.js
Outdated
router.get('/profile/:id', loginBlock.isLoggedIn, userController.profile); | ||
router.post('/profile/:id', loginBlock.isLoggedIn, userController.change); | ||
router.get('/result/:id', loginBlock.isLoggedIn, userController.result); | ||
router.delete('/delete/search/:id', userController.deleteSearch); |
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.
you should check DELETE
and PUT
request too whether the user has been authenticated or not, not only the GET
and POST
request
controllers/homeController.js
Outdated
|
||
// save results to database only when logged in | ||
if (res.locals.currentUser) { | ||
saveData.saveResultAll(displayArray, itemArray, displaySortedArray, res.locals.currentUser._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.
I still think that this step is unnecessary. You should only save results that the user wants to save.
controllers/userController.js
Outdated
// .exec((err, searchResult) => { | ||
// if (err) console.log(err); | ||
|
||
AnalyzedList.find({username: req.params.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.
You can execute these two find at the same time too with asyncp
, just like how you did it with the yelp api call
if (errors) { | ||
res.render('profile', {'errors': errors}); | ||
} else { | ||
User.findOne({_id: req.params.id}, (err, result) => { |
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.
This steps should be unnecessary if user has logged in, you can just use req.user
if (data) { | ||
// add to database if entry is already in database | ||
|
||
// check to see if combination is already present |
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.
you can skip this step by putting the logic check in SavedList
model instead. https://stackoverflow.com/questions/16882938/how-to-check-if-that-data-already-exist-in-the-database-during-update-mongoose
Deliverable Submission
Please describe your comfort and completeness levels before submitting.
Comfort Level (1-5): 3.5
Completeness Level (1-5): 3.5
What did you think of this deliverable?: