diff --git a/lambdas/backend/tests/service/test_fhir_service.py b/lambdas/backend/tests/service/test_fhir_service.py index e0a5701919..6f6edcbe18 100644 --- a/lambdas/backend/tests/service/test_fhir_service.py +++ b/lambdas/backend/tests/service/test_fhir_service.py @@ -433,7 +433,8 @@ def test_patient_error(self): invalid_nhs_number = "9434765911" # check digit 1 doesn't match result (9) imms = create_covid_immunization_dict_no_id(invalid_nhs_number) expected_msg = ( - "Validation errors: contained[?(@.resourceType=='Patient')].identifier[0].value is not a valid NHS number" + "Validation errors: contained[?(@.resourceType=='Patient')].identifier" + "[?(@.system=='https://fhir.nhs.uk/Id/nhs-number')].value is not a valid NHS number" ) with self.assertRaises(CustomValidationError) as error: @@ -552,7 +553,8 @@ def test_update_immunization_raises_validation_exception_when_nhs_number_invalid self.imms_repo.update_immunization.assert_not_called() self.assertEqual( error.exception.message, - "Validation errors: contained[?(@.resourceType=='Patient')].identifier[0].value must be 10 characters", + "Validation errors: contained[?(@.resourceType=='Patient')].identifier" + "[?(@.system=='https://fhir.nhs.uk/Id/nhs-number')].value must be 10 characters", ) def test_update_immunization_raises_not_found_error_when_no_existing_immunisation(self): diff --git a/lambdas/recordforwarder/tests/controller/test_fhir_batch_controller.py b/lambdas/recordforwarder/tests/controller/test_fhir_batch_controller.py index 31b0376c1a..bfcf0aedd1 100644 --- a/lambdas/recordforwarder/tests/controller/test_fhir_batch_controller.py +++ b/lambdas/recordforwarder/tests/controller/test_fhir_batch_controller.py @@ -2,6 +2,7 @@ import uuid from unittest.mock import Mock, create_autospec +from common.models.constants import Urls from common.models.errors import ( CustomValidationError, IdentifierDuplicationError, @@ -18,6 +19,15 @@ def _make_immunization_pk(_id): return f"Immunization#{_id}" +def _missing_patient_nhs_number_error(): + return CustomValidationError( + message=( + "Validation errors: contained[?(@.resourceType=='Patient')].identifier" + f"[?(@.system=='{Urls.NHS_NUMBER}')].value does not exists" + ) + ) + + class TestCreateImmunizationBatchController(unittest.TestCase): def setUp(self): self.mock_repo = create_autospec(ImmunizationBatchRepository) @@ -57,9 +67,7 @@ def test_send_request_to_dynamo_create_badrequest(self): imms_id = str(uuid.uuid4()) imms_pk = _make_immunization_pk(imms_id) imms = create_covid_immunization(imms_id) - create_result = CustomValidationError( - message="Validation errors: contained[?(@.resourceType=='Patient')].identifier[0].value does not exists" - ) + create_result = _missing_patient_nhs_number_error() message_body = { "supplier": "test_supplier", @@ -177,9 +185,7 @@ def test_send_request_to_dynamo_update_badrequest(self): imms_id = str(uuid.uuid4()) imms_pk = _make_immunization_pk(imms_id) imms = create_covid_immunization(imms_id) - update_result = CustomValidationError( - message="Validation errors: contained[?(@.resourceType=='Patient')].identifier[0].value does not exists" - ) + update_result = _missing_patient_nhs_number_error() message_body = { "supplier": "test_supplier", "fhir_json": imms.json(), @@ -296,9 +302,7 @@ def test_send_request_to_dynamo_delete_badrequest(self): imms_id = str(uuid.uuid4()) imms_pk = _make_immunization_pk(imms_id) imms = create_covid_immunization(imms_id) - update_result = CustomValidationError( - message="Validation errors: contained[?(@.resourceType=='Patient')].identifier[0].value does not exists" - ) + update_result = _missing_patient_nhs_number_error() message_body = { "supplier": "test_supplier", "fhir_json": imms.json(), diff --git a/lambdas/shared/src/common/models/fhir_immunization_pre_validators.py b/lambdas/shared/src/common/models/fhir_immunization_pre_validators.py index a99a1d9ccd..08349235ad 100644 --- a/lambdas/shared/src/common/models/fhir_immunization_pre_validators.py +++ b/lambdas/shared/src/common/models/fhir_immunization_pre_validators.py @@ -45,6 +45,7 @@ def validate(self): self.pre_validate_practitioner_reference, self.pre_validate_patient_identifier_extension, self.pre_validate_patient_identifier, + self.pre_validate_patient_identifier_system, self.pre_validate_patient_identifier_value, self.pre_validate_patient_name, self.pre_validate_patient_name_given, @@ -271,17 +272,29 @@ def pre_validate_patient_identifier(self, values: dict) -> None: except (KeyError, IndexError): pass - def pre_validate_patient_identifier_value(self, values: dict) -> None: + def pre_validate_patient_identifier_system(self, values: dict) -> None: """ - Pre-validate that, if contained[?(@.resourceType=='Patient')].identifier[0].value ( - legacy CSV field name: NHS_NUMBER) exists, then it is a string of 10 characters - which does not contain spaces + Pre-validate that, if contained[?(@.resourceType=='Patient')].identifier[0].system exists, + then it is a non-empty string. """ - field_location = "contained[?(@.resourceType=='Patient')].identifier[0].value" + field_location = "contained[?(@.resourceType=='Patient')].identifier[0].system" try: field_value = [x for x in values["contained"] if x.get("resourceType") == "Patient"][0]["identifier"][0][ - "value" + "system" ] + PreValidation.for_string(field_value, field_location) + except (KeyError, IndexError): + pass + + def pre_validate_patient_identifier_value(self, values: dict) -> None: + """ + Pre-validate that, if the contained Patient has an NHS-number identifier, + its value is a string of 10 characters which does not contain spaces. + """ + field_location = f"contained[?(@.resourceType=='Patient')].identifier[?(@.system=='{Urls.NHS_NUMBER}')].value" + try: + patient = [x for x in values["contained"] if x.get("resourceType") == "Patient"][0] + field_value = [x for x in patient["identifier"] if x.get("system") == Urls.NHS_NUMBER][0]["value"] PreValidation.for_string(field_value, field_location, defined_length=10, spaces_allowed=False) PreValidation.for_nhs_number(field_value, field_location) except (KeyError, IndexError): diff --git a/lambdas/shared/src/common/models/field_locations.py b/lambdas/shared/src/common/models/field_locations.py index a10b661cff..df3963890b 100644 --- a/lambdas/shared/src/common/models/field_locations.py +++ b/lambdas/shared/src/common/models/field_locations.py @@ -25,7 +25,7 @@ class FieldLocations: target_disease = "protocolApplied[0].targetDisease" target_disease_codes = f"protocolApplied[0].targetDisease[0].coding[?(@.system=='{Urls.SNOMED}')].code" patient_identifier_value = ( - "contained[?(@.resourceType=='Patient')].identifier[0].value" # VED-1088 TODO: Fix to use nhs number url lookup + f"contained[?(@.resourceType=='Patient')].identifier[?(@.system=='{Urls.NHS_NUMBER}')].value" ) patient_name_given: str = field(init=False) patient_name_family: str = field(init=False) diff --git a/lambdas/shared/src/common/models/utils/generic_utils.py b/lambdas/shared/src/common/models/utils/generic_utils.py index 04978874f8..5cc80fe9ec 100644 --- a/lambdas/shared/src/common/models/utils/generic_utils.py +++ b/lambdas/shared/src/common/models/utils/generic_utils.py @@ -128,7 +128,10 @@ def get_occurrence_datetime(immunization: dict) -> datetime.datetime | None: def create_diagnostics(): - diagnostics = "Validation errors: contained[?(@.resourceType=='Patient')].identifier[0].value does not exists." + diagnostics = ( + "Validation errors: contained[?(@.resourceType=='Patient')].identifier" + "[?(@.system=='https://fhir.nhs.uk/Id/nhs-number')].value does not exists." + ) exp_error = {"diagnostics": diagnostics} return exp_error diff --git a/lambdas/shared/tests/test_common/test_immunization_post_validator.py b/lambdas/shared/tests/test_common/test_immunization_post_validator.py index 44f6418f46..b3910aedf9 100644 --- a/lambdas/shared/tests/test_common/test_immunization_post_validator.py +++ b/lambdas/shared/tests/test_common/test_immunization_post_validator.py @@ -206,7 +206,9 @@ def test_post_patient_identifier_value(self): """ Test that the JSON data is accepted when it does not contain patient_identifier_value """ - field_location = "contained[?(@.resourceType=='Patient')].identifier[0].value" + field_location = ( + "contained[?(@.resourceType=='Patient')].identifier[?(@.system=='https://fhir.nhs.uk/Id/nhs-number')].value" + ) self.mock_redis.hget.return_value = "COVID" self.mock_redis_getter.return_value = self.mock_redis MandationTests.test_missing_field_accepted(self, field_location) diff --git a/lambdas/shared/tests/test_common/test_immunization_pre_validator.py b/lambdas/shared/tests/test_common/test_immunization_pre_validator.py index 88a7013464..383f20c579 100644 --- a/lambdas/shared/tests/test_common/test_immunization_pre_validator.py +++ b/lambdas/shared/tests/test_common/test_immunization_pre_validator.py @@ -7,7 +7,7 @@ from jsonpath_ng.ext import parse -from common.models.constants import Constants +from common.models.constants import Constants, Urls from common.models.fhir_immunization import ImmunizationValidator from common.models.fhir_immunization_pre_validators import PreValidators from common.models.utils.generic_utils import ( @@ -457,7 +457,7 @@ def test_pre_validate_patient_identifier_value(self): """Test pre_validate_patient_identifier_value accepts valid values and rejects invalid values""" ValidatorModelTests.test_string_value( self, - field_location="contained[?(@.resourceType=='Patient')].identifier[0].value", + field_location=f"contained[?(@.resourceType=='Patient')].identifier[?(@.system=='{Urls.NHS_NUMBER}')].value", valid_strings_to_test=["9990548609"], defined_length=10, invalid_length_strings_to_test=["999054860", "99905486091", ""], @@ -470,6 +470,26 @@ def test_pre_validate_patient_identifier_value(self): ], ) + def test_pre_validate_patient_identifier_value_accepts_non_nhs_identifier(self): + """Test pre_validate_patient_identifier_value ignores non-NHS patient identifiers""" + valid_json_data = deepcopy(self.json_data) + valid_json_data["contained"][1]["identifier"] = [ + { + "system": "https://someother.codeableconcept.com/", + "value": "TVC15", + } + ] + + self.assertIsNone(self.validator.validate(valid_json_data)) + + def test_pre_validate_patient_identifier_system(self): + """Test pre_validate_patient_identifier_system accepts valid values and rejects invalid values""" + ValidatorModelTests.test_string_value( + self, + field_location="contained[?(@.resourceType=='Patient')].identifier[0].system", + valid_strings_to_test=[Urls.NHS_NUMBER, "https://someother.codeableconcept.com/"], + ) + def test_pre_validate_patient_name(self): """Test pre_validate_patient_name accepts valid values and rejects invalid values""" ValidatorModelTests.test_list_value( diff --git a/tests/e2e_automation/utilities/error_constants.py b/tests/e2e_automation/utilities/error_constants.py index 9b752a39c0..0bbaf25cdc 100644 --- a/tests/e2e_automation/utilities/error_constants.py +++ b/tests/e2e_automation/utilities/error_constants.py @@ -15,7 +15,7 @@ }, "empty_NHSNumber": { "code": "INVALID", - "diagnostics": "Validation errors: contained[?(@.resourceType=='Patient')].identifier[0].value must be a non-empty string", + "diagnostics": "Validation errors: contained[?(@.resourceType=='Patient')].identifier[?(@.system=='https://fhir.nhs.uk/Id/nhs-number')].value must be a non-empty string", }, "invalid_include": { "code": "INVALID", @@ -99,7 +99,7 @@ }, "invalid_nhsnumber_length": { "code": "INVARIANT", - "diagnostics": "Validation errors: contained[?(@.resourceType=='Patient')].identifier[0].value must be 10 characters", + "diagnostics": "Validation errors: contained[?(@.resourceType=='Patient')].identifier[?(@.system=='https://fhir.nhs.uk/Id/nhs-number')].value must be 10 characters", }, "no_forename": { "code": "INVARIANT", @@ -139,7 +139,7 @@ }, "invalid_mod11_nhsnumber": { "code": "INVARIANT", - "diagnostics": "Validation errors: contained[?(@.resourceType=='Patient')].identifier[0].value is not a valid NHS number", + "diagnostics": "Validation errors: contained[?(@.resourceType=='Patient')].identifier[?(@.system=='https://fhir.nhs.uk/Id/nhs-number')].value is not a valid NHS number", }, "should_be_string": { "code": "INVARIANT",