mirror of
https://github.com/TryGhost/Ghost.git
synced 2023-12-13 21:00:40 +01:00
Improved member importer error handling (#15843)
refs: https://github.com/TryGhost/Team/issues/1121 - This makes several key changes to the way errors are handled in the member importer, to ensure that we only show error messages to users that we wrote. - Fundamentally, we no longer trust all API errors, and instead only trust a set of very specific API errors. Anything outside of that is replaced with a generic error message. - Also switches the server-side error generated for email verification (which can throw during member import) to be a HostLimitError, as that is a more appropriate class. - Note: there are many other parts of Ghost admin that need a similar overhaul, and a similar change we need to introduce server side to fully resolve the underlying issue of bubbling up code errors to the UI.
This commit is contained in:
parent
5bb2b2c614
commit
dfffa309a8
4 changed files with 46 additions and 17 deletions
|
@ -4,6 +4,8 @@ import moment from 'moment-timezone';
|
|||
import unparse from '@tryghost/members-csv/lib/unparse';
|
||||
import {
|
||||
AcceptedResponse,
|
||||
isDataImportError,
|
||||
isHostLimitError,
|
||||
isRequestEntityTooLargeError,
|
||||
isUnsupportedMediaTypeError,
|
||||
isVersionMismatchError
|
||||
|
@ -11,7 +13,6 @@ import {
|
|||
import {computed} from '@ember/object';
|
||||
import {htmlSafe} from '@ember/template';
|
||||
import {inject} from 'ghost-admin/decorators/inject';
|
||||
import {isBlank} from '@ember/utils';
|
||||
import {inject as service} from '@ember/service';
|
||||
|
||||
export default ModalComponent.extend({
|
||||
|
@ -206,22 +207,27 @@ export default ModalComponent.extend({
|
|||
this.notifications.showAPIError(error);
|
||||
}
|
||||
|
||||
// Handle all the specific errors that we know about
|
||||
if (isUnsupportedMediaTypeError(error)) {
|
||||
message = 'The file type you uploaded is not supported.';
|
||||
} else if (isRequestEntityTooLargeError(error)) {
|
||||
message = 'The file you uploaded was larger than the maximum file size your server allows.';
|
||||
} else if (error.payload && error.payload.errors && !isBlank(error.payload.errors[0].message)) {
|
||||
} else if (isDataImportError(error, error.payload)) {
|
||||
message = htmlSafe(error.payload.errors[0].message);
|
||||
} else if (isHostLimitError(error) && error?.payload?.errors?.[0]?.code === 'EMAIL_VERIFICATION_NEEDED') {
|
||||
message = htmlSafe(error.payload.errors[0].message);
|
||||
|
||||
if (error.payload.errors[0].message.match(/great deliverability/gi)) {
|
||||
header = 'Woah there cowboy, that\'s a big list';
|
||||
this.set('showTryAgainButton', false);
|
||||
// NOTE: confirm makes sure to refresh the members data in the background
|
||||
this.confirm();
|
||||
}
|
||||
} else {
|
||||
header = 'Woah there cowboy, that\'s a big list';
|
||||
this.set('showTryAgainButton', false);
|
||||
// NOTE: confirm makes sure to refresh the members data in the background
|
||||
this.confirm();
|
||||
} else { // Generic fallback error
|
||||
message = 'An unexpected error occurred, please try again';
|
||||
|
||||
console.error(error); // eslint-disable-line
|
||||
message = 'Something went wrong :(';
|
||||
if (error?.payload?.errors?.[0]?.id) {
|
||||
console.error(`Error ID: ${error.payload.errors[0].id}`); // eslint-disable-line
|
||||
}
|
||||
}
|
||||
|
||||
this.set('errorMessage', message);
|
||||
|
|
|
@ -37,6 +37,22 @@ export function isVersionMismatchError(errorOrStatus, payload) {
|
|||
}
|
||||
}
|
||||
|
||||
/* DataImport error */
|
||||
|
||||
export class DataImportError extends AjaxError {
|
||||
constructor(payload) {
|
||||
super(payload, 'he server encountered an error whilst importing data.');
|
||||
}
|
||||
}
|
||||
|
||||
export function isDataImportError(errorOrStatus, payload) {
|
||||
if (isAjaxError(errorOrStatus)) {
|
||||
return errorOrStatus instanceof DataImportError;
|
||||
} else {
|
||||
return get(payload || {}, 'errors.firstObject.type') === 'DataImportError';
|
||||
}
|
||||
}
|
||||
|
||||
/* Server unreachable error */
|
||||
|
||||
export class ServerUnreachableError extends AjaxError {
|
||||
|
@ -133,6 +149,8 @@ export function isHostLimitError(errorOrStatus, payload) {
|
|||
}
|
||||
}
|
||||
|
||||
/* Email error */
|
||||
|
||||
export class EmailError extends AjaxError {
|
||||
constructor(payload) {
|
||||
super(payload, 'Please verify your email settings');
|
||||
|
@ -343,6 +361,10 @@ class ajaxService extends AjaxService {
|
|||
return isUnsupportedMediaTypeError(status);
|
||||
}
|
||||
|
||||
isDataImportError(status) {
|
||||
return isDataImportError(status);
|
||||
}
|
||||
|
||||
isMaintenanceError(status, headers, payload) {
|
||||
return isMaintenanceError(status, payload);
|
||||
}
|
||||
|
|
|
@ -126,7 +126,7 @@ describe('Integration: Component: modal-import-members-test', function () {
|
|||
await click('.gh-btn-green');
|
||||
|
||||
expect(findAll('.failed').length, 'error message is displayed').to.equal(1);
|
||||
expect(find('.failed').textContent).to.match(/Error: UnknownError/);
|
||||
expect(find('.failed').textContent).to.match(/An unexpected error occurred, please try again/);
|
||||
});
|
||||
|
||||
it('handles unknown failure', async function () {
|
||||
|
@ -141,7 +141,7 @@ describe('Integration: Component: modal-import-members-test', function () {
|
|||
await click('.gh-btn-green');
|
||||
|
||||
expect(findAll('.failed').length, 'error message is displayed').to.equal(1);
|
||||
expect(find('.failed').textContent).to.match(/Something went wrong/);
|
||||
expect(find('.failed').textContent).to.match(/An unexpected error occurred, please try again/);
|
||||
});
|
||||
|
||||
it('triggers notifications.showAPIError for VersionMismatchError', async function () {
|
||||
|
|
|
@ -50,8 +50,8 @@ class VerificationTrigger {
|
|||
}
|
||||
|
||||
/**
|
||||
*
|
||||
* @param {MemberCreatedEvent} event
|
||||
*
|
||||
* @param {MemberCreatedEvent} event
|
||||
*/
|
||||
async _handleMemberCreatedEvent(event) {
|
||||
const source = event.data?.source;
|
||||
|
@ -156,7 +156,7 @@ class VerificationTrigger {
|
|||
} else if (source === 'admin') {
|
||||
verificationMessage = messages.emailVerificationEmailMessageAdmin;
|
||||
}
|
||||
|
||||
|
||||
this._sendVerificationEmail({
|
||||
message: verificationMessage,
|
||||
subject: messages.emailVerificationEmailSubject,
|
||||
|
@ -164,8 +164,9 @@ class VerificationTrigger {
|
|||
});
|
||||
|
||||
if (throwOnTrigger) {
|
||||
throw new errors.ValidationError({
|
||||
message: messages.emailVerificationNeeded
|
||||
throw new errors.HostLimitError({
|
||||
message: messages.emailVerificationNeeded,
|
||||
code: 'EMAIL_VERIFICATION_NEEDED'
|
||||
});
|
||||
}
|
||||
|
||||
|
|
Loading…
Reference in a new issue