From 2938191cbf559a7515dbdcaa4463510a6a927c69 Mon Sep 17 00:00:00 2001 From: NogaNHS Date: Mon, 15 Jan 2024 11:53:35 +0000 Subject: [PATCH] catching 404 from PDS --- lambdas/handlers/bulk_upload_handler.py | 3 +- lambdas/models/bulk_upload_status.py | 4 +- lambdas/services/bulk_upload_service.py | 67 +++--- .../unit/handlers/test_bulk_upload_handler.py | 7 +- .../expected_bulk_upload_report.csv | 2 +- .../unit/models/test_bulk_upload_status.py | 7 +- .../test_bulk_upload_report_service.py | 18 +- .../unit/services/test_bulk_upload_service.py | 192 +++++++++++------- .../unit/utils/test_lloyd_george_validator.py | 31 ++- lambdas/utils/lloyd_george_validator.py | 33 +-- 10 files changed, 232 insertions(+), 132 deletions(-) diff --git a/lambdas/handlers/bulk_upload_handler.py b/lambdas/handlers/bulk_upload_handler.py index 5fb14521c..043883094 100644 --- a/lambdas/handlers/bulk_upload_handler.py +++ b/lambdas/handlers/bulk_upload_handler.py @@ -3,8 +3,7 @@ from utils.audit_logging_setup import LoggingService from utils.decorators.override_error_check import override_error_check from utils.decorators.set_audit_arg import set_request_context_for_logging -from utils.exceptions import (InvalidMessageException, - PdsTooManyRequestsException) +from utils.exceptions import InvalidMessageException, PdsTooManyRequestsException from utils.lloyd_george_validator import LGInvalidFilesException logger = LoggingService(__name__) diff --git a/lambdas/models/bulk_upload_status.py b/lambdas/models/bulk_upload_status.py index 5c7ad06fd..85419bf8b 100644 --- a/lambdas/models/bulk_upload_status.py +++ b/lambdas/models/bulk_upload_status.py @@ -15,6 +15,7 @@ class UploadStatusBaseClass(BaseModel): timestamp: int = Field(default_factory=lambda: int(datetime.now().timestamp())) date: str = Field(default_factory=lambda: date_string_yyyymmdd(datetime.now())) file_path: str + ods_code: str = "" class SuccessfulUpload(UploadStatusBaseClass): @@ -24,7 +25,6 @@ class SuccessfulUpload(UploadStatusBaseClass): class FailedUpload(UploadStatusBaseClass): upload_status: Literal["failed"] = "failed" failure_reason: str - ods_code: str FieldNamesForBulkUploadReport = [ @@ -35,7 +35,7 @@ class FailedUpload(UploadStatusBaseClass): "Date", "Timestamp", "ID", - "OdsCode" + "OdsCode", ] diff --git a/lambdas/services/bulk_upload_service.py b/lambdas/services/bulk_upload_service.py index 283d1b7de..5dd016269 100644 --- a/lambdas/services/bulk_upload_service.py +++ b/lambdas/services/bulk_upload_service.py @@ -12,19 +12,28 @@ from services.s3_service import S3Service from services.sqs_service import SQSService from utils.audit_logging_setup import LoggingService -from utils.exceptions import (DocumentInfectedException, - InvalidMessageException, - PatientRecordAlreadyExistException, - PdsTooManyRequestsException, - S3FileNotFoundException, TagNotFoundException, - VirusScanFailedException, - VirusScanNoResultException) -from utils.lloyd_george_validator import (LGInvalidFilesException, - validate_lg_file_names, validate_with_pds_service, - getting_patient_info_from_pds) +from utils.exceptions import ( + DocumentInfectedException, + InvalidMessageException, + PatientRecordAlreadyExistException, + PdsTooManyRequestsException, + S3FileNotFoundException, + TagNotFoundException, + VirusScanFailedException, + VirusScanNoResultException, +) +from utils.lloyd_george_validator import ( + LGInvalidFilesException, + validate_lg_file_names, + validate_with_pds_service, + getting_patient_info_from_pds, +) from utils.request_context import request_context -from utils.unicode_utils import (contains_accent_char, convert_to_nfc_form, - convert_to_nfd_form) +from utils.unicode_utils import ( + contains_accent_char, + convert_to_nfc_form, + convert_to_nfd_form, +) from utils.utilities import create_reference_id logger = LoggingService(__name__) @@ -69,10 +78,13 @@ def handle_sqs_message(self, message: dict): request_context.patient_nhs_no = staging_metadata.nhs_number logger.info("Running validation for file names...") file_names = [ - os.path.basename(metadata.file_path) for metadata in staging_metadata.files + os.path.basename(metadata.file_path) + for metadata in staging_metadata.files ] validate_lg_file_names(file_names, staging_metadata.nhs_number) - pds_patient_details = getting_patient_info_from_pds(staging_metadata.nhs_number) + pds_patient_details = getting_patient_info_from_pds( + staging_metadata.nhs_number + ) patient_ods_code = pds_patient_details.general_practice_ods validate_with_pds_service(file_names, pds_patient_details) @@ -89,10 +101,11 @@ def handle_sqs_message(self, message: dict): logger.info("Will stop processing Lloyd George record for this patient.") failure_reason = str(error) - self.report_upload_failure(staging_metadata, failure_reason, patient_ods_code) + self.report_upload_failure( + staging_metadata, failure_reason, patient_ods_code + ) return - logger.info("File validation complete. Checking virus scan results") try: @@ -115,7 +128,9 @@ def handle_sqs_message(self, message: dict): logger.info("Will stop processing Lloyd George record for this patient") self.report_upload_failure( - staging_metadata, "One or more of the files failed virus scanner check", patient_ods_code + staging_metadata, + "One or more of the files failed virus scanner check", + patient_ods_code, ) return except S3FileNotFoundException as e: @@ -128,7 +143,7 @@ def handle_sqs_message(self, message: dict): self.report_upload_failure( staging_metadata, "One or more of the files is not accessible from staging bucket", - patient_ods_code + patient_ods_code, ) return @@ -155,7 +170,7 @@ def handle_sqs_message(self, message: dict): self.report_upload_failure( staging_metadata, "Validation passed but error occurred during file transfer", - patient_ods_code + patient_ods_code, ) return @@ -207,7 +222,9 @@ def check_virus_result(self, staging_metadata: StagingMetadata): f"Verified that all documents for patient {staging_metadata.nhs_number} are clean." ) - def put_staging_metadata_back_to_queue(self, staging_metadata: StagingMetadata, patient_ods_code: str): + def put_staging_metadata_back_to_queue( + self, staging_metadata: StagingMetadata, patient_ods_code: str + ): if staging_metadata.retries > 14: err = "File was not scanned for viruses before maximum retries attempted" self.report_upload_failure(staging_metadata, err, patient_ods_code) @@ -358,13 +375,13 @@ def rollback_transaction(self): f"Failed to rollback the incomplete transaction due to error: {e}" ) - def report_upload_complete(self, staging_metadata: StagingMetadata, ods_code: str = ""): + def report_upload_complete( + self, staging_metadata: StagingMetadata, ods_code: str = "" + ): nhs_number = staging_metadata.nhs_number for file in staging_metadata.files: dynamo_record = SuccessfulUpload( - nhs_number=nhs_number, - file_path=file.file_path, - ods_code=ods_code + nhs_number=nhs_number, file_path=file.file_path, ods_code=ods_code ) self.dynamo_service.create_item( table_name=self.bulk_upload_report_dynamo_table, @@ -381,7 +398,7 @@ def report_upload_failure( nhs_number=nhs_number, failure_reason=failure_reason, file_path=file.file_path, - ods_code=ods_code + ods_code=ods_code, ) self.dynamo_service.create_item( table_name=self.bulk_upload_report_dynamo_table, diff --git a/lambdas/tests/unit/handlers/test_bulk_upload_handler.py b/lambdas/tests/unit/handlers/test_bulk_upload_handler.py index ec295722b..b4e40e51a 100644 --- a/lambdas/tests/unit/handlers/test_bulk_upload_handler.py +++ b/lambdas/tests/unit/handlers/test_bulk_upload_handler.py @@ -1,9 +1,10 @@ import pytest from handlers.bulk_upload_handler import lambda_handler from tests.unit.helpers.data.bulk_upload.test_data import ( - TEST_EVENT_WITH_10_SQS_MESSAGES, TEST_EVENT_WITH_SQS_MESSAGES) -from utils.exceptions import (InvalidMessageException, - PdsTooManyRequestsException) + TEST_EVENT_WITH_10_SQS_MESSAGES, + TEST_EVENT_WITH_SQS_MESSAGES, +) +from utils.exceptions import InvalidMessageException, PdsTooManyRequestsException @pytest.fixture diff --git a/lambdas/tests/unit/helpers/data/bulk_upload/expected_bulk_upload_report.csv b/lambdas/tests/unit/helpers/data/bulk_upload/expected_bulk_upload_report.csv index ceaf947ba..a6c7ae5e8 100644 --- a/lambdas/tests/unit/helpers/data/bulk_upload/expected_bulk_upload_report.csv +++ b/lambdas/tests/unit/helpers/data/bulk_upload/expected_bulk_upload_report.csv @@ -1,3 +1,3 @@ NhsNumber,UploadStatus,FailureReason,FilePath,Date,Timestamp,ID,OdsCode -9000000009,complete,,/9000000009/1of1_Lloyd_George_Record_[Joe Bloggs]_[9000000009]_[25-12-2019].pdf,2023-10-30,1698661500,1234-4567-8912-HSDF-TEST, +9000000009,complete,,/9000000009/1of1_Lloyd_George_Record_[Joe Bloggs]_[9000000009]_[25-12-2019].pdf,2023-10-30,1698661500,1234-4567-8912-HSDF-TEST,Y12345 9000000025,failed,File name not matching Lloyd George naming convention,/9000000025/invalid_filename.pdf,2023-10-30,1698661500,1234-4567-8912-HSDF-TEST, diff --git a/lambdas/tests/unit/models/test_bulk_upload_status.py b/lambdas/tests/unit/models/test_bulk_upload_status.py index 978232c3f..59a304fbb 100644 --- a/lambdas/tests/unit/models/test_bulk_upload_status.py +++ b/lambdas/tests/unit/models/test_bulk_upload_status.py @@ -9,6 +9,7 @@ "Date": "2023-10-30", "UploadStatus": "complete", "FilePath": "/9000000009/1of1_Lloyd_George_Record_[Joe Bloggs]_[9000000009]_[25-12-2019].pdf", + "OdsCode": "Y12345", } MOCK_FAILURE_REASON = "File name not matching Lloyd George naming convention" @@ -33,6 +34,7 @@ def test_create_successful_upload(): date="2023-10-30", upload_status="complete", file_path="/9000000009/1of1_Lloyd_George_Record_[Joe Bloggs]_[9000000009]_[25-12-2019].pdf", + ods_code="Y12345", ).model_dump(by_alias=True) assert actual == expected @@ -48,7 +50,7 @@ def test_create_failed_upload(): upload_status="failed", failure_reason=MOCK_FAILURE_REASON, file_path="/9000000025/invalid_filename.pdf", - ods_code="" + ods_code="", ).model_dump(by_alias=True) assert actual == expected @@ -62,6 +64,7 @@ def test_successful_upload_ids_and_timestamp_are_auto_populated_if_not_given(moc actual = SuccessfulUpload( nhs_number="9000000009", file_path="/9000000009/1of1_Lloyd_George_Record_[Joe Bloggs]_[9000000009]_[25-12-2019].pdf", + ods_code="Y12345", ).model_dump(by_alias=True) assert actual == expected @@ -76,7 +79,7 @@ def test_failed_upload_ids_and_timestamp_are_auto_populated_if_not_given(mocker) nhs_number="9000000025", file_path="/9000000025/invalid_filename.pdf", failure_reason=MOCK_FAILURE_REASON, - ods_code="" + ods_code="", ).model_dump(by_alias=True) assert actual == expected diff --git a/lambdas/tests/unit/services/test_bulk_upload_report_service.py b/lambdas/tests/unit/services/test_bulk_upload_report_service.py index b3f338926..b85e5244c 100644 --- a/lambdas/tests/unit/services/test_bulk_upload_report_service.py +++ b/lambdas/tests/unit/services/test_bulk_upload_report_service.py @@ -6,14 +6,22 @@ from boto3.dynamodb.conditions import Attr from freezegun import freeze_time from services.bulk_upload_report_service import BulkUploadReportService -from tests.unit.conftest import (MOCK_BULK_REPORT_TABLE_NAME, - MOCK_LG_STAGING_STORE_BUCKET) +from tests.unit.conftest import ( + MOCK_BULK_REPORT_TABLE_NAME, + MOCK_LG_STAGING_STORE_BUCKET, +) from tests.unit.helpers.data.bulk_upload.test_data import readfile from tests.unit.helpers.data.dynamo_scan_response import ( - EXPECTED_RESPONSE, MOCK_EMPTY_RESPONSE, MOCK_RESPONSE, - MOCK_RESPONSE_WITH_LAST_KEY, UNEXPECTED_RESPONSE) + EXPECTED_RESPONSE, + MOCK_EMPTY_RESPONSE, + MOCK_RESPONSE, + MOCK_RESPONSE_WITH_LAST_KEY, + UNEXPECTED_RESPONSE, +) from tests.unit.models.test_bulk_upload_status import ( - MOCK_DATA_COMPLETE_UPLOAD, MOCK_DATA_FAILED_UPLOAD) + MOCK_DATA_COMPLETE_UPLOAD, + MOCK_DATA_FAILED_UPLOAD, +) @pytest.fixture() diff --git a/lambdas/tests/unit/services/test_bulk_upload_service.py b/lambdas/tests/unit/services/test_bulk_upload_service.py index 70ac93488..e5c61ad4a 100644 --- a/lambdas/tests/unit/services/test_bulk_upload_service.py +++ b/lambdas/tests/unit/services/test_bulk_upload_service.py @@ -8,26 +8,42 @@ from models.pds_models import Patient from services.bulk_upload_service import BulkUploadService -from tests.unit.conftest import (MOCK_BULK_REPORT_TABLE_NAME, MOCK_LG_BUCKET, - MOCK_LG_METADATA_SQS_QUEUE, - MOCK_LG_STAGING_STORE_BUCKET, - MOCK_LG_TABLE_NAME, TEST_OBJECT_KEY) +from tests.unit.conftest import ( + MOCK_BULK_REPORT_TABLE_NAME, + MOCK_LG_BUCKET, + MOCK_LG_METADATA_SQS_QUEUE, + MOCK_LG_STAGING_STORE_BUCKET, + MOCK_LG_TABLE_NAME, + TEST_OBJECT_KEY, +) from tests.unit.helpers.data.bulk_upload.test_data import ( - TEST_DOCUMENT_REFERENCE, TEST_DOCUMENT_REFERENCE_LIST, TEST_FILE_METADATA, - TEST_NHS_NUMBER_FOR_BULK_UPLOAD, TEST_SQS_MESSAGE, - TEST_SQS_MESSAGE_WITH_INVALID_FILENAME, TEST_STAGING_METADATA, - TEST_STAGING_METADATA_WITH_INVALID_FILENAME, build_test_sqs_message, - build_test_staging_metadata_from_patient_name, make_s3_file_paths, - make_valid_lg_file_names) -from tests.unit.utils.test_unicode_utils import (NAME_WITH_ACCENT_NFC_FORM, - NAME_WITH_ACCENT_NFD_FORM) + TEST_DOCUMENT_REFERENCE, + TEST_DOCUMENT_REFERENCE_LIST, + TEST_FILE_METADATA, + TEST_NHS_NUMBER_FOR_BULK_UPLOAD, + TEST_SQS_MESSAGE, + TEST_SQS_MESSAGE_WITH_INVALID_FILENAME, + TEST_STAGING_METADATA, + TEST_STAGING_METADATA_WITH_INVALID_FILENAME, + build_test_sqs_message, + build_test_staging_metadata_from_patient_name, + make_s3_file_paths, + make_valid_lg_file_names, +) +from tests.unit.utils.test_unicode_utils import ( + NAME_WITH_ACCENT_NFC_FORM, + NAME_WITH_ACCENT_NFD_FORM, +) from tests.unit.helpers.data.pds.pds_patient_response import PDS_PATIENT -from utils.exceptions import (DocumentInfectedException, - InvalidMessageException, - PatientRecordAlreadyExistException, - S3FileNotFoundException, TagNotFoundException, - VirusScanFailedException, - VirusScanNoResultException) +from utils.exceptions import ( + DocumentInfectedException, + InvalidMessageException, + PatientRecordAlreadyExistException, + S3FileNotFoundException, + TagNotFoundException, + VirusScanFailedException, + VirusScanNoResultException, +) from utils.lloyd_george_validator import LGInvalidFilesException @@ -47,25 +63,37 @@ def mock_check_virus_result(mocker): def mock_validate_files(mocker): yield mocker.patch("services.bulk_upload_service.validate_lg_file_names") + @pytest.fixture def mock_pds_service(mocker): patient = Patient.model_validate(PDS_PATIENT) patient_details = patient.get_minimum_patient_details("9000000009") - mocker.patch("services.bulk_upload_service.getting_patient_info_from_pds", return_value=patient_details) + mocker.patch( + "services.bulk_upload_service.getting_patient_info_from_pds", + return_value=patient_details, + ) yield patient_details + @pytest.fixture def mock_pds_validation(mocker): yield mocker.patch("services.bulk_upload_service.validate_with_pds_service") + def build_resolved_file_names_cache( - file_path_in_metadata: list[str], file_path_in_s3: list[str] + file_path_in_metadata: list[str], file_path_in_s3: list[str] ) -> dict: return dict(zip(file_path_in_metadata, file_path_in_s3)) def test_handle_sqs_message_happy_path( - set_env, mocker, mock_uuid, mock_check_virus_result, mock_validate_files, mock_pds_service, mock_pds_validation + set_env, + mocker, + mock_uuid, + mock_check_virus_result, + mock_validate_files, + mock_pds_service, + mock_pds_validation, ): mock_create_lg_records_and_copy_files = mocker.patch.object( BulkUploadService, "create_lg_records_and_copy_files" @@ -89,7 +117,7 @@ def test_handle_sqs_message_happy_path( def set_up_mocks_for_non_ascii_files( - service: BulkUploadService, mocker, patient_name_on_s3: str + service: BulkUploadService, mocker, patient_name_on_s3: str ): service.s3_service = mocker.MagicMock() service.dynamo_service = mocker.MagicMock() @@ -103,9 +131,9 @@ def mock_file_exist_on_s3(s3_bucket_name: str, file_key: str) -> bool: def mock_get_tag_value(s3_bucket_name: str, file_key: str, tag_key: str) -> str: if ( - s3_bucket_name == MOCK_LG_STAGING_STORE_BUCKET - and tag_key == SCAN_RESULT_TAG_KEY - and file_key in expected_s3_file_paths + s3_bucket_name == MOCK_LG_STAGING_STORE_BUCKET + and tag_key == SCAN_RESULT_TAG_KEY + and file_key in expected_s3_file_paths ): return VirusScanResult.CLEAN @@ -114,12 +142,12 @@ def mock_get_tag_value(s3_bucket_name: str, file_key: str, tag_key: str) -> str: ) def mock_copy_across_bucket( - source_bucket: str, source_file_key: str, dest_bucket: str, **_kwargs + source_bucket: str, source_file_key: str, dest_bucket: str, **_kwargs ): if ( - source_bucket == MOCK_LG_STAGING_STORE_BUCKET - and dest_bucket == MOCK_LG_BUCKET - and source_file_key in expected_s3_file_paths + source_bucket == MOCK_LG_STAGING_STORE_BUCKET + and dest_bucket == MOCK_LG_BUCKET + and source_file_key in expected_s3_file_paths ): return @@ -141,13 +169,13 @@ def mock_copy_across_bucket( ids=["NFC --> NFC", "NFC --> NFD", "NFD --> NFC", "NFD --> NFD"], ) def test_handle_sqs_message_happy_path_with_non_ascii_filenames( - set_env, - mocker, - mock_validate_files, - mock_pds_service, - mock_pds_validation, - patient_name_on_s3, - patient_name_in_metadata_file, + set_env, + mocker, + mock_validate_files, + mock_pds_service, + mock_pds_validation, + patient_name_on_s3, + patient_name_in_metadata_file, ): mock_report_upload_complete = mocker.patch.object( BulkUploadService, "report_upload_complete" @@ -169,7 +197,7 @@ def test_handle_sqs_message_happy_path_with_non_ascii_filenames( def test_handle_sqs_message_calls_report_upload_failure_when_patient_record_already_in_repo( - set_env, mocker, mock_uuid, mock_validate_files, mock_pds_validation + set_env, mocker, mock_uuid, mock_validate_files, mock_pds_validation ): mock_create_lg_records_and_copy_files = mocker.patch.object( BulkUploadService, "create_lg_records_and_copy_files" @@ -200,7 +228,7 @@ def test_handle_sqs_message_calls_report_upload_failure_when_patient_record_alre def test_handle_sqs_message_calls_report_upload_failure_when_lg_file_name_invalid( - set_env, mocker, mock_uuid, mock_validate_files, mock_pds_validation + set_env, mocker, mock_uuid, mock_validate_files, mock_pds_validation ): mock_create_lg_records_and_copy_files = mocker.patch.object( BulkUploadService, "create_lg_records_and_copy_files" @@ -228,7 +256,13 @@ def test_handle_sqs_message_calls_report_upload_failure_when_lg_file_name_invali def test_handle_sqs_message_report_failure_when_document_is_infected( - set_env, mocker, mock_uuid, mock_validate_files, mock_check_virus_result, mock_pds_service, mock_pds_validation + set_env, + mocker, + mock_uuid, + mock_validate_files, + mock_check_virus_result, + mock_pds_service, + mock_pds_validation, ): mock_report_upload_failure = mocker.patch.object( BulkUploadService, "report_upload_failure" @@ -245,14 +279,22 @@ def test_handle_sqs_message_report_failure_when_document_is_infected( service.handle_sqs_message(message=TEST_SQS_MESSAGE) mock_report_upload_failure.assert_called_with( - TEST_STAGING_METADATA, "One or more of the files failed virus scanner check", mock_pds_service.general_practice_ods + TEST_STAGING_METADATA, + "One or more of the files failed virus scanner check", + mock_pds_service.general_practice_ods, ) mock_create_lg_records_and_copy_files.assert_not_called() mock_remove_ingested_file_from_source_bucket.assert_not_called() def test_handle_sqs_message_report_failure_when_document_not_exist( - set_env, mocker, mock_uuid, mock_validate_files, mock_check_virus_result, mock_pds_service, mock_pds_validation + set_env, + mocker, + mock_uuid, + mock_validate_files, + mock_check_virus_result, + mock_pds_service, + mock_pds_validation, ): mock_check_virus_result.side_effect = S3FileNotFoundException mock_report_upload_failure = mocker.patch.object( @@ -267,12 +309,18 @@ def test_handle_sqs_message_report_failure_when_document_not_exist( mock_report_upload_failure.assert_called_with( TEST_STAGING_METADATA, "One or more of the files is not accessible from staging bucket", - mock_pds_service.general_practice_ods + mock_pds_service.general_practice_ods, ) def test_handle_sqs_message_put_staging_metadata_back_to_queue_when_virus_scan_result_not_available( - set_env, mocker, mock_uuid, mock_validate_files, mock_check_virus_result, mock_pds_service, mock_pds_validation + set_env, + mocker, + mock_uuid, + mock_validate_files, + mock_check_virus_result, + mock_pds_service, + mock_pds_validation, ): mock_check_virus_result.side_effect = VirusScanNoResultException mock_report_upload_failure = mocker.patch.object( @@ -291,7 +339,9 @@ def test_handle_sqs_message_put_staging_metadata_back_to_queue_when_virus_scan_r service = BulkUploadService() service.handle_sqs_message(message=TEST_SQS_MESSAGE) - mock_put_staging_metadata_back_to_queue.assert_called_with(TEST_STAGING_METADATA, mock_pds_service.general_practice_ods) + mock_put_staging_metadata_back_to_queue.assert_called_with( + TEST_STAGING_METADATA, mock_pds_service.general_practice_ods + ) mock_report_upload_failure.assert_not_called() mock_create_lg_records_and_copy_files.assert_not_called() @@ -299,7 +349,13 @@ def test_handle_sqs_message_put_staging_metadata_back_to_queue_when_virus_scan_r def test_handle_sqs_message_rollback_transaction_when_validation_pass_but_file_transfer_failed_halfway( - set_env, mocker, mock_uuid, mock_check_virus_result, mock_pds_service, mock_validate_files, mock_pds_validation + set_env, + mocker, + mock_uuid, + mock_check_virus_result, + mock_pds_service, + mock_validate_files, + mock_pds_validation, ): mock_rollback_transaction = mocker.patch.object( BulkUploadService, "rollback_transaction" @@ -328,13 +384,13 @@ def test_handle_sqs_message_rollback_transaction_when_validation_pass_but_file_t mock_report_upload_failure.assert_called_with( TEST_STAGING_METADATA, "Validation passed but error occurred during file transfer", - mock_pds_service.general_practice_ods + mock_pds_service.general_practice_ods, ) mock_remove_ingested_file_from_source_bucket.assert_not_called() def test_handle_sqs_message_raise_InvalidMessageException_when_failed_to_extract_data_from_message( - set_env, mocker + set_env, mocker ): invalid_message = {"body": "invalid content"} mock_create_lg_records_and_copy_files = mocker.patch.object( @@ -349,9 +405,8 @@ def test_handle_sqs_message_raise_InvalidMessageException_when_failed_to_extract def test_validate_files_propagate_PatientRecordAlreadyExistException_when_patient_record_already_in_repo( - set_env, mocker, mock_validate_files + set_env, mocker, mock_validate_files ): - mock_validate_files.side_effect = PatientRecordAlreadyExistException("test text") service = BulkUploadService() service.s3_service = mocker.MagicMock() @@ -362,32 +417,29 @@ def test_validate_files_propagate_PatientRecordAlreadyExistException_when_patien service.handle_sqs_message(message=TEST_SQS_MESSAGE) mock_report_upload_failure.assert_called_with( - TEST_STAGING_METADATA, - "test text", - "" + TEST_STAGING_METADATA, "test text", "" ) def test_validate_files_raise_LGInvalidFilesException_when_file_names_invalid( - set_env, mocker, mock_validate_files + set_env, mocker, mock_validate_files ): service = BulkUploadService() service.s3_service = mocker.MagicMock() service.dynamo_service = mocker.MagicMock() - mock_validate_files.side_effect=LGInvalidFilesException("test text") + mock_validate_files.side_effect = LGInvalidFilesException("test text") mock_report_upload_failure = mocker.patch.object( BulkUploadService, "report_upload_failure" ) service.handle_sqs_message(message=TEST_SQS_MESSAGE) mock_report_upload_failure.assert_called_with( - TEST_STAGING_METADATA, - "test text", - "" + TEST_STAGING_METADATA, "test text", "" ) + def test_check_virus_result_raise_no_error_when_all_files_are_clean( - set_env, mocker, caplog + set_env, mocker, caplog ): service = BulkUploadService() service.s3_service = mocker.MagicMock() @@ -402,7 +454,7 @@ def test_check_virus_result_raise_no_error_when_all_files_are_clean( def test_check_virus_result_raise_VirusScanNoResultException_when_one_file_not_scanned( - set_env, mocker + set_env, mocker ): service = BulkUploadService() service.s3_service = mocker.MagicMock() @@ -419,7 +471,7 @@ def test_check_virus_result_raise_VirusScanNoResultException_when_one_file_not_s def test_check_virus_result_raise_DocumentInfectedException_when_one_file_was_infected( - set_env, mocker + set_env, mocker ): service = BulkUploadService() service.s3_service = mocker.MagicMock() @@ -436,7 +488,7 @@ def test_check_virus_result_raise_DocumentInfectedException_when_one_file_was_in def test_check_virus_result_raise_S3FileNotFoundException_when_one_file_not_exist_in_bucket( - set_env, mocker + set_env, mocker ): service = BulkUploadService() service.s3_service = mocker.MagicMock() @@ -458,7 +510,7 @@ def test_check_virus_result_raise_S3FileNotFoundException_when_one_file_not_exis def test_check_virus_result_raise_VirusScanFailedException_for_special_cases( - set_env, mocker + set_env, mocker ): service = BulkUploadService() service.s3_service = mocker.MagicMock() @@ -495,13 +547,16 @@ def test_put_staging_metadata_back_to_queue_and_increases_retries(set_env, mocke nhs_number=TEST_STAGING_METADATA.nhs_number, ) + @freeze_time("2023-10-2 13:00:00") def test_reports_failure_when_max_retries_reached(set_env, mocker, mock_uuid): service = BulkUploadService() service.sqs_service = mocker.MagicMock() service.dynamo_service = mocker.MagicMock() - mock_failure_reason = "File was not scanned for viruses before maximum retries attempted" + mock_failure_reason = ( + "File was not scanned for viruses before maximum retries attempted" + ) TEST_STAGING_METADATA.retries = 15 mocker.patch("uuid.uuid4", return_value="123412342") @@ -519,7 +574,7 @@ def test_reports_failure_when_max_retries_reached(set_env, mocker, mock_uuid): "Timestamp": 1696251600, "UploadStatus": "failed", "FailureReason": mock_failure_reason, - "OdsCode": "test_ods" + "OdsCode": "test_ods", } service.dynamo_service.create_item.assert_any_call( item=expected_dynamo_db_record, table_name=MOCK_BULK_REPORT_TABLE_NAME @@ -550,7 +605,7 @@ def test_resolve_source_file_path_when_filenames_dont_have_accented_chars(set_en ids=["NFC --> NFC", "NFC --> NFD", "NFD --> NFC", "NFD --> NFD"], ) def test_resolve_source_file_path_when_filenames_have_accented_chars( - set_env, mocker, patient_name_on_s3, patient_name_in_metadata_file + set_env, mocker, patient_name_on_s3, patient_name_in_metadata_file ): service = BulkUploadService() @@ -574,7 +629,7 @@ def test_resolve_source_file_path_when_filenames_have_accented_chars( def test_resolve_source_file_path_raise_S3FileNotFoundException_if_filename_cant_match( - set_env, mocker + set_env, mocker ): service = BulkUploadService() patient_name_on_s3 = "Some Name That Not Matching Metadata File" @@ -633,7 +688,7 @@ def test_create_lg_records_and_copy_files(set_env, mocker, mock_uuid): def test_create_lg_records_and_copy_files_keep_track_of_successfully_ingested_files( - set_env, mocker, mock_uuid + set_env, mocker, mock_uuid ): service = BulkUploadService() service.s3_service = mocker.MagicMock() @@ -739,6 +794,7 @@ def test_report_upload_complete_add_record_to_dynamodb(set_env, mocker, mock_uui "NhsNumber": TEST_STAGING_METADATA.nhs_number, "Timestamp": 1696165200, "UploadStatus": "complete", + "OdsCode": "", } service.dynamo_service.create_item.assert_any_call( item=expected_dynamo_db_record, table_name=MOCK_BULK_REPORT_TABLE_NAME @@ -764,7 +820,7 @@ def test_report_upload_failure_add_record_to_dynamodb(set_env, mocker, mock_uuid "Timestamp": 1696251600, "UploadStatus": "failed", "FailureReason": mock_failure_reason, - "OdsCode": "" + "OdsCode": "", } service.dynamo_service.create_item.assert_any_call( item=expected_dynamo_db_record, table_name=MOCK_BULK_REPORT_TABLE_NAME diff --git a/lambdas/tests/unit/utils/test_lloyd_george_validator.py b/lambdas/tests/unit/utils/test_lloyd_george_validator.py index 1f6764e6c..d7f379baa 100644 --- a/lambdas/tests/unit/utils/test_lloyd_george_validator.py +++ b/lambdas/tests/unit/utils/test_lloyd_george_validator.py @@ -10,15 +10,24 @@ from tests.unit.conftest import TEST_NHS_NUMBER from tests.unit.helpers.data.pds.pds_patient_response import PDS_PATIENT from tests.unit.models.test_document_reference import MOCK_DOCUMENT_REFERENCE -from utils.exceptions import (PatientRecordAlreadyExistException, - PdsTooManyRequestsException) +from utils.exceptions import ( + PatientRecordAlreadyExistException, + PdsTooManyRequestsException, +) from utils.lloyd_george_validator import ( - LGInvalidFilesException, check_for_duplicate_files, + LGInvalidFilesException, + check_for_duplicate_files, check_for_file_names_agrees_with_each_other, check_for_number_of_files_match_expected, - check_for_patient_already_exist_in_repo, extract_info_from_filename, - validate_file_name, validate_lg_file_type, validate_with_pds_service, getting_patient_info_from_pds, - validate_lg_file_names) + check_for_patient_already_exist_in_repo, + extract_info_from_filename, + validate_file_name, + validate_lg_file_type, + validate_with_pds_service, + getting_patient_info_from_pds, + validate_lg_file_names, +) + @pytest.fixture() def mock_pds_patient_details(): @@ -26,6 +35,7 @@ def mock_pds_patient_details(): patient_details = patient.get_minimum_patient_details("9000000009") return patient_details + def test_catching_error_when_file_type_not_pdf(): with pytest.raises(LGInvalidFilesException): file_type = "image/png" @@ -241,7 +251,9 @@ def test_mismatch_nhs_id_no_pds_service(mocker): ] mocker.patch("utils.lloyd_george_validator.check_for_patient_already_exist_in_repo") - mocker.patch("utils.lloyd_george_validator.check_for_number_of_files_match_expected") + mocker.patch( + "utils.lloyd_george_validator.check_for_number_of_files_match_expected" + ) mocker.patch("utils.lloyd_george_validator.validate_file_name") with pytest.raises(LGInvalidFilesException): @@ -280,7 +292,6 @@ def test_mismatch_ods_with_pds_service(mocker, mock_pds_patient_details): def test_mismatch_dob_with_pds_service(mocker, mock_pds_patient_details): - lg_file_list = [ "1of2_Lloyd_George_Record_[Jane Plain Smith]_[9000000009]_[14-01-2000].pdf", "2of2_Lloyd_George_Record_[Jane Plain Smith]_[9000000009]_[14-01-2000].pdf", @@ -298,7 +309,7 @@ def test_patient_not_found_with_pds_service(mock_pds_call): response.status_code = 404 mock_pds_call.return_value = response - with pytest.raises(HTTPError): + with pytest.raises(LGInvalidFilesException): getting_patient_info_from_pds("9000000009") mock_pds_call.assert_called_with(nhs_number="9000000009", retry_on_expired=True) @@ -308,7 +319,7 @@ def test_bad_request_with_pds_service(mock_pds_patient_details, mock_pds_call): response.status_code = 400 mock_pds_call.return_value = response - with pytest.raises(HTTPError): + with pytest.raises(LGInvalidFilesException): getting_patient_info_from_pds("9000000009") mock_pds_call.assert_called_with(nhs_number="9000000009", retry_on_expired=True) diff --git a/lambdas/utils/lloyd_george_validator.py b/lambdas/utils/lloyd_george_validator.py index c85a580b8..2d8903288 100644 --- a/lambdas/utils/lloyd_george_validator.py +++ b/lambdas/utils/lloyd_george_validator.py @@ -12,8 +12,10 @@ from services.document_service import DocumentService from services.ssm_service import SSMService from utils.audit_logging_setup import LoggingService -from utils.exceptions import (PatientRecordAlreadyExistException, - PdsTooManyRequestsException) +from utils.exceptions import ( + PatientRecordAlreadyExistException, + PdsTooManyRequestsException, +) from utils.unicode_utils import REGEX_PATIENT_NAME_PATTERN, names_are_matching from utils.utilities import get_pds_service @@ -98,6 +100,7 @@ def validate_lg_file_names(file_name_list: list[str], nhs_number: str): check_for_duplicate_files(file_name_list) check_for_file_names_agrees_with_each_other(file_name_list) + def extract_info_from_filename(filename: str) -> dict: page_number = r"(?P[1-9][0-9]*)" total_page_number = r"(?P[1-9][0-9]*)" @@ -124,7 +127,9 @@ def check_for_file_names_agrees_with_each_other(file_name_list: list[str]): raise LGInvalidFilesException("File names does not match with each other") -def validate_with_pds_service(file_name_list: list[str], patient_details: PatientDetails): +def validate_with_pds_service( + file_name_list: list[str], patient_details: PatientDetails +): try: file_name_info = extract_info_from_filename(file_name_list[0]) @@ -151,31 +156,31 @@ def validate_with_pds_service(file_name_list: list[str], patient_details: Patien except (ValidationError, ClientError, ValueError) as e: logger.error(e) raise LGInvalidFilesException(e) - except HTTPError as e: - logger.error(e) - if "404" in str(e): - raise LGInvalidFilesException("Could not find the given patient on PDS") - else: - raise LGInvalidFilesException("Failed to retrieve patient data from PDS") + def getting_patient_info_from_pds(nhs_number: str): pds_service_class = get_pds_service() pds_service = pds_service_class(SSMService()) - pds_response = pds_service.pds_request( - nhs_number=nhs_number, retry_on_expired=True - ) + pds_response = pds_service.pds_request(nhs_number=nhs_number, retry_on_expired=True) if pds_response.status_code == 429: logger.error("Got 429 Too Many Requests error from PDS.") raise PdsTooManyRequestsException( "Failed to validate filename against PDS record due to too many requests" ) - - pds_response.raise_for_status() + try: + pds_response.raise_for_status() + except HTTPError as e: + logger.error(e) + if "404" in str(e): + raise LGInvalidFilesException("Could not find the given patient on PDS") + else: + raise LGInvalidFilesException("Failed to retrieve patient data from PDS") patient = Patient.model_validate(pds_response.json()) patient_details = patient.get_minimum_patient_details(nhs_number) return patient_details + def get_user_ods_code(): if os.getenv("PDS_FHIR_IS_STUBBED") in ["True", "true"]: return "Y12345"