From 71e2a06b25ad93a9d4bb812eab433b52baf81973 Mon Sep 17 00:00:00 2001 From: Naz Date: Thu, 23 Sep 2021 13:21:44 +0200 Subject: [PATCH] Reworked ensure-settings module to take in singular file path refs https://linear.app/tryghost/issue/CORE-35/refactor-route-and-redirect-settings - This is a micro-step towards getting rid of multiple "knownSettings" concept. Since the introduction of an array of knowSettings there was never-ever a need to handle anything but a single `routes.yaml` file. Getting rid of this concept first to have a simpler module. Next step would be getting rid of filesystem reads/writes in the "frontend" --- .../services/settings/ensure-settings.js | 54 ++++++++----------- core/frontend/services/settings/index.js | 8 ++- .../services/settings/ensure-settings.test.js | 26 ++------- 3 files changed, 30 insertions(+), 58 deletions(-) diff --git a/core/frontend/services/settings/ensure-settings.js b/core/frontend/services/settings/ensure-settings.js index 199278e432..cbe53ffce9 100644 --- a/core/frontend/services/settings/ensure-settings.js +++ b/core/frontend/services/settings/ensure-settings.js @@ -7,41 +7,33 @@ const errors = require('@tryghost/errors'); const config = require('../../../shared/config'); /** - * Makes sure that all supported settings files are in the - * `/content/settings` directory. If not, copy the default files - * over. - * @param {Array} knownSettings - * @returns {Promise} - * @description Reads the `/settings` folder of the content path and makes - * sure that the associated yaml file for each setting exists. If it doesn't - * copy the default yaml file over. +* Makes sure file is in the `/content/settings` directory. If not, copy the default over. + * @param {String} fileName - name of the setting file + * @returns {Promise} */ -module.exports = function ensureSettingsFiles(knownSettings) { +module.exports = function ensureSettingsFile(fileName) { const contentPath = config.getContentPath('settings'); const defaultSettingsPath = config.get('paths').defaultSettings; - return Promise.each(knownSettings, function (setting) { - const fileName = `${setting}.yaml`; - const defaultFileName = `default-${fileName}`; - const filePath = path.join(contentPath, fileName); + const defaultFileName = `default-${fileName}`; + const filePath = path.join(contentPath, fileName); - return Promise.resolve(fs.readFile(filePath, 'utf8')) - .catch({code: 'ENOENT'}, () => { - const defaultFilePath = path.join(defaultSettingsPath, defaultFileName); - // CASE: file doesn't exist, copy it from our defaults - return fs.copy( - defaultFilePath, - filePath - ).then(() => { - debug(`'${defaultFileName}' copied to ${contentPath}.`); - }); - }).catch((error) => { - // CASE: we might have a permission error, as we can't access the directory - throw new errors.GhostError({ - message: i18n.t('errors.services.settings.ensureSettings', {path: contentPath}), - err: error, - context: error.path - }); + return Promise.resolve(fs.readFile(filePath, 'utf8')) + .catch({code: 'ENOENT'}, () => { + const defaultFilePath = path.join(defaultSettingsPath, defaultFileName); + // CASE: file doesn't exist, copy it from our defaults + return fs.copy( + defaultFilePath, + filePath + ).then(() => { + debug(`'${defaultFileName}' copied to ${contentPath}.`); }); - }); + }).catch((error) => { + // CASE: we might have a permission error, as we can't access the directory + throw new errors.GhostError({ + message: i18n.t('errors.services.settings.ensureSettings', {path: contentPath}), + err: error, + context: error.path + }); + }); }; diff --git a/core/frontend/services/settings/index.js b/core/frontend/services/settings/index.js index fb04b6a927..77cd6e7218 100644 --- a/core/frontend/services/settings/index.js +++ b/core/frontend/services/settings/index.js @@ -2,7 +2,7 @@ const _ = require('lodash'); const crypto = require('crypto'); const debug = require('@tryghost/debug')('frontend:services:settings:index'); const SettingsLoader = require('./loader'); -const ensureSettingsFiles = require('./ensure-settings'); +const ensureSettingsFile = require('./ensure-settings'); const errors = require('@tryghost/errors'); @@ -21,13 +21,11 @@ const calculateHash = (data) => { module.exports = { init: function () { - const knownSettings = this.knownSettings(); - - debug('init settings service for:', knownSettings); + debug('init routes settings service'); // Make sure that supported settings files are available // inside of the `content/setting` directory - return ensureSettingsFiles(knownSettings); + return ensureSettingsFile('routes.yaml'); }, /** diff --git a/test/unit/services/settings/ensure-settings.test.js b/test/unit/services/settings/ensure-settings.test.js index fe5846794b..22ab690bb5 100644 --- a/test/unit/services/settings/ensure-settings.test.js +++ b/test/unit/services/settings/ensure-settings.test.js @@ -19,32 +19,14 @@ describe('UNIT > Settings Service ensure settings:', function () { describe('Ensure settings files', function () { it('returns yaml file from settings folder if it exists', function () { - fs.readFile.withArgs(path.join(__dirname, '../../../utils/fixtures/settings/badroutes.yaml'), 'utf8').resolves('content'); fs.readFile.withArgs(path.join(__dirname, '../../../utils/fixtures/settings/goodroutes.yaml'), 'utf8').resolves('content'); - return ensureSettings(['goodroutes', 'badroutes']).then(() => { - fs.readFile.callCount.should.be.eql(2); + return ensureSettings('goodroutes.yaml').then(() => { + fs.readFile.callCount.should.be.eql(1); fs.copy.called.should.be.false(); }); }); - it('copies default settings file if not found but does not overwrite existing files', function () { - const expectedDefaultSettingsPath = path.join(__dirname, '../../../../core/frontend/services/settings/default-globals.yaml'); - const expectedContentPath = path.join(__dirname, '../../../utils/fixtures/settings/globals.yaml'); - const fsError = new Error('not found'); - fsError.code = 'ENOENT'; - - fs.readFile.withArgs(path.join(__dirname, '../../../utils/fixtures/settings/routes.yaml'), 'utf8').resolves('content'); - fs.readFile.withArgs(path.join(__dirname, '../../../utils/fixtures/settings/globals.yaml'), 'utf8').rejects(fsError); - fs.copy.withArgs(expectedDefaultSettingsPath, expectedContentPath).resolves(); - - return ensureSettings(['routes', 'globals']) - .then(() => { - fs.readFile.calledTwice.should.be.true(); - fs.copy.calledOnce.should.be.true(); - }); - }); - it('copies default settings file if no file found', function () { const expectedDefaultSettingsPath = path.join(__dirname, '../../../../core/frontend/services/settings/default-routes.yaml'); const expectedContentPath = path.join(__dirname, '../../../utils/fixtures/settings/routes.yaml'); @@ -54,7 +36,7 @@ describe('UNIT > Settings Service ensure settings:', function () { fs.readFile.withArgs(path.join(__dirname, '../../../utils/fixtures/settings/routes.yaml'), 'utf8').rejects(fsError); fs.copy.withArgs(expectedDefaultSettingsPath, expectedContentPath).resolves(); - return ensureSettings(['routes']).then(() => { + return ensureSettings('routes.yaml').then(() => { fs.readFile.calledOnce.should.be.true(); fs.copy.calledOnce.should.be.true(); }); @@ -67,7 +49,7 @@ describe('UNIT > Settings Service ensure settings:', function () { fs.readFile.withArgs(path.join(__dirname, '../../../utils/fixtures/settings/routes.yaml'), 'utf8').rejects(fsError); - return ensureSettings(['routes']) + return ensureSettings('routes.yaml') .then(() => { throw new Error('Expected test to fail'); })