From a8e485557c8829daf90a255de087695a55172467 Mon Sep 17 00:00:00 2001 From: Robert Knight Date: Fri, 6 Dec 2024 09:32:01 +0000 Subject: [PATCH] Don't show error if user navigates while fetching members Don't show error messages when requests fail due to being canceled from the client end while in flight. This can happen when navigating (eg. changing page) while members are being fetched. --- .../components/EditGroupMembersForm.tsx | 22 +++++++--- .../test/EditGroupMembersForm-test.js | 40 +++++++++++++++++-- 2 files changed, 53 insertions(+), 9 deletions(-) diff --git a/h/static/scripts/group-forms/components/EditGroupMembersForm.tsx b/h/static/scripts/group-forms/components/EditGroupMembersForm.tsx index 5aeaaa9d732..95714c33f4f 100644 --- a/h/static/scripts/group-forms/components/EditGroupMembersForm.tsx +++ b/h/static/scripts/group-forms/components/EditGroupMembersForm.tsx @@ -6,12 +6,13 @@ import { Pagination, Select, } from '@hypothesis/frontend-shared'; -import { useContext, useEffect, useState } from 'preact/hooks'; +import { useCallback, useContext, useEffect, useState } from 'preact/hooks'; import { Config } from '../config'; import type { APIConfig, Group } from '../config'; import type { GroupMember, GroupMembersResponse, Role } from '../utils/api'; import { callAPI } from '../utils/api'; +import type { APIError } from '../utils/api'; import FormContainer from './forms/FormContainer'; import ErrorNotice from './ErrorNotice'; import GroupFormHeader from './GroupFormHeader'; @@ -180,9 +181,18 @@ export default function EditGroupMembersForm({ const [totalMembers, setTotalMembers] = useState(null); const totalPages = totalMembers !== null ? Math.ceil(totalMembers / pageSize) : null; + const [errorMessage, setErrorMessage] = useState(null); + + const setError = useCallback((context: string, err: Error) => { + const apiErr = err as APIError; + if (apiErr.aborted) { + return; + } + const message = `${context}: ${err.message}`; + setErrorMessage(message); + }, []); // Fetch group members when the form loads. - const [errorMessage, setErrorMessage] = useState(null); const [members, setMembers] = useState(null); useEffect(() => { // istanbul ignore next @@ -201,12 +211,12 @@ export default function EditGroupMembersForm({ setTotalMembers(total); }) .catch(err => { - setErrorMessage(`Failed to fetch group members: ${err.message}`); + setError('Failed to fetch group members', err); }); return () => { abort.abort(); }; - }, [config.api.readGroupMembers, currentUserid, pageIndex]); + }, [config.api.readGroupMembers, currentUserid, pageIndex, setError]); const columns: TableColumn[] = [ { @@ -256,7 +266,7 @@ export default function EditGroupMembersForm({ ); } catch (err) { updateMember(member.userid, { busy: false }); - setErrorMessage(err.message); + setError('Failed to remove member', err); } }; @@ -274,7 +284,7 @@ export default function EditGroupMembersForm({ } catch (err) { const prevRole = member.role; updateMember(member.userid, { role: prevRole, busy: false }); - setErrorMessage(err.message); + setError('Failed to change member role', err); } }; diff --git a/h/static/scripts/group-forms/components/test/EditGroupMembersForm-test.js b/h/static/scripts/group-forms/components/test/EditGroupMembersForm-test.js index 7f0af0466c0..a3e8176cf59 100644 --- a/h/static/scripts/group-forms/components/test/EditGroupMembersForm-test.js +++ b/h/static/scripts/group-forms/components/test/EditGroupMembersForm-test.js @@ -1,7 +1,13 @@ -import { mount, waitFor, waitForElement } from '@hypothesis/frontend-testing'; +import { + delay, + mount, + waitFor, + waitForElement, +} from '@hypothesis/frontend-testing'; import { Select } from '@hypothesis/frontend-shared'; import { act } from 'preact/test-utils'; +import { APIError } from '../../utils/api'; import { Config } from '../../config'; import { $imports, @@ -201,6 +207,15 @@ describe('EditGroupMembersForm', () => { ); }; + /** Construct an APIError corresponding to an aborted request. */ + const abortError = () => { + const abortError = new Error('Aborted'); + abortError.name = 'AbortError'; + return new APIError('Something went wrong', { + cause: abortError, + }); + }; + it('fetches and displays members', async () => { const wrapper = createForm(); assert.calledWith( @@ -293,6 +308,19 @@ describe('EditGroupMembersForm', () => { ); }); + // Don't show an error if fetching members is canceled due to a navigation + // (eg. page change) happening during the fetch. + it('does not display error if member fetch is aborted', async () => { + fakeCallAPI.withArgs('/api/groups/1234/members').rejects(abortError()); + const wrapper = createForm(); + + await delay(0); + + wrapper.update(); + assert.equal(wrapper.find('ErrorNotice').prop('message'), null); + assert.deepEqual(getRenderedUsernames(wrapper), []); + }); + it('handles member fetch being canceled', () => { const wrapper = createForm(); assert.calledWith(fakeCallAPI, '/api/groups/1234/members'); @@ -371,7 +399,10 @@ describe('EditGroupMembersForm', () => { await waitForError(wrapper); const error = wrapper.find('ErrorNotice'); - assert.equal(error.prop('message'), 'User not found'); + assert.equal( + error.prop('message'), + 'Failed to remove member: User not found', + ); // Controls should be re-enabled after saving fails. assert.include(getRenderedUsernames(wrapper), '@bob'); @@ -470,7 +501,10 @@ describe('EditGroupMembersForm', () => { // Wait for the role change to fail. An error should be displayed. await waitForError(wrapper); const error = wrapper.find('ErrorNotice'); - assert.equal(error.prop('message'), 'Invalid role'); + assert.equal( + error.prop('message'), + 'Failed to change member role: Invalid role', + ); // The displayed role should revert to the original value. wrapper.update();