Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show error summary from API when transcript fails to load #1030

Merged
merged 2 commits into from
Jul 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions via/static/scripts/video_player/components/TranscriptError.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import type { APIError } from '../utils/api';

export type TranscriptErrorProps = {
error: APIError;
};

export default function TranscriptError({ error }: TranscriptErrorProps) {
return (
<div className="p-3">
<h2 className="text-lg">Unable to load transcript</h2>
<p className="mb-3">{error.error?.title ?? error.message}</p>
</div>
Copy link
Member Author

@robertknight robertknight Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Styling is obvious not final here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, don't worry about that for now. I've got it.

);
}
5 changes: 2 additions & 3 deletions via/static/scripts/video_player/components/VideoPlayerApp.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import { formatTranscript, mergeSegments } from '../utils/transcript';
import HypothesisClient from './HypothesisClient';
import Transcript from './Transcript';
import type { TranscriptControls } from './Transcript';
import TranscriptError from './TranscriptError';
import YouTubeVideoPlayer from './YouTubeVideoPlayer';
import { PauseIcon, PlayIcon, SyncIcon } from './icons';

Expand Down Expand Up @@ -459,9 +460,7 @@ export default function VideoPlayerApp({
</Transcript>
)}
{transcript instanceof Error && (
<div data-testid="transcript-error">
Unable to load transcript: {transcript.message}
</div>
<TranscriptError error={transcript} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's neat that the same reference can accommodate either an actual transcript or an error (and am of the opinion that typing makes this possible versus a bug waiting to happen). But I admit I did a double-take at error={transcript} here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I might rename this to transcriptOrError in future.

)}
<div
id={bucketContainerId}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { mount } from 'enzyme';

import { APIError } from '../../utils/api';
import TranscriptError from '../TranscriptError';

describe('TranscriptError', () => {
it('displays just `Error.message` if there are no error details', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this test description need adjustment in light of the details removal?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still valid as "error details" includes the title. This case is for when we don't have a server-generated error at all, or it doesn't include even a title.

const wrapper = mount(
<TranscriptError error={new Error('Something went wrong')} />
);
assert.equal(
wrapper.text(),
['Unable to load transcript', 'Something went wrong'].join('')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[...].join('') is used here to separate out parts that are on different lines in the UI, but that don't have actual spaces between them.

);
});

it('displays `APIError.error.title` field if present', () => {
const error = new APIError(404, {
code: 'VideoNotFound',
title: 'The video was not found',
detail: 'Some long details here',
});
const wrapper = mount(<TranscriptError error={error} />);
assert.equal(
wrapper.text(),
['Unable to load transcript', 'The video was not found'].join('')
);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -195,15 +195,9 @@ describe('VideoPlayerApp', () => {
fakeCallAPI.withArgs(videoPlayerConfig.api.transcript).rejects(error);

const wrapper = createVideoPlayerUsingAPI();
const errorDisplay = await waitForElement(
wrapper,
'[data-testid="transcript-error"]'
);
const errorDisplay = await waitForElement(wrapper, 'TranscriptError');

assert.equal(
errorDisplay.text(),
`Unable to load transcript: ${error.message}`
);
assert.equal(errorDisplay.prop('error'), error);
});
});

Expand Down
51 changes: 35 additions & 16 deletions via/static/scripts/video_player/utils/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,22 +10,17 @@ export type APIMethod = {
url: string;
};

export class APIError extends Error {
/** The HTTP status code of the response. */
/**
* Structure of a JSON API error.
*
* See https://jsonapi.org/format/#error-objects.
*/
export type JSONAPIError = {
status: number;

/**
* Payload of the response.
*/
data: unknown;

constructor(status: number, data: unknown) {
super('API call failed');

this.status = status;
this.data = data;
}
}
code: string;
title: string;
detail: string;
};

/**
* Structure of a JSON API response.
Expand All @@ -37,9 +32,25 @@ export type JSONAPIObject<Properties extends object> = {
type: string;
id: string;
attributes: Properties;
errors: JSONAPIError[];
};
};

export class APIError extends Error {
/** The HTTP status code of the response. */
status: number;

/** Error details returned by the API. */
error: JSONAPIError | undefined;

constructor(status: number, error?: JSONAPIError) {
super('API call failed');

this.status = status;
this.error = error;
}
}

/**
* Make an API call to the backend.
*
Expand All @@ -57,7 +68,15 @@ export async function callAPI<T = unknown>(api: APIMethod): Promise<T> {
const resultData = await result.json().catch(() => null);

if (result.status >= 400 && result.status < 600) {
throw new APIError(result.status, resultData);
let error;
if (
resultData &&
Array.isArray(resultData.errors) &&
resultData.errors.length > 0
) {
error = resultData.errors[0];
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JSON:API format allows for multiple errors, but we currently only report the first one on the frontend. I'm not aware of a scenario where we'd expect the backend to report multiple yet.

}
throw new APIError(result.status, error);
}

return resultData;
Expand Down
97 changes: 65 additions & 32 deletions via/static/scripts/video_player/utils/test/api-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,38 +33,71 @@ describe('callAPI', () => {
assert.deepEqual(result, transcriptsAPIResponse);
});

it('throws exception if result is not JSON', async () => {
fakeFetch
.withArgs(videoPlayerConfig.api.transcript.url)
.resolves(new Response('<b>Oh no</b>', { status: 500 }));

let error;
try {
await callAPI(videoPlayerConfig.api.transcript);
} catch (e) {
error = e;
}

assert.instanceOf(error, APIError);
assert.equal(error.status, 500);
assert.equal(error.data, null);
assert.equal(error.message, 'API call failed');
});
context('when request fails', () => {
it('throws exception if result is not JSON', async () => {
fakeFetch
.withArgs(videoPlayerConfig.api.transcript.url)
.resolves(new Response('<b>Oh no</b>', { status: 500 }));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just confirming that we intend to invent a 500 here in these cases :). Granted this is not new behavior in these changes, so feel free to ignore me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is mean to simulate something like eg. a 5xx "Bad Gateway" error if the server failed to respond within the various timeouts that the proxies in front have.


it('throws exception if API request fails', async () => {
fakeFetch
.withArgs(videoPlayerConfig.api.transcript.url)
.resolves(jsonResponse(404));

let error;
try {
await callAPI(videoPlayerConfig.api.transcript);
} catch (e) {
error = e;
}

assert.instanceOf(error, APIError);
assert.equal(error.status, 404);
assert.equal(error.message, 'API call failed');
let error;
try {
await callAPI(videoPlayerConfig.api.transcript);
} catch (e) {
error = e;
}

assert.instanceOf(error, APIError);
assert.equal(error.status, 500);
assert.isUndefined(error.error);
assert.equal(error.message, 'API call failed');
});

[{}, { errors: [] }].forEach(responseBody => {
it('throws exception without `error` if `errors` field is missing or empty', async () => {
fakeFetch
.withArgs(videoPlayerConfig.api.transcript.url)
.resolves(jsonResponse(404, responseBody));

let error;
try {
await callAPI(videoPlayerConfig.api.transcript);
} catch (e) {
error = e;
}

assert.instanceOf(error, APIError);
assert.isUndefined(error.error);
assert.equal(error.status, 404);
assert.equal(error.message, 'API call failed');
});
});

it('throws exception with `error` if `errors` field is present', async () => {
const responseBody = {
errors: [
{
code: 'VideoNotFound',
title: 'This video does not exist',
detail: 'Video ID is invalid',
},
],
};

fakeFetch
.withArgs(videoPlayerConfig.api.transcript.url)
.resolves(jsonResponse(404, responseBody));

let error;
try {
await callAPI(videoPlayerConfig.api.transcript);
} catch (e) {
error = e;
}

assert.instanceOf(error, APIError);
assert.deepEqual(error.error, responseBody.errors[0]);
assert.equal(error.status, 404);
assert.equal(error.message, 'API call failed');
});
});
});