Grading PR - Feedback and Comments

README.md

@@ -68,6 +68,7 @@ Color {
 ### Connection and Server
 
 ### Dependencies
+You don't want to duplicate this from the package.json, doesn't really add much
 
 - Express
 - MongoDB/Mongoose

lib/app.js

@@ -18,8 +18,9 @@ app.use('/api/color', color);
 app.use('/api/block', block);
 
 app.use('/api/auth', auth);
-app.use('/api/user', ensureAuth, user);
 
+// shouldn't one of these by "moods"?
+app.use('/api/user', ensureAuth, user);
 app.use('/api/users', users);
 
 

lib/models/color.js

@@ -1,5 +1,4 @@
 const mongoose = require('mongoose');
-//const validator = require('node-mongoose-validator');
 const Schema = mongoose.Schema;
 
 var colorSchema = new Schema({
@@ -9,12 +8,10 @@ var colorSchema = new Schema({
         hexColor: {
             type: String,
             required: true
-            //validate: validator.$notEmpty({ msg: 'Please provide a name.' })
         },
         mood: {
             type: String,
-            required: true,
-            //validate: validator.$notEmpty({ msg: 'Please provide a mood' })
+            required: true
         }
     }
 });

lib/models/user.js

@@ -7,7 +7,6 @@ const userSchema = new Schema({
     username: {
         type: String,
         required: true
-        //validate: validator.$isAlpha({msg: 'Please create a username with letters'})
     },
     email: {
         type: String,

lib/routes/blocks.js

@@ -4,8 +4,8 @@ const bodyParser = require('body-parser').json();
 
 router
     .get('/', (req, res, next) => {
-        console.log('this is block ', req.body);
         Block.find()
+            .lean()
             .then(block => res.send(block))
             .catch(next);
     })    

lib/routes/colors.js

@@ -5,6 +5,7 @@ const bodyParser = require('body-parser').json();
 router
     .get('/', (req, res, next) => {
         Color.find()
+            .lean()
             .then(color => res.send(color))
             .catch(next);
     })    

lib/routes/user.js

@@ -1,4 +1,3 @@
-const mongoose = require('mongoose');
 const router = require('express').Router();
 const bodyParser = require('body-parser').json();
 const User = require('../models/user');
@@ -19,43 +18,65 @@ router
             })
             .catch(next);
     })
+    // you should combine modified queries as same get route:
     .get('/moods', (req, res, next) => {
-        const nId = (req.user.id);
-        const query = { userId: nId };
+        const query = { userId: req.user.id };
         const date = req.query.date;
         if (date) query.date = date;
+        else {
+            const month = req.query.month;
+            if (month) {
+                const findDate = new Date(month);
+                const year = findDate.getFullYear();
+                const month = findDate.getMonth();
+                query.date = { 
+                    $gte: new Date(year, month, 1), 
+                    $lte: new Date(year, month + 1, 0) 
+                };
+            }
+        }
+
         Mood.find(query)
             .populate('color block')
+            .lean()
             .then(moods => res.send(moods))
             .catch(next);
     })
+    // This should have been query param on above route, not a separate route
     .get('/moods/month', (req, res, next) => {
         const nId = (req.user.id);
         const findDate = new Date(req.query.month);
 
-        let startDate = new Date(findDate.getFullYear(), findDate.getMonth(), 1);
-        let endDate = new Date(findDate.getFullYear(), findDate.getMonth() + 1, 0);
+        // good work here!
+        // except:
+        // 1) don't repeat yourself:
+        const year = findDate.getFullYear();
+        const month = findDate.getMonth();
+        // 2) don't use let when it's a const
+        const startDate = new Date(year, month, 1);
+        const endDate = new Date(year, month + 1, 0);
         const query = { userId: nId, date: { $gte: startDate, $lte: endDate } };
 
         Mood.find(query)
             .populate('color block')
+            .lean()
             .then(moods => res.send(moods))
             .catch(next);
     })
-    .post('/moods/add', ensureAuth, bodyParser, weatherApi, (req, res, next) => {
-        let newEntry = {
-            userId: req.user.id,
-            date: req.body.date,
-            color: req.body.colorId,
-            comment: req.body.comment,
-            block: req.body.blockId,
-            weather: req.body.weather,
-            zipcode: req.body.zipcode
-        };
+    // a post to a resource is an add, DON'T put that fact in the route name
+    .post('/moods', ensureAuth, bodyParser, weatherApi, (req, res, next) => {
+        // mongoose will already prevent undefined fields from being saved.
+        // so just pass the body to the model after adding the user id
+        const mood = req.body;
+        mood.userId = req.userId;
 
-        let mood = new Mood(newEntry);
-        mood.save();
-        res.status(201).send(mood);
+        // mood.save() is async, wait till it's done to respond:
+        new Mood(mood)
+            .save()
+            .then(mood => {
+                res.status(201).send(mood);
+            })
+            .catch(next);
     });
 
 module.exports = router;

lib/routes/users.js

@@ -2,66 +2,21 @@ const router = require('express').Router();
 const Mood = require('../models/mood');
 
 router
+    // These should all be same .get route:
     .get('/moods', (req, res, next) => {
-        Mood.find()
-            .populate('block color')
+        const query = req.query;
+        let find = query.type ? Mood.distinct(query.type) : Mood.find();
+        find = find.populate('block color');
+        if(query.date) find = find.where('date', query.date);
+        if(query.zipcode) find = find.where('zipcode', query.zipcode);
+        if(query.description) find = find.where('weather.description', query.description);
+        if(query.city) find = find.where('weather.city', query.city);
+        if(query.state) find = find.where('weather.state', query.state);
+        if(query.country) find = find.where('weather.country', query.country);
+
+        find
             .then(moods => res.send(moods))
             .catch(next);
-    })
-    .get('/moods/date', (req, res, next) => {
-        const date = req.query.date;
-        Mood.find()
-            .where('date', date)
-            .populate('block color')
-            .then(moods => res.send(moods))
-            .catch(next);
-    })
-    .get('/moods/zipcode', (req, res, next) => {
-        const zipcode = req.query.zipcode;
-        Mood.find()
-            .where('zipcode', zipcode)
-            .populate('block color')
-            .then(moods => res.send(moods))
-            .catch(next);               
-    })
-    .get('/moods/description', (req, res, next) => {
-        const description = req.query.description;
-        console.log('description', description);
-        Mood.find()
-            .where('weather.description', description)
-            .populate('block color')            
-            .then(moods => res.send(moods))
-            .catch(next);
-    })
-    .get('/moods/city', (req, res, next) => {
-        const city = req.query.city;
-        Mood.find()
-            .where('weather.city', city)
-            .populate('block color')            
-            .then(moods => res.send(moods))
-            .catch(next);               
-    })
-    .get('/moods/state', (req, res, next) => {
-        const state = req.query.state;
-        Mood.find()
-            .where('weather.state', state)
-            .populate('block color')            
-            .then(moods => res.send(moods))
-            .catch(next);               
-    })
-    .get('/moods/country', (req, res, next) => {
-        const country = req.query.country;
-        Mood.find()
-            .where('weather.country', country)
-            .populate('block color')            
-            .then(moods => res.send(moods))
-            .catch(next);               
-    })
-    .get('/moods/types', (req, res, next) => {
-        const type = req.query.type;
-        Mood.distinct(type)
-        .then(moods => res.send(moods))
-        .catch(next);
     });
 
 module.exports = router;

lib/routes/weather-api.js

@@ -1,22 +1,19 @@
 const apiKey = process.env.WEATHER_API_KEY;
-console.log('apikey',apiKey)
 const requestProxy = require('request-promise');
 //when it's done:
 //save the weather data on req.weather
 module.exports = function weatherApi(req, res, next) {
     requestProxy(`http://api.wunderground.com/api/${apiKey}/conditions/q/${req.body.zipcode}.json`)
         .then(
             result => {
-                result = JSON.parse(result)
+                result = JSON.parse(result);
                 req.body.weather = {
                     temp: result.current_observation.temperature_string,
                     description: result.current_observation.weather,
                     city: result.current_observation.display_location.city,
                     state: result.current_observation.display_location.state,
                     country: result.current_observation.display_location.country
                 };
-                console.log(req.body.weather);
-
                 next();
             }
         )

test/mood-api.test.js

@@ -1,5 +1,4 @@
 const mongoose = require('mongoose');
-const Schema = mongoose.Schema;
 const assert = require('chai').assert;
 const User = require('../lib/models/user');
 const Mood = require('../lib/models/mood');
@@ -14,6 +13,9 @@ let moodOne;
 
 describe('saves new user, color, and block number so we can test and populate the mood schema', () => {
 
+    // tests with no "assert" are not really tests :(
+
+
     it('tests the mood schema with each field populated', () => {
         //saving new user
         const userOne = new User({ 

test/user-schema.test.js

@@ -5,16 +5,16 @@ const assert = require('chai').assert;
 const User = require('../lib/models/user');
 
 describe('testing the user schema', () => {
-        const testOne = new User({ 
-            username: 'colordiary',
-            email: 'not correct email format',
-            password: 'password',
-        })
-        const testTwo = new User({ 
-            username: 'testTwo',
-            email: 'colordiary@gmail.com',
-            password: 'password',
-        })
+    const testOne = new User({ 
+        username: 'colordiary',
+        email: 'not correct email format',
+        password: 'password',
+    })
+    const testTwo = new User({ 
+        username: 'testTwo',
+        email: 'colordiary@gmail.com',
+        password: 'password',
+    })
 
     it('validates email and username', () => {
 

test/weather-api.test.js

@@ -52,16 +52,13 @@ describe('posts a new entry with weather added to it', () => {
     });
 
     it('', () => {
-       return request('/api/user/add')
-        .set('Authorization', process.env.TOKEN)
-        .send(newEntry)
-        .then(res => {
-            res = res.body;
-            console.log(res.body)
-        })
-        .then(res => {
-            res.body = newEntry;
-        })
+        return request('/api/user/add')
+            .set('Authorization', process.env.TOKEN)
+            .send(newEntry)
+            .then(res => {
+                res = res.body;
+                console.log(res.body)
+            });
     })
 })