Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

subscription page search field ui selector update #1702

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

subscription page search field ui selector update #1702

wants to merge 3 commits into from
+3 −3

Conversation

vijaysawant
Copy link
Contributor

Problem Statement

  • Test case test_positive_access_with_non_admin_user_with_manifest was failing due to Error
    TypeError: 'NoneType' object is not subscriptable
  • Class variable search_field from airgun view SubscriptionSearch need to be updated

Solution

Update search_field with expected locator.

Robottelo PR

SatelliteQE/robottelo#17382

@vijaysawant vijaysawant added CherryPick PR needs CherryPick to previous branches Stream 6.16.z labels Jan 20, 2025
@vijaysawant vijaysawant mentioned this pull request Jan 20, 2025
Open
@vijaysawant
Copy link
Contributor Author 

trigger: test-robottelo
pytest: tests/foreman/ui/test_subscription.py -k 'test_positive_access_with_non_admin_user_with_manifest'
robottelo: 17382

@vijaysawant vijaysawant mentioned this pull request Jan 20, 2025
Closed
sambible
sambible approved these changes Jan 20, 2025
View reviewed changes
Copy link
Contributor

@sambible sambible left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Could potentially use 'aria-label' as a (possibly) more durable locator, but I don't feel strongly about this.

sambible
sambible requested changes Jan 20, 2025
View reviewed changes
Copy link
Contributor

@sambible sambible left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon further review, the actual solution to this problem is to change from using "SearchableViewMixin" and having the custom search logic to simply using PF4SearchableViewMixin.

This will also make test changes not necessary, at least in the case of one of the tests you are changing.

@vijaysawant
Copy link
Contributor Author 

trigger: test-robottelo
pytest: tests/foreman/ui/test_subscription.py -k 'test_positive_access_with_non_admin_user_with_manifest'

@Satellite-QE
Copy link
Contributor

PRT Result

Build Number: 477
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/ui/test_subscription.py -k test_positive_access_with_non_admin_user_with_manifest --external-logging
Test Result : ========== 1 passed, 7 deselected, 16 warnings in 3652.52s (1:00:52) ===========

@Satellite-QE Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label Jan 20, 2025
@vijaysawant vijaysawant requested a review from sambible January 21, 2025 06:14
@vijaysawant
Copy link
Contributor Author

vijaysawant commented Jan 21, 2025 •
edited
Loading

Local test execution result :

============================= test session starts ==============================
collecting ... 2025-01-21 13:34:32 - robottelo.collection - INFO - Processing test items to add testimony token markers
collected 8 items / 7 deselected / 1 selected

tests/foreman/ui/test_subscription.py::test_positive_access_with_non_admin_user_with_manifest 

=========== 1 passed, 7 deselected, 3 warnings in 124.06s (0:02:04) ============
PASSED
Process finished with exit code 0

shweta83
shweta83 reviewed Jan 22, 2025
View reviewed changes
airgun/views/subscription.py
@@ -115,7 +115,7 @@ def fill(self, values):
self.close()


class SubscriptionListView(BaseLoggedInView, SubscriptionSearchableViewMixin):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we change SubscriptionSearchableViewMixin to SearchableViewMixinPF4 ?
Since you have already updated SubscriptionSearchableViewMixin above we can keep this as it is. wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, class SubscriptionSearchableViewMixin alread imports SearchableViewMixinPF4 in class definition.
Testing changes locally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running PRT comment on robottelo PR SatelliteQE/robottelo#17382 to test changes in CI.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes are not reflected here. Can you please update it.

@vijaysawant vijaysawant force-pushed the subscription-page-search-box-fix branch from 3d579b2 to 719fb6b Compare January 22, 2025 06:58
@shweta83
Copy link
Contributor

trigger: test-robottelo
pytest: tests/foreman/ui/test_subscription.py -k 'test_positive_access_with_non_admin_user_with_manifest'

@Satellite-QE
Copy link
Contributor

PRT Result

Build Number: 478
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/ui/test_subscription.py -k test_positive_access_with_non_admin_user_with_manifest --external-logging
Test Result : =========== 1 failed, 7 deselected, 16 warnings in 635.30s (0:10:35) ===========

@Satellite-QE Satellite-QE added PRT-Failed Indicates that latest PRT run is failed for the PR and removed PRT-Passed Indicates that latest PRT run is passed for the PR labels Jan 22, 2025
@vijaysawant
Copy link
Contributor Author 

trigger: test-robottelo
pytest: tests/foreman/ui/test_subscription.py -k 'test_positive_access_with_non_admin_user_with_manifest'
robottelo: 17382

@vijaysawant
Copy link
Contributor Author 

trigger: test-robottelo
pytest: tests/foreman/ui/test_subscription.py -k 'test_positive_access_with_non_admin_user_with_manifest'

@Satellite-QE
Copy link
Contributor

PRT Result

Build Number: 482
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/ui/test_subscription.py -k test_positive_access_with_non_admin_user_with_manifest --external-logging
Test Result : =========== 1 passed, 7 deselected, 16 warnings in 643.41s (0:10:43) ===========

@Satellite-QE Satellite-QE added PRT-Passed Indicates that latest PRT run is passed for the PR and removed PRT-Failed Indicates that latest PRT run is failed for the PR labels Jan 22, 2025
@Satellite-QE
Copy link
Contributor

PRT Result

Build Number: 9952
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/ui/test_subscription.py -k test_positive_access_with_non_admin_user_with_manifest --external-logging
Test Result: =========== 1 passed, 7 deselected, 16 warnings in 670.81s (0:11:10) ===========

shweta83
shweta83 reviewed Jan 22, 2025
View reviewed changes
airgun/views/subscription.py
@@ -115,7 +115,7 @@ def fill(self, values):
self.close()


class SubscriptionListView(BaseLoggedInView, SubscriptionSearchableViewMixin):
class SubscriptionListView(BaseLoggedInView, SearchableViewMixinPF4):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
class SubscriptionListView(BaseLoggedInView, SearchableViewMixinPF4):
class SubscriptionListView(BaseLoggedInView, SubscriptionSearchableViewMixin):

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggested changes didn't work locally as well as in CI, that's why I revert back to previous changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shweta83 shweta83 shweta83 left review comments

@synkd synkd synkd approved these changes

@chris1984 chris1984 chris1984 approved these changes

@ColeHiggins2 ColeHiggins2 Awaiting requested review from ColeHiggins2

@vsedmik vsedmik Awaiting requested review from vsedmik

@LadislavVasina1 LadislavVasina1 Awaiting requested review from LadislavVasina1

@damoore044 damoore044 Awaiting requested review from damoore044

@sambible sambible Awaiting requested review from sambible

Requested changes must be addressed to merge this pull request.

Assignees

@vijaysawant vijaysawant

Labels
6.16.z CherryPick PR needs CherryPick to previous branches PRT-Passed Indicates that latest PRT run is passed for the PR Stream
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vijaysawant @Satellite-QE @shweta83 @chris1984 @sambible @synkd