code review 1

server/app/routes/animals/index.js

@@ -17,6 +17,7 @@ var Animal = mongoose.model('Animal');
 
 // Current URL: '/api/animals'
 
+// GTND: good good
 router.param('id', function(req, res, next, id) {
    Animal.findById(id).then(function(animal){
        req.animal = animal;
@@ -25,6 +26,7 @@ router.param('id', function(req, res, next, id) {
 });
 
 router.get('/', function (req, res, next) {
+  // GTND: maybe say var query = req.query ? { animalName: new RegExp(req.query.name, "i")} : {}
 	if (req.query) {
 		Animal.find({ animalName: new RegExp(req.query.name, "i")}).then(function (animals) {
 			res.json(animals);
@@ -56,12 +58,14 @@ router.put('/:id', function (req, res, next) {
 	}).catch(next);
 });
 
+// GTND: POST '/animals/:id/reviews' (RESTful)
 router.post('/:id/addReview', function (req, res, next) {
 	var animal = req.animal;
+  // GTND: req.body shouldn't have an id...
 	animal.reviews.push(req.body._id);
 	animal.save()
 	.then(function (savedAnimal) {
 		req.animal = savedAnimal; // ==> may not be necessary
 		res.status(201).json(req.animal);
 	}).catch(next);
-});
+});

server/app/routes/cart/index.js

@@ -51,26 +51,32 @@ router.use(function (req, res, next) {
 
 // === Below here, all middlewares have a req.session.cart
 
+// GTND: '/me' or something, since '/' means all
 router.get('/', function (req, res) {
 	res.json(req.session.cart);
 });
 
+// GTND: also POST '/me/items' or something
 router.put('/', function (req, res, next) {
-	Cart.findById(req.session.cart._id)
+	Cart.findById(req.session.cart._id) // GTND: why?
 	.then(function (cart) {
 		return cart.addItem(req.body.animal, req.body.quantity);
 	})
 	.then (function (savedCart) {
+    // GTND: instead update req.session.cart here
 		res.status(201).json(savedCart);
 	}).catch(next);
 });
 
+// GTND: also '/me/items/:itemId' or something
+// GTND: don't assume req.body for DELETE
 router.delete('/', function (req, res, next) {
 	Cart.findById(req.session.cart._id)
 	.then(function (cart) {
+    // GTND: what if you want to remove every copy of it?
 		return cart.deleteOneItem(req.body.id);
 	})
 	.then(function (updatedCart) {
 		res.status(200).json(updatedCart);
 	}).catch(next);
-});
+});

server/db/models/animal.js

@@ -8,6 +8,7 @@ var schema = new mongoose.Schema({
     // code: {
     //     type: String
     // },
+    // GTND: you can just call this name
     animalName: {
         type: String,
         required: true
@@ -16,13 +17,16 @@ var schema = new mongoose.Schema({
         type: String
     },
     price: {
+        // GTND: required?
+        // GTND: in cents plz
         type: Number
     },
     description: {
         // Description of product
         type: String
     },
     category: {
+        // GTND: not enumed? just tags?
         type: [String]
     },
     countryCode: {
@@ -33,25 +37,33 @@ var schema = new mongoose.Schema({
         enum: ['Near Threatened', 'Vulnerable', 'Endangered', 'Critically Endangered', 'Extinct in the Wild', 'Extinct']
     },
     reviews: {
+        // GTND: ref the other way
         // Reference to reviews
         type: [mongoose.Schema.Types.ObjectId],
         ref: 'Review'
     },
     rating: {
+        // GTND: min/max?
+        // GTND: how is this updated? a hook?
         // average of stars from reviews, default to zero
         type: Number,
         default: 0
     }
 });
 
+// GTND: animal.getAllReviews
+// GTND: animal.calculateRating
 
+// GTND: cat? a little misleading
+// GTND: so categories is a string?
 schema.statics.findByCat = function (categories) {
     var catArr = categories.split(/[\s,]+/);
     return this.find({category: {$in: catArr}});
 };
 
 schema.methods.getSimilar = function () {
     var myCat = this.category;
+    // GTND: what's wrong with this.find?
     return this.constructor
     .find({
         _id: {$ne: this._id},
@@ -60,4 +72,4 @@ schema.methods.getSimilar = function () {
 };
 
 
-mongoose.model('Animal', schema);
+mongoose.model('Animal', schema);

server/db/models/cart.js

@@ -2,23 +2,31 @@
 
 var mongoose = require('mongoose');
 
+// GTND: will this be used for order history too?
+
 var CartSchema = new mongoose.Schema({
 	user: {
 		type: mongoose.Schema.Types.ObjectId,
 		ref: 'User'
 	},
-	animals: {
-		type: [mongoose.Schema.Types.ObjectId],
-		ref:'Animal'
-	}
+	animals: [{
+    quantity: Number,
+    price: Number,
+    animal:{
+  		type: [mongoose.Schema.Types.ObjectId],
+  		ref:'Animal'
+    }
+	}]
 });
 
+// GTND: maybe .pull? or _.pull? (ok don't do this then)
 CartSchema.methods.deleteOneItem = function(animalId) {
   var index = this.animals.indexOf(animalId);
   if (index > -1) this.animals.splice(index, 1);
   return this.save();
 };
 
+// GTND: probably better to have the object in the array have a quantity
 CartSchema.methods.addItem = function(animalId, quantity) {
   for (var i = 0; i < quantity; i++) {
 		this.animals.push(animalId);

server/db/models/user.js

@@ -3,8 +3,11 @@ var crypto = require('crypto');
 var mongoose = require('mongoose');
 
 var schema = new mongoose.Schema({
+    // GTND: how about a name?
     email: {
         type: String
+        // GTND: guarantee uniqueness?
+        // GTND: email validator mongoose plugin
     },
     password: {
         type: String
@@ -57,4 +60,4 @@ schema.method('correctPassword', function (candidatePassword) {
     return encryptPassword(candidatePassword, this.salt) === this.password;
 });
 
-mongoose.model('User', schema);
+mongoose.model('User', schema);

tests/server/models/animal-test.js

@@ -19,7 +19,7 @@ describe('Animal model', function () {
         if (mongoose.connection.db) return done();
         mongoose.connect(dbURI, done);
     });
-      
+
     afterEach('Clear test database', function (done) {
         clearDB(done);
     });
@@ -48,7 +48,7 @@ describe('Animal model', function () {
                     animal = createdAnimal;
                 });
         });
-        
+
         afterEach('destroy animal', function () {
             return Animal.remove({ animalName: 'TestReviewsAnimal' });
         });
@@ -64,6 +64,7 @@ describe('Animal model', function () {
                 dangerLevel: 3
             })
             .then(function (newReview) {
+                // GTND: no requests in model tests, you should just test methods/statics/hooks
                 agent
                 .post('/api/animals/' + animal._id + '/addReview')
                 .send(newReview)
@@ -80,9 +81,9 @@ describe('Animal model', function () {
         });
 
     });
-    
+
     describe('Methods', function() {
-        beforeEach('populate Database', function() {        
+        beforeEach('populate Database', function() {
             return Animal.create({
                     animalName: "lemur",
                     category: ["mammal", "madagascar"]

tests/server/models/review-test.js

@@ -33,27 +33,28 @@ describe('Review model', function () {
                 expect(error.message).to.equal('Review validation failed');
             });
         });
-      
+
         it('should require stars', function() {
             return Review.create({ content: 'I forgot to add stars!' }).then(null, function(error) {
                 expect(error).to.exist;
                 expect(error.message).to.equal('Review validation failed');
             });
         });
-
+
+        // GTND: you don't really need to test mongoose's validations
         describe('stars field should not allow', function() {
             var myReview = {
                 content: 'I\'m going to put the wrong number of stars!'
             };
-            
+
             it('more than 5 stars', function() {
                 myReview.stars = 600;
                 return Review.create(myReview).then(null, function(error) {
                     expect(error).to.exist;
                     expect(error.message).to.equal('Review validation failed');
                 });
             });
-            
+
             it('fewer than 0 stars', function() {
                 myReview.stars = -3;
                 return Review.create(myReview).then(null, function(error) {

tests/server/routes/animal-api-test.js

@@ -13,8 +13,10 @@ var clearDB = require('mocha-mongoose')(dbURI);
 var supertest = require('supertest');
 var app = require('../../../server/app');
 
+
+// GTND: cool, good tests here
 describe('Animal Route', function () {
-  
+
   var agent,
       baseUrl = '/api/animals/',
       animalInfo = {
@@ -46,8 +48,8 @@ describe('Animal Route', function () {
       clearDB(done);
   });
 
-  describe('Get all animals', function () { 
-    
+  describe('Get all animals', function () {
+
       it('should get with 200 response with an array as the body', function (done) {
           agent.get(baseUrl).expect(200).end(function (err, response) {
               if (err) return done(err);
@@ -58,7 +60,7 @@ describe('Animal Route', function () {
       });
 
   });
-  
+
   describe('Create an animal', function() {
       it('should get a 201 response with an animal as the body', function (done) {
           agent
@@ -73,7 +75,7 @@ describe('Animal Route', function () {
           });
       });
   });
-  
+
   describe('Get animal by id', function() {
       it('should get a 200 response with an animal as the body', function(done) {
         agent
@@ -87,7 +89,7 @@ describe('Animal Route', function () {
         });
       });
   });
-  
+
   describe('Update an animal', function() {
     it('should get a 200 response with an animal as the body', function(done) {
       animalInfo.rating = 0.4;
@@ -104,14 +106,14 @@ describe('Animal Route', function () {
           done();
       })
     })
-  
+
 //    describe('Add a review to animal', function() {
 //      it('should get a 200 response with an a body')
 //    })
-    
-    
+
+
   })
 
-    
-    
+
+
 });