✨ New fully-loaded & validated activeTheme concept (#8146)

📡 Add debug for the 3 theme activation methods
There are 3 different ways that a theme can be activated in Ghost:

A. On boot: we load the active theme from the file system, according to the `activeTheme` setting
B. On API "activate": when an /activate/ request is triggered for a theme, we validate & change the `activeTheme` setting
C. On API "override": if uploading a theme with the same name, we override. Using a dirty hack to make this work.

A: setting is done, should load & validate + next request does mounting
B: load is done, should validate & change setting + next request does mounting
C: load, validate & setting are all done + a hack is needed to ensure the next request does mounting

✨ Validate w/ gscan when theme activating on boot
- use the new gscan validation validate.check() method when activating on boot

✨ New concept of active theme
- add ActiveTheme class
- make it possible to set a theme to be active, and to get the active theme
- call the new themes.activate() method in all 3 cases where we activate a theme

🎨 Use new activeTheme to simplify theme code
- make use of the new concept where we can, to reduce & simplify code
- use new hasPartials() method so we don't have to do file lookups
- use path & name getters to reduce use of getContentPath etc
- remove requirement on req.app.get('activeTheme') from static-theme middleware (more on this soon)

🚨 Improve theme unit tests (TODO: fix inter-dep)
- The theme unit tests are borked! They all pass because they don't test the right things.
- This improves them, but they are still dependent on each-other
- configHbsForContext tests don't pass if the activateTheme tests aren't run first
- I will fix this in a later PR

core/server/api/themes.js

@@ -1,6 +1,7 @@
 // # Themes API
 // RESTful API for Themes
-var Promise = require('bluebird'),
+var debug = require('debug')('ghost:api:themes'),
+    Promise = require('bluebird'),
     fs = require('fs-extra'),
     config = require('../config'),
     errors = require('../errors'),
@@ -55,7 +56,10 @@ themes = {
                 return settingsModel.edit(newSettings, options);
             })
             .then(function hasEditedSetting() {
-                // @TODO actually do things to activate the theme, other than just the setting?
+                // Activate! (sort of)
+                debug('Activating theme (method B on API "activate")', themeName);
+                themeUtils.activate(loadedTheme, checkedTheme);
+
                 return themeUtils.toJSON(themeName, checkedTheme);
             });
     },
@@ -107,7 +111,15 @@ themes = {
                 // Sets the theme on the themeList
                 return themeUtils.loadOne(zip.shortName);
             })
-            .then(function () {
+            .then(function (loadedTheme) {
+                // If this is the active theme, we are overriding
+                // This is a special case of activation
+                if (zip.shortName === settingsCache.get('activeTheme')) {
+                    // Activate! (sort of)
+                    debug('Activating theme (method C, on API "override")', zip.shortName);
+                    themeUtils.activate(loadedTheme, checkedTheme);
+                }
+
                 // @TODO: unify the name across gscan and Ghost!
                 return themeUtils.toJSON(zip.shortName, checkedTheme);
             })

core/server/middleware/static-theme.js

@@ -2,6 +2,7 @@ var _       = require('lodash'),
     express = require('express'),
     path    = require('path'),
     config  = require('../config'),
+    themeUtils = require('../themes'),
     utils   = require('../utils');
 
 function isBlackListedFileType(file) {
@@ -17,12 +18,12 @@ function isWhiteListedFile(file) {
 }
 
 function forwardToExpressStatic(req, res, next) {
-    if (!req.app.get('activeTheme')) {
+    if (!themeUtils.getActive()) {
         next();
     } else {
         var configMaxAge = config.get('caching:theme:maxAge');
 
-        express.static(path.join(config.getContentPath('themes'), req.app.get('activeTheme')),
+        express.static(themeUtils.getActive().path,
             {maxAge: (configMaxAge || configMaxAge === 0) ? configMaxAge : utils.ONE_YEAR_MS}
         )(req, res, next);
     }

core/server/middleware/theme-handler.js

@@ -1,13 +1,11 @@
 var _      = require('lodash'),
-    fs     = require('fs'),
-    path   = require('path'),
     hbs    = require('express-hbs'),
     config = require('../config'),
     utils = require('../utils'),
     errors = require('../errors'),
     i18n = require('../i18n'),
     settingsCache = require('../settings/cache'),
-    themeList = require('../themes').list,
+    themeUtils = require('../themes'),
     themeHandler;
 
 themeHandler = {
@@ -52,33 +50,29 @@ themeHandler = {
 
     // ### Activate Theme
     // Helper for updateActiveTheme
-    activateTheme: function activateTheme(blogApp, activeThemeName) {
-        var themePartialsPath = path.join(config.getContentPath('themes'), activeThemeName, 'partials'),
-            hbsOptions = {
+    activateTheme: function activateTheme(blogApp) {
+        var hbsOptions = {
                 partialsDir: [config.get('paths').helperTemplates],
                 onCompile: function onCompile(exhbs, source) {
                     return exhbs.handlebars.compile(source, {preventIndent: true});
                 }
             };
 
-        fs.stat(themePartialsPath, 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(themePartialsPath);
-            }
-        });
+        if (themeUtils.getActive().hasPartials()) {
+            hbsOptions.partialsDir.push(themeUtils.getActive().partialsPath);
+        }
 
         // reset the asset hash
         config.set('assetHash', null);
         // clear the view cache
         blogApp.cache = {};
         // Set the views and engine
-        blogApp.set('views', path.join(config.getContentPath('themes'), activeThemeName));
+        blogApp.set('views', themeUtils.getActive().path);
         blogApp.engine('hbs', hbs.express3(hbsOptions));
 
         // Set active theme variable on the express server
         // Note: this is effectively the "mounted" theme, which has been loaded into the express app
-        blogApp.set('activeTheme', activeThemeName);
+        blogApp.set('activeTheme', themeUtils.getActive().name);
     },
 
     // ### updateActiveTheme
@@ -87,11 +81,13 @@ themeHandler = {
     // is not yet activated.
     updateActiveTheme: function updateActiveTheme(req, res, next) {
         var blogApp = req.app,
+            // We use the settingsCache here, because the setting will be set, even if the theme itself is
+            // not usable because it is invalid or missing.
             activeThemeName = settingsCache.get('activeTheme'),
             mountedThemeName = blogApp.get('activeTheme');
 
         // This means that the theme hasn't been loaded yet i.e. there is no active theme
-        if (!themeList.get(activeThemeName)) {
+        if (!themeUtils.getActive()) {
             // This is the one place we ACTUALLY throw an error for a missing theme
             // As it's a request we cannot serve
             return next(new errors.InternalServerError({
@@ -101,7 +97,7 @@ themeHandler = {
             // If there is an active theme AND it has changed, call activate
         } else if (activeThemeName !== mountedThemeName) {
             // This is effectively "mounting" a theme into express, the theme is already "active"
-            themeHandler.activateTheme(blogApp, activeThemeName);
+            themeHandler.activateTheme(blogApp);
         }
 
         next();

core/server/themes/active.js

@@ -0,0 +1,88 @@
+'use strict';
+
+/**
+ * # Active Theme
+ *
+ * This file defines a class of active theme, and also controls the getting and setting a single instance, as there
+ * can only ever be one active theme. Unlike a singleton, the active theme can change, however only in a controlled way.
+ *
+ * I've made use of the new class & constructor syntax here, as we are now only supporting Node v4 and above, which has
+ * full support for these features.
+ *
+ * There are several different patterns available for keeping data private. Elsewhere in Ghost we use the
+ * naming convention of the _ prefix. Even though this has the downside of not being truly private, it is still one
+ * of the preferred options for keeping data private with the new class syntax, therefore I have kept it.
+ *
+ * No properties marked with an _ should be used directly.
+ *
+ */
+var _ = require('lodash'),
+    join = require('path').join,
+    // Current instance of ActiveTheme
+    currentActiveTheme;
+
+class ActiveTheme {
+    /**
+     * @TODO this API needs to be simpler, but for now should work!
+     * @param {object} loadedTheme - the loaded theme object from the theme list
+     * @param {object} checkedTheme - the result of gscan.format for the theme we're activating
+     */
+    constructor(loadedTheme, checkedTheme) {
+        // Assign some data, mark it all as pseudo-private
+        this._name = loadedTheme.name;
+        this._path = loadedTheme.path;
+
+        // @TODO: get gscan to return validated, useful package.json fields for us!
+        this._packageInfo = loadedTheme['package.json'];
+        this._partials = checkedTheme.partials;
+        // @TODO: get gscan to return a template collection for us
+        this._templates = _.reduce(checkedTheme.files, function (templates, entry) {
+            var tplMatch = entry.file.match(/(^[^\/]+).hbs$/);
+            if (tplMatch) {
+                templates.push(tplMatch[1]);
+            }
+            return templates;
+        }, []);
+    }
+
+    get name() {
+        return this._name;
+    }
+
+    get path() {
+        return this._path;
+    }
+
+    get partialsPath() {
+        return join(this.path, 'partials');
+    }
+
+    hasPartials() {
+        return this._partials.length > 0;
+    }
+
+    hasTemplate(templateName) {
+        return this._templates.indexOf(templateName) > -1;
+    }
+}
+
+module.exports = {
+    get() {
+        return currentActiveTheme;
+    },
+    /**
+     * Set theme
+     *
+     * At this point we trust that the theme has been validated.
+     * Any handling for invalid themes should happen before we get here
+     *
+     * @TODO this API needs to be simpler, but for now should work!
+     * @param {object} loadedTheme - the loaded theme object from the theme list
+     * @param {object} checkedTheme - the result of gscan.format for the theme we're activating
+     * @return {ActiveTheme}
+     */
+    set(loadedTheme, checkedTheme) {
+        currentActiveTheme = new ActiveTheme(loadedTheme, checkedTheme);
+        return currentActiveTheme;
+    }
+};

core/server/themes/index.js

@@ -1,8 +1,12 @@
 var debug = require('debug')('ghost:themes'),
+    _ = require('lodash'),
     events = require('../events'),
+    errors = require('../errors'),
     logging = require('../logging'),
     i18n = require('../i18n'),
     themeLoader = require('./loader'),
+    active = require('./active'),
+    validate = require('./validate'),
     settingsCache = require('../settings/cache');
 
 // @TODO: reduce the amount of things we expose to the outside world
@@ -11,7 +15,9 @@ module.exports = {
     // Init themes module
     // TODO: move this once we're clear what needs to happen here
     init: function initThemes() {
-        var activeThemeName = settingsCache.get('activeTheme');
+        var activeThemeName = settingsCache.get('activeTheme'),
+            self = this;
+
         debug('init themes', activeThemeName);
 
         // Register a listener for server-start to load all themes
@@ -22,6 +28,20 @@ module.exports = {
         // Just read the active theme for now
         return themeLoader
             .loadOneTheme(activeThemeName)
+            .then(function activeThemeHasLoaded(theme) {
+                // Validate
+                return validate
+                    .check(theme)
+                    .then(function resultHandler(checkedTheme) {
+                        // Activate! (sort of)
+                        debug('Activating theme (method A on boot)', activeThemeName);
+                        self.activate(theme, checkedTheme);
+                    })
+                    .catch(function () {
+                        // Active theme is not valid, we don't want to exit because the admin panel will still work
+                        logging.warn(i18n.t('errors.middleware.themehandler.invalidTheme', {theme: activeThemeName}));
+                    });
+            })
             .catch(function () {
                 // Active theme is missing, we don't want to exit because the admin panel will still work
                 logging.warn(i18n.t('errors.middleware.themehandler.missingTheme', {theme: activeThemeName}));
@@ -31,6 +51,17 @@ module.exports = {
     loadAll: themeLoader.loadAllThemes,
     loadOne: themeLoader.loadOneTheme,
     list: require('./list'),
-    validate: require('./validate'),
-    toJSON: require('./to-json')
+    validate: validate,
+    toJSON: require('./to-json'),
+    getActive: active.get,
+    activate: function activate(loadedTheme, checkedTheme) {
+        if (!_.has(checkedTheme, 'results.score.level') || checkedTheme.results.score.level !== 'passing') {
+            throw new errors.InternalServerError({
+                message: i18n.t('errors.middleware.themehandler.invalidTheme', {theme: loadedTheme.name})
+            });
+        }
+
+        // Use the two theme objects to set the current active theme
+        active.set(loadedTheme, checkedTheme);
+    }
 };

core/test/unit/middleware/static-theme_spec.js

@@ -1,104 +1,124 @@
-var sinon        = require('sinon'),
-    should       = require('should'),
+var sinon = require('sinon'),
+    should = require('should'),
 
-    express      = require('express'),
-    staticTheme  = require('../../../server/middleware/static-theme');
+    express = require('express'),
+    themeUtils = require('../../../server/themes'),
+    staticTheme = require('../../../server/middleware/static-theme'),
+
+    sandbox = sinon.sandbox.create();
 
 describe('staticTheme', function () {
-    var next;
+    var expressStaticStub, activeThemeStub, req, res;
 
     beforeEach(function () {
-        next = sinon.spy();
+        req = {};
+        res = {};
+
+        activeThemeStub = sandbox.stub(themeUtils, 'getActive').returns({
+            path: 'my/fake/path'
+        });
+
+        expressStaticStub = sandbox.spy(express, 'static');
     });
 
-    it('should call next if hbs file type', function () {
-        var req = {
-            path: 'mytemplate.hbs'
-        };
-
-        staticTheme(null)(req, null, next);
-        next.called.should.be.true();
+    afterEach(function () {
+        sandbox.restore();
     });
 
-    it('should call next if md file type', function () {
-        var req = {
-            path: 'README.md'
-        };
+    it('should skip for .hbs file', function (done) {
+        req.path = 'mytemplate.hbs';
 
-        staticTheme(null)(req, null, next);
-        next.called.should.be.true();
+        staticTheme()(req, res, function next() {
+            activeThemeStub.called.should.be.false();
+            expressStaticStub.called.should.be.false();
+
+            done();
+        });
     });
 
-    it('should call next if json file type', function () {
-        var req = {
-            path: 'sample.json'
-        };
+    it('should skip for .md file', function (done) {
+        req.path = 'README.md';
 
-        staticTheme(null)(req, null, next);
-        next.called.should.be.true();
+        staticTheme()(req, res, function next() {
+            activeThemeStub.called.should.be.false();
+            expressStaticStub.called.should.be.false();
+
+            done();
+        });
     });
 
-    it('should call express.static if valid file type', function (done) {
-        var req = {
-                path: 'myvalidfile.css',
-                app: {
-                    get: function () { return 'casper'; }
-                }
-            },
-            activeThemeStub,
-            sandbox = sinon.sandbox.create(),
-            expressStatic = sinon.spy(express, 'static');
+    it('should skip for .json file', function (done) {
+        req.path = 'sample.json';
 
-        activeThemeStub = sandbox.spy(req.app, 'get');
+        staticTheme()(req, res, function next() {
+            activeThemeStub.called.should.be.false();
+            expressStaticStub.called.should.be.false();
+
+            done();
+        });
+    });
+
+    it('should call express.static for .css file', function (done) {
+        req.path = 'myvalidfile.css';
+
+        staticTheme()(req, res, function next() {
+            // Specifically gets called twice
+            activeThemeStub.calledTwice.should.be.true();
+            expressStaticStub.called.should.be.true();
+
+            // Check that express static gets called with the theme path + maxAge
+            should.exist(expressStaticStub.firstCall.args);
+            expressStaticStub.firstCall.args[0].should.eql('my/fake/path');
+            expressStaticStub.firstCall.args[1].should.be.an.Object().with.property('maxAge');
+
+            done();
+        });
+    });
+
+    it('should call express.static for .js file', function (done) {
+        req.path = 'myvalidfile.js';
+
+        staticTheme()(req, res, function next() {
+            // Specifically gets called twice
+            activeThemeStub.calledTwice.should.be.true();
+            expressStaticStub.called.should.be.true();
+
+            // Check that express static gets called with the theme path + maxAge
+            should.exist(expressStaticStub.firstCall.args);
+            expressStaticStub.firstCall.args[0].should.eql('my/fake/path');
+            expressStaticStub.firstCall.args[1].should.be.an.Object().with.property('maxAge');
 
-        staticTheme(null)(req, null, function (reqArg, res, next2) {
-            /*jshint unused:false */
-            sandbox.restore();
-            next.called.should.be.false();
-            activeThemeStub.called.should.be.true();
-            expressStatic.called.should.be.true();
-            should.exist(expressStatic.args[0][1].maxAge);
             done();
         });
     });
 
     it('should not error if active theme is missing', function (done) {
-        var req = {
-                path: 'myvalidfile.css',
-                app: {
-                    get: function () { return undefined; }
-                }
-            },
-            activeThemeStub,
-            sandbox = sinon.sandbox.create();
+        req.path = 'myvalidfile.css';
 
-        activeThemeStub = sandbox.spy(req.app, 'get');
+        // make the active theme not exist
+        activeThemeStub.returns(undefined);
+
+        staticTheme()(req, res, function next() {
+            activeThemeStub.calledOnce.should.be.true();
+            expressStaticStub.called.should.be.false();
 
-        staticTheme(null)(req, null, function (reqArg, res, next2) {
-            /*jshint unused:false */
-            sandbox.restore();
-            next.called.should.be.false();
             done();
         });
     });
 
-    it('should not call next if file is on whitelist', function (done) {
-        var req = {
-                path: 'manifest.json',
-                app: {
-                    get: function () { return 'casper'; }
-                }
-            },
-            activeThemeStub,
-            sandbox = sinon.sandbox.create();
+    it('should NOT skip if file is on whitelist', function (done) {
+        req.path = 'manifest.json';
 
-        activeThemeStub = sandbox.spy(req.app, 'get');
+        staticTheme()(req, res, function next() {
+            // Specifically gets called twice
+            activeThemeStub.calledTwice.should.be.true();
+            expressStaticStub.called.should.be.true();
+
+            // Check that express static gets called with the theme path + maxAge
+            should.exist(expressStaticStub.firstCall.args);
+            expressStaticStub.firstCall.args[0].should.eql('my/fake/path');
+            expressStaticStub.firstCall.args[1].should.be.an.Object().with.property('maxAge');
 
-        staticTheme(null)(req, null, function (reqArg, res, next2) {
-            /*jshint unused:false */
-            sandbox.restore();
-            next.called.should.be.false();
-            activeThemeStub.called.should.be.true();
             done();
         });
     });

core/test/unit/middleware/theme-handler_spec.js

@@ -1,23 +1,25 @@
 var sinon = require('sinon'),
     should = require('should'),
     express = require('express'),
-    fs = require('fs'),
     hbs = require('express-hbs'),
-    themeList = require('../../../server/themes').list,
+    configUtils = require('../../utils/configUtils'),
+    themeUtils = require('../../../server/themes'),
+    themeList = themeUtils.list,
     themeHandler = require('../../../server/middleware/theme-handler'),
     settingsCache = require('../../../server/settings/cache'),
 
     sandbox = sinon.sandbox.create();
 
 describe('Theme Handler', function () {
-    var req, res, next, blogApp;
+    var req, res, blogApp, getActiveThemeStub;
 
     beforeEach(function () {
         req = sinon.spy();
         res = sinon.spy();
-        next = sinon.spy();
         blogApp = express();
         req.app = blogApp;
+
+        getActiveThemeStub = sandbox.stub(themeUtils, 'getActive').returns({});
     });
 
     afterEach(function () {
@@ -26,106 +28,143 @@ describe('Theme Handler', function () {
     });
 
     describe('activateTheme', function () {
+        var hbsStub;
+
+        beforeEach(function () {
+            hbsStub = sandbox.spy(hbs, 'express3');
+        });
+
         it('should activate new theme with partials', function () {
-            var fsStub = sandbox.stub(fs, 'stat', function (path, cb) {
-                    cb(null, {isDirectory: function () { return true; }});
-                }),
-                hbsStub = sandbox.spy(hbs, 'express3');
+            getActiveThemeStub.returns({
+                name: 'casper',
+                path: 'my/fake/path',
+                partialsPath: 'my/fake/path/partials',
+                hasPartials: function () {return true;}
+            });
 
-            themeHandler.activateTheme(blogApp, 'casper');
+            themeHandler.activateTheme(blogApp);
 
-            fsStub.calledOnce.should.be.true();
+            // hasPartials, partialsPath, path & name
+            getActiveThemeStub.callCount.should.be.eql(4);
             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);
+            hbsStub.firstCall.args[0].partialsDir.should.be.an.Array().with.lengthOf(2);
+            hbsStub.firstCall.args[0].partialsDir[1].should.eql('my/fake/path/partials');
+
+            // Check the asset hash gets reset
+            should(configUtils.config.get('assetHash')).eql(null);
+
             blogApp.get('activeTheme').should.equal('casper');
+            blogApp.get('views').should.eql('my/fake/path');
         });
 
         it('should activate new theme without partials', function () {
-            var fsStub = sandbox.stub(fs, 'stat', function (path, cb) {
-                    cb(null, null);
-                }),
-                hbsStub = sandbox.spy(hbs, 'express3');
+            getActiveThemeStub.returns({
+                name: 'casper',
+                path: 'my/fake/path',
+                hasPartials: function () {return false;}
+            });
 
-            themeHandler.activateTheme(blogApp, 'casper');
+            themeHandler.activateTheme(blogApp);
 
-            fsStub.calledOnce.should.be.true();
+            // hasPartials, path & name
+            getActiveThemeStub.callCount.should.eql(3);
             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);
+
+            // Check the asset hash gets reset
+            should(configUtils.config.get('assetHash')).eql(null);
+
             blogApp.get('activeTheme').should.equal('casper');
+            blogApp.get('views').should.eql('my/fake/path');
         });
     });
 
+    // NOTE: These tests are totally dependent on the previous tests
+    // @TODO: properly fix these tests once theme refactor is finished
     describe('configHbsForContext', function () {
-        it('handles non secure context', function () {
-            res.locals = {};
-            themeHandler.configHbsForContext(req, res, next);
+        var updateOptionsSpy;
 
-            should.not.exist(res.locals.secure);
-            next.called.should.be.true();
+        beforeEach(function () {
+            updateOptionsSpy = sandbox.spy(hbs, 'updateTemplateOptions');
         });
 
-        it('sets view path', function () {
-            req.secure = true;
+        it('handles non secure context', function (done) {
             res.locals = {};
-            blogApp.set('activeTheme', 'casper');
+            themeHandler.configHbsForContext(req, res, function next() {
+                updateOptionsSpy.calledOnce.should.be.true();
+                should.not.exist(res.locals.secure);
 
-            themeHandler.configHbsForContext(req, res, next);
-
-            blogApp.get('views').should.not.be.undefined();
+                done();
+            });
         });
 
-        it('sets view path', function () {
+        it('handles secure context', function (done) {
             req.secure = true;
             res.locals = {};
-            blogApp.set('activeTheme', 'casper');
+            themeHandler.configHbsForContext(req, res, function next() {
+                updateOptionsSpy.calledOnce.should.be.true();
+                should.exist(res.locals.secure);
+                res.locals.secure.should.be.true();
 
-            themeHandler.configHbsForContext(req, res, next);
-
-            blogApp.get('views').should.not.be.undefined();
+                done();
+            });
         });
     });
 
     describe('updateActiveTheme', function () {
+        var activateThemeStub,
+            settingsCacheStub;
+
         beforeEach(function () {
-            themeList.init({casper: {}});
+            activateThemeStub = sandbox.stub(themeHandler, 'activateTheme');
+            settingsCacheStub = sandbox.stub(settingsCache, 'get').withArgs('activeTheme').returns('casper');
         });
 
         it('updates the active theme if changed', function (done) {
-            var activateThemeSpy = sandbox.spy(themeHandler, 'activateTheme');
-
-            sandbox.stub(settingsCache, 'get').withArgs('activeTheme').returns('casper');
             blogApp.set('activeTheme', 'not-casper');
 
-            themeHandler.updateActiveTheme(req, res, function () {
-                activateThemeSpy.called.should.be.true();
+            themeHandler.updateActiveTheme(req, res, function next(err) {
+                // Did not throw an error
+                should.not.exist(err);
+
+                settingsCacheStub.calledWith('activeTheme').should.be.true();
+                getActiveThemeStub.called.should.be.true();
+                activateThemeStub.called.should.be.true();
+                activateThemeStub.calledWith(blogApp).should.be.true();
+
                 done();
             });
         });
 
         it('does not update the active theme if not changed', function (done) {
-            var activateThemeSpy = sandbox.spy(themeHandler, 'activateTheme');
-
-            sandbox.stub(settingsCache, 'get').withArgs('activeTheme').returns('casper');
             blogApp.set('activeTheme', 'casper');
 
-            themeHandler.updateActiveTheme(req, res, function () {
-                activateThemeSpy.called.should.be.false();
+            themeHandler.updateActiveTheme(req, res, function next(err) {
+                // Did not throw an error
+                should.not.exist(err);
+
+                settingsCacheStub.calledWith('activeTheme').should.be.true();
+                getActiveThemeStub.called.should.be.true();
+                activateThemeStub.called.should.be.false();
+
                 done();
             });
         });
 
         it('throws error if theme is missing', function (done) {
-            var activateThemeSpy = sandbox.spy(themeHandler, 'activateTheme');
+            getActiveThemeStub.returns(undefined);
 
-            sandbox.stub(settingsCache, 'get').withArgs('activeTheme').returns('rasper');
-            blogApp.set('activeTheme', 'not-casper');
-
-            themeHandler.updateActiveTheme(req, res, function (err) {
+            themeHandler.updateActiveTheme(req, res, function next(err) {
+                // Did throw an error
                 should.exist(err);
-                activateThemeSpy.called.should.be.false();
-                err.message.should.eql('The currently active theme "rasper" is missing.');
+                err.message.should.eql('The currently active theme "casper" is missing.');
+
+                settingsCacheStub.calledWith('activeTheme').should.be.true();
+                getActiveThemeStub.called.should.be.true();
+                activateThemeStub.called.should.be.false();
+
                 done();
             });
         });