Hrms bounday service #1544
Hrms bounday service #1544
Conversation
📝 WalkthroughWalkthroughThis pull request introduces several changes across multiple files, primarily focusing on enhancing the proxy middleware setup and modifying the Changes
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 (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (7)
- micro-ui/web/micro-ui-internals/example/src/setupProxy.js (1 hunks)
- micro-ui/web/micro-ui-internals/packages/libraries/src/hooks/useLocation.js (1 hunks)
- micro-ui/web/micro-ui-internals/packages/libraries/src/services/atoms/urls.js (1 hunks)
- micro-ui/web/micro-ui-internals/packages/libraries/src/services/elements/Localities.js (1 hunks)
- micro-ui/web/micro-ui-internals/packages/libraries/src/services/elements/Location.js (1 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/hrms/src/components/pageComponents/jurisdiction.js (4 hunks)
- micro-ui/web/micro-ui-internals/packages/react-components/src/molecules/LocationDropdownWrapper.js (1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
micro-ui/web/micro-ui-internals/example/src/setupProxy.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/libraries/src/hooks/useLocation.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/libraries/src/services/atoms/urls.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/libraries/src/services/elements/Localities.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/libraries/src/services/elements/Location.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/modules/hrms/src/components/pageComponents/jurisdiction.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/react-components/src/molecules/LocationDropdownWrapper.js (1)
Pattern
**/*.js: check
🪛 Biome
micro-ui/web/micro-ui-internals/packages/libraries/src/services/elements/Localities.js
[error] 4-4: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
🔇 Additional comments (11)
micro-ui/web/micro-ui-internals/packages/libraries/src/hooks/useLocation.js (1)
Line range hint
4-13: Summary of changes and recommendationsThe
useLocationhook has been updated to support additional parameters for including children and parents in Ward location queries. While the implementation is generally correct, we've identified two areas for improvement:
- Add default values for the new parameters in the function signature to maintain backward compatibility.
- Update the query key in the 'Ward' case to include the new parameters for proper caching behavior.
These changes will enhance the robustness and reliability of the updated hook while preserving existing functionality.
micro-ui/web/micro-ui-internals/packages/libraries/src/services/elements/Localities.js (2)
4-4: 🛠️ Refactor suggestionVerify the change in
hierarchyTypeusage and consider using template literals.The function now uses
hierarchyTypedirectly instead ofhierarchyType.code. Please ensure this change is intentional and consistent with howhierarchyTypeis passed to this function throughout the codebase.Additionally, consider using template literals for improved readability and slight performance benefits:
return `${tenantId.replace(".", "_").toUpperCase()}_${hierarchyType}`;This change addresses both the logical modification and the static analysis suggestion.
To verify the usage of
hierarchyType, please run the following script:This will help ensure that the change is consistent across the codebase.
🧰 Tools
🪛 Biome
[error] 4-4: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
Line range hint
15-24: Verify the impact ongetLocalitiesfunctionThe change in the
ADMIN_CODEfunction might affect thegetLocalitiesfunction. Please ensure that thetenantBoundryobject passed togetLocalitiescontains the correcthierarchyTypeproperty, and that the resultingi18nkeyis still correctly formed.To verify this, please run the following script:
This will help ensure that the
getLocalitiesfunction and its callers are consistent with the newADMIN_CODEimplementation.✅ Verification successful
Impact on
getLocalitiesFunction Verified Successfully
No issues found with thegetLocalitiesfunction related to the changes inADMIN_CODE. All usages oftenantBoundryare consistent, and there are no dependencies involvinghierarchyType.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the usage of tenantBoundry and hierarchyType in getLocalities # Search for getLocalities function calls echo "Searching for getLocalities function calls:" rg --type js 'getLocalities\s*\(' -A 5 # Search for tenantBoundry usage echo "\nSearching for tenantBoundry usage:" rg --type js 'tenantBoundry' # Search for hierarchyType usage in relation to tenantBoundry echo "\nSearching for hierarchyType usage in relation to tenantBoundry:" rg --type js 'tenantBoundry.*hierarchyType|hierarchyType.*tenantBoundry'Length of output: 6171
🧰 Tools
🪛 Biome
[error] 4-4: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
micro-ui/web/micro-ui-internals/example/src/setupProxy.js (1)
28-28: LGTM! Verify the new boundary service integration.The addition of the "/boundary-service" route to the proxy middleware is consistent with the existing code structure and follows the established pattern. This change appears to be correct and well-placed within the list of routes.
To ensure the new boundary service is properly integrated, please run the following verification script:
✅ Verification successful
Verified: Boundary service integration is correctly implemented.
The "/boundary-service" route has been properly added to the proxy middleware and is consistently utilized across multiple components and services within the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the boundary service integration # Test 1: Check if the boundary service is properly configured in the environment echo "Checking boundary service configuration..." rg --type js "REACT_APP_BOUNDARY_SERVICE" . # Test 2: Verify usage of the boundary service in the codebase echo "Checking boundary service usage..." rg --type js "boundary-service" . # Note: Review the output to ensure the boundary service is properly configured and used in the application.Length of output: 3279
micro-ui/web/micro-ui-internals/packages/react-components/src/molecules/LocationDropdownWrapper.js (1)
32-32: Please clarify the purpose of the additional argument inuseLocation.A second argument
truehas been added to theuseLocationhook call. The purpose and impact of this change are not immediately clear from the context provided.Could you please explain:
- What does this additional argument do?
- How does it affect the behavior of the
useLocationhook?- Are there any potential side effects or performance implications of this change?
To help verify the impact of this change, please run the following script:
This will help us understand if this change is consistent with other usages of the
useLocationhook in the codebase.micro-ui/web/micro-ui-internals/packages/libraries/src/services/atoms/urls.js (2)
3-3: LGTM: Well-implemented configuration constant.The
boundarySearchPathconstant is well-implemented. It uses optional chaining for safe access to the global configuration and provides a sensible default value. This approach enhances the configurability of the application while maintaining robustness.
13-15: LGTM: Improved configurability and query specificity.The updates to the
locationobject effectively utilize the new constants, enhancing configurability. The additional query parameters forlocalitiesandwardsprovide more precise and flexible queries.However, I noticed that
revenue_localitiesdoesn't use thehierarchyTypeconstant:revenue_localities: `/boundary-service/boundary-relationships/_search?hierarchyType=REVENUE&boundaryType=locality`,Is this intentional? If not, consider updating it to use the
boundarySearchPathconstant for consistency:revenue_localities: `/${boundarySearchPath}hierarchyType=REVENUE&boundaryType=locality`,micro-ui/web/micro-ui-internals/packages/modules/hrms/src/components/pageComponents/jurisdiction.js (4)
127-130: Verify the mapping ofcodeandnameingethierarchylistdata.In the
gethierarchylistdatafunction, bothcodeandnameare assigned the value ofele.hierarchyType. Please confirm if this is intentional. Typically,codeshould be a unique identifier, andnameshould be a human-readable label. Consider updatingnameto reflect a more descriptive property if available.
141-143: Properly handling additional loading states withisBoundaryServiceLoading.Including
isBoundaryServiceLoadingin the loading condition ensures that the loader is displayed correctly while boundary data is being fetched. This improvement enhances user experience during data loading.
165-165: PassinghierarchyOptionsprop toJurisdictioncomponent correctly.The
hierarchyOptionsprop is passed down to theJurisdictioncomponent, allowing it to access the necessary hierarchy data. This change ensures that the component functions with the updated data source.
186-186: IncludehierarchyOptionsin theJurisdictioncomponent's props.Adding
hierarchyOptionsto the component's props provides access to the updated hierarchy data within the component, enabling proper data handling and rendering.
| switch(locationType) { | ||
| case 'Locality': | ||
| return useQuery(["LOCALITY_DETAILS", tenantId ], () => LocationService.getLocalities(tenantId), config); | ||
| case 'Ward': | ||
| return useQuery(["WARD_DETAILS", tenantId ], () => LocationService.getWards(tenantId), config); | ||
| return useQuery(["WARD_DETAILS", tenantId], () => LocationService.getWards(tenantId,includeChildren,includeParents), config); |
There was a problem hiding this comment.
Update the query key to include new parameters for proper caching.
The 'Ward' case has been correctly updated to pass the new includeChildren and includeParents parameters to LocationService.getWards. However, the query key for this case hasn't been updated to reflect these new parameters.
To ensure proper caching behavior with React Query, update the query key to include the new parameters:
- return useQuery(["WARD_DETAILS", tenantId], () => LocationService.getWards(tenantId,includeChildren,includeParents), config);
+ return useQuery(["WARD_DETAILS", tenantId, includeChildren, includeParents], () => LocationService.getWards(tenantId, includeChildren, includeParents), config);This change will ensure that React Query creates separate cache entries for different combinations of includeChildren and includeParents, preventing potential issues with stale or incorrect data.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return useQuery(["WARD_DETAILS", tenantId], () => LocationService.getWards(tenantId,includeChildren,includeParents), config); | |
| return useQuery(["WARD_DETAILS", tenantId, includeChildren, includeParents], () => LocationService.getWards(tenantId, includeChildren, includeParents), config); |
| @@ -1,12 +1,12 @@ | |||
| import { useQuery } from 'react-query'; | |||
| import { LocationService } from "../services/elements/Location"; | |||
|
|
|||
| const useLocation = (tenantId, locationType, config = {}) => { | |||
| const useLocation = (tenantId, locationType, config = {},includeChildren,includeParents) => { | |||
There was a problem hiding this comment.
Consider adding default values for new parameters to maintain backward compatibility.
The function signature has been updated to include two new parameters, includeChildren and includeParents. While this change allows for more flexibility, it could potentially break existing code that calls this function without providing these new parameters.
To maintain backward compatibility, consider adding default values for these new parameters:
- const useLocation = (tenantId, locationType, config = {},includeChildren,includeParents) => {
+ const useLocation = (tenantId, locationType, config = {}, includeChildren = false, includeParents = false) => {This change would ensure that existing calls to useLocation continue to work as before, while still allowing the new functionality to be used when needed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const useLocation = (tenantId, locationType, config = {},includeChildren,includeParents) => { | |
| const useLocation = (tenantId, locationType, config = {}, includeChildren = false, includeParents = false) => { |
| getWards: (tenantId,includeChildren,includeParents) => { | ||
| return ServiceRequest({ | ||
| serviceName: "getWards", | ||
| url: Urls.location.wards, | ||
| params: { tenantId: tenantId }, | ||
| params: { tenantId: tenantId,includeChildren: includeChildren, includeParents}, | ||
| useCache: true, | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Update getWards method to correctly pass includeParents parameter
The changes to the getWards method look good overall. The method signature has been updated to include the new parameters, and they are being passed to the ServiceRequest function. However, there's a small issue with how the includeParents parameter is being passed.
Apply this diff to fix the includeParents parameter:
getWards: (tenantId,includeChildren,includeParents) => {
return ServiceRequest({
serviceName: "getWards",
url: Urls.location.wards,
- params: { tenantId: tenantId,includeChildren: includeChildren, includeParents},
+ params: { tenantId, includeChildren, includeParents },
useCache: true,
});
}This change ensures that:
- The
includeParentsparameter is correctly passed with its value. - We use object property shorthand for consistency and brevity.
Consider adding spaces after commas in the method signature for better readability:
getWards: (tenantId, includeChildren, includeParents) => {| localities[item?.code] = item?.children.map(item => ({ code: item.code, name: t(`${headerLocale}_ADMIN_${item?.code}`), i18nKey: `${headerLocale}_ADMIN_${item?.code}`, label: item?.label })) | ||
| wards.push({ code: item.code, name: t(`${headerLocale}_ADMIN_${item?.code}`), i18nKey: `${headerLocale}_ADMIN_${item?.code}` }) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Consider extracting the translation key construction.
The changes correctly implement localization for ward and locality names using the t function. This is a good practice for internationalization.
To improve readability, consider extracting the translation key construction into a separate function:
const getTranslationKey = (code) => `${headerLocale}_ADMIN_${code}`;
// Then use it like this:
localities[item?.code] = item?.children.map(item => ({
code: item.code,
name: t(getTranslationKey(item?.code)),
i18nKey: getTranslationKey(item?.code),
label: item?.label
}));
wards.push({
code: item.code,
name: t(getTranslationKey(item?.code)),
i18nKey: getTranslationKey(item?.code)
});This would make the code more DRY and easier to maintain.
| @@ -1,15 +1,18 @@ | |||
| const mdmsV1Path = window?.globalConfigs?.getConfig("MDMS_V1_CONTEXT_PATH") || "egov-mdms-service"; | |||
| const mdmsV2Path = window?.globalConfigs?.getConfig("MDMS_V2_CONTEXT_PATH") || "mdms-v2"; | |||
| const boundarySearchPath = window?.globalConfigs?.getConfig("BOUNDARY_CONTEXT") || "boundary-service/boundary-relationships/_search?"; | |||
| const hierarchyType = window?.globalConfigs?.getConfig("HIERARCHY_TYPE") || "hierarchyType=ADMIN"; | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM: Well-implemented configuration constant with a minor suggestion.
The hierarchyType constant is well-implemented, consistent with boundarySearchPath. However, consider simplifying the default value:
const hierarchyType = window?.globalConfigs?.getConfig("HIERARCHY_TYPE") || "ADMIN";Then, when using this constant, you can add the "hierarchyType=" prefix:
`hierarchyType=${hierarchyType}`This approach separates the configuration value from its usage, potentially making it more flexible for future changes.
| const { isBoundaryServiceLoading, data: hierarchyOptions } = Digit.Hooks.useLocation( | ||
| tenantId, | ||
| "Ward", | ||
| { | ||
| select: (data) => { | ||
| return data; | ||
| }, | ||
| }, | ||
| true | ||
| ); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Simplify the useLocation hook by removing the redundant select function.
The select function in the Digit.Hooks.useLocation hook is returning the data without any modification, making it unnecessary. Removing it will simplify the code without affecting its functionality.
Apply this diff to simplify the code:
const { isBoundaryServiceLoading, data: hierarchyOptions } = Digit.Hooks.useLocation(
tenantId,
"Ward",
- {
- select: (data) => {
- return data;
- },
- },
true
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { isBoundaryServiceLoading, data: hierarchyOptions } = Digit.Hooks.useLocation( | |
| tenantId, | |
| "Ward", | |
| { | |
| select: (data) => { | |
| return data; | |
| }, | |
| }, | |
| true | |
| ); | |
| const { isBoundaryServiceLoading, data: hierarchyOptions } = Digit.Hooks.useLocation( | |
| tenantId, | |
| "Ward", | |
| true | |
| ); | |
| const filteredBoundaryTypes = hierarchyOptions?.TenantBoundary?.filter((ele) => ele.hierarchyType === jurisdiction?.hierarchy?.code).map( | ||
| (item) => ({ | ||
| ...item.boundary[0], | ||
| i18text: Digit.Utils.locale.convertToLocale(item.boundary[0].boundaryType, "EGOV_LOCATION_BOUNDARYTYPE"), | ||
| }) | ||
| ); | ||
| }, [jurisdiction?.hierarchy, data?.MdmsRes]); | ||
| selectBoundaryType(filteredBoundaryTypes); | ||
| }, [jurisdiction?.hierarchy, hierarchyOptions]); |
There was a problem hiding this comment.
Ensure safe access to item.boundary[0] to prevent potential runtime errors.
In the mapping function within the useEffect hook, accessing item.boundary[0] without checking if item.boundary exists and contains elements could lead to runtime errors if item.boundary is undefined or empty. Please add a check to ensure item.boundary is defined and not empty before accessing item.boundary[0].
Apply this diff to add safety checks:
useEffect(() => {
const filteredBoundaryTypes = hierarchyOptions?.TenantBoundary?.filter(
(ele) => ele.hierarchyType === jurisdiction?.hierarchy?.code
).map((item) => {
+ if (!item.boundary || item.boundary.length === 0) {
+ return null; // or handle the case accordingly
+ }
return {
...item.boundary[0],
i18text: Digit.Utils.locale.convertToLocale(item.boundary[0].boundaryType, "EGOV_LOCATION_BOUNDARYTYPE"),
};
}).filter(Boolean);
selectBoundaryType(filteredBoundaryTypes);
}, [jurisdiction?.hierarchy, hierarchyOptions]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const filteredBoundaryTypes = hierarchyOptions?.TenantBoundary?.filter((ele) => ele.hierarchyType === jurisdiction?.hierarchy?.code).map( | |
| (item) => ({ | |
| ...item.boundary[0], | |
| i18text: Digit.Utils.locale.convertToLocale(item.boundary[0].boundaryType, "EGOV_LOCATION_BOUNDARYTYPE"), | |
| }) | |
| ); | |
| }, [jurisdiction?.hierarchy, data?.MdmsRes]); | |
| selectBoundaryType(filteredBoundaryTypes); | |
| }, [jurisdiction?.hierarchy, hierarchyOptions]); | |
| const filteredBoundaryTypes = hierarchyOptions?.TenantBoundary?.filter((ele) => ele.hierarchyType === jurisdiction?.hierarchy?.code).map( | |
| (item) => { | |
| if (!item.boundary || item.boundary.length === 0) { | |
| return null; // or handle the case accordingly | |
| } | |
| return { | |
| ...item.boundary[0], | |
| i18text: Digit.Utils.locale.convertToLocale(item.boundary[0].boundaryType, "EGOV_LOCATION_BOUNDARYTYPE"), | |
| }; | |
| } | |
| ).filter(Boolean); | |
| selectBoundaryType(filteredBoundaryTypes); | |
| }, [jurisdiction?.hierarchy, hierarchyOptions]); |
Summary by CodeRabbit
Release Notes
New Features
/boundary-serviceendpoint.useLocationhook with additional parameters for improved location data handling.Jurisdictioncomponent and refined hierarchy data fetching.Improvements
LocationDropdownWrappercomponent.Bug Fixes
getLocalitiesfunction to include full hierarchy information.