Pagination cleanup & improvements

no issue

- switching from using fetch to fetch all means some code can be removed from the fetchPage method
- updating tests to reflect cleaner code
- ensure coverage is at 100%

core/server/models/base/pagination.js

@@ -48,12 +48,12 @@ paginationUtils = {
     /**
      * ### Query
      * Apply the necessary parameters to paginate the query
-     * @param {Bookshelf.Model|Bookshelf.Collection} itemCollection
+     * @param {bookshelf.Model} model
      * @param {options} options
      */
-    addLimitAndOffset: function addLimitAndOffset(itemCollection, options) {
+    addLimitAndOffset: function addLimitAndOffset(model, options) {
         if (_.isNumber(options.limit)) {
-            itemCollection
+            model
                 .query('limit', options.limit)
                 .query('offset', options.limit * (options.page - 1));
         }
@@ -97,26 +97,26 @@ paginationUtils = {
 /**
  * ### Pagination Object
  * @typedef {Object} pagination
- * @property {Number} `page` \- page in set to display
+ * @property {Number} page \- page in set to display
- * @property {Number|String} `limit` \- no. results per page, or 'all'
+ * @property {Number|String} limit \- no. results per page, or 'all'
- * @property {Number} `pages` \- total no. pages in the full set
+ * @property {Number} pages \- total no. pages in the full set
- * @property {Number} `total` \- total no. items in the full set
+ * @property {Number} total \- total no. items in the full set
- * @property {Number|null} `next` \- next page
+ * @property {Number|null} next \- next page
- * @property {Number|null} `prev` \- previous page
+ * @property {Number|null} prev \- previous page
  */
 
 /**
  * ### Fetch Page Options
  * @typedef {Object} options
- * @property {Number} `page` \- page in set to display
+ * @property {Number} page \- page in set to display
- * @property {Number|String} `limit` \- no. results per page, or 'all'
+ * @property {Number|String} limit \- no. results per page, or 'all'
- * @property {Object} `order` \- set of order by params and directions
+ * @property {Object} order \- set of order by params and directions
  */
 
 /**
  * ### Fetch Page Response
  * @typedef {Object} paginatedResult
- * @property {Array} `collection` \- set of results
+ * @property {Array} collection \- set of results
  * @property {pagination} pagination \- pagination metadata
  */
 
@@ -141,8 +141,6 @@ 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 using collection, rather than model
-                collection = this.constructor.collection(),
                 countPromise,
                 collectionPromise,
                 self = this;
@@ -156,48 +154,31 @@ pagination = function pagination(bookshelf) {
                 bookshelf.knex.raw('count(distinct ' + 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.addLimitAndOffset(collection, options);
+            paginationUtils.addLimitAndOffset(self, options);
 
             // Apply ordering options if they are present
             if (options.order && !_.isEmpty(options.order)) {
                 _.forOwn(options.order, function (direction, property) {
-                    collection.query('orderBy', tableName + '.' + property, direction);
+                    self.query('orderBy', tableName + '.' + property, direction);
                 });
             }
 
             if (options.groups && !_.isEmpty(options.groups)) {
                 _.each(options.groups, function (group) {
-                    collection.query('groupBy', group);
+                    self.query('groupBy', group);
                 });
             }
 
             // Apply count options if they are present
-            baseUtils.collectionQuery.count(collection, options);
+            baseUtils.collectionQuery.count(self, options);
-
-            this.resetQuery();
-            if (this.relatedData) {
-                collection.relatedData = this.relatedData;
-            }
-
-            // ensure that our model (self) gets the correct events fired upon it
-            collection
-                .on('fetching', function (collection, columns, options) {
-                    return self.triggerThen('fetching:collection', collection, columns, options);
-                })
-                .on('fetched', function (collection, resp, options) {
-                    return self.triggerThen('fetched:collection', collection, resp, options);
-                });
 
             // Setup the promise to do a fetch on our collection, running the specified query
             // @TODO: ensure option handling is done using an explicit pick elsewhere
-            collectionPromise = collection.fetch(_.omit(options, ['page', 'limit']));
+            collectionPromise = self.fetchAll(_.omit(options, ['page', 'limit']));
 
             // Resolve the two promises
             return Promise.join(collectionPromise, countPromise).then(function formatResponse(results) {

core/server/models/base/utils.js

@@ -8,9 +8,9 @@ var _ = require('lodash'),
     addPostCount,
     tagUpdate;
 
-addPostCount = function addPostCount(options, itemCollection) {
+addPostCount = function addPostCount(options, model) {
     if (options.include && options.include.indexOf('post_count') > -1) {
-        itemCollection.query('columns', 'tags.*', function (qb) {
+        model.query('columns', 'tags.*', function (qb) {
             qb.count('posts_tags.post_id').from('posts_tags').whereRaw('tag_id = tags.id').as('post_count');
         });
 
@@ -20,8 +20,8 @@ addPostCount = function addPostCount(options, itemCollection) {
 };
 
 collectionQuery = {
-    count: function count(collection, options) {
+    count: function count(model, options) {
-        addPostCount(options, collection);
+        addPostCount(options, model);
     }
 };
 

core/test/integration/api/advanced_browse_spec.js

@@ -512,5 +512,35 @@ describe('Filter Param Spec', function () {
                 done();
             });
         });
+
+        describe('Empty results', function () {
+            it('Will return empty result if tag has no posts', function (done) {
+                PostAPI.browse({filter: 'tag:no-posts', include: 'tag,author'}).then(function (result) {
+                    // 1. Result should have the correct base structure
+                    should.exist(result);
+                    result.should.have.property('posts');
+                    result.should.have.property('meta');
+
+                    // 2. The data part of the response should be correct
+                    // We should have 4 matching items
+                    result.posts.should.be.an.Array.with.lengthOf(0);
+
+                    // 3. The meta object should contain the right details
+                    result.meta.should.have.property('pagination');
+                    result.meta.pagination.should.be.an.Object.with.properties(['page', 'limit', 'pages', 'total', 'next', 'prev']);
+                    result.meta.pagination.page.should.eql(1);
+                    result.meta.pagination.limit.should.eql(15);
+                    result.meta.pagination.pages.should.eql(1);
+                    result.meta.pagination.total.should.eql(0);
+                    should.equal(result.meta.pagination.next, null);
+                    should.equal(result.meta.pagination.prev, null);
+
+                    // NOTE: new query does not have meta filter
+                    result.meta.should.not.have.property('filters');
+
+                    done();
+                }).catch(done);
+            });
+        });
     });
 });

core/test/unit/models_pagination_spec.js

@@ -175,7 +175,7 @@ describe('pagination', function () {
     });
 
     describe('fetchPage', function () {
-        var model, bookshelf, knex, on, mockQuery, fetch, colQuery;
+        var model, bookshelf, knex, mockQuery;
 
         before(function () {
             paginationUtils = pagination.__get__('paginationUtils');
@@ -195,21 +195,10 @@ describe('pagination', function () {
             mockQuery.clone.returns(mockQuery);
             mockQuery.select.returns([{aggregate: 1}]);
 
-            fetch = sandbox.stub().returns(Promise.resolve({}));
-            colQuery = sandbox.stub();
-            on = function () { return this; };
-            on = sandbox.spy(on);
-
             model = function () {};
-            model.prototype.constructor = {
+
-                collection: sandbox.stub().returns({
+            model.prototype.fetchAll = sandbox.stub().returns(Promise.resolve({}));
-                    on: on,
-                    fetch: fetch,
-                    query: colQuery
-                })
-            };
             model.prototype.query = sandbox.stub();
-            model.prototype.resetQuery = sandbox.stub();
             model.prototype.query.returns(mockQuery);
 
             knex = {raw: sandbox.stub().returns(Promise.resolve())};
@@ -230,16 +219,11 @@ describe('pagination', function () {
             bookshelf.Model.prototype.fetchPage().then(function () {
                 sinon.assert.callOrder(
                     paginationUtils.parseOptions,
-                    model.prototype.constructor.collection,
                     model.prototype.query,
                     mockQuery.clone,
                     mockQuery.select,
-                    model.prototype.query,
-                    mockQuery.clone,
                     paginationUtils.addLimitAndOffset,
-                    on,
+                    model.prototype.fetchAll,
-                    on,
-                    fetch,
                     paginationUtils.formatResponse
                 );
 
@@ -249,26 +233,17 @@ describe('pagination', function () {
                 paginationUtils.addLimitAndOffset.calledOnce.should.be.true;
                 paginationUtils.formatResponse.calledOnce.should.be.true;
 
-                model.prototype.constructor.collection.calledOnce.should.be.true;
+                model.prototype.query.calledOnce.should.be.true;
-                model.prototype.constructor.collection.calledWith().should.be.true;
-
-                model.prototype.query.calledTwice.should.be.true;
                 model.prototype.query.firstCall.calledWith().should.be.true;
-                model.prototype.query.secondCall.calledWith().should.be.true;
 
-                mockQuery.clone.calledTwice.should.be.true;
+                mockQuery.clone.calledOnce.should.be.true;
                 mockQuery.clone.firstCall.calledWith().should.be.true;
-                mockQuery.clone.secondCall.calledWith().should.be.true;
 
                 mockQuery.select.calledOnce.should.be.true;
                 mockQuery.select.calledWith().should.be.true;
 
-                on.calledTwice.should.be.true;
+                model.prototype.fetchAll.calledOnce.should.be.true;
-                on.firstCall.calledWith('fetching').should.be.true;
+                model.prototype.fetchAll.calledWith({}).should.be.true;
-                on.secondCall.calledWith('fetched').should.be.true;
-
-                fetch.calledOnce.should.be.true;
-                fetch.calledWith({}).should.be.true;
 
                 done();
             }).catch(done);
@@ -276,22 +251,17 @@ describe('pagination', function () {
 
         it('fetchPage calls all paginationUtils and methods when order set', function (done) {
             var orderOptions = {order: {id: 'DESC'}};
-
             paginationUtils.parseOptions.returns(orderOptions);
+
             bookshelf.Model.prototype.fetchPage(orderOptions).then(function () {
                 sinon.assert.callOrder(
                     paginationUtils.parseOptions,
-                    model.prototype.constructor.collection,
                     model.prototype.query,
                     mockQuery.clone,
                     mockQuery.select,
-                    model.prototype.query,
-                    mockQuery.clone,
                     paginationUtils.addLimitAndOffset,
-                    colQuery,
+                    model.prototype.query,
-                    on,
+                    model.prototype.fetchAll,
-                    on,
-                    fetch,
                     paginationUtils.formatResponse
                 );
 
@@ -301,29 +271,57 @@ describe('pagination', function () {
                 paginationUtils.addLimitAndOffset.calledOnce.should.be.true;
                 paginationUtils.formatResponse.calledOnce.should.be.true;
 
-                model.prototype.constructor.collection.calledOnce.should.be.true;
-                model.prototype.constructor.collection.calledWith().should.be.true;
-
                 model.prototype.query.calledTwice.should.be.true;
                 model.prototype.query.firstCall.calledWith().should.be.true;
-                model.prototype.query.secondCall.calledWith().should.be.true;
+                model.prototype.query.secondCall.calledWith('orderBy', 'undefined.id', 'DESC').should.be.true;
 
-                mockQuery.clone.calledTwice.should.be.true;
+                mockQuery.clone.calledOnce.should.be.true;
                 mockQuery.clone.firstCall.calledWith().should.be.true;
-                mockQuery.clone.secondCall.calledWith().should.be.true;
 
                 mockQuery.select.calledOnce.should.be.true;
                 mockQuery.select.calledWith().should.be.true;
 
-                colQuery.calledOnce.should.be.true;
+                model.prototype.fetchAll.calledOnce.should.be.true;
-                colQuery.calledWith('orderBy', 'undefined.id', 'DESC').should.be.true;
+                model.prototype.fetchAll.calledWith(orderOptions).should.be.true;
 
-                on.calledTwice.should.be.true;
+                done();
-                on.firstCall.calledWith('fetching').should.be.true;
+            }).catch(done);
-                on.secondCall.calledWith('fetched').should.be.true;
+        });
 
-                fetch.calledOnce.should.be.true;
+        it('fetchPage calls all paginationUtils and methods when group by set', function (done) {
-                fetch.calledWith(orderOptions).should.be.true;
+            var groupOptions = {groups: ['posts.id']};
+            paginationUtils.parseOptions.returns(groupOptions);
+
+            bookshelf.Model.prototype.fetchPage(groupOptions).then(function () {
+                sinon.assert.callOrder(
+                    paginationUtils.parseOptions,
+                    model.prototype.query,
+                    mockQuery.clone,
+                    mockQuery.select,
+                    paginationUtils.addLimitAndOffset,
+                    model.prototype.query,
+                    model.prototype.fetchAll,
+                    paginationUtils.formatResponse
+                );
+
+                paginationUtils.parseOptions.calledOnce.should.be.true;
+                paginationUtils.parseOptions.calledWith(groupOptions).should.be.true;
+
+                paginationUtils.addLimitAndOffset.calledOnce.should.be.true;
+                paginationUtils.formatResponse.calledOnce.should.be.true;
+
+                model.prototype.query.calledTwice.should.be.true;
+                model.prototype.query.firstCall.calledWith().should.be.true;
+                model.prototype.query.secondCall.calledWith('groupBy', 'posts.id').should.be.true;
+
+                mockQuery.clone.calledOnce.should.be.true;
+                mockQuery.clone.firstCall.calledWith().should.be.true;
+
+                mockQuery.select.calledOnce.should.be.true;
+                mockQuery.select.calledWith().should.be.true;
+
+                model.prototype.fetchAll.calledOnce.should.be.true;
+                model.prototype.fetchAll.calledWith(groupOptions).should.be.true;
 
                 done();
             }).catch(done);
@@ -340,5 +338,20 @@ describe('pagination', function () {
                 done();
             });
         });
+
+        it('fetchPage returns expected response even when aggregate is empty', function (done) {
+            // override aggregate response
+            mockQuery.select.returns([]);
+            paginationUtils.parseOptions.returns({});
+
+            bookshelf.Model.prototype.fetchPage().then(function (result) {
+                result.should.have.ownProperty('collection');
+                result.should.have.ownProperty('pagination');
+                result.collection.should.be.an.Object;
+                result.pagination.should.be.an.Object;
+
+                done();
+            });
+        });
     });
 });