reduce cognitive complexity and use utils and constants

Akol125 · Akol125 · commit 1ae026a45f59 · 2025-09-18T12:00:18.000+01:00
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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()
