role added #90
role added #90
Conversation
WalkthroughThe update adds a new path variable, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as CallAllocationController
participant Service as CallAllocationImpl
Client->>Controller: GET /.../{psmId}/{phoneNoType}/{recordType}/{fDate}/{tDate}/{preferredLanguage}/{role}
Controller->>Service: getEligibleRecordsLanguageInfo(psmId, phoneNoType, recordType, fDate, tDate, preferredLanguage, role)
Service-->>Controller: ResponseEligibleCallRecordsDTO
Controller-->>Client: ResponseEntity<ResponseEligibleCallRecordsDTO>
Possibly related PRs
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: 0
🔭 Outside diff range comments (1)
src/main/java/com/iemr/ecd/controller/callallocation/CallAllocationController.java (1)
156-156: 🛠️ Refactor suggestionAdd parameter validation for the role parameter.
The new
roleparameter lacks validation, which could lead to security issues or unexpected behavior if invalid values are passed.Consider adding validation to ensure only valid roles are accepted:
public ResponseEntity<ResponseEligibleCallRecordsDTO> getEligibleRecordsLanguageInfo(@PathVariable int psmId, @PathVariable String phoneNoType, @PathVariable String recordType, @PathVariable String fDate, - @PathVariable String tDate, @PathVariable String preferredLanguage,@PathVariable String role) throws ECDException { + @PathVariable String tDate, @PathVariable String preferredLanguage, + @PathVariable @Pattern(regexp = "^(ADMIN|SUPERVISOR|AGENT|USER)$", message = "Invalid role") String role) throws ECDException {Also consider adding the necessary import for validation:
+import javax.validation.constraints.Pattern;
🧹 Nitpick comments (1)
src/main/java/com/iemr/ecd/controller/callallocation/CallAllocationController.java (1)
145-160: Consider URL design and security implications.Including the role as a path parameter exposes it in URLs, logs, and browser history. Consider if this is the most appropriate approach for role-based access control.
Alternative approaches to consider:
- Use request headers: Pass role information in headers instead of URL path
- Extract from authentication context: Derive the role from the authenticated user's context
- Use request parameters: Pass role as a query parameter if it's optional
Example with header approach:
-@GetMapping(value = "/getEligibleRecordsLanguageInfo/{psmId}/{phoneNoType}/{recordType}/{fDate}/{tDate}/{preferredLanguage}/{role}", produces = MediaType.APPLICATION_JSON_VALUE) +@GetMapping(value = "/getEligibleRecordsLanguageInfo/{psmId}/{phoneNoType}/{recordType}/{fDate}/{tDate}/{preferredLanguage}", produces = MediaType.APPLICATION_JSON_VALUE) public ResponseEntity<ResponseEligibleCallRecordsDTO> getEligibleRecordsLanguageInfo(@PathVariable int psmId, @PathVariable String phoneNoType, @PathVariable String recordType, @PathVariable String fDate, - @PathVariable String tDate, @PathVariable String preferredLanguage,@PathVariable String role) throws ECDException { + @PathVariable String tDate, @PathVariable String preferredLanguage, + @RequestHeader("X-User-Role") String role) throws ECDException {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/iemr/ecd/controller/callallocation/CallAllocationController.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (2)
src/main/java/com/iemr/ecd/controller/callallocation/CallAllocationController.java (2)
145-145:Details
❓ Verification inconclusive
Breaking change: Verify all existing clients are updated.
Adding the
{role}path variable to the endpoint URL is a breaking change. Existing clients calling this endpoint will receive 404 errors until they update their API calls to include the role parameter.Please run the following script to identify potential API consumers that need updating:
🏁 Script executed:
#!/bin/bash # Description: Find references to the old endpoint pattern that need updating # Search for the old endpoint pattern in the codebase rg -A 3 -B 3 "getEligibleRecordsLanguageInfo.*psmId.*phoneNoType.*recordType.*fDate.*tDate.*preferredLanguage" --type java --type js --type ts # Also search for any OpenAPI/Swagger documentation that might reference this endpoint rg -A 5 -B 5 "getEligibleRecordsLanguageInfo" --type yaml --type jsonLength of output: 2397
Breaking change alert: New
rolepath parameter ingetEligibleRecordsLanguageInfoI didn’t find any internal calls or OpenAPI/Swagger docs referencing the old endpoint pattern, which means external clients will break until they include the new
{role}segment. Please verify that:
- All external consumers (web/mobile clients, integration tests, third-party integrations) are updated to call
/getEligibleRecordsLanguageInfo/{psmId}/{phoneNoType}/{recordType}/{fDate}/{tDate}/{preferredLanguage}/{role}- API documentation (Swagger/OpenAPI definitions, Postman collections, README) is updated to reflect the added parameter
- You’ve considered versioning this endpoint (or providing a fallback) if you need to maintain backward compatibility
158-158:Details
✅ Verification successful
Verify service layer method signature compatibility.
The service call passes the new
roleparameter, but we need to ensure the service layer method signature has been updated accordingly.Run the following script to verify the service layer method signature:
🏁 Script executed:
#!/bin/bash # Description: Verify the service layer method signature matches the controller call # Search for the service method implementation ast-grep --pattern 'getEligibleRecordsLanguageInfo($_, $_, $_, $_, $_, $_, $_) { $$$ }' # Also search for the method declaration rg -A 10 "getEligibleRecordsLanguageInfo.*psmId.*phoneNoType.*recordType.*fDate.*tDate.*preferredLanguage.*role" --type javaLength of output: 2957
🏁 Script executed:
#!/bin/bash # Locate the service implementation class where CallAllocationImpl is defined rg -n "class .*CallAllocationImpl" --type javaLength of output: 165
🏁 Script executed:
#!/bin/bash # Verify the service implementation method signature rg -n "getEligibleRecordsLanguageInfo" -A3 src/main/java/com/iemr/ecd/service/call_conf_allocation/CallAllocationImpl.javaLength of output: 416
Service method signature updated to include ‘role’.
Verified in CallAllocationImpl (src/main/java/com/iemr/ecd/service/call_conf_allocation/CallAllocationImpl.java:1057–1059) that
public ResponseEligibleCallRecordsDTO getEligibleRecordsLanguageInfo( int psmId, String phoneNoType, String recordType, String fDate, String tDate, String preferredLanguage, String role ) { … }matches the controller call. No further action required.
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