From 6c483688d9cd886bc86d19f0a0d52b23258f5035 Mon Sep 17 00:00:00 2001 From: amarauzoma Date: Thu, 5 Mar 2026 16:17:09 +0000 Subject: [PATCH 1/6] VED-1088-Validate-system-URL-in-patient-identifier-resource --- .../tests/controller/test_fhir_batch_controller.py | 6 +++--- lambdas/shared/src/common/models/field_locations.py | 2 +- .../tests/test_common/test_immunization_post_validator.py | 4 +++- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/lambdas/recordforwarder/tests/controller/test_fhir_batch_controller.py b/lambdas/recordforwarder/tests/controller/test_fhir_batch_controller.py index 31b0376c1a..67855a1786 100644 --- a/lambdas/recordforwarder/tests/controller/test_fhir_batch_controller.py +++ b/lambdas/recordforwarder/tests/controller/test_fhir_batch_controller.py @@ -58,7 +58,7 @@ def test_send_request_to_dynamo_create_badrequest(self): 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" + message="Validation errors: contained[?(@.resourceType=='Patient')].identifier[?(@.system=='https://fhir.nhs.uk/Id/nhs-number')].value does not exists" ) message_body = { @@ -178,7 +178,7 @@ def test_send_request_to_dynamo_update_badrequest(self): 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" + message="Validation errors: contained[?(@.resourceType=='Patient')].identifier[?(@.system=='https://fhir.nhs.uk/Id/nhs-number')].value does not exists" ) message_body = { "supplier": "test_supplier", @@ -297,7 +297,7 @@ def test_send_request_to_dynamo_delete_badrequest(self): 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" + message="Validation errors: contained[?(@.resourceType=='Patient')].identifier[?(@.system=='https://fhir.nhs.uk/Id/nhs-number')].value does not exists" ) message_body = { "supplier": "test_supplier", diff --git a/lambdas/shared/src/common/models/field_locations.py b/lambdas/shared/src/common/models/field_locations.py index 9fb377ef7d..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" # 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/tests/test_common/test_immunization_post_validator.py b/lambdas/shared/tests/test_common/test_immunization_post_validator.py index 994c5f9b9d..6ef06247e8 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) From 6687d81cd78d3124b230683200fc44181a81a508 Mon Sep 17 00:00:00 2001 From: James Wharmby Date: Fri, 6 Mar 2026 16:28:23 +0000 Subject: [PATCH 2/6] pre_validate_patient_identifier_system --- .../models/fhir_immunization_pre_validators.py | 16 ++++++++++++++++ .../test_immunization_pre_validator.py | 10 ++++++++++ 2 files changed, 26 insertions(+) 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..a600ba45bf 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,6 +272,21 @@ def pre_validate_patient_identifier(self, values: dict) -> None: except (KeyError, IndexError): pass + def pre_validate_patient_identifier_system(self, values: dict) -> None: + """ + Pre-validate that, if contained[?(@.resourceType=='Patient')].identifier[0].system ( + legacy CSV field name: NHS_NUMBER) exists, then it is 'https://fhir.nhs.uk/Id/nhs-number' + """ + field_location = "contained[?(@.resourceType=='Patient')].identifier[0].system" + predefined_values = [Urls.NHS_NUMBER] + try: + field_value = [x for x in values["contained"] if x.get("resourceType") == "Patient"][0]["identifier"][0][ + "system" + ] + PreValidation.for_string(field_value, field_location, predefined_values=predefined_values) + except (KeyError, IndexError): + pass + def pre_validate_patient_identifier_value(self, values: dict) -> None: """ Pre-validate that, if contained[?(@.resourceType=='Patient')].identifier[0].value ( 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..af7cb25d7b 100644 --- a/lambdas/shared/tests/test_common/test_immunization_pre_validator.py +++ b/lambdas/shared/tests/test_common/test_immunization_pre_validator.py @@ -453,6 +453,16 @@ def test_pre_validate_patient_identifier_extension(self): expected_error_message="contained[?(@.resourceType=='Patient')].identifier[0] must not include an extension", ) + 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", + predefined_values=["https://fhir.nhs.uk/Id/nhs-number"], + valid_strings_to_test=["https://fhir.nhs.uk/Id/nhs-number"], + invalid_strings_to_test=["https://google.com/nhs", "Not-a-URI"], + ) + 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( From 9963a994a83656e3d4385a2034bf0a14f8b1e36f Mon Sep 17 00:00:00 2001 From: amarauzoma Date: Tue, 17 Mar 2026 08:58:32 +0000 Subject: [PATCH 3/6] Updated changes in relation to the comments --- .../controller/test_fhir_batch_controller.py | 22 ++++++++------- .../fhir_immunization_pre_validators.py | 24 ++++++++++++----- .../test_immunization_pre_validator.py | 27 +++++++++++++++++-- 3 files changed, 56 insertions(+), 17 deletions(-) diff --git a/lambdas/recordforwarder/tests/controller/test_fhir_batch_controller.py b/lambdas/recordforwarder/tests/controller/test_fhir_batch_controller.py index 67855a1786..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[?(@.system=='https://fhir.nhs.uk/Id/nhs-number')].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[?(@.system=='https://fhir.nhs.uk/Id/nhs-number')].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[?(@.system=='https://fhir.nhs.uk/Id/nhs-number')].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..71ed0a2862 100644 --- a/lambdas/shared/src/common/models/fhir_immunization_pre_validators.py +++ b/lambdas/shared/src/common/models/fhir_immunization_pre_validators.py @@ -271,17 +271,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/tests/test_common/test_immunization_pre_validator.py b/lambdas/shared/tests/test_common/test_immunization_pre_validator.py index 88a7013464..e08421d3cb 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,29 @@ 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 non-empty system values""" + # Test NHS identifier system - should pass + valid_json_data = deepcopy(self.json_data) + self.assertIsNone(self.validator.validate(valid_json_data)) + + # Test non-NHS identifier system - should also pass + valid_json_data = deepcopy(self.json_data) + valid_json_data["contained"][1]["identifier"][0]["system"] = "https://someother.codeableconcept.com/" + self.assertIsNone(self.validator.validate(valid_json_data)) + def test_pre_validate_patient_name(self): """Test pre_validate_patient_name accepts valid values and rejects invalid values""" ValidatorModelTests.test_list_value( From da62ceb76dab36b53e8e6da90e5a052a37c5c686 Mon Sep 17 00:00:00 2001 From: amarauzoma Date: Tue, 17 Mar 2026 10:46:49 +0000 Subject: [PATCH 4/6] Updated Error message log --- lambdas/backend/tests/service/test_fhir_service.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) 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): From e6df0d16c2601a2879b0d4ce7ce8a6f8b5e5a440 Mon Sep 17 00:00:00 2001 From: amarauzoma Date: Wed, 18 Mar 2026 10:43:41 +0000 Subject: [PATCH 5/6] Updated Error Constants --- tests/e2e_automation/utilities/error_constants.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/e2e_automation/utilities/error_constants.py b/tests/e2e_automation/utilities/error_constants.py index 9b752a39c0..ffe1a5931f 100644 --- a/tests/e2e_automation/utilities/error_constants.py +++ b/tests/e2e_automation/utilities/error_constants.py @@ -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", From 49ca8b9449e9e02758d5391928ab23cbc11e2a63 Mon Sep 17 00:00:00 2001 From: amarauzoma Date: Thu, 19 Mar 2026 09:15:55 +0000 Subject: [PATCH 6/6] Updated comments --- .../src/common/models/utils/generic_utils.py | 5 ++++- .../test_immunization_pre_validator.py | 15 ++++++--------- tests/e2e_automation/utilities/error_constants.py | 2 +- 3 files changed, 11 insertions(+), 11 deletions(-) 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_pre_validator.py b/lambdas/shared/tests/test_common/test_immunization_pre_validator.py index e08421d3cb..383f20c579 100644 --- a/lambdas/shared/tests/test_common/test_immunization_pre_validator.py +++ b/lambdas/shared/tests/test_common/test_immunization_pre_validator.py @@ -483,15 +483,12 @@ def test_pre_validate_patient_identifier_value_accepts_non_nhs_identifier(self): self.assertIsNone(self.validator.validate(valid_json_data)) def test_pre_validate_patient_identifier_system(self): - """Test pre_validate_patient_identifier_system accepts non-empty system values""" - # Test NHS identifier system - should pass - valid_json_data = deepcopy(self.json_data) - self.assertIsNone(self.validator.validate(valid_json_data)) - - # Test non-NHS identifier system - should also pass - valid_json_data = deepcopy(self.json_data) - valid_json_data["contained"][1]["identifier"][0]["system"] = "https://someother.codeableconcept.com/" - self.assertIsNone(self.validator.validate(valid_json_data)) + """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""" diff --git a/tests/e2e_automation/utilities/error_constants.py b/tests/e2e_automation/utilities/error_constants.py index ffe1a5931f..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",