-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HCMPRE-2212 making facilityIds in plan search a set for multi select dropdown #1360
Conversation
…down search, changes filterMap signature.
…down search, changes filterMap signature.
…down search, changes filterMap signature.
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 comprehensive changes to the health services plan service, focusing on enhancing plan facility management, search capabilities, and data enrichment. The modifications span multiple components, including data models, utilities, and configuration settings. Key improvements include support for additional fields, refined search criteria, mixed strategy operations, and Excel file processing utilities. Changes
Sequence DiagramsequenceDiagram
participant Client
participant PlanService
participant PlanEnricher
participant PlanRepository
participant QueryBuilder
participant RowMapper
Client->>PlanService: Search Plan Request
PlanService->>PlanEnricher: Enrich Search Request
PlanEnricher-->>PlanService: Enriched Request
PlanService->>PlanRepository: Execute Search
PlanRepository->>QueryBuilder: Build Query
QueryBuilder-->>PlanRepository: SQL Query
PlanRepository->>RowMapper: Map Results
RowMapper->>PlanRepository: Mapped Plan Data
PlanRepository-->>PlanService: Plan Results
PlanService-->>Client: Return Plans
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 (47)
health-services/resource-generator/src/main/java/org/egov/processor/util/ParsingUtil.java (1)
394-429: 🧹 Nitpick (assertive)
Add support for JSON array type extraction.
The method handles various JSON node types well and uses
BigDecimal
for numeric precision. However, it's missing support for JSON array type.} else if (node.isObject()) { return objectMapper.convertValue(node, Map.class); // Return the object node as a Map +} else if (node.isArray()) { + return objectMapper.convertValue(node, List.class); // Return the array node as a List }📝 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./** * Extracts provided field from the additional details object * * @param additionalDetails the additionalDetails object from PlanConfigurationRequest * @param fieldToExtract the name of the field to be extracted from the additional details * @return the value of the specified field as a string * @throws CustomException if the field does not exist */ public Object extractFieldsFromJsonObject(Object additionalDetails, String fieldToExtract) { try { String jsonString = objectMapper.writeValueAsString(additionalDetails); JsonNode rootNode = objectMapper.readTree(jsonString); JsonNode node = rootNode.get(fieldToExtract); if (node != null && !node.isNull()) { // Check for different types of JSON nodes if (node.isDouble() || node.isFloat()) { return BigDecimal.valueOf(node.asDouble()); // Convert Double to BigDecimal } else if (node.isLong() || node.isInt()) { return BigDecimal.valueOf(node.asLong()); // Convert Long to BigDecimal } else if (node.isBoolean()) { return node.asBoolean(); } else if (node.isTextual()) { return node.asText(); } else if (node.isObject()) { return objectMapper.convertValue(node, Map.class); // Return the object node as a Map } else if (node.isArray()) { return objectMapper.convertValue(node, List.class); // Return the array node as a List } } log.debug("The field to be extracted - " + fieldToExtract + " is not present in additional details."); return null; } catch (Exception e) { log.error(e.getMessage() + fieldToExtract); throw new CustomException(PROVIDED_KEY_IS_NOT_PRESENT_IN_JSON_OBJECT_CODE, PROVIDED_KEY_IS_NOT_PRESENT_IN_JSON_OBJECT_MESSAGE + fieldToExtract); } }
health-services/resource-generator/src/main/java/org/egov/processor/util/OutputEstimationGenerationUtil.java (3)
188-216: 🧹 Nitpick (assertive)
Gracefully handle missing boundary codes.
When retrieving boundary codes, consider scenarios where a row might not contain a valid boundary code or the cell is empty. Right now, missing boundary codes map to an empty string. Confirm that is an acceptable fallback or consider logging a warning.
247-261: 🧹 Nitpick (assertive)
MDMS-based sheet allowance logic works but can be simplified.
You’re checking both the MDMS read-me name and boundary data to determine if a sheet is processed. Splitting the logic or adding explanatory comments could make it clearer why certain sheets are excluded. This is a minor readability improvement.
51-75: 🧹 Nitpick (assertive)
Consider validating workbook contents before processing.
Although you remove unnecessary sheets, if the workbook has zero or very few sheets (e.g., empty or malformed workbook), interactions further below might raise errors. Adding a quick validation or at least a log statement for an empty workbook scenario could improve robustness.public void processOutputFile(Workbook workbook, PlanConfigurationRequest request, String filestoreId) { + if (workbook.getNumberOfSheets() == 0) { + // Log or manage empty workbook scenario + } LocaleResponse localeResponse = ...Committable suggestion skipped: line range outside the PR's diff.
health-services/plan-service/src/main/java/digit/service/validator/PlanEmployeeAssignmentValidator.java (1)
320-322: 💡 Codebase verification
Validation bypass during reactivation confirmed
The codebase analysis confirms that:
- Campaign details are not re-validated when inactive records are reactivated
- No safeguards exist to prevent reactivation of records with invalid campaign details
- This could lead to invalid campaign associations becoming active in the system
Recommendations:
- Add validation during status transitions (inactive → active)
- Consider moving campaign validation to a separate method that can be called during both updates and reactivations
- Alternatively, always validate campaign details regardless of active status
🔗 Analysis chain
Verify the implications of skipping validation for inactive records.
The change to validate campaign details only for active records could have significant implications:
- What happens when an inactive record is later reactivated? The campaign details might be invalid at that point.
- Are there any system components that assume all records (active/inactive) are validated?
Consider these alternatives:
- Always validate but handle validation failures differently for inactive records
- Add validation during status transitions (inactive → active)
Let's verify the reactivation flow with this script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there's a separate validation during record reactivation # Expected: Find any reactivation logic that might need additional validation # Search for status/active field updates rg -A 5 "setActive|isActive|getActive" --type java # Search for reactivation related methods ast-grep --pattern 'method $_($$$) { $$$ $_.setActive(true) $$$ }'Length of output: 65767
health-services/resource-generator/src/main/java/org/egov/processor/service/ExcelParser.java (7)
72-76: 🧹 Nitpick (assertive)
Added new utility dependencies as class fields
Ensure each of these fields is used and tested. The spacing between fields is fine, though some teams prefer no extra blank lines for consistency.
79-80: 🧹 Nitpick (assertive)
Constructor with many injected dependencies
This constructor is getting quite large. Consider refactoring or leveraging a builder/factory pattern to keep it more manageable and maintainable.Also applies to: 94-96
239-239: 🧹 Nitpick (assertive)
Deeply nested conditional blocks
These nested if statements might become unwieldy. Consider refactoring to separate methods or strategy patterns for different statuses.Also applies to: 241-241, 245-245
256-303: 🧹 Nitpick (assertive)
New method ‘fetchFixedPostDetails’ returns a boundary-to-boolean map
- The name could be more descriptive since it specifically maps boundary codes to a "fixed post" flag.
- The
sheet
parameter isn’t utilized internally—review if it’s necessary.
322-328: 🧹 Nitpick (assertive)
Process rows with fixed post details
Fetching fixed-post map per sheet may affect performance if used repeatedly. Evaluate whether you can do it once for the entire workbook if results are the same.
415-416: 🧹 Nitpick (assertive)
Update doc comment for new parameters
The method doc references columns but does not describe the newboundaryCodeToFixedPostMap
. Add it for clarity and completeness.
40-40: 🧹 Nitpick (assertive)
Avoid wildcard static imports
Static wildcard imports can cause namespace ambiguity. Consider explicitly importing specific constants for clarity.health-services/plan-service/src/main/java/digit/util/QueryUtil.java (1)
132-149: 🧹 Nitpick (assertive)
Add input validation and consider stack depth limitations.
The implementation correctly handles Set values, but consider adding input validation and being mindful of stack depth for deeply nested structures.
private void prepareNestedQueryMap(int index, String[] nestedKeyArray, Map<String, Object> currentQueryMap, Set<String> values) { + // Validate input parameters + if (nestedKeyArray == null || currentQueryMap == null || values == null) { + throw new IllegalArgumentException("Input parameters cannot be null"); + } + + // Prevent stack overflow for deeply nested structures + if (index > 100) { // Configure max depth as needed + throw new IllegalArgumentException("Nested structure exceeds maximum allowed depth"); + } + // Return when all levels have been reached. if (index == nestedKeyArray.length) return;📝 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 void prepareNestedQueryMap(int index, String[] nestedKeyArray, Map<String, Object> currentQueryMap, Set<String> values) { // Validate input parameters if (nestedKeyArray == null || currentQueryMap == null || values == null) { throw new IllegalArgumentException("Input parameters cannot be null"); } // Prevent stack overflow for deeply nested structures if (index > 100) { // Configure max depth as needed throw new IllegalArgumentException("Nested structure exceeds maximum allowed depth"); } // Return when all levels have been reached. if (index == nestedKeyArray.length) return; // For the final level simply put the value in the map. else if (index == nestedKeyArray.length - 1) { currentQueryMap.put(nestedKeyArray[index], values); return; } // For non terminal levels, add a child map. currentQueryMap.put(nestedKeyArray[index], new HashMap<>()); // Make a recursive call to enrich data in next level. prepareNestedQueryMap(index + 1, nestedKeyArray, (Map<String, Object>) currentQueryMap.get(nestedKeyArray[index]), values);
health-services/resource-generator/src/main/java/org/egov/processor/util/MixedStrategyUtil.java (4)
25-35: 🧹 Nitpick (assertive)
Ensure non-null references for injected dependencies.
While the constructor parameters (e.g.,mdmsV2Util
,parsingUtil
,mapper
) are likely required, consider validating these to avoid unintendedNullPointerException
at runtime.
67-85: 🛠️ Refactor suggestion
Add null safety checks for distribution process fields.
Though the MDMS data typically ensures non-null fields, calls tologic.getRegistrationProcess()
andlogic.getDistributionProcess()
may still causeNullPointerException
if the data is malformed. Consider adding checks or ensuring all fields are always non-null inMixedStrategyOperationLogic
.
87-119: 🧹 Nitpick (assertive)
Optimize result map processing.
While this logic is correct, you could combine categories lookup and output key iteration in a single pass to reduce overhead if the data set grows large.-for (String category : categoriesNotAllowed) { - List<String> outputKeys = categoryNotAllowedToOutputMap.getOrDefault(category, Collections.emptyList()); - for (String outputKey : outputKeys) { - if (resultMap.containsKey(outputKey)) { - resultMap.put(outputKey, null); - } - } -} +categoryNotAllowedToOutputMap + .entrySet() + .stream() + .filter(entry -> categoriesNotAllowed.contains(entry.getKey())) + .flatMap(entry -> entry.getValue().stream()) + .filter(resultMap::containsKey) + .forEach(key -> resultMap.put(key, 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./** * Nullifies result values in the map for outputs belonging to the categories not allowed. * Exits early if no restrictions are specified. * * @param resultMap A map containing output keys and their corresponding result values. * @param operations A list of operations. * @param categoriesNotAllowed A list of categories that are restricted and should not have associated outputs. */ public void processResultMap(Map<String, BigDecimal> resultMap, List<Operation> operations, List<String> categoriesNotAllowed) { // If all the categories are allowed, don't process further. if(CollectionUtils.isEmpty(categoriesNotAllowed)) return; // Map categories not allowed to its corresponding list of output keys Map<String, List<String>> categoryNotAllowedToOutputMap = operations.stream() .filter(op -> op.getActive() && categoriesNotAllowed.contains(op.getCategory())) .collect(Collectors.groupingBy( Operation::getCategory, Collectors.mapping(Operation::getOutput, Collectors.toList()))); categoryNotAllowedToOutputMap .entrySet() .stream() .filter(entry -> categoriesNotAllowed.contains(entry.getKey())) .flatMap(entry -> entry.getValue().stream()) .filter(resultMap::containsKey) .forEach(key -> resultMap.put(key, null)); } }
1-22: 💡 Codebase verification
The review comment is correct - tenant ID splitting needs defensive checks
The code infetchMixedStrategyOperationLogicFromMDMS
directly accesses array index without validation, which could causeArrayIndexOutOfBoundsException
. Additionally, a null check ongetTenantId()
would further improve robustness.+if (request.getPlanConfiguration().getTenantId() == null) { + throw new CustomException("INVALID_TENANT", "Tenant ID cannot be null"); +} +String[] tenantArr = request.getPlanConfiguration().getTenantId().split("\\."); +if (tenantArr.length == 0) { + throw new CustomException("INVALID_TENANT_FORMAT", "Tenant ID is not in the expected format"); +} +String rootTenantId = tenantArr[0];🔗 Analysis chain
Use defensive checks when extracting the root tenant ID.
Splitting the tenant ID by"."
might causeArrayIndexOutOfBoundsException
if the string isn't in the expected format. Consider adding a defensive check before using[0]
.+String[] tenantArr = request.getPlanConfiguration().getTenantId().split("\\."); +if(tenantArr.length == 0) { + throw new CustomException("INVALID_TENANT_FORMAT", "Tenant ID is not in the expected format"); +} +String rootTenantId = tenantArr[0];🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find the complete implementation of MixedStrategyUtil.java cat health-services/resource-generator/src/main/java/org/egov/processor/util/MixedStrategyUtil.javaLength of output: 5832
health-services/resource-generator/src/main/java/org/egov/processor/util/MdmsUtil.java (3)
88-90: 🧹 Nitpick (assertive)
Confirm plan module removal if unused.
getPlanModuleDetail()
remains in the class, but the new logic calls onlygetAdminConsoleModuleDetail()
. If the plan module details are no longer relevant, consider removing that method.Would you like a follow-up patch removing the unused plan module methods?
132-168:
⚠️ Potential issueVerify safe JSON parsing of master data.
UsingJsonUtils.parseJson
is suitable, but ensure that users do not inject malicious or malformed JSON. Apply robust error handling here or upstream.Add robust checks or move to a safer library if there's any chance of user-supplied JSON.
75-75: 🧹 Nitpick (assertive)
Improve error message formatting.
UseString.format
or placeholder-based logging to avoid string concatenation in exceptions for better readability.-throw new CustomException(NO_MDMS_DATA_FOUND_FOR_GIVEN_TENANT_CODE, - "no data found for the given tenantid "+tenantId + " for master name "+ServiceConstants.MDMS_MASTER_ADMIN_SCHEMA); +throw new CustomException( + NO_MDMS_DATA_FOUND_FOR_GIVEN_TENANT_CODE, + String.format("No data found for tenantId '%s' under master '%s'", tenantId, ServiceConstants.MDMS_MASTER_ADMIN_SCHEMA) +);Committable suggestion skipped: line range outside the PR's diff.
health-services/resource-generator/src/main/java/org/egov/processor/web/models/planFacility/PlanFacilityResponse.java (1)
26-33: 🧹 Nitpick (assertive)
Consider using a standard naming convention for fields as well.
While you are using PascalCase in the JSON properties (PlanFacility
,TotalCount
), the underlying field names are in camelCase, which is typical. Ensure this consistently adheres to prior code style.health-services/resource-generator/src/main/java/org/egov/processor/web/models/mdmsV2/MixedStrategyOperationLogic.java (1)
26-33: 🧹 Nitpick (assertive)
Consider adding validation constraints for required fields.
The properties appear to be required for the mixed strategy operation logic to work correctly. Consider adding
@NotNull
constraints where appropriate.@JsonProperty("RegistrationProcess") +@NotNull private String registrationProcess; @JsonProperty("DistributionProcess") +@NotNull private String distributionProcess; @JsonProperty("CategoriesNotAllowed") +@NotNull private List<String> categoriesNotAllowed;📝 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("RegistrationProcess") @NotNull private String registrationProcess; @JsonProperty("DistributionProcess") @NotNull private String distributionProcess; @JsonProperty("CategoriesNotAllowed") @NotNull private List<String> categoriesNotAllowed;
health-services/resource-generator/src/main/java/org/egov/processor/web/models/planFacility/PlanFacilitySearchCriteria.java (1)
68-69:
⚠️ Potential issueUpdate filtersMap type to support multi-select values.
The filtersMap type should be
Map<String, Set<String>>
instead ofMap<String, String>
to support multiple values for each key, aligning with the PR objective of enabling multi-select dropdown for facilityIds.Apply this diff:
- private Map<String, String> filtersMap = null; + private Map<String, Set<String>> filtersMap = 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.@JsonIgnore private Map<String, Set<String>> filtersMap = null;
health-services/resource-generator/src/main/java/org/egov/processor/util/PlanFacilityUtil.java (1)
39-41:
⚠️ Potential issueImprove error handling in search method.
Catching a generic Exception and returning null silently is not a good practice. Consider:
- Using specific exceptions
- Propagating the error to the caller
- Including more context in the error log
Apply this diff:
- } catch (Exception e) { - log.error(ServiceConstants.ERROR_WHILE_SEARCHING_PLAN_FACILITY, e); + } catch (IllegalArgumentException e) { + log.error("Error converting response for planFacilitySearchRequest: {}", planfacilitySearchRequest, e); + throw new RuntimeException("Error processing plan facility search response", e); + } catch (Exception e) { + log.error("Unexpected error while searching plan facility for request: {}", planfacilitySearchRequest, e); + throw new RuntimeException("Error searching plan facility", e);Committable suggestion skipped: line range outside the PR's diff.
health-services/resource-generator/src/main/java/org/egov/processor/web/models/planFacility/PlanFacility.java (1)
61-64: 🧹 Nitpick (assertive)
Consider using Set instead of List for serviceBoundaries.
Since this PR aims to support multi-select functionality and prevent duplicates, using
Set<String>
instead ofList<String>
would be more appropriate forserviceBoundaries
.- private List<String> serviceBoundaries; + private Set<String> serviceBoundaries;📝 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("serviceBoundaries") @NotNull @Valid private Set<String> serviceBoundaries;
health-services/resource-generator/src/main/java/org/egov/processor/web/models/census/Census.java (1)
51-51: 🧹 Nitpick (assertive)
Consider adding @Builder.Default for assignee initialization.
Since
assignee
is a collection that might need initialization, consider adding@Builder.Default
annotation to ensure it's properly initialized when using the builder pattern.- private List<String> assignee = null; + @Builder.Default + private List<String> assignee = new ArrayList<>();📝 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.@Builder.Default private List<String> assignee = new ArrayList<>();
health-services/resource-generator/src/main/java/org/egov/processor/util/ExcelStylingUtil.java (2)
69-77: 🧹 Nitpick (assertive)
Extract magic numbers into constants.
The calculation of cell width uses magic numbers that should be extracted into named constants for better maintainability.
+ private static final int CHAR_WIDTH_UNITS = 256; // 1/256th of character width + private static final int DEFAULT_CHAR_WIDTH = 1; + // Calculate the width needed for the current cell content String cellValue = cell.toString(); // Convert cell content to string - int cellWidth = cellValue.length() * 256; // Approximate width (1/256th of character width) + int cellWidth = cellValue.length() * CHAR_WIDTH_UNITS * DEFAULT_CHAR_WIDTH;📝 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 static final int CHAR_WIDTH_UNITS = 256; // 1/256th of character width private static final int DEFAULT_CHAR_WIDTH = 1; // Calculate the width needed for the current cell content String cellValue = cell.toString(); // Convert cell content to string int cellWidth = cellValue.length() * CHAR_WIDTH_UNITS * DEFAULT_CHAR_WIDTH; // Use the maximum width seen so far, including padding for readability int padding = COLUMN_PADDING; // Adjust padding as needed int newWidth = Math.max(maxWidth, cellWidth + padding); sheet.setColumnWidth(columnIndex, newWidth);
88-112: 🛠️ Refactor suggestion
Add input validation and resource cleanup in hexToXSSFColor method.
- Add regex validation for hex color format
- Add null check for workbook parameter
public static XSSFColor hexToXSSFColor(String hexColor, XSSFWorkbook xssfWorkbook) { + if (xssfWorkbook == null) { + throw new IllegalArgumentException("XSSFWorkbook cannot be null"); + } - if (hexColor == null || hexColor.length() < 6) + if (hexColor == null || !hexColor.matches("^[0-9A-Fa-f]{6}$")) throw new IllegalArgumentException(INVALID_HEX + hexColor);📝 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 XSSFColor hexToXSSFColor(String hexColor, XSSFWorkbook xssfWorkbook) { if (xssfWorkbook == null) { throw new IllegalArgumentException("XSSFWorkbook cannot be null"); } if (hexColor == null || !hexColor.matches("^[0-9A-Fa-f]{6}$")) throw new IllegalArgumentException(INVALID_HEX + hexColor); // Convert HEX to RGB int red = Integer.valueOf(hexColor.substring(0, 2), 16); int green = Integer.valueOf(hexColor.substring(2, 4), 16); int blue = Integer.valueOf(hexColor.substring(4, 6), 16); red = (int) (red * BRIGHTEN_FACTOR); // increase red component by 10% green = (int) (green * BRIGHTEN_FACTOR); // increase green component by 10% blue = (int) (blue * BRIGHTEN_FACTOR); // increase blue component by 10% // Clamp the values to be between 0 and 255 red = Math.min(255, Math.max(0, red)); green = Math.min(255, Math.max(0, green)); blue = Math.min(255, Math.max(0, blue)); // Create IndexedColorMap from the workbook's styles source IndexedColorMap colorMap = xssfWorkbook.getStylesSource().getIndexedColors(); // Create XSSFColor using the XSSFWorkbook context and colorMap return new XSSFColor(new Color(red, green, blue), colorMap); }
health-services/resource-generator/src/main/java/org/egov/processor/util/PlanUtil.java (2)
156-167: 🧹 Nitpick (assertive)
Add error handling for type casting.
The type casting of value to Map could fail at runtime. Consider adding try-catch block to handle ClassCastException.
private void extractNestedFields(Map<String, Object> details, String prefix, Map<String, Object> fieldsToBeUpdated) { for (Map.Entry<String, Object> entry : details.entrySet()) { String key = entry.getKey(); Object value = entry.getValue(); if (value instanceof Map) { + try { Map<String, Object> nestedMap = (Map<String, Object>) value; if (nestedMap.containsKey(CODE)) { fieldsToBeUpdated.put(prefix + "|" + key + "|" + CODE, nestedMap.get(CODE)); } + } catch (ClassCastException e) { + log.error("Failed to cast nested value to Map for key: {}", key, e); + } } } }Committable suggestion skipped: line range outside the PR's diff.
112-146: 🧹 Nitpick (assertive)
Consider adding validation for field types.
While the method handles null checks well, consider adding type validation for the extracted fields to ensure they match expected types before updating.
private Object enrichAdditionalDetails(Map<String, Object> boundaryCodeToCensusAdditionalDetails, String boundaryCodeValue) { if(!CollectionUtils.isEmpty(boundaryCodeToCensusAdditionalDetails)) { Object censusAdditionalDetails = boundaryCodeToCensusAdditionalDetails.get(boundaryCodeValue); if(ObjectUtils.isEmpty(censusAdditionalDetails)) return null; + // Validate extracted fields match expected types + if (!(parsingUtil.extractFieldsFromJsonObject(censusAdditionalDetails, FACILITY_ID) instanceof String)) { + log.error("Invalid type for facilityId in census additional details"); + return null; + } String facilityId = (String) parsingUtil.extractFieldsFromJsonObject(censusAdditionalDetails, FACILITY_ID);Committable suggestion skipped: line range outside the PR's diff.
health-services/plan-service/src/main/java/digit/repository/rowmapper/PlanRowMapper.java (1)
241-273: 🧹 Nitpick (assertive)
Consider initializing collections with expected capacity.
To improve performance, consider initializing ArrayList with an expected capacity when creating additionalFields list.
if (CollectionUtils.isEmpty(planEntry.getAdditionalFields())) { - List<AdditionalField> additionalFields = new ArrayList<>(); + // Initialize with expected capacity to avoid resizing + List<AdditionalField> additionalFields = new ArrayList<>(10); // Adjust size based on typical usage additionalFields.add(additionalField); planEntry.setAdditionalFields(additionalFields); }📝 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 AdditionalField object to the plan entry based on the result set. * * @param rs The ResultSet containing the data. * @param additionalFieldSet A set to keep track of added AdditionalField objects. * @param planEntry The Plan entry to which the AdditionalField object will be added. * @throws SQLException If an SQL error occurs. */ private void addAdditionalField(ResultSet rs, Plan planEntry, Set<String> additionalFieldSet) throws SQLException { String additionalFieldId = rs.getString("plan_additional_field_id"); if (ObjectUtils.isEmpty(additionalFieldId) || additionalFieldSet.contains(additionalFieldId)) { return; } AdditionalField additionalField = new AdditionalField(); additionalField.setId(rs.getString("plan_additional_field_id")); additionalField.setKey(rs.getString("plan_additional_field_key")); additionalField.setValue(rs.getBigDecimal("plan_additional_field_value")); additionalField.setShowOnUi(rs.getBoolean("plan_additional_field_show_on_ui")); additionalField.setEditable(rs.getBoolean("plan_additional_field_editable")); additionalField.setOrder(rs.getInt("plan_additional_field_order")); if (CollectionUtils.isEmpty(planEntry.getAdditionalFields())) { // Initialize with expected capacity to avoid resizing List<AdditionalField> additionalFields = new ArrayList<>(10); // Adjust size based on typical usage additionalFields.add(additionalField); planEntry.setAdditionalFields(additionalFields); } else { planEntry.getAdditionalFields().add(additionalField); } additionalFieldSet.add(additionalFieldId); }
health-services/plan-service/src/main/java/digit/service/PlanEnricher.java (4)
245-285: 🧹 Nitpick (assertive)
Consider extracting filter addition logic into a helper method.
The repetitive pattern of checking and adding filters could be extracted into a helper method to improve readability and maintainability.
+ private void addFilterIfNotEmpty(Map<String, Set<String>> filtersMap, String key, Object value) { + if (!ObjectUtils.isEmpty(value)) { + if (value instanceof Set) { + filtersMap.put(key, (Set<String>) value); + } else { + filtersMap.put(key, Collections.singleton(value.toString())); + } + } + } + public void enrichSearchRequest(PlanSearchRequest planSearchRequest) { PlanSearchCriteria planSearchCriteria = planSearchRequest.getPlanSearchCriteria(); Map<String, Set<String>> filtersMap = new LinkedHashMap<>(); - if (!ObjectUtils.isEmpty(planSearchCriteria.getFacilityIds())) { - filtersMap.put(FACILITY_ID_SEARCH_PARAMETER_KEY, planSearchCriteria.getFacilityIds()); - } - - if (!ObjectUtils.isEmpty(planSearchCriteria.getTerrain())) { - filtersMap.put(TERRAIN_CONDITION_SEARCH_PARAMETER_KEY, Collections.singleton(planSearchCriteria.getTerrain())); - } - - if (!ObjectUtils.isEmpty(planSearchCriteria.getOnRoadCondition())) { - filtersMap.put(ROAD_CONDITION_SEARCH_PARAMETER_KEY, Collections.singleton(planSearchCriteria.getOnRoadCondition())); - } - - if (!ObjectUtils.isEmpty(planSearchCriteria.getSecurityQ1())) { - filtersMap.put(SECURITY_Q1_SEARCH_PARAMETER_KEY, Collections.singleton(planSearchCriteria.getSecurityQ1())); - } - - if (!ObjectUtils.isEmpty(planSearchCriteria.getSecurityQ2())) { - filtersMap.put(SECURITY_Q2_SEARCH_PARAMETER_KEY, Collections.singleton(planSearchCriteria.getSecurityQ2())); - } + addFilterIfNotEmpty(filtersMap, FACILITY_ID_SEARCH_PARAMETER_KEY, planSearchCriteria.getFacilityIds()); + addFilterIfNotEmpty(filtersMap, TERRAIN_CONDITION_SEARCH_PARAMETER_KEY, planSearchCriteria.getTerrain()); + addFilterIfNotEmpty(filtersMap, ROAD_CONDITION_SEARCH_PARAMETER_KEY, planSearchCriteria.getOnRoadCondition()); + addFilterIfNotEmpty(filtersMap, SECURITY_Q1_SEARCH_PARAMETER_KEY, planSearchCriteria.getSecurityQ1()); + addFilterIfNotEmpty(filtersMap, SECURITY_Q2_SEARCH_PARAMETER_KEY, planSearchCriteria.getSecurityQ2()); if(!CollectionUtils.isEmpty(filtersMap)) planSearchCriteria.setFiltersMap(filtersMap); }📝 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./** * Enriches the PlanSearchRequest by populating the filters map from the fields in search criteria. * This filterMap is populated to search the fields in plan additional detail object. * * @param planSearchRequest the planSearchRequest object whose search criteria need enrichment. */ private void addFilterIfNotEmpty(Map<String, Set<String>> filtersMap, String key, Object value) { if (!ObjectUtils.isEmpty(value)) { if (value instanceof Set) { filtersMap.put(key, (Set<String>) value); } else { filtersMap.put(key, Collections.singleton(value.toString())); } } } public void enrichSearchRequest(PlanSearchRequest planSearchRequest) { PlanSearchCriteria planSearchCriteria = planSearchRequest.getPlanSearchCriteria(); // Filter map for filtering plan metadata present in additional details Map<String, Set<String>> filtersMap = new LinkedHashMap<>(); addFilterIfNotEmpty(filtersMap, FACILITY_ID_SEARCH_PARAMETER_KEY, planSearchCriteria.getFacilityIds()); addFilterIfNotEmpty(filtersMap, TERRAIN_CONDITION_SEARCH_PARAMETER_KEY, planSearchCriteria.getTerrain()); addFilterIfNotEmpty(filtersMap, ROAD_CONDITION_SEARCH_PARAMETER_KEY, planSearchCriteria.getOnRoadCondition()); addFilterIfNotEmpty(filtersMap, SECURITY_Q1_SEARCH_PARAMETER_KEY, planSearchCriteria.getSecurityQ1()); addFilterIfNotEmpty(filtersMap, SECURITY_Q2_SEARCH_PARAMETER_KEY, planSearchCriteria.getSecurityQ2()); if(!CollectionUtils.isEmpty(filtersMap)) planSearchCriteria.setFiltersMap(filtersMap); }
245-285: 🛠️ Refactor suggestion
Enhance method documentation and add null checks.
The method documentation could be more detailed, and null checks should be added for the input parameter.
Apply these improvements:
/** * Enriches the PlanSearchRequest by populating the filters map from the fields in search criteria. * This filterMap is populated to search the fields in plan additional detail object. * - * @param planSearchRequest the planSearchRequest object whose search criteria need enrichment. + * @param planSearchRequest The planSearchRequest object whose search criteria need enrichment + * @throws IllegalArgumentException if planSearchRequest or its search criteria is null */ public void enrichSearchRequest(PlanSearchRequest planSearchRequest) { + if (planSearchRequest == null || planSearchRequest.getPlanSearchCriteria() == null) { + throw new IllegalArgumentException("PlanSearchRequest and its search criteria cannot be null"); + } + PlanSearchCriteria planSearchCriteria = planSearchRequest.getPlanSearchCriteria(); // Filter map for filtering plan metadata present in additional details Map<String, Set<String>> filtersMap = new LinkedHashMap<>();📝 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./** * Enriches the PlanSearchRequest by populating the filters map from the fields in search criteria. * This filterMap is populated to search the fields in plan additional detail object. * * @param planSearchRequest The planSearchRequest object whose search criteria need enrichment * @throws IllegalArgumentException if planSearchRequest or its search criteria is null */ public void enrichSearchRequest(PlanSearchRequest planSearchRequest) { if (planSearchRequest == null || planSearchRequest.getPlanSearchCriteria() == null) { throw new IllegalArgumentException("PlanSearchRequest and its search criteria cannot be null"); } PlanSearchCriteria planSearchCriteria = planSearchRequest.getPlanSearchCriteria(); // Filter map for filtering plan metadata present in additional details Map<String, Set<String>> filtersMap = new LinkedHashMap<>(); // Add facility id as a filter if present in search criteria if (!ObjectUtils.isEmpty(planSearchCriteria.getFacilityIds())) { filtersMap.put(FACILITY_ID_SEARCH_PARAMETER_KEY, planSearchCriteria.getFacilityIds()); } // Add terrain as a filter if present in search criteria if (!ObjectUtils.isEmpty(planSearchCriteria.getTerrain())) { filtersMap.put(TERRAIN_CONDITION_SEARCH_PARAMETER_KEY, Collections.singleton(planSearchCriteria.getTerrain())); } // Add onRoadCondition as a filter if present in search criteria if (!ObjectUtils.isEmpty(planSearchCriteria.getOnRoadCondition())) { filtersMap.put(ROAD_CONDITION_SEARCH_PARAMETER_KEY, Collections.singleton(planSearchCriteria.getOnRoadCondition())); } // Add securityQ1 as a filter if present in search criteria if (!ObjectUtils.isEmpty(planSearchCriteria.getSecurityQ1())) { filtersMap.put(SECURITY_Q1_SEARCH_PARAMETER_KEY, Collections.singleton(planSearchCriteria.getSecurityQ1())); } // Add securityQ2 as a filter if present in search criteria if (!ObjectUtils.isEmpty(planSearchCriteria.getSecurityQ2())) { filtersMap.put(SECURITY_Q2_SEARCH_PARAMETER_KEY, Collections.singleton(planSearchCriteria.getSecurityQ2())); } if(!CollectionUtils.isEmpty(filtersMap)) planSearchCriteria.setFiltersMap(filtersMap); }
246-285: 🧹 Nitpick (assertive)
Extract repeated pattern into a helper method for better maintainability.
The method has repeated patterns for adding filters. Consider extracting this pattern into a helper method.
+ private void addFilterIfNotEmpty(Map<String, Set<String>> filtersMap, String key, Object value) { + if (!ObjectUtils.isEmpty(value)) { + if (value instanceof Set) { + filtersMap.put(key, (Set<String>) value); + } else { + filtersMap.put(key, Collections.singleton(value.toString())); + } + } + } + public void enrichSearchRequest(PlanSearchRequest planSearchRequest) { PlanSearchCriteria planSearchCriteria = planSearchRequest.getPlanSearchCriteria(); Map<String, Set<String>> filtersMap = new LinkedHashMap<>(); - if (!ObjectUtils.isEmpty(planSearchCriteria.getFacilityIds())) { - filtersMap.put(FACILITY_ID_SEARCH_PARAMETER_KEY, planSearchCriteria.getFacilityIds()); - } - - if (!ObjectUtils.isEmpty(planSearchCriteria.getTerrain())) { - filtersMap.put(TERRAIN_CONDITION_SEARCH_PARAMETER_KEY, Collections.singleton(planSearchCriteria.getTerrain())); - } - - if (!ObjectUtils.isEmpty(planSearchCriteria.getOnRoadCondition())) { - filtersMap.put(ROAD_CONDITION_SEARCH_PARAMETER_KEY, Collections.singleton(planSearchCriteria.getOnRoadCondition())); - } - - if (!ObjectUtils.isEmpty(planSearchCriteria.getSecurityQ1())) { - filtersMap.put(SECURITY_Q1_SEARCH_PARAMETER_KEY, Collections.singleton(planSearchCriteria.getSecurityQ1())); - } - - if (!ObjectUtils.isEmpty(planSearchCriteria.getSecurityQ2())) { - filtersMap.put(SECURITY_Q2_SEARCH_PARAMETER_KEY, Collections.singleton(planSearchCriteria.getSecurityQ2())); - } + addFilterIfNotEmpty(filtersMap, FACILITY_ID_SEARCH_PARAMETER_KEY, planSearchCriteria.getFacilityIds()); + addFilterIfNotEmpty(filtersMap, TERRAIN_CONDITION_SEARCH_PARAMETER_KEY, planSearchCriteria.getTerrain()); + addFilterIfNotEmpty(filtersMap, ROAD_CONDITION_SEARCH_PARAMETER_KEY, planSearchCriteria.getOnRoadCondition()); + addFilterIfNotEmpty(filtersMap, SECURITY_Q1_SEARCH_PARAMETER_KEY, planSearchCriteria.getSecurityQ1()); + addFilterIfNotEmpty(filtersMap, SECURITY_Q2_SEARCH_PARAMETER_KEY, planSearchCriteria.getSecurityQ2()); if(!CollectionUtils.isEmpty(filtersMap)) planSearchCriteria.setFiltersMap(filtersMap); }📝 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./** * Enriches the PlanSearchRequest by populating the filters map from the fields in search criteria. * This filterMap is populated to search the fields in plan additional detail object. * * @param planSearchRequest the planSearchRequest object whose search criteria need enrichment. */ private void addFilterIfNotEmpty(Map<String, Set<String>> filtersMap, String key, Object value) { if (!ObjectUtils.isEmpty(value)) { if (value instanceof Set) { filtersMap.put(key, (Set<String>) value); } else { filtersMap.put(key, Collections.singleton(value.toString())); } } } public void enrichSearchRequest(PlanSearchRequest planSearchRequest) { PlanSearchCriteria planSearchCriteria = planSearchRequest.getPlanSearchCriteria(); // Filter map for filtering plan metadata present in additional details Map<String, Set<String>> filtersMap = new LinkedHashMap<>(); addFilterIfNotEmpty(filtersMap, FACILITY_ID_SEARCH_PARAMETER_KEY, planSearchCriteria.getFacilityIds()); addFilterIfNotEmpty(filtersMap, TERRAIN_CONDITION_SEARCH_PARAMETER_KEY, planSearchCriteria.getTerrain()); addFilterIfNotEmpty(filtersMap, ROAD_CONDITION_SEARCH_PARAMETER_KEY, planSearchCriteria.getOnRoadCondition()); addFilterIfNotEmpty(filtersMap, SECURITY_Q1_SEARCH_PARAMETER_KEY, planSearchCriteria.getSecurityQ1()); addFilterIfNotEmpty(filtersMap, SECURITY_Q2_SEARCH_PARAMETER_KEY, planSearchCriteria.getSecurityQ2()); if(!CollectionUtils.isEmpty(filtersMap)) planSearchCriteria.setFiltersMap(filtersMap); }
57-60: 🧹 Nitpick (assertive)
Consider extracting UUID generation logic to avoid duplication.
The UUID generation logic for additional fields is duplicated in both create and update methods.
Extract the logic into a private method:
+ private void enrichAdditionalFieldsWithUuid(List<AdditionalField> additionalFields) { + if (!CollectionUtils.isEmpty(additionalFields)) { + additionalFields.forEach(field -> UUIDEnrichmentUtil.enrichRandomUuid(field, "id")); + } + } public void enrichPlanCreate(PlanRequest body) { // ... existing code ... - if(!CollectionUtils.isEmpty(body.getPlan().getAdditionalFields())) - body.getPlan().getAdditionalFields().forEach(additionalField -> UUIDEnrichmentUtil.enrichRandomUuid(additionalField, "id")); + enrichAdditionalFieldsWithUuid(body.getPlan().getAdditionalFields()); } public void enrichPlanUpdate(PlanRequest body) { // ... existing code ... - if(!CollectionUtils.isEmpty(body.getPlan().getAdditionalFields())) { - body.getPlan().getAdditionalFields().forEach(additionalFields -> { - if(ObjectUtils.isEmpty(additionalFields.getId())) { - UUIDEnrichmentUtil.enrichRandomUuid(additionalFields, "id"); - } - }); - } + if (!CollectionUtils.isEmpty(body.getPlan().getAdditionalFields())) { + body.getPlan().getAdditionalFields().stream() + .filter(field -> ObjectUtils.isEmpty(field.getId())) + .forEach(field -> UUIDEnrichmentUtil.enrichRandomUuid(field, "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.// Generate id for additional fields enrichAdditionalFieldsWithUuid(body.getPlan().getAdditionalFields());
health-services/plan-service/src/main/java/digit/service/validator/PlanFacilityValidator.java (2)
150-156: 🧹 Nitpick (assertive)
Consider adding null check for boundaries.
The method should handle the case where boundaries is null.
private Set<String> fetchAllBoundaryCodes(CampaignDetail campaignDetail) { + if (campaignDetail == null || campaignDetail.getBoundaries() == null) { + return Collections.emptySet(); + } Set<String> boundaryCodes = campaignDetail.getBoundaries().stream() .map(Boundary::getCode) .collect(Collectors.toSet()); return boundaryCodes; }📝 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 Set<String> fetchAllBoundaryCodes(CampaignDetail campaignDetail) { if (campaignDetail == null || campaignDetail.getBoundaries() == null) { return Collections.emptySet(); } Set<String> boundaryCodes = campaignDetail.getBoundaries().stream() .map(Boundary::getCode) .collect(Collectors.toSet()); return boundaryCodes; }
129-138: 🧹 Nitpick (assertive)
Improve code readability with descriptive variable names and comments.
The variable names could be more descriptive, and the comments could better explain the validation strategy.
Apply these improvements:
- Set<String> lowestHierarchyBCodes = fetchBoundaryCodes(campaignResponse.getCampaignDetails().get(0), lowestHierarchy); - Set<String> allBoundaryCodes = fetchAllBoundaryCodes(campaignResponse.getCampaignDetails().get(0)); + // Fetch boundary codes for the lowest hierarchy level (e.g., ward, locality) + Set<String> lowestHierarchyBoundaryCodes = fetchBoundaryCodes(campaignResponse.getCampaignDetails().get(0), lowestHierarchy); + + // Fetch boundary codes for all hierarchy levels (e.g., zone, ward, locality) + Set<String> allHierarchyBoundaryCodes = fetchAllBoundaryCodes(campaignResponse.getCampaignDetails().get(0)); - validateResidingBoundaries(allBoundaryCodes, planFacility); + // Validate residing boundaries against all hierarchy levels since a facility can be in any jurisdiction level + validateResidingBoundaries(allHierarchyBoundaryCodes, planFacility); - validateServiceBoundaries(lowestHierarchyBCodes, planFacility); + // Validate service boundaries against lowest hierarchy level since services are delivered at the lowest level + validateServiceBoundaries(lowestHierarchyBoundaryCodes, planFacility);📝 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 boundary codes for the lowest hierarchy level (e.g., ward, locality) Set<String> lowestHierarchyBoundaryCodes = fetchBoundaryCodes(campaignResponse.getCampaignDetails().get(0), lowestHierarchy); // Fetch boundary codes for all hierarchy levels (e.g., zone, ward, locality) Set<String> allHierarchyBoundaryCodes = fetchAllBoundaryCodes(campaignResponse.getCampaignDetails().get(0)); // Validate residing boundaries against all hierarchy levels since a facility can be in any jurisdiction level validateResidingBoundaries(allHierarchyBoundaryCodes, planFacility); // Validate service boundaries against lowest hierarchy level since services are delivered at the lowest level validateServiceBoundaries(lowestHierarchyBoundaryCodes, planFacility);
health-services/plan-service/src/main/resources/db/migration/main/V20242112151500__plan_additional_field_create_ddl.sql (3)
10-11: 🧹 Nitpick (assertive)
Add composite index for common queries.
Based on the learnings, indexes are planned for a future release. Consider adding them now for better query performance.
CONSTRAINT uk_plan_additional_field_id PRIMARY KEY (id), - FOREIGN KEY (plan_id) REFERENCES plan(id) + FOREIGN KEY (plan_id) REFERENCES plan(id), + CONSTRAINT uk_plan_additional_field_plan_key UNIQUE (plan_id, "key") ); + +CREATE INDEX idx_plan_additional_field_plan_id ON plan_additional_field(plan_id); +CREATE INDEX idx_plan_additional_field_key ON plan_additional_field("key");📝 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.CONSTRAINT uk_plan_additional_field_id PRIMARY KEY (id), FOREIGN KEY (plan_id) REFERENCES plan(id), CONSTRAINT uk_plan_additional_field_plan_key UNIQUE (plan_id, "key") ); CREATE INDEX idx_plan_additional_field_plan_id ON plan_additional_field(plan_id); CREATE INDEX idx_plan_additional_field_key ON plan_additional_field("key");
6-6: 🧹 Nitpick (assertive)
Consider using JSONB for flexible value types.
The
value
column is strictly numeric(12,2), which might be limiting for different types of additional fields (e.g., text, dates, arrays).- "value" numeric(12,2) NOT NULL, + "value" jsonb NOT 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."value" jsonb NOT NULL,
1-12: 🛠️ Refactor suggestion
Add essential database design elements.
Several important elements are missing from the table definition:
- Audit fields (created_by, created_time, etc.)
- Tenant_id for multi-tenancy support
- Indexes for performance (as planned in future release per learnings)
Apply this diff to add the missing columns:
CREATE TABLE plan_additional_field ( id character varying(64) NOT NULL, plan_id character varying(64) NOT NULL, "key" character varying(64) NOT NULL, "value" numeric(12,2) NOT NULL, show_on_ui boolean DEFAULT true NOT NULL, editable boolean DEFAULT true NOT NULL, "order" bigint NOT NULL, + tenant_id character varying(64) NOT NULL, + created_by character varying(64) NOT NULL, + created_time bigint NOT NULL, + last_modified_by character varying(64), + last_modified_time bigint, CONSTRAINT uk_plan_additional_field_id PRIMARY KEY (id), FOREIGN KEY (plan_id) REFERENCES plan(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.-- Table: plan_additional_field CREATE TABLE plan_additional_field ( id character varying(64) NOT NULL, plan_id character varying(64) NOT NULL, "key" character varying(64) NOT NULL, "value" numeric(12,2) NOT NULL, show_on_ui boolean DEFAULT true NOT NULL, editable boolean DEFAULT true NOT NULL, "order" bigint NOT NULL, tenant_id character varying(64) NOT NULL, created_by character varying(64) NOT NULL, created_time bigint NOT NULL, last_modified_by character varying(64), last_modified_time bigint, CONSTRAINT uk_plan_additional_field_id PRIMARY KEY (id), FOREIGN KEY (plan_id) REFERENCES plan(id) );
health-services/resource-generator/src/main/resources/application.properties (3)
70-72: 🧹 Nitpick (assertive)
Add missing configuration properties.
The plan facility endpoint is added, but related properties like create and update endpoints are missing.
#Plan Facility egov.plan.facility.search.endpoint=/plan-service/plan/facility/_search +egov.plan.facility.create.endpoint=/plan-service/plan/facility/_create +egov.plan.facility.update.endpoint=/plan-service/plan/facility/_update📝 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.#Plan Facility egov.plan.facility.search.endpoint=/plan-service/plan/facility/_search egov.plan.facility.create.endpoint=/plan-service/plan/facility/_create egov.plan.facility.update.endpoint=/plan-service/plan/facility/_update
62-65: 🛠️ Refactor suggestion
Use environment variables for host URLs.
Hardcoded URLs should be configurable through environment variables.
-egov.plan.config.host=https://unified-dev.digit.org +egov.plan.config.host=${EGOV_PLAN_CONFIG_HOST:https://unified-dev.digit.org}📝 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.#Plan Config egov.plan.config.host=${EGOV_PLAN_CONFIG_HOST:https://unified-dev.digit.org} egov.plan.config.endpoint=/plan-service/config/_search
101-102: 🧹 Nitpick (assertive)
Use consistent property naming.
The pagination property names have been updated, but consider using a more consistent naming pattern across all pagination configs.
-default.offset.for.mdms.data=0 -default.limit.for.mdms.data=10 +egov.mdms.pagination.default.offset=0 +egov.mdms.pagination.default.limit=10📝 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.egov.mdms.pagination.default.offset=0 egov.mdms.pagination.default.limit=10
health-services/plan-service/src/main/java/digit/repository/querybuilder/PlanQueryBuilder.java (3)
33-34: 🧹 Nitpick (assertive)
Add comments to explain the query structure.
The query has been extended to include additional fields, but it would benefit from comments explaining the purpose of each section and the relationships between tables.
Add comments above the query string to document:
- The purpose of each table join
- The meaning of each field group
- The aliasing convention used
Also applies to: 39-40
33-40: 🧹 Nitpick (assertive)
Consider using table aliases consistently for better readability.
The query uses a mix of full table names and aliases. For instance,
plan_additional_field
is aliased aspaf
in the JOIN clause but uses the full name in column selection.Apply this diff to maintain consistency:
- "\t plan_target.id as plan_target_id, plan_target.metric as plan_target_metric, plan_target.metric_value as plan_target_metric_value, plan_target.metric_comparator as plan_target_metric_comparator, plan_target.metric_unit as plan_target_metric_unit, plan_target.plan_id as plan_target_plan_id, plan_target.activity_code as plan_target_activity_code, plan_target.created_by as plan_target_created_by, plan_target.created_time as plan_target_created_time, plan_target.last_modified_by as plan_target_last_modified_by, plan_target.last_modified_time as plan_target_last_modified_time, \n" + - "\t paf.id as plan_additional_field_id, paf.plan_id as plan_additional_field_plan_id, paf.key as plan_additional_field_key, paf.value as plan_additional_field_value, paf.show_on_ui as plan_additional_field_show_on_ui, paf.editable as plan_additional_field_editable, paf.order as plan_additional_field_order \n" + + "\t pt.id as plan_target_id, pt.metric as plan_target_metric, pt.metric_value as plan_target_metric_value, pt.metric_comparator as plan_target_metric_comparator, pt.metric_unit as plan_target_metric_unit, pt.plan_id as plan_target_plan_id, pt.activity_code as plan_target_activity_code, pt.created_by as plan_target_created_by, pt.created_time as plan_target_created_time, pt.last_modified_by as plan_target_last_modified_by, pt.last_modified_time as plan_target_last_modified_time, \n" + + "\t paf.id as plan_additional_field_id, paf.plan_id as plan_additional_field_plan_id, paf.key as plan_additional_field_key, paf.value as plan_additional_field_value, paf.show_on_ui as plan_additional_field_show_on_ui, paf.editable as plan_additional_field_editable, paf.order as plan_additional_field_order \n" + "\t FROM plan \n" + "\t LEFT JOIN plan_activity ON plan.id = plan_activity.plan_id\n" + "\t LEFT JOIN plan_activity_condition ON plan_activity.id = plan_activity_condition.activity_id\n" + "\t LEFT JOIN plan_resource ON plan.id = plan_resource.plan_id\n" + - "\t LEFT JOIN plan_target ON plan.id = plan_target.plan_id\n" + + "\t LEFT JOIN plan_target pt ON plan.id = pt.plan_id\n" + "\t LEFT JOIN plan_additional_field paf ON plan.id = paf.plan_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."\t pt.id as plan_target_id, pt.metric as plan_target_metric, pt.metric_value as plan_target_metric_value, pt.metric_comparator as plan_target_metric_comparator, pt.metric_unit as plan_target_metric_unit, pt.plan_id as plan_target_plan_id, pt.activity_code as plan_target_activity_code, pt.created_by as plan_target_created_by, pt.created_time as plan_target_created_time, pt.last_modified_by as plan_target_last_modified_by, pt.last_modified_time as plan_target_last_modified_time, \n" + "\t paf.id as plan_additional_field_id, paf.plan_id as plan_additional_field_plan_id, paf.key as plan_additional_field_key, paf.value as plan_additional_field_value, paf.show_on_ui as plan_additional_field_show_on_ui, paf.editable as plan_additional_field_editable, paf.order as plan_additional_field_order \n" + "\t FROM plan \n" + "\t LEFT JOIN plan_activity ON plan.id = plan_activity.plan_id\n" + "\t LEFT JOIN plan_activity_condition ON plan_activity.id = plan_activity_condition.activity_id\n" + "\t LEFT JOIN plan_resource ON plan.id = plan_resource.plan_id\n" + "\t LEFT JOIN plan_target pt ON plan.id = pt.plan_id\n" + "\t LEFT JOIN plan_additional_field paf ON plan.id = paf.plan_id";
175-180: 🧹 Nitpick (assertive)
💡 Codebase verification
Consider adding a GIN index on the additional_details column.
The code uses the JSONB containment operator (@>) on the additional_details column, but no GIN index exists. Adding a GIN index will improve query performance, especially with large datasets. You can create the index using:
CREATE INDEX idx_plan_additional_details ON plan USING GIN (additional_details);🔗 Analysis chain
Consider adding an index on the additional_details column.
The JSONB containment operator (@>) can benefit from a GIN index on the additional_details column for better query performance, especially with large datasets.
Let's verify if an index already exists:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if a GIN index exists on the additional_details column # Search for CREATE INDEX statements in SQL files rg -l "CREATE.*INDEX.*ON.*plan.*additional_details.*GIN" --type sql # Alternative: Search for index-related comments rg -l "idx.*plan.*additional_details" --type sqlLength of output: 120
Script:
#!/bin/bash # Search for SQL migration files and their content fd -e sql | while read -r file; do echo "=== $file ===" cat "$file" done # Alternatively, search for any CREATE INDEX statements rg "CREATE.*INDEX.*plan" --type sql -A 2 -B 2Length of output: 126472
Script:
#!/bin/bash # Search for plan table creation and structure rg -B 5 -A 10 "CREATE TABLE.*plan" --type sql # Alternative: Search for additional_details column definition rg -B 2 -A 2 "additional_details.*jsonb" --type sqlLength of output: 27624
Script:
#!/bin/bash # Search for any index creation on plan.additional_details rg -i "create.*index.*plan.*additional_details" --type sql # Also search for any index creation on plan table rg -i "create.*index.*on.*plan" --type sqlLength of output: 475
health-services/plan-service/src/main/java/digit/config/ServiceConstants.java (1)
363-376: 🧹 Nitpick (assertive)
Group related constants together and add documentation.
The facility and security-related constants should be grouped together with similar constants, and documentation should explain their purpose.
Apply these improvements:
+ // Facility search parameter constants public static final String FACILITY_NAME_SEARCH_PARAMETER_KEY = "facilityName"; public static final String FACILITY_ID_SEARCH_PARAMETER_KEY = "facilityId"; public static final String FACILITY_STATUS_SEARCH_PARAMETER_KEY = "facilityStatus"; public static final String FACILITY_TYPE_SEARCH_PARAMETER_KEY = "facilityType"; - + + // Accessibility search parameter constants public static final String TERRAIN_CONDITION_SEARCH_PARAMETER_KEY = "accessibilityDetails|terrain|code"; - public static final String ROAD_CONDITION_SEARCH_PARAMETER_KEY = "accessibilityDetails|roadCondition|code"; - + + // Security search parameter constants public static final String SECURITY_Q1_SEARCH_PARAMETER_KEY = "securityDetails|1|code"; - public static final String SECURITY_Q2_SEARCH_PARAMETER_KEY = "securityDetails|2|code";Committable suggestion skipped: line range outside the PR's diff.
…down search, changes filterMap signature.
…down search, changes filterMap signature.
…down search, changes filterMap signature.
Summary by CodeRabbit
Based on the comprehensive changes across multiple files, here are the release notes:
New Features
Improvements
Bug Fixes
Technical Enhancements