diff --git a/models/comment.js b/models/comment.js index 9bd9163c4..3f8156b24 100644 --- a/models/comment.js +++ b/models/comment.js @@ -137,27 +137,18 @@ CommentSchema.statics.findByAssetId = (asset_id) => Comment.find({ }); /** - * Finds the accepted comments by the asset_id get the comments that are - * accepted. - * @param {String} asset_id identifier of the asset which owns the comments (uuid) - * @return {Promise} + * findByAssetIdWithStatuses finds all the comments where the asset id matches + * what's provided and the status is one of the ones listed in the statuses + * array. + * @param {String} asset_id the asset id to search by + * @param {Array} [statuses=[]] the array of statuses to search by + * @return {Promise} resolves to an array of comments */ -CommentSchema.statics.findAcceptedByAssetId = (asset_id) => Comment.find({ +CommentSchema.statics.findByAssetIdWithStatuses = (asset_id, statuses = []) => Comment.find({ asset_id, - status: 'accepted' -}); - -/** - * Finds the new and accepted comments by the asset_id. - * @param {String} asset_id identifier of the asset which owns the comments (uuid) - * @return {Promise} - */ -CommentSchema.statics.findAcceptedAndNewByAssetId = (asset_id) => Comment.find({ - asset_id, - $or: [ - {status: 'accepted'}, - {status: null} - ] + status: { + $in: statuses + } }); /** @@ -198,19 +189,12 @@ CommentSchema.statics.findByStatus = (status = null) => { */ CommentSchema.statics.moderationQueue = (status, asset_id = null) => { - /** - * This adds the asset_id requirement to the query if the asset_id is defined. - */ - const assetIDWrap = (query) => { - if (asset_id) { - query = query.where('asset_id', asset_id); - } + // Fetch the comments with statuses. + let comments = Comment.findByStatus(status); - return query; - }; - - // Pre-moderation: New comments are shown in the moderator queues immediately. - let comments = assetIDWrap(Comment.findByStatus(status)); + if (asset_id) { + comments = comments.where('asset_id', asset_id); + } return comments; }; diff --git a/routes/api/stream/index.js b/routes/api/stream/index.js index 540e36e52..06845a21a 100644 --- a/routes/api/stream/index.js +++ b/routes/api/stream/index.js @@ -48,20 +48,11 @@ router.get('/', (req, res, next) => { settings.merge(asset.settings); } - // Fetch the appropriate comments stream. - let comments; - - if (settings.moderation === 'pre') { - comments = Comment.findAcceptedByAssetId(asset.id); - } else { - comments = Comment.findAcceptedAndNewByAssetId(asset.id); - } - return Promise.all([ // This is the promised component... Fetch the comments based on the // moderation settings. - comments, + Comment.findByAssetIdWithStatuses(asset.id, [null, 'accepted']), // Send back the reference to the asset. asset, diff --git a/tests/models/comment.js b/tests/models/comment.js index 20590dc3d..455b937f3 100644 --- a/tests/models/comment.js +++ b/tests/models/comment.js @@ -161,28 +161,6 @@ describe('models.Comment', () => { expect(result[2]).to.have.property('body', 'comment 40'); }); }); - - it('should find an array of accepted comments by asset id', () => { - return Comment.findAcceptedByAssetId('123').then((result) => { - expect(result).to.have.length(1); - result.sort((a, b) => { - if (a.body < b.body) {return -1;} - else {return 1;} - }); - expect(result[0]).to.have.property('body', 'comment 20'); - }); - }); - - it('should find an array of new and accepted comments by asset id', () => { - return Comment.findAcceptedAndNewByAssetId('123').then((result) => { - expect(result).to.have.length(2); - result.sort((a, b) => { - if (a.body < b.body) {return -1;} - else {return 1;} - }); - expect(result[0]).to.have.property('body', 'comment 10'); - }); - }); }); describe('#moderationQueue()', () => { diff --git a/tests/routes/api/comments/index.js b/tests/routes/api/comments/index.js index 5edb52099..7a3a2c592 100644 --- a/tests/routes/api/comments/index.js +++ b/tests/routes/api/comments/index.js @@ -192,6 +192,7 @@ describe('/api/v1/comments', () => { .then((res) => { expect(res).to.have.status(201); expect(res.body).to.have.property('id'); + expect(res.body).to.have.property('status', 'premod'); }); }); @@ -214,6 +215,7 @@ describe('/api/v1/comments', () => { expect(res).to.have.status(201); expect(res.body).to.have.property('id'); expect(res.body).to.have.property('status', null); + return Promise.all([ res.body, Action.findByType('flag', 'comments') @@ -252,6 +254,27 @@ describe('/api/v1/comments', () => { }); }); + it('should create a comment with null status if it\'s asset is has post-moderation enabled', () => { + return Asset + .findOrCreateByUrl('https://coralproject.net/article1') + .then((asset) => { + return Asset + .overrideSettings(asset.id, {moderation: 'post'}) + .then(() => asset); + }) + .then((asset) => { + return chai.request(app).post('/api/v1/comments') + .set(passport.inject({roles: []})) + .send({'body': 'Something body.', 'author_id': '123', 'asset_id': asset.id, 'parent_id': ''}); + }) + .then((res) => { + expect(res).to.have.status(201); + expect(res.body).to.have.property('id'); + expect(res.body).to.have.property('asset_id'); + expect(res.body).to.have.property('status', null); + }); + }); + it('should create a rejected comment if the body is above the character count', () => { return Asset .findOrCreateByUrl('https://coralproject.net/article1') diff --git a/tests/routes/api/stream/index.js b/tests/routes/api/stream/index.js index d9b59d6fe..104801996 100644 --- a/tests/routes/api/stream/index.js +++ b/tests/routes/api/stream/index.js @@ -24,6 +24,21 @@ describe('/api/v1/stream', () => { } }; + const assets = [ + { + url: 'https://example.com/article/1' + }, + { + url: 'https://example.com/article/2', + settings: { + moderation: 'pre' + } + }, + { + url: 'https://example.com/article/3' + } + ]; + const comments = [{ id: 'abc', body: 'comment 10', @@ -58,6 +73,28 @@ describe('/api/v1/stream', () => { status_history: [{ type: 'rejected' }] + }, { + body: 'comment 50', + status: 'premod', + status_history: [{ + type: 'premod' + }] + }, { + body: 'comment 60', + status: 'accepted', + status_history: [{ + type: 'accepted' + }] + }, { + body: 'comment 70', + status: 'rejected', + status_history: [{ + type: 'rejected' + }] + }, { + body: 'comment 70', + status: null, + status_history: [] }]; const users = [{ @@ -79,42 +116,46 @@ describe('/api/v1/stream', () => { }]; beforeEach(() => { - return Setting.init(settings).then(() => { - return Promise.all([ - User.createLocalUsers(users), - Asset.findOrCreateByUrl('http://test.com'), - Asset - .findOrCreateByUrl('http://coralproject.net/asset2') - .then((asset) => { - return Asset - .overrideSettings(asset.id, {moderation: 'pre'}) - .then(() => asset); - }) - ]) - .then(([users, asset1, asset2]) => { + return Setting.init(settings) + .then(() => Promise.all([ + User.createLocalUsers(users), + Promise.all(assets.map((asset) => Asset.create(asset))) + ])) + .then(([mockUsers, mockAssets]) => { - comments[0].author_id = users[0].id; - comments[1].author_id = users[1].id; - comments[2].author_id = users[0].id; - comments[3].author_id = users[1].id; - - comments[0].asset_id = asset1.id; - comments[1].asset_id = asset1.id; - comments[2].asset_id = asset2.id; - comments[3].asset_id = asset2.id; - - return Promise.all([ - Comment.create(comments), - Action.create(actions) - ]); + // Map the id's over. + mockAssets.forEach((asset, i) => { + assets[i].id = asset.id; }); + + mockUsers.forEach((user, i) => { + users[i].id = user.id; + }); + + comments.forEach((comment, i) => { + comments[i].author_id = users[(i % 2) === 0 ? 0 : 1].id; + }); + + comments[0].asset_id = assets[0].id; + comments[1].asset_id = assets[0].id; + comments[2].asset_id = assets[1].id; + comments[3].asset_id = assets[1].id; + comments[4].asset_id = assets[2].id; + comments[5].asset_id = assets[2].id; + comments[6].asset_id = assets[2].id; + comments[7].asset_id = assets[2].id; + + return Promise.all([ + Comment.create(comments), + Action.create(actions) + ]); }); }); it('should return a stream with comments, users and actions for an existing asset', () => { return chai.request(app) .get('/api/v1/stream') - .query({'asset_url': 'http://test.com'}) + .query({asset_url: assets[0].url}) .then(res => { expect(res).to.have.status(200); expect(res.body.assets.length).to.equal(1); @@ -138,15 +179,47 @@ describe('/api/v1/stream', () => { it('should merge the settings when the asset contains settings to override it with', () => { return chai.request(app) .get('/api/v1/stream') - .query({'asset_url': 'http://coralproject.net/asset2'}) + .query({asset_url: assets[1].url}) .then((res) => { expect(res).to.have.status(200); - expect(res.body.assets.length).to.equal(1); - expect(res.body.comments.length).to.equal(1); - expect(res.body.users.length).to.equal(1); + expect(res.body.assets).to.have.length(1); + expect(res.body.comments).to.have.length(1); + expect(res.body.users).to.have.length(1); expect(res.body.settings).to.have.property('moderation', 'pre'); expect(res.body.settings).to.not.have.property('wordlist'); }); }); + + it('should not change the previously displayed comments based on moderation state changes', () => { + + let preComments, postComments; + + return chai.request(app) + .get('/api/v1/stream') + .query({asset_url: assets[2].url}) + .then((res) => { + expect(res).to.have.status(200); + expect(res.body.comments.length).to.equal(2); + expect(res.body.settings).to.have.property('moderation', 'post'); + + preComments = res.body.comments; + + return Asset.overrideSettings(assets[2].id, {moderation: 'pre'}); + }) + .then(() => { + return chai.request(app) + .get('/api/v1/stream') + .query({asset_url: assets[2].url}); + }) + .then((res) => { + expect(res).to.have.status(200); + expect(res.body.comments.length).to.equal(2); + expect(res.body.settings).to.have.property('moderation', 'pre'); + + postComments = res.body.comments; + + expect(preComments).to.deep.equal(postComments); + }); + }); }); }); diff --git a/util.js b/util.js index 6907d90cd..04c822867 100644 --- a/util.js +++ b/util.js @@ -38,5 +38,6 @@ util.onshutdown = (jobs) => { // Attach to the SIGTERM + SIGINT handles to ensure a clean shutdown in the // event that we have an external event. -process.on('SIGTERM', () => util.shutdown()); -process.on('SIGINT', () => util.shutdown()); +process.on('SIGTERM', () => util.shutdown()); +process.on('SIGINT', () => util.shutdown()); +process.once('SIGUSR2', () => util.shutdown());