🐛 Fixed incorrect slug increments for post title changes

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);

ghost/admin/app/services/slug-generator.js

@@ -10,15 +10,20 @@ export default class SlugGeneratorService extends Service {
     @service ghostPaths;
     @service ajax;
 
-    generateSlug(slugType, textToSlugify) {
+    generateSlug(slugType, textToSlugify, modelId) {
         let url;
 
         if (!textToSlugify) {
             return resolve('');
         }
 
         // 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;

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: [

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();
+        });
+    });
 });

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({

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({

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

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));

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",
+}
+`;

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
+            });
+    });
 });

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(() => {