From 2450f181702c1c8f7896e76e8c23a31f68c1146e Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Sat, 28 Feb 2015 12:53:00 +0000 Subject: [PATCH] Make the {{navigation}} helper global refs #4535 - Rather than storing navigation data as a top level key, store it as @blog.navigation - Reference the global data from the helper --- core/server/api/settings.js | 3 +- core/server/config/index.js | 5 + core/server/controllers/frontend.js | 101 +++++++----------- core/server/helpers/navigation.js | 20 ++-- .../unit/server_helpers/navigation_spec.js | 73 +++++++++---- 5 files changed, 105 insertions(+), 97 deletions(-) diff --git a/core/server/api/settings.js b/core/server/api/settings.js index ce9ab057de..94aa2b65b3 100644 --- a/core/server/api/settings.js +++ b/core/server/api/settings.js @@ -40,7 +40,8 @@ updateConfigTheme = function () { title: (settingsCache.title && settingsCache.title.value) || '', description: (settingsCache.description && settingsCache.description.value) || '', logo: (settingsCache.logo && settingsCache.logo.value) || '', - cover: (settingsCache.cover && settingsCache.cover.value) || '' + cover: (settingsCache.cover && settingsCache.cover.value) || '', + navigation: (settingsCache.navigation && JSON.parse(settingsCache.navigation.value)) || [] } }); }; diff --git a/core/server/config/index.js b/core/server/config/index.js index a6be1155e0..ac101f5aff 100644 --- a/core/server/config/index.js +++ b/core/server/config/index.js @@ -118,6 +118,11 @@ ConfigManager.prototype.set = function (config) { // local copy with properties that have been explicitly set. _.merge(this._config, config); + // Special case for the them.navigation JSON object, which should be overridden not merged + if (config && config.theme && config.theme.navigation) { + this._config.theme.navigation = config.theme.navigation; + } + // Protect against accessing a non-existant object. // This ensures there's always at least a paths object // because it's referenced in multiple places. diff --git a/core/server/controllers/frontend.js b/core/server/controllers/frontend.js index d0aea6124f..c6dca3b118 100644 --- a/core/server/controllers/frontend.js +++ b/core/server/controllers/frontend.js @@ -37,25 +37,10 @@ function getPostPage(options) { }); } -/** - * returns a promise with an array of values used in {{navigation}} - * TODO(nsfmc): should this be in the 'prePostsRender' pipeline? - * @return {Promise} containing an array of navigation items - */ -function getSiteNavigation() { - return Promise.resolve(api.settings.read('navigation')).then(function (result) { - if (result && result.settings && result.settings.length) { - return JSON.parse(result.settings[0].value) || []; - } - return []; - }); -} - /** * formats variables for handlebars in multi-post contexts. * If extraValues are available, they are merged in the final value - * TODO(nsfmc): should this be in the 'prePostsRender' pipeline? - * @return {Promise} containing page variables + * @return {Object} containing page variables */ function formatPageResponse(posts, page, extraValues) { // Delete email from author for frontend output @@ -68,20 +53,16 @@ function formatPageResponse(posts, page, extraValues) { }); extraValues = extraValues || {}; - return getSiteNavigation().then(function (navigation) { - var resp = { - posts: posts, - pagination: page.meta.pagination, - navigation: navigation || {} - }; - return _.extend(resp, extraValues); - }); + var resp = { + posts: posts, + pagination: page.meta.pagination + }; + return _.extend(resp, extraValues); } /** * similar to formatPageResponse, but for single post pages - * TODO(nsfmc): should this be in the 'prePostsRender' pipeline? - * @return {Promise} containing page variables + * @return {Object} containing page variables */ function formatResponse(post) { // Delete email from author for frontend output @@ -90,12 +71,9 @@ function formatResponse(post) { delete post.author.email; } - return getSiteNavigation().then(function (navigation) { - return { - post: post, - navigation: navigation - }; - }); + return { + post: post + }; } function handleError(next) { @@ -192,9 +170,7 @@ frontendControllers = { } setResponseContext(req, res); - formatPageResponse(posts, page).then(function (result) { - res.render(view, result); - }); + res.render(view, formatPageResponse(posts, page)); }); }); }).catch(handleError(next)); @@ -237,19 +213,18 @@ frontendControllers = { // Render the page of posts filters.doFilter('prePostsRender', page.posts).then(function (posts) { getActiveThemePaths().then(function (paths) { - var view = template.getThemeViewForTag(paths, options.tag); + var view = template.getThemeViewForTag(paths, options.tag), + // Format data for template + result = formatPageResponse(posts, page, { + tag: page.meta.filters.tags ? page.meta.filters.tags[0] : '' + }); - // Format data for template - formatPageResponse(posts, page, { - tag: page.meta.filters.tags ? page.meta.filters.tags[0] : '' - }).then(function (result) { - // If the resulting tag is '' then 404. - if (!result.tag) { - return next(); - } - setResponseContext(req, res); - res.render(view, result); - }); + // If the resulting tag is '' then 404. + if (!result.tag) { + return next(); + } + setResponseContext(req, res); + res.render(view, result); }); }); }).catch(handleError(next)); @@ -292,20 +267,19 @@ frontendControllers = { // Render the page of posts filters.doFilter('prePostsRender', page.posts).then(function (posts) { getActiveThemePaths().then(function (paths) { - var view = paths.hasOwnProperty('author.hbs') ? 'author' : 'index'; - + var view = paths.hasOwnProperty('author.hbs') ? 'author' : 'index', // Format data for template - formatPageResponse(posts, page, { - author: page.meta.filters.author ? page.meta.filters.author : '' - }).then(function (result) { - // If the resulting author is '' then 404. - if (!result.author) { - return next(); - } + result = formatPageResponse(posts, page, { + author: page.meta.filters.author ? page.meta.filters.author : '' + }); - setResponseContext(req, res); - res.render(view, result); - }); + // If the resulting author is '' then 404. + if (!result.author) { + return next(); + } + + setResponseContext(req, res); + res.render(view, result); }); }); }).catch(handleError(next)); @@ -378,13 +352,12 @@ frontendControllers = { filters.doFilter('prePostsRender', post).then(function (post) { getActiveThemePaths().then(function (paths) { - var view = template.getThemeViewForPost(paths, post); + var view = template.getThemeViewForPost(paths, post), + response = formatResponse(post); - return formatResponse(post).then(function (response) { - setResponseContext(req, res, response); + setResponseContext(req, res, response); - res.render(view, response); - }); + res.render(view, response); }); }); } diff --git a/core/server/helpers/navigation.js b/core/server/helpers/navigation.js index 6d12417b98..6499608ce3 100644 --- a/core/server/helpers/navigation.js +++ b/core/server/helpers/navigation.js @@ -4,28 +4,30 @@ var _ = require('lodash'), hbs = require('express-hbs'), + errors = require('../errors'), template = require('./template'), navigation; navigation = function (options) { /*jshint unused:false*/ - var navigation, - context, - currentUrl = this.relativeUrl; + var navigationData = options.data.blog.navigation, + currentUrl = options.data.root.relativeUrl, + output, + context; - if (!_.isObject(this.navigation) || _.isFunction(this.navigation)) { + if (!_.isObject(navigationData) || _.isFunction(navigationData)) { return errors.logAndThrowError('navigation data is not an object or is a function'); } - if (this.navigation.filter(function (e) { + if (navigationData.filter(function (e) { return (_.isUndefined(e.label) || _.isUndefined(e.url)); }).length > 0) { return errors.logAndThrowError('All values must be defined for label, url and current'); } // check for non-null string values - if (this.navigation.filter(function (e) { + if (navigationData.filter(function (e) { return ((!_.isNull(e.label) && !_.isString(e.label)) || (!_.isNull(e.url) && !_.isString(e.url))); }).length > 0) { @@ -37,11 +39,11 @@ navigation = function (options) { } // {{navigation}} should no-op if no data passed in - if (this.navigation.length === 0) { + if (navigationData.length === 0) { return new hbs.SafeString(''); } - navigation = this.navigation.map(function (e) { + output = navigationData.map(function (e) { var out = {}; out.current = e.url === currentUrl; out.label = e.label; @@ -50,7 +52,7 @@ navigation = function (options) { return out; }); - context = _.merge({}, {navigation: navigation}); + context = _.merge({}, {navigation: output}); return template.execute('navigation', context); }; diff --git a/core/test/unit/server_helpers/navigation_spec.js b/core/test/unit/server_helpers/navigation_spec.js index 3f2c423c78..55cb18ae48 100644 --- a/core/test/unit/server_helpers/navigation_spec.js +++ b/core/test/unit/server_helpers/navigation_spec.js @@ -1,4 +1,4 @@ -/*globals describe, before, it*/ +/*globals describe, before, beforeEach, it*/ /*jshint expr:true*/ var should = require('should'), hbs = require('express-hbs'), @@ -9,6 +9,13 @@ var should = require('should'), helpers = require('../../../server/helpers'); describe('{{navigation}} helper', function () { + var runHelper = function (data) { + return function () { + helpers.navigation(data); + }; + }, + optionsData; + before(function (done) { utils.loadHelpers(); hbs.express3({partialsDir: [utils.config.paths.helperTemplates]}); @@ -17,27 +24,43 @@ describe('{{navigation}} helper', function () { }); }); + beforeEach(function () { + optionsData = { + data: { + blog: { + navigation: [] + }, + root: { + relativeUrl: '' + } + } + }; + }); + it('has loaded navigation helper', function () { should.exist(handlebars.helpers.navigation); }); it('should throw errors on invalid data', function () { - var runHelper = function (data) { - return function () { - helpers.navigation.call(data); - }; - }; + // Test 1: navigation = string + optionsData.data.blog.navigation = 'not an object'; + runHelper(optionsData).should.throwError('navigation data is not an object or is a function'); - runHelper('not an object').should.throwError('navigation data is not an object or is a function'); - runHelper(function () {}).should.throwError('navigation data is not an object or is a function'); + // Test 2: navigation = function + optionsData.data.blog.navigation = function () {}; + runHelper(optionsData).should.throwError('navigation data is not an object or is a function'); - runHelper({navigation: [{label: 1, url: 'bar'}]}).should.throwError('Invalid value, Url and Label must be strings'); - runHelper({navigation: [{label: 'foo', url: 1}]}).should.throwError('Invalid value, Url and Label must be strings'); + // Test 3: invalid label + optionsData.data.blog.navigation = [{label: 1, url: 'bar'}]; + runHelper(optionsData).should.throwError('Invalid value, Url and Label must be strings'); + + // Test 4: invalid url + optionsData.data.blog.navigation = [{label: 'foo', url: 1}]; + runHelper(optionsData).should.throwError('Invalid value, Url and Label must be strings'); }); it('can render empty nav', function () { - var navigation = {navigation:[]}, - rendered = helpers.navigation.call(navigation); + var rendered = helpers.navigation(optionsData); should.exist(rendered); rendered.string.should.be.equal(''); @@ -45,9 +68,11 @@ describe('{{navigation}} helper', function () { it('can render one item', function () { var singleItem = {label: 'Foo', url: '/foo'}, - navigation = {navigation: [singleItem]}, - rendered = helpers.navigation.call(navigation), - testUrl = 'href="' + utils.config.url + '/foo"'; + testUrl = 'href="' + utils.config.url + '/foo"', + rendered; + + optionsData.data.blog.navigation = [singleItem]; + rendered = helpers.navigation(optionsData); should.exist(rendered); rendered.string.should.containEql('li'); @@ -58,10 +83,12 @@ describe('{{navigation}} helper', function () { it('can render multiple items', function () { var firstItem = {label: 'Foo', url: '/foo'}, secondItem = {label: 'Bar Baz Qux', url: '/qux'}, - navigation = {navigation: [firstItem, secondItem]}, - rendered = helpers.navigation.call(navigation), testUrl = 'href="' + utils.config.url + '/foo"', - testUrl2 = 'href="' + utils.config.url + '/qux"'; + testUrl2 = 'href="' + utils.config.url + '/qux"', + rendered; + + optionsData.data.blog.navigation = [firstItem, secondItem]; + rendered = helpers.navigation(optionsData); should.exist(rendered); rendered.string.should.containEql('nav-foo'); @@ -73,11 +100,11 @@ describe('{{navigation}} helper', function () { it('can annotate the current url', function () { var firstItem = {label: 'Foo', url: '/foo'}, secondItem = {label: 'Bar', url: '/qux'}, - navigation = { - relativeUrl: '/foo', - navigation: [firstItem, secondItem] - }, - rendered = helpers.navigation.call(navigation); + rendered; + + optionsData.data.blog.navigation = [firstItem, secondItem]; + optionsData.data.root.relativeUrl = '/foo'; + rendered = helpers.navigation(optionsData); should.exist(rendered); rendered.string.should.containEql('nav-foo');