Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wizard: add Administrator checkbox to users step (HMS-4903) #2717

Merged
merged 1 commit into from
Jan 21, 2025

Conversation

mgold1234
Copy link
Collaborator

this commit add Administrator checkbox to users step

@mgold1234 mgold1234 force-pushed the add_admin branch 2 times, most recently from b224c1a to 30864e5 Compare January 6, 2025 11:37
@regexowl
Copy link
Collaborator

regexowl commented Jan 7, 2025

An empty groups array hangs in the request if there are no groups added, causing the error in Users.test.tsx tests. Might need something like "if there's at least one group then add the groups array to the request".

@mgold1234 mgold1234 force-pushed the add_admin branch 2 times, most recently from 766b1fa to ef66848 Compare January 8, 2025 09:02
Copy link

codecov bot commented Jan 8, 2025

Codecov Report

Attention: Patch coverage is 95.34884% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84.71%. Comparing base (bdd259f) to head (3a3a321).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/store/wizardSlice.ts 88.88% 2 Missing ⚠️

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2717      +/-   ##
==========================================
+ Coverage   84.69%   84.71%   +0.02%     
==========================================
  Files         186      186              
  Lines       21199    21241      +42     
  Branches     2076     2083       +7     
==========================================
+ Hits        17955    17995      +40     
- Misses       3222     3224       +2     
  Partials       22       22              
Files with missing lines Coverage Δ
...eateImageWizard/steps/Users/component/UserInfo.tsx 95.09% <100.00%> (+1.19%) ⬆️
...nents/CreateImageWizard/utilities/requestMapper.ts 94.71% <100.00%> (+0.02%) ⬆️
src/test/fixtures/editMode.ts 100.00% <100.00%> (ø)
src/store/wizardSlice.ts 88.78% <88.88%> (+<0.01%) ⬆️

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 bdd259f...3a3a321. Read the comment docs.

@mgold1234 mgold1234 requested a review from regexowl January 8, 2025 09:53
@mgold1234 mgold1234 force-pushed the add_admin branch 4 times, most recently from 4d51518 to 474bae9 Compare January 8, 2025 13:24
@mgold1234 mgold1234 force-pushed the add_admin branch 4 times, most recently from 8155881 to ce05e17 Compare January 13, 2025 09:16
Copy link
Collaborator

@lucasgarfield lucasgarfield left a comment

Choose a reason for hiding this comment

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

Looking good, works nicely.

Please add tests for both edit and import mode where isAdmin === true.

src/store/wizardSlice.ts Outdated Show resolved Hide resolved
src/store/wizardSlice.ts Outdated Show resolved Hide resolved
src/store/wizardSlice.ts Outdated Show resolved Hide resolved
@mgold1234 mgold1234 force-pushed the add_admin branch 2 times, most recently from 6f6d3b3 to c4f72ed Compare January 15, 2025 16:06
Copy link
Contributor

@avitova avitova left a comment

Choose a reason for hiding this comment

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

Thanks, I genuinely love that we have this split into more pull requests!

@mgold1234 mgold1234 force-pushed the add_admin branch 4 times, most recently from b04bf98 to 41fb4dd Compare January 19, 2025 09:07
@mgold1234 mgold1234 requested a review from avitova January 19, 2025 10:39
@mgold1234 mgold1234 force-pushed the add_admin branch 3 times, most recently from 6031f54 to 999b5c4 Compare January 20, 2025 19:24
Copy link
Collaborator

@regexowl regexowl left a comment

Choose a reason for hiding this comment

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

Added a few nitpicks. otherwise looks good :)

src/store/wizardSlice.ts Outdated Show resolved Hide resolved
@mgold1234 mgold1234 force-pushed the add_admin branch 3 times, most recently from e85b3a0 to 878abd7 Compare January 21, 2025 09:34
@regexowl
Copy link
Collaborator

@mgold1234 there are some conflicts after merging the username validation, can you please resolve them?

this commit add Administrator checkbox to users step
Copy link
Collaborator

@regexowl regexowl left a comment

Choose a reason for hiding this comment

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

I think this looks good.

import tests will need to be added after adding user customization to on-prem mapper. Adding edit tests might be safer when the step functionality is fully implemented as well. That way we don't forget to update them with new fields.

I've created a checklist task in JIRA to track what was already done and what still needs to be addressed.

@regexowl regexowl dismissed lucasgarfield’s stale review January 21, 2025 11:59

Import tests need to be coupled with changes to on-prem mapper, edit tests can be also split. I've created a JIRA task to track what's already done for the Users step. That way we don't forget about the tests later.

@regexowl regexowl enabled auto-merge (rebase) January 21, 2025 11:59
@regexowl regexowl merged commit 25f1240 into osbuild:main Jan 21, 2025
17 of 18 checks passed
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.

4 participants