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

fixed in method for discovery rule and updated it's entities #1480

Merged
merged 1 commit into from
Aug 26, 2024
Merged

fixed in method for discovery rule and updated it's entities #1480

merged 1 commit into from
Aug 26, 2024

Conversation

amolpati30
Copy link
Contributor

@amolpati30 amolpati30 commented Jul 26, 2024 •
edited
Loading

  • Updated the views with Table() widget instead SatTable(), to read all the table values.

  • Added entities to read the page after deleting discovery rules table.

Dependent PR: SatelliteQE/robottelo#15783

@amolpati30 amolpati30 added No-CherryPick PR doesnt need CherryPick to previous branches Stream labels Jul 26, 2024
@amolpati30 amolpati30 mentioned this pull request Jul 26, 2024
Merged
@Satellite-QE
Copy link
Contributor

PRT Result

Build Number: 332
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/ui/test_discoveryrule.py -k test_positive_crud_with_non_admin_user --external-logging
Test Result : =========== 1 failed, 3 deselected, 16 warnings in 821.40s (0:13:41) ===========

@Satellite-QE Satellite-QE added the PRT-Failed Indicates that latest PRT run is failed for the PR label Jul 26, 2024
@amolpati30
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_discoveryrule.py -k test_positive_crud_with_non_admin_user
robottelo: 15783

@Satellite-QE
Copy link
Contributor

PRT Result

Build Number: 333
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/ui/test_discoveryrule.py -k test_positive_crud_with_non_admin_user --external-logging
Test Result : =========== 1 passed, 3 deselected, 16 warnings in 856.44s (0:14:16) ===========

@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 Jul 26, 2024
@shweta83
Copy link
Contributor

shweta83 commented Aug 2, 2024

The description of the PR differs from the code change here. Can you please either update the description or make the code changes to match it.

@amolpati30 amolpati30 force-pushed the discoveryrulefix_method branch 2 times, most recently from 9104abd to 314a17d Compare August 8, 2024 20:35
@amolpati30
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_discoveryrule.py -k 'crud_with_non_admin_user or end_to_end'
robottelo: 15783

@Satellite-QE
Copy link
Contributor

PRT Result

Build Number: 342
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/ui/test_discoveryrule.py -k crud_with_non_admin_user or end_to_end --external-logging
Test Result : =========== 2 passed, 2 deselected, 65 warnings in 966.25s (0:16:06) ===========

shweta83
shweta83 reviewed Aug 9, 2024
View reviewed changes
airgun/entities/discoveryrule.py Outdated
:return: Read the page and provide the values in a dict,
"""
view = self.navigate_to(self, 'All')
return view.page_info.read()
Copy link
Contributor

@shweta83 shweta83 Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I am in more favor to modify def read_all() to return view.read() . It would read entire page in that case and not just the table.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried using that option, but the issue is that even when we remove the table object from read_all(), it still attempts to read all the values present in DiscoveryRulesView(), such as title and table. Since the table has been deleted, it throws an error indicating that the table element is not present. Instead of removing it, I left it as is, so this doesn’t impact 2 out of the 4 test cases. For the cases where the rule is being deleted, I created a new read_after_del() method, which reads only the page_info object instead of the entire object.

Copy link
Contributor Author

@amolpati30 amolpati30 Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Rather than creating a new method, I simply updated the existing read_all() method and removed the read_after_del() method that I had previously added

@amolpati30 amolpati30 added CherryPick PR needs CherryPick to previous branches AutoMerge_Cherry_Picked Automatically merge the PR is PRT and all checks are passing 6.15.z and removed No-CherryPick PR doesnt need CherryPick to previous branches Stream labels Aug 9, 2024
@amolpati30
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_discoveryrule.py -k 'crud_with_non_admin_user or end_to_end'
robottelo: 15783

@Satellite-QE
Copy link
Contributor

PRT Result

Build Number: 365
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/ui/test_discoveryrule.py -k crud_with_non_admin_user or end_to_end --external-logging
Test Result : =========== 2 passed, 2 deselected, 53 warnings in 911.32s (0:15:11) ===========

shweta83
shweta83 requested changes Aug 25, 2024
View reviewed changes
airgun/entities/discoveryrule.py Outdated
@@ -50,13 +50,16 @@ def read(self, entity_name, widget_names=None):
return view.read(widget_names=widget_names)

def read_all(self):
"""Reads the whole discovery rules table.
"""Reads the entire discovery rules table or page after a rule is deleted
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
"""Reads the entire discovery rules table or page after a rule is deleted
"""Reads the entire discovery rules page

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated the comment.

airgun/entities/discoveryrule.py Outdated
@@ -50,13 +50,16 @@ def read(self, entity_name, widget_names=None):
return view.read(widget_names=widget_names)

def read_all(self):
"""Reads the whole discovery rules table.
"""Reads the entire discovery rules table or page after a rule is deleted

:return: list of table rows, each row is dict,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please update what it returns?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. Also, please update :return: according to new implementation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changes have been made.

@amolpati30 amolpati30 force-pushed the discoveryrulefix_method branch 2 times, most recently from 210a971 to d5f43e0 Compare August 26, 2024 06:23
shweta83
shweta83 approved these changes Aug 26, 2024
View reviewed changes
Copy link
Contributor

@shweta83 shweta83 left a comment

Choose a reason for hiding this comment

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

Ack

@shweta83
Copy link
Contributor

trigger: test-robottelo
pytest: tests/foreman/ui/test_discoveryrule.py -k 'crud_with_non_admin_user or end_to_end'
robottelo: 15783

@Satellite-QE
Copy link
Contributor

PRT Result

Build Number: 371
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/ui/test_discoveryrule.py -k crud_with_non_admin_user or end_to_end --external-logging
Test Result : =========== 2 passed, 2 deselected, 48 warnings in 807.88s (0:13:27) ===========

@shweta83 shweta83 merged commit e7ec818 into SatelliteQE:master Aug 26, 2024
7 of 8 checks passed
github-actions bot pushed a commit that referenced this pull request Aug 26, 2024
@amolpati30 @web-flow
fixed in method for discovery rule and updated it's entities (#1480)
26ceef5 
(cherry picked from commit e7ec818)
@Satellite-QE Satellite-QE mentioned this pull request Aug 26, 2024
Merged
github-actions bot pushed a commit that referenced this pull request Aug 26, 2024
@amolpati30 @web-flow
fixed in method for discovery rule and updated it's entities (#1480)
55f2955 
(cherry picked from commit e7ec818)
@Satellite-QE Satellite-QE mentioned this pull request Aug 26, 2024
Merged
Satellite-QE added a commit that referenced this pull request Aug 26, 2024
@Satellite-QE
fixed in method for discovery rule and updated it's entities (#1480) (#…
295dbad 
…1530)
This was referenced Aug 29, 2024
Merged
Merged
Gauravtalreja1 pushed a commit that referenced this pull request Sep 27, 2024
@Satellite-QE @amolpati30
fixed in method for discovery rule and updated it's entities (#1480) (#…
e6c1ffe 
…1529)

(cherry picked from commit e7ec818)

Co-authored-by: amolpati30 <151733635+amolpati30@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LadislavVasina1 LadislavVasina1 LadislavVasina1 approved these changes

@shweta83 shweta83 shweta83 approved these changes

@omkarkhatavkar omkarkhatavkar omkarkhatavkar approved these changes

Assignees

@amolpati30 amolpati30

Labels
6.15.z 6.16.z AutoMerge_Cherry_Picked Automatically merge the PR is PRT and all checks are passing CherryPick PR needs CherryPick to previous branches PRT-Passed Indicates that latest PRT run is passed for the PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@amolpati30 @Satellite-QE @shweta83 @omkarkhatavkar @LadislavVasina1