Skip to content

Commit

Permalink
Improve error handling in Pace Plans
Browse files Browse the repository at this point in the history
This change improves the error alert that is shown for various error
conditions within Pace Plans. It adds an expandable area where the raw
details of an error can be displayed, and for select errors (like
publishing), displays a "Retry" button for easy recovery.

This change adds the new `ExpandableErrorAlert` component to
`@canvas/alerts`, so it can be used elsewhere in Canvas, as well.

closes LS-2776
flag=pace_plans

test plan:
- visit `/pace_plans` for a course with pace plans enabled
- using the Chrome dev tools, set network throttling to 'Offline' on the
  'Network' tab (to simulate an error state)
- make some changes, and attempt to publish them
- verify that an error is displayed, with an option to view more details
- verify that clicking 'View details' and 'Hide details' correctly
  toggles the raw error details
- verify that the unpublished changes indicator is now red, and reads
  "Publishing error"
- disable network throttling, and click the "Retry" button
- verify that the error disappears, and your pace plan is successfully
  published
- verify that the red "Publishing error" text is gone
- repeat the above using Safari and VoiceOver (you may need to stop your
  rails server instead of using network throttling, as Safari doesn't
  have that feature)
- when the error first appears, verify that VoiceOver reads it promptly
  (without requiring navigation by the user to select it)
- verify that focus was automatically transferred to the alert

Change-Id: I80e7f0a0d1f40af14dd3ee56ce344d25bf12a00e
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/276602
Reviewed-by: Ed Schiebel <[email protected]>
Tested-by: Service Cloud Jenkins <[email protected]>
QA-Review: Ed Schiebel <[email protected]>
Product-Review: Peyton Craighill <[email protected]>
  • Loading branch information
rmsy committed Nov 11, 2021
1 parent 6311053 commit 08e8d91
Show file tree
Hide file tree
Showing 15 changed files with 482 additions and 65 deletions.
11 changes: 0 additions & 11 deletions ui/features/pace_plans/react/__tests__/app.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ const pollForPublishStatus = jest.fn()
const setResponsiveSize = jest.fn()

const defaultProps = {
errorMessage: '',
loadingMessage: '',
pollForPublishStatus,
responsiveSize: 'large' as const,
Expand All @@ -39,16 +38,6 @@ afterEach(() => {
})

describe('App', () => {
it('renders an error alert if there is an error message', () => {
const {getByText} = renderConnected(<App {...defaultProps} errorMessage="Ruh Roh!" />)
expect(getByText('Ruh Roh!')).toBeInTheDocument()
})

it('renders a loading indicator if there is a loading message', () => {
const {getByText} = renderConnected(<App {...defaultProps} errorMessage="Please wait..." />)
expect(getByText('Please wait...')).toBeInTheDocument()
})

it('starts polling for published status updates on mount', () => {
renderConnected(<App {...defaultProps} />)
expect(pollForPublishStatus).toHaveBeenCalled()
Expand Down
28 changes: 17 additions & 11 deletions ui/features/pace_plans/react/actions/__tests__/pace_plans.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,14 @@ describe('Pace plans actions', () => {
await thunkedAction(dispatch, getState)

expect(dispatch.mock.calls[0]).toEqual([uiActions.showLoadingOverlay('Starting publish...')])
expect(dispatch.mock.calls[1]).toEqual([pacePlanActions.setPacePlan(updatedPlan)])
expect(dispatch.mock.calls[2]).toEqual([pacePlanActions.setProgress(PROGRESS_RUNNING)])
expect(dispatch.mock.calls[1]).toEqual([uiActions.clearCategoryError('publish')])
expect(dispatch.mock.calls[2]).toEqual([pacePlanActions.setPacePlan(updatedPlan)])
expect(dispatch.mock.calls[3]).toEqual([pacePlanActions.setProgress(PROGRESS_RUNNING)])
// Compare dispatched functions by name since they won't be directly equal
expect(JSON.stringify(dispatch.mock.calls[3])).toEqual(
expect(JSON.stringify(dispatch.mock.calls[4])).toEqual(
JSON.stringify([pacePlanActions.pollForPublishStatus()])
)
expect(dispatch.mock.calls[4]).toEqual([uiActions.hideLoadingOverlay()])
expect(dispatch.mock.calls[5]).toEqual([uiActions.hideLoadingOverlay()])
expect(fetchMock.called(UPDATE_API, 'PUT')).toBe(true)
})

Expand All @@ -89,18 +90,20 @@ describe('Pace plans actions', () => {

it('Sets an error message if the plan update fails', async () => {
const updatedPlan = {...PRIMARY_PLAN, excludeWeekends: false}
const error = new Error("You don't actually want to publish this")
const getState = mockGetState(updatedPlan, PRIMARY_PLAN)
fetchMock.put(UPDATE_API, {
throws: new Error("You don't actually want to publish this")
throws: error
})

const thunkedAction = pacePlanActions.publishPlan()
await thunkedAction(dispatch, getState)

expect(dispatch.mock.calls).toEqual([
[uiActions.showLoadingOverlay('Starting publish...')],
[uiActions.clearCategoryError('publish')],
[uiActions.hideLoadingOverlay()],
[uiActions.setErrorMessage('There was an error publishing your plan.')]
[uiActions.setCategoryError('publish', error.toString())]
])
})
})
Expand Down Expand Up @@ -135,7 +138,8 @@ describe('Pace plans actions', () => {

await pacePlanActions.pollForPublishStatus()(dispatch, getState)

expect(dispatch.mock.calls).toEqual([[pacePlanActions.setProgress(progressUpdated)]])
expect(dispatch.mock.calls[0]).toEqual([pacePlanActions.setProgress(progressUpdated)])
expect(dispatch.mock.calls[1]).toEqual([uiActions.clearCategoryError('checkPublishStatus')])
expect(setTimeout).toHaveBeenCalledTimes(1)

const progressCompleted = {...PROGRESS_RUNNING, completion: 100, workflow_state: 'completed'}
Expand All @@ -144,8 +148,9 @@ describe('Pace plans actions', () => {
jest.advanceTimersByTime(PUBLISH_STATUS_POLLING_MS)

await waitFor(() => {
expect(dispatch.mock.calls.length).toBe(2)
expect(dispatch.mock.calls[1]).toEqual([pacePlanActions.setProgress(undefined)])
expect(dispatch.mock.calls.length).toBe(4)
expect(dispatch.mock.calls[1]).toEqual([uiActions.clearCategoryError('checkPublishStatus')])
expect(dispatch.mock.calls[2]).toEqual([pacePlanActions.setProgress(undefined)])
expect(screen.getByText('Finished publishing plan')).toBeInTheDocument()
})
})
Expand All @@ -155,12 +160,13 @@ describe('Pace plans actions', () => {
...DEFAULT_STORE_STATE,
pacePlan: {publishingProgress: {...PROGRESS_RUNNING}}
})
fetchMock.get(PROGRESS_API, {throws: new Error('Progress? What progress?')})
const error = new Error('Progress? What progress?')
fetchMock.get(PROGRESS_API, {throws: error})

await pacePlanActions.pollForPublishStatus()(dispatch, getState)

expect(dispatch.mock.calls).toEqual([
[uiActions.setErrorMessage('There was an error checking plan publishing status')]
[uiActions.setCategoryError('checkPublishStatus', error?.toString())]
])
expect(setTimeout).not.toHaveBeenCalled()
})
Expand Down
4 changes: 2 additions & 2 deletions ui/features/pace_plans/react/actions/autosaving.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,14 +60,14 @@ const updatePacePlan = (
dispatch(uiActions.hideLoadingOverlay())
}
dispatch(uiActions.autoSaveCompleted()) // Update the UI state
dispatch(uiActions.setErrorMessage(''))
dispatch(uiActions.clearCategoryError('autosaving'))
})
.catch(error => {
if (shouldBlock) {
dispatch(uiActions.hideLoadingOverlay())
}
dispatch(uiActions.autoSaveCompleted())
dispatch(uiActions.setErrorMessage('There was an error saving your changes'))
dispatch(uiActions.setCategoryError('autosaving', error?.toString()))
console.error(error) // eslint-disable-line no-console
})
}
Expand Down
21 changes: 10 additions & 11 deletions ui/features/pace_plans/react/actions/pace_plans.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ const thunkActions = {
publishPlan: (): ThunkAction<Promise<void>, StoreState, void, Action> => {
return (dispatch, getState) => {
dispatch(uiActions.showLoadingOverlay(I18n.t('Starting publish...')))
dispatch(uiActions.clearCategoryError('publish'))

return Api.publish(getState().pacePlan)
.then(responseBody => {
Expand All @@ -76,7 +77,7 @@ const thunkActions = {
})
.catch(error => {
dispatch(uiActions.hideLoadingOverlay())
dispatch(uiActions.setErrorMessage(I18n.t('There was an error publishing your plan.')))
dispatch(uiActions.setCategoryError('publish', error?.toString()))
console.log(error) // eslint-disable-line no-console
})
}
Expand All @@ -96,6 +97,7 @@ const thunkActions = {
updatedProgress.workflow_state !== 'completed' ? updatedProgress : undefined
)
)
dispatch(uiActions.clearCategoryError('checkPublishStatus'))
if (TERMINAL_PROGRESS_STATUSES.includes(updatedProgress.workflow_state)) {
showFlashAlert({
message: I18n.t('Finished publishing plan'),
Expand All @@ -108,11 +110,7 @@ const thunkActions = {
}
})
.catch(error => {
dispatch(
uiActions.setErrorMessage(
I18n.t('There was an error checking plan publishing status')
)
)
dispatch(uiActions.setCategoryError('checkPublishStatus', error?.toString()))
console.log(error) // eslint-disable-line no-console
})
return pollingLoop()
Expand All @@ -124,6 +122,7 @@ const thunkActions = {
): ThunkAction<void, StoreState, void, Action> => {
return async (dispatch, getState) => {
dispatch(uiActions.showLoadingOverlay(I18n.t('Loading...')))
dispatch(uiActions.clearCategoryError('resetToLastPublished'))

await Api.waitForActionCompletion(() => getState().ui.autoSaving)

Expand All @@ -135,9 +134,7 @@ const thunkActions = {
})
.catch(error => {
dispatch(uiActions.hideLoadingOverlay())
dispatch(
uiActions.setErrorMessage(I18n.t('There was an error resetting to the previous plan.'))
)
dispatch(uiActions.setCategoryError('resetToLastPublished', error?.toString()))
console.error(error) // eslint-disable-line no-console
})
}
Expand All @@ -149,6 +146,7 @@ const thunkActions = {
): ThunkAction<void, StoreState, void, Action> => {
return async (dispatch, getState) => {
dispatch(uiActions.showLoadingOverlay(I18n.t('Loading...')))
dispatch(uiActions.clearCategoryError('loading'))

await Api.waitForActionCompletion(() => getState().ui.autoSaving)

Expand All @@ -160,7 +158,7 @@ const thunkActions = {
})
.catch(error => {
dispatch(uiActions.hideLoadingOverlay())
dispatch(uiActions.setErrorMessage(I18n.t('There was an error loading the plan.')))
dispatch(uiActions.setCategoryError('loading', error?.toString()))
console.error(error) // eslint-disable-line no-console
})
}
Expand All @@ -171,6 +169,7 @@ const thunkActions = {
if (!pacePlanId) return Promise.reject(new Error(I18n.t('Cannot relink unsaved plans')))

dispatch(uiActions.showLoadingOverlay(I18n.t('Relinking plans...')))
dispatch(uiActions.clearCategoryError('relinkToParent'))

await Api.waitForActionCompletion(() => getState().ui.autoSaving)

Expand All @@ -182,7 +181,7 @@ const thunkActions = {
})
.catch(error => {
dispatch(uiActions.hideLoadingOverlay())
dispatch(uiActions.setErrorMessage(I18n.t('There was an error linking plan.')))
dispatch(uiActions.setCategoryError('relinkToParent', error?.toString()))
console.error(error) // eslint-disable-line no-console
})
}
Expand Down
7 changes: 5 additions & 2 deletions ui/features/pace_plans/react/actions/ui.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ import {pacePlanActions} from './pace_plans'
export enum Constants {
START_AUTO_SAVING = 'UI/START_AUTO_SAVING',
AUTO_SAVE_COMPLETED = 'UI/AUTO_SAVE_COMPLETED',
SET_ERROR_MESSAGE = 'UI/SET_ERROR_MESSAGE',
SET_CATEGORY_ERROR = 'UI/SET_CATEGORY_ERROR',
CLEAR_CATEGORY_ERROR = 'UI/CLEAR_CATEGORY_ERROR',
TOGGLE_DIVIDE_INTO_WEEKS = 'UI/TOGGLE_DIVIDE_INTO_WEEKS',
TOGGLE_SHOW_PROJECTIONS = 'UI/TOGGLE_SHOW_PROJECTIONS',
SET_SELECTED_PLAN_CONTEXT = 'UI/SET_SELECTED_PLAN_CONTEXT',
Expand All @@ -45,7 +46,9 @@ export enum Constants {
export const regularActions = {
startAutoSave: () => createAction(Constants.START_AUTO_SAVING),
autoSaveCompleted: () => createAction(Constants.AUTO_SAVE_COMPLETED),
setErrorMessage: (message: string) => createAction(Constants.SET_ERROR_MESSAGE, message),
setCategoryError: (category: string, error?: string) =>
createAction(Constants.SET_CATEGORY_ERROR, {category, error: error || ''}),
clearCategoryError: (category: string) => createAction(Constants.CLEAR_CATEGORY_ERROR, category),
toggleDivideIntoWeeks: () => createAction(Constants.TOGGLE_DIVIDE_INTO_WEEKS),
toggleShowProjections: () => createAction(Constants.TOGGLE_SHOW_PROJECTIONS),
showLoadingOverlay: (message: string) => createAction(Constants.SHOW_LOADING_OVERLAY, message),
Expand Down
19 changes: 3 additions & 16 deletions ui/features/pace_plans/react/app.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import React, {useEffect, useState} from 'react'
import {connect} from 'react-redux'

import {Alert} from '@instructure/ui-alerts'
import {Flex} from '@instructure/ui-flex'
import {Mask, Overlay} from '@instructure/ui-overlays'
import {Responsive} from '@instructure/ui-responsive'
Expand All @@ -31,17 +30,17 @@ import Body from './components/body'
import Footer from './components/footer'
import Header from './components/header/header'
import {ResponsiveSizes, StoreState} from './types'
import {getErrorMessage, getLoadingMessage, getShowLoadingOverlay} from './reducers/ui'
import {getLoadingMessage, getShowLoadingOverlay} from './reducers/ui'
import UnpublishedChangesTrayContents from './components/unpublished_changes_tray_contents'
// @ts-ignore: TS doesn't understand i18n scoped imports
import I18n from 'i18n!pace_plans_app'
import {getSummarizedChanges} from './reducers/pace_plans'
import {pacePlanActions} from './actions/pace_plans'
import {SummarizedChange} from './utils/change_tracking'
import {Tray} from '@instructure/ui-tray'
import Errors from './components/errors'

interface StoreProps {
readonly errorMessage: string
readonly loadingMessage: string
readonly showLoadingOverlay: boolean
readonly unpublishedChanges: SummarizedChange[]
Expand All @@ -59,7 +58,6 @@ type ResponsiveComponentProps = ComponentProps & {
}

export const App: React.FC<ResponsiveComponentProps> = ({
errorMessage,
loadingMessage,
setResponsiveSize,
showLoadingOverlay,
Expand All @@ -78,16 +76,6 @@ export const App: React.FC<ResponsiveComponentProps> = ({
setResponsiveSize(responsiveSize)
}, [responsiveSize, setResponsiveSize])

const renderErrorAlert = () => {
if (errorMessage) {
return (
<Alert variant="error" closeButtonLabel="Close" margin="small">
{errorMessage}
</Alert>
)
}
}

return (
<View>
<Overlay open={showLoadingOverlay} transition="fade" label={loadingMessage}>
Expand All @@ -97,7 +85,7 @@ export const App: React.FC<ResponsiveComponentProps> = ({
</Overlay>
<Flex as="div" direction="column" margin="small">
<View>
{renderErrorAlert()}
<Errors />
<Header handleDrawerToggle={() => setTrayOpen(!trayOpen)} />
</View>
<Body />
Expand Down Expand Up @@ -138,7 +126,6 @@ export const ResponsiveApp: React.FC<ComponentProps> = props => (

const mapStateToProps = (state: StoreState): StoreProps => {
return {
errorMessage: getErrorMessage(state),
loadingMessage: getLoadingMessage(state),
showLoadingOverlay: getShowLoadingOverlay(state),
unpublishedChanges: getSummarizedChanges(state)
Expand Down
62 changes: 62 additions & 0 deletions ui/features/pace_plans/react/components/__tests__/errors.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
/*
* Copyright (C) 2021 - present Instructure, Inc.
*
* This file is part of Canvas.
*
* Canvas is free software: you can redistribute it and/or modify it under
* the terms of the GNU Affero General Public License as published by the Free
* Software Foundation, version 3 of the License.
*
* Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
* WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
* A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
* details.
*
* You should have received a copy of the GNU Affero General Public License along
* with this program. If not, see <http://www.gnu.org/licenses/>.
*/

import {Errors, ErrorsProps} from '../errors'
import {act, render} from '@testing-library/react'
import React from 'react'
import userEvent from '@testing-library/user-event'

const publishPlan = jest.fn()
afterEach(jest.clearAllMocks)

describe('Errors', () => {
const defaultProps: ErrorsProps = {
errors: {
publish: 'TypeError: Failed to fetch',
loading: 'TypeError: Failed to fetch',
darkMode: 'E_THEME_TOO_DARK: Theme too dark, user could trip and fall'
},
responsiveSize: 'large',
publishPlan
}

it('renders nothing when there are no errors', () => {
const {container} = render(<Errors {...defaultProps} errors={{}} />)

expect(container.firstChild).toBeEmpty()
})

it('displays custom error messages for pre-defined categories, and generic error messages for unknown categories', () => {
const {getAllByText} = render(<Errors {...defaultProps} />)

for (const error of [
...getAllByText('There was an error publishing your pace plan.'),
...getAllByText('There was an error loading the plan.'),
...getAllByText('An error has occurred.')
]) {
expect(error).toBeInTheDocument()
}
})

it('triggers a re-publish when the retry button is clicked', () => {
const {getByRole} = render(<Errors {...defaultProps} />)

act(() => userEvent.click(getByRole('button', {name: 'Retry'})))
expect(publishPlan).toHaveBeenCalled()
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* with this program. If not, see <http://www.gnu.org/licenses/>.
*/

import {render} from '@testing-library/react'
import {act, render} from '@testing-library/react'
import {UnpublishedChangesIndicator} from '../unpublished_changes_indicator'
import React from 'react'
import userEvent from '@testing-library/user-event'
Expand Down Expand Up @@ -62,7 +62,7 @@ describe('UnpublishedChangesIndicator', () => {
<UnpublishedChangesIndicator {...defaultProps} changeCount={3} onClick={onClick} />
)

userEvent.click(getByRole('button', {name: '3 unpublished changes'}))
act(() => userEvent.click(getByRole('button', {name: '3 unpublished changes'})))
expect(onClick).toHaveBeenCalled()
})

Expand All @@ -71,7 +71,7 @@ describe('UnpublishedChangesIndicator', () => {
<UnpublishedChangesIndicator {...defaultProps} changeCount={0} onClick={onClick} />
)

userEvent.click(getByText('All changes published'))
act(() => userEvent.click(getByText('All changes published')))
expect(onClick).not.toHaveBeenCalled()
})
})
Expand Down
Loading

0 comments on commit 08e8d91

Please sign in to comment.