feat: 쿠폰 도메인에 메타데이터 필드 추가 #847
Conversation
Walkthrough이 풀 리퀘스트는 쿠폰 도메인의 기능을 확장하고 메타데이터를 추가합니다. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (2)
src/test/java/com/gdschongik/gdsc/domain/coupon/domain/CouponTest.java (1)
Line range hint
22-38: 새로 추가된 필드들에 대한 테스트 케이스가 부족합니다.다음과 같은 테스트 케이스들을 추가하는 것이 좋겠습니다:
- STUDY_COMPLETION 타입일 때 study가 null인 경우
- ADMIN 타입일 때 study가 존재하는 경우
- 각 IssuanceMethodType에 대한 케이스
예시 테스트 코드:
@Test void STUDY_COMPLETION_타입일때_스터디가_NULL이면_실패한다() { // when & then assertThatThrownBy(() -> Coupon.create(COUPON_NAME, Money.from(ONE), STUDY_COMPLETION, AUTOMATIC, null)) .isInstanceOf(CustomException.class) .hasMessageContaining(STUDY_REQUIRED_FOR_STUDY_COMPLETION.getMessage()); } @Test void ADMIN_타입일때_스터디가_존재하면_실패한다() { // given Study study = Study.create("테스트 스터디"); // when & then assertThatThrownBy(() -> Coupon.create(COUPON_NAME, Money.from(ONE), ADMIN, AUTOMATIC, study)) .isInstanceOf(CustomException.class) .hasMessageContaining(STUDY_NOT_ALLOWED_FOR_ADMIN.getMessage()); }src/test/java/com/gdschongik/gdsc/domain/coupon/application/CouponServiceTest.java (1)
Line range hint
31-195: 쿠폰 메타데이터 필드에 대한 테스트 케이스 보강 필요현재 테스트는
ADMIN타입의 쿠폰만 테스트하고 있습니다. 다음과 같은 테스트 케이스를 추가해야 합니다:
STUDY_COMPLETION타입 쿠폰 생성 테스트- 스터디 연관 관계가 있는 쿠폰 생성 테스트
- 잘못된 스터디 ID로 쿠폰 생성 시도 시 실패 테스트
🧹 Nitpick comments (8)
src/test/java/com/gdschongik/gdsc/domain/order/domain/OrderValidatorTest.java (1)
Line range hint
4-447: 쿠폰 타입에 대한 테스트 케이스 추가 필요현재 테스트는 쿠폰의 기본적인 유효성 검사만 다루고 있습니다. 새로 추가된
CouponType과 관련된 다음과 같은 테스트 케이스들을 추가하는 것이 좋겠습니다:
ADMIN타입 쿠폰 사용 시나리오STUDY_COMPLETION타입 쿠폰 사용 시나리오- 잘못된 쿠폰 타입 사용 시나리오
예시 테스트 케이스:
@Test void STUDY_COMPLETION_쿠폰은_스터디가_null이_아닐때만_사용_가능하다() { // given Member currentMember = createAssociateMember(1L); RecruitmentRound recruitmentRound = createRecruitmentRound( LocalDateTime.now().minusDays(1), LocalDateTime.now().plusDays(1), 2024, SemesterType.FIRST, MONEY_20000_WON); Membership membership = createMembership(currentMember, recruitmentRound); IssuedCoupon issuedCoupon = fixtureHelper.createAndIssue( MONEY_5000_WON, currentMember, CouponType.STUDY_COMPLETION, null); // study가 null이면 안됨 MoneyInfo moneyInfo = MoneyInfo.of(MONEY_20000_WON, MONEY_5000_WON, MONEY_15000_WON); // when & then assertThatThrownBy(() -> orderValidator.validatePendingOrderCreate( membership, issuedCoupon, moneyInfo, currentMember)) .isInstanceOf(CustomException.class) .hasMessage(COUPON_STUDY_REQUIRED.getMessage()); }src/main/java/com/gdschongik/gdsc/domain/coupon/domain/IssuanceMethodType.java (2)
6-8: 클래스 수준 문서화 추가를 제안드립니다.각 발급 방식의 의미와 사용 사례를 설명하는 Javadoc을 추가하면 더 명확할 것 같습니다.
다음과 같이 문서화를 추가해보세요:
+/** + * 쿠폰 발급 방식을 정의하는 열거형 + * <p> + * AUTOMATIC: 시스템에 의해 자동으로 발급되는 쿠폰 (예: 스터디 완료 시) + * MANUAL: 관리자에 의해 수동으로 발급되는 쿠폰 + */ @Getter @AllArgsConstructor public enum IssuanceMethodType {
9-13: API 지원을 위한 유틸리티 메서드 추가를 제안드립니다.PR 목표에 명시된 대로 프론트엔드에서 사용 가능한 발급 방식을 조회하기 위한 API가 필요합니다. 이를 지원하기 위한 유틸리티 메서드를 추가하면 좋을 것 같습니다.
다음과 같은 메서드 추가를 고려해보세요:
private final String value; + + /** + * 모든 발급 방식과 해당 설명을 Map으로 반환합니다. + * + * @return 발급 방식 enum과 설명을 담은 Map + */ + public static Map<String, String> getAvailableTypes() { + return Arrays.stream(values()) + .collect(Collectors.toMap( + IssuanceMethodType::name, + IssuanceMethodType::getValue + )); + } }src/main/java/com/gdschongik/gdsc/domain/coupon/domain/CouponType.java (1)
8-13: 각 enum 값에 대한 문서화가 필요합니다.각 쿠폰 타입의 용도와 사용 사례를 명확히 하기 위해 Javadoc 주석을 추가하는 것이 좋겠습니다.
public enum CouponType { + /** 관리자가 직접 발급하는 쿠폰 */ ADMIN("어드민"), + /** 스터디 완료 시 자동으로 발급되는 쿠폰 */ STUDY_COMPLETION("스터디 수료"); private final String value; }src/main/java/com/gdschongik/gdsc/domain/coupon/domain/Coupon.java (1)
46-48: 성능 최적화를 위한 인덱스 추가가 필요합니다.
study_id컬럼에 대한 인덱스를 추가하여 쿠폰 조회 성능을 개선하는 것이 좋겠습니다.@ManyToOne(fetch = FetchType.LAZY) -@JoinColumn(name = "study_id") +@JoinColumn(name = "study_id", index = @Index(name = "idx_coupon_study_id")) private Study study;src/test/java/com/gdschongik/gdsc/domain/coupon/domain/IssuedCouponTest.java (1)
26-26: 메타데이터 필드에 대한 테스트 케이스 추가 필요새로 추가된
CouponType과IssuanceMethodType필드에 대한 테스트 케이스가 부족합니다. 다음과 같은 테스트 케이스를 추가하는 것이 좋습니다:
- 스터디 완료 쿠폰 타입(
STUDY_COMPLETION)에 대한 테스트- 스터디 연관 관계가 있는 경우와 없는 경우에 대한 테스트
Also applies to: 41-41, 56-56, 75-75, 89-89, 103-103
src/test/java/com/gdschongik/gdsc/helper/FixtureHelper.java (1)
87-89: 테스트 헬퍼 메서드의 유연성 개선 필요
createAndIssue메서드가IssuanceMethodType.AUTOMATIC을 하드코딩하고 있습니다. 다양한 발급 방식을 테스트할 수 있도록 메서드를 개선하는 것이 좋습니다.다음과 같이 수정을 제안합니다:
- public IssuedCoupon createAndIssue(Money money, Member member, CouponType couponType, Study study) { - Coupon coupon = Coupon.create(COUPON_NAME, money, couponType, IssuanceMethodType.AUTOMATIC, study); + public IssuedCoupon createAndIssue(Money money, Member member, CouponType couponType, IssuanceMethodType issuanceMethodType, Study study) { + Coupon coupon = Coupon.create(COUPON_NAME, money, couponType, issuanceMethodType, study); return IssuedCoupon.create(coupon, member); }src/test/java/com/gdschongik/gdsc/helper/IntegrationTest.java (1)
225-229: 테스트 헬퍼 메서드가 적절히 수정되었습니다만, 문서화가 필요합니다.메서드의 파라미터가 늘어났으므로, 각 파라미터의 역할과 제약사항을 설명하는 JavaDoc 주석이 필요합니다.
다음과 같이 JavaDoc을 추가하는 것을 제안합니다:
+ /** + * 쿠폰을 생성하고 발급하는 테스트 헬퍼 메서드 + * @param money 쿠폰 금액 + * @param member 발급 대상 회원 + * @param couponType 쿠폰 타입 (ADMIN 또는 STUDY_COMPLETION) + * @param study 스터디 (couponType이 STUDY_COMPLETION인 경우에만 필요) + * @return 발급된 쿠폰 + */ protected IssuedCoupon createAndIssue(Money money, Member member, CouponType couponType, Study study) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/com/gdschongik/gdsc/domain/coupon/application/CouponService.java(4 hunks)src/main/java/com/gdschongik/gdsc/domain/coupon/domain/Coupon.java(2 hunks)src/main/java/com/gdschongik/gdsc/domain/coupon/domain/CouponType.java(1 hunks)src/main/java/com/gdschongik/gdsc/domain/coupon/domain/IssuanceMethodType.java(1 hunks)src/main/java/com/gdschongik/gdsc/domain/coupon/dto/request/CouponCreateRequest.java(1 hunks)src/test/java/com/gdschongik/gdsc/domain/coupon/application/CouponServiceTest.java(10 hunks)src/test/java/com/gdschongik/gdsc/domain/coupon/domain/CouponTest.java(3 hunks)src/test/java/com/gdschongik/gdsc/domain/coupon/domain/IssuedCouponTest.java(7 hunks)src/test/java/com/gdschongik/gdsc/domain/order/application/OrderServiceTest.java(14 hunks)src/test/java/com/gdschongik/gdsc/domain/order/domain/OrderValidatorTest.java(2 hunks)src/test/java/com/gdschongik/gdsc/helper/FixtureHelper.java(3 hunks)src/test/java/com/gdschongik/gdsc/helper/IntegrationTest.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-test
🔇 Additional comments (3)
src/test/java/com/gdschongik/gdsc/domain/order/domain/OrderValidatorTest.java (1)
8-8: 메서드 시그니처 변경이 올바르게 반영되었습니다.
CouponType임포트와createAndIssue메서드의 파라미터 추가가 적절하게 구현되었습니다.Also applies to: 49-49
src/main/java/com/gdschongik/gdsc/domain/coupon/domain/IssuanceMethodType.java (1)
1-5: 패키지 구조와 임포트가 잘 구성되어 있습니다!도메인 주도 설계(DDD) 원칙에 따라 패키지가 잘 구성되어 있으며, 필요한 Lombok 어노테이션만 임포트하여 깔끔합니다.
src/test/java/com/gdschongik/gdsc/helper/IntegrationTest.java (1)
17-18: 새로운 enum 타입들이 올바르게 임포트되었습니다.쿠폰 도메인의 메타데이터 필드 추가에 필요한
CouponType과IssuanceMethodTypeenum이 적절하게 임포트되었습니다.
| public record CouponCreateRequest( | ||
| @NotBlank String name, | ||
| @Positive BigDecimal discountAmount, | ||
| CouponType couponType, | ||
| @Nullable @Schema(description = "스터디 관련 쿠폰이 아니라면 null을 가집니다.") Long studyId) {} |
There was a problem hiding this comment.
couponType 필드에 대한 유효성 검증이 누락되었습니다.
couponType 필드가 null이 되지 않도록 @NotNull 어노테이션을 추가해야 합니다. 또한 studyId가 필요한 상황에서 누락되는 것을 방지하기 위한 커스텀 밸리데이션도 고려해보세요.
public record CouponCreateRequest(
@NotBlank String name,
@Positive BigDecimal discountAmount,
- CouponType couponType,
+ @NotNull CouponType couponType,
@Nullable @Schema(description = "스터디 관련 쿠폰이 아니라면 null을 가집니다.") Long studyId) {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public record CouponCreateRequest( | |
| @NotBlank String name, | |
| @Positive BigDecimal discountAmount, | |
| CouponType couponType, | |
| @Nullable @Schema(description = "스터디 관련 쿠폰이 아니라면 null을 가집니다.") Long studyId) {} | |
| public record CouponCreateRequest( | |
| @NotBlank String name, | |
| @Positive BigDecimal discountAmount, | |
| @NotNull CouponType couponType, | |
| @Nullable @Schema(description = "스터디 관련 쿠폰이 아니라면 null을 가집니다.") Long studyId) {} |
There was a problem hiding this comment.
🔗 Analysis chain
@Sangwook02 확인했습니다. 변경사항이 잘 반영되었네요! 👍
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if @NotNull annotation was added to couponType field
rg "@NotNull.*CouponType couponType" src/main/java/com/gdschongik/gdsc/domain/coupon/dto/request/CouponCreateRequest.java
Length of output: 199
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| @Enumerated(EnumType.STRING) | ||
| private CouponType couponType; | ||
|
|
||
| @Enumerated(EnumType.STRING) | ||
| private IssuanceMethodType issuanceMethodType; | ||
|
|
||
| @ManyToOne(fetch = FetchType.LAZY) | ||
| @JoinColumn(name = "study_id") | ||
| private Study study; |
There was a problem hiding this comment.
도메인 무결성 검증이 필요합니다.
새로 추가된 필드들에 대한 도메인 규칙 검증이 누락되었습니다. CouponType과 Study 사이의 관계에 대한 검증이 특히 중요합니다.
다음과 같은 검증 메소드를 추가해주세요:
private static void validateStudyRequirement(CouponType couponType, Study study) {
if (couponType == CouponType.STUDY_COMPLETION && study == null) {
throw new CustomException(STUDY_REQUIRED_FOR_STUDY_COMPLETION);
}
if (couponType == CouponType.ADMIN && study != null) {
throw new CustomException(STUDY_NOT_ALLOWED_FOR_ADMIN);
}
}| public static Coupon create( | ||
| String name, | ||
| Money discountAmount, | ||
| CouponType couponType, | ||
| IssuanceMethodType issuanceMethodType, | ||
| Study study) { | ||
| validateDiscountAmountPositive(discountAmount); | ||
| return Coupon.builder().name(name).discountAmount(discountAmount).build(); | ||
| return Coupon.builder() | ||
| .name(name) | ||
| .discountAmount(discountAmount) | ||
| .couponType(couponType) | ||
| .issuanceMethodType(issuanceMethodType) | ||
| .study(study) | ||
| .build(); |
There was a problem hiding this comment.
도메인 검증 로직이 create 메소드에서 누락되었습니다.
create 메소드에서 새로 추가된 필드들에 대한 검증을 수행해야 합니다.
public static Coupon create(
String name,
Money discountAmount,
CouponType couponType,
IssuanceMethodType issuanceMethodType,
Study study) {
validateDiscountAmountPositive(discountAmount);
+ validateStudyRequirement(couponType, study);
return Coupon.builder()
.name(name)
.discountAmount(discountAmount)
.couponType(couponType)
.issuanceMethodType(issuanceMethodType)
.study(study)
.build();
}Committable suggestion skipped: line range outside the PR's diff.
| // TODO: 요청할 때마다 새로운 쿠폰 생성되는 문제 수정: 스터디마다 하나의 쿠폰만 존재하도록 쿠폰 타입 및 참조 식별자 추가 | ||
| Coupon coupon = Coupon.create(couponName, Money.from(5000L)); | ||
| Coupon coupon = Coupon.create(couponName, Money.from(5000L), CouponType.STUDY_COMPLETION, AUTOMATIC, study); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
하드코딩된 할인 금액 값 개선 필요
스터디 완료 쿠폰의 할인 금액이 5000원으로 하드코딩되어 있습니다. 이 값은 설정 파일이나 상수로 관리되어야 합니다.
There was a problem hiding this comment.
저도 이 의견에 동의합니다. 할인액은 상수로 관리하면 좋을 것 같아요
There was a problem hiding this comment.
@kckc0608 감사합니다. 이 피드백을 반영하여 다음 PR에서 할인액을 상수로 관리하도록 하겠습니다.
✏️ Learnings added
Learnt from: kckc0608
PR: GDSC-Hongik/gdsc-server#847
File: src/main/java/com/gdschongik/gdsc/domain/coupon/application/CouponService.java:115-115
Timestamp: 2025-01-22T12:26:59.977Z
Learning: Discount amounts in the coupon domain should be managed as constants rather than hardcoded values, especially for specific coupon types like study completion coupons.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
저 부분은 바로 윗줄 todo 처리할 때 아예 제거될거라서 그때 처리하면 될 것 같아요
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| String couponName = couponNameUtil.generateStudyCompletionCouponName(study); | ||
| // TODO: 요청할 때마다 새로운 쿠폰 생성되는 문제 수정: 스터디마다 하나의 쿠폰만 존재하도록 쿠폰 타입 및 참조 식별자 추가 | ||
| Coupon coupon = Coupon.create(couponName, Money.from(5000L)); | ||
| Coupon coupon = Coupon.create(couponName, Money.from(5000L), CouponType.STUDY_COMPLETION, AUTOMATIC, study); |
There was a problem hiding this comment.
스터디별 중복 쿠폰 생성 문제 해결 필요
TODO 주석에서 언급된 것처럼, 현재 구현은 요청할 때마다 새로운 쿠폰이 생성되는 문제가 있습니다.
다음과 같은 해결 방안을 제안합니다:
- 스터디 ID와 쿠폰 타입을 복합 유니크 키로 설정
- 쿠폰 생성 전에 기존 쿠폰 존재 여부를 확인하는 로직 추가
+ Optional<Coupon> existingCoupon = couponRepository.findByStudyAndCouponType(study, CouponType.STUDY_COMPLETION);
+ Coupon coupon = existingCoupon.orElseGet(() -> {
String couponName = couponNameUtil.generateStudyCompletionCouponName(study);
- Coupon coupon = Coupon.create(couponName, Money.from(5000L), CouponType.STUDY_COMPLETION, AUTOMATIC, study);
- couponRepository.save(coupon);
+ Coupon newCoupon = Coupon.create(couponName, Money.from(5000L), CouponType.STUDY_COMPLETION, AUTOMATIC, study);
+ return couponRepository.save(newCoupon);
+ });Committable suggestion skipped: line range outside the PR's diff.
| @@ -83,7 +84,7 @@ class 임시주문_생성할때 { | |||
|
|
|||
| Membership membership = createMembership(member, recruitmentRound); | |||
|
|
|||
| IssuedCoupon issuedCoupon = createAndIssue(MONEY_5000_WON, member); | |||
| IssuedCoupon issuedCoupon = createAndIssue(MONEY_5000_WON, member, ADMIN, null); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
테스트 케이스들이 새로운 쿠폰 메타데이터를 반영하고 있으나, 테스트 커버리지 개선이 필요합니다.
모든 테스트 케이스에서 ADMIN 타입의 쿠폰만 테스트하고 있습니다. STUDY_COMPLETION 타입에 대한 테스트도 필요합니다.
다음과 같은 테스트 케이스 추가를 제안합니다:
- 스터디 완료 쿠폰으로 주문 생성
- 스터디 없이 STUDY_COMPLETION 타입 쿠폰 생성 시 실패 케이스
- 쿠폰 타입별 할인 정책 검증
예시 테스트 코드:
@Test
void 스터디완료쿠폰으로_주문생성_성공() {
// given
Member member = createMember();
Study study = createStudy(member, START_TO_END_PERIOD, START_TO_END_PERIOD);
IssuedCoupon issuedCoupon = createAndIssue(MONEY_5000_WON, member, STUDY_COMPLETION, study);
// when & then
// ... 주문 생성 및 검증 로직
}Also applies to: 122-122, 163-163, 199-199, 233-233, 288-288, 368-368, 410-410, 455-455, 503-503, 548-548, 583-583, 616-616
There was a problem hiding this comment.
스터디 관련 test는 #842 에서 추가
There was a problem hiding this comment.
@Sangwook02 알겠습니다. 스터디 관련 테스트 케이스들을 #842 이슈에서 다루시는 것으로 하겠습니다.
🔗 Analysis chain
@Sangwook02 알겠습니다. 스터디 관련 테스트 케이스들을 #842 이슈에서 다루시는 것으로 하겠습니다.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if issue #842 exists
gh issue view 842
Length of output: 187
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
이거 이런 스크립트는 굳이 언급할 필요 없을듯 합니다
코드래빗이 나중에 계속해서 반복되는 내용 리뷰할 때 학습 필요한 부분 있으면 그때 알려주면 되는 거라서요
불필요한 내용 learning에 추가하면 이상한 소리하더라고요
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
| @@ -83,7 +84,7 @@ class 임시주문_생성할때 { | |||
|
|
|||
| Membership membership = createMembership(member, recruitmentRound); | |||
|
|
|||
| IssuedCoupon issuedCoupon = createAndIssue(MONEY_5000_WON, member); | |||
| IssuedCoupon issuedCoupon = createAndIssue(MONEY_5000_WON, member, ADMIN, null); | |||
There was a problem hiding this comment.
이거 이런 스크립트는 굳이 언급할 필요 없을듯 합니다
코드래빗이 나중에 계속해서 반복되는 내용 리뷰할 때 학습 필요한 부분 있으면 그때 알려주면 되는 거라서요
불필요한 내용 learning에 추가하면 이상한 소리하더라고요
|
|
||
| @Getter | ||
| @AllArgsConstructor | ||
| public enum IssuanceMethodType { |
There was a problem hiding this comment.
| public enum IssuanceMethodType { | |
| public enum IssuanceType { |
Method 와 Type 이 의미 상 같은 표현으로 보입니다
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/main/java/com/gdschongik/gdsc/domain/coupon/application/CouponService.java(4 hunks)src/main/java/com/gdschongik/gdsc/domain/coupon/domain/Coupon.java(2 hunks)src/main/java/com/gdschongik/gdsc/domain/coupon/domain/IssuanceType.java(1 hunks)src/main/java/com/gdschongik/gdsc/domain/coupon/dto/request/CouponCreateRequest.java(1 hunks)src/test/java/com/gdschongik/gdsc/domain/coupon/domain/CouponTest.java(3 hunks)src/test/java/com/gdschongik/gdsc/domain/coupon/domain/IssuedCouponTest.java(7 hunks)src/test/java/com/gdschongik/gdsc/helper/FixtureHelper.java(3 hunks)src/test/java/com/gdschongik/gdsc/helper/IntegrationTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/java/com/gdschongik/gdsc/helper/IntegrationTest.java
- src/test/java/com/gdschongik/gdsc/domain/coupon/domain/IssuedCouponTest.java
- src/test/java/com/gdschongik/gdsc/helper/FixtureHelper.java
🧰 Additional context used
📓 Learnings (2)
src/test/java/com/gdschongik/gdsc/domain/coupon/domain/CouponTest.java (1)
Learnt from: kckc0608
PR: GDSC-Hongik/gdsc-server#847
File: src/main/java/com/gdschongik/gdsc/domain/coupon/application/CouponService.java:115-115
Timestamp: 2025-01-22T12:27:00.185Z
Learning: Discount amounts in the coupon domain should be managed as constants rather than hardcoded values, especially for specific coupon types like study completion coupons.
src/main/java/com/gdschongik/gdsc/domain/coupon/application/CouponService.java (1)
Learnt from: kckc0608
PR: GDSC-Hongik/gdsc-server#847
File: src/main/java/com/gdschongik/gdsc/domain/coupon/application/CouponService.java:115-115
Timestamp: 2025-01-22T12:27:00.185Z
Learning: Discount amounts in the coupon domain should be managed as constants rather than hardcoded values, especially for specific coupon types like study completion coupons.
🔇 Additional comments (8)
src/main/java/com/gdschongik/gdsc/domain/coupon/domain/IssuanceType.java (1)
8-11: 깔끔한 구현입니다!발급 유형을 명확하게 구분하고, 한글 값을 사용하여 가독성을 높였습니다.
src/main/java/com/gdschongik/gdsc/domain/coupon/dto/request/CouponCreateRequest.java (1)
11-15: 검증 로직이 잘 구현되어 있습니다!필수 필드에 대한 검증 어노테이션과 명확한 스키마 문서화가 잘 되어있습니다.
src/test/java/com/gdschongik/gdsc/domain/coupon/domain/CouponTest.java (1)
Line range hint
28-38: 새로운 필드에 대한 테스트 케이스가 부족합니다.
CouponType과IssuanceType에 대한 다양한 조합의 테스트 케이스를 추가해주세요. 특히:
- STUDY_COMPLETION 타입일 때 study가 null인 경우
- ADMIN 타입일 때 study가 null이 아닌 경우
- 각 IssuanceType에 대한 케이스
src/main/java/com/gdschongik/gdsc/domain/coupon/domain/Coupon.java (2)
40-48: 엔티티 매핑이 잘 구현되어 있습니다.
@Enumerated(EnumType.STRING)와@ManyToOne(fetch = FetchType.LAZY)설정이 적절합니다.
59-68:⚠️ Potential issue도메인 규칙 검증이 누락되었습니다.
create메소드에CouponType과Study사이의 관계를 검증하는 로직이 필요합니다.다음과 같은 검증 메소드를 추가해주세요:
public static Coupon create( String name, Money discountAmount, CouponType couponType, IssuanceType issuanceType, Study study) { validateDiscountAmountPositive(discountAmount); + validateCouponTypeAndStudy(couponType, study); return Coupon.builder() .name(name) .discountAmount(discountAmount) .couponType(couponType) .issuanceType(issuanceType) .study(study) .build(); } +private static void validateCouponTypeAndStudy(CouponType couponType, Study study) { + if (couponType == CouponType.STUDY_COMPLETION && study == null) { + throw new CustomException(ErrorCode.STUDY_REQUIRED_FOR_STUDY_COMPLETION_COUPON); + } + if (couponType == CouponType.ADMIN && study != null) { + throw new CustomException(ErrorCode.STUDY_NOT_ALLOWED_FOR_ADMIN_COUPON); + } +}Likely invalid or redundant comment.
src/main/java/com/gdschongik/gdsc/domain/coupon/application/CouponService.java (3)
3-3: 새로운 의존성이 적절히 추가되었습니다.도메인 확장을 위해 필요한 import 문들이 잘 구성되어 있습니다.
Also applies to: 10-10, 21-22, 27-27
44-44: StudyRepository 의존성이 적절히 주입되었습니다.기존 패턴을 따르고 있으며, @requiredargsconstructor를 통해 올바르게 초기화됩니다.
113-115: 스터디별 중복 쿠폰 생성 및 하드코딩된 할인 금액 문제
- 이전 리뷰에서 지적된 하드코딩된 할인 금액이 아직 수정되지 않았습니다.
- TODO 주석에서 언급된 중복 쿠폰 생성 문제가 있습니다.
기존 리뷰 의견에 동의하며, 다음 사항들을 추가로 제안합니다:
- 할인 금액을 상수나 설정 파일로 관리
- 스터디별로 쿠폰 중복 생성 방지를 위한 유니크 제약 조건 추가
| @@ -18,7 +20,7 @@ class 쿠폰_생성할때 { | |||
| @Test | |||
| void 성공한다() { | |||
| // when | |||
| Coupon coupon = Coupon.create(COUPON_NAME, Money.from(ONE)); | |||
| Coupon coupon = Coupon.create(COUPON_NAME, Money.from(ONE), ADMIN, AUTOMATIC, null); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
상수를 사용한 할인 금액 관리가 필요합니다.
이전 피드백에 따라 할인 금액을 상수로 관리하는 것이 좋습니다. ONE 대신 의미 있는 상수를 정의하여 사용하세요.
-Coupon coupon = Coupon.create(COUPON_NAME, Money.from(ONE), ADMIN, AUTOMATIC, null);
+Coupon coupon = Coupon.create(COUPON_NAME, Money.from(DEFAULT_DISCOUNT_AMOUNT), ADMIN, AUTOMATIC, null);Committable suggestion skipped: line range outside the PR's diff.
| Study study = studyHistories.get(0).getStudy(); | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
스터디 히스토리의 일관성 검증이 필요합니다.
코드베이스 분석 결과, 현재 구현에서는 모든 히스토리가 동일한 스터디에서 왔다고 가정하고 있으며, 이를 검증하는 로직이 없습니다. 이는 잘못된 쿠폰 발급으로 이어질 수 있습니다.
다음과 같은 검증 로직 추가를 제안합니다:
Set<Study> uniqueStudies = studyHistories.stream()
.map(StudyHistory::getStudy)
.collect(Collectors.toSet());
if (uniqueStudies.size() > 1) {
throw new CustomException(MULTIPLE_STUDIES_NOT_ALLOWED);
}
Study study = studyHistories.get(0).getStudy();🔗 Analysis chain
스터디 히스토리의 일관성 검증이 필요합니다.
첫 번째 히스토리에서 스터디 정보를 가져오는 방식은 모든 히스토리가 동일한 스터디에서 왔다고 가정합니다. 이 가정이 깨질 경우 잘못된 쿠폰이 발급될 수 있습니다.
다음과 같은 검증 로직 추가를 제안합니다:
+Set<Study> uniqueStudies = studyHistories.stream()
+ .map(StudyHistory::getStudy)
+ .collect(Collectors.toSet());
+if (uniqueStudies.size() > 1) {
+ throw new CustomException(MULTIPLE_STUDIES_NOT_ALLOWED);
+}
Study study = studyHistories.get(0).getStudy();🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for StudyHistory related patterns
ast-grep --pattern 'List<StudyHistory> $_ = $_'
ast-grep --pattern 'studyHistories.stream().$$$'
# Search for validation patterns
rg "throw\s+new\s+CustomException" -A 2
# Search for Study related validations
rg "Study.*validate" -A 2
Length of output: 35307
Script:
#!/bin/bash
# Check StudyHistoryValidator implementation
ast-grep --pattern 'class StudyHistoryValidator {
$$$
}'
# Search for any study-related consistency checks
rg "studyHistories.*stream\(\).*collect" -A 2
Length of output: 108
Script:
#!/bin/bash
# Search for any stream operations on studyHistories
rg "studyHistories\." -A 3
# Look for study validation patterns
ast-grep --pattern 'validateStudy($$$)'
ast-grep --pattern 'validate($$$) {
$$$
study
$$$
}'
Length of output: 7962
| Optional<Study> study = Optional.ofNullable(request.studyId()).flatMap(studyRepository::findById); | ||
| Coupon coupon = Coupon.create( | ||
| request.name(), Money.from(request.discountAmount()), request.couponType(), MANUAL, study.orElse(null)); |
There was a problem hiding this comment.
💡 Codebase verification
쿠폰 타입과 스터디 관계 검증이 필요합니다
현재 구현에서 ADMIN 타입 쿠폰에 스터디가 연결되거나, STUDY_COMPLETION 타입 쿠폰에 스터디가 없는 경우를 방지하는 검증 로직이 없습니다. 다음과 같은 검증 로직 추가를 제안합니다:
Optional<Study> study = Optional.ofNullable(request.studyId()).flatMap(studyRepository::findById);
+if (request.couponType() == CouponType.ADMIN && study.isPresent()) {
+ throw new CustomException(new ErrorCode("어드민 쿠폰은 스터디와 연결될 수 없습니다."));
+}
+if (request.couponType() == CouponType.STUDY_COMPLETION && study.isEmpty()) {
+ throw new CustomException(new ErrorCode("스터디 수료 쿠폰은 반드시 스터디와 연결되어야 합니다."));
+}
Coupon coupon = Coupon.create(
request.name(), Money.from(request.discountAmount()), request.couponType(), MANUAL, study.orElse(null));🔗 Analysis chain
쿠폰 타입과 스터디 관계에 대한 유효성 검증이 필요합니다.
현재 구현에서는 쿠폰 타입과 스터디 관계의 유효성을 검증하지 않고 있습니다. 예를 들어, ADMIN 타입 쿠폰에 스터디가 연결되거나, STUDY_COMPLETION 타입 쿠폰에 스터디가 없는 경우를 방지해야 합니다.
다음과 같은 검증 로직 추가를 제안합니다:
Optional<Study> study = Optional.ofNullable(request.studyId()).flatMap(studyRepository::findById);
+if (request.couponType() == CouponType.ADMIN && study.isPresent()) {
+ throw new CustomException(INVALID_COUPON_TYPE_STUDY_COMBINATION);
+}
+if (request.couponType() == CouponType.STUDY_COMPLETION && study.isEmpty()) {
+ throw new CustomException(STUDY_REQUIRED_FOR_STUDY_COMPLETION_COUPON);
+}
Coupon coupon = Coupon.create(
request.name(), Money.from(request.discountAmount()), request.couponType(), MANUAL, study.orElse(null));🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Find and examine CouponType enum and Coupon-related files
echo "=== CouponType enum definition ==="
fd -e java -E test "CouponType.java" --exec cat {}
echo -e "\n=== Coupon entity and related implementations ==="
fd -e java -E test "Coupon.java" --exec cat {}
echo -e "\n=== Searching for existing validation patterns ==="
rg -A 5 "CouponType.*Study" --type java
Length of output: 8929
Script:
#!/bin/bash
# Check for error codes related to coupon validation
echo "=== Error Codes ==="
fd -e java "ErrorCode.java" --exec cat {}
echo -e "\n=== Searching for coupon-related exceptions ==="
rg "COUPON.*" --type java
Length of output: 19835
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/com/gdschongik/gdsc/domain/coupon/application/CouponService.java(4 hunks)src/main/java/com/gdschongik/gdsc/domain/coupon/domain/Coupon.java(2 hunks)src/test/java/com/gdschongik/gdsc/domain/coupon/domain/CouponTest.java(3 hunks)src/test/java/com/gdschongik/gdsc/domain/coupon/domain/IssuedCouponTest.java(7 hunks)src/test/java/com/gdschongik/gdsc/helper/FixtureHelper.java(3 hunks)src/test/java/com/gdschongik/gdsc/helper/IntegrationTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/java/com/gdschongik/gdsc/domain/coupon/domain/CouponTest.java
- src/test/java/com/gdschongik/gdsc/domain/coupon/domain/IssuedCouponTest.java
- src/test/java/com/gdschongik/gdsc/helper/FixtureHelper.java
🧰 Additional context used
📓 Learnings (2)
src/test/java/com/gdschongik/gdsc/helper/IntegrationTest.java (1)
Learnt from: kckc0608
PR: GDSC-Hongik/gdsc-server#847
File: src/main/java/com/gdschongik/gdsc/domain/coupon/application/CouponService.java:115-115
Timestamp: 2025-01-22T12:27:00.185Z
Learning: Discount amounts in the coupon domain should be managed as constants rather than hardcoded values, especially for specific coupon types like study completion coupons.
src/main/java/com/gdschongik/gdsc/domain/coupon/application/CouponService.java (1)
Learnt from: kckc0608
PR: GDSC-Hongik/gdsc-server#847
File: src/main/java/com/gdschongik/gdsc/domain/coupon/application/CouponService.java:115-115
Timestamp: 2025-01-22T12:27:00.185Z
Learning: Discount amounts in the coupon domain should be managed as constants rather than hardcoded values, especially for specific coupon types like study completion coupons.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-test
🔇 Additional comments (8)
src/main/java/com/gdschongik/gdsc/domain/coupon/domain/Coupon.java (3)
40-48: 필드 선언이 적절합니다!
couponType과issuanceType에@Enumerated(EnumType.STRING)어노테이션을 사용하여 enum 값이 문자열로 저장되도록 한 것이 좋습니다.study필드에@ManyToOne(fetch = FetchType.LAZY)를 사용하여 N+1 문제를 방지한 것이 좋습니다.
70-78:⚠️ Potential issue도메인 검증 로직이 createManual 메소드에서 누락되었습니다.
createManual메소드에서도 새로 추가된 필드들에 대한 검증을 수행해야 합니다.public static Coupon createManual(String name, Money discountAmount, CouponType couponType, Study study) { validateDiscountAmountPositive(discountAmount); + validateStudyRequirement(couponType, study); return Coupon.builder() .name(name) .discountAmount(discountAmount) .couponType(couponType) .issuanceType(IssuanceType.MANUAL) .study(study) .build(); }Likely invalid or redundant comment.
59-67:⚠️ Potential issue도메인 검증 로직이 createAutomatic 메소드에서 누락되었습니다.
createAutomatic메소드에서 새로 추가된 필드들에 대한 검증을 수행해야 합니다.public static Coupon createAutomatic(String name, Money discountAmount, CouponType couponType, Study study) { validateDiscountAmountPositive(discountAmount); + validateStudyRequirement(couponType, study); return Coupon.builder() .name(name) .discountAmount(discountAmount) .couponType(couponType) .issuanceType(IssuanceType.AUTOMATIC) .study(study) .build(); }Likely invalid or redundant comment.
src/main/java/com/gdschongik/gdsc/domain/coupon/application/CouponService.java (4)
113-113:⚠️ Potential issue스터디별 중복 쿠폰 생성 문제 해결 필요
TODO 주석에서 언급된 것처럼, 현재 구현은 요청할 때마다 새로운 쿠폰이 생성되는 문제가 있습니다.
String couponName = couponNameUtil.generateStudyCompletionCouponName(study); +Optional<Coupon> existingCoupon = couponRepository.findByStudyAndCouponType(study, CouponType.STUDY_COMPLETION); +Coupon coupon = existingCoupon.orElseGet(() -> { -Coupon coupon = Coupon.createAutomatic(couponName, Money.from(5000L), CouponType.STUDY_COMPLETION, study); -couponRepository.save(coupon); + Coupon newCoupon = Coupon.createAutomatic(couponName, Money.from(STUDY_COMPLETION_COUPON_AMOUNT), CouponType.STUDY_COMPLETION, study); + return couponRepository.save(newCoupon); +});Likely invalid or redundant comment.
110-111:⚠️ Potential issue스터디 히스토리의 일관성 검증이 필요합니다.
첫 번째 히스토리에서 스터디 정보를 가져오는 방식은 모든 히스토리가 동일한 스터디에서 왔다고 가정합니다.
+Set<Study> uniqueStudies = studyHistories.stream() + .map(StudyHistory::getStudy) + .collect(Collectors.toSet()); +if (uniqueStudies.size() > 1) { + throw new CustomException(MULTIPLE_STUDIES_NOT_ALLOWED); +} Study study = studyHistories.get(0).getStudy();Likely invalid or redundant comment.
114-114: 🛠️ Refactor suggestion하드코딩된 할인 금액 값 개선 필요
스터디 완료 쿠폰의 할인 금액이 5000원으로 하드코딩되어 있습니다. 이 값은 상수로 관리되어야 합니다.
-Coupon coupon = Coupon.createAutomatic(couponName, Money.from(5000L), CouponType.STUDY_COMPLETION, study); +Coupon coupon = Coupon.createAutomatic(couponName, Money.from(STUDY_COMPLETION_COUPON_AMOUNT), CouponType.STUDY_COMPLETION, study);Likely invalid or redundant comment.
50-54:⚠️ Potential issue쿠폰 타입과 스터디 관계 검증이 필요합니다.
createCoupon메소드에서 쿠폰 타입과 스터디 관계에 대한 검증이 누락되었습니다.Optional<Study> study = Optional.ofNullable(request.studyId()).flatMap(studyRepository::findById); +if (request.couponType() == CouponType.ADMIN && study.isPresent()) { + throw new CustomException(STUDY_NOT_ALLOWED_FOR_ADMIN); +} +if (request.couponType() == CouponType.STUDY_COMPLETION && study.isEmpty()) { + throw new CustomException(STUDY_REQUIRED_FOR_STUDY_COMPLETION); +} Coupon coupon = Coupon.createManual( request.name(), Money.from(request.discountAmount()), request.couponType(), study.orElse(null));Likely invalid or redundant comment.
src/test/java/com/gdschongik/gdsc/helper/IntegrationTest.java (1)
224-228: 테스트 헬퍼 메소드가 적절하게 구현되었습니다!
createAndIssue메소드가 새로운 쿠폰 생성 로직을 올바르게 반영하고 있습니다.
| private Coupon(String name, Money discountAmount, CouponType couponType, IssuanceType issuanceType, Study study) { | ||
| this.name = name; | ||
| this.discountAmount = discountAmount; | ||
| this.couponType = couponType; | ||
| this.issuanceType = issuanceType; | ||
| this.study = study; | ||
| } |
There was a problem hiding this comment.
도메인 무결성 검증이 필요합니다.
생성자에서 couponType과 study 간의 관계에 대한 도메인 규칙 검증이 누락되었습니다.
다음과 같은 검증 메소드를 추가해주세요:
private static void validateStudyRequirement(CouponType couponType, Study study) {
if (couponType == CouponType.STUDY_COMPLETION && study == null) {
throw new CustomException(STUDY_REQUIRED_FOR_STUDY_COMPLETION);
}
if (couponType == CouponType.ADMIN && study != null) {
throw new CustomException(STUDY_NOT_ALLOWED_FOR_ADMIN);
}
}
🌱 관련 이슈
📌 작업 내용 및 특이사항
CouponType과IssuanceMethodType은 enum 처리했고CouponType종류는 기획팀과 논의하여 우선ADMIN,STUDY_COMPLETION만 만들어 뒀습니다.CouponType이 스터디 관련인 경우, 어떤 study에서 발급된건지 저장하기 위한 목적으로 study 필드도 있습니다.ADMIN인 경우에는 이 필드가 null로 채워져서 정규화의 필요성에 대해 코어타임에 논의했지만 쿠폰의 수가 많지 않아 단일테이블을 유지하기로 했습니다.📝 참고사항
📚 기타
Summary by CodeRabbit
새로운 기능
버그 수정
리팩터링