Addressed PR comments

Author: Andyg79

docs/utility-guides/Oracle.md

@@ -1,13 +1,18 @@
 # Utility Guide: Oracle
 
-The Oracle Utility provides an easy way to interact with an Oracle database directly from your Playwright test suite.It can be used to run SQL queries or stored procedures on the Oracle database.
+The Oracle Utility provides an easy way to interact with an Oracle database directly from your Playwright test suite. It can be used to run SQL queries or stored procedures on the Oracle database.
 
 ## When and Why to Use the Oracle Utility
 
-You might need to use this utility in scenarios such as:
+You might use this utility for:
 
-- To run SQL queries or stored procedures on the Oracle database.
-- Verifying that data is correctly written to or updated in the database after a workflow is completed in your application.
+- Running SQL queries or stored procedures on the Oracle database
+
+- Validating application workflows against live database records
+
+- Creating test data dynamically via helper functions
+
+- Verifying data integrity after system events
 
 ## Table of Contents
 
@@ -25,6 +30,9 @@ You might need to use this utility in scenarios such as:
 ## Using the Oracle Utility
 
 To use the Oracle Utility, import the 'OracleDB' class into your test file and then call the OracleDB methods from within your tests, as required.
+```python
+from utils.oracle.oracle import OracleDB
+```
 
 ## Required arguments
 
@@ -42,6 +50,7 @@ The docstrings also specify when arguments are optional, and what the default va
 - **execute_stored_procedure(self, `procedure_name`, params=None)**: Executes a named stored procedure with optional parameters.
 - **exec_bcss_timed_events(self, nhs_number_df)**: Runs the `bcss_timed_events` stored procedure for each NHS number provided in a DataFrame.
 - **get_subject_id_from_nhs_number(self, nhs_number)**: Retrieves the `subject_screening_id` for a given NHS number.
+- **create_subjects_via_sspi(...)**: Creates synthetic subjects using stored procedure `PKG_SSPI.p_process_pi_subject`
 
 For full implementation details, see utils/oracle/oracle.py.
 
@@ -68,6 +77,16 @@ def test_oracle_query() -> None:
 
 def run_stored_procedure() -> None:
     OracleDB().execute_stored_procedure("bcss_timed_events")
+
+def create_subjects_dynamically() -> None:
+OracleDB().create_subjects_via_sspi(
+    count=5,
+    screening_centre="BCS01",
+    base_age=60,
+    start_offset=-2,
+    end_offset=4,
+    nhs_start=9200000000
+)
 ```
 
 ---

pages/communication_production/batch_list_page.py

@@ -104,38 +104,47 @@ def enter_type_filter(self, option_text: str) -> None:
         logging.info(f"[FILTER] Type filter set to '{option_text}'")
 
     def enter_original_filter(self, search_text: str) -> None:
+        """Enter text in the Original filter and press Enter"""
         self.original_filter.fill(search_text)
         self.original_filter.press("Enter")
 
     def enter_event_code_filter(self, search_text: str) -> None:
+        """Enter text in the Event Code filter and press Enter"""
         self.event_code_filter.fill(search_text)
         self.event_code_filter.press("Enter")
 
     def enter_description_filter(self, search_text: str) -> None:
+        """Enter text in the Description filter and press Enter"""
         self.description_filter.fill(search_text)
         self.description_filter.press("Enter")
 
     def enter_batch_split_by_filter(self, search_text: str) -> None:
+        """Enter text in the 'Batch Split By' filter and press Enter"""
         self.batch_split_by_filter.fill(search_text)
         self.batch_split_by_filter.press("Enter")
 
     def enter_screening_centre_filter(self, search_text: str) -> None:
+        """Enter text in the Screening Centre filter and press Enter"""
         self.screening_centre_filter.fill(search_text)
         self.screening_centre_filter.press("Enter")
 
     def enter_count_filter(self, search_text: str) -> None:
+        """Enter text in the Count filter and press Enter"""
         self.count_filter.fill(search_text)
         self.count_filter.press("Enter")
 
     def enter_deadline_date_filter(self, date: datetime) -> None:
+        """Enter a date in the Deadline Date filter and press Enter"""
         self.click(self.deadline_calendar_picker)
         CalendarPicker(self.page).v2_calendar_picker(date)
 
     def clear_deadline_filter_date(self) -> None:
+        """Clear the date in the Deadline Date filter"""
         self.click(self.deadline_calendar_picker)
         self.click(self.deadline_date_clear_button)
 
     def verify_deadline_date_filter_input(self, expected_text: str) -> None:
+        """Verifies that the Deadline Date filter input has the expected value."""
         expect(self.deadline_date_filter_with_input).to_have_value(expected_text)
 
     def open_letter_batch(
@@ -178,6 +187,23 @@ def open_letter_batch(
         expect(view_link).to_be_visible()
         view_link.click()
 
+    def select_first_batch_row(self, table_selector: str, timeout_ms: int = 0) -> None:
+        """
+        Clicks the first batch link in the specified batch list table.
+
+        Args:
+            table_selector (str): CSS selector for the table (e.g., "table#batchList").
+            timeout_ms (int): Optional timeout to wait before interaction (default: 0).
+        """
+        first_link = self.page.locator(f"{table_selector} tbody tr td.id a").first
+        if timeout_ms > 0:
+            first_link.wait_for(timeout=timeout_ms)
+
+        assert (
+            first_link.count() > 0
+        ), f"No batch links found in table '{table_selector}'"
+        first_link.click()
+
 
 class ActiveBatchListPage(BatchListPage):
     """Active Batch List Page-specific methods."""
@@ -186,10 +212,8 @@ def __init__(self, page: Page):
         super().__init__(page, table_selector=BATCH_LIST_SELECTOR)
 
     def select_first_active_batch(self) -> None:
-        """Clicks the first batch ID link in the active batch list."""
-        first_link = self.page.locator(f"{self.table_selector} tbody tr td.id a").first
-        assert first_link.count() > 0, "No active batch links found"
-        first_link.click()
+        """Selects the first active batch in the batch list table."""
+        self.select_first_batch_row(self.table_selector, timeout_ms=10000)
 
     def is_batch_present(self, batch_type: str) -> bool:
         """Checks if a batch of the given type exists in the active batch list."""
@@ -210,13 +234,13 @@ def get_open_original_batch_row(self) -> Locator | None:
         table = TableUtils(self.page, BATCH_LIST_SELECTOR)
         row_count = table.get_row_count()
 
-        for i in range(row_count):
-            row_data = table.get_row_data_with_headers(i)
+        for row in range(row_count):
+            row_data = table.get_row_data_with_headers(row)
             if (
                 row_data.get("Type", "").strip() == "Original"
                 and row_data.get("Status", "").strip() == "Open"
             ):
-                return table.pick_row(i)
+                return table.pick_row(row)
         return None
 
     def assert_s83f_batch_present(self) -> None:
@@ -237,11 +261,8 @@ def __init__(self, page: Page):
         super().__init__(page, table_selector=BATCH_LIST_SELECTOR)
 
     def select_first_archived_batch(self) -> None:
-        """Clicks the first batch ID link in the archived batch list."""
-        first_batch_link = self.page.locator("table#batchList tbody tr td.id a").first
-        first_batch_link.wait_for(timeout=10000)
-        assert first_batch_link.count() > 0, "No archived batch links found"
-        first_batch_link.click()
+        """Selects the first archived batch in the batch list table."""
+        self.select_first_batch_row("table#batchList", timeout_ms=10000)
 
     def get_archived_batch_row(
         self, batch_type: str, event_code: str, description: str
@@ -268,8 +289,8 @@ def get_archived_batch_row(
         if row_count == 0:
             return None
 
-        for i in range(row_count):
-            row = rows.nth(i)
+        for row in range(row_count):
+            row = rows.nth(row)
 
             try:
                 type_text = row.locator("td").nth(1).inner_text().strip()

pages/communication_production/letter_library_index_page.py

@@ -11,13 +11,23 @@ class LetterLibraryIndexPage(BasePage):
     def __init__(self, page: Page):
         super().__init__(page)
         self.page = page
-        self.table_utils = TableUtils(page, DISPLAY_RS_SELECTOR)
+        self.table_utils = TableUtils(self.page, DISPLAY_RS_SELECTOR)
         # Letter Library Index - page locators, methods
 
-        self.letter_library_index_table = page.locator(DISPLAY_RS_SELECTOR)
-        self.define_supplementary_letter_button = page.locator(
+        self.letter_library_index_table = self.page.locator(DISPLAY_RS_SELECTOR)
+        self.define_supplementary_letter_button = self.page.locator(
             "input.HeaderButtons[value='Define Supplementary Letter']"
         )
+        self.letter_description_input = self.page.locator('input[name="A_C_LETT_DESC"]')
+        self.destination_dropdown = self.page.locator("#A_C_DESTINATION_ID")
+        self.priority_dropdown = self.page.locator("#A_C_PRIORITY_ID")
+        self.signatory_input = self.page.locator("#A_C_SIGNATORY")
+        self.job_title_input = self.page.locator("#A_C_JOB_TITLE")
+        self.paragraph_input = self.page.locator("#A_C_PARAGRAPH_1")
+        self.save_button = self.page.get_by_role("button", name="Save")
+        self.event_code_input = self.page.get_by_role(
+            "textbox", name="Enter text to filter the list"
+        )
 
     def verify_letter_library_index_title(self) -> None:
         """Verify the Letter Library Index page title is displayed as expected"""
@@ -62,16 +72,10 @@ def filter_by_event_code(self, event_code: str) -> None:
         Args:
             event_code (str): The event code to filter the list (e.g., 'S1')
         """
-        event_code_input = self.page.get_by_role(
-            "textbox", name="Enter text to filter the list"
-        )
-        expect(event_code_input).to_be_visible()
-        event_code_input.click()
-        event_code_input.fill(event_code)
-        event_code_input.press("Enter")
-
-        # Optional: wait for the filtered list to update
-        self.page.wait_for_timeout(500)  # tweak or replace with smart wait
+        expect(self.event_code_input).to_be_visible()
+        self.event_code_input.click()
+        self.event_code_input.fill(event_code)
+        self.event_code_input.press("Enter")
 
     def click_first_letter_code_link_in_table(self) -> None:
         """Clicks the first link from the Letter Library Index table."""
@@ -108,16 +112,15 @@ def define_supplementary_letter(
             job_title (str): Signatory's job title
             paragraph_text (str): Main body text of the letter
         """
-        self.page.locator('input[name="A_C_LETT_DESC"]').fill(description)
-        self.page.locator("#A_C_DESTINATION_ID").select_option(destination_id)
-        self.page.locator("#A_C_PRIORITY_ID").select_option(priority_id)
-        self.page.locator("#A_C_SIGNATORY").fill(signatory)
-        self.page.locator("#A_C_JOB_TITLE").fill(job_title)
-        self.page.locator("#A_C_PARAGRAPH_1").fill(paragraph_text)
-
-        # Handle the modal popup when saving
-        self.page.once("dialog", lambda dialog: dialog.accept())
-        self.page.get_by_role("button", name="Save").click()
+        self.letter_description_input.fill(description)
+        self.destination_dropdown.select_option(destination_id)
+        self.priority_dropdown.select_option(priority_id)
+        self.signatory_input.fill(signatory)
+        self.job_title_input.fill(job_title)
+        self.paragraph_input.fill(paragraph_text)
+
+        # Save and accept dialog safely
+        self.safe_accept_dialog(self.save_button)
 
 
 class LetterDefinitionDetailPage(BasePage):

pages/communication_production/manage_active_batch_page.py

@@ -2,6 +2,7 @@
 from pages.base_page import BasePage
 from playwright.sync_api import expect
 import re
+import logging
 
 
 class ManageActiveBatchPage(BasePage):
@@ -19,6 +20,13 @@ def __init__(self, page: Page):
         self.retrieve_button_text = self.page.locator('text="Retrieve"')
         self.confirm_button_text = self.page.locator('text="Confirm Printed"')
         self.reprepare_batch_text = self.page.locator('text="Re-Prepare Batch"')
+        self.button = self.page.get_by_role(
+            "button", name="Retrieve and Confirm Letters"
+        )
+        self.message = self.page.locator(
+            'text="Batch Successfully Archived and Printed"'
+        )
+        self.title_text = self.page.locator("#page-title").inner_text()
 
     def click_prepare_button(self) -> None:
         """Click the Prepare Batch button"""
@@ -40,20 +48,17 @@ def assert_active_batch_details_visible(self) -> None:
 
     def retrieve_and_confirm_letters(self) -> None:
         """Clicks the Retrieve and Confirm Letters button."""
-        button = self.page.get_by_role("button", name="Retrieve and Confirm Letters")
-        expect(button).to_be_enabled()
-        button.click()
+        expect(self.button).to_be_enabled()
+        self.button.click()
 
     def assert_confirmation_success_message(self) -> None:
         """Verifies the confirmation message is shown after printing."""
-        message = self.page.locator('text="Batch Successfully Archived and Printed"')
-        expect(message).to_be_visible()
+        expect(self.message).to_be_visible()
 
     def get_batch_id(self) -> str:
         """Extracts the batch ID from the page title."""
-        title_text = self.page.locator("#page-title").inner_text()
-        print(f"[DEBUG] Page title text: '{title_text}'")
-        match = re.search(r"\d+", title_text)
+        logging.debug(f"[BATCH ID] Page title text: '{self.title_text}'")
+        match = re.search(r"\d+", self.title_text)
         if match:
             return match.group()
         raise ValueError("Batch ID not found in page title.")

pages/communication_production/manage_archived_batch_page.py

@@ -8,21 +8,24 @@ class ManageArchivedBatchPage(BasePage):
 
     def __init__(self, page: Page):
         super().__init__(page)
+        self.page = page
+        self.header = self.page.locator("#page-title")
+        self.reprint_button = self.page.locator(
+            "input.ReprintButton[value='Reprint Batch']"
+        )
+        self.confirmation_msg = self.page.locator(
+            'text="Batch Successfully Archived and Printed"'
+        )
 
     def assert_archived_batch_details_visible(self) -> None:
         """Verifies the Manage Archived Batch page has loaded."""
-        header = self.page.locator("#page-title")
-        expect(header).to_have_text("Manage Archived Batch")
+        expect(self.header).to_have_text("Manage Archived Batch")
 
     def click_reprint_button(self) -> None:
         """Clicks the 'Reprint' button on the Archived Batch details page."""
-        reprint_button = self.page.locator("input.ReprintButton[value='Reprint Batch']")
-        expect(reprint_button).to_be_visible()
-        reprint_button.click()
+        expect(self.reprint_button).to_be_visible()
+        self.reprint_button.click()
 
     def confirm_archived_message_visible(self) -> None:
         """Verifies that the batch was successfully archived and confirmation message is shown."""
-        confirmation_msg = self.page.locator(
-            'text="Batch Successfully Archived and Printed"'
-        )
-        expect(confirmation_msg).to_be_visible()
+        expect(self.confirmation_msg).to_be_visible()