Fixes #38934 - Add persistence column to host packages UI #11589

qcjames53 · 2025-12-05T22:26:18Z

What are the changes introduced in this pull request?

The host details content tab's packages chart now shows the persistence info for packages installed on a bootc host. This column is sortable and is not displayed unless the host is a bootc host. Changes were based on this design.

Considerations taken when implementing this change?

The Figma shows the 'Status' column as sortable. I did not make this change.
The Figma shows a tooltip question mark icon next to the 'Persistence' header. I did not know what to include under this tooltip so it was not included.
Do we need to capitalize 'Persistent' and 'Transient' like in the figma? I'll happily remove if not.

What are the testing steps for this pull request?

Set up a regular host and a bootc host, syncing package info. You can mock some persistence info by following the steps in this PR, if you choose.
Go to the host in the web UI and open the regular host's content -> packages tab. It will look normal.
Open the bootc host's content -> packages tab and check out the new column.
Validate my unit tests. I'll remove the test for capitalization if we drop that design choice.

Summary by Sourcery

Add persistence information to the host packages tab for bootc hosts and adjust table behavior accordingly.

New Features:

Display a Persistence column in the host packages table when viewing bootc hosts, including capitalized persistence values and a dash for missing data.
Enable sorting of the packages table by persistence for bootc hosts.

Tests:

Add unit tests verifying visibility of the Persistence column and values for bootc versus non-bootc hosts, and capitalization/placeholder behavior of persistence values.

sourcery-ai

Hey there - I've reviewed your changes - here's some feedback:

You destructure persistence from the package object (const { ... persistence: persistence, ... }) but then use pkg.persistence directly; either use the destructured variable or drop it to avoid confusion.
The persistence cell formatting (pkg.persistence ? (pkg.persistence.charAt(0).toUpperCase() + pkg.persistence.slice(1)) : '—') would be clearer and easier to reuse/test if extracted into a small helper (e.g., formatPersistence(pkg.persistence)), especially since you’re also asserting this capitalization and placeholder behavior in tests.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- You destructure `persistence` from the package object (`const { ... persistence: persistence, ... }`) but then use `pkg.persistence` directly; either use the destructured variable or drop it to avoid confusion.
- The persistence cell formatting (`pkg.persistence ? (pkg.persistence.charAt(0).toUpperCase() + pkg.persistence.slice(1)) : '—'`) would be clearer and easier to reuse/test if extracted into a small helper (e.g., `formatPersistence(pkg.persistence)`), especially since you’re also asserting this capitalization and placeholder behavior in tests.

## Individual Comments

### Comment 1
<location> `webpack/components/extensions/HostDetails/Tabs/__tests__/packagesTab.test.js:574` </location>
<code_context>
+  act(done);
+});
+
+test('Capitalizes persistence values and shows dash for null', async (done) => {
+  const autocompleteScope = mockForemanAutocomplete(nockInstance, autocompleteUrl);
+  const bootcFacetAttributes = {
</code_context>

<issue_to_address>
**issue (testing):** Explicitly assert that a null persistence value renders as a dash

This test is meant to cover both capitalization and the dash placeholder for `null`, but it never actually asserts the dash is rendered. If the fixtures only contain `persistent` and `transient`, the test will still pass without exercising the `null` → `"—"` path.

Please ensure the fixtures include at least one `persistence: null` entry and add a direct assertion for the dash (e.g. `expect(getByText('—')).toBeInTheDocument();` or an equivalent targeted query), so the placeholder behavior is explicitly verified.
</issue_to_address>

### Comment 2
<location> `webpack/components/extensions/HostDetails/Tabs/__tests__/packagesTab.test.js:555` </location>
<code_context>
+  act(done);
+});
+
+test('Does not show persistence column for non-bootc hosts', async (done) => {
+  const autocompleteScope = mockForemanAutocomplete(nockInstance, autocompleteUrl);
+  const scope = nockInstance
</code_context>

<issue_to_address>
**suggestion (testing):** Add tests for sorting behavior of the new Persistence column on bootc hosts

The new column is now sortable for bootc hosts, but none of the added tests verify its sort behavior.

Please add a test that renders `PackagesTab` for a bootc host, clicks the `Persistence` header, and asserts that the correct sort parameter (e.g. `persistence`) is used (in the request, state, or URL, consistent with existing sort tests). You can likely mirror the existing sort tests for columns like `Status` or `Installed version`.

Suggested implementation:

```javascript
  const { getAllByText, getByText } = renderWithRedux(
    <PackagesTab />,
    renderOptions(bootcFacetAttributes),
  );

  await patientlyWaitFor(() => expect(getAllByText(firstPackage.name)[0]).toBeInTheDocument());
});

test('Sorts by Persistence for bootc hosts', async done => {
  const autocompleteScope = mockForemanAutocomplete(nockInstance, autocompleteUrl);

  const persistenceSortedQuery = Object.assign({}, defaultQuery, {
    // NOTE: Adjust this key/value to match how sorting is specified in existing sort tests,
    // e.g. `order: 'persistence ASC'` or `sort_by: 'persistence'`.
    sort_by: 'persistence',
  });

  const scope = nockInstance
    .get(hostPackages)
    .query(persistenceSortedQuery)
    .reply(200, mockPackagesData);

  const { getByText } = renderWithRedux(
    <PackagesTab />,
    renderOptions(bootcFacetAttributes),
  );

  // Click the "Persistence" column header to trigger sorting
  fireEvent.click(getByText('Persistence'));

  // Wait for the sorted request to be made
  await patientlyWaitFor(() => {
    expect(scope.isDone()).toBe(true);
  });

  assertNockRequest(autocompleteScope);
  assertNockRequest(scope);
  act(done);

```

1. Ensure `fireEvent` is imported from your testing library (e.g. `import { fireEvent } from '@testing-library/react';`) if it is not already imported at the top of this file.
2. Update the `persistenceSortedQuery` construction to match the exact sort parameter convention used in your existing sort tests (for columns like `Status` or `Installed version`). For example, you might need:
   - `order: 'persistence ASC'`, or
   - `order_by: 'persistence'`, or
   - `sort: 'persistence'`.
3. If existing sort tests verify sort behavior via URL state or Redux actions instead of Nock query parameters, adapt the expectation accordingly (e.g. asserting on `history.location.search` or dispatched actions) while keeping the same overall test structure.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

webpack/components/extensions/HostDetails/Tabs/__tests__/packagesTab.test.js

ianballou

Looking good so far!

qcjames53 · 2025-12-08T21:57:22Z

Thanks for the review, Ian. Unit tests are again good to go. I'm going to see if we get the same automation errors before recording everything.

ianballou · 2025-12-08T22:38:03Z

webpack/components/extensions/HostDetails/Tabs/PackagesTab/PackagesTab.js

+      __('Upgradable to'),
+    ];
+
+  const COLUMNS_TO_SORT_PARAMS = isBootcHost ?


This transience feature will only work for newer machines when it does come out. What do you think about adding an API boolean that signals if any packages have persistence values set and disable the column if it is false?

For some users, they may see this column as permanently blank if, for whatever reason, they are stuck back on a certain version. Question is - how long would an admin possible see these blanks? RHEL at least supports y-streams for a few years: https://access.redhat.com/support/policy/updates/errata

The UI-only alternative would be to disable the column on a per-pagination basis, but seeing the column appear and disappear as you flip pages might end up being a strange UX experience.

I think that sounds like a cleaner solution on the UI side but also I could see the argument that an admin would want to know they aren't getting package persistence info for a bootc host. If the column was missing I'm not sure it would be as clear.

I could add something to the page telling the user that no package persistence data was reported? Maybe a second info box below the "Package changes not persistent" one?

That could work:

Maybe something below that says, along the lines of, "Persistence column hidden because subscription-manager reported no persistence data."

@MariSvirik I think we need some help here - package persistence data is likely to be blank for host packages for a while until subscription-manager persistence data becomes more widely available.

It would be nice to both hide the column and tell admins why the column is hidden. What do you think?

For reference, the Persistence values otherwise would all be just dashes "-" (unless you have a comment too about what missing persistence should look like for a single package)

To update this: My current implementation hides the column unless the host contains one or more installed packages reporting persistence info. I've added .contains_package_with_reported_persistence to the other properties reported in the /hosts/:id endpoint.

webpack/components/extensions/HostDetails/Tabs/__tests__/packagesTab.test.js

qcjames53 · 2025-12-10T21:39:17Z

I added an API endpoint for determining if the host contains 1+ installed packages which report persistence data. The Web UI will now refresh hostDetails properly on completion of a package profile sync, meaning React can properly redraw the table if the persistence column needs to be added/removed.

This new endpoint is on a per-host basis so you can still see a table of all blanks if there's a package on for example page 4 returning persistence info.

qcjames53 · 2025-12-10T21:41:31Z

app/models/katello/concerns/host_managed_extensions.rb

      end

+      def contains_package_with_reported_persistence?
+        host_installed_packages.where.not(persistence: nil).exists?


Claude assures me that .exists? is fast enough and a better solution than storing a new column which pre-computes whether an installed package has persistence. What do you think?

From reading online exists? seems to use a limit of 1 in the generated SQL, so it should be performant.

webpack/components/extensions/HostDetails/Tabs/PackagesTab/PackagesTab.js

ianballou

Working as I expect now! I think we should still get some UX design validation about the disappearing column if possible.

Code comments below:

ianballou · 2025-12-11T20:25:09Z

app/models/katello/concerns/host_managed_extensions.rb

      end

+      def contains_package_with_reported_persistence?
+        host_installed_packages.where.not(persistence: nil).exists?


From reading online exists? seems to use a limit of 1 in the generated SQL, so it should be performant.

ianballou · 2025-12-11T21:39:17Z

app/controllers/katello/api/v2/host_packages_controller.rb

+        options[:custom_sort] = lambda do |query|
+          query.joins(:host_installed_packages)
+            .where(katello_host_installed_packages: {host_id: @host.id})
+            .order("katello_host_installed_packages.persistence #{params[:sort_order] || 'asc'}")


The param here is being sent from the user directly into the DB, so it's a possible vector for an injection attack. Could we limit it to asc or desc? I suppose we must already have some paradigm for how the sort order gets sent into the database query.

Good catch, thanks! How does the current implementation look?

Looks fine to me, thanks.

webpack/components/extensions/HostDetails/Tabs/PackagesTab/PackagesTab.js

ianballou · 2025-12-11T21:45:21Z

webpack/components/extensions/HostDetails/Tabs/PackagesTab/PackagesTab.js

    <div>
      <div id="packages-tab">
-        {hostIsImageMode({ hostDetails }) && <ImageModeHostAlert />}
+        {isBootcHost && <ImageModeHostAlert />}


What is this change for?

I was using isBootcHost all over the place during development. This is the last place it's used. I'll roll back this change.

qcjames53 · 2025-12-12T16:47:59Z

@ianballou Re: refreshHostDetails (my comment is not showing up on main PR page) https://github.com/Katello/katello/pull/11589/changes#r2614903175

ianballou

I noticed that the hiding of the column after package persistence data goes away requires a hard refresh of the page. The same goes for the column appearing once the data is available. Perhaps not a blocker, especially if it's technically complex to fix for whatever reason.

Aside from that this is working well, thanks. Before acking I'm just curious about reactions to the above.

webpack/components/extensions/HostDetails/Tabs/PackagesTab/PackagesTab.js

ianballou · 2025-12-12T18:14:05Z

app/controllers/katello/api/v2/host_packages_controller.rb

+        options[:custom_sort] = lambda do |query|
+          query.joins(:host_installed_packages)
+            .where(katello_host_installed_packages: {host_id: @host.id})
+            .order("katello_host_installed_packages.persistence #{params[:sort_order] || 'asc'}")


Looks fine to me, thanks.

qcjames53 · 2025-12-12T19:44:25Z

I noticed that the hiding of the column after package persistence data goes away requires a hard refresh of the page. The same goes for the column appearing once the data is available. Perhaps not a blocker, especially if it's technically complex to fix for whatever reason.

Aside from that this is working well, thanks. Before acking I'm just curious about reactions to the above.

Would you mind reloading the page with cleared cache? This should be working as of yesterday I believe.

ianballou · 2025-12-12T20:15:02Z

I noticed that the hiding of the column after package persistence data goes away requires a hard refresh of the page. The same goes for the column appearing once the data is available. Perhaps not a blocker, especially if it's technically complex to fix for whatever reason.

We had some chats about this outside the PR, and the decision is to always show the column with dashes. The added complexity to the UI to make the column nicely hide-able was determined to be too high to be worth it.

Plus, at least with image mode, I expect more users will use up to date versions of RHEL and so on because it's such a new technology. On top of that, it's easy to upgrade: just bootc switch. So, perhaps more users will have newer RHEL after all.

qcjames53 · 2025-12-12T20:18:58Z

Thank you for the great discussion! This should be all cleaned up and ready to go. I kept the contains_package_with_reported_persistence boolean in host details for our future feature work since it's already done and tested.

ianballou

Looking good, thanks!

qcjames53 · 2025-12-12T20:27:21Z

I have to fix one of the npx tests. Thank you for the approval!

qcjames53 · 2025-12-12T20:42:34Z

All righty. I significantly cleaned up the unit tests.

pr-processor bot added the Not yet reviewed label Dec 5, 2025

sourcery-ai bot reviewed Dec 5, 2025

View reviewed changes

webpack/components/extensions/HostDetails/Tabs/__tests__/packagesTab.test.js Outdated Show resolved Hide resolved

webpack/components/extensions/HostDetails/Tabs/__tests__/packagesTab.test.js Show resolved Hide resolved

pr-processor bot removed the Not yet reviewed label Dec 5, 2025

ianballou reviewed Dec 8, 2025

View reviewed changes

qcjames53 force-pushed the 38934 branch from 22d27b9 to 15746da Compare December 8, 2025 21:56

qcjames53 force-pushed the 38934 branch from 15746da to b13c5f6 Compare December 8, 2025 22:08

ianballou reviewed Dec 8, 2025

View reviewed changes

qcjames53 force-pushed the 38934 branch 2 times, most recently from b9638b7 to ef78d07 Compare December 10, 2025 21:35

qcjames53 commented Dec 10, 2025

View reviewed changes

webpack/components/extensions/HostDetails/Tabs/PackagesTab/PackagesTab.js Show resolved Hide resolved

qcjames53 force-pushed the 38934 branch from ef78d07 to 2e75496 Compare December 10, 2025 21:58

ianballou reviewed Dec 11, 2025

View reviewed changes

qcjames53 force-pushed the 38934 branch from 2e75496 to d6bb2d8 Compare December 12, 2025 16:45

ianballou reviewed Dec 12, 2025

View reviewed changes

qcjames53 force-pushed the 38934 branch from d6bb2d8 to 4bedc1c Compare December 12, 2025 20:13

ianballou approved these changes Dec 12, 2025

View reviewed changes

Fixes #38934 - Add persistence column to host packages UI

408fa0c

qcjames53 force-pushed the 38934 branch from 4bedc1c to 408fa0c Compare December 12, 2025 20:42

qcjames53 merged commit 01a7b2d into Katello:master Dec 12, 2025
22 checks passed

qcjames53 deleted the 38934 branch December 12, 2025 21:16

Fixes #38934 - Add persistence column to host packages UI #11589

Fixes #38934 - Add persistence column to host packages UI #11589

Uh oh!

Conversation

qcjames53 commented Dec 5, 2025 • edited by sourcery-ai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What are the changes introduced in this pull request?

Considerations taken when implementing this change?

What are the testing steps for this pull request?

Summary by Sourcery

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ianballou left a comment

Choose a reason for hiding this comment

Uh oh!

qcjames53 commented Dec 8, 2025

Uh oh!

ianballou Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qcjames53 Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

qcjames53 commented Dec 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ianballou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qcjames53 commented Dec 12, 2025

Uh oh!

ianballou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qcjames53 commented Dec 12, 2025

Uh oh!

ianballou commented Dec 12, 2025

Uh oh!

qcjames53 commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

qcjames53 commented Dec 5, 2025 •

edited by sourcery-ai bot

Loading

ianballou Dec 8, 2025 •

edited

Loading

qcjames53 Dec 10, 2025 •

edited

Loading

qcjames53 commented Dec 12, 2025 •

edited

Loading