diff --git a/core/frontend/services/themes/ThemeStorage.js b/core/frontend/services/themes/ThemeStorage.js index a4a5e4fc3f..8992427aac 100644 --- a/core/frontend/services/themes/ThemeStorage.js +++ b/core/frontend/services/themes/ThemeStorage.js @@ -57,8 +57,26 @@ class ThemeStorage extends LocalFileStorage { }; } + /** + * Rename a file / folder + * + * + * @param String fileName + */ + rename(srcName, destName) { + let src = path.join(this.getTargetDir(), srcName); + let dest = path.join(this.getTargetDir(), destName); + + return fs.move(src, dest); + } + + /** + * Rename a file / folder + * + * @param String backupName + */ delete(fileName) { - return fs.remove(path.join(this.storagePath, fileName)); + return fs.remove(path.join(this.getTargetDir(), fileName)); } } diff --git a/core/frontend/services/themes/storage.js b/core/frontend/services/themes/storage.js index 1e7fd77429..7fe5111fdf 100644 --- a/core/frontend/services/themes/storage.js +++ b/core/frontend/services/themes/storage.js @@ -12,6 +12,7 @@ const {i18n} = require('../../../server/lib/common'); const logging = require('../../../shared/logging'); const errors = require('@tryghost/errors'); const debug = require('ghost-ignition').debug('api:themes'); +const ObjectID = require('bson-objectid'); let themeStorage; @@ -37,6 +38,7 @@ module.exports = { }, setFromZip: (zip) => { const shortName = getStorage().getSanitizedFileName(zip.name.split('.zip')[0]); + const backupName = `${shortName}_${ObjectID()}`; // check if zip name is casper.zip if (zip.name === 'casper.zip') { @@ -54,9 +56,9 @@ module.exports = { return getStorage().exists(shortName); }) .then((themeExists) => { - // CASE: delete existing theme + // CASE: move the existing theme to a backup folder if (themeExists) { - return getStorage().delete(shortName); + return getStorage().rename(shortName, backupName); } }) .then(() => { @@ -86,14 +88,20 @@ module.exports = { }) .finally(() => { // @TODO: we should probably do this as part of saving the theme - // CASE: remove extracted dir from gscan - // happens in background + // CASE: remove extracted dir from gscan happens in background if (checkedTheme) { fs.remove(checkedTheme.path) .catch((err) => { logging.error(new errors.GhostError({err: err})); }); } + + // CASE: remove the backup we created earlier + getStorage() + .delete(backupName) + .catch((err) => { + logging.error(new errors.GhostError({err: err})); + }); }); }, destroy: function (themeName) { diff --git a/test/api-acceptance/admin/themes_spec.js b/test/api-acceptance/admin/themes_spec.js index 584a114bb3..50417bd9c9 100644 --- a/test/api-acceptance/admin/themes_spec.js +++ b/test/api-acceptance/admin/themes_spec.js @@ -118,20 +118,11 @@ describe('Themes API', function () { tmpFolderContents.splice(index, 1); } }); - tmpFolderContents.should.be.an.Array().with.lengthOf(10); - tmpFolderContents.should.eql([ - 'broken-theme', - 'casper', - 'casper-1.4', - 'casper.zip', - 'invalid.zip', - 'test-theme', - 'test-theme-channels', - 'valid', - 'valid.zip', - 'warnings.zip' - ]); + // Note: at this point, the tmpFolder can legitimately still contain a valid_34324324 backup + // As it is deleted asynchronously + tmpFolderContents.should.containEql('valid'); + tmpFolderContents.should.containEql('valid.zip'); // Check the Themes API returns the correct result return ownerRequest @@ -157,6 +148,16 @@ describe('Themes API', function () { should.exist(addedTheme); localUtils.API.checkResponse(addedTheme, 'theme'); addedTheme.active.should.be.false(); + + // Note: at this point, the API should not return a valid_34324324 backup folder as a theme + _.map(jsonResponse.themes, 'name').should.eql([ + 'broken-theme', + 'casper', + 'casper-1.4', + 'test-theme', + 'test-theme-channels', + 'valid' + ]); }); });