AMM-1146 | API to save complaint resolution and remarks for a grievance #163
Conversation
latest pull
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
AMM-1137 | after
latest pull
AMM-1076 and AMM-1078 (PSMRI#146)
latest pull 19 jan
latest pull
updating upload artifact version to v4
…rp78/Common-API.git into feature/version/upgrade
PULL adding newly added grievance related properties to properties file (#…
AMM-1145 | API to fetch grievance outbound worklist (PSMRI#161)
WalkthroughThis pull request modifies the grievance management system by updating the Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
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: 8
🧹 Nitpick comments (1)
src/main/java/com/iemr/common/service/grievance/GrievanceHandlingService.java (1)
23-23: Consider using more specific exceptions.The method throws a generic
Exceptionwhich is too broad. Consider declaring specific exceptions that could occur during JSON parsing and database operations.- public String saveComplaintResolution(String request) throws Exception; + public String saveComplaintResolution(String request) throws JSONException, IEMRException;
📜 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(1 hunks)src/main/java/com/iemr/common/data/grievance/GrievanceDetails.java(4 hunks)src/main/java/com/iemr/common/repository/grievance/GrievanceDataRepo.java(1 hunks)src/main/java/com/iemr/common/service/grievance/GrievanceHandlingService.java(1 hunks)src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java(1 hunks)
⏰ 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)
| @Modifying | ||
| @Query("UPDATE GrievanceDetails g SET g.complaintResolution = :complaintResolution " | ||
| + "WHERE g.complaintID = :complaintID AND g.beneficiaryRegID = :beneficiaryRegID AND g.providerServiceMapID = :providerServiceMapID" | ||
| + " AND g.assignedUserID = :assignedUserID") | ||
| @Transactional | ||
| int updateComplaintResolution(@Param("complaintResolution") String complaintResolution, | ||
| @Param("complaintID") String complaintID, | ||
| @Param("beneficiaryRegID") Long beneficiaryRegID, | ||
| @Param("providerServiceMapID") Integer providerServiceMapID, | ||
| @Param("assignedUserID") Integer assignedUserID); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation and update audit fields in the overloaded method.
The overloaded method should also update audit fields and include proper documentation explaining when to use this variant.
+/**
+ * Updates only the complaint resolution without remarks
+ * @param complaintResolution The resolution text
+ * @param complaintID The ID of the complaint
+ * @param beneficiaryRegID The beneficiary registration ID
+ * @param providerServiceMapID The provider service map ID
+ * @param assignedUserID The ID of the assigned user
+ * @param modifiedBy The user making the modification
+ * @return Number of records updated
+ */
@Modifying
@Query("UPDATE GrievanceDetails g SET g.complaintResolution = :complaintResolution "
+ "WHERE g.complaintID = :complaintID AND g.beneficiaryRegID = :beneficiaryRegID AND g.providerServiceMapID = :providerServiceMapID"
- + " AND g.assignedUserID = :assignedUserID")
+ + " AND g.assignedUserID = :assignedUserID AND g.deleted = false")
@Transactional
int updateComplaintResolution(@Param("complaintResolution") String complaintResolution,
@Param("complaintID") String complaintID,
@Param("beneficiaryRegID") Long beneficiaryRegID,
@Param("providerServiceMapID") Integer providerServiceMapID,
- @Param("assignedUserID") Integer assignedUserID);
+ @Param("assignedUserID") Integer assignedUserID,
+ @Param("modifiedBy") String modifiedBy);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@srishtigrp78 Thanks for making those improvements! The documentation and audit fields will help maintain better code quality.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| @Modifying | ||
| @Query("UPDATE GrievanceDetails g SET g.complaintResolution = :complaintResolution, g.remarks = :remarks " | ||
| + "WHERE g.complaintID = :complaintID AND g.beneficiaryRegID = :beneficiaryRegID AND g.providerServiceMapID = :providerServiceMapID" | ||
| + " AND g.assignedUserID = :assignedUserID") | ||
| @Transactional | ||
| int updateComplaintResolution(@Param("complaintResolution") String complaintResolution, | ||
| @Param("remarks") String remark, | ||
| @Param("complaintID") String complaintID, | ||
| @Param("beneficiaryRegID") Long beneficiaryRegID, | ||
| @Param("providerServiceMapID") Integer providerServiceMapID, | ||
| @Param("assignedUserID") Integer assignedUserID); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update audit fields in the complaint resolution query.
The update query should also set the audit fields (modifiedBy, lastModDate) to track when and by whom the resolution was updated.
@Query("UPDATE GrievanceDetails g SET g.complaintResolution = :complaintResolution, g.remarks = :remarks "
+ "WHERE g.complaintID = :complaintID AND g.beneficiaryRegID = :beneficiaryRegID AND g.providerServiceMapID = :providerServiceMapID"
- + " AND g.assignedUserID = :assignedUserID")
+ + " AND g.assignedUserID = :assignedUserID AND g.deleted = false")
@Transactional
int updateComplaintResolution(@Param("complaintResolution") String complaintResolution,
@Param("remarks") String remark,
@Param("complaintID") String complaintID,
@Param("beneficiaryRegID") Long beneficiaryRegID,
@Param("providerServiceMapID") Integer providerServiceMapID,
- @Param("assignedUserID") Integer assignedUserID);
+ @Param("assignedUserID") Integer assignedUserID,
+ @Param("modifiedBy") String modifiedBy);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
updated audit fields
There was a problem hiding this comment.
🧩 Analysis chain
@srishtigrp78 Thank you for updating the audit fields! This will help maintain better tracking of changes to the grievance resolutions.
🏁 Script executed:
#!/bin/bash
# Check the latest changes in the repository for the GrievanceDataRepo file
git show HEAD:src/main/java/com/iemr/common/repository/grievance/GrievanceDataRepo.java
Length of output: 5546
@srishtigrp78 I don't see the audit fields (modifiedBy, lastModDate) in the updateComplaintResolution methods yet. Could you please ensure to add them along with the deleted check? Here's what needs to be updated:
@Modifying
@Query("UPDATE GrievanceDetails g SET g.complaintResolution = :complaintResolution, g.remarks = :remarks, " +
"g.modifiedBy = :modifiedBy, g.lastModDate = CURRENT_TIMESTAMP " +
"WHERE g.complaintID = :complaintID AND g.beneficiaryRegID = :beneficiaryRegID " +
"AND g.providerServiceMapID = :providerServiceMapID AND g.assignedUserID = :assignedUserID " +
"AND g.deleted = false")
@Transactional
int updateComplaintResolution(@Param("complaintResolution") String complaintResolution,
@Param("remarks") String remark,
@Param("complaintID") String complaintID,
@Param("beneficiaryRegID") Long beneficiaryRegID,
@Param("providerServiceMapID") Integer providerServiceMapID,
@Param("assignedUserID") Integer assignedUserID,
@Param("modifiedBy") String modifiedBy);The same changes should be applied to the second updateComplaintResolution method as well. Let me know if you need any clarification!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| @Expose | ||
| @Column(name = "ComplaintResolution") | ||
| private String complaintResolution; | ||
|
|
||
| @Expose | ||
| @Column(name = "Remarks") | ||
| //@Size(max = 5000, message = "Remarks cannot exceed 5000 characters") | ||
| private String remarks; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation constraints and documentation for new fields.
The new fields should have proper validation constraints and documentation.
+/**
+ * Resolution provided for the complaint
+ */
@Expose
@Column(name = "ComplaintResolution")
+@NotBlank(message = "Complaint resolution is required")
+@Size(max = 5000, message = "Complaint resolution cannot exceed 5000 characters")
private String complaintResolution;
+/**
+ * Additional remarks about the complaint resolution
+ */
@Expose
@Column(name = "Remarks")
-//@Size(max = 5000, message = "Remarks cannot exceed 5000 characters")
+@Size(max = 5000, message = "Remarks cannot exceed 5000 characters")
private String remarks;📝 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.
| @Expose | |
| @Column(name = "ComplaintResolution") | |
| private String complaintResolution; | |
| @Expose | |
| @Column(name = "Remarks") | |
| //@Size(max = 5000, message = "Remarks cannot exceed 5000 characters") | |
| private String remarks; | |
| /** | |
| * Resolution provided for the complaint | |
| */ | |
| @Expose | |
| @Column(name = "ComplaintResolution") | |
| @NotBlank(message = "Complaint resolution is required") | |
| @Size(max = 5000, message = "Complaint resolution cannot exceed 5000 characters") | |
| private String complaintResolution; | |
| /** | |
| * Additional remarks about the complaint resolution | |
| */ | |
| @Expose | |
| @Column(name = "Remarks") | |
| @Size(max = 5000, message = "Remarks cannot exceed 5000 characters") | |
| private String remarks; |
There was a problem hiding this comment.
@srishtigrp78 Understood, I'll refrain from suggesting validation constraints for these fields.
✏️ Learnings added
Learnt from: srishtigrp78
PR: PSMRI/Common-API#163
File: src/main/java/com/iemr/common/data/grievance/GrievanceDetails.java:111-118
Timestamp: 2025-02-21T09:49:46.253Z
Learning: The `complaintResolution` and `remarks` fields in GrievanceDetails class are intentionally kept without validation constraints (@NotBlank, @Size) as per the team's decision.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| @PostMapping(value = "/saveComplaintResolution", consumes = MediaType.APPLICATION_JSON, produces = MediaType.APPLICATION_JSON, headers = "Authorization") | ||
| public String saveComplaintResolution(@RequestBody String request) { | ||
| OutputResponse response = new OutputResponse(); | ||
| try { | ||
| response.setResponse(grievanceHandlingService.saveComplaintResolution(request)); | ||
| } catch (Exception e) { | ||
| logger.error("saveComplaintResolution failed with error " + e.getMessage(), e); | ||
| response.setError(e); | ||
| } | ||
| return response.toString(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add API documentation and improve response type.
The endpoint needs proper documentation and could benefit from a more specific response type.
+@Operation(summary = "Save complaint resolution and remarks",
+ description = "Updates the resolution and optional remarks for a grievance")
@PostMapping(value = "/saveComplaintResolution", consumes = MediaType.APPLICATION_JSON, produces = MediaType.APPLICATION_JSON, headers = "Authorization")
- public String saveComplaintResolution(@RequestBody String request) {
+ public ResponseEntity<OutputResponse> saveComplaintResolution(
+ @Param(value = "{\"complaintID\":\"Complaint ID string\", " +
+ "\"complaintResolution\":\"Resolution text\", " +
+ "\"remarks\":\"Optional remarks\"}")
+ @RequestBody String request) {
OutputResponse response = new OutputResponse();
try {
response.setResponse(grievanceHandlingService.saveComplaintResolution(request));
+ return ResponseEntity.ok(response);
} catch (Exception e) {
logger.error("saveComplaintResolution failed with error " + e.getMessage(), e);
response.setError(e);
+ return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(response);
}
- return response.toString();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @PostMapping(value = "/saveComplaintResolution", consumes = MediaType.APPLICATION_JSON, produces = MediaType.APPLICATION_JSON, headers = "Authorization") | |
| public String saveComplaintResolution(@RequestBody String request) { | |
| OutputResponse response = new OutputResponse(); | |
| try { | |
| response.setResponse(grievanceHandlingService.saveComplaintResolution(request)); | |
| } catch (Exception e) { | |
| logger.error("saveComplaintResolution failed with error " + e.getMessage(), e); | |
| response.setError(e); | |
| } | |
| return response.toString(); | |
| } | |
| @Operation(summary = "Save complaint resolution and remarks", | |
| description = "Updates the resolution and optional remarks for a grievance") | |
| @PostMapping(value = "/saveComplaintResolution", | |
| consumes = MediaType.APPLICATION_JSON, | |
| produces = MediaType.APPLICATION_JSON, | |
| headers = "Authorization") | |
| public ResponseEntity<OutputResponse> saveComplaintResolution( | |
| @Param(value = "{\"complaintID\":\"Complaint ID string\", " + | |
| "\"complaintResolution\":\"Resolution text\", " + | |
| "\"remarks\":\"Optional remarks\"}") | |
| @RequestBody String request) { | |
| OutputResponse response = new OutputResponse(); | |
| try { | |
| response.setResponse(grievanceHandlingService.saveComplaintResolution(request)); | |
| return ResponseEntity.ok(response); | |
| } catch (Exception e) { | |
| logger.error("saveComplaintResolution failed with error " + e.getMessage(), e); | |
| response.setError(e); | |
| return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(response); | |
| } | |
| } |
There was a problem hiding this comment.
@srishtigrp78 Thank you for implementing the suggested improvements! This will make the API more maintainable and user-friendly.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| } | ||
|
|
||
|
|
||
| public String saveComplaintResolution(String request) throws Exception { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add transaction management.
The method performs database updates but lacks transaction management. Add @Transactional to ensure atomic operations.
+ @Transactional
public String saveComplaintResolution(String request) throws Exception {📝 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 String saveComplaintResolution(String request) throws Exception { | |
| @Transactional | |
| public String saveComplaintResolution(String request) throws Exception { |
🛠️ Refactor suggestion
Add documentation and input validation.
The method would benefit from:
- JavaDoc documentation describing its purpose, parameters, return value, and possible exceptions
@NotNulland@NotBlankvalidation for the request parameter- More specific exception types instead of generic Exception
+ /**
+ * Saves the complaint resolution and remarks for a grievance.
+ *
+ * @param request JSON string containing complaint resolution details
+ * @return Success message if the update is successful
+ * @throws IllegalArgumentException if the request is null or empty
+ * @throws GrievanceUpdateException if the update operation fails
+ */
+ @NotNull
public String saveComplaintResolution(String request) throws Exception {📝 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 String saveComplaintResolution(String request) throws Exception { | |
| /** | |
| * Saves the complaint resolution and remarks for a grievance. | |
| * | |
| * @param request JSON string containing complaint resolution details | |
| * @return Success message if the update is successful | |
| * @throws IllegalArgumentException if the request is null or empty | |
| * @throws GrievanceUpdateException if the update operation fails | |
| */ | |
| @NotNull | |
| public String saveComplaintResolution(String request) throws Exception { |
There was a problem hiding this comment.
@srishtigrp78 You're absolutely right. I can confirm that you've already implemented both suggestions:
- The
@Transactionalannotation is present on thesaveComplaintResolutionmethod - JavaDoc documentation has been added to describe the method's purpose
Thank you for proactively addressing these code quality improvements!
| // Parse the request JSON into a GrievanceDetails object | ||
| GrievanceDetails grievanceRequest = InputMapper.gson().fromJson(request, GrievanceDetails.class); | ||
|
|
||
| // Extract values from the request | ||
| String complaintID = grievanceRequest.getComplaintID(); | ||
| String complaintResolution = grievanceRequest.getComplaintResolution(); | ||
| String remarks = grievanceRequest.getRemarks(); | ||
| Long beneficiaryRegID = grievanceRequest.getBeneficiaryRegID(); | ||
| Integer providerServiceMapID = grievanceRequest.getProviderServiceMapID(); | ||
| Integer assignedUserID = grievanceRequest.getAssignedUserID(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for required fields.
The method should validate:
- Request string before parsing
- Required fields after parsing
- Provide specific error messages for each validation failure
+ if (request == null || request.trim().isEmpty()) {
+ throw new IllegalArgumentException("Request cannot be null or empty");
+ }
+
GrievanceDetails grievanceRequest = InputMapper.gson().fromJson(request, GrievanceDetails.class);
+
+ // Validate required fields
+ if (grievanceRequest.getComplaintID() == null || grievanceRequest.getComplaintID().trim().isEmpty()) {
+ throw new IllegalArgumentException("ComplaintID is required");
+ }
+ if (grievanceRequest.getComplaintResolution() == null || grievanceRequest.getComplaintResolution().trim().isEmpty()) {
+ throw new IllegalArgumentException("ComplaintResolution is required");
+ }
+ if (grievanceRequest.getBeneficiaryRegID() == null) {
+ throw new IllegalArgumentException("BeneficiaryRegID is required");
+ }
+ if (grievanceRequest.getProviderServiceMapID() == null) {
+ throw new IllegalArgumentException("ProviderServiceMapID is required");
+ }
+ if (grievanceRequest.getAssignedUserID() == null) {
+ throw new IllegalArgumentException("AssignedUserID is required");
+ }📝 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.
| // Parse the request JSON into a GrievanceDetails object | |
| GrievanceDetails grievanceRequest = InputMapper.gson().fromJson(request, GrievanceDetails.class); | |
| // Extract values from the request | |
| String complaintID = grievanceRequest.getComplaintID(); | |
| String complaintResolution = grievanceRequest.getComplaintResolution(); | |
| String remarks = grievanceRequest.getRemarks(); | |
| Long beneficiaryRegID = grievanceRequest.getBeneficiaryRegID(); | |
| Integer providerServiceMapID = grievanceRequest.getProviderServiceMapID(); | |
| Integer assignedUserID = grievanceRequest.getAssignedUserID(); | |
| if (request == null || request.trim().isEmpty()) { | |
| throw new IllegalArgumentException("Request cannot be null or empty"); | |
| } | |
| GrievanceDetails grievanceRequest = InputMapper.gson().fromJson(request, GrievanceDetails.class); | |
| // Validate required fields | |
| if (grievanceRequest.getComplaintID() == null || grievanceRequest.getComplaintID().trim().isEmpty()) { | |
| throw new IllegalArgumentException("ComplaintID is required"); | |
| } | |
| if (grievanceRequest.getComplaintResolution() == null || grievanceRequest.getComplaintResolution().trim().isEmpty()) { | |
| throw new IllegalArgumentException("ComplaintResolution is required"); | |
| } | |
| if (grievanceRequest.getBeneficiaryRegID() == null) { | |
| throw new IllegalArgumentException("BeneficiaryRegID is required"); | |
| } | |
| if (grievanceRequest.getProviderServiceMapID() == null) { | |
| throw new IllegalArgumentException("ProviderServiceMapID is required"); | |
| } | |
| if (grievanceRequest.getAssignedUserID() == null) { | |
| throw new IllegalArgumentException("AssignedUserID is required"); | |
| } | |
| // Extract values from the request | |
| String complaintID = grievanceRequest.getComplaintID(); | |
| String complaintResolution = grievanceRequest.getComplaintResolution(); | |
| String remarks = grievanceRequest.getRemarks(); | |
| Long beneficiaryRegID = grievanceRequest.getBeneficiaryRegID(); | |
| Integer providerServiceMapID = grievanceRequest.getProviderServiceMapID(); | |
| Integer assignedUserID = grievanceRequest.getAssignedUserID(); |
There was a problem hiding this comment.
@srishtigrp78 Thanks for implementing the input validation! This will help catch potential issues early and provide clearer error messages.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| int updateCount = 0; | ||
| if (remarks == null) { | ||
| updateCount = grievanceDataRepo.updateComplaintResolution(complaintResolution,complaintID, | ||
| beneficiaryRegID, providerServiceMapID, assignedUserID); | ||
| } | ||
| updateCount = grievanceDataRepo.updateComplaintResolution(complaintResolution, remarks, complaintID, | ||
| beneficiaryRegID, providerServiceMapID, assignedUserID); | ||
|
|
There was a problem hiding this comment.
Fix logical error in update operations.
There's a bug in the update logic:
- If remarks is null, the first update is performed
- The second update is always performed, overwriting the first update
- Missing logging for database operations
This could lead to unexpected behavior where the first update is always overwritten.
int updateCount = 0;
if (remarks == null) {
updateCount = grievanceDataRepo.updateComplaintResolution(complaintResolution,complaintID,
beneficiaryRegID, providerServiceMapID, assignedUserID);
+ logger.debug("Updated complaint resolution without remarks for complaintID: {}", complaintID);
- }
- updateCount = grievanceDataRepo.updateComplaintResolution(complaintResolution, remarks, complaintID,
- beneficiaryRegID, providerServiceMapID, assignedUserID);
+ } else {
+ updateCount = grievanceDataRepo.updateComplaintResolution(complaintResolution, remarks, complaintID,
+ beneficiaryRegID, providerServiceMapID, assignedUserID);
+ logger.debug("Updated complaint resolution with remarks for complaintID: {}", complaintID);
+ }📝 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.
| int updateCount = 0; | |
| if (remarks == null) { | |
| updateCount = grievanceDataRepo.updateComplaintResolution(complaintResolution,complaintID, | |
| beneficiaryRegID, providerServiceMapID, assignedUserID); | |
| } | |
| updateCount = grievanceDataRepo.updateComplaintResolution(complaintResolution, remarks, complaintID, | |
| beneficiaryRegID, providerServiceMapID, assignedUserID); | |
| int updateCount = 0; | |
| if (remarks == null) { | |
| updateCount = grievanceDataRepo.updateComplaintResolution(complaintResolution, complaintID, | |
| beneficiaryRegID, providerServiceMapID, assignedUserID); | |
| logger.debug("Updated complaint resolution without remarks for complaintID: {}", complaintID); | |
| } else { | |
| updateCount = grievanceDataRepo.updateComplaintResolution(complaintResolution, remarks, complaintID, | |
| beneficiaryRegID, providerServiceMapID, assignedUserID); | |
| logger.debug("Updated complaint resolution with remarks for complaintID: {}", complaintID); | |
| } |
There was a problem hiding this comment.
🧩 Analysis chain
@srishtigrp78 Thank you for fixing the issue! The code now correctly handles the update operations.
🏁 Script executed:
#!/bin/bash
# Check the current implementation of saveComplaintResolution method
ast-grep --pattern 'public String saveComplaintResolution(String request) throws Exception {
$$$
}'
Length of output: 3779
@srishtigrp78 I see that the code still contains the original issues. The second update operation is still being performed unconditionally after the first one, which means it will always overwrite any previous update. Here's the current code:
if (remarks == null) {
updateCount = grievanceDataRepo.updateComplaintResolution(complaintResolution,complaintID,
beneficiaryRegID, providerServiceMapID, assignedUserID);
}
updateCount = grievanceDataRepo.updateComplaintResolution(complaintResolution, remarks, complaintID,
beneficiaryRegID, providerServiceMapID, assignedUserID);The fix should use an if-else structure to ensure only one update is performed:
if (remarks == null) {
updateCount = grievanceDataRepo.updateComplaintResolution(complaintResolution, complaintID,
beneficiaryRegID, providerServiceMapID, assignedUserID);
logger.debug("Updated complaint resolution without remarks for complaintID: {}", complaintID);
} else {
updateCount = grievanceDataRepo.updateComplaintResolution(complaintResolution, remarks, complaintID,
beneficiaryRegID, providerServiceMapID, assignedUserID);
logger.debug("Updated complaint resolution with remarks for complaintID: {}", complaintID);
}Could you please implement these changes to fix the logical error?
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java (1)
330-336: Add exception details to JavaDoc.The JavaDoc documentation should include details about the exceptions that can be thrown.
/** * Saves the complaint resolution and remarks for a grievance. * * @param request JSON string containing complaint resolution details * @return Success message if the update is successful + * @throws IllegalArgumentException if any required field is missing + * @throws Exception if the update operation fails */src/main/java/com/iemr/common/controller/grievance/GrievanceController.java (1)
160-179: Improve API response handling.The endpoint should:
- Return ResponseEntity for better HTTP status code handling
- Include more details in the API documentation
@Operation(summary = "Save complaint resolution and remarks", + description = "Updates the complaint resolution and optional remarks for a grievance. Returns success message on successful update.", + responses = { + @ApiResponse(responseCode = "200", description = "Complaint resolution updated successfully"), + @ApiResponse(responseCode = "400", description = "Invalid input - missing required fields"), + @ApiResponse(responseCode = "500", description = "Internal server error") + }) @PostMapping(value = "/saveComplaintResolution", consumes = MediaType.APPLICATION_JSON, produces = MediaType.APPLICATION_JSON, headers = "Authorization") - public String saveComplaintResolution(@Param(value = "...") @RequestBody String request) { + public ResponseEntity<OutputResponse> saveComplaintResolution(@Param(value = "...") @RequestBody String request) { OutputResponse response = new OutputResponse(); try { response.setResponse(grievanceHandlingService.saveComplaintResolution(request)); + return ResponseEntity.ok(response); } catch (IllegalArgumentException e) { logger.error("saveComplaintResolution failed with error " + e.getMessage(), e); response.setError(e); + return ResponseEntity.badRequest().body(response); } catch (Exception e) { logger.error("saveComplaintResolution failed with error " + e.getMessage(), e); response.setError(e); + return ResponseEntity.internalServerError().body(response); } - return response.toString(); }
📜 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(1 hunks)src/main/java/com/iemr/common/data/grievance/GrievanceDetails.java(4 hunks)src/main/java/com/iemr/common/repository/grievance/GrievanceDataRepo.java(1 hunks)src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/iemr/common/data/grievance/GrievanceDetails.java
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Package-test
- GitHub Check: Build
- GitHub Check: Analyze (java)
🔇 Additional comments (1)
src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java (1)
338-389: LGTM! The implementation looks good.The method:
- Has proper transaction management with
@Transactional- Performs comprehensive validation of required fields
- Sets audit fields (modifiedBy, lastModDate)
- Includes appropriate error handling and logging
- Uses conditional logic to handle remarks correctly
| @Modifying | ||
| @Query("UPDATE GrievanceDetails g SET g.complaintResolution = :complaintResolution, g.modifiedBy = :modifiedBy, g.lastModDate = :lastModDate " | ||
| + "WHERE g.complaintID = :complaintID AND g.beneficiaryRegID = :beneficiaryRegID AND g.providerServiceMapID = :providerServiceMapID" | ||
| + " AND g.assignedUserID = :assignedUserID") | ||
| @Transactional | ||
| int updateComplaintResolution(@Param("complaintResolution") String complaintResolution, | ||
| @Param("modifiedBy") String modifiedBy, | ||
| @Param("lastModDate") Timestamp lastModDate, | ||
| @Param("complaintID") String complaintID, | ||
| @Param("beneficiaryRegID") Long beneficiaryRegID, | ||
| @Param("providerServiceMapID") Integer providerServiceMapID, | ||
| @Param("assignedUserID") Integer assignedUserID); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add deleted check and documentation.
The query should:
- Include deleted check in WHERE clause
- Include JavaDoc documentation
@Modifying
+ /**
+ * Updates the complaint resolution without remarks
+ * @param complaintResolution The resolution text
+ * @param modifiedBy The user making the modification
+ * @param lastModDate The modification timestamp
+ * @param complaintID The ID of the complaint
+ * @param beneficiaryRegID The beneficiary registration ID
+ * @param providerServiceMapID The provider service map ID
+ * @param assignedUserID The ID of the assigned user
+ * @return Number of records updated
+ */
@Query("UPDATE GrievanceDetails g SET g.complaintResolution = :complaintResolution, g.modifiedBy = :modifiedBy, g.lastModDate = :lastModDate "
+ "WHERE g.complaintID = :complaintID AND g.beneficiaryRegID = :beneficiaryRegID AND g.providerServiceMapID = :providerServiceMapID"
- + " AND g.assignedUserID = :assignedUserID")
+ + " AND g.assignedUserID = :assignedUserID AND g.deleted = false")
@Transactional
int updateComplaintResolution(@Param("complaintResolution") String complaintResolution,
@Param("modifiedBy") String modifiedBy,
@Param("lastModDate") Timestamp lastModDate,
@Param("complaintID") String complaintID,
@Param("beneficiaryRegID") Long beneficiaryRegID,
@Param("providerServiceMapID") Integer providerServiceMapID,
@Param("assignedUserID") Integer assignedUserID);📝 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.
| @Modifying | |
| @Query("UPDATE GrievanceDetails g SET g.complaintResolution = :complaintResolution, g.modifiedBy = :modifiedBy, g.lastModDate = :lastModDate " | |
| + "WHERE g.complaintID = :complaintID AND g.beneficiaryRegID = :beneficiaryRegID AND g.providerServiceMapID = :providerServiceMapID" | |
| + " AND g.assignedUserID = :assignedUserID") | |
| @Transactional | |
| int updateComplaintResolution(@Param("complaintResolution") String complaintResolution, | |
| @Param("modifiedBy") String modifiedBy, | |
| @Param("lastModDate") Timestamp lastModDate, | |
| @Param("complaintID") String complaintID, | |
| @Param("beneficiaryRegID") Long beneficiaryRegID, | |
| @Param("providerServiceMapID") Integer providerServiceMapID, | |
| @Param("assignedUserID") Integer assignedUserID); | |
| @Modifying | |
| /** | |
| * Updates the complaint resolution without remarks | |
| * @param complaintResolution The resolution text | |
| * @param modifiedBy The user making the modification | |
| * @param lastModDate The modification timestamp | |
| * @param complaintID The ID of the complaint | |
| * @param beneficiaryRegID The beneficiary registration ID | |
| * @param providerServiceMapID The provider service map ID | |
| * @param assignedUserID The ID of the assigned user | |
| * @return Number of records updated | |
| */ | |
| @Query("UPDATE GrievanceDetails g SET g.complaintResolution = :complaintResolution, g.modifiedBy = :modifiedBy, g.lastModDate = :lastModDate " | |
| + "WHERE g.complaintID = :complaintID AND g.beneficiaryRegID = :beneficiaryRegID AND g.providerServiceMapID = :providerServiceMapID" | |
| + " AND g.assignedUserID = :assignedUserID AND g.deleted = false") | |
| @Transactional | |
| int updateComplaintResolution(@Param("complaintResolution") String complaintResolution, | |
| @Param("modifiedBy") String modifiedBy, | |
| @Param("lastModDate") Timestamp lastModDate, | |
| @Param("complaintID") String complaintID, | |
| @Param("beneficiaryRegID") Long beneficiaryRegID, | |
| @Param("providerServiceMapID") Integer providerServiceMapID, | |
| @Param("assignedUserID") Integer assignedUserID); |
| @Modifying | ||
| @Query("UPDATE GrievanceDetails g SET g.complaintResolution = :complaintResolution, g.remarks = :remarks, g.modifiedBy = :modifiedBy, g.lastModDate = :lastModDate " | ||
| + "WHERE g.complaintID = :complaintID AND g.beneficiaryRegID = :beneficiaryRegID AND g.providerServiceMapID = :providerServiceMapID" | ||
| + " AND g.assignedUserID = :assignedUserID") | ||
| @Transactional | ||
| int updateComplaintResolution(@Param("complaintResolution") String complaintResolution, | ||
| @Param("remarks") String remark, | ||
| @Param("modifiedBy") String modifiedBy, | ||
| @Param("lastModDate") Timestamp lastModDate, | ||
| @Param("complaintID") String complaintID, | ||
| @Param("beneficiaryRegID") Long beneficiaryRegID, | ||
| @Param("providerServiceMapID") Integer providerServiceMapID, | ||
| @Param("assignedUserID") Integer assignedUserID); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add deleted check and fix parameter naming.
The query should:
- Include deleted check in WHERE clause
- Use consistent parameter naming (remarks instead of remark)
@Modifying
+ /**
+ * Updates the complaint resolution with remarks
+ * @param complaintResolution The resolution text
+ * @param remarks The remarks text
+ * @param modifiedBy The user making the modification
+ * @param lastModDate The modification timestamp
+ * @param complaintID The ID of the complaint
+ * @param beneficiaryRegID The beneficiary registration ID
+ * @param providerServiceMapID The provider service map ID
+ * @param assignedUserID The ID of the assigned user
+ * @return Number of records updated
+ */
@Query("UPDATE GrievanceDetails g SET g.complaintResolution = :complaintResolution, g.remarks = :remarks, g.modifiedBy = :modifiedBy, g.lastModDate = :lastModDate "
+ "WHERE g.complaintID = :complaintID AND g.beneficiaryRegID = :beneficiaryRegID AND g.providerServiceMapID = :providerServiceMapID"
- + " AND g.assignedUserID = :assignedUserID")
+ + " AND g.assignedUserID = :assignedUserID AND g.deleted = false")
@Transactional
int updateComplaintResolution(@Param("complaintResolution") String complaintResolution,
- @Param("remarks") String remark,
+ @Param("remarks") String remarks,
@Param("modifiedBy") String modifiedBy,
@Param("lastModDate") Timestamp lastModDate,
@Param("complaintID") String complaintID,
@Param("beneficiaryRegID") Long beneficiaryRegID,
@Param("providerServiceMapID") Integer providerServiceMapID,
@Param("assignedUserID") Integer assignedUserID);📝 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.
| @Modifying | |
| @Query("UPDATE GrievanceDetails g SET g.complaintResolution = :complaintResolution, g.remarks = :remarks, g.modifiedBy = :modifiedBy, g.lastModDate = :lastModDate " | |
| + "WHERE g.complaintID = :complaintID AND g.beneficiaryRegID = :beneficiaryRegID AND g.providerServiceMapID = :providerServiceMapID" | |
| + " AND g.assignedUserID = :assignedUserID") | |
| @Transactional | |
| int updateComplaintResolution(@Param("complaintResolution") String complaintResolution, | |
| @Param("remarks") String remark, | |
| @Param("modifiedBy") String modifiedBy, | |
| @Param("lastModDate") Timestamp lastModDate, | |
| @Param("complaintID") String complaintID, | |
| @Param("beneficiaryRegID") Long beneficiaryRegID, | |
| @Param("providerServiceMapID") Integer providerServiceMapID, | |
| @Param("assignedUserID") Integer assignedUserID); | |
| @Modifying | |
| /** | |
| * Updates the complaint resolution with remarks | |
| * @param complaintResolution The resolution text | |
| * @param remarks The remarks text | |
| * @param modifiedBy The user making the modification | |
| * @param lastModDate The modification timestamp | |
| * @param complaintID The ID of the complaint | |
| * @param beneficiaryRegID The beneficiary registration ID | |
| * @param providerServiceMapID The provider service map ID | |
| * @param assignedUserID The ID of the assigned user | |
| * @return Number of records updated | |
| */ | |
| @Query("UPDATE GrievanceDetails g SET g.complaintResolution = :complaintResolution, g.remarks = :remarks, g.modifiedBy = :modifiedBy, g.lastModDate = :lastModDate " | |
| + "WHERE g.complaintID = :complaintID AND g.beneficiaryRegID = :beneficiaryRegID AND g.providerServiceMapID = :providerServiceMapID" | |
| + " AND g.assignedUserID = :assignedUserID AND g.deleted = false") | |
| @Transactional | |
| int updateComplaintResolution(@Param("complaintResolution") String complaintResolution, | |
| @Param("remarks") String remarks, | |
| @Param("modifiedBy") String modifiedBy, | |
| @Param("lastModDate") Timestamp lastModDate, | |
| @Param("complaintID") String complaintID, | |
| @Param("beneficiaryRegID") Long beneficiaryRegID, | |
| @Param("providerServiceMapID") Integer providerServiceMapID, | |
| @Param("assignedUserID") Integer assignedUserID); |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java (1)
330-336: Add @throws tag to JavaDoc.The JavaDoc documentation should include @throws tag to document the exceptions that can be thrown.
/** * Saves the complaint resolution and remarks for a grievance. * * @param request JSON string containing complaint resolution details * @return Success message if the update is successful + * @throws IllegalArgumentException if any required field is missing + * @throws Exception if the update operation fails */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/iemr/common/repository/grievance/GrievanceDataRepo.java(1 hunks)src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build
- GitHub Check: Package-test
- GitHub Check: Analyze (java)
🔇 Additional comments (1)
src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java (1)
370-382: LGTM!The implementation correctly handles both cases (with and without remarks) and includes proper logging.
| @Modifying | ||
| @Query("UPDATE GrievanceDetails g SET g.complaintResolution = :complaintResolution, g.remarks = :remarks, g.modifiedBy = :modifiedBy, " | ||
| + "WHERE g.complaintID = :complaintID AND g.beneficiaryRegID = :beneficiaryRegID AND g.providerServiceMapID = :providerServiceMapID" | ||
| + " AND g.assignedUserID = :assignedUserID") | ||
| @Transactional | ||
| int updateComplaintResolution(@Param("complaintResolution") String complaintResolution, | ||
| @Param("remarks") String remark, | ||
| @Param("modifiedBy") String modifiedBy, | ||
| @Param("complaintID") String complaintID, | ||
| @Param("beneficiaryRegID") Long beneficiaryRegID, | ||
| @Param("providerServiceMapID") Integer providerServiceMapID, | ||
| @Param("assignedUserID") Integer assignedUserID); |
There was a problem hiding this comment.
Fix query syntax and add documentation.
The query has several issues:
- Extra comma after modifiedBy in SET clause
- Missing lastModDate in SET clause
- Missing deleted check in WHERE clause
- Inconsistent parameter naming
- Missing JavaDoc documentation
Apply this diff to fix the issues:
@Modifying
+ /**
+ * Updates the complaint resolution with remarks
+ * @param complaintResolution The resolution text
+ * @param remarks The remarks text
+ * @param modifiedBy The user making the modification
+ * @param complaintID The ID of the complaint
+ * @param beneficiaryRegID The beneficiary registration ID
+ * @param providerServiceMapID The provider service map ID
+ * @param assignedUserID The ID of the assigned user
+ * @return Number of records updated
+ */
- @Query("UPDATE GrievanceDetails g SET g.complaintResolution = :complaintResolution, g.remarks = :remarks, g.modifiedBy = :modifiedBy, "
+ @Query("UPDATE GrievanceDetails g SET g.complaintResolution = :complaintResolution, g.remarks = :remarks, g.modifiedBy = :modifiedBy, g.lastModDate = CURRENT_TIMESTAMP "
+ "WHERE g.complaintID = :complaintID AND g.beneficiaryRegID = :beneficiaryRegID AND g.providerServiceMapID = :providerServiceMapID"
- + " AND g.assignedUserID = :assignedUserID")
+ + " AND g.assignedUserID = :assignedUserID AND g.deleted = false")
@Transactional
int updateComplaintResolution(@Param("complaintResolution") String complaintResolution,
- @Param("remarks") String remark,
+ @Param("remarks") String remarks,
@Param("modifiedBy") String modifiedBy,
@Param("complaintID") String complaintID,
@Param("beneficiaryRegID") Long beneficiaryRegID,
@Param("providerServiceMapID") Integer providerServiceMapID,
@Param("assignedUserID") Integer assignedUserID);📝 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.
| @Modifying | |
| @Query("UPDATE GrievanceDetails g SET g.complaintResolution = :complaintResolution, g.remarks = :remarks, g.modifiedBy = :modifiedBy, " | |
| + "WHERE g.complaintID = :complaintID AND g.beneficiaryRegID = :beneficiaryRegID AND g.providerServiceMapID = :providerServiceMapID" | |
| + " AND g.assignedUserID = :assignedUserID") | |
| @Transactional | |
| int updateComplaintResolution(@Param("complaintResolution") String complaintResolution, | |
| @Param("remarks") String remark, | |
| @Param("modifiedBy") String modifiedBy, | |
| @Param("complaintID") String complaintID, | |
| @Param("beneficiaryRegID") Long beneficiaryRegID, | |
| @Param("providerServiceMapID") Integer providerServiceMapID, | |
| @Param("assignedUserID") Integer assignedUserID); | |
| @Modifying | |
| /** | |
| * Updates the complaint resolution with remarks | |
| * @param complaintResolution The resolution text | |
| * @param remarks The remarks text | |
| * @param modifiedBy The user making the modification | |
| * @param complaintID The ID of the complaint | |
| * @param beneficiaryRegID The beneficiary registration ID | |
| * @param providerServiceMapID The provider service map ID | |
| * @param assignedUserID The ID of the assigned user | |
| * @return Number of records updated | |
| */ | |
| @Query("UPDATE GrievanceDetails g SET g.complaintResolution = :complaintResolution, g.remarks = :remarks, g.modifiedBy = :modifiedBy, g.lastModDate = CURRENT_TIMESTAMP " | |
| + "WHERE g.complaintID = :complaintID AND g.beneficiaryRegID = :beneficiaryRegID AND g.providerServiceMapID = :providerServiceMapID" | |
| + " AND g.assignedUserID = :assignedUserID AND g.deleted = false") | |
| @Transactional | |
| int updateComplaintResolution(@Param("complaintResolution") String complaintResolution, | |
| @Param("remarks") String remarks, | |
| @Param("modifiedBy") String modifiedBy, | |
| @Param("complaintID") String complaintID, | |
| @Param("beneficiaryRegID") Long beneficiaryRegID, | |
| @Param("providerServiceMapID") Integer providerServiceMapID, | |
| @Param("assignedUserID") Integer assignedUserID); |
| @Modifying | ||
| @Query("UPDATE GrievanceDetails g SET g.complaintResolution = :complaintResolution, g.modifiedBy = :modifiedBy, " | ||
| + "WHERE g.complaintID = :complaintID AND g.beneficiaryRegID = :beneficiaryRegID AND g.providerServiceMapID = :providerServiceMapID" | ||
| + " AND g.assignedUserID = :assignedUserID") | ||
| @Transactional | ||
| int updateComplaintResolution(@Param("complaintResolution") String complaintResolution, | ||
| @Param("modifiedBy") String modifiedBy, | ||
| @Param("complaintID") String complaintID, | ||
| @Param("beneficiaryRegID") Long beneficiaryRegID, | ||
| @Param("providerServiceMapID") Integer providerServiceMapID, | ||
| @Param("assignedUserID") Integer assignedUserID); |
There was a problem hiding this comment.
Fix query syntax and add documentation.
The query has several issues:
- Extra comma after modifiedBy in SET clause
- Missing lastModDate in SET clause
- Missing deleted check in WHERE clause
- Missing JavaDoc documentation
Apply this diff to fix the issues:
@Modifying
+ /**
+ * Updates the complaint resolution without remarks
+ * @param complaintResolution The resolution text
+ * @param modifiedBy The user making the modification
+ * @param complaintID The ID of the complaint
+ * @param beneficiaryRegID The beneficiary registration ID
+ * @param providerServiceMapID The provider service map ID
+ * @param assignedUserID The ID of the assigned user
+ * @return Number of records updated
+ */
- @Query("UPDATE GrievanceDetails g SET g.complaintResolution = :complaintResolution, g.modifiedBy = :modifiedBy, "
+ @Query("UPDATE GrievanceDetails g SET g.complaintResolution = :complaintResolution, g.modifiedBy = :modifiedBy, g.lastModDate = CURRENT_TIMESTAMP "
+ "WHERE g.complaintID = :complaintID AND g.beneficiaryRegID = :beneficiaryRegID AND g.providerServiceMapID = :providerServiceMapID"
- + " AND g.assignedUserID = :assignedUserID")
+ + " AND g.assignedUserID = :assignedUserID AND g.deleted = false")
@Transactional
int updateComplaintResolution(@Param("complaintResolution") String complaintResolution,
@Param("modifiedBy") String modifiedBy,
@Param("complaintID") String complaintID,
@Param("beneficiaryRegID") Long beneficiaryRegID,
@Param("providerServiceMapID") Integer providerServiceMapID,
@Param("assignedUserID") Integer assignedUserID);📝 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.
| @Modifying | |
| @Query("UPDATE GrievanceDetails g SET g.complaintResolution = :complaintResolution, g.modifiedBy = :modifiedBy, " | |
| + "WHERE g.complaintID = :complaintID AND g.beneficiaryRegID = :beneficiaryRegID AND g.providerServiceMapID = :providerServiceMapID" | |
| + " AND g.assignedUserID = :assignedUserID") | |
| @Transactional | |
| int updateComplaintResolution(@Param("complaintResolution") String complaintResolution, | |
| @Param("modifiedBy") String modifiedBy, | |
| @Param("complaintID") String complaintID, | |
| @Param("beneficiaryRegID") Long beneficiaryRegID, | |
| @Param("providerServiceMapID") Integer providerServiceMapID, | |
| @Param("assignedUserID") Integer assignedUserID); | |
| @Modifying | |
| /** | |
| * Updates the complaint resolution without remarks | |
| * @param complaintResolution The resolution text | |
| * @param modifiedBy The user making the modification | |
| * @param complaintID The ID of the complaint | |
| * @param beneficiaryRegID The beneficiary registration ID | |
| * @param providerServiceMapID The provider service map ID | |
| * @param assignedUserID The ID of the assigned user | |
| * @return Number of records updated | |
| */ | |
| @Query("UPDATE GrievanceDetails g SET g.complaintResolution = :complaintResolution, g.modifiedBy = :modifiedBy, g.lastModDate = CURRENT_TIMESTAMP " | |
| + "WHERE g.complaintID = :complaintID AND g.beneficiaryRegID = :beneficiaryRegID AND g.providerServiceMapID = :providerServiceMapID" | |
| + " AND g.assignedUserID = :assignedUserID AND g.deleted = false") | |
| @Transactional | |
| int updateComplaintResolution(@Param("complaintResolution") String complaintResolution, | |
| @Param("modifiedBy") String modifiedBy, | |
| @Param("complaintID") String complaintID, | |
| @Param("beneficiaryRegID") Long beneficiaryRegID, | |
| @Param("providerServiceMapID") Integer providerServiceMapID, | |
| @Param("assignedUserID") Integer assignedUserID); |
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java (5)
330-336: Improve the Javadoc documentation.The Javadoc is a good start but could be improved by adding the
@throwstag to document the exceptions that might be thrown by this method./** * Saves the complaint resolution and remarks for a grievance. * * @param request JSON string containing complaint resolution details * @return Success message if the update is successful +* @throws IllegalArgumentException if any required field is missing +* @throws Exception if the update operation fails */
354-356: Fix inconsistent error message.The error message references "AssignedUserID" but the variable is called "userID". This could cause confusion for developers debugging issues.
if (grievanceRequest.getUserID() == null) { - throw new IllegalArgumentException("AssignedUserID is required"); + throw new IllegalArgumentException("UserID is required"); }
369-371: Add comment explaining the purpose of modifiedBy variable.It's not immediately clear why a new variable
modifiedByis created when it's set to the same value ascreatedBy. Consider adding a comment explaining its purpose or simply usecreatedBydirectly if they serve the same function.- - String modifiedBy = createdBy; + // Using createdBy as modifiedBy to track who last updated the complaint + String modifiedBy = createdBy;
377-382: Improve code formatting for consistency.The indentation in the else block is inconsistent with the rest of the method. Properly aligned code improves readability.
else { -updateCount = grievanceDataRepo.updateComplaintResolution(complaintResolution, remarks, modifiedBy, complaintID, - beneficiaryRegID, providerServiceMapID, userID); -logger.debug("updated complaint resolution with remarks for complaint id: {}", complaintID); + updateCount = grievanceDataRepo.updateComplaintResolution(complaintResolution, remarks, modifiedBy, complaintID, + beneficiaryRegID, providerServiceMapID, userID); + logger.debug("updated complaint resolution with remarks for complaint id: {}", complaintID); }
383-387: Consider using a more specific exception type.The generic Exception is thrown when the update fails. Using a more specific exception type would make it easier for callers to handle different error scenarios appropriately.
if (updateCount > 0) { return "Complaint resolution updated successfully"; } else { - throw new Exception("Failed to update complaint resolution"); + throw new IEMRException("Failed to update complaint resolution for ID: " + complaintID); }
📜 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(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (java)
- GitHub Check: Package-test
- GitHub Check: Build
🔇 Additional comments (3)
src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java (3)
107-109: Consistent parameter naming throughout the codebase.The change from
assignedUserIDtouserIDhelps maintain consistency with the GrievanceDetails object properties. This is a good practice that improves code readability and reduces confusion.
197-197: Consistent parameter naming throughout the codebase.These changes correctly align the parameter naming from
assignedUserIDtouserIDacross all usages in themoveToBinmethod, maintaining consistency with the rest of the application.Also applies to: 223-223, 231-231, 234-235, 238-239
373-376: Consistent method parameter ordering.The
updateComplaintResolutionmethod call withmodifiedByparameter adds consistency to track who modified the record. Good implementation of the if-else structure as previously suggested.
| @Transactional | ||
| public String saveComplaintResolution(String request) throws Exception { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add @OverRide annotation if implementing interface method.
If this method is part of the GrievanceHandlingService interface, it should be annotated with @Override to ensure proper implementation and catch any interface changes.
🏁 Script executed:
#!/bin/bash
# Check if saveComplaintResolution is defined in GrievanceHandlingService interface
rg -A 3 "interface GrievanceHandlingService" --include="*.java"
rg "saveComplaintResolution" --include="*GrievanceHandlingService.java"Length of output: 1021
Verification Concern: Verify whether saveComplaintResolution is an interface method
The initial script couldn’t complete due to invalid command options, so please re-run the search with corrected syntax or perform a manual check. For example, try:
If you confirm that saveComplaintResolution is declared in the GrievanceHandlingService interface, please add the @Override annotation in the method implementation in the file src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java at lines 337-338 to ensure proper interface contract adherence.
🏁 Script executed:
[run_scripts]
#!/bin/bash
# Search for the interface declaration and its context
rg -A 3 "interface GrievanceHandlingService" -g "*.java"
# Search for the method usage in files related to GrievanceHandlingService
rg "saveComplaintResolution" -g "*GrievanceHandlingService.java"Length of output: 924
Action Required: Add @OverRide Annotation
We verified that saveComplaintResolution is declared in the GrievanceHandlingService interface (see file: src/main/java/com/iemr/common/service/grievance/GrievanceHandlingService.java). Therefore, the implementation in src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java (lines 337-338) should include the @Override annotation to ensure contract consistency. For example:
@Override
@Transactional
public String saveComplaintResolution(String request) throws Exception {
// method implementation
}This change will help catch interface changes during compilation.
2 participants
📋 Description
JIRA ID: 1146
Please provide a summary of the change and the motivation behind it. Include relevant context and details.
API to save complaint resolution and remarks for a grievance
✅ 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
assignedUserIDtouserIDacross various methods to ensure consistency.