From 1b705c79403f7226cf776e40cd16731d357784b6 Mon Sep 17 00:00:00 2001 From: Matt Jarvis Date: Thu, 12 Jun 2025 10:55:00 +0100 Subject: [PATCH 1/6] Use consistent error format in delta. Wait until service is ready to run e2e tests. --- azure/templates/post-deploy.yml | 27 +++++++++++--------- delta_backend/src/converter.py | 32 ++++++++++++++---------- delta_backend/src/extractor.py | 44 ++++++++++++++++++--------------- 3 files changed, 58 insertions(+), 45 deletions(-) diff --git a/azure/templates/post-deploy.yml b/azure/templates/post-deploy.yml index 83346a33c6..a2fc8967d5 100644 --- a/azure/templates/post-deploy.yml +++ b/azure/templates/post-deploy.yml @@ -96,7 +96,8 @@ steps: response=$(curl -H "apikey: $(status-endpoint-api-key)" -s "$endpoint") response_code=$(jq -r '.checks.healthcheck.responseCode' <<< "$response") response_body=$(jq -r '.checks.healthcheck.outcome' <<< "$response") - if [ "$response_code" -eq 200 ] && [ "$response_body" == "OK" ]; then + status=$(jq -r '.status' <<< "$response") + if [ "$response_code" -eq 200 ] && [ "$response_body" == "OK" ] && [ "$status" == "pass" ]; then echo "Status test successful" break else @@ -106,8 +107,10 @@ steps: sleep 30 fi done - if [ $counter -eq 11 ]; then + if [ $counter -eq 21 ]; then echo "Status test failed: Maximum number of attempts reached" + echo "Last response received:" + echo "$response" exit 1 fi fi @@ -164,7 +167,7 @@ steps: export DEFAULT_CLIENT_SECRET="$(INT_CLIENT_SECRET)" echo "running: $test_cmd -v -c test_deployment.py test_proxy.py" $test_cmd -v -c test_deployment.py test_proxy.py - + elif [[ $APIGEE_ENVIRONMENT == "prod" ]]; then echo "Proxy test completed successfully as part of terraform resource up status check" @@ -175,35 +178,35 @@ steps: workingDirectory: "$(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)/e2e" displayName: Run Full Test Suite - + - bash: | set -e if ! [[ "$APIGEE_ENVIRONMENT" == "prod" || "$APIGEE_ENVIRONMENT" == "int" || "$APIGEE_ENVIRONMENT" == *"sandbox" ]]; then echo "Running E2E batch folder test cases" - + export AWS_PROFILE="apim-dev" aws_account_no="$(aws sts get-caller-identity --query Account --output text)" echo "Using AWS Account: $aws_account_no" - + service_name="${FULLY_QUALIFIED_SERVICE_NAME}" - + pr_no=$(echo "$service_name" | { grep -oE '[0-9]+$' || true; }) if [ -z "$pr_no" ]; then workspace="$APIGEE_ENVIRONMENT" else workspace="pr-$pr_no" fi - + poetry install --no-root # Install dependencies defined in pyproject.toml - + ENV="$workspace" poetry run python -m unittest -v -c - + echo "E2E batch folder test cases executed successfully" else echo "Skipping E2E batch folder test cases as the environment is prod-int-sandbox" fi - + displayName: Run full batch test suite workingDirectory: "$(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)/e2e_batch" condition: eq(1, 2) # Disable task but make this step visible in the pipeline @@ -213,4 +216,4 @@ steps: condition: always() inputs: testResultsFiles: '$(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)/tests/test-report.xml' - failTaskOnFailedTests: true \ No newline at end of file + failTaskOnFailedTests: true diff --git a/delta_backend/src/converter.py b/delta_backend/src/converter.py index 2701041549..91f801990c 100644 --- a/delta_backend/src/converter.py +++ b/delta_backend/src/converter.py @@ -11,24 +11,24 @@ def __init__(self, fhir_data, action_flag = ActionFlag.UPDATE, report_unexpected self.error_records = [] self.action_flag = action_flag self.report_unexpected_exception = report_unexpected_exception - + try: if not fhir_data: raise ValueError("FHIR data is required for initialization.") - + self.extractor = Extractor(fhir_data, self.report_unexpected_exception) - self.conversion_layout = ConversionLayout(self.extractor) + self.conversion_layout = ConversionLayout(self.extractor) except Exception as e: if report_unexpected_exception: - self._log_error(f"Initialization failed: [{e.__class__.__name__}] {e}") + self._log_error(None, None, f"Initialization failed: [{e.__class__.__name__}] {e}") raise def run_conversion(self): conversions = self.conversion_layout.get_conversion_layout() - + for conversion in conversions: self._convert_data(conversion) - + self.error_records.extend(self.extractor.get_error_records()) # Add CONVERSION_ERRORS as the 35th field @@ -36,9 +36,8 @@ def run_conversion(self): return self.converted def _convert_data(self, conversion: ConversionField): + flat_field = conversion.field_name_flat try: - flat_field = conversion.field_name_flat - if flat_field == "ACTION_FLAG": self.converted[flat_field] = self.action_flag else: @@ -47,17 +46,24 @@ def _convert_data(self, conversion: ConversionField): self.converted[flat_field] = converted except Exception as e: - self._log_error(f"Conversion error [{e.__class__.__name__}]: {e}", code=exception_messages.PARSING_ERROR) + self._log_error( + flat_field, + None, + f"Conversion error [{e.__class__.__name__}]: {e}", + code=exception_messages.PARSING_ERROR + ) self.converted[flat_field] = "" - def _log_error(self,e,code=exception_messages.UNEXPECTED_EXCEPTION): + def _log_error(self, field_name, field_value, e, code=exception_messages.UNEXPECTED_EXCEPTION): error_obj = { "code": code, + "field": field_name, + "value": field_value, "message": str(e) } - + if self.report_unexpected_exception: self.error_records.append(error_obj) - + def get_error_records(self): - return self.error_records \ No newline at end of file + return self.error_records diff --git a/delta_backend/src/extractor.py b/delta_backend/src/extractor.py index e93ebc6d64..44bfb008cc 100644 --- a/delta_backend/src/extractor.py +++ b/delta_backend/src/extractor.py @@ -29,7 +29,6 @@ def _get_patient(self): return next((c for c in contained if isinstance(c, dict) and c.get("resourceType") == "Patient"), "") def _get_valid_names(self, names, occurrence_time): - official_names = [n for n in names if n.get("use") == "official" and self._is_current_period(n, occurrence_time)] if official_names: return official_names[0] @@ -37,13 +36,11 @@ def _get_valid_names(self, names, occurrence_time): valid_names = [n for n in names if self._is_current_period(n, occurrence_time) and n.get("use") != "old"] if valid_names: return valid_names[0] - - return names[0] - + return names[0] def _get_person_names(self): - occurrence_time = self._get_occurance_date_time() + occurrence_time = self._get_occurrence_date_time() patient = self._get_patient() names = patient.get("name", []) names = [n for n in names if "given" in n and "family" in n] @@ -56,12 +53,12 @@ def _get_person_names(self): if person_forename and person_surname: return person_forename, person_surname - + return "", "" - + def _get_practitioner_names(self): contained = self.fhir_json_data.get("contained", []) - occurrence_time = self._get_occurance_date_time() + occurrence_time = self._get_occurrence_date_time() practitioner = next((c for c in contained if isinstance(c, dict) and c.get("resourceType") == "Practitioner"), None) if not practitioner or "name" not in practitioner: return "", "" @@ -77,7 +74,6 @@ def _get_practitioner_names(self): return performing_professional_forename, performing_professional_surname - def _is_current_period(self, name, occurrence_time): period = name.get("period") if not isinstance(period, dict): @@ -94,18 +90,26 @@ def _is_current_period(self, name, occurrence_time): return (not start or start <= occurrence_time) and (not end or occurrence_time <= end) - def _get_occurance_date_time(self) -> str: + def _get_occurrence_date_time(self) -> datetime: + occurrence_datetime_str = self.fhir_json_data.get("occurrenceDateTime", "") + try: - occurrence_time = datetime.fromisoformat(self.fhir_json_data.get("occurrenceDateTime", "")) - if occurrence_time and occurrence_time.tzinfo is None: - occurrence_time = occurrence_time.replace(tzinfo=timezone.utc) - return occurrence_time - return occurrence_time + occurrence_datetime = datetime.fromisoformat(occurrence_datetime_str) + + if occurrence_datetime and occurrence_datetime.tzinfo is None: + occurrence_datetime = occurrence_datetime.replace(tzinfo=timezone.utc) + + return occurrence_datetime except Exception as e: message = "DateTime conversion error [%s]: %s" % (e.__class__.__name__, e) - error = self._log_error(ConversionFieldName.DATE_AND_TIME, message, e, code=exception_messages.UNEXPECTED_EXCEPTION) - return error + self._log_error( + ConversionFieldName.DATE_AND_TIME, + occurrence_datetime_str, + message, + code=exception_messages.UNEXPECTED_EXCEPTION + ) + raise def _get_first_snomed_code(self, coding_container: dict) -> str: codings = coding_container.get("coding", []) @@ -255,7 +259,7 @@ def normalize(self, value): return value.lower() if isinstance(value, str) else value def extract_valid_address(self): - occurrence_time = self._get_occurance_date_time() + occurrence_time = self._get_occurrence_date_time() patient = self._get_patient() addresses = patient.get("address", []) @@ -279,8 +283,8 @@ def extract_valid_address(self): ) return selected_address.get("postalCode") or self.DEFAULT_POSTCODE - - def extract_date_time(self) -> str: + + def extract_date_time(self) -> str: date = self.fhir_json_data.get("occurrenceDateTime","") if date: return self._convert_date_to_safe_format(ConversionFieldName.DATE_AND_TIME, date) From 47e1f405a8993362f526e0f08e7ad162e33e922c Mon Sep 17 00:00:00 2001 From: Matt Jarvis Date: Thu, 12 Jun 2025 11:29:23 +0100 Subject: [PATCH 2/6] Increase number of attempts. Run check for sandbox too. --- azure/templates/post-deploy.yml | 55 ++++++++++++++++----------------- 1 file changed, 26 insertions(+), 29 deletions(-) diff --git a/azure/templates/post-deploy.yml b/azure/templates/post-deploy.yml index a2fc8967d5..6efca433ea 100644 --- a/azure/templates/post-deploy.yml +++ b/azure/templates/post-deploy.yml @@ -80,41 +80,38 @@ steps: - bash: | set -ex - if ! [[ $APIGEE_ENVIRONMENT =~ .*-*sandbox ]]; then - counter=0 - base_path="$SERVICE_BASE_PATH" - endpoint="" + endpoint="" + if [[ $APIGEE_ENVIRONMENT =~ "prod" ]]; then + endpoint="https://api.service.nhs.uk/${SERVICE_BASE_PATH}/_status" + else + endpoint="https://${APIGEE_ENVIRONMENT}.api.service.nhs.uk/${SERVICE_BASE_PATH}/_status" + fi - if [[ $APIGEE_ENVIRONMENT =~ "prod" ]]; then - endpoint="https://api.service.nhs.uk/${base_path}/_status" + counter=0 + while [[ $counter -lt 21 ]]; do + response=$(curl -H "apikey: $(status-endpoint-api-key)" -s "$endpoint") + response_code=$(jq -r '.checks.healthcheck.responseCode' <<< "$response") + response_body=$(jq -r '.checks.healthcheck.outcome' <<< "$response") + status=$(jq -r '.status' <<< "$response") + if [ "$response_code" -eq 200 ] && [ "$response_body" == "OK" ] && [ "$status" == "pass" ]; then + echo "Status test successful" + break else - endpoint="https://${APIGEE_ENVIRONMENT}.api.service.nhs.uk/${base_path}/_status" + echo "Waiting for $endpoint to return a 200 response with 'OK' body..." + ((counter=counter+1)) # Increment counter by 1 + echo "Attempt $counter" + sleep 30 fi + done - while [[ $counter -lt 11 ]]; do - response=$(curl -H "apikey: $(status-endpoint-api-key)" -s "$endpoint") - response_code=$(jq -r '.checks.healthcheck.responseCode' <<< "$response") - response_body=$(jq -r '.checks.healthcheck.outcome' <<< "$response") - status=$(jq -r '.status' <<< "$response") - if [ "$response_code" -eq 200 ] && [ "$response_body" == "OK" ] && [ "$status" == "pass" ]; then - echo "Status test successful" - break - else - echo "Waiting for $endpoint to return a 200 response with 'OK' body..." - ((counter=counter+1)) # Increment counter by 1 - echo "Attempt $counter" - sleep 30 - fi - done - if [ $counter -eq 21 ]; then - echo "Status test failed: Maximum number of attempts reached" - echo "Last response received:" - echo "$response" - exit 1 - fi + if [ $counter -eq 21 ]; then + echo "Status test failed: Maximum number of attempts reached" + echo "Last response received:" + echo "$response" + exit 1 fi - displayName: Waiting for TF resources to be UP + displayName: Waiting for API to be available workingDirectory: "$(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)" - bash: | From d441a4a296e6823ff7d0eb978afdc0cfe3e05f82 Mon Sep 17 00:00:00 2001 From: Matt Jarvis Date: Tue, 7 Oct 2025 15:38:48 +0100 Subject: [PATCH 3/6] Remove unused switch. Tidy up error handling in Converter init. --- delta_backend/src/converter.py | 29 ++++-------- delta_backend/src/extractor.py | 26 +++++------ delta_backend/tests/test_convert.py | 70 ++++++----------------------- 3 files changed, 34 insertions(+), 91 deletions(-) diff --git a/delta_backend/src/converter.py b/delta_backend/src/converter.py index 91f801990c..f78da264c5 100644 --- a/delta_backend/src/converter.py +++ b/delta_backend/src/converter.py @@ -6,22 +6,16 @@ class Converter: - def __init__(self, fhir_data, action_flag = ActionFlag.UPDATE, report_unexpected_exception=True): + def __init__(self, fhir_data, action_flag = ActionFlag.UPDATE): self.converted = {} self.error_records = [] self.action_flag = action_flag - self.report_unexpected_exception = report_unexpected_exception - try: - if not fhir_data: - raise ValueError("FHIR data is required for initialization.") + if not fhir_data: + raise ValueError("FHIR data is required for initialization.") - self.extractor = Extractor(fhir_data, self.report_unexpected_exception) - self.conversion_layout = ConversionLayout(self.extractor) - except Exception as e: - if report_unexpected_exception: - self._log_error(None, None, f"Initialization failed: [{e.__class__.__name__}] {e}") - raise + self.extractor = Extractor(fhir_data) + self.conversion_layout = ConversionLayout(self.extractor) def run_conversion(self): conversions = self.conversion_layout.get_conversion_layout() @@ -44,26 +38,21 @@ def _convert_data(self, conversion: ConversionField): converted = conversion.expression_rule() if converted is not None: self.converted[flat_field] = converted - except Exception as e: self._log_error( flat_field, - None, f"Conversion error [{e.__class__.__name__}]: {e}", code=exception_messages.PARSING_ERROR ) self.converted[flat_field] = "" - def _log_error(self, field_name, field_value, e, code=exception_messages.UNEXPECTED_EXCEPTION): - error_obj = { + def _log_error(self, field_name, e, code): + self.error_records.append({ "code": code, "field": field_name, - "value": field_value, + "value": None, "message": str(e) - } - - if self.report_unexpected_exception: - self.error_records.append(error_obj) + }) def get_error_records(self): return self.error_records diff --git a/delta_backend/src/extractor.py b/delta_backend/src/extractor.py index 2c46881204..b08cc3354a 100644 --- a/delta_backend/src/extractor.py +++ b/delta_backend/src/extractor.py @@ -19,9 +19,8 @@ class Extractor: DATE_CONVERT_FORMAT = "%Y%m%d" DEFAULT_POSTCODE = "ZZ99 3CZ" - def __init__(self, fhir_json_data, report_unexpected_exception = True): + def __init__(self, fhir_json_data): self.fhir_json_data = json.loads(fhir_json_data, parse_float=decimal.Decimal) if isinstance(fhir_json_data, str) else fhir_json_data - self.report_unexpected_exception = report_unexpected_exception self.error_records = [] def _get_patient(self): @@ -163,18 +162,17 @@ def _get_site_information(self): return site_code, site_code_type_uri def _log_error(self, field_name, field_value, e, code=exception_messages.RECORD_CHECK_FAILED): - if self.report_unexpected_exception: - if isinstance(e, Exception): - message = exception_messages.MESSAGES[exception_messages.UNEXPECTED_EXCEPTION] % (e.__class__.__name__, str(e)) - else: - message = str(e) - - self.error_records.append({ - "code": code, - "field": field_name, - "value": field_value, - "message": message - }) + if isinstance(e, Exception): + message = exception_messages.MESSAGES[exception_messages.UNEXPECTED_EXCEPTION] % (e.__class__.__name__, str(e)) + else: + message = str(e) + + self.error_records.append({ + "code": code, + "field": field_name, + "value": field_value, + "message": message + }) def _convert_date(self, field_name, date, format) -> str: """ diff --git a/delta_backend/tests/test_convert.py b/delta_backend/tests/test_convert.py index a8e2d20259..e0d10a8138 100644 --- a/delta_backend/tests/test_convert.py +++ b/delta_backend/tests/test_convert.py @@ -28,7 +28,7 @@ def setUp(self): # Start moto AWS mocks self.mock = mock_aws() self.mock.start() - + """Set up mock DynamoDB table.""" self.dynamodb_resource = boto3_resource("dynamodb", "eu-west-2") self.table = self.dynamodb_resource.create_table( @@ -74,7 +74,7 @@ def tearDown(self): self.logger_exception_patcher.stop() self.logger_info_patcher.stop() self.mock_firehose_logger.stop() - + self.mock.stop() @staticmethod @@ -126,34 +126,18 @@ def test_fhir_converter_json_direct_data(self): json_data = json.dumps(ValuesForTests.json_data) fhir_converter = Converter(json_data) - FlatFile = fhir_converter.run_conversion() + result = fhir_converter.run_conversion() - flatJSON = json.dumps(FlatFile) + result_str = json.dumps(result) expected_imms_value = deepcopy(ValuesForTests.expected_imms2) # UPDATE is currently the default action-flag expected_imms = json.dumps(expected_imms_value) - self.assertEqual(flatJSON, expected_imms) + self.assertEqual(result_str, expected_imms) - errorRecords = fhir_converter.get_error_records() + error_records = fhir_converter.get_error_records() - self.assertEqual(len(errorRecords), 0) - - def test_fhir_converter_json_direct_data(self): - """it should convert fhir json data to flat json""" - json_data = json.dumps(ValuesForTests.json_data) - - fhir_converter = Converter(json_data) - FlatFile = fhir_converter.run_conversion() + self.assertEqual(len(error_records), 0) - flatJSON = json.dumps(FlatFile) - expected_imms_value = deepcopy(ValuesForTests.expected_imms2) # UPDATE is currently the default action-flag - expected_imms = json.dumps(expected_imms_value) - self.assertEqual(flatJSON, expected_imms) - - errorRecords = fhir_converter.get_error_records() - - self.assertEqual(len(errorRecords), 0) - - def test_fhir_converter_json_error_scenario_reporting_on(self): + def test_fhir_converter_json_error_scenario(self): """it should convert fhir json data to flat json - error scenarios""" error_test_cases = [ErrorValuesForTests.missing_json, ErrorValuesForTests.json_dob_error] @@ -163,42 +147,18 @@ def test_fhir_converter_json_error_scenario_reporting_on(self): fhir_converter = Converter(json_data) fhir_converter.run_conversion() - errorRecords = fhir_converter.get_error_records() + error_records = fhir_converter.get_error_records() # Check if bad data creates error records - self.assertTrue(len(errorRecords) > 0) - - def test_fhir_converter_json_error_scenario_reporting_off(self): - """it should convert fhir json data to flat json - error scenarios""" - error_test_cases = [ErrorValuesForTests.missing_json, ErrorValuesForTests.json_dob_error] - - for test_case in error_test_cases: - json_data = json.dumps(test_case) - - fhir_converter = Converter(json_data, report_unexpected_exception=False) - fhir_converter.run_conversion() - - errorRecords = fhir_converter.get_error_records() + self.assertTrue(len(error_records) > 0) - # Check if bad data creates error records - self.assertTrue(len(errorRecords) == 0) - - def test_fhir_converter_json_incorrect_data_scenario_reporting_on(self): + def test_fhir_converter_json_incorrect_data_scenario(self): """it should convert fhir json data to flat json - error scenarios""" with self.assertRaises(ValueError): fhir_converter = Converter(None) - errorRecords = fhir_converter.get_error_records() - self.assertTrue(len(errorRecords) > 0) - - - def test_fhir_converter_json_incorrect_data_scenario_reporting_off(self): - """it should convert fhir json data to flat json - error scenarios""" - - with self.assertRaises(ValueError): - fhir_converter = Converter(None, report_unexpected_exception=False) - errorRecords = fhir_converter.get_error_records() - self.assertTrue(len(errorRecords) == 0) + error_records = fhir_converter.get_error_records() + self.assertTrue(len(error_records) > 0) def test_handler_imms_convert_to_flat_json(self): """Test that the Imms field contains the correct flat JSON data for CREATE, UPDATE, and DELETE operations.""" @@ -231,8 +191,6 @@ def test_handler_imms_convert_to_flat_json(self): response ) - result = self.table.scan() - items = result.get("Items", []) self.clear_table() def clear_table(self): @@ -240,8 +198,6 @@ def clear_table(self): with self.table.batch_writer() as batch: for item in scan.get("Items", []): batch.delete_item(Key={"PK": item["PK"]}) - result = self.table.scan() - items = result.get("Items", []) if __name__ == "__main__": unittest.main() From 748840d8aba49c01a7d1224951c6390ef1e535a4 Mon Sep 17 00:00:00 2001 From: Thomas-Boyle Date: Wed, 15 Apr 2026 14:02:01 +0100 Subject: [PATCH 4/6] Refactor error handling and improve test coverage for Converter and Extractor classes --- lambdas/delta_backend/src/converter.py | 19 ++++--- lambdas/delta_backend/src/extractor.py | 12 ++--- lambdas/delta_backend/tests/test_convert.py | 60 ++++++++++++--------- 3 files changed, 50 insertions(+), 41 deletions(-) diff --git a/lambdas/delta_backend/src/converter.py b/lambdas/delta_backend/src/converter.py index d662a82543..cf5eb14aab 100644 --- a/lambdas/delta_backend/src/converter.py +++ b/lambdas/delta_backend/src/converter.py @@ -18,9 +18,7 @@ def __init__(self, fhir_data, action_flag=ActionFlag.UPDATE): self.conversion_layout = ConversionLayout(self.extractor) def run_conversion(self): - conversions = self.conversion_layout.get_conversion_layout() - - for conversion in conversions: + for conversion in self.conversion_layout.get_conversion_layout(): self._convert_data(conversion) self.error_records.extend(self.extractor.get_error_records()) @@ -30,18 +28,19 @@ def run_conversion(self): return self.converted def _convert_data(self, conversion: ConversionField): + flat_field = conversion.field_name_flat + try: - flat_field = conversion.field_name_flat if flat_field == "ACTION_FLAG": self.converted[flat_field] = self.action_flag - else: - converted = conversion.expression_rule() - if converted is not None: - self.converted[flat_field] = converted - except Exception as e: + return + + if (converted := conversion.expression_rule()) is not None: + self.converted[flat_field] = converted + except Exception as error: self._log_error( flat_field, - f"Conversion error [{e.__class__.__name__}]: {e}", + f"Conversion error [{error.__class__.__name__}]: {error}", code=exception_messages.PARSING_ERROR, ) self.converted[flat_field] = "" diff --git a/lambdas/delta_backend/src/extractor.py b/lambdas/delta_backend/src/extractor.py index 03ab762a3c..2e3d37cf94 100644 --- a/lambdas/delta_backend/src/extractor.py +++ b/lambdas/delta_backend/src/extractor.py @@ -173,13 +173,11 @@ def _get_site_information(self): return site_code, site_code_type_uri def _log_error(self, field_name, field_value, e, code=exception_messages.RECORD_CHECK_FAILED): - if isinstance(e, Exception): - message = exception_messages.MESSAGES[exception_messages.UNEXPECTED_EXCEPTION] % ( - e.__class__.__name__, - str(e), - ) - else: - message = str(e) + message = ( + exception_messages.MESSAGES[exception_messages.UNEXPECTED_EXCEPTION] % (e.__class__.__name__, str(e)) + if isinstance(e, Exception) + else str(e) + ) self.error_records.append( { diff --git a/lambdas/delta_backend/tests/test_convert.py b/lambdas/delta_backend/tests/test_convert.py index 2e10c11341..0e897c9e12 100644 --- a/lambdas/delta_backend/tests/test_convert.py +++ b/lambdas/delta_backend/tests/test_convert.py @@ -89,6 +89,13 @@ def get_event(event_name=EventName.CREATE, operation="operation", supplier="EMIS """Returns test event data.""" return ValuesForTests.get_event(event_name, operation, supplier) + def assert_structured_error_records(self, error_records): + self.assertTrue(error_records) + + for error_record in error_records: + self.assertEqual(set(error_record), {"code", "field", "value", "message"}) + self.assertTrue(error_record["message"]) + def assert_dynamodb_record( self, operation_flag, @@ -141,40 +148,45 @@ def test_fhir_converter_json_direct_data(self): fhir_converter = Converter(json_data) result = fhir_converter.run_conversion() - result_str = json.dumps(result) expected_imms_value = deepcopy(ValuesForTests.expected_imms2) # UPDATE is currently the default action-flag - expected_imms = json.dumps(expected_imms_value) - self.assertEqual(result_str, expected_imms) - - error_records = fhir_converter.get_error_records() - - self.assertEqual(len(error_records), 0) + self.assertEqual(result, expected_imms_value) + self.assertFalse(fhir_converter.get_error_records()) def test_fhir_converter_json_error_scenario(self): """it should convert fhir json data to flat json - error scenarios""" - error_test_cases = [ - ErrorValuesForTests.missing_json, - ErrorValuesForTests.json_dob_error, - ] - - for test_case in error_test_cases: - json_data = json.dumps(test_case) + error_test_cases = { + "missing_json": ErrorValuesForTests.missing_json, + "json_dob_error": ErrorValuesForTests.json_dob_error, + } - fhir_converter = Converter(json_data) - fhir_converter.run_conversion() + for test_name, test_case in error_test_cases.items(): + with self.subTest(test_name=test_name): + fhir_converter = Converter(json.dumps(test_case)) + fhir_converter.run_conversion() - error_records = fhir_converter.get_error_records() - - # Check if bad data creates error records - self.assertTrue(len(error_records) > 0) + self.assert_structured_error_records(fhir_converter.get_error_records()) def test_fhir_converter_json_incorrect_data_scenario(self): """it should convert fhir json data to flat json - error scenarios""" - with self.assertRaises(ValueError): - fhir_converter = Converter(None) - error_records = fhir_converter.get_error_records() - self.assertTrue(len(error_records) > 0) + with self.assertRaisesRegex(ValueError, "FHIR data is required for initialization."): + Converter(None) + + def test_handler_persists_structured_conversion_errors(self): + event = self.get_event(operation=Operation.UPDATE) + event["Records"][0]["dynamodb"]["NewImage"]["Resource"]["S"] = json.dumps(ErrorValuesForTests.json_dob_error) + + response = handler(event, None) + + result = self.table.scan() + items = result.get("Items", []) + self.assertTrue(response) + self.assertEqual(len(items), 1) + + error_records = items[0]["Imms"]["CONVERSION_ERRORS"] + self.assert_structured_error_records(error_records) + self.assertEqual(error_records[0]["field"], "PERSON_DOB") + self.assertEqual(error_records[0]["value"], "196513-28") def test_handler_imms_convert_to_flat_json(self): """Test that the Imms field contains the correct flat JSON data for CREATE, UPDATE, and DELETE operations.""" From 7749ea456649f969b9e7fb5e5c922cbbe20c56c8 Mon Sep 17 00:00:00 2001 From: Thomas-Boyle Date: Wed, 15 Apr 2026 14:06:17 +0100 Subject: [PATCH 5/6] Enhance error reporting in DeltaHandlerTestCase to include detailed conversion error records --- lambdas/delta_backend/tests/test_delta.py | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/lambdas/delta_backend/tests/test_delta.py b/lambdas/delta_backend/tests/test_delta.py index 6579a0a472..949f52d51e 100644 --- a/lambdas/delta_backend/tests/test_delta.py +++ b/lambdas/delta_backend/tests/test_delta.py @@ -511,9 +511,17 @@ def test_dps_record_skipped(self, mock_logger_info): @patch("delta.Converter") def test_partial_success_with_errors(self, mock_converter): + expected_error_records = [ + { + "code": 10, + "field": "PERSON_DOB", + "value": "196513-28", + "message": "Unexpected exception [ValueError]: Invalid isoformat string: '196513-28'", + } + ] mock_converter_instance = MagicMock() mock_converter_instance.run_conversion.return_value = {"ABC": "DEF"} - mock_converter_instance.get_error_records.return_value = [{"error": "Invalid field"}] + mock_converter_instance.get_error_records.return_value = expected_error_records mock_converter.return_value = mock_converter_instance # Mock DynamoDB put_item success @@ -533,13 +541,16 @@ def test_partial_success_with_errors(self, mock_converter): args, kwargs = self.mock_send_log_to_firehose.call_args sent_payload = args[1] # Second positional arg - # Navigate to the specific message - status_desc = sent_payload["operation_outcome"]["statusDesc"] + operation_outcome = sent_payload["operation_outcome"] - # Assert the expected message is present self.assertIn( "Partial success: successfully synced into delta, but issues found within record", - status_desc, + operation_outcome["statusDesc"], + ) + self.assertEqual(operation_outcome["diagnostics"], expected_error_records) + self.mock_logger.warning.assert_called_once_with( + "Partial success: record synced with conversion errors", + extra={"conversion_errors": expected_error_records}, ) def test_send_message_multi_records_diverse(self): From c9e25b3515e9251d0ff7391fcc892890050fef07 Mon Sep 17 00:00:00 2001 From: Thomas-Boyle Date: Thu, 16 Apr 2026 11:07:36 +0100 Subject: [PATCH 6/6] Refactor Converter class to improve type annotations and enhance code clarity --- lambdas/delta_backend/src/converter.py | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/lambdas/delta_backend/src/converter.py b/lambdas/delta_backend/src/converter.py index cf5eb14aab..e03927b259 100644 --- a/lambdas/delta_backend/src/converter.py +++ b/lambdas/delta_backend/src/converter.py @@ -1,14 +1,19 @@ # Main validation engine +from typing import Any + import exception_messages from conversion_layout import ConversionField, ConversionLayout from extractor import Extractor from mappings import ActionFlag +ConversionErrorRecord = dict[str, Any] +ConvertedRecord = dict[str, Any] + class Converter: - def __init__(self, fhir_data, action_flag=ActionFlag.UPDATE): - self.converted = {} - self.error_records = [] + def __init__(self, fhir_data: str | dict[str, Any], action_flag: str = ActionFlag.UPDATE) -> None: + self.converted: ConvertedRecord = {} + self.error_records: list[ConversionErrorRecord] = [] self.action_flag = action_flag if not fhir_data: @@ -17,7 +22,7 @@ def __init__(self, fhir_data, action_flag=ActionFlag.UPDATE): self.extractor = Extractor(fhir_data) self.conversion_layout = ConversionLayout(self.extractor) - def run_conversion(self): + def run_conversion(self) -> ConvertedRecord: for conversion in self.conversion_layout.get_conversion_layout(): self._convert_data(conversion) @@ -27,7 +32,7 @@ def run_conversion(self): self.converted["CONVERSION_ERRORS"] = self.error_records return self.converted - def _convert_data(self, conversion: ConversionField): + def _convert_data(self, conversion: ConversionField) -> None: flat_field = conversion.field_name_flat try: @@ -45,7 +50,7 @@ def _convert_data(self, conversion: ConversionField): ) self.converted[flat_field] = "" - def _log_error(self, field_name, e, code): + def _log_error(self, field_name: str, e: Exception | str, code: str) -> None: self.error_records.append( { "code": code, @@ -55,5 +60,5 @@ def _log_error(self, field_name, e, code): } ) - def get_error_records(self): + def get_error_records(self) -> list[ConversionErrorRecord]: return self.error_records