From b5377a8f64f141201cebd4b3dd44ec8172709394 Mon Sep 17 00:00:00 2001 From: NogaNHS <127490765+NogaNHS@users.noreply.github.com> Date: Thu, 18 Dec 2025 16:01:33 +0000 Subject: [PATCH 1/2] Refactor document review status handling: replace APPROVED_PENDING_DOCUMENTS with REVIEW_IN_PROGRESS and update related methods --- app/src/types/blocks/documentReview.ts | 2 +- lambdas/enums/document_review_status.py | 2 +- .../document_upload_review_service.py | 9 -- .../update_document_review_service.py | 35 +----- .../test_document_upload_review_service.py | 6 +- .../test_update_document_review_service.py | 117 ++---------------- 6 files changed, 19 insertions(+), 152 deletions(-) diff --git a/app/src/types/blocks/documentReview.ts b/app/src/types/blocks/documentReview.ts index 7aa86912e8..007c6d0b9d 100644 --- a/app/src/types/blocks/documentReview.ts +++ b/app/src/types/blocks/documentReview.ts @@ -23,7 +23,7 @@ export type DocumentReviewStatusDto = { export enum DocumentReviewStatus { PENDING_REVIEW = "PENDING_REVIEW", APPROVED = "APPROVED", - APPROVED_PENDING_DOCUMENTS = "APPROVED_PENDING_DOCUMENTS", + REVIEW_IN_PROGRESS = "REVIEW_IN_PROGRESS", REJECTED = "REJECTED", AWAITING_DOCUMENTS = "AWAITING_DOCUMENTS", REJECTED_DUPLICATE = "REJECTED_DUPLICATE", diff --git a/lambdas/enums/document_review_status.py b/lambdas/enums/document_review_status.py index 462df49d0d..266e182719 100644 --- a/lambdas/enums/document_review_status.py +++ b/lambdas/enums/document_review_status.py @@ -4,7 +4,7 @@ class DocumentReviewStatus(StrEnum): PENDING_REVIEW = "PENDING_REVIEW" APPROVED = "APPROVED" - APPROVED_PENDING_DOCUMENTS = "APPROVED_PENDING_DOCUMENTS" + REVIEW_IN_PROGRESS = "REVIEW_IN_PROGRESS" REJECTED = "REJECTED" REJECTED_DUPLICATE = "REJECTED_DUPLICATE" REASSIGNED = "REASSIGNED" diff --git a/lambdas/services/document_upload_review_service.py b/lambdas/services/document_upload_review_service.py index da0e2c0b9a..624ed595f1 100644 --- a/lambdas/services/document_upload_review_service.py +++ b/lambdas/services/document_upload_review_service.py @@ -154,15 +154,6 @@ def update_pending_review_status( DocumentReviewStatus.PENDING_REVIEW, ) - def update_approved_pending_review_status( - self, review_update: DocumentUploadReviewReference, field_names: set[str] - ) -> None: - self.update_review_document_with_status_filter( - review_update, - field_names, - DocumentReviewStatus.APPROVED_PENDING_DOCUMENTS, - ) - def update_review_document_with_status_filter( self, review_update: DocumentUploadReviewReference, diff --git a/lambdas/services/update_document_review_service.py b/lambdas/services/update_document_review_service.py index a4a7f09c95..18b4c860d3 100644 --- a/lambdas/services/update_document_review_service.py +++ b/lambdas/services/update_document_review_service.py @@ -35,8 +35,6 @@ class UpdateDocumentReviewService: DocumentReviewStatus.APPROVED, ] - APPROVED_PENDING_REVIEW_STATUSES = [DocumentReviewStatus.APPROVED_PENDING_DOCUMENTS] - UNKNOWN_NHS_NUMBER = "0000000000" FAILED_LOG_MESSAGE = "Failed to update document review" @@ -112,7 +110,7 @@ def _validate_patient_id_match(self, document, patient_id: str): def _validate_review_status(self, document): if document.review_status not in [ DocumentReviewStatus.PENDING_REVIEW, - DocumentReviewStatus.APPROVED_PENDING_DOCUMENTS, + DocumentReviewStatus.REVIEW_IN_PROGRESS, ]: logger.error( f"Invalid review status for document_id: {document.id}. " @@ -133,17 +131,6 @@ def _validate_user_match_custodian(self, document, reviewer_ods_code: str): 403, LambdaError.DocumentReferenceUnauthorised ) - def _ensure_approved_status_follows_pending_review(self, document, update_data): - is_approved_pending_review = document.review_status == DocumentReviewStatus.APPROVED_PENDING_DOCUMENTS - is_update_status_approved = update_data.review_status == DocumentReviewStatus.APPROVED - - is_invalid_approved_pending_transition = is_approved_pending_review and not is_update_status_approved - is_approved_status_not_follows_pending_review = is_update_status_approved and not is_approved_pending_review - if is_approved_status_not_follows_pending_review or is_invalid_approved_pending_transition: - raise UpdateDocumentReviewException( - 400, LambdaError.UpdateDocStatusUnavailable - ) - def _process_review_status_update( self, document, @@ -151,7 +138,6 @@ def _process_review_status_update( document_id: str, reviewer_ods_code: str, ): - self._ensure_approved_status_follows_pending_review(document, update_data) self._set_review_metadata(document, update_data, reviewer_ods_code) self._execute_status_action(document, update_data, document_id) @@ -169,11 +155,9 @@ def _execute_status_action(self, document, update_data, document_id): elif document.review_status in self.APPROVED_REVIEW_STATUSES: document.document_reference_id = update_data.document_reference_id update_fields.add("document_reference_id") - self._handle_approval(document, update_fields) + self._handle_rejection_or_approval(document, update_fields) elif document.review_status in self.REJECTED_REVIEW_STATUSES: - self._handle_rejection(document, update_fields) - elif document.review_status in self.APPROVED_PENDING_REVIEW_STATUSES: - self._handle_approved_pending(document, update_fields) + self._handle_rejection_or_approval(document, update_fields) else: logger.error( f"Invalid status update attempted: {document.review_status}", @@ -183,23 +167,12 @@ def _execute_status_action(self, document, update_data, document_id): 400, LambdaError.DocumentReviewGeneralError ) - def _handle_rejection(self, document, update_fields): + def _handle_rejection_or_approval(self, document, update_fields): self.document_review_service.update_pending_review_status( review_update=document, field_names=update_fields ) self._handle_soft_delete(document) - def _handle_approval(self, document, update_fields): - self.document_review_service.update_approved_pending_review_status( - review_update=document, field_names=update_fields - ) - self._handle_soft_delete(document) - - def _handle_approved_pending(self, document, update_fields): - self.document_review_service.update_pending_review_status( - review_update=document, field_names=update_fields - ) - def _handle_soft_delete(self, review_document: DocumentUploadReviewReference): logger.info( f"Deleting document review files for document_id: {review_document.id}" diff --git a/lambdas/tests/unit/services/test_document_upload_review_service.py b/lambdas/tests/unit/services/test_document_upload_review_service.py index 9cd488f957..080f08cb0a 100644 --- a/lambdas/tests/unit/services/test_document_upload_review_service.py +++ b/lambdas/tests/unit/services/test_document_upload_review_service.py @@ -249,7 +249,7 @@ def test_update_document_review_for_patient_conditional_check_failed( field_names = {"review_status"} with pytest.raises(DocumentReviewException): - mock_service.update_approved_pending_review_status( + mock_service.update_pending_review_status( review_update=mock_review_update, field_names=field_names, ) @@ -260,7 +260,7 @@ def test_update_document_review_for_patient_conditional_check_failed( expected_condition = ( Attr(DocumentReferenceMetadataFields.ID.value).exists() & Attr("NhsNumber").eq(mock_review_update.nhs_number) - & Attr("ReviewStatus").eq(DocumentReviewStatus.APPROVED_PENDING_DOCUMENTS) + & Attr("ReviewStatus").eq(DocumentReviewStatus.PENDING_REVIEW) ) assert condition_expr == expected_condition @@ -285,7 +285,7 @@ def test_update_document_review_for_patient_other_client_error( field_names = {"review_status"} with pytest.raises(DocumentReviewException): - mock_service.update_approved_pending_review_status( + mock_service.update_pending_review_status( review_update=mock_review_update, field_names=field_names, ) diff --git a/lambdas/tests/unit/services/test_update_document_review_service.py b/lambdas/tests/unit/services/test_update_document_review_service.py index ecc35962aa..04c8b29e8c 100644 --- a/lambdas/tests/unit/services/test_update_document_review_service.py +++ b/lambdas/tests/unit/services/test_update_document_review_service.py @@ -88,7 +88,7 @@ def mock_pds_service(mocker): def test_update_document_review_successfully_approves_document( mock_service, mock_document_review ): - mock_document_review.review_status = DocumentReviewStatus.APPROVED_PENDING_DOCUMENTS + mock_document_review.review_status = DocumentReviewStatus.REVIEW_IN_PROGRESS update_data = PatchDocumentReviewRequest( review_status=DocumentReviewStatus.APPROVED, document_reference_id=TEST_DOCUMENT_REFERENCE_ID, @@ -108,10 +108,10 @@ def test_update_document_review_successfully_approves_document( mock_service.document_review_service.get_document_review_by_id.assert_called_once_with( document_id=TEST_DOCUMENT_ID, document_version=TEST_VERSION ) - mock_service.document_review_service.update_approved_pending_review_status.assert_called_once() + mock_service.document_review_service.update_pending_review_status.assert_called_once() call_args = ( - mock_service.document_review_service.update_approved_pending_review_status.call_args + mock_service.document_review_service.update_pending_review_status.call_args ) updated_doc = call_args.kwargs["review_update"] assert updated_doc.review_status == DocumentReviewStatus.APPROVED @@ -251,38 +251,6 @@ def test_update_document_review_successfully_reassigns_document_when_patient_unk assert existing_review.nhs_number == TEST_NHS_NUMBER -@freeze_time(TEST_FROZEN_TIME) -def test_update_document_review_successfully_sets_approved_pending_documents_status( - mock_service, mock_document_review -): - update_data = PatchDocumentReviewRequest( - review_status=DocumentReviewStatus.APPROVED_PENDING_DOCUMENTS, - ) - mock_service.document_review_service.get_document_review_by_id.return_value = ( - mock_document_review - ) - - mock_service.update_document_review( - patient_id=TEST_NHS_NUMBER, - document_id=TEST_DOCUMENT_ID, - document_version=TEST_VERSION, - update_data=update_data, - reviewer_ods_code=TEST_REVIEWER_ODS_CODE, - ) - - mock_service.document_review_service.update_pending_review_status.assert_called_once() - call_args = ( - mock_service.document_review_service.update_pending_review_status.call_args - ) - updated_doc = call_args.kwargs["review_update"] - assert updated_doc.review_status == DocumentReviewStatus.APPROVED_PENDING_DOCUMENTS - assert updated_doc.reviewer == TEST_REVIEWER_ODS_CODE - assert updated_doc.review_date == TEST_FROZEN_TIME_TIMESTAMP - assert "document_reference_id" not in call_args.kwargs["field_names"] - assert "reviewer" in call_args.kwargs["field_names"] - assert "review_date" in call_args.kwargs["field_names"] - - def test_update_document_review_raises_exception_when_document_not_found(mock_service): update_data = PatchDocumentReviewRequest( review_status=DocumentReviewStatus.APPROVED, @@ -307,31 +275,6 @@ def test_update_document_review_raises_exception_when_document_not_found(mock_se mock_service.document_review_service.update_pending_review_status.assert_not_called() -def test_update_document_review_raises_exception_when_approving_from_pending_review( - mock_service, mock_document_review -): - update_data = PatchDocumentReviewRequest( - review_status=DocumentReviewStatus.APPROVED, - document_reference_id=TEST_DOCUMENT_REFERENCE_ID, - ) - mock_service.document_review_service.get_document_review_by_id.return_value = ( - mock_document_review - ) - - with pytest.raises(UpdateDocumentReviewException) as exc_info: - mock_service.update_document_review( - patient_id=TEST_NHS_NUMBER, - document_id=TEST_DOCUMENT_ID, - document_version=TEST_VERSION, - update_data=update_data, - reviewer_ods_code=TEST_REVIEWER_ODS_CODE, - ) - - assert exc_info.value.status_code == 400 - assert exc_info.value.error == LambdaError.UpdateDocStatusUnavailable - mock_service.document_review_service.update_pending_review_status.assert_not_called() - - def test_fetch_document_returns_document_when_exists( mock_service, mock_document_review ): @@ -382,7 +325,7 @@ def test_validate_patient_id_match_raises_exception_when_nhs_numbers_dont_match( "valid_status", [ DocumentReviewStatus.PENDING_REVIEW, - DocumentReviewStatus.APPROVED_PENDING_DOCUMENTS, + DocumentReviewStatus.REVIEW_IN_PROGRESS, ], ) def test_validate_review_status_passes_when_status_is_valid( @@ -524,31 +467,11 @@ def test_create_reassigned_document_preserves_document_id( assert result.id == TEST_DOCUMENT_ID -@freeze_time(TEST_FROZEN_TIME) -def test_process_review_status_update_sets_review_date_and_reviewer( - mock_service, mock_document_review -): - update_data = PatchDocumentReviewRequest( - review_status=DocumentReviewStatus.APPROVED_PENDING_DOCUMENTS, - ) - - mock_service._process_review_status_update( - mock_document_review, update_data, TEST_DOCUMENT_ID, TEST_REVIEWER_ODS_CODE - ) - - assert mock_document_review.review_date == TEST_FROZEN_TIME_TIMESTAMP - assert mock_document_review.reviewer == TEST_REVIEWER_ODS_CODE - assert ( - mock_document_review.review_status - == DocumentReviewStatus.APPROVED_PENDING_DOCUMENTS - ) - - @freeze_time(TEST_FROZEN_TIME) def test_process_review_status_update_calls_approved_service_for_approval( mock_service, mock_document_review ): - mock_document_review.review_status = DocumentReviewStatus.APPROVED_PENDING_DOCUMENTS + mock_document_review.review_status = DocumentReviewStatus.REVIEW_IN_PROGRESS update_data = PatchDocumentReviewRequest( review_status=DocumentReviewStatus.APPROVED, document_reference_id=TEST_DOCUMENT_REFERENCE_ID, @@ -558,9 +481,9 @@ def test_process_review_status_update_calls_approved_service_for_approval( mock_document_review, update_data, TEST_DOCUMENT_ID, TEST_REVIEWER_ODS_CODE ) - mock_service.document_review_service.update_approved_pending_review_status.assert_called_once() + mock_service.document_review_service.update_pending_review_status.assert_called_once() call_args = ( - mock_service.document_review_service.update_approved_pending_review_status.call_args + mock_service.document_review_service.update_pending_review_status.call_args ) assert ( call_args.kwargs["review_update"].document_reference_id @@ -623,30 +546,12 @@ def test_process_review_status_update_calls_handle_reassignment_for_reassignment ) -@freeze_time(TEST_FROZEN_TIME) -def test_process_review_status_update_raises_exception_when_approving_from_wrong_status( - mock_service, mock_document_review -): - mock_document_review.review_status = DocumentReviewStatus.PENDING_REVIEW - update_data = PatchDocumentReviewRequest( - review_status=DocumentReviewStatus.APPROVED, - document_reference_id=TEST_DOCUMENT_REFERENCE_ID, - ) - - with pytest.raises(UpdateDocumentReviewException) as exc_info: - mock_service._process_review_status_update( - mock_document_review, update_data, TEST_DOCUMENT_ID, TEST_REVIEWER_ODS_CODE - ) - - assert exc_info.value.status_code == 400 - assert exc_info.value.error == LambdaError.UpdateDocStatusUnavailable - @freeze_time(TEST_FROZEN_TIME) def test_process_review_status_update_calls_soft_delete_for_approval( mock_service, mock_document_review, mocker ): - mock_document_review.review_status = DocumentReviewStatus.APPROVED_PENDING_DOCUMENTS + mock_document_review.review_status = DocumentReviewStatus.REVIEW_IN_PROGRESS update_data = PatchDocumentReviewRequest( review_status=DocumentReviewStatus.APPROVED, document_reference_id=TEST_DOCUMENT_REFERENCE_ID, @@ -725,11 +630,10 @@ def test_handle_reassignment_status_creates_new_document_and_updates_with_transa DocumentReviewStatus.REASSIGNED_PATIENT_UNKNOWN, ], ) -def test_update_document_review_raises_exception_when_updating_approved_pending_to_invalid_status( +def test_update_document_review_raises_exception_when_updating_approved_to_invalid_status( mock_service, mock_document_review, invalid_target_status ): - """Test that APPROVED_PENDING_DOCUMENTS can only transition to APPROVED""" - mock_document_review.review_status = DocumentReviewStatus.APPROVED_PENDING_DOCUMENTS + mock_document_review.review_status = DocumentReviewStatus.APPROVED update_data = PatchDocumentReviewRequest( review_status=invalid_target_status, nhs_number=TEST_REASSIGNED_NHS_NUMBER if "REASSIGNED" in invalid_target_status.value else None, @@ -764,7 +668,6 @@ def test_update_document_review_raises_exception_when_updating_approved_pending_ def test_update_document_review_raises_exception_when_approving_from_non_approved_pending_status( mock_service, mock_document_review, from_status ): - """Test that only APPROVED_PENDING_DOCUMENTS can transition to APPROVED""" mock_document_review.review_status = from_status update_data = PatchDocumentReviewRequest( review_status=DocumentReviewStatus.APPROVED, From e866afdf8cd795a1b18fb386b0bffe680a61eed2 Mon Sep 17 00:00:00 2001 From: NogaNHS <127490765+NogaNHS@users.noreply.github.com> Date: Thu, 18 Dec 2025 16:08:08 +0000 Subject: [PATCH 2/2] Update document review status handling: replace REVIEW_IN_PROGRESS with PENDING_REVIEW in tests and validation --- lambdas/services/update_document_review_service.py | 1 - .../unit/services/test_update_document_review_service.py | 7 +++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/lambdas/services/update_document_review_service.py b/lambdas/services/update_document_review_service.py index 18b4c860d3..90dc2a29d0 100644 --- a/lambdas/services/update_document_review_service.py +++ b/lambdas/services/update_document_review_service.py @@ -110,7 +110,6 @@ def _validate_patient_id_match(self, document, patient_id: str): def _validate_review_status(self, document): if document.review_status not in [ DocumentReviewStatus.PENDING_REVIEW, - DocumentReviewStatus.REVIEW_IN_PROGRESS, ]: logger.error( f"Invalid review status for document_id: {document.id}. " diff --git a/lambdas/tests/unit/services/test_update_document_review_service.py b/lambdas/tests/unit/services/test_update_document_review_service.py index 04c8b29e8c..3181cce04c 100644 --- a/lambdas/tests/unit/services/test_update_document_review_service.py +++ b/lambdas/tests/unit/services/test_update_document_review_service.py @@ -88,7 +88,7 @@ def mock_pds_service(mocker): def test_update_document_review_successfully_approves_document( mock_service, mock_document_review ): - mock_document_review.review_status = DocumentReviewStatus.REVIEW_IN_PROGRESS + mock_document_review.review_status = DocumentReviewStatus.PENDING_REVIEW update_data = PatchDocumentReviewRequest( review_status=DocumentReviewStatus.APPROVED, document_reference_id=TEST_DOCUMENT_REFERENCE_ID, @@ -325,7 +325,6 @@ def test_validate_patient_id_match_raises_exception_when_nhs_numbers_dont_match( "valid_status", [ DocumentReviewStatus.PENDING_REVIEW, - DocumentReviewStatus.REVIEW_IN_PROGRESS, ], ) def test_validate_review_status_passes_when_status_is_valid( @@ -471,7 +470,7 @@ def test_create_reassigned_document_preserves_document_id( def test_process_review_status_update_calls_approved_service_for_approval( mock_service, mock_document_review ): - mock_document_review.review_status = DocumentReviewStatus.REVIEW_IN_PROGRESS + mock_document_review.review_status = DocumentReviewStatus.PENDING_REVIEW update_data = PatchDocumentReviewRequest( review_status=DocumentReviewStatus.APPROVED, document_reference_id=TEST_DOCUMENT_REFERENCE_ID, @@ -551,7 +550,7 @@ def test_process_review_status_update_calls_handle_reassignment_for_reassignment def test_process_review_status_update_calls_soft_delete_for_approval( mock_service, mock_document_review, mocker ): - mock_document_review.review_status = DocumentReviewStatus.REVIEW_IN_PROGRESS + mock_document_review.review_status = DocumentReviewStatus.PENDING_REVIEW update_data = PatchDocumentReviewRequest( review_status=DocumentReviewStatus.APPROVED, document_reference_id=TEST_DOCUMENT_REFERENCE_ID,