diff --git a/app/src/components/blocks/_documentUpload/documentUploadCompleteStage/DocumentUploadCompleteStage.test.tsx b/app/src/components/blocks/_documentUpload/documentUploadCompleteStage/DocumentUploadCompleteStage.test.tsx index 708e94bc4e..c66bf706ff 100644 --- a/app/src/components/blocks/_documentUpload/documentUploadCompleteStage/DocumentUploadCompleteStage.test.tsx +++ b/app/src/components/blocks/_documentUpload/documentUploadCompleteStage/DocumentUploadCompleteStage.test.tsx @@ -73,12 +73,9 @@ describe('DocumentUploadCompleteStage', () => { expect(screen.getByTestId('dob').textContent).toEqual('Date of birth: ' + expectedDob); expect( - screen.queryByText( - 'You are not the data controller', - { - exact: false, - } - ), + screen.queryByText('You are not the data controller', { + exact: false, + }), ).not.toBeInTheDocument(); }); @@ -123,9 +120,29 @@ describe('DocumentUploadCompleteStage', () => { }); }); + it('should navigate to patient documents if partial upload complete', async () => { + documents.push({ + docType: DOCUMENT_TYPE.LLOYD_GEORGE, + id: '2', + file: buildLgFile(2), + attempts: 0, + state: DOCUMENT_UPLOAD_STATE.ERROR, + numPages: 3, + position: 2, + }); + + renderApp(documents); + + await userEvent.click(screen.getByTestId('patient-docs-btn')); + + await waitFor(async () => { + expect(mockNavigate).toHaveBeenCalledWith(routes.PATIENT_DOCUMENTS, { replace: true }); + }); + }); + it.each([ { docState: DOCUMENT_UPLOAD_STATE.SUCCEEDED, expectedTitle: 'Upload complete' }, - { docState: DOCUMENT_UPLOAD_STATE.FAILED, expectedTitle: 'Upload partially complete' }, + { docState: DOCUMENT_UPLOAD_STATE.ERROR, expectedTitle: 'Upload partially complete' }, ])('should set the page title based on upload success', async ({ docState, expectedTitle }) => { documents = [ { @@ -150,7 +167,7 @@ describe('DocumentUploadCompleteStage', () => { id: '2', file: buildLgFile(2), attempts: 0, - state: DOCUMENT_UPLOAD_STATE.FAILED, + state: DOCUMENT_UPLOAD_STATE.ERROR, numPages: 3, position: 2, }); @@ -172,16 +189,13 @@ describe('DocumentUploadCompleteStage', () => { renderApp(documents); expect( - screen.getByText( - 'You are not the data controller', - { - exact: false, - } - ), + screen.getByText('You are not the data controller', { + exact: false, + }), ).toBeInTheDocument(); }); - const renderApp = (documents: UploadDocument[]) => { + const renderApp = (documents: UploadDocument[]): void => { render( , diff --git a/app/src/components/blocks/_documentUpload/documentUploadCompleteStage/DocumentUploadCompleteStage.tsx b/app/src/components/blocks/_documentUpload/documentUploadCompleteStage/DocumentUploadCompleteStage.tsx index d44d78a0ef..660ce7559b 100644 --- a/app/src/components/blocks/_documentUpload/documentUploadCompleteStage/DocumentUploadCompleteStage.tsx +++ b/app/src/components/blocks/_documentUpload/documentUploadCompleteStage/DocumentUploadCompleteStage.tsx @@ -30,16 +30,13 @@ const DocumentUploadCompleteStage = ({ documents, documentConfig }: Props): Reac const journey = getJourney(); const [showFiles, setShowFiles] = useState(false); - const failedDocuments = documents.filter((doc) => doc.state === DOCUMENT_UPLOAD_STATE.FAILED); + const failedDocuments = documents.filter((doc) => doc.state === DOCUMENT_UPLOAD_STATE.ERROR); const pageTitle = failedDocuments.length > 0 ? 'Upload partially complete' : 'Upload complete'; useTitle({ pageTitle }); - const docsAreInFinishedState = () => - allDocsHaveState(documents, [ - DOCUMENT_UPLOAD_STATE.SUCCEEDED, - DOCUMENT_UPLOAD_STATE.FAILED, - ]); + const docsAreInFinishedState = (): boolean => + allDocsHaveState(documents, [DOCUMENT_UPLOAD_STATE.SUCCEEDED, DOCUMENT_UPLOAD_STATE.ERROR]); useEffect(() => { if (!docsAreInFinishedState() || patientDetails === null) { @@ -66,13 +63,12 @@ const DocumentUploadCompleteStage = ({ documents, documentConfig }: Props): Reac Date of birth: {dob} - - {failedDocuments.length > 0 && ( + {failedDocuments.length > 0 ? (

Some of your files failed to upload

)} - - )} - -

What happens next

- - {journey === 'update' && patientDetails.canManageRecord && ( -

- You can now view the updated {documentConfig.displayName} for this patient in - this service by{' '} - { - e.preventDefault(); - navigate(routes.SEARCH_PATIENT, { replace: true }); +
+

What you need to do

+

+ You must note which files uploaded successfully, then return to the + patient's record to upload any files that failed. +

+ + + ) : ( + <> +

What happens next

+ + {journey === 'update' && patientDetails.canManageRecord && ( +

+ You can now view the updated {documentConfig.displayName} for this + patient in this service by{' '} + { + e.preventDefault(); + navigate(routes.SEARCH_PATIENT, { replace: true }); + }} + data-testid="search-patient-link" + > + searching using their NHS number + + {'.'} +

+ )} - {patientDetails.canManageRecord === false && ( -

- You are not the data controller for this patient so you cannot view the files - you have uploaded in this service. -

- )} + {patientDetails.canManageRecord === false && ( +

+ You are not the data controller for this patient so you cannot view the + files you have uploaded in this service. +

+ )} -

- If you think you've made a mistake, contact the Patient Record Management team at{' '} - england.prmteam@nhs.net. -

+

+ If you think you've made a mistake, contact the Patient Record Management + team at{' '} + + england.prmteam@nhs.net + + . +

+ + {documentConfig.content.uploadFilesExtraParagraph && ( +

{documentConfig.content.uploadFilesExtraParagraph}

+ )} - {documentConfig.content.uploadFilesExtraParagraph && ( -

{documentConfig.content.uploadFilesExtraParagraph}

+

+ For information on destroying your paper records and removing the digital + files from your system, read the article{' '} + + Digitisation of Lloyd George records + + {'.'} +

+ + )} - -

- For information on destroying your paper records and removing the digital files from - your system, read the article{' '} - - Digitisation of Lloyd George records - - {'.'} -

- - ); }; diff --git a/app/src/helpers/utils/documentUpload.test.ts b/app/src/helpers/utils/documentUpload.test.ts index 9cb8730420..fb4e11fdc5 100644 --- a/app/src/helpers/utils/documentUpload.test.ts +++ b/app/src/helpers/utils/documentUpload.test.ts @@ -814,7 +814,7 @@ describe('documentUpload', () => { expect(updatedDocs[0].state).toBe(DOCUMENT_UPLOAD_STATE.INFECTED); }); - it('should set state to FAILED when virus-failed.pdf file is detected in local mode', async () => { + it('should set state to ERROR when virus-failed.pdf file is detected in local mode', async () => { vi.spyOn(isLocal, 'isLocal', 'get').mockReturnValue(true); const failedDoc = { @@ -839,7 +839,7 @@ describe('documentUpload', () => { await vi.advanceTimersByTimeAsync(1000); const updatedDocs = mockSetDocuments.mock.calls[0][0]; - expect(updatedDocs[0].state).toBe(DOCUMENT_UPLOAD_STATE.FAILED); + expect(updatedDocs[0].state).toBe(DOCUMENT_UPLOAD_STATE.ERROR); }); it('should not change state when already SCANNING in local mode', async () => { diff --git a/app/src/helpers/utils/documentUpload.ts b/app/src/helpers/utils/documentUpload.ts index 10c9042e60..a61ce070e4 100644 --- a/app/src/helpers/utils/documentUpload.ts +++ b/app/src/helpers/utils/documentUpload.ts @@ -225,53 +225,52 @@ export const startIntervalTimer = ( ): number => { return window.setInterval(async () => { interval.current = interval.current + 1; - if (isLocal) { - 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 = Math.min(doc.progress, 100); - if (doc.progress < 100) { - doc.state = DOCUMENT_UPLOAD_STATE.UPLOADING; - } else if (doc.state !== DOCUMENT_UPLOAD_STATE.SCANNING) { - 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; + try { + if (isLocal) { + 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 = Math.min(doc.progress, 100); + if (doc.progress < 100) { + doc.state = DOCUMENT_UPLOAD_STATE.UPLOADING; + } else if (doc.state !== DOCUMENT_UPLOAD_STATE.SCANNING) { + const hasVirusFile = documents.filter( + (d) => d.file.name.toLocaleLowerCase() === 'virus.pdf', + ); + + if (hasVirusFile.length > 0) { + doc.state = DOCUMENT_UPLOAD_STATE.INFECTED; + } else if (doc.file.name.toLocaleLowerCase() === 'virus-failed.pdf') { + doc.state = DOCUMENT_UPLOAD_STATE.ERROR; + } else { + doc.state = DOCUMENT_UPLOAD_STATE.SUCCEEDED; + } } - } - return doc; - }); - setDocuments(updatedDocuments); - } else if (patientDetails?.canManageRecord) { - const documentStatusResult = await getDocumentStatus({ - documents: uploadDocuments, - baseUrl, - baseHeaders, - nhsNumber, - }); - - handleDocStatusResult(documentStatusResult, setDocuments); - } else { - uploadDocuments.forEach(async (document) => { - void getDocumentReviewStatus({ - document, + return doc; + }); + setDocuments(updatedDocuments); + } else if (patientDetails?.canManageRecord) { + const documentStatusResult = await getDocumentStatus({ + documents: uploadDocuments, baseUrl, baseHeaders, nhsNumber, - }).then((result) => handleDocReviewStatusResult(result, setDocuments)); - }); - } + }); + + handleDocStatusResult(documentStatusResult, setDocuments); + } else { + uploadDocuments.forEach(async (document) => { + void getDocumentReviewStatus({ + document, + baseUrl, + baseHeaders, + nhsNumber, + }).then((result) => handleDocReviewStatusResult(result, setDocuments)); + }); + } + } catch {} }, timeout); }; diff --git a/app/src/pages/documentUploadPage/DocumentUploadPage.tsx b/app/src/pages/documentUploadPage/DocumentUploadPage.tsx index 151e3def23..17208aceca 100644 --- a/app/src/pages/documentUploadPage/DocumentUploadPage.tsx +++ b/app/src/pages/documentUploadPage/DocumentUploadPage.tsx @@ -182,16 +182,12 @@ const DocumentUploadPage = (): React.JSX.Element => { uploadSession, setDocuments, }); - } catch (e) { - window.clearInterval(intervalTimer); - markDocumentAsFailed(document); - - const error = e as AxiosError; - navigate(routes.SERVER_ERROR + errorToParams(error)); + } catch { + markDocumentWithError(document); } }; - const markDocumentAsFailed = (document: UploadDocument): void => { + const markDocumentWithError = (document: UploadDocument): void => { setSingleDocument(setDocuments, { id: document.id, state: DOCUMENT_UPLOAD_STATE.ERROR, @@ -265,7 +261,7 @@ const DocumentUploadPage = (): React.JSX.Element => { } const updateStateInterval = startIntervalTimer( - uploadingDocuments, + uploadingDocuments.filter(d => d.state !== DOCUMENT_UPLOAD_STATE.ERROR), interval, documents, setDocuments, diff --git a/lambdas/services/upload_document_reference_service.py b/lambdas/services/upload_document_reference_service.py index 4dcd438b3f..685d2e1544 100644 --- a/lambdas/services/upload_document_reference_service.py +++ b/lambdas/services/upload_document_reference_service.py @@ -182,17 +182,14 @@ def _process_preliminary_document_reference( else: self._update_dynamo_table(preliminary_document_reference) - except TransactionConflictException as e: - logger.error( - f"Transaction conflict while processing document {preliminary_document_reference.id}: {str(e)}" - ) - raise except Exception as e: logger.error( f"Error processing document reference {preliminary_document_reference.id}: {str(e)}" ) raise + self.delete_file_from_staging_bucket(object_key) + def _finalize_and_supersede_with_transaction(self, new_document: DocumentReference): """ Atomically update the new document to 'final' status AND supersede existing final documents. @@ -314,10 +311,16 @@ def _finalize_and_supersede_with_transaction(self, new_document: DocumentReferen ) raise - except TransactionConflictException: - logger.info( - f"Cancelling preliminary document {new_document.id} due to transaction conflict" - ) + except Exception as e: + if isinstance(e, TransactionConflictException): + logger.error( + f"Cancelling preliminary document {new_document.id} due to transaction conflict" + ) + else: + logger.error( + f"Unexpected error while finalizing document for {new_document.nhs_number}: {e}" + ) + new_document.doc_status = "cancelled" new_document.uploaded = False new_document.uploading = False @@ -326,12 +329,6 @@ def _finalize_and_supersede_with_transaction(self, new_document: DocumentReferen self.delete_file_from_bucket( new_document.file_location, new_document.s3_version_id ) - raise - except Exception as e: - logger.error( - f"Unexpected error while finalizing document for {new_document.nhs_number}: {e}" - ) - raise def document_reference_key(self, document_id): return {DocumentReferenceMetadataFields.ID.value: document_id} @@ -357,7 +354,7 @@ def _process_clean_document( """Process a document that passed virus scanning""" try: self.copy_files_from_staging_bucket(document_reference, object_key) - self.delete_file_from_staging_bucket(object_key) + logger.info( f"Successfully processed clean document: {document_reference.id}" ) diff --git a/lambdas/tests/unit/services/test_pdm_upload_document_reference_service.py b/lambdas/tests/unit/services/test_pdm_upload_document_reference_service.py index 4978cd8cb7..c0b3d14383 100644 --- a/lambdas/tests/unit/services/test_pdm_upload_document_reference_service.py +++ b/lambdas/tests/unit/services/test_pdm_upload_document_reference_service.py @@ -244,6 +244,7 @@ def test__process_preliminary_document_reference_clean_virus_scan( mock_finalize_transaction = mocker.patch.object( pdm_service, "_finalize_and_supersede_with_transaction" ) + mock_delete = mocker.patch.object(pdm_service, "delete_file_from_staging_bucket") pdm_service._process_preliminary_document_reference( mock_pdm_document_reference, object_key, 1222 @@ -254,7 +255,7 @@ def test__process_preliminary_document_reference_clean_virus_scan( assert mock_pdm_document_reference.doc_status == "final" assert mock_pdm_document_reference.uploaded is True assert mock_pdm_document_reference.uploading is False - + mock_delete.assert_called_once() def test__process_preliminary_document_reference_infected_virus_scan( pdm_service, mock_document_reference, mocker @@ -265,6 +266,8 @@ def test__process_preliminary_document_reference_infected_virus_scan( mocker.patch.object( pdm_service, "_perform_virus_scan", return_value=VirusScanResult.INFECTED ) + mock_delete = mocker.patch.object(pdm_service, "delete_file_from_staging_bucket") + mock_process_clean = mocker.patch.object(pdm_service, "_process_clean_document") mock_update_dynamo = mocker.patch.object(pdm_service, "_update_dynamo_table") pdm_service._process_preliminary_document_reference( @@ -273,6 +276,7 @@ def test__process_preliminary_document_reference_infected_virus_scan( mock_process_clean.assert_not_called() mock_update_dynamo.assert_called_once() + mock_delete.assert_called_once() def test_perform_virus_scan_returns_clean_hardcoded( @@ -302,7 +306,6 @@ def test_process_clean_document_success(pdm_service, mock_document_reference, mo object_key = "staging/test-doc-id" mock_copy = mocker.patch.object(pdm_service, "copy_files_from_staging_bucket") - mock_delete = mocker.patch.object(pdm_service, "delete_file_from_staging_bucket") pdm_service._process_clean_document( mock_document_reference, @@ -310,7 +313,6 @@ def test_process_clean_document_success(pdm_service, mock_document_reference, mo ) mock_copy.assert_called_once_with(mock_document_reference, object_key) - mock_delete.assert_called_once_with(object_key) def test_process_clean_document_exception_restores_original_values( diff --git a/lambdas/tests/unit/services/test_upload_document_reference_service.py b/lambdas/tests/unit/services/test_upload_document_reference_service.py index f8c525b168..e030d461fd 100644 --- a/lambdas/tests/unit/services/test_upload_document_reference_service.py +++ b/lambdas/tests/unit/services/test_upload_document_reference_service.py @@ -11,7 +11,7 @@ FinalOrPreliminaryAndNotSuperseded, PreliminaryStatus, ) -from utils.exceptions import DocumentServiceException, FileProcessingException +from utils.exceptions import DocumentServiceException, FileProcessingException, TransactionConflictException from utils.lambda_exceptions import InvalidDocTypeException from lambdas.enums.snomed_codes import SnomedCodes @@ -185,6 +185,8 @@ def test__process_preliminary_document_reference_clean_virus_scan( mocker.patch.object( service, "_perform_virus_scan", return_value=VirusScanResult.CLEAN ) + mock_delete = mocker.patch.object(service, "delete_file_from_staging_bucket") + mock_process_clean = mocker.patch.object(service, "_process_clean_document") mock_finalize_transaction = mocker.patch.object( service, "_finalize_and_supersede_with_transaction" @@ -199,6 +201,7 @@ def test__process_preliminary_document_reference_clean_virus_scan( assert mock_document_reference.doc_status == "final" assert mock_document_reference.uploaded is True assert mock_document_reference.uploading is False + mock_delete.assert_called_once_with(object_key) def test__process_preliminary_document_reference_infected_virus_scan( @@ -210,6 +213,8 @@ def test__process_preliminary_document_reference_infected_virus_scan( mocker.patch.object( service, "_perform_virus_scan", return_value=VirusScanResult.INFECTED ) + mock_delete = mocker.patch.object(service, "delete_file_from_staging_bucket") + mock_process_clean = mocker.patch.object(service, "_process_clean_document") mock_update_dynamo = mocker.patch.object(service, "_update_dynamo_table") service._process_preliminary_document_reference( @@ -218,7 +223,7 @@ def test__process_preliminary_document_reference_infected_virus_scan( mock_process_clean.assert_not_called() mock_update_dynamo.assert_called_once() - + mock_delete.assert_called_once_with(object_key) def test_perform_virus_scan_returns_clean_hardcoded(service, mock_document_reference): """Test virus scan returns hardcoded CLEAN result""" @@ -245,7 +250,6 @@ def test_process_clean_document_success(service, mock_document_reference, mocker object_key = "staging/test-doc-id" mock_copy = mocker.patch.object(service, "copy_files_from_staging_bucket") - mock_delete = mocker.patch.object(service, "delete_file_from_staging_bucket") service._process_clean_document( mock_document_reference, @@ -253,7 +257,6 @@ def test_process_clean_document_success(service, mock_document_reference, mocker ) mock_copy.assert_called_once_with(mock_document_reference, object_key) - mock_delete.assert_called_once_with(object_key) def test_process_clean_document_exception_restores_original_values( @@ -598,8 +601,6 @@ def test_finalize_and_supersede_with_transaction_skips_same_id( def test_finalize_and_supersede_with_transaction_handles_transaction_cancelled( service, mock_document_reference ): - """Test handling of TransactionCanceledException (concurrent update detected)""" - from utils.exceptions import TransactionConflictException new_doc = mock_document_reference new_doc.id = "new-doc-id" @@ -613,16 +614,13 @@ def test_finalize_and_supersede_with_transaction_handles_transaction_cancelled( mock_build_update = Mock(return_value={"Update": "transaction"}) service.dynamo_service.build_update_transaction_item = mock_build_update - # Simulate TransactionCanceledException transaction_error = ClientError( error_response={"Error": {"Code": "TransactionCanceledException"}}, operation_name="TransactWriteItems", ) service.dynamo_service.transact_write_items.side_effect = transaction_error - # Should raise TransactionConflictException - with pytest.raises(TransactionConflictException): - service._finalize_and_supersede_with_transaction(new_doc) + service._finalize_and_supersede_with_transaction(new_doc) def test_handle_upload_document_reference_request_no_document_found(service): @@ -660,7 +658,6 @@ def test_process_preliminary_document_reference_exception_during_processing( assert "Processing failed" in str(exc_info.value) - def test_get_infrastructure_for_document_key_non_pdm(service): assert service.table_name == "" infra = service._get_infrastructure_for_document_key(object_parts=["1234", "123"])