From d732f83a9ff8032cc76f64df01e20b2a1d41cb8d Mon Sep 17 00:00:00 2001 From: Steve Larson <9larsons@gmail.com> Date: Mon, 17 Jun 2024 16:00:13 -0500 Subject: [PATCH] =?UTF-8?q?=F0=9F=8E=A8=20Improved=20editor=20behavior=20t?= =?UTF-8?q?o=20automatically=20update=20slug=20for=20draft=20posts=20(#203?= =?UTF-8?q?88)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ref https://linear.app/tryghost/issue/ENG-1211 - the post slug now re-generates based on the post title for draft posts unless manually set - updated unit tests to be a bit more comprehensive It's frequently the case that draft posts might have WIP titles. We would generate a post slug based on the title and never change it, so unless writers remembered to come back in to update it for their final post, it could look off to readers. This should make that a bit more intelligent. Going forward, we will change the slug unless we expect it to be a custom slug (user-set in the side panel). If the title is cleared out and saved, we will also reset it. We will only ever automatically generate & change the slug for draft posts. --- ghost/admin/app/controllers/lexical-editor.js | 41 ++-- .../tests/unit/controllers/editor-test.js | 179 ++++++++++-------- 2 files changed, 127 insertions(+), 93 deletions(-) diff --git a/ghost/admin/app/controllers/lexical-editor.js b/ghost/admin/app/controllers/lexical-editor.js index c42b6cd975..ee15feab8f 100644 --- a/ghost/admin/app/controllers/lexical-editor.js +++ b/ghost/admin/app/controllers/lexical-editor.js @@ -25,11 +25,13 @@ import {isHostLimitError, isServerUnreachableError, isVersionMismatchError} from import {isInvalidError} from 'ember-ajax/errors'; import {mobiledocToLexical} from '@tryghost/kg-converters'; import {inject as service} from '@ember/service'; +import {slugify} from '@tryghost/string'; import {tracked} from '@glimmer/tracking'; const DEFAULT_TITLE = '(Untitled)'; // suffix that is applied to the title of a post when it has been duplicated const DUPLICATED_POST_TITLE_SUFFIX = '(Copy)'; + // time in ms to save after last content edit const AUTOSAVE_TIMEOUT = 3000; // time in ms to force a save if the user is continuously typing @@ -860,39 +862,44 @@ export default class LexicalEditorController extends Controller { // this is necessary to force a save when the title is blank this.set('hasDirtyAttributes', true); - // generate slug if post - // - is new and doesn't have a title yet - // - still has the default title - // - previously had a title that ended with the duplicated post title suffix - if ( - (post.get('isNew') && !currentTitle) || - (currentTitle === DEFAULT_TITLE) || - currentTitle?.endsWith(DUPLICATED_POST_TITLE_SUFFIX) - ) { - yield this.generateSlugTask.perform(); - } - + // always save updates automatically for drafts if (this.get('post.isDraft')) { + yield this.generateSlugTask.perform(); yield this.autosaveTask.perform(); } this.ui.updateDocumentTitle(); } + /* + // sync the post slug with the post title, except when: + // - the user has already typed a custom slug, which should not be overwritten + // - the post has been published, so that published URLs are not broken + */ @enqueueTask *generateSlugTask() { - let title = this.get('post.titleScratch'); + const currentTitle = this.get('post.title'); + const newTitle = this.get('post.titleScratch'); + const currentSlug = this.get('post.slug'); // Only set an "untitled" slug once per post - if (title === DEFAULT_TITLE && this.get('post.slug')) { + if (newTitle === DEFAULT_TITLE && currentSlug) { + return; + } + + // Update the slug unless the slug looks to be a custom slug or the title is a default/has been cleared out + if ( + (currentSlug && slugify(currentTitle) !== currentSlug) + && !(currentTitle === DEFAULT_TITLE || currentTitle?.endsWith(DUPLICATED_POST_TITLE_SUFFIX)) + ) { return; } try { - let slug = yield this.slugGenerator.generateSlug('post', title); + const newSlug = yield this.slugGenerator.generateSlug('post', newTitle); - if (!isBlank(slug)) { - this.set('post.slug', slug); + if (!isBlank(newSlug)) { + this.set('post.slug', newSlug); } } catch (error) { // Nothing to do (would be nice to log this somewhere though), diff --git a/ghost/admin/tests/unit/controllers/editor-test.js b/ghost/admin/tests/unit/controllers/editor-test.js index c430fff1ab..09cfd38df6 100644 --- a/ghost/admin/tests/unit/controllers/editor-test.js +++ b/ghost/admin/tests/unit/controllers/editor-test.js @@ -10,10 +10,23 @@ import {task} from 'ember-concurrency'; describe('Unit: Controller: lexical-editor', function () { setupTest(); - describe('generateSlug', function () { + describe.only('generateSlug', function () { + // beforeEach(function () { + // this.controller = this.owner.lookup('controller:lexical-editor'); + // this.controller.set('slugGenerator', EmberObject.create({ + // generateSlug(slugType, str) { + // console.log('--stubbed generateSlug', slugType, str); + // return RSVP.resolve(`${str}-slug`); + // } + // })); + // }); + + // afterEach(async function () { + // await this.controller.reset(); + // }); + 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({ generateSlug(slugType, str) { return RSVP.resolve(`${str}-slug`); @@ -33,7 +46,6 @@ describe('Unit: Controller: lexical-editor', function () { it('should not set the destination if the title is "(Untitled)" and the post already has a slug', async function () { let controller = this.owner.lookup('controller:lexical-editor'); - controller.set('slugGenerator', EmberObject.create({ generateSlug(slugType, str) { return RSVP.resolve(`${str}-slug`); @@ -48,15 +60,95 @@ describe('Unit: Controller: lexical-editor', function () { expect(controller.get('post.slug')).to.equal('whatever'); }); + + it('should generate a new slug if the previous title was (Untitled)', async function () { + let controller = this.owner.lookup('controller:lexical-editor'); + controller.set('slugGenerator', EmberObject.create({ + generateSlug(slugType, str) { + return RSVP.resolve(`${str}-slug`); + } + })); + controller.set('post', EmberObject.create({ + slug: '', + title: '(Untitled)', + titleScratch: 'title' + })); + + await controller.generateSlugTask.perform(); + + expect(controller.get('post.slug')).to.equal('title-slug'); + }); + + it('should generate a new slug if the previous title ended with (Copy)', async function () { + let controller = this.owner.lookup('controller:lexical-editor'); + controller.set('slugGenerator', EmberObject.create({ + generateSlug(slugType, str) { + return RSVP.resolve(`${str}-slug`); + } + })); + + controller.set('post', EmberObject.create({ + slug: '', + title: 'title (Copy)', + titleScratch: 'newTitle' + })); + + await controller.generateSlugTask.perform(); + + expect(controller.get('post.slug')).to.equal('newTitle-slug'); + }); + + it.only('should not generate a new slug if it appears a custom slug was set', async function () { + let controller = this.owner.lookup('controller:lexical-editor'); + controller.set('slugGenerator', EmberObject.create({ + generateSlug(slugType, str) { + return RSVP.resolve(`${str}-slug`); + } + })); + + controller.set('post', EmberObject.create({ + slug: 'custom-slug', + title: 'original title', + titleScratch: 'newTitle' + })); + + expect(controller.get('post.slug')).to.equal('custom-slug'); + expect(controller.get('post.titleScratch')).to.equal('newTitle'); + + await controller.generateSlugTask.perform(); + + expect(controller.get('post.slug')).to.equal('custom-slug'); + }); + + it('should generate new slugs if the title changes', async function () { + let controller = this.owner.lookup('controller:lexical-editor'); + controller.set('slugGenerator', EmberObject.create({ + generateSlug(slugType, str) { + return RSVP.resolve(`${str}-slug`); + } + })); + controller.set('post', EmberObject.create({ + slug: 'somepost', + title: 'somepost', + titleScratch: 'newtitle' + })); + + await controller.generateSlugTask.perform(); + + expect(controller.get('post.slug')).to.equal('newtitle-slug'); + }); }); describe('saveTitleTask', function () { beforeEach(function () { this.controller = this.owner.lookup('controller:lexical-editor'); this.controller.set('target', {send() {}}); + defineProperty(this.controller, 'autosaveTask', task(function * () { + yield RSVP.resolve(); + })); }); - it('should invoke generateSlug if the post is new and a title has not been set', async function () { + it('should invoke generateSlug if the post is not published', async function () { let {controller} = this; controller.set('target', {send() {}}); @@ -64,9 +156,10 @@ describe('Unit: Controller: lexical-editor', function () { this.set('post.slug', 'test-slug'); yield RSVP.resolve(); })); - controller.set('post', EmberObject.create({isNew: true})); - expect(controller.get('post.isNew')).to.be.true; + controller.set('post', EmberObject.create({isDraft: true})); + + expect(controller.get('post.isDraft')).to.be.true; expect(controller.get('post.titleScratch')).to.not.be.ok; controller.set('post.titleScratch', 'test'); @@ -76,62 +169,16 @@ describe('Unit: Controller: lexical-editor', function () { expect(controller.get('post.slug')).to.equal('test-slug'); }); - it('should invoke generateSlug if the post is not new and it\'s title is "(Untitled)"', async function () { + it('should not invoke generateSlug if the post is published', async function () { let {controller} = this; controller.set('target', {send() {}}); - defineProperty(controller, 'generateSlugTask', task(function * () { - this.set('post.slug', 'test-slug'); - yield RSVP.resolve(); - })); - controller.set('post', EmberObject.create({isNew: false, title: '(Untitled)'})); - - expect(controller.get('post.isNew')).to.be.false; - expect(controller.get('post.titleScratch')).to.not.be.ok; - - controller.set('post.titleScratch', 'New Title'); - - await controller.saveTitleTask.perform(); - - expect(controller.get('post.titleScratch')).to.equal('New Title'); - expect(controller.get('post.slug')).to.equal('test-slug'); - }); - - it('should invoke generateSlug if the post is a duplicated post', async function () { - let {controller} = this; - - controller.set('target', {send() {}}); - defineProperty(controller, 'generateSlugTask', task(function * () { - this.set('post.slug', 'test-slug'); - yield RSVP.resolve(); - })); - controller.set('post', EmberObject.create({isNew: false, title: 'Some Title (Copy)'})); - - expect(controller.get('post.isNew')).to.be.false; - expect(controller.get('post.titleScratch')).to.not.be.ok; - - controller.set('post.titleScratch', 'Some Title'); - - await controller.saveTitleTask.perform(); - - expect(controller.get('post.titleScratch')).to.equal('Some Title'); - expect(controller.get('post.slug')).to.equal('test-slug'); - }); - - it('should not invoke generateSlug if the post is new but has a title', async function () { - let {controller} = this; - - controller.set('target', {send() {}}); - defineProperty(controller, 'generateSlugTask', task(function * () { - expect(false, 'generateSlug should not be called').to.equal(true); - yield RSVP.resolve(); - })); controller.set('post', EmberObject.create({ - isNew: true, - title: 'a title' + title: 'a title', + isPublished: true })); - expect(controller.get('post.isNew')).to.be.true; + expect(controller.get('post.isPublished')).to.be.true; expect(controller.get('post.title')).to.equal('a title'); expect(controller.get('post.titleScratch')).to.not.be.ok; @@ -141,26 +188,6 @@ describe('Unit: Controller: lexical-editor', function () { expect(controller.get('post.titleScratch')).to.equal('test'); expect(controller.get('post.slug')).to.not.be.ok; }); - - it('should not invoke generateSlug if the post is not new and the title is not "(Untitled)"', async function () { - let {controller} = this; - - controller.set('target', {send() {}}); - defineProperty(controller, 'generateSlugTask', task(function * () { - expect(false, 'generateSlug should not be called').to.equal(true); - yield RSVP.resolve(); - })); - controller.set('post', EmberObject.create({isNew: false})); - - expect(controller.get('post.isNew')).to.be.false; - expect(controller.get('post.title')).to.not.be.ok; - - controller.set('post.titleScratch', 'title'); - await controller.saveTitleTask.perform(); - - expect(controller.get('post.titleScratch')).to.equal('title'); - expect(controller.get('post.slug')).to.not.be.ok; - }); }); describe('TK count in title', function () {