From 4b026996ae5f1f23c6696c02b842537aded6f6eb Mon Sep 17 00:00:00 2001 From: Griffin Telljohann Date: Wed, 4 Nov 2015 18:01:06 -0500 Subject: [PATCH 1/2] code review 1 --- server/app/routes/animals/index.js | 6 +++++- server/app/routes/cart/index.js | 10 ++++++++-- server/db/models/animal.js | 13 +++++++++++-- server/db/models/cart.js | 4 ++++ server/db/models/user.js | 3 ++- tests/server/models/animal-test.js | 9 +++++---- tests/server/models/review-test.js | 9 +++++---- tests/server/routes/animal-api-test.js | 24 +++++++++++++----------- 8 files changed, 53 insertions(+), 25 deletions(-) diff --git a/server/app/routes/animals/index.js b/server/app/routes/animals/index.js index 414ff0c..a31e7f0 100644 --- a/server/app/routes/animals/index.js +++ b/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); -}); \ No newline at end of file +}); diff --git a/server/app/routes/cart/index.js b/server/app/routes/cart/index.js index 368c53d..4945cd4 100644 --- a/server/app/routes/cart/index.js +++ b/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 '/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' 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); -}); \ No newline at end of file +}); diff --git a/server/db/models/animal.js b/server/db/models/animal.js index eca97aa..a3ac5e2 100644 --- a/server/db/models/animal.js +++ b/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,6 +17,8 @@ var schema = new mongoose.Schema({ type: String }, price: { + // GTND: required? + // GTND: in cents plz type: Number }, description: { @@ -23,6 +26,7 @@ var schema = new mongoose.Schema({ type: String }, category: { + // GTND: not enumed? just tags? type: [String] }, countryCode: { @@ -33,18 +37,22 @@ 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: 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}}); @@ -52,6 +60,7 @@ schema.statics.findByCat = function (categories) { 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 +69,4 @@ schema.methods.getSimilar = function () { }; -mongoose.model('Animal', schema); \ No newline at end of file +mongoose.model('Animal', schema); diff --git a/server/db/models/cart.js b/server/db/models/cart.js index 13ec608..226b1a5 100644 --- a/server/db/models/cart.js +++ b/server/db/models/cart.js @@ -2,6 +2,8 @@ var mongoose = require('mongoose'); +// GTND: will this be used for order history too? + var CartSchema = new mongoose.Schema({ user: { type: mongoose.Schema.Types.ObjectId, @@ -13,12 +15,14 @@ var CartSchema = new mongoose.Schema({ } }); +// GTND: maybe .pull? or _.pull? 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); diff --git a/server/db/models/user.js b/server/db/models/user.js index 4012f89..e073ce9 100644 --- a/server/db/models/user.js +++ b/server/db/models/user.js @@ -3,6 +3,7 @@ var crypto = require('crypto'); var mongoose = require('mongoose'); var schema = new mongoose.Schema({ + // GTND: how about a name? email: { type: String }, @@ -57,4 +58,4 @@ schema.method('correctPassword', function (candidatePassword) { return encryptPassword(candidatePassword, this.salt) === this.password; }); -mongoose.model('User', schema); \ No newline at end of file +mongoose.model('User', schema); diff --git a/tests/server/models/animal-test.js b/tests/server/models/animal-test.js index 0d247b6..276bcff 100644 --- a/tests/server/models/animal-test.js +++ b/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"] diff --git a/tests/server/models/review-test.js b/tests/server/models/review-test.js index d6a5fa0..8392564 100644 --- a/tests/server/models/review-test.js +++ b/tests/server/models/review-test.js @@ -33,19 +33,20 @@ 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) { @@ -53,7 +54,7 @@ describe('Review model', function () { 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) { diff --git a/tests/server/routes/animal-api-test.js b/tests/server/routes/animal-api-test.js index bcc1e82..0db85e7 100644 --- a/tests/server/routes/animal-api-test.js +++ b/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') // }) - - + + }) - - + + }); From 01e6f4ae7433a4fd96b7dbe7708b4a443a3ffa3b Mon Sep 17 00:00:00 2001 From: Griffin Telljohann Date: Wed, 4 Nov 2015 18:24:33 -0500 Subject: [PATCH 2/2] more comments --- server/app/routes/cart/index.js | 4 ++-- server/db/models/animal.js | 3 +++ server/db/models/cart.js | 14 +++++++++----- server/db/models/user.js | 2 ++ 4 files changed, 16 insertions(+), 7 deletions(-) diff --git a/server/app/routes/cart/index.js b/server/app/routes/cart/index.js index 4945cd4..5f6a997 100644 --- a/server/app/routes/cart/index.js +++ b/server/app/routes/cart/index.js @@ -56,7 +56,7 @@ router.get('/', function (req, res) { res.json(req.session.cart); }); -// GTND: also '/me/items' or something +// GTND: also POST '/me/items' or something router.put('/', function (req, res, next) { Cart.findById(req.session.cart._id) // GTND: why? .then(function (cart) { @@ -68,7 +68,7 @@ router.put('/', function (req, res, next) { }).catch(next); }); -// GTND: also '/me/items' or something +// 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) diff --git a/server/db/models/animal.js b/server/db/models/animal.js index a3ac5e2..4beae01 100644 --- a/server/db/models/animal.js +++ b/server/db/models/animal.js @@ -51,6 +51,9 @@ var schema = new mongoose.Schema({ } }); +// GTND: animal.getAllReviews +// GTND: animal.calculateRating + // GTND: cat? a little misleading // GTND: so categories is a string? schema.statics.findByCat = function (categories) { diff --git a/server/db/models/cart.js b/server/db/models/cart.js index 226b1a5..0b722c2 100644 --- a/server/db/models/cart.js +++ b/server/db/models/cart.js @@ -9,13 +9,17 @@ var CartSchema = new mongoose.Schema({ 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? +// 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); diff --git a/server/db/models/user.js b/server/db/models/user.js index e073ce9..5518e2c 100644 --- a/server/db/models/user.js +++ b/server/db/models/user.js @@ -6,6 +6,8 @@ var schema = new mongoose.Schema({ // GTND: how about a name? email: { type: String + // GTND: guarantee uniqueness? + // GTND: email validator mongoose plugin }, password: { type: String