Addressing PR comments

Author: adrianoaru-nhs

classes/analyser.py

@@ -6,7 +6,17 @@
 @dataclass
 class Analyser:
     """
-    Data class representing an analyser.
+    This data class is used to store information about a specific analyser device.
+    The attributes correspond to the columns in the analyser query results.
+    Attributes:
+        analyser_id (Optional[int]): The ID of the analyser.
+        analyser_code (Optional[str]): The code of the analyser.
+        hub_id (Optional[int]): The ID of the hub the analyser is connected to.
+        analyser_type_id (Optional[int]): The type ID of the analyser.
+        spoil_result_code (Optional[int]): The result code for spoilage.
+        tech_fail_result_code (Optional[int]): The result code for technical failure.
+        below_range_result_code (Optional[int]): The result code for below range.
+        above_range_result_code (Optional[int]): The result code for above range.
     """
 
     analyser_id: Optional[int] = None
@@ -38,6 +48,7 @@ def from_dataframe_row(row: pd.Series) -> "Analyser":
                 - analyser_code
                 - hub_id
                 - tk_analyser_type_id
+            The other columns are obtained from the SQL query as so are not present in this method.
 
         Returns:
             Analyser: The constructed Analyser object.

classes/analyser_result_code_type.py

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

classes/entities/kit_service_management_entity.py

@@ -55,7 +55,7 @@ def from_record(
             device_id=record.device_id,
             test_kit_type=(
                 str(record.test_kit_type) if record.test_kit_type is not None else None
-            ),
+            ),  # test_kit_type is converted to a string as this is what is required in the database
             test_kit_name=record.test_kit_name,
             test_kit_status=record.test_kit_status,
             logged_by_hub=record.logged_by_hub,

classes/episode_result_type.py

@@ -1,7 +1,8 @@
-from typing import Optional, Dict
+from enum import Enum
+from typing import Optional
 
 
-class EpisodeResultType:
+class EpisodeResultType(Enum):
     NO_RESULT = (20311, "No Result")
     NORMAL = (20312, "Normal (No Abnormalities Found)")
     LOW_RISK_ADENOMA = (20314, "Low-risk Adenoma")
@@ -37,80 +38,38 @@ class EpisodeResultType:
     NULL = (0, "Null")
     NOT_NULL = (0, "Not Null")
 
-    _all_types = [
-        NO_RESULT,
-        NORMAL,
-        LOW_RISK_ADENOMA,
-        INTERMEDIATE_RISK_ADENOMA,
-        HIGH_RISK_ADENOMA,
-        CANCER_DETECTED,
-        ABNORMAL,
-        CANCER_NOT_CONFIRMED,
-        HIGH_RISK_FINDINGS,
-        LNPCP,
-        BOWEL_SCOPE_NON_PARTICIPATION,
-        FOBT_INADEQUATE_PARTICIPATION,
-        DEFINITIVE_NORMAL_FOBT_OUTCOME,
-        DEFINITIVE_ABNORMAL_FOBT_OUTCOME,
-        HIGH_RISK_FINDINGS_SURVEILLANCE_NON_PARTICIPATION,
-        LNPCP_SURVEILLANCE_NON_PARTICIPATION,
-        HIGH_RISK_SURVEILLANCE_NON_PARTICIPATION,
-        INTERMEDIATE_RISK_SURVEILLANCE_NON_PARTICIPATION,
-        LYNCH_NON_PARTICIPATION,
-        ANY_SURVEILLANCE_NON_PARTICIPATION,
-        NULL,
-        NOT_NULL,
-    ]
+    def __init__(self, valid_value_id: int, description: str):
+        self._id = valid_value_id
+        self._description = description
 
-    _descriptions: Dict[str, "EpisodeResultType"] = {}
-    _lowercase_descriptions: Dict[str, "EpisodeResultType"] = {}
-    _valid_value_ids: Dict[int, "EpisodeResultType"] = {}
+    @property
+    def id(self) -> int:
+        """Return the valid value ID."""
+        return self._id
 
-    def __init__(self, valid_value_id: int, description: str):
-        self.valid_value_id = valid_value_id
-        self.description = description
+    @property
+    def description(self) -> str:
+        """Return the description."""
+        return self._description
 
     @classmethod
-    def _init_types(cls):
-        if not cls._descriptions:
-            for valid_value_id, description in cls._all_types:
-                instance = cls(valid_value_id, description)
-                cls._descriptions[description] = instance
-                cls._lowercase_descriptions[description.lower()] = instance
-                # Only map the first occurrence of each valid_value_id
-                if valid_value_id not in cls._valid_value_ids:
-                    cls._valid_value_ids[valid_value_id] = instance
+    def by_id(cls, valid_value_id: int) -> Optional["EpisodeResultType"]:
+        """Find an EpisodeResultType by its ID (returns first match if duplicates)."""
+        return next((m for m in cls if m.id == valid_value_id), None)
 
     @classmethod
     def by_description(cls, description: str) -> Optional["EpisodeResultType"]:
-        cls._init_types()
-        return cls._descriptions.get(description)
+        """Find an EpisodeResultType by its description (case-sensitive)."""
+        return next((m for m in cls if m.description == description), None)
 
     @classmethod
     def by_description_case_insensitive(
         cls, description: str
     ) -> Optional["EpisodeResultType"]:
-        cls._init_types()
-        return cls._lowercase_descriptions.get(description.lower())
-
-    @classmethod
-    def by_valid_value_id(cls, valid_value_id: int) -> Optional["EpisodeResultType"]:
-        cls._init_types()
-        return cls._valid_value_ids.get(valid_value_id)
-
-    def get_id(self) -> int:
-        return self.valid_value_id
-
-    def get_description(self) -> str:
-        return self.description
-
-    def __eq__(self, other):
-        if isinstance(other, EpisodeResultType):
-            return (
-                self.valid_value_id == other.valid_value_id
-                and self.description == other.description
-            )
-        return False
+        """Find an EpisodeResultType by its description (case-insensitive)."""
+        description = description.lower()
+        return next((m for m in cls if m.description.lower() == description), None)
 
-    def __repr__(self):
-        return f"EpisodeResultType({self.valid_value_id}, '{self.description}')"
+    def __str__(self) -> str:
+        """Return a string representation of the EpisodeResultType."""
+        return f"{self.name} ({self.id}: {self.description})"

classes/kit_service_management_record.py

@@ -66,7 +66,7 @@ def from_dataframe_row(row) -> "KitServiceManagementRecord":
             row (pd.Series): A row from a pandas DataFrame with columns matching the query.
 
         Returns:
-            KitServiceManagementRecord: The constructed object.
+            KitServiceManagementRecord:  A populated KitServiceManagementRecord object from the given DataFrame row.
         """
 
         def parse_decimal(value):

classes/repositories/analyser_repository.py

@@ -69,7 +69,14 @@ def get_technical_fail_result_code(self, analyser_type_id: int) -> int:
     def get_result_code(
         self, analyser_type_id: int, result_code_type: "AnalyserResultCodeType"
     ) -> int:
-        """ """
+        """
+        Gets the result code for the specified analyser type and result code type from the database.
+        Args:
+            analyser_type_id (int): The ID of the analyser type.
+            result_code_type (AnalyserResultCodeType): The type of result code.
+        Returns:
+            int: The result code for the specified analyser type and result code type
+        """
         query = """
         SELECT
             tkte.tk_analyser_type_id,

classes/repositories/general_repository.py

@@ -19,6 +19,10 @@ def run_database_transition(
     ) -> None:
         """
         Executes the PKG_EPISODE.p_set_episode_next_status stored procedure with the provided parameters.
+        Args:
+            database_transition_parameters (DatabaseTransitionParameters): The parameters for the database transition.
+        Raises:
+            oracledb.DatabaseError: If there is an error executing the database transition.
         """
         logging.debug(
             f"Running database transition with transition_id = {database_transition_parameters.transition_id}"
@@ -47,6 +51,7 @@ def run_database_transition(
             conn.commit()
             logging.debug("Database transition executed successfully.")
         except Exception as e:
+            logging.error("Database transition failed", exc_info=True)
             raise oracledb.DatabaseError(f"Error executing database transition: {e}")
         finally:
             self.oracle_db.disconnect_from_db(conn)

classes/repositories/kit_service_management_repository.py

@@ -19,7 +19,7 @@ def _device_filter(self, device_id: Optional[str]) -> list[str]:
         Args:
             device_id (str): The device ID of the subject
         Returns:
-            list[str]: A filter for device_id if it is provided, otherwise it returns an empty string
+            list[str]: A filter for device_id if it is provided, otherwise it returns an empty list
         """
         return ["AND kq.device_id = :device_id"] if device_id else []
 
@@ -29,7 +29,7 @@ def _archived_filter(self, archived: Optional[bool]) -> list[str]:
         Args:
             archived (bool): Whether to include archived records
         Returns:
-            list[str]: A filter for date_time_error_archived
+            list[str]: An SQL filter clause to include or exclude archived records based on the archived flag.
         """
         if archived is None:
             return []
@@ -42,7 +42,7 @@ def _hub_filter(self, issuing_hub_id: int, logged_hub_id: int) -> list[str]:
             issuing_hub_id (int): The ID of the issuing hub
             logged_hub_id (int): The ID of the logged hub
         Returns:
-            list[str]: A filter for the hubs
+            list[str]: An SQL filter clause for the hubs
         """
         hub_conditions = []
         if logged_hub_id > -1:
@@ -60,7 +60,7 @@ def _status_filter(
             status (str): The status of the test kit
             if_error_has_id (bool): Whether to include records with an error ID
         Returns:
-            list[str]: A filter for the statuses
+            list[str]: An SQL filter clause for the statuses
         """
         if not status:
             error_status = [

classes/repositories/user_repository.py

@@ -18,12 +18,12 @@ def __init__(self):
     def general_query(self, role: "UserRoleType") -> pd.DataFrame:
         """
         Gets the pio_id, org_id, role_id and org_code of a user
-
         Args:
             role (UserRoleType): A UserRoleType object containing the necessary information to run the query
-
         Returns:
             pd.DataFrame: A dataframe containing the pio_id, org_id, role_id and org_code of a user
+        Raises:
+            ValueError: If no user data is returned from the query / the dataframe is empty
         """
         sql = """
             SELECT

classes/user.py

@@ -147,8 +147,6 @@ def from_dataframe_row(self, row) -> "User":
         Returns:
             User: The constructed User object.
         """
-        from classes.organisation_complex import Organisation
-
         organisation = (
             Organisation(new_id=row["org_id"], new_code=row["org_code"])
             if "org_id" in row and "org_code" in row