Edit User Details #10027
Edit User Details #10027
Conversation
WalkthroughThis pull request introduces comprehensive enhancements to user management functionality across multiple components. The changes include adding localization support for user-related actions, creating a new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
src/components/Users/UserForm.tsx (1)
Line range hint
53-109: Review conditional schema for consistency and completenessThe form schema conditionally makes several fields optional based on
isEditMode. While this allows flexibility, please verify:
- All required fields are enforced correctly in both modes.
- Optional fields in edit mode still validate correctly if provided.
- Conditional logic does not introduce unintended side effects.
🧹 Nitpick comments (12)
src/components/Users/UserForm.tsx (8)
2-2: Consider removing unused importuseQueryThe
useQueryimport from@tanstack/react-queryis not used in this section of the code. Removing unused imports can help keep the code clean and improve readability.-import { useMutation, useQuery } from "@tanstack/react-query"; +import { useMutation } from "@tanstack/react-query";
124-130: Handle potential query issues when fetching user dataThe
useQueryhook depends onexistingUsername. Ensure thatexistingUsernameis defined before initiating the query to prevent unnecessary network requests or errors.Consider adding a check to prevent the query from running if
existingUsernameis undefined or empty.
132-146: Remove debug statements from production codeThe
console.log(userData);statement is useful for debugging but should be removed to keep the console clean in production.- console.log(userData);
157-160: Optimize form triggering for better performanceThe
form.trigger("username");is called inside auseEffectwithout specifying dependencies strictly. This could lead to unnecessary re-renders or validations.Ensure that
form.trigger("username");is only called when necessary, possibly by adjusting the dependencies of theuseEffect.
206-216: Enhance error handling for user creationThe
createUsermutation lacks detailed error handling. Consider capturing specific error messages from the API to provide more informative feedback to the user.You could access
error.response.dataif available to display validation errors returned from the server.
218-230: Implement comprehensive error handling for user updatesSimilarly, the
updateUsermutation could benefit from enhanced error handling to provide users with specific feedback when an update fails.
246-247: Remove leftover console logsThere is a
console.log(form.formState.errors);statement that should be removed to clean up the code.- console.log(form.formState.errors);
318-369: Confirm password update flowPassword fields are hidden in edit mode, meaning users cannot change their passwords through this form. If password updates are required, consider adding functionality to handle password changes securely.
If implementing password updates, ensure that current password verification and appropriate security measures are in place.
src/pages/Organization/components/EditUserSheet.tsx (2)
15-20: Props interface looks good, but consider adding validation props.The interface is well-structured. Consider adding validation props to handle cases where editing might be restricted.
interface EditUserSheetProps { existingUsername: string; open: boolean; setOpen: (open: boolean) => void; onUserUpdated?: (user: UserBase) => void; + isEditable?: boolean; + validationError?: string; }
31-34: Add ARIA labels for better accessibility.The sheet content should have appropriate ARIA labels for screen readers.
<SheetContent className="w-full sm:max-w-2xl overflow-y-auto" data-cy="add-user-form" + aria-label={t("edit_user_form")} >src/components/Users/UserSummary.tsx (1)
110-120: Consider adding loading state to the edit button.The edit button should reflect loading state during user updates.
<Button variant="outline" className="w-fit self-end" data-cy="edit-user-button" + disabled={isUpdating} onClick={() => setShowEditUserSheet(true)} > <CareIcon icon="l-pen" className="mr-2 h-4 w-4" /> - {t("edit_user")} + {isUpdating ? t("updating") : t("edit_user")} </Button>src/pages/Organization/components/EditUserRoleSheet.tsx (1)
249-258: Move edit button next to user info for better UX.Consider relocating the edit button to be closer to the user information section for improved user experience.
The edit button would be more intuitive if placed near the user's details (around line 154) rather than at the bottom of the action buttons.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
public/locale/en.json(5 hunks)src/common/constants.tsx(1 hunks)src/components/Patient/PatientRegistration.tsx(1 hunks)src/components/Users/UserForm.tsx(9 hunks)src/components/Users/UserSummary.tsx(3 hunks)src/pages/Organization/components/AddUserSheet.tsx(3 hunks)src/pages/Organization/components/EditUserRoleSheet.tsx(3 hunks)src/pages/Organization/components/EditUserSheet.tsx(1 hunks)src/types/user/user.ts(1 hunks)src/types/user/userApi.ts(2 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Users/UserForm.tsx (1)
Learnt from: rajku-dev
PR: ohcnetwork/care_fe#9887
File: src/components/Users/CreateUserForm.tsx:93-93
Timestamp: 2025-01-14T09:22:13.878Z
Learning: In CreateUserForm.tsx, the gender validation schema uses string literals that match GENDER_TYPES.map(g => g.id). Using GENDER_TYPES directly with z.enum() fails because it's marked with 'as const' which makes it a readonly tuple type incompatible with Zod's enum.
🔇 Additional comments (18)
src/components/Users/UserForm.tsx (7)
44-44: EnsureexistingUsernameis passed as a prop when in edit modeThe
existingUsernameprop is added to determine edit mode. Verify that this prop is correctly passed from the parent component when editing a user to prevent unexpected behavior.
49-49: Confirm the reliability of theisEditModecheckThe
isEditModeflag uses double negation onexistingUsername. Ensure thatexistingUsernameis reliably defined (not an empty string or falsy value) when in edit mode to prevent logic errors.
94-96: Re-evaluate optionalgeo_organizationfield in edit modeThe
geo_organizationfield is optional in edit mode. Confirm if the organization should be editable or required when updating a user, as changing a user's organization might have significant implications.
168-168: Prevent username availability check in edit modeThe username availability check should not run in edit mode, as the username is not intended to be changed. The
enabledcondition already accounts for this, but double-check to ensure it's functioning as expected.
251-278: Review conditional rendering of user type selectionThe user type selection is hidden in edit mode. Ensure that this aligns with the application's requirements. If user roles should be editable, consider displaying this field when appropriate permissions are granted.
552-568: Validate the necessity ofgeo_organizationin edit modeThe
geo_organizationfield is conditionally rendered only in create mode. Confirm whether users should be able to change their organization during an edit. If so, adjust the form to allow editing this field.
571-573: Button label correctly reflects the form modeThe submit button label changes based on
isEditMode, which enhances user experience by providing appropriate context.src/types/user/userApi.ts (2)
15-15: UpdateTBodyto useCreateUserModelChanging
TBodytoCreateUserModelin thecreatemethod ensures that the correct data structure is used when creating a user.
27-32: Addupdatemethod touserApiThe
updatemethod allows for updating user data via a PUT request. Ensure that the backend supports this endpoint and that it properly handles partial updates.Double-check if
Partial<UpdateUserModel>aligns with the backend expectations. If the backend supports PATCH requests for partial updates, consider usingHttpMethod.PATCHinstead.src/types/user/user.ts (2)
18-33: DefineCreateUserModelfor user creationThe
CreateUserModeltype includes all necessary fields for creating a new user, which improves type safety and clarity.
35-35: Review fields excluded inUpdateUserModelThe
UpdateUserModelomitsusernameandpassword, indicating these fields are not updatable. Verify if users should be able to change their passwords or usernames, and adjust the model accordingly.src/pages/Organization/components/AddUserSheet.tsx (2)
44-45: LGTM! Good use of translations.The translation keys are descriptive and follow the established pattern.
Line range hint
48-53: Verify UserForm compatibility.The switch from CreateUserForm to UserForm needs verification to ensure all creation-specific functionality is preserved.
✅ Verification successful
The switch to UserForm is compatible and safe
The UserForm component is a unified implementation that handles both user creation and editing. When used without the
existingUsernameprop (as in AddUserSheet), it operates in creation mode with all the necessary functionality preserved.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if UserForm maintains all props from CreateUserForm ast-grep --pattern 'interface $interface { $$$ }' | grep -A 10 'CreateUserForm|UserForm'Length of output: 92
Script:
#!/bin/bash # Find both component definitions and their types echo "=== Searching for UserForm ===" rg "UserForm" -A 5 -B 5 echo -e "\n=== Searching for CreateUserForm ===" rg "CreateUserForm" -A 5 -B 5 echo -e "\n=== Looking for type definitions ===" ast-grep --pattern 'type $name = { $$$ }' ast-grep --pattern 'export const $name: FC<{ $$$ }>'Length of output: 12532
src/components/Users/UserSummary.tsx (1)
103-108: LGTM! Clean integration of EditUserSheet.The component integration is well-structured and maintains proper prop passing.
src/pages/Organization/components/EditUserRoleSheet.tsx (1)
124-125: LGTM! Good permission handling.The use of useAuthUser and editUserPermissions follows best practices for permission management.
src/common/constants.tsx (1)
107-109: LGTM! Well-implemented constant definition.The implementation follows best practices by:
- Maintaining type safety with proper TypeScript types
- Reusing the existing
GENDER_TYPESconstant- Following the established pattern in the codebase
src/components/Patient/PatientRegistration.tsx (1)
48-48: LGTM! Good refactoring to use centralized constant.The change improves maintainability by using the centralized
GENDERSconstant instead of a local definition.public/locale/en.json (1)
352-352: LGTM! Well-structured localization additions.The new localization keys follow consistent naming patterns and provide clear, descriptive messages for user management functionality.
Also applies to: 702-702, 830-830, 2114-2114, 2152-2152
| if (isEditMode) { | ||
| updateUser({ | ||
| ...data, | ||
| } as UpdateUserModel); | ||
| } else { |
There was a problem hiding this comment.
Ensure correct data is sent in the update request
When updating a user, all fields from data are being sent. If the API expects only changed fields, consider sending only the modified fields to prevent unintended data overwrites.
Consider using diff or selectively including fields:
const updatedFields = { /* include only the fields that have changed */ };
updateUser(updatedFields as UpdateUserModel);
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist