Resolve comments part 1 - simple fixes

Author: dlzhry2nhs

lambdas/id_sync/src/exceptions/id_sync_exception.py

@@ -1,8 +1,6 @@
 class IdSyncException(Exception):
     """Custom exception for ID Sync errors."""
 
-    def __init__(self, message: str, nhs_numbers: list = None, exception=None):
+    def __init__(self, message: str):
         self.message = message
-        self.nhs_numbers = nhs_numbers
-        self.inner_exception = exception
         super().__init__(message)

lambdas/id_sync/src/ieds_db_operations.py

@@ -21,27 +21,15 @@ def get_ieds_table():
     return ieds_table
 
 
-def ieds_update_patient_id(old_id: str, new_id: str, items_to_update: list | None = None) -> dict:
+def ieds_update_patient_id(old_id: str, new_id: str, items_to_update: list) -> dict:
     """Update the patient ID (new NHS number) in the IEDS table."""
-    if not old_id or not new_id or not old_id.strip() or not new_id.strip():
-        return make_status("Old ID and New ID cannot be empty", status="error")
+    if not items_to_update:
+        logger.info("No items found to update for patient NHS Number")
+        return make_status("No items found to update for patient ID")
 
-    if old_id == new_id:
-        return make_status("No change in patient ID")
+    logger.info(f"Items to update: {len(items_to_update)}")
 
     try:
-        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("No items found to update for patient NHS Number")
-            return make_status("No items found to update for patient ID")
-
-        logger.info(f"Items to update: {len(items_to_update)}")
-
         # 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)
@@ -61,12 +49,9 @@ def ieds_update_patient_id(old_id: str, new_id: str, items_to_update: list | Non
 
     except Exception as e:
         logger.exception("Error updating patient ID")
-        logger.info("Error details: %s", e)
         raise IdSyncException(
             message="Error updating patient ID",
-            nhs_numbers=[old_id, new_id],
-            exception=e,
-        )
+        ) from e
 
 
 def get_items_from_patient_id(id: str) -> list:
@@ -80,12 +65,10 @@ def get_items_from_patient_id(id: str) -> list:
         return paginate_items_for_patient_pk(patient_pk)
     except IdSyncException:
         raise
-    except Exception as e:
+    except Exception:
         logger.exception("Error querying items for patient PK")
         raise IdSyncException(
             message="Error querying items for patient PK",
-            nhs_numbers=[patient_pk],
-            exception=e,
         )
 
 
@@ -111,8 +94,6 @@ def paginate_items_for_patient_pk(patient_pk: str) -> list:
             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", [])

lambdas/id_sync/src/pds_details.py

@@ -31,7 +31,7 @@ def pds_get_patient_details(nhs_number: str) -> dict:
     except Exception as e:
         msg = "Error retrieving patient details from PDS"
         logger.exception(msg)
-        raise IdSyncException(message=msg, exception=e)
+        raise IdSyncException(message=msg) from e
 
 
 def get_nhs_number_from_pds_resource(pds_resource: dict) -> str:

lambdas/id_sync/src/record_processor.py

@@ -77,7 +77,7 @@ def process_nhs_number(nhs_number: str) -> Dict[str, Any]:
         logger.info("No records matched PDS demographics: %d", discarded_count)
         return make_status("No records matched PDS demographics; update skipped")
 
-    response = ieds_update_patient_id(nhs_number, new_nhs_number, items_to_update=matching_records)
+    response = ieds_update_patient_id(nhs_number, new_nhs_number, matching_records)
     # add counts for observability
     response["matched"] = len(matching_records)
     response["discarded"] = discarded_count

lambdas/id_sync/tests/test_id_sync.py

@@ -196,7 +196,6 @@ def test_handler_aws_lambda_event_exception(self):
         self.mock_logger.exception.assert_called_once_with("Error processing id_sync event")
         self.mock_process_record.assert_not_called()
 
-        self.assertEqual(result.nhs_numbers, None)
         self.assertEqual(result.message, "Error processing id_sync event")
 
     def test_handler_process_record_exception(self):
@@ -216,7 +215,6 @@ def test_handler_process_record_exception(self):
         self.mock_process_record.assert_called_once_with(mock_event.records[0])
         self.mock_logger.exception.assert_called_once_with("Error processing id_sync event")
 
-        self.assertEqual(exception.nhs_numbers, None)
         self.assertEqual(exception.message, "Error processing id_sync event")
 
     def test_handler_process_record_missing_nhs_number(self):
@@ -241,7 +239,6 @@ def test_handler_process_record_missing_nhs_number(self):
         exception = exception_context.exception
 
         self.assertIsInstance(exception, IdSyncException)
-        self.assertIsNone(exception.nhs_numbers)
         self.assertEqual(exception.message, "Processed 1 records with 1 errors")
         self.mock_logger.exception.assert_called_once_with(f"id_sync error: {exception.message}")
 

lambdas/id_sync/tests/test_ieds_db_operations.py

@@ -331,20 +331,16 @@ def test_ieds_update_patient_id_success(self):
 
         # Mock items to update
         mock_items = [
-            {"PK": "Patient#old-patient-123", "PatientPK": "Patient#old-patient-123"},
-            {
-                "PK": "Patient#old-patient-123#record1",
-                "PatientPK": "Patient#old-patient-123",
-            },
+            {"PK": "Immunization#old-patient-123", "PatientPK": "Patient#old-patient-123"},
+            {"PK": "Patient#old-patient-123#record1", "PatientPK": "Patient#old-patient-123"},
         ]
-        self.mock_get_items_from_patient_id.return_value = mock_items
 
         # Mock successful transact_write_items response
         mock_transact_response = {"ResponseMetadata": {"HTTPStatusCode": 200}}
         self.mock_dynamodb_client_patcher.transact_write_items.return_value = mock_transact_response
 
         # Act
-        result = ieds_db_operations.ieds_update_patient_id(old_id, new_id)
+        result = ieds_db_operations.ieds_update_patient_id(old_id, new_id, mock_items)
 
         # Assert - Update expected message to match actual implementation
         expected_result = {
@@ -353,9 +349,6 @@ def test_ieds_update_patient_id_success(self):
         }
         self.assertEqual(result, expected_result)
 
-        # Verify get_items_from_patient_id was called
-        self.mock_get_items_from_patient_id.assert_called_once_with(old_id)
-
         # Verify transact_write_items was called
         self.mock_dynamodb_client_patcher.transact_write_items.assert_called_once()
 
@@ -367,14 +360,13 @@ def test_ieds_update_patient_id_non_200_response(self):
 
         # Mock items to update
         mock_items = [{"PK": "Patient#old-patient-123", "PatientPK": "Patient#old-patient-123"}]
-        self.mock_get_items_from_patient_id.return_value = mock_items
 
         # Mock failed transact_write_items response (not update_item)
         mock_transact_response = {"ResponseMetadata": {"HTTPStatusCode": 400}}
         self.mock_dynamodb_client_patcher.transact_write_items.return_value = mock_transact_response
 
         # Act
-        result = ieds_db_operations.ieds_update_patient_id(old_id, new_id)
+        result = ieds_db_operations.ieds_update_patient_id(old_id, new_id, mock_items)
 
         # Assert
         expected_result = {
@@ -392,11 +384,8 @@ def test_ieds_update_patient_id_no_items_found(self):
         old_id = "old-patient-123"
         new_id = "new-patient-456"
 
-        # Mock empty items list
-        self.mock_get_items_from_patient_id.return_value = []
-
         # Act
-        result = ieds_db_operations.ieds_update_patient_id(old_id, new_id)
+        result = ieds_db_operations.ieds_update_patient_id(old_id, new_id, [])
 
         # Assert
         expected_result = {
@@ -405,71 +394,9 @@ def test_ieds_update_patient_id_no_items_found(self):
         }
         self.assertEqual(result, expected_result)
 
-        # Verify get_items_from_patient_id was called
-        self.mock_get_items_from_patient_id.assert_called_once_with(old_id)
-
         # Verify no transact operation was attempted
         self.mock_table.transact_write_items.assert_not_called()
 
-    def test_ieds_update_patient_id_empty_old_id(self):
-        """Test update with empty old_id"""
-        # Arrange
-        old_id = ""
-        new_id = "new-patient-456"
-
-        # Act
-        result = ieds_db_operations.ieds_update_patient_id(old_id, new_id)
-
-        # Assert
-        expected_result = {
-            "status": "error",
-            "message": "Old ID and New ID cannot be empty",
-        }
-        self.assertEqual(result, expected_result)
-
-        # Verify no update was attempted
-        self.mock_table.transact_write_items.assert_not_called()
-        self.mock_get_ieds_table_patcher.assert_not_called()
-
-    def test_ieds_update_patient_id_empty_new_id(self):
-        """Test update with empty new_id"""
-        # Arrange
-        old_id = "old-patient-123"
-        new_id = ""
-
-        # Act
-        result = ieds_db_operations.ieds_update_patient_id(old_id, new_id)
-
-        # Assert
-        expected_result = {
-            "status": "error",
-            "message": "Old ID and New ID cannot be empty",
-        }
-        self.assertEqual(result, expected_result)
-
-        # Verify no update was attempted
-        self.mock_table.transact_write_items.assert_not_called()
-        self.mock_get_ieds_table_patcher.assert_not_called()
-
-    def test_ieds_update_patient_id_same_old_and_new_id(self):
-        """Test update when old_id and new_id are the same"""
-        # Arrange
-        patient_id = "same-patient-id"
-
-        # Act
-        result = ieds_db_operations.ieds_update_patient_id(patient_id, patient_id)
-
-        # Assert
-        expected_result = {
-            "status": "success",
-            "message": "No change in patient ID",
-        }
-        self.assertEqual(result, expected_result)
-
-        # Verify no update was attempted
-        self.mock_table.transact_write_items.assert_not_called()
-        self.mock_get_ieds_table_patcher.assert_not_called()
-
     def test_ieds_update_patient_id_update_exception(self):
         """Test exception handling during transact_write_items"""
         # Arrange
@@ -483,52 +410,24 @@ def test_ieds_update_patient_id_update_exception(self):
                 "PatientPK": "Patient#old-patient-error",
             }
         ]
-        self.mock_get_items_from_patient_id.return_value = mock_items
 
         test_exception = Exception("DynamoDB transact failed")
         self.mock_dynamodb_client_patcher.transact_write_items.side_effect = test_exception
 
         # Act & Assert
         with self.assertRaises(Exception) as context:
-            ieds_db_operations.ieds_update_patient_id(old_id, new_id)
+            ieds_db_operations.ieds_update_patient_id(old_id, new_id, mock_items)
 
         exception = context.exception
         self.assertIsInstance(exception, IdSyncException)
         self.assertEqual(exception.message, "Error updating patient ID")
-        self.assertEqual(exception.inner_exception, test_exception)
 
         # Verify transact was attempted
         self.mock_dynamodb_client_patcher.transact_write_items.assert_called_once()
 
         # Verify logger exception was called
         self.mock_logger.exception.assert_called_once_with("Error updating patient ID")
 
-    def test_ieds_update_patient_id_special_characters(self):
-        """Test update with special characters in IDs"""
-        # Arrange
-        old_id = "old-patient@123#$%"
-        new_id = "new-patient&456*()+"
-
-        # Mock items to update
-        mock_items = [{"PK": f"Patient#{old_id}", "PatientPK": f"Patient#{old_id}"}]
-        self.mock_get_items_from_patient_id.return_value = mock_items
-
-        mock_transact_response = {"ResponseMetadata": {"HTTPStatusCode": 200}}
-        self.mock_dynamodb_client_patcher.transact_write_items.return_value = mock_transact_response
-
-        # Act
-        result = ieds_db_operations.ieds_update_patient_id(old_id, new_id)
-
-        # Assert
-        self.assertEqual(result["status"], "success")
-        self.assertEqual(
-            result["message"],
-            f"IEDS update. {len(mock_items)} item(s) updated in 1 batch(es).",
-        )
-
-        # Verify transact_write_items was called with special characters
-        self.mock_dynamodb_client_patcher.transact_write_items.assert_called_once()
-
 
 class TestGetItemsToUpdate(TestIedsDbOperations):
     def setUp(self):
@@ -601,14 +500,6 @@ def setUp(self):
     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

lambdas/id_sync/tests/test_pds_details.py

@@ -100,12 +100,10 @@ def test_pds_get_patient_details_pds_service_exception(self):
         exception = context.exception
 
         # Assert
-        self.assertEqual(exception.inner_exception, mock_exception)
         self.assertEqual(
             exception.message,
             "Error retrieving patient details from PDS",
         )
-        self.assertEqual(exception.nhs_numbers, None)
 
         # Verify exception was logged
         self.mock_logger.exception.assert_called_once_with("Error retrieving patient details from PDS")
@@ -127,7 +125,6 @@ def test_pds_get_patient_details_cache_initialization_error(self):
             exception.message,
             "Error retrieving patient details from PDS",
         )
-        self.assertEqual(exception.nhs_numbers, None)
 
         # Verify exception was logged
         self.mock_logger.exception.assert_called_once_with("Error retrieving patient details from PDS")
@@ -149,7 +146,6 @@ def test_pds_get_patient_details_auth_initialization_error(self):
             exception.message,
             "Error retrieving patient details from PDS",
         )
-        self.assertEqual(exception.nhs_numbers, None)
 
         # Verify exception was logged
         self.mock_logger.exception.assert_called_once_with("Error retrieving patient details from PDS")
@@ -167,12 +163,10 @@ def test_pds_get_patient_details_exception(self):
 
         exception = context.exception
         # Assert
-        self.assertEqual(exception.inner_exception, test_exception)
         self.assertEqual(
             exception.message,
             "Error retrieving patient details from PDS",
         )
-        self.assertEqual(exception.nhs_numbers, None)
         # Verify logger.exception was called due to the caught exception
         self.mock_logger.exception.assert_called_once_with("Error retrieving patient details from PDS")
 

lambdas/id_sync/tests/test_record_processor.py

@@ -229,7 +229,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, items_to_update=[item])
+        self.mock_ieds_update_patient_id.assert_called_once_with(nhs_number, pds_id, [item])
 
     def test_process_record_no_records_exist(self):
         """Test when no records exist for the patient ID"""