-
Notifications
You must be signed in to change notification settings - Fork 222
refactor: added validation and fixed acces permission reset issues #2259
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
base: dev
Are you sure you want to change the base?
Conversation
Signed-off-by: Khushi Agrawal <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @khushiiagrawal. Thanks for your PR. I'm waiting for a kubestellar member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
instead of waiting for someone to click 'add user' - why not indicate, below the password entry, that the password is not long enough? Don't give a toast message - it's too late. Give it while the person is adding a password and needs guidance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR addresses two key issues in the Add User form: improving password validation error messaging by surfacing backend details messages, and fixing the administrator access toggle to properly reset permissions to an unselected state when toggled off. The changes introduce nullable permission values and refactor error handling.
Key Changes:
- Enhanced Axios error interceptor to prioritize backend
detailsfield for more specific error messages - Refactored permission state to support nullable values, allowing proper reset when admin toggle is disabled
- Added helper functions for API error extraction and permission sanitization
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| frontend/src/lib/api.ts | Updated Axios response interceptor to prioritize details field in error responses |
| frontend/src/components/admin/UserTypes.ts | Added PermissionValue type with null support and updated type definitions |
| frontend/src/components/admin/UserManagement.tsx | Added error extraction and permission sanitization helpers; updated error handling and permission state management |
| frontend/src/components/admin/UserFormModal.tsx | Fixed admin toggle to reset permissions to null; removed inline error display |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sure. should i do the same for the "user name already taken" toast or keep it like the toast ? |
|
yeah - no toast for either of these - dynamic response is better - less frustrating I think - what do you think? |
agree. it will be a better way to increase the user experience. |
Signed-off-by: Khushi Agrawal <[email protected]>
Signed-off-by: Khushi Agrawal <[email protected]>
|
@clubanderson @MAVRICK-1 PTAL Screen.Recording.2025-11-20.at.12.03.03.AM.mov |
|
Will check at EOD |
|
@khushiiagrawal there is missing formError prop in UserFormModal props (runtime mismatch) |
|
2nd validatePassword returns null for empty password unconditionally fix it |
|
@khushiiagrawal 3rd Username uniqueness checking is case-sensitive make it normalized soo ADMIN or admin shoud be treated same |
|
any updates..? |
Signed-off-by: Khushi Agrawal <[email protected]>
@btwshivam I have made all the requested changes, now the validation is as it should be. Also added trim fun so that it doesn't take spaces. Improved the UI of the "Edit user" card. |
|
/ok-to-test |
|
/lgtm |
|
LGTM label has been added. DetailsGit tree hash: ed53545f39eb3f0afc9af90104a7409235ce50a7 |
|
@khushiiagrawal Do NOT trim password inputs while typing... Either: |
|
why you hardcoded the validateUsername and validatePassword return messages like 'Username is already taken', 'Password must be at least 5 characters long', and the "Password length is valid" ??? |
|
ahh too many issuess.. dont just vibe code check also what you are doing... |
|
UserManagement passes formError to UserFormModal: formError={formError ?? undefined}. In UserFormModal the former top-of-form motion.div that rendered formError was removed! why?? Either reintroduce formError rendering in UserFormModal or remove passing the prop. |
| animate={{ opacity: 1, y: 0 }} | ||
| className="mt-1 flex items-center gap-1 text-xs text-green-500" | ||
| > | ||
| <FiCheck size={12} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from where you imported ficheck icon?? i cant see any
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
PTAL /cc @btwshivam Screen.Recording.2025-12-09.at.3.16.56.AM.mov |
@btwshivam Could you please clarify how exactly you want the password field to behave regarding spaces while typing and validation? I just want to make sure I implement it as expected. |
|
@btwshivam PTAL and let me know what is the requirement here so that i can proceed. |

Description
Fixes two issues in the Add User form:
The backend (
backend/utils/validation.go) enforceslen(password) >= 5and returns “password must be at least 5 characters long.” The frontend now surfaces that exactdetailsmessage via the Axios interceptor and user-management flow instead of the generic “Invalid password.”When Administrator Access is toggled off, per the expected behavior, every component permission is cleared (set to
null) so the user can reselect read/write; previously the radios stayed stuck on “write” after toggling back.Related Issue
Fixes #2133
Changes Made
response.data.detailsover generic error messages for better error visibilitynullwhen Administrator Access is disabledPermissionValue = 'read' | 'write' | null)handlePermissionChangecallback to check for unchanged permissions before object operationsextractApiErrorMessageandsanitizePermissionsfor better error handling and permission managementScreenshots or Logs (if applicable)
Screen.Recording.2025-11-19.at.2.31.27.PM.mov
Checklist