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

psp-7099 prevent infinite refresh loop of acq files when user is not … #3548

Merged
merged 2 commits into from
Oct 30, 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
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,11 @@
>('Failed to update Acquisition File');

const {
getAcquisitionFile: { execute: retrieveAcquisitionFile, loading: loadingAcquisitionFile },
getAcquisitionFile: {
execute: retrieveAcquisitionFile,
loading: loadingAcquisitionFile,
error,
},
updateAcquisitionProperties,
getAcquisitionProperties: {
execute: retrieveAcquisitionFileProperties,
Expand Down Expand Up @@ -121,6 +125,10 @@
// Retrieve acquisition file from API and save it to local state and side-bar context
const fetchAcquisitionFile = useCallback(async () => {
var retrieved = await retrieveAcquisitionFile(acquisitionFileId);
if (retrieved === undefined) {
return;

Check warning on line 129 in source/frontend/src/features/mapSideBar/acquisition/AcquisitionContainer.tsx

View check run for this annotation

Codecov / codecov/patch

source/frontend/src/features/mapSideBar/acquisition/AcquisitionContainer.tsx#L129

Added line #L129 was not covered by tests
}

// retrieve related entities (ie properties, checklist items) in parallel
const acquisitionPropertiesTask = retrieveAcquisitionFileProperties(acquisitionFileId);
const acquisitionChecklistTask = retrieveAcquisitionFileChecklist(acquisitionFileId);
Expand All @@ -130,9 +138,9 @@
if (retrieved) {
retrieved.fileProperties = acquisitionProperties;
retrieved.acquisitionFileChecklist = acquisitionChecklist;
setFile({ ...retrieved, fileType: FileTypes.Acquisition });
setStaleFile(false);
}
setFile({ ...retrieved, fileType: FileTypes.Acquisition });
setStaleFile(false);
}, [
acquisitionFileId,
retrieveAcquisitionFileProperties,
Expand Down Expand Up @@ -288,6 +296,7 @@
canRemove={canRemove}
formikRef={formikRef}
isFormValid={isValid}
error={error}
></View>
);
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ const DEFAULT_PROPS: IAcquisitionViewProps = {
},
formikRef: React.createRef(),
isFormValid: true,
error: undefined,
};

const history = createMemoryHistory();
Expand Down Expand Up @@ -234,4 +235,13 @@ describe('AcquisitionView component', () => {
expect(tab).toBeVisible();
expect(tab).toHaveClass('active');
});

it(`should display an error message when the error prop is set.`, async () => {
const { getByText } = await act(() => setup({ ...DEFAULT_PROPS, error: {} } as any));
expect(
getByText(
'Failed to load Acquisition File. Check the detailed error in the top right for more details.',
),
).toBeVisible();
});
});
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { AxiosError } from 'axios';
import { FormikProps } from 'formik';
import React, { useContext } from 'react';
import {
Expand All @@ -16,6 +17,7 @@ import GenericModal from '@/components/common/GenericModal';
import { FileTypes } from '@/constants';
import FileLayout from '@/features/mapSideBar/layout/FileLayout';
import MapSideBarLayout from '@/features/mapSideBar/layout/MapSideBarLayout';
import { IApiError } from '@/interfaces/IApiError';
import { Api_AcquisitionFile } from '@/models/api/AcquisitionFile';
import { Api_File } from '@/models/api/File';
import { stripTrailingSlash } from '@/utils';
Expand Down Expand Up @@ -48,6 +50,7 @@ export interface IAcquisitionViewProps {
setContainerState: React.Dispatch<Partial<AcquisitionContainerState>>;
formikRef: React.RefObject<FormikProps<any>>;
isFormValid: boolean;
error: AxiosError<IApiError, any> | undefined;
}

export const AcquisitionView: React.FunctionComponent<IAcquisitionViewProps> = ({
Expand All @@ -66,6 +69,7 @@ export const AcquisitionView: React.FunctionComponent<IAcquisitionViewProps> = (
setContainerState,
formikRef,
isFormValid,
error,
}) => {
// match for the current route
const location = useLocation();
Expand Down Expand Up @@ -158,6 +162,12 @@ export const AcquisitionView: React.FunctionComponent<IAcquisitionViewProps> = (
}
bodyComponent={
<StyledFormWrapper>
{error && (
<b>
Failed to load Acquisition File. Check the detailed error in the top right for
more details.
</b>
)}
Comment on lines +165 to +170
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: please add unit tests to cover this new logic
image

<AcquisitionRouter
formikRef={formikRef}
acquisitionFile={acquisitionFile}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,11 @@ import { Api_Person } from '@/models/api/Person';
import { Api_Product, Api_Project } from '@/models/api/Project';
import { Api_ExportProjectFilter } from '@/models/api/ProjectFilter';
import { UserOverrideCode } from '@/models/api/UserOverrideCode';
import { useAxiosErrorHandler, useAxiosSuccessHandler } from '@/utils';
import {
useAxiosErrorHandler,
useAxiosErrorHandlerWithAuthorization,
useAxiosSuccessHandler,
} from '@/utils';

const ignoreErrorCodes = [409];

Expand Down Expand Up @@ -72,7 +76,7 @@ export const useAcquisitionProvider = () => {
[getAcquisitionFile],
),
requestName: 'RetrieveAcquisitionFile',
onError: useAxiosErrorHandler('Failed to load Acquisition File'),
onError: useAxiosErrorHandlerWithAuthorization('Failed to load Acquisition File'),
});

const getLastUpdatedBy = useApiRequestWrapper<
Expand Down
23 changes: 23 additions & 0 deletions source/frontend/src/utils/axiosUtils.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import axios, { AxiosError } from 'axios';
import { useCallback } from 'react';
import { useHistory } from 'react-router-dom';
import { toast } from 'react-toastify';

import { IApiError } from '@/interfaces/IApiError';
Expand Down Expand Up @@ -34,6 +35,28 @@
);
}

/**
* Provides default boilerplate applicable to handling most common axios request errors.
* @param axiosError The request error object
*/
export function useAxiosErrorHandlerWithAuthorization(
message = 'Network error. Check responses and try again.',

Check warning on line 43 in source/frontend/src/utils/axiosUtils.ts

View check run for this annotation

Codecov / codecov/patch

source/frontend/src/utils/axiosUtils.ts#L43

Added line #L43 was not covered by tests
) {
const history = useHistory();
return useCallback(
(axiosError: AxiosError<IApiError>) => {
if (axiosError?.response?.status === 400) {
toast.error(axiosError?.response.data.error, { autoClose: 10000 });
} else if (axiosError?.response?.status === 403) {
history.push('/forbidden');
} else {
toast.error(message);

Check warning on line 53 in source/frontend/src/utils/axiosUtils.ts

View check run for this annotation

Codecov / codecov/patch

source/frontend/src/utils/axiosUtils.ts#L50-L53

Added lines #L50 - L53 were not covered by tests
}
},
[history, message],
);
}

export function useAxiosErrorHandlerWithConfirmation(
needsUserAction: (userOverrideCode: UserOverrideCode | null, message: string | null) => void,
message = 'Network error. Check responses and try again.',
Expand Down
Loading