Role added as extra param while fetching Eligible records by language #89
Conversation
i.e role to filter count
WalkthroughThe changes introduce a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant Service
participant Repository
Client->>Controller: getEligibleRecordsLanguageInfo(psmId, ..., role)
Controller->>Service: getEligibleRecordsLanguageInfo(psmId, ..., role)
alt role == "ANM" and recordType == "MOTHER"/"CHILD"
Service->>Repository: getMother/ChildUnAllocatedCountLRByLanguage(...)
Service-->>Controller: Return count
else role == "ASSOCIATE" and recordType == "MOTHER"/"CHILD"
Service->>Repository: getMother/ChildUnAllocatedCountIntroductoryByLanguage(...)
Service-->>Controller: Return count
else
Service-->>Controller: Return null
end
Controller-->>Client: Response
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 3
🔭 Outside diff range comments (1)
src/main/java/com/iemr/ecd/controller/callallocation/CallAllocationController.java (1)
145-158:⚠️ Potential issueCritical issue: Missing role parameter in URL mapping.
The method signature on line 156 expects
roleas a@PathVariable, but the URL mapping on line 145 doesn't include{role}in the path. This will cause a runtime exception when the endpoint is called.Apply this fix to the URL mapping:
-@GetMapping(value = "/getEligibleRecordsLanguageInfo/{psmId}/{phoneNoType}/{recordType}/{fDate}/{tDate}/{preferredLanguage}", produces = MediaType.APPLICATION_JSON_VALUE) +@GetMapping(value = "/getEligibleRecordsLanguageInfo/{psmId}/{phoneNoType}/{recordType}/{fDate}/{tDate}/{preferredLanguage}/{role}", produces = MediaType.APPLICATION_JSON_VALUE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/iemr/ecd/controller/callallocation/CallAllocationController.java(1 hunks)src/main/java/com/iemr/ecd/repo/call_conf_allocation/OutboundCallsRepo.java(3 hunks)src/main/java/com/iemr/ecd/service/call_conf_allocation/CallAllocationImpl.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/com/iemr/ecd/service/call_conf_allocation/CallAllocationImpl.java (1)
src/main/java/com/iemr/ecd/utils/constants/Constants.java (1)
Constants(5-24)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (4)
src/main/java/com/iemr/ecd/repo/call_conf_allocation/OutboundCallsRepo.java (4)
69-72: Good implementation of call type filtering.The query modification correctly adds filtering by
displayEcdCallType = 'introductory'for associate role records. This properly separates introductory calls from other call types.
357-363: Well-designed separation of concerns for introductory call counts.The new method
getMotherUnAllocatedCountIntroductoryByLanguageprovides a clean separation between introductory and non-introductory call counting, supporting the role-based logic in the service layer.
375-381: Consistent implementation across mother and child record types.The child record introductory count method follows the same pattern as the mother record method, ensuring consistency across the codebase.
352-353:Details
❓ Verification inconclusive
Verify the impact of excluding introductory calls on existing functionality.
The existing methods
getMotherUnAllocatedCountLRByLanguageandgetChildUnAllocatedCountLRByLanguagenow exclude introductory calls with the conditiont.displayEcdCallType != 'introductory'. Ensure this change doesn't break existing functionality that might depend on the previous behavior.Also applies to: 369-370
🏁 Script executed:
#!/bin/bash # Search for existing usages of these modified methods to assess impact echo "Searching for usages of getMotherUnAllocatedCountLRByLanguage:" rg "getMotherUnAllocatedCountLRByLanguage" -A 3 -B 3 echo -e "\nSearching for usages of getChildUnAllocatedCountLRByLanguage:" rg "getChildUnAllocatedCountLRByLanguage" -A 3 -B 3Length of output: 4887
Validate exclusion of ‘introductory’ calls
We’ve confirmed that both
getMotherUnAllocatedCountLRByLanguageandgetChildUnAllocatedCountLRByLanguageare only invoked inCallAllocationImpl.javato set the low-risk call counts:
- src/main/java/com/iemr/ecd/service/call_conf_allocation/CallAllocationImpl.java
• Line wheregetMotherUnAllocatedCountLRByLanguage(...)is called- src/main/java/com/iemr/ecd/service/call_conf_allocation/CallAllocationImpl.java
• Line wheregetChildUnAllocatedCountLRByLanguage(...)is calledPlease verify that excluding calls with
t.displayEcdCallType = 'introductory'aligns with business requirements and doesn’t inadvertently drop valid records in these use cases.
| throw new ECDException(e); | ||
| } | ||
|
|
||
| return null; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unreachable null return statement.
With the suggested exception handling above, this null return becomes unreachable and should be removed.
} catch (Exception e) {
throw new ECDException(e);
}
- return null;
}📝 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.
| return null; | |
| } catch (Exception e) { | |
| throw new ECDException(e); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/com/iemr/ecd/service/call_conf_allocation/CallAllocationImpl.java
at line 1105, remove the return null statement because with the updated
exception handling it is unreachable and unnecessary.
| public ResponseEligibleCallRecordsDTO getEligibleRecordsLanguageInfo(int psmId, String phoneNoType, | ||
| String recordType, String fDate, String tDate, String preferredLanguage, String role) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for the role parameter.
The method should validate that the role parameter is not null or empty before proceeding with the logic.
Add validation at the beginning of the method:
public ResponseEligibleCallRecordsDTO getEligibleRecordsLanguageInfo(int psmId, String phoneNoType,
String recordType, String fDate, String tDate, String preferredLanguage, String role) {
try {
+ if (role == null || role.trim().isEmpty()) {
+ throw new InvalidRequestException("role is required");
+ }
if (preferredLanguage == null || preferredLanguage.trim().isEmpty()) {
throw new InvalidRequestException("preferred language 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.
| public ResponseEligibleCallRecordsDTO getEligibleRecordsLanguageInfo(int psmId, String phoneNoType, | |
| String recordType, String fDate, String tDate, String preferredLanguage, String role) { | |
| public ResponseEligibleCallRecordsDTO getEligibleRecordsLanguageInfo(int psmId, String phoneNoType, | |
| String recordType, String fDate, String tDate, String preferredLanguage, String role) { | |
| try { | |
| if (role == null || role.trim().isEmpty()) { | |
| throw new InvalidRequestException("role is required"); | |
| } | |
| if (preferredLanguage == null || preferredLanguage.trim().isEmpty()) { | |
| throw new InvalidRequestException("preferred language is required"); | |
| } | |
| // ... rest of existing logic ... | |
| } catch (Exception e) { | |
| // existing exception handling | |
| } | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/com/iemr/ecd/service/call_conf_allocation/CallAllocationImpl.java
around lines 1057 to 1058, add input validation for the role parameter by
checking if it is null or empty at the start of the
getEligibleRecordsLanguageInfo method. If the role is invalid, handle it
appropriately, such as returning an error response or throwing an exception,
before proceeding with the rest of the method logic.
| if (recordType != null && recordType.equalsIgnoreCase(Constants.MOTHER) | ||
| && role.equalsIgnoreCase(Constants.ANM)) { | ||
| totalLowRisk = outboundCallsRepo.getMotherUnAllocatedCountLRByLanguage(Constants.UNALLOCATED, psmId, | ||
| tempFDateStamp, tempTDateStamp, phoneNoType, preferredLanguage); | ||
| responseEligibleCallRecordsDTO.setTotalLowRiskRecord(totalLowRisk); | ||
| return responseEligibleCallRecordsDTO; | ||
| } else if (recordType != null && recordType.equalsIgnoreCase(Constants.CHILD) | ||
| && role.equalsIgnoreCase(Constants.ANM)) { | ||
| totalLowRisk = outboundCallsRepo.getChildUnAllocatedCountLRByLanguage(Constants.UNALLOCATED, psmId, | ||
| tempFDateStamp, tempTDateStamp, phoneNoType, preferredLanguage); | ||
| responseEligibleCallRecordsDTO.setTotalLowRiskRecord(totalLowRisk); | ||
| return responseEligibleCallRecordsDTO; | ||
| } else if (recordType != null && recordType.equalsIgnoreCase(Constants.MOTHER) | ||
| && role.equalsIgnoreCase(Constants.ASSOCIATE)) { | ||
| totalIntroductoryCount = outboundCallsRepo.getMotherUnAllocatedCountIntroductoryByLanguage( | ||
| Constants.UNALLOCATED, psmId, tempFDateStamp, tempTDateStamp, phoneNoType, preferredLanguage); | ||
| responseEligibleCallRecordsDTO.setTotalIntroductoryRecord(totalIntroductoryCount); | ||
| return responseEligibleCallRecordsDTO; | ||
| } else if (recordType != null && recordType.equalsIgnoreCase(Constants.CHILD) | ||
| && role.equalsIgnoreCase(Constants.ASSOCIATE)) { | ||
| totalIntroductoryCount = outboundCallsRepo.getChildUnAllocatedCountIntroductoryByLanguage( | ||
| Constants.UNALLOCATED, psmId, tempFDateStamp, tempTDateStamp, phoneNoType, preferredLanguage); | ||
| responseEligibleCallRecordsDTO.setTotalIntroductoryRecord(totalIntroductoryCount); | ||
| return responseEligibleCallRecordsDTO; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for unsupported role and record type combinations.
The method returns null for unsupported role and record type combinations, which could lead to unexpected behavior. Consider throwing a descriptive exception instead.
Replace the implicit null return with explicit exception handling:
} else if (recordType != null && recordType.equalsIgnoreCase(Constants.CHILD)
&& role.equalsIgnoreCase(Constants.ASSOCIATE)) {
totalIntroductoryCount = outboundCallsRepo.getChildUnAllocatedCountIntroductoryByLanguage(
Constants.UNALLOCATED, psmId, tempFDateStamp, tempTDateStamp, phoneNoType, preferredLanguage);
responseEligibleCallRecordsDTO.setTotalIntroductoryRecord(totalIntroductoryCount);
return responseEligibleCallRecordsDTO;
+ } else {
+ throw new InvalidRequestException("Unsupported combination of role '" + role + "' and recordType '" + recordType + "'");
}
-📝 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 (recordType != null && recordType.equalsIgnoreCase(Constants.MOTHER) | |
| && role.equalsIgnoreCase(Constants.ANM)) { | |
| totalLowRisk = outboundCallsRepo.getMotherUnAllocatedCountLRByLanguage(Constants.UNALLOCATED, psmId, | |
| tempFDateStamp, tempTDateStamp, phoneNoType, preferredLanguage); | |
| responseEligibleCallRecordsDTO.setTotalLowRiskRecord(totalLowRisk); | |
| return responseEligibleCallRecordsDTO; | |
| } else if (recordType != null && recordType.equalsIgnoreCase(Constants.CHILD) | |
| && role.equalsIgnoreCase(Constants.ANM)) { | |
| totalLowRisk = outboundCallsRepo.getChildUnAllocatedCountLRByLanguage(Constants.UNALLOCATED, psmId, | |
| tempFDateStamp, tempTDateStamp, phoneNoType, preferredLanguage); | |
| responseEligibleCallRecordsDTO.setTotalLowRiskRecord(totalLowRisk); | |
| return responseEligibleCallRecordsDTO; | |
| } else if (recordType != null && recordType.equalsIgnoreCase(Constants.MOTHER) | |
| && role.equalsIgnoreCase(Constants.ASSOCIATE)) { | |
| totalIntroductoryCount = outboundCallsRepo.getMotherUnAllocatedCountIntroductoryByLanguage( | |
| Constants.UNALLOCATED, psmId, tempFDateStamp, tempTDateStamp, phoneNoType, preferredLanguage); | |
| responseEligibleCallRecordsDTO.setTotalIntroductoryRecord(totalIntroductoryCount); | |
| return responseEligibleCallRecordsDTO; | |
| } else if (recordType != null && recordType.equalsIgnoreCase(Constants.CHILD) | |
| && role.equalsIgnoreCase(Constants.ASSOCIATE)) { | |
| totalIntroductoryCount = outboundCallsRepo.getChildUnAllocatedCountIntroductoryByLanguage( | |
| Constants.UNALLOCATED, psmId, tempFDateStamp, tempTDateStamp, phoneNoType, preferredLanguage); | |
| responseEligibleCallRecordsDTO.setTotalIntroductoryRecord(totalIntroductoryCount); | |
| return responseEligibleCallRecordsDTO; | |
| } | |
| if (recordType != null && recordType.equalsIgnoreCase(Constants.MOTHER) | |
| && role.equalsIgnoreCase(Constants.ANM)) { | |
| totalLowRisk = outboundCallsRepo.getMotherUnAllocatedCountLRByLanguage(Constants.UNALLOCATED, psmId, | |
| tempFDateStamp, tempTDateStamp, phoneNoType, preferredLanguage); | |
| responseEligibleCallRecordsDTO.setTotalLowRiskRecord(totalLowRisk); | |
| return responseEligibleCallRecordsDTO; | |
| } else if (recordType != null && recordType.equalsIgnoreCase(Constants.CHILD) | |
| && role.equalsIgnoreCase(Constants.ANM)) { | |
| totalLowRisk = outboundCallsRepo.getChildUnAllocatedCountLRByLanguage(Constants.UNALLOCATED, psmId, | |
| tempFDateStamp, tempTDateStamp, phoneNoType, preferredLanguage); | |
| responseEligibleCallRecordsDTO.setTotalLowRiskRecord(totalLowRisk); | |
| return responseEligibleCallRecordsDTO; | |
| } else if (recordType != null && recordType.equalsIgnoreCase(Constants.MOTHER) | |
| && role.equalsIgnoreCase(Constants.ASSOCIATE)) { | |
| totalIntroductoryCount = outboundCallsRepo.getMotherUnAllocatedCountIntroductoryByLanguage( | |
| Constants.UNALLOCATED, psmId, tempFDateStamp, tempTDateStamp, phoneNoType, preferredLanguage); | |
| responseEligibleCallRecordsDTO.setTotalIntroductoryRecord(totalIntroductoryCount); | |
| return responseEligibleCallRecordsDTO; | |
| } else if (recordType != null && recordType.equalsIgnoreCase(Constants.CHILD) | |
| && role.equalsIgnoreCase(Constants.ASSOCIATE)) { | |
| totalIntroductoryCount = outboundCallsRepo.getChildUnAllocatedCountIntroductoryByLanguage( | |
| Constants.UNALLOCATED, psmId, tempFDateStamp, tempTDateStamp, phoneNoType, preferredLanguage); | |
| responseEligibleCallRecordsDTO.setTotalIntroductoryRecord(totalIntroductoryCount); | |
| return responseEligibleCallRecordsDTO; | |
| } else { | |
| throw new InvalidRequestException( | |
| "Unsupported combination of role '" + role + "' and recordType '" + recordType + "'"); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/com/iemr/ecd/service/call_conf_allocation/CallAllocationImpl.java
around lines 1076 to 1100, the method currently returns null for unsupported
combinations of role and recordType, which can cause unexpected behavior. To fix
this, add explicit error handling by throwing a descriptive exception when the
role and recordType combination is not supported, instead of returning null.
This will make the method's behavior clearer and safer.
2 participants
📋 Description
JIRA ID:
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
Bug Fixes