From a45949b820041aa0d2834962a4cb5f3221f72010 Mon Sep 17 00:00:00 2001 From: NogaNHS <127490765+NogaNHS@users.noreply.github.com> Date: Wed, 17 Dec 2025 15:48:22 +0000 Subject: [PATCH 1/4] [PRMP-1085] Introduce DocumentReviewReason enum and update review reason handling --- lambdas/enums/document_review_reason.py | 11 +++++++++++ lambdas/models/document_review.py | 5 ++++- lambdas/models/sqs/review_message_body.py | 10 ++++++++-- .../search_document_review/dynamo_response.py | 7 ++++--- .../test_document_review_processor_service.py | 13 +++++++------ .../tests/unit/services/test_document_service.py | 15 ++++++++++----- .../services/test_get_document_review_service.py | 3 ++- .../services/test_process_mns_message_service.py | 1 - .../test_update_document_review_service.py | 11 ++++++++--- 9 files changed, 54 insertions(+), 22 deletions(-) create mode 100644 lambdas/enums/document_review_reason.py diff --git a/lambdas/enums/document_review_reason.py b/lambdas/enums/document_review_reason.py new file mode 100644 index 0000000000..d79e8bc1f5 --- /dev/null +++ b/lambdas/enums/document_review_reason.py @@ -0,0 +1,11 @@ +from enum import StrEnum + + +class DocumentReviewReason(StrEnum): + UNKNOWN_NHS_NUMBER = "Unknown NHS number" + DEMOGRAPHIC_MISMATCHES = "Demographic mismatches" + DUPLICATE_RECORD = "Duplicate records error" + FILE_COUNT_MISMATCH = "More or less files than we expected" + FILE_NAME_MISMATCH = "Filename Naming convention error" + GP2GP_ERROR = "GP2GP error (placeholder)" + GENERAL_ERROR = "General error" diff --git a/lambdas/models/document_review.py b/lambdas/models/document_review.py index 41a366c0e4..6e55396074 100644 --- a/lambdas/models/document_review.py +++ b/lambdas/models/document_review.py @@ -1,6 +1,7 @@ import uuid from datetime import datetime, timezone +from enums.document_review_reason import DocumentReviewReason from enums.document_review_status import DocumentReviewStatus from enums.metadata_field_names import DocumentReferenceMetadataFields from enums.snomed_codes import SnomedCodes @@ -38,7 +39,9 @@ class DocumentUploadReviewReference(BaseModel): review_status: DocumentReviewStatus = Field( default=DocumentReviewStatus.PENDING_REVIEW ) - review_reason: str | None = None + review_reason: DocumentReviewReason = Field( + default=DocumentReviewReason.GENERAL_ERROR + ) review_date: int | None = Field(default=None) reviewer: str | None = Field(default=None) upload_date: int = Field( diff --git a/lambdas/models/sqs/review_message_body.py b/lambdas/models/sqs/review_message_body.py index ef24c791f7..2233a068de 100644 --- a/lambdas/models/sqs/review_message_body.py +++ b/lambdas/models/sqs/review_message_body.py @@ -1,4 +1,5 @@ -from pydantic import BaseModel, Field +from enums.document_review_reason import DocumentReviewReason +from pydantic import BaseModel, ConfigDict, Field class ReviewMessageFile(BaseModel): @@ -12,10 +13,15 @@ class ReviewMessageFile(BaseModel): class ReviewMessageBody(BaseModel): """Model for SQS message body from the document review queue.""" + model_config = ConfigDict( + use_enum_values=True, + ) upload_id: str files: list[ReviewMessageFile] nhs_number: str - failure_reason: str + failure_reason: DocumentReviewReason = Field( + default=DocumentReviewReason.GENERAL_ERROR + ) upload_date: str uploader_ods: str current_gp: str diff --git a/lambdas/tests/unit/helpers/data/search_document_review/dynamo_response.py b/lambdas/tests/unit/helpers/data/search_document_review/dynamo_response.py index 6e1df3c8d1..72f4641528 100644 --- a/lambdas/tests/unit/helpers/data/search_document_review/dynamo_response.py +++ b/lambdas/tests/unit/helpers/data/search_document_review/dynamo_response.py @@ -1,3 +1,4 @@ +from enums.document_review_reason import DocumentReviewReason from enums.document_review_status import DocumentReviewStatus from enums.snomed_codes import SnomedCodes from tests.unit.conftest import ( @@ -28,7 +29,7 @@ "Custodian": TEST_CURRENT_GP_ODS, "UploadDate": 1704110400, "NhsNumber": TEST_NHS_NUMBER, - "ReviewReason": "Failure", + "ReviewReason": DocumentReviewReason.FILE_COUNT_MISMATCH, "ReviewStatus": DocumentReviewStatus.PENDING_REVIEW, "LastUpdated": 1704110400, # Timestamp: 2024-01-01T12:00:00 "DocumentSnomedCodeType": SnomedCodes.LLOYD_GEORGE.value.code, @@ -50,7 +51,7 @@ "Custodian": TEST_CURRENT_GP_ODS, "UploadDate": 1704110400, "NhsNumber": TEST_NHS_NUMBER, - "ReviewReason": "Failure", + "ReviewReason": DocumentReviewReason.FILE_COUNT_MISMATCH, "ReviewStatus": DocumentReviewStatus.PENDING_REVIEW, "LastUpdated": 1704110400, # Timestamp: 2024-01-01T12:00:00 "DocumentSnomedCodeType": SnomedCodes.LLOYD_GEORGE.value.code, @@ -74,7 +75,7 @@ "Reviewer": None, "NhsNumber": TEST_NHS_NUMBER, "ReviewDate": None, - "ReviewReason": "Failure", + "ReviewReason": DocumentReviewReason.FILE_COUNT_MISMATCH, "ReviewStatus": DocumentReviewStatus.PENDING_REVIEW, "LastUpdated": 1704110400, # Timestamp: 2024-01-01T12:00:00 "DocumentSnomedCodeType": SnomedCodes.LLOYD_GEORGE.value.code, diff --git a/lambdas/tests/unit/services/test_document_review_processor_service.py b/lambdas/tests/unit/services/test_document_review_processor_service.py index df1d3fbaeb..ffff8abab3 100644 --- a/lambdas/tests/unit/services/test_document_review_processor_service.py +++ b/lambdas/tests/unit/services/test_document_review_processor_service.py @@ -1,5 +1,6 @@ import pytest from botocore.exceptions import ClientError +from enums.document_review_reason import DocumentReviewReason from enums.document_review_status import DocumentReviewStatus from models.document_review import ( DocumentReviewFileDetails, @@ -40,7 +41,7 @@ def sample_review_message(): ) ], nhs_number="9000000009", - failure_reason="Failed virus scan", + failure_reason=DocumentReviewReason.DEMOGRAPHIC_MISMATCHES, upload_date="2024-01-15T10:30:00Z", uploader_ods="Y12345", current_gp="Y12345", @@ -95,7 +96,7 @@ def test_process_review_message_multiple_files(service_under_test, mocker): ), ], nhs_number="9000000009", - failure_reason="Failed virus scan", + failure_reason=DocumentReviewReason.FILE_COUNT_MISMATCH, upload_date="2024-01-15T10:30:00Z", uploader_ods="Y12345", current_gp="Y12345", @@ -212,7 +213,7 @@ def test_build_review_record_success(service_under_test, sample_review_message): assert result.id == "test-review-id" assert result.nhs_number == "9000000009" assert result.review_status == DocumentReviewStatus.PENDING_REVIEW - assert result.review_reason == "Failed virus scan" + assert result.review_reason == "Demographic mismatches" assert result.author == "Y12345" assert result.custodian == "Y12345" assert len(result.files) == 1 @@ -237,7 +238,7 @@ def test_build_review_record_with_multiple_files(service_under_test): ), ], nhs_number="9000000009", - failure_reason="Failed virus scan", + failure_reason=DocumentReviewReason.FILE_COUNT_MISMATCH, upload_date="2024-01-15T10:30:00Z", uploader_ods="Y12345", current_gp="Y12345", @@ -302,7 +303,7 @@ def test_move_multiple_files_success(service_under_test, mocker): ), ], nhs_number="9000000009", - failure_reason="Failed virus scan", + failure_reason=DocumentReviewReason.FILE_COUNT_MISMATCH, upload_date="2024-01-15T10:30:00Z", uploader_ods="Y12345", current_gp="Y12345", @@ -409,7 +410,7 @@ def test_workflow_handles_multiple_different_patients(service_under_test): ) ], nhs_number=f"900000000{i}", - failure_reason="Test failure", + failure_reason=DocumentReviewReason.GENERAL_ERROR, upload_date="2024-01-15T10:30:00Z", uploader_ods="Y12345", current_gp="Y12345", diff --git a/lambdas/tests/unit/services/test_document_service.py b/lambdas/tests/unit/services/test_document_service.py index 0416cc296d..59b244a505 100644 --- a/lambdas/tests/unit/services/test_document_service.py +++ b/lambdas/tests/unit/services/test_document_service.py @@ -518,7 +518,10 @@ def test_get_item_success( @pytest.mark.parametrize( "document_id,mock_response", [ - ("non-existent-doc", {}, ), + ( + "non-existent-doc", + {}, + ), ( "invalid-doc", {"Item": {"ID": "invalid-doc", "InvalidField": "invalid-value"}}, @@ -526,7 +529,10 @@ def test_get_item_success( ], ) def test_get_item_returns_none( - mock_service, mock_dynamo_service, document_id, mock_response, + mock_service, + mock_dynamo_service, + document_id, + mock_response, ): """Test get_item returns None for not found or invalid documents.""" mock_dynamo_service.get_item.return_value = mock_response @@ -563,11 +569,10 @@ def test_get_item_with_custom_model_class( mock_dynamo_response = { "Item": { "ID": document_id, - "Author": "Y12345", "Custodian": "Y12345", "ReviewStatus": "PENDING_REVIEW", - "ReviewReason": "Test reason", + "ReviewReason": "General error", "UploadDate": 1699000000, "Files": [ { @@ -610,4 +615,4 @@ def test_get_item_document_id_with_sort_key(mock_service, mock_dynamo_service): mock_dynamo_service.get_item.assert_called_once_with( table_name=MOCK_LG_TABLE_NAME, key={"ID": document_id, **sort_key} - ) \ No newline at end of file + ) diff --git a/lambdas/tests/unit/services/test_get_document_review_service.py b/lambdas/tests/unit/services/test_get_document_review_service.py index 82b2696a2d..330b7eb70e 100644 --- a/lambdas/tests/unit/services/test_get_document_review_service.py +++ b/lambdas/tests/unit/services/test_get_document_review_service.py @@ -1,6 +1,7 @@ from unittest.mock import MagicMock import pytest +from enums.document_review_reason import DocumentReviewReason from enums.document_review_status import DocumentReviewStatus from enums.lambda_error import LambdaError from enums.snomed_codes import SnomedCodes @@ -65,7 +66,7 @@ def mock_document_review(): author=TEST_ODS_CODE, custodian=TEST_ODS_CODE, review_status=DocumentReviewStatus.PENDING_REVIEW, - review_reason="Uploaded for review", + review_reason=DocumentReviewReason.FILE_NAME_MISMATCH, upload_date=1699000000, files=files, nhs_number=TEST_NHS_NUMBER, diff --git a/lambdas/tests/unit/services/test_process_mns_message_service.py b/lambdas/tests/unit/services/test_process_mns_message_service.py index cbbb9b115a..dc00482328 100644 --- a/lambdas/tests/unit/services/test_process_mns_message_service.py +++ b/lambdas/tests/unit/services/test_process_mns_message_service.py @@ -73,7 +73,6 @@ def mock_document_references(mocker): @pytest.fixture def mock_document_review_references(mocker): - # Create a list of mock document review references reviews = [] for i in range(2): review = MagicMock(spec=DocumentUploadReviewReference) 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..1da9f85487 100644 --- a/lambdas/tests/unit/services/test_update_document_review_service.py +++ b/lambdas/tests/unit/services/test_update_document_review_service.py @@ -1,6 +1,7 @@ from unittest.mock import MagicMock import pytest +from enums.document_review_reason import DocumentReviewReason from enums.document_review_status import DocumentReviewStatus from enums.lambda_error import LambdaError from enums.snomed_codes import SnomedCodes @@ -56,7 +57,7 @@ def mock_document_review(): author=TEST_ODS_CODE, custodian=TEST_ODS_CODE, review_status=DocumentReviewStatus.PENDING_REVIEW, - review_reason="Uploaded for review", + review_reason=DocumentReviewReason.DUPLICATE_RECORD, upload_date=TEST_UPLOAD_DATE, files=files, nhs_number=TEST_NHS_NUMBER, @@ -732,7 +733,11 @@ def test_update_document_review_raises_exception_when_updating_approved_pending_ mock_document_review.review_status = DocumentReviewStatus.APPROVED_PENDING_DOCUMENTS update_data = PatchDocumentReviewRequest( review_status=invalid_target_status, - nhs_number=TEST_REASSIGNED_NHS_NUMBER if "REASSIGNED" in invalid_target_status.value else None, + nhs_number=( + TEST_REASSIGNED_NHS_NUMBER + if "REASSIGNED" in invalid_target_status.value + else None + ), ) mock_service.document_review_service.get_document_review_by_id.return_value = ( mock_document_review @@ -784,4 +789,4 @@ def test_update_document_review_raises_exception_when_approving_from_non_approve ) assert exc_info.value.status_code == 400 - assert exc_info.value.error == LambdaError.UpdateDocStatusUnavailable \ No newline at end of file + assert exc_info.value.error == LambdaError.UpdateDocStatusUnavailable From 881df81b401128a58ea587920f466041f8b7551a Mon Sep 17 00:00:00 2001 From: NogaNHS <127490765+NogaNHS@users.noreply.github.com> Date: Wed, 17 Dec 2025 15:52:46 +0000 Subject: [PATCH 2/4] [PRMP-1085] Update GP2GP error message for clarity --- lambdas/enums/document_review_reason.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lambdas/enums/document_review_reason.py b/lambdas/enums/document_review_reason.py index d79e8bc1f5..4cee878faa 100644 --- a/lambdas/enums/document_review_reason.py +++ b/lambdas/enums/document_review_reason.py @@ -7,5 +7,5 @@ class DocumentReviewReason(StrEnum): DUPLICATE_RECORD = "Duplicate records error" FILE_COUNT_MISMATCH = "More or less files than we expected" FILE_NAME_MISMATCH = "Filename Naming convention error" - GP2GP_ERROR = "GP2GP error (placeholder)" + GP2GP_ERROR = "GP2GP failure" GENERAL_ERROR = "General error" From 50037b7c10677f5933e7805ad8e8261d2571aff9 Mon Sep 17 00:00:00 2001 From: NogaNHS <127490765+NogaNHS@users.noreply.github.com> Date: Wed, 17 Dec 2025 15:55:14 +0000 Subject: [PATCH 3/4] [PRMP-1085] Update failure reasons in document review tests to use DocumentReviewReason enum --- .../test_document_review_processor_handler.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lambdas/tests/unit/handlers/test_document_review_processor_handler.py b/lambdas/tests/unit/handlers/test_document_review_processor_handler.py index 11c6bbdeac..1b1f388781 100644 --- a/lambdas/tests/unit/handlers/test_document_review_processor_handler.py +++ b/lambdas/tests/unit/handlers/test_document_review_processor_handler.py @@ -1,6 +1,8 @@ import json import pytest + +from enums.document_review_reason import DocumentReviewReason from handlers.document_review_processor_handler import lambda_handler from models.sqs.review_message_body import ReviewMessageBody, ReviewMessageFile @@ -27,7 +29,7 @@ def sample_review_message_body(): ) ], nhs_number="9000000009", - failure_reason="Failed virus scan", + failure_reason=DocumentReviewReason.GENERAL_ERROR, upload_date="2024-01-15T10:30:00Z", uploader_ods="Y12345", current_gp="Y12345", @@ -62,7 +64,7 @@ def sample_sqs_event_multiple_messages(sample_review_message_body): ) ], nhs_number="9000000009", - failure_reason="Failed virus scan", + failure_reason=DocumentReviewReason.UNKNOWN_NHS_NUMBER, upload_date="2024-01-15T10:30:00Z", uploader_ods="Y12345", current_gp="Y12345", @@ -77,7 +79,7 @@ def sample_sqs_event_multiple_messages(sample_review_message_body): ) ], nhs_number="9000000010", - failure_reason="Invalid file format", + failure_reason=DocumentReviewReason.FILE_NAME_MISMATCH, upload_date="2024-01-15T10:35:00Z", uploader_ods="Y12345", current_gp="Y12345", @@ -92,7 +94,7 @@ def sample_sqs_event_multiple_messages(sample_review_message_body): ) ], nhs_number="9000000011", - failure_reason="Missing metadata", + failure_reason=DocumentReviewReason.FILE_COUNT_MISMATCH, upload_date="2024-01-15T10:40:00Z", uploader_ods="Y67890", current_gp="Y67890", @@ -194,7 +196,7 @@ def test_lambda_handler_parses_json_body_correctly( {"file_name": "test.pdf", "file_path": "staging/test.pdf"} ], "nhs_number": "9000000009", - "failure_reason": "Test failure", + "failure_reason": "General error", "upload_date": "2024-01-15T10:30:00Z", "uploader_ods": "Y12345", "current_gp": "Y12345", From a1199313e721548a5d62d035757b7bda513711c3 Mon Sep 17 00:00:00 2001 From: NogaNHS <127490765+NogaNHS@users.noreply.github.com> Date: Fri, 19 Dec 2025 12:07:46 +0000 Subject: [PATCH 4/4] [PRMP-1085] document review reason handling to use updated DocumentReviewReason enum --- lambdas/enums/document_review_status.py | 3 --- lambdas/services/post_document_review_service.py | 6 ++++-- lambdas/tests/unit/services/test_ods_report_service.py | 4 +++- .../test_staged_document_review_processing_service.py | 4 +++- 4 files changed, 10 insertions(+), 7 deletions(-) diff --git a/lambdas/enums/document_review_status.py b/lambdas/enums/document_review_status.py index 462df49d0d..d63571ba93 100644 --- a/lambdas/enums/document_review_status.py +++ b/lambdas/enums/document_review_status.py @@ -13,6 +13,3 @@ class DocumentReviewStatus(StrEnum): REVIEW_PENDING_UPLOAD = "REVIEW_PENDING_UPLOAD" VIRUS_SCAN_FAILED = "VIRUS_SCAN_FAILED" - -class DocumentReviewReason(StrEnum): - REVIEW_FROM_DATA_CONTROLLER = "Needs review from data controller" diff --git a/lambdas/services/post_document_review_service.py b/lambdas/services/post_document_review_service.py index 9850d1a386..28085df6f2 100644 --- a/lambdas/services/post_document_review_service.py +++ b/lambdas/services/post_document_review_service.py @@ -3,7 +3,9 @@ from datetime import datetime, timezone from botocore.exceptions import ClientError -from enums.document_review_status import DocumentReviewReason, DocumentReviewStatus + +from enums.document_review_reason import DocumentReviewReason +from enums.document_review_status import DocumentReviewStatus from enums.lambda_error import LambdaError from enums.patient_ods_inactive_status import PatientOdsInactiveStatus from models.document_review import ( @@ -116,7 +118,7 @@ def create_review_reference_from_event( files=document_file_details, nhs_number=event.nhs_number, document_snomed_code_type=event.snomed_code.code, - review_reason=DocumentReviewReason.REVIEW_FROM_DATA_CONTROLLER, + review_reason=DocumentReviewReason.GP2GP_ERROR, ) return document_review_reference diff --git a/lambdas/tests/unit/services/test_ods_report_service.py b/lambdas/tests/unit/services/test_ods_report_service.py index 4cf472ab7f..81d95c3374 100644 --- a/lambdas/tests/unit/services/test_ods_report_service.py +++ b/lambdas/tests/unit/services/test_ods_report_service.py @@ -4,6 +4,8 @@ from unittest.mock import call import pytest + +from enums.document_review_reason import DocumentReviewReason from enums.document_review_status import DocumentReviewStatus from enums.dynamo_filter import AttributeOperator from enums.file_type import FileType @@ -61,7 +63,7 @@ def mock_review_result(): return DocumentUploadReviewReference( nhs_number="mock_nhs_number", author="mock_author", - review_reason="mock_review_reason", + review_reason=DocumentReviewReason.GENERAL_ERROR, document_snomed_code_type="mock_snomed_code", upload_date=int(datetime.now().timestamp()), files=[ diff --git a/lambdas/tests/unit/services/test_staged_document_review_processing_service.py b/lambdas/tests/unit/services/test_staged_document_review_processing_service.py index 7d9d1c0454..a801009f88 100644 --- a/lambdas/tests/unit/services/test_staged_document_review_processing_service.py +++ b/lambdas/tests/unit/services/test_staged_document_review_processing_service.py @@ -1,5 +1,7 @@ import pytest from botocore.exceptions import ClientError + +from enums.document_review_reason import DocumentReviewReason from enums.document_review_status import DocumentReviewStatus from enums.virus_scan_result import VirusScanResult from models.document_reference import S3_PREFIX @@ -65,7 +67,7 @@ def sample_document_reference(): author="Y12345", custodian="Y12345", review_status=DocumentReviewStatus.REVIEW_PENDING_UPLOAD, - review_reason="Test reason", + review_reason=DocumentReviewReason.FILE_COUNT_MISMATCH, upload_date=1704110400, files=[ DocumentReviewFileDetails(