Skip to content

Wizard: Move Users section from Advanced Settings to Image Overview (HMS-10579)#4467

Open
mgold1234 wants to merge 1 commit into
osbuild:mainfrom
mgold1234:fix_use_re
Open

Wizard: Move Users section from Advanced Settings to Image Overview (HMS-10579)#4467
mgold1234 wants to merge 1 commit into
osbuild:mainfrom
mgold1234:fix_use_re

Conversation

@mgold1234
Copy link
Copy Markdown
Collaborator

@mgold1234 mgold1234 commented May 27, 2026

Users are configured in Base Settings (for image mode in cockpit-image-builder), but the Review step displayed them under Advanced Settings.
Move the Users component to the Image Overview card so the review page matches where users are actually set.

JIRA: (HMS-10579)

@codecov
Copy link
Copy Markdown

codecov Bot commented May 27, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.36%. Comparing base (29382df) to head (159f12a).
⚠️ Report is 1 commits behind head on main.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4467      +/-   ##
==========================================
+ Coverage   75.34%   75.36%   +0.01%     
==========================================
  Files         229      229              
  Lines        7446     7452       +6     
  Branches     2770     2770              
==========================================
+ Hits         5610     5616       +6     
  Misses       1578     1578              
  Partials      258      258              
Files with missing lines Coverage Δ
...steps/Review/components/AdvancedSettings/index.tsx 100.00% <100.00%> (ø)
...rd/steps/Review/components/ImageOverview/index.tsx 100.00% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29382df...159f12a. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mgold1234 mgold1234 force-pushed the fix_use_re branch 2 times, most recently from aa8e4f9 to 40353b9 Compare May 28, 2026 10:09
@mgold1234 mgold1234 changed the title Wizard: Move Users section from Advanced Settings to Image Overview Wizard: Move Users section from Advanced Settings to Image Overview (HMS-10579) May 28, 2026
@mgold1234 mgold1234 marked this pull request as ready for review May 28, 2026 12:57
@mgold1234 mgold1234 requested a review from a team as a code owner May 28, 2026 12:57
Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 1 issue, and left some high level feedback:

  • The isOnPremise && isImageMode condition for whether to render the Users section is duplicated in both ImageOverview and AdvancedSettingsOverview; consider extracting this into a shared selector or helper to keep the visibility logic centralized and consistent.
  • Importing Users from ../AdvancedSettings/components couples ImageOverview to the Advanced Settings internals; it may be cleaner to move Users into a shared components module that both overview sections can consume.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The `isOnPremise && isImageMode` condition for whether to render the Users section is duplicated in both `ImageOverview` and `AdvancedSettingsOverview`; consider extracting this into a shared selector or helper to keep the visibility logic centralized and consistent.
- Importing `Users` from `../AdvancedSettings/components` couples `ImageOverview` to the Advanced Settings internals; it may be cleaner to move `Users` into a shared components module that both overview sections can consume.

## Individual Comments

### Comment 1
<location path="src/Components/CreateImageWizard/steps/Review/components/ImageOverview/index.tsx" line_range="79-80" />
<code_context>
           <PrivateClouds environments={privateClouds} />
           <PublicClouds environments={publicClouds} />
           <MiscFormats environments={miscFormats} />
+          {isOnPremise && isImageMode && (
+            <Users shouldHide={restrictions.users.shouldHide} />
+          )}
         </ReviewList>
</code_context>
<issue_to_address>
**suggestion:** Consider centralizing the condition that decides where the Users section is rendered.

You’re now using `isOnPremise && isImageMode` (and its negation) in both `ImageOverview` and `AdvancedSettingsOverview` to control where `Users` appears. Extracting a helper like `shouldShowUsersInOverview({ isOnPremise, isImageMode })` or at least a shared/local boolean would reduce duplication and the risk of these conditions drifting, and make it easier to evolve the logic if more modes/environments are added.

Suggested implementation:

```typescript
  const { restrictions } = useCustomizationRestrictions({
    selectedImageTypes: environments,
  });

  const releases = isOnPremise ? ON_PREM_RELEASES : RELEASES;
  const showUsersInOverview = isOnPremise && isImageMode;

```

```typescript
          <PrivateClouds environments={privateClouds} />
          <PublicClouds environments={publicClouds} />
          <MiscFormats environments={miscFormats} />
          {showUsersInOverview && (
            <Users shouldHide={restrictions.users.shouldHide} />
          )}

```

To fully centralize the logic across both overview components, you should:
1. Apply the same pattern in `AdvancedSettingsOverview` (introduce a shared/local boolean or helper function).
2. If this logic is expected to evolve further (e.g., more environments/modes), consider extracting `shouldShowUsersInOverview({ isOnPremise, isImageMode })` into a shared utility module used by both `ImageOverview` and `AdvancedSettingsOverview`.
</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.

Comment thread src/Components/CreateImageWizard/steps/Review/components/ImageOverview/index.tsx Outdated
@mgold1234 mgold1234 force-pushed the fix_use_re branch 2 times, most recently from c6a6d68 to 7ebc776 Compare June 2, 2026 09:59
…423)

In the on-prem image mode wizard, Users are configured under Base
Settings. The Review step now mirrors this by showing Users under
Image Overview when both isOnPremise and isImageMode are true.
For all other cases (hosted, or on-prem package mode), Users remain
under Advanced Settings.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants