Commit ab8ec27d authored by Robert Knight's avatar Robert Knight

Include some error details in toast shown when an import fails

This will make triaging any user reports of issues easier. It would look better
if we could present the details in a separate dialog, but this is a starting
point.

Also disable auto-dismiss for non-success statuses so the user is less likely to
miss them.

Part of https://github.com/hypothesis/client/issues/5741
parent 4c4c017d
...@@ -39,6 +39,17 @@ type ImportData = Pick< ...@@ -39,6 +39,17 @@ type ImportData = Pick<
> & > &
ImportExtra; ImportExtra;
/**
* Enum of the result of an import operation for a single annotation.
*/
export type ImportResult =
/** Annotation was successfully imported. */
| { type: 'import'; annotation: Annotation }
/** Annotation was skipped because it is a duplicate. */
| { type: 'duplicate'; annotation: Annotation }
/** Annotation import failed. */
| { type: 'error'; error: Error };
/** /**
* Return a copy of `ann` that contains only fields which can be preserved by * Return a copy of `ann` that contains only fields which can be preserved by
* an import performed on the client, overwriting some of them with the ones * an import performed on the client, overwriting some of them with the ones
...@@ -70,8 +81,17 @@ function importStatus(results: ImportResult[]): { ...@@ -70,8 +81,17 @@ function importStatus(results: ImportResult[]): {
messageType: 'success' | 'notice' | 'error'; messageType: 'success' | 'notice' | 'error';
message: string; message: string;
} { } {
const errorCount = results.filter(r => r.type === 'error').length; // In the event of an error we include the message of the first error as a
const errorMessage = errorCount > 0 ? `${errorCount} imports failed` : ''; // debugging aid. Ideally this would be presented separately from the main
// message in a notification, but our toast message component doesn't
// currently support that.
const errors = results.filter(r => r.type === 'error') as Array<
Extract<ImportResult, { type: 'error' }>
>;
const errorMessage =
errors.length > 0
? `${errors.length} imports failed (${errors[0].error.message})`
: '';
const importCount = results.filter(r => r.type === 'import').length; const importCount = results.filter(r => r.type === 'import').length;
const importMessage = const importMessage =
...@@ -81,7 +101,7 @@ function importStatus(results: ImportResult[]): { ...@@ -81,7 +101,7 @@ function importStatus(results: ImportResult[]): {
const dupMessage = dupCount > 0 ? `${dupCount} duplicates skipped` : ''; const dupMessage = dupCount > 0 ? `${dupCount} duplicates skipped` : '';
let messageType: 'success' | 'notice' | 'error'; let messageType: 'success' | 'notice' | 'error';
if (errorCount === 0) { if (errors.length === 0) {
if (importCount > 0) { if (importCount > 0) {
messageType = 'success'; messageType = 'success';
} else { } else {
...@@ -118,17 +138,6 @@ function duplicateMatch(a: APIAnnotationData, b: APIAnnotationData): boolean { ...@@ -118,17 +138,6 @@ function duplicateMatch(a: APIAnnotationData, b: APIAnnotationData): boolean {
return quote(a) === quote(b); return quote(a) === quote(b);
} }
/**
* Enum of the result of an import operation for a single annotation.
*/
export type ImportResult =
/** Annotation was successfully imported. */
| { type: 'import'; annotation: Annotation }
/** Annotation was skipped because it is a duplicate. */
| { type: 'duplicate'; annotation: Annotation }
/** Annotation import failed. */
| { type: 'error'; error: Error };
/** /**
* Imports annotations from a Hypothesis JSON file. * Imports annotations from a Hypothesis JSON file.
* *
...@@ -237,9 +246,13 @@ export class ImportAnnotationsService { ...@@ -237,9 +246,13 @@ export class ImportAnnotationsService {
if (messageType === 'success') { if (messageType === 'success') {
this._toastMessenger.success(message); this._toastMessenger.success(message);
} else if (messageType === 'notice') { } else if (messageType === 'notice') {
this._toastMessenger.notice(message); this._toastMessenger.notice(message, {
autoDismiss: false,
});
} else if (messageType === 'error') { } else if (messageType === 'error') {
this._toastMessenger.error(message); this._toastMessenger.error(message, {
autoDismiss: false,
});
} }
return results; return results;
......
...@@ -213,7 +213,7 @@ describe('ImportAnnotationsService', () => { ...@@ -213,7 +213,7 @@ describe('ImportAnnotationsService', () => {
assert.calledWith( assert.calledWith(
fakeToastMessenger.notice, fakeToastMessenger.notice,
'1 annotations imported, 1 imports failed', '1 annotations imported, 1 imports failed (Oh no)',
); );
}); });
...@@ -223,7 +223,10 @@ describe('ImportAnnotationsService', () => { ...@@ -223,7 +223,10 @@ describe('ImportAnnotationsService', () => {
await svc.import([generateAnnotation(), generateAnnotation()]); await svc.import([generateAnnotation(), generateAnnotation()]);
assert.calledWith(fakeToastMessenger.error, '2 imports failed'); assert.calledWith(
fakeToastMessenger.error,
'2 imports failed (Something went wrong)',
);
}); });
it('throws an error if called when user is logged out', async () => { it('throws an error if called when user is logged out', async () => {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment