TryGhost/Ghost
1
0
Fork 0
mirror of https://github.com/TryGhost/Ghost.git synced 2024-10-04 00:40:00 +03:00
Code Issues Projects Releases Wiki Activity

Fixed errors from 404 error handler for non-transition 404s

Browse Source 
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
This commit is contained in:
Kevin Ansfield 2024-09-13 10:56:37 +01:00
parent a44274d7f3
commit f054205e58
5 changed files with 81 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
ghost/admin/app/controllers/lexical-editor.js
View File

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

35
ghost/admin/app/routes/application.js
View File

@ -106,7 +106,7 @@ 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;
}
@ -114,22 +114,27 @@ export default Route.extend(ShortcutsRoute, {
if (isNotFoundError(error)) {
if (transition) {
transition.abort();
let routeInfo = transition?.to;
let router = this.router;
let params = [];
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\//, '');
return this.replaceWith('error404', url);
}
}
let routeInfo = transition.to;
let router = this.router;
let params = [];
for (let key of Object.keys(routeInfo.params)) {
params.push(routeInfo.params[key]);
}
let url = router.urlFor(routeInfo.name, ...params)
.replace(/^#\//, '')
.replace(/^\//, '')
.replace(/^ghost\//, '');
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)) {

43
ghost/admin/tests/acceptance/editor-test.js
View File

@ -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';
@ -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();
});
@ -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
ghost/admin/tests/acceptance/editor/unsaved-changes-test.js
View File

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

16
ghost/admin/tests/helpers/editor.js Normal file
View File

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