diff --git a/ghost/admin/app/controllers/lexical-editor.js b/ghost/admin/app/controllers/lexical-editor.js index c2746b846954..dbd2b0098749 100644 --- a/ghost/admin/app/controllers/lexical-editor.js +++ b/ghost/admin/app/controllers/lexical-editor.js @@ -784,7 +784,7 @@ export default class LexicalEditorController extends Controller { return; } - serverSlug = yield this.slugGenerator.generateSlug('post', newSlug); + serverSlug = yield this.slugGenerator.generateSlug('post', newSlug, this.get('post.id')); // If after getting the sanitized and unique slug back from the API // we end up with a slug that matches the existing slug, abort the change if (serverSlug === slug) { @@ -961,7 +961,7 @@ export default class LexicalEditorController extends Controller { } try { - const newSlug = yield this.slugGenerator.generateSlug('post', newTitle); + const newSlug = yield this.slugGenerator.generateSlug('post', newTitle, this.get('post.id')); if (!isBlank(newSlug)) { this.set('post.slug', newSlug); diff --git a/ghost/admin/app/services/slug-generator.js b/ghost/admin/app/services/slug-generator.js index 397340e075ca..e77481274a1e 100644 --- a/ghost/admin/app/services/slug-generator.js +++ b/ghost/admin/app/services/slug-generator.js @@ -10,7 +10,7 @@ export default class SlugGeneratorService extends Service { @service ghostPaths; @service ajax; - generateSlug(slugType, textToSlugify) { + generateSlug(slugType, textToSlugify, modelId) { let url; if (!textToSlugify) { @@ -18,7 +18,12 @@ export default class SlugGeneratorService extends Service { } // We already do a partial slugify at the client side to prevent issues with Pro returning a 404 page because of invalid (encoded) characters (a newline, %0A, for example) - url = this.get('ghostPaths.url').api('slugs', slugType, encodeURIComponent(slugify(textToSlugify))); + let name = encodeURIComponent(slugify(textToSlugify)); + if (modelId) { + url = this.get('ghostPaths.url').api('slugs', slugType, name, modelId); + } else { + url = this.get('ghostPaths.url').api('slugs', slugType, name); + } return this.ajax.request(url).then((response) => { let [firstSlug] = response.slugs; diff --git a/ghost/admin/mirage/config/slugs.js b/ghost/admin/mirage/config/slugs.js index e9dabe9eb74e..0f0eadfed3a0 100644 --- a/ghost/admin/mirage/config/slugs.js +++ b/ghost/admin/mirage/config/slugs.js @@ -9,6 +9,14 @@ export default function mockSlugs(server) { }; }); + server.get('/slugs/post/:slug/:id', function (schema, request) { + return { + slugs: [ + {slug: dasherize(decodeURIComponent(request.params.slug))} + ] + }; + }); + server.get('/slugs/user/:slug/', function (schema, request) { return { slugs: [ diff --git a/ghost/admin/tests/integration/services/slug-generator-test.js b/ghost/admin/tests/integration/services/slug-generator-test.js index 2ac4961e3497..452c856f6151 100644 --- a/ghost/admin/tests/integration/services/slug-generator-test.js +++ b/ghost/admin/tests/integration/services/slug-generator-test.js @@ -5,17 +5,31 @@ import {describe, it} from 'mocha'; import {expect} from 'chai'; import {setupTest} from 'ember-mocha'; -function stubSlugEndpoint(server, type, slug) { - server.get(`${ghostPaths().apiRoot}/slugs/:type/:slug/`, function (request) { - expect(request.params.type).to.equal(type); - expect(request.params.slug).to.equal(slug); - - return [ - 200, - {'Content-Type': 'application/json'}, - JSON.stringify({slugs: [{slug: dasherize(slug)}]}) - ]; - }); +function stubSlugEndpoint(server, type, slug, id) { + if (id) { + server.get(`${ghostPaths().apiRoot}/slugs/:type/:slug/:id`, function (request) { + expect(request.params.type).to.equal(type); + expect(request.params.slug).to.equal(slug); + expect(request.params.id).to.equal(id); + + return [ + 200, + {'Content-Type': 'application/json'}, + JSON.stringify({slugs: [{slug: dasherize(slug)}]}) + ]; + }); + } else { + server.get(`${ghostPaths().apiRoot}/slugs/:type/:slug/`, function (request) { + expect(request.params.type).to.equal(type); + expect(request.params.slug).to.equal(slug); + + return [ + 200, + {'Content-Type': 'application/json'}, + JSON.stringify({slugs: [{slug: dasherize(slug)}]}) + ]; + }); + } } describe('Integration: Service: slug-generator', function () { @@ -51,4 +65,16 @@ describe('Integration: Service: slug-generator', function () { done(); }); }); + + it('calls correct endpoint and returns correct data when passed an id', function (done) { + let rawSlug = 'a test post'; + stubSlugEndpoint(server, 'post', 'a-test-post', 'a-test-id'); + + let service = this.owner.lookup('service:slug-generator'); + + service.generateSlug('post', rawSlug, 'a-test-id').then(function (slug) { + expect(slug).to.equal(dasherize(rawSlug)); + done(); + }); + }); }); diff --git a/ghost/admin/tests/unit/controllers/editor-test.js b/ghost/admin/tests/unit/controllers/editor-test.js index d1557c5086ec..4c203fc26651 100644 --- a/ghost/admin/tests/unit/controllers/editor-test.js +++ b/ghost/admin/tests/unit/controllers/editor-test.js @@ -23,6 +23,28 @@ describe('Unit: Controller: lexical-editor', function () { }); describe('generateSlug', function () { + it('should generate a slug and set it on the post, passing the id if it exists', async function () { + let controller = this.owner.lookup('controller:lexical-editor'); + controller.set('slugGenerator', EmberObject.create({ + generateSlug(slugType, str, id) { + if (id !== 'fake-id') { + throw new Error('Expected id "fake-id" to be passed to generateSlug.'); + } + return RSVP.resolve(`${str}-slug`); + } + })); + controller.set('post', createPost({id: 'fake-id', slug: ''})); + + controller.set('post.titleScratch', 'title'); + await settled(); + + expect(controller.get('post.slug')).to.equal(''); + + await controller.generateSlugTask.perform(); + + expect(controller.get('post.slug')).to.equal('title-slug'); + }); + it('should generate a slug and set it on the post', async function () { let controller = this.owner.lookup('controller:lexical-editor'); controller.set('slugGenerator', EmberObject.create({ diff --git a/ghost/core/core/server/api/endpoints/slugs.js b/ghost/core/core/server/api/endpoints/slugs.js index 0e9ed157489d..e9aebc9e0844 100644 --- a/ghost/core/core/server/api/endpoints/slugs.js +++ b/ghost/core/core/server/api/endpoints/slugs.js @@ -21,7 +21,8 @@ const controller = { }, options: [ 'include', - 'type' + 'type', + 'id' ], data: [ 'name' @@ -32,6 +33,9 @@ const controller = { type: { required: true, values: Object.keys(allowedTypes) + }, + id: { + required: false } }, data: { @@ -41,7 +45,7 @@ const controller = { } }, query(frame) { - return models.Base.Model.generateSlug(allowedTypes[frame.options.type], frame.data.name, {status: 'all'}) + return models.Base.Model.generateSlug(allowedTypes[frame.options.type], frame.data.name, {status: 'all', modelId: frame.options.id}) .then((slug) => { if (!slug) { return Promise.reject(new errors.InternalServerError({ diff --git a/ghost/core/core/server/models/base/plugins/generate-slug.js b/ghost/core/core/server/models/base/plugins/generate-slug.js index 84d4dfa1b09d..1b1577058b82 100644 --- a/ghost/core/core/server/models/base/plugins/generate-slug.js +++ b/ghost/core/core/server/models/base/plugins/generate-slug.js @@ -39,6 +39,12 @@ module.exports = function (Bookshelf) { return slugToFind; } + if (options.modelId) { + if (found.id === options.modelId) { + return slugToFind; + } + } + slugTryCount += 1; // If we shortened, go back to the full version and try again diff --git a/ghost/core/core/server/web/api/endpoints/admin/routes.js b/ghost/core/core/server/web/api/endpoints/admin/routes.js index 090616e73ca6..6a9df77aeb23 100644 --- a/ghost/core/core/server/web/api/endpoints/admin/routes.js +++ b/ghost/core/core/server/web/api/endpoints/admin/routes.js @@ -168,6 +168,7 @@ module.exports = function apiRoutes() { // ## Slugs router.get('/slugs/:type/:name', mw.authAdminApi, http(api.slugs.generate)); + router.get('/slugs/:type/:name/:id', mw.authAdminApi, http(api.slugs.generate)); // ## Themes router.get('/themes/', mw.authAdminApi, http(api.themes.browse)); diff --git a/ghost/core/test/e2e-api/admin/__snapshots__/slugs.test.js.snap b/ghost/core/test/e2e-api/admin/__snapshots__/slugs.test.js.snap index 678c35b41ff4..ab9a7ca7e07d 100644 --- a/ghost/core/test/e2e-api/admin/__snapshots__/slugs.test.js.snap +++ b/ghost/core/test/e2e-api/admin/__snapshots__/slugs.test.js.snap @@ -22,3 +22,72 @@ Object { "x-powered-by": "Express", } `; + +exports[`Slug API Can handle collisions of a different resource if an id is provided 1: [body] 1`] = ` +Object { + "slugs": Array [ + Object { + "slug": "integrations-2", + }, + ], +} +`; + +exports[`Slug API Can handle collisions of a different resource if an id is provided 2: [headers] 1`] = ` +Object { + "access-control-allow-origin": "http://127.0.0.1:2369", + "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", + "content-length": "37", + "content-type": "application/json; charset=utf-8", + "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, + "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, + "vary": "Accept-Version, Origin, Accept-Encoding", + "x-powered-by": "Express", +} +`; + +exports[`Slug API Can handle collisions of the same resource if an id is provided 1: [body] 1`] = ` +Object { + "slugs": Array [ + Object { + "slug": "integrations", + }, + ], +} +`; + +exports[`Slug API Can handle collisions of the same resource if an id is provided 2: [headers] 1`] = ` +Object { + "access-control-allow-origin": "http://127.0.0.1:2369", + "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", + "content-length": "35", + "content-type": "application/json; charset=utf-8", + "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, + "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, + "vary": "Accept-Version, Origin, Accept-Encoding", + "x-powered-by": "Express", +} +`; + +exports[`Slug API Will increment the slug if there is a collision 1: [body] 1`] = ` +Object { + "slugs": Array [ + Object { + "slug": "integrations-2", + }, + ], +} +`; + +exports[`Slug API Will increment the slug if there is a collision 2: [headers] 1`] = ` +Object { + "access-control-allow-origin": "http://127.0.0.1:2369", + "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", + "content-length": "37", + "content-type": "application/json; charset=utf-8", + "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, + "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, + "vary": "Accept-Version, Origin, Accept-Encoding", + "x-powered-by": "Express", +} +`; diff --git a/ghost/core/test/e2e-api/admin/slugs.test.js b/ghost/core/test/e2e-api/admin/slugs.test.js index 9661b4ef4e5f..4c55f4a763fe 100644 --- a/ghost/core/test/e2e-api/admin/slugs.test.js +++ b/ghost/core/test/e2e-api/admin/slugs.test.js @@ -21,4 +21,37 @@ describe('Slug API', function () { etag: anyEtag }); }); + + it('Will increment the slug if there is a collision', async function () { + await agent + .get('slugs/post/integrations/') + .expectStatus(200) + .matchBodySnapshot() + .matchHeaderSnapshot({ + 'content-version': anyContentVersion, + etag: anyEtag + }); + }); + + it('Can handle collisions of the same resource if an id is provided', async function () { + await agent + .get('slugs/post/integrations/6194d3ce51e2700162531a71') + .expectStatus(200) + .matchBodySnapshot() + .matchHeaderSnapshot({ + 'content-version': anyContentVersion, + etag: anyEtag + }); + }); + + it('Can handle collisions of a different resource if an id is provided', async function () { + await agent + .get('slugs/post/integrations/000000000000000000000000') + .expectStatus(200) + .matchBodySnapshot() + .matchHeaderSnapshot({ + 'content-version': anyContentVersion, + etag: anyEtag + }); + }); }); diff --git a/ghost/core/test/unit/server/models/base/index.test.js b/ghost/core/test/unit/server/models/base/index.test.js index ebc716b5958f..5588eaa04a4b 100644 --- a/ghost/core/test/unit/server/models/base/index.test.js +++ b/ghost/core/test/unit/server/models/base/index.test.js @@ -39,6 +39,42 @@ describe('Models: base', function () { }); }); + it('slug exists but it does not exist for the id', function () { + let i = 0; + Model.findOne.callsFake(() => { + i = i + 1; + if (i === 1) { + return Promise.resolve({id: 'correct-model-id'}); + } + return Promise.resolve(null); + }); + + security.string.safe.withArgs('My-Slug').returns('my-slug'); + + return models.Base.Model.generateSlug(Model, 'My-Slug', {modelId: 'incorrect-model-id'}) + .then((slug) => { + slug.should.eql('my-slug-2'); + }); + }); + + it('slug exists but it exists for the id', function () { + let i = 0; + Model.findOne.callsFake(() => { + i = i + 1; + if (i === 1) { + return Promise.resolve({id: 'correct-model-id'}); + } + return Promise.resolve(null); + }); + + security.string.safe.withArgs('My-Slug').returns('my-slug'); + + return models.Base.Model.generateSlug(Model, 'My-Slug', {modelId: 'correct-model-id'}) + .then((slug) => { + slug.should.eql('my-slug'); + }); + }); + it('slug exists', function () { let i = 0; Model.findOne.callsFake(() => {