PF5 migration of Settings page - #1934
Conversation
Reviewer's GuideMigrates the Settings page and underlying widgets to PF5 by introducing a new editable-field widget, updating select component inheritance, and revising SettingsView imports and column widgets; also enhances navigation logic to handle nightly builds by exposing and using the product identifier. Sequence diagram for FieldWithEditBtn fill interactionsequenceDiagram
participant User as actor User
participant Widget as FieldWithEditBtn
User->>Widget: fill(item)
Widget->>Widget: editBtn.click()
alt textinput is displayed
Widget->>Widget: textinput.fill(item)
else textarea is displayed
Widget->>Widget: textarea.fill(item)
else dropdown is displayed
Widget->>Widget: dropdown.fill(item)
end
Widget->>Widget: confirmBtn.click()
Sequence diagram for ShowAllTasks navigation logic updatesequenceDiagram
participant ShowAllTasks
participant View
participant Menu
ShowAllTasks->>View: check product.is_displayed
alt Foreman is displayed
ShowAllTasks->>Menu: select('Monitor', 'Foreman Tasks', 'Tasks')
else Satellite is displayed
ShowAllTasks->>Menu: select('Monitor', 'Satellite Tasks', 'Tasks')
end
Class diagram for new FieldWithEditBtn widgetclassDiagram
class FieldWithEditBtn {
+TextInput textinput
+TextInput textarea
+FormSelect dropdown
+PF5Button editBtn
+PF5Button confirmBtn
+PF5Button cancelBtn
+Text text
+fill(item)
+read()
}
Widget <|-- FieldWithEditBtn
Class diagram for SettingsView migration to PF5classDiagram
class SettingsView {
+Text title
+Table table
}
BaseLoggedInView <|-- SettingsView
SearchableViewMixin <|-- SettingsView
Table o-- FieldWithEditBtn : column_widgets["Value"]
Class diagram for BaseMultiSelect inheritance updateclassDiagram
class BaseMultiSelect {
"PF5BaseSelect, PF5OUIADropdown"
}
PF5BaseSelect <|-- BaseMultiSelect
PF5OUIADropdown <|-- BaseMultiSelect
Class diagram for BaseLoggedInView product attribute additionclassDiagram
class BaseLoggedInView {
+Text permission_denied
+Text product
+select_logout()
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @lhellebr - I've reviewed your changes - here's some feedback:
- FieldWithEditBtn.fill checks widget.is_displayed without calling it and doesn’t handle the case where no editable widget is found; invoke widget.is_displayed() properly and add an error if none match.
- The ROOT locator for FieldWithEditBtn is hardcoded to '//td[2]' and may break if column positions change; consider targeting a more stable attribute or using the column header text.
- The product detection in ShowAllTasks.step mixes UI presence checks with business logic; extract this into a small helper or mapping to make the logic clearer and easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- FieldWithEditBtn.fill checks widget.is_displayed without calling it and doesn’t handle the case where no editable widget is found; invoke widget.is_displayed() properly and add an error if none match.
- The ROOT locator for FieldWithEditBtn is hardcoded to '//td[2]' and may break if column positions change; consider targeting a more stable attribute or using the column header text.
- The product detection in ShowAllTasks.step mixes UI presence checks with business logic; extract this into a small helper or mapping to make the logic clearer and easier to maintain.
## Individual Comments
### Comment 1
<location> `airgun/widgets.py:2704` </location>
<code_context>
+ cancelBtn = PF5Button(locator=".//button[@data-ouia-component-id='cancel-edit-btn']")
+ text = Text(locator=".//span")
+
+ def fill(self, item):
+ self.editBtn.click()
+ for widget_name in ['textinput', 'textarea', 'dropdown']:
+ widget = getattr(self, widget_name)
+ if widget.is_displayed:
+ widget.fill(item)
+ break
+ self.confirmBtn.click()
+
+ def read(self):
</code_context>
<issue_to_address>
Potential issue with widget.is_displayed being a property or method.
Please verify whether `is_displayed` is a property or a method and update the check accordingly to prevent logical errors.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def fill(self, item): | ||
| self.editBtn.click() | ||
| for widget_name in ['textinput', 'textarea', 'dropdown']: | ||
| widget = getattr(self, widget_name) | ||
| if widget.is_displayed: | ||
| widget.fill(item) | ||
| break | ||
| self.confirmBtn.click() |
There was a problem hiding this comment.
issue (bug_risk): Potential issue with widget.is_displayed being a property or method.
Please verify whether is_displayed is a property or a method and update the check accordingly to prevent logical errors.
|
trigger: test-robottelo |
Satellite-QE
added
the
PRT-Failed
| """ | ||
|
|
||
| ROOT = '//td[2]' | ||
| textinput = TextInput(locator=".//input[@data-ouia-component-type='PF5/TextInput']") |
There was a problem hiding this comment.
Does this TextInput have some data-ouia-component-id?
If so we could use from widgetastic_patternfly5.ouia import TextInput as PF5OUIATextInput
and then
text_input = PF5OUIATextInput('componentID')
also please use snake_case for all class variables if possible
There was a problem hiding this comment.
It does, but it changes. For example, there are inputs with ids setting-input-login_text, setting-input-proxy_request_timeout... and while it would be possible to use these, I think it would require greater changes than I intended to do.
There was a problem hiding this comment.
I see, in that case omit my comment
|
|
||
| ROOT = '//td[2]' | ||
| textinput = TextInput(locator=".//input[@data-ouia-component-type='PF5/TextInput']") | ||
| textarea = TextInput(locator=".//textarea") |
There was a problem hiding this comment.
same as above with the OUIA component, if applicable...
| ROOT = '//td[2]' | ||
| textinput = TextInput(locator=".//input[@data-ouia-component-type='PF5/TextInput']") | ||
| textarea = TextInput(locator=".//textarea") | ||
| dropdown = FormSelect(locator=".//select[@data-ouia-component-type='PF5/FormSelect']") |
There was a problem hiding this comment.
same as above with the OUIA component, but with formSelect if compontent-id applies
LadislavVasina1
left a comment
LadislavVasina1
left a comment
There was a problem hiding this comment.
@lhellebr Thank you for the addition, changes look good overall, just small changes requested.
Please look at the usage of OUIA components and the naming.
|
trigger: test-robottelo |
|
trigger: test-robottelo |
2 similar comments
|
trigger: test-robottelo |
|
trigger: test-robottelo |
|
trigger: test-robottelo |
|
trigger: test-robottelo |
|
trigger: test-robottelo |
|
trigger: test-robottelo |
LadislavVasina1
left a comment
LadislavVasina1
left a comment
There was a problem hiding this comment.
ACK from my side
|
Rerunning to see why
|
|
trigger: test-robottelo |
|
|
4 participants
Summary by Sourcery
Migrate the Settings page and related components to PF5 by introducing a new editable field widget, updating view mixins and multi-select, replacing legacy widgets, and adding a navbar locator, along with a navigation tweak for nightly builds
New Features:
Bug Fixes:
Enhancements: