From 6ed5f64f4b852426a24fe3ea78dcdc151b0c858d Mon Sep 17 00:00:00 2001 From: Naz Date: Thu, 25 Nov 2021 14:48:22 +0400 Subject: [PATCH] Moved backup path calculation outside redirects module refs https://github.com/TryGhost/Toolbox/issues/139 - Having tight coupling with backup file path calculation for redirects makes it extremely hard to test. In addition, having it injected will make it easier to swap this dependency to the mechanism similar to one used for routes files --- core/server/services/redirects/api.js | 19 ++++++------------- core/server/services/redirects/index.js | 2 ++ core/server/services/redirects/utils.js | 14 ++++++++++++++ .../server/services/redirects/api.test.js | 19 +++++++++++++------ 4 files changed, 35 insertions(+), 19 deletions(-) create mode 100644 core/server/services/redirects/utils.js diff --git a/core/server/services/redirects/api.js b/core/server/services/redirects/api.js index f4710e245b..0abe0b6e87 100644 --- a/core/server/services/redirects/api.js +++ b/core/server/services/redirects/api.js @@ -1,6 +1,5 @@ const fs = require('fs-extra'); const path = require('path'); -const moment = require('moment-timezone'); const yaml = require('js-yaml'); const logging = require('@tryghost/logging'); @@ -118,16 +117,6 @@ const parseRedirectsFile = (content, ext) => { throw new errors.IncorrectUsageError(); }; -/** - * @param {string} filePath - * @returns {string} - */ -const getBackupRedirectsFilePath = (filePath) => { - const {dir, name, ext} = path.parse(filePath); - - return path.join(dir, `${name}-${moment().format('YYYY-MM-DD-HH-mm-ss')}${ext}`); -}; - /** * @typedef {object} IRedirectManager */ @@ -137,17 +126,21 @@ class CustomRedirectsAPI { * @param {object} config * @param {string} config.basePath * @param {Function} config.validate - validates redirects configuration + * @param {Function} config.getBackupFilePath * @param {IRedirectManager} config.redirectManager */ - constructor({basePath, validate, redirectManager}) { + constructor({basePath, validate, redirectManager, getBackupFilePath}) { /** @private */ this.basePath = basePath; /** @private */ this.redirectManager = redirectManager; - /**private */ + /** @private */ this.validate = validate; + + /** @private */ + this.getBackupFilePath = getBackupFilePath; } async init() { diff --git a/core/server/services/redirects/index.js b/core/server/services/redirects/index.js index 0307905351..18cf5c4b6b 100644 --- a/core/server/services/redirects/index.js +++ b/core/server/services/redirects/index.js @@ -4,6 +4,7 @@ const urlUtils = require('../../../shared/url-utils'); const DynamicRedirectManager = require('@tryghost/express-dynamic-redirects'); const CustomRedirectsAPI = require('./api'); const validation = require('./validation'); +const {getBackupRedirectsFilePath} = require('./utils'); let customRedirectsAPI; let redirectManager; @@ -20,6 +21,7 @@ module.exports = { customRedirectsAPI = new CustomRedirectsAPI({ basePath: config.getContentPath('data'), redirectManager, + getBackupFilePath: getBackupRedirectsFilePath, validate: validation.validate.bind(validation) }); diff --git a/core/server/services/redirects/utils.js b/core/server/services/redirects/utils.js new file mode 100644 index 0000000000..bf75c0d874 --- /dev/null +++ b/core/server/services/redirects/utils.js @@ -0,0 +1,14 @@ +const path = require('path'); +const moment = require('moment-timezone'); + +/** + * @param {string} filePath + * @returns {string} + */ +const getBackupRedirectsFilePath = (filePath) => { + const {dir, name, ext} = path.parse(filePath); + + return path.join(dir, `${name}-${moment().format('YYYY-MM-DD-HH-mm-ss')}${ext}`); +}; + +module.exports.getBackupRedirectsFilePath = getBackupRedirectsFilePath; diff --git a/test/unit/server/services/redirects/api.test.js b/test/unit/server/services/redirects/api.test.js index 70dfa4ae09..69056a27f2 100644 --- a/test/unit/server/services/redirects/api.test.js +++ b/test/unit/server/services/redirects/api.test.js @@ -9,10 +9,11 @@ const CustomRedirectsAPI = require('../../../../../core/server/services/redirect describe('UNIT: redirects CustomRedirectsAPI class', function () { let customRedirectsAPI; + let redirectManager; const basePath = path.join(__dirname, '../../../../utils/fixtures/data/'); before(function () { - const redirectManager = new DynamicRedirectManager({ + redirectManager = new DynamicRedirectManager({ permanentMaxAge: 100, getSubdirectoryURL: (pathname) => { return `/ghost/${pathname}`; @@ -20,8 +21,11 @@ describe('UNIT: redirects CustomRedirectsAPI class', function () { }); customRedirectsAPI = new CustomRedirectsAPI({ - basePath - }, redirectManager); + basePath, + redirectManager, + getBackupFilePath: () => path.join(basePath, 'backup.json'), + validate: () => {} + }); }); beforeEach(function () { @@ -36,7 +40,7 @@ describe('UNIT: redirects CustomRedirectsAPI class', function () { describe('init', function () { it('initializes without errors when redirects file is not present', async function () { - const redirectManager = new DynamicRedirectManager({ + redirectManager = new DynamicRedirectManager({ permanentMaxAge: 100, getSubdirectoryURL: (pathname) => { return `/ghost/${pathname}`; @@ -44,8 +48,11 @@ describe('UNIT: redirects CustomRedirectsAPI class', function () { }); customRedirectsAPI = new CustomRedirectsAPI({ - basePath - }, redirectManager); + basePath, + redirectManager, + getBackupFilePath: {}, + validate: () => {} + }); await customRedirectsAPI.init(); logging.error.called.should.be.false();