🐛🎨 Theme API minor improvements (#8197)

no issue

🎨 🐛 Ensure cache is updated correctly for themes
- Insure the cache invalidation headers are always set correctly for the themes API

📖  Theme API comments / function naming
- this is an update for clarity, so we can see what further improvements can be made

🐛 🎨 Add permissions to themes.browse

core/server/api/index.js

@@ -36,8 +36,18 @@ var _              = require('lodash'),
     contentDispositionHeaderExport,
     contentDispositionHeaderSubscribers;
 
-function isActiveThemeOverride(method, endpoint, result) {
-    return method === 'POST' && endpoint === 'themes' && result.themes && result.themes[0] && result.themes[0].active === true;
+function isActiveThemeUpdate(method, endpoint, result) {
+    if (endpoint === 'themes') {
+        if (method === 'PUT') {
+            return true;
+        }
+
+        if (method === 'POST' && result.themes && result.themes[0] && result.themes[0].active === true) {
+            return true;
+        }
+    }
+
+    return false;
 }
 
 /**
@@ -64,7 +74,7 @@ cacheInvalidationHeader = function cacheInvalidationHeader(req, result) {
         hasStatusChanged,
         wasPublishedUpdated;
 
-    if (isActiveThemeOverride(method, endpoint, result)) {
+    if (isActiveThemeUpdate(method, endpoint, result)) {
         // Special case for if we're overwriting an active theme
         // @TODO: remove this crazy DIRTY HORRIBLE HACK
         req.app.set('activeTheme', null);

core/server/api/themes.js

@@ -23,8 +23,15 @@ var debug = require('debug')('ghost:api:themes'),
  * **See:** [API Methods](index.js.html#api%20methods)
  */
 themes = {
-    browse: function browse() {
-        return Promise.resolve(themeUtils.toJSON());
+    browse: function browse(options) {
+        return apiUtils
+            // Permissions
+            .handlePermissions('themes', 'browse')(options)
+            // Main action
+            .then(function makeApiResult() {
+                // Return JSON result
+                return themeUtils.toJSON();
+            });
     },
 
     activate: function activate(options) {
@@ -37,8 +44,10 @@ themes = {
             checkedTheme;
 
         return apiUtils
+            // Permissions
             .handlePermissions('themes', 'activate')(options)
-            .then(function activateTheme() {
+            // Validation
+            .then(function validateTheme() {
                 loadedTheme = themeList.get(themeName);
 
                 if (!loadedTheme) {
@@ -50,16 +59,19 @@ themes = {
 
                 return themeUtils.validate.check(loadedTheme);
             })
-            .then(function haveValidTheme(_checkedTheme) {
+            // Update setting
+            .then(function changeActiveThemeSetting(_checkedTheme) {
                 checkedTheme = _checkedTheme;
                 // We use the model, not the API here, as we don't want to trigger permissions
                 return settingsModel.edit(newSettings, options);
             })
+            // Call activate
             .then(function hasEditedSetting() {
                 // Activate! (sort of)
                 debug('Activating theme (method B on API "activate")', themeName);
                 themeUtils.activate(loadedTheme, checkedTheme);
 
+                // Return JSON result
                 return themeUtils.toJSON(themeName, checkedTheme);
             });
     },
@@ -83,22 +95,27 @@ themes = {
             throw new errors.ValidationError({message: i18n.t('errors.api.themes.overrideCasper')});
         }
 
-        return apiUtils.handlePermissions('themes', 'add')(options)
+        return apiUtils
+            // Permissions
+            .handlePermissions('themes', 'add')(options)
+            // Validation
             .then(function validateTheme() {
                 return themeUtils.validate.check(zip, true);
             })
+            // More validation (existence check)
             .then(function checkExists(_checkedTheme) {
                 checkedTheme = _checkedTheme;
 
                 return storageAdapter.exists(utils.url.urlJoin(config.getContentPath('themes'), zip.shortName));
             })
-            .then(function (themeExists) {
+            // If the theme existed we need to delete it
+            .then(function removeOldTheme(themeExists) {
                 // delete existing theme
                 if (themeExists) {
                     return storageAdapter.delete(zip.shortName, config.getContentPath('themes'));
                 }
             })
-            .then(function () {
+            .then(function storeNewTheme() {
                 events.emit('theme.uploaded', zip.shortName);
                 // store extracted theme
                 return storageAdapter.save({
@@ -106,12 +123,12 @@ themes = {
                     path: checkedTheme.path
                 }, config.getContentPath('themes'));
             })
-            .then(function () {
+            .then(function loadNewTheme() {
                 // Loads the theme from the filesystem
                 // Sets the theme on the themeList
                 return themeUtils.loadOne(zip.shortName);
             })
-            .then(function (loadedTheme) {
+            .then(function activateAndReturn(loadedTheme) {
                 // If this is the active theme, we are overriding
                 // This is a special case of activation
                 if (zip.shortName === settingsCache.get('activeTheme')) {
@@ -153,8 +170,10 @@ themes = {
             return Promise.reject(new errors.BadRequestError({message: i18n.t('errors.api.themes.invalidRequest')}));
         }
 
-        return apiUtils.handlePermissions('themes', 'read')(options)
-            .then(function () {
+        return apiUtils
+            // Permissions
+            .handlePermissions('themes', 'read')(options)
+            .then(function sendTheme() {
                 events.emit('theme.downloaded', themeName);
                 return storageAdapter.serve({isTheme: true, name: themeName});
             });
@@ -165,31 +184,36 @@ themes = {
      * remove theme folder
      */
     destroy: function destroy(options) {
-        var name = options.name,
+        var themeName = options.name,
             theme,
             storageAdapter = storage.getStorage('themes');
 
-        return apiUtils.handlePermissions('themes', 'destroy')(options)
-            .then(function () {
-                if (name === 'casper') {
+        return apiUtils
+            // Permissions
+            .handlePermissions('themes', 'destroy')(options)
+            // Validation
+            .then(function validateTheme() {
+                if (themeName === 'casper') {
                     throw new errors.ValidationError({message: i18n.t('errors.api.themes.destroyCasper')});
                 }
 
-                if (name === settingsCache.get('activeTheme')) {
+                if (themeName === settingsCache.get('activeTheme')) {
                     throw new errors.ValidationError({message: i18n.t('errors.api.themes.destroyActive')});
                 }
 
-                theme = themeList.get(name);
+                theme = themeList.get(themeName);
 
                 if (!theme) {
                     throw new errors.NotFoundError({message: i18n.t('errors.api.themes.themeDoesNotExist')});
                 }
 
-                return storageAdapter.delete(name, config.getContentPath('themes'));
+                // Actually do the deletion here
+                return storageAdapter.delete(themeName, config.getContentPath('themes'));
             })
-            .then(function () {
-                themeList.del(name);
-                events.emit('theme.deleted', name);
+            // And some extra stuff to maintain state here
+            .then(function deleteTheme() {
+                themeList.del(themeName);
+                events.emit('theme.deleted', themeName);
                 // Delete returns an empty 204 response
             });
     }