Edit feature for facilities in organization | pincode, geo_organization info auto populates #9662
Conversation
WalkthroughThis pull request introduces significant changes to facility management functionality, including the complete removal of the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
On facility edit the organisation is also not getting prefilled, I think we should solve that also |
You mean |
|
I don't think jeevan is working on it, I will assign it to you. lets have one PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/pages/Organization/components/OrganizationSelector.tsx (1)
35-55: Enhance defensive checks around JSON parsing.
Currently, atry/catchblock is in place to catchJSON.parseerrors, logging them to the console. Consider adding a fallback path or early return to ensure the component’s state remains stable in edge cases (e.g., an empty string or invalid structure) to avoid undesired behavior.useEffect(() => { if (value) { try { const parsedValue = typeof value === "string" ? JSON.parse(value) : value; if (parsedValue) { // ... } } catch (e) { + setSelectedLevels([]); console.error("Invalid value for geo_organization:", e); } } }, [value]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(1 hunks)src/components/Facility/FacilityCreate.tsx(4 hunks)src/pages/Organization/components/OrganizationSelector.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Facility/FacilityCreate.tsx
🔇 Additional comments (4)
src/pages/Organization/components/OrganizationSelector.tsx (3)
2-2: Import statement is correctly updated.
No issues found with the import of React hooks.
31-31: Prop destructuring for newvalueis fine.
Thevalueprop is properly introduced here, and the necessary props are being destructured clearly.
171-171: No issues with the Autocomplete value assignment.
Using the last selected organization’sidor an empty string is a sensible approach.public/locale/en.json (1)
1584-1584: New i18n key added.
The"select_location_from": "Select location from"entry aligns with the updated UI text references. Ensure that its usage is consistent across the app, and consider whether this prompt needs additional context or placeholders.
Mahendar0701
changed the title
|
the issue still presist, can you attach a video after fixing the issue |
Mahendar0701
changed the title
|
edit may work fine, but is it auto-filling when pincode is entered? |
Nope , i will fix that too 👍 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/hooks/useOrganization.ts (1)
22-32: Add type safety for API response.Consider adding type definitions for the API response to improve type safety and maintainability.
+interface OrganizationResponse { + results: Array<{ + // Add organization properties here + id: string; + name: string; + // ... other properties + }>; +} export function useOrganization({ orgType = "", parentId = "", name = "", enabled = true, }: UseOrganizationParams) { - const { data, isLoading, isError } = useQuery({ + const { data, isLoading, isError } = useQuery<OrganizationResponse>({ queryKey: ["organization", orgType, name, parentId], // ... rest of the implementation });Also applies to: 40-44
src/components/Facility/FacilityForm.tsx (4)
71-71: Consider improving geo_organization validation.The current validation only checks for a non-empty string, but based on the usage in the component, this field should validate an Organization ID.
- geo_organization: z.string().min(1, t("organization_required")), + geo_organization: z + .string() + .min(1, t("organization_required")) + .regex(/^[0-9a-fA-F]{24}$/, t("invalid_organization_id")),
73-75: Consider making phone number validation more flexible.The phone number validation is currently hardcoded for Indian numbers (+91). Consider making this configurable or supporting international formats.
- .regex(/^\+91[0-9]{10}$/, t("phone_number_validation")), + .regex(/^\+[1-9]\d{1,14}$/, t("phone_number_validation")), // Support E.164 format
169-170: Consider increasing geolocation timeout.The current timeout of 10 seconds might be too short for slower devices or poor GPS signals.
- { timeout: 10000 }, + { timeout: 30000, maximumAge: 300000, enableHighAccuracy: true },
200-221: Add cleanup to form reset effect.The effect that resets form data should clean up to prevent memory leaks.
useEffect(() => { + let mounted = true; if (facilityData) { + if (!mounted) return; setSelectedLevels([ facilityData.geo_organization as unknown as Organization, ]); form.reset({ // ... form reset logic }); } + return () => { + mounted = false; + }; }, [facilityData]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
public/locale/en.json(13 hunks)src/components/Facility/FacilityForm.tsx(12 hunks)src/hooks/useOrganization.ts(1 hunks)src/hooks/useStateAndDistrictFromPincode.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/hooks/useStateAndDistrictFromPincode.ts
- public/locale/en.json
🔇 Additional comments (4)
src/hooks/useOrganization.ts (3)
1-7: LGTM! Well-organized imports.The imports are properly organized and all dependencies are being used.
9-14: LGTM! Clean interface definition.The interface is well-defined with appropriate types and optionality.
34-38: LGTM! Proper error handling implementation.Error handling is correctly implemented using useEffect to prevent multiple toast notifications, as suggested in the previous review.
src/components/Facility/FacilityForm.tsx (1)
Line range hint
1-56: LGTM! Well-structured imports and type definitions.The imports are well-organized using absolute paths, and the
FacilityPropsinterface is properly defined with optional properties.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/hooks/useStateAndDistrictFromPincode.ts (1)
22-33: 🛠️ Refactor suggestionAdd error retry for pincode API and handle rate limiting.
The pincode API is a critical external dependency. Consider:
- Adding retry configuration
- Implementing rate limiting handling
- Caching successful responses
useQuery<PincodeResponse>({ queryKey: ["pincode-details", pincode], queryFn: () => getPincodeDetails(pincode, careConfig.govDataApiKey), enabled: pincode !== "" && validatePincode(pincode), + retry: (failureCount, error: any) => { + // Don't retry on rate limiting + if (error?.response?.status === 429) return false; + return failureCount < 3; + }, + staleTime: 1000 * 60 * 60, // Cache for 1 hour + cacheTime: 1000 * 60 * 60 * 24, // Keep in cache for 24 hours });
🧹 Nitpick comments (2)
src/hooks/useStateAndDistrictFromPincode.ts (2)
62-69: Enhance error handling with specific error messages.The current error handling uses generic toast messages. Consider:
- Adding specific error messages based on the error type
- Providing retry options for users
- Implementing a fallback UI for error states
useEffect(() => { if (isStateError || isPincodeError) { - toast.info(t("pincode_state_auto_fill_error")); + const errorMessage = isPincodeError + ? t("pincode_fetch_error") + : t("state_org_fetch_error"); + toast.error(errorMessage, { + action: { + label: t("retry"), + onClick: () => refetch(), + }, + }); } if (isDistrictError && !isStateError) { - toast.info(t("pincode_district_auto_fill_error")); + toast.error(t("district_org_fetch_error"), { + action: { + label: t("retry"), + onClick: () => refetch(), + }, + }); } }, [isStateError, isPincodeError, isDistrictError]);
73-78: Consider returning granular loading and error states.The current implementation combines all loading and error states. For better UX, consider returning individual states to allow more precise UI feedback.
return { stateOrg, districtOrg, - isLoading: isPincodeLoading || isStateLoading || isDistrictLoading, - isError: isPincodeError || isStateError || isDistrictError, + loadingStates: { + isPincodeLoading, + isStateLoading, + isDistrictLoading, + }, + errorStates: { + isPincodeError, + isStateError, + isDistrictError, + }, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/hooks/useStateAndDistrictFromPincode.ts(1 hunks)
🧰 Additional context used
🪛 eslint
src/hooks/useStateAndDistrictFromPincode.ts
[error] 7-7: Insert useOrganization·}·from·"@/hooks/useOrganization";⏎⏎import·{·
(prettier/prettier)
[error] 9-11: Delete ";⏎⏎import·{·useOrganization·}·from·"@/hooks/useOrganization
(prettier/prettier)
🪛 GitHub Check: lint
src/hooks/useStateAndDistrictFromPincode.ts
[failure] 7-7:
Insert useOrganization·}·from·"@/hooks/useOrganization";⏎⏎import·{·
[failure] 9-9:
Delete ";⏎⏎import·{·useOrganization·}·from·"@/hooks/useOrganization
🪛 GitHub Actions: Lint Code Base
src/hooks/useStateAndDistrictFromPincode.ts
[error] 7-7: Prettier formatting error: Missing import statement for useOrganization from @/hooks/useOrganization
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (2)
src/hooks/useStateAndDistrictFromPincode.ts (2)
13-20: LGTM! Well-defined interfaces.The interfaces are focused and follow TypeScript best practices.
38-50: Validate state organization selection.The current implementation assumes the first organization in the array is the correct one. Consider:
- Adding validation for the organization type and structure
- Handling cases where multiple organizations match the state name
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/Facility/FacilityForm.tsx (1)
201-212:⚠️ Potential issueImprove type safety for geo_organization handling.
The current implementation uses unsafe type casting. Consider using proper type checking.
🧹 Nitpick comments (1)
src/components/Facility/FacilityForm.tsx (1)
65-85: Consider adding validation for coordinates pair.While individual latitude and longitude validations are present, consider adding a custom validation to ensure both coordinates are either both present or both absent to prevent partial location data.
const facilityFormSchema = z.object({ // ... other fields latitude: z .string() .optional() .refine((val) => !val || validateLatitude(val), t("invalid_latitude")), longitude: z .string() .optional() .refine((val) => !val || validateLongitude(val), t("invalid_longitude")), + }).refine( + (data) => { + const hasLat = !!data.latitude; + const hasLng = !!data.longitude; + return (hasLat && hasLng) || (!hasLat && !hasLng); + }, + { message: t("both_coordinates_required") } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(13 hunks)src/components/Facility/FacilityForm.tsx(12 hunks)src/hooks/useStateAndDistrictFromPincode.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/hooks/useStateAndDistrictFromPincode.ts
- public/locale/en.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
🔇 Additional comments (4)
src/components/Facility/FacilityForm.tsx (4)
51-55: LGTM! Props interface is well-defined.The
FacilityPropsinterface clearly defines the component's contract with optional properties that make sense for both create and edit scenarios.
177-196: LGTM! Smart handling of pincode-based organization selection.The implementation intelligently manages organization selection based on pincode, with proper cleanup and timeout handling. The use of
useEffectwith appropriate dependencies and cleanup is particularly well done.
355-375: Well-implemented pincode autofill feedback.The UI feedback for pincode autofill is accessibility-aware with proper ARIA attributes and clear visual indicators.
524-542: LGTM! Comprehensive button state handling.The submit button implementation properly handles:
- Dirty form state check
- Loading states for both create and update
- Clear visual feedback
- Proper accessibility attributes
|
👋 Hi, @Mahendar0701, 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: 1
♻️ Duplicate comments (4)
src/components/Facility/FacilityForm.tsx (4)
116-127: 🛠️ Refactor suggestionAdd error handling to the update mutation.
The update mutation should handle errors and show appropriate error messages to users.
128-134: 🛠️ Refactor suggestionHandle loading and error states for facility data fetch.
The facility query should handle loading and error states to improve user experience.
201-203:⚠️ Potential issueImprove type safety for geo_organization handling.
The current implementation uses unsafe type casting.
379-393: 🛠️ Refactor suggestionAdd form label for OrganizationSelector.
The organization selector is missing a visible label, which impacts accessibility.
🧹 Nitpick comments (3)
src/components/Facility/FacilityForm.tsx (3)
51-56: Consider adding type constraints for facilityId.The
facilityIdprop is typed as an optional string without any constraints. Consider adding validation to ensure it's a valid UUID or matches your ID format.interface FacilityProps { organizationId?: string; - facilityId?: string; + facilityId?: `${string}-${string}-${string}-${string}-${string}`; onSubmitSuccess?: () => void; }
73-75: Improve phone number validation regex.The current regex
/^\+91[0-9]{10}$/is strict and only allows Indian phone numbers. Consider making it more flexible to support:
- Optional spaces or hyphens
- Optional country codes
- .regex(/^\+91[0-9]{10}$/, t("phone_number_validation")), + .regex(/^\+?91[-\s]?[0-9]{10}$/, t("phone_number_validation")),
522-525: Reconsider form submission disabled state.The current implementation disables the submit button when the form is not dirty (
!form.formState.isDirty). This prevents users from submitting a form they've reviewed without changes, which might be a valid use case.disabled={ - !form.formState.isDirty || (facilityId ? isUpdatePending : isPending) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(13 hunks)src/components/Facility/FacilityForm.tsx(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/Facility/FacilityForm.tsx (1)
65-85: Consider adding validation for coordinates format.The latitude and longitude validation could be enhanced to ensure proper decimal format.
latitude: z .string() .optional() - .refine((val) => !val || validateLatitude(val), t("invalid_latitude")), + .refine((val) => !val || validateLatitude(val), t("invalid_latitude")) + .refine( + (val) => !val || /^-?\d+(\.\d{1,6})?$/.test(val), + "Latitude must have up to 6 decimal places" + ), longitude: z .string() .optional() - .refine((val) => !val || validateLongitude(val), t("invalid_longitude")), + .refine((val) => !val || validateLongitude(val), t("invalid_longitude")) + .refine( + (val) => !val || /^-?\d+(\.\d{1,6})?$/.test(val), + "Longitude must have up to 6 decimal places" + ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Facility/FacilityForm.tsx(12 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (5)
src/components/Facility/FacilityForm.tsx (5)
51-56: LGTM! Props interface is well-defined.The
FacilityPropsinterface clearly defines the component's contract with optional properties for both creation and editing scenarios.
359-374: LGTM! Good accessibility implementation.The auto-fill notification includes proper ARIA attributes and visual feedback.
522-546: LGTM! Well-implemented loading states.The submit button correctly handles loading states and provides clear feedback during form submission.
116-134: 🛠️ Refactor suggestionAdd error handling to mutations and queries.
The mutations and queries should handle error cases to provide better user feedback.
const { mutate: updateFacility, isPending: isUpdatePending } = useMutation({ mutationFn: mutate(routes.updateFacility, { pathParams: { id: facilityId || "" }, }), onSuccess: (_data: FacilityModel) => { toast.success(t("facility_updated_successfully")); queryClient.invalidateQueries({ queryKey: ["organizationFacilities"] }); form.reset(); onSubmitSuccess?.(); }, + onError: (error: Error) => { + toast.error(t("facility_update_failed"), { + description: error.message + }); + }, }); - const { data: facilityData } = useQuery({ + const { data: facilityData, error: facilityError } = useQuery({ queryKey: ["facility", facilityId], queryFn: query(routes.getPermittedFacility, { pathParams: { id: facilityId || "" }, }), enabled: !!facilityId, + onError: (error: Error) => { + toast.error(t("facility_fetch_failed"), { + description: error.message + }); + }, });Likely invalid or redundant comment.
379-393: 🛠️ Refactor suggestionAdd form label for OrganizationSelector.
The organization selector is missing a visible label, which impacts accessibility.
<FormItem className="md:col-span-2 grid-cols-1 grid md:grid-cols-2 gap-5"> + <FormLabel required>{t("organization")}</FormLabel> <FormControl> <OrganizationSelector {...field} value={form.watch("geo_organization")} selected={selectedLevels} onChange={(value) => form.setValue("geo_organization", value) } required /> </FormControl> <FormMessage /> </FormItem>Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cypress/e2e/facility_spec/facility_creation.cy.ts (1)
5-8: Consider making LOCATION_HIERARCHY data dynamic.The hardcoded values for
localBodyandwardcould make tests brittle. Consider moving this to test data generation utilities like other facility data.-const LOCATION_HIERARCHY = { - localBody: "Aluva", - ward: "4", -}; +import { generateLocationHierarchy } from "../../utils/facilityData"; +const LOCATION_HIERARCHY = generateLocationHierarchy();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cypress/e2e/facility_spec/facility_creation.cy.ts(2 hunks)cypress/pageObject/facility/FacilityCreation.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (4)
cypress/e2e/facility_spec/facility_creation.cy.ts (1)
44-44: LGTM! Good test coverage for location hierarchy.The step is well-placed in the test flow and aligns with the PR objective of handling geo-organization information.
cypress/pageObject/facility/FacilityCreation.ts (3)
109-109: Consider the impact of reduced typing delay on test stability.The typing delay has been reduced from 500ms to 200ms. While this makes tests faster, it might impact stability on slower systems or under heavy load.
Consider adding a comment explaining why this value was chosen and what factors were considered.
129-133: LGTM! Well-structured location hierarchy implementation.The new method is well-implemented with:
- Proper TypeScript typing
- Reuse of existing cy.typeAndSelectOption helper
- Method chaining support
- Clear data-cy selectors
99-99: Verify if UI validation message matches the updated test assertion.The validation message case has been changed from "Invalid pincode" to "Invalid Pincode". Please ensure this matches the actual UI validation message.
✅ Verification successful
Validation message case is correctly updated
The updated test assertion "Invalid Pincode" matches the official translation in the application's locale files, confirming this is the correct case to use.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the validation message string in the codebase rg -i "invalid.?pincode" --type ts --type tsxLength of output: 75
Script:
#!/bin/bash # Search for pincode validation messages with various cases rg -i "invalid.*pincode|pincode.*invalid" # Search for pincode-related form fields or validations rg -i "pincode.*validation|validation.*pincode|pincode.*message"Length of output: 1266
|
conditional approving it. Enhancement as a another PR
auto-fill the dropdown, based on the current organisation page user |
|
@Mahendar0701 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
Fixes Switch to using Create Facility Form #9849
Pincode autofill and geo_organization info auto populates
Added edit feature for facility in orgainization
Added support for organization selector in facility form
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Removed Features
New Features
EditFacilitySheetfor inline facility editing.Internationalization
Performance & UX
Type Changes