Constants added and null check#77
Conversation
WalkthroughString literals for JWT token, user agent, and okhttp identifiers have been centralized into new constants within the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant JwtUserIdValidationFilter
participant RestTemplateUtil
Client->>JwtUserIdValidationFilter: Sends HTTP request with headers/cookies
JwtUserIdValidationFilter->>JwtUserIdValidationFilter: Reads JWT token and User-Agent using constants
JwtUserIdValidationFilter->>RestTemplateUtil: Forwards request details
RestTemplateUtil->>RestTemplateUtil: Checks for JWT token header (using constant)
RestTemplateUtil->>Client: Forwards request (if JWT token present, adds header)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/iemr/ecd/utils/constants/Constants.java(1 hunks)src/main/java/com/iemr/ecd/utils/mapper/JwtUserIdValidationFilter.java(4 hunks)src/main/java/com/iemr/ecd/utils/mapper/RestTemplateUtil.java(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/main/java/com/iemr/ecd/utils/mapper/RestTemplateUtil.java (1)
src/main/java/com/iemr/ecd/utils/constants/Constants.java (1)
Constants(5-24)
src/main/java/com/iemr/ecd/utils/mapper/JwtUserIdValidationFilter.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 (8)
src/main/java/com/iemr/ecd/utils/constants/Constants.java (1)
19-21: LGTM! Constants properly defined.The new constants are well-defined and follow proper naming conventions. Centralizing these string literals improves maintainability and reduces the risk of typos in header and cookie name usage across the codebase.
src/main/java/com/iemr/ecd/utils/mapper/RestTemplateUtil.java (2)
13-13: LGTM! Import added for constants.Good addition of the Constants import to support the refactoring to use centralized string literals.
44-46: Excellent defensive programming with null check.The null check before adding the JWT token header is a good practice that prevents potential issues with null header values. The use of
Constants.JWT_TOKENalso improves maintainability.src/main/java/com/iemr/ecd/utils/mapper/JwtUserIdValidationFilter.java (5)
9-9: LGTM! Import added for constants.Good addition of the Constants import to support the refactoring effort.
71-71: Good refactoring to use constant.Replacing the hardcoded
"JwtToken"string withConstants.JWT_TOKENimproves maintainability and consistency.
91-91: Good refactoring to use constant.Replacing the hardcoded
"User-Agent"string withConstants.USER_AGENTimproves maintainability and consistency.
116-116: Good refactoring to use constant.Replacing the hardcoded
"okhttp"string withConstants.OKHTTPimproves maintainability and consistency for mobile client detection.
122-122: Good refactoring to use constant.Replacing the hardcoded cookie name with
Constants.JWT_TOKENensures consistency between header and cookie name usage throughout the application.
| headers.add(Constants.JWT_TOKEN,requestHeader.getHeader(Constants.JWT_TOKEN)); | ||
| } | ||
| if(null != jwtTokenFromCookie) { | ||
| headers.add(HttpHeaders.COOKIE, "Jwttoken=" + jwtTokenFromCookie); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix inconsistent string literal usage.
Line 48 still uses a hardcoded string "Jwttoken=" instead of using the constant. For consistency with the refactoring effort, this should use the constant as well.
Apply this diff to maintain consistency:
- headers.add(HttpHeaders.COOKIE, "Jwttoken=" + jwtTokenFromCookie);
+ headers.add(HttpHeaders.COOKIE, Constants.JWT_TOKEN + "=" + jwtTokenFromCookie);📝 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.
| headers.add(HttpHeaders.COOKIE, "Jwttoken=" + jwtTokenFromCookie); | |
| headers.add(HttpHeaders.COOKIE, Constants.JWT_TOKEN + "=" + jwtTokenFromCookie); |
🤖 Prompt for AI Agents
In src/main/java/com/iemr/ecd/utils/mapper/RestTemplateUtil.java at line 48,
replace the hardcoded string "Jwttoken=" with the appropriate constant defined
for this token to maintain consistency with the rest of the code. Identify the
constant representing the JWT token prefix and use it in the headers.add method
instead of the literal string.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
src/main/java/com/iemr/ecd/repo/call_conf_allocation/OutboundCallsRepo.java (2)
95-124: 🛠️ Refactor suggestionApply the
isFurtherCallRequiredcondition consistently across all unallocated count queries.The changes correctly add the
isFurtherCallRequiredfiltering condition to mother-related count queries. However, there's an inconsistency issue:
- Child-related count queries (
getChildUnAllocatedCountLRat line 106 andgetChildUnAllocatedCountHRat line 121) do not have this condition- Language-specific mother count query (
getMotherUnAllocatedCountLRByLanguageat line 346) also lacks this conditionThis inconsistency could lead to mismatched counts between different query methods.
Consider applying the same filtering logic to maintain consistency:
// un-allocated, child low risk, @Query(value = " SELECT COUNT(1) FROM OutboundCalls AS t WHERE t.allocationStatus =:allocationStatus AND " + " t.psmId=:psmId AND ((:fDate between t.callDateFrom AND t.callDateTo) OR (:tDate between t.callDateFrom AND t.callDateTo)) AND " + " t.childId IS NOT NULL AND (t.isHrni = false OR t.isHrni IS NULL ) " - + " AND t.phoneNumberType=:phoneNoType AND t.deleted = false ") + + " AND t.phoneNumberType=:phoneNoType AND t.deleted = false AND (t.isFurtherCallRequired = true OR t.isFurtherCallRequired IS NULL ) ") int getChildUnAllocatedCountLR(@Param("allocationStatus") String allocationStatus, @Param("psmId") Integer psmId, @Param("fDate") Timestamp fDate, @Param("tDate") Timestamp tDate, @Param("phoneNoType") String phoneNoType); // un-allocated, child high risk, @Query(value = " SELECT COUNT(1) FROM OutboundCalls AS t WHERE t.allocationStatus =:allocationStatus AND " + " t.psmId=:psmId AND ((:fDate between t.callDateFrom AND t.callDateTo) OR (:tDate between t.callDateFrom AND t.callDateTo)) AND " - + " t.childId IS NOT NULL AND t.isHrni = true " + " AND t.phoneNumberType=:phoneNoType AND t.deleted = false ") + + " t.childId IS NOT NULL AND t.isHrni = true " + " AND t.phoneNumberType=:phoneNoType AND t.deleted = false AND (t.isFurtherCallRequired = true OR t.isFurtherCallRequired IS NULL )") int getChildUnAllocatedCountHR(@Param("allocationStatus") String allocationStatus, @Param("psmId") Integer psmId, @Param("fDate") Timestamp fDate, @Param("tDate") Timestamp tDate, @Param("phoneNoType") String phoneNoType);
342-355: 🛠️ Refactor suggestionAdd the
isFurtherCallRequiredcondition to language-specific count queries.The language-specific count queries should also include the
isFurtherCallRequiredfiltering condition for consistency with the updated non-language-specific queries.Apply this diff to maintain consistency:
@Query(value = " SELECT COUNT(1) FROM OutboundCalls AS t INNER JOIN MotherRecord AS mv ON t.motherId = mv.ecdIdNo WHERE t.allocationStatus =:allocationStatus 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 = false OR t.isHighRisk IS NULL ) " - + " AND t.phoneNumberType=:phoneNoType AND t.deleted = false AND mv.preferredLanguage = :preferredLanguage ") + + " AND t.phoneNumberType=:phoneNoType AND t.deleted = false AND mv.preferredLanguage = :preferredLanguage AND (t.isFurtherCallRequired = true OR t.isFurtherCallRequired IS NULL ) ") int getMotherUnAllocatedCountLRByLanguage(@Param("allocationStatus") String allocationStatus, @Param("psmId") Integer psmId, @Param("fDate") Timestamp fDate, @Param("tDate") Timestamp tDate, @Param("phoneNoType") String phoneNoType, @Param("preferredLanguage") String preferredLanguage); // un-allocated, child low risk, @Query(value = " SELECT COUNT(1) FROM OutboundCalls AS t INNER JOIN ChildRecord AS cv ON t.childId = cv.ecdIdNoChildId WHERE t.allocationStatus =:allocationStatus AND " + " t.psmId=:psmId AND ((:fDate between t.callDateFrom AND t.callDateTo) OR (:tDate between t.callDateFrom AND t.callDateTo)) AND " + " t.childId IS NOT NULL AND (t.isHrni = false OR t.isHrni IS NULL ) " - + " AND t.phoneNumberType=:phoneNoType AND t.deleted = false AND cv.preferredLanguage = :preferredLanguage") + + " AND t.phoneNumberType=:phoneNoType AND t.deleted = false AND cv.preferredLanguage = :preferredLanguage AND (t.isFurtherCallRequired = true OR t.isFurtherCallRequired IS NULL )") int getChildUnAllocatedCountLRByLanguage(@Param("allocationStatus") String allocationStatus, @Param("psmId") Integer psmId, @Param("fDate") Timestamp fDate, @Param("tDate") Timestamp tDate, @Param("phoneNoType") String phoneNoType, @Param("preferredLanguage") String preferredLanguage);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
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(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (1)
src/main/java/com/iemr/ecd/repo/call_conf_allocation/MotherRecordRepo.java (1)
1-116:⚠️ Potential issueCritical: File changes don't match PR objectives
The PR is titled "Constants added and null check" and the AI summary mentions JWT token constants and HTTP utility updates, but this file contains only a database query modification for mother records. This appears to be either:
- An incorrect file included in the PR
- An incorrect PR title/description
- A merge issue
Please clarify the intended changes for this PR.
Likely an incorrect or invalid review comment.
| @Query(value = " SELECT COUNT(1) FROM MotherRecord as t WHERE t.isAllocated=:isAllocated AND " | ||
| + " t.createdDate >=:fDate AND t.createdDate <=:tDate AND t.whomPhoneNo=:whomPhoneNo ") | ||
| + " t.createdDate >=:fDate AND t.createdDate <=:tDate AND t.whomPhoneNo=:whomPhoneNo AND (t.isFurtherCallRequired = true OR t.isFurtherCallRequired IS NULL ) ") | ||
| public int getRecordCount(@Param("isAllocated") Boolean isAllocated, @Param("fDate") Timestamp fDate, | ||
| @Param("tDate") Timestamp tDate, @Param("whomPhoneNo") String whomPhoneNo); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the business logic change for record filtering
The query modification adds filtering for isFurtherCallRequired field, excluding records where this flag is explicitly set to false. While the syntax is correct, please ensure:
- This business logic change is intended and documented
- All consumers of
getRecordCountmethod are aware of this behavioral change - This aligns with similar filtering in other repository methods
Run the following script to verify consistent filtering across related repositories:
🏁 Script executed:
#!/bin/bash
# Description: Check if similar isFurtherCallRequired filtering is applied consistently across other repository methods
# Search for other queries with isFurtherCallRequired conditions
rg -A 3 -B 3 "isFurtherCallRequired" --type java
# Search for other uses of getRecordCount to understand impact
ast-grep --pattern 'getRecordCount($$$)'Length of output: 24457
Review update: Filter consistency verified
The added filter for isFurtherCallRequired in MotherRecordRepo#getRecordCount matches the same condition used throughout OutboundCallsRepo, so the technical change is correct.
Please address the following:
- Document this business logic change (that records with
isFurtherCallRequired = falseare now excluded) in your API/repo documentation. - Communicate the updated behavior to all consumers of
getRecordCount.
Affected location:
- src/main/java/com/iemr/ecd/repo/call_conf_allocation/MotherRecordRepo.java (lines 66–69)
🤖 Prompt for AI Agents
In src/main/java/com/iemr/ecd/repo/call_conf_allocation/MotherRecordRepo.java
between lines 66 and 69, the query now filters records where
isFurtherCallRequired is true or null, excluding false values. This change
affects the business logic by omitting records with isFurtherCallRequired set to
false. Add documentation to clarify that getRecordCount now only counts records
with isFurtherCallRequired true or null, and ensure all consumers are aware of
this behavioral change to prevent misunderstandings.
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
isFurtherCallRequired.