adjusting roles selection view to pf5#2472
Open
pondrejk wants to merge 1 commit into
Open
Conversation
Contributor
Author
|
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider using
contains(@class, 'pf-v5-c-dual-list-selector')instead of an exact class match inaddAnsibleRoleto make the locator more robust against additional classes being added by PatternFly. - For
CHOSEN_ITEMSandAVAILABLE_ITEMS, it may be safer to anchor the XPath to the dual list selector container (e.g., relative toaddAnsibleRole’s root locator) so changes in surrounding layout don’t inadvertently affect these locators.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider using `contains(@class, 'pf-v5-c-dual-list-selector')` instead of an exact class match in `addAnsibleRole` to make the locator more robust against additional classes being added by PatternFly.
- For `CHOSEN_ITEMS` and `AVAILABLE_ITEMS`, it may be safer to anchor the XPath to the dual list selector container (e.g., relative to `addAnsibleRole`’s root locator) so changes in surrounding layout don’t inadvertently affect these locators.
## Individual Comments
### Comment 1
<location path="airgun/views/host_new.py" line_range="1128" />
<code_context>
- hostAssignedAnsibleRoles = Text(
- './/button[@class="pf-v5-c-dual-list-selector__item"]/span[1]//span[2]'
- )
+ hostAssignedAnsibleRoles = Text('.//span[contains(@class, "dual-list-selector__item-text")]')
selectRoles = PF5Button(locator='.//button[@aria-label="Add selected"]')
unselectRoles = PF5Button(locator='.//button[@aria-label="Remove selected"]')
</code_context>
<issue_to_address>
**issue (bug_risk):** hostAssignedAnsibleRoles locator may inadvertently match both available and chosen roles
This locator now targets any element with `dual-list-selector__item-text`, which exists in both available and chosen lists. If `hostAssignedAnsibleRoles` should only reflect assigned roles, please scope it under the chosen column (e.g., under a `pf-m-chosen` container or via the existing `CHOSEN_ITEMS` anchor) so we don’t mix available and assigned roles.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| hostAssignedAnsibleRoles = Text( | ||
| './/button[@class="pf-v5-c-dual-list-selector__item"]/span[1]//span[2]' | ||
| ) | ||
| hostAssignedAnsibleRoles = Text('.//span[contains(@class, "dual-list-selector__item-text")]') |
Contributor
There was a problem hiding this comment.
issue (bug_risk): hostAssignedAnsibleRoles locator may inadvertently match both available and chosen roles
This locator now targets any element with dual-list-selector__item-text, which exists in both available and chosen lists. If hostAssignedAnsibleRoles should only reflect assigned roles, please scope it under the chosen column (e.g., under a pf-m-chosen container or via the existing CHOSEN_ITEMS anchor) so we don’t mix available and assigned roles.
Contributor
|
PRT Result |
synkd
approved these changes
Jul 7, 2026
LadislavVasina1
approved these changes
Jul 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Dual list selector is being updated to PF5 in theforeman/foreman_ansible#796 this should prevent future breakage