-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: 쿠폰 도메인에 메타데이터 필드 추가 #847
Changes from 5 commits
6c0f086
8ceeb8d
5fad801
b0069cb
44f9086
144d2cd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,13 @@ | ||
package com.gdschongik.gdsc.domain.coupon.application; | ||
|
||
import static com.gdschongik.gdsc.domain.coupon.domain.IssuanceType.*; | ||
import static com.gdschongik.gdsc.global.exception.ErrorCode.*; | ||
|
||
import com.gdschongik.gdsc.domain.common.vo.Money; | ||
import com.gdschongik.gdsc.domain.coupon.dao.CouponRepository; | ||
import com.gdschongik.gdsc.domain.coupon.dao.IssuedCouponRepository; | ||
import com.gdschongik.gdsc.domain.coupon.domain.Coupon; | ||
import com.gdschongik.gdsc.domain.coupon.domain.CouponType; | ||
import com.gdschongik.gdsc.domain.coupon.domain.IssuedCoupon; | ||
import com.gdschongik.gdsc.domain.coupon.dto.request.CouponCreateRequest; | ||
import com.gdschongik.gdsc.domain.coupon.dto.request.CouponIssueRequest; | ||
|
@@ -16,10 +18,13 @@ | |
import com.gdschongik.gdsc.domain.member.dao.MemberRepository; | ||
import com.gdschongik.gdsc.domain.member.domain.Member; | ||
import com.gdschongik.gdsc.domain.study.dao.StudyHistoryRepository; | ||
import com.gdschongik.gdsc.domain.study.dao.StudyRepository; | ||
import com.gdschongik.gdsc.domain.study.domain.Study; | ||
import com.gdschongik.gdsc.domain.study.domain.StudyHistory; | ||
import com.gdschongik.gdsc.global.exception.CustomException; | ||
import com.gdschongik.gdsc.global.util.MemberUtil; | ||
import java.util.List; | ||
import java.util.Optional; | ||
import lombok.RequiredArgsConstructor; | ||
import lombok.extern.slf4j.Slf4j; | ||
import org.springframework.data.domain.Page; | ||
|
@@ -36,13 +41,16 @@ public class CouponService { | |
private final CouponNameUtil couponNameUtil; | ||
private final MemberUtil memberUtil; | ||
private final StudyHistoryRepository studyHistoryRepository; | ||
private final StudyRepository studyRepository; | ||
private final CouponRepository couponRepository; | ||
private final IssuedCouponRepository issuedCouponRepository; | ||
private final MemberRepository memberRepository; | ||
|
||
@Transactional | ||
public void createCoupon(CouponCreateRequest request) { | ||
Coupon coupon = Coupon.create(request.name(), Money.from(request.discountAmount())); | ||
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)); | ||
couponRepository.save(coupon); | ||
log.info("[CouponService] 쿠폰 생성: name={}, discountAmount={}", request.name(), request.discountAmount()); | ||
} | ||
|
@@ -100,11 +108,11 @@ public void createAndIssueCouponByStudyHistories(List<Long> studyHistoryIds) { | |
.map(studyHistory -> studyHistory.getStudent().getId()) | ||
.toList(); | ||
List<Member> students = memberRepository.findAllById(studentIds); | ||
Study study = studyHistories.get(0).getStudy(); | ||
|
||
Comment on lines
+110
to
111
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 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 executedThe 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 |
||
String couponName = couponNameUtil.generateStudyCompletionCouponName( | ||
studyHistories.get(0).getStudy()); | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion 하드코딩된 할인 금액 값 개선 필요 스터디 완료 쿠폰의 할인 금액이 5000원으로 하드코딩되어 있습니다. 이 값은 설정 파일이나 상수로 관리되어야 합니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 저도 이 의견에 동의합니다. 할인액은 상수로 관리하면 좋을 것 같아요 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kckc0608 감사합니다. 이 피드백을 반영하여 다음 PR에서 할인액을 상수로 관리하도록 하겠습니다. ✏️ Learnings added
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 저 부분은 바로 윗줄 todo 처리할 때 아예 제거될거라서 그때 처리하면 될 것 같아요 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 스터디별 중복 쿠폰 생성 문제 해결 필요 TODO 주석에서 언급된 것처럼, 현재 구현은 요청할 때마다 새로운 쿠폰이 생성되는 문제가 있습니다. 다음과 같은 해결 방안을 제안합니다:
+ 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);
+ });
|
||
couponRepository.save(coupon); | ||
|
||
List<IssuedCoupon> issuedCoupons = students.stream() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
package com.gdschongik.gdsc.domain.coupon.domain; | ||
|
||
import lombok.AllArgsConstructor; | ||
import lombok.Getter; | ||
|
||
@Getter | ||
@AllArgsConstructor | ||
public enum CouponType { | ||
ADMIN("어드민"), | ||
STUDY_COMPLETION("스터디 수료"); | ||
|
||
private final String value; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
package com.gdschongik.gdsc.domain.coupon.domain; | ||
|
||
import lombok.AllArgsConstructor; | ||
import lombok.Getter; | ||
|
||
@Getter | ||
@AllArgsConstructor | ||
public enum IssuanceType { | ||
AUTOMATIC("자동 발급"), | ||
MANUAL("수동 발급"); | ||
|
||
private final String value; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,15 @@ | ||
package com.gdschongik.gdsc.domain.coupon.dto.request; | ||
|
||
import com.gdschongik.gdsc.domain.coupon.domain.CouponType; | ||
import io.swagger.v3.oas.annotations.media.Schema; | ||
import jakarta.annotation.Nullable; | ||
import jakarta.validation.constraints.NotBlank; | ||
import jakarta.validation.constraints.NotNull; | ||
import jakarta.validation.constraints.Positive; | ||
import java.math.BigDecimal; | ||
|
||
public record CouponCreateRequest(@NotBlank String name, @Positive BigDecimal discountAmount) {} | ||
public record CouponCreateRequest( | ||
@NotBlank String name, | ||
@Positive BigDecimal discountAmount, | ||
@NotNull(message = "쿠폰 타입은 null이 될 수 없습니다.") CouponType couponType, | ||
@Nullable @Schema(description = "스터디 관련 쿠폰이 아니라면 null을 가집니다.") Long studyId) {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
package com.gdschongik.gdsc.domain.coupon.domain; | ||
|
||
import static com.gdschongik.gdsc.domain.coupon.domain.CouponType.*; | ||
import static com.gdschongik.gdsc.domain.coupon.domain.IssuanceType.*; | ||
import static com.gdschongik.gdsc.global.common.constant.CouponConstant.*; | ||
import static com.gdschongik.gdsc.global.exception.ErrorCode.*; | ||
import static java.math.BigDecimal.*; | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion 상수를 사용한 할인 금액 관리가 필요합니다. 이전 피드백에 따라 할인 금액을 상수로 관리하는 것이 좋습니다. -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);
|
||
|
||
// then | ||
assertThat(coupon).isNotNull(); | ||
|
@@ -30,7 +32,7 @@ class 쿠폰_생성할때 { | |
Money discountAmount = Money.from(ZERO); | ||
|
||
// when & then | ||
assertThatThrownBy(() -> Coupon.create(COUPON_NAME, discountAmount)) | ||
assertThatThrownBy(() -> Coupon.create(COUPON_NAME, discountAmount, ADMIN, AUTOMATIC, null)) | ||
.isInstanceOf(CustomException.class) | ||
.hasMessageContaining(COUPON_DISCOUNT_AMOUNT_NOT_POSITIVE.getMessage()); | ||
} | ||
|
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.
💡 Codebase verification
쿠폰 타입과 스터디 관계 검증이 필요합니다
현재 구현에서
ADMIN
타입 쿠폰에 스터디가 연결되거나,STUDY_COMPLETION
타입 쿠폰에 스터디가 없는 경우를 방지하는 검증 로직이 없습니다. 다음과 같은 검증 로직 추가를 제안합니다:🔗 Analysis chain
쿠폰 타입과 스터디 관계에 대한 유효성 검증이 필요합니다.
현재 구현에서는 쿠폰 타입과 스터디 관계의 유효성을 검증하지 않고 있습니다. 예를 들어,
ADMIN
타입 쿠폰에 스터디가 연결되거나,STUDY_COMPLETION
타입 쿠폰에 스터디가 없는 경우를 방지해야 합니다.다음과 같은 검증 로직 추가를 제안합니다:
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
Length of output: 8929
Script:
Length of output: 19835