From f7c58720dc37ac32ab5004533ab4f42eff2c4445 Mon Sep 17 00:00:00 2001 From: Akol125 Date: Fri, 12 Sep 2025 11:38:38 +0100 Subject: [PATCH 01/25] VED-755: refactor handler --- lambdas/id_sync/README.md | 6 +-- lambdas/id_sync/src/id_sync.py | 67 +++++++++++++++++------------- lambdas/id_sync/src/pds_details.py | 1 - 3 files changed, 38 insertions(+), 36 deletions(-) diff --git a/lambdas/id_sync/README.md b/lambdas/id_sync/README.md index 634805364..cfd2da256 100644 --- a/lambdas/id_sync/README.md +++ b/lambdas/id_sync/README.md @@ -28,8 +28,4 @@ - Code is located in the `lambdas/id_sync/src/` directory. - Unit tests are in the `lambdas/id_sync/tests/` directory. -- Use the provided Makefile and Dockerfile for building, testing, and packaging. - -## License - -This project is maintained by NHS. See [LICENSE](../LICENSE) for details. +- Use the provided Makefile and Dockerfile for building, testing, and packaging. \ No newline at end of file diff --git a/lambdas/id_sync/src/id_sync.py b/lambdas/id_sync/src/id_sync.py index 91855efef..6be7896ae 100644 --- a/lambdas/id_sync/src/id_sync.py +++ b/lambdas/id_sync/src/id_sync.py @@ -1,46 +1,53 @@ -from common.clients import logger -from common.clients import STREAM_NAME +from typing import Any, Dict + +from common.clients import logger, STREAM_NAME from common.log_decorator import logging_decorator from common.aws_lambda_event import AwsLambdaEvent from exceptions.id_sync_exception import IdSyncException from record_processor import process_record -''' -Lambda function handler for processing SQS events.Lambda for ID Sync. Fired by SQS -''' +""" +- Parses the incoming AWS event into `AwsLambdaEvent` and iterate its `records`. +- Delegate each record to `process_record` and collect `nhs_number` from each result. +- If any record has status == "error" raise `IdSyncException` with aggregated nhs_numbers. +- Any unexpected error is wrapped into `IdSyncException(message="Error processing id_sync event")`. +""" @logging_decorator(prefix="id_sync", stream_name=STREAM_NAME) -def handler(event_data, _): - +def handler(event_data: Dict[str, Any], _context) -> Dict[str, Any]: try: logger.info("id_sync handler invoked") event = AwsLambdaEvent(event_data) - record_count = len(event.records) - if record_count > 0: - logger.info("id_sync processing event with %d records", record_count) - error_count = 0 - nhs_numbers = [] - for record in event.records: - record_result = process_record(record) - nhs_numbers.append(record_result["nhs_number"]) - if record_result["status"] == "error": - error_count += 1 - if error_count > 0: - raise IdSyncException(message=f"Processed {record_count} records with {error_count} errors", - nhs_numbers=nhs_numbers) - - else: - response = {"status": "success", - "message": f"Successfully processed {record_count} records", - "nhs_numbers": nhs_numbers} - else: - response = {"status": "success", "message": "No records found in event"} + records = event.records + + if not records: + return {"status": "success", "message": "No records found in event"} + + logger.info("id_sync processing event with %d records", len(records)) + + # Process records in order. Let any unexpected exception bubble to the outer handler + # so tests that expect a wrapped IdSyncException keep working. + 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") + + if error_count: + raise IdSyncException(message=f"Processed {len(records)} records with {error_count} errors", + nhs_numbers=nhs_numbers) + + response = {"status": "success", + "message": f"Successfully processed {len(records)} records", + "nhs_numbers": nhs_numbers} + logger.info("id_sync handler completed: %s", response) return response + except IdSyncException as e: + # Preserve domain exceptions but ensure they're logged logger.exception(f"id_sync error: {e.message}") - raise e - except Exception as e: + raise + except Exception: msg = "Error processing id_sync event" logger.exception(msg) - raise IdSyncException(message=msg, exception=e) + # Raise a domain exception with a predictable message for callers/tests + raise IdSyncException(message=msg) diff --git a/lambdas/id_sync/src/pds_details.py b/lambdas/id_sync/src/pds_details.py index e8fecb5a7..1c340fbf1 100644 --- a/lambdas/id_sync/src/pds_details.py +++ b/lambdas/id_sync/src/pds_details.py @@ -48,7 +48,6 @@ def pds_get_patient_id(nhs_number: str) -> str: return patient_details["identifier"][0]["value"] - # ✅ Remove the IdSyncException catch since you're just re-raising except Exception as e: msg = f"Error getting PDS patient ID for {nhs_number}" logger.exception(msg) From 2f9d5e445b7fb9eaecbd42d094b36e0a4d58e0f7 Mon Sep 17 00:00:00 2001 From: Akol125 Date: Fri, 12 Sep 2025 12:16:45 +0100 Subject: [PATCH 02/25] VED-755: refactor handler2 --- lambdas/id_sync/src/id_sync.py | 1 + 1 file changed, 1 insertion(+) diff --git a/lambdas/id_sync/src/id_sync.py b/lambdas/id_sync/src/id_sync.py index 6be7896ae..acab352f8 100644 --- a/lambdas/id_sync/src/id_sync.py +++ b/lambdas/id_sync/src/id_sync.py @@ -13,6 +13,7 @@ - Any unexpected error is wrapped into `IdSyncException(message="Error processing id_sync event")`. """ + @logging_decorator(prefix="id_sync", stream_name=STREAM_NAME) def handler(event_data: Dict[str, Any], _context) -> Dict[str, Any]: try: From bc9f4bc3e6f1d66f04a36b4c5479bef1bc2de2d7 Mon Sep 17 00:00:00 2001 From: Akol125 Date: Fri, 12 Sep 2025 15:54:24 +0100 Subject: [PATCH 03/25] VED-755: Refactor existing id_sync codebase --- lambdas/id_sync/src/id_sync.py | 10 ++++---- lambdas/id_sync/src/pds_details.py | 2 ++ lambdas/id_sync/src/record_processor.py | 34 ++++++++++++------------- 3 files changed, 23 insertions(+), 23 deletions(-) diff --git a/lambdas/id_sync/src/id_sync.py b/lambdas/id_sync/src/id_sync.py index acab352f8..cc2bd9341 100644 --- a/lambdas/id_sync/src/id_sync.py +++ b/lambdas/id_sync/src/id_sync.py @@ -26,8 +26,6 @@ def handler(event_data: Dict[str, Any], _context) -> Dict[str, Any]: logger.info("id_sync processing event with %d records", len(records)) - # Process records in order. Let any unexpected exception bubble to the outer handler - # so tests that expect a wrapped IdSyncException keep working. 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") @@ -36,9 +34,11 @@ def handler(event_data: Dict[str, Any], _context) -> Dict[str, Any]: raise IdSyncException(message=f"Processed {len(records)} records with {error_count} errors", nhs_numbers=nhs_numbers) - response = {"status": "success", - "message": f"Successfully processed {len(records)} records", - "nhs_numbers": nhs_numbers} + response = { + "status": "success", + "message": f"Successfully processed {len(records)} records", + "nhs_numbers": nhs_numbers + } logger.info("id_sync handler completed: %s", response) return response diff --git a/lambdas/id_sync/src/pds_details.py b/lambdas/id_sync/src/pds_details.py index 1c340fbf1..4729a0aec 100644 --- a/lambdas/id_sync/src/pds_details.py +++ b/lambdas/id_sync/src/pds_details.py @@ -13,6 +13,7 @@ safe_tmp_dir = tempfile.mkdtemp(dir="/tmp") # NOSONAR +# Get Patient details from external service PDS using NHS number from MNS notification def pds_get_patient_details(nhs_number: str) -> dict: try: logger.info(f"get patient details. nhs_number: {nhs_number}") @@ -34,6 +35,7 @@ def pds_get_patient_details(nhs_number: str) -> dict: raise IdSyncException(message=msg, exception=e) +# Extract Patient identifier value from PDS patient details def pds_get_patient_id(nhs_number: str) -> str: """ Get PDS patient ID from NHS number. diff --git a/lambdas/id_sync/src/record_processor.py b/lambdas/id_sync/src/record_processor.py index 146095431..2ff9407c2 100644 --- a/lambdas/id_sync/src/record_processor.py +++ b/lambdas/id_sync/src/record_processor.py @@ -1,26 +1,20 @@ -''' - record Processor -''' from common.clients import logger -from typing import Optional +from typing import Dict, Any from pds_details import pds_get_patient_id from ieds_db_operations import ieds_check_exist, ieds_update_patient_id import json import ast -def process_record(event_record): +def process_record(event_record) -> Dict[str, Any]: logger.info("process_record. Processing record: %s", event_record) body_text = event_record.get('body', '') - # convert body to json if isinstance(body_text, str): try: - # Try JSON first body = json.loads(body_text) except json.JSONDecodeError: try: - # Fall back to Python dict syntax body = ast.literal_eval(body_text) except (ValueError, SyntaxError): logger.error("Failed to parse body: %s", body_text) @@ -30,26 +24,25 @@ def process_record(event_record): nhs_number = body.get("subject") logger.info("process record NHS number: %s", nhs_number) if nhs_number: + if not _is_valid_nhs(nhs_number): + logger.error("Invalid NHS number format: %s", nhs_number) + return {"status": "error", "message": "Invalid NHS number format", "nhs_number": nhs_number} return process_nhs_number(nhs_number) else: logger.info("No NHS number found in event record") return {"status": "error", "message": "No NHS number found in event record"} -def process_nhs_number(nhs_number: str) -> Optional[str]: +def process_nhs_number(nhs_number: str) -> Dict[str, Any]: # get patient details from PDS - logger.info(f"process_nhs_number. Processing NHS number: {nhs_number}") - patient_details_id = pds_get_patient_id(nhs_number) + new_nhs_number = pds_get_patient_id(nhs_number) base_log_data = {"nhs_number": nhs_number} - if patient_details_id: - logger.info(f"process_nhs_number. Patient details ID: {patient_details_id}") - # if patient NHS != id, update patient index of vax events to new number - if patient_details_id != nhs_number and patient_details_id: - logger.info(f"process_nhs_number. Update patient ID from {nhs_number} to {patient_details_id}") + if new_nhs_number: + if new_nhs_number != nhs_number: + logger.info("process_nhs_number. Update patient ID from %s to %s", nhs_number, new_nhs_number) if ieds_check_exist(nhs_number): - logger.info("process_nhs_number. IEDS record found, updating patient ID") - response = ieds_update_patient_id(nhs_number, patient_details_id) + response = ieds_update_patient_id(nhs_number, new_nhs_number) else: logger.info("process_nhs_number. No ieds record found for: %s", nhs_number) response = {"status": "success", "message": f"No records returned for ID: {nhs_number}"} @@ -59,3 +52,8 @@ def process_nhs_number(nhs_number: str) -> Optional[str]: response = {"status": "success", "message": f"No patient ID found for NHS number: {nhs_number}"} response.update(base_log_data) return response + + +def _is_valid_nhs(nhs: str) -> bool: + """Basic validation: NHS number must be 10 digits. (Optional: add MOD11 check later)""" + return isinstance(nhs, str) and nhs.isdigit() and len(nhs) == 10 From 13610c5c9a59e8f7d713b956bc7fea47f9367588 Mon Sep 17 00:00:00 2001 From: Akol125 Date: Fri, 12 Sep 2025 16:29:58 +0100 Subject: [PATCH 04/25] VED-755: refactoring record_processor --- lambdas/id_sync/src/record_processor.py | 54 ++++++++++--------- .../id_sync/tests/test_record_processor.py | 8 +-- 2 files changed, 33 insertions(+), 29 deletions(-) diff --git a/lambdas/id_sync/src/record_processor.py b/lambdas/id_sync/src/record_processor.py index 2ff9407c2..ad28c48c7 100644 --- a/lambdas/id_sync/src/record_processor.py +++ b/lambdas/id_sync/src/record_processor.py @@ -6,10 +6,12 @@ import ast -def process_record(event_record) -> Dict[str, Any]: +def process_record(event_record: Dict[str, Any]) -> Dict[str, Any]: logger.info("process_record. Processing record: %s", event_record) body_text = event_record.get('body', '') + + # convert body to json (try JSON first, then fall back to Python literal) if isinstance(body_text, str): try: body = json.loads(body_text) @@ -21,39 +23,41 @@ def process_record(event_record) -> Dict[str, Any]: return {"status": "error", "message": "Invalid body format"} else: body = body_text + nhs_number = body.get("subject") logger.info("process record NHS number: %s", nhs_number) if nhs_number: - if not _is_valid_nhs(nhs_number): - logger.error("Invalid NHS number format: %s", nhs_number) - return {"status": "error", "message": "Invalid NHS number format", "nhs_number": nhs_number} return process_nhs_number(nhs_number) - else: - logger.info("No NHS number found in event record") - return {"status": "error", "message": "No NHS number found in event record"} + + logger.info("No NHS number found in event record") + return {"status": "error", "message": "No NHS number found in event record"} def process_nhs_number(nhs_number: str) -> Dict[str, Any]: # get patient details from PDS new_nhs_number = pds_get_patient_id(nhs_number) - base_log_data = {"nhs_number": nhs_number} - if new_nhs_number: - if new_nhs_number != nhs_number: - logger.info("process_nhs_number. Update patient ID from %s to %s", nhs_number, new_nhs_number) - if ieds_check_exist(nhs_number): - response = ieds_update_patient_id(nhs_number, new_nhs_number) - else: - logger.info("process_nhs_number. No ieds record found for: %s", nhs_number) - response = {"status": "success", "message": f"No records returned for ID: {nhs_number}"} - else: - response = {"status": "success", "message": "No update required"} + if not new_nhs_number: + return { + "status": "success", + "message": "No patient ID found for NHS number", + "nhs_number": nhs_number, + } + + if new_nhs_number == nhs_number: + return { + "status": "success", + "message": "No update required", + "nhs_number": nhs_number, + } + + logger.info("Update patient ID from %s to %s", nhs_number, new_nhs_number) + + if ieds_check_exist(nhs_number): + response = ieds_update_patient_id(nhs_number, new_nhs_number) else: - response = {"status": "success", "message": f"No patient ID found for NHS number: {nhs_number}"} - response.update(base_log_data) - return response - + logger.info("No IEDS record found for: %s", nhs_number) + response = {"status": "success", "message": f"No records returned for ID: {nhs_number}"} -def _is_valid_nhs(nhs: str) -> bool: - """Basic validation: NHS number must be 10 digits. (Optional: add MOD11 check later)""" - return isinstance(nhs, str) and nhs.isdigit() and len(nhs) == 10 + response["nhs_number"] = nhs_number + return response \ No newline at end of file diff --git a/lambdas/id_sync/tests/test_record_processor.py b/lambdas/id_sync/tests/test_record_processor.py index 0d301ce25..e7d95ec99 100644 --- a/lambdas/id_sync/tests/test_record_processor.py +++ b/lambdas/id_sync/tests/test_record_processor.py @@ -45,8 +45,8 @@ def test_process_record_success_no_update_required(self): def test_process_record_success_update_required(self): """Test successful processing when patient ID differs""" # Arrange - pds_id = "pds-id-1" - nhs_number = "nhs-number-1" + pds_id = "9000000008" + nhs_number = "9000000009" test_sqs_record = {"body": {"subject": nhs_number}} self.mock_pds_get_patient_id.return_value = pds_id @@ -54,7 +54,7 @@ def test_process_record_success_update_required(self): self.mock_ieds_update_patient_id.return_value = success_response # Act result = process_record(test_sqs_record) - + self.maxDiff = None # Assert expected_result = success_response self.assertEqual(result, expected_result) @@ -89,7 +89,7 @@ def test_process_record_pds_returns_none_id(self): # Act & Assert result = process_record(test_record) self.assertEqual(result["status"], "success") - self.assertEqual(result["message"], f"No patient ID found for NHS number: {test_id}") + self.assertEqual(result["message"], f"No patient ID found for NHS number") self.mock_ieds_check_exist.assert_not_called() self.mock_ieds_update_patient_id.assert_not_called() From bac82fef74e98e373057ec4cc4408a8740ed029c Mon Sep 17 00:00:00 2001 From: Akol125 Date: Mon, 15 Sep 2025 05:30:38 +0100 Subject: [PATCH 05/25] resolve lint --- lambdas/id_sync/src/record_processor.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lambdas/id_sync/src/record_processor.py b/lambdas/id_sync/src/record_processor.py index ad28c48c7..8335d46f7 100644 --- a/lambdas/id_sync/src/record_processor.py +++ b/lambdas/id_sync/src/record_processor.py @@ -50,7 +50,6 @@ def process_nhs_number(nhs_number: str) -> Dict[str, Any]: "message": "No update required", "nhs_number": nhs_number, } - logger.info("Update patient ID from %s to %s", nhs_number, new_nhs_number) if ieds_check_exist(nhs_number): @@ -60,4 +59,4 @@ def process_nhs_number(nhs_number: str) -> Dict[str, Any]: response = {"status": "success", "message": f"No records returned for ID: {nhs_number}"} response["nhs_number"] = nhs_number - return response \ No newline at end of file + return response From 516ac9a683fddf61156b152abcfcb86ec55e3ce0 Mon Sep 17 00:00:00 2001 From: Akol125 Date: Mon, 15 Sep 2025 06:25:05 +0100 Subject: [PATCH 06/25] resolve lint2 --- lambdas/id_sync/src/id_sync.py | 3 --- lambdas/id_sync/tests/test_record_processor.py | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/lambdas/id_sync/src/id_sync.py b/lambdas/id_sync/src/id_sync.py index cc2bd9341..c3a243dd0 100644 --- a/lambdas/id_sync/src/id_sync.py +++ b/lambdas/id_sync/src/id_sync.py @@ -17,7 +17,6 @@ @logging_decorator(prefix="id_sync", stream_name=STREAM_NAME) def handler(event_data: Dict[str, Any], _context) -> Dict[str, Any]: try: - logger.info("id_sync handler invoked") event = AwsLambdaEvent(event_data) records = event.records @@ -44,11 +43,9 @@ def handler(event_data: Dict[str, Any], _context) -> Dict[str, Any]: return response except IdSyncException as e: - # Preserve domain exceptions but ensure they're logged logger.exception(f"id_sync error: {e.message}") raise except Exception: msg = "Error processing id_sync event" logger.exception(msg) - # Raise a domain exception with a predictable message for callers/tests raise IdSyncException(message=msg) diff --git a/lambdas/id_sync/tests/test_record_processor.py b/lambdas/id_sync/tests/test_record_processor.py index e7d95ec99..b1348a785 100644 --- a/lambdas/id_sync/tests/test_record_processor.py +++ b/lambdas/id_sync/tests/test_record_processor.py @@ -89,7 +89,7 @@ def test_process_record_pds_returns_none_id(self): # Act & Assert result = process_record(test_record) self.assertEqual(result["status"], "success") - self.assertEqual(result["message"], f"No patient ID found for NHS number") + self.assertEqual(result["message"], "No patient ID found for NHS number") self.mock_ieds_check_exist.assert_not_called() self.mock_ieds_update_patient_id.assert_not_called() From cd020663601c5af287a665d21ecc203361ffc85c Mon Sep 17 00:00:00 2001 From: Akol125 Date: Mon, 15 Sep 2025 12:18:24 +0100 Subject: [PATCH 07/25] add logic for demographics match and integrate with nhs number processing --- lambdas/id_sync/src/ieds_db_operations.py | 39 +++++++ lambdas/id_sync/src/pds_details.py | 19 ++++ lambdas/id_sync/src/record_processor.py | 126 +++++++++++++++++++++- 3 files changed, 181 insertions(+), 3 deletions(-) diff --git a/lambdas/id_sync/src/ieds_db_operations.py b/lambdas/id_sync/src/ieds_db_operations.py index d5bf82f7d..3b9f71d64 100644 --- a/lambdas/id_sync/src/ieds_db_operations.py +++ b/lambdas/id_sync/src/ieds_db_operations.py @@ -1,3 +1,4 @@ +import json from boto3.dynamodb.conditions import Key from os_vars import get_ieds_table_name from common.aws_dynamodb import get_dynamodb_table @@ -141,3 +142,41 @@ def get_items_from_patient_id(id: str, limit=BATCH_SIZE) -> list: nhs_numbers=[patient_pk], exception=e ) + +def extract_patient_resource_from_item(item: dict) -> dict | None: + """Extract a Patient resource dict from an IEDS item. + + Accepts common shapes: item['Resource'] (dict or JSON string), item['resource'], or a wrapper with 'resource'. + Returns the patient resource dict or None if not found/parsible. + """ + if not isinstance(item, dict): + return None + + candidate = item.get("Resource") or item.get("resource") or item.get("Body") or item.get("body") + if not candidate: + return None + + # candidate might be a JSON string + if isinstance(candidate, str): + try: + candidate = json.loads(candidate) + except Exception: + return None + + if isinstance(candidate, dict): + # if wrapped like {"resource": {...}} + if "resource" in candidate and isinstance(candidate["resource"], dict): + candidate = candidate["resource"] + + # If this dict is the Patient resource itself + if candidate.get("resourceType") == "Patient": + return candidate + + # If it's a bundle/entry, search for Patient + if isinstance(candidate.get("entry"), list): + for entry in candidate.get("entry", []): + r = entry.get("resource") or entry.get("Resource") + if isinstance(r, dict) and r.get("resourceType") == "Patient": + return r + + return None \ No newline at end of file diff --git a/lambdas/id_sync/src/pds_details.py b/lambdas/id_sync/src/pds_details.py index 4729a0aec..dce3d12d2 100644 --- a/lambdas/id_sync/src/pds_details.py +++ b/lambdas/id_sync/src/pds_details.py @@ -54,3 +54,22 @@ def pds_get_patient_id(nhs_number: str) -> str: msg = f"Error getting PDS patient ID for {nhs_number}" logger.exception(msg) raise IdSyncException(message=msg, exception=e) + +def normalize_name_from_pds(pds_get_patient_details: dict) -> str | None: + """Return a normalized full name (given + family) from PDS patient details or None.""" + try: + name = pds_get_patient_details.get("name") + if not name: + return None + name_entry = name[0] if isinstance(name, list) else name + given = name_entry.get("given") + given_str = None + if isinstance(given, list) and given: + given_str = given[0] + elif isinstance(given, str): + given_str = given + family = name_entry.get("family") + parts = [p for p in [given_str, family] if p] + return " ".join(parts).strip().lower() if parts else None + except Exception: + return None diff --git a/lambdas/id_sync/src/record_processor.py b/lambdas/id_sync/src/record_processor.py index 8335d46f7..aa56d03ae 100644 --- a/lambdas/id_sync/src/record_processor.py +++ b/lambdas/id_sync/src/record_processor.py @@ -1,7 +1,12 @@ from common.clients import logger from typing import Dict, Any -from pds_details import pds_get_patient_id -from ieds_db_operations import ieds_check_exist, ieds_update_patient_id +from pds_details import pds_get_patient_id, pds_get_patient_details, normalize_name_from_pds +from ieds_db_operations import ( + ieds_check_exist, + ieds_update_patient_id, + extract_patient_resource_from_item, + get_items_from_patient_id, +) import json import ast @@ -53,10 +58,125 @@ def process_nhs_number(nhs_number: str) -> Dict[str, Any]: logger.info("Update patient ID from %s to %s", nhs_number, new_nhs_number) if ieds_check_exist(nhs_number): - response = ieds_update_patient_id(nhs_number, new_nhs_number) + # Fetch PDS details for demographic comparison + try: + pds_details = pds_get_patient_details(nhs_number) + except Exception: + logger.exception("process_nhs_number: failed to fetch PDS details, aborting update") + return { + "status": "error", + "message": "Failed to fetch PDS details for demographic comparison", + "nhs_number": nhs_number, + } + + # Get IEDS items for this patient id and compare demographics + try: + items = get_items_from_patient_id(nhs_number) + except Exception: + logger.exception("process_nhs_number: failed to fetch IEDS items, aborting update") + return { + "status": "error", + "message": "Failed to fetch IEDS items for demographic comparison", + "nhs_number": nhs_number, + } + + # If at least one IEDS item matches demographics, proceed with update + match_found = False + for item in items: + try: + if demographics_match(pds_details, item): + match_found = True + break + except Exception: + logger.exception("process_nhs_number: error while comparing demographics for item: %s", item) + + if not match_found: + logger.info("process_nhs_number: No IEDS items matched PDS demographics. Skipping update for %s", nhs_number) + response = { + "status": "success", + "message": "No IEDS items matched PDS demographics; update skipped", + } + else: + response = ieds_update_patient_id(nhs_number, new_nhs_number) else: logger.info("No IEDS record found for: %s", nhs_number) response = {"status": "success", "message": f"No records returned for ID: {nhs_number}"} response["nhs_number"] = nhs_number return response + +def extract_normalized_name_from_patient(patient: dict) -> str | None: + """Return a normalized 'given family' name string from a Patient resource or None.""" + if not patient: + return None + name = patient.get("name") + if not name: + return None + try: + name_entry = name[0] if isinstance(name, list) else name + given = name_entry.get("given") + given_str = None + if isinstance(given, list) and given: + given_str = given[0] + elif isinstance(given, str): + given_str = given + family = name_entry.get("family") + parts = [p for p in [given_str, family] if p] + return " ".join(parts).strip().lower() if parts else None + except Exception: + return None + + +def demographics_match(pds_details: dict, ieds_item: dict) -> bool: + """Compare PDS patient details to an IEDS item (FHIR Patient resource). + + Parameters: + - pds_details: dict returned by PDS (patient details) + - ieds_item: dict representing a single IEDS item containing a FHIR Patient resource + + Returns True if name, birthDate and gender match (when present in both sources). + If required fields are missing or unparsable on the IEDS side the function returns False. + """ + try: + # extract pds values + pds_name = normalize_name_from_pds(pds_details) if isinstance(pds_details, dict) else None + pds_gender = pds_details.get("gender") if isinstance(pds_details, dict) else None + pds_birth = pds_details.get("birthDate") if isinstance(pds_details, dict) else None + + patient = extract_patient_resource_from_item(ieds_item) + if not patient: + logger.debug("demographics_match: no patient resource in item") + return False + + # normalize incoming patient name + incoming_name = extract_normalized_name_from_patient(patient) + + incoming_gender = patient.get("gender") + incoming_birth = patient.get("birthDate") + + def _norm_str(x): + return str(x).strip().lower() if x is not None else None + + # Compare birthDate (strict if both present) + if pds_birth and incoming_birth: + if str(pds_birth).strip() != str(incoming_birth).strip(): + logger.debug("demographics_match: birthDate mismatch %s != %s", pds_birth, incoming_birth) + return False + + # Compare gender (case-insensitive) + if pds_gender and incoming_gender: + if _norm_str(pds_gender) != _norm_str(incoming_gender): + logger.debug("demographics_match: gender mismatch %s != %s", pds_gender, incoming_gender) + return False + + # Compare names if both present (normalized) + if pds_name and incoming_name: + if _norm_str(pds_name) != _norm_str(incoming_name): + logger.debug("demographics_match: name mismatch %s != %s", pds_name, incoming_name) + return False + + # If we reached here, all present fields matched (or were not present to compare) + return True + except Exception: + logger.exception("demographics_match: comparison failed with exception") + return False \ No newline at end of file From b36d8ed91657c9ffc273a1c2015334c8ff3c53c4 Mon Sep 17 00:00:00 2001 From: Akol125 Date: Mon, 15 Sep 2025 12:54:38 +0100 Subject: [PATCH 08/25] add unit test for demographics test --- .../id_sync/tests/test_record_processor.py | 87 +++++++++++++++++-- 1 file changed, 78 insertions(+), 9 deletions(-) diff --git a/lambdas/id_sync/tests/test_record_processor.py b/lambdas/id_sync/tests/test_record_processor.py index b1348a785..cc953997d 100644 --- a/lambdas/id_sync/tests/test_record_processor.py +++ b/lambdas/id_sync/tests/test_record_processor.py @@ -7,18 +7,27 @@ class TestRecordProcessor(unittest.TestCase): def setUp(self): """Set up test fixtures and mocks""" + # Patch logger self.logger_patcher = patch('record_processor.logger') self.mock_logger = self.logger_patcher.start() + # PDS helpers self.pds_get_patient_id_patcher = patch('record_processor.pds_get_patient_id') self.mock_pds_get_patient_id = self.pds_get_patient_id_patcher.start() + self.pds_get_patient_details_patcher = patch('record_processor.pds_get_patient_details') + self.mock_pds_get_patient_details = self.pds_get_patient_details_patcher.start() + + # IEDS helpers self.ieds_check_exist_patcher = patch('record_processor.ieds_check_exist') self.mock_ieds_check_exist = self.ieds_check_exist_patcher.start() self.ieds_update_patient_id_patcher = patch('record_processor.ieds_update_patient_id') self.mock_ieds_update_patient_id = self.ieds_update_patient_id_patcher.start() + self.get_items_from_patient_id_patcher = patch('record_processor.get_items_from_patient_id') + self.mock_get_items_from_patient_id = self.get_items_from_patient_id_patcher.start() + def tearDown(self): patch.stopall() @@ -27,7 +36,6 @@ def test_process_record_success_no_update_required(self): # Arrange test_id = "54321" with patch('record_processor.ieds_check_exist', return_value=True): - test_record = {"body": {"subject": test_id}} self.mock_pds_get_patient_id.return_value = test_id @@ -43,34 +51,82 @@ def test_process_record_success_no_update_required(self): self.mock_pds_get_patient_id.assert_called_once_with(test_id) def test_process_record_success_update_required(self): - """Test successful processing when patient ID differs""" + """Test successful processing when patient ID differs and demographics match""" # Arrange pds_id = "9000000008" nhs_number = "9000000009" test_sqs_record = {"body": {"subject": nhs_number}} self.mock_pds_get_patient_id.return_value = pds_id + + # pds_get_patient_details should return details used by demographics_match + self.mock_pds_get_patient_details.return_value = { + "name": [{"given": ["John"], "family": "Doe"}], + "gender": "male", + "birthDate": "1980-01-01", + } + + # Provide one IEDS item that will match demographics via demographics_match + matching_item = { + "Resource": { + "resourceType": "Patient", + "name": [{"given": ["John"], "family": "Doe"}], + "gender": "male", + "birthDate": "1980-01-01", + } + } + self.mock_get_items_from_patient_id.return_value = [matching_item] + success_response = {"status": "success"} self.mock_ieds_update_patient_id.return_value = success_response + # Act result = process_record(test_sqs_record) - self.maxDiff = None - # Assert - expected_result = success_response - self.assertEqual(result, expected_result) - # Verify calls + # Assert + self.assertEqual(result, success_response) self.mock_pds_get_patient_id.assert_called_once_with(nhs_number) + def test_process_record_demographics_mismatch_skips_update(self): + """If no IEDS item matches demographics, the update should be skipped""" + # Arrange + pds_id = "pds-1" + nhs_number = "nhs-1" + test_sqs_record = {"body": {"subject": nhs_number}} + + self.mock_pds_get_patient_id.return_value = pds_id + self.mock_pds_get_patient_details.return_value = { + "name": [{"given": ["Alice"], "family": "Smith"}], + "gender": "female", + "birthDate": "1995-05-05", + } + + # IEDS items exist but do not match demographics + non_matching_item = { + "Resource": { + "resourceType": "Patient", + "name": [{"given": ["Bob"], "family": "Jones"}], + "gender": "male", + "birthDate": "1990-01-01", + } + } + self.mock_get_items_from_patient_id.return_value = [non_matching_item] + + # Act + result = process_record(test_sqs_record) + + # Assert: update skipped + self.assertEqual(result["status"], "success") + self.assertIn("update skipped", result["message"]) + def test_process_record_no_records_exist(self): """Test when no records exist for the patient ID""" - # Arrange test_id = "12345" with patch('record_processor.ieds_check_exist', return_value=False): + test_record = {"body": {"subject": test_id}} # Act - test_record = {"body": {"subject": test_id}} result = process_record(test_record) # Assert @@ -86,6 +142,7 @@ def test_process_record_pds_returns_none_id(self): test_id = "12345a" self.mock_pds_get_patient_id.return_value = None test_record = {"body": {"subject": test_id}} + # Act & Assert result = process_record(test_record) self.assertEqual(result["status"], "success") @@ -112,6 +169,18 @@ def test_body_is_string(self): new_test_id = "nhs-number-2" self.mock_pds_get_patient_id.return_value = new_test_id + # Mock demographics so update proceeds + self.mock_pds_get_patient_details.return_value = { + "name": [{"given": ["A"], "family": "B"}], + "gender": "female", "birthDate": "1990-01-01" + } + self.mock_get_items_from_patient_id.return_value = [{ + "Resource": { + "resourceType": "Patient", + "name": [{"given": ["A"], "family": "B"}], + "gender": "female", "birthDate": "1990-01-01" + } + }] self.mock_ieds_update_patient_id.return_value = {"status": "success"} # Act result = process_record(test_record) From 001fda1b1d2bc8b1d40fa76cb8cd4991017711d2 Mon Sep 17 00:00:00 2001 From: Akol125 Date: Mon, 15 Sep 2025 17:34:02 +0100 Subject: [PATCH 09/25] refactor all functions --- lambdas/id_sync/src/ieds_db_operations.py | 42 ++++--------------- .../id_sync/tests/test_ieds_db_operations.py | 39 +++++++++++++++-- 2 files changed, 45 insertions(+), 36 deletions(-) diff --git a/lambdas/id_sync/src/ieds_db_operations.py b/lambdas/id_sync/src/ieds_db_operations.py index 3b9f71d64..4b047e951 100644 --- a/lambdas/id_sync/src/ieds_db_operations.py +++ b/lambdas/id_sync/src/ieds_db_operations.py @@ -1,4 +1,3 @@ -import json from boto3.dynamodb.conditions import Key from os_vars import get_ieds_table_name from common.aws_dynamodb import get_dynamodb_table @@ -143,40 +142,17 @@ def get_items_from_patient_id(id: str, limit=BATCH_SIZE) -> list: exception=e ) -def extract_patient_resource_from_item(item: dict) -> dict | None: - """Extract a Patient resource dict from an IEDS item. - Accepts common shapes: item['Resource'] (dict or JSON string), item['resource'], or a wrapper with 'resource'. - Returns the patient resource dict or None if not found/parsible. +def extract_patient_resource_from_item(item: dict) -> dict | None: """ - if not isinstance(item, dict): + Extract a Patient resource dict from an IEDS database. + """ + patient_resource = item.get("Resource", None) + if not isinstance(patient_resource, dict): return None - candidate = item.get("Resource") or item.get("resource") or item.get("Body") or item.get("body") - if not candidate: - return None + for res in patient_resource.get("contained", []): + if isinstance(res, dict) and res.get("resourceType") == "Patient": + return res - # candidate might be a JSON string - if isinstance(candidate, str): - try: - candidate = json.loads(candidate) - except Exception: - return None - - if isinstance(candidate, dict): - # if wrapped like {"resource": {...}} - if "resource" in candidate and isinstance(candidate["resource"], dict): - candidate = candidate["resource"] - - # If this dict is the Patient resource itself - if candidate.get("resourceType") == "Patient": - return candidate - - # If it's a bundle/entry, search for Patient - if isinstance(candidate.get("entry"), list): - for entry in candidate.get("entry", []): - r = entry.get("resource") or entry.get("Resource") - if isinstance(r, dict) and r.get("resourceType") == "Patient": - return r - - return None \ No newline at end of file + return None diff --git a/lambdas/id_sync/tests/test_ieds_db_operations.py b/lambdas/id_sync/tests/test_ieds_db_operations.py index b0a0151cf..1effa175c 100644 --- a/lambdas/id_sync/tests/test_ieds_db_operations.py +++ b/lambdas/id_sync/tests/test_ieds_db_operations.py @@ -673,6 +673,39 @@ def test_ieds_check_exist_limit_parameter(self): # Assert - Verify the limit parameter is correctly passed self.mock_get_items_from_patient_id.assert_called_once_with(patient_id, 1) - # ✅ Remove tests that are no longer relevant: - # - test_ieds_check_exist_missing_count_field (no longer uses Count) - # - test_ieds_check_exist_count_greater_than_one (no longer uses Count) + + +class TestExtractPatientResource(TestIedsDbOperations): + def test_extract_patient_from_immunization_contained(self): + """Should return the contained Patient resource from an Immunization Resource""" + item = { + "Resource": { + "resourceType": "Immunization", + "contained": [ + { + "resourceType": "Patient", + "id": "Pat1", + "name": [{"family": "Taylor", "given": ["Sarah"]}], + "gender": "unknown", + "birthDate": "1965-02-28" + } + ] + } + } + + patient = ieds_db_operations.extract_patient_resource_from_item(item) + self.assertIsNotNone(patient) + self.assertEqual(patient.get("resourceType"), "Patient") + self.assertEqual(patient.get("id"), "Pat1") + + def test_extract_returns_none_when_no_patient_in_contained(self): + """Should return None when contained exists but has no Patient entries""" + item = {"Resource": {"resourceType": "Immunization", "contained": [{"resourceType": "Practitioner"}]}} + patient = ieds_db_operations.extract_patient_resource_from_item(item) + self.assertIsNone(patient) + + def test_extract_returns_none_when_resource_not_dict(self): + """Should return None when Resource attribute is present but not a dict""" + item = {"Resource": '{"not": "a dict string"}'} + patient = ieds_db_operations.extract_patient_resource_from_item(item) + self.assertIsNone(patient) From 973c8a2b4bb321528d621eb81cdb5d1c3887b833 Mon Sep 17 00:00:00 2001 From: Akol125 Date: Tue, 16 Sep 2025 08:44:35 +0100 Subject: [PATCH 10/25] VED-755: Adding demographics logic --- lambdas/id_sync/src/ieds_db_operations.py | 6 +- lambdas/id_sync/src/record_processor.py | 60 ++++++++----------- .../id_sync/tests/test_ieds_db_operations.py | 36 ----------- 3 files changed, 28 insertions(+), 74 deletions(-) diff --git a/lambdas/id_sync/src/ieds_db_operations.py b/lambdas/id_sync/src/ieds_db_operations.py index 4b047e951..54bae75ab 100644 --- a/lambdas/id_sync/src/ieds_db_operations.py +++ b/lambdas/id_sync/src/ieds_db_operations.py @@ -151,8 +151,8 @@ def extract_patient_resource_from_item(item: dict) -> dict | None: if not isinstance(patient_resource, dict): return None - for res in patient_resource.get("contained", []): - if isinstance(res, dict) and res.get("resourceType") == "Patient": - return res + for response in patient_resource.get("contained", []): + if isinstance(response, dict) and response.get("resourceType") == "Patient": + return response return None diff --git a/lambdas/id_sync/src/record_processor.py b/lambdas/id_sync/src/record_processor.py index aa56d03ae..04106d976 100644 --- a/lambdas/id_sync/src/record_processor.py +++ b/lambdas/id_sync/src/record_processor.py @@ -128,54 +128,44 @@ def extract_normalized_name_from_patient(patient: dict) -> str | None: def demographics_match(pds_details: dict, ieds_item: dict) -> bool: - """Compare PDS patient details to an IEDS item (FHIR Patient resource). - - Parameters: - - pds_details: dict returned by PDS (patient details) - - ieds_item: dict representing a single IEDS item containing a FHIR Patient resource - + """Compare PDS patient details from PDS to an IEDS item (FHIR Patient resource). Returns True if name, birthDate and gender match (when present in both sources). If required fields are missing or unparsable on the IEDS side the function returns False. """ try: - # extract pds values - pds_name = normalize_name_from_pds(pds_details) if isinstance(pds_details, dict) else None - pds_gender = pds_details.get("gender") if isinstance(pds_details, dict) else None - pds_birth = pds_details.get("birthDate") if isinstance(pds_details, dict) else None - + + def normalize_strings(item: Any) -> str | None: + return str(item).strip().lower() if item else None + + # Retrieve patient resource from PDS + pds_name = normalize_strings(normalize_name_from_pds(pds_details)) + pds_gender = normalize_strings(pds_details.get("gender")) + pds_birth = normalize_strings(pds_details.get("birthDate")) + + # 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 item") + logger.debug("demographics_match: no patient resource in IEDS table item") return False - # normalize incoming patient name - incoming_name = extract_normalized_name_from_patient(patient) - - incoming_gender = patient.get("gender") - incoming_birth = patient.get("birthDate") + # normalize patient name + ieds_name = extract_normalized_name_from_patient(patient) - def _norm_str(x): - return str(x).strip().lower() if x is not None else None + ieds_gender = normalize_strings(patient.get("gender")) + ieds_birth = patient.get("birthDate") - # Compare birthDate (strict if both present) - if pds_birth and incoming_birth: - if str(pds_birth).strip() != str(incoming_birth).strip(): - logger.debug("demographics_match: birthDate mismatch %s != %s", pds_birth, incoming_birth) - return False + if pds_birth and ieds_birth and pds_birth != ieds_birth: + logger.debug("demographics_match: birthDate mismatch %s != %s", pds_birth, ieds_birth) + return False - # Compare gender (case-insensitive) - if pds_gender and incoming_gender: - if _norm_str(pds_gender) != _norm_str(incoming_gender): - logger.debug("demographics_match: gender mismatch %s != %s", pds_gender, incoming_gender) - return False + if pds_gender and ieds_gender and pds_gender != ieds_gender: + logger.debug("demographics_match: gender mismatch %s != %s", pds_gender, ieds_gender) + return False - # Compare names if both present (normalized) - if pds_name and incoming_name: - if _norm_str(pds_name) != _norm_str(incoming_name): - logger.debug("demographics_match: name mismatch %s != %s", pds_name, incoming_name) - return False + if pds_name and ieds_name and pds_name != ieds_name: + logger.debug("demographics_match: name mismatch %s != %s", pds_name, ieds_name) + return False - # If we reached here, all present fields matched (or were not present to compare) return True except Exception: logger.exception("demographics_match: comparison failed with exception") diff --git a/lambdas/id_sync/tests/test_ieds_db_operations.py b/lambdas/id_sync/tests/test_ieds_db_operations.py index 1effa175c..4848e31d0 100644 --- a/lambdas/id_sync/tests/test_ieds_db_operations.py +++ b/lambdas/id_sync/tests/test_ieds_db_operations.py @@ -673,39 +673,3 @@ def test_ieds_check_exist_limit_parameter(self): # Assert - Verify the limit parameter is correctly passed self.mock_get_items_from_patient_id.assert_called_once_with(patient_id, 1) - - -class TestExtractPatientResource(TestIedsDbOperations): - def test_extract_patient_from_immunization_contained(self): - """Should return the contained Patient resource from an Immunization Resource""" - item = { - "Resource": { - "resourceType": "Immunization", - "contained": [ - { - "resourceType": "Patient", - "id": "Pat1", - "name": [{"family": "Taylor", "given": ["Sarah"]}], - "gender": "unknown", - "birthDate": "1965-02-28" - } - ] - } - } - - patient = ieds_db_operations.extract_patient_resource_from_item(item) - self.assertIsNotNone(patient) - self.assertEqual(patient.get("resourceType"), "Patient") - self.assertEqual(patient.get("id"), "Pat1") - - def test_extract_returns_none_when_no_patient_in_contained(self): - """Should return None when contained exists but has no Patient entries""" - item = {"Resource": {"resourceType": "Immunization", "contained": [{"resourceType": "Practitioner"}]}} - patient = ieds_db_operations.extract_patient_resource_from_item(item) - self.assertIsNone(patient) - - def test_extract_returns_none_when_resource_not_dict(self): - """Should return None when Resource attribute is present but not a dict""" - item = {"Resource": '{"not": "a dict string"}'} - patient = ieds_db_operations.extract_patient_resource_from_item(item) - self.assertIsNone(patient) From 7df9358c20828a5dbcce23c2cd09739f8333389a Mon Sep 17 00:00:00 2001 From: Akol125 Date: Tue, 16 Sep 2025 09:38:12 +0100 Subject: [PATCH 11/25] resolve failing test --- lambdas/id_sync/src/ieds_db_operations.py | 1 + .../id_sync/tests/test_record_processor.py | 40 ++++++++++++++----- 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/lambdas/id_sync/src/ieds_db_operations.py b/lambdas/id_sync/src/ieds_db_operations.py index 54bae75ab..ca5fd81dd 100644 --- a/lambdas/id_sync/src/ieds_db_operations.py +++ b/lambdas/id_sync/src/ieds_db_operations.py @@ -151,6 +151,7 @@ def extract_patient_resource_from_item(item: dict) -> dict | None: if not isinstance(patient_resource, dict): return None + # Otherwise, check contained for response in patient_resource.get("contained", []): if isinstance(response, dict) and response.get("resourceType") == "Patient": return response diff --git a/lambdas/id_sync/tests/test_record_processor.py b/lambdas/id_sync/tests/test_record_processor.py index cc953997d..5a6d6f069 100644 --- a/lambdas/id_sync/tests/test_record_processor.py +++ b/lambdas/id_sync/tests/test_record_processor.py @@ -69,10 +69,16 @@ def test_process_record_success_update_required(self): # Provide one IEDS item that will match demographics via demographics_match matching_item = { "Resource": { - "resourceType": "Patient", - "name": [{"given": ["John"], "family": "Doe"}], - "gender": "male", - "birthDate": "1980-01-01", + "resourceType": "Immunization", + "contained": [ + { + "resourceType": "Patient", + "id": "Pat1", + "name": [{"given": ["John"], "family": "Doe"}], + "gender": "male", + "birthDate": "1980-01-01", + } + ] } } self.mock_get_items_from_patient_id.return_value = [matching_item] @@ -104,10 +110,16 @@ def test_process_record_demographics_mismatch_skips_update(self): # IEDS items exist but do not match demographics non_matching_item = { "Resource": { - "resourceType": "Patient", - "name": [{"given": ["Bob"], "family": "Jones"}], - "gender": "male", - "birthDate": "1990-01-01", + "resourceType": "Immunization", + "contained": [ + { + "resourceType": "Patient", + "id": "Pat2", + "name": [{"given": ["Bob"], "family": "Jones"}], + "gender": "male", + "birthDate": "1990-01-01", + } + ] } } self.mock_get_items_from_patient_id.return_value = [non_matching_item] @@ -176,9 +188,15 @@ def test_body_is_string(self): } self.mock_get_items_from_patient_id.return_value = [{ "Resource": { - "resourceType": "Patient", - "name": [{"given": ["A"], "family": "B"}], - "gender": "female", "birthDate": "1990-01-01" + "resourceType": "Immunization", + "contained": [ + { + "resourceType": "Patient", + "id": "Pat3", + "name": [{"given": ["A"], "family": "B"}], + "gender": "female", "birthDate": "1990-01-01" + } + ] } }] self.mock_ieds_update_patient_id.return_value = {"status": "success"} From 0644072038a2b4bafdf9bfe6542b5b7a758d56fa Mon Sep 17 00:00:00 2001 From: Akol125 Date: Tue, 16 Sep 2025 10:04:01 +0100 Subject: [PATCH 12/25] VED-755: Resolve lint issues --- lambdas/id_sync/src/pds_details.py | 1 + lambdas/id_sync/src/record_processor.py | 6 +++--- lambdas/id_sync/tests/test_ieds_db_operations.py | 1 - 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lambdas/id_sync/src/pds_details.py b/lambdas/id_sync/src/pds_details.py index dce3d12d2..cae1b9c08 100644 --- a/lambdas/id_sync/src/pds_details.py +++ b/lambdas/id_sync/src/pds_details.py @@ -55,6 +55,7 @@ def pds_get_patient_id(nhs_number: str) -> str: logger.exception(msg) raise IdSyncException(message=msg, exception=e) + def normalize_name_from_pds(pds_get_patient_details: dict) -> str | None: """Return a normalized full name (given + family) from PDS patient details or None.""" try: diff --git a/lambdas/id_sync/src/record_processor.py b/lambdas/id_sync/src/record_processor.py index 04106d976..a4135ccdb 100644 --- a/lambdas/id_sync/src/record_processor.py +++ b/lambdas/id_sync/src/record_processor.py @@ -105,6 +105,7 @@ def process_nhs_number(nhs_number: str) -> Dict[str, Any]: response["nhs_number"] = nhs_number return response + def extract_normalized_name_from_patient(patient: dict) -> str | None: """Return a normalized 'given family' name string from a Patient resource or None.""" if not patient: @@ -133,10 +134,9 @@ def demographics_match(pds_details: dict, ieds_item: dict) -> bool: If required fields are missing or unparsable on the IEDS side the function returns False. """ try: - def normalize_strings(item: Any) -> str | None: return str(item).strip().lower() if item else None - + # Retrieve patient resource from PDS pds_name = normalize_strings(normalize_name_from_pds(pds_details)) pds_gender = normalize_strings(pds_details.get("gender")) @@ -169,4 +169,4 @@ def normalize_strings(item: Any) -> str | None: return True except Exception: logger.exception("demographics_match: comparison failed with exception") - return False \ No newline at end of file + return False diff --git a/lambdas/id_sync/tests/test_ieds_db_operations.py b/lambdas/id_sync/tests/test_ieds_db_operations.py index 4848e31d0..dc2171243 100644 --- a/lambdas/id_sync/tests/test_ieds_db_operations.py +++ b/lambdas/id_sync/tests/test_ieds_db_operations.py @@ -672,4 +672,3 @@ def test_ieds_check_exist_limit_parameter(self): # Assert - Verify the limit parameter is correctly passed self.mock_get_items_from_patient_id.assert_called_once_with(patient_id, 1) - From 4bb330a0cef7ed62e9fa3140766334b3e6477ad7 Mon Sep 17 00:00:00 2001 From: Akol125 Date: Tue, 16 Sep 2025 11:11:41 +0100 Subject: [PATCH 13/25] VED-755: remove duplication --- lambdas/id_sync/src/pds_details.py | 19 ------------------- lambdas/id_sync/src/record_processor.py | 6 +++--- 2 files changed, 3 insertions(+), 22 deletions(-) diff --git a/lambdas/id_sync/src/pds_details.py b/lambdas/id_sync/src/pds_details.py index cae1b9c08..8df448fe2 100644 --- a/lambdas/id_sync/src/pds_details.py +++ b/lambdas/id_sync/src/pds_details.py @@ -55,22 +55,3 @@ def pds_get_patient_id(nhs_number: str) -> str: logger.exception(msg) raise IdSyncException(message=msg, exception=e) - -def normalize_name_from_pds(pds_get_patient_details: dict) -> str | None: - """Return a normalized full name (given + family) from PDS patient details or None.""" - try: - name = pds_get_patient_details.get("name") - if not name: - return None - name_entry = name[0] if isinstance(name, list) else name - given = name_entry.get("given") - given_str = None - if isinstance(given, list) and given: - given_str = given[0] - elif isinstance(given, str): - given_str = given - family = name_entry.get("family") - parts = [p for p in [given_str, family] if p] - return " ".join(parts).strip().lower() if parts else None - except Exception: - return None diff --git a/lambdas/id_sync/src/record_processor.py b/lambdas/id_sync/src/record_processor.py index a4135ccdb..9acbbf144 100644 --- a/lambdas/id_sync/src/record_processor.py +++ b/lambdas/id_sync/src/record_processor.py @@ -1,6 +1,6 @@ from common.clients import logger from typing import Dict, Any -from pds_details import pds_get_patient_id, pds_get_patient_details, normalize_name_from_pds +from pds_details import pds_get_patient_id, pds_get_patient_details from ieds_db_operations import ( ieds_check_exist, ieds_update_patient_id, @@ -138,7 +138,7 @@ def normalize_strings(item: Any) -> str | None: return str(item).strip().lower() if item else None # Retrieve patient resource from PDS - pds_name = normalize_strings(normalize_name_from_pds(pds_details)) + 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")) @@ -149,7 +149,7 @@ def normalize_strings(item: Any) -> str | None: return False # normalize patient name - ieds_name = extract_normalized_name_from_patient(patient) + ieds_name = normalize_strings(extract_normalized_name_from_patient(patient)) ieds_gender = normalize_strings(patient.get("gender")) ieds_birth = patient.get("birthDate") From af29a78f93d197f6734fa52ac194b9ea1f03438c Mon Sep 17 00:00:00 2001 From: Akol125 Date: Tue, 16 Sep 2025 11:14:26 +0100 Subject: [PATCH 14/25] VED-755: remove duplication2 --- lambdas/id_sync/src/pds_details.py | 1 - 1 file changed, 1 deletion(-) diff --git a/lambdas/id_sync/src/pds_details.py b/lambdas/id_sync/src/pds_details.py index 8df448fe2..4729a0aec 100644 --- a/lambdas/id_sync/src/pds_details.py +++ b/lambdas/id_sync/src/pds_details.py @@ -54,4 +54,3 @@ def pds_get_patient_id(nhs_number: str) -> str: msg = f"Error getting PDS patient ID for {nhs_number}" logger.exception(msg) raise IdSyncException(message=msg, exception=e) - From 876485fb59770bd97430a62b007197d81bed986f Mon Sep 17 00:00:00 2001 From: Akol125 Date: Tue, 16 Sep 2025 13:08:26 +0100 Subject: [PATCH 15/25] VED-755: improve coverage --- lambdas/id_sync/src/record_processor.py | 10 +- .../id_sync/tests/test_record_processor.py | 178 +++++++++++++++++- 2 files changed, 184 insertions(+), 4 deletions(-) diff --git a/lambdas/id_sync/src/record_processor.py b/lambdas/id_sync/src/record_processor.py index 9acbbf144..4edbc8678 100644 --- a/lambdas/id_sync/src/record_processor.py +++ b/lambdas/id_sync/src/record_processor.py @@ -154,15 +154,19 @@ def normalize_strings(item: Any) -> str | None: ieds_gender = normalize_strings(patient.get("gender")) ieds_birth = patient.get("birthDate") - if pds_birth and ieds_birth and pds_birth != ieds_birth: + if not all([pds_name, pds_gender, pds_birth, ieds_name, ieds_gender, ieds_birth]): + logger.debug("demographics_match: missing required demographics") + return False + + if pds_birth != ieds_birth: logger.debug("demographics_match: birthDate mismatch %s != %s", pds_birth, ieds_birth) return False - if pds_gender and ieds_gender and pds_gender != ieds_gender: + if pds_gender != ieds_gender: logger.debug("demographics_match: gender mismatch %s != %s", pds_gender, ieds_gender) return False - if pds_name and ieds_name and pds_name != ieds_name: + if pds_name != ieds_name: logger.debug("demographics_match: name mismatch %s != %s", pds_name, ieds_name) return False diff --git a/lambdas/id_sync/tests/test_record_processor.py b/lambdas/id_sync/tests/test_record_processor.py index 5a6d6f069..8aa8ce81b 100644 --- a/lambdas/id_sync/tests/test_record_processor.py +++ b/lambdas/id_sync/tests/test_record_processor.py @@ -127,10 +127,84 @@ def test_process_record_demographics_mismatch_skips_update(self): # Act result = process_record(test_sqs_record) - # Assert: update skipped + # Assert self.assertEqual(result["status"], "success") self.assertIn("update skipped", result["message"]) + def test_invalid_body_parsing_returns_error(self): + """When body is a malformed string, process_record should return an error""" + bad_record = {"body": "not-a-json-or-python-literal"} + result = process_record(bad_record) + self.assertEqual(result["status"], "error") + self.assertIn("Invalid body format", result["message"]) + + def test_no_subject_in_body_returns_error(self): + """When body doesn't contain a subject, return an error""" + result = process_record({"body": {"other": "value"}}) + self.assertEqual(result["status"], "error") + self.assertIn("No NHS number found", result["message"]) + + def test_pds_details_exception_aborts_update(self): + """If fetching PDS details raises, function should return error""" + nhs_number = "nhs-exc-1" + test_sqs_record = {"body": {"subject": nhs_number}} + # pds returns a different id to force update path + self.mock_pds_get_patient_id.return_value = "pds-new" + self.mock_ieds_check_exist.return_value = True + self.mock_pds_get_patient_details.side_effect = Exception("pds fail") + + result = process_record(test_sqs_record) + self.assertEqual(result["status"], "error") + self.assertIn("Failed to fetch PDS details", result["message"]) + + def test_get_items_exception_aborts_update(self): + """If fetching IEDS items raises, function should return error""" + nhs_number = "nhs-exc-2" + test_sqs_record = {"body": {"subject": nhs_number}} + self.mock_pds_get_patient_id.return_value = "pds-new" + self.mock_ieds_check_exist.return_value = True + self.mock_pds_get_patient_details.return_value = { + "name": [{"given": ["J"], "family": "K"}], + "gender": "male", "birthDate": "2000-01-01" + } + self.mock_get_items_from_patient_id.side_effect = Exception("dynamo fail") + + result = process_record(test_sqs_record) + self.assertEqual(result["status"], "error") + self.assertIn("Failed to fetch IEDS items", result["message"]) + + def test_update_called_on_match(self): + """Verify ieds_update_patient_id is called when demographics match""" + pds_id = "pds-match" + nhs_number = "nhs-match" + test_sqs_record = {"body": {"subject": nhs_number}} + self.mock_pds_get_patient_id.return_value = pds_id + self.mock_pds_get_patient_details.return_value = { + "name": [ + { + "given": ["Tom"], + "family": "Hanks"} + ], + "gender": "male", + "birthDate": "1956-07-09" + } + item = { + "Resource": { + "resourceType": "Immunization", + "contained": [{ + "resourceType": "Patient", + "id": "PatM", + "name": [{"given": ["Tom"], "family": "Hanks"}], + "gender": "male", "birthDate": "1956-07-09"} + ]} + } + self.mock_get_items_from_patient_id.return_value = [item] + self.mock_ieds_update_patient_id.return_value = {"status": "success"} + + result = process_record(test_sqs_record) + self.assertEqual(result["status"], "success") + self.mock_ieds_update_patient_id.assert_called_once_with(nhs_number, pds_id) + def test_process_record_no_records_exist(self): """Test when no records exist for the patient ID""" # Arrange @@ -205,3 +279,105 @@ def test_body_is_string(self): # Assert self.assertEqual(result["status"], "success") + + def test_process_record_birthdate_mismatch_skips_update(self): + """If birthDate differs between PDS and IEDS, update should be skipped""" + pds_id = "pds-2" + nhs_number = "nhs-2" + test_sqs_record = {"body": {"subject": nhs_number}} + + self.mock_pds_get_patient_id.return_value = pds_id + self.mock_pds_get_patient_details.return_value = { + "name": [{"given": ["John"], "family": "Doe"}], + "gender": "male", + "birthDate": "1980-01-01", + } + + # IEDS has different birthDate + item = { + "Resource": { + "resourceType": "Immunization", + "contained": [ + { + "resourceType": "Patient", + "id": "PatX", + "name": + [{ + "given": ["John"], + "family": "Doe" + }], + "gender": "male", + "birthDate": "1980-01-02"} + ] + } + } + self.mock_get_items_from_patient_id.return_value = [item] + + result = process_record(test_sqs_record) + self.assertEqual(result["status"], "success") + self.assertIn("update skipped", result["message"]) + + def test_process_record_gender_mismatch_skips_update(self): + """If gender differs between PDS and IEDS, update should be skipped""" + pds_id = "pds-3" + nhs_number = "nhs-3" + test_sqs_record = {"body": {"subject": nhs_number}} + + self.mock_pds_get_patient_id.return_value = pds_id + self.mock_pds_get_patient_details.return_value = { + "name": [{"given": ["Alex"], "family": "Smith"}], + "gender": "female", + "birthDate": "1992-03-03", + } + + # IEDS has different gender + item = { + "Resource": { + "resourceType": "Immunization", + "contained": [ + { + "resourceType": "Patient", + "id": "PatY", + "name": [{ + "given": ["Alex"], + "family": "Smith" + }], + "gender": "male", "birthDate": "1992-03-03"} + ] + }} + self.mock_get_items_from_patient_id.return_value = [item] + + result = process_record(test_sqs_record) + self.assertEqual(result["status"], "success") + self.assertIn("update skipped", result["message"]) + + def test_process_record_no_comparable_fields_skips_update(self): + """If PDS provides no comparable fields, do not update (skip)""" + pds_id = "pds-4" + nhs_number = "nhs-4" + test_sqs_record = {"body": {"subject": nhs_number}} + + self.mock_pds_get_patient_id.return_value = pds_id + # PDS returns minimal/empty details + self.mock_pds_get_patient_details.return_value = {} + + item = { + "Resource": { + "resourceType": "Immunization", + "contained": [ + { + "resourceType": "Patient", + "id": "PatZ", + "name": [{ + "given": ["Zoe"], + "family": "Lee" + }], + "gender": "female", + "birthDate": "2000-01-01"} + ] + }} + self.mock_get_items_from_patient_id.return_value = [item] + + result = process_record(test_sqs_record) + self.assertEqual(result["status"], "success") + self.assertIn("update skipped", result["message"]) From f637a9360a047585641c1c0733001c53a9d1c583 Mon Sep 17 00:00:00 2001 From: Akol125 Date: Tue, 16 Sep 2025 16:18:14 +0100 Subject: [PATCH 16/25] VED-755: refactor process_nhs_number --- lambdas/id_sync/src/ieds_db_operations.py | 1 - lambdas/id_sync/src/record_processor.py | 73 ++++++++----------- .../id_sync/tests/test_record_processor.py | 32 ++++---- 3 files changed, 50 insertions(+), 56 deletions(-) diff --git a/lambdas/id_sync/src/ieds_db_operations.py b/lambdas/id_sync/src/ieds_db_operations.py index ca5fd81dd..54bae75ab 100644 --- a/lambdas/id_sync/src/ieds_db_operations.py +++ b/lambdas/id_sync/src/ieds_db_operations.py @@ -151,7 +151,6 @@ def extract_patient_resource_from_item(item: dict) -> dict | None: if not isinstance(patient_resource, dict): return None - # Otherwise, check contained for response in patient_resource.get("contained", []): if isinstance(response, dict) and response.get("resourceType") == "Patient": return response diff --git a/lambdas/id_sync/src/record_processor.py b/lambdas/id_sync/src/record_processor.py index 4edbc8678..0f2fc70d9 100644 --- a/lambdas/id_sync/src/record_processor.py +++ b/lambdas/id_sync/src/record_processor.py @@ -57,55 +57,46 @@ def process_nhs_number(nhs_number: str) -> Dict[str, Any]: } logger.info("Update patient ID from %s to %s", nhs_number, new_nhs_number) - if ieds_check_exist(nhs_number): - # Fetch PDS details for demographic comparison - try: - pds_details = pds_get_patient_details(nhs_number) - except Exception: - logger.exception("process_nhs_number: failed to fetch PDS details, aborting update") - return { - "status": "error", - "message": "Failed to fetch PDS details for demographic comparison", - "nhs_number": nhs_number, - } - - # Get IEDS items for this patient id and compare demographics - try: - items = get_items_from_patient_id(nhs_number) - except Exception: - logger.exception("process_nhs_number: failed to fetch IEDS items, aborting update") - return { + if not ieds_check_exist(nhs_number): + logger.info("No IEDS record found for: %s", nhs_number) + response = {"status": "success", "message": f"No records returned for ID: {nhs_number}"} + return response + try: + pds_details, ieds_details = fetch_demographic_details(nhs_number) + except Exception as e: + logger.exception("process_nhs_number: %s, aborting update", e) + return { "status": "error", - "message": "Failed to fetch IEDS items for demographic comparison", + "message": str(e), "nhs_number": nhs_number, - } + } - # If at least one IEDS item matches demographics, proceed with update - match_found = False - for item in items: - try: - if demographics_match(pds_details, item): - match_found = True - break - except Exception: - logger.exception("process_nhs_number: error while comparing demographics for item: %s", item) - - if not match_found: - logger.info("process_nhs_number: No IEDS items matched PDS demographics. Skipping update for %s", nhs_number) - response = { - "status": "success", - "message": "No IEDS items matched PDS demographics; update skipped", - } - else: - response = ieds_update_patient_id(nhs_number, new_nhs_number) - else: - logger.info("No IEDS record found for: %s", nhs_number) - response = {"status": "success", "message": f"No records returned for ID: {nhs_number}"} + # If at least one IEDS item matches demographics, proceed with update + if not all(demographics_match(pds_details, detail) for detail in ieds_details): + logger.info("Not all IEDS items matched PDS demographics; skipping update for %s", nhs_number) + return { + "status": "success", + "message": "Not all IEDS items matched PDS demographics; update skipped", + "nhs_number": nhs_number, + } + response = ieds_update_patient_id(nhs_number, new_nhs_number) response["nhs_number"] = nhs_number return response +def fetch_demographic_details(nhs_number: str): + try: + pds = pds_get_patient_details(nhs_number) + except Exception as e: + raise RuntimeError("Failed to fetch PDS details") from e + try: + ieds = get_items_from_patient_id(nhs_number) + except Exception as e: + raise RuntimeError("Failed to fetch IEDS items") from e + return pds, ieds + + def extract_normalized_name_from_patient(patient: dict) -> str | None: """Return a normalized 'given family' name string from a Patient resource or None.""" if not patient: diff --git a/lambdas/id_sync/tests/test_record_processor.py b/lambdas/id_sync/tests/test_record_processor.py index 8aa8ce81b..1f33e7b39 100644 --- a/lambdas/id_sync/tests/test_record_processor.py +++ b/lambdas/id_sync/tests/test_record_processor.py @@ -129,7 +129,7 @@ def test_process_record_demographics_mismatch_skips_update(self): # Assert self.assertEqual(result["status"], "success") - self.assertIn("update skipped", result["message"]) + self.assertEqual(result["message"], "Not all IEDS items matched PDS demographics; update skipped") def test_invalid_body_parsing_returns_error(self): """When body is a malformed string, process_record should return an error""" @@ -182,8 +182,8 @@ def test_update_called_on_match(self): self.mock_pds_get_patient_details.return_value = { "name": [ { - "given": ["Tom"], - "family": "Hanks"} + "given": ["Sarah"], + "family": "Fowley"} ], "gender": "male", "birthDate": "1956-07-09" @@ -194,7 +194,7 @@ def test_update_called_on_match(self): "contained": [{ "resourceType": "Patient", "id": "PatM", - "name": [{"given": ["Tom"], "family": "Hanks"}], + "name": [{"given": ["Sarah"], "family": "Fowley"}], "gender": "male", "birthDate": "1956-07-09"} ]} } @@ -215,8 +215,6 @@ def test_process_record_no_records_exist(self): # Act result = process_record(test_record) - # Assert - self.assertEqual(result["status"], "success") self.assertEqual(result["message"], f"No records returned for ID: {test_id}") # Verify PDS was not called @@ -307,7 +305,8 @@ def test_process_record_birthdate_mismatch_skips_update(self): "family": "Doe" }], "gender": "male", - "birthDate": "1980-01-02"} + "birthDate": "1980-01-02" + } ] } } @@ -315,7 +314,7 @@ def test_process_record_birthdate_mismatch_skips_update(self): result = process_record(test_sqs_record) self.assertEqual(result["status"], "success") - self.assertIn("update skipped", result["message"]) + self.assertEqual(result["message"], "Not all IEDS items matched PDS demographics; update skipped") def test_process_record_gender_mismatch_skips_update(self): """If gender differs between PDS and IEDS, update should be skipped""" @@ -342,14 +341,17 @@ def test_process_record_gender_mismatch_skips_update(self): "given": ["Alex"], "family": "Smith" }], - "gender": "male", "birthDate": "1992-03-03"} + "gender": "male", + "birthDate": "1992-03-03" + } ] - }} + } + } self.mock_get_items_from_patient_id.return_value = [item] result = process_record(test_sqs_record) self.assertEqual(result["status"], "success") - self.assertIn("update skipped", result["message"]) + self.assertEqual(result["message"], "Not all IEDS items matched PDS demographics; update skipped") def test_process_record_no_comparable_fields_skips_update(self): """If PDS provides no comparable fields, do not update (skip)""" @@ -373,11 +375,13 @@ def test_process_record_no_comparable_fields_skips_update(self): "family": "Lee" }], "gender": "female", - "birthDate": "2000-01-01"} + "birthDate": "2000-01-01" + } ] - }} + } + } self.mock_get_items_from_patient_id.return_value = [item] result = process_record(test_sqs_record) self.assertEqual(result["status"], "success") - self.assertIn("update skipped", result["message"]) + self.assertEqual(result["message"], "Not all IEDS items matched PDS demographics; update skipped") From 31858cc1e55d1bffdbbf9858709338049152194e Mon Sep 17 00:00:00 2001 From: Akol125 Date: Wed, 17 Sep 2025 12:59:59 +0100 Subject: [PATCH 17/25] VED-755: added pagination, remove ieds_check_exist, modify test --- lambdas/id_sync/src/id_sync.py | 15 +- lambdas/id_sync/src/ieds_db_operations.py | 67 ++++++--- lambdas/id_sync/src/record_processor.py | 17 ++- .../id_sync/tests/test_ieds_db_operations.py | 139 ------------------ .../id_sync/tests/test_record_processor.py | 53 +++---- 5 files changed, 90 insertions(+), 201 deletions(-) diff --git a/lambdas/id_sync/src/id_sync.py b/lambdas/id_sync/src/id_sync.py index c3a243dd0..06c8d577d 100644 --- a/lambdas/id_sync/src/id_sync.py +++ b/lambdas/id_sync/src/id_sync.py @@ -1,11 +1,3 @@ -from typing import Any, Dict - -from common.clients import logger, STREAM_NAME -from common.log_decorator import logging_decorator -from common.aws_lambda_event import AwsLambdaEvent -from exceptions.id_sync_exception import IdSyncException -from record_processor import process_record - """ - Parses the incoming AWS event into `AwsLambdaEvent` and iterate its `records`. - Delegate each record to `process_record` and collect `nhs_number` from each result. @@ -13,6 +5,13 @@ - Any unexpected error is wrapped into `IdSyncException(message="Error processing id_sync event")`. """ +from typing import Any, Dict +from common.clients import logger, STREAM_NAME +from common.log_decorator import logging_decorator +from common.aws_lambda_event import AwsLambdaEvent +from exceptions.id_sync_exception import IdSyncException +from record_processor import process_record + @logging_decorator(prefix="id_sync", stream_name=STREAM_NAME) def handler(event_data: Dict[str, Any], _context) -> Dict[str, Any]: diff --git a/lambdas/id_sync/src/ieds_db_operations.py b/lambdas/id_sync/src/ieds_db_operations.py index 54bae75ab..14c8b5ad3 100644 --- a/lambdas/id_sync/src/ieds_db_operations.py +++ b/lambdas/id_sync/src/ieds_db_operations.py @@ -16,17 +16,6 @@ def get_ieds_table(): return ieds_table -def ieds_check_exist(id: str) -> bool: - """Check if a record exists in the IEDS table for the given ID.""" - logger.info(f"Check Id exists ID: {id}") - items = get_items_from_patient_id(id, 1) - - if items or len(items) > 0: - logger.info(f"Found patient ID: {id}") - return True - return False - - BATCH_SIZE = 25 @@ -118,28 +107,62 @@ def ieds_update_patient_id(old_id: str, new_id: str) -> dict: ) -def get_items_from_patient_id(id: str, limit=BATCH_SIZE) -> list: - """Get all items for patient ID.""" +def get_items_from_patient_id(id: str, filter_expression=None) -> list: + """Query the PatientGSI and paginate through all results. + + - Uses LastEvaluatedKey to page until all items are collected. + - If `filter_expression` is provided it will be included as `FilterExpression`. + - Raises `IdSyncException` if the DynamoDB response doesn't include 'Items' or + an underlying error occurs. + """ logger.info(f"Getting items for patient id: {id}") patient_pk = f"Patient#{id}" - try: - response = get_ieds_table().query( - IndexName='PatientGSI', # query the GSI - KeyConditionExpression=Key('PatientPK').eq(patient_pk), - Limit=limit - ) - if 'Items' not in response or not response['Items']: + all_items: list = [] + last_evaluated_key = None + try: + while True: + query_args = { + "IndexName": "PatientGSI", + "KeyConditionExpression": Key('PatientPK').eq(patient_pk), + } + if filter_expression is not None: + query_args["FilterExpression"] = filter_expression + if last_evaluated_key: + query_args["ExclusiveStartKey"] = last_evaluated_key + + response = get_ieds_table().query(**query_args) + + if "Items" not in response: + # Unexpected DynamoDB response shape - surface as IdSyncException + logger.exception("Unexpected DynamoDB response: missing 'Items'") + raise IdSyncException( + message="No Items in DynamoDB response", + nhs_numbers=[patient_pk], + exception=response, + ) + + items = response.get("Items", []) + all_items.extend(items) + + last_evaluated_key = response.get("LastEvaluatedKey") + if not last_evaluated_key: + break + + if not all_items: logger.warning(f"No items found for patient PK: {patient_pk}") return [] - return response['Items'] + return all_items + + except IdSyncException: + raise except Exception as e: logger.exception(f"Error querying items for patient PK: {patient_pk}") raise IdSyncException( message=f"Error querying items for patient PK: {patient_pk}", nhs_numbers=[patient_pk], - exception=e + exception=e, ) diff --git a/lambdas/id_sync/src/record_processor.py b/lambdas/id_sync/src/record_processor.py index 0f2fc70d9..1d4d6c1bc 100644 --- a/lambdas/id_sync/src/record_processor.py +++ b/lambdas/id_sync/src/record_processor.py @@ -2,7 +2,6 @@ from typing import Dict, Any from pds_details import pds_get_patient_id, pds_get_patient_details from ieds_db_operations import ( - ieds_check_exist, ieds_update_patient_id, extract_patient_resource_from_item, get_items_from_patient_id, @@ -57,20 +56,26 @@ def process_nhs_number(nhs_number: str) -> Dict[str, Any]: } logger.info("Update patient ID from %s to %s", nhs_number, new_nhs_number) - if not ieds_check_exist(nhs_number): - logger.info("No IEDS record found for: %s", nhs_number) - response = {"status": "success", "message": f"No records returned for ID: {nhs_number}"} - return response try: pds_details, ieds_details = fetch_demographic_details(nhs_number) except Exception as e: - logger.exception("process_nhs_number: %s, aborting update", e) + logger.exception("process_nhs_number: failed to fetch demographic details: %s", e) return { "status": "error", "message": str(e), "nhs_number": nhs_number, } + # If no IEDS items were returned, nothing to update — return a clear success + # message to match existing test expectations. + if not ieds_details: + logger.info("No IEDS records returned for NHS number: %s", nhs_number) + return { + "status": "success", + "message": f"No records returned for ID: {nhs_number}", + "nhs_number": nhs_number, + } + # If at least one IEDS item matches demographics, proceed with update if not all(demographics_match(pds_details, detail) for detail in ieds_details): logger.info("Not all IEDS items matched PDS demographics; skipping update for %s", nhs_number) diff --git a/lambdas/id_sync/tests/test_ieds_db_operations.py b/lambdas/id_sync/tests/test_ieds_db_operations.py index dc2171243..f228591d4 100644 --- a/lambdas/id_sync/tests/test_ieds_db_operations.py +++ b/lambdas/id_sync/tests/test_ieds_db_operations.py @@ -533,142 +533,3 @@ def test_get_items_from_patient_id_no_records(self): # Assert self.assertEqual(result, []) - - -class TestIedsCheckExists(TestIedsDbOperations): - - def setUp(self): - """Set up test fixtures""" - super().setUp() - - # Mock get_items_from_patient_id instead of table.query - self.get_items_from_patient_id_patcher = patch('ieds_db_operations.get_items_from_patient_id') - self.mock_get_items_from_patient_id = self.get_items_from_patient_id_patcher.start() - - def tearDown(self): - """Clean up patches""" - super().tearDown() - - def test_ieds_check_exist_record_exists(self): - """Test when record exists in IEDS table""" - # Arrange - patient_id = "test-patient-123" - mock_items = [{'PK': 'Patient#test-patient-123', 'PatientPK': 'Patient#test-patient-123'}] - self.mock_get_items_from_patient_id.return_value = mock_items - - # Act - result = ieds_db_operations.ieds_check_exist(patient_id) - - # Assert - self.assertTrue(result) - - # Verify get_items_from_patient_id was called with correct parameters - self.mock_get_items_from_patient_id.assert_called_once_with(patient_id, 1) - - def test_ieds_check_exist_record_not_exists(self): - """Test when no record exists in IEDS table""" - # Arrange - patient_id = "test-patient-456" - self.mock_get_items_from_patient_id.return_value = [] - - # Act - result = ieds_db_operations.ieds_check_exist(patient_id) - - # Assert - self.assertFalse(result) - - # Verify get_items_from_patient_id was called - self.mock_get_items_from_patient_id.assert_called_once_with(patient_id, 1) - - def test_ieds_check_exist_empty_id(self): - """Test with empty patient ID""" - # Arrange - patient_id = "" - self.mock_get_items_from_patient_id.return_value = [] - - # Act - result = ieds_db_operations.ieds_check_exist(patient_id) - - # Assert - self.assertFalse(result) - - # Verify get_items_from_patient_id was called with empty ID - self.mock_get_items_from_patient_id.assert_called_once_with(patient_id, 1) - - def test_ieds_check_exist_none_id(self): - """Test with None patient ID""" - # Arrange - patient_id = None - self.mock_get_items_from_patient_id.return_value = [] - - # Act - result = ieds_db_operations.ieds_check_exist(patient_id) - - # Assert - self.assertFalse(result) - - # Verify get_items_from_patient_id was called with None ID - self.mock_get_items_from_patient_id.assert_called_once_with(patient_id, 1) - - def test_ieds_check_exist_query_exception(self): - """Test exception handling during get_items_from_patient_id""" - # Arrange - patient_id = "test-patient-error" - test_exception = Exception("DynamoDB query failed") - self.mock_get_items_from_patient_id.side_effect = test_exception - - # Act & Assert - with self.assertRaises(Exception) as context: - ieds_db_operations.ieds_check_exist(patient_id) - - self.assertEqual(str(context.exception), "DynamoDB query failed") - - # Verify get_items_from_patient_id was attempted - self.mock_get_items_from_patient_id.assert_called_once_with(patient_id, 1) - - def test_ieds_check_exist_multiple_items_found(self): - """Test when multiple items are found (should still return True)""" - # Arrange - patient_id = "test-patient-multiple" - mock_items = [ - {'PK': 'Patient#test-patient-multiple', 'PatientPK': 'Patient#test-patient-multiple'}, - {'PK': 'Patient#test-patient-multiple#record1', 'PatientPK': 'Patient#test-patient-multiple'} - ] - self.mock_get_items_from_patient_id.return_value = mock_items - - # Act - result = ieds_db_operations.ieds_check_exist(patient_id) - - # Assert - self.assertTrue(result) - - # Verify get_items_from_patient_id was called with limit=1 - self.mock_get_items_from_patient_id.assert_called_once_with(patient_id, 1) - - def test_ieds_check_exist_single_item_found(self): - """Test when exactly one item is found""" - # Arrange - patient_id = "test-patient-single" - mock_items = [{'PK': 'Patient#test-patient-single', 'PatientPK': 'Patient#test-patient-single'}] - self.mock_get_items_from_patient_id.return_value = mock_items - - # Act - result = ieds_db_operations.ieds_check_exist(patient_id) - - # Assert - self.assertTrue(result) - - # Verify get_items_from_patient_id was called - self.mock_get_items_from_patient_id.assert_called_once_with(patient_id, 1) - - def test_ieds_check_exist_limit_parameter(self): - """Test that the function passes limit=1 to get_items_from_patient_id""" - # Arrange - patient_id = "test-patient-limit" - self.mock_get_items_from_patient_id.return_value = [] - - # Act - ieds_db_operations.ieds_check_exist(patient_id) - - # Assert - Verify the limit parameter is correctly passed - self.mock_get_items_from_patient_id.assert_called_once_with(patient_id, 1) diff --git a/lambdas/id_sync/tests/test_record_processor.py b/lambdas/id_sync/tests/test_record_processor.py index 1f33e7b39..e77cdf16e 100644 --- a/lambdas/id_sync/tests/test_record_processor.py +++ b/lambdas/id_sync/tests/test_record_processor.py @@ -18,10 +18,6 @@ def setUp(self): self.pds_get_patient_details_patcher = patch('record_processor.pds_get_patient_details') self.mock_pds_get_patient_details = self.pds_get_patient_details_patcher.start() - # IEDS helpers - self.ieds_check_exist_patcher = patch('record_processor.ieds_check_exist') - self.mock_ieds_check_exist = self.ieds_check_exist_patcher.start() - self.ieds_update_patient_id_patcher = patch('record_processor.ieds_update_patient_id') self.mock_ieds_update_patient_id = self.ieds_update_patient_id_patcher.start() @@ -35,20 +31,21 @@ def test_process_record_success_no_update_required(self): """Test successful processing when patient ID matches""" # Arrange test_id = "54321" - with patch('record_processor.ieds_check_exist', return_value=True): - test_record = {"body": {"subject": test_id}} - self.mock_pds_get_patient_id.return_value = test_id + # Simulate IEDS items exist + self.mock_get_items_from_patient_id.return_value = [{"Resource": {}}] + test_record = {"body": {"subject": test_id}} + self.mock_pds_get_patient_id.return_value = test_id - # Act - result = process_record(test_record) + # Act + result = process_record(test_record) - # Assert - self.assertEqual(result["nhs_number"], test_id) - self.assertEqual(result["message"], "No update required") - self.assertEqual(result["status"], "success") + # Assert + self.assertEqual(result["nhs_number"], test_id) + self.assertEqual(result["message"], "No update required") + self.assertEqual(result["status"], "success") - # Verify calls - self.mock_pds_get_patient_id.assert_called_once_with(test_id) + # Verify calls + self.mock_pds_get_patient_id.assert_called_once_with(test_id) def test_process_record_success_update_required(self): """Test successful processing when patient ID differs and demographics match""" @@ -150,7 +147,7 @@ def test_pds_details_exception_aborts_update(self): test_sqs_record = {"body": {"subject": nhs_number}} # pds returns a different id to force update path self.mock_pds_get_patient_id.return_value = "pds-new" - self.mock_ieds_check_exist.return_value = True + self.mock_get_items_from_patient_id.return_value = [{"Resource": {}}] self.mock_pds_get_patient_details.side_effect = Exception("pds fail") result = process_record(test_sqs_record) @@ -162,7 +159,7 @@ def test_get_items_exception_aborts_update(self): nhs_number = "nhs-exc-2" test_sqs_record = {"body": {"subject": nhs_number}} self.mock_pds_get_patient_id.return_value = "pds-new" - self.mock_ieds_check_exist.return_value = True + self.mock_get_items_from_patient_id.return_value = [{"Resource": {}}] self.mock_pds_get_patient_details.return_value = { "name": [{"given": ["J"], "family": "K"}], "gender": "male", "birthDate": "2000-01-01" @@ -209,16 +206,17 @@ def test_process_record_no_records_exist(self): """Test when no records exist for the patient ID""" # Arrange test_id = "12345" - with patch('record_processor.ieds_check_exist', return_value=False): - test_record = {"body": {"subject": test_id}} + # Simulate no IEDS items + self.mock_get_items_from_patient_id.return_value = [] + test_record = {"body": {"subject": test_id}} - # Act - result = process_record(test_record) + # Act + result = process_record(test_record) - self.assertEqual(result["message"], f"No records returned for ID: {test_id}") + self.assertEqual(result["message"], f"No records returned for ID: {test_id}") - # Verify PDS was not called - self.mock_pds_get_patient_id.assert_called_once() + # Verify PDS was not called + self.mock_pds_get_patient_id.assert_called_once() def test_process_record_pds_returns_none_id(self): """Test when PDS returns none """ @@ -231,7 +229,8 @@ def test_process_record_pds_returns_none_id(self): result = process_record(test_record) self.assertEqual(result["status"], "success") self.assertEqual(result["message"], "No patient ID found for NHS number") - self.mock_ieds_check_exist.assert_not_called() + # No IEDS lookups should have been attempted when PDS returns None + self.mock_get_items_from_patient_id.assert_not_called() self.mock_ieds_update_patient_id.assert_not_called() def test_process_record_ieds_returns_false(self): @@ -240,7 +239,9 @@ def test_process_record_ieds_returns_false(self): test_id = "12345a" pds_id = "pds-id-1" self.mock_pds_get_patient_id.return_value = pds_id - self.mock_ieds_check_exist.return_value = False + # Simulate no items returned from IEDS + self.mock_get_items_from_patient_id.return_value = [] + # Act & Assert result = process_record({"body": {"subject": test_id}}) self.assertEqual(result["status"], "success") From 2ffffb73407abb349c45e9cac919e39fd46da63e Mon Sep 17 00:00:00 2001 From: Akol125 Date: Wed, 17 Sep 2025 16:27:02 +0100 Subject: [PATCH 18/25] refactor pagination --- lambdas/id_sync/src/ieds_db_operations.py | 92 ++++++++++++----------- 1 file changed, 47 insertions(+), 45 deletions(-) diff --git a/lambdas/id_sync/src/ieds_db_operations.py b/lambdas/id_sync/src/ieds_db_operations.py index 14c8b5ad3..9a77936e4 100644 --- a/lambdas/id_sync/src/ieds_db_operations.py +++ b/lambdas/id_sync/src/ieds_db_operations.py @@ -107,58 +107,20 @@ def ieds_update_patient_id(old_id: str, new_id: str) -> dict: ) -def get_items_from_patient_id(id: str, filter_expression=None) -> list: - """Query the PatientGSI and paginate through all results. +def get_items_from_patient_id(id: str) -> list: + """Public wrapper: build PatientPK and return all matching items. - - Uses LastEvaluatedKey to page until all items are collected. - - If `filter_expression` is provided it will be included as `FilterExpression`. - - Raises `IdSyncException` if the DynamoDB response doesn't include 'Items' or - an underlying error occurs. + Delegates actual paging to the internal helper `_paginate_items_for_patient_pk`. + Raises IdSyncException on error. """ - logger.info(f"Getting items for patient id: {id}") + logger.info("Getting items for patient id: %s", id) patient_pk = f"Patient#{id}" - - all_items: list = [] - last_evaluated_key = None try: - while True: - query_args = { - "IndexName": "PatientGSI", - "KeyConditionExpression": Key('PatientPK').eq(patient_pk), - } - if filter_expression is not None: - query_args["FilterExpression"] = filter_expression - if last_evaluated_key: - query_args["ExclusiveStartKey"] = last_evaluated_key - - response = get_ieds_table().query(**query_args) - - if "Items" not in response: - # Unexpected DynamoDB response shape - surface as IdSyncException - logger.exception("Unexpected DynamoDB response: missing 'Items'") - raise IdSyncException( - message="No Items in DynamoDB response", - nhs_numbers=[patient_pk], - exception=response, - ) - - items = response.get("Items", []) - all_items.extend(items) - - last_evaluated_key = response.get("LastEvaluatedKey") - if not last_evaluated_key: - break - - if not all_items: - logger.warning(f"No items found for patient PK: {patient_pk}") - return [] - - return all_items - + return paginate_items_for_patient_pk(patient_pk) except IdSyncException: raise except Exception as e: - logger.exception(f"Error querying items for patient PK: {patient_pk}") + logger.exception("Error querying items for patient PK: %s", patient_pk) raise IdSyncException( message=f"Error querying items for patient PK: {patient_pk}", nhs_numbers=[patient_pk], @@ -166,6 +128,46 @@ def get_items_from_patient_id(id: str, filter_expression=None) -> list: ) +def paginate_items_for_patient_pk(patient_pk: str) -> list: + """Internal helper that pages through the PatientGSI and returns all items. + + Raises IdSyncException when the DynamoDB response is malformed. + """ + all_items: list = [] + last_evaluated_key = None + while True: + query_args = { + "IndexName": "PatientGSI", + "KeyConditionExpression": Key('PatientPK').eq(patient_pk), + } + if last_evaluated_key: + query_args["ExclusiveStartKey"] = last_evaluated_key + + response = get_ieds_table().query(**query_args) + + if "Items" not in response: + # Unexpected DynamoDB response shape - surface as IdSyncException + logger.exception("Unexpected DynamoDB response: missing 'Items'") + raise IdSyncException( + message="No Items in DynamoDB response", + nhs_numbers=[patient_pk], + exception=response, + ) + + items = response.get("Items", []) + all_items.extend(items) + + last_evaluated_key = response.get("LastEvaluatedKey") + if not last_evaluated_key: + break + + if not all_items: + logger.warning("No items found for patient PK: %s", patient_pk) + return [] + + return all_items + + def extract_patient_resource_from_item(item: dict) -> dict | None: """ Extract a Patient resource dict from an IEDS database. From 9da9a17429e33141049707417a5b131b7a9105a0 Mon Sep 17 00:00:00 2001 From: Akol125 Date: Wed, 17 Sep 2025 19:38:07 +0100 Subject: [PATCH 19/25] VED-755: update records whose vaccination match --- lambdas/id_sync/src/ieds_db_operations.py | 9 +++-- lambdas/id_sync/src/record_processor.py | 35 ++++++++++++++----- .../id_sync/tests/test_record_processor.py | 12 +++---- 3 files changed, 39 insertions(+), 17 deletions(-) diff --git a/lambdas/id_sync/src/ieds_db_operations.py b/lambdas/id_sync/src/ieds_db_operations.py index 9a77936e4..6f7c1e1ec 100644 --- a/lambdas/id_sync/src/ieds_db_operations.py +++ b/lambdas/id_sync/src/ieds_db_operations.py @@ -19,7 +19,7 @@ def get_ieds_table(): BATCH_SIZE = 25 -def ieds_update_patient_id(old_id: str, new_id: str) -> dict: +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.""" 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(): @@ -33,8 +33,11 @@ def ieds_update_patient_id(old_id: str, new_id: str) -> dict: new_patient_pk = f"Patient#{new_id}" - logger.info("Getting items to update in IEDS table...") - items_to_update = get_items_from_patient_id(old_id) + if items_to_update is None: + logger.info("Getting items to update in IEDS table...") + items_to_update = get_items_from_patient_id(old_id) + else: + logger.info("Using provided items_to_update list, size=%d", len(items_to_update)) if not items_to_update: logger.warning(f"No items found to update for patient ID: {old_id}") diff --git a/lambdas/id_sync/src/record_processor.py b/lambdas/id_sync/src/record_processor.py index 1d4d6c1bc..af3d28d8c 100644 --- a/lambdas/id_sync/src/record_processor.py +++ b/lambdas/id_sync/src/record_processor.py @@ -1,5 +1,5 @@ from common.clients import logger -from typing import Dict, Any +from typing import Dict, List, Any from pds_details import pds_get_patient_id, pds_get_patient_details from ieds_db_operations import ( ieds_update_patient_id, @@ -77,16 +77,34 @@ def process_nhs_number(nhs_number: str) -> Dict[str, Any]: } # If at least one IEDS item matches demographics, proceed with update - if not all(demographics_match(pds_details, detail) for detail in ieds_details): - logger.info("Not all IEDS items matched PDS demographics; skipping update for %s", nhs_number) + matching_records: List[Dict[str, Any]] = [] + discarded_records: List[Dict[str, Any]] = [] + for detail in ieds_details: + if demographics_match(pds_details, detail): + matching_records.append(detail) + else: + discarded_records.append(detail) + + if not matching_records: + logger.info( + "No records matched PDS demographics; skipping update for %s", + nhs_number, + ) return { "status": "success", - "message": "Not all IEDS items matched PDS demographics; update skipped", + "message": "No records matched PDS demographics; update skipped", "nhs_number": nhs_number, + "matched": 0, + "discarded": len(discarded_records), } - response = ieds_update_patient_id(nhs_number, new_nhs_number) + response = ieds_update_patient_id( + nhs_number, new_nhs_number, items_to_update=matching_records + ) response["nhs_number"] = nhs_number + # add counts for observability + response["matched"] = len(matching_records) + response["discarded"] = len(discarded_records) return response @@ -144,16 +162,17 @@ def normalize_strings(item: Any) -> str | None: logger.debug("demographics_match: no patient resource in IEDS table item") return False - # normalize patient name + # normalize patient fields from IEDS ieds_name = normalize_strings(extract_normalized_name_from_patient(patient)) - ieds_gender = normalize_strings(patient.get("gender")) - ieds_birth = patient.get("birthDate") + ieds_birth = normalize_strings(patient.get("birthDate")) + # All required fields must be present if not all([pds_name, pds_gender, pds_birth, ieds_name, ieds_gender, ieds_birth]): logger.debug("demographics_match: missing required demographics") return False + # Compare fields if pds_birth != ieds_birth: logger.debug("demographics_match: birthDate mismatch %s != %s", pds_birth, ieds_birth) return False diff --git a/lambdas/id_sync/tests/test_record_processor.py b/lambdas/id_sync/tests/test_record_processor.py index e77cdf16e..c0b0dba81 100644 --- a/lambdas/id_sync/tests/test_record_processor.py +++ b/lambdas/id_sync/tests/test_record_processor.py @@ -121,12 +121,12 @@ def test_process_record_demographics_mismatch_skips_update(self): } self.mock_get_items_from_patient_id.return_value = [non_matching_item] - # Act + # Act result = process_record(test_sqs_record) # Assert self.assertEqual(result["status"], "success") - self.assertEqual(result["message"], "Not all IEDS items matched PDS demographics; update skipped") + self.assertEqual(result["message"], "No records matched PDS demographics; update skipped") def test_invalid_body_parsing_returns_error(self): """When body is a malformed string, process_record should return an error""" @@ -200,7 +200,7 @@ def test_update_called_on_match(self): result = process_record(test_sqs_record) self.assertEqual(result["status"], "success") - self.mock_ieds_update_patient_id.assert_called_once_with(nhs_number, pds_id) + self.mock_ieds_update_patient_id.assert_called_once_with(nhs_number, pds_id, items_to_update=[item]) def test_process_record_no_records_exist(self): """Test when no records exist for the patient ID""" @@ -315,7 +315,7 @@ def test_process_record_birthdate_mismatch_skips_update(self): result = process_record(test_sqs_record) self.assertEqual(result["status"], "success") - self.assertEqual(result["message"], "Not all IEDS items matched PDS demographics; update skipped") + self.assertEqual(result["message"], "No records matched PDS demographics; update skipped") def test_process_record_gender_mismatch_skips_update(self): """If gender differs between PDS and IEDS, update should be skipped""" @@ -352,7 +352,7 @@ def test_process_record_gender_mismatch_skips_update(self): result = process_record(test_sqs_record) self.assertEqual(result["status"], "success") - self.assertEqual(result["message"], "Not all IEDS items matched PDS demographics; update skipped") + self.assertEqual(result["message"], "No records matched PDS demographics; update skipped") def test_process_record_no_comparable_fields_skips_update(self): """If PDS provides no comparable fields, do not update (skip)""" @@ -385,4 +385,4 @@ def test_process_record_no_comparable_fields_skips_update(self): result = process_record(test_sqs_record) self.assertEqual(result["status"], "success") - self.assertEqual(result["message"], "Not all IEDS items matched PDS demographics; update skipped") + self.assertEqual(result["message"], "No records matched PDS demographics; update skipped") From 9c446b55c6798dada6074c01ba411fa0a4ec838d Mon Sep 17 00:00:00 2001 From: Akol125 Date: Thu, 18 Sep 2025 10:24:10 +0100 Subject: [PATCH 20/25] refactoring record processor.py --- lambdas/id_sync/src/record_processor.py | 55 +++++++------------------ 1 file changed, 16 insertions(+), 39 deletions(-) diff --git a/lambdas/id_sync/src/record_processor.py b/lambdas/id_sync/src/record_processor.py index af3d28d8c..8647a8d29 100644 --- a/lambdas/id_sync/src/record_processor.py +++ b/lambdas/id_sync/src/record_processor.py @@ -1,5 +1,5 @@ from common.clients import logger -from typing import Dict, List, Any +from typing import Dict, Any from pds_details import pds_get_patient_id, pds_get_patient_details from ieds_db_operations import ( ieds_update_patient_id, @@ -42,43 +42,24 @@ def process_nhs_number(nhs_number: str) -> Dict[str, Any]: new_nhs_number = pds_get_patient_id(nhs_number) if not new_nhs_number: - return { - "status": "success", - "message": "No patient ID found for NHS number", - "nhs_number": nhs_number, - } + return log_status("No patient ID found for NHS number", nhs_number) if new_nhs_number == nhs_number: - return { - "status": "success", - "message": "No update required", - "nhs_number": nhs_number, - } + return log_status("No update required", nhs_number) logger.info("Update patient ID from %s to %s", nhs_number, new_nhs_number) try: pds_details, ieds_details = fetch_demographic_details(nhs_number) except Exception as e: logger.exception("process_nhs_number: failed to fetch demographic details: %s", e) - return { - "status": "error", - "message": str(e), - "nhs_number": nhs_number, - } - - # If no IEDS items were returned, nothing to update — return a clear success - # message to match existing test expectations. + return log_status(str(e), nhs_number, "error") + if not ieds_details: logger.info("No IEDS records returned for NHS number: %s", nhs_number) - return { - "status": "success", - "message": f"No records returned for ID: {nhs_number}", - "nhs_number": nhs_number, - } - - # If at least one IEDS item matches demographics, proceed with update - matching_records: List[Dict[str, Any]] = [] - discarded_records: List[Dict[str, Any]] = [] + return log_status(f"No records returned for ID: {nhs_number}", nhs_number) + + # Compare demographics from PDS to each IEDS item, keep only matching records + matching_records, discarded_records = [], [] for detail in ieds_details: if demographics_match(pds_details, detail): matching_records.append(detail) @@ -86,17 +67,8 @@ def process_nhs_number(nhs_number: str) -> Dict[str, Any]: discarded_records.append(detail) if not matching_records: - logger.info( - "No records matched PDS demographics; skipping update for %s", - nhs_number, - ) - return { - "status": "success", - "message": "No records matched PDS demographics; update skipped", - "nhs_number": nhs_number, - "matched": 0, - "discarded": len(discarded_records), - } + logger.info("No records matched PDS demographics: %d", len(discarded_records)) + return log_status("No records matched PDS demographics; update skipped", nhs_number) response = ieds_update_patient_id( nhs_number, new_nhs_number, items_to_update=matching_records @@ -189,3 +161,8 @@ def normalize_strings(item: Any) -> str | None: except Exception: logger.exception("demographics_match: comparison failed with exception") return False + + +def log_status(msg: str, nhs_number: str, status: str = "success") -> Dict[str, Any]: + message = {"status": status, "message": msg, "nhs_number": nhs_number} + return message From 1ae026a45f59ecc9e55b8186d0e6b202cdf399d2 Mon Sep 17 00:00:00 2001 From: Akol125 Date: Thu, 18 Sep 2025 12:00:18 +0100 Subject: [PATCH 21/25] reduce cognitive complexity and use utils and constants --- lambdas/id_sync/src/ieds_db_operations.py | 123 ++++++++++-------- lambdas/id_sync/src/record_processor.py | 5 +- lambdas/id_sync/src/utils.py | 15 +++ .../id_sync/tests/test_ieds_db_operations.py | 6 + .../test_ieds_db_operations_conditional.py | 74 +++++++++++ 5 files changed, 165 insertions(+), 58 deletions(-) create mode 100644 lambdas/id_sync/src/utils.py create mode 100644 lambdas/id_sync/tests/test_ieds_db_operations_conditional.py diff --git a/lambdas/id_sync/src/ieds_db_operations.py b/lambdas/id_sync/src/ieds_db_operations.py index 6f7c1e1ec..72e55239d 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 +from utils import log_status, BATCH_SIZE from exceptions.id_sync_exception import IdSyncException ieds_table = None @@ -16,23 +17,18 @@ def get_ieds_table(): return ieds_table -BATCH_SIZE = 25 - - 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.""" 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 {"status": "error", "message": "Old ID and New ID cannot be empty"} + return log_status("Old ID and New ID cannot be empty", old_id, "error") if old_id == new_id: - return {"status": "success", "message": f"No change in patient ID: {old_id}"} + return log_status(f"No change in patient ID: {old_id}", old_id) try: logger.info(f"Updating patient ID in IEDS from {old_id} to {new_id}") - new_patient_pk = f"Patient#{new_id}" - if items_to_update is None: logger.info("Getting items to update in IEDS table...") items_to_update = get_items_from_patient_id(old_id) @@ -41,64 +37,27 @@ def ieds_update_patient_id(old_id: str, new_id: str, items_to_update: list | Non if not items_to_update: logger.warning(f"No items found to update for patient ID: {old_id}") - return { - "status": "success", - "message": f"No items found to update for patient ID: {old_id}" - } - - transact_items = [] + return log_status(f"No items found to update for patient ID: {old_id}", old_id) logger.info(f"Items to update: {len(items_to_update)}") - ieds_table_name = get_ieds_table_name() - for item in items_to_update: - transact_items.append({ - 'Update': { - 'TableName': ieds_table_name, - 'Key': { - 'PK': {'S': item['PK']}, - }, - 'UpdateExpression': 'SET PatientPK = :new_val', - 'ExpressionAttributeValues': { - ':new_val': {'S': new_patient_pk} - } - } - }) - - logger.info("Transacting items in IEDS table...") - # success tracking - all_batches_successful = True - total_batches = 0 - # Batch transact in chunks of BATCH_SIZE - for i in range(0, len(transact_items), BATCH_SIZE): - batch = transact_items[i:i+BATCH_SIZE] - total_batches += 1 - logger.info(f"Transacting batch {total_batches} of size: {len(batch)}") + # Build transact items and execute them in batches via helpers to keep + # the top-level function easy to read and test. + transact_items = build_transact_items(old_id, new_id, items_to_update) - response = dynamodb_client.transact_write_items(TransactItems=batch) - logger.info("Batch update complete. Response: %s", response) - - # Check each batch response - if response['ResponseMetadata']['HTTPStatusCode'] != 200: - all_batches_successful = False - logger.error( - f"Batch {total_batches} failed with status: {response['ResponseMetadata']['HTTPStatusCode']}") + all_batches_successful, total_batches = execute_transaction_in_batches(transact_items) # Consolidated response handling logger.info( f"All batches complete. Total batches: {total_batches}, All successful: {all_batches_successful}") if all_batches_successful: - return { - "status": "success", - "message": - f"IEDS update, patient ID: {old_id}=>{new_id}. {len(items_to_update)} updated {total_batches}." - } + return log_status( + f"IEDS update, patient ID: {old_id}=>{new_id}. {len(items_to_update)} updated {total_batches}.", + old_id, + ) else: - return { - "status": "error", - "message": f"Failed to update some batches for patient ID: {old_id}" - } + return log_status(f"Failed to update some batches for patient ID: {old_id}", old_id, "error") except Exception as e: logger.exception("Error updating patient ID") @@ -184,3 +143,59 @@ def extract_patient_resource_from_item(item: dict) -> dict | None: return response return None + + +def build_transact_items(old_id: str, new_id: str, items_to_update: list) -> list: + """Construct the list of TransactItems for DynamoDB TransactWriteItems. + + Each item uses a conditional expression to ensure PatientPK hasn't changed + since it was read. + """ + transact_items = [] + ieds_table_name = get_ieds_table_name() + new_patient_pk = f"Patient#{new_id}" + + for item in items_to_update: + old_patient_pk = item.get('PatientPK', f"Patient#{old_id}") + + transact_items.append({ + 'Update': { + 'TableName': ieds_table_name, + 'Key': { + 'PK': {'S': item['PK']}, + }, + 'UpdateExpression': 'SET PatientPK = :new_val', + "ConditionExpression": "PatientPK = :expected_old", + 'ExpressionAttributeValues': { + ':new_val': {'S': new_patient_pk}, + ':expected_old': {'S': old_patient_pk} + } + } + }) + + return transact_items + + +def execute_transaction_in_batches(transact_items: list) -> tuple: + """Execute transact write items in batches of BATCH_SIZE. + + Returns (all_batches_successful: bool, total_batches: int). + """ + all_batches_successful = True + total_batches = 0 + + for i in range(0, len(transact_items), BATCH_SIZE): + batch = transact_items[i:i+BATCH_SIZE] + total_batches += 1 + logger.info(f"Transacting batch {total_batches} of size: {len(batch)}") + + response = dynamodb_client.transact_write_items(TransactItems=batch) + logger.info("Batch update complete. Response: %s", response) + + # Check each batch response + if response['ResponseMetadata']['HTTPStatusCode'] != 200: + all_batches_successful = False + logger.error( + f"Batch {total_batches} failed with status: {response['ResponseMetadata']['HTTPStatusCode']}") + + return all_batches_successful, total_batches diff --git a/lambdas/id_sync/src/record_processor.py b/lambdas/id_sync/src/record_processor.py index 8647a8d29..ecb46c54c 100644 --- a/lambdas/id_sync/src/record_processor.py +++ b/lambdas/id_sync/src/record_processor.py @@ -6,6 +6,7 @@ extract_patient_resource_from_item, get_items_from_patient_id, ) +from utils import log_status import json import ast @@ -162,7 +163,3 @@ def normalize_strings(item: Any) -> str | None: logger.exception("demographics_match: comparison failed with exception") return False - -def log_status(msg: str, nhs_number: str, status: str = "success") -> Dict[str, Any]: - message = {"status": status, "message": msg, "nhs_number": nhs_number} - return message diff --git a/lambdas/id_sync/src/utils.py b/lambdas/id_sync/src/utils.py new file mode 100644 index 000000000..765095f44 --- /dev/null +++ b/lambdas/id_sync/src/utils.py @@ -0,0 +1,15 @@ +from typing import Dict, Any + + +def log_status(msg: str, nhs_number: str | None = None, status: str = "success") -> Dict[str, Any]: + """Return a simple status dict used by record processing for observability. + + If `nhs_number` is None the key is omitted which keeps the output shape + compatible with callers that expect only a status/message. + """ + result = {"status": status, "message": msg} + if nhs_number is not None: + result["nhs_number"] = nhs_number + return result + +BATCH_SIZE = 25 diff --git a/lambdas/id_sync/tests/test_ieds_db_operations.py b/lambdas/id_sync/tests/test_ieds_db_operations.py index f228591d4..b6bd943b3 100644 --- a/lambdas/id_sync/tests/test_ieds_db_operations.py +++ b/lambdas/id_sync/tests/test_ieds_db_operations.py @@ -312,6 +312,7 @@ def test_ieds_update_patient_id_success(self): "status": "success", "message": f"IEDS update, patient ID: {old_id}=>{new_id}. {len(mock_items)} updated 1." } + expected_result["nhs_number"] = old_id self.assertEqual(result, expected_result) # Verify get_items_from_patient_id was called @@ -342,6 +343,7 @@ def test_ieds_update_patient_id_non_200_response(self): "status": "error", "message": f"Failed to update some batches for patient ID: {old_id}" } + expected_result["nhs_number"] = old_id self.assertEqual(result, expected_result) # Verify transact_write_items was called (not update_item) @@ -364,6 +366,7 @@ def test_ieds_update_patient_id_no_items_found(self): "status": "success", "message": f"No items found to update for patient ID: {old_id}" } + expected_result["nhs_number"] = old_id self.assertEqual(result, expected_result) # Verify get_items_from_patient_id was called @@ -386,6 +389,7 @@ def test_ieds_update_patient_id_empty_old_id(self): "status": "error", "message": "Old ID and New ID cannot be empty" } + expected_result["nhs_number"] = old_id self.assertEqual(result, expected_result) # Verify no update was attempted @@ -406,6 +410,7 @@ def test_ieds_update_patient_id_empty_new_id(self): "status": "error", "message": "Old ID and New ID cannot be empty" } + expected_result["nhs_number"] = old_id self.assertEqual(result, expected_result) # Verify no update was attempted @@ -425,6 +430,7 @@ def test_ieds_update_patient_id_same_old_and_new_id(self): "status": "success", "message": f"No change in patient ID: {patient_id}" } + expected_result["nhs_number"] = patient_id self.assertEqual(result, expected_result) # Verify no update was attempted diff --git a/lambdas/id_sync/tests/test_ieds_db_operations_conditional.py b/lambdas/id_sync/tests/test_ieds_db_operations_conditional.py new file mode 100644 index 000000000..de70e8a3a --- /dev/null +++ b/lambdas/id_sync/tests/test_ieds_db_operations_conditional.py @@ -0,0 +1,74 @@ +import unittest +from unittest.mock import patch, MagicMock + +import ieds_db_operations + + +class TestIedsDbOperationsConditional(unittest.TestCase): + def setUp(self): + # Patch logger to suppress output + self.logger_patcher = patch('ieds_db_operations.logger') + self.mock_logger = self.logger_patcher.start() + + # Patch get_ieds_table_name and get_ieds_table + self.get_ieds_table_name_patcher = patch('ieds_db_operations.get_ieds_table_name') + self.mock_get_ieds_table_name = self.get_ieds_table_name_patcher.start() + self.mock_get_ieds_table_name.return_value = 'test-table' + + self.get_ieds_table_patcher = patch('ieds_db_operations.get_ieds_table') + self.mock_get_ieds_table = self.get_ieds_table_patcher.start() + + # Patch dynamodb client + self.dynamodb_client_patcher = patch('ieds_db_operations.dynamodb_client') + self.mock_dynamodb_client = self.dynamodb_client_patcher.start() + + def tearDown(self): + patch.stopall() + + def test_ieds_update_patient_id_empty_inputs(self): + res = ieds_db_operations.ieds_update_patient_id('', '') + self.assertEqual(res['status'], 'error') + + def test_ieds_update_patient_id_same_ids(self): + res = ieds_db_operations.ieds_update_patient_id('a', 'a') + self.assertEqual(res['status'], 'success') + + def test_ieds_update_with_items_to_update_uses_provided_list(self): + items = [{'PK': 'Patient#1'}, {'PK': 'Patient#1#r2'}] + # patch transact_write_items to return success + self.mock_dynamodb_client.transact_write_items = MagicMock(return_value={'ResponseMetadata': {'HTTPStatusCode': 200}}) + + res = ieds_db_operations.ieds_update_patient_id('1', '2', items_to_update=items) + self.assertEqual(res['status'], 'success') + # ensure transact called at least once + self.mock_dynamodb_client.transact_write_items.assert_called() + + def test_ieds_update_batches_multiple_calls(self): + # create 60 items to force 3 batches (25,25,10) + items = [{'PK': f'Patient#old#{i}'} for i in range(60)] + called = [] + + def fake_transact(TransactItems): + called.append(len(TransactItems)) + return {'ResponseMetadata': {'HTTPStatusCode': 200}} + + self.mock_dynamodb_client.transact_write_items = MagicMock(side_effect=fake_transact) + + res = ieds_db_operations.ieds_update_patient_id('old', 'new', items_to_update=items) + self.assertEqual(res['status'], 'success') + # should have been called 3 times + self.assertEqual(len(called), 3) + self.assertEqual(called[0], 25) + self.assertEqual(called[1], 25) + self.assertEqual(called[2], 10) + + def test_ieds_update_non_200_response(self): + items = [{'PK': 'Patient#1'}] + self.mock_dynamodb_client.transact_write_items = MagicMock(return_value={'ResponseMetadata': {'HTTPStatusCode': 500}}) + + res = ieds_db_operations.ieds_update_patient_id('1', '2', items_to_update=items) + self.assertEqual(res['status'], 'error') + + +if __name__ == '__main__': + unittest.main() From 8523b244a732320c4a1c439589792159215c057d Mon Sep 17 00:00:00 2001 From: Akol125 Date: Thu, 18 Sep 2025 12:18:03 +0100 Subject: [PATCH 22/25] fix lint errors --- lambdas/id_sync/src/record_processor.py | 1 - lambdas/id_sync/src/utils.py | 1 + .../id_sync/tests/test_ieds_db_operations_conditional.py | 6 ++++-- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/lambdas/id_sync/src/record_processor.py b/lambdas/id_sync/src/record_processor.py index ecb46c54c..173617206 100644 --- a/lambdas/id_sync/src/record_processor.py +++ b/lambdas/id_sync/src/record_processor.py @@ -162,4 +162,3 @@ def normalize_strings(item: Any) -> str | None: except Exception: logger.exception("demographics_match: comparison failed with exception") return False - diff --git a/lambdas/id_sync/src/utils.py b/lambdas/id_sync/src/utils.py index 765095f44..f34661be9 100644 --- a/lambdas/id_sync/src/utils.py +++ b/lambdas/id_sync/src/utils.py @@ -12,4 +12,5 @@ def log_status(msg: str, nhs_number: str | None = None, status: str = "success") result["nhs_number"] = nhs_number return result + BATCH_SIZE = 25 diff --git a/lambdas/id_sync/tests/test_ieds_db_operations_conditional.py b/lambdas/id_sync/tests/test_ieds_db_operations_conditional.py index de70e8a3a..f4469051a 100644 --- a/lambdas/id_sync/tests/test_ieds_db_operations_conditional.py +++ b/lambdas/id_sync/tests/test_ieds_db_operations_conditional.py @@ -36,7 +36,8 @@ def test_ieds_update_patient_id_same_ids(self): def test_ieds_update_with_items_to_update_uses_provided_list(self): items = [{'PK': 'Patient#1'}, {'PK': 'Patient#1#r2'}] # patch transact_write_items to return success - self.mock_dynamodb_client.transact_write_items = MagicMock(return_value={'ResponseMetadata': {'HTTPStatusCode': 200}}) + self.mock_dynamodb_client.transact_write_items = MagicMock( + return_value={'ResponseMetadata': {'HTTPStatusCode': 200}}) res = ieds_db_operations.ieds_update_patient_id('1', '2', items_to_update=items) self.assertEqual(res['status'], 'success') @@ -64,7 +65,8 @@ def fake_transact(TransactItems): def test_ieds_update_non_200_response(self): items = [{'PK': 'Patient#1'}] - self.mock_dynamodb_client.transact_write_items = MagicMock(return_value={'ResponseMetadata': {'HTTPStatusCode': 500}}) + self.mock_dynamodb_client.transact_write_items = MagicMock( + return_value={'ResponseMetadata': {'HTTPStatusCode': 500}}) res = ieds_db_operations.ieds_update_patient_id('1', '2', items_to_update=items) self.assertEqual(res['status'], 'error') From 1941dbad14441b23f4ff6a3a34ad83114883e15e Mon Sep 17 00:00:00 2001 From: Akol125 Date: Thu, 18 Sep 2025 16:11:10 +0100 Subject: [PATCH 23/25] VED-765: review based on changes --- lambdas/id_sync/src/ieds_db_operations.py | 12 +-- lambdas/id_sync/src/record_processor.py | 25 +++--- lambdas/id_sync/src/utils.py | 2 +- .../id_sync/tests/test_ieds_db_operations.py | 68 +++++++++++++++++ .../test_ieds_db_operations_conditional.py | 76 ------------------- 5 files changed, 89 insertions(+), 94 deletions(-) delete mode 100644 lambdas/id_sync/tests/test_ieds_db_operations_conditional.py diff --git a/lambdas/id_sync/src/ieds_db_operations.py b/lambdas/id_sync/src/ieds_db_operations.py index 72e55239d..09a066c3b 100644 --- a/lambdas/id_sync/src/ieds_db_operations.py +++ b/lambdas/id_sync/src/ieds_db_operations.py @@ -2,7 +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 -from utils import log_status, BATCH_SIZE +from utils import make_status, BATCH_SIZE from exceptions.id_sync_exception import IdSyncException ieds_table = None @@ -21,10 +21,10 @@ def ieds_update_patient_id(old_id: str, new_id: str, items_to_update: list | Non """Update the patient ID 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 log_status("Old ID and New ID cannot be empty", old_id, "error") + return make_status("Old ID and New ID cannot be empty", old_id, "error") if old_id == new_id: - return log_status(f"No change in patient ID: {old_id}", old_id) + return make_status(f"No change in patient ID: {old_id}", old_id) try: logger.info(f"Updating patient ID in IEDS from {old_id} to {new_id}") @@ -37,7 +37,7 @@ def ieds_update_patient_id(old_id: str, new_id: str, items_to_update: list | Non if not items_to_update: logger.warning(f"No items found to update for patient ID: {old_id}") - return log_status(f"No items found to update for patient ID: {old_id}", old_id) + return make_status(f"No items found to update for patient ID: {old_id}", old_id) logger.info(f"Items to update: {len(items_to_update)}") @@ -52,12 +52,12 @@ def ieds_update_patient_id(old_id: str, new_id: str, items_to_update: list | Non f"All batches complete. Total batches: {total_batches}, All successful: {all_batches_successful}") if all_batches_successful: - return log_status( + return make_status( f"IEDS update, patient ID: {old_id}=>{new_id}. {len(items_to_update)} updated {total_batches}.", old_id, ) else: - return log_status(f"Failed to update some batches for patient ID: {old_id}", old_id, "error") + return make_status(f"Failed to update some batches for patient ID: {old_id}", old_id, "error") except Exception as e: logger.exception("Error updating patient ID") diff --git a/lambdas/id_sync/src/record_processor.py b/lambdas/id_sync/src/record_processor.py index 173617206..4f13861c1 100644 --- a/lambdas/id_sync/src/record_processor.py +++ b/lambdas/id_sync/src/record_processor.py @@ -6,7 +6,7 @@ extract_patient_resource_from_item, get_items_from_patient_id, ) -from utils import log_status +from utils import make_status import json import ast @@ -43,33 +43,35 @@ def process_nhs_number(nhs_number: str) -> Dict[str, Any]: new_nhs_number = pds_get_patient_id(nhs_number) if not new_nhs_number: - return log_status("No patient ID found for NHS number", nhs_number) + return make_status("No patient ID found for NHS number", nhs_number) if new_nhs_number == nhs_number: - return log_status("No update required", nhs_number) + return make_status("No update required", nhs_number) + logger.info("Update patient ID from %s to %s", nhs_number, new_nhs_number) try: - pds_details, ieds_details = fetch_demographic_details(nhs_number) + # Fetch PDS Patient resource and IEDS resources for the old NHS number + pds_patient_resource, ieds_resources = fetch_pds_and_ieds_resources(nhs_number) except Exception as e: logger.exception("process_nhs_number: failed to fetch demographic details: %s", e) - return log_status(str(e), nhs_number, "error") + return make_status(str(e), nhs_number, "error") - if not ieds_details: + if not ieds_resources: logger.info("No IEDS records returned for NHS number: %s", nhs_number) - return log_status(f"No records returned for ID: {nhs_number}", nhs_number) + return make_status(f"No records returned for ID: {nhs_number}", nhs_number) # Compare demographics from PDS to each IEDS item, keep only matching records matching_records, discarded_records = [], [] - for detail in ieds_details: - if demographics_match(pds_details, detail): + for detail in ieds_resources: + if demographics_match(pds_patient_resource, detail): matching_records.append(detail) else: discarded_records.append(detail) if not matching_records: logger.info("No records matched PDS demographics: %d", len(discarded_records)) - return log_status("No records matched PDS demographics; update skipped", nhs_number) + return make_status("No records matched PDS demographics; update skipped", nhs_number) response = ieds_update_patient_id( nhs_number, new_nhs_number, items_to_update=matching_records @@ -81,7 +83,8 @@ def process_nhs_number(nhs_number: str) -> Dict[str, Any]: return response -def fetch_demographic_details(nhs_number: str): +# Function to fetch PDS Patient details and IEDS Immunisation records +def fetch_pds_and_ieds_resources(nhs_number: str): try: pds = pds_get_patient_details(nhs_number) except Exception as e: diff --git a/lambdas/id_sync/src/utils.py b/lambdas/id_sync/src/utils.py index f34661be9..32c340b4a 100644 --- a/lambdas/id_sync/src/utils.py +++ b/lambdas/id_sync/src/utils.py @@ -1,7 +1,7 @@ from typing import Dict, Any -def log_status(msg: str, nhs_number: str | None = None, status: str = "success") -> Dict[str, Any]: +def make_status(msg: str, nhs_number: str | None = None, status: str = "success") -> Dict[str, Any]: """Return a simple status dict used by record processing for observability. If `nhs_number` is None the key is omitted which keeps the output shape diff --git a/lambdas/id_sync/tests/test_ieds_db_operations.py b/lambdas/id_sync/tests/test_ieds_db_operations.py index b6bd943b3..6a9d956c5 100644 --- a/lambdas/id_sync/tests/test_ieds_db_operations.py +++ b/lambdas/id_sync/tests/test_ieds_db_operations.py @@ -539,3 +539,71 @@ def test_get_items_from_patient_id_no_records(self): # Assert self.assertEqual(result, []) + + +class TestIedsDbOperationsConditional(unittest.TestCase): + def setUp(self): + # Patch logger to suppress output + self.logger_patcher = patch('ieds_db_operations.logger') + self.mock_logger = self.logger_patcher.start() + + # Patch get_ieds_table_name and get_ieds_table + self.get_ieds_table_name_patcher = patch('ieds_db_operations.get_ieds_table_name') + self.mock_get_ieds_table_name = self.get_ieds_table_name_patcher.start() + self.mock_get_ieds_table_name.return_value = 'test-table' + + self.get_ieds_table_patcher = patch('ieds_db_operations.get_ieds_table') + self.mock_get_ieds_table = self.get_ieds_table_patcher.start() + + # Patch dynamodb client + self.dynamodb_client_patcher = patch('ieds_db_operations.dynamodb_client') + self.mock_dynamodb_client = self.dynamodb_client_patcher.start() + + def tearDown(self): + patch.stopall() + + def test_ieds_update_patient_id_empty_inputs(self): + res = ieds_db_operations.ieds_update_patient_id('', '') + self.assertEqual(res['status'], 'error') + + def test_ieds_update_patient_id_same_ids(self): + res = ieds_db_operations.ieds_update_patient_id('a', 'a') + self.assertEqual(res['status'], 'success') + + def test_ieds_update_with_items_to_update_uses_provided_list(self): + items = [{'PK': 'Patient#1'}, {'PK': 'Patient#1#r2'}] + # patch transact_write_items to return success + self.mock_dynamodb_client.transact_write_items = MagicMock( + return_value={'ResponseMetadata': {'HTTPStatusCode': 200}}) + + res = ieds_db_operations.ieds_update_patient_id('1', '2', items_to_update=items) + self.assertEqual(res['status'], 'success') + # ensure transact called at least once + self.mock_dynamodb_client.transact_write_items.assert_called() + + def test_ieds_update_batches_multiple_calls(self): + # create 60 items to force 3 batches (25,25,10) + items = [{'PK': f'Patient#old#{i}'} for i in range(60)] + called = [] + + def fake_transact(TransactItems): + called.append(len(TransactItems)) + return {'ResponseMetadata': {'HTTPStatusCode': 200}} + + self.mock_dynamodb_client.transact_write_items = MagicMock(side_effect=fake_transact) + + res = ieds_db_operations.ieds_update_patient_id('old', 'new', items_to_update=items) + self.assertEqual(res['status'], 'success') + # should have been called 3 times + self.assertEqual(len(called), 3) + self.assertEqual(called[0], 25) + self.assertEqual(called[1], 25) + self.assertEqual(called[2], 10) + + def test_ieds_update_non_200_response(self): + items = [{'PK': 'Patient#1'}] + self.mock_dynamodb_client.transact_write_items = MagicMock( + return_value={'ResponseMetadata': {'HTTPStatusCode': 500}}) + + res = ieds_db_operations.ieds_update_patient_id('1', '2', items_to_update=items) + self.assertEqual(res['status'], 'error') diff --git a/lambdas/id_sync/tests/test_ieds_db_operations_conditional.py b/lambdas/id_sync/tests/test_ieds_db_operations_conditional.py deleted file mode 100644 index f4469051a..000000000 --- a/lambdas/id_sync/tests/test_ieds_db_operations_conditional.py +++ /dev/null @@ -1,76 +0,0 @@ -import unittest -from unittest.mock import patch, MagicMock - -import ieds_db_operations - - -class TestIedsDbOperationsConditional(unittest.TestCase): - def setUp(self): - # Patch logger to suppress output - self.logger_patcher = patch('ieds_db_operations.logger') - self.mock_logger = self.logger_patcher.start() - - # Patch get_ieds_table_name and get_ieds_table - self.get_ieds_table_name_patcher = patch('ieds_db_operations.get_ieds_table_name') - self.mock_get_ieds_table_name = self.get_ieds_table_name_patcher.start() - self.mock_get_ieds_table_name.return_value = 'test-table' - - self.get_ieds_table_patcher = patch('ieds_db_operations.get_ieds_table') - self.mock_get_ieds_table = self.get_ieds_table_patcher.start() - - # Patch dynamodb client - self.dynamodb_client_patcher = patch('ieds_db_operations.dynamodb_client') - self.mock_dynamodb_client = self.dynamodb_client_patcher.start() - - def tearDown(self): - patch.stopall() - - def test_ieds_update_patient_id_empty_inputs(self): - res = ieds_db_operations.ieds_update_patient_id('', '') - self.assertEqual(res['status'], 'error') - - def test_ieds_update_patient_id_same_ids(self): - res = ieds_db_operations.ieds_update_patient_id('a', 'a') - self.assertEqual(res['status'], 'success') - - def test_ieds_update_with_items_to_update_uses_provided_list(self): - items = [{'PK': 'Patient#1'}, {'PK': 'Patient#1#r2'}] - # patch transact_write_items to return success - self.mock_dynamodb_client.transact_write_items = MagicMock( - return_value={'ResponseMetadata': {'HTTPStatusCode': 200}}) - - res = ieds_db_operations.ieds_update_patient_id('1', '2', items_to_update=items) - self.assertEqual(res['status'], 'success') - # ensure transact called at least once - self.mock_dynamodb_client.transact_write_items.assert_called() - - def test_ieds_update_batches_multiple_calls(self): - # create 60 items to force 3 batches (25,25,10) - items = [{'PK': f'Patient#old#{i}'} for i in range(60)] - called = [] - - def fake_transact(TransactItems): - called.append(len(TransactItems)) - return {'ResponseMetadata': {'HTTPStatusCode': 200}} - - self.mock_dynamodb_client.transact_write_items = MagicMock(side_effect=fake_transact) - - res = ieds_db_operations.ieds_update_patient_id('old', 'new', items_to_update=items) - self.assertEqual(res['status'], 'success') - # should have been called 3 times - self.assertEqual(len(called), 3) - self.assertEqual(called[0], 25) - self.assertEqual(called[1], 25) - self.assertEqual(called[2], 10) - - def test_ieds_update_non_200_response(self): - items = [{'PK': 'Patient#1'}] - self.mock_dynamodb_client.transact_write_items = MagicMock( - return_value={'ResponseMetadata': {'HTTPStatusCode': 500}}) - - res = ieds_db_operations.ieds_update_patient_id('1', '2', items_to_update=items) - self.assertEqual(res['status'], 'error') - - -if __name__ == '__main__': - unittest.main() From c93060875ff7e477cba2cec1bfb4fac0b968ebf6 Mon Sep 17 00:00:00 2001 From: Akol125 Date: Thu, 18 Sep 2025 16:26:14 +0100 Subject: [PATCH 24/25] VED-755: code review changes --- lambdas/id_sync/src/ieds_db_operations.py | 3 ++- lambdas/id_sync/src/utils.py | 3 --- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/lambdas/id_sync/src/ieds_db_operations.py b/lambdas/id_sync/src/ieds_db_operations.py index 09a066c3b..cfff62697 100644 --- a/lambdas/id_sync/src/ieds_db_operations.py +++ b/lambdas/id_sync/src/ieds_db_operations.py @@ -2,10 +2,11 @@ from os_vars import get_ieds_table_name from common.aws_dynamodb import get_dynamodb_table from common.clients import logger, dynamodb_client -from utils import make_status, BATCH_SIZE +from utils import make_status from exceptions.id_sync_exception import IdSyncException ieds_table = None +BATCH_SIZE = 25 # DynamoDB TransactWriteItems max batch size def get_ieds_table(): diff --git a/lambdas/id_sync/src/utils.py b/lambdas/id_sync/src/utils.py index 32c340b4a..ea9dcdef4 100644 --- a/lambdas/id_sync/src/utils.py +++ b/lambdas/id_sync/src/utils.py @@ -11,6 +11,3 @@ def make_status(msg: str, nhs_number: str | None = None, status: str = "success" if nhs_number is not None: result["nhs_number"] = nhs_number return result - - -BATCH_SIZE = 25 From 5dd076440d37a3f754ecc7692058f68b3190c1f7 Mon Sep 17 00:00:00 2001 From: Akol125 Date: Thu, 18 Sep 2025 16:52:58 +0100 Subject: [PATCH 25/25] VED-755: code review --- lambdas/id_sync/src/record_processor.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lambdas/id_sync/src/record_processor.py b/lambdas/id_sync/src/record_processor.py index 4f13861c1..5b5eac7c6 100644 --- a/lambdas/id_sync/src/record_processor.py +++ b/lambdas/id_sync/src/record_processor.py @@ -62,15 +62,16 @@ def process_nhs_number(nhs_number: str) -> Dict[str, Any]: return make_status(f"No records returned for ID: {nhs_number}", nhs_number) # Compare demographics from PDS to each IEDS item, keep only matching records - matching_records, discarded_records = [], [] + matching_records = [] + discarded_count = 0 for detail in ieds_resources: if demographics_match(pds_patient_resource, detail): matching_records.append(detail) else: - discarded_records.append(detail) + discarded_count += 1 if not matching_records: - logger.info("No records matched PDS demographics: %d", len(discarded_records)) + logger.info("No records matched PDS demographics: %d", discarded_count) return make_status("No records matched PDS demographics; update skipped", nhs_number) response = ieds_update_patient_id( @@ -79,7 +80,7 @@ def process_nhs_number(nhs_number: str) -> Dict[str, Any]: response["nhs_number"] = nhs_number # add counts for observability response["matched"] = len(matching_records) - response["discarded"] = len(discarded_records) + response["discarded"] = discarded_count return response