Skip to content
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.

Sign up for GitHub

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: 스터디 수료 쿠폰을 스터디 당 하나만 가지도록 변경 #851

Merged
merged 3 commits into from
Jan 28, 2025
Merged

feat: 스터디 수료 쿠폰을 스터디 당 하나만 가지도록 변경 #851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
95b867a
feat: 스터디 수료 쿠폰을 스터디 당 하나만 가지도록 변경
Sangwook02 Jan 26, 2025
e5f37fb
chore: todo 추가
Sangwook02 Jan 28, 2025
d2db7d6
feat: 쿠폰 unique constraint 추가
Sangwook02 Jan 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/main/java/com/gdschongik/gdsc/domain/common/vo/Money.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
public final class Money {

public static final Money ZERO = Money.from(BigDecimal.ZERO);
public static final Money FIVE_THOUSAND = Money.from(5000L);

private BigDecimal amount;

Expand Down
13 changes: 9 additions & 4 deletions src/main/java/com/gdschongik/gdsc/domain/coupon/application/CouponService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,10 +109,7 @@ public void createAndIssueCouponByStudyHistories(List<Long> studyHistoryIds) {
List<Member> students = memberRepository.findAllById(studentIds);
Study study = studyHistories.get(0).getStudy();

String couponName = couponNameUtil.generateStudyCompletionCouponName(study);
// TODO: 요청할 때마다 새로운 쿠폰 생성되는 문제 수정: 스터디마다 하나의 쿠폰만 존재하도록 쿠폰 타입 및 참조 식별자 추가
Coupon coupon = Coupon.createAutomatic(couponName, Money.from(5000L), CouponType.STUDY_COMPLETION, study);
couponRepository.save(coupon);
Coupon coupon = findOrCreate(CouponType.STUDY_COMPLETION, study);
Comment on lines 111 to +112
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

🛠️ Refactor suggestion

스터디 히스토리 유효성 검사 추가가 필요합니다

현재 구현에서는 다음과 같은 잠재적인 문제가 있습니다:

  1. studyHistories가 비어있는 경우 IndexOutOfBoundsException 발생 가능
  2. 서로 다른 스터디의 히스토리가 포함된 경우 잘못된 쿠폰이 발급될 수 있음

리뷰 코멘트에서 제안된 검증 로직 추가를 권장드립니다:

if (studyHistories.isEmpty()) {
    throw new CustomException(STUDY_HISTORY_NOT_FOUND);
}

boolean allSameStudy = studyHistories.stream()
        .map(StudyHistory::getStudy)
        .distinct()
        .count() == 1;
if (!allSameStudy) {
    throw new CustomException(INVALID_STUDY_HISTORIES);
}

추가로 INVALID_STUDY_HISTORIES 에러 코드도 정의가 필요합니다.

🔗 Analysis chain

studyHistories의 유효성 검사가 필요합니다.

다음 사항들을 확인해주세요:

  1. studyHistories가 비어있는 경우에 대한 처리
  2. 모든 studyHistory가 동일한 study에 속하는지 검증

아래는 검증을 위한 코드 제안입니다:

+        if (studyHistories.isEmpty()) {
+            throw new CustomException(STUDY_HISTORY_NOT_FOUND);
+        }
+
+        boolean allSameStudy = studyHistories.stream()
+                .map(StudyHistory::getStudy)
+                .distinct()
+                .count() == 1;
+        if (!allSameStudy) {
+            throw new CustomException(INVALID_STUDY_HISTORIES);
+        }
+
         Study study = studyHistories.get(0).getStudy();
🏁 Scripts executed

The following scripts were executed for the analysis:

Error: No shell scripts found

Script:

#!/bin/bash
# Get the complete implementation of CouponService
ast-grep --pattern 'class CouponService {
  $$$
}'

# Search for any existing validation logic related to studyHistories
rg "studyHistories" -A 5 -B 5

# Check for custom exception definitions
rg "STUDY_HISTORY_NOT_FOUND|INVALID_STUDY_HISTORIES" -A 2 -B 2

Length of output: 26389


List<IssuedCoupon> issuedCoupons = students.stream()
.map(student -> IssuedCoupon.create(coupon, student))
Expand All @@ -123,4 +120,12 @@ public void createAndIssueCouponByStudyHistories(List<Long> studyHistoryIds) {
"[CouponService] 스터디 수료 쿠폰 발급: issuedCouponIds={}",
issuedCoupons.stream().map(IssuedCoupon::getId).toList());
}

private Coupon findOrCreate(CouponType couponType, Study study) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저번에 findOrCreate 계열 따닥 이슈 있었는데, 이것도 unique constraint 있어야 방지할 수 있지 않을까 싶네요

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 그렇겠네요
쿠폰에 unique constraint 걸어두겠습니다~

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그리고 submitAssignment 에서도 findOrCreate 가 있는데, 여기서는 create 분기에서는 repository.save를 호출하지 않고 그냥 정적 팩토리로 생성된 객체를 리턴하고, 바깥에서 save를 호출하는 식으로 하고 있네요.

해당 로직에서는 find 분기로 가져온 과제제출이력을 수정할 니즈가 있어서, 바깥에서도 save를 항상 호출 가능한 상황이었지만...
여기서는 순수하게 조회 or 생성이기 때문에, 안에서 save를 호출하는 것이 맞을 것 같네요.

그래도 AssignmentHistory의 findOrCreate 내부에서 save를 호출하도록 투두 하나 추가해주면 좋을 것 같아요

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네네 todo 추가하고 issue도 만들어둘게요

return couponRepository.findByCouponTypeAndStudy(couponType, study).orElseGet(() -> {
String couponName = couponNameUtil.generateStudyCompletionCouponName(study);
Coupon coupon = Coupon.createAutomatic(couponName, Money.FIVE_THOUSAND, couponType, study);
return couponRepository.save(coupon);
});
}
}
7 changes: 6 additions & 1 deletion src/main/java/com/gdschongik/gdsc/domain/coupon/dao/CouponRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
package com.gdschongik.gdsc.domain.coupon.dao;

import com.gdschongik.gdsc.domain.coupon.domain.Coupon;
import com.gdschongik.gdsc.domain.coupon.domain.CouponType;
import com.gdschongik.gdsc.domain.study.domain.Study;
import java.util.Optional;
import org.springframework.data.jpa.repository.JpaRepository;

public interface CouponRepository extends JpaRepository<Coupon, Long> {}
public interface CouponRepository extends JpaRepository<Coupon, Long> {
Optional<Coupon> findByCouponTypeAndStudy(CouponType couponType, Study study);
}
3 changes: 3 additions & 0 deletions src/main/java/com/gdschongik/gdsc/domain/coupon/domain/Coupon.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,16 @@
import jakarta.persistence.Id;
import jakarta.persistence.JoinColumn;
import jakarta.persistence.ManyToOne;
import jakarta.persistence.Table;
import jakarta.persistence.UniqueConstraint;
import lombok.AccessLevel;
import lombok.Builder;
import lombok.Getter;
import lombok.NoArgsConstructor;

@Getter
@Entity
@Table(uniqueConstraints = {@UniqueConstraint(columnNames = {"coupon_type", "study_id"})})
@NoArgsConstructor(access = AccessLevel.PROTECTED)
public class Coupon extends BaseEntity {

Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/gdschongik/gdsc/domain/study/application/StudentStudyHistoryService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ public List<StudentMyCompleteStudyResponse> getMyCompletedStudies() {
}

private AssignmentHistory findOrCreate(Member currentMember, StudyDetail studyDetail) {
// todo: create로 분기할 경우 내부에서 save 호출하도록 수정
return assignmentHistoryRepository
.findByMemberAndStudyDetail(currentMember, studyDetail)
.orElseGet(() -> AssignmentHistory.create(studyDetail, currentMember));
Expand Down
Loading