fix: fix bugs in the endpoint for updating endorsements #208
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Summary by CodeRabbit
WalkthroughThe update method for endorsements was modified to accept an optional "dev" boolean parameter, enabling stricter authorization checks when set. The validation for the endorsement message was strengthened to require non-blank values. Integration tests were updated to re-enable and exercise these new behaviors using the "dev" parameter. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant EndorsementsApi
participant EndorsementService
participant Database
participant RdsService
Client->>EndorsementsApi: PATCH /endorsements/{id}?dev=true
EndorsementsApi->>EndorsementService: update(id, body, isDev=true)
EndorsementService->>Database: Retrieve endorsement by id
Database-->>EndorsementService: Endorsement details
EndorsementService->>RdsService: Get current user details
RdsService-->>EndorsementService: User details
EndorsementService->>EndorsementService: Check if user is endorser
alt User is endorser
EndorsementService->>Database: Update endorsement
Database-->>EndorsementService: Updated endorsement
EndorsementService-->>EndorsementsApi: EndorsementViewModel
else Not endorser or error
EndorsementService-->>EndorsementsApi: Error/Forbidden
end
EndorsementsApi-->>Client: Response
Assessment against linked issues
Suggested reviewers
Poem
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 (
|
Shyam-Vishwakarma
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (3)
skill-tree/src/main/java/com/RDS/skilltree/services/EndorsementServiceImplementation.java (3)
167-167: 🧹 Nitpick (assertive)Use parameterized logging instead of String.format.
For consistency with the rest of the codebase and better performance, use parameterized logging.
-log.info(String.format("Endorsement with id: %s not found", endorsementId)); +log.info("Endorsement with id: {} not found", endorsementId);
179-188:⚠️ Potential issueFetch user details before database changes to prevent inconsistencies.
According to the PR objectives, user details should be fetched before making database changes to avoid database errors if user detail retrieval fails. The current implementation saves the endorsement first, which could lead to data inconsistencies.
-Endorsement savedEndorsementDetails = endorsementRepository.save(endorsement); RdsGetUserDetailsResDto endorseDetails = - rdsService.getUserDetails(savedEndorsementDetails.getEndorseId()); + rdsService.getUserDetails(endorsement.getEndorseId()); RdsGetUserDetailsResDto endorserDetails = - rdsService.getUserDetails(savedEndorsementDetails.getEndorserId()); + rdsService.getUserDetails(endorsement.getEndorserId()); + +Endorsement savedEndorsementDetails = endorsementRepository.save(endorsement); return EndorsementViewModel.toViewModel( savedEndorsementDetails, UserViewModel.toViewModel(endorseDetails.getUser()), UserViewModel.toViewModel(endorserDetails.getUser()));
130-188: 🧹 Nitpick (assertive)🛠️ Refactor suggestion
Consider unifying the authorization logic across both paths.
The current implementation creates two distinct behaviors based on the
isDevflag:
- The dev path includes ownership checks and early user fetching
- The non-dev path lacks these security features
This dual behavior raises several concerns:
- Security: The non-dev path allows any authenticated user to update any endorsement, which could be a security vulnerability
- Maintainability: Having two different code paths increases complexity and the chance of bugs
- Code duplication: Both paths share similar logic that could be refactored
Consider applying the ownership check and early user fetching to both paths, removing the need for the
isDevparameter. If a gradual rollout is needed, use a proper feature flag system that can be toggled without code changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
skill-tree/src/main/java/com/RDS/skilltree/apis/EndorsementsApi.java(1 hunks)skill-tree/src/main/java/com/RDS/skilltree/services/EndorsementService.java(1 hunks)skill-tree/src/main/java/com/RDS/skilltree/services/EndorsementServiceImplementation.java(2 hunks)skill-tree/src/main/java/com/RDS/skilltree/viewmodels/UpdateEndorsementViewModel.java(1 hunks)skill-tree/src/test/java/com/RDS/skilltree/integration/skills/UpdateEndorsementsIntegrationTest.java(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
skill-tree/src/main/java/com/RDS/skilltree/services/EndorsementServiceImplementation.java (1)
skill-tree/src/main/java/com/RDS/skilltree/utils/Constants.java (1)
ExceptionMessages(6-18)
skill-tree/src/main/java/com/RDS/skilltree/viewmodels/UpdateEndorsementViewModel.java (1)
skill-tree/src/main/java/com/RDS/skilltree/utils/Constants.java (1)
ExceptionMessages(6-18)
🔇 Additional comments (7)
skill-tree/src/main/java/com/RDS/skilltree/viewmodels/UpdateEndorsementViewModel.java (2)
4-4: Good improvement in validation annotation.The change from
@NotNullto@NotBlankimport is correct and necessary for the enhanced validation.
11-11: Excellent validation enhancement.Changing from
@NotNullto@NotBlankis a significant improvement that prevents empty strings and whitespace-only messages, not just null values. This directly addresses the PR objective of preventing endorsements from being updated with empty messages.skill-tree/src/test/java/com/RDS/skilltree/integration/skills/UpdateEndorsementsIntegrationTest.java (4)
62-62: Good addition of test constant.The
isDevconstant is well-named and will be used to test the new authorization logic when the dev flag is enabled.
208-209: Correct test modification for authorization scenario.The addition of
isDevparameter to this test case is appropriate since this test verifies that users cannot update endorsements they didn't create. This enables the stricter authorization check that should prevent the update.
258-259: Appropriate use of dev flag in error scenario test.Adding the
isDevparameter to this test is correct as it tests the scenario where RDS service fails to get endorser details. With the dev flag enabled, the authorization check will be attempted, making this test relevant for the new functionality.
287-288: Correct application of dev flag for endorse details failure test.The
isDevparameter is appropriately added to test the scenario where getting endorse details fails. This ensures the new authorization logic is exercised even when user detail retrieval fails.skill-tree/src/main/java/com/RDS/skilltree/services/EndorsementServiceImplementation.java (1)
130-162: Good implementation of authorization checks and early user fetching.The new conditional logic when
isDev=truecorrectly implements:
- Ownership verification before allowing updates
- Early user detail fetching to prevent database changes if user retrieval fails
- Proper exception handling with descriptive messages
This aligns well with the PR objectives.
skill-tree/src/main/java/com/RDS/skilltree/services/EndorsementService.java
Show resolved
Hide resolved
skill-tree/src/main/java/com/RDS/skilltree/services/EndorsementServiceImplementation.java
Outdated
Show resolved
Hide resolved
skill-tree/src/main/java/com/RDS/skilltree/services/EndorsementServiceImplementation.java
Show resolved
Hide resolved
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
...ee/src/test/java/com/RDS/skilltree/integration/skills/UpdateEndorsementsIntegrationTest.java
Outdated
Show resolved
Hide resolved
skill-tree/src/main/java/com/RDS/skilltree/services/EndorsementServiceImplementation.java
Show resolved
Hide resolved
skill-tree/src/main/java/com/RDS/skilltree/services/EndorsementServiceImplementation.java
Outdated
Show resolved
Hide resolved
iamitprakash
left a comment
iamitprakash
left a comment
There was a problem hiding this comment.
please check below solution and make changes accordingly
@iamitprakash sir, I've made the changes: UserDetails are needed to create |
3 participants
Date: 3rd June 2025
Developer Name: @Shyam-Vishwakarma
Issue Ticket Number
endorseorendorserdetails #205Description
Fixed the bugs in endpoint for updating endorsements:
Enabled the test cases that were disabled because they were failing due to bugs that are fixed in this pr.
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
Build success:cmd:
mvn verify --file skill-tree/pom.xml -P git-build-profile -Dskip-tests=trueTest Coverage
Screenshot 1
All tests passing:
Description by Korbit AI
What change is being made?
Add development mode parameter to endpoint for updating endorsements, implement checks for authorization and invalid operation handling, and enhance tests to verify non-dev mode restriction.
Why are these changes being made?
The changes address bugs by ensuring updates to endorsements only occur in development mode through a new
devquery parameter. This addition helps differentiate between environments and provides error handling for attempts to perform updates outside of development settings, improving security and intention clarity. Additionally, tests previously disabled due to bugs have been enabled and extended to ensure proper functionality under the new constraint.