Skip to content

Commit

Permalink
Improve handling of grading submission errors in frontend
Browse files Browse the repository at this point in the history
 - Handle the `canvas_submission_course_not_available` error with a dedicated
   message and revise the wording of the existing submission error.

 - Continue to show the assignment content underneath the submission error. The
   error dialog already had a translucent model backdrop, so all that was
   required was to move the error dialog above the content in the stacking
   order, and avoid hiding the content if an error occurs after it has been
   loaded.

 - Allow the user to dismiss submission errors, so they can continue
   making annotations. The option to dismiss the error is rendered as a
   secondary "Cance" action, to reduce the chances of the user dismissing it
   without reading the dialog.
  • Loading branch information
robertknight committed Jun 20, 2024
1 parent fa7a24f commit 2ffe597
Show file tree
Hide file tree
Showing 5 changed files with 136 additions and 38 deletions.
44 changes: 31 additions & 13 deletions lms/static/scripts/frontend_apps/components/BasicLTILaunchApp.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,19 @@ export type ErrorState =
| 'error-reporting-submission'
| LTILaunchServerErrorCode;

/**
* Return true if the user can dimiss an error.
*
* This is true for errors which are important, but don't completely prevent
* the user from participating in the assignment.
*/
function canDismissError(e: ErrorState) {
return [
'error-reporting-submission',
'canvas_submission_course_not_available',
].includes(e);
}

/**
* Application displayed when an assignment is launched if the LMS backend
* is unable to directly render the content in an iframe. This happens when
Expand Down Expand Up @@ -88,8 +101,6 @@ export default function BasicLTILaunchApp() {
const authWindow = useRef<AuthWindow | null>(null);

const contentReady = !!contentURL;
const showContent = contentReady && !errorState;
const showSpinner = fetchCount > 0 && !errorState;

const incFetchCount = () => {
setFetchCount(count => count + 1);
Expand Down Expand Up @@ -244,7 +255,9 @@ export default function BasicLTILaunchApp() {
data: submissionParams,
});
} catch (e) {
// If reporting the submission failed, replace the content with an error.
// If reporting the submission failed, show a modal error dialog above
// the content.
//
// This avoids the student trying to complete the assignment without
// knowing that there was a problem, and the teacher then not seeing a
// submission.
Expand Down Expand Up @@ -318,27 +331,32 @@ export default function BasicLTILaunchApp() {
}
}, [authToken, authURL, fetchContentURL, fetchGroups]);

const showSpinner = fetchCount > 0 && !errorState;

return (
<div className="h-full">
{showSpinner && <SpinnerOverlay />}
{errorState && (
<LaunchErrorDialog
busy={fetchCount > 0}
errorState={errorState}
error={error}
onRetry={authorizeAndFetchURL}
/>
)}
<div
className={classnames('flex flex-col h-full', {
invisible: !showContent,
visible: showContent,
invisible: !contentReady,
visible: contentReady,
})}
data-testid="content-wrapper"
>
<InstructorToolbar />
<ContentFrame url={contentURL ?? ''} />
</div>
{errorState && (
<LaunchErrorDialog
busy={fetchCount > 0}
errorState={errorState}
error={error}
onDismiss={
canDismissError(errorState) ? () => setErrorState(null) : undefined
}
onRetry={authorizeAndFetchURL}
/>
)}
</div>
);
}
46 changes: 40 additions & 6 deletions lms/static/scripts/frontend_apps/components/LaunchErrorDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { useConfig } from '../config';
import type { ErrorLike } from '../errors';
import type { ErrorState } from './BasicLTILaunchApp';
import ErrorModal from './ErrorModal';
import type { ErrorModalProps } from './ErrorModal';

export type LaunchErrorDialogProps = {
/**
Expand All @@ -18,6 +19,9 @@ export type LaunchErrorDialogProps = {
error: ErrorLike | null;
/** Callback invoked when user clicks the "Try again" button */
onRetry: () => void;

/** Callback invoked when user clicks the "Dismiss" button */
onDismiss?: () => void;
};

/**
Expand Down Expand Up @@ -62,6 +66,7 @@ export default function LaunchErrorDialog({
busy,
error,
errorState,
onDismiss,
onRetry,
}: LaunchErrorDialogProps) {
const { instructorToolbar } = useConfig();
Expand All @@ -79,7 +84,7 @@ export default function LaunchErrorDialog({
}

// Common properties for error dialog.
const defaultProps = {
const defaultProps: ErrorModalProps = {
busy,
extraActions,
error,
Expand All @@ -90,6 +95,11 @@ export default function LaunchErrorDialog({
onRetry,
};

if (onDismiss) {
defaultProps.onCancel = onDismiss;
defaultProps.cancelLabel = 'Dismiss';
}

switch (errorState) {
case 'error-authorizing':
// nb. There are no error details shown here, since failing to authorize
Expand Down Expand Up @@ -625,18 +635,42 @@ export default function LaunchErrorDialog({
</ErrorModal>
);

case 'canvas_submission_course_not_available':
return (
<ErrorModal
{...defaultProps}
onRetry={undefined}
title="Grading submission failed"
>
<p>
Your annotation was saved, but Hypothesis could not record a
submission for grading.
</p>
<p>
This may be because the course has ended and we are no longer
allowed to create submissions.
</p>
<p>
Your instructor is not aware you have saved an annotation. You may
need to reach out to them.
</p>
</ErrorModal>
);

case 'error-reporting-submission':
// nb. There is no retry action here as we just suggest reloading the entire
// page.
return (
<ErrorModal
{...defaultProps}
onRetry={undefined}
title="Something went wrong"
title="Grading submission failed"
>
<p>
There was a problem submitting this Hypothesis assignment.{' '}
<b>To fix this problem, try reloading the page.</b>
Your annotation was saved, but Hypothesis could not record a
submission for grading.
</p>
<p>
Your instructor is not aware you have saved an annotation. You may
need to reach out to them.
</p>
</ErrorModal>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -465,24 +465,44 @@ describe('BasicLTILaunchApp', () => {
assert.isTrue(wrapper.exists('iframe'));
});

it('displays an error if reporting the submission fails', async () => {
// Make the grading submission API call fail.
function makeSubmissionFail() {
const error = new APIError(400, {});
fakeApiCall
.withArgs(sinon.match({ path: '/api/lti/submissions' }))
.rejects(error);
return error;
}

const wrapper = renderLTILaunchApp();

async function waitForErrorDialog(wrapper) {
// Wait for the API call to fail and check that an error is displayed.
// There should be no "Try again" button in this context, instead we just
// ask the user to reload the page.
const errorDialog = await waitForElement(
wrapper,
'LaunchErrorDialog[errorState="error-reporting-submission"]',
);
return errorDialog;
}

it('displays an error if reporting the submission fails', async () => {
const error = makeSubmissionFail();
const wrapper = renderLTILaunchApp();
const errorDialog = await waitForErrorDialog(wrapper);
assert.equal(errorDialog.prop('error'), error);
});

it('allows user to dismiss error', async () => {
makeSubmissionFail();
const wrapper = renderLTILaunchApp();
const errorDialog = await waitForErrorDialog(wrapper);

errorDialog.prop('onDismiss')();
wrapper.update();

assert.isFalse(wrapper.exists('LaunchErrorDialog'));
});

it('does not report a submission if grading is not enabled for the current user', async () => {
// If the assignment is not gradable or the user is an instructor, the
// `speedGrader` config is omitted.
Expand Down Expand Up @@ -544,22 +564,28 @@ describe('BasicLTILaunchApp', () => {
assert.isFunction(annotationActivityCalls[0].args[1]);
});

// Simulate annotation activity being reported from the client to the
// LMS frontend.
async function reportActivity() {
const annotationActivityCalls = getOnActivityCalls(fakeRpcServer.on);
const callback = annotationActivityCalls[0].callback;
callback('create', { annotation: { isShared: true } });
await delay(0);
return callback;
}

context(
'`annotationActivity` event that qualifies for submission',
() => {
it('submits a submission', async () => {
it('creates submission', async () => {
renderLTILaunchApp();
assert.isFalse(
fakeApiCall.calledWith(
sinon.match({ path: '/api/lti/submissions' }),
),
);
const annotationActivityCalls = getOnActivityCalls(
fakeRpcServer.on,
);
const callback = annotationActivityCalls[0].callback;
callback('create', { annotation: { isShared: true } });
await delay(0);

await reportActivity();

assert.calledOnce(fakeApiCall);
assert.calledWith(
Expand All @@ -581,12 +607,7 @@ describe('BasicLTILaunchApp', () => {
it('deregisters callback after submission is made', async () => {
renderLTILaunchApp();

const annotationActivityCalls = getOnActivityCalls(
fakeRpcServer.on,
);
const callback = annotationActivityCalls[0].args[1];
callback('create', { annotation: { isShared: true } });
await delay(0);
const callback = await reportActivity();

assert.calledOnce(fakeApiCall);
assert.calledWith(
Expand Down Expand Up @@ -803,7 +824,6 @@ describe('BasicLTILaunchApp', () => {
wrapper,
'LaunchErrorDialog[errorState="error-authorizing"]',
);
await contentHidden(wrapper);
});

context('when auth fails multiple times in a row', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -259,11 +259,17 @@ describe('LaunchErrorDialog', () => {
},
{
errorState: 'error-reporting-submission',
expectedText: 'There was a problem submitting this Hypothesis assignment',
expectedTitle: 'Something went wrong',
expectedText:
'Your annotation was saved, but Hypothesis could not record a submission for grading.',
expectedTitle: 'Grading submission failed',
hasRetry: false,
withError: true,
},
{
errorState: 'canvas_submission_course_not_available',
expectedText: 'This may be because the course has ended',
expectedTitle: 'Grading submission failed',
},
].forEach(
({
errorState,
Expand Down Expand Up @@ -338,4 +344,22 @@ describe('LaunchErrorDialog', () => {
assert.isTrue(editLink.exists());
assert.equal(editLink.prop('href'), '/app/content-item-selection');
});

it('does not allow dismissing error if `onDismiss` callback is not provided', () => {
const wrapper = renderDialog();
const errorModal = wrapper.find('ErrorModal');
assert.isUndefined(errorModal.prop('onCancel'));
assert.isUndefined(errorModal.prop('cancelLabel'));
});

it('allows dismissing error if `onDismiss` callback is provided', () => {
const onDismiss = sinon.stub();
const wrapper = renderDialog({ onDismiss });
const errorModal = wrapper.find('ErrorModal');

errorModal.prop('onCancel')();

assert.equal(errorModal.prop('cancelLabel'), 'Dismiss');
assert.calledOnce(onDismiss);
});
});
2 changes: 2 additions & 0 deletions lms/static/scripts/frontend_apps/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export type LTILaunchServerErrorCode =
| 'canvas_studio_transcript_unavailable'
| 'canvas_studio_media_not_found'
| 'canvas_studio_admin_token_refresh_failed'
| 'canvas_submission_course_not_available'
| 'd2l_file_not_found_in_course_instructor'
| 'd2l_file_not_found_in_course_student'
| 'd2l_group_set_empty'
Expand Down Expand Up @@ -171,6 +172,7 @@ export function isLTILaunchServerError(error: ErrorLike): error is APIError {
'canvas_studio_transcript_unavailable',
'canvas_studio_media_not_found',
'canvas_studio_admin_token_refresh_failed',
'canvas_submission_course_not_available',
'vitalsource_user_not_found',
'vitalsource_no_book_license',
'moodle_page_not_found_in_course',
Expand Down

0 comments on commit 2ffe597

Please sign in to comment.