From 06262ecf33ad1aa89c570864e4928681de136e3a Mon Sep 17 00:00:00 2001 From: Chris Raible Date: Fri, 21 Apr 2023 10:04:05 +0100 Subject: [PATCH] Added logic for saving revisions on explicit saves (#16688) refs @TryGhost/Team#3076 - added `save_revision` option to edit post endpoint - this change covers the following cases: 1. we will not save a `post_revision` on every background autosave that occurs after 3 seconds of inactivity in the editor 2. we will save a `post_revision` when the user hits `cmd+s` in the editor to explicitly save 3. we will save a `post_revision` when the user navigates away from the editor (e.g. by clicking the 'Posts' breadcrumb in the editor) 4. we will save a `post_revision` when the user publishes a post 5. we will save a `post_revision` when a user updates an already published post --- ghost/admin/app/adapters/post.js | 4 ++ ghost/admin/app/controllers/lexical-editor.js | 34 ++++++++++++--- ghost/core/core/server/api/endpoints/posts.js | 1 + ghost/core/core/server/models/post.js | 11 +++-- ghost/core/test/e2e-api/admin/posts.test.js | 2 +- ghost/post-revisions/lib/post-revisions.js | 23 +++++++++-- ghost/post-revisions/test/hello.test.js | 41 ++++++++++++++++--- 7 files changed, 97 insertions(+), 19 deletions(-) diff --git a/ghost/admin/app/adapters/post.js b/ghost/admin/app/adapters/post.js index c6c744a563..4d76af9ce4 100644 --- a/ghost/admin/app/adapters/post.js +++ b/ghost/admin/app/adapters/post.js @@ -20,6 +20,10 @@ export default class Post extends ApplicationAdapter { } } + if (snapshot?.adapterOptions?.saveRevision) { + const saveRevision = snapshot.adapterOptions.saveRevision; + parsedUrl.searchParams.append('save_revision', saveRevision); + } return parsedUrl.toString(); } diff --git a/ghost/admin/app/controllers/lexical-editor.js b/ghost/admin/app/controllers/lexical-editor.js index 10f3a18ce9..fb0e14bfb6 100644 --- a/ghost/admin/app/controllers/lexical-editor.js +++ b/ghost/admin/app/controllers/lexical-editor.js @@ -406,11 +406,12 @@ export default class LexicalEditorController extends Controller { // separate task for autosave so that it doesn't override a manual save @dropTask - *autosaveTask() { + *autosaveTask(options) { if (!this.get('saveTask.isRunning')) { return yield this.saveTask.perform({ silent: true, - backgroundSave: true + backgroundSave: true, + ...options }); } } @@ -422,10 +423,11 @@ export default class LexicalEditorController extends Controller { let prevStatus = this.get('post.status'); let isNew = this.get('post.isNew'); let status; + const adapterOptions = {}; this.cancelAutosave(); - if (options.backgroundSave && !this.hasDirtyAttributes) { + if (options.backgroundSave && !this.hasDirtyAttributes && !options.leavingEditor) { return; } @@ -452,10 +454,15 @@ export default class LexicalEditorController extends Controller { // new publishing flow sets the post status manually on publish this.set('post.status', status); + const explicitSave = !options.backgroundSave; + const leavingEditor = options.leavingEditor; + if (explicitSave || leavingEditor) { + adapterOptions.saveRevision = 1; + } yield this.beforeSaveTask.perform(options); try { - let post = yield this._savePostTask.perform(options); + let post = yield this._savePostTask.perform({...options, adapterOptions}); post.set('statusScratch', null); @@ -826,6 +833,11 @@ export default class LexicalEditorController extends Controller { let hasDirtyAttributes = this.hasDirtyAttributes; let state = post.getProperties('isDeleted', 'isSaving', 'hasDirtyAttributes', 'isNew'); + // Check if anything has changed since the last revision + let postRevisions = post.get('postRevisions').toArray(); + let latestRevision = postRevisions[postRevisions.length - 1]; + let hasChangedSinceLastRevision = post.get('lexical') !== latestRevision.get('lexical'); + let fromNewToEdit = this.router.currentRouteName === 'lexical-editor.new' && transition.targetName === 'lexical-editor.edit' && transition.intent.contexts @@ -835,6 +847,17 @@ export default class LexicalEditorController extends Controller { let deletedWithoutChanges = state.isDeleted && (state.isSaving || !state.hasDirtyAttributes); + // If leaving the editor and the post has changed since we last saved a revision, always save a new revision + if (hasChangedSinceLastRevision) { + transition.abort(); + if (this._autosaveRunning) { + this.cancelAutosave(); + this.autosaveTask.cancelAll(); + } + await this.autosaveTask.perform({leavingEditor: true}); + return transition.retry(); + } + // controller is dirty and we aren't in a new->edit or delete->index // transition so show our "are you sure you want to leave?" modal if (!this._leaveConfirmed && !fromNewToEdit && !deletedWithoutChanges && hasDirtyAttributes) { @@ -851,7 +874,8 @@ export default class LexicalEditorController extends Controller { this.cancelAutosave(); this.autosaveTask.cancelAll(); - await this.autosaveTask.perform(); + // If leaving the editor, always save a revision + await this.autosaveTask.perform({leavingEditor: true}); return transition.retry(); } diff --git a/ghost/core/core/server/api/endpoints/posts.js b/ghost/core/core/server/api/endpoints/posts.js index 59dcffa231..1e1fe10e0f 100644 --- a/ghost/core/core/server/api/endpoints/posts.js +++ b/ghost/core/core/server/api/endpoints/posts.js @@ -176,6 +176,7 @@ module.exports = { 'email_segment', 'newsletter', 'force_rerender', + 'save_revision', // NOTE: only for internal context 'forUpdate', 'transacting' diff --git a/ghost/core/core/server/models/post.js b/ghost/core/core/server/models/post.js index 6af0f4e01b..bd6ac298b8 100644 --- a/ghost/core/core/server/models/post.js +++ b/ghost/core/core/server/models/post.js @@ -913,7 +913,7 @@ Post = ghostBookshelf.Model.extend({ const revisionModels = await ghostBookshelf.model('PostRevision') .findAll(Object.assign({ filter: `post_id:${model.id}`, - columns: ['id'] + columns: ['id', 'lexical', 'created_at', 'author_id', 'title'] }, _.pick(options, 'transacting'))); const revisions = revisionModels.toJSON(); @@ -932,7 +932,12 @@ Post = ghostBookshelf.Model.extend({ title: model.get('title') }; - const newRevisions = await postRevisions.getRevisions(previous, current, revisions); + // This can be refactored once we have the status stored in each revision + const revisionOptions = { + forceRevision: options.save_revision, + isPublished: newStatus === 'published' + }; + const newRevisions = await postRevisions.getRevisions(previous, current, revisions, revisionOptions); model.set('post_revisions', newRevisions); }); } @@ -1170,7 +1175,7 @@ Post = ghostBookshelf.Model.extend({ findPage: ['status'], findAll: ['columns', 'filter'], destroy: ['destroyAll', 'destroyBy'], - edit: ['filter', 'email_segment', 'force_rerender', 'newsletter'] + edit: ['filter', 'email_segment', 'force_rerender', 'newsletter', 'save_revision'] }; // The post model additionally supports having a formats option diff --git a/ghost/core/test/e2e-api/admin/posts.test.js b/ghost/core/test/e2e-api/admin/posts.test.js index b39fa87326..593b5e7520 100644 --- a/ghost/core/test/e2e-api/admin/posts.test.js +++ b/ghost/core/test/e2e-api/admin/posts.test.js @@ -389,7 +389,7 @@ describe('Posts API', function () { const [postResponse] = postBody.posts; await agent - .put(`/posts/${postResponse.id}/?formats=mobiledoc,lexical,html`) + .put(`/posts/${postResponse.id}/?formats=mobiledoc,lexical,html&save_revision=true`) .body({posts: [Object.assign({}, postResponse, {lexical: updatedLexical})]}) .expectStatus(200) .matchBodySnapshot({ diff --git a/ghost/post-revisions/lib/post-revisions.js b/ghost/post-revisions/lib/post-revisions.js index 0e4ece033e..5e713faeae 100644 --- a/ghost/post-revisions/lib/post-revisions.js +++ b/ghost/post-revisions/lib/post-revisions.js @@ -32,26 +32,41 @@ class PostRevisions { * @param {PostLike} previous * @param {PostLike} current * @param {Revision[]} revisions + * @param {object} options * @returns {boolean} */ - shouldGenerateRevision(previous, current, revisions) { + shouldGenerateRevision(previous, current, revisions, options) { + const latestRevision = revisions[revisions.length - 1]; if (!previous) { return false; } + // If there's no revisions for this post, we should always save a revision if (revisions.length === 0) { return true; } - return previous.html !== current.html || previous.title !== current.title; + const isPublished = options && options.isPublished; + if (isPublished) { + return true; + } + + const forceRevision = options && options.forceRevision; + const lexicalHasChangedSinceLatestRevision = latestRevision.lexical !== current.lexical; + const titleHasChanged = previous.title !== current.title; + if ((lexicalHasChangedSinceLatestRevision || titleHasChanged) && forceRevision) { + return true; + } + return false; } /** * @param {PostLike} previous * @param {PostLike} current * @param {Revision[]} revisions + * @param {object} options * @returns {Promise} */ - async getRevisions(previous, current, revisions) { - if (!this.shouldGenerateRevision(previous, current, revisions)) { + async getRevisions(previous, current, revisions, options) { + if (!this.shouldGenerateRevision(previous, current, revisions, options)) { return revisions; } diff --git a/ghost/post-revisions/test/hello.test.js b/ghost/post-revisions/test/hello.test.js index 3f23107ee7..ae1564a1c6 100644 --- a/ghost/post-revisions/test/hello.test.js +++ b/ghost/post-revisions/test/hello.test.js @@ -1,6 +1,6 @@ const assert = require('assert'); const sinon = require('sinon'); -const PostRevisions = require('../'); +const PostRevisions = require('..'); const config = { max_revisions: 10 @@ -43,7 +43,7 @@ describe('PostRevisions', function () { assert.equal(actual, expected); }); - it('should return true if the current and previous html values are different', function () { + it('should return true if forceRevision is true and the lexical has changed since the latest revision', function () { const postRevisions = new PostRevisions({config}); const expected = true; @@ -55,12 +55,16 @@ describe('PostRevisions', function () { html: 'blah2' }, [{ lexical: 'blah' - }]); + }, { + lexical: 'blah2' + }], { + forceRevision: true + }); assert.equal(actual, expected); }); - it('should return true if the current and previous title values are different', function () { + it('should return true if the current and previous title values are different and forceRevision is true', function () { const postRevisions = new PostRevisions({config}); const expected = true; @@ -74,7 +78,30 @@ describe('PostRevisions', function () { title: 'blah2' }, [{ lexical: 'blah' - }]); + }], { + forceRevision: true + }); + + assert.equal(actual, expected); + }); + + it('should always return true if isPublished is true', function () { + const postRevisions = new PostRevisions({config}); + + const expected = true; + const actual = postRevisions.shouldGenerateRevision({ + lexical: 'blah', + html: 'blah', + title: 'blah' + }, { + lexical: 'blah', + html: 'blah', + title: 'blah2' + }, [{ + lexical: 'blah' + }], { + isPublished: true + }); assert.equal(actual, expected); }); @@ -161,7 +188,9 @@ describe('PostRevisions', function () { id: '1', lexical: 'new', html: 'new' - }, revisions); + }, revisions, { + forceRevision: true + }); assert.equal(actual.length, 2); });