From c8f2dd11bada27e977beca249bf41898daa31278 Mon Sep 17 00:00:00 2001 From: kirrg001 Date: Fri, 6 Apr 2018 14:34:07 +0200 Subject: [PATCH] Fixed `post.unpublished` when deleting all content no issue - if you delete all content, we expect two events - `post.deleted` and `post.unpublished` - `post.unpublished` was never triggered, because the api implementation made use of `collection.invoke(`destroy`)` - what happened? - you fetch all posts (columns:id) - you destroy the post (only id column is available) - the model events are triggered - but you have no access to a default set of data - the result is that the event handler can't even tell if this is a post or a page - added a proper test to ensure which events are triggered --- core/server/api/db.js | 44 ++++++++++--- core/test/integration/api/api_db_spec.js | 78 +++++++++++++++++++----- 2 files changed, 97 insertions(+), 25 deletions(-) diff --git a/core/server/api/db.js b/core/server/api/db.js index 2c98625eff..a036921823 100644 --- a/core/server/api/db.js +++ b/core/server/api/db.js @@ -1,6 +1,9 @@ +'use strict'; + // # DB API // API for DB operations var Promise = require('bluebird'), + _ = require('lodash'), pipeline = require('../lib/promise/pipeline'), localUtils = require('./utils'), exporter = require('../data/export'), @@ -110,18 +113,39 @@ db = { options = options || {}; + /** + * @NOTE: + * We fetch all posts with `columns:id` to increase the speed of this endpoint. + * And if you trigger `post.destroy(..)`, this will trigger bookshelf and model events. + * But we only have to `id` available in the model. This won't work, because: + * - model layer can't trigger event e.g. `post.page` to trigger `post|page.unpublished`. + * - `onDestroyed` or `onDestroying` can contain custom logic + */ function deleteContent() { - var collections = [ - models.Post.findAll(queryOpts), - models.Tag.findAll(queryOpts) - ]; + return models.Base.transaction(function (transacting) { + queryOpts.transacting = transacting; - return Promise.each(collections, function then(Collection) { - return Collection.invokeThen('destroy', queryOpts); - }).return({db: []}) - .catch(function (err) { - throw new common.errors.GhostError({err: err}); - }); + return models.Post.findAll(queryOpts) + .then((response) => { + return Promise.map(response.models, (post) => { + return models.Post.destroy(_.merge({id: post.id}, queryOpts)); + }, {concurrency: 100}); + }) + .then(() => { + return models.Tag.findAll(queryOpts); + }) + .then((response) => { + return Promise.map(response.models, (tag) => { + return models.Tag.destroy(_.merge({id: tag.id}, queryOpts)); + }, {concurrency: 100}); + }) + .return({db: []}) + .catch((err) => { + throw new common.errors.GhostError({ + err: err + }); + }); + }); } tasks = [ diff --git a/core/test/integration/api/api_db_spec.js b/core/test/integration/api/api_db_spec.js index 6904d3b0a6..51fcc97cae 100644 --- a/core/test/integration/api/api_db_spec.js +++ b/core/test/integration/api/api_db_spec.js @@ -1,38 +1,86 @@ var should = require('should'), - testUtils = require('../../utils'), _ = require('lodash'), + sinon = require('sinon'), + testUtils = require('../../utils'), + common = require('../../../server/lib/common'), dbAPI = require('../../../server/api/db'), - models = require('../../../server/models'); + models = require('../../../server/models'), + sandbox = sinon.sandbox.create(); describe('DB API', function () { + var eventsTriggered; + + afterEach(function () { + sandbox.restore(); + }); + // Keep the DB clean before(testUtils.teardown); afterEach(testUtils.teardown); beforeEach(testUtils.setup('users:roles', 'settings', 'posts', 'subscriber', 'perms:db', 'perms:init')); + beforeEach(function () { + eventsTriggered = {}; + + sandbox.stub(common.events, 'emit').callsFake(function (eventName, eventObj) { + if (!eventsTriggered[eventName]) { + eventsTriggered[eventName] = []; + } + + eventsTriggered[eventName].push(eventObj); + }); + }); + should.exist(dbAPI); it('delete all content (owner)', function () { - return dbAPI.deleteAllContent(testUtils.context.owner).then(function (result) { - should.exist(result.db); - result.db.should.be.instanceof(Array); - result.db.should.be.empty(); - }).then(function () { - return models.Tag.findAll(testUtils.context.owner).then(function (results) { + return models.Post.findAll(testUtils.context.internal) + .then(function (results) { + results = results.toJSON(); + + results.length.should.eql(8); + + _.filter(results, {page: false, status: 'published'}).length.should.equal(4); + _.filter(results, {page: false, status: 'draft'}).length.should.equal(1); + _.filter(results, {page: false, status: 'scheduled'}).length.should.equal(1); + _.filter(results, {page: true, status: 'published'}).length.should.equal(1); + _.filter(results, {page: true, status: 'draft'}).length.should.equal(1); + }) + .then(function () { + return dbAPI.deleteAllContent(testUtils.context.owner); + }) + .then(function (result) { + should.exist(result.db); + result.db.should.be.instanceof(Array); + result.db.should.be.empty(); + + return models.Tag.findAll(testUtils.context.internal); + }) + .then(function (results) { should.exist(results); results.length.should.equal(0); - }); - }).then(function () { - return models.Post.findAll(testUtils.context.owner).then(function (results) { + + return models.Post.findAll(testUtils.context.internal); + }) + .then(function (results) { should.exist(results); results.length.should.equal(0); - }); - }).then(function () { - return models.Subscriber.findAll(testUtils.context.owner).then(function (results) { + + return models.Subscriber.findAll(testUtils.context.internal); + }) + .then(function (results) { should.exist(results); results.length.should.equal(1); + }) + .then(function () { + eventsTriggered['post.unpublished'].length.should.eql(4); + eventsTriggered['post.deleted'].length.should.eql(6); + + eventsTriggered['page.unpublished'].length.should.eql(1); + eventsTriggered['page.deleted'].length.should.eql(2); + + eventsTriggered['tag.deleted'].length.should.eql(5); }); - }); }); it('delete all content (admin)', function () {