Skip to content

Commit

Permalink
Fixed errors from 404 error handler for non-transition 404s
Browse files Browse the repository at this point in the history
ref https://linear.app/tryghost/issue/ONC-323

- our fallback 404 error handler assumed we always had a transition along with the error
  - this wasn't a bad assumption, it should be very unlikely that we see a 404 outside of navigating to a non-existent/deleted resource
  - unfortunately we weren't handling the error thrown by our error handler which meant the error was silent as far as the user was concerned
  - having a silent error meant that in very rare circumstances the editor could get into a state where saving was failing but there was no indication of that
- updated the fallback 404 error handler to only do something when navigation was occurring in which case it transitions to the 404 screen, otherwise let the error continue to our generic API error handling which will stay on the current screen but show an error alert
- adjusted the editor saving to actually trigger autosave-after-change when testing (albeit with 100ms wait compared to 3s) so the tests better reflect actual behaviour
  • Loading branch information
kevinansfield committed Sep 13, 2024
1 parent a44274d commit f054205
Show file tree
Hide file tree
Showing 5 changed files with 79 additions and 31 deletions.
4 changes: 2 additions & 2 deletions ghost/admin/app/controllers/lexical-editor.js
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ export default class LexicalEditorController extends Controller {

@computed('post.isDraft')
get _canAutosave() {
return config.environment !== 'test' && this.get('post.isDraft');
return this.post.isDraft;
}

TK_REGEX = new RegExp(/(^|.)([^\p{L}\p{N}\s]*(TK)+[^\p{L}\p{N}\s]*)(.)?/u);
Expand Down Expand Up @@ -1217,7 +1217,7 @@ export default class LexicalEditorController extends Controller {
return this.autosaveTask.perform();
}

yield timeout(AUTOSAVE_TIMEOUT);
yield timeout(config.environment === 'test' ? 100 : AUTOSAVE_TIMEOUT);
this.autosaveTask.perform();
}).restartable())
_autosaveTask;
Expand Down
31 changes: 18 additions & 13 deletions ghost/admin/app/routes/application.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,30 +106,35 @@ export default Route.extend(ShortcutsRoute, {
save: K,

error(error, transition) {
// unauthoirized errors are already handled in the ajax service
// unauthorized errors are already handled in the ajax service
if (isUnauthorizedError(error)) {
return false;
}

if (isNotFoundError(error)) {
if (transition) {
transition.abort();
}

let routeInfo = transition.to;
let router = this.router;
let params = [];
let routeInfo = transition?.to;
let router = this.router;
let params = [];

for (let key of Object.keys(routeInfo.params)) {
params.push(routeInfo.params[key]);
}
if (routeInfo) {
for (let key of Object.keys(routeInfo.params)) {
params.push(routeInfo.params[key]);
}

let url = router.urlFor(routeInfo.name, ...params)
.replace(/^#\//, '')
.replace(/^\//, '')
.replace(/^ghost\//, '');
let url = router.urlFor(routeInfo.name, ...params)
.replace(/^#\//, '')
.replace(/^\//, '')
.replace(/^ghost\//, '');

return this.replaceWith('error404', url);
}
}

return this.replaceWith('error404', url);
// when there's no transition we fall through to our generic error handler
// for network errors that will hit the isAjaxError branch below
}

if (isVersionMismatchError(error)) {
Expand Down
43 changes: 41 additions & 2 deletions ghost/admin/tests/acceptance/editor-test.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import ctrlOrCmd from 'ghost-admin/utils/ctrl-or-cmd';
import moment from 'moment-timezone';
import sinon from 'sinon';
import {Response} from 'miragejs';
import {authenticateSession, invalidateSession} from 'ember-simple-auth/test-support';
import {beforeEach, describe, it} from 'mocha';
import {blur, click, currentRouteName, currentURL, fillIn, find, findAll, triggerEvent, typeIn} from '@ember/test-helpers';
import {blur, click, currentRouteName, currentURL, fillIn, find, findAll, triggerEvent, typeIn, waitFor} from '@ember/test-helpers';
import {datepickerSelect} from 'ember-power-datepicker/test-support';
import {editorSelector, pasteInEditor, titleSelector} from '../helpers/editor';
import {enableLabsFlag} from '../helpers/labs-flag';
import {expect} from 'chai';
import {selectChoose} from 'ember-power-select/test-support';
Expand Down Expand Up @@ -114,9 +116,9 @@ describe('Acceptance: Editor', function () {
let author;

beforeEach(async function () {
this.server.loadFixtures();
let role = this.server.create('role', {name: 'Administrator'});
author = this.server.create('user', {roles: [role]});
this.server.loadFixtures('settings');

await authenticateSession();
});
Expand Down Expand Up @@ -669,5 +671,42 @@ describe('Acceptance: Editor', function () {
'TK reminder modal'
).to.exist;
});

// We shouldn't ever see 404s from the API but we do/have had a bug where
// a new post can enter a state where it appears saved but hasn't hit
// the API to create the post meaning it has no ID but the store is
// making PUT requests rather than a POST request in which case it's
// hitting `/posts/` rather than `/posts/:id` and receiving a 404. On top
// of that our application error handler was erroring because there was
// no transition alongside the error so this test makes sure that works
// and we enter a visible error state rather than letting unsaved changes
// pile up and contributing to larger potential data loss.
it('handles 404s from API requests', async function () {
// this doesn't match what we're actually seeing in the above mentioned
// bug state but it's a good enough simulation for testing our error handler
this.server.put('/posts/:id/', () => {
return new Response(404, {}, {
errors: [
{
message: 'Resource could not be found.',
errorType: 'NotFoundError',
statusCode: 404
}
]
});
});

await visit('/editor/post');
await waitFor(editorSelector);
await fillIn(titleSelector, 'Test 404 handling');
// this triggers the initial creation request - in the actual bug this doesn't happen
await blur(titleSelector);
expect(currentRouteName()).to.equal('lexical-editor.edit');
// this will trigger an autosave which will hit our simulated 404
await pasteInEditor('Testing');

// we should see an error - previously this was failing silently
expect(find('.gh-alert-content')).to.have.trimmed.text('Resource could not be found.');
});
});
});
16 changes: 2 additions & 14 deletions ghost/admin/tests/acceptance/editor/unsaved-changes-test.js
Original file line number Diff line number Diff line change
@@ -1,26 +1,14 @@
import loginAsRole from '../../helpers/login-as-role';
import {click, currentURL, fillIn, find, waitFor, waitUntil} from '@ember/test-helpers';
import {click, currentURL, fillIn, find} from '@ember/test-helpers';
import {editorSelector, pasteInEditor, titleSelector} from '../../helpers/editor';
import {expect} from 'chai';
import {setupApplicationTest} from 'ember-mocha';
import {setupMirage} from 'ember-cli-mirage/test-support';
import {visit} from '../../helpers/visit';

const titleSelector = '[data-test-editor-title-input]';
const editorSelector = '[data-secondary-instance="false"] [data-lexical-editor]';
const unsavedModalSelector = '[data-test-modal="unsaved-post-changes"]';
const backToPostsSelector = '[data-test-link="posts"]';

const pasteInEditor = async (text) => {
await waitFor(editorSelector);
await click(editorSelector);
const dataTransfer = new DataTransfer();
dataTransfer.setData('text/plain', text);
document.activeElement.dispatchEvent(new ClipboardEvent('paste', {clipboardData: dataTransfer, bubbles: true, cancelable: true}));
dataTransfer.clearData();
const editor = find(editorSelector);
await waitUntil(() => editor.textContent.includes(text));
};

describe('Acceptance: Editor: Unsaved changes', function () {
let hooks = setupApplicationTest();
setupMirage(hooks);
Expand Down
16 changes: 16 additions & 0 deletions ghost/admin/tests/helpers/editor.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import {click, find, settled, waitFor, waitUntil} from '@ember/test-helpers';

export const titleSelector = '[data-test-editor-title-input]';
export const editorSelector = '[data-secondary-instance="false"] [data-lexical-editor]';

export const pasteInEditor = async (text) => {
await waitFor(editorSelector);
await click(editorSelector);
const dataTransfer = new DataTransfer();
dataTransfer.setData('text/plain', text);
document.activeElement.dispatchEvent(new ClipboardEvent('paste', {clipboardData: dataTransfer, bubbles: true, cancelable: true}));
dataTransfer.clearData();
const editor = find(editorSelector);
await waitUntil(() => editor.textContent.includes(text));
await settled();
};

0 comments on commit f054205

Please sign in to comment.