Skip to content
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

Merged
merged 7 commits into from
Jan 28, 2025

Conversation

Priyanka-eGov
Copy link
Collaborator

@Priyanka-eGov Priyanka-eGov commented Jan 27, 2025

Summary by CodeRabbit

Based on the comprehensive changes across multiple files, here are the release notes:

  • New Features

    • Added support for additional fields in plans and census records
    • Introduced new search capabilities for plan facilities
    • Enhanced mixed strategy operations with more flexible configuration
    • Improved Excel file processing and localization
  • Improvements

    • Expanded search criteria for facilities, including terrain and security conditions
    • Added more robust error handling and validation
    • Improved data mapping and filtering mechanisms
    • Enhanced Excel styling and column width adjustments
  • Bug Fixes

    • Updated pagination and offset handling for MDMS data retrieval
    • Fixed boundary code and facility mapping processes
  • Technical Enhancements

    • Refactored utility classes for better modularity
    • Updated data models to support more complex scenarios
    • Improved JSON processing and additional details handling

…down search, changes filterMap signature.
…down search, changes filterMap signature.
…down search, changes filterMap signature.
Copy link
Contributor

coderabbitai bot commented Jan 27, 2025

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This 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

File/Group Change Summary
ServiceConstants.java Added new search parameter constants for facility, terrain, road conditions, and security details
PlanRepositoryImpl.java Updated to include additional fields when converting plan requests
PlanQueryBuilder.java Modified SQL queries to include additional fields from plan_additional_field table
PlanRowMapper.java Added method to handle additional fields during data extraction
Data Models Introduced new fields like additionalFields in Plan, PlanDTO, and Census classes
Search Criteria Enhanced PlanSearchCriteria with new search parameters like facilityIds, terrain, securityQ1
Utilities Added new utility classes like MixedStrategyUtil, OutputEstimationGenerationUtil, PlanFacilityUtil
Database Migration Created new plan_additional_field table to support additional field storage

Sequence Diagram

Loading
sequenceDiagram
    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

  • sathishp-eGov
  • kavi-egov

Poem

🐰 A Rabbit's Ode to Plan Service Delight

In fields of data, fresh and bright,
Additional details take their flight,
Search criteria dance with glee,
Enriched and flexible, now we see!

Hop, hop, hooray for microplans so neat! 🌟


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@Priyanka-eGov Priyanka-eGov changed the base branch from master to microplanning-v0.2 January 27, 2025 12:56
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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:

  1. Campaign details are not re-validated when inactive records are reactivated
  2. No safeguards exist to prevent reactivation of records with invalid campaign details
  3. 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:

  1. What happens when an inactive record is later reactivated? The campaign details might be invalid at that point.
  2. Are there any system components that assume all records (active/inactive) are validated?

Consider these alternatives:

  1. Always validate but handle validation failures differently for inactive records
  2. 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

  1. The name could be more descriptive since it specifically maps boundary codes to a "fixed post" flag.
  2. 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 new boundaryCodeToFixedPostMap. 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 unintended NullPointerException at runtime.


67-85: 🛠️ Refactor suggestion

Add null safety checks for distribution process fields.
Though the MDMS data typically ensures non-null fields, calls to logic.getRegistrationProcess() and logic.getDistributionProcess() may still cause NullPointerException if the data is malformed. Consider adding checks or ensuring all fields are always non-null in MixedStrategyOperationLogic.


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 in fetchMixedStrategyOperationLogicFromMDMS directly accesses array index without validation, which could cause ArrayIndexOutOfBoundsException. Additionally, a null check on getTenantId() 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 cause ArrayIndexOutOfBoundsException 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.java

Length 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 only getAdminConsoleModuleDetail(). 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 issue

Verify safe JSON parsing of master data.
Using JsonUtils.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.
Use String.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 issue

Update filtersMap type to support multi-select values.

The filtersMap type should be Map<String, Set<String>> instead of Map<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 issue

Improve error handling in search method.

Catching a generic Exception and returning null silently is not a good practice. Consider:

  1. Using specific exceptions
  2. Propagating the error to the caller
  3. 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 of List<String> would be more appropriate for serviceBoundaries.

-    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.

  1. Add regex validation for hex color format
  2. 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:

  1. Audit fields (created_by, created_time, etc.)
  2. Tenant_id for multi-tenancy support
  3. 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 as paf 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 sql

Length 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 2

Length 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 sql

Length 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 sql

Length 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.
@Priyanka-eGov Priyanka-eGov merged commit 33c8666 into microplanning-v0.2 Jan 28, 2025
2 of 4 checks passed
@Priyanka-eGov Priyanka-eGov deleted the HCMPRE-2212 branch January 28, 2025 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant