sandbox enhancement #1411
sandbox enhancement #1411
Conversation
nabeelmd-eGov
commented
Sep 20, 2024
nabeelmd-eGov
commented
- Added CustomErrorComponent, RoleBasedEmployeeHome, css enhancement
- added new localisation module
- upgrade ui component version
- adding coutn check of master
- adding count check of master
WalkthroughWalkthroughThe changes encompass enhancements to the styling and functionality of the sandbox component within the application. New CSS classes have been introduced, existing hooks have been modified to include new functionalities, and state management has been improved in the Changes
Possibly related PRs
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: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/sandbox.scss (2 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/hooks/index.js (1 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/hooks/useGetMasterDataCount.js (1 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/applicationMgmt/SetupMaster.js (6 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/applicationMgmt/config/setupMasterConfig.js (5 hunks)
Additional context used
Path-based instructions (4)
micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/hooks/index.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/modules/sandbox/src/hooks/useGetMasterDataCount.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/applicationMgmt/SetupMaster.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/applicationMgmt/config/setupMasterConfig.js (1)
Pattern
**/*.js: check
Additional comments not posted (18)
micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/hooks/index.js (2)
4-4: LGTM!The import statement for the
useGetMasterDataCounthook follows the correct syntax and naming convention. The import path aligns with the file structure.
9-9: Looks good!The
useGetMasterDataCounthook is correctly added as a property of thesandboxobject. This addition expands the functionality of thesandboxobject, providing a way to retrieve the count of master data, which could be useful for various parts of the application.micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/hooks/useGetMasterDataCount.js (2)
2-18: LGTM!The
getMasterDataCountServicefunction is implemented correctly:
- It makes an API call to fetch the count of master data based on the provided filter and tenantId.
- It handles errors by throwing a custom error with the error description from the API response.
- The function logic is clear and follows the expected behavior.
20-22: LGTM!The
useGetMasterDataCountcustom hook is implemented correctly:
- It uses the
useQueryhook from thereact-querylibrary to manage the API call state.- It passes the necessary parameters to the
getMasterDataCountServicefunction to fetch the count of master data.- The hook provides a convenient way to fetch and manage the count of master data in the React components.
micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/applicationMgmt/config/setupMasterConfig.js (4)
1-1: LGTM!The change to make
setupMasterConfiga function that acceptsexistingUseris a valid refactoring. It allows for dynamic generation of the configuration based on the user's status while maintaining backward compatibility with the default value offalse.
11-11: LGTM!The change to conditionally set the
actionTextbased onexistingUseris a valid enhancement. It improves the user experience by displaying "EDIT_NOW" for existing users and "SETUP_NOW" for new users.
49-49: LGTM!The change to conditionally set the
actionTextbased onexistingUserfor the "HCM" setup item is consistent with the previous change for the "COMPLAINT_MANAGEMENT" setup item. It improves the user experience by displaying appropriate action text based on the user's status.
63-63: LGTM!The change to conditionally set the
actionTextbased onexistingUserfor the "HRMS" setup item is consistent with the previous changes for the "COMPLAINT_MANAGEMENT" and "HCM" setup items. It improves the user experience by displaying appropriate action text based on the user's status.micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/sandbox.scss (2)
295-306: LGTM!The new CSS class and its child element styles are well-structured and serve a specific purpose for the sandbox setup master info component. The styles for
.h1.headerFlexhelp in aligning the child elements horizontally with a gap, while the styles for.digit-card-header.subHeadercustomize the font size, weight, and color of the subheader appropriately.
388-394: Looks good!The reformatting of the nested structure within
.pgr-citizen-wrapperimproves the readability and consistency of the CSS. The styles for.date-wrapand.status-highlightensure proper alignment and centering of their child elements while maintaining the core layout and functionality. The changes enhance the code quality without altering the intended behavior.Also applies to: 399-406
micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/applicationMgmt/SetupMaster.js (8)
18-19: State initialization is appropriateThe initialization of
filtersandisUserExiststate variables usinguseStatehooks is appropriate and sets up the component state correctly.
21-22: Dependency addition to useMemo hook is correctIncluding
isUserExistin the dependencies of theuseMemohook ensures that the configuration updates whenisUserExistchanges. This is necessary for the component to respond to state changes accurately.
30-50: Data processing logic is soundThe
selectfunction within the custom MDMS hook processesmoduleMasterDatacorrectly to generaterespDataandmoduleMasterPayload. The use of filtering and mapping is appropriate for transforming the data.
57-61:useEffecthook correctly updatesfiltersstateThe
useEffecthook updatesfilterswhenmoduleMasterDataormoduleMasterLoadingchanges. This ensures that the component's state remains in sync with the fetched data.
79-83:useEffecthook properly setsisUserExistThe
useEffecthook updatesisUserExistbased onmasterCount, ensuring the component reacts appropriately to the loading state.
150-153: Button label conditionally renders based onisUserExistThe conditional rendering of the button label enhances user experience by displaying
EDIT_MASTERorconfig.actionTextaccordingly. This provides clarity to the user based on their current state.
Line range hint
176-188: Data passed to theTablecomponent is correctPassing
moduleMasterData.respDatato thedataprop of theTablecomponent ensures that the table displays the correct information fetched from the server.
236-246: Conditional navigation logic is implemented correctlyThe
onClickhandler usesisUserExistto determine whether to navigate to a different route or invokehandleSetupMaster. This conditional logic correctly handles user interaction based on the application's state.
| if (moduleMasterLoading && masterCountLoading) { | ||
| return <Loader />; | ||
| } |
There was a problem hiding this comment.
Loader condition should use logical OR
The loading condition uses logical AND (&&), which means the loader will only show if both moduleMasterLoading and masterCountLoading are true. To display the loader when either of them is loading, use logical OR (||).
Proposed fix:
- if (moduleMasterLoading && masterCountLoading) {
+ if (moduleMasterLoading || masterCountLoading) {
return <Loader />;
}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.
| if (moduleMasterLoading && masterCountLoading) { | |
| return <Loader />; | |
| } | |
| if (moduleMasterLoading || masterCountLoading) { | |
| return <Loader />; | |
| } |
| const { isLoading: masterCountLoading, data: masterCount } = Digit.Hooks.sandbox.useGetMasterDataCount({ | ||
| tenantId: tenantId, | ||
| filter: filters, | ||
| config: { | ||
| enabled: Boolean(filters), | ||
| select: (data) => { | ||
| const resp = data?.MdmsRes; | ||
| const checkMasterDataCompleteness = Object.values(resp).every((category) => | ||
| Object.values(category).every((items) => items.every((item) => parseInt(item.count) > 0)) | ||
| ); | ||
|
|
||
| return checkMasterDataCompleteness; | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Add null-check for resp before processing
In the select function of useGetMasterDataCount, if data is undefined, data?.MdmsRes will be undefined, leading to an error when calling Object.values(resp). To prevent a potential runtime error, add a null-check for resp before proceeding.
Proposed fix:
select: (data) => {
const resp = data?.MdmsRes;
+ if (!resp) return false;
const checkMasterDataCompleteness = Object.values(resp).every((category) =>
Object.values(category).every((items) => items.every((item) => parseInt(item.count) > 0))
);
return checkMasterDataCompleteness;
},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 { isLoading: masterCountLoading, data: masterCount } = Digit.Hooks.sandbox.useGetMasterDataCount({ | |
| tenantId: tenantId, | |
| filter: filters, | |
| config: { | |
| enabled: Boolean(filters), | |
| select: (data) => { | |
| const resp = data?.MdmsRes; | |
| const checkMasterDataCompleteness = Object.values(resp).every((category) => | |
| Object.values(category).every((items) => items.every((item) => parseInt(item.count) > 0)) | |
| ); | |
| return checkMasterDataCompleteness; | |
| }, | |
| }, | |
| }); | |
| const { isLoading: masterCountLoading, data: masterCount } = Digit.Hooks.sandbox.useGetMasterDataCount({ | |
| tenantId: tenantId, | |
| filter: filters, | |
| config: { | |
| enabled: Boolean(filters), | |
| select: (data) => { | |
| const resp = data?.MdmsRes; | |
| if (!resp) return false; | |
| const checkMasterDataCompleteness = Object.values(resp).every((category) => | |
| Object.values(category).every((items) => items.every((item) => parseInt(item.count) > 0)) | |
| ); | |
| return checkMasterDataCompleteness; | |
| }, | |
| }, | |
| }); |
| const moduleMasterPayload = respStructure | ||
| ?.filter((i) => i.type === "common" || i.type === "module") | ||
| ?.map((item) => { | ||
| return { | ||
| moduleName: item?.code?.split(".")?.[0], | ||
| masterDetails: [ | ||
| { | ||
| name: item?.code?.split(".")?.[1], | ||
| }, | ||
| ], | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Optimize by avoiding repeated splitting of item.code
In the mapping function, item.code is split multiple times. To improve performance and readability, consider splitting once and storing the result in a variable.
Proposed refactor:
const moduleMasterPayload = respStructure
?.filter((i) => i.type === "common" || i.type === "module")
?.map((item) => {
+ const codeParts = item?.code?.split(".");
return {
- moduleName: item?.code?.split(".")?.[0],
+ moduleName: codeParts?.[0],
masterDetails: [
{
- name: item?.code?.split(".")?.[1],
+ name: codeParts?.[1],
},
],
};
});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 moduleMasterPayload = respStructure | |
| ?.filter((i) => i.type === "common" || i.type === "module") | |
| ?.map((item) => { | |
| return { | |
| moduleName: item?.code?.split(".")?.[0], | |
| masterDetails: [ | |
| { | |
| name: item?.code?.split(".")?.[1], | |
| }, | |
| ], | |
| }; | |
| }); | |
| const moduleMasterPayload = respStructure | |
| ?.filter((i) => i.type === "common" || i.type === "module") | |
| ?.map((item) => { | |
| const codeParts = item?.code?.split("."); | |
| return { | |
| moduleName: codeParts?.[0], | |
| masterDetails: [ | |
| { | |
| name: codeParts?.[1], | |
| }, | |
| ], | |
| }; | |
| }); |
This reverts commit 78eecd1.
