Conversation
WalkthroughThe changes introduce support for both credit and debit point transactions by adding a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PointFacade
participant PointService
User->>PointFacade: rechargePoints(RechargePointRequest)
PointFacade->>PointService: validateFestivalAndHost()
alt TransactionType is CREDIT
PointFacade->>PointService: rechargePoint(participant, amount)
else TransactionType is DEBIT
PointFacade->>PointService: dischargePoint(participant, amount)
end
Possibly related PRs
Suggested labels
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ 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 (
|
Test Results45 tests 45 ✅ 1s ⏱️ Results for commit 9c089f3. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
src/test/java/org/festimate/team/api/facade/PointFacadeTest.java (1)
98-113: 🛠️ Refactor suggestionAdd test cases for the new discharge functionality.
While you've updated existing tests to use
TransactionType.CREDIT, there are no tests for the new discharge (debit) functionality. Consider adding tests that verify point discharge operations work correctly, including validation of insufficient points.For example, add a test like:
@Test @DisplayName("포인트 차감 성공 (호스트 권한)") void dischargePoints_success() { // given RechargePointRequest request = new RechargePointRequest(TransactionType.DEBIT, 200L, 5); when(userService.getUserByIdOrThrow(1L)).thenReturn(host); when(festivalService.getFestivalByIdOrThrow(100L)).thenReturn(festival); when(festivalService.isHost(host, festival)).thenReturn(true); when(participantService.getParticipantById(200L)).thenReturn(participant); // when pointFacade.rechargePoints(1L, 100L, request); // then verify(pointService).dischargePoint(participant, 5); } @Test @DisplayName("포인트 차감 실패 - 잔액 부족") void dischargePoints_insufficientPoints_throws() { // given RechargePointRequest request = new RechargePointRequest(TransactionType.DEBIT, 200L, 5); when(userService.getUserByIdOrThrow(1L)).thenReturn(host); when(festivalService.getFestivalByIdOrThrow(100L)).thenReturn(festival); when(festivalService.isHost(host, festival)).thenReturn(true); when(participantService.getParticipantById(200L)).thenReturn(participant); when(pointService.dischargePoint(participant, 5)).thenThrow(new FestimateException(ResponseError.INSUFFICIENT_POINTS)); // expect assertThatThrownBy(() -> pointFacade.rechargePoints(1L, 100L, request) ).isInstanceOf(FestimateException.class); }src/main/java/org/festimate/team/api/facade/PointFacade.java (1)
45-61: 🛠️ Refactor suggestionRequest validation for negative point values is missing.
The current implementation doesn't validate that point values are positive. For DEBIT transactions, a negative value would effectively become a credit, and vice versa. Consider adding validation to ensure point values are always positive, improving transaction integrity.
@Transactional public void rechargePoints(Long userId, Long festivalId, RechargePointRequest request) { User user = userService.getUserByIdOrThrow(userId); Festival festival = festivalService.getFestivalByIdOrThrow(festivalId); isHost(user, festival); Participant participant = participantService.getParticipantById(request.participantId()); if (participant.getFestival() != festival) { throw new FestimateException(ResponseError.FORBIDDEN_RESOURCE); } + + if (request.point() <= 0) { + throw new FestimateException(ResponseError.INVALID_INPUT_VALUE, "Point value must be positive"); + } if(request.type().equals(TransactionType.CREDIT)) { pointService.rechargePoint(participant, request.point()); }else if (request.type().equals(TransactionType.DEBIT)) { pointService.dischargePoint(participant, request.point()); } }
🧹 Nitpick comments (3)
src/main/java/org/festimate/team/domain/point/service/impl/PointServiceImpl.java (1)
57-66: Consider refactoring to reduce code duplication.The
dischargePointmethod is correctly implemented and checks for sufficient points before proceeding. However, there's significant code duplication between this method and theusePointmethod. Both methods check for sufficient points and create a debit transaction.Consider refactoring to eliminate duplication:
@Transactional @Override public void usePoint(Participant participant) { - int totalPoint = getTotalPointByParticipant(participant); - if (totalPoint < 1) { - throw new FestimateException(ResponseError.INSUFFICIENT_POINTS); - } - - Point pointUsage = createPointTransaction(participant, 1, TransactionType.DEBIT); - pointRepository.save(pointUsage); + dischargePoint(participant, 1); } @Transactional @Override public void dischargePoint(Participant participant, int amount) { int totalPoint = getTotalPointByParticipant(participant); if (totalPoint < amount) { throw new FestimateException(ResponseError.INSUFFICIENT_POINTS); } Point pointUsage = createPointTransaction(participant, amount, TransactionType.DEBIT); pointRepository.save(pointUsage); }src/main/java/org/festimate/team/api/facade/PointFacade.java (2)
55-60: Consider renaming method to better reflect its dual purpose.The method name
rechargePointsnow handles both crediting and debiting points, which makes it slightly misleading. Consider renaming to something more generic likeprocessPointTransactionto better reflect its expanded functionality.Additionally, there's no default case handling for potential future transaction types. Consider adding an else clause with appropriate error handling.
-public void rechargePoints(Long userId, Long festivalId, RechargePointRequest request) { +public void processPointTransaction(Long userId, Long festivalId, RechargePointRequest request) {if(request.type().equals(TransactionType.CREDIT)) { pointService.rechargePoint(participant, request.point()); }else if (request.type().equals(TransactionType.DEBIT)) { pointService.dischargePoint(participant, request.point()); +}else { + throw new FestimateException(ResponseError.INVALID_INPUT_VALUE, "Unsupported transaction type"); }
36-43: Consistent method naming across facade and service layer.For maintainability, ensure that the service method names are consistently aligned with the facade. If you rename the facade method to
processPointTransaction, consider similar naming conventions in the service layer for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/org/festimate/team/api/facade/PointFacade.java(2 hunks)src/main/java/org/festimate/team/api/point/dto/RechargePointRequest.java(1 hunks)src/main/java/org/festimate/team/domain/point/service/PointService.java(1 hunks)src/main/java/org/festimate/team/domain/point/service/impl/PointServiceImpl.java(2 hunks)src/test/java/org/festimate/team/api/facade/PointFacadeTest.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (6)
src/main/java/org/festimate/team/domain/point/service/PointService.java (1)
17-19: LGTM! New functionality added correctly.The
dischargePointmethod is a logical addition to support debit transactions, complementing the existingrechargePointmethod. The@Transactionalannotation ensures data integrity during point deduction operations.src/test/java/org/festimate/team/api/facade/PointFacadeTest.java (3)
10-10: LGTM! Import added for new functionality.The import for
TransactionTypehas been correctly added to support the updated request structure.
119-119: LGTM! Updated to include transaction type.The
RechargePointRequesthas been correctly updated to includeTransactionType.CREDIT.
137-137: LGTM! Updated to include transaction type.The
RechargePointRequesthas been correctly updated to includeTransactionType.CREDIT.src/main/java/org/festimate/team/api/facade/PointFacade.java (2)
10-10: Good addition of TransactionType import for the new functionality.The addition of this import is appropriate for the new transaction type handling.
45-61:Details
✅ Verification successful
Verify sufficient point balance is checked in the service layer.
While not visible in this file, make sure that the
dischargePointmethod in the service layer validates that the participant has sufficient points before deducting them to prevent negative balances.
🏁 Script executed:
#!/bin/bash # Verify that the service implementation checks for sufficient points rg -A 10 "dischargePoint" --include="*.java" | grep -i "sufficient\|balance\|enough"Length of output: 526
The previous ripgrep invocation used an unsupported flag. Let’s re-run the search correctly to locate and inspect the
dischargePointimplementation:
🏁 Script executed:
#!/bin/bash # Find all dischargePoint implementations/usages in Java files with context rg -n "dischargePoint" -g "*.java" -A5Length of output: 1774
Balance validation already in place
ThePointServiceImpl.dischargePointmethod (src/main/java/org/festimate/team/domain/point/service/impl/PointServiceImpl.java:59–63) retrieves the participant’s total points and throws aFestimateException(ResponseError.INSUFFICIENT_POINTS)ifamountexceeds the balance. No further checks are needed here.
src/main/java/org/festimate/team/api/point/dto/RechargePointRequest.java
Show resolved
Hide resolved
1 participant
📌 PR 제목
[feat] #128 포인트 차감 기능 구현 및 트랜잭션 타입 분기 처리
📌 PR 내용
🛠 작업 내용
🔍 관련 이슈
Closes #128
📸 스크린샷 (Optional)
📚 레퍼런스 (Optional)
트랜잭션 타입 분기 처리는 기존 TransactionType enum 기반으로 처리하였습니다.
Summary by CodeRabbit