-
Notifications
You must be signed in to change notification settings - Fork 24
fix: avoid creating duplicate endorsements, add integration-tests for endorsement creation #198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: avoid creating duplicate endorsements, add integration-tests for endorsement creation #198
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
WalkthroughA custom exception for duplicate endorsements was added, along with a global handler returning HTTP 409. The endorsement service now checks for existing endorsements before creation, throwing the new exception if found. The repository gained a custom existence-check method. Comprehensive integration tests were introduced to verify correct endorsement creation and error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant EndorsementService
participant EndorsementRepository
participant ExceptionHandler
Client->>Controller: POST /endorsements
Controller->>EndorsementService: create(endorsementData)
EndorsementService->>EndorsementRepository: existsByEndorseIdAndEndorserIdAndSkillId(...)
alt Endorsement exists
EndorsementService->>ExceptionHandler: throw EndorsementAlreadyExistsException
ExceptionHandler-->>Client: HTTP 409 Conflict + error message
else Endorsement does not exist
EndorsementService->>EndorsementRepository: save(endorsement)
Controller-->>Client: HTTP 201 Created + endorsement data
end
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
skill-tree/src/main/java/com/RDS/skilltree/exceptions/EndorsementAlreadyExistsException.java(1 hunks)skill-tree/src/main/java/com/RDS/skilltree/exceptions/GlobalExceptionHandler.java(1 hunks)skill-tree/src/main/java/com/RDS/skilltree/repositories/EndorsementRepository.java(1 hunks)skill-tree/src/main/java/com/RDS/skilltree/services/EndorsementServiceImplementation.java(2 hunks)skill-tree/src/test/java/com/RDS/skilltree/integration/skills/CreateEndorsementIntegrationTest.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
skill-tree/src/main/java/com/RDS/skilltree/services/EndorsementServiceImplementation.java (1)
skill-tree/src/main/java/com/RDS/skilltree/exceptions/EndorsementAlreadyExistsException.java (1)
EndorsementAlreadyExistsException(3-7)
🔇 Additional comments (6)
skill-tree/src/main/java/com/RDS/skilltree/exceptions/EndorsementAlreadyExistsException.java (1)
1-7: Well-designed exception class for handling duplicate endorsements.The exception class follows good Java practices by extending RuntimeException and providing a constructor that accepts a message. This will integrate well with the existing exception handling system.
skill-tree/src/main/java/com/RDS/skilltree/exceptions/GlobalExceptionHandler.java (1)
157-163: Appropriate exception handler for EndorsementAlreadyExistsException.The implementation correctly returns HTTP 409 Conflict status, which is the standard response for resource conflicts. The handler follows the same pattern as other exception handlers in this class, maintaining consistency.
skill-tree/src/main/java/com/RDS/skilltree/services/EndorsementServiceImplementation.java (3)
4-4: Import for new exception class.The import for EndorsementAlreadyExistsException has been correctly added.
86-86: Improved logging with parameterized format.Good update to use parameterized logging instead of string concatenation, which is more efficient and follows best practices.
90-98: Effective duplicate endorsement prevention.This validation logic correctly checks for existing endorsements before attempting to create a new one. The error message and logging are clear and informative.
skill-tree/src/main/java/com/RDS/skilltree/repositories/EndorsementRepository.java (1)
6-7: Added necessary imports for Query annotation.The imports for Query and Param annotations have been correctly added to support the new query method.
skill-tree/src/main/java/com/RDS/skilltree/repositories/EndorsementRepository.java
Outdated
Show resolved
Hide resolved
...ree/src/test/java/com/RDS/skilltree/integration/skills/CreateEndorsementIntegrationTest.java
Show resolved
Hide resolved
...ree/src/test/java/com/RDS/skilltree/integration/skills/CreateEndorsementIntegrationTest.java
Show resolved
Hide resolved
...ree/src/test/java/com/RDS/skilltree/integration/skills/CreateEndorsementIntegrationTest.java
Show resolved
Hide resolved
...ree/src/test/java/com/RDS/skilltree/integration/skills/CreateEndorsementIntegrationTest.java
Outdated
Show resolved
Hide resolved
skill-tree/src/main/java/com/RDS/skilltree/repositories/EndorsementRepository.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
Show resolved
Hide resolved
...ree/src/test/java/com/RDS/skilltree/integration/skills/CreateEndorsementIntegrationTest.java
Show resolved
Hide resolved
mbramani
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
...ree/src/test/java/com/RDS/skilltree/integration/skills/CreateEndorsementIntegrationTest.java
Show resolved
Hide resolved
...ree/src/test/java/com/RDS/skilltree/integration/skills/CreateEndorsementIntegrationTest.java
Show resolved
Hide resolved
...ree/src/test/java/com/RDS/skilltree/integration/skills/CreateEndorsementIntegrationTest.java
Show resolved
Hide resolved
skill-tree/src/main/java/com/RDS/skilltree/services/EndorsementServiceImplementation.java
Show resolved
Hide resolved
|
@Shyam-Vishwakarma Please update the title and try and keep concerns separated from next time for bug fix and tests |
|
Could you please attach the integration tests for GET /v1/skills/{id}/endorsements document at the end of the description or additional note section, rather than between the "Documentation Updated" and "Under Feature Flag" sections? |
|
Could you please clarify why this feature is not placed under a feature flag? |
|
I noticed that you attached a Postman screenshot—could you please confirm whether a bearer token was included in the authorization when calling this API endpoint? If authorization was not provided, does that mean the endpoint can be accessed without authentication? |
I removed that doc. That was not related to this route. |
This is a small bug fix. I've added just a check before creating an endorsement. I don't think this should be under ff. |
Yes, Bearer token was included. We can't access any endpoint without a bearer token. |
updated. |
skill-tree/src/main/java/com/RDS/skilltree/repositories/EndorsementRepository.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
...ree/src/test/java/com/RDS/skilltree/integration/skills/CreateEndorsementIntegrationTest.java
Outdated
Show resolved
Hide resolved
...ree/src/test/java/com/RDS/skilltree/integration/skills/CreateEndorsementIntegrationTest.java
Show resolved
Hide resolved
...ree/src/test/java/com/RDS/skilltree/integration/skills/CreateEndorsementIntegrationTest.java
Show resolved
Hide resolved
skill-tree/src/main/java/com/RDS/skilltree/exceptions/GlobalExceptionHandler.java
Outdated
Show resolved
Hide resolved
skill-tree/src/main/java/com/RDS/skilltree/repositories/EndorsementRepository.java
Outdated
Show resolved
Hide resolved
5af0b68 to
789dde0
Compare
Date: 08 May 2025
Developer Name: @Shyam-Vishwakarma
Issue Ticket Number
Description
Currently we can create a duplicate endorsement for a user-skill to resolve this I have done:
added validation to check whether an endorsement already exists or not
If an endorsement already exists then raise an exception (i.e.
EndorsementAlreadyExistsException) otherwise proceed to create endorsementadded integration-tests for the route
POST v1/skills/{id}/endorsementsDocumentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
Build success:Screenshot 2
409 when creating duplicate endorsementTest Coverage
Screenshot 1
- Test coverage for all tests:Screenshot 2
Coverage of tests for POST /v1/skills/{skillId}/endorsements only:Additional Notes
mvn verify --file skill-tree/pom.xml -Dskip-ut=truecommand to run integration tests, andmvn test --file skill-tree/pom.xmlto run unit tests