From 4e29d9e987d4c019b1e710500be8e45802c949f0 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Sun, 11 Oct 2015 21:21:54 +0100 Subject: [PATCH] Simplify theme middleware + improve tests refs #5286, #4172, #5888 - no need to pass blogApp around in middleware - improve test coverage to 100% --- core/server/middleware/index.js | 16 +- core/server/middleware/theme-handler.js | 214 +++++++++--------- .../unit/middleware/theme-handler_spec.js | 196 +++++++++++----- 3 files changed, 258 insertions(+), 168 deletions(-) diff --git a/core/server/middleware/index.js b/core/server/middleware/index.js index 6deac4825f..20d32782d5 100644 --- a/core/server/middleware/index.js +++ b/core/server/middleware/index.js @@ -20,21 +20,18 @@ var bodyParser = require('body-parser'), cacheControl = require('./cache-control'), checkSSL = require('./check-ssl'), decideIsAdmin = require('./decide-is-admin'), + oauth = require('./oauth'), privateBlogging = require('./private-blogging'), redirectToSetup = require('./redirect-to-setup'), serveSharedFile = require('./serve-shared-file'), spamPrevention = require('./spam-prevention'), staticTheme = require('./static-theme'), - uncapitalise = require('./uncapitalise'), - oauth = require('./oauth'), - themeHandler = require('./theme-handler'), - privateBlogging = require('./private-blogging'), + uncapitalise = require('./uncapitalise'), ClientPasswordStrategy = require('passport-oauth2-client-password').Strategy, BearerStrategy = require('passport-http-bearer').Strategy, - blogApp, middleware, setupMiddleware; @@ -51,7 +48,7 @@ middleware = { } }; -setupMiddleware = function setupMiddleware(blogAppInstance, adminApp) { +setupMiddleware = function setupMiddleware(blogApp, adminApp) { var logging = config.logging, corePath = config.paths.corePath, oauthServer = oauth2orize.createServer(); @@ -61,7 +58,6 @@ setupMiddleware = function setupMiddleware(blogAppInstance, adminApp) { passport.use(new BearerStrategy(authStrategies.bearerStrategy)); // Cache express server instance - blogApp = blogAppInstance; middleware.api.cacheOauthServer(oauthServer); oauth.init(oauthServer, spamPrevention.resetCounter); @@ -88,8 +84,8 @@ setupMiddleware = function setupMiddleware(blogAppInstance, adminApp) { // First determine whether we're serving admin or theme content blogApp.use(decideIsAdmin); - blogApp.use(themeHandler(blogApp).updateActiveTheme); - blogApp.use(themeHandler(blogApp).configHbsForContext); + blogApp.use(themeHandler.updateActiveTheme); + blogApp.use(themeHandler.configHbsForContext); // Admin only config blogApp.use('/ghost', express['static'](config.paths.clientAssets, {maxAge: utils.ONE_YEAR_MS})); @@ -143,7 +139,7 @@ setupMiddleware = function setupMiddleware(blogAppInstance, adminApp) { blogApp.use(authenticate); // local data - blogApp.use(themeHandler(blogApp).ghostLocals); + blogApp.use(themeHandler.ghostLocals); // ### Routing // Set up API routes diff --git a/core/server/middleware/theme-handler.js b/core/server/middleware/theme-handler.js index 8439cdfe2a..eabbdfa770 100644 --- a/core/server/middleware/theme-handler.js +++ b/core/server/middleware/theme-handler.js @@ -7,118 +7,118 @@ var _ = require('lodash'), errors = require('../errors'), themeHandler; -themeHandler = function (blogApp) { - var handlerMethods = { - // ### GhostLocals Middleware - // Expose the standard locals that every external page should have available, - // separating between the theme and the admin - ghostLocals: function ghostLocals(req, res, next) { - // Make sure we have a locals value. - res.locals = res.locals || {}; - res.locals.version = config.ghostVersion; - res.locals.safeVersion = config.ghostVersion.match(/^(\d+\.)?(\d+)/)[0]; - // relative path from the URL - res.locals.relativeUrl = req.path; +themeHandler = { + // ### GhostLocals Middleware + // Expose the standard locals that every external page should have available, + // separating between the theme and the admin + ghostLocals: function ghostLocals(req, res, next) { + // Make sure we have a locals value. + res.locals = res.locals || {}; + res.locals.version = config.ghostVersion; + res.locals.safeVersion = config.ghostVersion.match(/^(\d+\.)?(\d+)/)[0]; + // relative path from the URL + res.locals.relativeUrl = req.path; - next(); - }, + next(); + }, - // ### configHbsForContext Middleware - // Setup handlebars for the current context (admin or theme) - configHbsForContext: function configHbsForContext(req, res, next) { - var themeData = config.theme; - if (req.secure && config.urlSSL) { - // For secure requests override .url property with the SSL version - themeData = _.clone(themeData); - themeData.url = config.urlSSL.replace(/\/$/, ''); - } + // ### configHbsForContext Middleware + // Setup handlebars for the current context (admin or theme) + configHbsForContext: function configHbsForContext(req, res, next) { + var themeData = config.theme, + blogApp = req.app; - hbs.updateTemplateOptions({data: {blog: themeData}}); - - if (config.paths.themePath && blogApp.get('activeTheme')) { - blogApp.set('views', path.join(config.paths.themePath, blogApp.get('activeTheme'))); - } - - // Pass 'secure' flag to the view engine - // so that templates can choose 'url' vs 'urlSSL' - res.locals.secure = req.secure; - - next(); - }, - - // ### Activate Theme - // Helper for updateActiveTheme - activateTheme: function activateTheme(activeTheme) { - var hbsOptions, - themePartials = path.join(config.paths.themePath, activeTheme, 'partials'); - - // clear the view cache - blogApp.cache = {}; - - // set view engine - hbsOptions = { - partialsDir: [config.paths.helperTemplates], - onCompile: function onCompile(exhbs, source) { - return exhbs.handlebars.compile(source, {preventIndent: true}); - } - }; - - fs.stat(themePartials, function stat(err, stats) { - // Check that the theme has a partials directory before trying to use it - if (!err && stats && stats.isDirectory()) { - hbsOptions.partialsDir.push(themePartials); - } - }); - - blogApp.engine('hbs', hbs.express3(hbsOptions)); - - // Update user error template - errors.updateActiveTheme(activeTheme); - - // Set active theme variable on the express server - blogApp.set('activeTheme', activeTheme); - }, - - // ### updateActiveTheme - // Updates the blogApp's activeTheme variable and subsequently - // activates that theme's views with the hbs templating engine if it - // is not yet activated. - updateActiveTheme: function updateActiveTheme(req, res, next) { - api.settings.read({context: {internal: true}, key: 'activeTheme'}).then(function then(response) { - var activeTheme = response.settings[0]; - - // Check if the theme changed - if (activeTheme.value !== blogApp.get('activeTheme')) { - // Change theme - if (!config.paths.availableThemes.hasOwnProperty(activeTheme.value)) { - if (!res.isAdmin) { - // Throw an error if the theme is not available, but not on the admin UI - return errors.throwError('The currently active theme ' + activeTheme.value + ' is missing.'); - } else { - // At this point the activated theme is not present and the current - // request is for the admin client. In order to allow the user access - // to the admin client we set an hbs instance on the app so that middleware - // processing can continue. - blogApp.engine('hbs', hbs.express3()); - errors.logWarn('The currently active theme "' + activeTheme.value + '" is missing.'); - - return next(); - } - } else { - handlerMethods.activateTheme(activeTheme.value); - } - } - next(); - }).catch(function handleError(err) { - // Trying to start up without the active theme present, setup a simple hbs instance - // and render an error page straight away. - blogApp.engine('hbs', hbs.express3()); - next(err); - }); + if (req.secure && config.urlSSL) { + // For secure requests override .url property with the SSL version + themeData = _.clone(themeData); + themeData.url = config.urlSSL.replace(/\/$/, ''); } - }; - return handlerMethods; + hbs.updateTemplateOptions({data: {blog: themeData}}); + + if (config.paths.themePath && blogApp.get('activeTheme')) { + blogApp.set('views', path.join(config.paths.themePath, blogApp.get('activeTheme'))); + } + + // Pass 'secure' flag to the view engine + // so that templates can choose 'url' vs 'urlSSL' + res.locals.secure = req.secure; + + next(); + }, + + // ### Activate Theme + // Helper for updateActiveTheme + activateTheme: function activateTheme(blogApp, activeTheme) { + var hbsOptions, + themePartials = path.join(config.paths.themePath, activeTheme, 'partials'); + + // clear the view cache + blogApp.cache = {}; + + // set view engine + hbsOptions = { + partialsDir: [config.paths.helperTemplates], + onCompile: function onCompile(exhbs, source) { + return exhbs.handlebars.compile(source, {preventIndent: true}); + } + }; + + fs.stat(themePartials, function stat(err, stats) { + // Check that the theme has a partials directory before trying to use it + if (!err && stats && stats.isDirectory()) { + hbsOptions.partialsDir.push(themePartials); + } + }); + + blogApp.engine('hbs', hbs.express3(hbsOptions)); + + // Update user error template + errors.updateActiveTheme(activeTheme); + + // Set active theme variable on the express server + blogApp.set('activeTheme', activeTheme); + }, + + // ### updateActiveTheme + // Updates the blogApp's activeTheme variable and subsequently + // activates that theme's views with the hbs templating engine if it + // is not yet activated. + updateActiveTheme: function updateActiveTheme(req, res, next) { + var blogApp = req.app; + + api.settings.read({context: {internal: true}, key: 'activeTheme'}).then(function then(response) { + var activeTheme = response.settings[0]; + + // Check if the theme changed + if (activeTheme.value !== blogApp.get('activeTheme')) { + // Change theme + if (!config.paths.availableThemes.hasOwnProperty(activeTheme.value)) { + if (!res.isAdmin) { + // Throw an error if the theme is not available, but not on the admin UI + return errors.throwError('The currently active theme "' + activeTheme.value + '" is missing.'); + } else { + // At this point the activated theme is not present and the current + // request is for the admin client. In order to allow the user access + // to the admin client we set an hbs instance on the app so that middleware + // processing can continue. + blogApp.engine('hbs', hbs.express3()); + errors.logWarn('The currently active theme "' + activeTheme.value + '" is missing.'); + + return next(); + } + } else { + themeHandler.activateTheme(blogApp, activeTheme.value); + } + } + next(); + }).catch(function handleError(err) { + // Trying to start up without the active theme present, setup a simple hbs instance + // and render an error page straight away. + blogApp.engine('hbs', hbs.express3()); + next(err); + }); + } }; module.exports = themeHandler; diff --git a/core/test/unit/middleware/theme-handler_spec.js b/core/test/unit/middleware/theme-handler_spec.js index b3263b98a1..d90833fa3f 100644 --- a/core/test/unit/middleware/theme-handler_spec.js +++ b/core/test/unit/middleware/theme-handler_spec.js @@ -4,27 +4,31 @@ var _ = require('lodash'), sinon = require('sinon'), should = require('should'), express = require('express'), + Promise = require('bluebird'), - // Stuff we test +// Stuff we test + fs = require('fs'), + hbs = require('express-hbs'), themeHandler = require('../../../server/middleware/theme-handler'), errors = require('../../../server/errors'), + api = require('../../../server/api'), config = require('../../../server/config'), origConfig = _.cloneDeep(config), - defaultConfig = require('../../../../config.example')[process.env.NODE_ENV]; + defaultConfig = require('../../../../config.example')[process.env.NODE_ENV], + sandbox = sinon.sandbox.create(); should.equal(true, true); describe('Theme Handler', function () { - var req, res, next, blogApp, handler, sandbox; + var req, res, next, blogApp; beforeEach(function () { req = sinon.spy(); res = sinon.spy(); next = sinon.spy(); blogApp = express(); - handler = themeHandler(blogApp); - sandbox = sinon.sandbox.create(); + req.app = blogApp; }); afterEach(function () { @@ -38,7 +42,7 @@ describe('Theme Handler', function () { it('sets all locals', function () { req.path = '/awesome-post'; - handler.ghostLocals(req, res, next); + themeHandler.ghostLocals(req, res, next); res.locals.should.be.an.Object; res.locals.version.should.exist; @@ -49,31 +53,65 @@ describe('Theme Handler', function () { }); describe('activateTheme', function () { - it('should activate new theme', function () { - var errorStub = sandbox.stub(errors, 'updateActiveTheme'); - handler.activateTheme('casper'); + it('should activate new theme with partials', function () { + var errorStub = sandbox.stub(errors, 'updateActiveTheme'), + fsStub = sandbox.stub(fs, 'stat', function (path, cb) { + cb(null, {isDirectory: function () { return true; }}); + }), + hbsStub = sandbox.spy(hbs, 'express3'); + + themeHandler.activateTheme(blogApp, 'casper'); errorStub.calledWith('casper').should.be.true; + fsStub.calledOnce.should.be.true; + hbsStub.calledOnce.should.be.true; + hbsStub.firstCall.args[0].should.be.an.Object.and.have.property('partialsDir'); + hbsStub.firstCall.args[0].partialsDir.should.have.lengthOf(2); + blogApp.get('activeTheme').should.equal('casper'); + }); + + it('should activate new theme without partials', function () { + var errorStub = sandbox.stub(errors, 'updateActiveTheme'), + fsStub = sandbox.stub(fs, 'stat', function (path, cb) { + cb(null, null); + }), + hbsStub = sandbox.spy(hbs, 'express3'); + + themeHandler.activateTheme(blogApp, 'casper'); + + errorStub.calledWith('casper').should.be.true; + fsStub.calledOnce.should.be.true; + hbsStub.calledOnce.should.be.true; + hbsStub.firstCall.args[0].should.be.an.Object.and.have.property('partialsDir'); + hbsStub.firstCall.args[0].partialsDir.should.have.lengthOf(1); blogApp.get('activeTheme').should.equal('casper'); }); }); describe('configHbsForContext', function () { - it('calls next', function () { - req.secure = true; + it('handles non secure context', function () { res.locals = {}; - handler.configHbsForContext(req, res, next); + themeHandler.configHbsForContext(req, res, next); + should.not.exist(res.locals.secure); next.called.should.be.true; }); - it('sets secure local variable', function () { + it('handles secure context', function () { + var themeOptSpy = sandbox.stub(hbs, 'updateTemplateOptions'); req.secure = true; res.locals = {}; + config.set({urlSSL: 'https://secure.blog'}); - handler.configHbsForContext(req, res, next); + themeHandler.configHbsForContext(req, res, next); - res.locals.secure.should.equal(req.secure); + themeOptSpy.calledOnce.should.be.true; + themeOptSpy.firstCall.args[0].should.be.an.Object.and.have.property('data'); + themeOptSpy.firstCall.args[0].data.should.be.an.Object.and.have.property('blog'); + themeOptSpy.firstCall.args[0].data.blog.should.be.an.Object.and.have.property('url'); + themeOptSpy.firstCall.args[0].data.blog.url.should.eql('https://secure.blog'); + res.locals.secure.should.equal(true); + next.called.should.be.true; }); it('sets view path', function () { @@ -81,45 +119,101 @@ describe('Theme Handler', function () { res.locals = {}; blogApp.set('activeTheme', 'casper'); - handler.configHbsForContext(req, res, next); + themeHandler.configHbsForContext(req, res, next); + + blogApp.get('views').should.not.be.undefined; + }); + + it('sets view path', function () { + req.secure = true; + res.locals = {}; + blogApp.set('activeTheme', 'casper'); + + themeHandler.configHbsForContext(req, res, next); blogApp.get('views').should.not.be.undefined; }); }); - // describe('updateActiveTheme', function () { - // it('updates the active theme if changed', function () { - // var activateThemeSpy = sinon.spy(handler, 'activateTheme'); - // sandbox.stub(api.settings, 'read').withArgs(sinon.match.has('key', 'activeTheme')).returns(Promise.resolve({ - // settings: [{ - // key: 'activeKey', - // value: 'casper' - // }] - // })); - // blogApp.set('activeTheme', 'not-casper'); - // config.set({paths: {availableThemes: {casper: {}}}}); - // - // handler.updateActiveTheme(req, res, next); - // - // activateThemeSpy.called.should.be.false; - // next.called.should.be.false; - // }); - // - // it('throws error if theme is missing', function () { - // var errorSpy = sinon.spy(errors, 'throwError'); - // sandbox.stub(api.settings, 'read').withArgs(sinon.match.has('key', 'activeTheme')).returns(Promise.resolve({ - // settings: [{ - // key: 'activeKey', - // value: 'rasper' - // }] - // })); - // blogApp.set('activeTheme', 'not-casper'); - // config.set({paths: {availableThemes: {casper: {}}}}); - // - // handler.updateActiveTheme(req, res, next); - // - // errorSpy.called.should.be.true; - // next.called.should.be.false; - // }); - // }); + describe('updateActiveTheme', function () { + it('updates the active theme if changed', function (done) { + var activateThemeSpy = sandbox.spy(themeHandler, 'activateTheme'); + sandbox.stub(api.settings, 'read').withArgs(sandbox.match.has('key', 'activeTheme')).returns(Promise.resolve({ + settings: [{ + key: 'activeKey', + value: 'casper' + }] + })); + blogApp.set('activeTheme', 'not-casper'); + config.set({paths: {availableThemes: {casper: {}}}}); + + themeHandler.updateActiveTheme(req, res, function () { + activateThemeSpy.called.should.be.true; + done(); + }); + }); + + it('does not update the active theme if not changed', function (done) { + var activateThemeSpy = sandbox.spy(themeHandler, 'activateTheme'); + sandbox.stub(api.settings, 'read').withArgs(sandbox.match.has('key', 'activeTheme')).returns(Promise.resolve({ + settings: [{ + key: 'activeKey', + value: 'casper' + }] + })); + blogApp.set('activeTheme', 'casper'); + config.set({paths: {availableThemes: {casper: {}}}}); + + themeHandler.updateActiveTheme(req, res, function () { + activateThemeSpy.called.should.be.false; + done(); + }); + }); + + it('throws error if theme is missing', function (done) { + var errorSpy = sandbox.spy(errors, 'throwError'), + activateThemeSpy = sandbox.spy(themeHandler, 'activateTheme'); + + sandbox.stub(api.settings, 'read').withArgs(sandbox.match.has('key', 'activeTheme')).returns(Promise.resolve({ + settings: [{ + key: 'activeKey', + value: 'rasper' + }] + })); + blogApp.set('activeTheme', 'not-casper'); + config.set({paths: {availableThemes: {casper: {}}}}); + + themeHandler.updateActiveTheme(req, res, function (err) { + should.exist(err); + errorSpy.called.should.be.true; + activateThemeSpy.called.should.be.false; + err.message.should.eql('The currently active theme "rasper" is missing.'); + done(); + }); + }); + + it('throws only warns if theme is missing for admin req', function (done) { + var errorSpy = sandbox.spy(errors, 'throwError'), + warnSpy = sandbox.spy(errors, 'logWarn'), + activateThemeSpy = sandbox.spy(themeHandler, 'activateTheme'); + + sandbox.stub(api.settings, 'read').withArgs(sandbox.match.has('key', 'activeTheme')).returns(Promise.resolve({ + settings: [{ + key: 'activeKey', + value: 'rasper' + }] + })); + res.isAdmin = true; + blogApp.set('activeTheme', 'not-casper'); + config.set({paths: {availableThemes: {casper: {}}}}); + + themeHandler.updateActiveTheme(req, res, function () { + errorSpy.called.should.be.false; + activateThemeSpy.called.should.be.false; + warnSpy.called.should.be.true; + warnSpy.calledWith('The currently active theme "rasper" is missing.').should.be.true; + done(); + }); + }); + }); });