Fixes #38842 - Replace FormControl with TextInput for InputFactory #10729

ofedoren · 2025-10-17T15:22:06Z

InputFactory seems to be used by FormField (which is not commonly used and probably will have adjustments or removal) and by Tags::ReactInput via react_form_input.

react_form_input seems to cover any usages by either passing registered component name (such as autocomplete) or using a component directly instead of using InputFactory (such as textarea).

Given that I think we don't even need a fallback component, which previously was a simple input wrapped with FormControl. But to replace it, I went with TextInput since it's quite general component, even though not as flexible as input with type={type}.

I've tried to make this backwards compatible, but please verify if it's enough. Also, tried to replace snap tests with normal ones, but not sure if they are enough as well.

Copilot

Pull Request Overview

This PR replaces the legacy FormControl component from patternfly-react with the modern TextInput component from @patternfly/react-core in the InputFactory component. The change modernizes the component while maintaining backwards compatibility for custom registered input components.

Key Changes:

Replaced FormControl with TextInput as the default fallback component
Updated prop mapping to use PatternFly 4 conventions (isDisabled, isRequired)
Migrated tests from snapshot-based to behavior-based testing with React Testing Library

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
`InputFactory.js`	Replaced `FormControl` with `TextInput`, updated prop handling and onChange signature
`InputFactory.test.js`	Replaced snapshot tests with comprehensive behavior-based tests using React Testing Library
`InputFactory.test.js.snap`	Removed snapshot file as tests no longer use snapshots

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-21T11:51:33Z

webpack/assets/javascripts/react_app/components/common/forms/InputFactory.js

+      isDisabled={disabled}
+      isRequired={required}
+      className={className}
+      onChange={(_event, value) => onChange(value)}


The onChange handler should check if onChange exists before calling it. While defaultProps defines onChange as noop, if a parent component explicitly passes undefined or null, this will throw a runtime error.

Suggested change

onChange={(_event, value) => onChange(value)}

onChange={(_event, value) => typeof onChange === 'function' && onChange(value)}

MariaAga

Needs a lint and tests fix, also

I've tried to make this backwards compatible, but please verify if it's enough.

Could you please share where its used?

MariaAga · 2025-10-27T12:24:02Z

webpack/assets/javascripts/react_app/components/common/forms/__tests__/InputFactory.test.js

+    it('should render MemoryAllocationInput for type="memory"', () => {
+      render(<InputFactory type="memory" name="test" id="test-memory" />);
+      expect(screen.getByRole('spinbutton')).toBeInTheDocument();
+    });
+
+    it('should render CounterInput for type="counter"', () => {
+      render(<InputFactory type="counter" name="test" id="test-counter" />);
+      expect(screen.getByRole('spinbutton')).toBeInTheDocument();
+    });
+


These test the same output

MariaAga · 2025-10-27T12:24:18Z

webpack/assets/javascripts/react_app/components/common/forms/__tests__/InputFactory.test.js

+    it('should pass setError and setWarning to custom components', () => {
+      const setError = jest.fn();
+      const setWarning = jest.fn();
+
+      render(
+        <InputFactory
+          {...selectProps}
+          setError={setError}
+          setWarning={setWarning}
+        />
+      );
+
+      expect(screen.getByText('Grouped select')).toBeInTheDocument();


How does it test it?

MariaAga · 2025-10-27T12:54:29Z

webpack/assets/javascripts/react_app/components/common/forms/InputFactory.js

-      type={type}
-      {...validations}
-      {...controlProps}
+    <TextInput


I got this error when rendering it: Text input: Text input requires either an id or aria-label to be specified Error Component Stack

ogajduse · 2025-11-11T19:38:30Z

This PR has a target version set to 3.17, so we'd likely want to cherry-pick it to 3.17-stable

MariaAga · 2025-11-12T09:50:36Z

Thanks, I dont think we have to have it in 3.17

ofedoren

Thanks, @MariaAga, updated.

Could you please share where its used?

IIUC, currently the whole InputFactory is used only for FormField, which also seems to be used only in foreman_webhooks. I think potentially we can just drop both 🤷

Anyway, InputFactory's fallback (TextInput now) seems to be used in foreman_webhooks ForemanFormikField (https://github.com/theforeman/foreman_webhooks/blob/master/webpack/ForemanWebhooks/Routes/Webhooks/Components/WebhookForm/Components/ForemanFormikField.js#L108), but the actual content will still be custom. I've checked if webhook forms still work with these changes and it seems so, but given how UI sometimes weird to me, I'd ask to re-test that.

Copilot

Pull Request Overview

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

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

Copilot · 2025-11-13T15:31:45Z

webpack/assets/javascripts/react_app/components/common/forms/InputFactory.js

+      onChange={(_event, val) => onChange(val)}
+      validated={validated}
+      ouiaId={`input-factory-text-input-${id}`}
+      aria-label={`text-input-${id}`}


The aria-label will produce invalid values like "text-input-null" or "text-input-undefined" when id is null or undefined. Based on the FormField test snapshots, id can indeed be null.

Consider making aria-label conditional or providing a fallback:

aria-label={id ? `text-input-${id}` : 'text-input'}

Alternatively, only include aria-label when id is provided, or use a more descriptive fallback based on the input's purpose.

Suggested change

aria-label={`text-input-${id}`}

aria-label={id ? `text-input-${id}` : 'text-input'}

Copilot · 2025-11-13T15:31:45Z

webpack/assets/javascripts/react_app/components/common/forms/InputFactory.js

+      className={className}
+      onChange={(_event, val) => onChange(val)}
+      validated={validated}
+      ouiaId={`input-factory-text-input-${id}`}


The ouiaId will produce invalid values like "input-factory-text-input-null" or "input-factory-text-input-undefined" when id is null or undefined. Based on the FormField test snapshots, id can indeed be null.

Consider making ouiaId conditional:

ouiaId={id ? `input-factory-text-input-${id}` : undefined}

OUIA IDs should be meaningful identifiers; having null or undefined in them defeats their purpose.

Suggested change

ouiaId={`input-factory-text-input-${id}`}

ouiaId={id ? `input-factory-text-input-${id}` : undefined}

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

kfamilonidis · 2025-11-14T09:47:51Z

...javascripts/react_app/components/common/forms/__tests__/__snapshots__/FormField.test.js.snap

          "useKeyShortcuts": false,
        }
      }
+      id={null}


why these are null ? Don't we need to remove this file altogether ?

These are null, because they are not being provided via fixtures props. Some fixtures do provide them though, see below.

kfamilonidis

The snap files are preventing testing. Please remove them. [/FormField.test.js.snap]‎(https://github.com/theforeman/foreman/pull/10729/files#diff-78eb07ad74cba7f7b5be93c011a04c294006aa76157dd671f22fcd2794901981) is needed to be connected with the input

ofedoren

The snap files are preventing testing.

How exactly? I've removed the snap which is directly related to the component I'm changing, since that snap is now covered by react-test lib tests.

I can't just remove snap for FormField component, since I don't almost change it nor re-writing into PF5 nor I change the test suite from snap-based into react-test-based. That's kinda out of scope for this PR.

ofedoren · 2025-11-14T13:00:26Z

...javascripts/react_app/components/common/forms/__tests__/__snapshots__/FormField.test.js.snap

          "useKeyShortcuts": false,
        }
      }
+      id={null}


These are null, because they are not being provided via fixtures props. Some fixtures do provide them though, see below.

MariaAga

I dont think its used anywhere, anyway I tested it with packit to make sure in webhooks that use InputFactory and report generate that also uses it for the date.
I replaced a place where text_f was used with react_form_input(nil, f, attr, options) and it works well on new and edit.
The only issue, that might be out of the scope of the PR as this was a bug before as well :

Warning: `value` prop on `input` should not be null. Consider using an empty string to clear the component or `undefined` for uncontrolled components.

ofedoren · 2025-11-26T10:29:31Z

Yeah, I'm afraid that bug is not related to this PR or should be dealt with in since here we pass what's provided and if value is null instead of undefined it should be fixed in the component that's passing this in. We already have here some expectations: https://github.com/theforeman/foreman/pull/10729/files#diff-ff434a0e57df48e0aa222e0ef65e260a875b88a7934563b572f26976779e11ffL48-L53

MariaAga

LGTM, I opened https://issues.redhat.com/browse/SAT-40642 to track it

pr-processor bot added the Not yet reviewed label Oct 17, 2025

github-actions bot added the UI label Oct 17, 2025

MariaAga requested a review from Copilot October 21, 2025 11:51

MariaAga added Waiting on contributor breaks-robottelo labels Oct 21, 2025

Copilot AI reviewed Oct 21, 2025

View reviewed changes

pr-processor bot removed the Not yet reviewed label Oct 21, 2025

MariaAga requested changes Oct 27, 2025

View reviewed changes

ofedoren force-pushed the feat-38842-pf5-input-factory branch from f682ebb to 283bfec Compare November 12, 2025 11:57

pr-processor bot added Needs re-review and removed Waiting on contributor labels Nov 12, 2025

ofedoren commented Nov 12, 2025

View reviewed changes

pr-processor bot removed the Needs re-review label Nov 12, 2025

ofedoren force-pushed the feat-38842-pf5-input-factory branch from 283bfec to badbc33 Compare November 13, 2025 14:14

Fixes #38842 - Replace FormControl with TextInput for InputFactory

64280f8

ofedoren force-pushed the feat-38842-pf5-input-factory branch from badbc33 to 64280f8 Compare November 13, 2025 14:24

ofedoren requested a review from Copilot November 13, 2025 15:25

Copilot started reviewing on behalf of ofedoren November 13, 2025 15:26 View session

Copilot finished reviewing on behalf of ofedoren November 13, 2025 15:30

Copilot AI reviewed Nov 13, 2025

View reviewed changes

kfamilonidis reviewed Nov 14, 2025

View reviewed changes

kfamilonidis suggested changes Nov 14, 2025

View reviewed changes

pr-processor bot added the Waiting on contributor label Nov 14, 2025

ofedoren commented Nov 14, 2025

View reviewed changes

MariaAga reviewed Nov 25, 2025

View reviewed changes

MariaAga approved these changes Nov 26, 2025

View reviewed changes

MariaAga merged commit 0b71365 into theforeman:develop Nov 26, 2025
46 checks passed

	onChange={(_event, value) => onChange(value)}
	onChange={(_event, value) => typeof onChange === 'function' && onChange(value)}

	aria-label={`text-input-${id}`}
	aria-label={id ? `text-input-${id}` : 'text-input'}

	ouiaId={`input-factory-text-input-${id}`}
	ouiaId={id ? `input-factory-text-input-${id}` : undefined}

Fixes #38842 - Replace FormControl with TextInput for InputFactory #10729

Fixes #38842 - Replace FormControl with TextInput for InputFactory #10729

Conversation

ofedoren commented Oct 17, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 21, 2025

Choose a reason for hiding this comment

Uh oh!

MariaAga left a comment • 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!

MariaAga Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

MariaAga Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

MariaAga Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

ogajduse commented Nov 11, 2025

Uh oh!

MariaAga commented Nov 12, 2025

Uh oh!

ofedoren left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

kfamilonidis Nov 14, 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!

ofedoren Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

kfamilonidis left a comment • 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!

ofedoren left a comment

Choose a reason for hiding this comment

Uh oh!

ofedoren Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

MariaAga left a comment

Choose a reason for hiding this comment

Uh oh!

ofedoren commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MariaAga left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

MariaAga left a comment •

edited

Loading

kfamilonidis Nov 14, 2025 •

edited

Loading

kfamilonidis left a comment •

edited

Loading

ofedoren commented Nov 26, 2025 •

edited

Loading