Feature/43 payments#79
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive payment and subscription system integrated with Toss Payments, including entities for billing keys, payment history, subscriptions, and user plans. Key feedback identifies critical bugs in the payment amount calculation where the plan price was omitted, and logic errors that allow subscriptions to be created or renewed even when payments fail. Additionally, improvements were suggested to prevent potential infinite loops in the billing scheduler, fix typos in entity methods, and modernize the tech stack by replacing legacy JSON libraries and low-level HTTP clients with Spring's RestClient.
| long amount = exchangeRateService.getKrwRate() | ||
| .setScale(0, RoundingMode.HALF_UP) | ||
| .longValue(); |
There was a problem hiding this comment.
결제 금액(amount) 계산 로직에 오류가 있습니다. 현재 코드는 1달러당 환율(krwRate)만을 금액으로 사용하고 있습니다. 플랜의 가격(plan.getPrice())을 곱해주어야 정확한 결제 금액이 산출됩니다. 또한, 서비스 간 결합도를 낮추기 위해 환율 조회와 같은 서비스 호출은 컨트롤러 레이어에서 오케스트레이션하는 것을 권장합니다.
| long amount = exchangeRateService.getKrwRate() | |
| .setScale(0, RoundingMode.HALF_UP) | |
| .longValue(); | |
| long amount = exchangeRateService.getKrwRate() | |
| .multiply(BigDecimal.valueOf(plan.getPrice())) | |
| .setScale(0, RoundingMode.HALF_UP) | |
| .longValue(); |
References
- To reduce coupling between services, orchestrate calls to different services from the controller layer instead of having services call each other directly.
| long amount = exchangeRateService.getKrwRate() | ||
| .setScale(0, RoundingMode.HALF_UP) | ||
| .longValue(); |
There was a problem hiding this comment.
정기 결제 처리 시에도 결제 금액 계산 로직에서 플랜 가격이 누락되었습니다. 환율에 플랜 가격을 곱하도록 수정이 필요합니다. 추가로, 서비스 간 직접적인 호출보다는 컨트롤러에서 각 서비스를 호출하여 결합도를 낮추는 방향을 고려해 주세요.
| long amount = exchangeRateService.getKrwRate() | |
| .setScale(0, RoundingMode.HALF_UP) | |
| .longValue(); | |
| long amount = exchangeRateService.getKrwRate() | |
| .multiply(BigDecimal.valueOf(plan.getPrice())) | |
| .setScale(0, RoundingMode.HALF_UP) | |
| .longValue(); |
References
- To reduce coupling between services, orchestrate calls to different services from the controller layer instead of having services call each other directly.
| Subscription subscription = Subscription.createSubscription(user, plan, billingKey); | ||
| subscriptionRepository.save(subscription); | ||
|
|
||
| userPlan.montlyInitialize(); |
There was a problem hiding this comment.
결제가 실패한 경우에도 구독 정보가 생성되고 사용자의 혜택 정보가 초기화됩니다. 결제가 성공한 경우(PaymentStatus.SUCCESS)에만 구독을 생성하고 초기화 로직이 실행되도록 수정해야 합니다.
if (paymentHistory.getStatus() == PaymentStatus.SUCCESS) {
Subscription subscription = Subscription.createSubscription(user, plan, billingKey);
subscriptionRepository.save(subscription);
userPlan.monthlyInitialize();
}| if (paymentHistory.getStatus() == PaymentStatus.SUCCESS) { | ||
| subscription.renew(); | ||
| } | ||
|
|
||
| userPlan.montlyInitialize(); |
There was a problem hiding this comment.
정기 결제 시에도 결제가 실패한 경우 사용자의 혜택 정보를 초기화하지 않아야 합니다. 초기화 로직을 성공 체크 블록 내부로 이동시켜 주세요.
| if (paymentHistory.getStatus() == PaymentStatus.SUCCESS) { | |
| subscription.renew(); | |
| } | |
| userPlan.montlyInitialize(); | |
| if (paymentHistory.getStatus() == PaymentStatus.SUCCESS) { | |
| subscription.renew(); | |
| userPlan.monthlyInitialize(); | |
| } |
| // prometheus | ||
| implementation 'io.micrometer:micrometer-registry-prometheus' | ||
|
|
||
| implementation 'com.googlecode.json-simple:json-simple:1.1.1' |
There was a problem hiding this comment.
json-simple은 다소 오래된 라이브러리이며, Spring Boot 프로젝트에서는 이미 Jackson(ObjectMapper)이 기본적으로 포함되어 있습니다. 별도의 의존성을 추가하기보다는 프로젝트에 이미 포함된 Jackson을 활용하는 것을 권장합니다.
| this.teamSize = plan.getTeamSize(); | ||
| } | ||
|
|
||
| public void montlyInitialize() { |
There was a problem hiding this comment.
메서드 이름에 오타가 있습니다. 'montlyInitialize'를 'monthlyInitialize'로 수정하여 가독성을 높여주세요.
| public void montlyInitialize() { | |
| public void monthlyInitialize() { |
|
|
||
| @Repository | ||
| public interface BillingKeyRepository extends JpaRepository<BillingKey, Long> { | ||
| Optional<BillingKey> findByUser(User user); |
There was a problem hiding this comment.
사용자가 카드를 변경하는 등의 이유로 여러 개의 빌링키를 가질 수 있습니다. 현재 활성화된 빌링키만 조회하도록 'findByUserAndIsActiveTrue'와 같은 메서드를 사용하는 것이 안전합니다.
| Optional<BillingKey> findByUser(User user); | |
| Optional<BillingKey> findByUserAndIsActiveTrue(User user); |
| public JSONObject sendRequest(JSONObject requestData, String uriPath) throws IOException { | ||
| try { | ||
| String requestUrl = baseUrl + uriPath; | ||
| HttpURLConnection connection = createConnection(tossSecretKey, requestUrl); |
There was a problem hiding this comment.
HttpURLConnection은 현대적인 Spring Boot 애플리케이션에서 사용하기에는 너무 저수준 API입니다. Spring 6.1+에서 제공하는 RestClient를 사용하면 코드의 가독성을 높이고 예외 처리를 더 효율적으로 할 수 있습니다.
1 participant
#️⃣ 연관된 이슈
#️⃣ 작업 내용
#️⃣ 테스트 결과
#️⃣ 변경 사항 체크리스트
#️⃣ 스크린샷 (선택)
#️⃣ 리뷰 요구사항 (선택)
📎 참고 자료 (선택)