preferred Language changes#65
Conversation
WalkthroughThis update introduces preferred language filtering to record allocation and outbound call repository methods, especially for associate users. It also enhances call closure logic for nuanced status handling, updates JWT validation exclusions to cover API documentation endpoints, and modifies configuration files to enable dynamic control of Swagger and Springdoc features via environment variables. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Service
participant MotherRepo
participant ChildRepo
participant OutboundCallsRepo
User->>Service: Request allocation (with/without preferredLanguage)
alt preferredLanguage provided
Service->>OutboundCallsRepo: Fetch unallocated records filtered by language
OutboundCallsRepo-->>Service: Return paged OutboundCalls
Service->>OutboundCallsRepo: Update allocation status and user assignment
else
Service->>MotherRepo/ChildRepo: Fetch unallocated records
MotherRepo/ChildRepo-->>Service: Return records
Service->>OutboundCallsRepo: Create new OutboundCalls entities
Service->>MotherRepo/ChildRepo: Mark records as allocated
end
Service-->>User: Return allocation result
sequenceDiagram
participant Client
participant JwtFilter
Client->>JwtFilter: Request to /swagger-ui or /v3/api-docs
JwtFilter-->>Client: Bypass JWT validation
Client->>JwtFilter: Request to protected endpoint
JwtFilter->>JwtFilter: Validate JWT
JwtFilter-->>Client: Allow or deny access
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ 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: 4
🔭 Outside diff range comments (1)
src/main/java/com/iemr/ecd/service/associate/CallClosureImpl.java (1)
195-198:⚠️ Potential issueAllocation status set to
OPENinstead ofUNALLOCATED
allocationStatusis expected to hold eitherallocatedorunallocated(seeConstants). Setting it toOPENhere mixes the call-status vocabulary with the allocation vocabulary and breaks downstream filters.- callObj.setAllocationStatus(Constants.OPEN); + callObj.setAllocationStatus(Constants.UNALLOCATED);Without this fix, UI/cron jobs that look for
UNALLOCATEDrecords will miss these calls.
♻️ Duplicate comments (1)
src/main/java/com/iemr/ecd/repo/call_conf_allocation/OutboundCallsRepo.java (1)
274-285: Same overload duplication for child queriesExactly the same comment applies to the Child query variants:
getAllocatedRecordsUserByRecordTypeAndPhoneTypeChildAssociateis overloaded
twice. Consider consolidating as suggested above.
🧹 Nitpick comments (9)
src/main/java/com/iemr/ecd/utils/mapper/JwtUserIdValidationFilter.java (1)
58-60: Good enhancement to JWT validation exclusionsAdding exclusions for Swagger UI, API docs endpoints, and token refresh is appropriate and consistent with the configuration changes in the properties files. This allows access to API documentation without requiring authentication.
Consider also adding brief comments above each excluded path group to explain why they're excluded, enhancing code maintainability:
// Skip login and public endpoints if (path.equals(contextPath + "/user/userAuthenticate") || path.equalsIgnoreCase(contextPath + "/user/logOutUserFromConcurrentSession") + // API documentation endpoints || path.startsWith(contextPath + "/swagger-ui") || path.startsWith(contextPath + "/v3/api-docs") + // Token refresh endpoint || path.startsWith(contextPath + "/user/refreshToken") || path.startsWith(contextPath + "/public")) {src/main/java/com/iemr/ecd/repo/call_conf_allocation/ChildRecordRepo.java (1)
43-47: Ensure deterministic ordering & DB-agnostic boolean comparison
- The native query currently returns an unspecified record order. On a busy table this will pick random rows per call, which makes troubleshooting and pagination hard.
IsAllocated is falseis Postgres-specific; MySQL/MariaDB expectIsAllocated = false(or= 0). If the project ever swaps DBs, this will break silently.-SELECT * FROM t_childvaliddata WHERE IsAllocated is false AND +SELECT * FROM t_childvaliddata WHERE IsAllocated = false AND CreatedDate BETWEEN :fDate AND :tDate AND Phone_No_of = :phoneType AND preferredLanguage = :preferredLanguage + ORDER BY CreatedDate -- deterministic fetch order LIMIT :recordLimitAdding an
ORDER BY(e.g.,CreatedDateor a PK) and the portable boolean comparison keeps the query predictable and future-proof.src/main/java/com/iemr/ecd/repo/call_conf_allocation/MotherRecordRepo.java (1)
48-53: Replicate the portability & ordering fix applied to the child queryThe same comments made for the child query apply here: use
= falseinstead ofis false, and introduce anORDER BYto guarantee stable ordering.-SELECT * FROM t_mothervalidrecord WHERE IsAllocated is false AND +SELECT * FROM t_mothervalidrecord WHERE IsAllocated = false AND CreatedDate BETWEEN :fDate AND :tDate AND PhoneNo_Of_Whom = :phoneType AND preferredLanguage = :preferredLanguage + ORDER BY CreatedDate LIMIT :recordLimitsrc/main/java/com/iemr/ecd/service/call_conf_allocation/CallAllocationImpl.java (4)
122-132: Prefer constants & avoid duplicate repository methodsHard-coding
"unallocated"while a constant (Constants.UNALLOCATED) already exists causes brittle code and inconsistency.
In addition,getMotherRecordsForAssociate()always expects a language filter, so you needed to introduce another repo method for the “no-language” path and ended up falling back tomotherRecordRepo. Re-using the existinggetMotherRecordsForANM()(or adding an overload without the language predicate) would keep the service simpler and prevent repository explosion.
624-644: Branch explosion inmoveAllocatedCallsToBinThe nested
if/else iftree is growing quickly (ANM vs ASSOCIATE vs default).
Consider extracting role–specific behaviour into separate strategy classes or at
least a helper that takes(role, recordType, …)and returns the page. This
keeps the method readable and makes future role additions trivial.
695-725: Overloaded helper names are confusing
getOutboundCallsForMotherAssociate&getOutboundCallsForChildAssociatewrap
two repo calls that differ only by an optional language filter. The same
pattern appears many times in the repo interface.Overloading at the repository layer is already complicated; wrapping the same
idea again in the service adds another indirection. Either:
- Expose one repo method with an
@Nullable String preferredLanguage
argument and build the query dynamically (@Query→@Query(nativeQuery…)
CASE WHEN), or- Keep two clearly named repo methods (
…WithLanguage,…WithoutLanguage)
and call them directly.Both options avoid surprise overload resolution and naming duplication.
745-772: Duplicate method names hide intentCalling
outboundCallsRepo.getAllocatedRecordsCountMotherUserAssociate(…)with
either five or six parameters relies on readers remembering which overload
includes a language filter. Give the methods semantic names, e.g.
…AssociateByLanguageto make call-sites self-documenting and reduce the risk
of accidentally skipping the filter.src/main/java/com/iemr/ecd/repo/call_conf_allocation/OutboundCallsRepo.java (2)
65-70:getMotherRecordsForAssociateduplicates ANM queryThe JPQL is byte-for-byte identical to
getMotherRecordsForANM. Unless the
logic needs to diverge later, keep one method and reuse it; duplicate queries
bloat the interface and maintenance surface.
237-252: Repository interface growing uncontrollablyYou now have four variants of “Mother records by phone type”:
- default
- ANM
- ASSOCIATE with language
- ASSOCIATE without language
Factor out the common part—build the predicate dynamically with
@Query+ SpEL
or useQuerydsl/Specifications. The current approach is not scalable and
risks hitting method-name length limits.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/main/environment/ecd_ci.properties(1 hunks)src/main/environment/ecd_example.properties(1 hunks)src/main/environment/ecd_uat.properties(1 hunks)src/main/java/com/iemr/ecd/repo/call_conf_allocation/ChildRecordRepo.java(1 hunks)src/main/java/com/iemr/ecd/repo/call_conf_allocation/MotherRecordRepo.java(1 hunks)src/main/java/com/iemr/ecd/repo/call_conf_allocation/OutboundCallsRepo.java(5 hunks)src/main/java/com/iemr/ecd/service/associate/CallClosureImpl.java(2 hunks)src/main/java/com/iemr/ecd/service/call_conf_allocation/CallAllocationImpl.java(9 hunks)src/main/java/com/iemr/ecd/utils/mapper/JwtUserIdValidationFilter.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/main/environment/ecd_ci.properties (1)
Learnt from: ravishanigarapu
PR: PSMRI/ECD-API#55
File: src/main/environment/ecd_uat.properties:19-20
Timestamp: 2025-01-22T12:20:25.782Z
Learning: In the ECD-API project, Swagger UI and API docs should be enabled (set to true) in all lower environments (DEV, TEST, UAT) to facilitate API testing and documentation. Only the production environment should have these disabled.
🧬 Code Graph Analysis (2)
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-21)
src/main/java/com/iemr/ecd/service/associate/CallClosureImpl.java (1)
src/main/java/com/iemr/ecd/utils/constants/Constants.java (1)
Constants(5-21)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (3)
src/main/environment/ecd_uat.properties (1)
25-25: LGTM - Minor whitespace addition is acceptableThis change only adds a blank line at the end of the file, which is a common convention for ensuring files end with a newline.
src/main/environment/ecd_ci.properties (1)
28-29: Parameterizing Swagger configuration is a good approachThe change allows dynamic control of Swagger and Springdoc API features via environment variables, which is more flexible than hardcoded values. This aligns with the PR's focus on preferred language changes and enhances configurability for different environments.
src/main/environment/ecd_example.properties (1)
19-22: Enabling Swagger documentation for development environmentEnabling Swagger UI and API docs in the example properties file is appropriate and aligns with the retrieved learning that API documentation should be enabled in non-production environments to facilitate testing and development.
| } else if (null != childRecordsForAssociate && !childRecordsForAssociate.isEmpty()) { | ||
| outBoundCallList = childRecordsForAssociate.getContent(); | ||
|
|
||
| if (!outBoundCallList.isEmpty()) { | ||
| for (OutboundCalls outboundCall : outBoundCallList) { | ||
| try { | ||
| outboundCall.setAllocationStatus(Constants.ALLOCATED); | ||
| outboundCall.setAllocatedUserId(callAllocationDto.getToUserIds()[callCountPointer | ||
| / callAllocationDto.getNoOfCalls()]); | ||
|
|
||
| outboundCall.setCallAttemptNo(0); | ||
|
|
||
| callCountPointer++; | ||
| } catch (Exception e) { | ||
| callCountPointer++; | ||
| } | ||
| } | ||
| outboundCallsRepo.saveAll(outBoundCallList); | ||
| } | ||
| } else { | ||
| throw new ECDException("no eligible record available to allocate, please contact administrator"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same “zero records” edge-case applies to child allocation
The branch that handles childRecordsForAssociate can also end up returning a
success message with an empty list. Mirror the guard added for the mother
path.
| @Query(value = " SELECT COUNT(1) FROM OutboundCalls AS t INNER JOIN MotherRecord AS mv ON t.motherId = mv.ecdIdNo WHERE t.allocatedUserId=:allocatedUserId AND " | ||
| + " ((:fDate between t.callDateFrom AND t.callDateTo) OR (:tDate between t.callDateFrom AND t.callDateTo)) AND t.callStatus=:callStatus " | ||
| + " AND t.phoneNumberType=:phoneNoType AND t.childId IS NULL AND t.motherId IS NOT NULL AND mv.preferredLanguage = :preferredLanguage") | ||
| int getAllocatedRecordsCountMotherUserAssociate(@Param("allocatedUserId") Integer allocatedUserId, | ||
| @Param("fDate") Timestamp fDate, @Param("tDate") Timestamp tDate, @Param("callStatus") String callStatus, | ||
| @Param("phoneNoType") String phoneNoType,@Param("preferredLanguage") String preferredLanguage); | ||
|
|
||
| @Query(value = " SELECT COUNT(1) FROM OutboundCalls AS t INNER JOIN MotherRecord AS mv ON t.motherId = mv.ecdIdNo WHERE t.allocatedUserId=:allocatedUserId AND " | ||
| + " ((:fDate between t.callDateFrom AND t.callDateTo) OR (:tDate between t.callDateFrom AND t.callDateTo)) AND t.callStatus=:callStatus " | ||
| + " AND t.phoneNumberType=:phoneNoType AND t.childId IS NULL AND t.motherId IS NOT NULL") | ||
| int getAllocatedRecordsCountMotherUserAssociate(@Param("allocatedUserId") Integer allocatedUserId, | ||
| @Param("fDate") Timestamp fDate, @Param("tDate") Timestamp tDate, @Param("callStatus") String callStatus, | ||
| @Param("phoneNoType") String phoneNoType); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ambiguous overloads may trip developers
Two methods named getAllocatedRecordsCountMotherUserAssociate differ only by
the presence of preferredLanguage. Spring Data JPA supports overloading, but
the names give no clue which version is in use, and IDE autocomplete often
lists them consecutively without distinction.
Rename to something expressive, e.g.
getAllocatedRecordsCountMotherUserAssociateByLanguage and
getAllocatedRecordsCountMotherUserAssociateAllLang.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/com/iemr/ecd/service/call_conf_allocation/CallAllocationImpl.java (1)
285-306: Same "zero records" edge-case applies to child allocationThe branch that handles
childRecordsForAssociatecan also end up returning a success message with an empty list. Mirror the guard added for the mother path.
🧹 Nitpick comments (1)
src/main/java/com/iemr/ecd/service/call_conf_allocation/CallAllocationImpl.java (1)
683-696: Method naming inconsistencyThere's an inconsistency in the method naming convention between this method and the one at line 698.
- private Page<OutboundCalls> getOutboundCallsForMotherAssociate(Pageable pageable, + private Page<OutboundCalls> getOutboundCallsForMotherAssociate(Pageable pageable,- private Page<OutboundCalls> getOutboundcallsForChildAssociate(Pageable pageable, + private Page<OutboundCalls> getOutboundCallsForChildAssociate(Pageable pageable,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/iemr/ecd/repo/call_conf_allocation/OutboundCallsRepo.java(5 hunks)src/main/java/com/iemr/ecd/service/associate/CallClosureImpl.java(3 hunks)src/main/java/com/iemr/ecd/service/call_conf_allocation/CallAllocationImpl.java(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/iemr/ecd/service/associate/CallClosureImpl.java
- src/main/java/com/iemr/ecd/repo/call_conf_allocation/OutboundCallsRepo.java
🧰 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-21)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (6)
src/main/java/com/iemr/ecd/service/call_conf_allocation/CallAllocationImpl.java (6)
125-191: Good implementation of preferred language filtering logicThe implementation correctly branches based on whether a preferred language is specified, using the appropriate repository methods. The code also properly throws exceptions when no eligible records are found.
228-235: Good implementation of child record language filteringThe conditional logic for fetching either language-filtered or regular child records is implemented correctly.
683-713: Good extraction of common repository query logicThese helper methods effectively encapsulate the repository calls for fetching records, making the code more modular and maintainable.
736-747: Good implementation of associate role handlingThe code correctly handles the associate role by implementing separate logic branches with appropriate repository calls based on preferred language presence.
752-760: Good implementation of child record counts for associatesThe logic correctly branches based on preferred language presence and uses the appropriate repository methods.
612-631: Good integration of associate role in bin operationThe implementation correctly integrates the associate role with proper repository call delegation to helper methods.
| // Assuming updateIsAllocatedStatus updates all in one batch | ||
| } | ||
|
|
||
| outboundCallsRepo.saveAll(outBoundCallList); |
There was a problem hiding this comment.
Redundant database call
This saveAll operation is redundant as it's already handled in both conditional branches (lines 188 and 151). This could lead to unnecessary database operations and potential performance issues.
- outboundCallsRepo.saveAll(outBoundCallList);📝 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.
| outboundCallsRepo.saveAll(outBoundCallList); |
2 participants
📋 Description
JIRA ID: AMM-1295,AMM-1296,AMM-1297,AMM-1298
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
Chores