Muster roll service configurability and hcm payment changes #1854
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces extensive configuration and functionality enhancements for the muster roll service. The changes span multiple files and focus on adding configurable options for workflow management, attendance tracking, and user interactions. Key modifications include new configuration properties in the Changes
Sequence DiagramsequenceDiagram
participant Client
participant MusterRollController
participant MusterRollService
participant MusterRollValidator
participant WorkflowService
participant NotificationService
Client->>MusterRollController: Create/Update Muster Roll
MusterRollController->>MusterRollService: Process Request
MusterRollService->>MusterRollValidator: Validate Request
alt Workflow Enabled
MusterRollService->>WorkflowService: Update Workflow Status
end
alt Notification Enabled
MusterRollService->>NotificationService: Send Notifications
end
MusterRollService-->>Client: Return Muster Roll Response
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
Documentation and Community
|
This reverts commit e50e62c.
holashchand
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
🔭 Outside diff range comments (3)
backend/muster-roll/src/main/java/org/egov/util/MusterRollServiceConstants.java (1)
Line range hint
1-43: Consider grouping related constants for better maintainability.The constants in this file would benefit from logical grouping. Consider organizing them into sections with comments, grouping related constants like:
- Workflow actions (
ACTION_*,WF_*)- Status constants (
STATUS_*)- Event types (
*_EVENT)- MDMS related (
MDMS_*,MASTER_*)- Notification codes (
*_NOTIFICATION_*)Example structure:
public class MusterRollServiceConstants { // Workflow Actions public static final String ACTION_APPROVE = "APPROVE"; public static final String ACTION_REJECT = "REJECT"; public static final String WF_APPROVE_CODE = "APPROVE"; public static final String WF_SEND_BACK_TO_CBO_CODE = "SENDBACKTOCBO"; // Status Constants public static final String STATUS_APPROVED = "APPROVED"; // Event Types public static final String ENTRY_EVENT = "ENTRY"; public static final String EXIT_EVENT = "EXIT"; // ... other groupings }backend/muster-roll/src/main/resources/application.properties (1)
Line range hint
8-11: Security: Avoid committing sensitive information.Database credentials and sensitive configuration should not be committed directly in the properties file. Consider:
- Using environment variables or external configuration management
- Implementing a secure secrets management solution
-spring.datasource.url=jdbc:postgresql://localhost:5432/digit-works -spring.datasource.username=postgres -spring.datasource.password=egov +spring.datasource.url=${SPRING_DATASOURCE_URL} +spring.datasource.username=${SPRING_DATASOURCE_USERNAME} +spring.datasource.password=${SPRING_DATASOURCE_PASSWORD}backend/muster-roll/src/main/java/org/egov/service/MusterRollService.java (1)
Line range hint
292-297: Typo Error: Incorrect Constant NameCOMPUTE_ATTENDENSEThere's a typo in the constant name
COMPUTE_ATTENDENSE. It should beCOMPUTE_ATTENDANCEto maintain consistency and avoid confusion. This typo can lead to issues when the code tries to find the value in the JSON node.Please correct the constant name and its usages:
- private static final String COMPUTE_ATTENDENSE = "computeAttendance"; + private static final String COMPUTE_ATTENDANCE = "computeAttendance";And update its usage in
isComputeAttendancemethod:- if (node.findValue(COMPUTE_ATTENDENSE) != null && StringUtils.isNotBlank(node.findValue(COMPUTE_ATTENDENSE).textValue())) { + if (node.findValue(COMPUTE_ATTENDANCE) != null && StringUtils.isNotBlank(node.findValue(COMPUTE_ATTENDANCE).textValue())) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (20)
backend/muster-roll/src/main/java/org/egov/config/MusterRollServiceConfiguration.java(3 hunks)backend/muster-roll/src/main/java/org/egov/repository/querybuilder/MusterRollQueryBuilder.java(1 hunks)backend/muster-roll/src/main/java/org/egov/service/CalculationService.java(1 hunks)backend/muster-roll/src/main/java/org/egov/service/EnrichmentService.java(2 hunks)backend/muster-roll/src/main/java/org/egov/service/MusterRollService.java(4 hunks)backend/muster-roll/src/main/java/org/egov/service/NotificationService.java(1 hunks)backend/muster-roll/src/main/java/org/egov/util/MusterRollServiceConstants.java(1 hunks)backend/muster-roll/src/main/java/org/egov/util/MusterRollServiceUtil.java(5 hunks)backend/muster-roll/src/main/java/org/egov/util/UserUtil.java(4 hunks)backend/muster-roll/src/main/java/org/egov/validator/MusterRollValidator.java(9 hunks)backend/muster-roll/src/main/java/org/egov/web/controllers/MusterRollApiController.java(2 hunks)backend/muster-roll/src/main/java/org/egov/web/models/AttendanceRegister.java(1 hunks)backend/muster-roll/src/main/java/org/egov/web/models/AttendanceRegisterRequest.java(1 hunks)backend/muster-roll/src/main/java/org/egov/web/models/IndividualEntry.java(2 hunks)backend/muster-roll/src/main/java/org/egov/web/models/MusterRollSearchCriteria.java(1 hunks)backend/muster-roll/src/main/java/org/egov/web/models/MusterRollSearchRequest.java(1 hunks)backend/muster-roll/src/main/resources/application.properties(3 hunks)backend/muster-roll/src/test/java/org/egov/service/MusterRollServiceTest.java(1 hunks)backend/muster-roll/src/test/java/org/egov/validator/MusterRollValidatorTest.java(3 hunks)build/build-config.yml(1 hunks)
🔇 Additional comments (24)
backend/muster-roll/src/main/java/org/egov/util/MusterRollServiceConstants.java (1)
18-18: Verify the distinction betweenACTION_APPROVEandWF_APPROVE_CODE.The constant addition follows consistent naming. However, there's already a constant
WF_APPROVE_CODEwith the same value "APPROVE". Let's verify they serve different purposes.✅ Verification successful
The constants
ACTION_APPROVEandWF_APPROVE_CODEserve distinct purposes and are correctly separated.While both constants share the same value "APPROVE", they are used in different contexts -
ACTION_APPROVEfor action handling in the main service andWF_APPROVE_CODEspecifically for workflow notifications. This separation is intentional and follows good practices.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of both constants to understand their distinct purposes echo "Searching for ACTION_APPROVE usage:" rg "ACTION_APPROVE" -A 3 echo -e "\nSearching for WF_APPROVE_CODE usage:" rg "WF_APPROVE_CODE" -A 3Length of output: 3084
build/build-config.yml (2)
45-51: Verify Dockerfile consistency across health services.The new health-muster-roll service uses
maven-java17/Dockerfilewhile the existing health-attendance service usesmaven/Dockerfile. This inconsistency might lead to version compatibility issues.Run this script to check Dockerfile usage across health services:
47-48: Review shared work directory with digit-works muster-roll.Both
health-muster-rollanddigit-works/backend/muster-rollservices use the same work directorybackend/muster-roll. This might cause conflicts or unintended sharing of resources.Run this script to check for potential conflicts:
backend/muster-roll/src/main/java/org/egov/service/EnrichmentService.java (1)
164-165: LGTM! Appropriate conditional logging for workflow actions.The workflow action logging is correctly guarded by the configuration flag, aligning with the PR's objective of making workflow configurable.
backend/muster-roll/src/main/java/org/egov/util/UserUtil.java (3)
24-28: Good improvement in field declarations!Making the fields
finalimproves thread safety and enforces immutability, which is a good practice for dependency injection.
34-37: Constructor changes look good!The constructor properly initializes all final fields and correctly incorporates the new configuration dependency.
50-52: Verify endpoint configuration values.The endpoint checks now use configuration methods instead of hardcoded values. Please ensure that all required endpoint configurations are properly set in the configuration files.
✅ Verification successful
Endpoint configurations are properly set and complete ✅
All required user endpoint configurations are present in
backend/muster-roll/src/main/resources/application.properties:
- User host and context path
- Create endpoint
- Search endpoint
- Update endpoint
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for endpoint configurations in application properties files rg -l "user\.(search|create|update)\.endpoint" --type propertiesLength of output: 101
Script:
#!/bin/bash # Find configuration files echo "=== Configuration Files ===" fd -e yml -e yaml -e properties # Search for endpoint configurations in Java files echo -e "\n=== Java Configurations ===" rg "user\.(search|create|update)\.endpoint" -t java # Search within found configuration files echo -e "\n=== Configuration Content ===" fd -e yml -e yaml -e properties -x rg -l "user\.(search|create|update)"Length of output: 3438
Script:
#!/bin/bash # Check content of muster-roll's application.properties echo "=== Muster Roll Application Properties ===" cat backend/muster-roll/src/main/resources/application.properties | grep -A 2 -B 2 "user\."Length of output: 454
backend/muster-roll/src/main/java/org/egov/web/models/AttendanceRegisterRequest.java (2)
1-6: LGTM! Clean and minimal imports.The package structure and imports are well-organized and contain only the necessary dependencies.
13-15: LGTM! Well-implemented builder pattern.The static factory method for the builder follows best practices.
backend/muster-roll/src/main/java/org/egov/web/models/IndividualEntry.java (2)
42-43: LGTM! Good validation constraint.The @min(0) constraint appropriately prevents negative values for actual attendance, which aligns with business logic.
46-47: 🧹 Nitpick (assertive)Consider additional validation between actual and modified attendance.
While the @min(0) constraint is good, consider adding validation to ensure modifiedTotalAttendance doesn't exceed some reasonable threshold compared to actualTotalAttendance, preventing potential data entry errors.
Let's verify if such validation exists elsewhere:
backend/muster-roll/src/main/java/org/egov/web/models/MusterRollSearchRequest.java (1)
1-21: LGTM! Well-structured request wrapper.The implementation follows eGov patterns and includes proper validation annotations.
backend/muster-roll/src/main/java/org/egov/web/models/MusterRollSearchCriteria.java (1)
34-36: Verify the coexistence of registerId and registerIds fields.Having both singular and plural versions of the same field could lead to confusion. Please clarify:
- Can both fields be used simultaneously?
- Should there be validation to ensure only one is used at a time?
✅ Verification successful
Both fields serve distinct purposes and can coexist safely
The codebase shows a clear separation of concerns:
registerId: Used for single record operations (exact match)registerIds: Used for search/filter operations (multiple matches)This is a common and valid pattern in search criteria objects.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage patterns of registerId and registerIds echo "Searching for usage patterns..." rg -A 5 "registerId|registerIds" --type javaLength of output: 84289
backend/muster-roll/src/main/resources/application.properties (1)
123-130: 🧹 Nitpick (assertive)Review security implications of default configurations.
Several critical configurations are enabled by default which might have security implications:
- Bank account details feature is enabled (
musterroll.add.bank.account.details.enabled=true)- Attendance register validation is disabled (
musterroll.validate.attendance.register.enabled=false)Consider:
- Disabling sensitive features by default
- Enabling validation by default
- Making these configurations environment-specific
-musterroll.add.bank.account.details.enabled=true +musterroll.add.bank.account.details.enabled=${MUSTERROLL_ADD_BANK_DETAILS_ENABLED:false} -musterroll.validate.attendance.register.enabled=false +musterroll.validate.attendance.register.enabled=${MUSTERROLL_VALIDATE_ATTENDANCE_REGISTER:true}backend/muster-roll/src/main/java/org/egov/util/MusterRollServiceUtil.java (2)
153-163: LGTM: Role information added correctlyThe logic for adding roles to the
additionalDetailswhenindividualEntryRolesEnabledis configured is correctly implemented. The exception handling appropriately addresses scenarios where no user is found for a given individual ID.
290-315: MethodupdateAttendanceRegisteradded correctlyThe new method
updateAttendanceRegisteris appropriately implemented to update the attendance register. The request construction and error handling enhance the robustness of the service.backend/muster-roll/src/main/java/org/egov/validator/MusterRollValidator.java (4)
100-105: Good Practice: Configurable Validation Checks ImplementedThe addition of configuration checks for attendance validation and workflow enables flexibility in the validation process. By checking
serviceConfiguration.isValidateAttendanceRegisterEnabled()andserviceConfiguration.isMusterRollWorkflowEnabled(), the system can dynamically enable or disable features based on configuration, enhancing maintainability.Also applies to: 134-139
198-200: Enhanced Flexibility: Conditional Start Date ValidationThe start date validation now checks the configuration
isValidateStartDateMondayEnabledbefore enforcing that the start date must be a Monday. This provides greater configurability, allowing the system to adapt to different business requirements without code changes.
208-212: Improvement: Configurable Default Duration for Muster RollBy introducing
isMusterRollSetDefaultDurationEnabled, the system can conditionally set the end date of the muster roll based on a default duration specified in the configuration. This enhances the flexibility and adaptability of the muster roll creation process.
313-341: Well-Structured: Attendance Validation and Enrichment Logic AddedThe new
validateAndEnrichAttendancemethod adds robust validation to ensure that attendance days do not exceed the days available in the attendance register. It also aligns the muster roll's start and end dates with those of the attendance register, ensuring data consistency.backend/muster-roll/src/main/java/org/egov/service/MusterRollService.java (1)
130-134: Configurable Workflow Handling ImplementedThe inclusion of configuration checks
config.isMusterRollWorkflowEnabled()before updating the workflow status in bothcreateMusterRollandupdateMusterRollmethods is commendable. This allows for dynamic enabling or disabling of workflow features without code modifications.Also applies to: 218-220
backend/muster-roll/src/main/java/org/egov/service/NotificationService.java (1)
45-45: Good Practice: Added Configuration Check for NotificationsThe addition of
if(!config.isSendNotificationEnabled()) return;ensures that notifications are sent only when the feature is enabled. This allows for easier management of notification settings and prevents unintended message sending.backend/muster-roll/src/test/java/org/egov/service/MusterRollServiceTest.java (1)
84-84: LGTM! Test setup correctly enables workflow validation.The mock configurations ensure that workflow validation is enabled during the tests, which is necessary for verifying the workflow-related error scenarios.
Also applies to: 145-145
backend/muster-roll/src/test/java/org/egov/validator/MusterRollValidatorTest.java (1)
50-50: LGTM! Test setup correctly enables required validations.The mock configurations ensure that both start date validation and workflow validation are enabled during the tests, which is necessary for verifying the validation logic.
Also applies to: 135-135
backend/muster-roll/src/main/java/org/egov/service/EnrichmentService.java
Outdated
Show resolved
Hide resolved
backend/muster-roll/src/main/java/org/egov/web/models/AttendanceRegister.java
Show resolved
Hide resolved
backend/muster-roll/src/main/java/org/egov/web/models/AttendanceRegister.java
Show resolved
Hide resolved
backend/muster-roll/src/main/java/org/egov/web/models/AttendanceRegister.java
Show resolved
Hide resolved
backend/muster-roll/src/main/java/org/egov/validator/MusterRollValidator.java
Outdated
Show resolved
Hide resolved
backend/muster-roll/src/main/java/org/egov/service/MusterRollService.java
Outdated
Show resolved
Hide resolved
backend/muster-roll/src/main/java/org/egov/service/MusterRollService.java
Outdated
Show resolved
Hide resolved
backend/muster-roll/src/main/java/org/egov/config/MusterRollServiceConfiguration.java
Show resolved
Hide resolved
backend/muster-roll/src/main/java/org/egov/service/CalculationService.java
Outdated
Show resolved
Hide resolved
|
Muster roll service changes -
Changes
New features
/v2/_searchAPI to search muster roll, to be able to use request body instead of params for bills with support of search using multiple register idsBackward Compatibility
Summary by CodeRabbit
Based on the comprehensive changes across multiple files, here are the release notes:
Release Notes: Muster Roll Service Enhancements
New Features
Configuration Improvements
Validation Enhancements
Performance Optimizations
API Updates
/v2/_searchendpoint for muster roll searchesMinor Improvements