Dev To Main #209
Dev To Main #209
Conversation
test: add integration tests for PATCH v1/endorsements/{id} endpoint
* fix: ensure empty endorsements msg not allowed * fix: add feature-flag in endorsement controller * fix: update service impl to ensure no unauthorized endorsement update, user-not-found bug * fix: enable disabled test cases * fix: disable endorsements update when not in dev mode * fix: run all tests in dev mode, add test case to validate updation is no allowed in non-dev mode * fix: update name of test case * fix: import only used exceptions * fix: update control flow
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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 (
|
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Unclear Boolean Parameter Name ▹ view | 🧠 Incorrect |
 |
Inconsistent Error Message for User Not Found Scenario ▹ view | |
|
Deep Nesting in Update Method ▹ view | |
 |
Exposed Development Flag in Production API ▹ view | 🧠 Not in scope |
Files scanned
| File Path | Reviewed |
|---|---|
| skill-tree/src/main/java/com/RDS/skilltree/viewmodels/UpdateEndorsementViewModel.java | ✅ |
| skill-tree/src/main/java/com/RDS/skilltree/services/EndorsementService.java | ✅ |
| skill-tree/src/main/java/com/RDS/skilltree/apis/EndorsementsApi.java | ✅ |
| skill-tree/src/main/java/com/RDS/skilltree/services/external/RdsServiceImplementation.java | ✅ |
| skill-tree/src/main/java/com/RDS/skilltree/utils/Constants.java | ✅ |
| skill-tree/src/main/java/com/RDS/skilltree/exceptions/GlobalExceptionHandler.java | ✅ |
| skill-tree/src/main/java/com/RDS/skilltree/services/EndorsementServiceImplementation.java | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
|
|
||
| EndorsementViewModel update(Integer endorsementId, UpdateEndorsementViewModel endorsement); | ||
| EndorsementViewModel update( | ||
| Integer endorsementId, UpdateEndorsementViewModel endorsement, boolean isDev); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
In this repository, isDev is used as a feature flag, which is descriptive enough to indicate dev mode
There was a problem hiding this comment.
Fair point. If 'isDev' is the standard feature flag naming in your codebase, keeping it consistent makes sense. I'll resolve this comment.
| return new ResponseEntity<>(endorsementService.update(id, body), HttpStatus.OK); | ||
| @PathVariable Integer id, | ||
| @Valid @RequestBody UpdateEndorsementViewModel body, | ||
| @RequestParam(name = "dev", required = false, defaultValue = "false") boolean isDev) { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
- currently we are using this method only for feature flags or dev mode
There was a problem hiding this comment.
While I understand it's used for feature flags, exposing dev flags via API parameters creates security risks. Consider using a proper feature flag management system or environment-based configuration instead. This provides better security and control over feature rollouts.
| if (isDev) { | ||
| Optional<Endorsement> existingEndorsement = endorsementRepository.findById(endorsementId); | ||
|
|
||
| if (existingEndorsement.isEmpty()) { | ||
| log.info("Endorsement with id: {} not found", endorsementId); | ||
| throw new EndorsementNotFoundException(ExceptionMessages.ENDORSEMENT_NOT_FOUND); | ||
| } | ||
|
|
||
| Endorsement endorsement = existingEndorsement.get(); | ||
|
|
||
| JwtUser jwtDetails = | ||
| (JwtUser) SecurityContextHolder.getContext().getAuthentication().getPrincipal(); | ||
| String userId = jwtDetails.getRdsUserId(); | ||
|
|
||
| if (endorsement.getEndorserId().equals(userId)) { | ||
| RdsGetUserDetailsResDto endorseDetails = | ||
| rdsService.getUserDetails(endorsement.getEndorseId()); | ||
| RdsGetUserDetailsResDto endorserDetails = rdsService.getUserDetails(userId); | ||
|
|
||
| endorsement.setMessage(body.getMessage()); | ||
| Endorsement savedEndorsementDetails = endorsementRepository.save(endorsement); | ||
|
|
||
| return EndorsementViewModel.toViewModel( | ||
| savedEndorsementDetails, | ||
| UserViewModel.toViewModel(endorseDetails.getUser()), | ||
| UserViewModel.toViewModel(endorserDetails.getUser())); | ||
| } else { | ||
| log.warn("User: {} is not authorized to update endorsement: {}", userId, endorsementId); | ||
| throw new ForbiddenException(ExceptionMessages.UNAUTHORIZED_ENDORSEMENT_UPDATE); | ||
| } | ||
| } | ||
|
|
||
| Endorsement endorsement = exitingEndorsement.get(); | ||
| String updatedMessage = body.getMessage(); | ||
|
|
||
| if (updatedMessage != null) { | ||
| endorsement.setMessage(updatedMessage); | ||
| } | ||
|
|
||
| Endorsement savedEndorsementDetails = endorsementRepository.save(endorsement); | ||
| RdsGetUserDetailsResDto endorseDetails = | ||
| rdsService.getUserDetails(savedEndorsementDetails.getEndorseId()); | ||
| RdsGetUserDetailsResDto endorserDetails = | ||
| rdsService.getUserDetails(savedEndorsementDetails.getEndorserId()); | ||
|
|
||
| return EndorsementViewModel.toViewModel( | ||
| savedEndorsementDetails, | ||
| UserViewModel.toViewModel(endorseDetails.getUser()), | ||
| UserViewModel.toViewModel(endorserDetails.getUser())); | ||
| throw new IllegalStateException(ExceptionMessages.UPDATE_DISABLED_IN_NON_DEV_MODE); |
There was a problem hiding this comment.
Deep Nesting in Update Method 
Tell me more
What is the issue?
The update method has deeply nested logic within an if-statement, making the code flow harder to follow. The dev mode check should be inverted to handle the non-dev case first.
Why this matters
Deep nesting reduces code readability and makes it harder to understand the main flow of the method. Early returns improve code clarity by reducing cognitive load.
Suggested change ∙ Feature Preview
public EndorsementViewModel update(Integer endorsementId, UpdateEndorsementViewModel body, boolean isDev) {
if (!isDev) {
throw new IllegalStateException(ExceptionMessages.UPDATE_DISABLED_IN_NON_DEV_MODE);
}
Optional<Endorsement> existingEndorsement = endorsementRepository.findById(endorsementId);
// ... rest of the logic ...
}Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| public static final String ENDORSEMENT_ALREADY_EXISTS = "Endorsement already exists"; | ||
| public static final String ENDORSEMENT_NOT_FOUND = "Endorsement not found"; | ||
| public static final String ENDORSEMENT_MESSAGE_EMPTY = "Endorsement message cannot be empty"; | ||
| public static final String USER_NOT_FOUND = "Error getting user details"; |
There was a problem hiding this comment.
Inconsistent Error Message for User Not Found Scenario 
Tell me more
What is the issue?
The error message 'Error getting user details' does not accurately reflect the USER_NOT_FOUND constant name and scenario.
Why this matters
Using a generic error message instead of a specific one can make debugging harder and may confuse users about the actual issue - that the user was not found in the system.
Suggested change ∙ Feature Preview
Change the message to be consistent with the constant name and specific to the scenario:
public static final String USER_NOT_FOUND = "User not found";Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
Date: 10th June 2025
Developer Name: @Shyam-Vishwakarma
Issue Ticket Number
endorseorendorserdetails #205PRs going to sync
Description
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
demo.-.1-1749547788272.1.mp4
Screenshot 2
demo-2-403-1749548312568.1.mp4
Test Coverage
Screenshot 1
Description by Korbit AI
What change is being made?
Add the ability to update endorsements based on a development mode check, introduce a more comprehensive exception handling mechanism, and refactor error messages into a centralized constants class.
Why are these changes being made?
These changes allow for the controlled update of endorsements, restricted to development mode for enhanced security. Consolidating error messages in a constants file helps maintain consistency across the codebase and simplifies maintenance. Furthermore, the newly added integration tests ensure reliable validation of update operations and exception handling under various scenarios.