Addressing PR comments

Author: adrianoaru-nhs

classes/address/address.py

@@ -26,21 +26,13 @@ def set_address_line(self, line_number: int, address_line: str) -> None:
         Raises:
             ValueError: If line_number is not between 1 and 5.
         """
-        if line_number == 1:
-            self.address_line1 = address_line
-        elif line_number == 2:
-            self.address_line2 = address_line
-        elif line_number == 3:
-            self.address_line3 = address_line
-        elif line_number == 4:
-            self.address_line4 = address_line
-        elif line_number == 5:
-            self.address_line5 = address_line
-        else:
+        if not 1 <= line_number <= 5:
             raise ValueError(
                 f"Invalid line number {line_number}, must be between 1 and 5"
             )
 
+        setattr(self, f"address_line{line_number}", address_line)
+
     def __str__(self) -> str:
         """
         Returns the formatted address as a single string.

classes/datasets/intended_extent_type.py

@@ -1,5 +1,5 @@
 from enum import Enum
-from typing import Optional, Dict
+from typing import Optional
 
 
 class IntendedExtentType(Enum):

classes/datasets/symptomatic_procedure_result_type.py

@@ -1,5 +1,5 @@
 from enum import Enum
-from typing import Optional, Dict
+from typing import Optional
 
 
 class SymptomaticProcedureResultType(Enum):

classes/diagnostic/diagnostic_test_has_outcome_of_result.py

@@ -5,6 +5,7 @@
 class DiagnosticTestHasOutcomeOfResult(Enum):
     """
     Enum representing possible outcomes of a diagnostic test, mapped to IDs and descriptions.
+    'Yes' and 'No' have negative IDs to clearly mark them as special, non-database values.
     """
 
     NO = (-1, "No")

classes/diagnostic/diagnostic_test_type.py

@@ -6,6 +6,7 @@ class DiagnosticTestType(Enum):
     """
     Enum representing diagnostic test types, mapped to valid value IDs, descriptions, categories, and allowed status.
     Provides utility methods for lookup by description (case-sensitive and insensitive) and by valid value ID.
+    'ANY' is a special placeholder representing a wildcard or default fallback when a specific diagnostic test type is not required.
     """
 
     BARIUM_ENEMA = (16003, "Barium Enema", "RADIOLOGY", "DISALLOWED")

classes/diagnostic/which_diagnostic_test.py

@@ -6,6 +6,11 @@ class WhichDiagnosticTest(Enum):
     """
     Enum representing which diagnostic test to select, with description and test number.
     Provides utility methods for lookup by description (case-sensitive and insensitive).
+    Conventions:
+        - test_number > 0: Refers to an explicit test index within the latest episode
+        (e.g., test 1, test 2, ...).
+        - test_number == 0: Indicates a selection rule or condition rather than a
+        fixed test index (e.g., "any test", "latest test", "earliest not-void test").
     """
 
     ANY_TEST_IN_ANY_EPISODE = ("any test in any episode", 0)

classes/notify/notify_message_type.py

@@ -5,6 +5,7 @@
 class NotifyMessageType(Enum):
     """
     Enum representing notify message types, with description and event status ID.
+    '11197' is the event status id for all S1 notify message types
     """
 
     NONE = ("None", None)

classes/yes_no/yes_no_type.py

@@ -4,7 +4,7 @@
 
 class YesNoType(Enum):
     """
-    Enum representing Yes/No values for investigation datasets, with valid value IDs and descriptions.
+    Enum representing Yes/No values, with valid value IDs and descriptions.
     """
 
     YES = (17058, "Yes")

utils/oracle/oracle_specific_functions/kit_management.py

@@ -66,15 +66,15 @@ def get_kit_id_logged_from_db(smokescreen_properties: dict) -> pd.DataFrame:
     AND se.episode_status_id = :open_episode_status_id
     AND tk.tk_type_id = 2
     AND se.latest_event_status_id = :s43_event_status
-    AND tk.logged_in_at = :logged_in_at
+    AND tk.logged_in_at = :test_org_id
     AND tk.reading_flag = 'N'
     AND tk.test_results IS NULL
     fetch first :subjects_to_retrieve rows only
     """
 
     params = {
         "s43_event_status": SqlQueryValues.S43_EVENT_STATUS,
-        "logged_in_at": smokescreen_properties["c3_fit_kit_results_test_org_id"],
+        "test_org_id": smokescreen_properties["c3_fit_kit_results_test_org_id"],
         "open_episode_status_id": SqlQueryValues.OPEN_EPISODE_STATUS_ID,
         "subjects_to_retrieve": smokescreen_properties["c3_total_fit_kits_to_retrieve"],
     }
@@ -146,18 +146,22 @@ def update_kit_service_management_entity(
         subject_nhs_number (str): The NHS Number of the affected subject
     """
     get_service_management_df = get_service_management_by_device_id(device_id)
-
-    # Extract the NHS number from the DataFrame
-    subject_nhs_number = get_service_management_df["subject_nhs_number"].iloc[0]
-    test_kit_name = get_service_management_df["test_kit_name"].iloc[0]
-    test_kit_type = get_service_management_df["test_kit_type"].iloc[0]
-    logged_by_hub = get_service_management_df["logged_by_hub"].iloc[0]
-    date_time_logged = get_service_management_df["date_time_logged"].iloc[0]
-    calculated_result = get_service_management_df["calculated_result"].iloc[0]
-    post_response = get_service_management_df["post_response"].iloc[0]
-    post_attempts = get_service_management_df["post_attempts"].iloc[0]
-    put_response = get_service_management_df["put_response"].iloc[0]
-    put_attempts = get_service_management_df["put_attempts"].iloc[0]
+    try:
+        # Extract the NHS number from the DataFrame
+        subject_nhs_number = get_service_management_df["subject_nhs_number"].iloc[0]
+        test_kit_name = get_service_management_df["test_kit_name"].iloc[0]
+        test_kit_type = get_service_management_df["test_kit_type"].iloc[0]
+        logged_by_hub = get_service_management_df["logged_by_hub"].iloc[0]
+        date_time_logged = get_service_management_df["date_time_logged"].iloc[0]
+        calculated_result = get_service_management_df["calculated_result"].iloc[0]
+        post_response = get_service_management_df["post_response"].iloc[0]
+        post_attempts = get_service_management_df["post_attempts"].iloc[0]
+        put_response = get_service_management_df["put_response"].iloc[0]
+        put_attempts = get_service_management_df["put_attempts"].iloc[0]
+    except IndexError as e:
+        raise IndexError(
+            f"No service management records found for device_id={device_id}"
+        ) from e
     # format date
     date_time_authorised = (
         datetime.now().strftime("%d-%b-%y %H.%M.%S.")

utils/oracle/oracle_specific_functions/organisation.py

@@ -9,16 +9,14 @@ def delete_organisation_relationships_created_for_test(org_codes: list[str]) ->
     Args:
         org_codes (list[str]): A list of organisation codes to delete relationships for.
     """
-    placeholders = ", ".join([f":org{i}" for i in range(len(org_codes))])
+    placeholders, params = _make_placeholders_and_params("org", org_codes)
     subquery = f"SELECT ORG_ID FROM ORG WHERE ORG_CODE IN ({placeholders})"
 
     query = f"""
     DELETE FROM ORG_RELATIONSHIPS
     WHERE CHILD_ORG_ID IN ({subquery})
     OR PARENT_ORG_ID IN ({subquery})
     """
-
-    params = {f"org{i}": code for i, code in enumerate(org_codes)}
     OracleDB().update_or_insert_data_to_table(query, params)
 
 
@@ -29,15 +27,13 @@ def delete_people_in_org_created_for_test(org_codes: list[str]) -> None:
     Args:
         org_codes (list[str]): A list of organisation codes to delete people in organisations for.
     """
-    placeholders = ", ".join([f":org{i}" for i in range(len(org_codes))])
+    placeholders, params = _make_placeholders_and_params("org", org_codes)
     subquery = f"SELECT ORG_ID FROM ORG WHERE ORG_CODE IN ({placeholders})"
 
     query = f"""
     DELETE FROM PERSON_IN_ORG
     WHERE ORG_ID IN ({subquery})
     """
-
-    params = {f"org{i}": code for i, code in enumerate(org_codes)}
     OracleDB().update_or_insert_data_to_table(query, params)
 
 
@@ -48,13 +44,11 @@ def delete_orgs_created_for_test(org_codes: list[str]) -> None:
     Args:
         org_codes (list[str]): A list of organisation codes to delete.
     """
-    placeholders = ", ".join([f":org{i}" for i in range(len(org_codes))])
+    placeholders, params = _make_placeholders_and_params("org", org_codes)
     query = f"""
     DELETE FROM ORG
     WHERE ORG_CODE IN ({placeholders})
     """
-
-    params = {f"org{i}": code for i, code in enumerate(org_codes)}
     OracleDB().update_or_insert_data_to_table(query, params)
 
 
@@ -82,15 +76,25 @@ def delete_sites_created_for_test(site_codes: list[str]) -> None:
     logging.info("Start: delete_sites_created_for_test(%s)", site_codes)
 
     # Dynamically create placeholders like :site0, :site1, ...
-    placeholders = ", ".join([f":site{i}" for i in range(len(site_codes))])
+    placeholders, params = _make_placeholders_and_params("site", site_codes)
     query = f"""
     DELETE FROM SITES
     WHERE SITE_CODE IN ({placeholders})
     """
-
-    # Bind each site code to its placeholder
-    params = {f"site{i}": code for i, code in enumerate(site_codes)}
-
     OracleDB().update_or_insert_data_to_table(query, params)
 
     logging.info("End: delete_sites_created_for_test(%s)", site_codes)
+
+
+def _make_placeholders_and_params(prefix: str, values: list[str]) -> tuple[str, dict]:
+    """
+    Helper to generate SQL placeholders and parameter dict for IN clauses.
+    Args:
+        prefix (str): The prefix for the placeholder (e.g., 'org', 'site').
+        values (list[str]): The values to bind.
+    Returns:
+        tuple: (placeholders string, params dict)
+    """
+    placeholders = ", ".join([f":{prefix}{i}" for i in range(len(values))])
+    params = {f"{prefix}{i}": value for i, value in enumerate(values)}
+    return placeholders, params