🐛 Fixed theme settings reset when editing Source or Casper

apps/admin/src/settings/app/components/settings/site/theme/theme-code-editor-modal.tsx

@@ -766,7 +766,11 @@ const ThemeCodeEditorModal: React.FC<{themeName: string}> = ({themeName}) => {
             const formData = new FormData();
             formData.append('file', blob, `${nextThemeName}.zip`);
 
-            const response = await fetch(`${getGhostPaths().apiRoot}/themes/upload/`, {
+            // when saving under a new name, carry over the original theme's
+            // settings so activating the copy keeps the site's design
+            const uploadQuery = isSaveAs ? `?copy_settings_from=${encodeURIComponent(previousThemeName)}` : '';
+
+            const response = await fetch(`${getGhostPaths().apiRoot}/themes/upload/${uploadQuery}`, {
                 method: 'POST',
                 credentials: 'include',
                 headers: {

apps/admin/src/settings/site/theme.acceptance.test.tsx

@@ -241,7 +241,10 @@ describe("Theme settings", () => {
         fakeThemeWorld();
         await fakeThemeDownload("casper");
         await fakeThemeDownload("casper-edited");
-        const uploadApi = fakeAdminEndpoint("POST", "/themes/upload/", { themes: [theme({ name: "casper-edited" })] });
+        // saving under a new name carries over the original theme's settings
+        const uploadApi = fakeAdminEndpoint("POST", "/themes/upload/?copy_settings_from=casper", {
+            themes: [theme({ name: "casper-edited" })],
+        });
         await renderAdminApp("/settings/theme/edit/casper");
 
         const editor = await editorTextbox();

ghost/core/core/server/api/endpoints/themes.js

@@ -106,6 +106,9 @@ const controller = {
         headers: {
             cacheInvalidate: false
         },
+        options: [
+            'copy_settings_from'
+        ],
         permissions: {
             method: 'add'
         },
@@ -123,7 +126,9 @@ const controller = {
                 name: frame.file.originalname
             };
 
-            const {theme, themeOverridden} = await themeService.api.setFromZip(zip);
+            const {theme, themeOverridden} = await themeService.api.setFromZip(zip, {
+                copySettingsFrom: frame.options.copy_settings_from
+            });
             if (themeOverridden) {
                 frame.setHeader('X-Cache-Invalidate', '/*');
             }

ghost/core/core/server/services/themes/storage.js

@@ -8,6 +8,7 @@ const errors = require('@tryghost/errors');
 
 const validate = require('./validate');
 const list = require('./list');
+const customThemeSettings = require('../custom-theme-settings');
 const ThemeStorage = require('./theme-storage');
 const themeLoader = require('./loader');
 const activator = require('./activation-bridge');
@@ -24,7 +25,8 @@ const messages = {
     invalidThemeName: 'Please select a valid theme.',
     overrideDefaultTheme: 'Please rename your zip, it\'s not allowed to override the default theme.',
     destroyDefaultTheme: 'Deleting the default theme is not allowed.',
-    destroyActive: 'Deleting the active theme is not allowed.'
+    destroyActive: 'Deleting the active theme is not allowed.',
+    copySettingsFromDoesNotExist: 'Theme to copy settings from is not installed.'
 };
 
 const INVALID_THEME_REGEX = /^[./]*$/;
@@ -51,7 +53,7 @@ module.exports = {
             name: themeName
         });
     },
-    setFromZip: async (zip) => {
+    setFromZip: async (zip, {copySettingsFrom} = {}) => {
         const themeName = getStorage().getSanitizedFileName(zip.name.split('.zip')[0]);
         const backupName = `${themeName}_${ObjectID()}`;
 
@@ -69,12 +71,27 @@ module.exports = {
             });
         }
 
+        if (copySettingsFrom && !list.get(copySettingsFrom)) {
+            throw new errors.ValidationError({
+                message: tpl(messages.copySettingsFromDoesNotExist)
+            });
+        }
+
         let checkedTheme;
         let overrideTheme;
         let renamedExisting = false;
 
         try {
             checkedTheme = await validate.checkSafe(themeName, zip, true);
+
+            // CASE: theme uploaded as a copy of another theme, carry over that
+            // theme's settings so activating the copy keeps the site's design.
+            // Happens before any file changes so a failed copy leaves the
+            // installed themes untouched
+            if (copySettingsFrom) {
+                await customThemeSettings.api.copySettingsBetweenThemes(copySettingsFrom, themeName);
+            }
+
             const themeExists = await getStorage().exists(themeName);
             // CASE: move the existing theme to a backup folder
             if (themeExists) {

ghost/core/core/shared/custom-theme-settings-cache/custom-theme-settings-bread-service.js

@@ -26,4 +26,8 @@ module.exports = class CustomThemeSettingsBREADService {
     async destroy(data, options = {}) {
         return this.Model.destroy(data, options);
     }
+
+    async transaction(fn) {
+        return this.Model.transaction(fn);
+    }
 };

ghost/core/core/shared/custom-theme-settings-cache/custom-theme-settings-service.js

@@ -164,6 +164,45 @@ module.exports = class CustomThemeSettingsService {
         return settingsObjects;
     }
 
+    /**
+     * Duplicate stored settings from one theme to another, e.g. when a theme
+     * is saved as a copy under a new name.
+     *
+     * No-ops if the destination theme already has stored settings so existing
+     * customisations are never overwritten. Values are copied verbatim - they
+     * are reconciled against the destination theme's settings definition by
+     * the sync that runs when that theme is activated.
+     *
+     * @param {string} fromThemeName
+     * @param {string} toThemeName
+     */
+    async copySettingsBetweenThemes(fromThemeName, toThemeName) {
+        const sourceCollection = await this._repository.browse({filter: `theme:'${fromThemeName}'`});
+
+        // single transaction so a failure part-way leaves no partial copy
+        // behind that would make later attempts skip the copy. The
+        // destination check locks inside the same transaction so concurrent
+        // copies can't both see an empty destination and insert duplicates
+        await this._repository.transaction(async (transacting) => {
+            const destinationCollection = await this._repository.browse({filter: `theme:'${toThemeName}'`, transacting, forUpdate: true});
+
+            if (destinationCollection.toJSON().length > 0) {
+                debug(`Skipping copy of custom theme settings from '${fromThemeName}' to '${toThemeName}' - destination already has settings`);
+                return;
+            }
+
+            for (const setting of sourceCollection.toJSON()) {
+                debug(`Copying custom theme setting '${fromThemeName}.${setting.key}' to '${toThemeName}'`);
+                await this._repository.add({
+                    theme: toThemeName,
+                    key: setting.key,
+                    type: setting.type,
+                    value: setting.value
+                }, {transacting});
+            }
+        });
+    }
+
     // Private -----------------------------------------------------------------
 
     /**

ghost/core/test/e2e-api/admin/themes.test.js

@@ -3,6 +3,7 @@ const {assertExists} = require('../../utils/assertions');
 const sinon = require('sinon');
 const path = require('path');
 const fs = require('fs');
+const os = require('os');
 const _ = require('lodash');
 const supertest = require('supertest');
 const nock = require('nock');
@@ -19,9 +20,10 @@ describe('Themes API', function () {
         const themePath = options.themePath;
         const fieldName = 'file';
         const request = options.request || ownerRequest;
+        const query = options.query || '';
 
         return request
-            .post(localUtils.API.getApiQuery('themes/upload'))
+            .post(localUtils.API.getApiQuery(`themes/upload${query}`))
             .set('Origin', config.get('url'))
             .attach(fieldName, themePath);
     };
@@ -425,6 +427,75 @@ describe('Themes API', function () {
         mockManager.restoreLimitService();
     });
 
+    it('Can copy custom theme settings when uploading a theme under a new name', async function () {
+        const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'theme-settings-copy-'));
+
+        try {
+            // start from a known active theme and customise its settings
+            await ownerRequest
+                .put(localUtils.API.getApiQuery('themes/source/activate'))
+                .set('Origin', config.get('url'))
+                .expect(200);
+
+            await ownerRequest
+                .put(localUtils.API.getApiQuery('custom_theme_settings/'))
+                .set('Origin', config.get('url'))
+                .send({custom_theme_settings: [
+                    {key: 'title_font', value: 'Elegant serif'},
+                    {key: 'site_background_color', value: '#123456'}
+                ]})
+                .expect(200);
+
+            // save a copy of the default theme under a new name, as the theme editor does
+            const zipPath = path.join(tmpDir, 'source-edited.zip');
+            fs.copyFileSync(path.join(__dirname, '..', '..', 'utils', 'fixtures', 'themes', 'source.zip'), zipPath);
+
+            const uploadRes = await uploadTheme({
+                themePath: zipPath,
+                query: '?copy_settings_from=source'
+            });
+            assert.equal(uploadRes.statusCode, 200);
+            assert.equal(uploadRes.body.themes[0].name, 'source-edited');
+
+            await ownerRequest
+                .put(localUtils.API.getApiQuery('themes/source-edited/activate'))
+                .set('Origin', config.get('url'))
+                .expect(200);
+
+            // customised values survived the switch to the renamed copy
+            const settingsRes = await ownerRequest
+                .get(localUtils.API.getApiQuery('custom_theme_settings/'))
+                .set('Origin', config.get('url'))
+                .expect(200);
+
+            const settingsByKey = Object.fromEntries(settingsRes.body.custom_theme_settings.map(setting => [setting.key, setting.value]));
+            assert.equal(settingsByKey.title_font, 'Elegant serif');
+            assert.equal(settingsByKey.site_background_color, '#123456');
+        } finally {
+            fs.rmSync(tmpDir, {recursive: true, force: true});
+
+            // best-effort restore of the pre-test theme state, no assertions so
+            // cleanup completes even when the test fails part-way through
+            await ownerRequest
+                .put(localUtils.API.getApiQuery('themes/source/activate'))
+                .set('Origin', config.get('url'));
+
+            await ownerRequest
+                .del(localUtils.API.getApiQuery('themes/source-edited'))
+                .set('Origin', config.get('url'));
+        }
+    });
+
+    it('Errors when asked to copy settings from an unknown theme', async function () {
+        const res = await uploadTheme({
+            themePath: path.join(__dirname, '..', '..', 'utils', 'fixtures', 'themes', 'valid.zip'),
+            query: '?copy_settings_from=unknown-theme'
+        });
+
+        assert.equal(res.statusCode, 422);
+        assert.equal(res.body.errors[0].type, 'ValidationError');
+    });
+
     it('Can re-upload the active theme to override', async function () {
         // The tricky thing about this test is the default active theme is Source and you're not allowed to override it.
         // So we upload a valid theme, activate it, and then upload again.

ghost/core/test/unit/shared/custom-theme-settings-cache/service.test.js

@@ -64,6 +64,18 @@ class ModelStub {
         this.knownSettings = this.knownSettings.filter(setting => setting !== destroyedSetting);
         return destroyedSetting;
     }
+
+    async transaction(fn) {
+        const snapshot = this.knownSettings.slice();
+        this.lastTransacting = {};
+
+        try {
+            return await fn(this.lastTransacting);
+        } catch (error) {
+            this.knownSettings = snapshot;
+            throw error;
+        }
+    }
 }
 
 describe('Service', function () {
@@ -485,6 +497,84 @@ describe('Service', function () {
         });
     });
 
+    describe('copySettingsBetweenThemes()', function () {
+        const settingsForTheme = async (themeName) => {
+            const collection = await model.findAll({filter: `theme:'${themeName}'`});
+            return collection.toJSON().map(({theme, key, type, value}) => ({theme, key, type, value}));
+        };
+
+        it('copies settings rows to the new theme name', async function () {
+            await service.copySettingsBetweenThemes('test', 'test-edited');
+
+            // the destination check is locked inside the transaction so
+            // concurrent copies can't both see an empty destination
+            const destinationCheck = model.findAll.getCalls().find(call => call.firstArg.filter === `theme:'test-edited'`);
+            assert.equal(destinationCheck.firstArg.transacting, model.lastTransacting);
+            assert.equal(destinationCheck.firstArg.forUpdate, true);
+
+            // every insert runs in the same transaction
+            model.add.getCalls().forEach((call) => {
+                assert.equal(call.args[1].transacting, model.lastTransacting);
+            });
+
+            assert.deepEqual(await settingsForTheme('test-edited'), [{
+                theme: 'test-edited',
+                key: 'one',
+                type: 'select',
+                value: '1'
+            }, {
+                theme: 'test-edited',
+                key: 'two',
+                type: 'select',
+                value: '2'
+            }]);
+
+            // source theme rows are untouched
+            assert.equal((await settingsForTheme('test')).length, 2);
+        });
+
+        it('does not overwrite existing settings for the destination theme', async function () {
+            await model.add({theme: 'test-edited', key: 'one', type: 'select', value: 'existing'});
+
+            await service.copySettingsBetweenThemes('test', 'test-edited');
+
+            assert.deepEqual(await settingsForTheme('test-edited'), [{
+                theme: 'test-edited',
+                key: 'one',
+                type: 'select',
+                value: 'existing'
+            }]);
+        });
+
+        it('is a no-op when the source theme has no settings', async function () {
+            await service.copySettingsBetweenThemes('unknown', 'test-edited');
+
+            assert.equal((await settingsForTheme('test-edited')).length, 0);
+            sinon.assert.notCalled(model.add);
+        });
+
+        it('leaves no partial copy behind when the copy fails part-way', async function () {
+            const originalAdd = model.add;
+            let addCalls = 0;
+            model.add = async function (data, options) {
+                addCalls += 1;
+                if (addCalls === 2) {
+                    throw new Error('second add failed');
+                }
+                return originalAdd.call(model, data, options);
+            };
+            // atomicity must not depend on individual deletes succeeding
+            model.destroy = async () => {
+                throw new Error('destroy failed');
+            };
+
+            await assert.rejects(service.copySettingsBetweenThemes('test', 'test-edited'), /second add failed/);
+
+            model.add = originalAdd;
+            assert.equal((await settingsForTheme('test-edited')).length, 0);
+        });
+    });
+
     describe('updateSettings()', function () {
         it('saves new values', async function () {
             // activate theme so settings are loaded in internal cache