Switch frontend controller to use new filter param

refs #5943, #5091

- updated fetch-data to handle multiple api queries
- using named keys for queries so that the names of items in the result are correct (tag instead of tags etc)
- updated channel configs in frontend controller
- removed old filter code from frontend controller
- added test coverage for fetch-data and format-response
- fixes / removes tests which are broken by the refactor

core/server/controllers/frontend/fetch-data.js

@@ -1,16 +1,122 @@
-var api = require('../../api');
+/**
+ * # Fetch Data
+ * Dynamically build and execute queries on the API for channels
+ */
+var api = require('../../api'),
+    _   = require('lodash'),
+    Promise = require('bluebird'),
+    queryDefaults,
+    defaultPostQuery = {};
+
+// The default settings for a default post query
+queryDefaults = {
+    type: 'browse',
+    resource: 'posts',
+    options: {}
+};
+
+// Default post query needs to always include author & tags
+_.extend(defaultPostQuery, queryDefaults, {
+    options: {
+        include: 'author,tags,fields'
+    }
+});
+
+/**
+ * ## Fetch Posts Per page
+ * Grab the postsPerPage setting from the database
+ *
+ * @param {Object} options
+ * @returns {Object} postOptions
+ */
+function fetchPostsPerPage(options) {
+    options = options || {};
 
-function fetchData(options) {
     return api.settings.read('postsPerPage').then(function then(response) {
-        var postPP = response.settings[0],
-            postsPerPage = parseInt(postPP.value, 10);
+        var postsPerPage = parseInt(response.settings[0].value);
 
         // No negative posts per page, must be number
         if (!isNaN(postsPerPage) && postsPerPage > 0) {
             options.limit = postsPerPage;
         }
-        options.include = 'author,tags,fields';
-        return api.posts.browse(options);
+
+        // Ensure the options key is present, so this can be merged with other options
+        return {options: options};
+    });
+}
+
+/**
+ * @typedef query
+ * @
+ */
+
+/**
+ * ## Process Query
+ * Takes a 'query' object, ensures that type, resource and options are set
+ * Replaces occurrences of `%s` in options with slugParam
+ * Converts the query config to a promise for the result
+ *
+ * @param {{type: String, resource: String, options: Object}} query
+ * @param {String} slugParam
+ * @returns {Promise} promise for an API call
+ */
+function processQuery(query, slugParam) {
+    query = _.cloneDeep(query);
+
+    // Ensure that all the properties are filled out
+    _.defaultsDeep(query, queryDefaults);
+
+    // Replace any slugs
+    _.each(query.options, function (option, name) {
+        query.options[name] = _.isString(option) ? option.replace(/%s/g, slugParam) : option;
+    });
+
+    // Return a promise for the api query
+    return api[query.resource][query.type](query.options);
+}
+
+/**
+ * ## Fetch Data
+ * Calls out to get posts per page, builds the final posts query & builds any additional queries
+ * Wraps the queries using Promise.props to ensure it gets named responses
+ * Does a first round of formatting on the response, and returns
+ *
+ * @param {Object} channelOptions
+ * @param {String} slugParam
+ * @returns {Promise} response
+ */
+function fetchData(channelOptions, slugParam) {
+    return fetchPostsPerPage(channelOptions.postOptions).then(function fetchData(pageOptions) {
+        var postQuery,
+            props = {};
+
+        // All channels must have a posts query, use the default if not provided
+        postQuery = _.defaultsDeep({}, pageOptions, defaultPostQuery);
+        props.posts = processQuery(postQuery, slugParam);
+
+        _.each(channelOptions.data, function (query, name) {
+            props[name] = processQuery(query, slugParam);
+        });
+
+        return Promise.props(props).then(function formatResponse(results) {
+            var response = _.cloneDeep(results.posts);
+            delete results.posts;
+
+            // process any remaining data
+            if (!_.isEmpty(results)) {
+                response.data = {};
+
+                _.each(results, function (result, name) {
+                    if (channelOptions.data[name].type === 'browse') {
+                        response.data[name] = result;
+                    } else {
+                        response.data[name] = result[channelOptions.data[name].resource];
+                    }
+                });
+            }
+
+            return response;
+        });
     });
 }
 

core/server/controllers/frontend/format-response.js

@@ -5,14 +5,25 @@ var _   = require('lodash');
  * If extraValues are available, they are merged in the final value
  * @return {Object} containing page variables
  */
-function formatPageResponse(posts, page, extraValues) {
-    extraValues = extraValues || {};
-
-    var resp = {
-        posts: posts,
-        pagination: page.meta.pagination
+function formatPageResponse(result) {
+    var response = {
+        posts: result.posts,
+        pagination: result.meta.pagination
     };
-    return _.extend(resp, extraValues);
+
+    _.each(result.data, function (data, name) {
+        if (data.meta) {
+            // Move pagination to be a top level key
+            response[name] = data;
+            response[name].pagination = data.meta.pagination;
+            delete response[name].meta;
+        } else {
+            // This is a single object, don't wrap it in an array
+            response[name] = data[0];
+        }
+    });
+
+    return response;
 }
 
 /**

core/server/controllers/frontend/index.js

@@ -44,27 +44,23 @@ function renderPost(req, res) {
 }
 
 function renderChannel(channelOpts) {
-    channelOpts = channelOpts || {};
+    // Ensure we at least have an empty object for postOptions
+    channelOpts.postOptions = channelOpts.postOptions || {};
 
     return function renderChannel(req, res, next) {
+        // Parse the parameters we need from the URL
         var pageParam = req.params.page !== undefined ? parseInt(req.params.page, 10) : 1,
-            options = {
-                page: pageParam
-            },
-            hasSlug,
-            filter, filterKey;
+            slugParam = req.params.slug ? safeString(req.params.slug) : undefined;
 
-        // Add the slug if it exists in the route
-        if (channelOpts.route.indexOf(':slug') !== -1 && req.params.slug) {
-            options[channelOpts.name] = safeString(req.params.slug);
-            hasSlug = true;
-        }
+        // Set page on postOptions for the query made later
+        channelOpts.postOptions.page = pageParam;
 
+        // @TODO this should really use the url building code in config.url
         function createUrl(page) {
             var url = config.paths.subdir + channelOpts.route;
 
-            if (hasSlug) {
-                url = url.replace(':slug', options[channelOpts.name]);
+            if (slugParam) {
+                url = url.replace(':slug', slugParam);
             }
 
             if (page && page > 1) {
@@ -74,49 +70,41 @@ function renderChannel(channelOpts) {
             return url;
         }
 
+        // If the page parameter isn't something sensible, redirect
         if (isNaN(pageParam) || pageParam < 1 || (req.params.page !== undefined && pageParam === 1)) {
             return res.redirect(createUrl());
         }
 
-        return fetchData(options).then(function then(page) {
+        // Call fetchData to get everything we need from the API
+        return fetchData(channelOpts, slugParam).then(function handleResult(result) {
             // If page is greater than number of pages we have, redirect to last page
-            if (pageParam > page.meta.pagination.pages) {
-                return res.redirect(createUrl(page.meta.pagination.pages));
+            if (pageParam > result.meta.pagination.pages) {
+                return res.redirect(createUrl(result.meta.pagination.pages));
             }
 
-            setRequestIsSecure(req, page.posts);
-            if (channelOpts.filter && page.meta.filters[channelOpts.filter]) {
-                filterKey = page.meta.filters[channelOpts.filter];
-                filter = (_.isArray(filterKey)) ? filterKey[0] : filterKey;
-                setRequestIsSecure(req, filter);
-            }
+            // @TODO: figure out if this can be removed, it's supposed to ensure that absolutely URLs get generated
+            // correctly for the various objects, but I believe it doesn't work and a different approach is needed.
+            setRequestIsSecure(req, result.posts);
+            _.each(result.data, function (data) {
+                setRequestIsSecure(req, data);
+            });
 
-            filters.doFilter('prePostsRender', page.posts, res.locals).then(function then(posts) {
+            // @TODO: properly design these filters
+            filters.doFilter('prePostsRender', result.posts, res.locals).then(function then(posts) {
                 getActiveThemePaths().then(function then(paths) {
-                    var view = 'index',
-                        result,
-                        extra = {};
-
+                    // Calculate which template to use to render the data
+                    var view = 'index';
                     if (channelOpts.firstPageTemplate && paths.hasOwnProperty(channelOpts.firstPageTemplate + '.hbs')) {
                         view = (pageParam > 1) ? 'index' : channelOpts.firstPageTemplate;
                     } else if (channelOpts.slugTemplate) {
-                        view = template.getThemeViewForChannel(paths, channelOpts.name, options[channelOpts.name]);
+                        view = template.getThemeViewForChannel(paths, channelOpts.name, slugParam);
                     } else if (paths.hasOwnProperty(channelOpts.name + '.hbs')) {
                         view = channelOpts.name;
                     }
 
-                    if (channelOpts.filter) {
-                        extra[channelOpts.name] = (filterKey) ? filter : '';
-
-                        if (!extra[channelOpts.name]) {
-                            return next();
-                        }
-
-                        result = formatResponse.channel(posts, page, extra);
-                    } else {
-                        result = formatResponse.channel(posts, page);
-                    }
-
+                    // Do final data formatting and then render
+                    result.posts = posts;
+                    result = formatResponse.channel(result);
                     setResponseContext(req, res);
                     res.render(view, result);
                 });
@@ -134,13 +122,31 @@ frontendControllers = {
     tag: renderChannel({
         name: 'tag',
         route: '/' + config.routeKeywords.tag + '/:slug/',
-        filter: 'tags',
+        postOptions: {
+            filter: 'tags:%s'
+        },
+        data: {
+            tag: {
+                type: 'read',
+                resource: 'tags',
+                options: {slug: '%s'}
+            }
+        },
         slugTemplate: true
     }),
     author: renderChannel({
         name: 'author',
         route: '/' + config.routeKeywords.author + '/:slug/',
-        filter: 'author',
+        postOptions: {
+            filter: 'author:%s'
+        },
+        data: {
+            author: {
+                type: 'read',
+                resource: 'users',
+                options: {slug: '%s'}
+            }
+        },
         slugTemplate: true
     }),
     preview: function preview(req, res, next) {

core/test/functional/routes/frontend_spec.js

@@ -515,6 +515,8 @@ describe('Frontend Routing', function () {
             testUtils.clearData().then(function () {
                 // we initialise data, but not a user. No user should be required for navigating the frontend
                 return testUtils.initData();
+            }).then(function () {
+                return testUtils.fixtures.overrideOwnerUser('ghost-owner');
             }).then(function () {
                 return testUtils.fixtures.insertPosts();
             }).then(function () {

core/test/unit/controllers/frontend/fetch-data_spec.js

@@ -12,10 +12,15 @@ var should   = require('should'),
 
 describe('fetchData', function () {
     var apiSettingsStub,
-        apiPostsStub;
+        apiPostsStub,
+        apiTagStub,
+        apiUserStub;
 
     beforeEach(function () {
-        apiPostsStub = sandbox.stub(api.posts, 'browse').returns(new Promise.resolve({}));
+        apiPostsStub = sandbox.stub(api.posts, 'browse')
+            .returns(new Promise.resolve({posts: [], meta: {pagination: {}}}));
+        apiTagStub = sandbox.stub(api.tags, 'read').returns(new Promise.resolve({tags: []}));
+        apiUserStub = sandbox.stub(api.users, 'read').returns(new Promise.resolve({users: []}));
         apiSettingsStub = sandbox.stub(api.settings, 'read');
     });
 
@@ -23,6 +28,136 @@ describe('fetchData', function () {
         sandbox.restore();
     });
 
+    describe('channel config', function () {
+        beforeEach(function () {
+            apiSettingsStub.withArgs('postsPerPage').returns(Promise.resolve({
+                settings: [{
+                    key: 'postsPerPage',
+                    value: '10'
+                }]
+            }));
+        });
+
+        it('should handle no post options', function (done) {
+            fetchData({}).then(function (result) {
+                should.exist(result);
+                result.should.be.an.Object.with.properties('posts', 'meta');
+                result.should.not.have.property('data');
+
+                apiSettingsStub.calledOnce.should.be.true;
+                apiPostsStub.calledOnce.should.be.true;
+                apiPostsStub.firstCall.args[0].should.be.an.Object;
+                apiPostsStub.firstCall.args[0].should.have.property('include');
+                apiPostsStub.firstCall.args[0].should.have.property('limit', 10);
+
+                done();
+            }).catch(done);
+        });
+
+        it('should handle post options with only page', function (done) {
+            fetchData({postOptions: {page: 2}}).then(function (result) {
+                should.exist(result);
+                result.should.be.an.Object.with.properties('posts', 'meta');
+                result.should.not.have.property('data');
+
+                apiSettingsStub.calledOnce.should.be.true;
+                apiPostsStub.calledOnce.should.be.true;
+                apiPostsStub.firstCall.args[0].should.be.an.Object;
+                apiPostsStub.firstCall.args[0].should.have.property('include');
+                apiPostsStub.firstCall.args[0].should.have.property('limit', 10);
+                apiPostsStub.firstCall.args[0].should.have.property('page', 2);
+
+                done();
+            }).catch(done);
+        });
+
+        it('should handle multiple queries', function (done) {
+            var channelOpts = {
+                data: {
+                    featured: {
+                        type: 'browse',
+                        resource: 'posts',
+                        options: {filter: 'featured:true', limit: 3}
+                    }
+                }
+            };
+            fetchData(channelOpts).then(function (result) {
+                should.exist(result);
+                result.should.be.an.Object.with.properties('posts', 'meta', 'data');
+                result.data.should.be.an.Object.with.properties('featured');
+                result.data.featured.should.be.an.Object.with.properties('posts', 'meta');
+                result.data.featured.should.not.have.properties('data');
+
+                apiSettingsStub.calledOnce.should.be.true;
+                apiPostsStub.calledTwice.should.be.true;
+                apiPostsStub.firstCall.args[0].should.have.property('include', 'author,tags,fields');
+                apiPostsStub.firstCall.args[0].should.have.property('limit', 10);
+                apiPostsStub.secondCall.args[0].should.have.property('filter', 'featured:true');
+                apiPostsStub.secondCall.args[0].should.have.property('limit', 3);
+                done();
+            }).catch(done);
+        });
+
+        it('should handle multiple queries with page param', function (done) {
+            var channelOpts = {
+                postOptions: {page: 2},
+                data: {
+                    featured: {
+                        type: 'browse',
+                        resource: 'posts',
+                        options: {filter: 'featured:true', limit: 3}
+                    }
+                }
+            };
+            fetchData(channelOpts).then(function (result) {
+                should.exist(result);
+
+                result.should.be.an.Object.with.properties('posts', 'meta', 'data');
+                result.data.should.be.an.Object.with.properties('featured');
+                result.data.featured.should.be.an.Object.with.properties('posts', 'meta');
+                result.data.featured.should.not.have.properties('data');
+
+                apiSettingsStub.calledOnce.should.be.true;
+                apiPostsStub.calledTwice.should.be.true;
+                apiPostsStub.firstCall.args[0].should.have.property('include', 'author,tags,fields');
+                apiPostsStub.firstCall.args[0].should.have.property('limit', 10);
+                apiPostsStub.firstCall.args[0].should.have.property('page', 2);
+                apiPostsStub.secondCall.args[0].should.have.property('filter', 'featured:true');
+                apiPostsStub.secondCall.args[0].should.have.property('limit', 3);
+                done();
+            }).catch(done);
+        });
+
+        it('should handle queries with slug replacements', function (done) {
+            var channelOpts = {
+                postOptions: {
+                    filter: 'tags:%s'
+                },
+                data: {
+                    tag: {
+                        type: 'read',
+                        resource: 'tags',
+                        options: {slug: '%s'}
+                    }
+                }
+            },
+                slugParam = 'testing';
+
+            fetchData(channelOpts, slugParam).then(function (result) {
+                should.exist(result);
+                result.should.be.an.Object.with.properties('posts', 'meta', 'data');
+                result.data.should.be.an.Object.with.properties('tag');
+
+                apiSettingsStub.calledOnce.should.be.true;
+                apiPostsStub.calledOnce.should.be.true;
+                apiPostsStub.firstCall.args[0].should.have.property('include');
+                apiPostsStub.firstCall.args[0].should.have.property('limit', 10);
+                apiTagStub.firstCall.args[0].should.have.property('slug', 'testing');
+                done();
+            }).catch(done);
+        });
+    });
+
     describe('valid postsPerPage', function () {
         beforeEach(function () {
             apiSettingsStub.withArgs('postsPerPage').returns(Promise.resolve({
@@ -43,15 +178,6 @@ describe('fetchData', function () {
                 done();
             }).catch(done);
         });
-
-        it('Throws error if no options are passed', function (done) {
-            fetchData().then(function () {
-                done('Should have thrown an error here');
-            }).catch(function (err) {
-                should.exist(err);
-                done();
-            });
-        });
     });
 
     describe('invalid postsPerPage', function () {

core/test/unit/controllers/frontend/format-response_spec.js

@@ -0,0 +1,71 @@
+/*globals describe, it*/
+var should   = require('should'),
+
+    // Stuff we are testing
+    formatResponse = require('../../../../server/controllers/frontend/format-response');
+
+// To stop jshint complaining
+should.equal(true, true);
+
+describe('formatResponse', function () {
+    describe('single', function () {
+        it('should return the post object wrapped in a post key', function () {
+            var formatted,
+                postObject = {slug: 'sluggy-thing'};
+
+            formatted = formatResponse.single(postObject);
+
+            formatted.should.be.an.Object.with.property('post');
+            formatted.post.should.eql(postObject);
+        });
+    });
+
+    describe('channel', function () {
+        it('should return posts and posts pagination as top level keys', function () {
+            var formatted,
+                data = {
+                    posts: [{slug: 'a-post'}],
+                    meta: {pagination: {}}
+                };
+
+            formatted = formatResponse.channel(data);
+
+            formatted.should.be.an.Object.with.properties('posts', 'pagination');
+            formatted.posts.should.eql(data.posts);
+            formatted.pagination.should.eql(data.meta.pagination);
+        });
+
+        it('should flatten api read responses which have no pagination data', function () {
+            var formatted,
+                data = {
+                    posts: [{slug: 'a-post'}],
+                    meta: {pagination: {}},
+                    data: {tag: [{name: 'video', slug: 'video', id: 1}]}
+                };
+
+            formatted = formatResponse.channel(data);
+
+            formatted.should.be.an.Object.with.properties('posts', 'pagination', 'tag');
+            formatted.tag.should.eql(data.data.tag[0]);
+        });
+
+        it('should remove the meta key from api browse responses', function () {
+            var formatted,
+                data = {
+                    posts: [{slug: 'a-post'}],
+                    meta: {pagination: {}},
+                    data: {
+                        featured: {
+                            posts: [{id: 1, title: 'featured post 1', slug: 'featured-1'}],
+                            meta: {pagination: {}}
+                        }
+                    }
+                };
+
+            formatted = formatResponse.channel(data);
+
+            formatted.should.be.an.Object.with.properties('posts', 'pagination', 'featured');
+            formatted.featured.should.be.an.Object.with.properties('posts', 'pagination');
+        });
+    });
+});

core/test/unit/controllers/frontend/index_spec.js

@@ -12,20 +12,17 @@ var moment   = require('moment'),
     frontend = require('../../../../server/controllers/frontend'),
 
     config   = require('../../../../server/config'),
-    origConfig = _.cloneDeep(config);
+    origConfig = _.cloneDeep(config),
+
+    sandbox = sinon.sandbox.create();
 
 // To stop jshint complaining
 should.equal(true, true);
 
 describe('Frontend Controller', function () {
-    var sandbox,
-        apiSettingsStub,
+    var apiSettingsStub,
         adminEditPagePath = '/ghost/editor/';
 
-    beforeEach(function () {
-        sandbox = sinon.sandbox.create();
-    });
-
     afterEach(function () {
         config.set(origConfig);
         sandbox.restore();
@@ -34,9 +31,9 @@ describe('Frontend Controller', function () {
     // Helper function to prevent unit tests
     // from failing via timeout when they
     // should just immediately fail
-    function failTest(done, msg) {
-        return function () {
-            done(new Error(msg));
+    function failTest(done) {
+        return function (err) {
+            done(err);
         };
     }
 
@@ -251,20 +248,12 @@ describe('Frontend Controller', function () {
             }];
 
         beforeEach(function () {
-            sandbox.stub(api.posts, 'browse', function () {
-                return Promise.resolve({
-                    posts: [{}],
-                    meta: {
-                        pagination: {
-                            page: 1,
-                            pages: 1
-                        },
-                        filters: {
-                            tags: [mockTags[0]]
-                        }
-                    }
-                });
-            });
+            sandbox.stub(api.posts, 'browse').returns(new Promise.resolve({
+                posts: [{}],
+                meta: {pagination: {page: 1, pages: 1}}
+            }));
+
+            sandbox.stub(api.tags, 'read').returns(new Promise.resolve({tags: [mockTags[0]]}));
 
             apiSettingsStub = sandbox.stub(api.settings, 'read');
 
@@ -309,7 +298,7 @@ describe('Frontend Controller', function () {
             req.route = {path: '/tag/:slug'};
             res.render = function (view, context) {
                 view.should.equal('tag-video');
-                context.tag.should.equal(mockTags[0]);
+                context.tag.should.eql(mockTags[0]);
                 done();
             };
 
@@ -327,7 +316,7 @@ describe('Frontend Controller', function () {
             req.route = {path: '/tag/:slug'};
             res.render = function (view, context) {
                 view.should.equal('tag');
-                context.tag.should.equal(mockTags[0]);
+                context.tag.should.eql(mockTags[0]);
                 done();
             };
 
@@ -344,7 +333,7 @@ describe('Frontend Controller', function () {
             req.route = {path: '/tag/:slug'};
             res.render = function (view, context) {
                 view.should.equal('index');
-                context.tag.should.equal(mockTags[0]);
+                context.tag.should.eql(mockTags[0]);
                 done();
             };
 
@@ -365,9 +354,8 @@ describe('Frontend Controller', function () {
                 path: '/', params: {}, route: {}
             };
 
-            sandbox.stub(api.posts, 'browse', function () {
-                return Promise.resolve({posts: {}, meta: {pagination: {pages: 3}}});
-            });
+            sandbox.stub(api.posts, 'browse').returns(new Promise.resolve({posts: [{}], meta: {pagination: {pages: 3}}}));
+            sandbox.stub(api.tags, 'read').returns(new Promise.resolve({tags: [{}]}));
 
             apiSettingsStub = sandbox.stub(api.settings, 'read');
             apiSettingsStub.withArgs('postsPerPage').returns(Promise.resolve({

core/test/utils/index.js

@@ -215,11 +215,14 @@ fixtures = {
         });
     },
 
-    overrideOwnerUser: function overrideOwnerUser() {
+    overrideOwnerUser: function overrideOwnerUser(slug) {
         var user,
             knex = config.database.knex;
 
         user = DataGenerator.forKnex.createUser(DataGenerator.Content.users[0]);
+        if (slug) {
+            user.slug = slug;
+        }
 
         return knex('users')
             .where('id', '=', '1')