Skip to content

MVP submission#49

Open
chongct wants to merge 27 commits into
wdi-sg:masterfrom
chongct:master
Open

MVP submission#49
chongct wants to merge 27 commits into
wdi-sg:masterfrom
chongct:master

Conversation

@chongct

@chongct chongct commented Jan 10, 2018

Copy link
Copy Markdown

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
Copy Markdown
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

Comment thread routes/routes.js
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
Copy Markdown
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

Comment thread 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);

Copy link
Copy Markdown
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.

Comment thread controllers/userController.js Outdated
// .exec((err, searchResult) => {
// if (err) console.log(err);

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

@primaulia primaulia Jan 11, 2018

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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.

2 participants