From 2edb37908f1a95a0f1fc9ad8f7c663aa3f16832a Mon Sep 17 00:00:00 2001 From: martypdx Date: Wed, 8 Mar 2017 22:18:24 -0800 Subject: [PATCH 1/2] initial commit --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index 53d86db..2d8e99e 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,8 @@ # dbSimsPDX A backend game of life. +**Initial Grading Commit** + # [Visit the game on Heroku](https://dbsimspdx.herokuapp.com/) The goal of the game is to retire as young as possible with a networth of $1,000,000! From c80f253a46ab3a5eea5faa17217ddfcd24161fc1 Mon Sep 17 00:00:00 2001 From: martypdx Date: Thu, 9 Mar 2017 12:01:45 -0800 Subject: [PATCH 2/2] comments, feedback and suggestions --- README.md | 2 +- lib/models/admin.model.js | 0 lib/routes/assets.routes.js | 1 + lib/routes/jobs.routes.js | 6 +- lib/routes/user.routes.js | 241 ++++++++++++++++++++++-------------- lib/update-user.js | 105 ++++++++-------- server.js | 9 +- test/api/admin.api.test.js | 123 +++++++++--------- test/api/assets.api.test.js | 2 +- test/api/user.api.test.js | 5 +- test/user.model.test.js | 2 + 11 files changed, 273 insertions(+), 223 deletions(-) delete mode 100644 lib/models/admin.model.js diff --git a/README.md b/README.md index 2d8e99e..6a0150a 100644 --- a/README.md +++ b/README.md @@ -1,7 +1,7 @@ # dbSimsPDX A backend game of life. -**Initial Grading Commit** +**Nice Job on README** # [Visit the game on Heroku](https://dbsimspdx.herokuapp.com/) diff --git a/lib/models/admin.model.js b/lib/models/admin.model.js deleted file mode 100644 index e69de29..0000000 diff --git a/lib/routes/assets.routes.js b/lib/routes/assets.routes.js index ad37cbe..097d602 100644 --- a/lib/routes/assets.routes.js +++ b/lib/routes/assets.routes.js @@ -15,6 +15,7 @@ assetRouter }) /* __NOT__ a feature for a user */ + // so how do you prevent them from accessing???? .post('/', bodyParser, (req, res, next) => { new Asset (req.body).save() .then(asset => { diff --git a/lib/routes/jobs.routes.js b/lib/routes/jobs.routes.js index 00d0766..65d3645 100644 --- a/lib/routes/jobs.routes.js +++ b/lib/routes/jobs.routes.js @@ -8,14 +8,14 @@ const jobRouter = Router(); jobRouter .get('/', (req, res, next) => { Job.find() + .lean() + // .select(only needed properties) .then(jobs => res.send(jobs)) .catch(next); }) .post('/', bodyParser, (req, res, next) => { return new Job(req.body).save() - .then(job => { - res.send(job); - }) + .then(job => res.send(job)) .catch(next); }); diff --git a/lib/routes/user.routes.js b/lib/routes/user.routes.js index db10246..af86189 100644 --- a/lib/routes/user.routes.js +++ b/lib/routes/user.routes.js @@ -1,7 +1,6 @@ const express = require('express'); const Router = express.Router; const userRouter = Router(); -const mongoose = require('mongoose'); const User = require('../models/user.model'); const Asset = require('../models/asset.model'); const Job = require('../models/job.model'); @@ -10,7 +9,6 @@ const Education = require('../models/education.model'); const token = require('../auth/token.js'); const bodyParser = require('body-parser').json(); const ensureAuth = require('../auth/ensure-auth')(); -const ensureRole = require('../auth/ensure-roles')(); function hasUserNameAndPassword(req, res, next) { const user = req.body; @@ -24,28 +22,29 @@ function hasUserNameAndPassword(req, res, next) { } function getJob(query) { - let entryJob = {}; - return Job.find( { jobLevel: 'Entry' } ) - .then(jobs => { - //TODO: investigate using aggregate $sample - if(!jobs.length) return; - let job = jobs.filter(j => { - return j.jobType === query; - }); - entryJob.start_date = new Date(); - entryJob.months_worked = 0; - entryJob.job_name = job[0]._id; - return entryJob; - }); + // it's really this easy to have mongo do the work: + return Job.aggregate([ + { $match: { jobLevel: 'Entry', jobType: query } }, + { $sample: { size: 1 } } + ]) + .then(jobs => { + if(!jobs.length) return; + return { + start_date: new Date(), + months_worked: 0, + job_name: jobs[0]._id, + + }; + }); } function getFirstEducation() { - let firstEd = {}; + // don't define variables in broader scope than needed! + // let firstEd = {}; return Education.findOne( { educationLevel: 'High School' } ) .then(school => { if(!school) return; - firstEd = school._id; - return firstEd; + return school._id; }); } @@ -54,6 +53,10 @@ userRouter res.send({ valid: true }); }) +/* don't put comment bands like this in your code. :( + Find a way to beter modularize your code +*/ + //////////////////////////////USER///////////////////////////////// .post('/signup', bodyParser, hasUserNameAndPassword, (req, res, next) => { @@ -65,23 +68,23 @@ userRouter error: `username ${data.username} already exists` }; else { - data.retired = false; - data.age = 18; - data.bank_account = Math.floor(Math.random()*200000) + 1; - data.networth = data.bank_account; - data.assets = []; - data.activities = []; - data.original_signup = new Date(); - data.last_sign_in = data.original_signup; - data.roles = 'user'; - return getJob('Unskilled'); + return Promise.all([ + getJob('Unskilled'), + getFirstEducation(), + ]); } }) - .then(job => { + .then(([job, education]) => { + data.retired = false; + data.age = 18; + data.bank_account = Math.floor(Math.random()*200000) + 1; + data.networth = data.bank_account; + data.assets = []; + data.activities = []; + data.original_signup = new Date(); + data.last_sign_in = data.original_signup; + data.roles = 'user'; data.job = job; - return getFirstEducation(); - }) - .then(education => { data.education = education; return new User(data).save(); }) @@ -115,80 +118,133 @@ userRouter let data = req.body; let desiredAsset = {}; let savedAsset = {}; - const username = req.params.username; - Asset.findById(data._id) - .then(asset => { - desiredAsset = asset; - }); - return User.findOne({username: username}) - .then(user => { - const cash = user.bank_account; - if(desiredAsset.purchase_price > cash) { - res.status(400).send({error: 'You do not have funds for this purchase'}); - } else { - - user.bank_account = user.bank_account - desiredAsset.purchase_price; - savedAsset.purchase_date = new Date(); - savedAsset.asset_name = desiredAsset._id; - user.assets.push(savedAsset); - } - return user.save(); - }) - .then(user => res.send(user)) - .catch(next); + + // security hole: you are allowing any user to change another user's + // data. If this is intended to be the same as logged in user, + // you need to use req.user.id + + // Also, these are parallel, not sequention actions: + + Promise.all([ + Asset.findById(data._id), + User.findById(req.user.id) + ]) + .then(([desiredAsset, user]) => { + + const cash = user.bank_account; + if(desiredAsset.purchase_price > cash) { + // use your error handler via next: + throw { code: 400, error: 'You do not have funds for this purchase'}; + } + // throw exists, so no need to else here + user.bank_account = user.bank_account - desiredAsset.purchase_price; + savedAsset.purchase_date = new Date(); + savedAsset.asset_name = desiredAsset._id; + user.assets.push(savedAsset); + return user.save(); + }) + .then(user => res.send(user)) + .catch(next); }) .post('/:username/activities', bodyParser, (req, res, next) => { - function getActivityOutcome(user, activity) { + // The code in this route is a mess, I've cleaned it up as + // example + + // original function was mixing asynchronous and synchronous behavior + // (has user.save(), but returns a value) + + // this could go at top of module... + const REWARD_MESSAGE = 'YOU WON THE LOTTERY!!!! Congratulations!!! You have achieved or surpassed a total networth of $1M dollars. You have won the dbSimsPDX game and are able to retire at the young age of ' + `${user.age}` + ' years old ... !!!'; + const NO_REWARD_MESSAGE = 'You got what you paid for! No reward today. Womp.'; + + function isReword(odds, networth) { let randomNum = Math.floor(Math.random()*100 + 1); - if (randomNum <= activity.rewardOdds) { - user.activities.wonReward = true; + return randomNum <= odds && networth > 1000000; + } + + // this functionality should be moved onto user model: + // user.addActivity(activity) + function getActivityOutcome(user, activity) { + const wonReward = isReword(activity.rewardOdds, user.networth); + const newActivity = { activity_name: activity._id }; + user.activities.wonReward = wonReward; + user.activities.push(newActivity); + activity.rewardMessage = NO_REWARD_MESSAGE; + + if (wonReward) { user.bank_account = user.bank_account + activity.rewardAmount; user.networth = user.networth + activity.rewardAmount; - user.activities.push(savedActivity); if(user.networth > 1000000) { - activity.rewardMessage = 'YOU WON THE LOTTERY!!!! Congratulations!!! You have achieved or surpassed a total networth of $1M dollars. You have won the dbSimsPDX game and are able to retire at the young age of ' + `${user.age}` + ' years old ... !!!'; + activity.rewardMessage = REWARD_MESSAGE; } - user.save(); - return activity.rewardMessage; - } else { - user.activities.wonReward = false; - user.activities.push(savedActivity); - user.save(); - return 'You got what you paid for! No reward today. Womp.'; } + + return user + .save() + .then(() => ({ message: activity.rewardMessage })); } - let reqActivityId = req.body; - let desiredActivity = {}; - let savedActivity = {}; - const username = req.params.username; + + Promise.all([ + Activity.findById(req.body), + User.findById(req.user.id) + ]) + .then(([desiredActivity, user]) => { + const cash = user.bank_account; + + if(desiredActivity.purchasePrice > cash) { + throw { code: 400, error: 'You do have funds for this purchase' }; + } + + user.bank_account -= desiredActivity.purchasePrice; + user.networth -= desiredActivity.purchasePrice; + return getActivityOutcome(user, desiredActivity); + }) + .then(message => res.send(message)) + .catch(next); + + // request body is a string? use an object instead: { id: } + // let reqActivityId = req.body; + // let desiredActivity = {}; + // let savedActivity = {}; + // const username = req.params.username; + // You've cut and pasted this comment WITHOUT + // understanding what it means :( + // (it doesn't apply here) //TODO: investigate using aggregate $sample - Activity.findById(reqActivityId) - .then(activity => { - if(!activity) return; - else {desiredActivity = activity;} - }); - return User.findOne( { username: username } ) - .then(user => { - const cash = user.bank_account; - - if(desiredActivity.purchasePrice > cash) { - res.status(400).send({error: 'You do have funds for this purchase'}); - } else { - user.bank_account = user.bank_account - desiredActivity.purchasePrice; - user.networth = user.networth - desiredActivity.purchasePrice; - savedActivity.activity_name = desiredActivity._id; + + // Activity.findById(reqActivityId) + // this then block does nothing + // .then(activity => { + // if(!activity) return; + // else {desiredActivity = activity;} + // }); + + // There is no connection from previous promise and this one. + // Are are relying on unguaranteed order of execution + // return User.findOne( { username: username } ) + // .then(user => { + // const cash = user.bank_account; + + // if(desiredActivity.purchasePrice > cash) { + // res.status(400).send({error: 'You do have funds for this purchase'}); + // } else { + // user.bank_account = user.bank_account - desiredActivity.purchasePrice; + // user.networth = user.networth - desiredActivity.purchasePrice; + // savedActivity.activity_name = desiredActivity._id; - return getActivityOutcome(user, desiredActivity); - } - }) - .then(message => { - res.send(message); - }) - .catch(next); + // return getActivityOutcome(user, desiredActivity); + // } + // }) + // .then(message => { + // res.send(message); + // }) + // .catch(next); + + }) .post('/:username/education', bodyParser, (req, res, next) => { @@ -196,7 +252,8 @@ userRouter const username = req.params.username; - + // WAY TOO NESTED! + // return promises and handle in next "this" Education.findById(reqEdId) .then(desiredEd => { if(!desiredEd) return; diff --git a/lib/update-user.js b/lib/update-user.js index 42c69c0..a7cd7b1 100644 --- a/lib/update-user.js +++ b/lib/update-user.js @@ -2,61 +2,58 @@ const User = require('./models/user.model'); const Job = require('./models/job.model'); module.exports = function update() { + +// WAY TO MUCH nesting here! User.find() - .then(users => { - users.forEach(function(user) { - return User.findById(user._id) - .then(user => { - user.age += .084; - user.job.months_worked += 1; - user.save(); - return user; + .then(users => users.forEach(user => { + updateUser(user) + .catch(err => { + console.log('Unable to update user', user, err) + }); + })) + // at least a console log if things fail??? + .catch(); +}; + +function updateUser(user) { + // no need to refetch, we just retrieved the user + // User.findById(user._id) + // .then(user => { + user.age += .084; + user.job.months_worked += 1; + // what? save then return without even waiting for the save to complete??? + // don't save in chunks like this, just save once... + + // user.save(); + // return user; + // }) + // .then(user => { + return Job.findById(user.job.job_name) + .then(job => { + user.bank_account += job.monthlySalary; + user.networth += job.monthlySalary; + if (user.networth >= 1000000) { + user.retired = true; + return user.save(); + } + if (job.promotionInterval === 0 || job.promotionInterval > user.job.months_worked) { + return user.save(); + } + else { + const level = job.jobLevel === 'Entry' ? 'Mid-level' : 'Senior'; + + return Job.findOne({ + jobType: job.jobType, + jobLevel: level }) - .then(user => { - let userJob = user.job; - return Job.findById(userJob.job_name) - .then(job => { - user.bank_account += job.monthlySalary; - user.networth += job.monthlySalary; - if (user.networth >= 1000000) { - user.retired = true; - user.save(); - return 'RETIRED'; - } - user.save(); - if (job.promotionInterval === 0 || job.promotionInterval > user.job.months_worked) { - user.save(); - return user.job; - } else { - let newJob = {}; - return Job.find( { jobType: job.jobType } ) - .then(jobs => { - if(job.jobLevel === 'Entry') { - let foundJob = jobs.filter(j => { - return j.jobLevel === 'Mid-level'; - }); - newJob.start_date = new Date(); - newJob.months_worked = 0; - newJob.job_name = foundJob[0]._id; - user.job = newJob; - user.save(); - return user; - } else if(job.jobLevel === 'Mid-level') { - let foundJob = jobs.filter(j => { - return j.jobLevel === 'Senior'; - }); - user.job.months_worked = 0; - newJob.start_date = new Date(); - newJob.months_worked = 0; - newJob.job_name = foundJob[0]._id; - user.save(); - return user.job = newJob; - } - }); - } - }); + .then(nextJob => { + user.job = { + start_date: new Date(), + months_worked: 0, + job_name: nextJob._id, + }; + return user.save(); }); + } }); - }) - .catch(); -}; +} diff --git a/server.js b/server.js index bff8599..7002a19 100644 --- a/server.js +++ b/server.js @@ -1,7 +1,7 @@ - const http = require('http'); const app = require('./lib/app'); -require('mongoose'); +// AFAICT, this isn't needed here +// require('mongoose'); const findAndUpdate = require('./lib/update-user'); @@ -14,5 +14,6 @@ const port = process.env.PORT || 3000; server.listen(port, () => { console.log('server is running on ', server.address()); }); - -setInterval(findAndUpdate, 86400000); \ No newline at end of file +// good to make big second number more visible +const interval = 1 /*day*/ * 24 /*hrs*/ * 60 /*min*/ * 60 /*sec*/ * 1000 /*ms*/; +setInterval(findAndUpdate, interval); \ No newline at end of file diff --git a/test/api/admin.api.test.js b/test/api/admin.api.test.js index f3e3342..334cdf9 100644 --- a/test/api/admin.api.test.js +++ b/test/api/admin.api.test.js @@ -17,19 +17,17 @@ describe('admin user', () =>{ describe('admin management', () => { const admin = { - username: 'admin', - password: 'supersekritadminpassword' + username: 'admin', + password: 'supersekritadminpassword' }; - let token = ''; - let newAsset = { - asset_type: 'Vehicle', - model: 'Tricycle', - purchase_price: 100, - current_value: 100, - monthly_appreciation_percentage: 0 - }; + asset_type: 'Vehicle', + model: 'Tricycle', + purchase_price: 100, + current_value: 100, + monthly_appreciation_percentage: 0 + }; let newEd = { educationLevel: 'High School', @@ -43,44 +41,37 @@ describe('admin user', () =>{ rewardAmount: 100, rewardMessage: 'Congratulations! You just won $100. Keep rolling!' }; - - const badRequest = (url, data, error) => - request - .post(url) - .send(data) - .then( - () => { throw new Error('status should not be ok'); }, - res => { - assert.equal(res.status, 400); - assert.equal(res.response.body.error, error); - } - ); - const badAdminRequest = (url, data, error) => + /* Refactor these two functions, they are nearly identical + Use a higher order function (or just add a status code property) */ + + const badRequest = (url, data, statusCode, error) => request .post(url) .send(data) .then( () => { throw new Error('status should not be ok'); }, res => { - console.log(res.status); - assert.equal(res.status, 401); + assert.equal(res.status, statusCode); assert.equal(res.response.body.error, error); } ); it('admin signup requires username', () => - badRequest('/admin/signup', {password: 'supersekritadminpassword'}, 'username and password must be provided') + badRequest('/admin/signup', {password: 'supersekritadminpassword'}, 400, 'username and password must be provided') ); it('admin signup requires password', () => - badRequest('/admin/signup', {username: 'horatio'}, 'username and password must be provided') + badRequest('/admin/signup', {username: 'horatio'}, 400, 'username and password must be provided') ); it('admin signup requires username and special admin password', () => - badAdminRequest('/admin/signup', {username: 'horatio', password: 'notsupersekritadminpassword'}, 'Unauthorized to Create Admin Account') + badRequest('/admin/signup', {username: 'horatio', password: 'notsupersekritadminpassword'}, 401, 'Unauthorized to Create Admin Account') ); + // move this closer so it is near where it is assigned + let token = ''; + it('admin signup', () => request .post('/admin/signup') @@ -88,6 +79,7 @@ describe('admin user', () =>{ .then(res => assert.ok(token = res.body.token)) ); + // why not use the badRequest function to test? it('can\'t use same user name', () => request .post('/admin/signup') @@ -100,13 +92,24 @@ describe('admin user', () =>{ } ) ); + + // Make a new describe block to group tests, or seperate files, but not a comment! ///////////////Admin Asset CRUD Tests//////////// - it('can create new assets', () => { + // 1) All of these tests start the same way. + // 2) One option is to put that functionality in a function: + + function getAdminToken(){ return request .post('/admin/signin') .send(admin) - .then(res => res.body.token) + .then(res => res.body.token); + } + + + it('can create new assets', () => { + // 3) then just start with that promise + return getAdminToken() .then((token) => { return request .post('/admin/assets') @@ -118,24 +121,19 @@ describe('admin user', () =>{ }); }); - it('can update an asset', () =>{ - let assetId = ''; + + it('can update an asset', () =>{ + // 4) better option would be to put in `before` and store admin token. + /// then code would just be: return request - .post('/admin/signin') - .send(admin) - .then(res => res.body.token) - .then((token) => { - return request - .post('/admin/assets') - .send(newAsset) - .set('Authorization', token) - .then(res => { - assetId = res.body._id; - return res.body; - }); + .post('/admin/assets') + .send(newAsset) + .set('Authorization', adminToken) + .then(res => { + return res.body._id; }) - .then(() => { + .then(assetId => { return request .patch('/admin/assets') .send({_id: assetId, model: 'Volvo'}) @@ -147,8 +145,6 @@ describe('admin user', () =>{ }); it('can delete an asset', () => { - let assetId = ''; - return request .post('/admin/signin') .send(admin) @@ -157,13 +153,12 @@ describe('admin user', () =>{ return request .post('/admin/assets') .send(newAsset) - .set('Authorization', token) - .then(res => { - assetId = res.body._id; - return res.body; - }); + .set('Authorization', token); }) - .then(() => { + .then(res => { + return res.body._id; + }) + .then(assetId => { return request .delete('/admin/assets') .send({_id: assetId}) @@ -180,21 +175,20 @@ describe('admin user', () =>{ .post('/admin/signin') .send(admin) .then(res => res.body.token) - .then((token) => { - console.log(token); + .then(token => { return request - .post('/admin/education') - .send(newEd) - .set('Authorization', token); + .post('/admin/education') + .send(newEd) + .set('Authorization', token); }) .then(res => { assert.equal(res.status, 200); + // anything else you can assert? + // is a response body expected? }); }); it('can update an education', () =>{ - let edId = ''; - return request .post('/admin/signin') .send(admin) @@ -204,12 +198,11 @@ describe('admin user', () =>{ .post('/admin/education') .send(newEd) .set('Authorization', token) - .then(res => { - edId = res.body._id; - return res.body; - }); }) - .then(() => { + .then(res => { + return res.body._id; + }) + .then(edId => { return request .patch('/admin/education') .send({_id: edId, educationLevel: 'College'}) diff --git a/test/api/assets.api.test.js b/test/api/assets.api.test.js index 6e8546b..68cdc88 100644 --- a/test/api/assets.api.test.js +++ b/test/api/assets.api.test.js @@ -48,7 +48,7 @@ describe('assets REST API', () => { return request .post('/user/signup') .send(janeDoe) - .then((res) => { + .then(res => { console.log('JANE', res.body); janeDoeToken = res.body.token; return Promise.all([ diff --git a/test/api/user.api.test.js b/test/api/user.api.test.js index fdb6920..e8fe7b0 100644 --- a/test/api/user.api.test.js +++ b/test/api/user.api.test.js @@ -121,7 +121,6 @@ describe('user', () => { .set('Authorization', token); }) .then(res => { - console.log(res.body.username); assert.equal(res.body.username, 'hungrymonkey'); }); }); @@ -145,6 +144,8 @@ describe('user', () => { .get('/user/mrbigglesworth') .set('Authorization', token) .then(res => { + // better test would be to signin with new password + userHash = res.body.hash; return request .patch('/user/me/changeAccountInfo') @@ -154,7 +155,6 @@ describe('user', () => { }); }) .then(res => { - console.log('NEWHASH', newHash, 'USERHASH', userHash); assert.notEqual(newHash, userHash); }); }); @@ -218,7 +218,6 @@ describe('user', () => { .send({ johnDoeToken, _id: testAsset3._id }) .set('Authorization', johnDoeToken) .then(res => { - console.log('RESPONSE', res.body); assert.equal(res.body.assets.length, 1); assert.ok(res.body.assets[0].asset_name); }) diff --git a/test/user.model.test.js b/test/user.model.test.js index 446e58e..d8fcb1a 100644 --- a/test/user.model.test.js +++ b/test/user.model.test.js @@ -2,6 +2,8 @@ const User = require('../lib/models/user.model'); const assert = require('chai').assert; describe('user model', () => { + // you alread have `test-invalid.js`, + // don't duplicate code! const Model = function(User) { return (data) => new User(data) .validate()