From c80282dd918b28ec197273bdb95ce473993b8a4c Mon Sep 17 00:00:00 2001 From: David Erwin Date: Fri, 4 Nov 2016 22:24:06 -0400 Subject: [PATCH 01/11] Implement asset upsert and find --- app.js | 4 +++ models/asset.js | 74 +++++++++++++++++++++++++++++++++++++++ mongoose.js | 2 +- package.json | 1 + routes/api/asset/index.js | 30 ++++++++++++++++ routes/api/index.js | 5 +-- 6 files changed, 113 insertions(+), 3 deletions(-) create mode 100644 models/asset.js create mode 100644 routes/api/asset/index.js diff --git a/app.js b/app.js index 7ce7c91ab..060836f19 100644 --- a/app.js +++ b/app.js @@ -4,6 +4,10 @@ const express = require('express'); const app = express(); +const bodyParser = require('body-parser'); + +app.use(bodyParser.json()); // for parsing application/json +app.use(bodyParser.urlencoded({ extended: true })); // for parsing application/x-www-form-urlencoded app.use('/api/v1', require('./routes/api')); app.use('/client/', express.static('./dist')); diff --git a/models/asset.js b/models/asset.js new file mode 100644 index 000000000..9553ce2cd --- /dev/null +++ b/models/asset.js @@ -0,0 +1,74 @@ +const mongoose = require('../mongoose'); +const uuid = require('uuid'); +const Schema = mongoose.Schema; + +const AssetSchema = new Schema({ + + id: { + type: String, + default: uuid.v4, + unique: true, + index: true + }, + url: { + type: String, + unique: true, + index: true + }, + type: { + type: String, + default: 'article' + }, + headline: String, + summary: String, + section: String, + subsection: String, + authors: [String], + publication_date: Date +},{ + _id: false, + timestamps: { + createdAt: 'created_at', + updatedAt: 'updated_at' + } +}); + + +/** + * Finds an asset by its id. + * @param {String} id identifier of the asset (uuid) +*/ +AssetSchema.statics.findById = function(id) { + + return Asset.findOne({id}); + +}; + +/** + * Finds a asset by its url. + * @param {String} url identifier of the asset (uuid) +*/ +AssetSchema.statics.findByUrl = function(url) { + + return Asset.findOne({url: url}); + +}; + + +/** + * Upserts an asset. +*/ +AssetSchema.statics.upsert = function(data) { + + // If an id is not sent, create one. + if (typeof data.id === 'undefined') { + data.id = uuid.v4(); + } + + return Asset.update({id: data.id}, data, {upsert: true}); + +}; + +const Asset = mongoose.model('Asset', AssetSchema); + +module.exports = Asset; diff --git a/mongoose.js b/mongoose.js index 0aff4c22c..953eb0d11 100644 --- a/mongoose.js +++ b/mongoose.js @@ -1,6 +1,6 @@ const mongoose = require('mongoose'); const enabled = require('debug').enabled; -const url = process.env.TALK_MONGO_URL || 'mongodb://localhost'; +const url = process.env.TALK_MONGO_URL || 'mongodb://localhost/coral-talk'; // Use native promises mongoose.Promise = global.Promise; diff --git a/package.json b/package.json index 096863202..d392d0b09 100644 --- a/package.json +++ b/package.json @@ -41,6 +41,7 @@ }, "homepage": "https://github.com/coralproject/talk#readme", "dependencies": { + "body-parser": "^1.15.2", "debug": "^2.2.0", "express": "^4.14.0", "mongoose": "^4.6.5" diff --git a/routes/api/asset/index.js b/routes/api/asset/index.js new file mode 100644 index 000000000..77fc55341 --- /dev/null +++ b/routes/api/asset/index.js @@ -0,0 +1,30 @@ +const express = require('express'); +const router = express.Router(); +const Asset = require('../../../models/asset'); + +// Get an asset by url +router.get('/url/:url', (req, res) => { + + Asset.findByUrl(req.params.url) + .then((asset) => { + res.json(asset); + }); + +}); + +// Upsert an asset +router.put('/', (req, res) => { + + Asset.upsert(req.body) + .then((asset) => { + res.json(asset); + }) + .catch((err) => { + console.error(err); + res.json(err); + }); + +}); + + +module.exports = router; \ No newline at end of file diff --git a/routes/api/index.js b/routes/api/index.js index c7e5601ed..acd007dae 100644 --- a/routes/api/index.js +++ b/routes/api/index.js @@ -2,9 +2,10 @@ const express = require('express'); const router = express.Router(); +router.use('/asset', require('./asset')); router.use('/comments', require('./comments')); -router.use('/stream', require('./stream')); -router.use('/settings', require('./settings')); router.use('/queue', require('./queue')); +router.use('/settings', require('./settings')); +router.use('/stream', require('./stream')); module.exports = router; From cb08f139ca29a91fd782cb1bf8e7d82f0e9069f2 Mon Sep 17 00:00:00 2001 From: David Erwin Date: Sat, 5 Nov 2016 08:50:34 -0400 Subject: [PATCH 02/11] Swagger and linting --- app.js | 2 +- bin/init.js | 2 +- routes/api/asset/index.js | 18 +++++++--- swagger.yaml | 69 ++++++++++++++++++++++++++++++++++++++- 4 files changed, 84 insertions(+), 7 deletions(-) diff --git a/app.js b/app.js index 060836f19..935990e9b 100644 --- a/app.js +++ b/app.js @@ -7,7 +7,7 @@ const app = express(); const bodyParser = require('body-parser'); app.use(bodyParser.json()); // for parsing application/json -app.use(bodyParser.urlencoded({ extended: true })); // for parsing application/x-www-form-urlencoded +app.use(bodyParser.urlencoded({extended: true})); // for parsing application/x-www-form-urlencoded app.use('/api/v1', require('./routes/api')); app.use('/client/', express.static('./dist')); diff --git a/bin/init.js b/bin/init.js index d114d4819..6ee35bfe4 100644 --- a/bin/init.js +++ b/bin/init.js @@ -3,5 +3,5 @@ const defaults = {id: '1', moderation: 'pre'}; Setting.update({id: '1'}, {$setOnInsert: defaults}, {upsert: true}).then(() => { console.log('created settings object.'); - process.exit(); +// process.exit(); // Removed to satisfy hooks }).catch(console.error); diff --git a/routes/api/asset/index.js b/routes/api/asset/index.js index 77fc55341..a65f9687a 100644 --- a/routes/api/asset/index.js +++ b/routes/api/asset/index.js @@ -2,6 +2,16 @@ const express = require('express'); const router = express.Router(); const Asset = require('../../../models/asset'); +// Get an asset by id +router.get('/:id', (req, res) => { + + Asset.findById(req.params.id) + .then((asset) => { + res.json(asset); + }); + +}); + // Get an asset by url router.get('/url/:url', (req, res) => { @@ -17,14 +27,14 @@ router.put('/', (req, res) => { Asset.upsert(req.body) .then((asset) => { - res.json(asset); + res.json(asset); }) .catch((err) => { - console.error(err); - res.json(err); + console.error(err); + res.json(err); }); }); -module.exports = router; \ No newline at end of file +module.exports = router; diff --git a/swagger.yaml b/swagger.yaml index a7fdc973d..d9624c960 100644 --- a/swagger.yaml +++ b/swagger.yaml @@ -3,7 +3,7 @@ info: title: Talk API description: A commenting platform from The Coral Project. https://coralproject.net version: "0.0.1" -host: api.talk-coralproject.net +host: talk-stg.coralproject.net/api/v1 schemes: - https basePath: /v1 @@ -158,6 +158,33 @@ paths: responses: 204: description: OK + /asset: + get: + tags: + - Asset + description: Get an asset by id. + responses: + 200: + description: OK + 404: + description: Not Found + put: + tags: + - Asset + description: Upsert an asset. + responses: + 204: + description: OK + /asset/url/{url}: + get: + tags: + - Asset + description: Get an asset by its url. + responses: + 200: + description: OK + 404: + description: Not Found @@ -212,3 +239,43 @@ definitions: type: string user_id: type: string + Asset: + type: object + properties: + id: + type: string + description: The uuid.v4 id of the asset. + url: + type: string + description: The url where the asset is found. + type: + type: string + description: What kind of asset it is. + default: article + headline: + type: string + description: The headline of the asset, inferred on initial load. + summary: + type: string + description: A summary of the asset, inferred on initial load. + section: + type: string + description: The section the asset is in. + subsection: + type: string + description: The subsection that the asset is in. + authors: + type: string + description: An array of the authors for this asset. + publication_date: + type: date + desctipion: When this asset was published. + + + + + + + + + From 17ccc830d94e2ad808af9171e634cfd326ccc03e Mon Sep 17 00:00:00 2001 From: David Erwin Date: Sat, 5 Nov 2016 08:54:59 -0400 Subject: [PATCH 03/11] Update swagger --- swagger.yaml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/swagger.yaml b/swagger.yaml index d9624c960..36af7a4c5 100644 --- a/swagger.yaml +++ b/swagger.yaml @@ -179,6 +179,11 @@ paths: get: tags: - Asset + parameters: + - name: url + in: query + description: The url of the asset. + required: true description: Get an asset by its url. responses: 200: From a6b2215a4be3d6c9f6ec88b4ba1c37d68026d2ae Mon Sep 17 00:00:00 2001 From: David Erwin Date: Sat, 5 Nov 2016 11:14:21 -0400 Subject: [PATCH 04/11] Implement asset tests --- models/asset.js | 44 ++++++++++++++-- package.json | 2 +- routes/api/asset/index.js | 14 +++++- tests/asset.js | 103 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 157 insertions(+), 6 deletions(-) create mode 100644 tests/asset.js diff --git a/models/asset.js b/models/asset.js index 9553ce2cd..de7cf8e65 100644 --- a/models/asset.js +++ b/models/asset.js @@ -27,6 +27,7 @@ const AssetSchema = new Schema({ publication_date: Date },{ _id: false, + versionKey: false, timestamps: { createdAt: 'created_at', updatedAt: 'updated_at' @@ -34,9 +35,18 @@ const AssetSchema = new Schema({ }); +/** + * Search for assets. Currently only returns all. +*/ +AssetSchema.statics.search = function() { + + return Asset.find({}); + +}; + /** * Finds an asset by its id. - * @param {String} id identifier of the asset (uuid) + * @param {String} id identifier of the asset (uuid). */ AssetSchema.statics.findById = function(id) { @@ -46,11 +56,11 @@ AssetSchema.statics.findById = function(id) { /** * Finds a asset by its url. - * @param {String} url identifier of the asset (uuid) + * @param {String} url identifier of the asset (uuid). */ AssetSchema.statics.findByUrl = function(url) { - return Asset.findOne({url: url}); + return Asset.findOne({'url': url}); }; @@ -65,10 +75,36 @@ AssetSchema.statics.upsert = function(data) { data.id = uuid.v4(); } - return Asset.update({id: data.id}, data, {upsert: true}); + // Perform the upsert against the id field. + let updatePromise = Asset.update({id: data.id}, data, {upsert: true}) + .then((updateRes) => { + + // Pull the freshly minted asset out and return. + return Asset.findById(data.id); + + }) + .catch((err) => { + + console.error('Error upserting asset.', err); + //return new Promise(); // ??? what do we return on error? + + }); + return updatePromise; + }; +/** + * Remove assets from the db. + * @param {String} query bson query to identify assets to be removed. +*/ +AssetSchema.statics.removeAll = function(query) { + + return Asset.remove(query); + +}; + + const Asset = mongoose.model('Asset', AssetSchema); module.exports = Asset; diff --git a/package.json b/package.json index d392d0b09..e581146ee 100644 --- a/package.json +++ b/package.json @@ -7,7 +7,6 @@ "start": "./bin/www", "build": "webpack --config ./client/coral-embed-stream/webpack.config.js", "lint": "eslint .", - "pretest": "npm install", "test": "mocha tests", "embed-start": "node client/coral-embed-stream/dev-server.js" }, @@ -42,6 +41,7 @@ "homepage": "https://github.com/coralproject/talk#readme", "dependencies": { "body-parser": "^1.15.2", + "chai-http": "^3.0.0", "debug": "^2.2.0", "express": "^4.14.0", "mongoose": "^4.6.5" diff --git a/routes/api/asset/index.js b/routes/api/asset/index.js index a65f9687a..126e9ba06 100644 --- a/routes/api/asset/index.js +++ b/routes/api/asset/index.js @@ -2,6 +2,16 @@ const express = require('express'); const router = express.Router(); const Asset = require('../../../models/asset'); +// Get many assets +router.get('/', (req, res) => { + + Asset.search(req.params.id) + .then((asset) => { + res.json(asset); + }); + +}); + // Get an asset by id router.get('/:id', (req, res) => { @@ -22,12 +32,14 @@ router.get('/url/:url', (req, res) => { }); -// Upsert an asset +// Upsert an asset and return the affected document. router.put('/', (req, res) => { Asset.upsert(req.body) .then((asset) => { + res.json(asset); + }) .catch((err) => { console.error(err); diff --git a/tests/asset.js b/tests/asset.js new file mode 100644 index 000000000..14ab23125 --- /dev/null +++ b/tests/asset.js @@ -0,0 +1,103 @@ +/* eslint-env node, mocha */ +const Asset = require('../models/asset'); + +const expect = require('chai').expect; +let chai = require('chai'); +let chaiHttp = require('chai-http'); +let server = require('../app'); +let should = chai.should(); + +chai.use(chaiHttp); + +var fixture = { + "url": "simple", + "type": "article", + "headline": "The Total Perspective Vortex", + "summary": "You are an insignificant dot on an insignificant dot.", + "section": "Everything", + "authors": ["Ford Prefect"] +}; + + +describe('Asset', () => { + + beforeEach((done) => { + + // TODO: implement asset remove + Asset.removeAll({}) + .then((asset) => { + done(); + }); + + }); + + describe('/GET Asset', () => { + describe.only('#get', () => { + it('It should get an empty array when there are no assets.', (done) => { + + chai.request(server) + .get('/api/v1/asset') + .end((err, res) => { + + if (err) { + throw new Error(err); + } + + res.should.have.status(200); + res.body.should.be.a('array'); + res.body.length.should.be.eql(0); + done(); + }); + + }); + }); + }); + + // This test checks PUT and read + describe('/PUT Asset', () => { + describe.only('#put', () => { + it('It should save an asset and load it again.', (done) => { + + chai.request(server) + .put('/api/v1/asset') + .send(fixture) + .end((err, res) => { + + if (err) { + throw new Error(err); + } + + res.should.have.status(200); + res.body.should.be.a('object'); + + // Id should be generated by the model if absent. + res.body.should.have.property('id'); + + // Save the asset id to compare with GET result. + let assetId = res.body.id; + + // Load the asset to make sure it's really there. + chai.request(server) + .get('/api/v1/asset/url/' + fixture.url) + .end((err, res) => { + + if (err) { + throw new Error(err); + } + + res.should.have.status(200); + res.body.should.be.a('object'); + res.body.should.have.property('id'); + + // ensure the asset has the same id as above + expect(assetId).to.equal(res.body.id); + + done(); + + }); + }); + }); + }); + }); // End describe /PUT Asset + +}); From 712841679d52d0cb29caf830141690aa39b99ed8 Mon Sep 17 00:00:00 2001 From: David Erwin Date: Sat, 5 Nov 2016 11:18:18 -0400 Subject: [PATCH 05/11] Linting and clarifying tests --- models/asset.js | 2 +- tests/asset.js | 21 ++++++++++----------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/models/asset.js b/models/asset.js index de7cf8e65..eb55094ac 100644 --- a/models/asset.js +++ b/models/asset.js @@ -77,7 +77,7 @@ AssetSchema.statics.upsert = function(data) { // Perform the upsert against the id field. let updatePromise = Asset.update({id: data.id}, data, {upsert: true}) - .then((updateRes) => { + .then(() => { // Pull the freshly minted asset out and return. return Asset.findById(data.id); diff --git a/tests/asset.js b/tests/asset.js index 14ab23125..187a9f1e7 100644 --- a/tests/asset.js +++ b/tests/asset.js @@ -5,17 +5,16 @@ const expect = require('chai').expect; let chai = require('chai'); let chaiHttp = require('chai-http'); let server = require('../app'); -let should = chai.should(); chai.use(chaiHttp); var fixture = { - "url": "simple", - "type": "article", - "headline": "The Total Perspective Vortex", - "summary": "You are an insignificant dot on an insignificant dot.", - "section": "Everything", - "authors": ["Ford Prefect"] + 'url': 'simple', + 'type': 'article', + 'headline': 'The Total Perspective Vortex', + 'summary': 'You are an insignificant dot on an insignificant dot.', + 'section': 'Everything', + 'authors': ['Ford Prefect'] }; @@ -24,8 +23,8 @@ describe('Asset', () => { beforeEach((done) => { // TODO: implement asset remove - Asset.removeAll({}) - .then((asset) => { + return Asset.removeAll({}) + .then(() => { done(); }); @@ -94,8 +93,8 @@ describe('Asset', () => { done(); - }); - }); + }); + }); }); }); }); // End describe /PUT Asset From 2c217c240a52e4d2ed64ec7c03f16095967ebf8f Mon Sep 17 00:00:00 2001 From: David Erwin Date: Sat, 5 Nov 2016 11:26:01 -0400 Subject: [PATCH 06/11] Rectify linting and testing disagreements --- tests/asset.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tests/asset.js b/tests/asset.js index 187a9f1e7..7ba98679d 100644 --- a/tests/asset.js +++ b/tests/asset.js @@ -5,6 +5,8 @@ const expect = require('chai').expect; let chai = require('chai'); let chaiHttp = require('chai-http'); let server = require('../app'); +let should = chai.should(); +should; // nullop to satisfy linting chai.use(chaiHttp); @@ -23,11 +25,13 @@ describe('Asset', () => { beforeEach((done) => { // TODO: implement asset remove - return Asset.removeAll({}) + Asset.removeAll({}) .then(() => { done(); }); + Asset; // nullop to satisfy linting. + }); describe('/GET Asset', () => { @@ -88,7 +92,8 @@ describe('Asset', () => { res.body.should.be.a('object'); res.body.should.have.property('id'); - // ensure the asset has the same id as above + // Ensure the asset has the same id as above. + // This tests the single url per Id concept. expect(assetId).to.equal(res.body.id); done(); From c1359a9af9458c380253c29f223341223eddb6ef Mon Sep 17 00:00:00 2001 From: David Erwin Date: Sat, 5 Nov 2016 14:44:42 -0400 Subject: [PATCH 07/11] Move url route into general asset search --- models/asset.js | 4 ++-- routes/api/asset/index.js | 20 ++++++++------------ tests/asset.js | 13 ++++++++----- 3 files changed, 18 insertions(+), 19 deletions(-) diff --git a/models/asset.js b/models/asset.js index eb55094ac..afae0c677 100644 --- a/models/asset.js +++ b/models/asset.js @@ -38,9 +38,9 @@ const AssetSchema = new Schema({ /** * Search for assets. Currently only returns all. */ -AssetSchema.statics.search = function() { +AssetSchema.statics.search = function(query) { - return Asset.find({}); + return Asset.find(query); }; diff --git a/routes/api/asset/index.js b/routes/api/asset/index.js index 126e9ba06..0b3959204 100644 --- a/routes/api/asset/index.js +++ b/routes/api/asset/index.js @@ -2,10 +2,16 @@ const express = require('express'); const router = express.Router(); const Asset = require('../../../models/asset'); -// Get many assets +// Search assets. router.get('/', (req, res) => { - Asset.search(req.params.id) + let query = {}; + + if (typeof req.query.url !== 'undefined') { + query.url = req.query.url; + } + + Asset.search(query) .then((asset) => { res.json(asset); }); @@ -22,16 +28,6 @@ router.get('/:id', (req, res) => { }); -// Get an asset by url -router.get('/url/:url', (req, res) => { - - Asset.findByUrl(req.params.url) - .then((asset) => { - res.json(asset); - }); - -}); - // Upsert an asset and return the affected document. router.put('/', (req, res) => { diff --git a/tests/asset.js b/tests/asset.js index 7ba98679d..a2019966f 100644 --- a/tests/asset.js +++ b/tests/asset.js @@ -11,7 +11,7 @@ should; // nullop to satisfy linting chai.use(chaiHttp); var fixture = { - 'url': 'simple', + 'url': 'http://hhgg.com/total-perspective-vortex', 'type': 'article', 'headline': 'The Total Perspective Vortex', 'summary': 'You are an insignificant dot on an insignificant dot.', @@ -81,7 +81,7 @@ describe('Asset', () => { // Load the asset to make sure it's really there. chai.request(server) - .get('/api/v1/asset/url/' + fixture.url) + .get('/api/v1/asset?url=' + fixture.url) .end((err, res) => { if (err) { @@ -89,12 +89,15 @@ describe('Asset', () => { } res.should.have.status(200); - res.body.should.be.a('object'); - res.body.should.have.property('id'); + res.body.should.be.an('array'); + + let asset = res.body[0]; + + expect(asset).to.have.property('id'); // Ensure the asset has the same id as above. // This tests the single url per Id concept. - expect(assetId).to.equal(res.body.id); + expect(assetId).to.equal(asset.id); done(); From c84fc24f783952fa9b465f83d0315061cfaec9bd Mon Sep 17 00:00:00 2001 From: David Erwin Date: Mon, 7 Nov 2016 13:07:25 -0500 Subject: [PATCH 08/11] Update based on review --- .eslintignore | 1 + models/asset.js | 9 ++++----- routes/api/asset/index.js | 19 ++++++++----------- swagger.yaml | 5 +---- 4 files changed, 14 insertions(+), 20 deletions(-) diff --git a/.eslintignore b/.eslintignore index b051c6c57..6cb6a3bc3 100644 --- a/.eslintignore +++ b/.eslintignore @@ -1 +1,2 @@ client +dist diff --git a/models/asset.js b/models/asset.js index afae0c677..5864d6409 100644 --- a/models/asset.js +++ b/models/asset.js @@ -26,7 +26,6 @@ const AssetSchema = new Schema({ authors: [String], publication_date: Date },{ - _id: false, versionKey: false, timestamps: { createdAt: 'created_at', @@ -40,7 +39,7 @@ const AssetSchema = new Schema({ */ AssetSchema.statics.search = function(query) { - return Asset.find(query); + return Asset.find(query).exec(); }; @@ -50,7 +49,7 @@ AssetSchema.statics.search = function(query) { */ AssetSchema.statics.findById = function(id) { - return Asset.findOne({id}); + return Asset.findOne({id}).exec(); }; @@ -60,7 +59,7 @@ AssetSchema.statics.findById = function(id) { */ AssetSchema.statics.findByUrl = function(url) { - return Asset.findOne({'url': url}); + return Asset.findOne({'url': url}).exec(); }; @@ -100,7 +99,7 @@ AssetSchema.statics.upsert = function(data) { */ AssetSchema.statics.removeAll = function(query) { - return Asset.remove(query); + return Asset.remove(query).exec(); }; diff --git a/routes/api/asset/index.js b/routes/api/asset/index.js index 0b3959204..235958aaa 100644 --- a/routes/api/asset/index.js +++ b/routes/api/asset/index.js @@ -3,7 +3,7 @@ const router = express.Router(); const Asset = require('../../../models/asset'); // Search assets. -router.get('/', (req, res) => { +router.get('/', (req, res, next) => { let query = {}; @@ -14,33 +14,30 @@ router.get('/', (req, res) => { Asset.search(query) .then((asset) => { res.json(asset); - }); + }) + .catch(next); }); // Get an asset by id -router.get('/:id', (req, res) => { +router.get('/:id', (req, res, next) => { Asset.findById(req.params.id) .then((asset) => { res.json(asset); - }); + }) + .catch(next); }); // Upsert an asset and return the affected document. -router.put('/', (req, res) => { +router.put('/', (req, res, next) => { Asset.upsert(req.body) .then((asset) => { - res.json(asset); - }) - .catch((err) => { - console.error(err); - res.json(err); - }); + .catch(next); }); diff --git a/swagger.yaml b/swagger.yaml index ca7ef29d0..acc696ec6 100644 --- a/swagger.yaml +++ b/swagger.yaml @@ -179,7 +179,7 @@ paths: responses: 204: description: OK - /asset/url/{url}: + /asset?url={url}: get: tags: - Asset @@ -192,9 +192,6 @@ paths: responses: 200: description: OK - 404: - description: Not Found - definitions: Item: From 7b2b2eaa77478a9eea5c7297cd8cf614c52feafe Mon Sep 17 00:00:00 2001 From: David Erwin Date: Mon, 7 Nov 2016 13:11:39 -0500 Subject: [PATCH 09/11] Add more review suggestions. --- mongoose.js | 2 +- tests/asset.js | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/mongoose.js b/mongoose.js index 953eb0d11..0aff4c22c 100644 --- a/mongoose.js +++ b/mongoose.js @@ -1,6 +1,6 @@ const mongoose = require('mongoose'); const enabled = require('debug').enabled; -const url = process.env.TALK_MONGO_URL || 'mongodb://localhost/coral-talk'; +const url = process.env.TALK_MONGO_URL || 'mongodb://localhost'; // Use native promises mongoose.Promise = global.Promise; diff --git a/tests/asset.js b/tests/asset.js index a2019966f..f523ce987 100644 --- a/tests/asset.js +++ b/tests/asset.js @@ -2,10 +2,10 @@ const Asset = require('../models/asset'); const expect = require('chai').expect; -let chai = require('chai'); -let chaiHttp = require('chai-http'); -let server = require('../app'); -let should = chai.should(); +const chai = require('chai'); +const chaiHttp = require('chai-http'); +const server = require('../app'); +const should = chai.should(); should; // nullop to satisfy linting chai.use(chaiHttp); From d3213dd33e418afe98e6dd8fd820ff49c68b8531 Mon Sep 17 00:00:00 2001 From: David Erwin Date: Mon, 7 Nov 2016 13:18:46 -0500 Subject: [PATCH 10/11] More review updates --- models/asset.js | 2 +- swagger.yaml | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/models/asset.js b/models/asset.js index 5864d6409..afd446b01 100644 --- a/models/asset.js +++ b/models/asset.js @@ -75,7 +75,7 @@ AssetSchema.statics.upsert = function(data) { } // Perform the upsert against the id field. - let updatePromise = Asset.update({id: data.id}, data, {upsert: true}) + let updatePromise = Asset.update({id: data.id}, data, {upsert: true}).exec() .then(() => { // Pull the freshly minted asset out and return. diff --git a/swagger.yaml b/swagger.yaml index acc696ec6..ba945e2e1 100644 --- a/swagger.yaml +++ b/swagger.yaml @@ -3,10 +3,10 @@ info: title: Talk API description: A commenting platform from The Coral Project. https://coralproject.net version: "0.0.1" -host: talk-stg.coralproject.net/api/v1 +host: talk-stg.coralproject.net schemes: - https -basePath: /v1 +basePath: /api/v1 produces: - application/json paths: @@ -170,8 +170,6 @@ paths: responses: 200: description: OK - 404: - description: Not Found put: tags: - Asset From 4a9c1775f1d4fee255b33e7e03349572160916e0 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Mon, 7 Nov 2016 11:50:53 -0700 Subject: [PATCH 11/11] Updated linting rules + fixed test layout to standardize --- .eslintrc.json | 15 +++++++- models/action.js | 2 +- models/asset.js | 11 ++---- models/comment.js | 3 +- models/user.js | 2 +- mongoose.js | 2 +- routes/api/asset/index.js | 1 - routes/api/comments/index.js | 1 - routes/api/stream/index.js | 2 +- tests/.eslintrc.json | 5 ++- tests/index.js | 5 --- tests/models/action.js | 11 +++--- tests/{ => models}/asset.js | 41 +++++++------------- tests/models/comment.js | 10 ++--- tests/models/user.js | 13 +++---- tests/routes/api/comments/index.js | 62 +++++++++++++++--------------- tests/routes/api/stream/index.js | 36 +++++++++-------- tests/utils/mongoose.js | 8 ++-- 18 files changed, 106 insertions(+), 124 deletions(-) rename tests/{ => models}/asset.js (74%) diff --git a/.eslintrc.json b/.eslintrc.json index eec982dac..5df3444a6 100644 --- a/.eslintrc.json +++ b/.eslintrc.json @@ -45,6 +45,19 @@ "space-infix-ops": ["error"], "no-const-assign": [2], "no-duplicate-imports": [2], - "prefer-template": [1] + "prefer-template": [1], + "comma-spacing": [ + "error", + { + "after": true + } + ], + "no-var": [2], + "no-lonely-if": [2], + "curly": [2], + "no-multiple-empty-lines": [ + "error", + {"max": 1} + ] } } diff --git a/models/action.js b/models/action.js index cdbf92e8f..27c63747a 100644 --- a/models/action.js +++ b/models/action.js @@ -12,7 +12,7 @@ const ActionSchema = new Schema({ item_type: String, item_id: String, user_id: String -},{ +}, { timestamps: { createdAt: 'created_at', updatedAt: 'updated_at' diff --git a/models/asset.js b/models/asset.js index afd446b01..de8155131 100644 --- a/models/asset.js +++ b/models/asset.js @@ -25,7 +25,7 @@ const AssetSchema = new Schema({ subsection: String, authors: [String], publication_date: Date -},{ +}, { versionKey: false, timestamps: { createdAt: 'created_at', @@ -33,7 +33,6 @@ const AssetSchema = new Schema({ } }); - /** * Search for assets. Currently only returns all. */ @@ -58,12 +57,11 @@ AssetSchema.statics.findById = function(id) { * @param {String} url identifier of the asset (uuid). */ AssetSchema.statics.findByUrl = function(url) { - + return Asset.findOne({'url': url}).exec(); }; - /** * Upserts an asset. */ @@ -88,7 +86,7 @@ AssetSchema.statics.upsert = function(data) { //return new Promise(); // ??? what do we return on error? }); - + return updatePromise; }; @@ -98,12 +96,11 @@ AssetSchema.statics.upsert = function(data) { * @param {String} query bson query to identify assets to be removed. */ AssetSchema.statics.removeAll = function(query) { - + return Asset.remove(query).exec(); }; - const Asset = mongoose.model('Asset', AssetSchema); module.exports = Asset; diff --git a/models/comment.js b/models/comment.js index 468478810..d6a814e64 100644 --- a/models/comment.js +++ b/models/comment.js @@ -21,7 +21,7 @@ const CommentSchema = new Schema({ default: '' }, parent_id: String -},{ +}, { timestamps: { createdAt: 'created_at', updatedAt: 'updated_at' @@ -44,7 +44,6 @@ CommentSchema.statics.findByAssetId = function(asset_id) { return Comment.find({asset_id}); }; - const Comment = mongoose.model('Comment', CommentSchema); module.exports = Comment; diff --git a/models/user.js b/models/user.js index 76635abfb..4592ec847 100644 --- a/models/user.js +++ b/models/user.js @@ -11,7 +11,7 @@ const UserProfileSchema = new Schema({ }, display_name: String, auth_user_id: String -},{ +}, { timestamps: { createdAt: 'created_at', updatedAt: 'updated_at' diff --git a/mongoose.js b/mongoose.js index 0aff4c22c..8d9bb3bfb 100644 --- a/mongoose.js +++ b/mongoose.js @@ -11,7 +11,7 @@ if (enabled('talk:db')) { try { mongoose.connect(url, (err) => { - if (err) throw err; + if (err) {throw err;} console.log('Connected to MongoDB!'); }); } catch (err) { diff --git a/routes/api/asset/index.js b/routes/api/asset/index.js index 235958aaa..b261da883 100644 --- a/routes/api/asset/index.js +++ b/routes/api/asset/index.js @@ -41,5 +41,4 @@ router.put('/', (req, res, next) => { }); - module.exports = router; diff --git a/routes/api/comments/index.js b/routes/api/comments/index.js index df9bbc456..212c47ac6 100644 --- a/routes/api/comments/index.js +++ b/routes/api/comments/index.js @@ -7,7 +7,6 @@ const router = express.Router(); // Routes //============================================================================== - router.get('/', (req, res) => { res.send('Read all of the comments ever'); }); diff --git a/routes/api/stream/index.js b/routes/api/stream/index.js index c63bf871d..67216a252 100644 --- a/routes/api/stream/index.js +++ b/routes/api/stream/index.js @@ -17,7 +17,7 @@ router.get('/', (req, res, next) => { Action.findByItemIdArray(comments.map((comment) => comment.id)) ]); }).then(([comments, users, actions]) => { - res.json([...comments,...users,...actions]); + res.json([...comments, ...users, ...actions]); }).catch(error => { next(error); }); diff --git a/tests/.eslintrc.json b/tests/.eslintrc.json index 97d8c197e..363ef5a28 100644 --- a/tests/.eslintrc.json +++ b/tests/.eslintrc.json @@ -1,9 +1,10 @@ { "env": { "es6": true, - "node": true + "node": true, + "mocha": true }, - "extends": "eslint:recommended", + "extends": "../.eslintrc.json", "rules": { "no-undef": [0] } diff --git a/tests/index.js b/tests/index.js index 51ccc54ee..0c3d947ce 100644 --- a/tests/index.js +++ b/tests/index.js @@ -1,10 +1,5 @@ -/* eslint-env node, mocha */ -'use strict'; - -// require('./utils/mongoose') const expect = require('chai').expect; - describe('Comment', () => { describe('#add', () => { it('should add a comment', () => { diff --git a/tests/models/action.js b/tests/models/action.js index 8736edcab..34a9f2dca 100644 --- a/tests/models/action.js +++ b/tests/models/action.js @@ -1,19 +1,18 @@ -/* eslint-env node, mocha */ - require('../utils/mongoose'); + const Action = require('../../models/action'); const expect = require('chai').expect; describe('Action: models', () => { - var mockActions; + let mockActions; beforeEach(() => { return Action.create([{ action_type: 'flag', item_id: '123' - },{ + }, { action_type: 'like', item_id: '789' - },{ + }, { action_type: 'flag', item_id: '456' }]).then((actions) => { @@ -32,7 +31,7 @@ describe('Action: models', () => { describe('#findByItemIdArray()', () => { it('should find an array of actions from an array of item_ids', () => { - return Action.findByItemIdArray(['123','456']).then((result) => { + return Action.findByItemIdArray(['123', '456']).then((result) => { expect(result).to.have.length(2); }); }); diff --git a/tests/asset.js b/tests/models/asset.js similarity index 74% rename from tests/asset.js rename to tests/models/asset.js index f523ce987..b4d950a93 100644 --- a/tests/asset.js +++ b/tests/models/asset.js @@ -1,16 +1,14 @@ -/* eslint-env node, mocha */ -const Asset = require('../models/asset'); +require('../utils/mongoose'); -const expect = require('chai').expect; const chai = require('chai'); -const chaiHttp = require('chai-http'); -const server = require('../app'); -const should = chai.should(); -should; // nullop to satisfy linting +const expect = chai.expect; +const server = require('../../app'); -chai.use(chaiHttp); +// Setup chai. +chai.should(); +chai.use(require('chai-http')); -var fixture = { +let fixture = { 'url': 'http://hhgg.com/total-perspective-vortex', 'type': 'article', 'headline': 'The Total Perspective Vortex', @@ -19,23 +17,10 @@ var fixture = { 'authors': ['Ford Prefect'] }; +describe('Asset: models', () => { -describe('Asset', () => { - - beforeEach((done) => { - - // TODO: implement asset remove - Asset.removeAll({}) - .then(() => { - done(); - }); - - Asset; // nullop to satisfy linting. - - }); - describe('/GET Asset', () => { - describe.only('#get', () => { + describe('#get', () => { it('It should get an empty array when there are no assets.', (done) => { chai.request(server) @@ -58,7 +43,7 @@ describe('Asset', () => { // This test checks PUT and read describe('/PUT Asset', () => { - describe.only('#put', () => { + describe('#put', () => { it('It should save an asset and load it again.', (done) => { chai.request(server) @@ -81,7 +66,7 @@ describe('Asset', () => { // Load the asset to make sure it's really there. chai.request(server) - .get('/api/v1/asset?url=' + fixture.url) + .get(`/api/v1/asset?url=${encodeURIComponent(fixture.url)}`) .end((err, res) => { if (err) { @@ -94,9 +79,9 @@ describe('Asset', () => { let asset = res.body[0]; expect(asset).to.have.property('id'); - + // Ensure the asset has the same id as above. - // This tests the single url per Id concept. + // This tests the single url per Id concept. expect(assetId).to.equal(asset.id); done(); diff --git a/tests/models/comment.js b/tests/models/comment.js index a8a543183..b3f0c0f30 100644 --- a/tests/models/comment.js +++ b/tests/models/comment.js @@ -1,20 +1,18 @@ -/* eslint-env node, mocha */ - require('../utils/mongoose'); const Comment = require('../../models/comment'); const expect = require('chai').expect; describe('Comment: models', () => { - var mockComments; + let mockComments; beforeEach(() => { return Comment.create([{ body: 'comment 10', asset_id: '123' - },{ + }, { body: 'comment 20', asset_id: '123' - },{ + }, { body: 'comment 30', asset_id: '456' }]).then((comments) => { @@ -35,7 +33,7 @@ describe('Comment: models', () => { it('should find an array of comments by asset id', () => { return Comment.findByAssetId('123').then((result) => { expect(result).to.have.length(2); - result.sort((a,b) => { + result.sort((a, b) => { if (a.body < b.body) {return -1;} else {return 1;} }); diff --git a/tests/models/user.js b/tests/models/user.js index e1063e21c..46fc7703c 100644 --- a/tests/models/user.js +++ b/tests/models/user.js @@ -1,17 +1,16 @@ -/* eslint-env node, mocha */ - require('../utils/mongoose'); + const User = require('../../models/user'); const expect = require('chai').expect; describe('User: models', () => { - var mockUsers; + let mockUsers; beforeEach(() => { return User.create([{ display_name: 'Stampi', - },{ + }, { display_name: 'Sockmonster', - },{ + }, { display_name: 'Marvel', }]).then((users) => { mockUsers = users; @@ -29,12 +28,10 @@ describe('User: models', () => { describe('#findByIdArray()', () => { it('should find an array of users from an array of ids', () => { - const ids = mockUsers.map((user) => user.id) + const ids = mockUsers.map((user) => user.id); return User.findByIdArray(ids).then((result) => { expect(result).to.have.length(3); }); }); }); - - // }); }); diff --git a/tests/routes/api/comments/index.js b/tests/routes/api/comments/index.js index ad21e54ec..1115a189b 100644 --- a/tests/routes/api/comments/index.js +++ b/tests/routes/api/comments/index.js @@ -4,10 +4,11 @@ require('../../../utils/mongoose'); const app = require('../../../../app'); const chai = require('chai'); -const chaiHttp = require('chai-http'); -chai.use(chaiHttp); -var expect = chai.expect; +const expect = chai.expect; +// Setup chai. +chai.should(); +chai.use(require('chai-http')); const Comment = require('../../../../models/comment'); const Action = require('../../../../models/action'); @@ -17,36 +18,36 @@ describe('Post /comments', () => { const users = [{ id: '123', display_name: 'John', - },{ + }, { id: '456', display_name: 'Paul', - }] + }]; const actions = [{ action_type: 'flag', item_id: 'abc' - },{ + }, { action_type: 'like', item_id: 'hij' - }] + }]; beforeEach(() => { return User.create(users).then(() => { - return Action.create(actions) - }) - }) + return Action.create(actions); + }); + }); it('it should create a comment', function(done) { chai.request(app) .post('/api/v1/comments') .query({'body': 'Something body.', 'author_id': '123', 'asset_id': '1', 'parent_id': ''}) .end(function(err, res){ - expect(res).to.have.status(201) - done() - }) - }) + expect(res).to.have.status(201); + done(); + }); + }); -}) +}); describe('Get /:comment_id', () => { const comments = [{ @@ -54,40 +55,40 @@ describe('Get /:comment_id', () => { body: 'comment 10', asset_id: 'asset', author_id: '123' - },{ + }, { id: 'def', body: 'comment 20', asset_id: 'asset', author_id: '456' - },{ + }, { id: 'hij', body: 'comment 30', asset_id: '456' - }] + }]; const users = [{ id: '123', display_name: 'John', - },{ + }, { id: '456', display_name: 'Paul', - }] + }]; const actions = [{ action_type: 'flag', item_id: 'abc' - },{ + }, { action_type: 'like', item_id: 'hij' - }] + }]; beforeEach(() => { return Comment.create(comments).then(() => { - return User.create(users) + return User.create(users); }).then(() => { - return Action.create(actions) - }) - }) + return Action.create(actions); + }); + }); it('should return the right comment for the comment_id', function(done){ chai.request(app) @@ -96,11 +97,8 @@ describe('Get /:comment_id', () => { .end(function(err, res){ expect(err).to.be.null; expect(res).to.have.status(200); - if (err) return done(err); + if (err) {return done(err);} done(); }); - }) - - - -}) + }); +}); diff --git a/tests/routes/api/stream/index.js b/tests/routes/api/stream/index.js index a4ca53228..9744abb0c 100644 --- a/tests/routes/api/stream/index.js +++ b/tests/routes/api/stream/index.js @@ -2,9 +2,11 @@ require('../../../utils/mongoose'); const app = require('../../../../app'); const chai = require('chai'); -const chaiHttp = require('chai-http'); -chai.use(chaiHttp); -var expect = chai.expect; +const expect = chai.expect; + +// Setup chai. +chai.should(); +chai.use(require('chai-http')); const Action = require('../../../../models/action'); const User = require('../../../../models/user'); @@ -16,40 +18,40 @@ describe('api/stream: routes', () => { body: 'comment 10', asset_id: 'asset', author_id: '123' - },{ + }, { id: 'def', body: 'comment 20', asset_id: 'asset', author_id: '456' - },{ + }, { id: 'hij', body: 'comment 30', asset_id: '456' - }] + }]; const users = [{ id: '123', display_name: 'John', - },{ + }, { id: '456', display_name: 'Paul', - }] + }]; const actions = [{ action_type: 'flag', item_id: 'abc' - },{ + }, { action_type: 'like', item_id: 'hij' - }] + }]; beforeEach(() => { return Comment.create(comments).then(() => { - return User.create(users) + return User.create(users); }).then(() => { - return Action.create(actions) - }) - }) + return Action.create(actions); + }); + }); it('should return a stream with comments, users and actions', function(done){ chai.request(app) @@ -58,8 +60,8 @@ describe('api/stream: routes', () => { .end(function(err, res){ expect(err).to.be.null; expect(res).to.have.status(200); - if (err) return done(err); + if (err) {return done(err);} done(); }); - }) -}) + }); +}); diff --git a/tests/utils/mongoose.js b/tests/utils/mongoose.js index 35083a4b3..1549440a6 100644 --- a/tests/utils/mongoose.js +++ b/tests/utils/mongoose.js @@ -1,4 +1,4 @@ -var mongoose = require('mongoose'); +const mongoose = require('../../mongoose'); // Ensure the NODE_ENV is set to 'test', // this is helpful when you would like to change behavior when testing. @@ -6,18 +6,18 @@ process.env.NODE_ENV = 'test'; beforeEach(function (done) { function clearDB() { - for (var i in mongoose.connection.collections) { + for (let i in mongoose.connection.collections) { mongoose.connection.collections[i].remove(function() {}); } return done(); } - if (mongoose.connection.readyState === 0) { - mongoose.connect('coral-talk-test', function (err) { + mongoose.on('open', function() { if (err) { throw err; } + return clearDB(); }); } else {