From 9c9e2b54505d65ca28d91bde7e61ded9fe30700f Mon Sep 17 00:00:00 2001 From: NogaNHS Date: Fri, 12 Jan 2024 14:29:33 +0000 Subject: [PATCH] ODS code added to bulk upload --- lambdas/services/bulk_upload_service.py | 19 ++- .../unit/services/test_bulk_upload_service.py | 68 +++++--- .../unit/utils/test_lloyd_george_validator.py | 157 ++++++++---------- lambdas/utils/lloyd_george_validator.py | 2 +- 4 files changed, 127 insertions(+), 119 deletions(-) diff --git a/lambdas/services/bulk_upload_service.py b/lambdas/services/bulk_upload_service.py index 9122d6fed..283d1b7de 100644 --- a/lambdas/services/bulk_upload_service.py +++ b/lambdas/services/bulk_upload_service.py @@ -52,6 +52,8 @@ def __init__( self.file_path_cache = {} def handle_sqs_message(self, message: dict): + patient_ods_code = "" + try: logger.info("Parsing message from sqs...") staging_metadata_json = message["body"] @@ -71,6 +73,7 @@ def handle_sqs_message(self, message: dict): ] validate_lg_file_names(file_names, 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) except PdsTooManyRequestsException as error: @@ -86,9 +89,10 @@ 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) + self.report_upload_failure(staging_metadata, failure_reason, patient_ods_code) return + logger.info("File validation complete. Checking virus scan results") try: @@ -101,7 +105,7 @@ def handle_sqs_message(self, message: dict): logger.info( f"Waiting on virus scan results for: {staging_metadata.nhs_number}, adding message back to queue" ) - self.put_staging_metadata_back_to_queue(staging_metadata) + self.put_staging_metadata_back_to_queue(staging_metadata, patient_ods_code) return except (VirusScanFailedException, DocumentInfectedException) as e: logger.info(e) @@ -111,7 +115,7 @@ 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" + staging_metadata, "One or more of the files failed virus scanner check", patient_ods_code ) return except S3FileNotFoundException as e: @@ -124,6 +128,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 ) return @@ -150,6 +155,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 ) return @@ -201,10 +207,10 @@ 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): + 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) + self.report_upload_failure(staging_metadata, err, patient_ods_code) return request_context.patient_nhs_no = staging_metadata.nhs_number setattr(staging_metadata, "retries", (staging_metadata.retries + 1)) @@ -352,12 +358,13 @@ def rollback_transaction(self): f"Failed to rollback the incomplete transaction due to error: {e}" ) - def report_upload_complete(self, staging_metadata: StagingMetadata): + 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 ) self.dynamo_service.create_item( table_name=self.bulk_upload_report_dynamo_table, diff --git a/lambdas/tests/unit/services/test_bulk_upload_service.py b/lambdas/tests/unit/services/test_bulk_upload_service.py index ca798322d..70ac93488 100644 --- a/lambdas/tests/unit/services/test_bulk_upload_service.py +++ b/lambdas/tests/unit/services/test_bulk_upload_service.py @@ -5,6 +5,8 @@ from botocore.exceptions import ClientError from enums.virus_scan_result import SCAN_RESULT_TAG_KEY, VirusScanResult from freezegun import freeze_time + +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, @@ -19,6 +21,7 @@ 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, @@ -46,7 +49,10 @@ def mock_validate_files(mocker): @pytest.fixture def mock_pds_service(mocker): - yield mocker.patch("services.bulk_upload_service.getting_patient_info_from_pds") + 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) + yield patient_details @pytest.fixture def mock_pds_validation(mocker): @@ -163,7 +169,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_service, 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" @@ -181,18 +187,20 @@ def test_handle_sqs_message_calls_report_upload_failure_when_patient_record_alre mock_validate_files.side_effect = mocked_error service = BulkUploadService() + service.s3_service = mocker.MagicMock() + service.handle_sqs_message(message=TEST_SQS_MESSAGE) mock_create_lg_records_and_copy_files.assert_not_called() mock_remove_ingested_file_from_source_bucket.assert_not_called() mock_report_upload_failure.assert_called_with( - TEST_STAGING_METADATA, str(mocked_error) + TEST_STAGING_METADATA, str(mocked_error), "" ) def test_handle_sqs_message_calls_report_upload_failure_when_lg_file_name_invalid( - set_env, mocker, mock_uuid, mock_validate_files, mock_pds_service, 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" @@ -215,7 +223,7 @@ def test_handle_sqs_message_calls_report_upload_failure_when_lg_file_name_invali mock_remove_ingested_file_from_source_bucket.assert_not_called() mock_report_upload_failure.assert_called_with( - TEST_STAGING_METADATA_WITH_INVALID_FILENAME, str(mocked_error) + TEST_STAGING_METADATA_WITH_INVALID_FILENAME, str(mocked_error), "" ) @@ -237,7 +245,7 @@ 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" + 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() @@ -252,11 +260,14 @@ def test_handle_sqs_message_report_failure_when_document_not_exist( ) service = BulkUploadService() + service.s3_service = mocker.MagicMock() + service.dynamo_service = mocker.MagicMock() service.handle_sqs_message(message=TEST_SQS_MESSAGE) 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 ) @@ -280,7 +291,7 @@ 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_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() @@ -317,6 +328,7 @@ 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_remove_ingested_file_from_source_bucket.assert_not_called() @@ -337,34 +349,42 @@ 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 + set_env, mocker, mock_validate_files ): - mocker.patch( - "utils.lloyd_george_validator.check_for_patient_already_exist_in_repo", - side_effect=PatientRecordAlreadyExistException, - ) + + mock_validate_files.side_effect = PatientRecordAlreadyExistException("test text") service = BulkUploadService() service.s3_service = mocker.MagicMock() service.dynamo_service = mocker.MagicMock() + mock_report_upload_failure = mocker.patch.object( + BulkUploadService, "report_upload_failure" + ) + service.handle_sqs_message(message=TEST_SQS_MESSAGE) - with pytest.raises(PatientRecordAlreadyExistException): - service.handle_sqs_message(message=TEST_SQS_MESSAGE) + mock_report_upload_failure.assert_called_with( + TEST_STAGING_METADATA, + "test text", + "" + ) def test_validate_files_raise_LGInvalidFilesException_when_file_names_invalid( - set_env, mocker + set_env, mocker, mock_validate_files ): service = BulkUploadService() service.s3_service = mocker.MagicMock() service.dynamo_service = mocker.MagicMock() - mocker.patch( - "utils.lloyd_george_validator.check_for_patient_already_exist_in_repo", - side_effect=LGInvalidFilesException, + 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) - with pytest.raises(LGInvalidFilesException): - service.handle_sqs_message(message=TEST_SQS_MESSAGE) - + mock_report_upload_failure.assert_called_with( + TEST_STAGING_METADATA, + "test text", + "" + ) def test_check_virus_result_raise_no_error_when_all_files_are_clean( set_env, mocker, caplog @@ -466,7 +486,7 @@ def test_put_staging_metadata_back_to_queue_and_increases_retries(set_env, mocke metadata_copy = copy.deepcopy(TEST_STAGING_METADATA) metadata_copy.retries = 3 - service.put_staging_metadata_back_to_queue(TEST_STAGING_METADATA) + service.put_staging_metadata_back_to_queue(TEST_STAGING_METADATA, "") service.sqs_service.send_message_with_nhs_number_attr_fifo.assert_called_with( group_id="back_to_queue_bulk_upload_123412342", @@ -486,7 +506,7 @@ def test_reports_failure_when_max_retries_reached(set_env, mocker, mock_uuid): mocker.patch("uuid.uuid4", return_value="123412342") - service.put_staging_metadata_back_to_queue(TEST_STAGING_METADATA) + service.put_staging_metadata_back_to_queue(TEST_STAGING_METADATA, "test_ods") service.sqs_service.send_message_with_nhs_number_attr_fifo.assert_not_called() @@ -499,7 +519,7 @@ def test_reports_failure_when_max_retries_reached(set_env, mocker, mock_uuid): "Timestamp": 1696251600, "UploadStatus": "failed", "FailureReason": mock_failure_reason, - "OdsCode": "" + "OdsCode": "test_ods" } 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 7dfe419a4..1f6764e6c 100644 --- a/lambdas/tests/unit/utils/test_lloyd_george_validator.py +++ b/lambdas/tests/unit/utils/test_lloyd_george_validator.py @@ -3,7 +3,7 @@ import pytest from botocore.exceptions import ClientError from enums.supported_document_types import SupportedDocumentTypes -from requests import Response +from requests import Response, HTTPError from models.pds_models import Patient from services.document_service import DocumentService @@ -17,7 +17,8 @@ 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) + 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(): @@ -212,42 +213,39 @@ def test_files_for_different_patients(): check_for_file_names_agrees_with_each_other(lg_file_list) assert str(e.value) == "File names does not match with each other" -# -# def test_validate_nhs_id_with_pds_service(mocker, mock_pds_call): -# response = Response() -# response.status_code = 200 -# response._content = json.dumps(PDS_PATIENT).encode("utf-8") -# patient = Patient.model_validate(response.json()) -# patient_details = patient.get_minimum_patient_details("9000000009") -# lg_file_list = [ -# "1of2_Lloyd_George_Record_[Jane Smith]_[9000000009]_[22-10-2010].pdf", -# "2of2_Lloyd_George_Record_[Jane Smith]_[9000000009]_[22-10-2010].pdf", -# ] -# mock_pds_call.return_value = response -# mock_odc_code = mocker.patch( -# "utils.lloyd_george_validator.get_user_ods_code", return_value="Y12345" -# ) -# -# validate_with_pds_service(lg_file_list, patient_details) -# -# mock_pds_call.assert_called_with(nhs_number="9000000009", retry_on_expired=True) -# mock_odc_code.assert_called_once() - - -# def test_mismatch_nhs_id_no_pds_service(mocker, mock_pds_call): -# lg_file_list = [ -# "1of2_Lloyd_George_Record_[Jane Smith]_[9000000005]_[22-10-2010].pdf", -# "2of2_Lloyd_George_Record_[Jane Smith]_[9000000005]_[22-10-2010].pdf", -# ] -# mock_odc_code = mocker.patch( -# "utils.lloyd_george_validator.get_user_ods_code", return_value="Y12345" -# ) -# -# with pytest.raises(LGInvalidFilesException): -# validate_with_pds_service(lg_file_list, "9000000009") -# -# mock_pds_call.assert_not_called() -# mock_odc_code.assert_not_called() + +def test_validate_nhs_id_with_pds_service(mocker): + response = Response() + response.status_code = 200 + response._content = json.dumps(PDS_PATIENT).encode("utf-8") + patient = Patient.model_validate(response.json()) + patient_details = patient.get_minimum_patient_details("9000000009") + lg_file_list = [ + "1of2_Lloyd_George_Record_[Jane Smith]_[9000000009]_[22-10-2010].pdf", + "2of2_Lloyd_George_Record_[Jane Smith]_[9000000009]_[22-10-2010].pdf", + ] + mock_pds_call.return_value = response + mock_odc_code = mocker.patch( + "utils.lloyd_george_validator.get_user_ods_code", return_value="Y12345" + ) + + validate_with_pds_service(lg_file_list, patient_details) + + mock_odc_code.assert_called_once() + + +def test_mismatch_nhs_id_no_pds_service(mocker): + lg_file_list = [ + "1of2_Lloyd_George_Record_[Jane Smith]_[9000000005]_[22-10-2010].pdf", + "2of2_Lloyd_George_Record_[Jane Smith]_[9000000005]_[22-10-2010].pdf", + ] + + 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.validate_file_name") + + with pytest.raises(LGInvalidFilesException): + validate_lg_file_names(lg_file_list, "9000000009") def test_mismatch_name_with_pds_service(mocker, mock_pds_patient_details): @@ -295,38 +293,25 @@ def test_mismatch_dob_with_pds_service(mocker, mock_pds_patient_details): mock_odc_code.assert_not_called() -# def test_patient_not_found_with_pds_service(mocker, mock_pds_patient_details): -# response = Response() -# response.status_code = 404 -# lg_file_list = [ -# "1of2_Lloyd_George_Record_[Jane Smith]_[9000000009]_[22-10-2010].pdf", -# "2of2_Lloyd_George_Record_[Jane Smith]_[9000000009]_[22-10-2010].pdf", -# ] -# mock_odc_code = mocker.patch("utils.lloyd_george_validator.get_user_ods_code") -# -# with pytest.raises(LGInvalidFilesException) as e: -# validate_with_pds_service(lg_file_list, mock_pds_patient_details) -# assert str(e.value) == "Could not find the given patient on PDS" -# -# mock_odc_code.assert_not_called() - -# -# def test_bad_request_with_pds_service(mocker, mock_pds_patient_details): -# response = Response() -# response.status_code = 400 -# lg_file_list = [ -# "1of2_Lloyd_George_Record_[Jane Plain Smith]_[9000000009]_[22-10-2010].pdf", -# "2of2_Lloyd_George_Record_[Jane Plain Smith]_[9000000009]_[22-10-2010].pdf", -# ] -# mock_pds_call.return_value = response -# mock_odc_code = mocker.patch("utils.lloyd_george_validator.get_user_ods_code") -# -# with pytest.raises(LGInvalidFilesException) as e: -# validate_with_pds_service(lg_file_list, mock_pds_patient_details) -# assert str(e.value) == "Failed to retrieve patient data from PDS" -# -# mock_pds_call.assert_called_with(nhs_number="9000000009", retry_on_expired=True) -# mock_odc_code.assert_not_called() +def test_patient_not_found_with_pds_service(mock_pds_call): + response = Response() + response.status_code = 404 + mock_pds_call.return_value = response + + with pytest.raises(HTTPError): + getting_patient_info_from_pds("9000000009") + mock_pds_call.assert_called_with(nhs_number="9000000009", retry_on_expired=True) + + +def test_bad_request_with_pds_service(mock_pds_patient_details, mock_pds_call): + response = Response() + response.status_code = 400 + mock_pds_call.return_value = response + + with pytest.raises(HTTPError): + getting_patient_info_from_pds("9000000009") + + mock_pds_call.assert_called_with(nhs_number="9000000009", retry_on_expired=True) def test_raise_client_error_from_ssm_with_pds_service(mocker, mock_pds_patient_details): @@ -347,24 +332,20 @@ def test_raise_client_error_from_ssm_with_pds_service(mocker, mock_pds_patient_d mock_odc_code.assert_called_once() -# def test_validate_with_pds_service_raise_PdsTooManyRequestsException( -# mocker, mock_pds_call -# ): -# response = Response() -# response.status_code = 429 -# response._content = b"Too Many Requests" -# lg_file_list = [ -# "1of2_Lloyd_George_Record_[Jane Smith]_[9000000009]_[22-10-2010].pdf", -# "2of2_Lloyd_George_Record_[Jane Smith]_[9000000009]_[22-10-2010].pdf", -# ] -# mock_pds_call.return_value = response -# -# mocker.patch("utils.lloyd_george_validator.get_user_ods_code") -# -# with pytest.raises(PdsTooManyRequestsException): -# validate_with_pds_service(lg_file_list, "9000000009") -# -# mock_pds_call.assert_called_with(nhs_number="9000000009", retry_on_expired=True) +def test_validate_with_pds_service_raise_PdsTooManyRequestsException( + mocker, mock_pds_call +): + response = Response() + response.status_code = 429 + response._content = b"Too Many Requests" + mock_pds_call.return_value = response + + mocker.patch("utils.lloyd_george_validator.get_user_ods_code") + + with pytest.raises(PdsTooManyRequestsException): + getting_patient_info_from_pds("9000000009") + + mock_pds_call.assert_called_with(nhs_number="9000000009", retry_on_expired=True) def test_check_for_patient_already_exist_in_repo_return_none_when_patient_record_not_exist( diff --git a/lambdas/utils/lloyd_george_validator.py b/lambdas/utils/lloyd_george_validator.py index ae25cb4e3..c85a580b8 100644 --- a/lambdas/utils/lloyd_george_validator.py +++ b/lambdas/utils/lloyd_george_validator.py @@ -146,7 +146,7 @@ def validate_with_pds_service(file_name_list: list[str], patient_details: Patien current_user_ods = get_user_ods_code() if patient_details.general_practice_ods != current_user_ods: - raise LGInvalidFilesException("User is not allowed to access patient") + raise LGInvalidFilesException("Patient not registered at your practice") except (ValidationError, ClientError, ValueError) as e: logger.error(e)