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

MVP submission #49

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

MVP submission #49

wants to merge 27 commits into from

Conversation

chongct
Copy link

@chongct chongct commented Jan 10, 2018

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?:

@primaulia
Copy link
Contributor

Suggestions

  • Update your ERD
  • Should check if user has typed keyword at the homepage
  • I don't think this route: /result/5a5700fed0c9470014fd3bd1 need the id. Since only logged in user can see this route

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

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


// save results to database only when logged in
if (res.locals.currentUser) {
saveData.saveResultAll(displayArray, itemArray, displaySortedArray, res.locals.currentUser._id);
Copy link
Contributor

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.

// .exec((err, searchResult) => {
// if (err) console.log(err);

AnalyzedList.find({username: req.params.id})
Copy link
Contributor

@primaulia primaulia Jan 11, 2018

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

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

Choose a reason for hiding this comment

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

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.

None yet

2 participants