PF5 migration of Bookmarks - #1938
Merged
Merged
Conversation
Contributor
Reviewer's GuideThe PR migrates the BookmarkCreateView selectors to PatternFly 5 standards by extending existing XPaths with aria-labels and data-ouia-component-id attributes, ensuring backward compatibility while adopting new PF5 conventions. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Author
|
trigger: test-robottelo |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey @lhellebr - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `airgun/views/common.py:686` </location>
<code_context>
- submit = Text(".//button[@type='submit' or @ng-click='ok()']")
- # may contain <span> inside, using normalize-space
- cancel = Text(".//button[normalize-space(.)='Cancel']")
+ error_message = Text(
+ ".//span[@class='error-message' or (ancestor::div[contains(@class, 'pf-m-error')] and contains(@class, 'item-text'))]"
+ )
+ public = Checkbox(
</code_context>
<issue_to_address>
The XPath for error_message may match more elements than intended.
This XPath may select spans that are not actual error messages if they share the 'item-text' class within error containers, potentially causing false positives.
</issue_to_address>
### Comment 2
<location> `airgun/views/common.py:689` </location>
<code_context>
+ error_message = Text(
+ ".//span[@class='error-message' or (ancestor::div[contains(@class, 'pf-m-error')] and contains(@class, 'item-text'))]"
+ )
+ public = Checkbox(
+ locator="//input[@data-ouia-component-id='isPublic-checkbox' or (@type='checkbox' and (@name='public' or @name='publik'))]"
+ )
+ submit = Text(
</code_context>
<issue_to_address>
The locator for the public checkbox is now more flexible but could match unintended elements.
This may cause ambiguous selection if multiple checkboxes meet these criteria on the page.
</issue_to_address>
### Comment 3
<location> `airgun/views/common.py:692` </location>
<code_context>
+ public = Checkbox(
+ locator="//input[@data-ouia-component-id='isPublic-checkbox' or (@type='checkbox' and (@name='public' or @name='publik'))]"
+ )
+ submit = Text(
+ ".//button[@data-ouia-component-id='submit-btn' or @type='submit' or @ng-click='ok()']"
+ )
+ cancel = Text(".//button[@data-ouia-component-id='cancel-btn' or normalize-space(.)='Cancel']")
</code_context>
<issue_to_address>
The submit button selector may match multiple buttons if present.
If multiple buttons meet these criteria, the selector may not uniquely identify the intended submit button. Consider refining the selector for uniqueness.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
submit = Text(
".//button[@data-ouia-component-id='submit-btn' or @type='submit' or @ng-click='ok()']"
)
=======
submit = Text(
# Prefer the unique data-ouia-component-id, otherwise fallback to the first visible submit button
".//button[@data-ouia-component-id='submit-btn'] | (.//button[(@type='submit' or @ng-click='ok()') and not(@data-ouia-component-id)])[1]"
)
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `airgun/views/common.py:695` </location>
<code_context>
+ submit = Text(
+ ".//button[@data-ouia-component-id='submit-btn' or @type='submit' or @ng-click='ok()']"
+ )
+ cancel = Text(".//button[@data-ouia-component-id='cancel-btn' or normalize-space(.)='Cancel']")
@property
</code_context>
<issue_to_address>
The cancel button selector could be more robust to localization.
Relying on button text may fail in localized environments. Use data-ouia-component-id or another stable attribute instead.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
cancel = Text(".//button[@data-ouia-component-id='cancel-btn' or normalize-space(.)='Cancel']")
=======
cancel = Text(".//button[@data-ouia-component-id='cancel-btn']")
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+686
to
+687
| error_message = Text( | ||
| ".//span[@class='error-message' or (ancestor::div[contains(@class, 'pf-m-error')] and contains(@class, 'item-text'))]" |
Contributor
There was a problem hiding this comment.
issue (bug_risk): The XPath for error_message may match more elements than intended.
This XPath may select spans that are not actual error messages if they share the 'item-text' class within error containers, potentially causing false positives.
Comment on lines
+689
to
+690
| public = Checkbox( | ||
| locator="//input[@data-ouia-component-id='isPublic-checkbox' or (@type='checkbox' and (@name='public' or @name='publik'))]" |
Contributor
There was a problem hiding this comment.
issue (bug_risk): The locator for the public checkbox is now more flexible but could match unintended elements.
This may cause ambiguous selection if multiple checkboxes meet these criteria on the page.
Comment on lines
+692
to
+694
| submit = Text( | ||
| ".//button[@data-ouia-component-id='submit-btn' or @type='submit' or @ng-click='ok()']" | ||
| ) |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): The submit button selector may match multiple buttons if present.
If multiple buttons meet these criteria, the selector may not uniquely identify the intended submit button. Consider refining the selector for uniqueness.
Suggested change
| submit = Text( | |
| ".//button[@data-ouia-component-id='submit-btn' or @type='submit' or @ng-click='ok()']" | |
| ) | |
| submit = Text( | |
| # Prefer the unique data-ouia-component-id, otherwise fallback to the first visible submit button | |
| ".//button[@data-ouia-component-id='submit-btn'] | (.//button[(@type='submit' or @ng-click='ok()') and not(@data-ouia-component-id)])[1]" | |
| ) |
| submit = Text( | ||
| ".//button[@data-ouia-component-id='submit-btn' or @type='submit' or @ng-click='ok()']" | ||
| ) | ||
| cancel = Text(".//button[@data-ouia-component-id='cancel-btn' or normalize-space(.)='Cancel']") |
Contributor
There was a problem hiding this comment.
suggestion: The cancel button selector could be more robust to localization.
Relying on button text may fail in localized environments. Use data-ouia-component-id or another stable attribute instead.
Suggested change
| cancel = Text(".//button[@data-ouia-component-id='cancel-btn' or normalize-space(.)='Cancel']") | |
| cancel = Text(".//button[@data-ouia-component-id='cancel-btn']") |
Satellite-QE
added
the
PRT-Failed
Contributor
Author
|
The only tests failing in PRT are the |
LadislavVasina1
approved these changes
Aug 4, 2025
Gauravtalreja1
approved these changes
Aug 14, 2025
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
4 participants
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.
Summary by Sourcery
Migrate BookmarkCreateView locators to PatternFly 5 and OUIA attributes by enhancing selectors while retaining backwards compatibility
Enhancements: