From 7a588eb3f58b6a665955fd411a0d4dd955cb38a0 Mon Sep 17 00:00:00 2001 From: adamwhitingnhs Date: Wed, 11 Feb 2026 15:50:17 +0000 Subject: [PATCH] [PRMP-1440] Fix doc upload status interval --- ...viewDetailsDocumentUploadingStage.test.tsx | 118 +++------------ .../ReviewDetailsDocumentUploadingStage.tsx | 141 ++++-------------- app/src/helpers/utils/documentUpload.test.ts | 62 ++------ app/src/helpers/utils/documentUpload.ts | 21 ++- .../documentUploadPage/DocumentUploadPage.tsx | 10 +- 5 files changed, 73 insertions(+), 279 deletions(-) diff --git a/app/src/components/blocks/_admin/reviewDetailsDocumentUploadingStage/ReviewDetailsDocumentUploadingStage.test.tsx b/app/src/components/blocks/_admin/reviewDetailsDocumentUploadingStage/ReviewDetailsDocumentUploadingStage.test.tsx index 65719e502..625649c64 100644 --- a/app/src/components/blocks/_admin/reviewDetailsDocumentUploadingStage/ReviewDetailsDocumentUploadingStage.test.tsx +++ b/app/src/components/blocks/_admin/reviewDetailsDocumentUploadingStage/ReviewDetailsDocumentUploadingStage.test.tsx @@ -11,8 +11,13 @@ import { import { DOCUMENT_TYPE } from '../../../../helpers/utils/documentType'; import { ReviewDetails } from '../../../../types/generic/reviews'; import { routes, routeChildren } from '../../../../types/generic/routes'; -import { buildLgFile, buildPatientDetails } from '../../../../helpers/test/testBuilders'; +import { + buildLgFile, + buildPatientDetails, + buildUploadSession, +} from '../../../../helpers/test/testBuilders'; import * as uploadDocumentsModule from '../../../../helpers/requests/uploadDocuments'; +import * as documentUploadModule from '../../../../helpers/utils/documentUpload'; import * as mergePdfsModule from '../../../../helpers/utils/mergePdfs'; import { JSX } from 'react'; @@ -42,7 +47,8 @@ vi.mock('../../../../helpers/hooks/useBaseAPIHeaders', () => ({ default: (): typeof mockUseBaseAPIHeaders => mockUseBaseAPIHeaders, })); -vi.mock('../../../../helpers/utils/urlManipulations', () => ({ +vi.mock('../../../../helpers/utils/urlManipulations', async () => ({ + ...(await vi.importActual('../../../../helpers/utils/urlManipulations')), useEnhancedNavigate: (): any => { const fn = mockNavigate; (fn as any).withParams = mockNavigate; @@ -264,7 +270,7 @@ describe('ReviewDetailsDocumentUploadingStage', (): void => { const error = { response: { status: 500 }, }; - vi.spyOn(uploadDocumentsModule, 'default').mockRejectedValueOnce(error); + vi.spyOn(documentUploadModule, 'getUploadSession').mockRejectedValueOnce(error); const stitchedReviewData = new ReviewDetails( 'test-review-id', @@ -365,6 +371,11 @@ describe('ReviewDetailsDocumentUploadingStage', (): void => { it('sets up interval timer for document status polling', async (): Promise => { const setIntervalSpy = vi.spyOn(window, 'setInterval'); + vi.spyOn(documentUploadModule, 'getUploadSession').mockResolvedValueOnce( + buildUploadSession(mockDocuments), + ); + vi.spyOn(uploadDocumentsModule, 'uploadDocumentToS3').mockResolvedValue(); + const stitchedReviewData = new ReviewDetails( 'test-review-id', '16521000000101' as DOCUMENT_TYPE, @@ -437,109 +448,14 @@ describe('ReviewDetailsDocumentUploadingStage', (): void => { }); }); - describe('Session handling', (): void => { - it('navigates to SESSION_EXPIRED on 403 error during upload', async (): Promise => { - const error = { - response: { status: 403 }, - }; - vi.spyOn(uploadDocumentsModule, 'default').mockRejectedValueOnce(error); - - const stitchedReviewData = new ReviewDetails( - 'test-review-id', - '16521000000101' as DOCUMENT_TYPE, - '2023-10-01T12:00:00Z', - 'Test Uploader', - '2023-10-01T12:00:00Z', - 'Test Reason', - '1', - '1234567890', - ); - - const documentsWithExisting = [ - { - ...mockDocuments[0], - type: UploadDocumentType.EXISTING, - versionId: 'v1', - }, - ]; - - renderComponent(documentsWithExisting, stitchedReviewData); - - await waitFor( - (): void => { - expect(screen.queryByText('Preparing documents')).not.toBeInTheDocument(); - }, - { timeout: 2000 }, - ); - - await waitFor((): void => { - expect(screen.getByTestId('start-upload-button')).toBeInTheDocument(); - }); - - const startButton = screen.getByTestId('start-upload-button'); - await act(async () => { - startButton.click(); - }); - - await waitFor((): void => { - expect(mockNavigate).toHaveBeenCalledWith(routes.SESSION_EXPIRED); - }); - }); - - it('navigates to SESSION_EXPIRED on 403 error during S3 upload', async (): Promise => { - const error = { - response: { status: 403 }, - }; - vi.spyOn(uploadDocumentsModule, 'uploadDocumentToS3').mockRejectedValueOnce(error); - - const stitchedReviewData = new ReviewDetails( - 'test-review-id', - '16521000000101' as DOCUMENT_TYPE, - '2023-10-01T12:00:00Z', - 'Test Uploader', - '2023-10-01T12:00:00Z', - 'Test Reason', - '1', - '1234567890', - ); - - const documentsWithExisting = [ - { - ...mockDocuments[0], - type: UploadDocumentType.EXISTING, - versionId: 'v1', - }, - ]; - - renderComponent(documentsWithExisting, stitchedReviewData); - - await waitFor( - (): void => { - expect(screen.queryByText('Preparing documents')).not.toBeInTheDocument(); - }, - { timeout: 2000 }, - ); - - await waitFor((): void => { - expect(screen.getByTestId('start-upload-button')).toBeInTheDocument(); - }); - - const startButton = screen.getByTestId('start-upload-button'); - await act(async () => { - startButton.click(); - }); - - await waitFor((): void => { - expect(mockNavigate).toHaveBeenCalledWith(routes.SESSION_EXPIRED); - }); - }); - }); - describe('S3 Upload error handling', (): void => { it('marks document as failed and navigates to SERVER_ERROR on S3 upload failure', async (): Promise => { const error = { response: { status: 500 }, }; + vi.spyOn(documentUploadModule, 'getUploadSession').mockResolvedValueOnce( + buildUploadSession(mockDocuments), + ); vi.spyOn(uploadDocumentsModule, 'uploadDocumentToS3').mockRejectedValueOnce(error); const stitchedReviewData = new ReviewDetails( diff --git a/app/src/components/blocks/_admin/reviewDetailsDocumentUploadingStage/ReviewDetailsDocumentUploadingStage.tsx b/app/src/components/blocks/_admin/reviewDetailsDocumentUploadingStage/ReviewDetailsDocumentUploadingStage.tsx index 60bbbb841..410668107 100644 --- a/app/src/components/blocks/_admin/reviewDetailsDocumentUploadingStage/ReviewDetailsDocumentUploadingStage.tsx +++ b/app/src/components/blocks/_admin/reviewDetailsDocumentUploadingStage/ReviewDetailsDocumentUploadingStage.tsx @@ -8,9 +8,8 @@ import { import useBaseAPIHeaders from '../../../../helpers/hooks/useBaseAPIHeaders'; import useBaseAPIUrl from '../../../../helpers/hooks/useBaseAPIUrl'; import usePatient from '../../../../helpers/hooks/usePatient'; -import uploadDocuments, { +import { generateStitchedFileName, - getDocumentStatus, uploadDocumentToS3, } from '../../../../helpers/requests/uploadDocuments'; import { DOCUMENT_TYPE, getConfigForDocType } from '../../../../helpers/utils/documentType'; @@ -24,9 +23,8 @@ import { import { useEnhancedNavigate } from '../../../../helpers/utils/urlManipulations'; import { ReviewDetails } from '../../../../types/generic/reviews'; import { routeChildren, routes } from '../../../../types/generic/routes'; -import { DocumentStatusResult, UploadSession } from '../../../../types/generic/uploadResult'; +import { UploadSession } from '../../../../types/generic/uploadResult'; import { - DOCUMENT_STATUS, DOCUMENT_UPLOAD_STATE, ReviewUploadDocument, UploadDocument, @@ -34,6 +32,7 @@ import { } from '../../../../types/pages/UploadDocumentsPage/types'; import Spinner from '../../../generic/spinner/Spinner'; import DocumentUploadingStage from '../../_documentUpload/documentUploadingStage/DocumentUploadingStage'; +import { getUploadSession, startIntervalTimer } from '../../../../helpers/utils/documentUpload'; type Props = { reviewData: ReviewDetails | null; @@ -55,7 +54,7 @@ const ReviewDetailsDocumentUploadingStage = ({ const navigate = useEnhancedNavigate(); const completeRef = useRef(false); const virusReference = useRef(false); - const interval = useRef(0); + const [interval, setInterval] = useState(0); const intervalTimerRef = useRef(null); const documentsRef = useRef(documents); @@ -145,7 +144,7 @@ const ReviewDetailsDocumentUploadingStage = ({ }, [documents]); useEffect(() => { - if (interval.current * UPDATE_DOCUMENT_STATE_FREQUENCY_MILLISECONDS > MAX_POLLING_TIME) { + if (interval * UPDATE_DOCUMENT_STATE_FREQUENCY_MILLISECONDS > MAX_POLLING_TIME) { clearIntervalTimer(); navigate(routes.SERVER_ERROR); return; @@ -174,7 +173,7 @@ const ReviewDetailsDocumentUploadingStage = ({ ), ); } - }, [baseHeaders, baseUrl, documents, navigate, nhsNumber, setDocuments]); + }, [baseHeaders, baseUrl, documents, navigate, nhsNumber, setDocuments, interval]); useEffect(() => { return (): void => { @@ -225,15 +224,15 @@ const ReviewDetailsDocumentUploadingStage = ({ const startUpload = async (): Promise => { try { - const uploadSession: UploadSession = isLocal - ? getMockUploadSession(documents) - : await uploadDocuments({ - nhsNumber, - documents: documents, - baseUrl, - baseHeaders, - documentReferenceId: existingId, - }); + const uploadSession: UploadSession = await getUploadSession( + patientDetails!, + baseUrl, + baseHeaders, + existingId, + documents, + setDocuments, + ); + const uploadingDocuments = markDocumentsAsUploading(documents, uploadSession); setDocuments(uploadingDocuments); setIsLoading(false); @@ -242,7 +241,16 @@ const ReviewDetailsDocumentUploadingStage = ({ uploadAllDocuments(uploadingDocuments, uploadSession); } - const updateStateInterval = startIntervalTimer(uploadingDocuments); + const updateStateInterval = startIntervalTimer( + uploadingDocuments, + setInterval, + documents, + setDocuments, + patientDetails!, + baseUrl, + baseHeaders, + UPDATE_DOCUMENT_STATE_FREQUENCY_MILLISECONDS, + ); intervalTimerRef.current = updateStateInterval; } catch (e) { setIsLoading(false); @@ -269,105 +277,6 @@ const ReviewDetailsDocumentUploadingStage = ({ } }; - const startIntervalTimer = (uploadDocuments: Array): number => { - const startIntervalTimerIsLocal = (): void => { - const updatedDocuments = uploadDocuments.map((doc) => { - const min = (doc.progress ?? 0) + 40; - const max = 70; - doc.progress = Math.random() * (min + max - (min + 1)) + min; - doc.progress = doc.progress > 100 ? 100 : doc.progress; - if (doc.progress < 100) { - doc.state = DOCUMENT_UPLOAD_STATE.UPLOADING; - } else if (doc.state !== DOCUMENT_UPLOAD_STATE.SCANNING) { - doc.state = DOCUMENT_UPLOAD_STATE.SCANNING; - } else { - const hasVirusFile = documents.filter( - (d) => d.file.name.toLocaleLowerCase() === 'virus.pdf', - ); - const hasFailedFile = documents.filter( - (d) => d.file.name.toLocaleLowerCase() === 'virus-failed.pdf', - ); - if (hasVirusFile.length > 0) { - doc.state = DOCUMENT_UPLOAD_STATE.INFECTED; - } else if (hasFailedFile.length > 0) { - doc.state = DOCUMENT_UPLOAD_STATE.FAILED; - } else { - doc.state = DOCUMENT_UPLOAD_STATE.SUCCEEDED; - } - } - - return doc; - }); - setDocuments(updatedDocuments); - }; - - return window.setInterval(() => { - interval.current = interval.current + 1; - if (isLocal) { - startIntervalTimerIsLocal(); - } else { - getDocumentStatus({ - documents: documentsRef.current, - baseUrl, - baseHeaders, - nhsNumber, - }) - .then((documentStatusResult) => { - handleDocStatusResult(documentStatusResult); - }) - .catch((e) => { - const error = e as AxiosError; - if (error.response?.status === 403) { - navigate(routes.SESSION_EXPIRED); - return; - } - navigate(routes.SERVER_ERROR + errorToParams(error)); - }); - } - }, UPDATE_DOCUMENT_STATE_FREQUENCY_MILLISECONDS); - }; - - const handleDocStatusResult = (documentStatusResult: DocumentStatusResult): void => { - setDocuments((previousState) => - previousState.map((doc) => { - const docStatus = documentStatusResult[doc.ref!]; - - const updatedDoc = { - ...doc, - }; - - switch (docStatus?.status) { - case DOCUMENT_STATUS.FINAL: - updatedDoc.state = DOCUMENT_UPLOAD_STATE.SUCCEEDED; - break; - - case DOCUMENT_STATUS.INFECTED: - updatedDoc.state = DOCUMENT_UPLOAD_STATE.INFECTED; - break; - - case DOCUMENT_STATUS.NOT_FOUND: - case DOCUMENT_STATUS.CANCELLED: - updatedDoc.state = DOCUMENT_UPLOAD_STATE.ERROR; - updatedDoc.errorCode = docStatus.error_code; - break; - } - - return updatedDoc; - }), - ); - }; - - const getMockUploadSession = (documents: ReviewUploadDocument[]): UploadSession => { - const session: UploadSession = {}; - documents.forEach((doc) => { - session[doc.id] = { - url: 'https://example.com/', - } as any; - }); - - return session; - }; - if (!hasNormalisedOnEntry) { return ; } diff --git a/app/src/helpers/utils/documentUpload.test.ts b/app/src/helpers/utils/documentUpload.test.ts index 47b76db59..8fc6a32f2 100644 --- a/app/src/helpers/utils/documentUpload.test.ts +++ b/app/src/helpers/utils/documentUpload.test.ts @@ -199,7 +199,7 @@ describe('documentUpload', () => { mockPatientDetails, baseUrl, baseHeaders, - [], + undefined, mockDocuments, mockSetDocuments, ); @@ -226,7 +226,7 @@ describe('documentUpload', () => { patientWithPermission, baseUrl, baseHeaders, - existingDocs, + existingDocs[0].id, mockDocuments, mockSetDocuments, ); @@ -268,7 +268,7 @@ describe('documentUpload', () => { patientWithoutPermission, baseUrl, baseHeaders, - [], + undefined, mockDocuments, mockSetDocuments, ); @@ -305,7 +305,7 @@ describe('documentUpload', () => { patientWithoutPermission, baseUrl, baseHeaders, - [], + undefined, [mockDocuments[0]], mockSetDocuments, ); @@ -655,14 +655,14 @@ describe('documentUpload', () => { }, ]; - const mockInterval = { current: 0 }; + const setInterval = vi.fn(); + const mockSetDocuments = vi.fn(); const baseUrl = 'https://api.example.com'; const baseHeaders = { Authorization: 'Bearer token', 'Content-Type': 'application/json' }; const nhsNumber = '1234567890'; beforeEach(() => { - mockInterval.current = 0; vi.useFakeTimers(); vi.clearAllMocks(); }); @@ -682,13 +682,12 @@ describe('documentUpload', () => { startIntervalTimer( mockUploadDocuments, - mockInterval as any, + setInterval, mockUploadDocuments, mockSetDocuments, patientWithPermission, baseUrl, baseHeaders, - nhsNumber, 1000, ); @@ -701,7 +700,6 @@ describe('documentUpload', () => { nhsNumber, }); expect(mockSetDocuments).toHaveBeenCalled(); - expect(mockInterval.current).toBe(1); }); it('should call getDocumentReviewStatus when user cannot manage record', async () => { @@ -716,13 +714,12 @@ describe('documentUpload', () => { startIntervalTimer( mockUploadDocuments, - mockInterval as any, + setInterval, mockUploadDocuments, mockSetDocuments, patientWithoutPermission, baseUrl, baseHeaders, - nhsNumber, 1000, ); @@ -737,28 +734,6 @@ describe('documentUpload', () => { expect(mockSetDocuments).toHaveBeenCalled(); }); - it('should increment interval counter on each tick', async () => { - startIntervalTimer( - mockUploadDocuments, - mockInterval as any, - mockUploadDocuments, - mockSetDocuments, - { ...mockPatientDetails, canManageRecord: true }, - baseUrl, - baseHeaders, - nhsNumber, - 1000, - ); - - expect(mockInterval.current).toBe(0); - - await vi.advanceTimersByTimeAsync(1000); - expect(mockInterval.current).toBe(1); - - await vi.advanceTimersByTimeAsync(1000); - expect(mockInterval.current).toBe(2); - }); - it('should update documents locally when isLocal is true', async () => { vi.spyOn(isLocal, 'isLocal', 'get').mockReturnValue(true); @@ -772,13 +747,12 @@ describe('documentUpload', () => { startIntervalTimer( localDocs, - mockInterval as any, + setInterval, localDocs, mockSetDocuments, mockPatientDetails, baseUrl, baseHeaders, - nhsNumber, 1000, ); @@ -803,13 +777,12 @@ describe('documentUpload', () => { startIntervalTimer( localDocs, - mockInterval as any, + setInterval, localDocs, mockSetDocuments, mockPatientDetails, baseUrl, baseHeaders, - nhsNumber, 1000, ); @@ -832,13 +805,12 @@ describe('documentUpload', () => { startIntervalTimer( [virusDoc], - mockInterval as any, + setInterval, [virusDoc], mockSetDocuments, mockPatientDetails, baseUrl, baseHeaders, - nhsNumber, 1000, ); @@ -860,13 +832,12 @@ describe('documentUpload', () => { startIntervalTimer( [failedDoc], - mockInterval as any, + setInterval, [failedDoc], mockSetDocuments, mockPatientDetails, baseUrl, baseHeaders, - nhsNumber, 1000, ); @@ -889,13 +860,12 @@ describe('documentUpload', () => { startIntervalTimer( localDocs, - mockInterval as any, + setInterval, localDocs, mockSetDocuments, mockPatientDetails, baseUrl, baseHeaders, - nhsNumber, 1000, ); @@ -1071,7 +1041,7 @@ describe('documentUpload', () => { withParams: vi.fn(), }); - let mockInterval: { current: number }; + let mockInterval = 0; let mockVirusRef: { current: boolean }; let mockCompleteRef: { current: boolean }; @@ -1080,7 +1050,7 @@ describe('documentUpload', () => { vi.spyOn(globalThis, 'clearInterval'); vi.spyOn(window, 'clearInterval'); vi.spyOn(urlManipulations, 'getJourney').mockReturnValue('new'); - mockInterval = { current: 1 }; + mockInterval = 1; mockVirusRef = { current: false }; mockCompleteRef = { current: false }; }); @@ -1105,7 +1075,7 @@ describe('documentUpload', () => { }); it('should navigate to SERVER_ERROR when polling time exceeds MAX_POLLING_TIME', () => { - mockInterval.current = + mockInterval = Math.ceil(MAX_POLLING_TIME / UPDATE_DOCUMENT_STATE_FREQUENCY_MILLISECONDS) + 1; const intervalTimer = 456; diff --git a/app/src/helpers/utils/documentUpload.ts b/app/src/helpers/utils/documentUpload.ts index cd5f181fb..5e6eebffc 100644 --- a/app/src/helpers/utils/documentUpload.ts +++ b/app/src/helpers/utils/documentUpload.ts @@ -83,7 +83,7 @@ export const getUploadSession = async ( patientDetails: PatientDetails, baseUrl: string, baseHeaders: AuthHeaders, - existingDocuments: UploadDocument[], + existingDocumentId: string | undefined, documents: UploadDocument[], setDocuments: Dispatch>, ): Promise => { @@ -97,7 +97,7 @@ export const getUploadSession = async ( documents: documents, baseUrl, baseHeaders, - documentReferenceId: existingDocuments[0]?.id, + documentReferenceId: existingDocumentId, }); } @@ -222,17 +222,16 @@ export const handleDocReviewStatusResult = ( export const startIntervalTimer = ( uploadDocuments: Array, - interval: RefObject, + setInterval: Dispatch>, documents: Array, setDocuments: Dispatch>, - patientDetails: PatientDetails | null, + patientDetails: PatientDetails, baseUrl: string, baseHeaders: AuthHeaders, - nhsNumber: string, timeout: number, ): number => { return window.setInterval(async () => { - interval.current = interval.current + 1; + setInterval((prev) => prev + 1); try { if (isLocal) { const updatedDocuments = uploadDocuments.map((doc) => { @@ -260,12 +259,12 @@ export const startIntervalTimer = ( return doc; }); setDocuments(updatedDocuments); - } else if (patientDetails?.canManageRecord) { + } else if (patientDetails.canManageRecord) { const documentStatusResult = await getDocumentStatus({ documents: uploadDocuments, baseUrl, baseHeaders, - nhsNumber, + nhsNumber: patientDetails.nhsNumber, }); handleDocStatusResult(documentStatusResult, setDocuments); @@ -275,7 +274,7 @@ export const startIntervalTimer = ( document, baseUrl, baseHeaders, - nhsNumber, + nhsNumber: patientDetails.nhsNumber, }).then((result) => handleDocReviewStatusResult(result, setDocuments)); }); } @@ -321,7 +320,7 @@ export const handleDocumentStatusUpdates = ( journey: JourneyType, navigate: EnhancedNavigate, intervalTimer: number, - interval: RefObject, + interval: number, documents: UploadDocument[], virusReference: RefObject, completeRef: RefObject, @@ -334,7 +333,7 @@ export const handleDocumentStatusUpdates = ( return; } - if (interval.current * UPDATE_DOCUMENT_STATE_FREQUENCY_MILLISECONDS > MAX_POLLING_TIME) { + if (interval * UPDATE_DOCUMENT_STATE_FREQUENCY_MILLISECONDS > MAX_POLLING_TIME) { globalThis.clearInterval(intervalTimer); navigate(routes.SERVER_ERROR); return; diff --git a/app/src/pages/documentUploadPage/DocumentUploadPage.tsx b/app/src/pages/documentUploadPage/DocumentUploadPage.tsx index fd61ada2f..1f618fa35 100644 --- a/app/src/pages/documentUploadPage/DocumentUploadPage.tsx +++ b/app/src/pages/documentUploadPage/DocumentUploadPage.tsx @@ -67,7 +67,7 @@ const DocumentUploadPage = (): React.JSX.Element => { const [mergedPdfBlob, setMergedPdfBlob] = useState(); const [journey, setJourney] = useState(getJourney()); const config = useConfig(); - const interval = useRef(0); + const [interval, setInterval] = useState(0); const filesErrorPageRef = useRef(false); const [documentType, setDocumentType] = useState(DOCUMENT_TYPE.LLOYD_GEORGE); const [documentConfig, setDocumentConfig] = useState( @@ -108,6 +108,7 @@ const DocumentUploadPage = (): React.JSX.Element => { setDocuments, uploadSession, intervalTimer, + interval, ]); useEffect(() => { @@ -217,7 +218,7 @@ const DocumentUploadPage = (): React.JSX.Element => { patientDetails!, baseUrl, baseHeaders, - existingDocuments, + existingDocuments[0]?.id, documents, setDocuments, ); @@ -232,13 +233,12 @@ const DocumentUploadPage = (): React.JSX.Element => { const updateStateInterval = startIntervalTimer( uploadingDocuments.filter((d) => d.state !== DOCUMENT_UPLOAD_STATE.ERROR), - interval, + setInterval, documents, setDocuments, - patientDetails, + patientDetails!, baseUrl, baseHeaders, - nhsNumber, UPDATE_DOCUMENT_STATE_FREQUENCY_MILLISECONDS, ); setIntervalTimer(updateStateInterval);