Merge pull request #5613 from ErisDS/issue-5551-tag-pagination

Fix pagination for tags with post_count

core/server/models/base/pagination.js

@@ -3,6 +3,7 @@
 // Extends Bookshelf.Model with a `fetchPage` method. Handles everything to do with paginated requests.
 var _          = require('lodash'),
     Promise    = require('bluebird'),
+    baseUtils  = require('./utils'),
 
     defaults,
     paginationUtils,
@@ -140,36 +141,43 @@ pagination = function pagination(bookshelf) {
             // Get the table name and idAttribute for this model
             var tableName = _.result(this.constructor.prototype, 'tableName'),
                 idAttribute = _.result(this.constructor.prototype, 'idAttribute'),
-            // Create a new collection for running `this` query, ensuring we're definitely using collection,
-            // rather than model
+            // Create a new collection for running `this` query, ensuring we're using collection, rather than model
                 collection = this.constructor.collection(),
-            // Clone the base query & set up a promise to get the count of total items in the full set
-                countPromise = this.query().clone().count(tableName + '.' + idAttribute + ' as aggregate'),
+                countPromise,
                 collectionPromise,
-                self;
+                self = this;
+
+            // #### Pre count clauses
+            // Add any where or join clauses which need to be included with the aggregate query
+
+            // Clone the base query & set up a promise to get the count of total items in the full set
+            countPromise = this.query().clone().count(tableName + '.' + idAttribute + ' as aggregate');
 
             // Clone the base query into our collection
             collection._knex = this.query().clone();
 
+            // #### Post count clauses
+            // Add any where or join clauses which need to NOT be included with the aggregate query
+
             // Setup the pagination parameters so that we return the correct items from the set
             paginationUtils.query(collection, options);
 
             // Apply ordering options if they are present
-            // This is an optimisation, adding order before cloning for the count query would mean the count query
-            // was also ordered, when that is unnecessary.
-            if (options.order) {
+            if (options.order && !_.isEmpty(options.order)) {
                 _.forOwn(options.order, function (direction, property) {
                     collection.query('orderBy', tableName + '.' + property, direction);
                 });
             }
 
+            // Apply count options if they are present
+            baseUtils.collectionQuery.count(collection, options);
+
             this.resetQuery();
             if (this.relatedData) {
                 collection.relatedData = this.relatedData;
             }
 
             // ensure that our model (self) gets the correct events fired upon it
-            self = this;
             collection
                 .on('fetching', function (collection, columns, options) {
                     return self.triggerThen('fetching:collection', collection, columns, options);

core/server/models/base/utils.js

@@ -3,7 +3,26 @@
  * Parts of the model code which can be split out and unit tested
  */
 var _               = require('lodash'),
-    filtering;
+    collectionQuery,
+    filtering,
+    addPostCount;
+
+addPostCount = function addPostCount(options, itemCollection) {
+    if (options.include && options.include.indexOf('post_count') > -1) {
+        itemCollection.query('columns', 'tags.*', function (qb) {
+            qb.count('posts_tags.post_id').from('posts_tags').whereRaw('tag_id = tags.id').as('post_count');
+        });
+
+        options.withRelated = _.pull([].concat(options.withRelated), 'post_count');
+        options.include = _.pull([].concat(options.include), 'post_count');
+    }
+};
+
+collectionQuery = {
+    count: function count(collection, options) {
+        addPostCount(options, collection);
+    }
+};
 
 filtering = {
     preFetch: function preFetch(filterObjects) {
@@ -48,3 +67,5 @@ filtering = {
 };
 
 module.exports.filtering = filtering;
+module.exports.collectionQuery = collectionQuery;
+module.exports.addPostCount = addPostCount;

core/server/models/tag.js

@@ -1,21 +1,10 @@
 var _              = require('lodash'),
     ghostBookshelf = require('./base'),
     events         = require('../events'),
-
+    baseUtils      = require('./base/utils'),
     Tag,
     Tags;
 
-function addPostCount(options, obj) {
-    if (options.include && options.include.indexOf('post_count') > -1) {
-        obj.query('select', 'tags.*');
-        obj.query('count', 'posts_tags.id as post_count');
-        obj.query('leftJoin', 'posts_tags', 'tag_id', 'tags.id');
-        obj.query('groupBy', 'tag_id', 'tags.id');
-
-        options.include = _.pull([].concat(options.include), 'post_count');
-    }
-}
-
 Tag = ghostBookshelf.Model.extend({
 
     tableName: 'tags',
@@ -83,7 +72,6 @@ Tag = ghostBookshelf.Model.extend({
     },
 
     processOptions: function processOptions(itemCollection, options) {
-        addPostCount(options, itemCollection);
         return options;
     },
 
@@ -115,7 +103,7 @@ Tag = ghostBookshelf.Model.extend({
 
         var tag = this.forge(data);
 
-        addPostCount(options, tag);
+        baseUtils.addPostCount(options, tag);
 
         // Add related objects
         options.withRelated = _.union(options.withRelated, options.include);

core/test/integration/api/api_tags_spec.js

@@ -102,14 +102,42 @@ describe('Tags API', function () {
     });
 
     describe('Browse', function () {
+        beforeEach(testUtils.setup('tags'));
+
         it('can browse (internal)', function (done) {
             TagAPI.browse(testUtils.context.internal).then(function (results) {
                 should.exist(results);
                 should.exist(results.tags);
-                results.tags.length.should.be.above(0);
+                results.tags.should.have.lengthOf(15);
                 testUtils.API.checkResponse(results.tags[0], 'tag');
                 results.tags[0].created_at.should.be.an.instanceof(Date);
 
+                results.meta.pagination.should.have.property('page', 1);
+                results.meta.pagination.should.have.property('limit', 15);
+                results.meta.pagination.should.have.property('pages', 4);
+                results.meta.pagination.should.have.property('total', 55);
+                results.meta.pagination.should.have.property('next', 2);
+                results.meta.pagination.should.have.property('prev', null);
+
+                done();
+            }).catch(done);
+        });
+
+        it('can browse page 2 (internal)', function (done) {
+            TagAPI.browse(_.extend({}, testUtils.context.internal, {page: 2})).then(function (results) {
+                should.exist(results);
+                should.exist(results.tags);
+                results.tags.should.have.lengthOf(15);
+                testUtils.API.checkResponse(results.tags[0], 'tag');
+                results.tags[0].created_at.should.be.an.instanceof(Date);
+
+                results.meta.pagination.should.have.property('page', 2);
+                results.meta.pagination.should.have.property('limit', 15);
+                results.meta.pagination.should.have.property('pages', 4);
+                results.meta.pagination.should.have.property('total', 55);
+                results.meta.pagination.should.have.property('next', 3);
+                results.meta.pagination.should.have.property('prev', 1);
+
                 done();
             }).catch(done);
         });
@@ -162,15 +190,42 @@ describe('Tags API', function () {
             }).catch(done);
         });
 
-        it('with include post_count', function (done) {
+        it('can browse with include post_count', function (done) {
             TagAPI.browse({context: {user: 1}, include: 'post_count'}).then(function (results) {
                 should.exist(results);
                 should.exist(results.tags);
-                results.tags.length.should.be.above(0);
-
+                results.tags.should.have.lengthOf(15);
                 testUtils.API.checkResponse(results.tags[0], 'tag', 'post_count');
                 should.exist(results.tags[0].post_count);
 
+                results.tags[0].post_count.should.eql(2);
+                results.tags[1].post_count.should.eql(2);
+                results.meta.pagination.should.have.property('page', 1);
+                results.meta.pagination.should.have.property('limit', 15);
+                results.meta.pagination.should.have.property('pages', 4);
+                results.meta.pagination.should.have.property('total', 55);
+                results.meta.pagination.should.have.property('next', 2);
+                results.meta.pagination.should.have.property('prev', null);
+
+                done();
+            }).catch(done);
+        });
+
+        it('can browse page 4 with include post_count', function (done) {
+            TagAPI.browse({context: {user: 1}, include: 'post_count', page: 4}).then(function (results) {
+                should.exist(results);
+                should.exist(results.tags);
+                results.tags.should.have.lengthOf(10);
+                testUtils.API.checkResponse(results.tags[0], 'tag', 'post_count');
+                should.exist(results.tags[0].post_count);
+
+                results.meta.pagination.should.have.property('page', 4);
+                results.meta.pagination.should.have.property('limit', 15);
+                results.meta.pagination.should.have.property('pages', 4);
+                results.meta.pagination.should.have.property('total', 55);
+                results.meta.pagination.should.have.property('next', null);
+                results.meta.pagination.should.have.property('prev', 3);
+
                 done();
             }).catch(done);
         });

core/test/utils/index.js

@@ -3,6 +3,7 @@ var Promise       = require('bluebird'),
     _             = require('lodash'),
     fs            = require('fs-extra'),
     path          = require('path'),
+    uuid          = require('node-uuid'),
     migration     = require('../../server/data/migration/'),
     Models        = require('../../server/models'),
     SettingsAPI   = require('../../server/api/settings'),
@@ -131,6 +132,25 @@ fixtures = {
         }));
     },
 
+    insertMoreTags: function insertMoreTags(max) {
+        max = max || 50;
+        var tags = [],
+            tagName,
+            i,
+            knex = config.database.knex;
+
+        for (i = 0; i < max; i += 1) {
+            tagName = uuid.v4().split('-')[0];
+            tags.push(DataGenerator.forKnex.createBasic({name: tagName, slug: tagName}));
+        }
+
+        return sequence(_.times(tags.length, function (index) {
+            return function () {
+                return knex('tags').insert(tags[index]);
+            };
+        }));
+    },
+
     insertMorePostsTags: function insertMorePostsTags(max) {
         max = max || 50;
 
@@ -376,6 +396,7 @@ toDoList = {
 
     posts: function insertPosts() { return fixtures.insertPosts(); },
     'posts:mu': function insertMultiAuthorPosts() { return fixtures.insertMultiAuthorPosts(); },
+    tags: function insertMoreTags() { return fixtures.insertMoreTags(); },
     apps: function insertApps() { return fixtures.insertApps(); },
     settings: function populateSettings() {
         return Models.Settings.populateDefaults().then(function () { return SettingsAPI.updateSettingsCache(); });