Wizard: Review step revamp (HMS-10785)#4594
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #4594 +/- ##
==========================================
- Coverage 77.92% 74.56% -3.37%
==========================================
Files 255 259 +4
Lines 7118 7854 +736
Branches 2654 2874 +220
==========================================
+ Hits 5547 5856 +309
- Misses 1440 1780 +340
- Partials 131 218 +87
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 49 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
![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
Firewall.tsx,selectFirewallEnabled/isEnabledis now unused; either remove it or use it to control whether the section renders or to indicate enabled/disabled status so the state is reflected in the new UI. - The
Firstboottruncation logic currently splits the script into two separateCodeBlockCodesections, so when expanded the user sees only the tail; consider showing the full script when expanded (e.g., by toggling between truncated and complete content) to avoid losing context.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Firewall.tsx`, `selectFirewallEnabled` / `isEnabled` is now unused; either remove it or use it to control whether the section renders or to indicate enabled/disabled status so the state is reflected in the new UI.
- The `Firstboot` truncation logic currently splits the script into two separate `CodeBlockCode` sections, so when expanded the user sees only the tail; consider showing the full script when expanded (e.g., by toggling between truncated and complete content) to avoid losing context.
## Individual Comments
### Comment 1
<location path="src/Components/CreateImageWizard/steps/Review/components/AdvancedSettings/components/Filesystem.tsx" line_range="84-87" />
<code_context>
+ }
+ />
+ )}
+ {mode === 'advanced' && diskMinsize && (
+ <ReviewGroup
+ heading='Minimum disk size'
+ description={`${diskMinsize} ${diskUnit}`}
+ />
+ )}
</code_context>
<issue_to_address>
**issue (bug_risk):** Use an explicit undefined check for diskMinsize to avoid skipping valid falsy values
This condition hides the "Minimum disk size" section when `diskMinsize` is `0`, even though `0` may be valid. Basic mode already uses `minSize !== undefined` instead of a truthy check. For consistency and to avoid hiding a configured value, update this to an explicit undefined check (e.g. `diskMinsize !== undefined`) so `0` still renders.
</issue_to_address>
### Comment 2
<location path="src/Components/CreateImageWizard/steps/Review/components/AdvancedSettings/tests/AdvancedSettings.test.tsx" line_range="442-445" />
<code_context>
},
);
+ expect(screen.getByText('Languages')).toBeInTheDocument();
expect(screen.getByText('en_US.UTF-8')).toBeInTheDocument();
expect(screen.getByText('es_ES.UTF-8')).toBeInTheDocument();
expect(screen.getByText('fr_FR.UTF-8')).toBeInTheDocument();
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test case for single-language locale to assert the singular "Language" heading
Current tests cover multiple languages (asserting `Languages`) and zero languages (asserting neither heading). Please add a test where exactly one language is configured and assert that the singular `Language` heading is rendered, to cover the `languages.length > 1` branch and guard against regressions.
Suggested implementation:
```typescript
expect(screen.queryByText('Language')).not.toBeInTheDocument();
});
test('displays singular Language heading when a single language is configured', () => {
// Render AdvancedSettings with exactly one language configured
renderAdvancedSettings({
languages: ['en_US.UTF-8'],
});
// Singular heading should be present
expect(screen.getByText('Language')).toBeInTheDocument();
// The single language should be rendered
expect(screen.getByText('en_US.UTF-8')).toBeInTheDocument();
// Plural heading should not be present when there is only one language
expect(screen.queryByText('Languages')).not.toBeInTheDocument();
```
Because the surrounding context of the test file is partially shown, please verify and adjust the following when integrating this change:
1. Ensure `renderAdvancedSettings` is the correct helper used by the existing language-related tests. If a different helper is used (e.g. `renderReview`, `renderWithProviders`, etc.), replace `renderAdvancedSettings` with that helper and pass the `languages` configuration in the same way as the multi-language test.
2. Confirm that the inserted closing `});` aligns with the end of the preceding test. If there is already a `});` directly after the original `expect(screen.queryByText('Language')).not.toBeInTheDocument();`, remove the extra one to avoid a syntax error.
3. Place this new test near the other language-related tests (between the multi-language test and the zero-language test, if they are separate), to keep the file organized and consistent.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
kingsleyzissou
left a comment
kingsleyzissou
left a comment
There was a problem hiding this comment.
Nice, this is looking good. I had a few inline suggestions
| oscapServices, | ||
| }: AdvancedSettingsOverviewProps) => { | ||
| const usersHidden = | ||
| restrictions.users.shouldHide || restrictions.users.required; |
There was a problem hiding this comment.
These are kind of opposites though. If users are required, then restrictions.users.shouldHide should be false.
There was a problem hiding this comment.
This was a headwrecker, but I've gone through the code and tested it and it all works
There was a problem hiding this comment.
Thanks for looking into it! It sure was 🤓
| oscapServices={oscapServices} | ||
| /> | ||
| <Firewall shouldHide={restrictions.firewall.shouldHide} /> | ||
| {hasUserGroups && ( |
There was a problem hiding this comment.
This could get moved into the shouldHide prop because it's doing the same thing
There was a problem hiding this comment.
Other components like Locale have their own ReviewSection internally and return null when hidden, so no
empty heading/divider renders.
Users and UserGroups can't do the same because ImageOverview also renders them, without a ReviewSection wrapper. Since both places share the same component, the ReviewSection has to stay outside in the AdvancedSettings index, and the conditional rendering stays too to avoid an empty heading and divider.
There was a problem hiding this comment.
Thinking about this further (and this can absolutely be a follow up since this works). Maybe we should lift the should hide to the ReviewSection component? And that could return the null there? I guess the tradeoff is we'd be passing it down through two components.
But that shouldn't block this PR. And it was a bit of a thought in terms of keeping things consistent
There was a problem hiding this comment.
sounds nice, I'll check it out in a follow up then
Edit the Review step according to mocks
The name change reflects the usage better - the Users were not really required, but only moved to different parts of the Review steps based on bunch of conditions.
kingsleyzissou
left a comment
kingsleyzissou
left a comment
There was a problem hiding this comment.
Looks good.
Maybe we could possibly add a margin top to the line separator, to give a little bit more breathing room, but I know that's a bit tricky because a lot of the content is dynamic here.
Screenshot for reference (timezone in this one):
|
We could, I will also look into it in another PR, this is huge as is. Thank you for review! |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
2 participants
Edit the Review step according to mocks.
Renamed
requiredtoisStandaloneinRestrictionStrategy. The field controls whether the Users customization gets its own wizard step instead of being nested under Advanced Settings. The old name suggested a data validation constraint, but it actually determines where the section is placed in the UI.