Rework the themeService public API

refs: https://github.com/TryGhost/Team/issues/831

- This ultimately fixes the index.js file
- It also makes it super clear what methods in the themeService are used by the API, and which are part of the service loading logic
- It also moves the activate and init function into a single file in a way that highlights they are very similar
- They are also very similar to what happens in storage.setFromZip but that code is mixed up with storage code at the moment

core/server/api/canary/themes.js

@@ -16,7 +16,7 @@ module.exports = {
     browse: {
         permissions: true,
         query() {
-            return themeService.getJSON();
+            return themeService.api.getJSON();
         }
     },
 
@@ -47,14 +47,14 @@ module.exports = {
                 value: themeName
             }];
 
-            return themeService.activate(themeName)
+            return themeService.api.activate(themeName)
                 .then((checkedTheme) => {
                     // @NOTE: we use the model, not the API here, as we don't want to trigger permissions
                     return models.Settings.edit(newSettings, frame.options)
                         .then(() => checkedTheme);
                 })
                 .then((checkedTheme) => {
-                    return themeService.getJSON(themeName, checkedTheme);
+                    return themeService.api.getJSON(themeName, checkedTheme);
                 });
         }
     },
@@ -115,7 +115,7 @@ module.exports = {
                         path: downloadPath,
                         name: zipName
                     };
-                    const {theme, themeOverridden} = await themeService.storage.setFromZip(zip);
+                    const {theme, themeOverridden} = await themeService.api.setFromZip(zip);
 
                     if (themeOverridden) {
                         this.headers.cacheInvalidate = true;
@@ -160,7 +160,7 @@ module.exports = {
                 name: frame.file.originalname
             };
 
-            return themeService.storage.setFromZip(zip)
+            return themeService.api.setFromZip(zip)
                 .then(({theme, themeOverridden}) => {
                     if (themeOverridden) {
                         // CASE: clear cache
@@ -189,7 +189,7 @@ module.exports = {
         query(frame) {
             let themeName = frame.options.name;
 
-            return themeService.storage.getZip(themeName);
+            return themeService.api.getZip(themeName);
         }
     },
 
@@ -212,7 +212,7 @@ module.exports = {
         query(frame) {
             let themeName = frame.options.name;
 
-            return themeService.storage.destroy(themeName);
+            return themeService.api.destroy(themeName);
         }
     }
 };

core/server/api/v2/themes.js

@@ -9,7 +9,7 @@ module.exports = {
     browse: {
         permissions: true,
         query() {
-            return themeService.getJSON();
+            return themeService.api.getJSON();
         }
     },
 
@@ -40,14 +40,14 @@ module.exports = {
                 value: themeName
             }];
 
-            return themeService.activate(themeName)
+            return themeService.api.activate(themeName)
                 .then((checkedTheme) => {
                     // @NOTE: we use the model, not the API here, as we don't want to trigger permissions
                     return models.Settings.edit(newSettings, frame.options)
                         .then(() => checkedTheme);
                 })
                 .then((checkedTheme) => {
-                    return themeService.getJSON(themeName, checkedTheme);
+                    return themeService.api.getJSON(themeName, checkedTheme);
                 });
         }
     },
@@ -73,7 +73,7 @@ module.exports = {
                 name: frame.file.originalname
             };
 
-            return themeService.storage.setFromZip(zip)
+            return themeService.api.setFromZip(zip)
                 .then(({theme, themeOverridden}) => {
                     if (themeOverridden) {
                         // CASE: clear cache
@@ -102,7 +102,7 @@ module.exports = {
         query(frame) {
             let themeName = frame.options.name;
 
-            return themeService.storage.getZip(themeName);
+            return themeService.api.getZip(themeName);
         }
     },
 
@@ -125,7 +125,7 @@ module.exports = {
         query(frame) {
             let themeName = frame.options.name;
 
-            return themeService.storage.destroy(themeName);
+            return themeService.api.destroy(themeName);
         }
     }
 };

core/server/api/v3/themes.js

@@ -9,7 +9,7 @@ module.exports = {
     browse: {
         permissions: true,
         query() {
-            return themeService.getJSON();
+            return themeService.api.getJSON();
         }
     },
 
@@ -40,14 +40,14 @@ module.exports = {
                 value: themeName
             }];
 
-            return themeService.activate(themeName)
+            return themeService.api.activate(themeName)
                 .then((checkedTheme) => {
                     // @NOTE: we use the model, not the API here, as we don't want to trigger permissions
                     return models.Settings.edit(newSettings, frame.options)
                         .then(() => checkedTheme);
                 })
                 .then((checkedTheme) => {
-                    return themeService.getJSON(themeName, checkedTheme);
+                    return themeService.api.getJSON(themeName, checkedTheme);
                 });
         }
     },
@@ -71,7 +71,7 @@ module.exports = {
                 name: frame.file.originalname
             };
 
-            return themeService.storage.setFromZip(zip)
+            return themeService.api.setFromZip(zip)
                 .then(({theme, themeOverridden}) => {
                     if (themeOverridden) {
                         // CASE: clear cache
@@ -100,7 +100,7 @@ module.exports = {
         query(frame) {
             let themeName = frame.options.name;
 
-            return themeService.storage.getZip(themeName);
+            return themeService.api.getZip(themeName);
         }
     },
 
@@ -123,7 +123,7 @@ module.exports = {
         query(frame) {
             let themeName = frame.options.name;
 
-            return themeService.storage.destroy(themeName);
+            return themeService.api.destroy(themeName);
         }
     }
 };

core/server/services/themes/activate.js

@@ -0,0 +1,62 @@
+const debug = require('@tryghost/debug')('themes');
+
+const errors = require('@tryghost/errors');
+const logging = require('@tryghost/logging');
+const tpl = require('@tryghost/tpl');
+
+const activator = require('./activation-bridge');
+const list = require('./list');
+const themeLoader = require('./loader');
+const validate = require('./validate');
+
+const messages = {
+    activeThemeIsMissing: 'The currently active theme "{theme}" is missing.',
+    themeCannotBeActivated: '{themeName} cannot be activated because it was not found in the theme directory.'
+};
+
+module.exports.loadAndActivate = async (themeName) => {
+    debug('loadAndActivate', themeName);
+    try {
+        // Just read the active theme for now
+        const loadedTheme = await themeLoader.loadOneTheme(themeName);
+        // Validate
+        // @NOTE: this is now the only usage of check, rather than checkSafe...
+        const checkedTheme = await validate.check(loadedTheme);
+
+        if (!validate.canActivate(checkedTheme)) {
+            logging.error(validate.getThemeValidationError('activeThemeHasFatalErrors', themeName, checkedTheme));
+        } else if (checkedTheme.results.error.length) {
+            // CASE: inform that the theme has errors, but not fatal (theme still works)
+            logging.warn(validate.getThemeValidationError('activeThemeHasErrors', themeName, checkedTheme));
+        }
+
+        activator.activateFromBoot(themeName, loadedTheme, checkedTheme);
+    } catch (err) {
+        if (err instanceof errors.NotFoundError) {
+            // CASE: active theme is missing, we don't want to exit because the admin panel will still work
+            err.message = tpl(messages.activeThemeIsMissing, {theme: themeName});
+        }
+
+        // CASE: theme threw an odd error, we don't want to exit because the admin panel will still work
+        // This is the absolute catch-all, at this point, we do not know what went wrong!
+        logging.error(err);
+    }
+};
+
+module.exports.activate = async (themeName) => {
+    const loadedTheme = list.get(themeName);
+
+    if (!loadedTheme) {
+        throw new errors.ValidationError({
+            message: tpl(messages.themeCannotBeActivated, {themeName: themeName}),
+            errorDetails: themeName
+        });
+    }
+
+    // Validate
+    const checkedTheme = await validate.checkSafe(themeName, loadedTheme);
+    // Activate
+    activator.activateFromAPI(themeName, loadedTheme, checkedTheme);
+    // Return the checked theme
+    return checkedTheme;
+};

core/server/services/themes/index.js

@@ -1,73 +1,31 @@
-const debug = require('@tryghost/debug')('themes');
-const logging = require('@tryghost/logging');
-const errors = require('@tryghost/errors');
-const tpl = require('@tryghost/tpl');
+const activate = require('./activate');
 const themeLoader = require('./loader');
-const validate = require('./validate');
-const list = require('./list');
-const activator = require('./activation-bridge');
+const storage = require('./storage');
+const getJSON = require('./to-json');
+
 const settingsCache = require('../../../shared/settings-cache');
 
-const messages = {
-    activeThemeIsMissing: 'The currently active theme "{theme}" is missing.',
-    themeCannotBeActivated: '{themeName} cannot be activated because it was not found in the theme directory.'
-};
-
 module.exports = {
-    // Init themes module
-    // TODO: move this once we're clear what needs to happen here
+    /*
+     * Load the currently active theme
+     */
     init: async () => {
-        const activeThemeName = settingsCache.get('active_theme');
+        const themeName = settingsCache.get('active_theme');
 
-        debug('init themes', activeThemeName);
-        try {
-            // Just read the active theme for now
-            const theme = await themeLoader.loadOneTheme(activeThemeName);
-            // Validate
-            // @NOTE: this is now the only usage of check, rather than checkSafe...
-            const checkedTheme = await validate.check(theme);
-
-            if (!validate.canActivate(checkedTheme)) {
-                logging.error(validate.getThemeValidationError('activeThemeHasFatalErrors', activeThemeName, checkedTheme));
-            } else if (checkedTheme.results.error.length) {
-                // CASE: inform that the theme has errors, but not fatal (theme still works)
-                logging.warn(validate.getThemeValidationError('activeThemeHasErrors', activeThemeName, checkedTheme));
-            }
-
-            activator.activateFromBoot(activeThemeName, theme, checkedTheme);
-        } catch (err) {
-            if (err instanceof errors.NotFoundError) {
-                // CASE: active theme is missing, we don't want to exit because the admin panel will still work
-                err.message = tpl(messages.activeThemeIsMissing, {theme: activeThemeName});
-            }
-
-            // CASE: theme threw an odd error, we don't want to exit because the admin panel will still work
-            // This is the absolute catch-all, at this point, we do not know what went wrong!
-            logging.error(err);
-        }
+        return activate.loadAndActivate(themeName);
     },
-    getJSON: require('./to-json'),
-    activate: async (themeName) => {
-        const loadedTheme = list.get(themeName);
-
-        if (!loadedTheme) {
-            throw new errors.ValidationError({
-                message: tpl(messages.themeCannotBeActivated, {themeName: themeName}),
-                errorDetails: themeName
-            });
-        }
-
-        const checkedTheme = await validate.checkSafe(themeName, loadedTheme);
-
-        activator.activateFromAPI(themeName, loadedTheme, checkedTheme);
-
-        return checkedTheme;
-    },
-    storage: require('./storage'),
     /**
      * Load all inactive themes
      */
-    loadInactiveThemes: async () => {
-        return await themeLoader.loadAllThemes();
+    loadInactiveThemes: themeLoader.loadAllThemes,
+    /**
+     * Methods used in the API
+     */
+    api: {
+        getJSON,
+        activate: activate.activate,
+        getZip: storage.getZip,
+        setFromZip: storage.setFromZip,
+        destroy: storage.destroy
     }
 };

core/server/services/themes/storage.js

@@ -14,7 +14,6 @@ const activator = require('./activation-bridge');
 const toJSON = require('./to-json');
 
 const settingsCache = require('../../../shared/settings-cache');
-const bridge = require('../../../bridge');
 
 const messages = {
     themeDoesNotExist: 'Theme does not exist.',
@@ -47,8 +46,8 @@ module.exports = {
         });
     },
     setFromZip: async (zip) => {
-        const shortName = getStorage().getSanitizedFileName(zip.name.split('.zip')[0]);
-        const backupName = `${shortName}_${ObjectID()}`;
+        const themeName = getStorage().getSanitizedFileName(zip.name.split('.zip')[0]);
+        const backupName = `${themeName}_${ObjectID()}`;
 
         // check if zip name is casper.zip
         if (zip.name === 'casper.zip') {
@@ -62,38 +61,40 @@ module.exports = {
         let renamedExisting = false;
 
         try {
-            checkedTheme = await validate.checkSafe(shortName, zip, true);
-            const themeExists = await getStorage().exists(shortName);
+            checkedTheme = await validate.checkSafe(themeName, zip, true);
+            const themeExists = await getStorage().exists(themeName);
             // CASE: move the existing theme to a backup folder
             if (themeExists) {
+                debug('setFromZip Theme exists already');
                 renamedExisting = true;
-                await getStorage().rename(shortName, backupName);
+                await getStorage().rename(themeName, backupName);
             }
 
             // CASE: store extracted theme
             await getStorage().save({
-                name: shortName,
+                name: themeName,
                 path: checkedTheme.path
             });
             // CASE: loads the theme from the fs & sets the theme on the themeList
-            const loadedTheme = await themeLoader.loadOneTheme(shortName);
-            overrideTheme = (shortName === settingsCache.get('active_theme'));
+            const loadedTheme = await themeLoader.loadOneTheme(themeName);
+            overrideTheme = (themeName === settingsCache.get('active_theme'));
             // CASE: if this is the active theme, we are overriding
             if (overrideTheme) {
-                activator.activateFromOverride(shortName, loadedTheme, checkedTheme);
+                debug('setFromZip Theme is active alreadu');
+                activator.activateFromOverride(themeName, loadedTheme, checkedTheme);
             }
 
             // @TODO: unify the name across gscan and Ghost!
             return {
                 themeOverridden: overrideTheme,
-                theme: toJSON(shortName, checkedTheme)
+                theme: toJSON(themeName, checkedTheme)
             };
         } catch (error) {
             // restore backup if we renamed an existing theme but saving failed
             if (renamedExisting) {
-                return getStorage().exists(shortName).then((themeExists) => {
+                return getStorage().exists(themeName).then((themeExists) => {
                     if (!themeExists) {
-                        return getStorage().rename(backupName, shortName).then(() => {
+                        return getStorage().rename(backupName, themeName).then(() => {
                             throw error;
                         });
                     }