Addressing PR comments

Author: adrianoaru-nhs

pages/datasets/investigation_dataset_page.py

@@ -1,3 +1,4 @@
+import re
 from playwright.sync_api import Page, expect, Locator
 from pages.base_page import BasePage
 from enum import StrEnum
@@ -7,7 +8,6 @@
 )
 from typing import Optional
 import logging
-import re
 
 
 class InvestigationDatasetsPage(BasePage):
@@ -354,7 +354,6 @@ def select_polyp1_pathologist_option_index(self, option: int) -> None:
         """
         This method is designed to select a pathologist from the pathologist options.
         It clicks on the pathologist link and selects the given option by index.
-
         Args:
             option (int): The index of the option to select from the pathologist options.
         """
@@ -370,7 +369,6 @@ def select_lookup_option_index(self, option: int) -> None:
         """
         This method is designed to select a lookup option by index.
         It clicks on the lookup link and selects the given option by index.
-
         Args:
             option (int): The index of the option to select from the lookup options.
         """
@@ -394,7 +392,6 @@ def assert_polyp_algorithm_size(
         """
         This method asserts that the polyp size is as expected.
         It retrieves the polyp size from the page and compares it with the expected value.
-
         Args:
             polyp_number (int): The number of the polyp to check (1 or 2).
             expected_value (int): The expected value of the polyp size.
@@ -426,7 +423,6 @@ def assert_polyp_category(
         """
         This method asserts that the polyp category is as expected.
         It retrieves the polyp category from the page and compares it with the expected value.
-
         Args:
             polyp_number (int): The number of the polyp to check (1 or 2).
             expected_value (str): The expected value of the polyp category.
@@ -474,7 +470,6 @@ def get_dataset_id(self) -> int:
     def click_polyp_add_intervention_button(self, polyp_number: int) -> None:
         """
         Clicks the add intervention button for the specified polyp number.
-
         Args:
             polyp_number (int): The number of the polyp.
         """
@@ -490,15 +485,12 @@ def populate_select_by_id(
     ) -> None:
         """
         Populates a <select> element using a predictable ID pattern based on field name and polyp number.
-
         This method is useful when label-based selectors (e.g., using `right-of(:text(...))`) are unreliable
         due to ambiguous or repeated text labels on the page.
-
         Args:
             field_base (str): The base name of the field (e.g., "POLYP_PATHOLOGY_LOST").
             polyp_number (int): The polyp index (e.g., 1 for the first polyp).
             option (str): The value to be selected from the dropdown.
-
         Example:
             populate_select_by_id("POLYP_PATHOLOGY_LOST", 1, YesNoOptions.YES)
             # Selects 'Yes' in <select id="UI_POLYP_PATHOLOGY_LOST1_1">
@@ -519,14 +511,12 @@ def is_edit_dataset_button_visible(self) -> bool:
     def is_dataset_section_present(self, dataset_section_name: str) -> bool | None:
         """
         Checks if a section of the investigation dataset is present
-
         Args:
             dataset_section_name (str): The name of the section you want to check
                 - Investigation Dataset
                 - Drug Information
                 - Endoscopy Information
                 - etc...
-
         Returns:
             bool: True if it is present, False if it is not
         """
@@ -550,13 +540,10 @@ def is_dataset_section_present(self, dataset_section_name: str) -> bool | None:
     def get_dataset_section(self, dataset_section_name: str) -> Locator | None:
         """
         Retrieves a dataset section by matching its header text.
-
         Searches through all elements representing dataset sections, looking for one whose
         first <h4> header text matches the provided section name (case-insensitive).
-
         Args:
             dataset_section_name (str): The name of the dataset section to locate.
-
         Returns:
             Locator | None: A Playwright Locator for the matching section, or None if not found.
         """
@@ -583,17 +570,13 @@ def get_dataset_subsection(
     ) -> Locator | None:
         """
         Retrieves a specific subsection within a dataset section by matching the subsection's header text.
-
         The method first searches through elements with the `.DatasetSubSection` class.
         If the subsection is not found, it continues searching within `.DatasetSubSectionGroup` elements.
-
         Args:
             dataset_section_name (str): The name of the dataset section that contains the subsection.
             dataset_subsection_name (str): The name of the subsection to locate.
-
         Returns:
             Locator | None: A Playwright Locator for the found subsection, or None if not found.
-
         Raises:
             ValueError: If the specified dataset section cannot be found.
         """
@@ -647,7 +630,6 @@ def are_fields_on_page(
     ) -> bool:
         """
         Checks if the given fields are present in a section or subsection, with optional visibility checks.
-
         Args:
             section_name (str): The name of the dataset section.
             subsection_name (str | None): The name of the subsection, if any.
@@ -656,7 +638,6 @@ def are_fields_on_page(
                 - True → fields must be visible
                 - False → fields must be not visible
                 - None → visibility doesn't matter (just check presence)
-
         Returns:
             bool: True if all conditions are met; False otherwise.
         """
@@ -680,6 +661,13 @@ def are_fields_on_page(
         ]
 
         def label_matches(idx: int) -> bool:
+            """
+            Checks if the label at the given index matches the visibility condition.
+            Args:
+                idx (int): The index of the label to check.
+            Returns:
+                bool: True if the label matches the visibility condition, False otherwise.
+            """
             if visible is True:
                 return label_elements[idx].is_visible()
             if visible is False:
@@ -726,7 +714,6 @@ def check_visibility_of_drug_type(
             drug_type (str): The drug type to check
             drug_number (int): The number of the drug type cell to check.
             expected_text (str): The expected text content of the cell.
-
         Returns:
             bool: True if the visibility matches the expectation, False otherwise.
         """
@@ -738,12 +725,10 @@ def check_visibility_of_drug_dose(
     ) -> bool:
         """
         Checks the visibility of the drug dose input cell.
-
         Args:
             drug_type (str): The drug type to check
             drug_number (int): The number of the drug dose cell to check.
             visible (bool): True if the field should be visible, False if it should not.
-
         Returns:
             bool: True if the visibility matches the expectation, False otherwise.
         """
@@ -755,12 +740,10 @@ def assert_drug_type_text(
     ) -> None:
         """
         Asserts that the drug type input cell contains the expected text.
-
         Args:
             drug_type (str): The drug type to check
             drug_number (int): The number of the drug type cell to check.
             expected_text (str): The expected text content of the cell.
-
         Raises:
             AssertionError: If the actual text does not match the expected text.
         """
@@ -778,12 +761,10 @@ def assert_drug_dose_text(
     ) -> None:
         """
         Asserts that the drug dose input cell contains the expected text.
-
         Args:
             drug_type (str): The drug type to check
             drug_number (int): The number of the drug dose cell to check.
             expected_text (str): The expected text content of the cell.
-
         Raises:
             AssertionError: If the actual text does not match the expected text.
         """
@@ -799,7 +780,6 @@ def assert_drug_dose_text(
     def get_drug_type_locator(self, drug_type: str, drug_number: int) -> Locator:
         """
         Returns the locator for the matching drug type and number
-
         Args:
             drug_type (str): The drug type to check
             drug_number (int): The number of the drug to check
@@ -815,7 +795,6 @@ def get_drug_type_locator(self, drug_type: str, drug_number: int) -> Locator:
     def get_drug_dose_locator(self, drug_type: str, drug_number: int) -> Locator:
         """
         Returns the locator for the matching drug type and number
-
         Args:
             drug_type (str): The drug type to check
             drug_number (int): The number of the drug to check
@@ -833,7 +812,6 @@ def assert_drug_type_options(
     ) -> None:
         """
         Asserts that the options in the drug type dropdown are as expected.
-
         Args:
             drug_type (str): The drug type to check
             drug_number (int): The number of the drug to check
@@ -861,7 +839,6 @@ def assert_all_drug_information(
     ) -> None:
         """
         Loops through a dictionary of drug types/doses and calls the relevant assertion methods
-
         Args:
             drug_information (dict): A dictionary containing all drug types and dosages to assert
             drug_type_label (str): The type of drug to do the assertion against. E.g. 'Bowel Preparation Administered'
@@ -879,12 +856,12 @@ def assert_all_drug_information(
 
         max_drug_number = max(drug_numbers)
 
-        for i in range(1, max_drug_number + 1):
-            drug_type = drug_information.get(f"drug_type{i}")
-            drug_dose = drug_information.get(f"drug_dose{i}")
+        for drug_index in range(1, max_drug_number + 1):
+            drug_type = drug_information.get(f"drug_type{drug_index}")
+            drug_dose = drug_information.get(f"drug_dose{drug_index}")
 
             if drug_type is not None:
-                self.assert_drug_type_text(drug_type_label, i, drug_type)
+                self.assert_drug_type_text(drug_type_label, drug_index, drug_type)
 
                 # Match-case to assert drug dose unit
                 match drug_type:
@@ -920,15 +897,16 @@ def assert_all_drug_information(
                         expected_unit = None
 
                 if expected_unit is not None:
-                    self.assert_drug_dose_unit_text(drug_type_label, i, expected_unit)
+                    self.assert_drug_dose_unit_text(
+                        drug_type_label, drug_index, expected_unit
+                    )
 
             if drug_dose is not None:
-                self.assert_drug_dose_text(drug_type_label, i, drug_dose)
+                self.assert_drug_dose_text(drug_type_label, drug_index, drug_dose)
 
     def get_drug_dose_unit_locator(self, drug_type: str, drug_number: int) -> Locator:
         """
         Returns the locator for the matching drug type and number
-
         Args:
             drug_type (str): The drug type to check
             drug_number (int): The number of the drug to check
@@ -946,12 +924,10 @@ def assert_drug_dose_unit_text(
     ) -> None:
         """
         Asserts that the drug dose unit contains the expected text.
-
         Args:
             drug_type (str): The drug type to check
             drug_number (int): The number of the drug dose cell to check.
             expected_text (str): The expected text content of the cell.
-
         Raises:
             AssertionError: If the actual text does not match the expected text.
         """

pages/login/select_job_role_page.py

@@ -14,7 +14,6 @@ def __init__(self, page: Page):
     def select_option_for_job_role(self, job_role: str) -> None:
         """
         Selects a job from the job role dropdown
-
         Args:
             job_role (str): This is the text of the role you want to select (e.g. Screening Practitioner)
         """

tests/regression/subject/episodes/datasets/investigation/endoscopy/test_endoscopy_investigation_dataset_scenarios.py

@@ -168,7 +168,6 @@ def test_check_different_hub_roles_access_to_edit_endoscopy_investigation_datase
 ) -> None:
     """
     Scenario: Check different hub roles' access to edit an endoscopy investigation dataset
-
     This scenario only checks roles which have permission to at least view a dataset.
     """
     nhs_no = get_subject_with_incomplete_endoscopy_investigation_dataset()
@@ -243,7 +242,6 @@ def test_check_different_screening_centre_roles_access_to_edit_endoscopy_investi
 ) -> None:
     """
     Scenario: Check different screening centre roles' access to edit an endoscopy investigation dataset
-
     This scenario only checks roles which have permission to at least view a dataset.
     """
     nhs_no = get_subject_with_incomplete_endoscopy_investigation_dataset()
@@ -331,7 +329,6 @@ def test_check_different_national_and_qa_roles_access_to_edit_endoscopy_investig
 ) -> None:
     """
     Scenario: Check different national and QA roles' access to edit an endoscopy investigation dataset
-
     This scenario only checks roles which have permission to at least view a dataset.
     """
     nhs_no = get_subject_with_incomplete_endoscopy_investigation_dataset()
@@ -365,8 +362,7 @@ def test_check_correct_sections_displayed_in_colonoscopy_investigation_dataset(
     page: Page,
 ) -> None:
     """
-    Check the correct sections are displayed in new Colonoscopy investigation dataset
-
+    Scenario: Check the correct sections are displayed in new Colonoscopy investigation dataset
     This scenario just checks that the dataset contains the expected sections and that they are labelled for an endoscopy dataset rather than a radiology dataset.
     """
     nhs_no = get_subject_with_new_colonoscopy_investigation_dataset()
@@ -469,8 +465,7 @@ def test_check_field_visibility_and_default_values_in_colonoscopy_investigation_
     page: Page,
 ) -> None:
     """
-    Check field visibility and default values in the Investigation Dataset section of a new Colonoscopy investigation dataset
-
+    Scenario: Check field visibility and default values in the Investigation Dataset section of a new Colonoscopy investigation dataset
     The top section of the investigation dataset, labelled Investigation Dataset but referred to as the Diagnostic Test section in the spec, contains mostly read-only fields.  This scenario checks that the expected fields are included, and carries out limited checks of the default values (many of these values can only be sensibly tested in a scenario which creates the test from scratch).
     """
     nhs_no = get_subject_with_new_colonoscopy_investigation_dataset()
@@ -644,7 +639,6 @@ def test_check_cross_field_validation_is_mandatory_at_completion(page: Page) ->
 def test_check_behaviour_of_aspirant_endoscopist_fields(page: Page) -> None:
     """
     Scenario: Check the behaviour of the Aspirant Endoscopist fields
-
     This tests:
     > Aspirant Endoscopist Not Present field is enabled until an Aspirant Endoscopist is selected
     > If Aspirant Endoscopist Not Present is ticked, it is automatically unticked and disabled when an Aspirant Endoscopist is selected
@@ -843,17 +837,14 @@ def test_check_behaviour_of_drug_information_fields_in_incomplete_dataset(
 ) -> None:
     """
     Scenario: Check the behaviour of the Bowel Preparation Administered fields in the Drug Information section in an incomplete endoscopy investigation dataset
-
     This scenario tests:
     > The contents of dropdown lists.
     > The default values of fields.
     > The visibility of drug type/dose fields.
     > The correct dose units are displayed.
-
     Immediate validation:
     > A "drug administered" field cannot be set to "No" or null if associated drugs and doses are listed.
     > Dose values must be within the valid range for the drug type, and have the valid number of decimal places (although invalid values are not removed).
-
     "On save" validation:
     > Bowel Preparation Administered cannot be set to "No" if the Endoscopist Defined Extent is beyond Rectum. (The reverse of this is not tested as this scenario does not save changes).
     > The same drug type cannot be selected more than once
@@ -1253,18 +1244,15 @@ def check_role_access_to_edit_investigation_dataset(
 ) -> None:
     """
     Verifies whether a user with a specific role has access to edit the Investigation Dataset.
-
     This function logs in as a given user role, navigates to the subject's investigation dataset,
     and asserts the visibility of the "Edit Dataset" button based on expected access.
-
     Args:
         page (Page): The Playwright page object for interacting with the UI.
         nhs_no (str): NHS number used to search for the subject.
         role (str): The user role to log in as.
         edit_access (bool): True if the role is expected to have edit access; False otherwise.
         role_logging (str): Descriptive log message for tracking which role is being tested.
         role_type (Optional[str]): Optional job role selection (e.g., if multiple job roles exist).
-
     Raises:
         AssertionError: If the visibility of the "Edit Dataset" button does not match the expected access.
     """
@@ -1311,7 +1299,6 @@ def get_subject_with_incomplete_endoscopy_investigation_dataset() -> str:
     Gets a subject with the following criteria:
         "latest episode status": "open"
         "latest episode latest investigation dataset": "endoscopy_incomplete"
-
     Returns:
         str: The nhs number of a subject matching the criteria
     """
@@ -1342,7 +1329,6 @@ def get_subject_with_new_colonoscopy_investigation_dataset() -> str:
     Gets a subject with the following criteria:
         "latest episode status": "open",
         "latest episode latest investigation dataset": "colonoscopy_new",
-
     Returns:
         str: The nhs number of a subject matching the criteria
     """