From 0978a808d612bd615c4952863b557671ec5be21d Mon Sep 17 00:00:00 2001 From: "Fabien \"egg\" O'Carroll" Date: Thu, 20 Oct 2022 11:39:32 +0700 Subject: [PATCH] Refactored TiersAPI to use core slug generation refs https://github.com/TryGhost/Team/issues/2078 This removes the burden from the Tier object, and allows us to reuse the existing slug generation implementation we have in our bookshelf models. --- ghost/tiers/lib/InMemoryTierRepository.js | 10 ------- ghost/tiers/lib/Tier.js | 20 +++++++------- ghost/tiers/lib/TierSlugService.js | 33 ----------------------- ghost/tiers/lib/TiersAPI.js | 18 ++++++++----- ghost/tiers/test/Tier.test.js | 9 ++++--- ghost/tiers/test/TiersAPI.test.js | 7 ++++- 6 files changed, 33 insertions(+), 64 deletions(-) delete mode 100644 ghost/tiers/lib/TierSlugService.js diff --git a/ghost/tiers/lib/InMemoryTierRepository.js b/ghost/tiers/lib/InMemoryTierRepository.js index 4f6f16e9eb..5b6d461704 100644 --- a/ghost/tiers/lib/InMemoryTierRepository.js +++ b/ghost/tiers/lib/InMemoryTierRepository.js @@ -47,16 +47,6 @@ class InMemoryTierRepository { }); } - /** - * @param {string} slug - * @returns {Promise} - */ - async getBySlug(slug) { - return this.#store.find((item) => { - return item.slug === slug; - }); - } - /** * @param {object} [options] * @param {string} [options.filter] diff --git a/ghost/tiers/lib/Tier.js b/ghost/tiers/lib/Tier.js index 0f365ad740..cef403fab3 100644 --- a/ghost/tiers/lib/Tier.js +++ b/ghost/tiers/lib/Tier.js @@ -162,10 +162,9 @@ module.exports = class Tier { /** * @param {any} data - * @param {ISlugService} slugService * @returns {Promise} */ - static async create(data, slugService) { + static async create(data) { let id; if (!data.id) { id = new ObjectID(); @@ -181,13 +180,7 @@ module.exports = class Tier { let name = validateName(data.name); - let slug; - if (data.slug) { - slug = await slugService.validate(data.slug); - } else { - slug = await slugService.generate(name); - } - + let slug = validateSlug(data.slug); let description = validateDescription(data.description); let welcomePageURL = validateWelcomePageURL(data.welcome_page_url); let status = validateStatus(data.status || 'active'); @@ -221,6 +214,15 @@ module.exports = class Tier { } }; +function validateSlug(value) { + if (!value || typeof value !== 'string' || value.length > 191) { + throw new ValidationError({ + message: 'Tier slug must be a string with a maximum of 191 characters' + }); + } + return value; +} + function validateName(value) { if (typeof value !== 'string') { throw new ValidationError({ diff --git a/ghost/tiers/lib/TierSlugService.js b/ghost/tiers/lib/TierSlugService.js deleted file mode 100644 index b874b9b5f9..0000000000 --- a/ghost/tiers/lib/TierSlugService.js +++ /dev/null @@ -1,33 +0,0 @@ -const {ValidationError} = require('@tryghost/errors'); -const {slugify} = require('@tryghost/string'); - -module.exports = class TierSlugService { - /** @type {import('./TiersAPI').ITierRepository} */ - #repository; - - constructor(deps) { - this.#repository = deps.repository; - } - - async validate(slug) { - const exists = !!(await this.#repository.getBySlug(slug)); - - if (!exists) { - return slug; - } - - throw new ValidationError({ - message: 'Slug already exists' - }); - } - - async generate(input, n = 0) { - const slug = slugify(input + (n ? n : '')); - - try { - return await this.validate(slug); - } catch (err) { - return this.generate(input, n + 1); - } - } -}; diff --git a/ghost/tiers/lib/TiersAPI.js b/ghost/tiers/lib/TiersAPI.js index e9d2f3f46e..d1eabb7a4b 100644 --- a/ghost/tiers/lib/TiersAPI.js +++ b/ghost/tiers/lib/TiersAPI.js @@ -1,16 +1,19 @@ const ObjectID = require('bson-objectid').default; const {BadRequestError} = require('@tryghost/errors'); const Tier = require('./Tier'); -const TierSlugService = require('./TierSlugService'); /** * @typedef {object} ITierRepository * @prop {(id: ObjectID) => Promise} getById - * @prop {(slug: string) => Promise} getBySlug * @prop {(tier: Tier) => Promise} save * @prop {(options?: {filter?: string}) => Promise} getAll */ +/** + * @typedef {object} ISlugService + * @prop {(input: string) => Promise} generate + */ + /** * @template {Model} * @typedef {object} Page @@ -29,14 +32,12 @@ module.exports = class TiersAPI { /** @type {ITierRepository} */ #repository; - /** @type {TierSlugService} */ + /** @type {ISlugService} */ #slugService; constructor(deps) { this.#repository = deps.repository; - this.#slugService = new TierSlugService({ - repository: deps.repository - }); + this.#slugService = deps.slugService; } /** @@ -116,7 +117,10 @@ module.exports = class TiersAPI { message: 'Cannot create free Tier' }); } + + const slug = await this.#slugService.generate(data.slug || data.name); const tier = await Tier.create({ + slug, type: 'paid', status: 'active', visibility: data.visibility, @@ -128,7 +132,7 @@ module.exports = class TiersAPI { yearly_price: data.yearly_price, currency: data.currency, trial_days: data.trial_days - }, this.#slugService); + }); await this.#repository.save(tier); diff --git a/ghost/tiers/test/Tier.test.js b/ghost/tiers/test/Tier.test.js index 1665b8f8fb..10f8c104c7 100644 --- a/ghost/tiers/test/Tier.test.js +++ b/ghost/tiers/test/Tier.test.js @@ -35,6 +35,7 @@ const invalidInputs = [ {id: [100]}, {name: 100}, {name: ('a').repeat(200)}, + {slug: ('slug').repeat(50)}, {description: ['whatever?']}, {description: ('b').repeat(200)}, {welcome_page_url: 'hello world'}, @@ -77,7 +78,7 @@ describe('Tier', function () { let input = {}; Object.assign(input, validInput, invalidInput); await assertError(async function () { - await Tier.create(input, {validate: x => x, generate: x => x}); + await Tier.create(input); }); } }); @@ -86,12 +87,12 @@ describe('Tier', function () { for (const validInputItem of validInputs) { let input = {}; Object.assign(input, validInput, validInputItem); - await Tier.create(input, {validate: x => x, generate: x => x}); + await Tier.create(input); } }); it('Can create a Tier with valid input', async function () { - const tier = await Tier.create(validInput, {validate: x => x, generate: x => x}); + const tier = await Tier.create(validInput); const expectedProps = [ 'id', @@ -117,7 +118,7 @@ describe('Tier', function () { }); it('Errors when attempting to set invalid properties', async function () { - const tier = await Tier.create(validInput, {validate: x => x, generate: x => x}); + const tier = await Tier.create(validInput); assertError(() => { tier.name = 20; diff --git a/ghost/tiers/test/TiersAPI.test.js b/ghost/tiers/test/TiersAPI.test.js index 2895cb7ec3..18e6319137 100644 --- a/ghost/tiers/test/TiersAPI.test.js +++ b/ghost/tiers/test/TiersAPI.test.js @@ -12,7 +12,12 @@ describe('TiersAPI', function () { before(function () { repository = new InMemoryTierRepository(); api = new TiersAPI({ - repository + repository, + slugService: { + async generate(input) { + return input; + } + } }); });