Pfm 1419 user search #610
Pfm 1419 user search #610
Conversation
…atch-1 Update build-config.yml
…d on user role tenantid
…d on user role tenantid
…d on user role tenantid
…d on user role tenantid
…d on user role tenantid
WalkthroughThe updates enhance the HRMS and User services by introducing new search criteria, including state-level searches and tenant-based user retrievals. Changes include expanded jurisdiction limits, new methods for employee and user searches, and additional properties for endpoint configuration. The adjustments streamline data handling, improve search capabilities, and refine the system's flexibility in managing user and employee information across different tenants and jurisdictions. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 10
Configuration used: CodeRabbit UI
Files selected for processing (18)
- business-services/egov-hrms/src/main/java/org/egov/hrms/config/PropertiesManager.java (1 hunks)
- business-services/egov-hrms/src/main/java/org/egov/hrms/model/Employee.java (1 hunks)
- business-services/egov-hrms/src/main/java/org/egov/hrms/service/EmployeeService.java (6 hunks)
- business-services/egov-hrms/src/main/java/org/egov/hrms/service/UserService.java (3 hunks)
- business-services/egov-hrms/src/main/java/org/egov/hrms/utils/HRMSConstants.java (2 hunks)
- business-services/egov-hrms/src/main/java/org/egov/hrms/web/contract/EmployeeSearchByTenantRequestWrapper.java (1 hunks)
- business-services/egov-hrms/src/main/java/org/egov/hrms/web/contract/EmployeeSearchCriteria.java (1 hunks)
- business-services/egov-hrms/src/main/java/org/egov/hrms/web/controller/EmployeeController.java (4 hunks)
- business-services/egov-hrms/src/main/java/org/egov/hrms/web/validator/EmployeeValidator.java (3 hunks)
- business-services/egov-hrms/src/main/resources/application.properties (2 hunks)
- core-services/egov-user/src/main/java/org/egov/user/config/UserServiceConstants.java (1 hunks)
- core-services/egov-user/src/main/java/org/egov/user/domain/model/UserSearchCriteria.java (1 hunks)
- core-services/egov-user/src/main/java/org/egov/user/domain/service/UserService.java (3 hunks)
- core-services/egov-user/src/main/java/org/egov/user/persistence/repository/UserRepository.java (3 hunks)
- core-services/egov-user/src/main/java/org/egov/user/repository/builder/UserTypeQueryBuilder.java (3 hunks)
- core-services/egov-user/src/main/java/org/egov/user/web/contract/UserSearchByTenantsRequest.java (1 hunks)
- core-services/egov-user/src/main/java/org/egov/user/web/contract/UserSearchRequest.java (2 hunks)
- core-services/egov-user/src/main/java/org/egov/user/web/controller/UserController.java (1 hunks)
Additional comments: 17
business-services/egov-hrms/src/main/java/org/egov/hrms/web/contract/EmployeeSearchByTenantRequestWrapper.java (1)
- 1-33: The
EmployeeSearchByTenantRequestWrapperclass is well-structured and correctly uses validation and JSON serialization annotations. The use of@NotNullforrequestInfoand@Validforcriteriaensures proper validation of request data, which is essential for processing search requests effectively.core-services/egov-user/src/main/java/org/egov/user/web/contract/UserSearchRequest.java (1)
- 73-75: The addition of
isStateLevelSearchtoUserSearchRequestis well-implemented, including its mapping in thetoDomainmethod. Consider if any validation is needed for this field, although its boolean nature might not require it.core-services/egov-user/src/main/java/org/egov/user/domain/model/UserSearchCriteria.java (1)
- 32-33: The addition of
tenantIdsandisStateLevelSearchfields inUserSearchCriteriasupports the PR's objectives for enhanced search capabilities. Ensure these new fields are appropriately considered in the overall search logic and validation processes.business-services/egov-hrms/src/main/java/org/egov/hrms/web/controller/EmployeeController.java (2)
- 121-126: The method
_searchListOfEmployeeintroduces a new endpoint for searching a list of employees based on tenant-specific criteria. Ensure that theEmployeeSearchByTenantRequestWrapperis properly validated within theEmployeeService.searchListOfEmployeemethod to prevent any potential misuse or errors due to invalid input.- 139-147: The
countV1method provides a way to count employees based on roles and search criteria, including a state-level search flag. It's important to ensure that thevalidator.validateEmployeeCountRequestmethod adequately checks the validity of thetenantIdand other parameters to prevent SQL injection or other security vulnerabilities. Additionally, consider handling potential exceptions that might arise from theemployeeService.getEmployeeCountResponseV1call to ensure the system's robustness.business-services/egov-hrms/src/main/java/org/egov/hrms/service/UserService.java (3)
- 81-83: The addition of the
userSearchByTenantEndpointfield with its corresponding value injection is a good practice for externalizing configuration settings. Ensure that the property is correctly defined in theapplication.propertiesfile and that it points to a valid and secure endpoint.- 130-145: The
getUserByTenantidsmethod is a valuable addition for retrieving users based on specific tenant IDs. Ensure that the inputUserSearchCriteriais properly sanitized and validated to prevent SQL injection or other security vulnerabilities. Additionally, consider handling potential exceptions that might arise from theuserCallmethod to ensure the system's robustness.- 157-157: The update to the
userCallmethod to handle the newuserSearchByTenantEndpointis crucial for supporting the enhanced search functionality. Ensure that the conditional check for including this endpoint in thedobFormatassignment is correct and that it doesn't inadvertently affect other functionalities.core-services/egov-user/src/main/java/org/egov/user/web/controller/UserController.java (1)
- 222-243: The
getUserBytenantmethod and its helpersearchUsersBytenantintroduce a new endpoint for searching users by tenant IDs. Ensure that theUserSearchByTenantsRequestis properly validated to prevent any potential misuse or errors due to invalid input. Additionally, consider handling potential exceptions that might arise from theuserService.searchUsersByTenantscall to ensure the system's robustness. It's also important to ensure that the search criteria do not expose sensitive user information without proper authorization checks.core-services/egov-user/src/main/java/org/egov/user/repository/builder/UserTypeQueryBuilder.java (3)
- 48-48: The addition of
import org.springframework.util.ObjectUtils;is appropriate for the new logic introduced in this file, which utilizesObjectUtils.isEmpty()to check for null or empty values in a clean and concise manner.- 141-145: The logic added to handle
userSearchCriteria.getTenantIds()for building the query correctly checks iftenantIdsis not null and then appends the necessary SQL to include these IDs in the search criteria. This is a crucial enhancement for supporting searches across multiple tenants.- 244-261: The modifications for handling
userSearchCriteria.getTenantId()based onuserSearchCriteria.getIsStateLevelSearch()are well-implemented. The logic correctly differentiates between state-level searches and more specific tenant ID searches, adjusting the query accordingly. This allows for flexible search capabilities, aligning with the PR's objectives to enhance search functionalities.core-services/egov-user/src/main/java/org/egov/user/persistence/repository/UserRepository.java (2)
- 89-89: Changing the logging level from
debugtoinfofor the query strings in thefindAllmethod is appropriate for ensuring that important query information is logged at a level that is more likely to be captured in production environments, aiding in troubleshooting and monitoring.- 575-607: The addition of the
findUserByTenantmethod introduces new logic to search users based on role and tenant criteria. This method correctly handles the scenario where role codes are provided and tenant IDs are specified, allowing for a more granular search. The method also ensures that if no matching user IDs are found based on the role search, an empty list is returned early, which is a good practice for performance. The use oflog.infoto log the search query is consistent with the logging level adjustments made elsewhere in this file.business-services/egov-hrms/src/main/java/org/egov/hrms/service/EmployeeService.java (2)
- 65-65: The import of
ObjectUtilsis added to support the new checks in the search functionality. Ensure that this import is used effectively throughout the code and there are no unused imports to maintain code cleanliness.- 596-640: The
searchListOfEmployeemethod introduces a new way to search for employees based on roles and tenant IDs. This method is crucial for the expanded search capabilities. However, ensure that the method efficiently handles large datasets and consider optimizing the search queries to prevent potential performance issues. Additionally, verify that all edge cases are handled, such as whencriteria.getTenantIds()orcriteria.getRoles()are empty, to avoid unnecessary API calls or database queries.core-services/egov-user/src/main/java/org/egov/user/domain/service/UserService.java (1)
- 194-194: Encryption of
UserSearchCriteriais performed before callinguserRepository.findAllanduserRepository.findUserByTenant. While this is crucial for security, ensure that theuserRepositorymethods are designed to handle encrypted search criteria effectively. If these methods expect plaintext criteria, this could lead to no results being found due to mismatched query parameters.Also applies to: 669-669
| @@ -234,7 +234,7 @@ private void validateMdmsData(Employee employee, Map<String, String> errorMap, M | |||
| validateEmployee(employee, errorMap, mdmsData); | |||
| validateAssignments(employee, errorMap, mdmsData); | |||
| validateServiceHistory(employee, errorMap, mdmsData); | |||
| validateJurisdicton(employee, errorMap, mdmsData, boundaryMap); | |||
| //validateJurisdicton(employee, errorMap, mdmsData, boundaryMap); | |||
There was a problem hiding this comment.
Commenting out the call to validateJurisdicton in the validateMdmsData method removes jurisdiction-related validation. Ensure that this change aligns with the new requirements and does not inadvertently bypass necessary validation checks that could lead to inconsistent or invalid data being processed.
| @@ -251,7 +251,7 @@ private void validateMdmsData(Employee employee, Map<String, String> errorMap, M | |||
| public void validateDataConsistency(Employee employee, Map<String, String> errorMap, Map<String, List<String>> mdmsData, Employee existingEmp, RequestInfo requestInfo) { | |||
| validateUserData(existingEmp,employee,errorMap, requestInfo); | |||
| validateConsistencyAssignment(existingEmp,employee,errorMap); | |||
| validateConsistencyJurisdiction(existingEmp,employee,errorMap); | |||
| //validateConsistencyJurisdiction(existingEmp,employee,errorMap); | |||
There was a problem hiding this comment.
Similarly, commenting out the call to validateConsistencyJurisdiction in the validateDataConsistency method skips checks for jurisdiction consistency during data updates. This could potentially allow for jurisdiction data inconsistencies if not handled elsewhere. Confirm that this change is intentional and that consistency checks are performed through other means if necessary.
| if(!hierarchyTypes.contains(jurisdiction.getHierarchy())) { | ||
| errorMap.put(ErrorConstants.HRMS_INVALID_JURISDICTION_HEIRARCHY_CODE, ErrorConstants.HRMS_INVALID_JURISDICTION_HEIRARCHY_MSG); | ||
| if(!boundaryTypes.contains(jurisdiction.getBoundaryType())) | ||
| } | ||
| if(!boundaryTypes.contains(jurisdiction.getBoundaryType())) { | ||
| errorMap.put(ErrorConstants.HRMS_INVALID_JURISDICTION_BOUNDARY_TYPE_CODE, ErrorConstants.HRMS_INVALID_JURISDICTION_BOUNDARY_TYPE_MSG); | ||
| if(!boundaryValues.contains(jurisdiction.getBoundary())) | ||
| } | ||
| if(!boundaryValues.contains(jurisdiction.getBoundary())) { | ||
| errorMap.put(ErrorConstants.HRMS_INVALID_JURISDICTION_BOUNDARY_CODE, ErrorConstants.HRMS_INVALID_JURISDICTION_BOUNDARY_MSG); | ||
| } |
There was a problem hiding this comment.
The adjustments within the validateJurisdicton method, although the method itself is commented out, show an attempt to refine boundary checks. If re-enabling this validation in the future, ensure that these boundary checks are thoroughly tested to prevent any false negatives or positives in jurisdiction validation.
Consider revising the approach to jurisdiction validation to ensure it is both efficient and comprehensive, covering all necessary boundary and hierarchy checks without introducing performance bottlenecks.
| if(!ObjectUtils.isEmpty(criteria.isStateLevelSearch)) | ||
| userSearchCriteria.put(HRMSConstants.HRMS_IS_STATE_LEVEL_SEARCH_CODE, criteria.getIsStateLevelSearch()); | ||
| if(!ObjectUtils.isEmpty(criteria.getIsActive())) | ||
| userSearchCriteria.put(HRMSConstants.HRMS_IS_ACTIVE_SEARCH_CODE, criteria.getIsActive()); |
There was a problem hiding this comment.
The addition of checks for isStateLevelSearch and isActive in the search criteria is crucial for supporting the new search functionalities. However, ensure that the ObjectUtils.isEmpty method is correctly used to check for null or default values, as it might not be the most intuitive choice for boolean fields. Consider using more explicit checks for boolean fields to improve code readability.
- if(!ObjectUtils.isEmpty(criteria.isStateLevelSearch))
+ if(criteria.isStateLevelSearch != null)
- if(!ObjectUtils.isEmpty(criteria.getIsActive()))
+ if(criteria.getIsActive() != null)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.
| if(!ObjectUtils.isEmpty(criteria.isStateLevelSearch)) | |
| userSearchCriteria.put(HRMSConstants.HRMS_IS_STATE_LEVEL_SEARCH_CODE, criteria.getIsStateLevelSearch()); | |
| if(!ObjectUtils.isEmpty(criteria.getIsActive())) | |
| userSearchCriteria.put(HRMSConstants.HRMS_IS_ACTIVE_SEARCH_CODE, criteria.getIsActive()); | |
| if(criteria.isStateLevelSearch != null) | |
| userSearchCriteria.put(HRMSConstants.HRMS_IS_STATE_LEVEL_SEARCH_CODE, criteria.getIsStateLevelSearch()); | |
| if(criteria.getIsActive() != null) | |
| userSearchCriteria.put(HRMSConstants.HRMS_IS_ACTIVE_SEARCH_CODE, criteria.getIsActive()); |
| public Map<String,Object> getEmployeeCountResponseV1(RequestInfo requestInfo, List<String> roles, String tenantId, boolean isStateLevelSearch){ | ||
| EmployeeSearchCriteria activeEmployeeCriteria= EmployeeSearchCriteria.builder().roles(roles).tenantId(tenantId).isStateLevelSearch(isStateLevelSearch).isActive(true).build(); | ||
| EmployeeResponse activeEmployeeResponse = search(activeEmployeeCriteria, requestInfo); | ||
| EmployeeSearchCriteria inActiveEmployeeCriteria= EmployeeSearchCriteria.builder().roles(roles).tenantId(tenantId).isStateLevelSearch(isStateLevelSearch).isActive(false).build(); | ||
| EmployeeResponse inActiveEmployeeResponse = search(inActiveEmployeeCriteria, requestInfo); | ||
| Integer activeEmployeeCount= activeEmployeeResponse.getEmployees().size(); | ||
| Integer inActiveEmployeeCount = inActiveEmployeeResponse.getEmployees().size(); | ||
| Integer totalcount = activeEmployeeCount + inActiveEmployeeCount; | ||
| Map<String,String> results = new HashMap<>(); | ||
| Map<String,Object> response = new HashMap<>(); | ||
| ResponseInfo responseInfo = factory.createResponseInfoFromRequestInfo(requestInfo, true); | ||
|
|
||
| response.put("ResponseInfo",responseInfo); | ||
|
|
||
| if(totalcount == 0){ | ||
| Map<String,String> error = new HashMap<>(); | ||
| error.put("NO_RECORDS","No records found for the tenantId: "+tenantId); | ||
| throw new CustomException(error); | ||
| } | ||
| results.put("totalEmployee",totalcount.toString()); | ||
| results.put("activeEmployee",activeEmployeeCount.toString()); | ||
| results.put("inactiveEmployee",inActiveEmployeeCount.toString()); | ||
|
|
||
| response.put("EmployeCount",results); | ||
| return response; | ||
| } |
There was a problem hiding this comment.
The getEmployeeCountResponseV1 method is a significant addition that calculates employee counts based on different criteria. This method is well-structured and aligns with the PR's objectives. However, consider handling the potential CustomException more gracefully by providing more detailed error messages or alternative solutions when no records are found. This can enhance the user experience by offering clearer guidance on what actions to take next.
| import org.egov.hrms.web.contract.EmployeeResponse; | ||
| import org.egov.hrms.web.contract.EmployeeSearchCriteria; | ||
| import org.egov.hrms.web.contract.RequestInfoWrapper; | ||
| import org.egov.hrms.web.contract.*; |
There was a problem hiding this comment.
Using wildcard imports (import org.egov.hrms.web.contract.*;) can lead to namespace pollution and make it harder to identify where specific classes are coming from. Consider importing only the classes that are actually used.
| public Boolean isStateLevelSearch; | ||
|
|
||
| @Size(max = 250) | ||
| public String tenantId; | ||
|
|
||
| public List<String> tenantIds; |
There was a problem hiding this comment.
The addition of isStateLevelSearch and tenantIds fields enhances search capabilities. However, consider adding validation annotations (e.g., @NotNull for isStateLevelSearch, @NotEmpty for tenantIds) to ensure these fields are used correctly. Also, review the isCriteriaEmpty method to account for these new fields.
@NotNull
public Boolean isStateLevelSearch;
@NotEmpty
public List<String> tenantIds;| if(!ObjectUtils.isEmpty(searchCriteria.getIsStateLevelSearch())) { | ||
| if (searchCriteria.getIsStateLevelSearch()) { | ||
| searchCriteria.setTenantId(getStateLevelTenantForCitizen(searchCriteria.getTenantId(), searchCriteria.getType())); | ||
| } | ||
| } else { | ||
| searchCriteria.setTenantId(getStateLevelTenantForCitizen(searchCriteria.getTenantId(), searchCriteria.getType())); | ||
| } |
There was a problem hiding this comment.
The logic for handling isStateLevelSearch seems to be redundant. If isStateLevelSearch is not provided (null), the code defaults to treating the search as a state-level search by calling getStateLevelTenantForCitizen with the current tenantId and type. This could lead to unexpected behavior if the intention was to perform a non-state-level search but the flag was omitted. Consider clarifying the intended behavior or providing a default value for isStateLevelSearch that aligns with the most common use case.
| if(ObjectUtils.isEmpty(searchCriteria.getTenantIds())) { | ||
| return null; |
There was a problem hiding this comment.
Returning null when searchCriteria.getTenantIds() is empty could lead to potential NullPointerExceptions in the calling code. It's generally safer to return an empty list instead, which avoids the need for null checks and is more consistent with the principle of least astonishment for API consumers.
- return null;
+ return Collections.emptyList();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.
| if(ObjectUtils.isEmpty(searchCriteria.getTenantIds())) { | |
| return null; | |
| if(ObjectUtils.isEmpty(searchCriteria.getTenantIds())) { | |
| return Collections.emptyList(); |
| list = encryptionDecryptionUtil.decryptObject(list, "UserList", User.class, requestInfo); | ||
|
|
||
| setFileStoreUrlsByFileStoreIds(list); | ||
| return list; |
There was a problem hiding this comment.
The decryption of the user list and setting file store URLs are repeated in multiple methods (searchUsers and searchUsersByTenants). This repetition suggests a potential for refactoring to improve code maintainability and reduce the risk of future changes missing one of the instances. Consider extracting these operations into a separate method that can be called from both places.
private List<User> decryptAndSetFileStoreUrls(List<User> users, RequestInfo requestInfo) {
users = encryptionDecryptionUtil.decryptObject(users, "UserList", User.class, requestInfo);
setFileStoreUrlsByFileStoreIds(users);
return users;
}And then replace the repeated code blocks with calls to this new method.
Summary by CodeRabbit
New Features
Improvements
Refactor
Configuration