HCMPRE-1922 - Microplanning v0.2 master merge #1340
Conversation
HCMPRE-1661: Adding additional field table in Plan
…health-campaign-services into download-estimation-1578
Enriching additionalField ID after empty check
HCMPRE:1678 - Plan-facility mapping residing boundary validation update
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🔭 Outside diff range comments (1)
health-services/plan-service/src/main/java/digit/service/validator/PlanFacilityValidator.java (1)
Line range hint
158-171: Add parameter validation.Consider adding null checks for parameters to prevent NullPointerException:
private Set<String> fetchBoundaryCodes(CampaignDetail campaignDetail, String hierarchyType) { + if (campaignDetail == null || hierarchyType == null) { + throw new CustomException("INVALID_INPUT", "Campaign detail and hierarchy type cannot be null"); + } + Set<String> boundaryCodes = campaignDetail.getBoundaries().stream() .filter(boundary -> hierarchyType.equals(boundary.getType().toLowerCase())) .map(Boundary::getCode) .collect(Collectors.toSet()); return boundaryCodes; }
♻️ Duplicate comments (4)
health-services/resource-generator/src/main/java/org/egov/processor/web/models/planFacility/PlanFacilitySearchCriteria.java (2)
18-26: 🧹 Nitpick (assertive)Enhance class-level documentation.
While the class has basic documentation, consider expanding it to be more descriptive about its purpose and usage.
/** - * PlanFacilitySearchCriteria + * Search criteria class for filtering plan facilities. + * + * This class encapsulates various search parameters used to filter plan facilities based on: + * - Plan configuration details (ID, name) + * - Facility attributes (name, status, type, ID) + * - Geographic boundaries (residing boundaries, jurisdiction) + * - Pagination parameters + * + * @see PlanFacilitySearchRequest */
64-66: 🧹 Nitpick (assertive)Add upper bound for limit parameter.
While there's a lower bound of 1, consider adding an upper bound to prevent excessive resource usage.
@JsonProperty("limit") @Min(1) + @Max(100) private Integer limit = null;health-services/resource-generator/src/main/java/org/egov/processor/util/ExcelStylingUtil.java (2)
60-78: 🧹 Nitpick (assertive)Improve column width calculation accuracy.
The current width calculation uses a simple approximation that might not work well for all fonts and content types.
88-112: 🛠️ Refactor suggestionEnhance error handling and documentation for color conversion.
The color conversion logic needs better documentation and error handling.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
health-services/plan-service/src/main/java/digit/service/PlanEnricher.java(5 hunks)health-services/plan-service/src/main/java/digit/service/validator/PlanFacilityValidator.java(1 hunks)health-services/resource-generator/src/main/java/org/egov/processor/config/ServiceConstants.java(5 hunks)health-services/resource-generator/src/main/java/org/egov/processor/service/ExcelParser.java(16 hunks)health-services/resource-generator/src/main/java/org/egov/processor/util/CalculationUtil.java(1 hunks)health-services/resource-generator/src/main/java/org/egov/processor/util/EnrichmentUtil.java(6 hunks)health-services/resource-generator/src/main/java/org/egov/processor/util/ExcelStylingUtil.java(1 hunks)health-services/resource-generator/src/main/java/org/egov/processor/util/MdmsUtil.java(7 hunks)health-services/resource-generator/src/main/java/org/egov/processor/util/MixedStrategyUtil.java(1 hunks)health-services/resource-generator/src/main/java/org/egov/processor/util/OutputEstimationGenerationUtil.java(1 hunks)health-services/resource-generator/src/main/java/org/egov/processor/util/PlanFacilityUtil.java(1 hunks)health-services/resource-generator/src/main/java/org/egov/processor/util/PlanUtil.java(5 hunks)health-services/resource-generator/src/main/java/org/egov/processor/web/models/mdmsV2/MixedStrategyOperationLogic.java(1 hunks)health-services/resource-generator/src/main/java/org/egov/processor/web/models/planFacility/PlanFacility.java(1 hunks)health-services/resource-generator/src/main/java/org/egov/processor/web/models/planFacility/PlanFacilityResponse.java(1 hunks)health-services/resource-generator/src/main/java/org/egov/processor/web/models/planFacility/PlanFacilitySearchCriteria.java(1 hunks)
🧰 Additional context used
📓 Learnings (11)
health-services/resource-generator/src/main/java/org/egov/processor/web/models/planFacility/PlanFacility.java (3)
Learnt from: Saloni-eGov
PR: egovernments/health-campaign-services#906
File: health-services/plan-service/src/main/java/digit/web/models/PlanFacility.java:61-67
Timestamp: 2024-11-12T10:40:11.591Z
Learning: In the `addServiceBoundariesItem` method in `PlanFacility.java`, it's acceptable to initialize `serviceBoundaries` if it's null without adding a null check for `serviceBoundariesItem`.
Learnt from: Taniya-eGov
PR: egovernments/health-campaign-services#914
File: health-services/plan-service/src/main/java/digit/web/models/PlanFacility.java:1-69
Timestamp: 2024-11-12T10:40:11.591Z
Learning: In the `PlanFacility` model, the `active` field is set to `true` during creation, so it does not need to be initialized to `true` in the class definition.
Learnt from: Taniya-eGov
PR: egovernments/health-campaign-services#914
File: health-services/plan-service/src/main/java/digit/web/models/PlanFacilityResponse.java:37-43
Timestamp: 2024-11-10T20:01:49.000Z
Learning: In the `PlanFacilityResponse` class, the `addPlanFacilityItem` method is intended to be called only with valid `PlanFacility` objects, so adding null checks for `planFacilityItem` is unnecessary.
health-services/resource-generator/src/main/java/org/egov/processor/util/CalculationUtil.java (1)
Learnt from: devdatta-egov
PR: egovernments/health-campaign-services#784
File: health-services/resource-estimation-service/src/main/java/org/egov/processor/util/CalculationUtil.java:74-87
Timestamp: 2024-11-10T20:01:49.000Z
Learning: The `calculateResources` method in `CalculationUtil.java` is intentionally complex to meet specific design requirements or constraints.
health-services/resource-generator/src/main/java/org/egov/processor/web/models/planFacility/PlanFacilitySearchCriteria.java (1)
Learnt from: tanishi-egov
PR: egovernments/health-campaign-services#1340
File: health-services/resource-generator/src/main/java/org/egov/processor/web/models/planFacility/PlanFacilitySearchRequest.java:29-32
Timestamp: 2025-01-17T09:03:48.249Z
Learning: In the health-campaign-services codebase, `PlanFacilitySearchCriteria` and `facilitySearchCriteria` are distinct search criteria used by different services. `PlanFacilitySearchCriteria` is specifically used for searching facilities within the context of a plan.
health-services/resource-generator/src/main/java/org/egov/processor/util/PlanFacilityUtil.java (2)
Learnt from: tanishi-egov
PR: egovernments/health-campaign-services#1340
File: health-services/resource-generator/src/main/java/org/egov/processor/web/models/planFacility/PlanFacilitySearchRequest.java:29-32
Timestamp: 2025-01-17T09:03:48.249Z
Learning: In the health-campaign-services codebase, `PlanFacilitySearchCriteria` and `facilitySearchCriteria` are distinct search criteria used by different services. `PlanFacilitySearchCriteria` is specifically used for searching facilities within the context of a plan.
Learnt from: tanishi-egov
PR: egovernments/health-campaign-services#1340
File: health-services/resource-generator/src/main/java/org/egov/processor/web/models/planFacility/PlanFacilitySearchRequest.java:24-27
Timestamp: 2025-01-17T06:16:55.255Z
Learning: The `RequestInfo` property in request DTOs should use PascalCase ("RequestInfo") to maintain consistency across all modules in the codebase, as this convention is used throughout the system and changing it would require UI changes.
health-services/resource-generator/src/main/java/org/egov/processor/util/MixedStrategyUtil.java (2)
Learnt from: tanishi-egov
PR: egovernments/health-campaign-services#1340
File: health-services/resource-generator/src/main/java/org/egov/processor/web/models/mdmsV2/MixedStrategyOperationLogic.java:19-29
Timestamp: 2025-01-22T10:07:11.992Z
Learning: The property names in MixedStrategyOperationLogic model should match the MDMS master naming convention, even if they use mixed casing (e.g., isFixedPost in camelCase, RegistrationProcess/DistributionProcess/CategoriesNotAllowed in PascalCase).
Learnt from: tanishi-egov
PR: egovernments/health-campaign-services#1340
File: health-services/resource-generator/src/main/java/org/egov/processor/web/models/mdmsV2/MixedStrategyOperationLogic.java:19-29
Timestamp: 2025-01-22T10:07:11.992Z
Learning: The MDMS master data models in the health campaign services should maintain consistent property naming with the MDMS master definitions:
1. Boolean flags may use camelCase (e.g., "isFixedPost")
2. Other properties should use PascalCase (e.g., "RegistrationProcess", "DistributionProcess", "CategoriesNotAllowed")
health-services/resource-generator/src/main/java/org/egov/processor/web/models/planFacility/PlanFacilityResponse.java (2)
Learnt from: Taniya-eGov
PR: egovernments/health-campaign-services#914
File: health-services/plan-service/src/main/java/digit/web/models/PlanFacilityResponse.java:37-43
Timestamp: 2024-11-10T20:01:49.000Z
Learning: In the `PlanFacilityResponse` class, the `addPlanFacilityItem` method is intended to be called only with valid `PlanFacility` objects, so adding null checks for `planFacilityItem` is unnecessary.
Learnt from: Saloni-eGov
PR: egovernments/health-campaign-services#906
File: health-services/plan-service/src/main/java/digit/web/models/PlanFacilityResponse.java:25-30
Timestamp: 2024-11-12T10:40:11.591Z
Learning: In this codebase, PascalCase is preferred for JSON property names in `@JsonProperty` annotations.
health-services/resource-generator/src/main/java/org/egov/processor/web/models/mdmsV2/MixedStrategyOperationLogic.java (2)
Learnt from: tanishi-egov
PR: egovernments/health-campaign-services#1340
File: health-services/resource-generator/src/main/java/org/egov/processor/web/models/mdmsV2/MixedStrategyOperationLogic.java:19-29
Timestamp: 2025-01-22T10:07:11.992Z
Learning: The property names in MixedStrategyOperationLogic model should match the MDMS master naming convention, even if they use mixed casing (e.g., isFixedPost in camelCase, RegistrationProcess/DistributionProcess/CategoriesNotAllowed in PascalCase).
Learnt from: tanishi-egov
PR: egovernments/health-campaign-services#1340
File: health-services/resource-generator/src/main/java/org/egov/processor/web/models/mdmsV2/MixedStrategyOperationLogic.java:19-29
Timestamp: 2025-01-22T10:07:11.992Z
Learning: The MDMS master data models in the health campaign services should maintain consistent property naming with the MDMS master definitions:
1. Boolean flags may use camelCase (e.g., "isFixedPost")
2. Other properties should use PascalCase (e.g., "RegistrationProcess", "DistributionProcess", "CategoriesNotAllowed")
health-services/plan-service/src/main/java/digit/service/PlanEnricher.java (1)
Learnt from: tanishi-egov
PR: egovernments/health-campaign-services#1340
File: health-services/plan-service/src/main/java/digit/service/enrichment/PlanFacilityEnricher.java:6-6
Timestamp: 2025-01-17T08:48:36.911Z
Learning: In Java files, wildcard imports (import package.*) are preferred when there are more than 5 imports from the same package to keep the import section concise.
health-services/resource-generator/src/main/java/org/egov/processor/util/EnrichmentUtil.java (1)
Learnt from: tanishi-egov
PR: egovernments/health-campaign-services#1340
File: health-services/resource-generator/src/main/java/org/egov/processor/util/EnrichmentUtil.java:147-148
Timestamp: 2025-01-22T09:58:20.727Z
Learning: In the health-campaign-services codebase, null checks for maps containing additional details are preferred at the point of consumption rather than at the point of population.
health-services/resource-generator/src/main/java/org/egov/processor/util/OutputEstimationGenerationUtil.java (7)
Learnt from: Priyanka-eGov
PR: egovernments/health-campaign-services#1340
File: health-services/resource-generator/src/main/java/org/egov/processor/util/OutputEstimationGenerationUtil.java:34-39
Timestamp: 2025-01-16T08:14:51.877Z
Learning: In Spring applications, `@Autowired` annotation is not required on constructors as Spring's constructor injection works without it when there's only one constructor.
Learnt from: Priyanka-eGov
PR: egovernments/health-campaign-services#1340
File: health-services/resource-generator/src/main/java/org/egov/processor/util/OutputEstimationGenerationUtil.java:103-105
Timestamp: 2025-01-16T08:16:47.695Z
Learning: When iterating through Excel sheet columns from right to left (last to first) to localize output columns that are appended at the end, breaking the loop upon encountering the first non-localized column is the correct approach as all columns to its left will also be non-localized.
Learnt from: Priyanka-eGov
PR: egovernments/health-campaign-services#1340
File: health-services/resource-generator/src/main/java/org/egov/processor/util/OutputEstimationGenerationUtil.java:50-70
Timestamp: 2025-01-22T11:10:43.217Z
Learning: In the health-campaign-services repository, workbook validation in Excel processing is handled at the entry point in ExcelParser.java through try-with-resources and exception handling (FileNotFoundException, InvalidFormatException, IOException).
Learnt from: Priyanka-eGov
PR: egovernments/health-campaign-services#1340
File: health-services/resource-generator/src/main/java/org/egov/processor/util/OutputEstimationGenerationUtil.java:206-211
Timestamp: 2025-01-16T08:40:43.544Z
Learning: In the health-campaign-services repository, boundary code cell validation is handled in ExcelParser.java through the validateRows() method, which validates cells through validateTillBoundaryCode() and validateAttributes() methods.
Learnt from: Priyanka-eGov
PR: egovernments/health-campaign-services#1340
File: health-services/resource-generator/src/main/java/org/egov/processor/util/OutputEstimationGenerationUtil.java:244-245
Timestamp: 2025-01-16T08:00:17.932Z
Learning: In Java files, maintain a single empty line between methods for better code organization and readability.
Learnt from: Priyanka-eGov
PR: egovernments/health-campaign-services#1340
File: health-services/resource-generator/src/main/java/org/egov/processor/util/OutputEstimationGenerationUtil.java:206-211
Timestamp: 2025-01-16T08:40:43.544Z
Learning: In the health-campaign-services repository's ExcelParser.java, boundary code cell validation is comprehensively handled through validateRows() which includes:
1. validateTillBoundaryCode() - checks for null cells, BLANK cell type, and validates string values against regex patterns
2. validateAttributes() - validates cell types, boundary code existence in boundaryCodeList, and performs string value validation
Learnt from: Priyanka-eGov
PR: egovernments/health-campaign-services#1340
File: health-services/resource-generator/src/main/java/org/egov/processor/util/OutputEstimationGenerationUtil.java:244-245
Timestamp: 2025-01-16T08:00:17.932Z
Learning: In Java files, maintain consistent spacing with:
1. Single empty line between methods
2. Single empty line before the class closing brace
health-services/resource-generator/src/main/java/org/egov/processor/config/ServiceConstants.java (2)
Learnt from: tanishi-egov
PR: egovernments/health-campaign-services#1340
File: health-services/resource-generator/src/main/java/org/egov/processor/config/ServiceConstants.java:36-37
Timestamp: 2025-01-17T06:12:01.218Z
Learning: In ServiceConstants.java, error message constants should be kept together with their corresponding error code constants for better readability and maintainability, rather than grouping all codes separately from all messages.
Learnt from: tanishi-egov
PR: egovernments/health-campaign-services#1340
File: health-services/resource-generator/src/main/java/org/egov/processor/config/ServiceConstants.java:72-72
Timestamp: 2025-01-22T11:10:01.634Z
Learning: In the health-campaign-services Java codebase, error messages used with IllegalArgumentException don't require corresponding error code constants, unlike CustomException which requires both code and message constants.
🔇 Additional comments (29)
health-services/plan-service/src/main/java/digit/service/validator/PlanFacilityValidator.java (2)
129-130: LGTM! Clear separation of boundary codes.The code effectively separates boundary codes into two distinct sets, making the validation logic clearer and more maintainable.
132-138: Documentation addresses previous review feedback.The comments now clearly explain the business logic behind the different validation approaches for residing and service boundaries. This improves code maintainability.
health-services/resource-generator/src/main/java/org/egov/processor/web/models/planFacility/PlanFacilityResponse.java (2)
1-12: LGTM! Clean package structure and imports.The package declaration follows Java naming conventions, and imports are well-organized with no redundancy.
23-32: LGTM! Field declarations follow established conventions.The field declarations are well-structured with:
- Appropriate use of @JsonProperty with PascalCase naming (as per codebase convention)
- Proper validation annotations
- Clean field declarations without redundant null initialization
health-services/resource-generator/src/main/java/org/egov/processor/web/models/planFacility/PlanFacilitySearchCriteria.java (1)
1-16: LGTM! Package and imports are well-organized.The package name follows Java conventions, and all necessary imports are present without any unused ones.
health-services/resource-generator/src/main/java/org/egov/processor/util/CalculationUtil.java (1)
81-81: Verify the safety of passing null instead of mappedValues.The change from
mappedValuestonullin theplanUtil.createcall could lead to potential issues:
- Risk of NullPointerException if
planUtil.createuses this parameter- Loss of mapping information that might be needed for plan creation
Let's verify how
mappedValuesis used inplanUtil.create:✅ Verification successful
Passing null instead of mappedValues is safe in this context.
The change is safe because:
- mappedValues in PlanUtil.create is only used for boundary code extraction, which is already processed at this point
- When boundaryCodeToCensusAdditionalDetails is null (as in this case), mappedValues is not required
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how mappedValues is used in PlanUtil.create # Search for the create method implementation echo "Searching for PlanUtil.create implementation..." ast-grep --pattern 'class PlanUtil { $$$ create($$$) { $$$ } $$$ }' # Search for mappedValues usage within create method echo "Searching for mappedValues usage..." rg -A 5 'mappedValues' --type javaLength of output: 55132
health-services/resource-generator/src/main/java/org/egov/processor/web/models/planFacility/PlanFacility.java (2)
1-18: LGTM! Package structure and imports are well-organized.The code follows proper package naming conventions and uses appropriate imports.
82-88: LGTM! Helper method implementation is correct.The
addServiceBoundariesItemmethod follows the builder pattern and properly handles list initialization.health-services/resource-generator/src/main/java/org/egov/processor/util/ExcelStylingUtil.java (3)
1-12: LGTM! Package structure and imports are well organized.The class is properly annotated with
@Componentfor Spring dependency injection.
1-113: 🧹 Nitpick (assertive)Add unit tests and constant validation.
Add unit tests to verify:
- Cell styling behavior
- Column width calculations
- Color conversion logic
- Error handling scenarios
Consider adding validation for constants at startup:
- BRIGHTEN_FACTOR should be positive
- HEX_BACKGROUND_COLOR should be valid
- COLUMN_PADDING should be within reasonable bounds
Would you like me to help create:
- A test class with comprehensive test cases?
- A configuration validator for the constants?
14-53: 🛠️ Refactor suggestionAdd workbook type validation before casting.
While error handling has been improved, there's still a risk of ClassCastException at line 20.
Add type checking before casting:
Sheet sheet = cell.getSheet(); Workbook workbook = sheet.getWorkbook(); + if (!(workbook instanceof XSSFWorkbook)) { + throw new IllegalArgumentException("This method only supports XSSF workbooks"); + } XSSFWorkbook xssfWorkbook = (XSSFWorkbook) workbook;Likely invalid or redundant comment.
health-services/resource-generator/src/main/java/org/egov/processor/util/MdmsUtil.java (5)
3-3: LGTM! Well-structured import and field declarations.The additions of necessary imports and the private ParsingUtil field are clean and follow good practices.
Also applies to: 27-28, 39-40
41-46: LGTM! Clean constructor modification.The constructor update properly initializes the new ParsingUtil dependency while maintaining clean code principles.
75-75: LGTM! Error message accurately reflects the master schema.The error message has been updated to correctly reference the MDMS_MASTER_ADMIN_SCHEMA.
88-90: LGTM! Clean transition to admin console module.The method has been properly updated to use the admin console module details instead of the plan module.
109-121: LGTM! Well-structured and documented new method.The getAdminConsoleModuleDetail method is properly implemented with clear documentation and follows the established pattern.
health-services/resource-generator/src/main/java/org/egov/processor/util/PlanUtil.java (1)
143-143: Use a meaningful initial object inupdateFieldInAdditionalDetails.Using
new Object()may not be meaningful when updating additional details. Consider providing a more descriptive initial object, such asnew HashMap<String, Object>(), to enhance clarity and type safety.Apply this diff to improve:
return parsingUtil.updateFieldInAdditionalDetails( - new Object(), + new HashMap<String, Object>(), fieldsToBeUpdated );health-services/resource-generator/src/main/java/org/egov/processor/util/MixedStrategyUtil.java (2)
51-53: 🧹 Nitpick (assertive)Ensure Correct Mapping of Property Names in MDMS Data
Ensure that the property names in
MixedStrategyOperationLogicalign with the naming conventions used in the MDMS master data. This alignment is crucial for accurate data mapping and can handle differences in casing (e.g., camelCase vs. PascalCase). Verify that themapper.convertValuemethod correctly maps all fields, and consider custom mapping strategies if necessary.Would you like assistance in verifying or adjusting the property mappings to ensure consistency with the MDMS master definitions?
77-83: 🛠️ Refactor suggestionVerify Handling of Multiple Matching Logic Entries
The use of
findAny()ingetCategoriesNotAllowedassumes that only one matchingMixedStrategyOperationLogicentry exists for the given criteria. If multiple entries match, an arbitrary one will be returned, which may lead to inconsistent behavior. Ensure that the MDMS data is structured to prevent multiple matches or modify the code to handle multiple matching entries appropriately.health-services/resource-generator/src/main/java/org/egov/processor/util/OutputEstimationGenerationUtil.java (1)
1-263: Well-Structured Implementation Enhancing Excel ProcessingThe
OutputEstimationGenerationUtilclass is well-designed, with modular methods that enhance readability and maintainability. The processing of output files, localization of headers, and addition of facility information are effectively handled. The use of utility classes and dependency injection aligns with best practices.health-services/resource-generator/src/main/java/org/egov/processor/util/PlanFacilityUtil.java (2)
32-44:⚠️ Potential issueImprove error handling in search method.
The current implementation swallows exceptions and returns null, which could lead to NPEs downstream. Consider throwing a custom exception or handling the error more gracefully.
public PlanFacilityResponse search(PlanFacilitySearchRequest planfacilitySearchRequest) { - PlanFacilityResponse planFacilityResponse = null; try { Object response = serviceRequestRepository.fetchResult(getPlanFacilitySearchUri(), planfacilitySearchRequest); + if (response == null) { + throw new CustomException("EMPTY_RESPONSE", "Received null response from service"); + } planFacilityResponse = mapper.convertValue(response, PlanFacilityResponse.class); + if (planFacilityResponse == null) { + throw new CustomException("PARSING_ERROR", "Failed to parse service response"); + } + return planFacilityResponse; } catch (Exception e) { log.error(ServiceConstants.ERROR_WHILE_SEARCHING_PLAN_FACILITY, e); + throw new CustomException("SEARCH_ERROR", "Error while searching plan facility: " + e.getMessage()); } - - return planFacilityResponse; }⛔ Skipped due to learnings
Learnt from: tanishi-egov PR: egovernments/health-campaign-services#1340 File: health-services/resource-generator/src/main/java/org/egov/processor/web/models/planFacility/PlanFacilitySearchRequest.java:29-32 Timestamp: 2025-01-17T09:03:48.249Z Learning: In the health-campaign-services codebase, `PlanFacilitySearchCriteria` and `facilitySearchCriteria` are distinct search criteria used by different services. `PlanFacilitySearchCriteria` is specifically used for searching facilities within the context of a plan.
14-24: 🧹 Nitpick (assertive)Make injected fields final.
Since these fields are initialized through constructor injection and never changed, they should be marked as final.
- private Configuration config; + private final Configuration config; - private ServiceRequestRepository serviceRequestRepository; + private final ServiceRequestRepository serviceRequestRepository; - private ObjectMapper mapper; + private final ObjectMapper mapper;Likely invalid or redundant comment.
health-services/resource-generator/src/main/java/org/egov/processor/config/ServiceConstants.java (1)
168-172: 🧹 Nitpick (assertive)Add validation for Excel styling constants.
The Excel styling constants need documentation and validation:
- Add comments explaining the expected format and valid ranges
- Consider using an enum for background colors
- Consider making COLUMN_WIDTH configurable based on screen resolution
- public static final String HEX_BACKGROUND_COLOR = "93C47D"; // Background color in HEX format (RRGGBB) for Excel header rows - public static final boolean FREEZE_CELL = true; // Controls whether cells should be locked for editing - public static final int COLUMN_WIDTH = 40; //Default column width in characters (1-255) - public static final int COLUMN_PADDING = 512; + /** Background color in HEX format (RRGGBB) for Excel header rows */ + public static final String HEX_BACKGROUND_COLOR = "93C47D"; + + /** Controls whether cells should be locked for editing */ + public static final boolean FREEZE_CELL = true; + + /** Default column width in characters (1-255) */ + public static final int COLUMN_WIDTH = 40; + + /** Padding in 1/256th of a character width */ + public static final int COLUMN_PADDING = 512;Likely invalid or redundant comment.
health-services/plan-service/src/main/java/digit/service/PlanEnricher.java (1)
252-285: LGTM! Well-structured search request enrichment.The implementation is clean and follows best practices:
- Clear documentation
- Proper null checks
- Consistent use of constants
- Logical organization of filters
health-services/resource-generator/src/main/java/org/egov/processor/util/EnrichmentUtil.java (5)
21-21: LGTM!The added imports are necessary for the new null checks using Spring's utility classes.
Also applies to: 26-26
145-146: LGTM!The implementation follows the codebase convention of handling null checks at the point of consumption rather than population.
224-227: LGTM!The comment update accurately reflects the change from census to plan records.
248-254: LGTM!The null check for resources has been implemented as previously suggested and accepted.
271-281: Verify NOT_APPLICABLE constant value.The code uses a NOT_APPLICABLE constant for null estimated values. Please ensure this constant is properly defined in ServiceConstants and its value matches the business requirements.
✅ Verification successful
NOT_APPLICABLE constant is properly defined and appropriate
The constant is correctly defined in ServiceConstants.java as
public static final String NOT_APPLICABLE = "N/A", which is an appropriate value for indicating null estimated values in Excel cells.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for NOT_APPLICABLE constant definition rg -A 1 "NOT_APPLICABLE\s*=\s*" .Length of output: 343
health-services/plan-service/src/main/java/digit/service/validator/PlanFacilityValidator.java
Show resolved
Hide resolved
...generator/src/main/java/org/egov/processor/web/models/planFacility/PlanFacilityResponse.java
Show resolved
Hide resolved
...tor/src/main/java/org/egov/processor/web/models/planFacility/PlanFacilitySearchCriteria.java
Show resolved
Hide resolved
...tor/src/main/java/org/egov/processor/web/models/planFacility/PlanFacilitySearchCriteria.java
Show resolved
Hide resolved
...tor/src/main/java/org/egov/processor/web/models/planFacility/PlanFacilitySearchCriteria.java
Show resolved
Hide resolved
health-services/resource-generator/src/main/java/org/egov/processor/util/PlanUtil.java
Show resolved
Hide resolved
health-services/resource-generator/src/main/java/org/egov/processor/service/ExcelParser.java
Show resolved
Hide resolved
health-services/resource-generator/src/main/java/org/egov/processor/util/PlanFacilityUtil.java
Show resolved
Hide resolved
...enerator/src/main/java/org/egov/processor/web/models/mdmsV2/MixedStrategyOperationLogic.java
Show resolved
Hide resolved
health-services/resource-generator/src/main/java/org/egov/processor/util/EnrichmentUtil.java
Show resolved
Hide resolved
HCMPRE-2203: Changing facility_name constraint from VARCHAR(64) to VARCHAR(2000)
…down search, changes filterMap signature.
…down search, changes filterMap signature.
…down search, changes filterMap signature.
…down search, changes filterMap signature.
…down search, changes filterMap signature.
…down search, changes filterMap signature.
HCMPRE-2212 making facilityIds in plan search a set for multi select dropdown
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
health-services/plan-service/src/main/java/digit/config/ServiceConstants.java(2 hunks)health-services/plan-service/src/main/java/digit/repository/querybuilder/PlanQueryBuilder.java(3 hunks)health-services/plan-service/src/main/java/digit/service/PlanEnricher.java(5 hunks)health-services/plan-service/src/main/java/digit/util/QueryUtil.java(3 hunks)health-services/plan-service/src/main/java/digit/web/models/PlanSearchCriteria.java(4 hunks)
🧰 Additional context used
📓 Learnings (4)
health-services/plan-service/src/main/java/digit/repository/querybuilder/PlanQueryBuilder.java (1)
Learnt from: Priyanka-eGov
PR: egovernments/health-campaign-services#1340
File: health-services/plan-service/src/main/java/digit/repository/querybuilder/PlanQueryBuilder.java:175-180
Timestamp: 2025-01-22T10:01:01.695Z
Learning: Database optimization suggestions like adding indexes should be tracked as separate issues rather than being included in feature PRs, as they require careful planning and consideration of performance impacts.
health-services/plan-service/src/main/java/digit/web/models/PlanSearchCriteria.java (3)
Learnt from: tanishi-egov
PR: egovernments/health-campaign-services#1340
File: health-services/plan-service/src/main/java/digit/web/models/PlanSearchCriteria.java:46-50
Timestamp: 2025-01-22T09:59:37.847Z
Learning: In the health-campaign-services project, fields that get their values from MDMS should be kept as strings rather than enums to maintain flexibility, as MDMS values can be modified without requiring code changes.
Learnt from: tanishi-egov
PR: egovernments/health-campaign-services#1340
File: health-services/plan-service/src/main/java/digit/web/models/PlanSearchCriteria.java:74-76
Timestamp: 2025-01-22T09:49:48.816Z
Learning: In the health-campaign-services project, the team prefers to document field usage in the implementation code rather than in the model classes.
Learnt from: Priyanka-eGov
PR: egovernments/health-campaign-services#978
File: health-services/plan-service/src/main/java/digit/web/models/PlanSearchCriteria.java:48-50
Timestamp: 2024-11-12T10:40:17.779Z
Learning: In the `PlanSearchCriteria` class, the `jurisdiction` field can be empty, and no validation constraints are required for this field.
health-services/plan-service/src/main/java/digit/service/PlanEnricher.java (1)
Learnt from: tanishi-egov
PR: egovernments/health-campaign-services#1340
File: health-services/plan-service/src/main/java/digit/service/enrichment/PlanFacilityEnricher.java:6-6
Timestamp: 2025-01-17T08:48:36.911Z
Learning: In Java files, wildcard imports (import package.*) are preferred when there are more than 5 imports from the same package to keep the import section concise.
health-services/plan-service/src/main/java/digit/config/ServiceConstants.java (1)
Learnt from: Saloni-eGov
PR: egovernments/health-campaign-services#906
File: health-services/plan-service/src/main/java/digit/config/ServiceConstants.java:189-193
Timestamp: 2024-11-10T20:01:49.000Z
Learning: When reviewing error messages in `ServiceConstants.java`, avoid suggesting changes to make them more descriptive unless specifically requested.
🔇 Additional comments (17)
health-services/plan-service/src/main/java/digit/web/models/PlanSearchCriteria.java (5)
3-3: LGTM: Import statements are correctly added.The new imports for
MapandSetare necessary for the added fields.Also applies to: 14-14
46-50: LGTM: Fields correctly implemented as strings for MDMS flexibility.The implementation aligns with the team's approach of using strings for MDMS-driven values.
74-75: LGTM: Filters map implementation is correct.The
@JsonIgnoreannotation is properly used, and the typeMap<String, Set<String>>allows for multiple values per filter key.
43-44: Consider adding validation constraints for facilityIds.While the implementation is correct, consider adding validation constraints if there are any requirements around the minimum/maximum number of facility IDs that can be searched at once.
Run this script to check if similar validation exists in related search criteria classes:
52-56: Verify security question field constraints.For security-related fields, consider:
- Are there any format requirements or allowed patterns?
- Should there be length restrictions?
- Is there a need for input sanitization?
Run this script to check how security questions are validated in other parts of the system:
health-services/plan-service/src/main/java/digit/service/PlanEnricher.java (4)
1-1: Package name change looks good.
There appear to be no issues with the package name change fromdigit.service.enrichmenttodigit.service.
3-3: Wildcard import usage check.
Currently, fordigit.web.models.boundary, you have fewer than 5 imports, which aligns with your style guide that recommends wildcard imports only when exceeding 5. This configuration is consistent, and no further change is necessary.
115-122: Acknowledgement for ID generation in updates.
It's good to see that, inenrichPlanUpdate, IDs for additional fields are generated only when missing. This prevents overwriting existing IDs.
245-285:⚠️ Potential issueGuard
planSearchCriteriaagainst null.
Before invokingplanSearchRequest.getPlanSearchCriteria(), consider adding a null check to avoid potential NullPointerExceptions in edge cases.public void enrichSearchRequest(PlanSearchRequest planSearchRequest) { + if (ObjectUtils.isEmpty(planSearchRequest.getPlanSearchCriteria())) { + throw new IllegalArgumentException("PlanSearchCriteria is required."); + } PlanSearchCriteria planSearchCriteria = planSearchRequest.getPlanSearchCriteria(); ... }health-services/plan-service/src/main/java/digit/repository/querybuilder/PlanQueryBuilder.java (2)
33-34: Added columns for plan_target and plan_additional_field.
Expanding the selected columns to includepaf.*allows the system to capture additional field data. This looks correct and beneficial for more comprehensive query results.
175-190: Multi-select facility filters.
This logic to handleFACILITY_ID_SEARCH_PARAMETER_KEYdifferently through partial JSON strings is a neat approach. Confirm that skipping the direct addition topreparedStmtListis truly intended; the subsequentpreparePartialJsonStringFromFilterMapcall might still add placeholders.health-services/plan-service/src/main/java/digit/util/QueryUtil.java (4)
13-13: Wide import usage.
Importing fromjava.util.*is acceptable here, given the variety of utilities used, but remain mindful of overshadowed classes if the code grows more extensive.
98-99: Detailed documentation.
These additional lines clarify how nested keys are processed, which is vital for maintainers to understand the structure of JSON queries.
122-139: Clear function documentation.
This method doc thoroughly explains how facility multi-select is handled, ensuring new contributors can follow the logic. Good clarity and thoroughness.
140-176: Robust query assembly.
The approach of constructing JSON parts or OR conditions forfacilityIdis well-handled. It's essential for correctness that placeholders are added topreparedStmtListto avoid injection vulnerabilities. The method is consistent with best practices.health-services/plan-service/src/main/java/digit/config/ServiceConstants.java (2)
387-390: Verify PostgreSQL JSONB query syntax.The JSONB query constants use PostgreSQL's containment operator (@>). Ensure the syntax is compatible with your PostgreSQL version.
✅ Verification successful
PostgreSQL JSONB query syntax is valid and compatible ✅
The JSONB query syntax using the containment operator (@>) with both CAST and :: type casting methods is standard PostgreSQL syntax, supported since JSONB was introduced in PostgreSQL 9.4. The codebase consistently uses these patterns without any version-specific features that could cause compatibility issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usage of these JSONB queries to verify syntax # Look for repository files using these constants # Search for JSONB query usage rg "additional_details @> CAST.*AS jsonb" rg "additional_details @>.*::jsonb"Length of output: 602
Script:
#!/bin/bash # Search for other JSONB patterns and tests rg "@>.*jsonb" -g "!*.{md,txt}" # Look for test files related to these queries rg -l "PlanFacilityQueryBuilder" -g "*Test*.java" # Look for PostgreSQL version in config files rg -i "postgresql.*version" -g "*.{properties,yml,xml}"Length of output: 662
369-376: Verify the JSON path structure for nested fields.The constants use pipe (|) notation for nested JSON paths. Ensure this matches your JSON structure in the database.
✅ Verification successful
JSON path structure is correctly implemented and handled
The pipe notation in the constants is a readable format that gets properly transformed into valid JSON paths during processing. The system:
- Converts pipe-separated paths to dot notation
- Handles nested JSON path resolution via QueryUtil
- Builds proper JSON structures for database queries
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the JSON structure matches the defined paths # Search for JSON examples in the codebase that match these paths # Search for terrain field usage rg -A 5 "terrain.*code" # Search for security details field usage rg -A 5 "securityDetails.*code"Length of output: 1787
Script:
#!/bin/bash # Search for JSON examples and path handling # Look for JSON files in resources and test directories fd -e json -x cat {} # Search for classes that might handle path resolution rg -l "PathResolver|JsonPath|ObjectMapper" # Search for test files using these fields rg -l "accessibilityDetails|terrain|securityDetails" --type-add 'test:*{Test,IT}.java' -t testLength of output: 103636
Script:
#!/bin/bash # Search for JSON schema files and path handling code # Look for JSON schema files fd -e json -x grep -l "accessibilityDetails\|securityDetails" {} \; # Search for path handling utilities and JSON mapping code rg -l "JsonPath|ObjectMapper|PathResolver" --type java # Search for test files that might validate these paths rg -l "accessibilityDetails|terrain|securityDetails" --type-add 'test:*Test.java' -t test # Search for actual usage of these constants rg "TERRAIN_CONDITION_SEARCH_PARAMETER_KEY|SECURITY_Q1_SEARCH_PARAMETER_KEY" -A 3Length of output: 22746
Script:
#!/bin/bash # Search for search implementation and filter processing code # Look for search/filter implementation rg -A 10 "filtersMap.put.*TERRAIN_CONDITION_SEARCH_PARAMETER_KEY" # Search for classes that might process these paths rg -l "processSearchPath|searchFilters|filterProcessor" --type java # Look for test cases using these filters rg -A 5 "accessibilityDetails.*terrain.*code" --type-add 'test:*Test.java' -t testLength of output: 1733
Script:
#!/bin/bash # Search for repository and query implementation # Look for repository classes in plan-service rg -l "class.*Repository" health-services/plan-service/src/main/java/digit/repository/ # Search for SQL queries or search implementations rg -A 10 "filtersMap" health-services/plan-service/src/main/java/digit/repository/ # Look for test data or example JSON rg -l "accessibilityDetails|terrain|code" health-services/plan-service/src/test/Length of output: 2835
Script:
#!/bin/bash # Search for QueryUtil implementation # Look for QueryUtil class rg -A 20 "class QueryUtil" health-services/plan-service/src/main/java/digit/util/ # Search for JSON path handling methods rg -A 10 "preparePartialJsonStringFromFilterMap" health-services/plan-service/src/main/java/digit/util/ # Look for any related test cases rg -l "QueryUtil.*Test" health-services/plan-service/src/test/Length of output: 5229
Script:
#!/bin/bash # Search for path transformation logic # Look for constants and path transformation code rg -A 5 "DOT_SEPARATOR|DOT_REGEX|PIPE_SEPARATOR" health-services/plan-service/src/main/java/digit/ # Search for any path preprocessing rg -A 10 "prepareNestedQueryMap" health-services/plan-service/src/main/java/digit/util/ # Look for any path-related utility methods rg -A 5 "convertPath|transformPath|processPath" health-services/plan-service/src/main/java/digit/Length of output: 15661
health-services/plan-service/src/main/java/digit/service/PlanEnricher.java
Show resolved
Hide resolved
health-services/plan-service/src/main/java/digit/service/PlanEnricher.java
Show resolved
Hide resolved
health-services/plan-service/src/main/java/digit/repository/querybuilder/PlanQueryBuilder.java
Show resolved
Hide resolved
health-services/plan-service/src/main/java/digit/repository/querybuilder/PlanQueryBuilder.java
Show resolved
Hide resolved
health-services/plan-service/src/main/java/digit/repository/querybuilder/PlanQueryBuilder.java
Show resolved
Hide resolved
health-services/plan-service/src/main/java/digit/config/ServiceConstants.java
Show resolved
Hide resolved
health-services/plan-service/src/main/java/digit/config/ServiceConstants.java
Show resolved
Hide resolved
health-services/resource-generator/src/main/java/org/egov/processor/util/CalculationUtil.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
health-services/resource-generator/src/main/java/org/egov/processor/util/CalculationUtil.java (1)
Line range hint
67-84: Update method signature to match implementation.The method signature still shows old parameters, but the implementation has changed. Update the signature to match the current implementation:
- public void calculateResources(JsonNode jsonNode, PlanConfigurationRequest planConfigurationRequest, Map<String, BigDecimal> resultMap, - Map<String, String> mappedValues, Map<String, BigDecimal> assumptionValueMap) { + public void calculateResources(JsonNode jsonNode, PlanConfigurationRequest planConfigurationRequest, Map<String, BigDecimal> resultMap, + Map<String, Object> boundaryCodeToCensusAdditionalDetails) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
health-services/resource-generator/src/main/java/org/egov/processor/util/CalculationUtil.java(2 hunks)
🧰 Additional context used
📓 Learnings (1)
health-services/resource-generator/src/main/java/org/egov/processor/util/CalculationUtil.java (1)
Learnt from: devdatta-egov
PR: egovernments/health-campaign-services#784
File: health-services/resource-estimation-service/src/main/java/org/egov/processor/util/CalculationUtil.java:74-87
Timestamp: 2024-11-10T20:01:49.000Z
Learning: The `calculateResources` method in `CalculationUtil.java` is intentionally complex to meet specific design requirements or constraints.
health-services/resource-generator/src/main/java/org/egov/processor/util/CalculationUtil.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
health-services/resource-generator/pom.xml (1)
Line range hint
82-86: Fix Spring Test version mismatch.Spring Test version 5.3.10 is incompatible with Spring Boot 3.2.2. Remove the explicit version to let Spring Boot manage it:
<dependency> <groupId>org.springframework</groupId> <artifactId>spring-test</artifactId> - <version>5.3.10</version> <!-- Use the appropriate version --> </dependency>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
health-services/plan-service/CHANGELOG.md(1 hunks)health-services/plan-service/pom.xml(1 hunks)health-services/resource-generator/CHANGELOG.md(1 hunks)health-services/resource-generator/pom.xml(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
health-services/resource-generator/CHANGELOG.md
3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
4-4: Lists should be surrounded by blank lines
null
(MD032, blanks-around-lists)
health-services/plan-service/CHANGELOG.md
4-4: Multiple consecutive blank lines
Expected: 1; Actual: 2
(MD012, no-multiple-blanks)
5-5: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
6-6: Lists should be surrounded by blank lines
null
(MD032, blanks-around-lists)
🔇 Additional comments (7)
health-services/resource-generator/CHANGELOG.md (1)
3-3: Verify the release date.The changelog entry is dated January 30, 2025, which is a future date. Please verify if this is intentional or needs to be updated to the actual release date.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below(MD022, blanks-around-headings)
health-services/plan-service/CHANGELOG.md (1)
5-8: Ensure feature list consistency across changelogs.The resource-generator changelog mentions "Microplan Estimation Downloadable Excel Report Enhancements" which is missing from this changelog. Consider adding this feature if it's relevant to the plan service module as well.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
5-5: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below(MD022, blanks-around-headings)
6-6: Lists should be surrounded by blank lines
null(MD032, blanks-around-lists)
health-services/plan-service/pom.xml (2)
7-7: Version update looks good but consider documenting changes.The version increment to 1.0.1 aligns with the Microplanning v0.2 changes. Consider adding a CHANGELOG.md entry to document the changes in this version.
Line range hint
63-77: Consider using release versions instead of SNAPSHOT dependencies.The following dependencies are using SNAPSHOT versions which is not recommended for production releases:
- tracer:2.9.0-SNAPSHOT
- mdms-client:2.9.0-SNAPSHOT
- services-common:2.9.0-SNAPSHOT
Consider upgrading to stable release versions before finalizing version 1.0.1.
Run this script to check if release versions are available:
health-services/resource-generator/pom.xml (3)
7-7: Version update is consistent with plan-service.The version increment to 1.0.1 maintains consistency with plan-service, which is good practice.
Line range hint
89-103: Consider using release versions instead of SNAPSHOT dependencies.Similar to plan-service, the following dependencies are using SNAPSHOT versions:
- tracer:2.9.0-SNAPSHOT
- services-common:2.9.0-SNAPSHOT
- mdms-client:2.9.0-SNAPSHOT
Consider upgrading to stable release versions before finalizing version 1.0.1.
9-10: Avoid using SNAPSHOT version for GeoTools dependency.The GeoTools version is set to
32-SNAPSHOTwhich could lead to unstable behavior. Consider using a stable release version.Run this script to check available stable versions:
HCMPRE-1922 Changelog and pom.xml changes
https://digit-discuss.atlassian.net/browse/HCMPRE-1922
Summary by CodeRabbit
Summary by CodeRabbit
Based on the comprehensive changes, here are the updated release notes:
New Features
Improvements
Technical Enhancements
Database Changes
plan_additional_fieldtable to store extended plan information.facility_namecolumn inplan_facility_linkagetable to support longer names.These changes provide more flexibility and detailed tracking for health service planning and resource management.