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.

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

Fixes #36866 - Remove table hooks from katello and use foreman #10783

Merged
merged 1 commit into from
Nov 7, 2023

Conversation

parthaa
Copy link
Contributor

@parthaa parthaa commented Oct 31, 2023

What are the changes introduced in this pull request?

Now that TableHooks.js code has been ported to Foreman, we need to have Katello consume it from there so code isn't duplicated. This PR does the clean up / walden job

What are the testing steps for this pull request?

  • Check out this PR
  • Try some pages that use Table hooks.
  • Make sure they look ok

@parthaa parthaa force-pushed the byebye-table-hooks branch 2 times, most recently from 4525579 to 38cfd27 Compare October 31, 2023 23:59
@jeremylenz
Copy link
Member

@parthaa Do we know if Foreman TableHooks has diverged at all from Katello?

@chris1984
Copy link
Member

@jeremylenz do you mind reviewing the code, while I test the pages to make sure there are not any breakages?

Copy link
Member

@jeremylenz jeremylenz left a comment

Choose a reason for hiding this comment

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

Code LGTM 👍

@@ -228,7 +229,7 @@ export const ErrataTab = () => {
});

const installErrataAction = () => installErrata({
hostname, search: fetchBulkParams(),
hostname, search: fetchBulkParams({}),
Copy link
Member

Choose a reason for hiding this comment

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

The need to pass {} here is eliminated in theforeman/foreman#9878, but I suppose it's harmless either way. If that gets merged first, you could remove all of these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed!. May be I should wait for your PR before merging

@chris1984
Copy link
Member

@sjha4 said he would test the CV pages, working on testing the other stuff that was changed

Copy link
Member

@sjha4 sjha4 left a comment

Choose a reason for hiding this comment

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

Acking CV/ACS and change content source tables/URL params and useSet functionality. Code looks good..

Copy link
Member

@chris1984 chris1984 left a comment

Choose a reason for hiding this comment

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

ACK works well, all the tables/pages/modals render that I tested

@parthaa parthaa merged commit a0111d7 into Katello:master Nov 7, 2023
5 of 6 checks passed
@parthaa parthaa deleted the byebye-table-hooks branch November 7, 2023 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants