hrms edit and deactivate bug fixes #1655
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request introduce a new constant 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 (
|
| @@ -62,6 +63,9 @@ const Details = () => { | |||
| return <Loader />; | |||
| } | |||
|
|
|||
| console.log(":data of the employee", data); | |||
| console.log("role data is", data?.Employees?.[0]?.user.roles.map((ele) => t(`ACCESSCONTROL_ROLES_ROLES_` + ele?.code))); | |||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range comments (3)
micro-ui/web/micro-ui-internals/packages/modules/hrms/src/pages/createEmployee.js (2)
Line range hint
183-194: Consider refactoring role mapping for better maintainability.The nested array mapping could be simplified using array methods.
Consider this more maintainable approach:
- let roles = data?.Jurisdictions?.map((ele) => { - return ele.roles?.map((item) => { - if(isMultiRootTenant){ - item["tenantId"] = tenantId; - } - else{ - item["tenantId"] = ele.boundary; - } - return item; - }); - }); - const mappedroles = [].concat.apply([], roles); + const mappedroles = data?.Jurisdictions?.flatMap(ele => + ele.roles?.map(item => ({ + ...item, + tenantId: isMultiRootTenant ? tenantId : ele.boundary + })) + ) || [];
Line range hint
235-251: Enhance mutation error handling.The current error handling only displays the first error. Consider handling multiple errors and providing more detailed feedback.
Consider this enhancement:
mutationCreate.mutate( { Employees: Employees, }, { onError: (error, variables) => { + const errors = error?.response?.data?.Errors; + const errorMessage = errors?.length > 1 + ? `Multiple errors: ${errors.map(e => e.message).join(', ')}` + : errors?.[0]?.message || 'HRMS_CREATE_ERROR'; setShowToast({ key: "error", - label: error?.response?.data?.Errors?.[0]?.code ? error?.response?.data?.Errors?.[0]?.code : "HRMS_CREATE_ERROR", + label: errorMessage, }); }, onSuccess: async (data) => { navigateToAcknowledgement({ id: data?.Employees?.[0]?.code, message: "HRMS_CREATE_EMPLOYEE_RESPONSE_MESSAGE" }); }, } );micro-ui/web/micro-ui-internals/packages/modules/hrms/src/pages/EmployeeDetails.js (1)
Line range hint
1-238: Consider splitting the component for better maintainability.The
Detailscomponent is handling multiple responsibilities:
- Personal details display
- Jurisdiction details
- Assignment details
- Action management
- Modal handling
Consider breaking this into smaller, focused components:
EmployeePersonalDetailsEmployeeJurisdictionsEmployeeAssignmentsEmployeeActionsThis would improve:
- Code maintainability
- Testing
- Reusability
- Performance (through selective re-rendering)
Would you like assistance in restructuring this component?
🧰 Tools
🪛 Biome
[error] 67-67: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
- micro-ui/web/micro-ui-internals/packages/modules/hrms/src/pages/EditEmployee/EditForm.js (3 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/hrms/src/pages/EmployeeDetails.js (3 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/hrms/src/pages/createEmployee.js (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
micro-ui/web/micro-ui-internals/packages/modules/hrms/src/pages/EditEmployee/EditForm.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/modules/hrms/src/pages/EmployeeDetails.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/modules/hrms/src/pages/createEmployee.js (1)
Pattern
**/*.js: check
🪛 Biome
micro-ui/web/micro-ui-internals/packages/modules/hrms/src/pages/EmployeeDetails.js
[error] 67-67: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 177-177: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 177-177: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
🔇 Additional comments (5)
micro-ui/web/micro-ui-internals/packages/modules/hrms/src/pages/EditEmployee/EditForm.js (3)
85-87: Verify role visibility changes.The conditional role filtering based on
isMultiRootTenantsignificantly changes which roles are visible to users. Please ensure:
- This change aligns with the intended access control requirements
- No unintended role visibility issues are introduced
Let's check for potential role-related issues:
#!/bin/bash # Search for role filtering patterns in other components rg "roles\.filter.*tenantId" --type js
194-199:⚠️ Potential issueSecurity: Verify tenant role assignment logic.
The conditional tenantId assignment for roles is a critical security change. Please:
- Add unit tests to verify correct role-tenant mapping
- Document the security implications of this change
Let's check for similar role-tenant assignments:
#!/bin/bash # Search for similar role-tenant assignments rg "item\[.?tenantId.?\].*=.*tenant" --type jsFix the inconsistent indentation:
- if(isMultiRootTenant){ - item["tenantId"] = tenantId; - } - else{ - item["tenantId"] = ele.boundary; - } + if (isMultiRootTenant) { + item["tenantId"] = tenantId; + } else { + item["tenantId"] = ele.boundary; + }
17-17: Document the purpose and impact of isMultiRootTenant.This new configuration affects critical tenant-specific behavior. Please add:
- JSDoc comments explaining the purpose and impact of this flag
- Error handling in case getMultiRootTenant() fails
Let's check if this utility is properly defined:
micro-ui/web/micro-ui-internals/packages/modules/hrms/src/pages/createEmployee.js (2)
Line range hint
157-182: LGTM: Comprehensive validation checks added.The enhanced error handling includes critical validations for:
- Mandatory fields
- Current assignment presence
- Base tenant requirement
- Duplicate jurisdiction prevention
These checks will help prevent invalid data submission and improve user feedback.
Line range hint
15-15: Verify multi-tenant configuration initialization.The new
isMultiRootTenantconstant is critical for role management. Ensure thatDigit.Utils.getMultiRootTenant()is properly initialized during application bootstrap.✅ Verification successful
Based on the search results, I can now provide a comprehensive verification of the multi-tenant configuration initialization. Let me analyze the findings:
- The
getMultiRootTenant()function is defined inmicro-ui/web/micro-ui-internals/packages/libraries/src/utils/index.jsand it reads the configuration from window globals:const getMultiRootTenant = () => { return window?.globalConfigs?.getConfig("MULTI_ROOT_TENANT") || false; };
- The initialization of
isMultiRootTenantincreateEmployee.jsis properly using this utility function:const isMultiRootTenant = Digit.Utils.getMultiRootTenant();
- The configuration is consistently used across multiple modules (HRMS, PGR, Core) and affects various functionalities like:
- Tenant ID handling
- Role management
- URL transformations
- UI component behavior
Let me verify the bootstrap configuration by checking the application initialization:
Let me try a broader search to find the configuration initialization:
Let me check the sample global config and core initialization:
Based on the comprehensive analysis of the codebase, I can now provide the final verification response:
Multi-tenant configuration is properly initialized and safe to use
The
isMultiRootTenantconstant increateEmployee.jsis correctly implemented throughDigit.Utils.getMultiRootTenant(). This utility function:
- Is properly defined in the libraries package
- Safely reads from the global configuration with a fallback
- Is consistently used across the application
- Gets initialized during application bootstrap through the global config system
The implementation is safe because:
- The global config is initialized before the application renders
- It has a proper fallback value (false) if not configured
- The configuration is immutable (defined in a closure)
- The usage pattern is consistent across all modules (HRMS, PGR, Core)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for getMultiRootTenant initialization and usage rg -A 3 "getMultiRootTenant"Length of output: 36240
Script:
#!/bin/bash # Search for MULTI_ROOT_TENANT configuration initialization rg -A 3 "MULTI_ROOT_TENANT.*config"Length of output: 37
Script:
#!/bin/bash # Search for global config initialization patterns rg -l "globalConfigs|window\.config|MULTI_ROOT_TENANT" --type js # Also check any config files fd -e config.js -e config.jsonLength of output: 2607
Script:
#!/bin/bash # Check sample global config cat micro-ui/web/micro-ui-internals/sampleGlobalConfig.js # Check core App initialization cat micro-ui/web/core/App.jsLength of output: 3915
| console.log(":data of the employee", data); | ||
| console.log("role data is", data?.Employees?.[0]?.user.roles.map((ele) => t(`ACCESSCONTROL_ROLES_ROLES_` + ele?.code))); | ||
|
|
There was a problem hiding this comment.
Remove console.log statements.
Debug console logs should not be committed to production code. These statements expose sensitive employee data and role information which could be a security concern.
Apply this diff to remove the debug statements:
- console.log(":data of the employee", data);
- console.log("role data is", data?.Employees?.[0]?.user.roles.map((ele) => t(`ACCESSCONTROL_ROLES_ROLES_` + ele?.code)));
-📝 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.
| console.log(":data of the employee", data); | |
| console.log("role data is", data?.Employees?.[0]?.user.roles.map((ele) => t(`ACCESSCONTROL_ROLES_ROLES_` + ele?.code))); |
🧰 Tools
🪛 Biome
[error] 67-67: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- micro-ui/web/micro-ui-internals/packages/modules/hrms/src/pages/EmployeeDetails.js (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
micro-ui/web/micro-ui-internals/packages/modules/hrms/src/pages/EmployeeDetails.js (1)
Pattern
**/*.js: check
🪛 Biome
micro-ui/web/micro-ui-internals/packages/modules/hrms/src/pages/EmployeeDetails.js
[error] 175-175: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 175-175: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
🔇 Additional comments (1)
micro-ui/web/micro-ui-internals/packages/modules/hrms/src/pages/EmployeeDetails.js (1)
24-24: LGTM: Multi-root tenant configuration.The addition of
isMultiRootTenantconstant properly integrates with the existing utility functions and supports the PR's objective of fixing employee role management issues.
| text={ | ||
| isMultiRootTenant? data?.Employees?.[0]?.user.roles.map((ele) => t(`ACCESSCONTROL_ROLES_ROLES_` + ele?.code)):(data?.Employees?.[0]?.user.roles.filter((ele) => ele.tenantId == element?.boundary).map((ele) => t(`ACCESSCONTROL_ROLES_ROLES_` + ele?.code)))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve role display logic for better maintainability and safety.
The current implementation has several areas for improvement:
- Uses string concatenation instead of template literals
- Uses loose equality comparison
- Lacks null safety checks
- Has complex nested logic
Consider this enhanced version of the previous refactoring suggestion:
text={
- isMultiRootTenant? data?.Employees?.[0]?.user.roles.map((ele) => t(`ACCESSCONTROL_ROLES_ROLES_` + ele?.code)):(data?.Employees?.[0]?.user.roles.filter((ele) => ele.tenantId == element?.boundary).map((ele) => t(`ACCESSCONTROL_ROLES_ROLES_` + ele?.code)))}
+ (() => {
+ const roles = data?.Employees?.[0]?.user?.roles;
+ if (!roles?.length) return 'NA';
+
+ const filteredRoles = isMultiRootTenant
+ ? roles
+ : roles.filter((role) => role?.tenantId === element?.boundary);
+
+ return filteredRoles.map((role) =>
+ t(`ACCESSCONTROL_ROLES_ROLES_${role?.code || ''}`)
+ );
+ })()
+ }This refactoring:
- Uses template literals (fixes static analysis warning)
- Adds comprehensive null safety checks
- Uses strict equality comparison
- Handles empty roles gracefully
- Improves readability by breaking down the logic
- Uses meaningful variable names
📝 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.
| text={ | |
| isMultiRootTenant? data?.Employees?.[0]?.user.roles.map((ele) => t(`ACCESSCONTROL_ROLES_ROLES_` + ele?.code)):(data?.Employees?.[0]?.user.roles.filter((ele) => ele.tenantId == element?.boundary).map((ele) => t(`ACCESSCONTROL_ROLES_ROLES_` + ele?.code)))} | |
| text={ | |
| (() => { | |
| const roles = data?.Employees?.[0]?.user?.roles; | |
| if (!roles?.length) return 'NA'; | |
| const filteredRoles = isMultiRootTenant | |
| ? roles | |
| : roles.filter((role) => role?.tenantId === element?.boundary); | |
| return filteredRoles.map((role) => | |
| t(`ACCESSCONTROL_ROLES_ROLES_${role?.code || ''}`) | |
| ); | |
| })() | |
| } |
🧰 Tools
🪛 Biome
[error] 175-175: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 175-175: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
Choose the appropriate template for your PR:
hrms Bug fixes
Summary by CodeRabbit
New Features
Bug Fixes
Chores