Hcm expense calculator #1867
Hcm expense calculator #1867
Conversation
* added pdf and excel file generate for bill * Updated localization and added error queue * Updated the workflow issue * Removed incorrect files * Resolved conflict * Updated bill create topic name and consumer group --------- Co-authored-by: Shailesh <[email protected]> Co-authored-by: Shailesh <[email protected]>
…nts/DIGIT-Works into hcm-expense-calculator
…nts/DIGIT-Works into hcm-expense-calculator
…nts/DIGIT-Works into hcm-expense-calculator
* Resolved asyc issue of report generate * pdf/excel retry logic fix * Updated report generation according to demo feedback --------- Co-authored-by: Shailesh <[email protected]> Co-authored-by: shubhang-egov <[email protected]> Co-authored-by: Shailesh <[email protected]>
…T-Works into hcm-expense-calculator
shailesh-egov
requested review from
sathishp-eGov,
jagankumar-egov,
subhashini-egov and
a team
as code owners
|
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 WalkthroughThis pull request introduces significant enhancements to the expense calculator module, focusing on health-related bill generation and reporting. The changes span multiple components, including configuration updates, new utility classes, enhanced service methods, and database schema modifications. The primary objectives include improving bill generation processes, adding health integration capabilities, implementing Redis caching, and creating comprehensive bill reporting functionality. Changes
Sequence DiagramsequenceDiagram
participant Client
participant WorksCalculatorAPI
participant ExpenseCalculatorService
participant HealthBillReportGenerator
participant RedisService
participant ExternalServices
Client->>WorksCalculatorAPI: Request Bill Generation
WorksCalculatorAPI->>ExpenseCalculatorService: createWageBillHealth
ExpenseCalculatorService->>RedisService: Check Bill Cache
RedisService-->>ExpenseCalculatorService: Cache Status
ExpenseCalculatorService->>ExternalServices: Fetch Project/Individual Details
ExternalServices-->>ExpenseCalculatorService: Return Details
ExpenseCalculatorService->>HealthBillReportGenerator: Generate Report
HealthBillReportGenerator->>FileStore: Upload Report
HealthBillReportGenerator-->>ExpenseCalculatorService: Return Bill Response
ExpenseCalculatorService-->>WorksCalculatorAPI: Return Bill Response
WorksCalculatorAPI-->>Client: Return Bill Details
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
|
shailesh-egov
changed the base branch from
develop-backup-muktav1.0
to
develop
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
🛑 Comments failed to post (98)
backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/web/models/BillDetail.java (3)
90-90: 🧹 Nitpick (assertive)
Review ArrayList initialization approach.
The current implementation has two issues:
- The ArrayList initialization at declaration is inconsistent with the builder pattern
- The initialization makes the null checks in
addLineItemsandaddPayableLineItemsmethods redundantConsider either:
- Removing the initialization and keeping the null checks in add methods, or
- Using
@Builder.Defaultannotation for proper builder pattern support@JsonProperty("lineItems") @Valid + @Builder.Default private List<LineItem> lineItems = new ArrayList<>(); @JsonProperty("payableLineItems") @NotNull @Valid + @Builder.Default private List<LineItem> payableLineItems = new ArrayList<>();Also applies to: 95-95
65-66: 🧹 Nitpick (assertive)
Convert inline comment to Javadoc format.
The comment for
referenceIdshould follow Javadoc conventions for better documentation and IDE support.- // Muster-roll id + /** + * Represents the Muster-roll identifier + */ private String referenceId;📝 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./** * Represents the Muster-roll identifier */ private String referenceId;
72-74: 🧹 Nitpick (assertive)
Add validation constraints for the status field.
The newly added
statusfield lacks validation constraints. Consider adding@NotNullif this field is required.@JsonProperty("status") + @NotNull private Status status;📝 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.@JsonProperty("status") @NotNull private Status status;backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/web/models/BillResponse.java (2)
38-39: 🧹 Nitpick (assertive)
Consider enhancing the
statusCodefield with validation and documentation.The new field would benefit from:
- Documentation explaining its purpose and possible values
- Validation constraints to ensure valid status codes
- Consider using an enum instead of String for type safety
+ /** + * Status code indicating the result of bill processing. + * Possible values: SUCCESS, PENDING, FAILED, etc. + */ @JsonProperty("statusCode") + @NotBlank(message = "Status code cannot be blank") + @Pattern(regexp = "^(SUCCESS|PENDING|FAILED)$", message = "Invalid status code") private String statusCode;Alternatively, consider using an enum:
public enum BillStatusCode { SUCCESS, PENDING, FAILED } @JsonProperty("statusCode") @NotNull(message = "Status code cannot be null") private BillStatusCode statusCode;
38-40: 🧹 Nitpick (assertive)
Document potential health-related extensions.
Since this class is now being used in health-related bill processing (as indicated by
createWageBillHealth), consider adding class-level documentation about this use case and potential future extensions needed for health-specific requirements./** * BillResponse + * + * This class represents a response for bill generation requests. + * It supports both general and health-specific bill processing. + * + * @see org.egov.digit.expense.calculator.service.ExpenseCalculatorService#createWageBillHealth */ @Validated📝 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./** * BillResponse * * This class represents a response for bill generation requests. * It supports both general and health-specific bill processing. * * @see org.egov.digit.expense.calculator.service.ExpenseCalculatorService#createWageBillHealth */ @Validated @JsonProperty("statusCode") private String statusCode;backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/web/models/BillCriteria.java (2)
44-45: 🛠️ Refactor suggestion
Add validation constraints to the localityCode field.
Consider adding size constraints and null validation similar to other String fields in this class. This ensures consistent validation across all fields.
@JsonProperty("localityCode") + @Size(min = 2, max = 64) private String localityCode;📝 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.@JsonProperty("localityCode") @Size(min = 2, max = 64) private String localityCode;
44-45: 💡 Codebase verification
Database implementation for localityCode is incomplete and requires attention
The verification reveals several critical issues that need to be addressed:
- No database migration scripts found for adding the localityCode field
- Repository layer still uses contractId without localityCode implementation
- Missing database indexes for the new field
- Inconsistency between model classes (using localityCode) and database layer (using contractId)
🔗 Analysis chain
Verify database queries and indexes for the new localityCode field.
Since this field replaces contractId and works with referenceId for bill identification, ensure that:
- All database queries are updated to use this field efficiently
- Appropriate indexes exist for performance
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for SQL queries and repository methods that might need updates echo "Searching for SQL queries using contractId..." rg -t java "contractId|localityCode" backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/repository/ echo "Checking for repository interfaces and implementations..." fd -e java . backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/repository/ --exec ast-grep --pattern 'interface $_ { $$$ }'Length of output: 2922
Script:
#!/bin/bash # Search for database migration scripts and table definitions echo "Searching for migration scripts..." fd -e sql . backend/expense-calculator/src/main/resources/db/migration --exec cat {} echo "Searching for localityCode implementation in services and repositories..." rg -t java "localityCode" backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/Length of output: 3004
backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/repository/IdGenRepository.java (1)
3-5: 💡 Codebase verification
Package migration needs attention in attendance module
The migration from
digit.models.coremodelstoorg.egov.common.contract.idgenis incomplete. While most active modules have adopted the new package, the attendance module still uses the old package:
backend/attendance/src/main/java/org/egov/repository/IdGenRepository.javabackend/attendance/src/test/java/org/egov/enrichment/RegisterEnrichmentTest.java🔗 Analysis chain
LGTM! Package restructuring looks good.
The migration from
digit.models.coremodelstoorg.egov.common.contract.idgenaligns with the standardization of common contracts across the codebase.Let's verify that this package change is consistent across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to the old package # to ensure complete migration # Search for any remaining references to the old package echo "Checking for old package references..." rg "digit\.models\.coremodels\.(IdGenerationRequest|IdGenerationResponse|IdRequest)" # Search for new package usage to confirm proper adoption echo "Checking new package adoption..." rg "org\.egov\.common\.contract\.idgen\.(IdGenerationRequest|IdGenerationResponse|IdRequest)"Length of output: 6577
backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/util/HRMSUtils.java (1)
5-5: 💡 Codebase verification
The import change is correct but not fully standardized across all services.
The migration to
org.egov.common.contract.models.RequestInfoWrapperis correct and consistent within the expense-calculator service. However, theattendanceservice still uses the old importdigit.models.coremodels.RequestInfoWrapper.
backend/attendance/*: Update all RequestInfoWrapper imports to use the new package🔗 Analysis chain
Verify consistent usage of the new import across the codebase.
The import change from
digit.models.coremodels.RequestInfoWrappertoorg.egov.common.contract.models.RequestInfoWrapperappears to be part of a standardization effort.Let's verify this change is consistent across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining old imports rg "import digit\.models\.coremodels\.RequestInfoWrapper" # Verify new import usage rg "import org\.egov\.common\.contract\.models\.RequestInfoWrapper"Length of output: 7162
backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/util/IndividualUtil.java (3)
34-41: 🧹 Nitpick (assertive)
Complete the Javadoc documentation.
The Javadoc is missing important details:
- Parameter descriptions
- Return value documentation
- Possible exceptions
Update the Javadoc to:
/** * Fetch the individual details from individual service * * @param ids List of individual IDs to fetch * @param requestInfo Request information containing authentication details * @param tenantId Tenant identifier for the request * @return List of Individual objects containing the fetched details * @throws RuntimeException if the API call fails */📝 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./** * Fetch the individual details from individual service * * @param ids List of individual IDs to fetch * @param requestInfo Request information containing authentication details * @param tenantId Tenant identifier for the request * @return List of Individual objects containing the fetched details * @throws RuntimeException if the API call fails */ public List<Individual> fetchIndividualDetails(List<String> ids, RequestInfo requestInfo, String tenantId) {
58-67: 🧹 Nitpick (assertive)
Consider pagination support and input validation.
The URL builder method has potential improvements:
- Hardcoded offset value (0) limits pagination support
- Missing validation for the limit parameter
Consider these improvements:
-private UriComponentsBuilder getSearchURLWithParams(String tenantId, int limit) { +private UriComponentsBuilder getSearchURLWithParams(String tenantId, int limit, int offset) { + if (limit <= 0) { + throw new IllegalArgumentException("Limit must be positive"); + } + if (offset < 0) { + throw new IllegalArgumentException("Offset cannot be negative"); + } + StringBuilder uri = new StringBuilder(); uri.append(config.getIndividualHost()).append(config.getIndividualSearchEndPoint()); UriComponentsBuilder uriBuilder = UriComponentsBuilder.fromHttpUrl(uri.toString()) .queryParam("limit", limit) - .queryParam("offset", 0) + .queryParam("offset", offset) .queryParam("tenantId", tenantId); return uriBuilder; }📝 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.private UriComponentsBuilder getSearchURLWithParams(String tenantId, int limit, int offset) { if (limit <= 0) { throw new IllegalArgumentException("Limit must be positive"); } if (offset < 0) { throw new IllegalArgumentException("Offset cannot be negative"); } StringBuilder uri = new StringBuilder(); uri.append(config.getIndividualHost()).append(config.getIndividualSearchEndPoint()); UriComponentsBuilder uriBuilder = UriComponentsBuilder.fromHttpUrl(uri.toString()) .queryParam("limit", limit) .queryParam("offset", offset) .queryParam("tenantId", tenantId); return uriBuilder; }
41-56: 🛠️ Refactor suggestion
Add error handling and input validation.
The method needs defensive programming:
- Missing error handling for API failures
- No validation for input parameters
- Log message uses inefficient string concatenation
Consider these improvements:
public List<Individual> fetchIndividualDetails(List<String> ids, RequestInfo requestInfo, String tenantId) { + if (ids == null || ids.isEmpty() || requestInfo == null || tenantId == null) { + throw new IllegalArgumentException("Invalid input parameters"); + } + String uri = getSearchURLWithParams(tenantId, ids.size()).toUriString(); IndividualSearch individualSearch = IndividualSearch.builder().id(ids).build(); IndividualSearchRequest individualSearchRequest = IndividualSearchRequest.builder() .requestInfo(requestInfo).individual(individualSearch).build(); - log.info("IndividualUtil::fetchIndividualDetails::call individual search with tenantId::" + tenantId + "::individual ids::" + ids); + log.info("IndividualUtil::fetchIndividualDetails::call individual search with tenantId:{}, individual ids:{}", tenantId, ids); - Object response = requestRepository.fetchResult(new StringBuilder(uri), individualSearchRequest); + try { + Object response = requestRepository.fetchResult(new StringBuilder(uri), individualSearchRequest); + IndividualBulkResponse individualBulkResponse = mapper.convertValue(response, IndividualBulkResponse.class); + return individualBulkResponse.getIndividual(); + } catch (Exception e) { + log.error("Error while fetching individual details for ids: {}", ids, e); + throw new RuntimeException("Failed to fetch individual details", e); + } - IndividualBulkResponse individualBulkResponse = mapper.convertValue(response, IndividualBulkResponse.class); - - return individualBulkResponse.getIndividual(); }📝 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.public List<Individual> fetchIndividualDetails(List<String> ids, RequestInfo requestInfo, String tenantId) { if (ids == null || ids.isEmpty() || requestInfo == null || tenantId == null) { throw new IllegalArgumentException("Invalid input parameters"); } String uri = getSearchURLWithParams(tenantId, ids.size()).toUriString(); IndividualSearch individualSearch = IndividualSearch.builder().id(ids).build(); IndividualSearchRequest individualSearchRequest = IndividualSearchRequest.builder() .requestInfo(requestInfo).individual(individualSearch).build(); log.info("IndividualUtil::fetchIndividualDetails::call individual search with tenantId:{}, individual ids:{}", tenantId, ids); try { Object response = requestRepository.fetchResult(new StringBuilder(uri), individualSearchRequest); IndividualBulkResponse individualBulkResponse = mapper.convertValue(response, IndividualBulkResponse.class); return individualBulkResponse.getIndividual(); } catch (Exception e) { log.error("Error while fetching individual details for ids: {}", ids, e); throw new RuntimeException("Failed to fetch individual details", e); } }backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/service/RedisService.java (3)
15-21: 🧹 Nitpick (assertive)
Constructor injection is well implemented!
Good practices observed:
- Using final field for RedisTemplate
- Constructor injection instead of field injection
- Proper generic type parameters
Since Spring 4.3, the
@Autowiredannotation is optional for single-constructor classes. You can safely remove it to reduce boilerplate.
23-30: 🛠️ Refactor suggestion
Consider enhancing the cache implementation.
Several improvements could make this implementation more robust:
- Add TTL to prevent unlimited cache growth
- Add input validation
- Consider more specific exception handling
- Document the significance of the boolean value
Here's a suggested implementation:
+ /** + * Sets a cache entry for bill report generation to prevent duplicate processing + * @param id The bill ID to cache + * @throws CustomException if Redis operations fail + * @throws IllegalArgumentException if id is null or empty + */ public void setCacheForBillReport(String id) { + if (id == null || id.trim().isEmpty()) { + throw new IllegalArgumentException("Bill ID cannot be null or empty"); + } try { - redisTemplate.opsForValue().set(getReportRedisKey(id), true); + redisTemplate.opsForValue().set(getReportRedisKey(id), true, Duration.ofDays(7)); - } catch (Exception e) { + } catch (RedisConnectionException e) { log.error("Error while pushing data to redis", e); - throw new CustomException("REDIS_ERROR", "Error while pushing data to redis"); + throw new CustomException("REDIS_CONNECTION_ERROR", "Failed to connect to Redis server"); + } catch (RedisException e) { + log.error("Error during Redis operation", e); + throw new CustomException("REDIS_OPERATION_ERROR", "Failed to set cache entry"); } }Committable suggestion skipped: line range outside the PR's diff.
32-38: 🛠️ Refactor suggestion
Add error handling and input validation.
The methods need additional error handling and validation:
- Redis operations should be wrapped in try-catch
- Input parameters should be validated
- Key template should be verified
Here's a suggested implementation:
+ /** + * Checks if a bill report is already being processed + * @param id The bill ID to check + * @return true if the bill is in cache, false otherwise + * @throws CustomException if Redis operations fail + * @throws IllegalArgumentException if id is null or empty + */ public Boolean isBillIdPresentInCache(String id) { + if (id == null || id.trim().isEmpty()) { + throw new IllegalArgumentException("Bill ID cannot be null or empty"); + } + try { return redisTemplate.hasKey(getReportRedisKey(id)); + } catch (RedisException e) { + log.error("Error checking Redis key", e); + throw new CustomException("REDIS_ERROR", "Failed to check cache entry"); + } } + /** + * Generates Redis key for bill report processing status + * @param id The bill ID + * @return Redis key string + * @throws IllegalStateException if key template is invalid + */ private String getReportRedisKey(String id) { + if (!REPORT_BILL_GEN_REDIS_KEY.contains("{billId}")) { + throw new IllegalStateException("Invalid Redis key template"); + } return REPORT_BILL_GEN_REDIS_KEY.replace("{billId}", id); }📝 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./** * Checks if a bill report is already being processed * @param id The bill ID to check * @return true if the bill is in cache, false otherwise * @throws CustomException if Redis operations fail * @throws IllegalArgumentException if id is null or empty */ public Boolean isBillIdPresentInCache(String id) { if (id == null || id.trim().isEmpty()) { throw new IllegalArgumentException("Bill ID cannot be null or empty"); } try { return redisTemplate.hasKey(getReportRedisKey(id)); } catch (RedisException e) { log.error("Error checking Redis key", e); throw new CustomException("REDIS_ERROR", "Failed to check cache entry"); } } /** * Generates Redis key for bill report processing status * @param id The bill ID * @return Redis key string * @throws IllegalStateException if key template is invalid */ private String getReportRedisKey(String id) { if (!REPORT_BILL_GEN_REDIS_KEY.contains("{billId}")) { throw new IllegalStateException("Invalid Redis key template"); } return REPORT_BILL_GEN_REDIS_KEY.replace("{billId}", id); }backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/util/BillUtils.java (2)
42-57: 🧹 Nitpick (assertive)
Add input validation and consider pagination configuration.
The implementation looks good but could benefit from these improvements:
- Add null checks for input parameters
- Validate the referenceId format/content
- Consider configuring pagination parameters instead of using an empty pagination object
Consider adding these validations:
public BillResponse searchBills(CalculationRequest calculationRequest, String referenceId) { + if (calculationRequest == null || referenceId == null || referenceId.trim().isEmpty()) { + throw new IllegalArgumentException("CalculationRequest and referenceId must not be null or empty"); + } + BillCriteria billCriteria = BillCriteria.builder()📝 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.public BillResponse searchBills(CalculationRequest calculationRequest, String referenceId) { if (calculationRequest == null || referenceId == null || referenceId.trim().isEmpty()) { throw new IllegalArgumentException("CalculationRequest and referenceId must not be null or empty"); } BillCriteria billCriteria = BillCriteria.builder() .tenantId(calculationRequest.getCriteria().getTenantId()) .referenceIds(Stream.of(referenceId).collect(Collectors.toSet())) .localityCode(calculationRequest.getCriteria().getLocalityCode()) .build(); BillSearchRequest billSearchRequest = BillSearchRequest.builder() .requestInfo(calculationRequest.getRequestInfo()) .billCriteria(billCriteria) .pagination(Pagination.builder().build()) .build(); Object responseObj = restRepo.fetchResult(getBillSearchURI(), billSearchRequest); return mapper.convertValue(responseObj, BillResponse.class); }
72-75: 🧹 Nitpick (assertive)
Consider adding debug-level logging for troubleshooting.
While the simplified logging is good for production, consider adding debug-level logs for request/response objects to aid in troubleshooting.
- log.info("Posting Bill to expense service"); + log.info("Posting Bill to expense service"); + log.debug("Bill request: {}", requestInfoWrapper); Object responseObj = restRepo.fetchResult(url, requestInfoWrapper); if(responseObj!=null) - log.info("Received Bill Response"); + log.info("Received Bill Response"); + log.debug("Bill response: {}", responseObj);📝 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.log.info("Posting Bill to expense service"); log.debug("Bill request: {}", requestInfoWrapper); Object responseObj = restRepo.fetchResult(url, requestInfoWrapper); if(responseObj!=null) log.info("Received Bill Response"); log.debug("Bill response: {}", responseObj);backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/util/FileStoreUtil.java (2)
56-62: 🧹 Nitpick (assertive)
🛠️ Refactor suggestion
Improve response handling and add proper validation.
The response parsing needs more robust error handling:
private String getFileStoreId(Object response) { + if (response == null) { + throw new FileStoreException("Received null response from filestore"); + } + Map<String,Object> res = objectMapper.convertValue(response, Map.class); + if (!res.containsKey("files")) { + throw new FileStoreException("Invalid response format: 'files' key not found"); + } + + @SuppressWarnings("unchecked") List<Map<String,String>> files = (List<Map<String, String>>) res.get("files"); - if(files.isEmpty()) - throw new RuntimeException("File not uploaded to filestore"); - return files.get(0).get("fileStoreId"); + if (files.isEmpty()) { + throw new FileStoreException("File not uploaded to filestore"); + } + + String fileStoreId = files.get(0).get("fileStoreId"); + if (fileStoreId == null || fileStoreId.trim().isEmpty()) { + throw new FileStoreException("Invalid fileStoreId received"); + } + + return fileStoreId; }Create a custom exception class.
Add a custom exception class for better error handling:
public class FileStoreException extends RuntimeException { public FileStoreException(String message) { super(message); } public FileStoreException(String message, Throwable cause) { super(message, cause); } }
34-54: 🧹 Nitpick (assertive)
🛠️ Refactor suggestion
Add input validation and improve error handling.
Several improvements are needed for robustness:
- Add null checks for input parameters
- Validate URI construction
- Use specific exceptions instead of generic Exception
- Consider validating Resource content type
public String uploadFileAndGetFileStoreId(String tenantId, Resource resource){ + if (tenantId == null || tenantId.trim().isEmpty()) { + throw new IllegalArgumentException("tenantId cannot be null or empty"); + } + if (resource == null || !resource.exists()) { + throw new IllegalArgumentException("resource cannot be null and must exist"); + } + HttpHeaders headers = new HttpHeaders(); - Object response = null; StringBuilder uri = new StringBuilder(); headers.setContentType(MediaType.MULTIPART_FORM_DATA); uri.append(config.getFileStoreHost()).append(config.getFileStoreEndpoint()); + + if (uri.length() == 0) { + throw new IllegalStateException("File store URL configuration is missing"); + } MultiValueMap<String, Object> body = new LinkedMultiValueMap<>(); body.add("tenantId", tenantId); body.add("file", resource); body.add("module", "BILL"); HttpEntity<MultiValueMap<String, Object>> requestEntity = new HttpEntity<>(body, headers); + Object response; try { response = restTemplate.postForObject(uri.toString(), requestEntity, Object.class); - }catch (Exception e){ + } catch (RestClientException e) { log.error("Exception while uploading file to filestore: ", e); - throw new RuntimeException(e.getMessage()); + throw new FileStoreException("Failed to upload file to filestore", e); + } catch (IllegalStateException e) { + log.error("Invalid state while uploading file: ", e); + throw new FileStoreException("Invalid state during file upload", e); } return getFileStoreId(response); }Extract module name to configuration.
The "BILL" module name should be configurable:
- body.add("module", "BILL"); + body.add("module", config.getFileStoreModule());Committable suggestion skipped: line range outside the PR's diff.
backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/util/PDFServiceUtil.java (5)
15-33: 🧹 Nitpick (assertive)
LGTM! Class structure follows best practices.
The class is well-structured with proper dependency injection, immutable fields, and clear encapsulation.
Remove unnecessary blank lines between lines 25-27 to maintain consistent spacing.
51-57: 🛠️ Refactor suggestion
Improve exception handling with proper context.
Current exception handling only logs the error without providing enough context for debugging.
Enhance the error handling:
try { result = restRepo.fetchResult(uri, request); + if (result == null) { + throw new ServiceCallException("PDF service returned null response"); + } filestoreids = JsonPath.read(result, PDF_RESPONSE_FILESTORE_ID_JSONPATH); } catch (Exception e) { - log.error("Exception while creating PDF: " + e); + log.error("Error creating PDF for tenant: {}, key: {}, error: {}", + tenantId, pdfKey, e.getMessage(), e); + throw new PDFGenerationException("Failed to generate PDF", e); }Committable suggestion skipped: line range outside the PR's diff.
58-61: 🧹 Nitpick (assertive)
Use Optional for cleaner null handling.
The null checking and list access can be simplified using Optional.
Consider refactoring to:
-if (null != filestoreids && filestoreids.size() > 0) { - filestoreId = filestoreids.get(0); -} +filestoreId = Optional.ofNullable(filestoreids) + .filter(list -> !list.isEmpty()) + .map(list -> list.get(0)) + .orElse(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. Thoroughly test & benchmark the code to ensure it meets the requirements.filestoreId = Optional.ofNullable(filestoreids) .filter(list -> !list.isEmpty()) .map(list -> list.get(0)) .orElse(null); return filestoreId;
43-47: 🛠️ Refactor suggestion
Add input validation for required parameters.
The method should validate input parameters before processing.
Add parameter validation:
public String createPDF(BillReportRequest request, String tenantId, String pdfKey) { + if (request == null) { + throw new IllegalArgumentException("Request cannot be null"); + } + if (StringUtils.isBlank(tenantId)) { + throw new IllegalArgumentException("Tenant ID cannot be blank"); + } + if (StringUtils.isBlank(pdfKey)) { + throw new IllegalArgumentException("PDF key cannot be blank"); + } Object result = null;Committable suggestion skipped: line range outside the PR's diff.
48-50:
⚠️ Potential issueUse UriComponentsBuilder for safe URI construction.
Current string concatenation is vulnerable to injection. Parameters should be properly encoded.
Replace with:
-uri.append(config.getPdfServiceHost()).append(config.getPdfServiceCreateEndpoint()) - .append("?tenantId=" + tenantId) - .append("&key=" + pdfKey); +UriComponentsBuilder builder = UriComponentsBuilder + .fromHttpUrl(config.getPdfServiceHost()) + .path(config.getPdfServiceCreateEndpoint()) + .queryParam("tenantId", tenantId) + .queryParam("key", pdfKey); +String uri = builder.toUriString();Committable suggestion skipped: line range outside the PR's diff.
backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/util/MdmsUtils.java (2)
220-224: 🧹 Nitpick (assertive)
Add error handling for invalid campaign IDs.
While the implementation is correct, consider adding validation for the
campaignIdparameter to handle null or empty values gracefully.public Object getWorkerRateFromMDMSV2(RequestInfo requestInfo, String tenantId, String campaignId) { + if (campaignId == null || campaignId.trim().isEmpty()) { + throw new IllegalArgumentException("Campaign ID cannot be null or empty"); + } String filter = getWorkerFilter(campaignId); MdmsCriteria mdmsCriteria = getMdmsCriteria(requestInfo, WORKER_RATES, tenantId, filter, HCM_CONSTANT); return serviceRequestRepository.fetchResult(getMDMSV2SearchUrl(), MdmsCriteriaReq.builder().requestInfo(requestInfo).mdmsCriteria(mdmsCriteria).build()); }📝 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.public Object getWorkerRateFromMDMSV2(RequestInfo requestInfo, String tenantId, String campaignId) { if (campaignId == null || campaignId.trim().isEmpty()) { throw new IllegalArgumentException("Campaign ID cannot be null or empty"); } String filter = getWorkerFilter(campaignId); MdmsCriteria mdmsCriteria = getMdmsCriteria(requestInfo, WORKER_RATES, tenantId, filter, HCM_CONSTANT); return serviceRequestRepository.fetchResult(getMDMSV2SearchUrl(), MdmsCriteriaReq.builder().requestInfo(requestInfo).mdmsCriteria(mdmsCriteria).build()); }
248-251: 🧹 Nitpick (assertive)
Consider supporting multiple campaign IDs.
The current implementation supports a single campaign ID. Consider enhancing it to support multiple campaign IDs, similar to how
getFilterFromSorListhandles multiple SOR IDs.-String getWorkerFilter(String campaignId) { +String getWorkerFilter(String... campaignIds) { + if (campaignIds == null || campaignIds.length == 0) { + throw new IllegalArgumentException("At least one campaign ID is required"); + } + StringBuilder filterString = new StringBuilder(FILTER_START) + .append(CAMPAIGN_ID_CONSTANT) + .append(campaignIds[0]); + for (int i = 1; i < campaignIds.length; i++) { + filterString.append(FILTER_OR_CONSTANT) + .append(CAMPAIGN_ID_CONSTANT) + .append(campaignIds[i]); + } + return filterString.append(FILTER_END).toString(); }📝 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.String getWorkerFilter(String... campaignIds) { if (campaignIds == null || campaignIds.length == 0) { throw new IllegalArgumentException("At least one campaign ID is required"); } StringBuilder filterString = new StringBuilder(FILTER_START) .append(CAMPAIGN_ID_CONSTANT) .append(campaignIds[0]); for (int i = 1; i < campaignIds.length; i++) { filterString.append(FILTER_OR_CONSTANT) .append(CAMPAIGN_ID_CONSTANT) .append(campaignIds[i]); } return filterString.append(FILTER_END).toString(); }backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/web/models/report/ReportBillDetail.java (6)
22-60: 🧹 Nitpick (assertive)
Consider adding audit fields and version control.
For financial records, it's important to track changes and maintain an audit trail.
Consider adding:
- Audit fields (createdBy, createdTime, lastModifiedBy, lastModifiedTime)
- Version field for optimistic locking
- Tenant ID for multi-tenancy support
Example addition:
@Schema(description = "Version for optimistic locking") @Version private Long version; @Schema(description = "Tenant ID") @NotBlank(message = "Tenant ID is mandatory") private String tenantId; @Schema(description = "Audit details") private AuditDetails auditDetails;
24-25: 🧹 Nitpick (assertive)
Add positive value constraint for serial number.
The serial number should always be a positive integer.
@Schema(description = "slNo") + @Positive(message = "Serial number must be positive") private Integer slNo;📝 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.@Schema(description = "slNo") @Positive(message = "Serial number must be positive") private Integer slNo;
12-15: 🧹 Nitpick (assertive)
Enhance class-level documentation for better clarity.
While the documentation is present, it could be more descriptive about the class's purpose and usage in the expense calculator context.
Consider expanding the documentation to include:
- The relationship with other report-related classes
- Key business rules or calculations it represents
- Example usage scenarios
/** - * Bill details of the individual payee + * Represents the detailed breakdown of expenses for individual payees in the HCM expense calculator. + * This class captures various components of a payee's bill including wages, food allowance, + * transport allowance, and other identifying information for reporting purposes. + * + * Used primarily in generating expense reports and calculating total payouts. */📝 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./** * Represents the detailed breakdown of expenses for individual payees in the HCM expense calculator. * This class captures various components of a payee's bill including wages, food allowance, * transport allowance, and other identifying information for reporting purposes. * * Used primarily in generating expense reports and calculating total payouts. */ @Schema(description = "Bill details of the individual payees")
36-40: 🛠️ Refactor suggestion
Add validation constraints for ID and mobile number fields.
Critical identifier fields should have proper validation to ensure data integrity.
Add the following validation constraints:
@Schema(description = "idNumber") + @NotBlank(message = "ID number is mandatory") + @Pattern(regexp = "^[A-Z0-9]+$", message = "ID number should only contain uppercase letters and numbers") private String idNumber; @Schema(description = "mobileNumber") + @NotBlank(message = "Mobile number is mandatory") + @Pattern(regexp = "^[0-9]{10}$", message = "Mobile number should be exactly 10 digits") private String mobileNumber;📝 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.@Schema(description = "idNumber") @NotBlank(message = "ID number is mandatory") @Pattern(regexp = "^[A-Z0-9]+$", message = "ID number should only contain uppercase letters and numbers") private String idNumber; @Schema(description = "mobileNumber") @NotBlank(message = "Mobile number is mandatory") @Pattern(regexp = "^[0-9]{10}$", message = "Mobile number should be exactly 10 digits") private String mobileNumber;
55-56: 🛠️ Refactor suggestion
Improve data type and validation for totalNumberOfDays.
Using Float for days count might lead to precision issues. Consider using either Integer for whole days or BigDecimal if fractional days are required.
@Schema(description = "totalNumberOfDays") - private Float totalNumberOfDays; + @NotNull(message = "Total number of days cannot be null") + @PositiveOrZero(message = "Total number of days cannot be negative") + @DecimalMax(value = "31.0", message = "Total number of days cannot exceed 31") + private BigDecimal totalNumberOfDays = BigDecimal.ZERO;📝 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.@Schema(description = "totalNumberOfDays") @NotNull(message = "Total number of days cannot be null") @PositiveOrZero(message = "Total number of days cannot be negative") @DecimalMax(value = "31.0", message = "Total number of days cannot exceed 31") private BigDecimal totalNumberOfDays = BigDecimal.ZERO;
42-54: 🛠️ Refactor suggestion
Ensure consistent initialization and add amount validations.
There are several issues with the amount fields:
- Inconsistent use of
@Builder.Default- Missing validation for negative amounts
- Potential for calculation errors
Apply these changes to all amount fields:
@Schema(description = "wageAmount") @Builder.Default + @NotNull(message = "Wage amount cannot be null") + @PositiveOrZero(message = "Wage amount cannot be negative") private BigDecimal wageAmount = BigDecimal.ZERO; @Schema(description = "foodAmount") + @Builder.Default + @NotNull(message = "Food amount cannot be null") + @PositiveOrZero(message = "Food amount cannot be negative") private BigDecimal foodAmount = BigDecimal.ZERO; @Schema(description = "transportAmount") + @Builder.Default + @NotNull(message = "Transport amount cannot be null") + @PositiveOrZero(message = "Transport amount cannot be negative") private BigDecimal transportAmount = BigDecimal.ZERO; @Schema(description = "totalWages") + @Builder.Default + @NotNull(message = "Total wages cannot be null") + @PositiveOrZero(message = "Total wages cannot be negative") private BigDecimal totalWages = BigDecimal.ZERO;📝 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.@Schema(description = "wageAmount") @Builder.Default @NotNull(message = "Wage amount cannot be null") @PositiveOrZero(message = "Wage amount cannot be negative") private BigDecimal wageAmount = BigDecimal.ZERO; @Schema(description = "foodAmount") @Builder.Default @NotNull(message = "Food amount cannot be null") @PositiveOrZero(message = "Food amount cannot be negative") private BigDecimal foodAmount = BigDecimal.ZERO; @Schema(description = "transportAmount") @Builder.Default @NotNull(message = "Transport amount cannot be null") @PositiveOrZero(message = "Transport amount cannot be negative") private BigDecimal transportAmount = BigDecimal.ZERO; @Schema(description = "totalWages") @Builder.Default @NotNull(message = "Total wages cannot be null") @PositiveOrZero(message = "Total wages cannot be negative") private BigDecimal totalWages = BigDecimal.ZERO;backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/util/BoundaryUtil.java (1)
32-52: 💡 Codebase verification
🛠️ Refactor suggestion
Add caching to improve boundary lookup performance
The Redis caching infrastructure is already configured in the application. Add caching to the
fetchBoundarymethod to improve performance and reduce load on the boundary service:+ @Cacheable(value = "boundaryCache", key = "#tenantId + '_' + #localityCode + '_' + #includeChildren") public List<TenantBoundary> fetchBoundary(RequestInfoWrapper request, String localityCode, String tenantId, boolean includeChildren) { if (request == null || localityCode == null || tenantId == null) { throw new CustomException("INVALID_INPUT", "Request, localityCode, and tenantId are required parameters"); }The other suggestions from the original review remain valid:
- Add input validation as shown above
- Enhance error logging with more context
- Remove the redundant comment
🔗 Analysis chain
Several improvements needed for robustness and performance.
- Add input validation:
public List<TenantBoundary> fetchBoundary(RequestInfoWrapper request, String localityCode, String tenantId, boolean includeChildren){ + if (request == null || localityCode == null || tenantId == null) { + throw new CustomException("INVALID_INPUT", "Request, localityCode, and tenantId are required parameters"); + } + if (localityCode.trim().isEmpty() || tenantId.trim().isEmpty()) { + throw new CustomException("INVALID_INPUT", "localityCode and tenantId cannot be empty"); + } try{
- Enhance error logging:
- log.error("Exception while searching boundaries for parent boundary codes"); + log.error("Exception while searching boundaries for locality: {}, tenant: {}", localityCode, tenantId, e);
- Consider adding caching to improve performance:
@Cacheable(value = "boundaryCache", key = "#tenantId + '_' + #localityCode + '_' + #includeChildren") public List<TenantBoundary> fetchBoundary(...)
- Remove the redundant comment on line 46.
Let's verify if caching is already configured in the application:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Redis or cache configuration rg -l 'cache|redis' --type javaLength of output: 3890
Script:
#!/bin/bash # Check Redis configuration and BoundaryUtil implementation echo "=== Redis Configuration ===" cat backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/config/RedisConfig.java echo -e "\n=== Redis Service ===" cat backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/service/RedisService.java echo -e "\n=== Checking BoundaryUtil for existing cache usage ===" rg -A 5 -B 5 "@Cacheable|@CacheEvict|@CachePut" backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/util/BoundaryUtil.javaLength of output: 3892
backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/web/controllers/WorksCalculatorApiController.java (2)
49-51: 🧹 Nitpick (assertive)
🛠️ Refactor suggestion
Add error handling and consider endpoint versioning.
The changes to this endpoint raise several concerns:
- The endpoint path
/v1/_calculateremains generic while the implementation is now health-specific.- No error handling is implemented for potential exceptions from
createWageBillHealth.- This change might break existing clients expecting the old wage/supervision bill behavior.
Consider these improvements:
@RequestMapping(value = "/v1/_calculate", method = RequestMethod.POST) public ResponseEntity<BillResponse> worksCalculatorV1CalculatePost( @Parameter(in = ParameterIn.DEFAULT, description = "", schema = @Schema()) @Valid @RequestBody CalculationRequest calculationRequest) { - BillResponse billResponse = expenseCalculatorService.createWageBillHealth(calculationRequest); - return new ResponseEntity<BillResponse>(billResponse, HttpStatus.OK); + try { + BillResponse billResponse = expenseCalculatorService.createWageBillHealth(calculationRequest); + return new ResponseEntity<BillResponse>(billResponse, HttpStatus.OK); + } catch (Exception e) { + log.error("Error while processing health bill calculation", e); + return new ResponseEntity<BillResponse>( + BillResponse.builder() + .responseInfo(responseInfoFactory.createResponseInfoFromRequestInfo(calculationRequest.getRequestInfo(), false)) + .build(), + HttpStatus.INTERNAL_SERVER_ERROR + ); + }Consider one of these architectural approaches:
- Create a new endpoint
/v1/health/_calculatefor health-specific calculations- Add a
typeparameter to distinguish between different calculation types- Version bump to
/v2/_calculateif breaking changes are intendedCommittable suggestion skipped: line range outside the PR's diff.
52-55: 💡 Codebase verification
⚠️ Potential issueRemove test endpoint and its implementation from production code
The implementation in
ExpenseCalculatorService.createWageOrSupervisionBillTest()confirms this is a performance/load testing endpoint that:
- Creates bills with random UUIDs
- Includes timing measurements
- Can generate multiple bills in a loop
This testing code should be moved to a proper test package to avoid security risks and potential abuse in production.
Locations to update:
- Remove
_calculatetestendpoint fromWorksCalculatorApiController.java- Remove
createWageOrSupervisionBillTest()method fromExpenseCalculatorService.java- Move the testing logic to appropriate test classes if needed
🔗 Analysis chain
Remove or properly document test endpoint.
The addition of a test endpoint in production code raises several concerns:
- Test endpoints should not be exposed in production.
- The purpose and differences from the main endpoint are not documented.
- This creates duplicate business logic that may diverge over time.
If this is for testing:
- Move it to a test class
- Use
@TestConfigurationif needed for integration testingIf this is for production:
- Document the purpose and differences
- Consider renaming to reflect its actual purpose
- Add proper API documentation using Swagger annotations
Let's check if this endpoint is used in tests:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to the test endpoint rg -l "_calculatetest" --type javaLength of output: 159
Script:
#!/bin/bash # Search for the test service method rg "createWageOrSupervisionBillTest" --type java -A 5 # Look for similar test endpoints pattern rg "Test.*@RequestMapping|@RequestMapping.*Test" --type javaLength of output: 2311
backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/util/ExpenseCalculatorServiceConstants.java (1)
71-75: 🧹 Nitpick (assertive)
Consider grouping related constants together.
While the constants are well-defined, consider reorganizing them to keep related constants together. Move
JSON_PATH_FOR_HCMcloser to its dependent constantsHCM_CONSTANTandWORKER_RATESfor better maintainability.public static final String HCM_CONSTANT = "HCM"; public static final String WORKER_RATES = "WORKER_RATES"; + public static final String JSON_PATH_FOR_HCM = MDMS_RESP_CONSTANT+HCM_CONSTANT + "." + WORKER_RATES; public static final String JSON_PATH_FOR_SOR = MDMS_RESP_CONSTANT+WORKS_SOR_CONSTANT + "." + SOR_CONSTANT; public static final String JSON_PATH_FOR_RATES = MDMS_RESP_CONSTANT+WORKS_SOR_CONSTANT + "." + RATES_CONSTANT; - public static final String JSON_PATH_FOR_HCM = MDMS_RESP_CONSTANT+HCM_CONSTANT + "." + WORKER_RATES;📝 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.public static final String HCM_CONSTANT = "HCM"; public static final String WORKER_RATES = "WORKER_RATES"; public static final String JSON_PATH_FOR_HCM = MDMS_RESP_CONSTANT+HCM_CONSTANT + "." + WORKER_RATES; public static final String JSON_PATH_FOR_SOR = MDMS_RESP_CONSTANT+WORKS_SOR_CONSTANT + "." + SOR_CONSTANT; public static final String JSON_PATH_FOR_RATES = MDMS_RESP_CONSTANT+WORKS_SOR_CONSTANT + "." + RATES_CONSTANT;backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/util/AttendanceUtil.java (3)
35-36: 🛠️ Refactor suggestion
Use parameterized logging instead of string concatenation
Using parameterized logging placeholders
{}improves performance and readability by deferring string concatenation until it's necessary.Apply this diff to update the log statement:
- log.info("Fetching attendance register with tenantId::" + tenantId - + " and offset::" + offset + " and batch size:: " +config.getRegisterBatchSize()); + log.info("Fetching attendance register with tenantId::{}, offset::{}, batch size::{}", + tenantId, offset, config.getRegisterBatchSize());📝 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.log.info("Fetching attendance register with tenantId::{}, offset::{}, batch size::{}", tenantId, offset, config.getRegisterBatchSize());
55-56: 🧹 Nitpick (assertive)
Use parameterized logging in error messages
For consistency and performance, use placeholders
{}in error logging statements instead of string concatenation.Apply this diff to update the error log:
- log.error("AttendanceUtil::Error thrown from attendance register service::" - + httpClientOrServerExc.getStatusCode()); + log.error("AttendanceUtil::Error thrown from attendance register service::{}", + httpClientOrServerExc.getStatusCode());📝 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.log.error("AttendanceUtil::Error thrown from attendance register service::{}", httpClientOrServerExc.getStatusCode());
54-59: 🧹 Nitpick (assertive)
Handle additional exceptions during REST call
Currently, only
HttpClientErrorExceptionandHttpServerErrorExceptionare caught. Other exceptions, such asResourceAccessExceptionfor network errors, might occur. Consider catching additional exceptions to make the application more robust.Apply this diff to handle additional exceptions:
} catch (HttpClientErrorException | HttpServerErrorException httpClientOrServerExc) { log.error("AttendanceUtil::Error thrown from attendance register service::{}", httpClientOrServerExc.getStatusCode()); throw new CustomException("ATTENDANCE_REGISTER_SERVICE_EXCEPTION", "Error thrown from attendance register service::" + httpClientOrServerExc.getStatusCode()); +} catch (Exception ex) { + log.error("AttendanceUtil::Unexpected exception during attendance register fetch", ex); + throw new CustomException("ATTENDANCE_REGISTER_UNEXPECTED_EXCEPTION", + "Unexpected error during attendance register fetch"); }📝 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.} catch (HttpClientErrorException | HttpServerErrorException httpClientOrServerExc) { log.error("AttendanceUtil::Error thrown from attendance register service::{}", httpClientOrServerExc.getStatusCode()); throw new CustomException("ATTENDANCE_REGISTER_SERVICE_EXCEPTION", "Error thrown from attendance register service::" + httpClientOrServerExc.getStatusCode()); } catch (Exception ex) { log.error("AttendanceUtil::Unexpected exception during attendance register fetch", ex); throw new CustomException("ATTENDANCE_REGISTER_UNEXPECTED_EXCEPTION", "Unexpected error during attendance register fetch"); }backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/service/SupervisionBillGeneratorService.java (1)
231-231: 🧹 Nitpick (assertive)
Consider using a more robust delimiter for bill reference construction.
While the changes to use
referenceIdare correct, the current approach of using an underscore as a delimiter inreferenceId + "_" + supervisionBillNumbercould potentially cause ambiguity if thereferenceIditself contains underscores.Consider using a more unique delimiter or a structured format:
- .referenceId(criteria.getReferenceId() + "_" + supervisionBillNumber) + .referenceId(String.format("REF-%s-BILL-%s", criteria.getReferenceId(), supervisionBillNumber))Also applies to: 234-234
backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/config/RedisConfig.java (6)
33-38: 🛠️ Refactor suggestion
Configure serializers for RedisTemplate.
The current template configuration lacks serializer settings, which could lead to serialization issues. Consider adding proper serializers for keys and values.
@Bean public RedisTemplate<String, Object> redisTemplate() { RedisTemplate<String, Object> redisTemplate = new RedisTemplate<>(); redisTemplate.setConnectionFactory(redisConnectionFactory()); + + // Configure serializers + redisTemplate.setKeySerializer(new StringRedisSerializer()); + redisTemplate.setHashKeySerializer(new StringRedisSerializer()); + redisTemplate.setHashValueSerializer(new GenericJackson2JsonRedisSerializer()); + redisTemplate.setValueSerializer(new GenericJackson2JsonRedisSerializer()); + redisTemplate.afterPropertiesSet(); + return redisTemplate; }📝 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.@Bean public RedisTemplate<String, Object> redisTemplate() { RedisTemplate<String, Object> redisTemplate = new RedisTemplate<>(); redisTemplate.setConnectionFactory(redisConnectionFactory()); // Configure serializers redisTemplate.setKeySerializer(new StringRedisSerializer()); redisTemplate.setHashKeySerializer(new StringRedisSerializer()); redisTemplate.setHashValueSerializer(new GenericJackson2JsonRedisSerializer()); redisTemplate.setValueSerializer(new GenericJackson2JsonRedisSerializer()); redisTemplate.afterPropertiesSet(); return redisTemplate; }
28-31: 🛠️ Refactor suggestion
Enhance Redis connection factory with pool and timeout settings.
The current connection factory configuration lacks important production-ready features:
- Connection pooling
- Connection timeout settings
- Retry policies
@Bean public RedisConnectionFactory redisConnectionFactory() { - return new LettuceConnectionFactory(redisHost, redisPort); + LettuceConnectionFactory factory = new LettuceConnectionFactory(); + factory.setHostName(redisHost); + factory.setPort(redisPort); + factory.setDatabase(0); + factory.setTimeout(redisTimeout); + + // Configure connection pooling + LettucePoolingClientConfiguration poolConfig = LettucePoolingClientConfiguration.builder() + .commandTimeout(Duration.ofMillis(redisTimeout)) + .poolConfig(new GenericObjectPoolConfig()) + .build(); + factory.setClientConfiguration(poolConfig); + + return factory; }Committable suggestion skipped: line range outside the PR's diff.
40-45: 🧹 Nitpick (assertive)
Enhance cache configuration with key prefix and serialization settings.
Consider adding:
- Key prefix strategy to prevent key conflicts
- Custom serialization settings
- Configurable TTL per cache
@Bean public RedisCacheConfiguration cacheConfiguration() { return RedisCacheConfiguration.defaultCacheConfig() .entryTtl(Duration.ofSeconds(redisTimeout)) - .disableCachingNullValues(); + .disableCachingNullValues() + .prefixCacheNameWith("expense-calculator::") + .serializeKeysWith(RedisSerializationContext.SerializationPair.fromSerializer(new StringRedisSerializer())) + .serializeValuesWith(RedisSerializationContext.SerializationPair.fromSerializer(new GenericJackson2JsonRedisSerializer())); }Committable suggestion skipped: line range outside the PR's diff.
19-26: 🧹 Nitpick (assertive)
Add validation for Redis connection properties.
Consider adding validation to ensure the connection properties are properly configured:
- Validate that host is not empty
- Ensure port is within valid range (0-65535)
- Verify timeout is positive
+ @PostConstruct + public void validateProperties() { + Assert.hasText(redisHost, "Redis host cannot be empty"); + Assert.isTrue(redisPort >= 0 && redisPort <= 65535, "Redis port must be between 0 and 65535"); + Assert.isTrue(redisTimeout > 0, "Redis timeout must be positive"); + }📝 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.@Value("${spring.data.redis.host}") private String redisHost; @Value("${spring.data.redis.port}") private int redisPort; @Value("${spring.data.redis.timeout}") private int redisTimeout; @PostConstruct public void validateProperties() { Assert.hasText(redisHost, "Redis host cannot be empty"); Assert.isTrue(redisPort >= 0 && redisPort <= 65535, "Redis port must be between 0 and 65535"); Assert.isTrue(redisTimeout > 0, "Redis timeout must be positive"); }
47-53: 🧹 Nitpick (assertive)
Consider implementing cache-specific configurations.
The current setup uses the same configuration for all caches. Consider:
- Implementing different TTL settings for different caches
- Adding transaction support if needed
- Monitoring and statistics collection
@Bean public RedisCacheManager cacheManager(RedisConnectionFactory redisConnectionFactory) { RedisCacheConfiguration redisCacheConfiguration = cacheConfiguration(); + + // Configure different TTLs for specific caches + Map<String, RedisCacheConfiguration> cacheConfigurations = new HashMap<>(); + cacheConfigurations.put("shortLivedCache", redisCacheConfiguration.entryTtl(Duration.ofMinutes(5))); + cacheConfigurations.put("longLivedCache", redisCacheConfiguration.entryTtl(Duration.ofHours(1))); + return RedisCacheManager.builder(redisConnectionFactory) .cacheDefaults(redisCacheConfiguration) + .withInitialCacheConfigurations(cacheConfigurations) + .transactionAware() .build(); }📝 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.@Bean public RedisCacheManager cacheManager(RedisConnectionFactory redisConnectionFactory) { RedisCacheConfiguration redisCacheConfiguration = cacheConfiguration(); // Configure different TTLs for specific caches Map<String, RedisCacheConfiguration> cacheConfigurations = new HashMap<>(); cacheConfigurations.put("shortLivedCache", redisCacheConfiguration.entryTtl(Duration.ofMinutes(5))); cacheConfigurations.put("longLivedCache", redisCacheConfiguration.entryTtl(Duration.ofHours(1))); return RedisCacheManager.builder(redisConnectionFactory) .cacheDefaults(redisCacheConfiguration) .withInitialCacheConfigurations(cacheConfigurations) .transactionAware() .build(); }
1-54: 💡 Codebase verification
Redis security configurations missing - Action Required
The verification confirms that Redis security measures are not implemented. Please add the following configurations to
application.properties:
- Redis authentication:
spring.data.redis.password=${REDIS_PASSWORD}
- SSL/TLS configuration:
spring.data.redis.ssl=true spring.data.redis.ssl.trust-store=/path/to/truststore spring.data.redis.ssl.trust-store-password=${TRUST_STORE_PASSWORD}🔗 Analysis chain
Implement security measures for Redis connection.
Consider implementing the following security measures for production deployment:
- SSL/TLS configuration
- Redis authentication
- Network security policies
Run the following script to check if SSL properties are defined in the application properties:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for Redis SSL configuration in application properties # Expected: Properties related to Redis SSL configuration rg -l "spring\.redis\.ssl" || echo "No Redis SSL configuration found" rg -l "spring\.redis\.password" || echo "No Redis authentication configuration found"Length of output: 234
Script:
#!/bin/bash # Search for any Redis-related properties in yaml/properties files fd -e yml -e yaml -e properties --exec rg -l "spring\.data\.redis" {} \; # Search for any Redis security related configurations rg -i "ssl|password|auth|security.*redis" -g "*.{yml,yaml,properties}"Length of output: 8688
backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/util/ProjectUtil.java (2)
124-130: 🛠️ Refactor suggestion
Add input validation for required parameters.
While the builder pattern is well implemented, consider adding validation for required parameters before creating the request object to prevent potential NullPointerException.
+ if (projectId == null || tenantId == null) { + throw new IllegalArgumentException("Project ID and Tenant ID are required parameters"); + } ProjectRequest projectRequest = ProjectRequest.builder() .requestInfo(requestInfo) .projects(Collections.singletonList(Project.builder().id(projectId)📝 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 (projectId == null || tenantId == null) { throw new IllegalArgumentException("Project ID and Tenant ID are required parameters"); } ProjectRequest projectRequest = ProjectRequest.builder() .requestInfo(requestInfo) .projects(Collections.singletonList(Project.builder().id(projectId) .tenantId(tenantId) .address(Address.builder().boundary(localityCode).build()) .build())) .build();
114-117: 🧹 Nitpick (assertive)
Consider using UriComponentsBuilder for safer URL construction.
While the URL construction works, using string concatenation could be vulnerable to injection if the parameters contain special characters. Consider using Spring's UriComponentsBuilder for safer URL construction.
- uriBuilder.append("?").append(TENANT_ID).append(EQUAL_TO).append(tenantId) - .append(AMPERSAND) - .append("isAncestorProjectId").append(EQUAL_TO).append("true") - .append(AMPERSAND) - .append("includeDescendants").append(EQUAL_TO).append("false") + UriComponentsBuilder builder = UriComponentsBuilder.fromUriString(uriBuilder.toString()) + .queryParam(TENANT_ID, tenantId) + .queryParam("isAncestorProjectId", true) + .queryParam("includeDescendants", false);Committable suggestion skipped: line range outside the PR's diff.
backend/expense-calculator/src/main/resources/application.properties (3)
122-124:
⚠️ Potential issueSecurity Risk: Remove hardcoded localhost URLs from configuration.
Several service endpoints are configured to use localhost URLs:
- Project service:
http://localhost:8287/- Individual service:
http://localhost:8286- Attendance service:
http://localhost:8088This poses security risks and deployment issues in production environments.
Recommendations:
- Move these configurations to environment-specific property files
- Use environment variables or configuration service for different environments
- Consider using service discovery for microservice communication
Example structure:
src/main/resources/ ├── application.properties # Common properties ├── application-dev.properties # Development properties ├── application-prod.properties # Production propertiesAlso applies to: 163-165, 168-170
184-194: 🧹 Nitpick (assertive)
Enhance report configuration with validation properties.
The report configuration is well-structured but could benefit from additional properties:
- Maximum report size limits
- Report generation timeout
- Retry attempts configuration
- File format validations
Add the following properties:
report.max.size.mb=10 report.generation.timeout.seconds=300 report.retry.max.attempts=3 report.allowed.formats=PDF,XLSX
201-207: 🛠️ Refactor suggestion
Enhance Redis and async processing configuration for production use.
The current configuration lacks important production-ready properties for Redis and async processing:
- Redis:
- Missing SSL configuration
- No connection pool settings
- No sentinel/cluster configuration
- Async Processing:
- Missing error handling configuration
- No timeout settings
Add the following properties:
# Redis Production Properties spring.data.redis.ssl=true spring.data.redis.pool.max-active=8 spring.data.redis.pool.max-idle=8 spring.data.redis.pool.min-idle=2 spring.data.redis.pool.max-wait=-1ms # Async Processing bill.generation.async.max.retries=3 bill.generation.async.timeout.seconds=300 bill.generation.async.error.topic=bill-generation-error-topicbackend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/validator/ExpenseCalculatorServiceValidator.java (2)
75-81: 🛠️ Refactor suggestion
Add null check for attendance registers.
The new validation method for attendance register approval should include a null check before processing the list.
Apply this diff to add null check:
public void validateAttendanceRegisterApproval(List<AttendanceRegister> attendanceRegisters) { + if (attendanceRegisters == null || attendanceRegisters.isEmpty()) { + throw new CustomException("INVALID_ATTENDANCE_REGISTERS", "Attendance registers cannot be null or empty"); + } attendanceRegisters.stream().forEach(attendanceRegister -> { if(!"APPROVED".equalsIgnoreCase(attendanceRegister.getReviewStatus())) {📝 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.public void validateAttendanceRegisterApproval(List<AttendanceRegister> attendanceRegisters) { if (attendanceRegisters == null || attendanceRegisters.isEmpty()) { throw new CustomException("INVALID_ATTENDANCE_REGISTERS", "Attendance registers cannot be null or empty"); } attendanceRegisters.stream().forEach(attendanceRegister -> { if(!"APPROVED".equalsIgnoreCase(attendanceRegister.getReviewStatus())) { throw new CustomException("ATTENDANCE_REGISTERS_APPROVED", "Atleast one AttendanceRegister is not approved"); } });
255-261: 🧹 Nitpick (assertive)
Use constant for error message and status.
The contract validation logic should use constants for error messages and status codes for consistency and maintainability.
Apply this diff to use constants:
+ private static final String INVALID_CONTRACT_ID = "INVALID_CONTRACT_ID"; + private static final String CONTRACT_NOT_FOUND = "Contract not found"; + private void validateContractIdAgainstService(CalculationRequest calculationRequest) { final Criteria criteria = calculationRequest.getCriteria(); if(StringUtils.isNotBlank(criteria.getReferenceId())) { List<Contract> contracts = expenseCalculatorUtil.fetchContract(calculationRequest.getRequestInfo(), criteria.getTenantId(), criteria.getReferenceId()); if (CollectionUtils.isEmpty(contracts)) { log.error("ExpenseCalculatorServiceValidator:No matched contract found for contractId - "+criteria.getReferenceId()); - throw new CustomException("INVALID_CONTRACT_ID", "Contract not found"); + throw new CustomException(INVALID_CONTRACT_ID, CONTRACT_NOT_FOUND); }Committable suggestion skipped: line range outside the PR's diff.
backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/util/ExpenseCalculatorUtil.java (3)
8-8: 🧹 Nitpick (assertive)
Consider replacing wildcard imports with explicit imports.
Wildcard imports can make it harder to track dependencies and may lead to naming conflicts. Consider explicitly importing only the required classes from:
org.egov.digit.expense.calculator.web.models.*org.egov.works.services.common.models.musterroll.*Also applies to: 13-13, 19-19
95-106: 🧹 Nitpick (assertive)
Add method documentation and remove unnecessary empty line.
Please add Javadoc to explain the purpose of this method and its parameters. Also, remove the unnecessary empty line at line 100.
108-119: 🧹 Nitpick (assertive)
Document the difference between V2 and non-V2 versions.
Please add Javadoc to:
- Explain the purpose of this method
- Document how it differs from
fetchMusterRollByRegIds- Remove the unnecessary empty line at line 113
backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/web/models/Criteria.java (2)
49-55: 🧹 Nitpick (assertive)
Consider providing migration guidance for breaking changes.
The introduction of two new mandatory fields (
@NotNull) is a breaking change that affects all existing clients. Consider:
- Adding migration documentation
- Implementing a backward compatibility strategy
- Versioning the API appropriately
53-55: 💡 Codebase verification
Adding mandatory localityCode field requires client updates
The addition of a mandatory
localityCodefield with@NotNullconstraint is a breaking change that will impact all clients:
- The API contract (mukta-calculator-service-1.0.0.yaml) doesn't define this field
- The ExpenseCalculatorServiceValidator only validates tenantId and either musterRollId or contractId
- No existing clients appear to be using this field
Before making this change:
- Update the API contract to include the new field
- Add validation logic in ExpenseCalculatorServiceValidator
- Notify clients about the new requirement
- Consider making it optional initially with a migration period
🔗 Analysis chain
Ensure all clients can provide the locality code.
The addition of a mandatory
localityCodefield with@NotNullconstraint is a breaking change that requires all clients to provide this value.Let's verify if the clients are prepared for this new requirement:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Criteria object construction in tests and implementations ast-grep --pattern 'Criteria($$$)' # Search for locality-related configurations or data rg -i "locality.*code" --type java --type jsonLength of output: 66720
Script:
#!/bin/bash # Search for files importing or using Criteria class rg -t java "import.*Criteria|class.*Criteria|extends.*Criteria|implements.*Criteria" # Search for expense calculator test files fd -e java -e json test backend/expense-calculator/ # Search for API specs or documentation fd -e yaml -e yml openapi swagger apiLength of output: 21945
Script:
#!/bin/bash # Get implementation of services using Criteria ast-grep --pattern 'class SupervisionBillGeneratorService { $$$ }' # Get validator implementation ast-grep --pattern 'class ExpenseCalculatorServiceValidator { $$$ }' # Search for test files in parent directories fd -e java test backend/expense-calculator/ ../Length of output: 10236
Script:
#!/bin/bash # Get content of service implementations cat backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/service/SupervisionBillGeneratorService.java cat backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/validator/ExpenseCalculatorServiceValidator.java # Search for API contracts fd -e yaml -e json "expense|calculator" -t fLength of output: 39167
Script:
#!/bin/bash # Get API contract content cat backend/expense-calculator/src/main/resources/mukta-calculator-service-1.0.0.yaml # Search for any existing usages of locality code in configs rg -g '*.{json,yaml,yml}' -i "locality.*code"Length of output: 81349
backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/service/WageSeekerBillGeneratorService.java (7)
54-54: 🧹 Nitpick (assertive)
Remove redundant
@Autowiredannotation on the constructorSince there is only one constructor in the class, the
@Autowiredannotation is optional. Removing it can simplify the code.
416-416: 🧹 Nitpick (assertive)
Reconsider the access level of
buildLineItemmethodChanging the
buildLineItemmethod fromprivatetopublicexposes it externally. If it's not intended to be part of the public API, consider keeping itprivateor making itprotectedto maintain encapsulation.
143-143:
⚠️ Potential issueInitialize
bill.getTotalAmount()before additionBefore adding to
bill.getTotalAmount(), ensure it is initialized toBigDecimal.ZEROto prevent aNullPointerException.Apply this diff to initialize the total amount if necessary:
+ if (bill.getTotalAmount() == null) { + bill.setTotalAmount(BigDecimal.ZERO); + } bill.setTotalAmount(bill.getTotalAmount().add(billDetail.getTotalAmount()));📝 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 (bill.getTotalAmount() == null) { bill.setTotalAmount(BigDecimal.ZERO); } bill.setTotalAmount(bill.getTotalAmount().add(billDetail.getTotalAmount()));
101-101:
⚠️ Potential issueHandle potential duplicate keys when creating
individualMapUsing
Collectors.toMapwithout handling duplicate keys can lead to ajava.lang.IllegalStateExceptionif there are individuals with the same ID. Ensure that individual IDs are unique or handle duplicates appropriately.Apply this diff to handle duplicate keys:
- Map<String, Individual> individualMap = individuals.stream() - .collect(Collectors.toMap(Individual::getId, individual -> individual)); + Map<String, Individual> individualMap = individuals.stream() + .collect(Collectors.toMap(Individual::getId, individual -> individual, (existing, replacement) -> existing));📝 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.Map<String, Individual> individualMap = individuals.stream().collect(Collectors.toMap(Individual::getId, individual -> individual, (existing, replacement) -> existing));
109-111:
⚠️ Potential issueAdd null check for
individual.getSkills()To prevent a potential
NullPointerException, ensure thatindividual.getSkills()is not null before callingisEmpty().Apply this diff to add the null check:
- if (individual.getSkills().isEmpty() || StringUtils.isBlank(individual.getSkills().get(0).getType())) { + if (individual.getSkills() == null || individual.getSkills().isEmpty() || StringUtils.isBlank(individual.getSkills().get(0).getType())) {📝 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 (individual.getSkills() == null || individual.getSkills().isEmpty() || StringUtils.isBlank(individual.getSkills().get(0).getType())) { log.error("Skill not present in individual service :: " + individualEntry.getIndividualId()); } else {
418-418: 🛠️ Refactor suggestion
Use
RoundingModeenum instead of deprecated constantThe constant
BigDecimal.ROUND_HALF_UPis deprecated. Replace it withRoundingMode.HALF_UPfor better compatibility.Apply this diff to update the rounding mode:
- BigDecimal roundOffAmount = actualAmountToPay.setScale(2, BigDecimal.ROUND_HALF_UP); + BigDecimal roundOffAmount = actualAmountToPay.setScale(2, RoundingMode.HALF_UP);📝 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.BigDecimal roundOffAmount = actualAmountToPay.setScale(2, RoundingMode.HALF_UP);
114-114:
⚠️ Potential issueCheck for empty
workerMdmslist before accessingAccessing
workerMdms.get(0)without verifying thatworkerMdmsis not empty can lead to anIndexOutOfBoundsExceptionif the list is empty. Add a check to ensureworkerMdmscontains at least one element before accessing it.Apply this diff to add the check:
+ if (workerMdms.isEmpty()) { + log.error("workerMdms is empty"); + continue; + } Optional<WorkerRate> rate = workerMdms.get(0).getRates().stream() .filter(workerRate -> workerRate.getSkillCode() != null && workerRate.getSkillCode().equalsIgnoreCase(skillCode)) .findAny();Committable suggestion skipped: line range outside the PR's diff.
backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/kafka/ExpenseCalculatorConsumer.java (3)
110-111:
⚠️ Potential issueAvoid including sensitive data in error messages
Including
consumerRecordin the error message may expose sensitive or confidential information. Consider logging only the exception message or a sanitized version of the consumer record to avoid potential data leakage.Apply this diff to address the issue:
- producer.push(configs.getReportErrorQueueTopic(), exception.getMessage() + " : " + consumerRecord); + producer.push(configs.getReportErrorQueueTopic(), exception.getMessage());📝 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.producer.push(configs.getReportErrorQueueTopic(), exception.getMessage()); }
123-124:
⚠️ Potential issueAvoid including sensitive data in error messages
Including
consumerRecordin the error message may expose sensitive or confidential information. Consider logging only the exception message or a sanitized version of the consumer record to prevent potential data leakage.Apply this diff to address the issue:
- producer.push(configs.getCalculatorErrorTopic(), exception.getMessage() + " : " + consumerRecord); + producer.push(configs.getCalculatorErrorTopic(), exception.getMessage());📝 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.log.error("Error occurred while processing the report from topic : " + topic, exception); producer.push(configs.getCalculatorErrorTopic(), exception.getMessage());
81-113:
⚠️ Potential issueAvoid blocking the Kafka listener thread with
Thread.sleepUsing
Thread.sleepwithin the Kafka listener can block the consumer thread, leading to reduced throughput and potential delays in processing other messages. Instead of sleeping the thread, consider implementing a non-blocking retry mechanism using a scheduled task or leveraging Kafka's delayed retries and dead-letter queues to handle message reprocessing without blocking the consumer thread.Apply this diff to address the issue:
- Thread.sleep(10 * 1000); - producer.push(configs.getReportRetryQueueTopic(), request); + // Remove Thread.sleep and implement a non-blocking retry mechanism + // For example, configure retry policies or use a scheduling framework to retry without blocking + producer.push(configs.getReportRetryQueueTopic(), request);Committable suggestion skipped: line range outside the PR's diff.
backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/util/BillExcelGenerate.java (2)
208-220: 🧹 Nitpick (assertive)
Remove commented-out code used for local testing
The code between lines 209 and 219 is commented out and intended for local testing. Consider removing this code to keep the codebase clean. If you need to retain it for future reference, it might be better to document it externally or use a debugging tool.
Apply this diff to clean up the code:
- /* - // This code is for local testing, it saves the file to the specified path - try (FileOutputStream outputStream = new FileOutputStream("Sample.xlsx")) { - workbook.write(outputStream); - workbook.close(); - log.info("Excel file saved successfully at: " + "Sample.xlsx"); - } catch (IOException e) { - log.error("Exception while saving Excel file: " + e); - throw new CustomException("EXCEL_SAVE_ERROR", "Exception while saving Excel file to local system"); - } - return null; - */
400-406: 🧹 Nitpick (assertive)
Correct the typo in method name
The method
getFormatedDateTimehas a typo in the name. It should begetFormattedDateTimeto reflect the correct spelling.Apply this diff to correct the typo:
- private String getFormatedDateTime(long timestamp) { + private String getFormattedDateTime(long timestamp) {Also, update all references to this method:
- setCellValueWithAlignment(auditCellDateAndTimeValue, getFormatedDateTime(reportBill.getCreatedTime()), textStyle, numberStyle); + setCellValueWithAlignment(auditCellDateAndTimeValue, getFormattedDateTime(reportBill.getCreatedTime()), textStyle, numberStyle);Committable suggestion skipped: line range outside the PR's diff.
backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/service/HealthBillReportGenerator.java (3)
323-326:
⚠️ Potential issueAdd null check for
individual.getId()To prevent a potential
NullPointerException, ensure thatindividual.getId()is not null before using it as a key in the map lookup.Apply this diff to add the null check:
- if (individualMusterAttendanceMap.containsKey(billDetail.getReferenceId()) && individualMusterAttendanceMap.get(billDetail.getReferenceId()).containsKey(individual.getId())) { + if (individual.getId() != null && individualMusterAttendanceMap.containsKey(billDetail.getReferenceId()) && individualMusterAttendanceMap.get(billDetail.getReferenceId()).containsKey(individual.getId())) {📝 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 (individual.getId() != null && individualMusterAttendanceMap.containsKey(billDetail.getReferenceId()) && individualMusterAttendanceMap.get(billDetail.getReferenceId()).containsKey(individual.getId())) { totalNumberOfDays = individualMusterAttendanceMap.get(billDetail.getReferenceId()).get(individual.getId()); reportBillDetail.setTotalNumberOfDays(individualMusterAttendanceMap.get(billDetail.getReferenceId()).get(individual.getId()).floatValue()); }
339-344: 🧹 Nitpick (assertive)
Remove unused method
perDayAmountThe method
perDayAmountis not used within the class. Removing unused methods helps in maintaining a clean and understandable codebase.Apply this diff to remove the unused method:
- private BigDecimal perDayAmount(BigDecimal amount, BigDecimal totalNumberOfDays) { - if (totalNumberOfDays.compareTo(BigDecimal.ZERO) == 0) { - return BigDecimal.ZERO; - } - return amount.divide(totalNumberOfDays, 2, RoundingMode.HALF_UP); - }
408-415:
⚠️ Potential issueEnsure
additionalDetailsis aMapbefore castingIn the
updateReportStatusmethod,additionalDetailsis cast toMap<String, Object>without verifying its actual type. This could lead to aClassCastExceptionifadditionalDetailsis not aMap. To prevent this, check the instance type before casting or handle the scenario where it might be of a different type.Apply this diff to address the issue:
// Ensure additionalDetails is initialized Object additionalDetails = billRequest.getBill().getAdditionalDetails(); if (additionalDetails == null) { additionalDetails = new HashMap<String, Object>(); + } else if (!(additionalDetails instanceof Map)) { + log.error("additionalDetails is not of type Map"); + throw new CustomException("INVALID_ADDITIONAL_DETAILS", "additionalDetails must be a Map"); } // Cast additionalDetails to Map Map<String, Object> additionalDetailsMap = (Map<String, Object>) additionalDetails;Committable suggestion skipped: line range outside the PR's diff.
backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/service/ExpenseCalculatorService.java (4)
61-61: 🛠️ Refactor suggestion
Potential confusion due to similarly named fields and constructor parameters
The class declares two fields,
projectUtilsandprojectUtil, which are of the same typeProjectUtil. Similarly, the constructor parameters include bothprojectUtilsandprojectUtil. This can lead to confusion and potential errors when assigning values. Consider renaming one of them to clearly distinguish their purposes.Also applies to: 67-67, 85-85
754-756: 🧹 Nitpick (assertive)
Clarify boundary type comparison in
isBoundaryDistrictLevelmethodIn the method
isBoundaryDistrictLevel, the boundary type is compared usingequals("DISTRICT"). Boundary types might vary in case or format. Consider usingequalsIgnoreCaseor validating against a constant to ensure the comparison is case-insensitive and less error-prone.Apply this diff to use
equalsIgnoreCase:private boolean isBoundaryDistrictLevel(TenantBoundary boundary) { - return boundary.getBoundary().get(0).getBoundaryType().equals("DISTRICT"); + return "DISTRICT".equalsIgnoreCase(boundary.getBoundary().get(0).getBoundaryType()); }📝 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.private boolean isBoundaryDistrictLevel(TenantBoundary boundary) { return "DISTRICT".equalsIgnoreCase(boundary.getBoundary().get(0).getBoundaryType()); }
192-248:
⚠️ Potential issueRemove the test method
createWageOrSupervisionBillTestfrom production codeThe method
createWageOrSupervisionBillTestappears to be a test or debugging method included in the production code. Including test methods in the main service class is not a best practice and can introduce unintended behavior. Please remove this method or move it to an appropriate test class.Apply this diff to remove the method:
- public List<Bill> createWageOrSupervisionBillTest(CalculationRequest calculationRequest){ - Long startTime = System.currentTimeMillis(); - log.info("Start time :: " + startTime); - Bill bill = Bill.builder().build(); - String projectId = UUID.randomUUID().toString(); - for (int i = 0; i < Integer.valueOf(String.valueOf(calculationRequest.getCriteria().getFromPeriod())); i++) { - - LineItem payableLineItem = wageSeekerBillGeneratorService - .buildLineItem("mz", BigDecimal.TEN, "FOOD", LineItem.TypeEnum.PAYABLE); - LineItem payableLineItem2 = wageSeekerBillGeneratorService - .buildLineItem("mz", BigDecimal.TEN, "TRAVEL", LineItem.TypeEnum.PAYABLE); - LineItem payableLineItem3 = wageSeekerBillGeneratorService - .buildLineItem("mz", BigDecimal.TEN, "PER_DAY", LineItem.TypeEnum.PAYABLE); - List<LineItem> lineItems = new ArrayList<>(); - lineItems.add(payableLineItem); - lineItems.add(payableLineItem2); - lineItems.add(payableLineItem3); - BillDetail billDetail = BillDetail.builder() - .payableLineItems(lineItems) - .lineItems(new ArrayList<>()) - .totalAmount(BigDecimal.valueOf(10000)) - .referenceId(projectId) - .tenantId("mz") - .payee(Party.builder() - .tenantId("mz") - .identifier(UUID.randomUUID().toString()) - .type("IND") - .build()) - .build(); - bill.addBillDetailsItem(billDetail); - } - bill.setFromPeriod(1734373800000L); - bill.setBillDate(System.currentTimeMillis()); - bill.setToPeriod(1736101799000L); - bill.setTenantId("mz"); - bill.setReferenceId(projectId); - bill.setBusinessService("EXPENSE.WAGES"); - bill.setStatus("ACTIVE"); - bill.setLocalityCode("MICROPLAN_MO_13_05_TCHIEN_TEST"); - bill.setPayer(Party.builder().identifier(UUID.randomUUID().toString()).tenantId("mz").type("ORG").build()); - Set<String> distinctRegisters = bill.getBillDetails().stream().map(BillDetail::getReferenceId).collect(Collectors.toSet()); - ObjectNode additionalDetails = mapper.createObjectNode(); - if(bill.getAdditionalDetails() != null){ - additionalDetails = objectMapper.convertValue(bill.getAdditionalDetails(), ObjectNode.class); - } - additionalDetails.put("noOfRegisters", distinctRegisters.size()); - bill.setAdditionalDetails(additionalDetails); - - Workflow workflow = Workflow.builder() - .action(WF_SUBMIT_ACTION_CONSTANT) - .build(); - BillResponse billResponse = postCreateBill(calculationRequest.getRequestInfo(), bill,workflow); - - log.info("Time taken to create bill is " + (System.currentTimeMillis() - startTime)/1000 + " seconds"); - return billResponse.getBills(); - }
382-403:
⚠️ Potential issueHandle potential
NullPointerExceptionin exception blocksWithin the
catchblocks starting at line 381 and 395, there is an assumption thatprojectResponse.getProject()will not be null. IfprojectResponseorprojectResponse.getProject()is null, it will cause aNullPointerException. Ensure that null checks are in place before accessing these methods.Apply this diff to add null checks:
} catch (CustomException customException) { String referenceId = null; - if (projectResponse.getProject().get(0).getProjectHierarchy() == null) { + if (projectResponse != null && projectResponse.getProject() != null && !projectResponse.getProject().isEmpty() && + projectResponse.getProject().get(0).getProjectHierarchy() == null) { referenceId = projectResponse.getProject().get(0).getId(); } else { referenceId = projectResponse.getProject().get(0).getProjectHierarchy(); } // ... remaining code ... } catch (Exception e) { String referenceId = null; + if (projectResponse != null && projectResponse.getProject() != null && !projectResponse.getProject().isEmpty()) { referenceId = projectResponse.getProject().get(0).getProjectHierarchy(); + } else { + log.error("Project response is null or empty", e); + throw e; + } // ... remaining code ... }Committable suggestion skipped: line range outside the PR's diff.
backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/web/models/TenantBoundary.java (1)
19-29: 🛠️ Refactor suggestion
Add validation constraints and documentation.
The class lacks proper validation constraints and documentation. Consider the following improvements:
- Add
@NotNullconstraints for required fields- Add Javadoc comments describing the purpose of each field
- Consider using
@Sizeconstraints for the boundary list if applicable@Builder public class TenantBoundary { + /** + * Unique identifier of the tenant + */ @JsonProperty("tenantId") + @NotNull private String tenantId = null; + /** + * Type of boundary hierarchy + */ @JsonProperty("hierarchyType") + @NotNull private String hierarchyType = null; + /** + * List of boundaries associated with the tenant + */ @JsonProperty("boundary") + @NotNull + @Size(min = 1) private List<Boundary> boundary = null; }Committable suggestion skipped: line range outside the PR's diff.
backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/web/models/report/BillReportRequest.java (2)
15-21: 🧹 Nitpick (assertive)
Remove redundant Lombok annotations.
@Getterand@Setterannotations are redundant when using@Data.@Data @AllArgsConstructor @NoArgsConstructor @Builder -@Getter -@Setter public class BillReportRequest {📝 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.@Data @AllArgsConstructor @NoArgsConstructor @Builder public class BillReportRequest {
11-13: 🧹 Nitpick (assertive)
Fix incorrect class name in Javadoc.
The Javadoc comment refers to "BillRequest" instead of "BillReportRequest".
/** - * BillRequest + * Request object for bill report generation */📝 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./** * Request object for bill report generation */backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/web/models/Boundary.java (1)
18-31: 🛠️ Refactor suggestion
Add validation constraints and documentation for hierarchical structure.
The class represents a hierarchical boundary structure but lacks proper validation and documentation:
- Add validation constraints for required fields
- Add documentation explaining the hierarchical nature
- Consider adding depth limit validation for children to prevent excessive nesting
+/** + * Represents a boundary in a hierarchical structure. + * Each boundary can have child boundaries, forming a tree structure. + */ @Builder public class Boundary { + /** + * Unique identifier of the boundary + */ @JsonProperty("id") + @NotNull private String id = null; + /** + * Unique code of the boundary + */ @JsonProperty("code") + @NotNull private String code = null; + /** + * Type of the boundary + */ @JsonProperty("boundaryType") + @NotNull private String boundaryType = null; + /** + * Child boundaries in the hierarchy + */ @JsonProperty("children") + @Valid private List<Boundary> children = 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. Thoroughly test & benchmark the code to ensure it meets the requirements./** * Represents a boundary in a hierarchical structure. * Each boundary can have child boundaries, forming a tree structure. */ @Builder public class Boundary { /** * Unique identifier of the boundary */ @JsonProperty("id") @NotNull private String id = null; /** * Unique code of the boundary */ @JsonProperty("code") @NotNull private String code = null; /** * Type of the boundary */ @JsonProperty("boundaryType") @NotNull private String boundaryType = null; /** * Child boundaries in the hierarchy */ @JsonProperty("children") @Valid private List<Boundary> children = null;backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/web/models/BoundaryResponse.java (2)
27-29: 🧹 Nitpick (assertive)
Fix inconsistent JSON property name.
The JSON property name "TenantBoundary" doesn't match the field name "boundary". Consider making them consistent.
- @JsonProperty("TenantBoundary") + @JsonProperty("boundary") @Valid private List<TenantBoundary> boundary = 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. Thoroughly test & benchmark the code to ensure it meets the requirements.@JsonProperty("boundary") @Valid private List<TenantBoundary> boundary = null;
32-38: 🧹 Nitpick (assertive)
Add documentation for the helper method.
The helper method lacks documentation explaining its purpose and behavior.
+ /** + * Adds a TenantBoundary item to the boundary list. + * Initializes the list if it's null. + * + * @param boundaryItem The TenantBoundary item to add + * @return The current BoundaryResponse instance for method chaining + */ public BoundaryResponse addBoundaryItem(TenantBoundary boundaryItem) {📝 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./** * Adds a TenantBoundary item to the boundary list. * Initializes the list if it's null. * * @param boundaryItem The TenantBoundary item to add * @return The current BoundaryResponse instance for method chaining */ public BoundaryResponse addBoundaryItem(TenantBoundary boundaryItem) { if (this.boundary == null) { this.boundary = new ArrayList<>(); } this.boundary.add(boundaryItem); return this; }backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/web/controllers/HealthReportApiController.java (3)
42-48:
⚠️ Potential issueRemove test code marked with TODO.
The endpoint and its implementation are marked as test code that should be deleted. If this is production code, remove the TODO comment. If it's truly test code, it should be removed before merging.
49-62: 🛠️ Refactor suggestion
Clean up commented-out code.
Remove the commented-out Excel generation code if it's no longer needed. If it's needed for future implementation, create a separate issue to track it and remove the code from this file.
23-26: 🧹 Nitpick (assertive)
Enhance API documentation.
Consider adding comprehensive API documentation using Swagger annotations to describe:
- Request/response models
- Possible response codes
- Error scenarios
backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/repository/ExpenseCalculatorRepository.java (2)
48-60: 🛠️ Refactor suggestion
Move SQL queries to QueryBuilder class.
For consistency and maintainability, move the SQL query to the
ExpenseCalculatorQueryBuilderclass, following the pattern used in other methods.
85-106: 🛠️ Refactor suggestion
Add error handling for database operations.
The
getBillStatusByReferenceIdmethod should include proper error handling for database exceptions and null checks for the result map.Example:
public List<BillStatus> getBillStatusByReferenceId(String referenceId) { + try { String sql = "SELECT * FROM eg_expense_bill_gen_status " + " WHERE referenceid = :referenceId "; Map<String, Object> params = new HashMap<>(); params.put("referenceId", referenceId); List<Map<String, Object>> objectMap = namedParameterJdbcTemplate.queryForList(sql, params); List<BillStatus> billStatusList = new ArrayList<>(); for (Map<String, Object> row : objectMap) { + if (row == null) continue; BillStatus billStatus = BillStatus.builder() .id((String) row.get("id")) .referenceId((String) row.get("referenceid")) .tenantId((String) row.get("tenantid")) .status((String) row.get("status")).build(); billStatusList.add(billStatus); } return billStatusList; + } catch (Exception e) { + throw new CustomDBException("Failed to fetch bill status", e); + } }Committable suggestion skipped: line range outside the PR's diff.
backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/config/ExpenseCalculatorConfiguration.java (3)
21-290: 🧹 Nitpick (assertive)
Consider splitting configuration into domain-specific classes.
The configuration class has grown quite large with many responsibilities. Consider splitting it into domain-specific configuration classes:
- UserConfiguration
- WorkflowConfiguration
- ReportConfiguration
- HealthConfiguration
etc.
215-216: 🛠️ Refactor suggestion
Add default value for feature flag.
The health integration feature flag should have a default value to prevent potential issues if the property is missing from the configuration.
-@Value("${is.health.integration.enabled}") +@Value("${is.health.integration.enabled:false}") private boolean isHealthIntegrationEnabled;📝 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.@Value("${is.health.integration.enabled:false}") private boolean isHealthIntegrationEnabled;
282-283: 🛠️ Refactor suggestion
Add default value for attendance approval flag.
Similar to the health integration flag, add a default value for the attendance approval requirement.
-@Value("${is.attendance.approval.required}") +@Value("${is.attendance.approval.required:true}") private boolean isAttendanceApprovalRequired;📝 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.@Value("${is.attendance.approval.required:true}") private boolean isAttendanceApprovalRequired;backend/expense-calculator/src/main/resources/db/migration/main/V20250108123015__eg_expense_calc_status_ddl.sql (1)
2-11: 🛠️ Refactor suggestion
Add primary key constraint and indexes for better performance.
Consider the following improvements to the table structure:
- Add a primary key constraint on the
idcolumn.- Add an index on
referenceidcolumn as it's likely to be frequently queried.- Add column comments to improve documentation.
Apply this diff to enhance the table structure:
CREATE TABLE IF NOT EXISTS eg_expense_bill_gen_status ( id character varying(64) NOT NULL, tenantid character varying(64) NOT NULL, referenceid character varying(256) NOT NULL, status character varying(64) NOT NULL, - error text + error text, + CONSTRAINT pk_eg_expense_bill_gen_status PRIMARY KEY (id), + CONSTRAINT uk_eg_expense_bill_gen_status_ref UNIQUE (referenceid) ) +COMMENT ON TABLE eg_expense_bill_gen_status IS 'Tracks the status of expense bill generation'; +COMMENT ON COLUMN eg_expense_bill_gen_status.id IS 'Unique identifier for the status record'; +COMMENT ON COLUMN eg_expense_bill_gen_status.tenantid IS 'Tenant identifier'; +COMMENT ON COLUMN eg_expense_bill_gen_status.referenceid IS 'Reference to the bill being generated'; +COMMENT ON COLUMN eg_expense_bill_gen_status.status IS 'Current status of bill generation'; +COMMENT ON COLUMN eg_expense_bill_gen_status.error IS 'Error message if bill generation fails'; + +CREATE INDEX idx_eg_expense_bill_gen_status_ref ON eg_expense_bill_gen_status(referenceid);📝 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.CREATE TABLE IF NOT EXISTS eg_expense_bill_gen_status ( id character varying(64) NOT NULL, tenantid character varying(64) NOT NULL, referenceid character varying(256) NOT NULL, status character varying(64) NOT NULL, error text, CONSTRAINT pk_eg_expense_bill_gen_status PRIMARY KEY (id), CONSTRAINT uk_eg_expense_bill_gen_status_ref UNIQUE (referenceid) ) COMMENT ON TABLE eg_expense_bill_gen_status IS 'Tracks the status of expense bill generation'; COMMENT ON COLUMN eg_expense_bill_gen_status.id IS 'Unique identifier for the status record'; COMMENT ON COLUMN eg_expense_bill_gen_status.tenantid IS 'Tenant identifier'; COMMENT ON COLUMN eg_expense_bill_gen_status.referenceid IS 'Reference to the bill being generated'; COMMENT ON COLUMN eg_expense_bill_gen_status.status IS 'Current status of bill generation'; COMMENT ON COLUMN eg_expense_bill_gen_status.error IS 'Error message if bill generation fails'; CREATE INDEX idx_eg_expense_bill_gen_status_ref ON eg_expense_bill_gen_status(referenceid);backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/web/models/WorkerRate.java (1)
18-22: 🧹 Nitpick (assertive)
Add validation constraints and improve documentation.
Consider the following improvements:
- Add validation constraints for required fields.
- Document the structure and expected keys of the
rateBreakupmap.Apply this diff to enhance the class:
@JsonProperty("skillCode") + @NotBlank(message = "Skill code is required") + @Size(max = 64, message = "Skill code length must not exceed 64 characters") private String skillCode; @JsonProperty("rateBreakup") + @NotNull(message = "Rate breakup is required") private Map<String, BigDecimal> rateBreakup; + + /** + * Rate breakup map structure: + * - Key: The type of rate (e.g., "BASE_RATE", "ALLOWANCE", etc.) + * - Value: The amount for that rate type + */📝 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.@JsonProperty("skillCode") @NotBlank(message = "Skill code is required") @Size(max = 64, message = "Skill code length must not exceed 64 characters") private String skillCode; @JsonProperty("rateBreakup") @NotNull(message = "Rate breakup is required") private Map<String, BigDecimal> rateBreakup; /** * Rate breakup map structure: * - Key: The type of rate (e.g., "BASE_RATE", "ALLOWANCE", etc.) * - Value: The amount for that rate type */backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/web/models/report/ReportBill.java (2)
38-53: 🧹 Nitpick (assertive)
Add validation constraints and improve documentation.
Add appropriate validation constraints and documentation for the fields.
Apply this diff:
@JsonProperty("campaignName") + @NotBlank(message = "Campaign name is required") + @Size(max = 256) private String campaignName; @JsonProperty("reportTitle") @Valid + @NotBlank(message = "Report title is required") + @Size(max = 256) private String reportTitle; @JsonProperty("numberOfIndividuals") + @Min(value = 0, message = "Number of individuals must be non-negative") private Integer numberOfIndividuals; @JsonProperty("createdBy") + @NotBlank(message = "Created by is required") + @Size(max = 64) private String createdBy; @JsonProperty("createdTime") + @NotNull(message = "Created time is required") private Long createdTime;📝 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.@JsonProperty("campaignName") @NotBlank(message = "Campaign name is required") @Size(max = 256) private String campaignName; @JsonProperty("reportTitle") @Valid @NotBlank(message = "Report title is required") @Size(max = 256) private String reportTitle; @JsonProperty("numberOfIndividuals") @Min(value = 0, message = "Number of individuals must be non-negative") private Integer numberOfIndividuals; @JsonProperty("createdBy") @NotBlank(message = "Created by is required") @Size(max = 64) private String createdBy; @JsonProperty("createdTime") @NotNull(message = "Created time is required") private Long createdTime;
33-37: 🧹 Nitpick (assertive)
Move default value to builder for better immutability.
Consider moving the default value for
totalAmountto the builder to maintain immutability.Apply this diff:
@JsonProperty("totalAmount") @Valid - @Default - private BigDecimal totalAmount = BigDecimal.ZERO; + private BigDecimal totalAmount; + + public static class ReportBillBuilder { + private BigDecimal totalAmount = BigDecimal.ZERO; + }Committable suggestion skipped: line range outside the PR's diff.
backend/expense-calculator/src/main/java/org/egov/digit/expense/calculator/util/BillReportConstraints.java (2)
3-3: 🧹 Nitpick (assertive)
Make class final and prevent instantiation.
The utility class should be final and have a private constructor to prevent inheritance and instantiation.
Apply this diff:
-public class BillReportConstraints { +public final class BillReportConstraints { + private BillReportConstraints() { + throw new IllegalStateException("Utility class"); + }📝 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.public final class BillReportConstraints { private BillReportConstraints() { throw new IllegalStateException("Utility class"); }
19-29: 🧹 Nitpick (assertive)
Encapsulate Excel column configuration.
Consider encapsulating the Excel column configuration in a dedicated inner class for better organization.
Apply this diff:
- public static final String[] BILL_EXCEL_COLUMNS = { - "PDF_STATIC_LABEL_BILL_TABLE_SERIAL_NUMBER", "PDF_STATIC_LABEL_BILL_TABLE_INDIVIDUAL_NAME", "PDF_STATIC_LABEL_BILL_TABLE_ROLE", - "PDF_STATIC_LABEL_BILL_TABLE_BOUNDARY_NAME", "PDF_STATIC_LABEL_BILL_TABLE_ID_NUMBER", "PDF_STATIC_LABEL_BILL_TABLE_MOBILE_NUMBER", - "PDF_STATIC_LABEL_BILL_TABLE_WAGE", "PDF_STATIC_LABEL_BILL_TABLE_FOOD", "PDF_STATIC_LABEL_BILL_TABLE_TRANSPORTATION", - "PDF_STATIC_LABEL_BILL_TABLE_TOTAL_WAGE", "PDF_STATIC_LABEL_BILL_TABLE_NUMBER_OF_DAYS", "PDF_STATIC_LABEL_BILL_TABLE_TOTAL_AMOUNT_TO_PAY" - }; - public static int[] BILL_EXCEL_COLUMN_WIDTH = { - 6, 24, 24, 24, - 30, 15, 10, 10, - 12, 15, 15, 15 - }; + public static final class ExcelConfig { + private ExcelConfig() {} + + public static final String[] COLUMNS = { + "PDF_STATIC_LABEL_BILL_TABLE_SERIAL_NUMBER", "PDF_STATIC_LABEL_BILL_TABLE_INDIVIDUAL_NAME", "PDF_STATIC_LABEL_BILL_TABLE_ROLE", + "PDF_STATIC_LABEL_BILL_TABLE_BOUNDARY_NAME", "PDF_STATIC_LABEL_BILL_TABLE_ID_NUMBER", "PDF_STATIC_LABEL_BILL_TABLE_MOBILE_NUMBER", + "PDF_STATIC_LABEL_BILL_TABLE_WAGE", "PDF_STATIC_LABEL_BILL_TABLE_FOOD", "PDF_STATIC_LABEL_BILL_TABLE_TRANSPORTATION", + "PDF_STATIC_LABEL_BILL_TABLE_TOTAL_WAGE", "PDF_STATIC_LABEL_BILL_TABLE_NUMBER_OF_DAYS", "PDF_STATIC_LABEL_BILL_TABLE_TOTAL_AMOUNT_TO_PAY" + }; + + public static final int[] COLUMN_WIDTHS = { + 6, 24, 24, 24, + 30, 15, 10, 10, + 12, 15, 15, 15 + }; + + static { + if (COLUMNS.length != COLUMN_WIDTHS.length) { + throw new IllegalStateException("Column definitions and widths must have the same length"); + } + } + }📝 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.public static final class ExcelConfig { private ExcelConfig() {} public static final String[] COLUMNS = { "PDF_STATIC_LABEL_BILL_TABLE_SERIAL_NUMBER", "PDF_STATIC_LABEL_BILL_TABLE_INDIVIDUAL_NAME", "PDF_STATIC_LABEL_BILL_TABLE_ROLE", "PDF_STATIC_LABEL_BILL_TABLE_BOUNDARY_NAME", "PDF_STATIC_LABEL_BILL_TABLE_ID_NUMBER", "PDF_STATIC_LABEL_BILL_TABLE_MOBILE_NUMBER", "PDF_STATIC_LABEL_BILL_TABLE_WAGE", "PDF_STATIC_LABEL_BILL_TABLE_FOOD", "PDF_STATIC_LABEL_BILL_TABLE_TRANSPORTATION", "PDF_STATIC_LABEL_BILL_TABLE_TOTAL_WAGE", "PDF_STATIC_LABEL_BILL_TABLE_NUMBER_OF_DAYS", "PDF_STATIC_LABEL_BILL_TABLE_TOTAL_AMOUNT_TO_PAY" }; public static final int[] COLUMN_WIDTHS = { 6, 24, 24, 24, 30, 15, 10, 10, 12, 15, 15, 15 }; static { if (COLUMNS.length != COLUMN_WIDTHS.length) { throw new IllegalStateException("Column definitions and widths must have the same length"); } } }backend/expense-calculator/pom.xml (1)
122-123: 🧹 Nitpick (assertive)
Remove explicit version for spring-boot-starter-data-redis.
The version for spring-boot-starter-data-redis should be managed by the spring-boot-starter-parent.
Apply this diff:
<dependency> <groupId>org.springframework.boot</groupId> <artifactId>spring-boot-starter-data-redis</artifactId> - <version>3.2.2</version> </dependency>Committable suggestion skipped: line range outside the PR's diff.
|
Summary by CodeRabbit
Based on the comprehensive summary of changes, here are the release notes:
Release Notes: Expense Calculator Service
New Features
Improvements
Technical Updates
Database Changes
eg_expense_bill_gen_statusfor tracking bill generation statusConfiguration Updates
Dependency Updates
health-services-modelsto version 1.0.24-SNAPSHOTworks-services-commonto version 1.0.2-hcm-SNAPSHOTjson-smart,poi-ooxml,spring-boot-starter-data-redis