From 302702c740707e2ff3ccdba815838ccdd3264b84 Mon Sep 17 00:00:00 2001 From: Kevin Ansfield Date: Thu, 21 Jul 2016 12:26:16 +0100 Subject: [PATCH] Revert "fix: ensure we initialise activeTheme on bootstrap (#6950)" This reverts commit 8f2afeed039ff71d602a7a78be64d1d5a2a9644a. --- core/server/api/settings.js | 1 - core/server/middleware/index.js | 16 +++--- core/server/middleware/theme-handler.js | 56 ++++++++++--------- .../unit/middleware/theme-handler_spec.js | 51 ++++++----------- 4 files changed, 54 insertions(+), 70 deletions(-) diff --git a/core/server/api/settings.js b/core/server/api/settings.js index 6591bba55c..fc1192fa82 100644 --- a/core/server/api/settings.js +++ b/core/server/api/settings.js @@ -52,7 +52,6 @@ updateConfigCache = function () { config.set({ theme: { - activeTheme: settingsCache.activeTheme.value, title: (settingsCache.title && settingsCache.title.value) || '', description: (settingsCache.description && settingsCache.description.value) || '', logo: (settingsCache.logo && settingsCache.logo.value) || '', diff --git a/core/server/middleware/index.js b/core/server/middleware/index.js index 4be7b194b6..a3f83c4729 100644 --- a/core/server/middleware/index.js +++ b/core/server/middleware/index.js @@ -1,21 +1,20 @@ var bodyParser = require('body-parser'), compress = require('compression'), + config = require('../config'), + errors = require('../errors'), express = require('express'), hbs = require('express-hbs'), logger = require('morgan'), - multer = require('multer'), - netjet = require('netjet'), path = require('path'), + routes = require('../routes'), serveStatic = require('express').static, slashes = require('connect-slashes'), - tmpdir = require('os').tmpdir, - passport = require('passport'), - config = require('../config'), - errors = require('../errors'), - routes = require('../routes'), storage = require('../storage'), + passport = require('passport'), utils = require('../utils'), sitemapHandler = require('../data/xml/sitemap/handler'), + multer = require('multer'), + tmpdir = require('os').tmpdir, authStrategies = require('./auth-strategies'), auth = require('./auth'), cacheControl = require('./cache-control'), @@ -31,6 +30,7 @@ var bodyParser = require('body-parser'), maintenance = require('./maintenance'), versionMatch = require('./api/version-match'), cors = require('./cors'), + netjet = require('netjet'), labs = require('./labs'), helpers = require('../helpers'), @@ -129,7 +129,7 @@ setupMiddleware = function setupMiddleware(blogApp) { // First determine whether we're serving admin or theme content blogApp.use(decideIsAdmin); - blogApp.use(themeHandler.updateActiveTheme(blogApp)); + blogApp.use(themeHandler.updateActiveTheme); blogApp.use(themeHandler.configHbsForContext); // Admin only config diff --git a/core/server/middleware/theme-handler.js b/core/server/middleware/theme-handler.js index cf867bbff9..5c065a14fb 100644 --- a/core/server/middleware/theme-handler.js +++ b/core/server/middleware/theme-handler.js @@ -48,6 +48,7 @@ themeHandler = { // Pass 'secure' flag to the view engine // so that templates can choose 'url' vs 'urlSSL' res.locals.secure = req.secure; + next(); }, @@ -88,37 +89,40 @@ themeHandler = { // Updates the blogApp's activeTheme variable and subsequently // activates that theme's views with the hbs templating engine if it // is not yet activated. - // - // on server bootstrap we activate the default theme (which is casper) - updateActiveTheme: function updateActiveTheme(blog) { - themeHandler.activateTheme(blog, config.theme.activeTheme); + updateActiveTheme: function updateActiveTheme(req, res, next) { + var blogApp = req.app; - return function updateActiveThemeDynamically(req, res, next) { - var blogApp = req.app; + api.settings.read({context: {internal: true}, key: 'activeTheme'}).then(function then(response) { + var activeTheme = response.settings[0]; - 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(i18n.t('errors.middleware.themehandler.missingTheme', {theme: activeTheme.value})); - } else { - errors.logWarn(i18n.t('errors.middleware.themehandler.missingTheme', {theme: activeTheme.value})); - return next(); - } + // 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(i18n.t('errors.middleware.themehandler.missingTheme', {theme: activeTheme.value})); } else { - themeHandler.activateTheme(blogApp, activeTheme.value); + // 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(i18n.t('errors.middleware.themehandler.missingTheme', {theme: activeTheme.value})); + + return next(); } + } else { + themeHandler.activateTheme(blogApp, activeTheme.value); } - next(); - }).catch(function handleError(err) { - next(err); - }); - }; + } + 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); + }); } }; diff --git a/core/test/unit/middleware/theme-handler_spec.js b/core/test/unit/middleware/theme-handler_spec.js index d07ed9e49e..023414c18e 100644 --- a/core/test/unit/middleware/theme-handler_spec.js +++ b/core/test/unit/middleware/theme-handler_spec.js @@ -127,34 +127,19 @@ describe('Theme Handler', function () { }); describe('updateActiveTheme', function () { - it('sets active theme on bootstrap', function () { - should.not.exist(blogApp.get('activeTheme')); - - var activateThemeSpy = sandbox.spy(themeHandler, 'activateTheme'); - configUtils.set({theme: {activeTheme: 'casper'}}); - configUtils.set({paths: {availableThemes: {casper: {}}}}); - - themeHandler.updateActiveTheme(blogApp); - activateThemeSpy.calledOnce.should.eql(true); - blogApp.get('activeTheme').should.eql('casper'); - }); - 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: 'activateTheme', - value: 'new-theme' + key: 'activeKey', + value: 'casper' }] })); + blogApp.set('activeTheme', 'not-casper'); + configUtils.set({paths: {availableThemes: {casper: {}}}}); - configUtils.set({theme: {activeTheme: 'casper'}}); - configUtils.set({paths: {availableThemes: {casper: {}, 'new-theme': {}}}}); - - themeHandler.updateActiveTheme(blogApp)(req, res, function () { - activateThemeSpy.calledTwice.should.be.true(); - blogApp.get('activeTheme').should.eql('new-theme'); + themeHandler.updateActiveTheme(req, res, function () { + activateThemeSpy.called.should.be.true(); done(); }); }); @@ -163,17 +148,15 @@ describe('Theme Handler', function () { var activateThemeSpy = sandbox.spy(themeHandler, 'activateTheme'); sandbox.stub(api.settings, 'read').withArgs(sandbox.match.has('key', 'activeTheme')).returns(Promise.resolve({ settings: [{ - key: 'activateTheme', + key: 'activeKey', value: 'casper' }] })); - - configUtils.set({theme: {activeTheme: 'casper'}}); + blogApp.set('activeTheme', 'casper'); configUtils.set({paths: {availableThemes: {casper: {}}}}); - themeHandler.updateActiveTheme(blogApp)(req, res, function () { - activateThemeSpy.calledOnce.should.be.true(); - blogApp.get('activeTheme').should.eql('casper'); + themeHandler.updateActiveTheme(req, res, function () { + activateThemeSpy.called.should.be.false(); done(); }); }); @@ -188,14 +171,13 @@ describe('Theme Handler', function () { value: 'rasper' }] })); - - configUtils.set({theme: {activeTheme: 'casper'}}); + blogApp.set('activeTheme', 'not-casper'); configUtils.set({paths: {availableThemes: {casper: {}}}}); - themeHandler.updateActiveTheme(blogApp)(req, res, function (err) { + themeHandler.updateActiveTheme(req, res, function (err) { should.exist(err); errorSpy.called.should.be.true(); - activateThemeSpy.calledOnce.should.be.true(); + activateThemeSpy.called.should.be.false(); err.message.should.eql('The currently active theme "rasper" is missing.'); done(); }); @@ -212,14 +194,13 @@ describe('Theme Handler', function () { value: 'rasper' }] })); - res.isAdmin = true; - configUtils.set({theme: {activeTheme: 'casper'}}); + blogApp.set('activeTheme', 'not-casper'); configUtils.set({paths: {availableThemes: {casper: {}}}}); - themeHandler.updateActiveTheme(blogApp)(req, res, function () { + themeHandler.updateActiveTheme(req, res, function () { errorSpy.called.should.be.false(); - activateThemeSpy.calledOnce.should.be.true(); + activateThemeSpy.called.should.be.false(); warnSpy.called.should.be.true(); warnSpy.calledWith('The currently active theme "rasper" is missing.').should.be.true(); done();