From 4ee2c1d6b76135b8df2caab26cf27416e974b36e Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 22 Aug 2024 11:59:26 +0200 Subject: [PATCH] Add error handling to filters --- .../dashboard/DashboardActivityFilters.tsx | 300 +++++++++++------- .../test/DashboardActivityFilters-test.js | 81 ++++- package.json | 2 +- yarn.lock | 10 +- 4 files changed, 270 insertions(+), 123 deletions(-) diff --git a/lms/static/scripts/frontend_apps/components/dashboard/DashboardActivityFilters.tsx b/lms/static/scripts/frontend_apps/components/dashboard/DashboardActivityFilters.tsx index ca0c8b5ea4..1865e32f05 100644 --- a/lms/static/scripts/frontend_apps/components/dashboard/DashboardActivityFilters.tsx +++ b/lms/static/scripts/frontend_apps/components/dashboard/DashboardActivityFilters.tsx @@ -1,9 +1,13 @@ import { + Button, CancelIcon, IconButton, MultiSelect, + RefreshIcon, } from '@hypothesis/frontend-shared'; -import { useMemo } from 'preact/hooks'; +import type { ComponentChildren } from 'preact'; +import type { MutableRef } from 'preact/hooks'; +import { useMemo, useRef } from 'preact/hooks'; import { useParams } from 'wouter-preact'; import type { @@ -15,6 +19,7 @@ import type { StudentsResponse, } from '../../api-types'; import { useConfig } from '../../config'; +import type { PaginatedFetchResult } from '../../utils/api'; import { usePaginatedAPIFetch } from '../../utils/api'; import { formatDateTime } from '../../utils/date'; @@ -55,8 +60,6 @@ export type DashboardActivityFiltersProps = { onClearSelection?: () => void; }; -type FiltersStudent = Student & { has_display_name: boolean }; - /** * Checks if provided element's scroll is at the bottom. * @param offset - Return true if the difference between the element's current @@ -69,12 +72,20 @@ function elementScrollIsAtBottom(element: HTMLElement, offset = 20): boolean { return distanceToTop >= triggerPoint; } +type PropsWithElementRef = T & { + /** Ref to be used on a `Select.Option` element */ + elementRef?: MutableRef; +}; + /** * Represents a `Select.Option` for a specific assignment */ -function AssignmentOption({ assignment }: { assignment: Assignment }) { +function AssignmentOption({ + assignment, + elementRef, +}: PropsWithElementRef<{ assignment: Assignment }>) { return ( - +
{assignment.title}
@@ -85,14 +96,37 @@ function AssignmentOption({ assignment }: { assignment: Assignment }) { ); } +/** + * Represents a `Select.Option` for a specific student + */ +function StudentOption({ + student, + elementRef, +}: PropsWithElementRef<{ student: Student }>) { + const hasDisplayName = !!student.display_name; + const displayName = + student.display_name ?? + `Student name unavailable (ID: ${student.lms_id.substring(0, 5)})`; + + return ( + + + {displayName} + + + ); +} + +type FiltersEntity = 'courses' | 'assignments' | 'students'; + /** * Placeholder to indicate a loading is in progress in one of the dropdowns */ -function LoadingOption({ - entity, -}: { - entity: 'courses' | 'assignments' | 'students'; -}) { +function LoadingOption({ entity }: { entity: FiltersEntity }) { return (
void; +}; + +/** + * Indicates an error occurred while loading filters, and presents a button to + * retry. + */ +function LoadingError({ entity, retry }: LoadingErrorProps) { + return ( +
+ Error loading {entity} + +
+ ); +} + +type PaginatedMultiSelectProps = { + result: PaginatedFetchResult[]>; + activeItem?: TResult; + renderOption: ( + item: NonNullable, + ref?: MutableRef, + ) => ComponentChildren; + entity: FiltersEntity; + buttonContent?: ComponentChildren; + value: TSelect[]; + onChange: (newValue: TSelect[]) => void; +}; + +/** + * A MultiSelect whose data is fetched from a paginated API. + * It includes loading and error indicators, and transparently loads more data + * while scrolling. + */ +function PaginatedMultiSelect({ + result, + activeItem, + entity, + renderOption, + buttonContent, + value, + onChange, +}: PaginatedMultiSelectProps) { + const lastOptionRef = useRef(null); + + return ( + { + if (elementScrollIsAtBottom(e.target as HTMLUListElement)) { + result.loadNextPage(); + } + }} + > + + All {entity} + + {activeItem ? ( + renderOption(activeItem, lastOptionRef) + ) : ( + <> + {result.data?.map((item, index, list) => + renderOption( + item, + list.length - 1 === index ? lastOptionRef : undefined, + ), + )} + {result.isLoading && } + {result.error && ( + { + // Focus last option before retrying, to avoid the listbox to + // be closed: + // - Starting the fetch retry will cause the result to no + // longer be in the error state, hence the Retry button will + // be umounted. + // - If the retry button had focus when unmounted, the focus + // would revert to the document body. + // - Since the body is outside the select dropdown, this would + // cause the select dropdown to auto-close. + // - To avoid this we focus a different element just before + // initiating the retry. + lastOptionRef.current?.focus(); + result.retry(); + }} + /> + )} + + )} + + ); +} + /** * Renders drop-downs to select courses, assignments and/or students, used to * filter dashboard activity metrics. @@ -187,30 +338,18 @@ export default function DashboardActivityFilters({ Student[], StudentsResponse >('students', routes.students, studentsFilters); - const studentsWithFallbackName: FiltersStudent[] | undefined = useMemo( - () => - studentsResult.data?.map(({ display_name, ...s }) => ({ - ...s, - display_name: - display_name ?? - `Student name unavailable (ID: ${s.lms_id.substring(0, 5)})`, - has_display_name: !!display_name, - })), - [studentsResult.data], - ); return (
- 'onChange' in courses ? courses.onChange(newCourseIds) : newCourseIds.length === 0 && courses.onClear() } - aria-label="Select courses" - containerClasses="!w-auto min-w-[180px]" buttonContent={ activeCourse ? ( activeCourse.title @@ -225,44 +364,26 @@ export default function DashboardActivityFilters({ <>{selectedCourseIds.length} courses ) } - data-testid="courses-select" - onListboxScroll={e => { - if (elementScrollIsAtBottom(e.target as HTMLUListElement)) { - coursesResult.loadNextPage(); - } - }} - > - All courses - {activeCourse ? ( + activeItem={activeCourse} + renderOption={(course, elementRef) => ( - {activeCourse.title} + {course.title} - ) : ( - <> - {coursesResult.data?.map(course => ( - - {course.title} - - ))} - {coursesResult.isLoading && !coursesResult.isLoadingFirstPage && ( - - )} - )} - - + 'onChange' in assignments ? assignments.onChange(newAssignmentIds) : newAssignmentIds.length === 0 && assignments.onClear() } - aria-label="Select assignments" - containerClasses="!w-auto min-w-[180px]" buttonContent={ activeAssignment ? ( activeAssignment.title @@ -278,76 +399,41 @@ export default function DashboardActivityFilters({ <>{selectedAssignmentIds.length} assignments ) } - data-testid="assignments-select" - onListboxScroll={e => { - if (elementScrollIsAtBottom(e.target as HTMLUListElement)) { - assignmentsResults.loadNextPage(); - } - }} - > - - All assignments - - {activeAssignment ? ( - - ) : ( - <> - {assignmentsResults.data?.map(assignment => ( - - ))} - {assignmentsResults.isLoading && - !assignmentsResults.isLoadingFirstPage && ( - - )} - + activeItem={activeAssignment} + renderOption={(assignment, elementRef) => ( + )} - - + students.onChange(newStudentIds)} - aria-label="Select students" - containerClasses="!w-auto min-w-[180px]" buttonContent={ studentsResult.isLoadingFirstPage ? ( <>... ) : students.selectedIds.length === 0 ? ( <>All students ) : students.selectedIds.length === 1 ? ( - studentsWithFallbackName?.find( + studentsResult.data?.find( s => s.h_userid === students.selectedIds[0], )?.display_name ?? '1 student' ) : ( <>{students.selectedIds.length} students ) } - data-testid="students-select" - onListboxScroll={e => { - if (elementScrollIsAtBottom(e.target as HTMLUListElement)) { - studentsResult.loadNextPage(); - } - }} - > - All students - {studentsWithFallbackName?.map(student => ( - - - {student.display_name} - - - ))} - {studentsResult.isLoading && !studentsResult.isLoadingFirstPage && ( - + renderOption={(student, elementRef) => ( + )} - + /> {hasSelection && onClearSelection && ( { * @param {object} options * @param {boolean} options.isLoading * @param {boolean} [options.isLoadingFirstPage] - Defaults to isLoading + * @param {Error} [options.error] - Defaults to null + * @param {() => void} [options.retry] - Defaults to noop */ function configureFakeAPIFetch(options) { - const { isLoading, isLoadingFirstPage = isLoading } = options; + const { + isLoading, + isLoadingFirstPage = isLoading, + error = null, + retry = () => {}, + } = options; fakeUsePaginatedAPIFetch.onCall(0).returns({ isLoading, isLoadingFirstPage, - data: isLoading ? null : courses, + data: isLoading || error ? null : courses, loadNextPage: fakeLoadNextCoursesPage, + error, + retry, }); fakeUsePaginatedAPIFetch.onCall(1).returns({ isLoading, isLoadingFirstPage, - data: isLoading ? null : assignments, + data: isLoading || error ? null : assignments, loadNextPage: fakeLoadNextAssignmentsPage, + error, + retry, }); fakeUsePaginatedAPIFetch.onCall(2).returns({ isLoading, isLoadingFirstPage, - data: isLoading ? null : students, + data: isLoading || error ? null : students, loadNextPage: fakeLoadNextStudentsPage, + error, + retry, }); } @@ -257,23 +270,23 @@ describe('DashboardActivityFilters', () => { }); }); - [true, false].forEach(isLoadingFirstPage => { - it('shows page loading indicators when loading but not initially loading', () => { - configureFakeAPIFetch({ isLoading: true, isLoadingFirstPage }); + [true, false].forEach(isLoading => { + it('shows page loading indicators when loading', () => { + configureFakeAPIFetch({ isLoading }); const wrapper = createComponent(); assert.equal( wrapper.exists('[data-testid="loading-more-courses"]'), - !isLoadingFirstPage, + isLoading, ); assert.equal( wrapper.exists('[data-testid="loading-more-assignments"]'), - !isLoadingFirstPage, + isLoading, ); assert.equal( wrapper.exists('[data-testid="loading-more-students"]'), - !isLoadingFirstPage, + isLoading, ); }); }); @@ -544,6 +557,54 @@ describe('DashboardActivityFilters', () => { ); }); + describe('error handling', () => { + let fakeRetry; + + beforeEach(() => { + fakeRetry = sinon.stub(); + configureFakeAPIFetch({ + isLoading: false, + error: new Error('An error occurred'), + retry: fakeRetry, + }); + }); + + function clickRetry(wrapper, entity) { + wrapper + .find(`[data-testid="${entity}-error"]`) + .find('button') + .simulate('click'); + } + + ['courses', 'assignments', 'students'].forEach(entity => { + it('shows error message when loading filters fails', () => { + const wrapper = createComponent(); + assert.isTrue(wrapper.exists(`[data-testid="${entity}-error"]`)); + }); + + it('retries loading when retry button is clicked', () => { + const wrapper = createComponent(); + + clickRetry(wrapper, entity); + assert.called(fakeRetry); + }); + + it('focuses last option when retry button is clicked', () => { + const wrapper = createComponent(); + + const lastOption = wrapper + .find(`[data-testid="${entity}-select"]`) + .find('[role="option"]') + .last() + .getDOMNode(); + const focusOption = sinon.stub(lastOption, 'focus'); + + clickRetry(wrapper, entity); + assert.called(focusOption); + }); + }); + }); + it( 'should pass a11y checks', checkAccessibility({ diff --git a/package.json b/package.json index 8c496aff4e..4e625d955e 100644 --- a/package.json +++ b/package.json @@ -26,7 +26,7 @@ "@babel/preset-react": "^7.24.7", "@babel/preset-typescript": "^7.24.7", "@hypothesis/frontend-build": "^3.0.0", - "@hypothesis/frontend-shared": "^8.3.0", + "@hypothesis/frontend-shared": "^8.4.0", "@rollup/plugin-babel": "^6.0.4", "@rollup/plugin-commonjs": "^26.0.1", "@rollup/plugin-node-resolve": "^15.2.3", diff --git a/yarn.lock b/yarn.lock index 7a4747f9d9..f2ca590b45 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2081,15 +2081,15 @@ __metadata: languageName: node linkType: hard -"@hypothesis/frontend-shared@npm:^8.3.0": - version: 8.3.0 - resolution: "@hypothesis/frontend-shared@npm:8.3.0" +"@hypothesis/frontend-shared@npm:^8.4.0": + version: 8.4.0 + resolution: "@hypothesis/frontend-shared@npm:8.4.0" dependencies: highlight.js: ^11.6.0 wouter-preact: ^3.0.0 peerDependencies: preact: ^10.4.0 - checksum: e08dfe4c290f8c8aa3f18a64ca1dfca52180e1fe82fe4c3c45ea08a293fb59acdcc37d48d07ade6dd6179d694ea3fcc708b2a04fa2a8e0b92809ca291df56345 + checksum: 34d7ae1cab013825be55504c5a9af582b0f20962a358192e66dad19097dfaa6ee052129399dcec9e317c41ba5a2e4b595f33b40b639404ab5f9e997b27cc93e1 languageName: node linkType: hard @@ -7321,7 +7321,7 @@ __metadata: "@babel/preset-react": ^7.24.7 "@babel/preset-typescript": ^7.24.7 "@hypothesis/frontend-build": ^3.0.0 - "@hypothesis/frontend-shared": ^8.3.0 + "@hypothesis/frontend-shared": ^8.4.0 "@hypothesis/frontend-testing": ^1.2.2 "@rollup/plugin-babel": ^6.0.4 "@rollup/plugin-commonjs": ^26.0.1