diff --git a/lambdas/id_sync/src/id_sync.py b/lambdas/id_sync/src/id_sync.py index c42c333f3..ef9b5f595 100644 --- a/lambdas/id_sync/src/id_sync.py +++ b/lambdas/id_sync/src/id_sync.py @@ -24,11 +24,21 @@ def handler(event_data: Dict[str, Any], _context) -> Dict[str, Any]: logger.info("id_sync processing event with %d records", len(records)) - results = [process_record(record) for record in records] - nhs_numbers = [result["nhs_number"] for result in results] - error_count = sum(1 for result in results if result.get("status") == "error") + results = [] + nhs_numbers = [] + error_count = 0 - if error_count: + for record in records: + result = process_record(record) + results.append(result) + + if "nhs_number" in result: + nhs_numbers.append(result["nhs_number"]) + + if result.get("status") == "error": + error_count += 1 + + if error_count > 0: raise IdSyncException(message=f"Processed {len(records)} records with {error_count} errors", nhs_numbers=nhs_numbers) diff --git a/lambdas/id_sync/src/ieds_db_operations.py b/lambdas/id_sync/src/ieds_db_operations.py index cfff62697..e918e6dae 100644 --- a/lambdas/id_sync/src/ieds_db_operations.py +++ b/lambdas/id_sync/src/ieds_db_operations.py @@ -2,6 +2,7 @@ from os_vars import get_ieds_table_name from common.aws_dynamodb import get_dynamodb_table from common.clients import logger, dynamodb_client +import json from utils import make_status from exceptions.id_sync_exception import IdSyncException @@ -19,7 +20,7 @@ def get_ieds_table(): def ieds_update_patient_id(old_id: str, new_id: str, items_to_update: list | None = None) -> dict: - """Update the patient ID in the IEDS table.""" + """Update the patient ID (new NHS number) in the IEDS table.""" logger.info(f"ieds_update_patient_id. Update patient ID from {old_id} to {new_id}") if not old_id or not new_id or not old_id.strip() or not new_id.strip(): return make_status("Old ID and New ID cannot be empty", old_id, "error") @@ -133,13 +134,23 @@ def paginate_items_for_patient_pk(patient_pk: str) -> list: def extract_patient_resource_from_item(item: dict) -> dict | None: """ - Extract a Patient resource dict from an IEDS database. + Extract a Patient resource from an IEDS database. """ patient_resource = item.get("Resource", None) + + if isinstance(patient_resource, str): + try: + patient_resource_parsed = json.loads(patient_resource) + except json.JSONDecodeError: + logger.error("Failed to decode patient_resource JSON string") + return None + patient_resource = patient_resource_parsed + if not isinstance(patient_resource, dict): return None - for response in patient_resource.get("contained", []): + contained = patient_resource.get("contained") or [] + for response in contained: if isinstance(response, dict) and response.get("resourceType") == "Patient": return response diff --git a/lambdas/id_sync/src/pds_details.py b/lambdas/id_sync/src/pds_details.py index e9af7601a..d0d5f4074 100644 --- a/lambdas/id_sync/src/pds_details.py +++ b/lambdas/id_sync/src/pds_details.py @@ -43,7 +43,6 @@ def pds_get_patient_id(nhs_number: str) -> str: :return: PDS patient ID """ try: - logger.info(f"get_pds_patient_id. nhs_number: {nhs_number}") patient_details = pds_get_patient_details(nhs_number) if not patient_details: return None diff --git a/lambdas/id_sync/src/record_processor.py b/lambdas/id_sync/src/record_processor.py index 5b5eac7c6..67263ea06 100644 --- a/lambdas/id_sync/src/record_processor.py +++ b/lambdas/id_sync/src/record_processor.py @@ -57,6 +57,8 @@ def process_nhs_number(nhs_number: str) -> Dict[str, Any]: logger.exception("process_nhs_number: failed to fetch demographic details: %s", e) return make_status(str(e), nhs_number, "error") + logger.info("Fetched IEDS resources. IEDS count: %d", len(ieds_resources) if ieds_resources else 0) + if not ieds_resources: logger.info("No IEDS records returned for NHS number: %s", nhs_number) return make_status(f"No records returned for ID: {nhs_number}", nhs_number) @@ -65,6 +67,7 @@ def process_nhs_number(nhs_number: str) -> Dict[str, Any]: matching_records = [] discarded_count = 0 for detail in ieds_resources: + logger.info("Processing IEDS record: %s", detail) if demographics_match(pds_patient_resource, detail): matching_records.append(detail) else: @@ -84,16 +87,21 @@ def process_nhs_number(nhs_number: str) -> Dict[str, Any]: return response -# Function to fetch PDS Patient details and IEDS Immunisation records +# Function to fetch PDS Patient details and IEDS Immunisation records. def fetch_pds_and_ieds_resources(nhs_number: str): + logger.info("fetch_pds_and_ieds_resources: fetching for %s", nhs_number) try: pds = pds_get_patient_details(nhs_number) except Exception as e: + logger.exception("fetch_pds_and_ieds_resources: failed to fetch PDS details for %s", nhs_number) raise RuntimeError("Failed to fetch PDS details") from e + try: ieds = get_items_from_patient_id(nhs_number) except Exception as e: + logger.exception("fetch_pds_and_ieds_resources: failed to fetch IEDS items for %s", nhs_number) raise RuntimeError("Failed to fetch IEDS items") from e + return pds, ieds @@ -132,11 +140,13 @@ def normalize_strings(item: Any) -> str | None: pds_name = normalize_strings(extract_normalized_name_from_patient(pds_details)) pds_gender = normalize_strings(pds_details.get("gender")) pds_birth = normalize_strings(pds_details.get("birthDate")) + logger.debug("demographics_match: demographics match for name=%s, gender=%s, birthDate=%s", + pds_name, pds_gender, pds_birth) # Retrieve patient resource from IEDS item patient = extract_patient_resource_from_item(ieds_item) if not patient: - logger.debug("demographics_match: no patient resource in IEDS table item") + logger.info("demographics_match: no patient resource in IEDS table item") return False # normalize patient fields from IEDS diff --git a/lambdas/id_sync/tests/test_id_sync.py b/lambdas/id_sync/tests/test_id_sync.py index 3d7c54cf8..d85b0293a 100644 --- a/lambdas/id_sync/tests/test_id_sync.py +++ b/lambdas/id_sync/tests/test_id_sync.py @@ -235,29 +235,30 @@ def test_handler_process_record_exception(self): self.assertEqual(exception.message, "Error processing id_sync event") def test_handler_process_record_missing_nhs_number(self): - """Test handler when process_record returns incomplete data""" + """Test handler when process_record returns error and missing NHS number""" + # Setup mocks mock_event = MagicMock() mock_event.records = [MagicMock()] self.mock_aws_lambda_event.return_value = mock_event - # Missing "nhs_number" in response + # Return result without 'nhs_number' but with an 'error' status self.mock_process_record.return_value = { - "status": "success" - # Missing "nhs_number" + "status": "error", + "message": "Missing NHS number" + # No 'nhs_number' } - # Call handler + # Call handler and expect exception with self.assertRaises(IdSyncException) as exception_context: handler(self.single_sqs_event, None) exception = exception_context.exception - # convert exception payload to json self.assertIsInstance(exception, IdSyncException) - self.assertEqual(exception.nhs_numbers, None) - self.assertEqual(exception.message, "Error processing id_sync event") - self.mock_logger.exception.assert_called_once_with("Error processing id_sync event") + self.assertEqual(exception.nhs_numbers, []) + self.assertEqual(exception.message, "Processed 1 records with 1 errors") + self.mock_logger.exception.assert_called_once_with(f"id_sync error: {exception.message}") def test_handler_context_parameter_ignored(self): """Test that context parameter is properly ignored""" diff --git a/lambdas/id_sync/tests/test_ieds_db_operations.py b/lambdas/id_sync/tests/test_ieds_db_operations.py index 6a9d956c5..b4a55060e 100644 --- a/lambdas/id_sync/tests/test_ieds_db_operations.py +++ b/lambdas/id_sync/tests/test_ieds_db_operations.py @@ -1,10 +1,51 @@ import unittest + +from ieds_db_operations import extract_patient_resource_from_item from unittest.mock import patch, MagicMock from exceptions.id_sync_exception import IdSyncException - import ieds_db_operations +class TestExtractPatientResourceFromItem(unittest.TestCase): + + def test_extract_from_dict_with_contained_patient(self): + item = { + "Resource": { + "resourceType": "Immunization", + "contained": [ + {"resourceType": "Patient", "id": "P1", "name": [{"family": "Doe"}]} + ], + } + } + + patient = extract_patient_resource_from_item(item) + self.assertIsNotNone(patient) + self.assertIsInstance(patient, dict) + self.assertEqual(patient.get("resourceType"), "Patient") + self.assertEqual(patient.get("id"), "P1") + + def test_extract_from_json_string(self): + resource_json = '{"resourceType": "Immunization", "contained": [{"resourceType": "Patient", "id": "P2"}]}' + item = {"Resource": resource_json} + + patient = extract_patient_resource_from_item(item) + self.assertIsNotNone(patient) + self.assertEqual(patient.get("id"), "P2") + + def test_malformed_json_string_returns_none(self): + # A malformed JSON string should not raise, but return None + item = {"Resource": "{not: valid json}"} + self.assertIsNone(extract_patient_resource_from_item(item)) + + def test_non_dict_resource_returns_none(self): + item = {"Resource": 12345} + self.assertIsNone(extract_patient_resource_from_item(item)) + + def test_missing_resource_returns_none(self): + item = {} + self.assertIsNone(extract_patient_resource_from_item(item)) + + class TestIedsDbOperations(unittest.TestCase): """Base test class for IEDS database operations""" diff --git a/lambdas/shared/src/common/clients.py b/lambdas/shared/src/common/clients.py index 9f7bf308b..501418c12 100644 --- a/lambdas/shared/src/common/clients.py +++ b/lambdas/shared/src/common/clients.py @@ -2,9 +2,9 @@ import logging from boto3 import client as boto3_client, resource as boto3_resource -logging.basicConfig(level="INFO") +logging.basicConfig(level=logging.INFO) logger = logging.getLogger() -logger.setLevel("INFO") +logger.setLevel(logging.INFO) STREAM_NAME = os.getenv("SPLUNK_FIREHOSE_NAME", "firehose-name-not-defined") CONFIG_BUCKET_NAME = os.getenv("CONFIG_BUCKET_NAME", "variconfig-bucketable-not-defined") diff --git a/lambdas/shared/tests/test_common/test_clients.py b/lambdas/shared/tests/test_common/test_clients.py index 54f86ef8d..d9680a047 100644 --- a/lambdas/shared/tests/test_common/test_clients.py +++ b/lambdas/shared/tests/test_common/test_clients.py @@ -1,63 +1,61 @@ import unittest -from unittest.mock import patch -import common.clients as clients +from unittest.mock import patch, MagicMock +import logging import importlib +import common.clients as clients class TestClients(unittest.TestCase): - BUCKET_NAME = "default-bucket" AWS_REGION = "eu-west-2" def setUp(self): - self.boto3_client_patch = patch("boto3.client") + # Patch boto3.client + self.boto3_client_patch = patch("boto3.client", autospec=True) self.mock_boto3_client = self.boto3_client_patch.start() - self.logging_patch = patch("logging.getLogger") - self.mock_logging = self.logging_patch.start() - self.logger_info_patcher = patch("logging.Logger.info") - self.mock_logger_info = self.logger_info_patcher.start() - self.getenv_patch = patch("os.getenv") + self.addCleanup(self.boto3_client_patch.stop) + + # Patch logging.getLogger + self.logging_patch = patch("logging.getLogger", autospec=True) + self.mock_getLogger = self.logging_patch.start() + self.addCleanup(self.logging_patch.stop) + + # Patch os.getenv + self.getenv_patch = patch("os.getenv", autospec=True) self.mock_getenv = self.getenv_patch.start() + self.addCleanup(self.getenv_patch.stop) + + # Set environment variable mock return values self.mock_getenv.side_effect = lambda key, default=None: { "CONFIG_BUCKET_NAME": self.BUCKET_NAME, "AWS_REGION": self.AWS_REGION, }.get(key, default) - self.mock_boto3_client.return_value = self.mock_boto3_client - self.mock_boto3_client.return_value.send_message = {} - - def tearDown(self): - patch.stopall() + # Simulate logger instance and patch setLevel + self.mock_logger_instance = MagicMock() + self.mock_getLogger.return_value = self.mock_logger_instance - def test_os_environ(self): - # Test if environment variables are set correctly + # Reload the module under test to apply patches importlib.reload(clients) + + def test_env_variables_loaded(self): + """Test that environment variables are loaded correctly""" self.assertEqual(clients.CONFIG_BUCKET_NAME, self.BUCKET_NAME) self.assertEqual(clients.REGION_NAME, self.AWS_REGION) - def test_boto3_client(self): - ''' Test boto3 client is created with correct parameters ''' - importlib.reload(clients) + def test_boto3_client_created_for_s3(self): + """Test that S3 boto3 client is created with correct region""" self.mock_boto3_client.assert_any_call("s3", region_name=self.AWS_REGION) - def test_firehose_client(self): - ''' Test firehose client is created with correct parameters ''' - importlib.reload(clients) + def test_boto3_client_created_for_firehose(self): + """Test that Firehose boto3 client is created with correct region""" self.mock_boto3_client.assert_any_call("firehose", region_name=self.AWS_REGION) - def test_logging_setup(self): - ''' Test logging is set up correctly ''' - importlib.reload(clients) - self.assertTrue(hasattr(clients, 'logger')) + def test_logger_is_initialized(self): + """Test that a logger instance is initialized""" + self.mock_getLogger.assert_called_once_with() + self.assertTrue(hasattr(clients, "logger")) - def test_logging_configuration(self): - ''' Test logging configuration ''' - importlib.reload(clients) - clients.logger.setLevel.assert_called_once_with("INFO") - - def test_logging_initialization(self): - ''' Test logging initialization ''' - importlib.reload(clients) - self.mock_logging.assert_called_once_with() - self.assertTrue(hasattr(clients, 'logger')) - clients.logger.setLevel.assert_any_call("INFO") + def test_logger_set_level(self): + """Test that logger level is set to INFO""" + self.mock_logger_instance.setLevel.assert_called_once_with(logging.INFO)