Fixes #38992 - Update host vmware form to PF5 by MariaAga · Pull Request #10817 · theforeman/foreman

MariaAga · 2026-01-14T15:03:27Z

Depends on #10811 (packing review is because of that PR)
Tests were created with help of AI.

To test:
In the host form -> select a compute resource with vmware type -> go to the virtual machine tab
a. see the new style of the alarm that says "Please select a cluster" at the bottom of the form
b. select a cluster, the Storage section is updated to PF5

Screenshots of the new form:

Copilot

Pull request overview

This PR updates the VMware host storage form components to use PatternFly 5 (PF5) components, migrating from the deprecated patternfly-react library. The changes modernize UI components including alerts, buttons, selects, form groups, and input fields.

Changes:

Migrated storage form components from patternfly-react to @patternfly/react-core
Replaced custom form components with PF5 native components and @patternfly/react-templates
Updated tests from Enzyme to React Testing Library
Added new FormStatus component for displaying loading and error states

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
webpack/assets/javascripts/react_app/components/hosts/storage/vmware/index.js	Updated Alert and Button imports, replaced AlertBody with PF5 Alert title prop, removed bsStyle prop
webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/index.js	Migrated to SimpleSelect, added Form/FormGroup structure, updated Button API, removed unused _updateDisk function
webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/disk/index.js	Complete rewrite using PF5 components (TypeaheadSelect, NumberInput, Checkbox), added state management for selections, implemented FormStatus integration
webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/disk/FormStatus.js	New component to display loading spinners and error popovers based on STATUS values
webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/disk/disk.test.js	Rewrote tests using React Testing Library instead of Enzyme
webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/controller.test.js	Rewrote tests using React Testing Library with fake timers for async operations
webpack/assets/javascripts/react_app/components/hosts/storage/vmware/tests/integration.test.js	Updated integration tests to use rtlHelpers and React Testing Library
webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/disk/disk.scss	Removed custom CSS classes replaced by PF5 components, kept minimal styling
webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/controller.scss	Deleted file - CSS no longer needed with PF5 Form components
webpack/assets/javascripts/react_app/components/hosts/storage/vmware/StorageContainer.scss	Removed unused .controller-container margin rule
package.json	Updated PatternFly packages to 5.4.x versions, added @patternfly/react-templates

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/disk/index.js

webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/index.js

...k/assets/javascripts/react_app/components/hosts/storage/vmware/controller/disk/FormStatus.js

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

webpack/assets/javascripts/react_app/components/hosts/storage/vmware/index.js

...k/assets/javascripts/react_app/components/hosts/storage/vmware/controller/disk/FormStatus.js

...k/assets/javascripts/react_app/components/hosts/storage/vmware/controller/controller.test.js

webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/disk/index.js

...ck/assets/javascripts/react_app/components/hosts/storage/vmware/controller/disk/disk.test.js

...k/assets/javascripts/react_app/components/hosts/storage/vmware/__tests__/integration.test.js

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-14T15:33:20Z

webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/disk/index.js

+            onChange={event =>
+              updateDisk('sizeGb', parseInt(event.target.value, 10) || 0)
+            }


The NumberInput onChange handler may not handle all edge cases properly. When the input is cleared or contains invalid characters, parseInt(event.target.value, 10) || 0 will default to 0, which bypasses the min={1} constraint. This could result in a value of 0 being set even though the minimum is 1. Consider validating that the parsed value respects the minimum constraint before calling updateDisk.

Copilot · 2026-01-14T15:33:20Z

webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/disk/index.js

+            onMinus={() =>
+              updateDisk('sizeGb', (parseInt(sizeGb, 10) || 0) - 1)
+            }


The onMinus handler could produce a value less than the minimum (1) when sizeGb is 1. The calculation (parseInt(sizeGb, 10) || 0) - 1 would result in 0 when starting from 1. While the NumberInput component may have its own validation, it's better to enforce the minimum constraint explicitly in the handler to prevent invalid values from being set in the application state.

...k/assets/javascripts/react_app/components/hosts/storage/vmware/controller/disk/FormStatus.js

webpack/assets/javascripts/react_app/components/hosts/storage/vmware/index.js

webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/index.js

webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/disk/index.js

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/disk/index.js

Copilot · 2026-01-15T08:00:34Z

...ck/assets/javascripts/react_app/components/hosts/storage/vmware/controller/disk/disk.test.js

+      }
+    });
+
+    it('handles null sizeGb value', () => {


The test name 'handles null sizeGb value' is misleading because the test actually uses sizeGb={0}, not null. The test should be renamed to 'handles zero sizeGb value' or the test should be updated to actually test with null/undefined.

Suggested change

it('handles null sizeGb value', () => {

it('handles zero sizeGb value', () => {

webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/disk/index.js

MariaAga · 2026-01-21T13:51:49Z

Updated buttons to only have 1 primary as before

adamruzicka

Not sure if it is caused by this, but submitting the form with fields missing leads to a page with rails traceback:

New:

Old:

adamruzicka · 2026-01-22T09:23:01Z

webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/disk/index.js

+                <Button
+                  variant="danger"
+                  className="close"
+                  onClick={removeDisk}
+                  ouiaId="btn-disk-remove"
+                  aria-label="Remove disk"
+                >


It was not immediately obvious to me what the red button would do with it being so close to the name of the disk. It was much clearer before this change

New:

Old:

I think it would be best to give it text, as I didnt see the old page like that, and with a wide screen the old page makes sense.

+1 for text

Added text to the button and moved it to the right

Will it be better to name it "remove volume" instead of "remove disk" to match "add volume"?

Yes, volumes all the way

updated, thanks

webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/index.js

webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/disk/index.js

MariaAga · 2026-01-26T12:56:55Z

@adamruzicka I checked this

Not sure if it is caused by this, but submitting the form with fields missing leads to a page with rails traceback:

And I see a page with rails traceback even without this PR, by just opening the create host page and clicking submit

adamruzicka · 2026-01-26T13:10:13Z

And I see a page with rails traceback even without this PR,

"Good". Let's not worry about it here then

adamruzicka

I don't have anything to add apart from those last two comments

webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/index.js

adamruzicka · 2026-01-26T14:50:06Z

webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/index.js

+              }
+            />
+            <Button
+              variant="secondary"


Should this be a danger variant to be in line with volume removal?

MariaAga · 2026-01-30T12:14:01Z

This PR is waiting for a UI review from @MariSvirik and a code review from @kmalyjur

kmalyjur

Looks good! I'm leaving a few comments, but they are not that important.

webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/disk/index.js

kmalyjur · 2026-01-30T18:10:58Z

webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/index.js

-          </Button>
-        </div>
+        <Form>
+          <FormGroup label={__('Create controller')}>


When having more controllers, there isn't a clear separation between them. It seems like Create controller label is a part of the previous form, but we can probably leave that for @MariSvirik to determine.

kmalyjur · 2026-01-30T18:20:16Z

webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/disk/index.js

+                  ouiaId="btn-volume-remove"
+                  aria-label="Remove volume"
+                >
+                  {__('Remove volume')}


Maybe a nitpick, but 'Remove volume' has a lowercase 'v' while 'Add Controller' has an uppercase 'C'. It could be nicer to unify them.

thanks I missed them! Updated the add/remove controller buttons to use sentence case to match.

MariaAga · 2026-02-04T11:15:37Z

Screenshot after the latest changes:

kmalyjur · 2026-02-09T12:17:16Z

I'd ACK it, but let's wait for @MariSvirik's comments.

MariaAga requested a review from a team as a code owner January 14, 2026 15:03

pr-processor bot added the Not yet reviewed label Jan 14, 2026

github-actions bot added the UI label Jan 14, 2026

MariaAga requested review from Copilot January 14, 2026 15:04

Copilot started reviewing on behalf of MariaAga January 14, 2026 15:05 View session

Copilot AI reviewed Jan 14, 2026

View reviewed changes

pr-processor bot removed the Not yet reviewed label Jan 14, 2026

Copilot AI reviewed Jan 14, 2026

View reviewed changes

MariaAga force-pushed the update-vmware branch from bbf7669 to 53c71ec Compare January 14, 2026 15:28

MariaAga requested a review from Copilot January 14, 2026 15:28

Copilot started reviewing on behalf of MariaAga January 14, 2026 15:29 View session

Copilot AI reviewed Jan 14, 2026

View reviewed changes

MariaAga force-pushed the update-vmware branch from 53c71ec to ead6fa7 Compare January 15, 2026 07:57

MariaAga requested a review from Copilot January 15, 2026 07:57

Copilot started reviewing on behalf of MariaAga January 15, 2026 07:57 View session

Copilot AI reviewed Jan 15, 2026

View reviewed changes

MariaAga force-pushed the update-vmware branch from ead6fa7 to 49fed73 Compare January 15, 2026 09:39

This was referenced Jan 15, 2026

Fixes #38998 - Deprecate pf3 select form field #10818

Merged

Refs #38822: update pf3 buttons to pf5 - hosts #10808

Draft

MariaAga force-pushed the update-vmware branch from 49fed73 to e551a1a Compare January 16, 2026 11:24

MariaAga added the breaks-robottelo label Jan 21, 2026

MariaAga force-pushed the update-vmware branch from e551a1a to 9717b3b Compare January 21, 2026 13:51

adamruzicka requested changes Jan 22, 2026

View reviewed changes

pr-processor bot added the Waiting on contributor label Jan 22, 2026

MariaAga force-pushed the update-vmware branch from 9717b3b to 9ce5018 Compare January 22, 2026 15:25

pr-processor bot added Needs re-review and removed Waiting on contributor labels Jan 22, 2026

pr-processor bot removed the Needs re-review label Jan 22, 2026

MariaAga force-pushed the update-vmware branch from 9ce5018 to 7d5fd01 Compare January 26, 2026 14:41

adamruzicka reviewed Jan 26, 2026

View reviewed changes

kmalyjur suggested changes Jan 30, 2026

View reviewed changes

pr-processor bot added the Waiting on contributor label Jan 30, 2026

Fixes #38992 - Update host vmware form to PF5

2787150

MariaAga force-pushed the update-vmware branch from 7d5fd01 to 2787150 Compare February 4, 2026 11:13

pr-processor bot added Needs re-review and removed Waiting on contributor Needs re-review labels Feb 4, 2026

	it('handles null sizeGb value', () => {
	it('handles zero sizeGb value', () => {

Conversation

MariaAga commented Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MariaAga commented Jan 21, 2026

Uh oh!

adamruzicka 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!

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!

Uh oh!

Uh oh!

MariaAga commented Jan 26, 2026

Uh oh!

adamruzicka commented Jan 26, 2026

Uh oh!

adamruzicka left a comment

MariaAga commented Jan 14, 2026 •

edited

Loading