User Management Redesign and User Details Page #9080
Conversation
- Creating a separate user detail page (for user management) Pt 1
- Remove role editing from role management component - Use passable formData for formValidation - Minor customizations for role management issues - Only show role management options for pages of users with Roles: Doctor and Nurse
- Import the form into UserInformationTab (removing the current form) - Moved out the form components into its own file - Import the form into UserAdd
- Modified to use a new route instead - Added a new route to Backend rather than changing permissions for UserModel - Display view details button based on permissions
- Moving out additional common functionality inyo UserListAndCard - Removed memoization since it was causing hook errors - Switched back to using getFacilityUsers route in FacilityUsers as that should be faster
- password reset form fix - use passed in username than authUser's username - UserAddEditForm - pass in facility Ids than full facilities in handleSubmit
- Added error message notifications for the form submits
WalkthroughThe pull request implements extensive modifications across various components primarily focused on user management and localization. Key changes include the enhancement of the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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: 37
🧹 Outside diff range and nitpick comments (67)
src/Routers/routes/UserRoutes.tsx (1)
11-17: Consider deprecating the "/user/profile" route.The PR summary mentions plans to remove/redirect the existing profile page. Since you've introduced a new user details page at
/users/:username, consider:
- Adding a deprecation notice for
/user/profile- Implementing a redirect from
/user/profileto the new user details pageExample implementation:
"/user/profile": () => { console.warn('Deprecated: Use /users/:username instead'); const username = getCurrentUser()?.username; return username ? <Navigate to={`/users/${username}`} replace /> : <UserProfile />; },src/components/Common/UserColumns.tsx (3)
3-6: Follow TypeScript naming conventions for type definitions.The type name
userChildPropsshould be in PascalCase as per TypeScript conventions.-export type userChildProps = { +export type UserChildProps = { userData: UserModel; username: string; };
8-13: Improve component type safety and structure.Consider these improvements:
- Use PascalCase for component name
- Use React.FC for better type safety
- Group props into a single interface
+interface UserColumnsProps { + heading: string; + note: string; + Child: (childProps: UserChildProps) => JSX.Element | undefined; + childProps: UserChildProps; +} -export default function userColumns( - heading: string, - note: string, - Child: (childProps: userChildProps) => JSX.Element | undefined, - childProps: userChildProps, -) { +const UserColumns: React.FC<UserColumnsProps> = ({ + heading, + note, + Child, + childProps, +}) => {
1-29: Well-structured component that aligns with PR objectives.The component provides a consistent and reusable layout pattern for user management sections, which aligns well with the PR's goal of improving user management UI. The split-column design effectively organizes content, with clear separation between section headers and content.
Consider documenting usage examples in comments or creating a Storybook story to showcase different use cases of this reusable component.
src/components/Users/LinkedFacilitiesTab.tsx (2)
7-10: Consider using interface for better documentation.Consider using an
interfaceinstead oftypefor Props to allow for better documentation and extensibility.-type Props = { +interface Props { userData: UserModel; username: string; -}; +}
12-32: Consider adding error boundary and loading state.The component could benefit from error handling and loading states to improve user experience.
Consider wrapping the component with an error boundary and adding a loading state:
export default function LinkedFacilitiesTab(props: Props) { const { userData } = props; const { t } = useTranslation(); if (!userData) { return null; } return ( <ErrorBoundary fallback={<div>{t("error_message")}</div>}> <Suspense fallback={<LoadingSpinner />}> <div className="mt-10 flex flex-col gap-y-12"> {userColumns( t("linked_facilities"), t("linked_facilities_note"), LinkedFacilities, props, )} </div> </Suspense> </ErrorBoundary> ); }src/components/Users/UnlinkSkillDialog.tsx (2)
32-38: Enhance accessibility and error handling.While the translation implementation is good, consider these improvements:
- Add aria-label for better screen reader support
- Add error handling for undefined skillName
- <div className="flex leading-relaxed text-secondary-800"> + <div + className="flex leading-relaxed text-secondary-800" + role="alert" + aria-label={t("unlink_skill_confirmation_message")} + > <span> - {t("unlink_skill_confirm")} <strong>{props.skillName}</strong>{" "} + {t("unlink_skill_confirm")} <strong>{props.skillName || t("unknown_skill")}</strong>{" "} {t("from_user")} <strong>{props.userName}</strong>?{" "} {t("unlink_skill_access")} </span> </div>
24-25: Consider using template literals for translation keys.The action prop is hardcoded while the title uses translation. Consider using translation for both for consistency.
- action="Unlink" + action={t("unlink_action")} title={t("unlink_skill")}src/components/Users/RoleAndSkillsTab.tsx (2)
13-16: Add component documentation.Consider adding JSDoc documentation to describe the component's purpose, props, and usage example.
+/** + * RoleAndSkillsTab displays user qualifications and linked skills based on user type. + * Qualifications are only shown for Doctors and Nurses. + * @param {Props} props - Component props + * @param {UserModel} props.userData - User data containing user type and other details + * @param {string} props.username - Username of the user + */ export default function RoleAndSkillsTab(props: Props) {
17-19: Improve guard clause implementation.The current implementation silently returns undefined. Consider returning null explicitly or implementing an error boundary for better debugging and maintainability.
if (!userData || !username) { - return; + return null; // Explicit return for better readability }src/components/Common/SkillSelect.tsx (3)
4-4: Consider moving SkillModel to a shared location.Since
SkillSelectis in the Common components directory but importsSkillModelfrom the Users module, this could create tight coupling or potential circular dependencies. Consider moving the model to a shared location (e.g.,@/modelsor@/types).
Line range hint
54-59: Add type safety to optionLabel callback.The
optionLabelcallback usesanytype which bypasses TypeScript's type checking. This could lead to runtime errors if the option object structure changes.Apply this fix:
- optionLabel={(option: any) => + optionLabel={(option: SkillModel) => option.name + (option.district_object ? `, ${option.district_object.name}` : "") }
Line range hint
35-47: Add error handling for API response.The
skillSearchcallback assumes the API response will always have aresultsproperty. Consider adding error handling to gracefully handle API failures or unexpected response formats.Consider this improvement:
const skillSearch = useCallback( async (text: string) => { const query = { limit: 50, offset: 0, search_text: text, all: searchAll, }; - const { data } = await request(routes.getAllSkills, { query }); - return data?.results.filter( - (skill) => - !userSkills?.some( - (userSkill) => userSkill.skill_object.id === skill.id, - ), - ); + try { + const { data } = await request(routes.getAllSkills, { query }); + return data?.results?.filter( + (skill) => + !userSkills?.some( + (userSkill) => userSkill.skill_object.id === skill.id, + ), + ) ?? []; + } catch (error) { + console.error('Failed to fetch skills:', error); + return []; + } }, [searchAll, userSkills], );src/components/Common/Page.tsx (1)
20-20: Add JSDoc documentation for the new prop.Please add documentation for the
hideTitleOnPageprop to maintain consistency with other props (likecollapseSidebar) and improve maintainability.+ /** + * If true, hides the title on the page while preserving other PageTitle functionality. + * @default false + */ hideTitleOnPage?: boolean;src/components/Users/ConfirmFacilityModal.tsx (3)
6-20: Consider improving type safety and maintainability.
- Extract the props interface for better reusability and documentation.
- Use a union type for the
typeprop instead of string to restrict valid values.+type FacilityActionType = "unlink_facility" | "clear_home_facility" | "replace_home_facility"; + +interface ConfirmFacilityModalProps { + username: string; + currentFacility?: FacilityModel; + homeFacility?: FacilityModel; + handleCancel: () => void; + handleOk: () => void; + type: FacilityActionType; +} + -const ConfirmFacilityModal = ({ - username, - currentFacility, - homeFacility, - handleCancel, - handleOk, - type, -}: { - username: string; - currentFacility?: FacilityModel; - homeFacility?: FacilityModel; - handleCancel: () => void; - handleOk: () => void; - type: string; -}) => { +const ConfirmFacilityModal = ({ + username, + currentFacility, + homeFacility, + handleCancel, + handleOk, + type, +}: ConfirmFacilityModalProps) => {
21-64: Improve code organization and consistency.
- The switch statement could be simplified by extracting the modal configurations.
- The body content structure is inconsistent (some cases have extra div wrappers).
+interface ModalConfig { + action: string; + body: JSX.Element; +} + +const getModalConfig = ( + type: FacilityActionType, + t: (key: string) => string, + username: string, + currentFacility?: FacilityModel, + homeFacility?: FacilityModel +): ModalConfig => { + const configs: Record<FacilityActionType, ModalConfig> = { + unlink_facility: { + action: "Unlink", + body: ( + <div> + {t("unlink_facility_confirm")}{" "} + <strong>{currentFacility?.name}</strong> {t("from_user")}{" "} + <strong>{username}</strong> + <br /> + {t("unlink_facility_access")} + </div> + ) + }, + clear_home_facility: { + action: "Clear", + body: ( + <div> + {t("clear_home_facility_confirm")}{" "} + <strong>{currentFacility?.name}</strong> {t("from_user")}{" "} + <strong>{username}</strong> + </div> + ) + }, + replace_home_facility: { + action: "Replace", + body: ( + <div> + {t("replace_home_facility_confirm")}{" "} + <strong>{homeFacility?.name}</strong> {t("with")}{" "} + <strong>{currentFacility?.name}</strong>{" "} + {t("replace_home_facility_confirm_as")} <strong>{username}</strong>? + </div> + ) + } + }; + return configs[type]; +};
65-76: Consider improving modal control and presentation.
- The
showprop could be controlled externally for better flexibility.- The danger variant might not be appropriate for all actions (e.g., replace_home_facility).
- The extra div wrapper around the body content is redundant.
- show={true} + show={show} - variant="danger" + variant={type === "unlink_facility" ? "danger" : "warning"} - <div className="flex leading-relaxed text-secondary-800">{body}</div> + {body}src/components/Users/UserSummary.tsx (2)
21-28: Enhance error handling for undefined userDataThe early return for undefined userData could be improved to provide better user feedback.
Consider this enhancement:
if (!userData) { - return; + return ( + <div className="flex justify-center p-4"> + {t("user_data_not_found")} + </div> + ); }
84-93: Improve button implementationThe button implementation can be enhanced for better performance and accessibility.
Consider these improvements:
<ButtonV2 - authorizeFor={() => deletePermitted} + authorizeFor={deletePermitted} onClick={() => setshowDeleteDialog(true)} variant="danger" data-testid="user-delete-button" - className="my-1 inline-flex" + className="my-1 inline-flex items-center gap-2" + aria-label={t("delete_account_aria_label")} > <CareIcon icon="l-trash" className="h-4" /> - <span className="">{t("delete_account_btn")}</span> + <span>{t("delete_account_btn")}</span> </ButtonV2>src/components/Users/UserInformation.tsx (2)
66-77: Add confirmation dialog and loading state for avatar deletion.The avatar deletion could benefit from a confirmation dialog and loading state to prevent accidental deletions and provide better user feedback.
const handleAvatarDelete = async (onError: () => void) => { + const confirmed = window.confirm(t("confirm_delete_avatar")); + if (!confirmed) return; + + setIsDeleting(true); try { const { res } = await request(routes.deleteProfilePicture, { pathParams: { username }, }); if (res?.ok) { Notification.Success({ msg: "Profile picture deleted" }); await refetchUserData(); setEditAvatar(false); } else { onError(); } } catch (error) { onError(); } finally { + setIsDeleting(false); } };
79-116: Enhance accessibility with ARIA labels and roles.While the component structure is good, it could benefit from improved accessibility.
<div className="overflow-visible rounded-lg bg-white px-4 py-5 shadow sm:rounded-lg sm:px-6"> - <div className="my-4 flex justify-between"> + <div className="my-4 flex justify-between" role="region" aria-label={t("profile_section")}> <div className="flex items-center"> <Avatar imageUrl={userData?.read_profile_picture_url} name={formatDisplayName(userData)} className="h-20 w-20" + aria-label={t("profile_picture")} /> <div className="my-4 ml-4 flex flex-col gap-2"> <ButtonV2 onClick={(_) => setEditAvatar(!editAvatar)} type="button" id="edit-cancel-profile-button" className="border border-gray-200 bg-gray-50 text-black hover:bg-gray-100" shadow={false} + aria-label={t("change_avatar_button")} >src/components/Form/Form.tsx (3)
36-36: LGTM! Consider adding JSDoc documentation.The new optional
resetFormValsprop is well-typed and appropriately positioned. Consider adding JSDoc documentation to explain its purpose and behavior.+/** Whether to reset form values to initial state on cancel. */ resetFormVals?: boolean;
83-88: LGTM! Consider adding error handling.The cancel handler implementation is clean and correctly implements the reset functionality. However, consider wrapping the state reset in a try-catch block to handle potential dispatch errors.
const handleCancel = () => { if (props.resetFormVals) { + try { dispatch({ type: "set_form", form: formVals.current }); + } catch (error) { + console.error('Failed to reset form:', error); + Notification.Error({ msg: "Failed to reset form" }); + } } props.onCancel?.(); };
Line range hint
36-88: Consider form reset feature documentation and testing.The form reset feature is a good addition that aligns with the user management redesign objectives. To ensure proper usage across the application:
- Document the reset behavior in the component's README or documentation
- Add unit tests covering the reset functionality
- Consider adding a FormProvider context property to expose the reset capability programmatically
Would you like me to help with generating the documentation or test cases?
src/components/Users/UserHome.tsx (5)
23-26: Consider improving the tabChildProp interface naming and documentation.The interface could be more descriptive and include documentation:
-export interface tabChildProp { +export interface TabContentProps { + /** Component to render the tab content */ body: (childProps: userChildProps) => JSX.Element | undefined; + /** Optional custom name for the tab. If not provided, the tab key is used */ name?: string; }
46-49: Simplify role visibility check and improve naming.The function can be simplified and renamed to better reflect its purpose.
-const roleInfoBeVisible = () => { - if (["Doctor", "Nurse"].includes(userData?.user_type ?? "")) return true; - return false; -}; +const hasQualifications = () => ["Doctor", "Nurse"].includes(userData?.user_type ?? "");
51-62: Improve tab configuration maintainability and type safety.Consider extracting the tab configuration to a separate constant and improving type safety:
+export const enum TabKeys { + PROFILE = 'PROFILE', + SKILLS = 'SKILLS', + FACILITIES = 'FACILITIES' +} +type TabConfig = Record<TabKeys, TabContentProps>; -const TABS: { - PROFILE: tabChildProp; - SKILLS: tabChildProp; - FACILITIES: tabChildProp; -} = { +const TABS: TabConfig = { [TabKeys.PROFILE]: { body: UserSummaryTab }, [TabKeys.SKILLS]: { body: RoleAndSkillsTab, - name: roleInfoBeVisible() ? "QUALIFICATIONS_SKILLS" : "SKILLS", + name: hasQualifications() ? "QUALIFICATIONS_SKILLS" : "SKILLS", }, [TabKeys.FACILITIES]: { body: LinkedFacilitiesTab }, };
94-117: Enhance navigation accessibility and loading state.Consider improving the tab navigation accessibility and loading experience:
- Add ARIA roles and states to the tab navigation
- Consider using a skeleton loader instead of a simple loading spinner
<nav className="flex space-x-6 overflow-x-auto" id="usermanagement_tab_nav" + role="tablist" + aria-label={t("user_management_navigation")} > {keysOf(TABS).map((p) => { const tabName = TABS[p]?.name ?? p; return ( <Link key={p} className={classNames(/*...*/)} href={`/users/${username}/${p.toLocaleLowerCase()}`} + role="tab" + aria-selected={currentTab === p} + aria-controls={`${p.toLowerCase()}-panel`} >
28-127: Add unit tests for the UserHome component.To ensure reliability and meet PR objectives, consider adding tests for:
- Tab navigation and rendering
- User data loading and error states
- Role-based visibility logic
- Accessibility features
Would you like me to help create a test suite for this component?
src/components/Users/UserBanner.tsx (2)
19-25: Consider memoizing the online status calculation.The
isUserOnlinecheck runs on every render due to the useEffect. Consider memoizing this value usinguseMemoto optimize performance, especially ifisUserOnlineis computationally expensive.-const [userOnline, setUserOnline] = useState(false); +const userOnline = useMemo(() => userData ? isUserOnline(userData) : false, [userData]); -useEffect(() => { - if (!userData) return; - setUserOnline(isUserOnline(userData)); -}, [userData]);
116-124: Enhance experience calculation robustness.The current experience calculation doesn't handle edge cases like future dates. Consider extracting this logic into a utility function with proper validation.
+// Add to utils.ts +export const calculateExperienceYears = (startDate: string): number => { + const start = dayjs(startDate); + if (!start.isValid() || start.isAfter(dayjs())) { + return 0; + } + return dayjs().diff(start, "years", false); +}; // In component -{dayjs().diff( - userData.doctor_experience_commenced_on, - "years", - false, -)}{" "} +{calculateExperienceYears(userData.doctor_experience_commenced_on)}{" "}src/components/Users/UserResetPassword.tsx (2)
40-50: Enhance password validation security.While the current validation covers basic requirements, consider strengthening it by:
- Adding special character requirement
- Making the validation function more configurable
- Adding maximum length validation
Example implementation:
-const validateNewPassword = (password: string) => { +const validateNewPassword = (password: string, options = { + minLength: 8, + maxLength: 128, + requireSpecialChar: true +}) => { if ( - password.length < 8 || + password.length < options.minLength || + password.length > options.maxLength || !/\d/.test(password) || password === password.toUpperCase() || - password === password.toLowerCase() + password === password.toLowerCase() || + (options.requireSpecialChar && !/[!@#$%^&*(),.?":{}|<>]/.test(password)) ) { return false; } return true; };
98-192: Enhance form accessibility and validation UX.
- Add proper ARIA attributes for better screen reader support
- Show validation messages consistently, not just on focus
- Consider extracting validation rules to a configuration object
Example improvements:
-<form action="#" method="POST"> +<form + action="#" + method="POST" + aria-label="Password reset form" + noValidate +> <div className="grid grid-cols-6 gap-4"> <TextFormField name="old_password" label={t("current_password")} className="col-span-6 sm:col-span-3" type="password" value={changePasswordForm.old_password} onChange={(e) => setChangePasswordForm({ ...changePasswordForm, old_password: e.value, })} error={changePasswordErrors.old_password} + aria-required="true" + aria-invalid={!!changePasswordErrors.old_password} + aria-describedby="old-password-error" required /> + {changePasswordErrors.old_password && ( + <div id="old-password-error" className="text-error-600"> + {changePasswordErrors.old_password} + </div> + )} </div> - <div className="text-small mb-2 hidden pl-2 text-secondary-500 peer-focus-within:block"> + <div className="text-small mb-2 pl-2 text-secondary-500" + role="alert" + aria-live="polite"> {/* ... validation messages ... */} </div> </form>src/components/ABDM/LinkAbhaNumber/CreateWithAadhaar.tsx (2)
Line range hint
466-484: Refactor validation logic to improve maintainability.The ABHA address validation regex pattern is duplicated. Consider:
- Extracting the regex pattern to a constant
- Creating a reusable validation function
Apply this refactor:
+const ABHA_ADDRESS_REGEX = /^(?![\d.])[a-zA-Z0-9._]{4,}(?<!\.)$/; + +const validateAbhaAddress = (healthId: string) => ABHA_ADDRESS_REGEX.test(healthId); + function ChooseAbhaAddress({ // ... <ButtonV2 className="w-full" - disabled={ - memory?.isLoading || - !/^(?![\d.])[a-zA-Z0-9._]{4,}(?<!\.)$/.test(healthId) - } + disabled={memory?.isLoading || !validateAbhaAddress(healthId)} onClick={handleSubmit} >Also applies to: 516-519
Line range hint
252-269: Enhance error handling for better user experience.The error handling for API calls could be more robust. Consider:
- Adding specific error messages for different failure scenarios
- Handling session expiry gracefully
- Managing error states more effectively in the form state
Apply this enhancement:
const handleResendOtp = async () => { setMemory((prev) => ({ ...prev, isLoading: true })); + try { const { res, data } = await request( routes.abdm.healthId.abhaCreateLinkMobileNumber, { body: { mobile: memory?.mobileNumber.replace("+91", "").replace(/ /g, ""), transaction_id: memory?.transactionId, }, }, ); if (res?.status === 200 && data) { setMemory((prev) => ({ ...prev, transactionId: data.transaction_id, resendOtpCount: prev.resendOtpCount + 1, })); Notify.Success({ msg: data.detail ?? t("mobile_otp_send_success"), }); - } else { + } else if (res?.status === 401) { + Notify.Error({ + msg: t("session_expired_please_retry"), + }); + // Handle session expiry + } else if (res?.status === 429) { + Notify.Error({ + msg: t("too_many_attempts_please_wait"), + }); + // Handle rate limiting + } else { setMemory((prev) => ({ ...prev, resendOtpCount: Infinity, })); - Notify.Success({ + Notify.Error({ msg: t("mobile_otp_send_error"), }); } + } catch (error) { + Notify.Error({ + msg: t("network_error_please_retry"), + }); + } finally { setMemory((prev) => ({ ...prev, isLoading: false })); + } };Also applies to: 359-376
src/components/Users/UserProfile.tsx (4)
Line range hint
279-291: Internationalize password validation messagesThe password validation messages should use the translation function (t) to support multiple languages, as mentioned in the PR objectives for internationalization of UI text.
const rules = [ { test: (p: string) => p.length >= 8, - message: "Password should be at least 8 characters long", + message: t("password_min_length_message"), }, { test: (p: string) => p !== p.toUpperCase(), - message: "Password should contain at least 1 lowercase letter", + message: t("password_lowercase_message"), }, { test: (p: string) => p !== p.toLowerCase(), - message: "Password should contain at least 1 uppercase letter", + message: t("password_uppercase_message"), }, { test: (p: string) => /\d/.test(p), - message: "Password should contain at least 1 number", + message: t("password_number_message"), }, ];
Line range hint
293-397: Enhance form validation implementationSeveral improvements can be made to the validation logic:
- Extract magic numbers into named constants
- Internationalize error messages
- Consider extracting field-specific validation logic into separate functions for better maintainability
+ const MIN_AGE = 17; + const MAX_EXPERIENCE_YEARS = 100; + const MAX_WEEKLY_HOURS = 168; const validateForm = () => { const errors = { ...initError }; let invalidForm = false; Object.keys(states.form).forEach((field) => { switch (field) { case "date_of_birth": if (!states.form[field]) { - errors[field] = "Enter a valid date of birth"; + errors[field] = t("enter_valid_dob"); invalidForm = true; } else if ( !dayjs(states.form[field]).isValid() || - dayjs(states.form[field]).isAfter(dayjs().subtract(17, "year")) + dayjs(states.form[field]).isAfter(dayjs().subtract(MIN_AGE, "year")) ) { - errors[field] = "Enter a valid date of birth"; + errors[field] = t("enter_valid_dob"); invalidForm = true; } return; // Similar changes for other validation messages
Line range hint
467-503: Enhance form submission error handling and internationalizationThe form submission handler needs improvements:
- Add error handling for date parsing
- Internationalize success message
- Consider adding loading state during submission
const handleSubmit = async (e: FormEvent) => { e.preventDefault(); const validForm = validateForm(); if (validForm) { + try { + setIsSubmitting(true); const data = { // ... other fields ... doctor_experience_commenced_on: states.form.user_type === "Doctor" ? dayjs() .subtract( parseInt( (states.form.doctor_experience_commenced_on as string) ?? "0", ), "years", ) .format("YYYY-MM-DD") : undefined, }; const { res } = await request(routes.partialUpdateUser, { pathParams: { username: authUser.username }, body: data, }); if (res?.ok) { Notification.Success({ - msg: "Details updated successfully", + msg: t("details_updated_successfully"), }); await refetchUserData(); setShowEdit(false); } + } catch (error) { + Notification.Error({ + msg: t("error_updating_details"), + }); + } finally { + setIsSubmitting(false); + } } };
Line range hint
544-586: Improve password change handler implementationThe password change handler needs several improvements:
- Add proper TypeScript typing for the event parameter
- Internationalize error messages
- Add loading state during submission
- Consider using a more structured form validation approach
- const changePassword = async (e: any) => { + const changePassword = async (e: FormEvent) => { e.preventDefault(); + try { + setIsChangingPassword(true); if ( changePasswordForm.new_password_1 !== changePasswordForm.new_password_2 ) { Notification.Error({ - msg: "Passwords are different in new password and confirmation password column.", + msg: t("password_mismatch_error"), }); } else if (!validateNewPassword(changePasswordForm.new_password_1)) { Notification.Error({ - msg: "Entered New Password is not valid, please check!", + msg: t("invalid_password_error"), }); } else if ( changePasswordForm.new_password_1 === changePasswordForm.old_password ) { Notification.Error({ - msg: "New password is same as old password, Please enter a different new password.", + msg: t("same_password_error"), }); } else { const form: UpdatePasswordForm = { old_password: changePasswordForm.old_password, username: authUser.username, new_password: changePasswordForm.new_password_1, }; const { res, data, error } = await request(routes.updatePassword, { body: form, }); if (res?.ok) { Notification.Success({ msg: data?.message }); } else if (!error) { Notification.Error({ - msg: "There was some error. Please try again in some time.", + msg: t("generic_error_message"), }); } setChangePasswordForm({ ...changePasswordForm, new_password_1: "", new_password_2: "", old_password: "", }); } + } catch (error) { + Notification.Error({ + msg: t("password_change_error"), + }); + } finally { + setIsChangingPassword(false); + } };public/locale/en.json (1)
658-660: Consider enhancing the username validation message.While most validation messages are clear, the username validation message could be more specific about allowed special characters.
Consider updating the message to be more explicit:
- "invalid_username": "Required. 150 characters or fewer. Letters, digits and @/./+/-/_ only.", + "invalid_username": "Required. 150 characters or fewer. Only lowercase letters, numbers, and the special characters @ . + - _ are allowed.",Also applies to: 709-709, 818-818, 1322-1322
src/components/Users/UserAdd.tsx (4)
Line range hint
16-19: Includerel="noopener noreferrer"when usingtarget="_blankto prevent security risks.When using
target="_blank``, addingrel="noopener noreferrer"prevents the new page from accessing thewindow.opener` property, which can pose a security risk. This ensures that the external page cannot manipulate the original page.Apply this diff to fix the issue:
href="https://school.ohc.network/targets/12953" className="inline-block rounded border border-secondary-600 bg-secondary-50 px-4 py-2 text-secondary-600 transition hover:bg-secondary-100" - target="_blank" + target="_blank" rel="noopener noreferrer" >
Line range hint
20-21: Internationalize the 'Need Help?' text.The text
'Need Help?'is currently hardcoded and not wrapped in the translation functiont(). To ensure consistent internationalization across the application, please use the translation function so that the text can be translated into other languages.Apply this diff to fix the issue:
- <CareIcon icon="l-question-circle" className="text-lg" /> Need - Help? + <CareIcon icon="l-question-circle" className="text-lg" /> {t("Need Help?")}
Line range hint
15-23: Use an<a>element instead ofLinkfor external URLs.The
Linkcomponent fromravigeris intended for internal routing within the application. When linking to external URLs, it's appropriate to use an<a>element to ensure proper behavior and accessibility.Apply this diff to fix the issue:
- <Link + <a href="https://school.ohc.network/targets/12953" className="inline-block rounded border border-secondary-600 bg-secondary-50 px-4 py-2 text-secondary-600 transition hover:bg-secondary-100" target="_blank" rel="noopener noreferrer" - > + > <CareIcon icon="l-question-circle" className="text-lg" /> {t("Need Help?")} - </Link> + </a>
1-1: Remove unused import ofLinkfromraviger.After replacing the
Linkcomponent with an<a>element, the import statement is no longer needed and can be removed to keep the code clean.Apply this diff to fix the issue:
-import { Link } from "raviger";src/components/Facility/FacilityUsers.tsx (2)
28-41: Ensure robust handling of pagination parametersThe calculation of
offsetdepends onqParams.page. IfqParams.pageis undefined or invalid, it could lead to incorrect offsets. Consider providing a default value to enhance robustness.Apply this diff to provide a default value for
qParams.page:- (qParams.page ? qParams.page - 1 : 0) * resultsPerPage + ((qParams.page ?? 1) - 1) * resultsPerPage
49-55: Consider updating the icon inCountBlockfor clarityThe icon
"l-user-injured"may not best represent the total number of users. Using an icon like"l-users"might provide better clarity to users.Apply this diff to change the icon:
- icon="l-user-injured" + icon="l-users"src/components/Users/LinkedSkills.tsx (1)
130-141: Avoid passing empty strings toerrorspropThe
errorsprop is passed as an empty string toSkillSelect. If there are no errors, it's better to passundefinedor omit the prop entirely to prevent unnecessary rendering or checks within the child component.Apply this diff:
<SkillSelect id="select-skill" multiple={false} name="skill" disabled={!authorizeForAddSkill} showNOptions={Infinity} selected={selectedSkill} setSelected={setSelectedSkill} - errors="" + errors={undefined} className="z-10 w-full" userSkills={skills?.results || []} />src/components/Users/UserQualifications.tsx (3)
179-180: Localize notification messagesThe success message is hardcoded and not localized. To support internationalization, wrap the message with the
t()function.- msg: "Details updated successfully", + msg: t("details_updated_successfully"),
220-223: Add maximum value constraint to the years of experience fieldTo prevent users from entering an invalid number of years of experience, consider adding a
maxattribute to the input field.type="number" min={0} + max={99} label={t("years_of_experience")}
88-91: Consider storing and handling experience dates directlyCalculating dates by subtracting years from the current date might lead to inaccuracies over time. It's more reliable to store and handle
doctor_experience_commenced_onas a date rather than calculating it from years of experience.Also applies to: 157-167
src/components/Users/LinkedFacilities.tsx (2)
164-164: Remove commented-out code to improve code cleanlinessThe lines
//setIsLoading(true);and//setIsLoading(false);are commented out. If these lines are no longer needed, consider removing them to keep the codebase clean. If they are placeholders for future functionality, add a TODO comment explaining their purpose.Also applies to: 180-180
184-218: Refactor render functions to eliminate code duplicationThe functions
renderFacilityButtonsandrenderHomeFacilityButtonhave similar structures. Refactoring them into a single function with conditional logic can reduce code duplication and enhance maintainability.Consider combining them as follows:
const renderFacilityButton = (facility: FacilityModel, isHomeFacility: boolean) => { if (!facility) return null; return ( <div id={`facility_${facility.id}`} key={`facility_${facility.id}`}> <div className="flex flex-row items-center rounded-sm border bg-secondary-100"> <div className="rounded p-1 text-sm">{facility.name}</div> <div className="border-l-3 rounded-r bg-secondary-300 px-2 py-1"> {isHomeFacility ? ( <button onClick={() => handleOnClick("clear_home_facility", facility)} title={t("clear_home_facility")} aria-label={t("clear_home_facility")} > <CareIcon icon="l-multiply" className="text-sm" /> </button> ) : ( <DropdownMenu> <DropdownMenuTrigger> <CareIcon icon="l-setting" className="text-sm" /> </DropdownMenuTrigger> <DropdownMenuContent> <DropdownMenuItem onClick={() => handleOnClick( homeFacility ? "replace_home_facility" : "set_home_facility", facility, ) } > {t("set_home_facility")} </DropdownMenuItem> <DropdownMenuItem onClick={() => handleOnClick("unlink_facility", facility)} > {t("unlink_this_facility")} </DropdownMenuItem> </DropdownMenuContent> </DropdownMenu> )} </div> </div> </div> ); };Also applies to: 220-240
src/components/Users/UserListAndCard.tsx (6)
268-272: Simplify conditional rendering for district information.The conditional logic for displaying district information in
UserListRowcan be simplified to improve readability.You can simplify it as follows:
- {"district_object" in user && user.district_object - ? user.district_object?.name - : "district" in user && user.district - ? user.district - : ""} + {user.district_object?.name || user.district || ""}
180-193: Simplify conditional rendering for district information.The conditional logic for displaying district information in
UserCardcan be simplified for better readability.You can simplify it as follows:
- {"district_object" in user && user.district_object && ( - <div className="text-sm"> - <div className="text-gray-500">{t("district")}</div> - <div className="font-medium"> - {user.district_object.name} - </div> - </div> - )} - {"district" in user && user.district && ( - <div className="text-sm"> - <div className="text-gray-500">{t("district")}</div> - <div className="font-medium">{user.district}</div> - </div> - )} + {(user.district_object?.name || user.district) && ( + <div className="text-sm"> + <div className="text-gray-500">{t("district")}</div> + <div className="font-medium"> + {user.district_object?.name || user.district} + </div> + </div> + )}
162-162: Use full user name in Avatar component for better display.Currently, the
nameprop for theAvatarcomponent is set touser.usernameon line 162. Consider using the user's full name for a better user experience.Apply this diff:
- name={user.username ?? ""} + name={formatName(user)}
249-249: Use full user name in Avatar component for better display.Currently, the
nameprop for theAvatarcomponent is set touser.usernameon line 249. Consider using the user's full name for a better user experience.Apply this diff:
- name={user.username ?? ""} + name={formatName(user)}
326-326: Localize static text 'Card' for internationalization.The text 'Card' on line 326 is not wrapped with the translation function
t. This may lead to missing translations in other languages.Apply this diff:
- <span>Card</span> + <span>{t("card")}</span>
336-336: Localize static text 'List' for internationalization.The text 'List' on line 336 is not wrapped with the translation function
t. This may lead to missing translations in other languages.Apply this diff:
- <span>List</span> + <span>{t("list")}</span>src/components/Users/ManageUsers.tsx (2)
Line range hint
109-127: Simplify component rendering by eliminating unnecessary variableThe use of the
manageUsersvariable adds unnecessary complexity. Consider rendering the user list components directly within the return statement to improve readability and maintainability.You can refactor the code as follows:
- let manageUsers: JSX.Element = <></>; - if (userListLoading || districtDataLoading || !userListData?.results) { - return <Loading />; - } - if (userListData?.results.length) { - manageUsers = ( - <div> - <UserListView - users={userListData?.results ?? []} - onSearch={(username) => updateQuery({ username })} - searchValue={qParams.username} - /> - <Pagination totalCount={userListData.count} /> - </div> - ); - } else if (userListData?.results && userListData?.results.length === 0) { - manageUsers = ( - <div> - <div className="h-full space-y-2 rounded-lg bg-white p-7 shadow"> - <div className="flex w-full items-center justify-center text-xl font-bold text-secondary-500"> - No Users Found - </div> - </div> - </div> - ); - } - return ( - <Page title={t("user_management")} hideBack={true} breadcrumbs={false}> - {/* ...other components... */} - <div>{manageUsers}</div> - </Page> - ); + if (userListLoading || districtDataLoading || !userListData?.results) { + return <Loading />; + } + return ( + <Page title={t("user_management")} hideBack={true} breadcrumbs={false}> + {/* ...other components... */} + {userListData?.results.length ? ( + <div> + <UserListView + users={userListData?.results ?? []} + onSearch={(username) => updateQuery({ username })} + searchValue={qParams.username} + /> + <Pagination totalCount={userListData.count} /> + </div> + ) : ( + <div> + <div className="h-full space-y-2 rounded-lg bg-white p-7 shadow"> + <div className="flex w-full items-center justify-center text-xl font-bold text-secondary-500"> + {t("no_users_found")} + </div> + </div> + </div> + )} + </Page> + );
Line range hint
124-127: Internationalize the 'No Users Found' messageThe text
'No Users Found'is hardcoded and not internationalized. Use the translation function to support multiple languages and improve accessibility.Apply this diff to internationalize the text:
- No Users Found + {t("no_users_found")}Also, ensure the translation key
'no_users_found'is added to the localization files.src/components/Users/UserAddEditForm.tsx (7)
182-182: Simplify boolean assignment of 'editUser'.Instead of using a ternary operator, you can directly convert
usernameto a boolean.Apply this diff to simplify the assignment:
- const editUser = username ? true : false; + const editUser = !!username;🧰 Tools
🪛 Biome
[error] 182-182: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
305-305: Correct the typo in the error message.The word "availabality" should be corrected to "availability".
Apply this diff to fix the typo:
Notification.Error({ - msg: "Some error checking username availabality. Please try again later.", + msg: "Some error checking username availability. Please try again later.", });
480-480: Fix the typo in the validation message."Atleast" should be "at least" in the error message.
Apply this diff to correct the message:
- return "Please select atleast one of the facilities you are linked to"; + return "Please select at least one of the facilities you are linked to";
883-952: Remove unnecessary fragment around a single child element.The fragment wrapping the
<div>is redundant since it contains only a single child. Removing it simplifies the JSX.Apply this diff to remove the unnecessary fragment:
- <> <div className="text-small pl-2 text-secondary-500"> {/* ... */} </div> - </>🧰 Tools
🪛 Biome
[error] 886-920: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
956-1023: Eliminate redundant fragment wrapping a single element.The fragment here is unnecessary and can be removed to improve code clarity.
Apply this diff:
- <> <div className="flex flex-col justify-between gap-x-3 sm:flex-row"> {/* ... */} </div> - </>🧰 Tools
🪛 Biome
[error] 956-1023: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
1082-1105: Remove unnecessary fragment to simplify JSX structure.Since the fragment contains only one child, it can be omitted for cleaner code.
Apply this diff:
- <> <div className="flex flex-col justify-between gap-x-3 sm:flex-row"> {/* ... */} </div> - </>🧰 Tools
🪛 Biome
[error] 1082-1105: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
883-952: Refactor validation rule displays into a reusable component.The code for displaying username and password validation rules is similar. Extracting this into a separate component reduces duplication and enhances maintainability.
Also applies to: 972-997
🧰 Tools
🪛 Biome
[error] 886-920: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (33)
public/locale/en.json(25 hunks)src/Routers/routes/UserRoutes.tsx(1 hunks)src/Utils/request/api.tsx(1 hunks)src/components/ABDM/LinkAbhaNumber/CreateWithAadhaar.tsx(1 hunks)src/components/Auth/ResetPassword.tsx(1 hunks)src/components/Common/Page.tsx(2 hunks)src/components/Common/PageTitle.tsx(3 hunks)src/components/Common/SkillSelect.tsx(2 hunks)src/components/Common/UserColumns.tsx(1 hunks)src/components/Common/UserDetails.tsx(1 hunks)src/components/Facility/FacilityUsers.tsx(2 hunks)src/components/Form/Form.tsx(5 hunks)src/components/Users/ConfirmFacilityModal.tsx(1 hunks)src/components/Users/ConfirmHomeFacilityUpdateDialog.tsx(0 hunks)src/components/Users/LinkFacilityDialog.tsx(0 hunks)src/components/Users/LinkedFacilities.tsx(1 hunks)src/components/Users/LinkedFacilitiesTab.tsx(1 hunks)src/components/Users/LinkedSkills.tsx(1 hunks)src/components/Users/ManageUsers.tsx(4 hunks)src/components/Users/RoleAndSkillsTab.tsx(1 hunks)src/components/Users/SkillsSlideOver.tsx(0 hunks)src/components/Users/SkillsSlideOverComponents.tsx(0 hunks)src/components/Users/UnlinkSkillDialog.tsx(2 hunks)src/components/Users/UserAdd.tsx(2 hunks)src/components/Users/UserAddEditForm.tsx(1 hunks)src/components/Users/UserBanner.tsx(1 hunks)src/components/Users/UserHome.tsx(1 hunks)src/components/Users/UserInformation.tsx(1 hunks)src/components/Users/UserListAndCard.tsx(1 hunks)src/components/Users/UserProfile.tsx(1 hunks)src/components/Users/UserQualifications.tsx(1 hunks)src/components/Users/UserResetPassword.tsx(1 hunks)src/components/Users/UserSummary.tsx(1 hunks)
💤 Files with no reviewable changes (4)
- src/components/Users/ConfirmHomeFacilityUpdateDialog.tsx
- src/components/Users/LinkFacilityDialog.tsx
- src/components/Users/SkillsSlideOver.tsx
- src/components/Users/SkillsSlideOverComponents.tsx
✅ Files skipped from review due to trivial changes (2)
- src/components/Auth/ResetPassword.tsx
- src/components/Common/UserDetails.tsx
🧰 Additional context used
🪛 Biome
src/components/Users/UserAddEditForm.tsx
[error] 182-182: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
[error] 619-619: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 886-920: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
[error] 956-1023: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
[error] 1082-1105: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
src/components/Users/UserBanner.tsx
[error] 34-133: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
🔇 Additional comments (36)
src/Routers/routes/UserRoutes.tsx (1)
2-3: LGTM! Import changes are well-structured.
The import statements follow React conventions and align with the user management redesign objectives.
src/components/Users/LinkedFacilitiesTab.tsx (1)
1-32: Verify integration with user details page.
The component implementation aligns with PR objectives for user management redesign. However, let's verify its integration with the user details page.
✅ Verification successful
Integration with user details page verified successfully
The LinkedFacilitiesTab component is properly integrated in UserHome.tsx as one of the main tabs (FACILITIES) alongside PROFILE and SKILLS tabs, which aligns with the user management redesign objectives. The implementation and integration are correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how LinkedFacilitiesTab is integrated into the user details page
rg -l "LinkedFacilitiesTab" --type tsx
Length of output: 66
Script:
#!/bin/bash
# Check how LinkedFacilitiesTab is integrated using ripgrep without file type filter
rg -l "LinkedFacilitiesTab"
Length of output: 106
Script:
#!/bin/bash
# Check the integration in UserHome.tsx
rg "LinkedFacilitiesTab" src/components/Users/UserHome.tsx -A 5 -B 5
Length of output: 956
src/components/Users/UnlinkSkillDialog.tsx (1)
15-15: Verify translation key definitions.
Let's ensure all translation keys used in the component are properly defined in the localization files.
Also applies to: 24-38
✅ Verification successful
All translation keys are properly defined in the localization file
All translation keys used in the UnlinkSkillDialog component (unlink_skill, unlink_skill_confirm, from_user, unlink_skill_access) are correctly defined in the English locale file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all translation keys are defined in the English locale file
# Keys to check
KEYS=("unlink_skill" "unlink_skill_confirm" "from_user" "unlink_skill_access")
# Check each key in the locale file
for key in "${KEYS[@]}"; do
echo "Checking key: $key"
if ! rg "\"$key\":" public/locale/en.json; then
echo "Warning: Translation key '$key' not found!"
fi
done
Length of output: 866
src/components/Users/RoleAndSkillsTab.tsx (2)
1-11: LGTM! Well-structured imports and type definitions.
The imports are well-organized and the Props type is properly defined with required fields, which aligns well with the component's guard clause.
21-41: 🛠️ Refactor suggestion
Enhance type safety for user type checks.
The current string literal comparison for user types could be improved using TypeScript enums or constants for better type safety and maintainability.
+// Add to UserModel.ts or a constants file
+export enum UserType {
+ Doctor = "Doctor",
+ Nurse = "Nurse",
+}
return (
<>
<div className="mt-10 flex flex-col gap-y-12">
{userData.user_type &&
- ["Doctor", "Nurse"].includes(userData.user_type) &&
+ Object.values(UserType).includes(userData.user_type as UserType) &&
userColumns(
t("user_qualifications"),
t("user_qualifications_note"),
UserQualifications,
props,
)}Additionally, verify that these user types are consistently used across the application.
src/components/Common/Page.tsx (1)
55-55: Verify the relationship between hideTitleOnPage and hideBack props.
The component already has a hideBack prop that affects padding when the back button is hidden. Please clarify how these props interact and whether they serve distinct purposes.
Let's check the PageTitle component implementation to understand the difference:
✅ Verification successful
The hideBack and hideTitleOnPage props serve distinct purposes
Based on the implementation in PageTitle.tsx:
hideBackis passed to theBreadcrumbscomponent to control the visibility of the back button/navigationhideTitleOnPagecontrols the visibility of the page title (h2element) in the main content area- These props are independent and do not interact with each other
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Analyze the relationship between hideTitleOnPage and hideBack props
# Expected: Find the implementation showing distinct purposes for these props
# Search for the PageTitle component implementation
ast-grep --pattern 'export default function PageTitle($$$) {
$$$
}'
# Search for usage patterns of both props
rg -A 5 'hideTitleOnPage|hideBack' src/components/Common/PageTitle.tsx
Length of output: 5285
src/components/Common/PageTitle.tsx (3)
22-22: LGTM! Well-structured prop addition.
The new prop follows TypeScript and React naming conventions, and its optional nature is appropriate for backward compatibility.
39-39: LGTM! Clean props destructuring.
The prop destructuring is consistent with the interface definition and maintains the logical grouping of display-related props.
75-77: Consider accessibility implications when hiding the title.
While the conditional rendering works correctly, hiding the h2 element might affect the page's heading hierarchy and accessibility. Consider:
- Providing an alternative heading for screen readers when the visible title is hidden
- Ensuring the page maintains a logical heading structure
Consider this alternative implementation that maintains accessibility:
- {!hideTitleOnPage && (
- <h2 className="ml-0 text-2xl leading-tight">{title}</h2>
- )}
+ <h2
+ className={classNames(
+ "ml-0 text-2xl leading-tight",
+ hideTitleOnPage && "sr-only"
+ )}
+ >
+ {title}
+ </h2>This approach keeps the heading in the document structure while visually hiding it when needed.
✅ Verification successful
Based on the search results, I can now generate the final response as I have enough context about the heading structure in the codebase.
Hiding the title is safe from an accessibility perspective
The conditional rendering of the title in PageTitle.tsx is acceptable because:
- The component is used consistently as a secondary heading (h2)
- Other components maintain their own heading hierarchy independently
- The title hiding is likely used in specific UI scenarios where the same information is presented through other means
While the suggested sr-only approach is a good practice, the current implementation doesn't pose significant accessibility concerns as it's a controlled hide/show of a secondary heading that doesn't break document outline.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are other headings that might be affected by hiding this h2
rg -g '*.tsx' -g '*.jsx' '<h[1-6]' --type-add 'tsx:*.tsx' --type-add 'jsx:*.jsx'
Length of output: 22902
src/components/Users/ConfirmFacilityModal.tsx (2)
79-79: LGTM!
The export statement follows React component conventions.
1-79: Overall implementation aligns well with PR objectives.
The component successfully implements the facility management confirmation dialogs needed for the user management redesign. It provides clear user feedback and handles different facility-related actions appropriately.
Some suggestions for improvement have been provided above, but the core functionality is solid and meets the requirements.
Let's verify the translation keys are properly defined:
✅ Verification successful
Translation keys are properly defined and accessible
The verification confirms that all required translation keys used in the ConfirmFacilityModal component are present in the locale file (public/locale/en.json), ensuring proper internationalization support for all facility management actions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all translation keys used in the component exist in the locale file
# Test: Check if all required translation keys exist
rg -q "unlink_facility|unlink_facility_confirm|unlink_facility_access|clear_home_facility_confirm|replace_home_facility_confirm|replace_home_facility_confirm_as|from_user|with" "public/locale/en.json" && echo "All translation keys found" || echo "Missing translation keys"
Length of output: 263
src/components/Users/UserSummary.tsx (2)
1-20: LGTM! Well-organized imports
The imports are properly organized with clear separation between external dependencies and internal modules.
47-48: Verify user permission handling
Let's verify that the permission checks align with the PR objectives.
src/components/Users/UserInformation.tsx (2)
1-19: LGTM! Well-organized imports with clear separation of concerns.
The imports are properly organized and all dependencies are relevant to the component's functionality.
1-117: Well-implemented component that aligns with PR objectives.
The UserInformation component successfully contributes to the user management redesign by providing a clean and functional interface for managing user profiles and avatars. It aligns well with the objectives outlined in Issue #8878.
src/components/Form/Form.tsx (2)
134-134: LGTM! Handler update is correct.
The Cancel button's onClick handler is properly updated to use the new handleCancel function.
47-47: Verify handling of props.defaults changes.
The useRef initialization looks good, but consider how changes to props.defaults after initial render might affect the reset behavior.
Consider using useEffect to update formVals.current when props.defaults changes:
const formVals = useRef(props.defaults);
+useEffect(() => {
+ formVals.current = props.defaults;
+}, [props.defaults]);src/components/Users/UserBanner.tsx (1)
103-113: LGTM: Role-specific information display.
The conditional rendering of qualifications for Doctors and Nurses, along with Doctor-specific experience information, aligns well with the PR objectives for role-specific displays.
Also applies to: 114-129
src/components/Users/UserResetPassword.tsx (2)
1-19: LGTM! Well-structured component setup.
The imports are properly organized and the component interface is well-defined with appropriate TypeScript types.
13-17: Verify user permissions for password changes.
According to the PR objectives, password changes should be restricted to logged-in users and District Admins or higher. However, the current implementation doesn't include these permission checks.
Let's verify the permission implementation:
src/components/ABDM/LinkAbhaNumber/CreateWithAadhaar.tsx (2)
13-13: LGTM! Good refactoring of validation rules.
Moving validateRule to UserAddEditForm promotes code reuse and maintains consistency across user-related forms.
Line range hint 32-38: Consider enhancing security measures for sensitive data handling.
While basic security measures are in place, consider implementing:
- UI masking for verified mobile numbers
- Rate limiting for OTP attempts beyond the current count-based limit
- Session-based validation for transaction IDs
Let's verify the current security measures:
Also applies to: 89-91
src/components/Users/UserProfile.tsx (1)
20-20: LGTM: Import path update is correct
The import path change from UserAdd to UserAddEditForm aligns with the refactoring objectives.
src/Utils/request/api.tsx (1)
1046-1049: LGTM! The endpoint change is consistent with the codebase.
The change to a static endpoint for getUserDetails is appropriate while maintaining other user-specific endpoints that correctly use the username parameter. This change aligns with the PR objectives for user management redesign.
public/locale/en.json (6)
211-214: LGTM! Well-structured tab labels.
The new user management tab labels follow a consistent naming pattern and provide clear, hierarchical navigation.
393-394: LGTM! Clear and context-aware profile management labels.
The labels appropriately distinguish between self-update and admin-update scenarios, and provide helpful guidance for avatar uploads.
Also applies to: 1019-1020
460-460: LGTM! Comprehensive password-related messages.
The messages provide clear validation requirements, user-friendly error messages, and helpful instructions for password management.
Also applies to: 980-981, 1027-1028, 1129-1129
829-829: LGTM! Clear and cautionary facility management messages.
The messages effectively communicate the implications of facility management actions and maintain consistent terminology.
Also applies to: 1110-1112, 1279-1281
831-831: LGTM! Consistent skill management messages.
The messages follow the same clear pattern as facility management, effectively communicating actions and their consequences.
Also applies to: 1282-1284
1356-1356: LGTM! Clear weekly hours validation message.
The message clearly defines the valid range using the logical maximum of 168 hours (24*7).
src/components/Users/UserAdd.tsx (1)
Line range hint 8-29: Code implementation looks good.
The UserAdd component is well-structured and correctly renders the UserAddEditForm within the Page component. The use of useTranslation for the title is appropriate, and the component follows good React practices.
src/components/Facility/FacilityUsers.tsx (4)
13-13: Good use of explicit prop typing
Explicitly typing props as { facilityId: number } enhances type safety and improves code readability.
15-18: Confirm the cacheBlacklist configuration in useFilters
Using cacheBlacklist: ["username"] prevents the username parameter from being cached between navigations. Please verify if this aligns with the intended behavior.
57-61: Verify that onSearch correctly updates the user list
Ensure that the onSearch handler updates the query parameters and triggers a re-fetch of the user list when the search input changes.
63-63: Confirm the functionality of the Pagination component
Check that the Pagination component interacts properly with qParams.page and updateQuery to handle page changes and that it correctly updates the user list.
src/components/Users/LinkedSkills.tsx (1)
56-58: Verify the usage of AuthorizeFor within useIsAuthorized
Ensure that AuthorizeFor is correctly used as an argument for useIsAuthorized. Depending on its implementation, AuthorizeFor might not be necessary here, or you might need to pass the roles array directly to useIsAuthorized.
Run the following script to inspect the implementations of useIsAuthorized and AuthorizeFor:
This script will help determine if AuthorizeFor is appropriately used within useIsAuthorized and whether any adjustments are necessary.
✅ Verification successful
Let me gather more information about the implementation details.
Let me check one more thing to see how AuthorizeFor is implemented since we didn't get its implementation in the previous search.
Let me try one final search pattern to find the AuthorizeFor implementation.
AuthorizeFor is correctly used with useIsAuthorized
The implementation shows that:
AuthorizeForis a function that takes an array ofUserRoles and returns a callback function matching theAuthorizedForCBtypeuseIsAuthorizedexpects a parameter of typeAuthorizedForCB, which is exactly whatAuthorizeFor(["DistrictAdmin", "StateAdmin"])returns- This pattern is consistently used across the codebase in multiple components (e.g., LocationManagement.tsx, BedManagement.tsx)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the correct usage of `useIsAuthorized` and `AuthorizeFor`.
# Test: Find definitions of `useIsAuthorized` and `AuthorizeFor`.
# Expect: Confirm whether `AuthorizeFor` should be used as an argument to `useIsAuthorized`.
# Search for the definition of `useIsAuthorized`
fd --extension ts --type f | xargs rg 'export function useIsAuthorized'
# Search for the definition of `AuthorizeFor`
fd --extension ts --type f | xargs rg 'export default function AuthorizeFor'
# Search for examples where `useIsAuthorized` is used
rg --type ts 'useIsAuthorized'
# Search for examples where `AuthorizeFor` is used
rg --type ts 'AuthorizeFor'
Length of output: 4086
Script:
#!/bin/bash
# Get the implementation of useIsAuthorized hook
rg -A 10 "export const useIsAuthorized" src/hooks/useIsAuthorized.ts
# Get the implementation of AuthorizeFor
rg -A 10 "export default function AuthorizeFor" src/Utils/AuthorizeFor.tsx
# Get the type definition of AuthorizedForCB
rg "type AuthorizedForCB" src/Utils/AuthorizeFor.tsx
Length of output: 413
Script:
#!/bin/bash
# Search for AuthorizeFor function implementation with a broader pattern
rg -A 10 "AuthorizeFor.*=.*\[" src/Utils/AuthorizeFor.tsx
# Alternative search pattern
rg -A 10 "function AuthorizeFor" src/Utils/AuthorizeFor.tsx
Length of output: 121
Script:
#!/bin/bash
# Try to find any content in AuthorizeFor.tsx
cat src/Utils/AuthorizeFor.tsx
# Alternative search for exports from this file
rg "export" src/Utils/AuthorizeFor.tsx
Length of output: 1620
|
👋 Hi, @Jacobjeevan, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
@Jacobjeevan what is the status on this PR |
@nihal467 Still waiting on the backend 👍 I'll get the conflicts addressed though. |
|
👋 Hi, @Jacobjeevan, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/components/Facility/FacilityUsers.tsx (1)
7-7: Well-structured separation of concerns!Good architectural decision to:
- Extract filtering logic into a reusable
useFiltershook- Use a dedicated
UserListViewcomponent for displaying usersAlso applies to: 9-9, 16-19
src/components/Users/ManageUsers.tsx (2)
110-121: Consider memoizing the user list viewTo prevent unnecessary re-renders, consider wrapping the UserListView with useMemo:
- manageUsers = ( + manageUsers = useMemo(() => ( <div> <UserListView users={userListData?.results ?? []} onSearch={(username) => updateQuery({ username })} searchValue={qParams.username} activeTab={activeTab} onTabChange={setActiveTab} /> <Pagination totalCount={userListData.count} /> </div> + ), [userListData, qParams.username, activeTab]);
Line range hint
280-285: Add error boundary for facility management operationsConsider implementing an error boundary to gracefully handle failures in facility management operations.
class FacilityErrorBoundary extends React.Component { componentDidCatch(error, errorInfo) { // Log error to monitoring service Notification.Error({ msg: "An error occurred while managing facilities", }); } // ... rest of implementation }Wrap the facility management section with this error boundary:
+ <FacilityErrorBoundary> <div className="h-full"> {/* ... existing code ... */} </div> + </FacilityErrorBoundary>src/components/Users/UserAdd.tsx (1)
Line range hint
49-55: Consider moving the help URL to configurationThe help documentation URL is hardcoded. Consider moving it to a configuration file or environment variable for better maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/components/Facility/FacilityUsers.tsx(2 hunks)src/components/Users/ManageUsers.tsx(3 hunks)src/components/Users/UserAdd.tsx(3 hunks)
🔇 Additional comments (7)
src/components/Facility/FacilityUsers.tsx (2)
34-38: Remove unnecessary toString() conversion
The request utility will handle string conversion automatically.
- limit: resultsPerPage.toString(),
+ limit: resultsPerPage,51-67: Clean and efficient implementation!
The component effectively:
- Displays user count with loading states
- Integrates search functionality
- Handles pagination properly
src/components/Users/ManageUsers.tsx (2)
71-71: Fix the district query parameter key
The parameter key should be 'district' instead of 'district_id' to match the API's expected parameter.
- district_id: qParams.district,
+ district: qParams.district,106-108: Good loading state handling!
Appropriate use of loading component while waiting for data.
src/components/Users/UserAdd.tsx (3)
Line range hint 42-63: Clean and well-structured implementation
The component follows good practices:
- Uses internationalization for text
- Provides clear navigation with back button
- Delegates form handling to a specialized component
- Includes help documentation link
60-60: Verify age validation implementation
Since Issue #9001 mentions implementing age validation (minimum 15 years), verify that this validation is properly implemented in the UserAddEditForm component.
✅ Verification successful
Age validation is properly implemented
The code shows proper age validation in the UserAddEditForm component with a minimum age requirement of 16 years. This is implemented in the validation logic:
else if (dayjs(formData[field]).isAfter(dayjs().subtract(16, "year"))) {
errors[field] = t("enter_valid_dob_age");
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for age validation implementation in UserAddEditForm
rg -A 10 "age|birth|dob" src/components/Users/UserAddEditForm.tsx
Length of output: 4048
Line range hint 12-41: Move validateRule to UserAddEditForm component
The comment indicates this is a temporary placement. Since this function is related to form validation, it should be moved to UserAddEditForm to maintain code organization and follow the component's responsibility separation.
Let's verify if this function is already defined in UserAddEditForm:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
public/locale/en.json (3)
377-377: Improve avatar-related messages for better clarity and consistency.The avatar-related messages could be improved for better user experience.
Apply these changes:
- "avatar_updated_success": "Avatar updated successfully", + "avatar_updated_success": "Profile picture updated successfully", - "change_avatar": "Change Avatar", + "change_avatar": "Change Profile Picture", - "change_avatar_note": "JPG, GIF or PNG. 1MB max.", + "change_avatar_note": "Accepted formats: JPG, GIF, or PNG. Maximum size: 1MB", - "edit_avatar": "Edit Avatar", + "edit_avatar": "Edit Profile Picture", - "edit_avatar_note": "Change the avatar of the user", + "edit_avatar_note": "Change the user's profile picture", - "edit_avatar_note_self": "Change your avatar", + "edit_avatar_note_self": "Change your profile picture", - "edit_avatar_permission_error": "You do not have permissions to edit the avatar of this user", + "edit_avatar_permission_error": "You do not have permission to change this user's profile picture"Also applies to: 420-421, 661-663
291-291: Improve facility management messages for better clarity and consistency.The facility management messages could be enhanced for better user experience.
Apply these changes:
- "facility_linked_success": "Facility linked successfully", + "facility_linked_success": "Facility has been linked successfully", - "home_facility_cleared_success": "Home Facility cleared successfully", + "home_facility_cleared_success": "Home facility has been removed successfully", - "home_facility_updated_error": "Error while updating Home Facility", + "home_facility_updated_error": "Failed to update home facility. Please try again.", - "home_facility_updated_success": "Home Facility updated successfully", + "home_facility_updated_success": "Home facility has been updated successfully", - "replace_home_facility": "Replace Home Facility", + "replace_home_facility": "Replace home facility", - "replace_home_facility_confirm": "Are you sure you want to replace", + "replace_home_facility_confirm": "Are you sure you want to replace the current home facility", - "unlink_facility": "Unlink Facility", + "unlink_facility": "Remove facility access", - "unlink_facility_access": "The user will lose access to the facility", + "unlink_facility_access": "Warning: The user will no longer have access to this facility", - "unlink_facility_error": "Error while unlinking facility. Try again later.", + "unlink_facility_error": "Failed to remove facility access. Please try again.", - "unlink_facility_success": "Facility unlinked successfully", + "unlink_facility_success": "Facility access has been removed successfully", - "unlink_home_facility_error": "Error while unlinking home facility. Try again later.", + "unlink_home_facility_error": "Failed to remove home facility. Please try again.", - "unlink_home_facility_success": "Home Facility cleared successfully" + "unlink_home_facility_success": "Home facility has been removed successfully"Also applies to: 734-734, 823-825, 1240-1242, 1443-1449
303-303: Improve skill management messages for better clarity and consistency.The skill management messages could be enhanced for better user experience.
Apply these changes:
- "skill_add_error": "Error while adding skill", + "skill_add_error": "Failed to add skill. Please try again.", - "skill_added_successfully": "Skill added successfully", + "skill_added_successfully": "Skill has been added successfully", - "unlink_skill": "Unlink Skill", + "unlink_skill": "Remove skill", - "unlink_skill_access": "The user will not have the skill associated anymore.", + "unlink_skill_access": "Warning: This skill will be removed from the user's profile", - "unlink_skill_error": "Error while unlinking skill. Try again later.", + "unlink_skill_error": "Failed to remove skill. Please try again.", - "unlink_skill_success": "Skill unlinked successfully" + "unlink_skill_success": "Skill has been removed successfully"Also applies to: 1362-1363, 1450-1454
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
public/locale/en.json(39 hunks)src/components/Common/Page.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Common/Page.tsx
🔇 Additional comments (7)
public/locale/en.json (7)
218-220: LGTM: User management tab labels are clear and consistent.
The tab labels for user management are descriptive and follow a consistent pattern:
- "Linked Facilities"
- "User Information"
- "Linked Skills"
385-385: LGTM: User information sections have clear and consistent messaging.
The messages for basic, contact, and professional information sections are well-structured and maintain consistency in their note patterns:
- View/update variations for self vs. other users
- Clear section headings
- Consistent terminology
Also applies to: 534-537, 1136-1138, 1204-1207
881-881: LGTM: WhatsApp validation message is clear.
The message for WhatsApp number validation is clear and concise.
1160-1160: LGTM: User type selection messages are consistent.
The messages for user type selection maintain consistency.
Also applies to: 1500-1500
1498-1499: LGTM: User qualification messages are clear.
The messages for user qualifications are descriptive and helpful.
942-942: LGTM: User management action messages are consistent.
The messages for user management actions maintain consistency in terminology.
Also applies to: 1494-1494, 1539-1540
1079-1083: 🛠️ Refactor suggestion
Enhance password-related messages for better security and user experience.
Applying the previously suggested password validation message improvements and adding new suggestions for the password reset flow.
Apply these changes:
- "password_length_validation": "Password must be at least 8 characters long",
- "password_lowercase_validation": "Password must contain at least one lowercase letter (a-z)",
- "password_number_validation": "Password must contain at least one number (0-9)",
- "password_uppercase_validation": "Password must contain at least one uppercase letter (A-Z)",
- "password_validation": "Password must contain at least: 8 characters, 1 uppercase letter (A-Z), 1 lowercase letter (a-z), and 1 number (0-9)",
+ "password_length_validation": "Password must be at least 8 characters long",
+ "password_lowercase_validation": "Password must contain at least one lowercase letter (a-z)",
+ "password_number_validation": "Password must contain at least one number (0-9)",
+ "password_uppercase_validation": "Password must contain at least one uppercase letter (A-Z)",
+ "password_validation": "Password must contain at least: 8 characters, 1 uppercase letter (A-Z), 1 lowercase letter (a-z), and 1 number (0-9)",
- "please_confirm_password": "Please confirm your new password.",
+ "please_confirm_password": "Please re-enter your new password to confirm",
- "please_enter_current_password": "Please enter your current password.",
+ "please_enter_current_password": "Please enter your current password for verification",
- "please_enter_new_password": "Please enter your new password.",
+ "please_enter_new_password": "Please enter a new password that meets the requirements",
- "reset_password_note": "Enter user's current password, then create and confirm the new password",
+ "reset_password_note": "To reset the password: 1) Enter current password, 2) Create new password, 3) Confirm new password",
- "reset_password_note_self": "Enter your current password, then create and confirm your new password",
+ "reset_password_note_self": "To reset your password: 1) Enter current password, 2) Create new password, 3) Confirm new password"Also applies to: 1087-1089, 1145-1145, 1147-1149, 1261-1262
github-actions
bot
added
merge conflict
|
👋 Hi, @Jacobjeevan, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
rithviknishad
removed
needs review
merge conflict
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
src/components/Form/Form.tsx (3)
35-37: LGTM! Consider adding JSDoc comments.The new props are well-typed and follow naming conventions. Consider adding JSDoc comments to document their purpose and usage.
+/** Reset form to initial values when cancel is clicked */ resetFormValsOnCancel?: boolean; +/** Reset form to initial values after successful submission */ resetFormValsOnSubmit?: boolean; +/** Hide the cancel button in the form footer */ hideCancelButton?: boolean;
88-93: LGTM! Consider adding error boundary.The cancel handler implementation is clean and maintains backward compatibility. Consider wrapping the onCancel callback in a try-catch for better error handling.
const handleCancel = () => { if (props.resetFormValsOnCancel) { dispatch({ type: "set_form", form: formVals.current }); } - props.onCancel?.(); + try { + props.onCancel?.(); + } catch (error) { + console.error('Error in form cancel handler:', error); + Notification.Error({ msg: "Failed to cancel form" }); + } };
Line range hint
1-156: Consider extracting form reset logic to a custom hook.The form reset functionality could be abstracted into a custom hook to promote reusability across other form components in the user management system.
Example implementation:
function useFormReset<T extends FormDetails>( defaults: T, dispatch: (action: any) => void ) { const formVals = useRef(defaults); useEffect(() => { formVals.current = defaults; }, [defaults]); const resetForm = () => { dispatch({ type: "set_form", form: formVals.current }); }; return { resetForm }; }src/components/Users/UserSummary.tsx (1)
49-51: Add explicit return valueThe early return statement should explicitly return null for better clarity and type safety.
if (!userData) { - return <></>; + return null; }public/locale/en.json (3)
1087-1097: Improve password validation message consistencyThe password validation messages should be more consistent in their wording and provide clearer guidance.
-"password_validation": "Password must contain at least: 8 characters, 1 uppercase letter (A-Z), 1 lowercase letter (a-z), and 1 number (0-9)", +"password_validation": "Password must contain at least: 8 characters, one uppercase letter (A-Z), one lowercase letter (a-z), and one number (0-9)", -"password_length_validation": "Password must be at least 8 characters long", +"password_length_validation": "Password must contain at least 8 characters", -"password_lowercase_validation": "Password must contain at least one lowercase letter (a-z)", +"password_lowercase_validation": "Password must contain at least one lowercase letter (a-z)", -"password_uppercase_validation": "Password must contain at least one uppercase letter (A-Z)", +"password_uppercase_validation": "Password must contain at least one uppercase letter (A-Z)", -"password_number_validation": "Password must contain at least one number (0-9)", +"password_number_validation": "Password must contain at least one number (0-9)"
1497-1503: Enhance error messages with specific guidanceError messages should provide more specific information about what went wrong and how to resolve it.
-"user_add_error": "Error while adding User", +"user_add_error": "Failed to add user. Please verify the information and try again", -"user_delete_error": "Error while deleting User", +"user_delete_error": "Failed to delete user. Please ensure you have the necessary permissions", -"user_details_update_error": "Error while updating user details", +"user_details_update_error": "Failed to update user details. Please verify the changes and try again"
601-603: Enhance account deletion confirmation messageThe account deletion message should provide more specific information about the consequences.
-"delete_account_note": "Deleting this account will remove all associated data and cannot be undone.", +"delete_account_note": "Deleting this account will permanently remove all associated data including linked facilities, skills, and user information. This action cannot be undone."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
public/locale/en.json(39 hunks)src/Utils/permissions.ts(1 hunks)src/Utils/utils.ts(1 hunks)src/components/Common/Page.tsx(2 hunks)src/components/Form/Form.tsx(4 hunks)src/components/Users/UserSummary.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Common/Page.tsx
- src/Utils/utils.ts
🧰 Additional context used
📓 Learnings (2)
src/components/Users/UserSummary.tsx (1)
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#9080
File: src/components/Users/UserSummary.tsx:53-71
Timestamp: 2024-11-22T12:04:39.044Z
Learning: In the `UserSummaryTab` component (`src/components/Users/UserSummary.tsx`), when there is an error during user deletion, the delete dialog is intentionally closed to prevent the user from immediately trying to delete the user again.
src/Utils/permissions.ts (1)
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#9080
File: src/Utils/permissions.ts:37-50
Timestamp: 2024-11-14T08:07:32.427Z
Learning: In the `showUserPasswordReset` function within `src/Utils/permissions.ts`, both `authUser` and `targetUser` are guaranteed to be provided. Therefore, null or undefined checks for these parameters are unnecessary.
🔇 Additional comments (10)
src/components/Form/Form.tsx (3)
83-84: LGTM! Form reset on successful submission.
The implementation correctly resets the form to initial values only after a successful submission when resetFormValsOnSubmit is true.
138-143: LGTM! Clean implementation of conditional Cancel button.
The conditional rendering and event handling for the Cancel button is well-implemented, maintaining existing styling and accessibility features.
43-43: Consider deps array for formVals ref
The formVals ref stores the initial form values, but it won't update if props.defaults changes. Consider if this is the intended behavior.
If updates to props.defaults should be reflected, consider using useEffect:
+useEffect(() => {
+ formVals.current = props.defaults;
+}, [props.defaults]);Also applies to: 49-49
src/Utils/permissions.ts (4)
25-37: LGTM! Good security improvements
The changes to prevent self-deletion and maintain proper hierarchy checks are well implemented.
46-56: LGTM! Well-structured permission checks
Both functions implement appropriate permission checks:
- Avatar editing is properly restricted to self or superuser
- Permission editing correctly combines superuser access with administrative jurisdiction
39-43: 🛠️ Refactor suggestion
Consider adding hierarchical access control
The current implementation only allows self-reset. Consider adding hierarchical access control similar to the delete functionality to allow administrators to reset passwords of users under their jurisdiction.
export const showUserPasswordReset = (
authUser: UserModel,
targetUser: UserModel,
) => {
- return authUser.username === targetUser.username;
+ // Allow self-reset
+ if (authUser.username === targetUser.username) {
+ return true;
+ }
+
+ // Check if auth user has higher privileges
+ if (USER_TYPES.indexOf(authUser.user_type) <= USER_TYPES.indexOf(targetUser.user_type)) {
+ return false;
+ }
+
+ // Allow admins to reset passwords for users in their jurisdiction
+ return checkIfStateOrDistrictAdminInSameLocation(authUser, targetUser);
};Likely invalid or redundant comment.
5-24: 🛠️ Refactor suggestion
Improve type safety and function naming
The helper function could be improved in several ways:
- The
hasLocationcheck is redundant as the subsequent checks implicitly handle this - Optional chaining makes the code less type-safe
- The function name could be more precise to reflect its purpose
-const checkIfStateOrDistrictAdminInSameLocation = (
+const hasAdministrativeJurisdiction = (
authUser: UserModel,
targetUser: UserModel,
) => {
- const hasLocation = Boolean(
- targetUser.state_object || targetUser.district_object,
- );
+ if (!targetUser.state_object && !targetUser.district_object) {
+ return false;
+ }
const isStateAdminOfSameState =
authUser.user_type === "StateAdmin" &&
- targetUser.state_object?.id === authUser.state;
+ targetUser.state_object &&
+ targetUser.state_object.id === authUser.state;
const isDistrictAdminOfSameDistrict =
authUser.user_type === "DistrictAdmin" &&
- targetUser.district_object?.id === authUser.district;
+ targetUser.district_object &&
+ targetUser.district_object.id === authUser.district;
- return (
- hasLocation && (isStateAdminOfSameState || isDistrictAdminOfSameDistrict)
- );
+ return isStateAdminOfSameState || isDistrictAdminOfSameDistrict;
};Likely invalid or redundant comment.
src/components/Users/UserSummary.tsx (3)
53-71: LGTM! Well-implemented delete handler
The delete handler implementation includes:
- Proper loading state management
- Error handling with user feedback
- Navigation after successful deletion
- Intentional dialog closure on error to prevent immediate retry
73-82: LGTM! Clean permission management
Well-organized permission checks with descriptive variable names and proper use of utility functions.
134-235: LGTM! Well-structured component layout
The component layout is well-organized with:
- Clean conditional rendering
- Consistent use of UserColumns for layout
- Proper permission-based feature gates
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
cypress/e2e/users_spec/UsersCreation.cy.ts (1)
Line range hint
1-199: Add test coverage for new user management requirements.Based on the PR objectives, the following test scenarios are missing:
- Conditional rendering of qualifications for Doctors and Nurses
- Password change restrictions for non-admin users
- User deletion permissions
- Facility user deletion edge cases (is_active flag)
Consider adding these test cases:
it("should only show qualifications for Doctors and Nurses", () => { // Test for Doctor role userCreationPage.selectUserType("Doctor"); cy.get("[data-testid=qualification-field]").should("be.visible"); // Test for Nurse role userCreationPage.selectUserType("Nurse"); cy.get("[data-testid=qualification-field]").should("be.visible"); // Test for other roles userCreationPage.selectUserType("Staff"); cy.get("[data-testid=qualification-field]").should("not.exist"); }); it("should restrict password changes to authorized users", () => { // Test as non-admin user loginPage.loginByRole("staff"); userProfilePage.visitUserProfile(someOtherUser); cy.get("[data-testid=change-password-button]").should("not.exist"); // Test as admin loginPage.loginByRole("districtAdmin"); userProfilePage.visitUserProfile(someOtherUser); cy.get("[data-testid=change-password-button]").should("be.visible"); }); it("should handle facility user deletion correctly", () => { // Test deletion and verify is_active flag loginPage.loginByRole("districtAdmin"); userPage.deleteUser(facilityUser); cy.intercept("DELETE", "/api/v1/users/*").as("deleteUser"); cy.wait("@deleteUser").then((interception) => { expect(interception.request.body).to.have.property("is_active", false); }); });public/locale/en.json (1)
542-545: Consider consolidating similar information notesThe information notes for contact, personal, and professional info sections follow the same pattern but are duplicated. Consider using a template string with placeholders.
Example approach:
+"info_note_template": "View{{edit}} {{user}}'s {{type}} information", -"contact_info_note": "View or update user's contact information", -"contact_info_note_self": "View or update your contact information", +"contact_info_note": "{{info_note_template, edit: ' or update', user: 'user', type: 'contact'}}", +"contact_info_note_self": "{{info_note_template, edit: ' or update', user: 'your', type: 'contact'}}"Also applies to: 1144-1146, 1212-1215
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
cypress/e2e/users_spec/UsersCreation.cy.ts(2 hunks)cypress/pageobject/Users/UserCreation.ts(1 hunks)public/locale/en.json(39 hunks)
🔇 Additional comments (9)
cypress/e2e/users_spec/UsersCreation.cy.ts (1)
167-167: Selector updates align with new component structure.
The selectors have been updated to:
- Include dynamic username (
#name-${username}) for better specificity - Use hyphen instead of underscore (
#home-facility)
These changes improve selector specificity and maintain consistent naming conventions.
Also applies to: 170-170
cypress/pageobject/Users/UserCreation.ts (1)
49-49: Verify UI button label consistency
The button text change from "Save User" to "Submit" should be consistent with other form submission buttons in the application.
✅ Verification successful
Button text change is consistent with application's form submission patterns
The change from "Save User" to "Submit" aligns with the application's consistent use of "Submit" as the primary action button text across various forms and operations. The codebase shows:
- The application uses "Submit" as the default button text in the core implementation:
Cypress.Commands.add("clickSubmitButton", (buttonText = "Submit") => {- "Submit" is used consistently across different form submissions:
- User management: "Submit" for user creation
- Patient management: "Submit" for prescriptions, consultations
- Asset management: "Submit" for updates
- Login forms: "Submit" for authentication
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in button labels across the codebase
# Look for other instances of "Save User" or "Submit" buttons
# Search for Save User button text
echo "Searching for 'Save User' button text..."
rg -i "save.?user" --type ts --type tsx
# Search for Submit button text
echo "Searching for 'Submit' button text..."
rg -i "submit.?button" --type ts --type tsx
Length of output: 331
Script:
#!/bin/bash
# Let's try a different approach to search for button patterns in TypeScript files
# Search for button text patterns in TypeScript files
echo "Searching for button text patterns..."
rg -t ts "cy\.(click|get).*Button.*['\"]" -A 1
# Search specifically for clickSubmitButton usage
echo "Searching for clickSubmitButton usage..."
rg -t ts "clickSubmitButton" -A 1
# Search for any button-related text in TypeScript files
echo "Searching for button text patterns..."
rg -t ts "(Save|Submit).*Button" -A 1
Length of output: 58245
public/locale/en.json (7)
226-228: LGTM: User interface tab labels are clear and consistent
The new tab labels for user management are descriptive and follow a consistent pattern.
428-429: LGTM: Avatar management strings are clear and include helpful notes
The avatar-related strings provide clear instructions and size limitations.
Also applies to: 669-671
1087-1097: Enhance password validation messages
The password validation messages could be improved for better clarity and consistency.
Also applies to: 1153-1156
831-833: LGTM: Facility management messages are comprehensive
The strings cover all facility management scenarios including success/error states and confirmation messages.
Also applies to: 1248-1250, 1450-1456
1457-1461: LGTM: Skill management messages follow facility management pattern
The skill management messages maintain consistency with facility management messages.
1497-1503: Enhance error messages with more specific information
The error messages should provide more specific information about what went wrong.
1509-1512: Improve username validation messages for better security
The username validation messages should be more secure by not revealing specific validation rules.
|
@Jacobjeevan Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
Proposed Changes
To Do:
is_active flagto false, which works fine for userList (/api/v1/users/), but not for getFacilityUsers (facility/facilityId/get_users/) as latter is checking fordeleted=Falseflag. BE PR herePotential things to check:
If user doesn't have edit permissions:
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
New Features
LinkedSkillsTabcomponent for displaying linked skills associated with a user.UserHomecomponent for user management with dynamic tab rendering.UserBannercomponent for displaying user information in a banner format.UserResetPasswordcomponent for managing password resets.Bug Fixes
Documentation
Tests