MyMicroplan search change and user-access roles changes #1680
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
abishekTa-egov
changed the title
abishekTa-egov
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range comments (3)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/services/searchSavedPlansWithCampaign.js (1)
Line range hint
22-91: Consider enhancing error handling and documentationThe function handles complex operations across multiple APIs but could benefit from the following improvements:
- Add JSDoc documentation specifying the expected shape of the
bodyparameter- Provide more specific error messages for different failure scenarios (API failures, validation errors, etc.)
- Consider using a try-catch block for the nested API calls
Example documentation:
/** * Searches for saved plans and merges them with campaign details * @param {Object} body - The search criteria * @param {Object} body.PlanConfigurationSearchCriteria - Plan search parameters * @param {string} body.PlanConfigurationSearchCriteria.name - Name to search for * @param {string} body.PlanConfigurationSearchCriteria.status - Status filter * @returns {Promise<{PlanConfiguration: Array}>} Merged plan and campaign details * @throws {Error} When API calls fail or validation errors occur */health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/RoleTableComposer.js (2)
Line range hint
346-355: Add error handling and loading state to the DropdownThe Dropdown implementation could be more robust with the following improvements:
- Add null checking for
state?.boundaryHierarchy- Include a loading state while hierarchy data is being fetched
Consider updating the implementation:
<Dropdown className="roleTableCell" selected={rowData?.find((item) => item?.rowIndex === row?.rowIndex)?.selectedHierarchy || null} disabled={nationalRoles?.includes(category) ? true : false} isMandatory={true} - option={state?.boundaryHierarchy.filter((item) => !(item.boundaryType === "Village" || item.boundaryType==="Country"))} + option={state?.boundaryHierarchy?.filter((item) => !(item.boundaryType === "Village" || item.boundaryType==="Country")) || []} + isLoading={!state?.boundaryHierarchy} select={(value) => { row.selectedHeirarchy = value; handleHierarchyChange(value, row); }} optionKey="boundaryType" t={t} />🧰 Tools
🪛 Biome
[error] 346-346: 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)
Line range hint
1-607: Consider architectural improvements for better performance and maintainabilityThe component could benefit from several architectural improvements:
- Debounce the search form submission to prevent excessive API calls
- Memoize row handlers to optimize re-renders
- Consider extracting the toast management into a custom hook
Here's an example implementation of these improvements:
// Create a custom hook for toast management const useToast = () => { const [toast, setToast] = useState(null); const showToast = useCallback((message) => { setToast(message); setTimeout(() => setToast(null), 1000); }, []); return [toast, showToast]; }; // Memoize row handlers const handleAssignEmployee = useCallback((row) => { // existing implementation }, [/* dependencies */]); // Debounce search const debouncedSearch = useMemo( () => debounce((searchParams) => { setFilters(searchParams); }, 300), [] );Additionally, consider splitting the component into smaller, more focused components:
- SearchForm
- EmployeeTable
- RoleAssignment
This would improve maintainability and make the code easier to test.
🧰 Tools
🪛 Biome
[error] 346-346: 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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (4)
- health/micro-ui/web/micro-ui-internals/example/src/UICustomizations.js (4 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/RoleTableComposer.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/UICustomizations.js (5 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/services/searchSavedPlansWithCampaign.js (2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
health/micro-ui/web/micro-ui-internals/example/src/UICustomizations.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/RoleTableComposer.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/UICustomizations.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/services/searchSavedPlansWithCampaign.js (1)
Pattern
**/*.js: check
🔇 Additional comments (5)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/services/searchSavedPlansWithCampaign.js (1)
38-38: Security Concern: Verify the removal of employee-based filteringThe removal of the
employeeIdfilter means that the API will return all plans regardless of employee assignment. This could potentially expose plans to unauthorized users.✅ Verification successful
Removal of employeeId filter is intentional and safe
The removal of
employeeIdfilter insearchSavedPlansWithCampaign.jsis safe because:
- This specific endpoint is used only for searching saved plans with campaign details
- The codebase shows proper employee-based filtering in all other critical endpoints:
- FacilityCatchmentMapping.js
- PopInbox.js
- PlanInbox.js
- viewVillage.js
- The
filterUniqueByPlanConfig: trueparameter ensures deduplication at plan configuration level- The search is still scoped by
tenantIdwhich provides tenant-level isolation🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any other access controls in place echo "Checking for alternative access controls..." rg -A 5 "PlanEmployeeAssignmentSearchCriteria" rg -A 5 "employeeId.*uuid" # Check if this is handled at the API level echo "Checking API endpoints for access controls..." rg -A 10 "/plan-service/employee/_search"Length of output: 19595
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/UICustomizations.js (2)
135-135: LGTM: Proper translation handling implementedThe translation implementation for microplan name is consistent with the application's localization approach.
227-231: LGTM: Proper role-based access control implementedThe implementation correctly checks for required roles using session storage and applies appropriate access restrictions.
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/RoleTableComposer.js (1)
348-348: LGTM: Boundary type filtering implemented correctlyThe filtering logic correctly excludes both "Village" and "Country" boundary types from the hierarchy dropdown options, which aligns with the requirement to hide country information for non-national roles.
health/micro-ui/web/micro-ui-internals/example/src/UICustomizations.js (1)
922-922:⚠️ Potential issuePotential Duplicate Assignment of
namePropertyAt line 922,
data.body.PlanConfigurationSearchCriteria.nameis being reassigned todata?.state?.searchForm?.microplanNameafter previously assigning it toname. This may unintentionally overwrite the initial value. Please verify if both assignments are necessary and ensure the correct value is being used.
...o-ui-internals/packages/modules/microplan/src/hooks/services/searchSavedPlansWithCampaign.js
Show resolved
Hide resolved
...h/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/UICustomizations.js
Show resolved
Hide resolved
...h/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/UICustomizations.js
Show resolved
Hide resolved
Microplan search working
Ui customization updated
User-access role doesn't show country for other than national roles
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Chores