From e9035fde4ec4e7291c6530f8bbedfc53a39df0b2 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Sat, 24 Oct 2015 20:04:02 +0100 Subject: [PATCH] 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 --- .../server/controllers/frontend/fetch-data.js | 118 +++++++++++++- .../controllers/frontend/format-response.js | 25 ++- core/server/controllers/frontend/index.js | 90 ++++++----- core/test/functional/routes/frontend_spec.js | 2 + .../controllers/frontend/fetch-data_spec.js | 148 ++++++++++++++++-- .../frontend/format-response_spec.js | 71 +++++++++ .../unit/controllers/frontend/index_spec.js | 48 +++--- core/test/utils/index.js | 5 +- 8 files changed, 410 insertions(+), 97 deletions(-) create mode 100644 core/test/unit/controllers/frontend/format-response_spec.js diff --git a/core/server/controllers/frontend/fetch-data.js b/core/server/controllers/frontend/fetch-data.js index f480bedda5..a0d4503394 100644 --- a/core/server/controllers/frontend/fetch-data.js +++ b/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; + }); }); } diff --git a/core/server/controllers/frontend/format-response.js b/core/server/controllers/frontend/format-response.js index d8f0ba04ae..48a7de8ae0 100644 --- a/core/server/controllers/frontend/format-response.js +++ b/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; } /** diff --git a/core/server/controllers/frontend/index.js b/core/server/controllers/frontend/index.js index 855f167be9..e92e33ec0e 100644 --- a/core/server/controllers/frontend/index.js +++ b/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) { diff --git a/core/test/functional/routes/frontend_spec.js b/core/test/functional/routes/frontend_spec.js index 3f0bd6dab8..5d5b57a1ed 100644 --- a/core/test/functional/routes/frontend_spec.js +++ b/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 () { diff --git a/core/test/unit/controllers/frontend/fetch-data_spec.js b/core/test/unit/controllers/frontend/fetch-data_spec.js index c4104a1741..7e61016950 100644 --- a/core/test/unit/controllers/frontend/fetch-data_spec.js +++ b/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 () { diff --git a/core/test/unit/controllers/frontend/format-response_spec.js b/core/test/unit/controllers/frontend/format-response_spec.js new file mode 100644 index 0000000000..56efcc32bf --- /dev/null +++ b/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'); + }); + }); +}); diff --git a/core/test/unit/controllers/frontend/index_spec.js b/core/test/unit/controllers/frontend/index_spec.js index 6e2bd7ca08..3ef95b8f60 100644 --- a/core/test/unit/controllers/frontend/index_spec.js +++ b/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({ diff --git a/core/test/utils/index.js b/core/test/utils/index.js index d731204f2e..0aee00aa1f 100644 --- a/core/test/utils/index.js +++ b/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')