AMM-1140 | API for fetching unallocated grievance record count for allocation screen #152
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
WalkthroughThis pull request introduces a new feature for fetching the count of unallocated grievances through a RESTful API. A new Changes
Possibly related PRs
Suggested reviewers
Poem
π Recent review detailsConfiguration used: CodeRabbit UI π Files selected for processing (1)
π§ Files skipped from review as they are similar to previous changes (1)
β° Context from checks skipped due to timeout of 90000ms (3)
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: 4
π§Ή Nitpick comments (4)
src/main/java/com/iemr/common/repository/grievance/GrievanceDataRepo.java (1)
18-20: Consider performance optimizations for the count query.The implementation is correct, but for optimal performance on large datasets, consider:
- Adding an index on the
isAllocatedcolumn if not already present- Using countQuery attribute or native query with EXISTS clause for better performance
- @Query("select count(request) " - + "from GrievanceDetails request where request.isAllocated = false") + @Query(value = "select count(1) from grievance_details where is_allocated = false", + nativeQuery = true)Also, consider adding an index if not present:
CREATE INDEX idx_grievance_is_allocated ON grievance_details(is_allocated);src/main/java/com/iemr/common/controller/grievance/GrievanceController.java (2)
28-30: Consider adding API versioning.To ensure backward compatibility and easier API evolution, consider adding version prefix to the endpoint (e.g., /v1/unallocatedGrievanceCount).
@CrossOrigin() @Operation(summary = "/unallocatedGrievanceCount") -@RequestMapping(value = "/unallocatedGrievanceCount", ...) +@RequestMapping(value = "/v1/unallocatedGrievanceCount", ...)
28-30: Consider adding rate limiting.Since this is a counting operation that could be resource-intensive on the database, consider adding rate limiting to prevent abuse.
@CrossOrigin() +@RateLimiter(name = "grievanceCount", fallbackMethod = "grievanceCountFallback") @Operation(summary = "/unallocatedGrievanceCount")src/main/java/com/iemr/common/service/grievance/GrievanceDataSyncImpl.java (1)
378-378: Add debug level check before logging.For better performance, wrap debug logs with level check.
-logger.debug("Request received for fetchUnallocatedGrievanceCount"); +if (logger.isDebugEnabled()) { + logger.debug("Request received for fetchUnallocatedGrievanceCount"); +}
π 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/repository/grievance/GrievanceDataRepo.java(1 hunks)src/main/java/com/iemr/common/service/grievance/GrievanceDataSync.java(1 hunks)src/main/java/com/iemr/common/service/grievance/GrievanceDataSyncImpl.java(3 hunks)
β° Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: style-check / checkstyle
- GitHub Check: Build
- GitHub Check: Analyze (java)
- GitHub Check: Package-test
π Additional comments (1)
src/main/java/com/iemr/common/repository/grievance/GrievanceDataRepo.java (1)
18-20: LGTM! Basic implementation looks good.The query is correctly implemented using JPQL with:
- Proper entity reference
- Clear condition for unallocated grievances
- Appropriate return type (Long) for count operation
|
|
||
| @CrossOrigin() | ||
| @Operation(summary = "/unallocatedGrievanceCount") | ||
| @RequestMapping(value = "/unallocatedGrievanceCount", method = RequestMethod.POST, consumes = MediaType.APPLICATION_JSON, produces = MediaType.APPLICATION_JSON, headers = "Authorization") |
There was a problem hiding this comment.
π οΈ Refactor suggestion
Use GET method instead of POST for read operations.
This endpoint retrieves data without making any modifications, so it should use HTTP GET to follow REST principles.
-@RequestMapping(value = "/unallocatedGrievanceCount", method = RequestMethod.POST, consumes = MediaType.APPLICATION_JSON, produces = MediaType.APPLICATION_JSON, headers = "Authorization")
+@GetMapping(value = "/unallocatedGrievanceCount", produces = MediaType.APPLICATION_JSON, headers = "Authorization")π 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.
| @RequestMapping(value = "/unallocatedGrievanceCount", method = RequestMethod.POST, consumes = MediaType.APPLICATION_JSON, produces = MediaType.APPLICATION_JSON, headers = "Authorization") | |
| @GetMapping(value = "/unallocatedGrievanceCount", produces = MediaType.APPLICATION_JSON, headers = "Authorization") |
There was a problem hiding this comment.
@srishtigrp78 Thank you for implementing the change! This will make the API more RESTful.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| } catch (Exception e) { | ||
| logger.error("UnallocatedGrievanceCount failed with error" + e.getMessage(), e); | ||
| responseData.setError(e); | ||
| } |
There was a problem hiding this comment.
π οΈ Refactor suggestion
Enhance error handling with specific error codes.
The current implementation catches all exceptions generically. Consider handling specific exceptions (IEMRException, JSONException) separately with appropriate error codes.
-} catch (Exception e) {
- logger.error("UnallocatedGrievanceCount failed with error" + e.getMessage(), e);
- responseData.setError(e);
-}
+} catch (IEMRException e) {
+ logger.error("Business logic error in UnallocatedGrievanceCount: " + e.getMessage(), e);
+ responseData.setError(e, "BUS_ERR_001");
+} catch (JSONException e) {
+ logger.error("JSON processing error in UnallocatedGrievanceCount: " + e.getMessage(), e);
+ responseData.setError(e, "JSON_ERR_001");
+} catch (Exception e) {
+ logger.error("Unexpected error in UnallocatedGrievanceCount: " + e.getMessage(), e);
+ responseData.setError(e, "SYS_ERR_001");
+}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@srishtigrp78 Thank you for implementing the enhanced error handling! This will make debugging and error tracking much more effective.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| Long unallocatedCount = grievanceDataRepo.fetchUnallocatedGrievanceCount(); | ||
|
|
||
| // Create a response JSON object | ||
| JSONObject result = new JSONObject(); | ||
| result.put("count", unallocatedCount); // Store the count of unallocated grievances | ||
|
|
||
| // Return the result as a string | ||
| return result.toString(); |
There was a problem hiding this comment.
π οΈ Refactor suggestion
Simplify method to return domain object and add null check.
The service layer should return domain objects and let the controller handle JSON serialization. Also, add null check for repository result.
-Long unallocatedCount = grievanceDataRepo.fetchUnallocatedGrievanceCount();
-
-// Create a response JSON object
-JSONObject result = new JSONObject();
-result.put("count", unallocatedCount); // Store the count of unallocated grievances
-
-// Return the result as a string
-return result.toString();
+Long unallocatedCount = grievanceDataRepo.fetchUnallocatedGrievanceCount();
+if (unallocatedCount == null) {
+ throw new IEMRException("Failed to fetch unallocated grievance count");
+}
+return unallocatedCount;Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@srishtigrp78 Thank you for implementing the suggested changes! This will make the code more robust by properly handling null cases.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
|
@srishtigrp78 saw the original ticket. |
There was a problem hiding this comment.
Actionable comments posted: 2
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (1)
src/main/java/com/iemr/common/controller/grievance/GrievanceController.java(1 hunks)
β° Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (java)
- GitHub Check: Package-test
- GitHub Check: Build
π Additional comments (4)
src/main/java/com/iemr/common/controller/grievance/GrievanceController.java (4)
21-31: Well-structured dependency injection and logging setup!Good practices observed:
- Constructor injection for dependencies
- Proper logger initialization
35-35: Use GET method for read operations.
1-20: Review placement of this API in Common-API.Based on the PR objectives and previous comments, there are concerns about whether this API belongs in the Common-API repository. The placement under
com.iemr.commonsuggests this is meant to be shared functionality, but the specific nature of "unallocated grievance counts" might indicate it belongs in a more specialized service.Let's analyze the usage patterns to determine if this truly belongs in Common-API:
β Verification successful
Placement in Common-API is appropriate
The grievance functionality, including the unallocated count feature, is correctly placed in Common-API as it's part of the core grievance data synchronization service. This is evidenced by:
- Integration with the existing GrievanceDataSync interface
- Shared usage through common repository and service layers
- Consistent architectural pattern with other grievance-related operations
π Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other grievance-related components to understand the context rg -l "grievance" --type java # Check if similar counting/allocation APIs exist in Common-API ast-grep --pattern 'class $_ { $$$ @$_Mapping($_) $_ fetch$_Count() { $$$ } $$$ }'Length of output: 1004
Script:
#!/bin/bash # Check existing grievance endpoints and their purposes rg -B 2 -A 2 "@(GetMapping|PostMapping|PutMapping|RequestMapping).+grievance" --type java # Examine the GrievanceDataSync interface to understand shared functionality cat src/main/java/com/iemr/common/service/grievance/GrievanceDataSync.java # Look for references to grievance allocation in existing code rg "grievance.*allocat" --type javaLength of output: 903
35-46: Verify security considerations for the endpoint.The endpoint handles sensitive grievance data and requires proper security controls:
- Verify Authorization header processing
- Check for proper access control
Let's analyze the security implementation:
β Verification successful
Security implementation verified for the endpoint
The endpoint has proper security controls implemented through:
- Mandatory Authorization header validation
- Token-based authentication via HTTPRequestInterceptor
- Redis-backed session management with proper expiry
- Request validation and IP checking capabilities
π Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for security filters and interceptors rg -A 5 "Authorization" --type java # Look for access control annotations ast-grep --pattern '@PreAuthorize($$$)' # Check for security configuration fd -e java -e xml -e properties -e yml | rg -l "security"Length of output: 81857
Script:
#!/bin/bash # Check for security filter chain config rg -A 10 "SecurityFilterChain|WebSecurityConfigurerAdapter|SecurityConfiguration" --type java # Look for role/authority checks rg -A 5 "hasRole|hasAuthority|@Secured|@RolesAllowed" --type java # Check security context usage rg -A 5 "SecurityContextHolder|Authentication|UserDetails" --type javaLength of output: 6499
Script:
#!/bin/bash # Check for custom filter implementations rg -A 10 "implements Filter|extends OncePerRequestFilter" --type java # Look for token validation logic rg -A 10 "validateToken|verifyToken|checkToken" --type java # Check session validation rg -A 10 "SessionObject|RedisSession|validateSession" --type javaLength of output: 72574
| @CrossOrigin() | ||
| @Operation(summary = "/unallocatedGrievanceCount") | ||
| @PostMapping(value = "/unallocatedGrievanceCount", consumes = MediaType.APPLICATION_JSON, produces = MediaType.APPLICATION_JSON, headers = "Authorization") |
There was a problem hiding this comment.
Restrict CORS and add rate limiting.
The unrestricted @CrossOrigin() annotation could pose a security risk. Consider:
- Explicitly specify allowed origins
- Add rate limiting for this endpoint to prevent abuse
Apply this diff:
-@CrossOrigin()
+@CrossOrigin(origins = "${app.allowed-origins}")
+@RateLimiter(name = "grievanceCount")Add to application.properties:
app.allowed-origins=https://example.com,https://admin.example.com| OutputResponse responseData = new OutputResponse(); | ||
| try { | ||
| responseData.setResponse(grievanceDataSync.fetchUnallocatedGrievanceCount()); | ||
| } catch (Exception e) { | ||
| logger.error("UnallocatedGrievanceCount failed with error" + e.getMessage(), e); | ||
| responseData.setError(e); | ||
| } | ||
| return responseData.toString(); |
There was a problem hiding this comment.
π οΈ Refactor suggestion
Enhance response handling and security.
- Validate response before toString() to prevent potential null pointer exceptions
- Consider sanitizing error messages to avoid exposing sensitive information
Apply this diff:
OutputResponse responseData = new OutputResponse();
try {
- responseData.setResponse(grievanceDataSync.fetchUnallocatedGrievanceCount());
+ Object count = grievanceDataSync.fetchUnallocatedGrievanceCount();
+ if (count != null) {
+ responseData.setResponse(count);
+ } else {
+ responseData.setError(new Exception("No data available"));
+ }
} catch (Exception e) {
logger.error("UnallocatedGrievanceCount failed with error" + e.getMessage(), e);
- responseData.setError(e);
+ // Sanitize error message for production
+ responseData.setError(new Exception("An internal error occurred"));
}
-return responseData.toString();
+return responseData != null ? responseData.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.
| OutputResponse responseData = new OutputResponse(); | |
| try { | |
| responseData.setResponse(grievanceDataSync.fetchUnallocatedGrievanceCount()); | |
| } catch (Exception e) { | |
| logger.error("UnallocatedGrievanceCount failed with error" + e.getMessage(), e); | |
| responseData.setError(e); | |
| } | |
| return responseData.toString(); | |
| OutputResponse responseData = new OutputResponse(); | |
| try { | |
| Object count = grievanceDataSync.fetchUnallocatedGrievanceCount(); | |
| if (count != null) { | |
| responseData.setResponse(count); | |
| } else { | |
| responseData.setError(new Exception("No data available")); | |
| } | |
| } catch (Exception e) { | |
| logger.error("UnallocatedGrievanceCount failed with error" + e.getMessage(), e); | |
| // Sanitize error message for production | |
| responseData.setError(new Exception("An internal error occurred")); | |
| } | |
| return responseData != null ? responseData.toString() : ""; |
There was a problem hiding this comment.
@srishtigrp78 Thank you for implementing the suggested changes to improve the response handling and security! π
/\___/\
( o o )
( =^= )
(______)
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: 1
β»οΈ Duplicate comments (1)
src/main/java/com/iemr/common/service/grievance/GrievanceDataSyncImpl.java (1)
377-389: π οΈ Refactor suggestionReturn domain object instead of JSON string.
The service layer should return domain objects and let the controller handle JSON serialization. This maintains better separation of concerns.
-public String fetchUnallocatedGrievanceCount() throws IEMRException, JSONException { +public Long fetchUnallocatedGrievanceCount() throws IEMRException { logger.debug("Request received for fetchUnallocatedGrievanceCount"); Long unallocatedCount = grievanceDataRepo.fetchUnallocatedGrievanceCount(); if (unallocatedCount == null) { throw new IEMRException("Failed to fetch unallocated grievance count"); } - JSONObject result = new JSONObject(); - result.put("count", unallocatedCount); - return result.toString(); + return unallocatedCount; }
π§Ή Nitpick comments (2)
src/main/java/com/iemr/common/service/grievance/GrievanceDataSyncImpl.java (2)
377-389: Enhance logging for better observability.Add error logging and include more context in debug logs for better traceability.
public String fetchUnallocatedGrievanceCount() throws IEMRException, JSONException { - logger.debug("Request received for fetchUnallocatedGrievanceCount"); + logger.debug("Fetching unallocated grievance count"); Long unallocatedCount = grievanceDataRepo.fetchUnallocatedGrievanceCount(); if (unallocatedCount == null) { + logger.error("Failed to retrieve unallocated grievance count from repository"); throw new IEMRException("Failed to fetch unallocated grievance count"); } + logger.debug("Successfully retrieved unallocated grievance count: {}", unallocatedCount); JSONObject result = new JSONObject(); result.put("count", unallocatedCount); return result.toString(); }
377-389: Wrap repository exceptions.Catch and wrap repository exceptions to provide more meaningful error messages.
public String fetchUnallocatedGrievanceCount() throws IEMRException, JSONException { logger.debug("Request received for fetchUnallocatedGrievanceCount"); - Long unallocatedCount = grievanceDataRepo.fetchUnallocatedGrievanceCount(); + Long unallocatedCount; + try { + unallocatedCount = grievanceDataRepo.fetchUnallocatedGrievanceCount(); + } catch (Exception e) { + logger.error("Error while fetching unallocated grievance count", e); + throw new IEMRException("Database error while fetching unallocated grievance count", e); + } if (unallocatedCount == null) { throw new IEMRException("Failed to fetch unallocated grievance count"); }
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (2)
src/main/java/com/iemr/common/controller/grievance/GrievanceController.java(1 hunks)src/main/java/com/iemr/common/service/grievance/GrievanceDataSyncImpl.java(3 hunks)
π§ Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/iemr/common/controller/grievance/GrievanceController.java
β° Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java)
π Additional comments (1)
src/main/java/com/iemr/common/service/grievance/GrievanceDataSyncImpl.java (1)
14-15: LGTM!The new imports are properly organized and all are being used in the implementation.
Also applies to: 41-42
src/main/java/com/iemr/common/service/grievance/GrievanceDataSyncImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (1)
src/main/java/com/iemr/common/config/PrimaryDBConfig.java(1 hunks)
β° Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Package-test
- GitHub Check: Analyze (java)
- GitHub Check: Build
π Additional comments (2)
src/main/java/com/iemr/common/config/PrimaryDBConfig.java (2)
50-50: Review architectural decision: Grievance functionality in Common-API.Based on the PR objectives and comments, there are concerns about placing grievance-related functionality in the Common-API. This warrants a deeper architectural discussion:
- Common-API should typically contain shared functionality used across multiple services
- If grievance management is specific to one domain, it might be better placed in a dedicated service
Consider:
- Is this functionality used by multiple services?
- Could this create unwanted dependencies?
- Would this make the Common-API too domain-specific?
β Verification successful
Architectural placement of grievance functionality in Common-API is appropriate
The grievance system exhibits characteristics of a shared service with cross-cutting concerns:
- Integrates with common services (SMS, feedback)
- Implements centralized data synchronization
- Maintains independent data persistence
π Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing grievance-related code to understand the scope rg -g '!target/' -g '!*.class' -l 'grievance' . # Check for potential service dependencies rg -g '!target/' -g '!*.class' -l '@Autowired.*Grievance|@Inject.*Grievance' .Length of output: 1088
50-50: Verify the existence of the grievance package structure.Let's ensure that the newly added package
com.iemr.common.data.grievanceexists and contains the necessary repository classes.β Verification successful
Package structure verification successful
The
com.iemr.common.data.grievancepackage exists and contains the necessary entity classes (GrievanceDetails, GrievanceTransaction) that are properly referenced by their corresponding repository classes in thecom.iemr.common.repository.grievancepackage. The component scanning configuration is correct.π Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the package directory exists and contains repository classes fd -t d "grievance$" src/main/java fd -t f ".*Repository\.java$|.*Repo\.java$" -p src/main/java/com/iemr/common/data/grievanceLength of output: 329
Script:
#!/bin/bash # Check contents of data and repository packages echo "=== Data Package Contents ===" fd -t f ".*\.java$" src/main/java/com/iemr/common/data/grievance echo -e "\n=== Repository Package Contents ===" fd -t f ".*\.java$" src/main/java/com/iemr/common/repository/grievance echo -e "\n=== Checking Repository References ===" rg -l "com\.iemr\.common\.data\.grievance" src/main/java/com/iemr/common/repository/grievanceLength of output: 1017
| @@ -47,7 +47,7 @@ | |||
| @Configuration | |||
| @EnableTransactionManagement | |||
| @EnableJpaRepositories(entityManagerFactoryRef = "entityManagerFactory", basePackages = { "com.iemr.common.repository", | |||
| "com.iemr.common.repo", "com.iemr.common.notification.agent", "com.iemr.common.covidVaccination", "com.iemr.common.repository.everwell.*", " com.iemr.common.repository.users" }) | |||
| "com.iemr.common.repo", "com.iemr.common.notification.agent", "com.iemr.common.covidVaccination", "com.iemr.common.repository.everwell.*", "com.iemr.common.data.grievance"," com.iemr.common.repository.users" }) | |||
There was a problem hiding this comment.
Fix syntax error in basePackages list.
There's a syntax error in the repository packages list that could cause runtime issues:
- Extra comma and space after "com.iemr.common.data.grievance"
- Incorrect string concatenation
Apply this diff to fix the syntax:
- "com.iemr.common.repo", "com.iemr.common.notification.agent", "com.iemr.common.covidVaccination", "com.iemr.common.repository.everwell.*", "com.iemr.common.data.grievance"," com.iemr.common.repository.users" })
+ "com.iemr.common.repo", "com.iemr.common.notification.agent", "com.iemr.common.covidVaccination", "com.iemr.common.repository.everwell.*", "com.iemr.common.data.grievance", "com.iemr.common.repository.users" })π 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.
| "com.iemr.common.repo", "com.iemr.common.notification.agent", "com.iemr.common.covidVaccination", "com.iemr.common.repository.everwell.*", "com.iemr.common.data.grievance"," com.iemr.common.repository.users" }) | |
| "com.iemr.common.repo", "com.iemr.common.notification.agent", "com.iemr.common.covidVaccination", "com.iemr.common.repository.everwell.*", "com.iemr.common.data.grievance", "com.iemr.common.repository.users" }) |
There was a problem hiding this comment.
@srishtigrp78 Thank you for fixing the syntax error! π
β¦β¨(β’α΄₯β’)β©β§
Happy!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
|
π Description
JIRA ID: AMM-1140
API for fetching unallocated grievance record count for allocation screen
β 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
Summary by CodeRabbit
New Features
Bug Fixes
Chores