ECD Reports and highrisk#50
Conversation
WalkthroughThis pull request introduces several modifications across multiple repository interfaces and service classes. Key changes include the simplification of method signatures in the Changes
Possibly related PRs
Suggested reviewers
📜 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 (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: 1
🧹 Outside diff range and nitpick comments (4)
src/main/java/com/iemr/ecd/repository/masters/AgentsViewMasterRepo.java (1)
36-37: LGTM! Consider adding an index for roleId.The JPQL query with DISTINCT is well-structured and properly parameterized. For optimal performance, consider adding a database index on the
roleIdcolumn if not already present.src/main/java/com/iemr/ecd/service/report/ReportServiceImpl.java (3)
772-775: Remove unnecessary blank linesConsider removing the extra blank lines to maintain consistent code formatting.
- - result = ecdReportRepo.getStillBirthReport(startDate, endDate, stillBirthReport.getAgentId(), - stillBirthReport.getPsmId(),stillBirthReport.getRole()); - + result = ecdReportRepo.getStillBirthReport(startDate, endDate, stillBirthReport.getAgentId(), + stillBirthReport.getPsmId(),stillBirthReport.getRole());
814-815: Remove unnecessary blank lineConsider removing the extra blank line to maintain consistent code formatting.
- - result = ecdReportRepo.getBabyDeathReport(startDate, endDate, babyDeathReport.getAgentId(), + result = ecdReportRepo.getBabyDeathReport(startDate, endDate, babyDeathReport.getAgentId(),
936-938: Remove unnecessary blank linesConsider removing the extra blank lines to maintain consistent code formatting.
- - result = ecdReportRepo.getMiscarriageReport(startDate, endDate, miscarriageReport.getAgentId(), - miscarriageReport.getPsmId(), miscarriageReport.getRole()); + result = ecdReportRepo.getMiscarriageReport(startDate, endDate, miscarriageReport.getAgentId(), + miscarriageReport.getPsmId(), miscarriageReport.getRole());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/main/java/com/iemr/ecd/repo/call_conf_allocation/OutboundCallsRepo.java(1 hunks)src/main/java/com/iemr/ecd/repository/masters/AgentsViewMasterRepo.java(1 hunks)src/main/java/com/iemr/ecd/repository/report/EcdReportRepo.java(3 hunks)src/main/java/com/iemr/ecd/service/call_conf_allocation/CallAllocationImpl.java(1 hunks)src/main/java/com/iemr/ecd/service/report/ReportServiceImpl.java(4 hunks)
🔇 Additional comments (8)
src/main/java/com/iemr/ecd/repository/report/EcdReportRepo.java (3)
143-146: Verify role parameter handling in stored procedure.
The addition of the role parameter to getMiscarriageReport needs verification in the corresponding stored procedure Pr_ECDMiscarriageReport.
128-131: Verify role parameter handling in stored procedure.
The addition of the role parameter to getBabyDeathReport needs verification in the corresponding stored procedure Pr_ECDBabyDeathReport.
99-101: Verify role parameter handling in stored procedure.
The addition of the role parameter to getAbortionReport needs verification in the corresponding stored procedure Pr_ECDAbortionReport.
src/main/java/com/iemr/ecd/service/report/ReportServiceImpl.java (4)
573-574: LGTM: Role parameter addition to abortion report
The addition of the role parameter to ecdReportRepo.getAbortionReport() enables role-based filtering of report data, which is consistent with the repository interface changes.
772-774: LGTM: Role parameter addition to still birth report
The addition of the role parameter to ecdReportRepo.getStillBirthReport() enables role-based filtering of report data, which is consistent with the repository interface changes.
814-815: LGTM: Role parameter addition to baby death report
The addition of the role parameter to ecdReportRepo.getBabyDeathReport() enables role-based filtering of report data, which is consistent with the repository interface changes.
936-938: LGTM: Role parameter addition to miscarriage report
The addition of the role parameter to ecdReportRepo.getMiscarriageReport() enables role-based filtering of report data, which is consistent with the repository interface changes.
src/main/java/com/iemr/ecd/service/call_conf_allocation/CallAllocationImpl.java (1)
519-519: LGTM: Simplified method signature
The removal of date parameters from getMotherUnAllocatedCountHR() call aligns with the repository interface changes to simplify the method signature. This change streamlines the counting of high-risk mother records without date filtering.
| + " t.psmId=:psmId AND " | ||
| + " t.childId IS NULL AND t.motherId IS NOT NULL AND t.isHighRisk = true " | ||
| + " AND t.phoneNumberType=:phoneNoType AND t.deleted = false ") | ||
| int getMotherUnAllocatedCountHR(@Param("allocationStatus") String allocationStatus, @Param("psmId") Integer psmId, | ||
| @Param("fDate") Timestamp fDate, @Param("tDate") Timestamp tDate, @Param("phoneNoType") String phoneNoType); | ||
| @Param("phoneNoType") String phoneNoType); |
There was a problem hiding this comment.
Critical: Verify impact of removing date filtering.
The removal of date parameters (fDate and tDate) from getMotherUnAllocatedCountHR could lead to:
- Counting outdated high-risk mother records
- Including future scheduled calls
- Performance issues with large datasets
Consider keeping date filtering to ensure accurate counts within relevant time periods.
Apply this diff to restore date filtering:
@Query(value = " SELECT count(1) FROM OutboundCalls AS t WHERE t.allocationStatus =:allocationStatus AND "
- + " t.psmId=:psmId AND "
+ + " t.psmId=:psmId AND ((:fDate between t.callDateFrom AND t.callDateTo) OR (:tDate between t.callDateFrom AND t.callDateTo)) AND "
+ " t.childId IS NULL AND t.motherId IS NOT NULL AND t.isHighRisk = true "
+ " AND t.phoneNumberType=:phoneNoType AND t.deleted = false ")
int getMotherUnAllocatedCountHR(@Param("allocationStatus") String allocationStatus, @Param("psmId") Integer psmId,
+ @Param("fDate") Timestamp fDate, @Param("tDate") Timestamp tDate,
@Param("phoneNoType") String phoneNoType);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/main/java/com/iemr/ecd/utils/constants/Constants.java (1)
18-18: Consider using an enum for call not answered reasons.The current implementation uses hardcoded strings which could be prone to typos and make maintenance difficult. Consider using an enum for better type safety and maintainability.
-public static final List<String> REASONFORCALLNOTANSWERED = List.of("Invalid number","Out of service","Out of Reach","Switched off","No reply","Number busy","Call not connected"); +public enum ReasonForCallNotAnswered { + INVALID_NUMBER("Invalid number"), + OUT_OF_SERVICE("Out of service"), + OUT_OF_REACH("Out of Reach"), + SWITCHED_OFF("Switched off"), + NO_REPLY("No reply"), + NUMBER_BUSY("Number busy"), + CALL_NOT_CONNECTED("Call not connected"); + + private final String reason; + + ReasonForCallNotAnswered(String reason) { + this.reason = reason; + } + + public String getReason() { + return reason; + } + + public static boolean contains(String reason) { + for (ReasonForCallNotAnswered value : values()) { + if (value.getReason().equals(reason)) { + return true; + } + } + return false; + } +}src/main/java/com/iemr/ecd/dao/associate/OutboundCalls.java (1)
120-121: Add documentation for the new field.Consider adding Javadoc to explain the purpose and behavior of the
isFurtherCallRequiredfield, including when it's set to true/false/null.+ /** + * Indicates whether additional calls are required for this outbound call record. + * Set to true when the call was not answered and requires follow-up. + * Set to false when no further calls are needed. + * Can be null when the status is undetermined. + */ @Column(name = "isFurtherCallRequired") private Boolean isFurtherCallRequired;src/main/java/com/iemr/ecd/service/associate/CallClosureImpl.java (3)
169-174: Remove commented code.The commented code block should be removed as it's no longer needed. If this code needs to be referenced later, it should be tracked in version control history.
- /* - * if (null != obj.getReceivedRoleName() && - * (obj.getReceivedRoleName().equalsIgnoreCase(Constants.ANM) || - * obj.getReceivedRoleName().equalsIgnoreCase(Constants.ASSOCIATE))) { - * callObj.setCallStatus(Constants.OPEN); } - */
132-135: Consolidate call status logic.The logic for handling unanswered calls is spread across multiple locations. Consider consolidating this logic into a private method for better maintainability.
+ private void handleUnansweredCall(Bencall bencall, OutboundCalls callObj, String reasonForCallNotAnswered, boolean isMaxcallsAttempted) { + if(null != reasonForCallNotAnswered && Constants.REASONFORCALLNOTANSWERED.contains(reasonForCallNotAnswered)) { + bencall.setIsFurtherCallRequired(true); + bencall.setIsCallDisconnected(true); + + if (!isMaxcallsAttempted) { + callObj.setCallStatus(Constants.OPEN); + } + } + }Also applies to: 200-202
141-141: Fix variable naming convention.The variable
isMaxcallsAttempteddoesn't follow the camelCase naming convention. Rename it toisMaxCallsAttempted.- boolean isMaxcallsAttempted=false; + boolean isMaxCallsAttempted = false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/iemr/ecd/dao/associate/OutboundCalls.java(1 hunks)src/main/java/com/iemr/ecd/repo/call_conf_allocation/OutboundCallsRepo.java(3 hunks)src/main/java/com/iemr/ecd/service/associate/CallClosureImpl.java(5 hunks)src/main/java/com/iemr/ecd/utils/constants/Constants.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/iemr/ecd/repo/call_conf_allocation/OutboundCallsRepo.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java)
| if(null != obj.getIsFurtherCallRequired() && !obj.getIsFurtherCallRequired()) { | ||
| if (callObj.getMotherId() != null && callObj.getChildId() != null) { | ||
| outboundCallsRepo.updateIsFurtherCallRequiredForUpcomingCallForChild(callObj.getChildId(), obj.getIsFurtherCallRequired()); | ||
| } else if (callObj.getMotherId() != null && callObj.getChildId() == null) { | ||
| outboundCallsRepo.updateIsFurtherCallRequiredForUpcomingCallForMother(callObj.getMotherId(), obj.getIsFurtherCallRequired()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling for repository calls.
The repository calls for updating isFurtherCallRequired should include error handling to manage potential database exceptions.
if(null != obj.getIsFurtherCallRequired() && !obj.getIsFurtherCallRequired()) {
if (callObj.getMotherId() != null && callObj.getChildId() != null) {
- outboundCallsRepo.updateIsFurtherCallRequiredForUpcomingCallForChild(callObj.getChildId(), obj.getIsFurtherCallRequired());
+ try {
+ outboundCallsRepo.updateIsFurtherCallRequiredForUpcomingCallForChild(callObj.getChildId(), obj.getIsFurtherCallRequired());
+ } catch (Exception e) {
+ logger.error("Failed to update isFurtherCallRequired for child: " + callObj.getChildId(), e);
+ throw new ECDException("Failed to update upcoming calls for child", e);
+ }
} else if (callObj.getMotherId() != null && callObj.getChildId() == null) {
- outboundCallsRepo.updateIsFurtherCallRequiredForUpcomingCallForMother(callObj.getMotherId(), obj.getIsFurtherCallRequired());
+ try {
+ outboundCallsRepo.updateIsFurtherCallRequiredForUpcomingCallForMother(callObj.getMotherId(), obj.getIsFurtherCallRequired());
+ } catch (Exception e) {
+ logger.error("Failed to update isFurtherCallRequired for mother: " + callObj.getMotherId(), e);
+ throw new ECDException("Failed to update upcoming calls for mother", e);
+ }
}
}Committable suggestion skipped: line range outside the PR's diff.
|
📋 Description
JIRA ID: AMM-1067
Beneficiary record appearing in worklist After Not Interested in further call section
✅ Type of Change
ℹ️ Additional Information
Please describe how the changes were tested, and include any relevant screenshots, logs, or other information that provides additional context.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores