Grievance worklist changes #178
Conversation
WalkthroughThis pull request refines the handling of grievance transactions by replacing a wildcard import with explicit Jakarta Persistence annotation imports in the transaction entity. Additionally, it introduces a new field in the grievance worklist DTO with corresponding constructor changes. A new repository method is added to fetch transaction data using native SQL, and the service implementation is updated to inject this repository and adjust data extraction logic accordingly. Furthermore, several query methods in the grievance data repository are modified to enhance specificity and reduce parameters. Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/java/com/iemr/common/repository/grievance/GrievanceTransactionRepo.java (1)
3-15: Consider returning a strongly typed projection or DTO instead of List<Object[]>.While returning a raw
List<Object[]>from a native query is functional, using an interface-based projection or a custom DTO reduces casting overhead and increases maintainability.src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java (1)
334-348: Refactor raw Object[] usage for readability.Fetching transactions via
grievanceTransactionRepo.getGrievanceTransactionworks, but consider typed queries or projections to minimize indexing errors and improve code clarity.src/main/java/com/iemr/common/dto/grivance/GrievanceWorklistDTO.java (1)
13-77: Consider additional improvements to the DTO designWhile the current changes are valid, several improvements could enhance code quality and maintainability:
- Add JavaDoc comments to explain the purpose of fields, especially clarifying the difference between
complaintIDandgrievanceId- Standardize field naming conventions for consistency (some fields use camelCase while others don't)
- Consider using enums instead of strings for fields like
severety(also appears misspelled - should be "severity")- Group related fields together for better readability
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/iemr/common/data/grievance/GrievanceTransaction.java(1 hunks)src/main/java/com/iemr/common/dto/grivance/GrievanceWorklistDTO.java(3 hunks)src/main/java/com/iemr/common/repository/grievance/GrievanceTransactionRepo.java(1 hunks)src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/main/java/com/iemr/common/data/grievance/GrievanceTransaction.java (1)
src/main/java/com/iemr/common/data/grievance/GrievanceDetails.java (1)
Entity(23-210)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: style-check / checkstyle
- GitHub Check: Build
- GitHub Check: Package-test
- GitHub Check: Analyze (java)
🔇 Additional comments (8)
src/main/java/com/iemr/common/data/grievance/GrievanceTransaction.java (1)
4-16: Use of explicit imports is a best practice.Replacing wildcard imports with explicit named imports fosters clarity and maintainability.
src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java (3)
34-34: Constructor-based injection of GrievanceTransactionRepo looks good.Injecting the new repository in the constructor is consistent with Spring best practices and keeps dependencies explicit. No issues here.
Also applies to: 48-48, 52-52, 56-56
300-301: Verify the type used for age casting.Casting
row[20]directly toLongcould cause aClassCastExceptionif the underlying DB column is mapped to a different type. Confirm that the result maps correctly toLong.
307-320: Possible confusion in column indexing.Line 330 reuses
row[13]forlastCallwhile line 319 uses the same index forlastModDate. Verify that both fields indeed reference the same column. If not, update the indexes accordingly.Also, ensure the field mapping aligns with the underlying query structure to avoid subtle data mismatch.
Also applies to: 328-330
src/main/java/com/iemr/common/dto/grivance/GrievanceWorklistDTO.java (4)
17-17: New field added for grievance identificationAdding the
grievanceIdfield of typeLongis a good addition that likely provides a more robust identifier for grievances compared to the String-basedcomplaintID. This change appears to support the enhanced grievance transaction handling mentioned in the PR summary.
49-49: Correctly initializing the new fieldThe new
grievanceIdfield is properly initialized in the constructor.
63-64: Parameter reordering reflected in assignmentsThe assignments for
firstNameandlastNamehave been correctly moved to reflect their new positions in the parameter list.
42-45:✅ Verification successful
Constructor signature changes need careful consideration
The constructor has been modified to include the new
grievanceIdparameter and reorder several parameters. While the implementation is correct, this change requires updating all places where this constructor is called to match the new signature.
🏁 Script executed:
#!/bin/bash # Find all instances where GrievanceWorklistDTO constructor is called # to verify they've been updated with the new parameter order echo "Searching for GrievanceWorklistDTO constructor usage..." rg "new GrievanceWorklistDTO\(" --type java -A 10 -B 2Length of output: 3581
Constructor Signature Verification – All Constructor Calls Match the New Order
- Verified that the parameterized constructor in
GrievanceWorklistDTO(now includinggrievanceIdas the second parameter) is correctly invoked insrc/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java.- Noted that the call in
src/main/java/com/iemr/common/controller/grievance/GrievanceController.javauses the no-argument constructor. Assuming that retaining a default constructor is intentional for error handling or similar cases, no changes appear necessary there.- Please ensure documentation clearly distinguishes the intended use between the parameterized and the default constructors.
| // Loop through the worklist data and format the response | ||
| for (Object[] row : worklistData) { | ||
| if (row == null || row.length < 30) | ||
| if (row == null || row.length < 22) |
There was a problem hiding this comment.
Potential off-by-one error in array length check.
Subsequent code references row[22] despite only ensuring row.length >= 22. For an array of length 22, row[22] would be out of range.
Consider changing the condition to if (row == null || row.length < 23) or remove references to row[22] if unintended.
There was a problem hiding this comment.
@coderabbitai the query returns only 22 columns. it should not be more than 22 columns.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
ravishanigarapu
force-pushed
the
develop
branch
from
1993622 to
0563e98
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/main/java/com/iemr/common/service/grievance/GrievanceDataSyncImpl.java (1)
258-305: 💡 Verification agent🧩 Analysis chain
Validate presence of the "transaction" key.
When fetchingJsonArray arr = transactionDetailsJson.get("transaction").getAsJsonArray();, no null-check is performed. If the key is missing or incorrectly named, a runtime exception may occur.
🏁 Script executed:
#!/bin/bash # Search for usage of "transaction" field to confirm we never store null or absent keys. rg -A 2 'get\("transaction"\)'Length of output: 414
Action Required: Add Null-Check for "transaction" Key
The current implementation in
src/main/java/com/iemr/common/service/grievance/GrievanceDataSyncImpl.java(lines 258-305) retrieves the"transaction"key without checking if it exists. This may lead to a runtime exception if the key is absent. Please add a null/absent check before callinggetAsJsonArray()to ensure the code handles missing or incorrectly named keys gracefully.
- File:
src/main/java/com/iemr/common/service/grievance/GrievanceDataSyncImpl.java- Location: Lines 258-305
♻️ Duplicate comments (1)
src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java (1)
292-296:⚠️ Potential issueOff-by-one error when referencing row[22].
You skip rows ifrow.length < 22, but you later accessrow[22]. This can lead to anArrayIndexOutOfBoundsException.- if (row == null || row.length < 22) + if (row == null || row.length < 23)
🧹 Nitpick comments (5)
src/main/java/com/iemr/common/controller/grievance/GrievanceController.java (2)
147-148: Consider returning an HTTP-aware ResponseEntity.
Changing fromResponseEntity<Map<String, Object>>toStringrestricts the ability to provide custom HTTP statuses, headers, and typed responses. If feasible, consider returningResponseEntity<String>instead, so that clients can handle status codes appropriately.
152-152: Remove or utilize the unused ObjectMapper instance.
ObjectMapper objectMapper = new ObjectMapper();is never used in this method. Minimizing unused variables can reduce confusion.- ObjectMapper objectMapper = new ObjectMapper();src/main/java/com/iemr/common/service/grievance/GrievanceDataSyncImpl.java (2)
125-131: Refactor the token check for clarity.
Having an emptyifblock is less readable. Using an early-return or inverting toif (token is expired) generate token;can be clearer:if (GRIEVANCE_AUTH_TOKEN != null && GRIEVANCE_TOKEN_EXP != null && GRIEVANCE_TOKEN_EXP > System.currentTimeMillis()) { - // do nothing } else { generateGrievanceAuthToken(); }
138-145: Setting Content-Length manually may be unnecessary.
In many cases,RestTemplatehandles theContent-Lengthheader automatically. Manually overriding it can cause issues if the request changes after setting the header.- headers.add("Content-Length", String.valueOf(contentLength));src/main/java/com/iemr/common/repository/grievance/GrievanceDataRepo.java (1)
47-135: Consistent architectural approach to query conditionsAll the changes in this file follow a consistent pattern of removing providerServiceMapID from query conditions, suggesting a deliberate architectural decision. This simplifies the queries and makes the code more maintainable, assuming that complaintID, beneficiaryRegID, and userID together form a sufficient unique key.
When making such a significant change to query conditions across multiple methods, consider adding a comment or documentation update explaining the rationale behind removing providerServiceMapID from query conditions. This would help future developers understand why this decision was made.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/com/iemr/common/controller/grievance/GrievanceController.java(4 hunks)src/main/java/com/iemr/common/dto/grivance/GrievanceWorklistDTO.java(4 hunks)src/main/java/com/iemr/common/repository/grievance/GrievanceDataRepo.java(3 hunks)src/main/java/com/iemr/common/service/grievance/GrievanceDataSyncImpl.java(6 hunks)src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/iemr/common/dto/grivance/GrievanceWorklistDTO.java
🔇 Additional comments (16)
src/main/java/com/iemr/common/controller/grievance/GrievanceController.java (3)
3-8: Imports look fine.
These imported classes support JSON serialization (java.lang.reflect.Type,java.text.SimpleDateFormat, etc.) without obvious issues.
27-35: Jackson and Gson library imports are valid.
No issues flagged with the newly introduced Jackson and Gson dependencies.
178-188: Gson serialization with custom date format looks good.
The custom date serializer ensures dates are returned in"yyyy-MM-dd HH:mm:ss"format. If privacy regulations require masking of sensitive data (e.g., phone numbers), consider applying it within this serialization step. Otherwise, this implementation is acceptable.src/main/java/com/iemr/common/service/grievance/GrievanceDataSyncImpl.java (2)
133-136: Creation of HTTP headers is standard.
The headers (Content-Type,User-Agent,AUTHORIZATION) are well-defined and align with typical REST usage.
488-493: Defensive checks for null recordSet elements are commendable.
These null-safe checks prevent unintended NullPointerExceptions. This is a good practice.src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java (4)
34-34: Repo injection looks correct.
The addition ofGrievanceTransactionRepoand its injection in the constructor appear consistent with the existing design. No immediate concerns.Also applies to: 48-48, 52-52, 56-56
298-303: Long-based age handling is acceptable.
Switching from a floating type to a long integer for age appears more consistent. This avoids floating point issues and clarifies the usage of age as an integer-based measure.
334-349: Transaction retrieval and mapping are structured well.
Fetching transactions viagrievanceTransactionRepo.getGrievanceTransactionand mapping toGrievanceTransactionDTOis clear and maintainable.
476-486: Remarks fetching logic is sound.
The code gracefully falls back to alternative sources if remarks are null. No immediate issues found here.src/main/java/com/iemr/common/repository/grievance/GrievanceDataRepo.java (7)
47-48: Filtered out completed grievances from the count queryThe addition of
grievance.isCompleted=falseto the query ensures that only active (not completed) grievances are counted, which aligns with the purpose of workload tracking and management.
102-103: Removed providerServiceMapID from overloaded resolution methodSimilar to the previous change, the providerServiceMapID condition has been removed from this overloaded method as well, maintaining consistency between both versions of updateComplaintResolution.
112-112: Added complaintResolution to getCallCounter result setThe query now returns additional information (complaint resolution status) along with call counter and retry status, which provides more context in a single database call.
117-118: Removed providerServiceMapID from completed status updateThe query has been simplified to only use complaintID, userID, and beneficiaryRegID as conditions for updating the completed status. This follows the pattern of other method changes in this file.
123-123: Removed providerServiceMapID parameter from method signatureThe parameter has been removed in line with the changes to the query condition, maintaining consistency between the method signature and implementation.
88-89:❓ Verification inconclusive
Removed providerServiceMapID condition from complaint resolution update
The query has been simplified by removing the providerServiceMapID condition. This potentially broadens the scope of which records can be updated.
🏁 Script executed:
#!/bin/bash # Verify if providerServiceMapID is used elsewhere to uniquely identify grievances # or if the combination of complaintID, beneficiaryRegID, and userID is sufficient # Check if there are any instances where two grievances share the same complaintID, beneficiaryRegID, and userID but have different providerServiceMapID rg -A 5 "complaintID.+beneficiaryRegID.+userID.+providerServiceMapID" --type javaLength of output: 81
Action Required: Confirm Uniqueness Conditions in Grievance Update Query
The removal of theproviderServiceMapIDcondition simplifies the query to use onlycomplaintID,beneficiaryRegID, anduserIDfor matching records. Although the initial automated search did not reveal any instances where duplicate grievances (differing byproviderServiceMapID) occur, the evidence remains inconclusive. Please manually verify that these three fields together uniquely identify a grievance to ensure the change won’t inadvertently broaden the update scope.
- Confirm that no grievances exist with the same
complaintID,beneficiaryRegID, anduserIDbut differentproviderServiceMapIDvalues.- Ensure the simplified query does not affect additional records unexpectedly.
128-129:✅ Verification successful
Removed providerServiceMapID from call counter update
This change is consistent with the pattern of simplifying queries throughout this file by relying on the combination of complaintID, beneficiaryRegID, and userID to uniquely identify grievance records.
🏁 Script executed:
#!/bin/bash # Check for any potential issues with concurrent updates after removing providerServiceMapID # Look for other places where these fields are used in combination rg -A 5 "complaintID.+beneficiaryRegID.+userID" --type java # Check if there are any service methods that might expect providerServiceMapID to be part of the update condition rg -A 10 "updateCallCounter|updateCompletedStatusInCall|updateComplaintResolution" --type javaLength of output: 18904
Verified: Simplified update query aligns with codebase patterns
The removal of
providerServiceMapIDfrom the call counter update inGrievanceDataRepo.javahas been confirmed. Our searches indicate that all related update methods across the repository and service implementations rely on the combination ofcomplaintID,beneficiaryRegID, anduserIDto uniquely identify grievance records. This consistent approach minimizes redundancy and potential concurrency issues.
- The query now solely uses
complaintID,beneficiaryRegID, anduserIDfor record matching.- Similar patterns in
GrievanceDataSyncImpl.java,GrievanceHandlingServiceImpl.java, and Everwell service classes further reinforce this change.
| if (response != null && response.hasBody()) { | ||
| JSONObject obj = new JSONObject(response.getBody()); | ||
| if (obj != null && obj.has("data") && obj.has("status") && obj.getInt("status") == 200) { | ||
| logger.info("Grievance data details response received successfully "); | ||
|
|
||
| String responseStr = response.getBody(); | ||
| JsonObject jsnOBJ = new JsonObject(); | ||
| JsonParser jsnParser = new JsonParser(); | ||
| JsonElement jsnElmnt = jsnParser.parse(responseStr); | ||
| jsnOBJ = jsnElmnt.getAsJsonObject(); | ||
| int total = jsnOBJ.get("total").getAsInt(); | ||
| String GrievanceUrl = updateGrievanceDetails.replace("PageNumber", String.valueOf(total)); | ||
| ResponseEntity<String> grievienceResponse = restTemplate.exchange(GrievanceUrl, HttpMethod.POST, | ||
| request, String.class); | ||
| String respStr = grievienceResponse.getBody(); | ||
|
|
||
| JsonElement jsnElmntResp = jsnParser.parse(respStr); | ||
| jsnOBJ = jsnElmntResp.getAsJsonObject(); | ||
| JsonArray grievanceJsonDataArray = jsnOBJ.getAsJsonArray("data"); | ||
| if (Integer.parseInt(jsnOBJ.get("total").toString()) > 0) { | ||
| for (JsonElement grievanceElement : grievanceJsonDataArray) { | ||
| String formattedComplaintId = null; | ||
| try { | ||
| JsonObject grievanceJsonData = grievanceElement.getAsJsonObject(); | ||
| GrievanceDetails grievance = new GrievanceDetails(); | ||
| String complaintId = grievanceJsonData.get("complainId").getAsString(); | ||
| formattedComplaintId = complaintId.replace("\\/", "/"); | ||
| boolean complaintExists = grievanceDataRepo.existsByComplaintId(formattedComplaintId); | ||
| if (complaintExists) { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify total and data fields exist in every response.
The code assumes "status" == 200 and that "total" / "data" fields exist. Unexpected or empty responses could cause exceptions at runtime.
🏁 Script executed:
#!/bin/bash
# Checking usage of fields "total" and "data" in the grievance responses
# to ensure robust error handling.
rg -A 5 'obj\.has\("total"\)'
rg -A 5 'obj\.has\("data"\)'Length of output: 2133
Attention: Add explicit validation for the total field in the grievance response.
While the code correctly checks for the presence of "data" and "status" (with "status" == 200), it directly uses the "total" field without verifying its existence. This could lead to runtime exceptions if "total" is missing or malformed in the response. I recommend adding a conditional check to ensure that "total" exists before attempting to use its value. For instance:
- In src/main/java/com/iemr/common/service/grievance/GrievanceDataSyncImpl.java (lines 150-178), verify that
jsnOBJ.has("total")(or its equivalent) is true before callingget("total")and proceeding with subsequent operations. - Ensure similar validations are applied consistently across related response handling utilities (e.g., compare with the check in src/main/java/com/iemr/common/utils/response/OutputResponse.java).
Implementing this check will prevent exceptions on unexpected or empty responses.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java (2)
478-487: Clarify resolution-based remarks retrieval logic.
Right now, the code only checks ifcomplaintResolutionis null or not. If the business needs a distinctly different flow for “resolved” vs. “unresolved,” consider an explicit comparison for better clarity.
522-522: Consider returning an empty string instead of “No remarks found.”
A literal message might confuse consumers if they parse remarks programmatically. An empty string could be clearer.- return "No remarks found"; + return "";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (java)
- GitHub Check: Build
- GitHub Check: Package-test
🔇 Additional comments (9)
src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java (9)
34-34: No concerns for the import statement.
48-48: New dependency injection looks good.
No issues noted with adding a final field for theGrievanceTransactionRepo.
292-292: Possible off-by-one error in the row length check.
This comment matches a past review. You’re ensuringrow.length >= 22, but the code referencesrow[22]in nearby lines, which would be out of range if the array length is exactly 22.
306-333: Check repeated use ofrow[13].
You’re assigningrow[13]to bothlastModDateandlastCall. If this is unintentional, consider adding a separate column index forlastCall.
336-349: Verify potential null return fromgetGrievanceTransaction.
If the repository method can returnnull, a NullPointerException could occur. Otherwise, returning an empty list is safe. Please confirm expected behavior.
406-411: Complaint resolution update logic looks solid.
The checks onupdateCountare appropriate, and both variants handle remarks or no remarks consistently.
517-518: Index check is well-handled.
You guard against emptiness before accessingbenCallResults.get(0). No issues found here.
301-302:❓ Verification inconclusive
Validate the type of
row[20].
Castingrow[20]to aLongcould raise aClassCastExceptionif the stored procedure doesn’t always return a numeric type for that index.Consider verifying the stored procedure schema:
🏁 Script executed:
#!/bin/bash # Searches for the definition/usage of getGrievanceWorklistData to confirm the type at index 20 rg -A 5 "getGrievanceWorklistData" fd -e sql -x cat {}Length of output: 1201
Action: Verify and Safeguard Row Index 20 Type Conversion
The verification confirms that the method retrieving worklist data from the database returns a raw
List<Object[]>. However, there isn’t conclusive evidence from the stored procedure's schema about the type at index 20. To avoid a potentialClassCastException, please ensure that:
- The stored procedure indeed returns a numeric type (preferably a Long) at index 20.
- If the returned type might vary (e.g., could be an Integer, BigDecimal, or even null), add an
instanceofcheck or safely convert to aLong(for example, by converting aNumberusing.longValue()).- Consider updating the code at
src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.javato handle cases whererow[20]may not be aLong.
52-56:✅ Verification successful
Verify external constructor references.
Ensure that any existing code instantiatingGrievanceHandlingServiceImplis updated to pass the newly requiredGrievanceTransactionRepo.You can run this script to locate all constructor invocations:
🏁 Script executed:
#!/bin/bash # Searches for all calls to the GrievanceHandlingServiceImpl constructor rg "GrievanceHandlingServiceImpl\s*\("Length of output: 248
Constructor Parameter Update Verified
We ran the search and confirmed that the only instance of a GrievanceHandlingServiceImpl constructor call is the definition within the class itself (in src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java). There are no direct external instantiation calls (e.g., via
new GrievanceHandlingServiceImpl(...)) in the codebase. If this class is being injected via a dependency injection framework (such as Spring), please double-check that the DI configuration has been updated to supply the newGrievanceTransactionRepoparameter.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java (1)
299-337:⚠️ Potential issuePotential out-of-bounds access for
row[22].Since the code references
row[22](line 336) but only checks forrow.length < 22(line 299), arrays of exactly length 22 will cause an out-of-bounds error. Increase the check to< 23if you truly need to accessrow[22], or remove any references to the 23rd column if the query only returns 22 columns.-if (row == null || row.length < 22) +if (row == null || row.length < 23) { logger.warn("invalid row data received"); continue; }
🧹 Nitpick comments (5)
src/main/java/com/iemr/common/repository/callhandling/BeneficiaryCallRepository.java (1)
194-195: Prefer returning a list of strings.Since only the
remarkscolumn is being selected here, consider returningList<String>rather thanList<Object[]>. This will make the usage in the calling code more straightforward and eliminate the need to handle object arrays for a single string value.-@Query("SELECT b.remarks FROM BeneficiaryCall b WHERE b.benCallID = :benCallID") -List<Object[]> fetchBenCallRemarks(@Param("benCallID") Long benCallID); +@Query("SELECT b.remarks FROM BeneficiaryCall b WHERE b.benCallID = :benCallID") +List<String> fetchBenCallRemarks(@Param("benCallID") Long benCallID);src/main/java/com/iemr/common/controller/grievance/GrievanceController.java (2)
140-141: Consider returning a typed response.Returning raw JSON strings can make status code handling and client-side error interpretation more cumbersome. If possible, consider returning
ResponseEntity<?>to leverage HTTP status codes more effectively, or ensure the client is prepared for a raw string response.
145-146: Remove or utilize the unusedObjectMapper.An
ObjectMapperis instantiated but never used. If GSON is the preferred serializer, you may remove this redundant code to avoid confusion.-ObjectMapper objectMapper = new ObjectMapper();src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java (2)
14-14: Consider usingorg.apache.commons.lang3.StringUtils.Using
org.apache.commons.lang.StringUtilsis generally fine, butorg.apache.commons.lang3.StringUtilsis the more modern library with additional improvements and ongoing support.
483-486: UseisBlank()if whitespace-only values should be treated as empty.Using
StringUtils.isEmpty()is fine if you only want to check fornullor zero-length strings. If you also want to treat whitespace as empty, considerisBlank()or trimming the input first.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/iemr/common/controller/grievance/GrievanceController.java(4 hunks)src/main/java/com/iemr/common/repository/callhandling/BeneficiaryCallRepository.java(1 hunks)src/main/java/com/iemr/common/repository/grievance/GrievanceDataRepo.java(3 hunks)src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/iemr/common/repository/grievance/GrievanceDataRepo.java
🔇 Additional comments (5)
src/main/java/com/iemr/common/controller/grievance/GrievanceController.java (1)
171-181: JSON date serialization looks good.Registering a custom
JsonSerializer<Date>with GSON provides flexible control over date formatting. This approach is clear and consistent.src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java (4)
41-41: Injection ofGrievanceTransactionRepois correct.Introducing
GrievanceTransactionRepoin the service constructor and storing it as a final field is a clean approach, ensuring proper dependency injection.Also applies to: 55-55, 59-59, 63-63
341-355: Adding transaction data looks good.Retrieving the transaction records from
GrievanceTransactionRepoand populating them into the DTO is logically sound, ensuring the main grievance object includes all pertinent transaction details.
411-411: Check parameter ordering for complaint resolution update.These calls to
updateComplaintResolutionappear correct but rely on a carefully ordered parameter list. Make sure the target repository method signature expects the same argument ordering to avoid subtle misalignment.Also applies to: 416-416
524-532: Fetching remarks fromBeneficiaryCallis well-handled.Verifying the query results and returning "No remarks found" in the absence of data is a clean approach. The null checks prevent potential
NullPointerExceptions.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java (2)
342-356: Consider minimizing repeated queries.
CallinggrievanceTransactionRepo.getGrievanceTransaction(...)inside the loop may cause extra DB calls for each row, potentially impacting performance at scale. A single batched query or an IN clause could be more efficient.
484-487: ConsiderisBlank()if whitespace-only remarks matter.
StringUtils.isEmpty()ignores only null/empty strings, but not whitespace-only content. If whitespace is significant, considerisBlank().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java(8 hunks)
🔇 Additional comments (4)
src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java (4)
3-14: Imports appear consistent.
These newly added imports forjava.lang.reflect.Typeandorg.apache.commons.lang.StringUtilsalign with the usage of custom Gson serialization and string checks, respectively. No conflicts or issues observed.
41-63: Repository integration looks good.
Introducing and injectingGrievanceTransactionRepoin the constructor follows recommended constructor injection practices. The code is clear and consistent.
299-299: Potential off-by-one error (duplicate).
You are referencingrow[22]below but the condition only checksrow.length < 22. An array with length 22 has valid indices0..21, makingrow[22]out of bounds.
305-308: Verify the type ofrow[20].
Castingrow[20]toLongmay causeClassCastExceptionif this column is returned as a different numeric type (e.g.,Integer) from the query.Would you like to confirm the actual database type of this column and ensure correct casting? You can run a script to inspect the data type or ask the DB administrator for clarification.
| (Long) row[1], //grievanceId | ||
| (String) row[2], // subjectOfComplaint | ||
| (String) row[3], // complaint | ||
| (Long) row[4], // beneficiaryRegID | ||
| (Integer) row[5],// providerServiceMapID | ||
| (String) row[6], // primaryNumber | ||
| (String) row[7], // severety | ||
| (String) row[8], // state | ||
| (Integer) row[9],// userId | ||
| (Boolean) row[10],// deleted | ||
| (String) row[11],// createdBy | ||
| (Timestamp) row[12], // createdDate | ||
| (Timestamp) row[13], // lastModDate | ||
| (Boolean) row[14], // isCompleted | ||
|
|
||
| (String) row[15], // firstName | ||
| (String) row[16], // lastName | ||
| (String) row[17], // gender | ||
| (String) row[18], // district | ||
| (Long) row[19], // beneficiaryID | ||
| ageFormatted, | ||
| (Boolean) row[26], // retryNeeded | ||
| (Integer) row[27], // callCounter | ||
| (Timestamp) row[18] //lastCall yet to fill | ||
| (Boolean) row[21], // retryNeeded | ||
| (Integer) row[22], // callCounter | ||
| (Timestamp) row[13] //lastCall | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Check repeated usage of row[13].
You're setting both lastModDate and lastCall from row[13]. If these columns differ in the DB, referencing the same index may be incorrect. Verify that the intended column for "lastCall" is also at index 13.
🏁 Script executed:
#!/bin/bash
# Validate whether the 'lastCall' DB column is indeed located at index 13.
# We'll look for references in the codebase where 'row[13]' is assigned for two different fields.
rg -A 4 'row\[13\]'Length of output: 1231
Attention: Potential Incorrect Database Column Mapping
In src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java, the code assigns (Timestamp) row[13] to both the lastModDate and lastCall fields. Since the database schema likely distinguishes between these two values, please verify whether lastCall should reference a different column index. If the intended DB column for lastCall differs from lastModDate, the mapping should be updated accordingly.
- Action Required:
- Check the DB schema to confirm the correct column index for the
lastCallfield. - Update the index for
lastCallif it is intended to come from a column other than what is currently referenced byrow[13].
- Check the DB schema to confirm the correct column index for the
| Long benCallID = grievance.getBenCallID(); | ||
| // Query t_bencall to fetch remarks based on benRegId | ||
| List<Object[]> benCallResults = beneficiaryCallRepo.fetchBenCallRemarks(beneficiaryRegID); | ||
| List<Object[]> benCallResults = beneficiaryCallRepo.fetchBenCallRemarks(benCallID); | ||
|
|
||
| if (benCallResults != null && !benCallResults.isEmpty()) { | ||
| return (String) benCallResults.get(0)[0]; // Fetch the remarks | ||
| if(null != benCallResults.get(0) && null != benCallResults.get(0)[0]) | ||
| return (String) benCallResults.get(0)[0]; // Fetch the remarks | ||
| } | ||
| } | ||
|
|
||
| return "No remarks found in t_bencall"; | ||
| return "No remarks found"; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify benCallID usage to avoid null scenarios.
Here, fetchBenCallRemarks(benCallID) is invoked without ensuring benCallID is non-null. A null or missing benCallID could cause a query issue or unexpected results.
🏁 Script executed:
#!/bin/bash
# Verify if benCallID can be null or unset, potentially leading to problems in fetchBenCallRemarks.
rg -A 3 'benCallID' Length of output: 65151
Action: Introduce an explicit null-check for benCallID before querying remarks
In the code within GrievanceHandlingServiceImpl.java at lines 523–533, benCallID is retrieved via grievance.getBenCallID() and immediately passed to beneficiaryCallRepo.fetchBenCallRemarks(benCallID) without verifying its non-nullity. Although many parts of the codebase assume a non-null benCallID, if for any reason grievance.getBenCallID() can return null, this may result in a query error or produce unexpected results. Consider adding a defensive null-check—such as returning a default message like "No remarks found" when benCallID is null—to ensure robust handling.
📋 Description
JIRA ID: AMM-1305, AMM-1333,AMM-1303
Even after 3 attempts, record is displaying in the grievance worklist
✅ Type of Change
ℹ️ Additional Information
Please describe how the changes were tested, and include any relevant screenshots, logs, or other information that provides additional context.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor