feat(RHINENG-20858): Use data driven forms Policy wizard#2724
Merged
LightOfHeaven1994 merged 1 commit intoRedHatInsights:masterfrom Mar 5, 2026
Merged
Conversation
LightOfHeaven1994
force-pushed
the
RHINENG-20858
branch
from
4a0dfbb to
8854991
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
RulesStepContent.onSelect, the branch forupdatedSelectedRuleRefIds.length > 0assumes theosMinorVersionalready exists in the array and accessesupdatedSelectedRuleRefIds[index]without checking forindex === -1, which will throw if a new minor version is selected after others have been added; add a fallback to create a new entry when not found. - In
FinishedStep'sEmptyStateWithErrors, theerrors.mapuseskey={error}and renders{error}directly, which will log[object Object]and trigger key warnings whenerrorscontain objects (as GraphQL errors typically do); use a stable primitive key and render a specific field such aserror.messageinstead. - The
CreatePolicyWrappercomponent contains aconsole.log('Is data driven forms enabled?', isDDFEnabled);which will log on every render in production; consider removing or gating this behind a development check.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `RulesStepContent.onSelect`, the branch for `updatedSelectedRuleRefIds.length > 0` assumes the `osMinorVersion` already exists in the array and accesses `updatedSelectedRuleRefIds[index]` without checking for `index === -1`, which will throw if a new minor version is selected after others have been added; add a fallback to create a new entry when not found.
- In `FinishedStep`'s `EmptyStateWithErrors`, the `errors.map` uses `key={error}` and renders `{error}` directly, which will log `[object Object]` and trigger key warnings when `errors` contain objects (as GraphQL errors typically do); use a stable primitive key and render a specific field such as `error.message` instead.
- The `CreatePolicyWrapper` component contains a `console.log('Is data driven forms enabled?', isDDFEnabled);` which will log on every render in production; consider removing or gating this behind a development check.
## Individual Comments
### Comment 1
<location> `src/SmartComponents/CreatePolicyDDF/components/RulesStep.js:99-108` </location>
<code_context>
+ };
+ }, {});
+
+ const onSelect = useCallback(
+ (
+ _securityGuideId,
+ { os_minor_version: osMinorVersion },
+ newSelectedRuleIds,
+ ) => {
+ const updatedSelectedRuleRefIds = structuredClone(
+ selectedRuleRefIds || [],
+ );
+
+ if (updatedSelectedRuleRefIds.length === 0) {
+ if (newSelectedRuleIds.length === 0) {
+ return;
+ }
+
+ updatedSelectedRuleRefIds.push({
+ osMinorVersion,
+ ruleRefIds: newSelectedRuleIds,
+ });
+ } else {
+ const index = updatedSelectedRuleRefIds.findIndex(
+ ({ osMinorVersion: _osMinorVersion }) =>
+ _osMinorVersion === osMinorVersion,
</code_context>
<issue_to_address>
**issue (bug_risk):** Selecting rules for a previously unseen osMinorVersion can cause an out-of-bounds array access.
In `onSelect`, after `findIndex`, you access `updatedSelectedRuleRefIds[index]` without handling the `index === -1` case. If a user selects rules for an `os_minor_version` not yet in `selectedRuleRefIds`, this will cause an invalid read when dereferencing `.ruleRefIds`. You should handle `index === -1` by inserting a new entry for that `osMinorVersion`, similar to the branch used when the list is initially empty.
</issue_to_address>
### Comment 2
<location> `src/SmartComponents/CreatePolicyDDF/components/FinishedStep.js:125-129` </location>
<code_context>
+ const tailorings = fetchPolicyResponse.data;
+ dispatchProgress();
+
+ for (const tailoring of tailorings) {
+ const rulesToAssign = selectedRuleRefIds.find(
+ ({ osMinorVersion }) =>
+ Number(osMinorVersion) === tailoring.os_minor_version,
+ ).ruleRefIds;
+
+ await assignRules({
</code_context>
<issue_to_address>
**issue (bug_risk):** Assigning rules to tailorings assumes a matching selectedRuleRefIds entry for every tailoring.
Within the `tailorings` loop, `selectedRuleRefIds.find(...)` may return `undefined`, but `.ruleRefIds` is accessed unconditionally. If a tailoring exists for an `os_minor_version` that isn’t in `selectedRuleRefIds`, this will throw. Please add a guard (e.g., treat missing entries as an empty rule list, or skip/log them) to handle this mismatch safely.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2724 +/- ##
==========================================
- Coverage 65.83% 60.43% -5.41%
==========================================
Files 247 258 +11
Lines 3401 3806 +405
Branches 987 1108 +121
==========================================
+ Hits 2239 2300 +61
- Misses 1162 1506 +344
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
LightOfHeaven1994
force-pushed
the
RHINENG-20858
branch
from
5e41d47 to
92688d6
Compare
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In
CreatePolicyWrapperyou still have aconsole.logfor the feature flag; this should be removed or switched to the app’s logging/telemetry mechanism before merging to avoid noisy console output in production. - In
DetailsStepContent, thethresholdlocal state is always initialized to100and never synchronized from the form values, so revisiting the step in the wizard will show 100 even if the user previously entered a different threshold; consider initializing and updating the local state fromvalues.complianceThresholdto keep the UI and form state consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `CreatePolicyWrapper` you still have a `console.log` for the feature flag; this should be removed or switched to the app’s logging/telemetry mechanism before merging to avoid noisy console output in production.
- In `DetailsStepContent`, the `threshold` local state is always initialized to `100` and never synchronized from the form values, so revisiting the step in the wizard will show 100 even if the user previously entered a different threshold; consider initializing and updating the local state from `values.complianceThreshold` to keep the UI and form state consistent.
## Individual Comments
### Comment 1
<location path="src/SmartComponents/CreatePolicyDDF/components/RulesStep.js" line_range="141-150" />
<code_context>
+ [change, selectedRuleRefIds],
+ );
+
+ useDeepCompareEffect(() => {
+ const newSelectedRuleRefIds = osMinorVersionCounts?.reduce(
+ (accum, { osMinorVersion }) => {
+ if (
+ selectedRuleRefIds?.some(
+ (entry) => entry.osMinorVersion === osMinorVersion,
+ )
+ ) {
+ return accum;
+ }
+ const refIdsPerMinorVersion = profilesAndRuleIds?.find(
+ ({ osMinorVersion: profileOsMinorVersion }) =>
+ profileOsMinorVersion === osMinorVersion,
+ ).ruleIds;
+
+ return [
+ ...accum,
+ ...(refIdsPerMinorVersion?.length
+ ? [{ osMinorVersion, ruleRefIds: refIdsPerMinorVersion }]
+ : []),
+ ];
+ },
+ [],
+ );
+ if (newSelectedRuleRefIds?.length !== 0) {
+ change('selectedRuleRefIds', [
+ ...(selectedRuleRefIds || []),
</code_context>
<issue_to_address>
**issue (bug_risk):** New selectedRuleRefIds calculation can spread `undefined`, causing a runtime error.
Because `reduce` is called without an explicit initial array, `newSelectedRuleRefIds` can be `undefined` when `osMinorVersionCounts` is `undefined`. In that case, `newSelectedRuleRefIds?.length !== 0` is `true` and the code enters the `if`, but `...[...newSelectedRuleRefIds]` will throw at runtime. You can avoid this either by giving `reduce` an initial empty array or by guarding the condition, e.g. `if (newSelectedRuleRefIds && newSelectedRuleRefIds.length !== 0)`.
</issue_to_address>
### Comment 2
<location path="src/SmartComponents/CreatePolicyDDF/components/FinishedStep.js" line_range="74-79" />
<code_context>
+ businessObjective,
+ complianceThreshold,
+ systems,
+ selectedRuleRefIds,
+ valueOverrides = {},
+ } = values;
+
+ const cloneFromProfileId = profile.id;
+ const minorVersions = selectedRuleRefIds.map(
+ ({ osMinorVersion }) => osMinorVersion,
+ );
</code_context>
<issue_to_address>
**issue (bug_risk):** Assuming `selectedRuleRefIds` is always defined can lead to a crash on policy creation.
In `createPolicyAsync`, `selectedRuleRefIds` is destructured from `values` and then used as `selectedRuleRefIds.map(...)`. If it ever comes through as `undefined`, this will throw and abort policy creation. Please either default it to an empty array in the destructuring (`selectedRuleRefIds = []`) or add a guard before calling `.map`.
</issue_to_address>
### Comment 3
<location path="src/SmartComponents/CreatePolicyDDF/components/SystemsStep.js" line_range="32" />
<code_context>
+const EmptyState = ({ osMajorVersion, onGoToStep1 }) => {
+ return (
+ <React.Fragment>
+ <Content className="pf-v6-u-mb-md">
+ <Content component="p">
+ You do not have any <b>RHEL {osMajorVersion}</b> systems connected to{' '}
+ Red Hat Lightspeed and enabled for Compliance.
+ <br />
+ </Content>
+ </Content>
+ <Content className="pf-v6-u-mb-md">
+ <Content component="p">
+ Choose a different RHEL version , or connect{' '}
+ <b>RHEL {osMajorVersion}</b> systems to Red Hat Lightspeed.
+ </Content>
</code_context>
<issue_to_address>
**nitpick (typo):** Minor punctuation/spacing issue in the Systems step empty state text.
The text currently reads `Choose a different RHEL version , or connect ...` with an extra space before the comma. Please update it to `version, or` to fix the spacing in the UI string.
```suggestion
Choose a different RHEL version, or connect{' '}
```
</issue_to_address>
### Comment 4
<location path="src/SmartComponents/CreatePolicyDDF/schema.js" line_range="25" />
<code_context>
+ component: 'create-scap-policy-step',
+ name: 'scap-policy-selection',
+ validate: [
+ (value) =>
+ value !== 'valid'
+ ? 'Profile selection validation failed'
</code_context>
<issue_to_address>
**issue (complexity):** Consider introducing a shared `stepValidityValidator` with an explicit `{ isValid: boolean }` value shape to replace the repeated `'valid'` string checks and inline lambdas.
You can reduce the “magic string” coupling and repetition without changing behavior by making the validity contract explicit and centralized.
### 1. Replace `'valid'` sentinel with an explicit object shape
Define a shared validator that checks an explicit `isValid` flag instead of a string:
```js
// validation.js
export const stepValidityValidator = (value) =>
value?.isValid === true ? undefined : 'Step validation pending or failed';
```
Then update the schema to use this validator instead of inline functions:
```js
import { componentTypes } from '@data-driven-forms/react-form-renderer';
import { stepValidityValidator } from './validation';
const schema = {
fields: [
{
component: componentTypes.WIZARD,
name: 'create-policy-wizard',
// ...
fields: [
{
name: 'step-create-scap-policy',
title: 'Create SCAP policy',
nextStep: 'step-details',
fields: [
{
component: 'create-scap-policy-step',
name: 'scap-policy-selection',
validate: [stepValidityValidator],
},
],
},
{
name: 'step-details',
title: 'Details',
nextStep: 'step-systems',
fields: [
{
component: 'details-step',
name: 'details-content',
validate: [stepValidityValidator],
},
],
},
{
name: 'step-rules',
title: 'Rules',
nextStep: 'step-review',
fields: [
{
component: 'rules-step',
name: 'rules-content',
validate: [stepValidityValidator],
},
],
},
// ...
],
},
],
};
export default schema;
```
In your step components, switch from the string sentinel to the explicit shape:
```js
// before:
// input.onChange('valid');
// after:
input.onChange({ isValid: true });
// to mark invalid/pending:
input.onChange({ isValid: false });
```
You can still customize messages per step if needed:
```js
const makeStepValidityValidator = (message) => (value) =>
value?.isValid === true ? undefined : message;
{
component: 'create-scap-policy-step',
name: 'scap-policy-selection',
validate: [makeStepValidityValidator('Profile selection validation failed')],
}
```
This keeps your current gating behavior but:
- Removes the non-obvious `'valid'` protocol.
- Documents the contract via the `value` shape (`{ isValid: boolean }`).
- Centralizes validation logic instead of repeating inline lambdas.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
LightOfHeaven1994
force-pushed
the
RHINENG-20858
branch
from
92688d6 to
32ce064
Compare
LightOfHeaven1994
force-pushed
the
RHINENG-20858
branch
from
32ce064 to
4a2fe96
Compare
bastilian
approved these changes
Mar 5, 2026
Member
bastilian
left a comment
bastilian
left a comment
There was a problem hiding this comment.
Works awesome! Thank you @LightOfHeaven1994!
- Add DDF implementation for Create policy wizard - Move common pieces to utilities/presential-components - Use feature flag
LightOfHeaven1994
force-pushed
the
RHINENG-20858
branch
from
4a2fe96 to
f327bff
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This PR brings data-driven-forms for Create policy wizard. Changes are shown when
compliance.data-driven-formsfeature flag is enabled (currently it's set to true in Stage).To test change:
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Introduce a data-driven-forms-based Create Policy wizard gated by feature flag and wire it into the existing Create Policy route.
New Features:
Enhancements:
Build: