Update webhook tests following the PF5 webhook upgrade

[adamlazik1]

Referring theforeman/foreman_webhooks#89

[adamlazik1]

trigger: test-robottelo
pytest: tests/foreman/ui/test_webhook.py
theforeman:
    foreman_webhooks: 89

[adamlazik1]

trigger: test-robottelo
pytest: tests/foreman/ui/test_webhook.py
theforeman:
    foreman_webhooks: 89

[adamlazik1]

While this is ready for review I would prefer to remove the UI webhook test completely. For more information see SatelliteQE/robottelo#20678

[sourcery-ai]

Hey - I've found 2 issues, and left some high level feedback:

• In WebhookFormView.read, the widget_names parameter is currently unused; either support limiting the fields based on it or remove the parameter to avoid confusion.
• In PF5TypeaheadSelect.fill, consider using browser.wait_for_element (or a similar browser helper) instead of a bare wait_for + is_displayed to reduce flakiness and make the waiting logic consistent with the rest of the codebase.
• The new wait_for_popup in WebhookFormView no longer returns a boolean like the old implementation did; if callers rely on the previous return value, either restore a boolean result or rename the method to better reflect that it only waits and does not report visibility.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `WebhookFormView.read`, the `widget_names` parameter is currently unused; either support limiting the fields based on it or remove the parameter to avoid confusion.
- In `PF5TypeaheadSelect.fill`, consider using `browser.wait_for_element` (or a similar browser helper) instead of a bare `wait_for` + `is_displayed` to reduce flakiness and make the waiting logic consistent with the rest of the codebase.
- The new `wait_for_popup` in `WebhookFormView` no longer returns a boolean like the old implementation did; if callers rely on the previous return value, either restore a boolean result or rename the method to better reflect that it only waits and does not report visibility.

## Individual Comments

### Comment 1
<location> `airgun/widgets.py:3132-3145` </location>
<code_context>
+        Args:
+            value: The text value to select
+        """
+        input_el = self.browser.element(self.locator)
+        self.browser.clear(input_el)
+        input_el.send_keys(value)
+        option_locator = (
+            f'//li[contains(@class, "pf-v5-c-menu__list-item")]'
+            f'//span[contains(@class, "pf-v5-c-menu__item-text") and normalize-space(.)="{value}"]'
</code_context>

<issue_to_address>
**suggestion (bug_risk):** Scope the option locator for `PF5TypeaheadSelect` to avoid ambiguity when multiple typeaheads are present.

Because `option_locator` is a global `//li...` XPath, `browser.is_displayed(option_locator)` / `browser.element(option_locator)` can hit the wrong dropdown when multiple PF5 typeahead selects exist on the page. Please scope the locator to an element related to `input_el` (e.g., via `aria-owns` or a closer ancestor) so it only targets the menu for this input.

```suggestion
        input_el = self.browser.element(self.locator)
        self.browser.clear(input_el)
        input_el.send_keys(value)

        # Scope the option locator to the menu associated with this input to avoid
        # ambiguity when multiple PF5 typeahead selects are present.
        menu_id = input_el.get_attribute("aria-owns") or input_el.get_attribute("aria-controls")
        if menu_id:
            option_locator = (
                f'//*[@id="{menu_id}"]'
                f'//li[contains(@class, "pf-v5-c-menu__list-item")]'
                f'//span[contains(@class, "pf-v5-c-menu__item-text") and normalize-space(.)="{value}"]'
            )
        else:
            # Fallback to the global locator if no association attribute is present
            option_locator = (
                f'//li[contains(@class, "pf-v5-c-menu__list-item")]'
                f'//span[contains(@class, "pf-v5-c-menu__item-text") and normalize-space(.)="{value}"]'
            )

        wait_for(
            lambda: self.browser.is_displayed(option_locator),
            timeout=10,
            delay=0.5,
            handle_exception=True,
        )
        self.browser.element(option_locator).click()
```
</issue_to_address>

### Comment 2
<location> `airgun/views/webhook.py:79-92` </location>
<code_context>
+                for field_name, value in tabs_to_fill[tab_name].items():
+                    getattr(self, field_name).fill(value)
+
+    def read(self, widget_names=None):
+        """Read form values from all tabs."""
+        result = {'general': {}, 'credentials': {}, 'additional': {}}
+        fields = {
+            'general': ['subscribe_to', 'name', 'target_url', 'template', 'http_method', 'enabled'],
+            'credentials': ['user', 'password', 'verify_ssl', 'capsule_auth', 'certs'],
+            'additional': ['content_type', 'headers'],
+        }
+        for tab_name, field_list in fields.items():
+            self._switch_to_tab(tab_name)
+            for field_name in field_list:
+                widget = getattr(self, field_name)
+                result[tab_name][field_name] = widget.read() if widget.is_displayed else None
+        return result

     @property
</code_context>

<issue_to_address>
**suggestion:** Either use or remove the `widget_names` parameter in `WebhookFormView.read`.

The parameter is unused while the docstring implies configurability. To keep the API clear, either implement `widget_names`-based filtering of which fields/tabs are read, or remove the parameter if configurability isn’t required.

```suggestion
    def read(self, widget_names=None):
        """Read form values from all tabs.

        :param widget_names: Optional iterable of "tab_name.field_name" strings
            (e.g. "general.name", "credentials.user"). When provided, only the
            specified widgets will be read. When None, all known widgets are read.
        """
        result = {'general': {}, 'credentials': {}, 'additional': {}}
        fields = {
            'general': ['subscribe_to', 'name', 'target_url', 'template', 'http_method', 'enabled'],
            'credentials': ['user', 'password', 'verify_ssl', 'capsule_auth', 'certs'],
            'additional': ['content_type', 'headers'],
        }

        allowed_widgets = None
        if widget_names is not None:
            # Normalize to a set of "tab.field" strings for fast membership checks
            allowed_widgets = {name for name in widget_names}

        for tab_name, field_list in fields.items():
            if allowed_widgets is not None:
                # Filter fields for this tab based on the allowed "tab.field" names
                field_list = [
                    field_name
                    for field_name in field_list
                    if f'{tab_name}.{field_name}' in allowed_widgets
                ]
                if not field_list:
                    # No requested fields for this tab; skip switching to it
                    continue

            self._switch_to_tab(tab_name)
            for field_name in field_list:
                widget = getattr(self, field_name)
                result[tab_name][field_name] = widget.read() if widget.is_displayed else None

        return result
```
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[adamlazik1]

trigger: test-robottelo
pytest: tests/foreman/ui/test_webhook.py
theforeman:
    foreman_webhooks: 89

[LadislavVasina1]

LGTM, Thx for the changes.