feat: 스터디 수료 시 회비 할인 쿠폰 발급 기능 구현 #843
Conversation
|
""" Walkthrough이 풀 리퀘스트는 스터디 수료 시 쿠폰 발급 기능을 구현합니다. Changes
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 4
🧹 Nitpick comments (4)
src/main/java/com/gdschongik/gdsc/domain/study/domain/StudyHistoryCompletionWithdrawnEvent.java (1)
4-5: TODO 항목들을 이슈로 추적하는 것을 추천드립니다.현재 TODO 주석으로 표시된 두 가지 개선사항이 있습니다:
- primitive 타입 사용
- 쿠폰 회수 로직 구현
이러한 개선사항들을 GitHub 이슈로 생성하여 추적하는 것이 좋을 것 같습니다.
이러한 TODO 항목들을 GitHub 이슈로 생성해드릴까요?
src/main/java/com/gdschongik/gdsc/domain/coupon/util/CouponNameUtil.java (1)
12-15: 메서드의 안정성과 가독성 개선이 필요합니다.다음과 같은 개선사항들을 제안드립니다:
- null 체크 추가
- String 연결 최적화
public String generateStudyCompletionCouponName(Study study) { + if (study == null) { + throw new IllegalArgumentException("스터디 정보는 null일 수 없습니다."); + } String academicYearAndSemesterName = SemesterFormatter.format(study); - return academicYearAndSemesterName + " " + study.getTitle() + " 수료 쿠폰"; + return String.format("%s %s 수료 쿠폰", academicYearAndSemesterName, study.getTitle()); }src/main/java/com/gdschongik/gdsc/domain/coupon/application/CouponEventHandler.java (1)
18-22: 이벤트 로깅 개선이 필요합니다.현재 로그는 기본적인 정보만 포함하고 있습니다. 트러블슈팅을 위해 다음과 같은 추가 정보를 로깅하는 것이 좋을 것 같습니다:
- 이벤트 발생 시간
- 이벤트를 발생시킨 사용자 정보
@TransactionalEventListener(phase = TransactionPhase.BEFORE_COMMIT) public void handleStudyHistoryCompletedEvent(StudyHistoriesCompletedEvent event) { - log.info("[CouponEventHandler] 스터디 수료 이벤트 수신: studyHistoryIds={}", event.studyHistoryIds()); + log.info("[CouponEventHandler] 스터디 수료 이벤트 수신: timestamp={}, studyHistoryIds={}, requestedBy={}", + LocalDateTime.now(), + event.studyHistoryIds(), + SecurityContextHolder.getContext().getAuthentication().getName()); couponService.createAndIssueCouponByStudyHistories(event.studyHistoryIds()); }src/main/java/com/gdschongik/gdsc/domain/coupon/application/CouponService.java (1)
106-106: 쿠폰 중복 생성 문제 해결이 필요합니다.TODO 주석에서 언급된 것처럼, 현재 구현은 동일한 스터디에 대해 중복된 쿠폰이 생성될 수 있습니다.
다음과 같은 해결 방안을 제안드립니다:
- Coupon 엔티티에 studyId 필드 추가
- 쿠폰 생성 전 해당 스터디에 대한 쿠폰이 이미 존재하는지 확인
- 존재하는 경우 기존 쿠폰을 재사용
예시 구현:
@Transactional(readOnly = true) public Optional<Coupon> findByStudyId(Long studyId) { return couponRepository.findByStudyId(studyId); }
📜 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/CouponEventHandler.java(1 hunks)src/main/java/com/gdschongik/gdsc/domain/coupon/application/CouponService.java(3 hunks)src/main/java/com/gdschongik/gdsc/domain/coupon/util/CouponNameUtil.java(1 hunks)src/main/java/com/gdschongik/gdsc/domain/study/application/MentorStudyHistoryService.java(5 hunks)src/main/java/com/gdschongik/gdsc/domain/study/domain/StudyHistoriesCompletedEvent.java(1 hunks)src/main/java/com/gdschongik/gdsc/domain/study/domain/StudyHistory.java(1 hunks)src/main/java/com/gdschongik/gdsc/domain/study/domain/StudyHistoryCompletionWithdrawnEvent.java(1 hunks)src/test/java/com/gdschongik/gdsc/domain/coupon/util/CouponNameUtilTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/com/gdschongik/gdsc/domain/study/domain/StudyHistoriesCompletedEvent.java
🔇 Additional comments (3)
src/main/java/com/gdschongik/gdsc/domain/coupon/application/CouponEventHandler.java (1)
14-14: 아키텍처 패턴의 일관성 검토가 필요합니다.TODO 주석에서 언급된 것처럼, 이벤트 핸들링 패턴이 다른 핸들러들과 일관성이 없습니다. 아키텍처의 일관성을 위해 패턴을 통일하는 것이 좋을 것 같습니다.
다른 이벤트 핸들러들의 패턴을 확인하기 위해 다음 스크립트를 실행해보겠습니다:
src/main/java/com/gdschongik/gdsc/domain/study/domain/StudyHistory.java (1)
87-87: 이벤트 등록 구현이 잘 되었습니다!도메인 이벤트 패턴을 일관성 있게 적용하여 스터디 수료 철회 시 이벤트를 발행하도록 구현하였습니다.
src/main/java/com/gdschongik/gdsc/domain/study/application/MentorStudyHistoryService.java (1)
49-51: 이벤트 발행 구현이 잘 되었습니다!스터디 수료 처리 후 이벤트를 발행하여 쿠폰 발급 프로세스를 트리거하는 구현이 적절합니다.
| @Test | ||
| void 스터디_수료_쿠폰_이름이_생성된다() { | ||
| // given | ||
| Member mentor = fixtureHelper.createMentor(1L); | ||
| Study study = Study.create( | ||
| 2025, | ||
| SemesterType.FIRST, | ||
| "기초 백엔드 스터디", | ||
| mentor, | ||
| STUDY_ONGOING_PERIOD, | ||
| Period.of(START_DATE.minusDays(10), START_DATE.minusDays(5)), | ||
| TOTAL_WEEK, | ||
| ONLINE_STUDY, | ||
| DAY_OF_WEEK, | ||
| STUDY_START_TIME, | ||
| STUDY_END_TIME); | ||
|
|
||
| // when | ||
| String couponName = couponNameUtil.generateStudyCompletionCouponName(study); | ||
|
|
||
| // then | ||
| assertThat(couponName).isEqualTo("2025-1 기초 백엔드 스터디 수료 쿠폰"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
테스트 케이스 보완이 필요합니다.
현재는 정상 케이스만 테스트하고 있습니다. 다음과 같은 테스트 케이스들을 추가하는 것이 좋을 것 같습니다:
- null 입력 처리
- 특수문자가 포함된 스터디 제목
- 최대 길이의 스터디 제목
@Test
void null_스터디_입력시_예외가_발생한다() {
assertThatThrownBy(() -> couponNameUtil.generateStudyCompletionCouponName(null))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("스터디 정보는 null일 수 없습니다.");
}
@Test
void 특수문자가_포함된_스터디_제목으로_쿠폰_이름이_생성된다() {
Study study = Study.create(
2025,
SemesterType.FIRST,
"백엔드 스터디 (Spring)",
mentor,
STUDY_ONGOING_PERIOD,
Period.of(START_DATE.minusDays(10), START_DATE.minusDays(5)),
TOTAL_WEEK,
ONLINE_STUDY,
DAY_OF_WEEK,
STUDY_START_TIME,
STUDY_END_TIME);
String couponName = couponNameUtil.generateStudyCompletionCouponName(study);
assertThat(couponName).isEqualTo("2025-1 백엔드 스터디 (Spring) 수료 쿠폰");
}| @@ -63,6 +69,8 @@ public void withdrawStudyCompletion(StudyCompleteRequest request) { | |||
|
|
|||
| studyHistories.forEach(StudyHistory::withdrawCompletion); | |||
|
|
|||
| studyHistoryRepository.saveAll(studyHistories); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
불필요한 saveAll 호출이 있습니다.
@Transactional 어노테이션이 적용된 메서드에서는 영속성 컨텍스트가 자동으로 변경사항을 감지하여 저장합니다. 따라서 명시적인 saveAll 호출은 불필요합니다.
- studyHistoryRepository.saveAll(studyHistories);There was a problem hiding this comment.
엔티티 수정 시 변경 감지 기능만으로는 이벤트가 발행되지 않습니다. 도메인 이벤트 발행을 위해서는 명시적 save 호출이 필요합니다. 앞으로 JPA 변경 감지로 인한 명시적 save 호출과 관련된 변경내용은 제안하지 않아도 됩니다.
There was a problem hiding this comment.
@uwoobeat 이해했습니다. JPA 변경 감지와 도메인 이벤트 발행에 대한 설명 감사합니다. 앞으로는 이러한 맥락에서의 명시적 save 호출에 대해 제안하지 않도록 하겠습니다.
✏️ Learnings added
Learnt from: uwoobeat
PR: GDSC-Hongik/gdsc-server#843
File: src/main/java/com/gdschongik/gdsc/domain/study/application/MentorStudyHistoryService.java:72-72
Timestamp: 2025-01-13T12:44:03.865Z
Learning: In Spring JPA applications with domain events, explicit save/saveAll calls are necessary for domain event publishing, even in @Transactional methods. JPA's automatic change detection alone does not trigger domain events. Therefore, explicit repository save calls should not be removed when they are used in conjunction with domain events.
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.
| String couponName = couponNameUtil.generateStudyCompletionCouponName( | ||
| studyHistories.get(0).getStudy()); | ||
| // TODO: 요청할 때마다 새로운 쿠폰 생성되는 문제 수정: 스터디마다 하나의 쿠폰만 존재하도록 쿠폰 타입 및 참조 식별자 추가 |
There was a problem hiding this comment.
NPE 방지가 필요합니다.
studyHistories 리스트가 비어있을 경우 get(0) 호출 시 IndexOutOfBoundsException이 발생할 수 있습니다. 리스트가 비어있지 않은지 검증이 필요합니다.
+ if (studyHistories.isEmpty()) {
+ throw new CustomException(STUDY_HISTORY_NOT_FOUND);
+ }
String couponName = couponNameUtil.generateStudyCompletionCouponName(
studyHistories.get(0).getStudy());Committable suggestion skipped: line range outside the PR's diff.
| String couponName = couponNameUtil.generateStudyCompletionCouponName( | ||
| studyHistories.get(0).getStudy()); | ||
| // TODO: 요청할 때마다 새로운 쿠폰 생성되는 문제 수정: 스터디마다 하나의 쿠폰만 존재하도록 쿠폰 타입 및 참조 식별자 추가 | ||
| Coupon coupon = Coupon.create(couponName, Money.from(5000L)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
할인 금액이 하드코딩되어 있습니다.
쿠폰의 할인 금액이 5000원으로 하드코딩되어 있습니다. 이는 유연성이 떨어지며, 향후 할인 금액 변경 시 코드 수정이 필요합니다.
할인 금액을 설정 파일이나 데이터베이스에서 관리하는 것을 추천드립니다:
- Coupon coupon = Coupon.create(couponName, Money.from(5000L));
+ Coupon coupon = Coupon.create(couponName, Money.from(couponProperties.getStudyCompletionDiscountAmount()));Committable suggestion skipped: line range outside the PR's diff.
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/test/java/com/gdschongik/gdsc/domain/study/domain/StudyHistoryTest.java (1)
72-72: 이벤트 ID 처리 방식에 대한 아키텍처 개선 제안이벤트 ID를 위해 테스트에서
setId를 강제로 사용하는 것은 테스트 유지보수성을 저하시킬 수 있습니다. 다음과 같은 대안을 고려해보시기 바랍니다:
- 이벤트 발행 시점에 ID를 자동 생성하는 방식
- 이벤트 ID를 별도의 식별자 클래스로 캡슐화
- 테스트용 이벤트 팩토리 메서드 도입
src/test/java/com/gdschongik/gdsc/helper/FixtureHelper.java (2)
29-32: 테스트 유틸리티 메서드 구현이 깔끔합니다제네릭을 활용한 타입 안전성 확보가 잘 되어있습니다. 다만 다음 사항들을 고려해주시면 좋겠습니다:
- ReflectionTestUtils 사용을 최소화하는 것이 좋습니다
- 이 메서드의 사용 범위를 테스트 데이터 준비에만 국한하도록 문서화가 필요합니다
- 향후 엔티티 ID 생성 전략이 변경될 경우를 대비한 주석 추가가 필요합니다
11-11: BaseEntity 임포트 위치 개선정적 임포트(static import) 그룹과 일반 임포트 그룹 사이에 빈 줄을 추가하여 가독성을 높이는 것이 좋겠습니다.
import static com.gdschongik.gdsc.global.common.constant.StudyConstant.*; import static com.gdschongik.gdsc.global.common.constant.TemporalConstant.*; + import com.gdschongik.gdsc.domain.common.model.BaseEntity;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/test/java/com/gdschongik/gdsc/domain/study/domain/StudyHistoryTest.java(1 hunks)src/test/java/com/gdschongik/gdsc/helper/FixtureHelper.java(2 hunks)
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
| applicationEventPublisher.publishEvent(new StudyHistoriesCompletedEvent( | ||
| studyHistories.stream().map(StudyHistory::getId).toList())); |
There was a problem hiding this comment.
철회랑 다르게 이 부분은 ApplicationEventPublisher를 써서 이벤트 발행한 이유가 있나요?
There was a problem hiding this comment.
StudyHistory ID 여러 개를 묶어서 하나의 이벤트로 발행해야 하기 때문에 StudyHistory 내부에서 발행하는게 불가능합니다
| @@ -69,6 +69,7 @@ class 스터디_수료_철회시 { | |||
| mentor, Period.of(now.plusDays(5), now.plusDays(10)), Period.of(now.minusDays(5), now)); | |||
|
|
|||
| StudyHistory studyHistory = StudyHistory.create(student, study); | |||
| fixtureHelper.setId(studyHistory, 1L); // TODO: 이벤트 ID 필드를 원시 타입으로 설정하는 것 vs setId를 테스트 사용 강제 간 trade-off 고민 | |||
There was a problem hiding this comment.
pr description에 써두신 내용 읽어봤는데요,
모든 domain test에서 id가 필요한건 아니고 일부에서만 필요한거라 이번처럼 ReflectionTestUtils 사용하는게 나쁘지 않을 것 같습니다
🌱 관련 이슈
📌 작업 내용 및 특이사항
XXXEventHandler와XXXService가 한 쌍이 된다는 것만 확인해주세요.📝 참고사항
분할 수료 처리에 대한 문제 (1)
분할 수료 처리에 대한 문제 (2)
(MANUAL, STUDY, null)과 같이 마이그레이션 후, 2번 스터디에 대한 수료 쿠폰의 경우(AUTO, STUDY, 2L)와 같이 저장하는 것이죠.수료 철회 시 발급된 쿠폰 회수
이벤트의 엔티티 ID를 원시 타입으로 설정할 때 발생하는 문제
XXEvent(Long id)와 같이 nullable 했던 id 필드를 원시 타입으로 바꾸어 null을 허용하지 않도록 하는 TODO를 만들어뒀던 적이 있습니다.StudyHistoryCompletionWithdrawnEvent에서는 원시타입을 사용해봤는데, 문제는 도메인 단위 테스트에서는 PK가 생성되지 않아 id 필드가 null이 되기 때문에, 이벤트 발행 시registerEvent(new StudyHistoryCompletionWithdrawnEvent(this.id));에서this.id호출 시 언박싱 과정에서 NPE가 발생하게 됩니다.📚 기타
Summary by CodeRabbit
새로운 기능
이벤트 처리
테스트