From f21ddcf6ff9f96fb446ef184fe6ff52c2d1404dd Mon Sep 17 00:00:00 2001 From: NogaNHS <127490765+NogaNHS@users.noreply.github.com> Date: Thu, 2 May 2024 14:19:52 +0100 Subject: [PATCH] PRMDR 0000 - bugs fixes (#358) * LG validator changes following pilot bugs --- lambdas/models/pds_models.py | 31 +-- lambdas/scripts/batch_update_ods_code.py | 6 +- .../create_document_reference_service.py | 6 +- .../document_reference_search_service.py | 12 +- .../helpers/data/pds/pds_patient_response.py | 7 + lambdas/tests/unit/models/test_pds_models.py | 21 ++ .../unit/utils/test_lloyd_george_validator.py | 188 ++++++++++++++++-- lambdas/utils/lloyd_george_validator.py | 58 ++++-- sonar-project.properties | 2 +- 9 files changed, 271 insertions(+), 60 deletions(-) diff --git a/lambdas/models/pds_models.py b/lambdas/models/pds_models.py index ae55779bf..6aa54eaed 100644 --- a/lambdas/models/pds_models.py +++ b/lambdas/models/pds_models.py @@ -14,14 +14,14 @@ class Address(BaseModel): model_config = conf use: str - period: Period + period: Optional[Period] = None postal_code: Optional[str] = "" class Name(BaseModel): use: str - period: Period - given: list[str] + period: Optional[Period] = None + given: Optional[list[str]] = None family: str @@ -36,21 +36,21 @@ class Meta(BaseModel): class GPIdentifier(BaseModel): - system: Optional[str] + system: Optional[str] = "" value: str - period: Optional[Period] + period: Optional[Period] = None class GeneralPractitioner(BaseModel): - id: Optional[str] - type: Optional[str] + id: Optional[str] = "" + type: Optional[str] = "" identifier: GPIdentifier class PatientDetails(BaseModel): model_config = conf - given_Name: Optional[list[str]] = [] + given_name: Optional[list[str]] = [""] family_name: Optional[str] = "" birth_date: Optional[date] = None postal_code: Optional[str] = "" @@ -69,7 +69,7 @@ class Patient(BaseModel): address: Optional[list[Address]] = [] name: list[Name] meta: Meta - general_practitioner: Optional[list[GeneralPractitioner]] = [] + general_practitioner: Optional[list[GeneralPractitioner]] = None def get_security(self) -> Security: security = self.meta.security[0] if self.meta.security[0] else None @@ -90,16 +90,17 @@ def get_current_usual_name(self) -> [Optional[Name]]: return entry def get_current_home_address(self) -> Optional[Address]: - if self.is_unrestricted(): + if self.is_unrestricted() and self.address: for entry in self.address: if entry.use.lower() == "home": return entry def get_active_ods_code_for_gp(self) -> str: - for entry in self.general_practitioner: - gp_end_date = entry.identifier.period.end - if not gp_end_date or gp_end_date >= date.today(): - return entry.identifier.value + if self.general_practitioner: + for entry in self.general_practitioner: + gp_end_date = entry.identifier.period.end + if not gp_end_date or gp_end_date >= date.today(): + return entry.identifier.value return "" def get_is_active_status(self) -> bool: @@ -113,7 +114,7 @@ def get_patient_details(self, nhs_number) -> PatientDetails: birthDate=self.birth_date, postalCode=( self.get_current_home_address().postal_code - if self.is_unrestricted() + if self.is_unrestricted() and self.address else "" ), nhsNumber=self.id, diff --git a/lambdas/scripts/batch_update_ods_code.py b/lambdas/scripts/batch_update_ods_code.py index c0626d179..596a9fd4a 100644 --- a/lambdas/scripts/batch_update_ods_code.py +++ b/lambdas/scripts/batch_update_ods_code.py @@ -192,9 +192,9 @@ def build_progress_dict(self, dynamodb_records: list[dict]) -> dict: progress_dict[nhs_number].doc_ref_ids.append(doc_ref_id) pds_code_at_current_row = ods_code if progress_dict[nhs_number].prev_ods_code != pds_code_at_current_row: - progress_dict[nhs_number].prev_ods_code = ( - "[multiple ods codes in records]" - ) + progress_dict[ + nhs_number + ].prev_ods_code = "[multiple ods codes in records]" self.logger.info(f"Totally {len(progress_dict)} patients found in record.") return progress_dict diff --git a/lambdas/services/create_document_reference_service.py b/lambdas/services/create_document_reference_service.py index c26c3ffa4..8a989f70b 100644 --- a/lambdas/services/create_document_reference_service.py +++ b/lambdas/services/create_document_reference_service.py @@ -70,9 +70,9 @@ def create_document_reference_request( 400, LambdaError.CreateDocInvalidType ) - url_responses[document_reference.file_name] = ( - self.prepare_pre_signed_url(document_reference) - ) + url_responses[ + document_reference.file_name + ] = self.prepare_pre_signed_url(document_reference) if lg_documents: validate_lg_files(lg_documents, nhs_number) diff --git a/lambdas/services/document_reference_search_service.py b/lambdas/services/document_reference_search_service.py index 9f1315537..6ce6f4232 100644 --- a/lambdas/services/document_reference_search_service.py +++ b/lambdas/services/document_reference_search_service.py @@ -33,12 +33,12 @@ def get_document_references(self, nhs_number: str): for table_name in list_of_table_names: logger.info(f"Searching for results in {table_name}") - documents: list[DocumentReference] = ( - self.fetch_documents_from_table_with_filter( - nhs_number, - table_name, - query_filter=delete_filter_expression, - ) + documents: list[ + DocumentReference + ] = self.fetch_documents_from_table_with_filter( + nhs_number, + table_name, + query_filter=delete_filter_expression, ) results.extend( diff --git a/lambdas/tests/unit/helpers/data/pds/pds_patient_response.py b/lambdas/tests/unit/helpers/data/pds/pds_patient_response.py index acefdd7a4..2eb44a5b5 100644 --- a/lambdas/tests/unit/helpers/data/pds/pds_patient_response.py +++ b/lambdas/tests/unit/helpers/data/pds/pds_patient_response.py @@ -1,3 +1,5 @@ +import copy + PDS_PATIENT = { "resourceType": "Patient", "id": "9000000009", @@ -524,3 +526,8 @@ }, }, } +PDS_PATIENT_WITH_MIDDLE_NAME = copy.deepcopy(PDS_PATIENT) +PDS_PATIENT_WITH_MIDDLE_NAME["name"][0]["given"].append("Jake") + +PDS_PATIENT_WITHOUT_ADDRESS = copy.deepcopy(PDS_PATIENT) +PDS_PATIENT_WITHOUT_ADDRESS.pop("address") diff --git a/lambdas/tests/unit/models/test_pds_models.py b/lambdas/tests/unit/models/test_pds_models.py index b4e8b304b..4e0d73287 100644 --- a/lambdas/tests/unit/models/test_pds_models.py +++ b/lambdas/tests/unit/models/test_pds_models.py @@ -5,6 +5,7 @@ PDS_PATIENT_RESTRICTED, PDS_PATIENT_WITH_GP_END_DATE, PDS_PATIENT_WITHOUT_ACTIVE_GP, + PDS_PATIENT_WITHOUT_ADDRESS, ) from tests.unit.helpers.data.pds.utils import create_patient from utils.utilities import validate_nhs_number @@ -109,3 +110,23 @@ def test_not_raise_error_when_gp_end_date_is_in_the_future(): patient.get_minimum_patient_details(patient.id) except ValueError: assert False, "No active GP practice for the patient" + + +def test_get_minimum_patient_details_missing_address(): + patient = create_patient(PDS_PATIENT_WITHOUT_ADDRESS) + + expected_patient_details = PatientDetails( + givenName=["Jane"], + familyName="Smith", + birthDate="2010-10-22", + postalCode="", + nhsNumber="9000000009", + superseded=False, + restricted=False, + generalPracticeOds="Y12345", + active=True, + ) + + result = patient.get_patient_details(patient.id) + + assert expected_patient_details == result diff --git a/lambdas/tests/unit/utils/test_lloyd_george_validator.py b/lambdas/tests/unit/utils/test_lloyd_george_validator.py index 0b941c86d..4f77a47e7 100644 --- a/lambdas/tests/unit/utils/test_lloyd_george_validator.py +++ b/lambdas/tests/unit/utils/test_lloyd_george_validator.py @@ -11,7 +11,10 @@ TEST_NHS_NUMBER_FOR_BULK_UPLOAD, TEST_STAGING_METADATA_WITH_INVALID_FILENAME, ) -from tests.unit.helpers.data.pds.pds_patient_response import PDS_PATIENT +from tests.unit.helpers.data.pds.pds_patient_response import ( + PDS_PATIENT, + PDS_PATIENT_WITH_MIDDLE_NAME, +) from tests.unit.models.test_document_reference import MOCK_DOCUMENT_REFERENCE from utils.common_query_filters import NotDeleted from utils.exceptions import ( @@ -25,6 +28,7 @@ check_for_file_names_agrees_with_each_other, check_for_number_of_files_match_expected, check_for_patient_already_exist_in_repo, + check_pds_response_status, extract_info_from_filename, get_allowed_ods_codes, getting_patient_info_from_pds, @@ -33,6 +37,8 @@ validate_lg_file_names, validate_lg_file_type, validate_lg_files, + validate_patient_date_of_birth, + validate_patient_name, ) @@ -224,12 +230,15 @@ def test_files_for_different_patients(): assert str(e.value) == "File names does not match with each other" -def test_validate_nhs_id_with_pds_service(mocker, mock_pds_patient_details): +def test_validate_nhs_id_with_pds_service(mock_pds_patient_details): 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", ] - validate_filename_with_patient_details(lg_file_list, mock_pds_patient_details) + try: + validate_filename_with_patient_details(lg_file_list, mock_pds_patient_details) + except LGInvalidFilesException: + assert False def test_mismatch_nhs_id(mocker): @@ -248,17 +257,112 @@ def test_mismatch_nhs_id(mocker): validate_lg_file_names(lg_file_list, "9000000009") -def test_mismatch_name_with_pds_service(mocker, mock_pds_patient_details): +def test_mismatch_name_with_pds_service(mock_pds_patient_details): 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", + "1of2_Lloyd_George_Record_[Jake Plain Smith]_[9000000009]_[22-10-2010].pdf", + "2of2_Lloyd_George_Record_[Jake Plain Smith]_[9000000009]_[22-10-2010].pdf", ] with pytest.raises(LGInvalidFilesException): validate_filename_with_patient_details(lg_file_list, mock_pds_patient_details) -def test_mismatch_dob_with_pds_service(mocker, mock_pds_patient_details): +def test_order_names_with_pds_service(): + lg_file_list = [ + "1of2_Lloyd_George_Record_[Jake Jane Smith]_[9000000009]_[22-10-2010].pdf", + "2of2_Lloyd_George_Record_[Jake Jane Smith]_[9000000009]_[22-10-2010].pdf", + ] + patient = Patient.model_validate(PDS_PATIENT_WITH_MIDDLE_NAME) + patient_details = patient.get_minimum_patient_details("9000000009") + try: + validate_filename_with_patient_details(lg_file_list, patient_details) + except LGInvalidFilesException: + assert False + + +def test_validate_name_with_correct_name(mock_pds_patient_details): + lg_file_patient_name = "Jane Smith" + try: + validate_patient_name(lg_file_patient_name, mock_pds_patient_details) + except LGInvalidFilesException: + assert False + + +def test_validate_name_with_file_missing_middle_name(): + lg_file_patient_name = "Jane Smith" + patient = Patient.model_validate(PDS_PATIENT_WITH_MIDDLE_NAME) + patient_details = patient.get_minimum_patient_details("9000000009") + try: + validate_patient_name(lg_file_patient_name, patient_details) + except LGInvalidFilesException: + assert False + + +def test_validate_name_with_additional_middle_name_in_file_mismatching_pds(): + lg_file_patient_name = "Jane David Smith" + patient = Patient.model_validate(PDS_PATIENT_WITH_MIDDLE_NAME) + patient_details = patient.get_minimum_patient_details("9000000009") + try: + validate_patient_name(lg_file_patient_name, patient_details) + except LGInvalidFilesException: + assert False + + +def test_validate_name_with_additional_middle_name_in_file_but_none_in_pds( + mock_pds_patient_details, +): + lg_file_patient_name = "Jane David Smith" + try: + validate_patient_name(lg_file_patient_name, mock_pds_patient_details) + except LGInvalidFilesException: + assert False + + +def test_validate_name_with_wrong_order(): + lg_file_patient_name = "Jake Jane Smith" + patient = Patient.model_validate(PDS_PATIENT_WITH_MIDDLE_NAME) + patient_details = patient.get_minimum_patient_details("9000000009") + try: + validate_patient_name(lg_file_patient_name, patient_details) + except LGInvalidFilesException: + assert False + + +def test_validate_name_with_wrong_first_name(mock_pds_patient_details): + lg_file_patient_name = "John Smith" + with pytest.raises(LGInvalidFilesException): + validate_patient_name(lg_file_patient_name, mock_pds_patient_details) + + +def test_validate_name_with_wrong_family_name(mock_pds_patient_details): + lg_file_patient_name = "Jane Johnson" + with pytest.raises(LGInvalidFilesException): + validate_patient_name(lg_file_patient_name, mock_pds_patient_details) + + +def test_validate_name_without_given_name(mock_pds_patient_details): + lg_file_patient_name = "Jane Smith" + mock_pds_patient_details.given_name = [""] + try: + validate_patient_name(lg_file_patient_name, mock_pds_patient_details) + except LGInvalidFilesException: + assert False + + +def test_missing_middle_name_names_with_pds_service(): + 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", + ] + patient = Patient.model_validate(PDS_PATIENT_WITH_MIDDLE_NAME) + patient_details = patient.get_minimum_patient_details("9000000009") + try: + validate_filename_with_patient_details(lg_file_list, patient_details) + except LGInvalidFilesException: + assert False + + +def test_mismatch_dob_with_pds_service(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", @@ -268,7 +372,31 @@ def test_mismatch_dob_with_pds_service(mocker, mock_pds_patient_details): validate_filename_with_patient_details(lg_file_list, mock_pds_patient_details) -def test_patient_not_found_with_pds_service(mocker, mock_pds_call): +def test_validate_date_of_birth_when_mismatch_dob_with_pds_service( + mock_pds_patient_details, +): + file_date_of_birth = "14-01-2000" + + with pytest.raises(LGInvalidFilesException): + validate_patient_date_of_birth(file_date_of_birth, mock_pds_patient_details) + + +def test_validate_date_of_birth_valid_with_pds_service(mock_pds_patient_details): + file_date_of_birth = "22-10-2010" + try: + validate_patient_date_of_birth(file_date_of_birth, mock_pds_patient_details) + except LGInvalidFilesException: + assert False + + +def test_validate_date_of_birth_none_with_pds_service(mock_pds_patient_details): + file_date_of_birth = "22-10-2010" + mock_pds_patient_details.birth_date = None + with pytest.raises(LGInvalidFilesException): + validate_patient_date_of_birth(file_date_of_birth, mock_pds_patient_details) + + +def test_patient_not_found_with_pds_service(mock_pds_call): response = Response() response.status_code = 404 @@ -281,7 +409,7 @@ def test_patient_not_found_with_pds_service(mocker, mock_pds_call): mock_pds_call.assert_called_with(nhs_number="9000000009", retry_on_expired=True) -def test_bad_request_with_pds_service(mocker, mock_pds_call): +def test_bad_request_with_pds_service(mock_pds_call): response = Response() response.status_code = 400 @@ -294,9 +422,7 @@ def test_bad_request_with_pds_service(mocker, mock_pds_call): 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 -): +def test_validate_with_pds_service_raise_pds_too_many_requests_exception(mock_pds_call): response = Response() response.status_code = 429 response._content = b"Too Many Requests" @@ -308,6 +434,40 @@ def test_validate_with_pds_service_raise_PdsTooManyRequestsException( mock_pds_call.assert_called_with(nhs_number="9000000009", retry_on_expired=True) +def test_check_pds_response_429_status_raise_too_many_requests_exception(): + response = Response() + response.status_code = 429 + + with pytest.raises(PdsTooManyRequestsException): + check_pds_response_status(response) + + +def test_check_pds_response_404_status_raise_lg_invalid_files_exception(): + response = Response() + response.status_code = 404 + + with pytest.raises(LGInvalidFilesException): + check_pds_response_status(response) + + +def test_check_pds_response_4xx_or_5xx_status_raise_lg_invalid_files_exception(): + response = Response() + response.status_code = 500 + + with pytest.raises(LGInvalidFilesException): + check_pds_response_status(response) + + +def test_check_pds_response_200_status_not_raise_exception(): + response = Response() + response.status_code = 200 + + try: + check_pds_response_status(response) + except LGInvalidFilesException: + assert False + + def test_check_for_patient_already_exist_in_repo_return_none_when_patient_record_not_exist( set_env, mock_fetch_available_document_references_by_type ): @@ -341,7 +501,7 @@ def test_check_check_for_patient_already_exist_in_repo_raise_exception_when_pati ) -def test_validate_bulk_files_raises_PatientRecordAlreadyExistException_when_patient_record_already_exists( +def test_validate_bulk_files_raises_patient_record_already_exist_exception_when_patient_record_already_exists( set_env, mocker ): mocker.patch( @@ -364,7 +524,7 @@ def test_get_allowed_ods_codes_return_a_list_of_ods_codes(mock_get_ssm_parameter assert actual == expected -def test_get_allowed_ods_codes_can_handle_the_ALL_option(mock_get_ssm_parameter): +def test_get_allowed_ods_codes_can_handle_the_all_option(mock_get_ssm_parameter): mock_get_ssm_parameter.return_value = "ALL" expected = ["ALL"] diff --git a/lambdas/utils/lloyd_george_validator.py b/lambdas/utils/lloyd_george_validator.py index 500f7c500..49732d23a 100644 --- a/lambdas/utils/lloyd_george_validator.py +++ b/lambdas/utils/lloyd_george_validator.py @@ -140,31 +140,56 @@ def validate_filename_with_patient_details( ): try: file_name_info = extract_info_from_filename(file_name_list[0]) - patient_name = file_name_info["patient_name"] - date_of_birth = file_name_info["date_of_birth"] - - date_of_birth = datetime.datetime.strptime(date_of_birth, "%d-%m-%Y").date() - if patient_details.birth_date != date_of_birth: - raise LGInvalidFilesException("Patient DoB does not match our records") - patient_full_name = ( - " ".join([name for name in patient_details.given_Name]) - + " " - + patient_details.family_name - ) - logger.info("Verifying patient name against the record in PDS...") - - if not names_are_matching(patient_name, patient_full_name): - raise LGInvalidFilesException("Patient name does not match our records") + file_patient_name = file_name_info["patient_name"] + file_date_of_birth = file_name_info["date_of_birth"] + validate_patient_date_of_birth(file_date_of_birth, patient_details) + validate_patient_name(file_patient_name, patient_details) except (ClientError, ValueError) as e: logger.error(e) raise LGInvalidFilesException(e) +def validate_patient_name(file_patient_name, pds_patient_details): + logger.info("Verifying patient name against the record in PDS...") + patient_name_split = file_patient_name.split(" ") + file_patient_first_name = patient_name_split[0] + file_patient_last_name = patient_name_split[-1] + is_file_first_name_in_patient_details = False + for patient_name in pds_patient_details.given_name: + if ( + names_are_matching(file_patient_first_name, patient_name) + or not patient_name + ): + is_file_first_name_in_patient_details = True + break + + if not is_file_first_name_in_patient_details or not names_are_matching( + file_patient_last_name, pds_patient_details.family_name + ): + raise LGInvalidFilesException("Patient name does not match our records") + + +def validate_patient_date_of_birth(file_date_of_birth, pds_patient_details): + date_of_birth = datetime.datetime.strptime(file_date_of_birth, "%d-%m-%Y").date() + if ( + not pds_patient_details.birth_date + or pds_patient_details.birth_date != date_of_birth + ): + raise LGInvalidFilesException("Patient DoB does not match our records") + + def getting_patient_info_from_pds(nhs_number: str) -> PatientDetails: 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) + check_pds_response_status(pds_response) + patient = Patient.model_validate(pds_response.json()) + patient_details = patient.get_minimum_patient_details(nhs_number) + return patient_details + + +def check_pds_response_status(pds_response): if pds_response.status_code == 429: logger.error("Got 429 Too Many Requests error from PDS.") raise PdsTooManyRequestsException( @@ -178,9 +203,6 @@ def getting_patient_info_from_pds(nhs_number: str) -> PatientDetails: except HTTPError as e: logger.error(e) 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_allowed_ods_codes() -> list[str]: diff --git a/sonar-project.properties b/sonar-project.properties index 849bb345e..26a730cba 100644 --- a/sonar-project.properties +++ b/sonar-project.properties @@ -14,7 +14,7 @@ sonar.python.coverage.reportPaths=lambdas/coverage.xml sonar.sources=lambdas/,app/src/ sonar.tests=lambdas/tests/,app/src/ -sonar.exclusions=**/*.test.tsx,app/src/helpers/test/,**/*.story.tsx,lambdas/scripts/,**/*.test.ts,**/*.story.ts +sonar.exclusions=**/*.test.tsx,app/src/helpers/test/,**/*.story.tsx,lambdas/scripts/,**/*.test.ts,**/*.story.ts,lambdas/tests/ sonar.test.inclusions=**/*.test.tsx,app/src/helpers/test/,**/*.test.ts # Encoding of the source code. Default is default system encoding