Skip to content

chore: refactor advisory systems table#1175

Merged
mkholjuraev merged 3 commits into
RedHatInsights:masterfrom
mkholjuraev:refactor-advisor-systems
Apr 17, 2024
Merged

chore: refactor advisory systems table#1175
mkholjuraev merged 3 commits into
RedHatInsights:masterfrom
mkholjuraev:refactor-advisor-systems

Conversation

@mkholjuraev

Copy link
Copy Markdown
Contributor

Description

Associated Jira ticket: # (issue)

This PR is intended to refactor the advisory systems table to make the code cleaner. The refactor only divides existing code to 2 components AdvisorySystems and AdvisorySystemsTable. AdvisorySystems is a wrapper around the table and the remediation wizard. AdvisorySystemsTable is solely for the table functionality.

It also has the tests rewritten

How to test the PR

Please include steps to test your PR.

Before the change

After the change

Dependent work link

Checklist:

  • The commit message has the Jira ticket linked
  • PR has a short description
  • Screenshots before and after the change are added
  • Tests for the changes have been added
  • README.md is updated if necessary
  • Needs additional dependent work

@mkholjuraev mkholjuraev requested a review from a team as a code owner March 14, 2024 15:57
@Fewwy

Fewwy commented Mar 14, 2024

Copy link
Copy Markdown
Contributor

It looks good, I ran the tests and they are passing. Do we need to update anything in them?

@mkholjuraev

Copy link
Copy Markdown
Contributor Author

Nope, the PR is ready to be reviewed and merged if everything is fine

@mkholjuraev

Copy link
Copy Markdown
Contributor Author

You were right, somehow I did not push the commit for tests rewrite

@mkholjuraev mkholjuraev force-pushed the refactor-advisor-systems branch from 0d6f87d to 07d32bf Compare April 3, 2024 09:05
@mkholjuraev

Copy link
Copy Markdown
Contributor Author

@Fewwy I completely forgot about this PR. Would you mind having another look?

@gkarat gkarat left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey, everything works well. Though, I have noticed an increase in network requests number (mostly, /system_profile/... and /tags requests). Can you please take a look and tell if it is expected? If not, can we make sure the performance doesn't downgrade after the PR changes?

Before:

Screenshot 2024-04-15 at 13 46 19

Screenshot 2024-04-15 at 13 46 31

After:

Screenshot 2024-04-15 at 13 46 29

Screenshot 2024-04-15 at 13 46 15

@gkarat gkarat left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No longer reproducible ^, I think it was not caused by your PR. Let's have it merged, thanks @mkholjuraev

@mkholjuraev mkholjuraev merged commit 42e843c into RedHatInsights:master Apr 17, 2024
@mkholjuraev

Copy link
Copy Markdown
Contributor Author

🎉 This PR is included in version 1.67.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants