PreferredLanguage changes - #45
Conversation
WalkthroughThe changes in this pull request introduce new methods for updating phone numbers in both the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HttpInterceptor
participant CallClosureImpl
participant ChildRecordRepo
participant MotherRecordRepo
Client->>HttpInterceptor: Send request with Authorization
HttpInterceptor->>HttpInterceptor: Validate Authorization key
alt Invalid key
HttpInterceptor-->>Client: Return error response
else Valid key
HttpInterceptor->>CallClosureImpl: Call closeCall()
CallClosureImpl->>ChildRecordRepo: updateCorrectPhoneNumber()
CallClosureImpl->>MotherRecordRepo: updateCorrectPhoneNumber()
CallClosureImpl-->>HttpInterceptor: Process complete
HttpInterceptor-->>Client: Return success response
end
Warning Tool Failures:Tool Failure Count: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 (
|
|
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
src/main/java/com/iemr/ecd/utils/http_request_interceptor/HttpInterceptor.java (2)
88-90: Add performance optimization for token validation.The current implementation performs a Redis lookup for every request. Consider adding a local cache with a short TTL to reduce Redis load.
Consider implementing:
- A local cache (e.g., Caffeine) with a short TTL (e.g., 1 minute)
- Batch prefetching of session data
- Redis connection pooling if not already implemented
Line range hint
47-90: Improve token extraction and validation logic.The current token extraction and validation process has several areas for improvement:
- Inconsistent Bearer token handling
- No validation of token format
- Generic exception handling
Consider implementing these security improvements:
- String authorization = null; - String preAuth = request.getHeader("Authorization"); - if(null != preAuth && preAuth.contains("Bearer ")) - authorization=preAuth.replace("Bearer ", ""); - else - authorization = preAuth; + String authorization = extractAndValidateToken(request); + + private String extractAndValidateToken(HttpServletRequest request) throws SecurityException { + String authHeader = request.getHeader("Authorization"); + if (authHeader == null || authHeader.isBlank()) { + throw new SecurityException("Missing authorization header"); + } + + if (!authHeader.startsWith("Bearer ")) { + throw new SecurityException("Invalid authorization header format"); + } + + String token = authHeader.substring(7); + if (!token.matches("^[a-zA-Z0-9-._~+/]+=*$")) { + throw new SecurityException("Invalid token format"); + } + + return token; + }src/main/java/com/iemr/ecd/repo/call_conf_allocation/ChildRecordRepo.java (1)
96-99: Consider adding transaction isolation level and parameter validation.The method could benefit from:
- Explicit transaction isolation level for concurrent updates
- Parameter validation to prevent null/empty phone numbers
- Index hints for optimal query performance
Consider these improvements:
@Modifying -@Transactional +@Transactional(isolation = Isolation.READ_COMMITTED) @Query(" UPDATE ChildRecord SET phoneNo = :correctPhoneNumber WHERE ecdIdNoChildId = :childId ") public int updateCorrectPhoneNumber(@Param("correctPhoneNumber") String correctPhoneNumber,@Param("childId") Long childId);src/main/java/com/iemr/ecd/repo/call_conf_allocation/MotherRecordRepo.java (1)
99-102: Consider adding Javadoc for the new method.Adding documentation would help other developers understand:
- The purpose of this method
- When it should be used
- Parameter descriptions
Add documentation like this:
+ /** + * Updates the phone number for a mother record. + * + * @param correctPhoneNumber The verified/corrected phone number to update + * @param motherId The unique identifier of the mother record + */ @Modifying @Transactional @Query(" UPDATE MotherRecord SET phoneNo = :correctPhoneNumber WHERE ecdIdNo = :motherId ") public void updateCorrectPhoneNumber(@Param("correctPhoneNumber") String correctPhoneNumber, @Param("motherId") Long motherId);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
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/service/associate/CallClosureImpl.java(5 hunks)src/main/java/com/iemr/ecd/utils/http_request_interceptor/HttpInterceptor.java(1 hunks)
🔇 Additional comments (6)
src/main/java/com/iemr/ecd/utils/http_request_interceptor/HttpInterceptor.java (1)
88-90: Verify security configuration and integration.
Let's verify the security configuration and integration with other components.
✅ Verification successful
Security configuration and Redis session integration are properly configured
Based on the verification results:
- The HttpInterceptor is properly registered in HttpInterceptorConfig and applies to all endpoints
- Redis session storage is configured with:
- Redis host and port settings in application.properties
- Session expiry time of 7200 seconds
- All REST endpoints require Authorization header
- Redis session validation is integrated into the security flow
The interceptor's Redis session validation is a valid security measure, working alongside Spring's session management and properly integrated into the application's security architecture.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for security configuration and dependencies
echo "Checking security configuration..."
rg -l "WebSecurityConfigurerAdapter|SecurityFilterChain|SecurityContextHolder"
echo "Checking for other interceptors and filters..."
fd -e java -x grep -l "implements.*Filter\|implements.*Interceptor" {}
echo "Checking for Redis configuration..."
rg -l "RedisTemplate|RedisConnectionFactory"
Length of output: 546
Script:
#!/bin/bash
# Let's check for Redis configuration and usage patterns
echo "Checking Redis configuration files..."
fd -e java -e xml -e properties -e yml -x grep -l "redis|Redis" {}
echo "Checking Redis session storage implementation..."
rg -A 5 "class.*RedisStorage"
echo "Checking WebMvc configuration for interceptor registration..."
rg -A 5 "implements.*WebMvcConfigurer|extends.*WebMvcConfigurerAdapter"
echo "Checking Spring security annotations..."
rg "@Security|@Secured|@PreAuthorize"
Length of output: 2151
Script:
#!/bin/bash
# Let's check the interceptor configuration and Redis session implementation
echo "Checking HttpInterceptorConfig implementation..."
rg -A 10 "public class HttpInterceptorConfig"
echo "Checking Redis session storage methods..."
rg -A 5 "getSessionObject|setSessionObject" src/main/java/com/iemr/ecd/utils/redis/RedisStorage.java
echo "Checking application properties for Redis configuration..."
fd -e properties -e yml -x cat {}
echo "Checking for authentication/security related endpoints..."
rg -A 5 "@RestController|@Controller"
Length of output: 20980
src/main/java/com/iemr/ecd/repo/call_conf_allocation/ChildRecordRepo.java (1)
96-99: Verify the impact on existing phone number update flows.
Since this method is part of a larger change involving phone number updates, we should verify:
- Integration with CallClosureImpl
- Existing phone number validation logic
- Impact on reporting/auditing
✅ Verification successful
Phone number update implementation is properly integrated
The verification shows that the updateCorrectPhoneNumber method is:
- Correctly integrated with
CallClosureImplwhich handles both mother and child record updates - Part of a consistent pattern across
ChildRecordRepoandMotherRecordRepo - Used within a transaction that includes proper success logging
The implementation aligns with existing phone number update flows and maintains consistency with the codebase's patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing phone number update implementations
echo "Searching for phone number update patterns..."
rg -t java "UPDATE.*SET.*phone.*=" --type java
# Look for related validation logic
echo "Searching for phone number validation..."
rg -t java "validatePhoneNumber|phoneNumberValidation" --type java
Length of output: 2464
Script:
#!/bin/bash
# Let's check for phone number related service implementations and validations
echo "Searching for phone number related service implementations..."
rg -t java -A 5 "updateCorrectPhoneNumber|updatePhoneNumber" --type java
# Look for call closure implementations
echo "Searching for CallClosureImpl..."
rg -t java -A 10 "class CallClosureImpl" --type java
# Check for any phone number related audit or logging
echo "Searching for audit/logging related to phone numbers..."
rg -t java "phone.*log|audit.*phone" --type java
Length of output: 3140
src/main/java/com/iemr/ecd/repo/call_conf_allocation/MotherRecordRepo.java (2)
99-102: LGTM! The implementation follows best practices.
The method is properly annotated with @Modifying and @transactional, and uses parameterized queries for safe database operations.
99-102: Verify the integration with CallClosureImpl.
The method appears to be part of a larger change involving phone number updates. Let's verify the integration points.
✅ Verification successful
Let me check the actual integration in CallClosureImpl.java to verify how updateCorrectPhoneNumber is being used.
Integration with CallClosureImpl is properly implemented
The integration is correctly implemented in CallClosureImpl.java where:
- The method is called within appropriate null checks for motherId
- It's used alongside childRecordRepo's similar method for consistent phone number updates
- The implementation follows the same pattern as other updates (like preferredLanguage)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of updateCorrectPhoneNumber to verify proper integration
rg -l "updateCorrectPhoneNumber" | grep -i "CallClosure"
Length of output: 120
Script:
#!/bin/bash
# Get the context of updateCorrectPhoneNumber usage in CallClosureImpl
rg -A 5 -B 5 "updateCorrectPhoneNumber" "src/main/java/com/iemr/ecd/service/associate/CallClosureImpl.java"
Length of output: 809
src/main/java/com/iemr/ecd/service/associate/CallClosureImpl.java (2)
52-52: LGTM: Constants import added appropriately
The addition of Constants import aligns with best practices for maintaining string literals as constants.
182-188: Verify ANM role handling logic
The special handling for ANM role resets the call attempt counter and reopens allocation. This could have significant workflow implications.
Consider adding a comment explaining why ANM role requires this special handling for better maintainability.
✅ Verification successful
ANM role handling is consistent with special allocation workflow
The special handling for ANM role in call closure is consistent with the broader system design. The codebase shows ANM (Auxiliary Nurse Midwife) has dedicated allocation paths for both mother and child records that specifically consider preferred language:
- Separate allocation methods
allocateMotherRecordsToANMandallocateChildRecordsToANM - Special database queries like
getMotherRecordsForANMandgetChildRecordsForANM - Distinct record counting methods for ANM users that factor in language preference
The call attempt reset and reopening behavior aligns with this specialized workflow where ANM users handle language-specific allocations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other ANM role-specific handling to ensure consistency
rg -l "Constants.ANM" | xargs rg -A 5 -B 5 "ANM"
Length of output: 21004
| String userRespFromRedis = redisStorage.getSessionObject(authorization); | ||
| if(userRespFromRedis == null) | ||
| throw new Exception("invalid Authorization key, please pass a valid key to proceed further. "); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider security hardening improvements.
While the Redis validation is a good addition, consider these security improvements:
- Use constant-time comparison for token validation to prevent timing attacks
- Make error messages generic to avoid information disclosure
- Add rate limiting for failed attempts
Apply this diff to implement the suggestions:
- String userRespFromRedis = redisStorage.getSessionObject(authorization);
- if(userRespFromRedis == null)
- throw new Exception("invalid Authorization key, please pass a valid key to proceed further. ");
+ try {
+ String userRespFromRedis = redisStorage.getSessionObject(authorization);
+ if (!MessageDigest.isEqual(
+ userRespFromRedis.getBytes(),
+ redisStorage.getSessionObject(authorization).getBytes())) {
+ throw new SecurityException("Authentication failed");
+ }
+ } catch (Exception e) {
+ throw new SecurityException("Authentication failed");
+ }Committable suggestion skipped: line range outside the PR's diff.
| @Modifying | ||
| @Transactional | ||
| @Query(" UPDATE ChildRecord SET phoneNo = :correctPhoneNumber WHERE ecdIdNoChildId = :childId ") | ||
| public void updateCorrectPhoneNumber(@Param("correctPhoneNumber") String correctPhoneNumber,@Param("childId") Long childId); |
There was a problem hiding this comment.
Return type should be consistent with other update methods.
The method should return int to indicate the number of rows affected, consistent with other update methods in this repository (e.g., updatePreferredLanguage, updateIsAllocatedStatus).
Apply this change:
-public void updateCorrectPhoneNumber(@Param("correctPhoneNumber") String correctPhoneNumber,@Param("childId") Long childId);
+public int updateCorrectPhoneNumber(@Param("correctPhoneNumber") String correctPhoneNumber,@Param("childId") Long childId);📝 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.
| @Modifying | |
| @Transactional | |
| @Query(" UPDATE ChildRecord SET phoneNo = :correctPhoneNumber WHERE ecdIdNoChildId = :childId ") | |
| public void updateCorrectPhoneNumber(@Param("correctPhoneNumber") String correctPhoneNumber,@Param("childId") Long childId); | |
| @Modifying | |
| @Transactional | |
| @Query(" UPDATE ChildRecord SET phoneNo = :correctPhoneNumber WHERE ecdIdNoChildId = :childId ") | |
| public int updateCorrectPhoneNumber(@Param("correctPhoneNumber") String correctPhoneNumber,@Param("childId") Long childId); |
| if(!StringUtil.isNullOrEmpty(request.getCorrectPhoneNumber())) { | ||
| obj.setPhoneNo(request.getCorrectPhoneNumber()); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling robustness
The current error handling could be more specific and robust:
- Generic exception handling masks specific errors
- Inconsistent null checks could lead to NPEs
Consider:
- Adding specific exception handling for different error scenarios
- Consistent null checks before operations
- Proper error messages for each failure case
try {
// ... existing code ...
+} catch (InvalidRequestException e) {
+ logger.error("Invalid request data: {}", e.getMessage());
+ throw e;
+} catch (IllegalStateException e) {
+ logger.error("Invalid state: {}", e.getMessage());
+ throw new ECDException("Failed to process call closure: " + e.getMessage());
} catch (Exception e) {
+ logger.error("Unexpected error during call closure: {}", e.getMessage());
throw new ECDException(e);
}Also applies to: 224-231
| if(!StringUtil.isNullOrEmpty(request.getCorrectPhoneNumber())) { | ||
| if (null != callObj.getMotherId() && callObj.getChildId() == null) { | ||
| motherRecordRepo.updateCorrectPhoneNumber(request.getCorrectPhoneNumber(), callObj.getMotherId()); | ||
| } | ||
| if (callObj.getMotherId() != null && callObj.getChildId() != null) { | ||
| childRecordRepo.updateCorrectPhoneNumber(request.getCorrectPhoneNumber(), callObj.getChildId()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add phone number format validation
The phone number update logic lacks format validation before updating the records. This could lead to invalid phone numbers being stored.
Consider:
- Adding phone number format validation
- Logging the phone number changes for audit purposes
- Adding a comment explaining the business logic for updating both mother and child records
Example validation:
if(!StringUtil.isNullOrEmpty(request.getCorrectPhoneNumber())) {
+ if (!request.getCorrectPhoneNumber().matches("^\\d{10}$")) {
+ throw new InvalidRequestException("Invalid phone number format");
+ }
if (null != callObj.getMotherId() && callObj.getChildId() == null) {
+ logger.info("Updating mother's phone number for motherId: {}", callObj.getMotherId());
motherRecordRepo.updateCorrectPhoneNumber(request.getCorrectPhoneNumber(), callObj.getMotherId());
}
if (callObj.getMotherId() != null && callObj.getChildId() != null) {
+ logger.info("Updating child's phone number for childId: {}", callObj.getChildId());
childRecordRepo.updateCorrectPhoneNumber(request.getCorrectPhoneNumber(), callObj.getChildId());
}
}Committable suggestion skipped: line range outside the PR's diff.
| if (request.getIsHrp() != null) { | ||
| callObj.setIsHighRisk(request.getIsHrp()); | ||
| } | ||
| // else { | ||
| // callObj.setIsHrni(request.getIsHrni()); | ||
| // } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance high-risk status change handling
Changes to high-risk status (HRP/HRNI) should be tracked for medical audit purposes.
Consider adding:
- Logging of risk status changes
- Reason for risk status change
- Audit trail
if (request.getIsHrp() != null) {
+ logger.info("Updating HRP status for motherId: {} to: {}", callObj.getMotherId(), request.getIsHrp());
callObj.setIsHighRisk(request.getIsHrp());
}
if (request.getIsHrni() != null) {
+ logger.info("Updating HRNI status for childId: {} to: {}", callObj.getChildId(), request.getIsHrni());
callObj.setIsHrni(request.getIsHrni());
}Committable suggestion skipped: line range outside the PR's diff.
2 participants
📋 Description
JIRA ID: AMM-922
Preferred Language Changes
✅ 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
Documentation