-
Notifications
You must be signed in to change notification settings - Fork 303
Fixes #38933 - Remove legacy Content Hosts UI #11587
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
Open
jeremylenz
wants to merge
3
commits into
Katello:master
Choose a base branch
from
jeremylenz:38933-remove-legacy-ch
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
+48
−7,289
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This completes the removal of the legacy Content Hosts UI by: - Removing all legacy Content Hosts UI tests - Updating links to use new host details pages via redirects - Adding /content_hosts redirects for backward compatibility - /content_hosts -> /new/hosts (index) - /content_hosts/:id -> /new/hosts/:id (details) - /content_hosts/:id/:tab -> /new/hosts/:id with hash fragments - Using Rails URL helpers instead of hard-coded paths - Retaining bulk action modals needed by Host Collections - Updating bulk actions to use tasks.details state The content-hosts AngularJS module now only contains bulk action functionality used by Host Collections. All Content Hosts list/detail pages have been removed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- The new redirects centralize host URLs nicely, but there’s still a mix of patterns (e.g. React
SubscriptionTypeFormatterstill points to/content_hosts/:idwhile other spots use/new/hostsorhost_details_page_path); consider standardizing on the helper-based new host details URL instead of hitting the redirect. - In
katello-features.run.jstheremoteActionsarray is now empty; if it’s no longer used at all it might be cleaner to drop the variable and any related feature-flag plumbing, or otherwise keep only the entries still needed by the remaining bulk action modals.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new redirects centralize host URLs nicely, but there’s still a mix of patterns (e.g. React `SubscriptionTypeFormatter` still points to `/content_hosts/:id` while other spots use `/new/hosts` or `host_details_page_path`); consider standardizing on the helper-based new host details URL instead of hitting the redirect.
- In `katello-features.run.js` the `remoteActions` array is now empty; if it’s no longer used at all it might be cleaner to drop the variable and any related feature-flag plumbing, or otherwise keep only the entries still needed by the remaining bulk action modals.
## Individual Comments
### Comment 1
<location> `engines/bastion_katello/app/assets/javascripts/bastion_katello/bastion-katello-bootstrap.js:33-40` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
engines/bastion_katello/app/assets/javascripts/bastion_katello/bastion-katello-bootstrap.js
Show resolved
Hide resolved
bc797fe to
56d907c
Compare
56d907c to
d89a19d
Compare
- Update SubscriptionTypeFormatter to use new host URL directly instead of relying on redirect (use urlBuilder with 'new/hosts/:id') - Remove empty remoteActions array and addStates call from katello-features.run.js since no states need feature flag routing - Update Jest snapshot to match new URL format 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
d89a19d to
52e529c
Compare
Member
Author
|
/packit build |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What are the changes introduced in this pull request?
This PR completes the removal of the legacy Content Hosts UI from Katello. The changes include:
First commit - Remove legacy Content Hosts UI:
Second commit - Clean up remaining legacy Content Hosts UI:
/content_hosts→/new/hosts(index)/content_hosts/:id→/new/hosts/:id(details, supports both hostname and database ID)/content_hosts/:id/:tab→/new/hosts/:id#/Content/:tab(with hash fragments for tabs)tasks.detailsstate instead of removedcontent-hosts.bulk-taskhost_details_page_path) instead of hard-coded pathsKey architectural decision:
The
content-hostsAngularJS module was retained but reduced to contain only:ContentHostsModalHelper,ContentHostsHelper,ContentHostStatusController) required by Host Collections, Errata, and Activation Keys pagesConsiderations taken when implementing this change?
/content_hostsURLs continue to work, with support for the friendly-id gem (both hostname and database ID work in URLs)#/Content/errata) to preserve deep-linking functionalityWhat are the testing steps for this pull request?
Manual testing completed:
/content_hostsURLs to new host details pages/content_hosts/123→/new/hosts/123/content_hosts/host.example.com→/new/hosts/host.example.com/content_hosts/:id/errata→/new/hosts/:id#/Content/errata/errata/:id/content-hosts)Automated testing:
To test:
/host_collections/1and verify all bulk action buttons work (Package Installation, Errata Installation, etc.)/content_hosts/123or/content_hosts/hostname.example.comand verify they redirect correctly/errata/:id/content-hostspage and verify content hosts list displaysSummary by Sourcery
Remove the legacy Content Hosts UI in favor of the new host details pages while preserving required bulk action functionality and URL compatibility.
New Features:
Bug Fixes:
Enhancements:
Tests: