AMM-306,AMM-1081,AMM-1082,AMM-1083,AMM-1084#47
Conversation
WalkthroughThe changes in this pull request involve modifications to several methods within the 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)
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
src/main/java/com/iemr/ecd/repository/report/EcdReportRepo.java (1)
97-100: Consider adding input validation for role parameterWhile the stored procedures might handle role validation, consider adding parameter validation at the repository level to fail fast and reduce unnecessary database calls.
@Query(value="call db_reporting.Pr_ECDAbortionReport(:startDate,:endDate,:agentId,:psmId,:role)", nativeQuery=true) public List<Object[]> getAbortionReport(@Param("startDate") Timestamp startDate, @Param("endDate") Timestamp endDate, @Param("agentId") Integer agentId, - @Param("psmId") Integer psmId, @Param("role") String role); + @Param("psmId") Integer psmId, @Param("role") @NotNull @Pattern(regexp = "^(ADMIN|ASSOCIATE|SUPERVISOR)$") String role);Also applies to: 122-125, 127-130, 142-145
src/main/java/com/iemr/ecd/service/associate/CallClosureImpl.java (1)
String literals for call status are used inconsistently across the codebase
The verification confirms that call status strings are hardcoded with inconsistent casing:
- Some places use
"Open"while others use"open"Constants.javadefinesOPEN = "open"but it's not consistently used- No constant defined for
"Completed"statusSuggested changes:
- Use the existing
Constants.OPENfor all "open" status assignments- Add
COMPLETEDconstant toConstants.java- Update all status assignments to use these constants
Example locations to fix:
CallClosureImpl.java: 7 instances of hardcoded status stringsCallAllocationImpl.java: 3 instances of hardcoded status strings🔗 Analysis chain
Line range hint
1-400: Consider architectural improvements for call status managementThe call status management logic is spread across multiple conditions with potential overlaps. This could lead to maintenance issues and unexpected behavior.
Suggestions for improvement:
- Create an enum for call statuses to replace string literals
- Extract the status determination logic into a separate method
- Add documentation explaining the role-based status rules
- Consider implementing a state machine pattern for managing call status transitions
Example structure:
public enum CallStatus { OPEN("Open"), COMPLETED("Completed"); private final String value; // ... constructor and methods } private CallStatus determineCallStatus(OutboundCalls callObj, Bencall benCall, String role) { // Extract status determination logic here // Document the rules and priorities }Let's verify the usage of status strings across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for hardcoded call status strings echo "Searching for hardcoded call status strings..." rg -i "setCallStatus\([\"'](?:Open|Completed)[\"']\)" # Search for other call status related constants echo "Searching for call status constants..." rg -i "public static final String .*(OPEN|COMPLETED|STATUS)"Length of output: 1792
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/main/java/com/iemr/ecd/repository/report/EcdReportRepo.java(3 hunks)src/main/java/com/iemr/ecd/service/associate/CallClosureImpl.java(1 hunks)src/main/java/com/iemr/ecd/service/report/ReportServiceImpl.java(4 hunks)src/main/java/com/iemr/ecd/utils/constants/Constants.java(1 hunks)
🔇 Additional comments (4)
src/main/java/com/iemr/ecd/utils/constants/Constants.java (1)
9-9: Verify consistent usage of ASSOCIATE constant
Let's verify that this constant is consistently used across the codebase, particularly in CallClosureImpl and report-related classes.
src/main/java/com/iemr/ecd/repository/report/EcdReportRepo.java (2)
97-100: Verify consistent role handling across layers
The repository changes suggest corresponding modifications in the service layer. Ensure proper role propagation from controllers through services to these repository methods.
Also applies to: 122-125, 127-130, 142-145
✅ Verification successful
Role parameter is consistently handled across all layers
The code analysis shows proper role propagation through all layers:
- Repository layer: All report methods (getAbortionReport, getStillBirthReport, getBabyDeathReport, getMiscarriageReport) consistently accept the role parameter
- Service layer (ReportServiceImpl): Properly extracts role from request objects and passes it to repository methods
- Controller layer: Receives JSON requests that contain role information and forwards them to service layer
The role parameter is consistently handled from controller through service to repository layer for all the mentioned report types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify role parameter handling in service and controller layers
# Check service layer implementations
ast-grep --pattern 'class $_ implements $_Service {
$$$
get(Abortion|StillBirth|BabyDeath|Miscarriage)Report($$$) {
$$$
}
$$$
}'
# Check controller layer implementations
ast-grep --pattern 'class $_Controller {
$$$
get(Abortion|StillBirth|BabyDeath|Miscarriage)Report($$$) {
$$$
}
$$$
}'
Length of output: 274
Script:
#!/bin/bash
# Let's try a different approach to find relevant service and controller implementations
# Search for service implementations related to report methods
rg -l "getAbortionReport|getStillBirthReport|getBabyDeathReport|getMiscarriageReport" --type java
# Then let's examine the content of any matching files
rg "getAbortionReport|getStillBirthReport|getBabyDeathReport|getMiscarriageReport" -A 5 -B 2 --type java
Length of output: 19759
97-100: Implementation of role-based access for sensitive medical reports
The addition of the role parameter to these specific report methods suggests implementation of role-based access control for sensitive medical data. This is a good security practice.
Let's verify the stored procedures handle the role parameter correctly:
Also applies to: 122-125, 127-130, 142-145
✅ Verification successful
Role parameter properly integrated across application layers
The code analysis shows that the role parameter has been consistently implemented across all layers:
- Controller layer has secured endpoints (with
headers = "Authorization") that accept JSON requests - Service layer properly propagates the role parameter through all report methods
- Repository layer correctly passes the role parameter to stored procedures
- The implementation includes proper criteria columns showing "Role" in the generated Excel reports
While we cannot verify the stored procedures directly (as they are in the database), the application-side implementation of role-based access control for these sensitive medical reports is complete and consistent.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify stored procedures properly implement role-based filtering
# Check stored procedures implementation
rg -A 10 "CREATE PROCEDURE.*ECDAbortionReport|CREATE PROCEDURE.*ECDStillBirthReport|CREATE PROCEDURE.*ECDBabyDeathReport|CREATE PROCEDURE.*ECDMiscarriageReport" --type sql
# Check for role parameter usage in procedures
rg "role\s+VARCHAR|role\s+CHAR|role\s+TEXT" --type sql
Length of output: 528
Script:
#!/bin/bash
# Let's try a broader search for SQL files first
fd -e sql
# Then search for any files that might contain stored procedures
fd . -t f -x grep -l "CREATE PROCEDURE" {} \;
# Let's also check if procedures might be defined in other file types
# or in different locations
rg "CREATE PROCEDURE.*ECDAbortionReport|CREATE PROCEDURE.*ECDStillBirthReport|CREATE PROCEDURE.*ECDBabyDeathReport|CREATE PROCEDURE.*ECDMiscarriageReport"
# Check for any files containing these procedure names
rg "Pr_ECDAbortionReport|Pr_ECDStillBirthReport|Pr_ECDBabyDeathReport|Pr_ECDMiscarriageReport"
Length of output: 1046
Script:
#!/bin/bash
# Let's check the service layer to see how these repository methods are used
# and how the role parameter is passed
# Search for service classes that might use these report methods
rg -A 5 "getAbortionReport|getStillBirthReport|getBabyDeathReport|getMiscarriageReport" --type java
# Also check for any controller layer usage to understand the complete flow
rg -B 5 "getAbortionReport|getStillBirthReport|getBabyDeathReport|getMiscarriageReport" --type java --glob '*Controller.java'
# Check for any role-related constants or enums that might define valid roles
rg "enum.*Role|String\s+role\s*=|ROLE_" --type java
Length of output: 20759
src/main/java/com/iemr/ecd/service/report/ReportServiceImpl.java (1)
573-573: Verify role-based data access implementation.
The role parameter has been added to filter data access, but we should verify the implementation in the repository layer.
Also applies to: 771-771, 811-811, 933-933
✅ Verification successful
Role-based filtering is properly implemented in the repository layer
The verification shows that role-based filtering is correctly implemented:
- The
EcdReportRepointerface has thegetAbortionReportmethod with role parameter:@Param("role") String role - The method uses a native SQL procedure call that includes the role parameter:
@Query(value="call db_reporting.Pr_ECDAbortionReport(:startDate,:endDate,:agentId,:psmId,:role)") - Similar role-based filtering is consistently implemented across other report methods like
getCallSummaryReport,getCumulativeDistrictReport,getStillBirthReport,getBabyDeathReport, andgetMiscarriageReport
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check repository implementation of role-based filtering
# Search for SQL queries using role parameter
rg -A 10 "role\s*=\s*:role" --type sql
# Check repository interface for role parameter usage
ast-grep --pattern 'interface EcdReportRepo {
$$$
getAbortionReport($_, $_, $_, $_, $_);
$$$
}'
Length of output: 293
Script:
#!/bin/bash
# Let's try a broader search to find repository and SQL implementations
# Search for repository interface and implementation
rg -t java "interface.*EcdReportRepo|class.*EcdReportRepo.*implements" -A 5
# Search for getAbortionReport method signatures and implementations
rg -t java "getAbortionReport" -A 5
# Search for any SQL queries or JPA repositories with role-based filtering
rg -t java "@Query.*role|where.*role" -A 5
Length of output: 14664
| public static final String MOTHER = "Mother"; | ||
| public static final String CHILD = "Child"; | ||
| public static final String ANM = "ANM"; | ||
| public static final String ASSOCIATE = "associate"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Maintain consistent naming convention for role constants
The value "associate" is lowercase while other role-based constants like "ANM" are uppercase. Consider maintaining consistency.
-public static final String ASSOCIATE = "associate";
+public static final String ASSOCIATE = "ASSOCIATE";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static final String ASSOCIATE = "associate"; | |
| public static final String ASSOCIATE = "ASSOCIATE"; |
| if (null != obj.getReceivedRoleName() && (obj.getReceivedRoleName().equalsIgnoreCase(Constants.ANM) | ||
| || obj.getReceivedRoleName().equalsIgnoreCase(Constants.ASSOCIATE))) { | ||
| callObj.setCallStatus("Open"); | ||
| } |
There was a problem hiding this comment.
Restructure role-based call status update logic
The current implementation has several potential issues:
- The role-based status update is nested within the HRP check, which might prevent it from executing in non-HRP cases
- The role comparison is case-sensitive and uses string literals
Consider applying these changes:
if (request.getIsHrp() != null) {
callObj.setIsHighRisk(request.getIsHrp());
- if (null != obj.getReceivedRoleName() && (obj.getReceivedRoleName().equalsIgnoreCase(Constants.ANM)
- || obj.getReceivedRoleName().equalsIgnoreCase(Constants.ASSOCIATE))) {
- callObj.setCallStatus("Open");
- }
}
+ // Move role check outside HRP block to ensure it runs independently
+ if (null != obj.getReceivedRoleName() && (obj.getReceivedRoleName().equalsIgnoreCase(Constants.ANM)
+ || obj.getReceivedRoleName().equalsIgnoreCase(Constants.ASSOCIATE))) {
+ callObj.setCallStatus(Constants.OPEN); // Use constant for consistency
+ }📝 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.
| if (null != obj.getReceivedRoleName() && (obj.getReceivedRoleName().equalsIgnoreCase(Constants.ANM) | |
| || obj.getReceivedRoleName().equalsIgnoreCase(Constants.ASSOCIATE))) { | |
| callObj.setCallStatus("Open"); | |
| } | |
| if (request.getIsHrp() != null) { | |
| callObj.setIsHighRisk(request.getIsHrp()); | |
| } | |
| // Move role check outside HRP block to ensure it runs independently | |
| if (null != obj.getReceivedRoleName() && (obj.getReceivedRoleName().equalsIgnoreCase(Constants.ANM) | |
| || obj.getReceivedRoleName().equalsIgnoreCase(Constants.ASSOCIATE))) { | |
| callObj.setCallStatus(Constants.OPEN); // Use constant for consistency | |
| } |
| c.setRole(abortionReport.getRole()); | ||
| result = ecdReportRepo.getAbortionReport(startDate, endDate, abortionReport.getAgentId(), | ||
| abortionReport.getPsmId()); | ||
| abortionReport.getPsmId(),abortionReport.getRole()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add role parameter validation and documentation.
The role parameter has been added to several repository method calls, but there are some concerns:
-
Inconsistent usage: Some report methods pass the role parameter while others don't. For consistency, consider standardizing the role parameter usage across all report methods.
-
Missing validation: The role parameter should be validated before being passed to the repository to prevent potential issues.
-
Missing documentation: The purpose and valid values for the role parameter should be documented.
Consider applying these improvements:
// Add documentation
+/**
+ * Generates the abortion report based on the provided criteria.
+ * @param request The report request containing filter criteria
+ * @param filename The name of the output file
+ * @param role The user role that determines data access level (e.g. ADMIN, USER)
+ * @return ByteArrayInputStream containing the report data
+ * @throws Exception if report generation fails
+ */
public ByteArrayInputStream getAbortionReport(String request, String filename, String role) throws Exception {
+ // Validate role parameter
+ if (role == null || role.trim().isEmpty()) {
+ throw new ECDException("Role parameter is required");
+ }
// ... rest of the method
}Apply similar changes to getStillBirthReport, getBabyDeathReport, and getMiscarriageReport methods.
Also applies to: 771-771, 811-811, 933-933
|
📋 Description
JIRA ID: AMM-306 & AMM-1081,1082,1083,1084
Please provide a summary of the change and the motivation behind it. Include relevant context and details.
✅ 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
roleparameter to several report generation methods, enhancing the filtering capabilities for reports like Abortion, Stillbirth, Baby Death, and Miscarriage.COMPLETEDandASSOCIATEfor improved role management within the application.Bug Fixes
closeCallmethod based on caller role, ensuring accurate processing of calls.