-
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: Study V2 도메인 구현 #853
Conversation
Warning Rate limit exceeded@uwoobeat has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
워크스루이 풀 리퀘스트는 Study V2 도메인 구현을 위한 주요 클래스들을 추가하고 있습니다. 변경 사항
연결된 이슈 평가
관련 가능성 있는 PR
제안된 라벨
제안된 리뷰어
시 (토끼의 노래)
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
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/java/com/gdschongik/gdsc/domain/studyv2/domain/StudySessionV2.java (1)
61-63
: [제안] 부모 엔티티에서 자식 엔티티의 생명주기를 명확히 제어하기 위한 설정 고민
@ManyToOne
을 통해 양방향 관계를 맺고 있습니다. StudyV2가 애그리거트 루트라면, 필요 시cascade = CascadeType.ALL
또는orphanRemoval = true
등을 고려하셔서 자식 엔티티의 생명주기를 명확히 제어할 수 있습니다.src/main/java/com/gdschongik/gdsc/domain/studyv2/domain/StudyV2.java (1)
92-93
: [제안] 애그리거트 루트의 자식 엔티티 관리 시점 재검토
@OneToMany(mappedBy = "studyV2")
로 자식 엔티티를 보유하고 있습니다. StudySessionV2의 생명주기를 StudyV2에서 온전히 관리하려면cascade
및orphanRemoval
옵션을 추가 고려해 볼 수 있습니다.src/main/java/com/gdschongik/gdsc/domain/common/vo/Semester.java (1)
18-21
: [사소한 제안] 학년도와 학기 타입에 대한 기본 유효성 검증 고려
academicYear
와semesterType
에 대해 적절한 범위나 값 검증이 추가되면 도메인 무결성을 더 강화할 수 있습니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/gdschongik/gdsc/domain/common/vo/Semester.java
(1 hunks)src/main/java/com/gdschongik/gdsc/domain/recruitment/domain/RecruitmentRoundValidator.java
(1 hunks)src/main/java/com/gdschongik/gdsc/domain/studyv2/domain/StudySessionV2.java
(1 hunks)src/main/java/com/gdschongik/gdsc/domain/studyv2/domain/StudyV2.java
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/com/gdschongik/gdsc/domain/recruitment/domain/RecruitmentRoundValidator.java
🔇 Additional comments (2)
src/main/java/com/gdschongik/gdsc/domain/studyv2/domain/StudySessionV2.java (2)
25-26
: [주석] Lombok과 JPA 엔티티 결합 사용이 적절해 보입니다.
이 부분은@RequiredArgsConstructor(access = AccessLevel.PROTECTED)
로 JPA 스펙을 준수하고 있어 무난해 보입니다.
90-107
: [확인 요망]create
메서드 내부에서 빌더 결과를 반환 또는 활용하지 않는 로직
create
메서드에서StudySessionV2.builder().build()
로 새로운 객체를 만든 뒤 반환값 없이 종료됩니다.
물론 빌더로 생성된 객체가 생성자 내부에서studyV2.getStudySessions().add(this);
를 호출하고 있지만, 실제로 호출부에서 반환값이 없어도 문제없이 세션이 영속화되는지(혹은 분리/삭제 시점에서 문제없는지) 확인이 필요합니다.
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
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.
확인했습니다
private Semester semester; | ||
|
||
@Comment("총 회차 수") | ||
private Integer totalWeek; |
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.
변수명도 회차라는 의미 담아서 다시 지으면 어떤가요?
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.
totalRound로 가겠읍니다
@Column(columnDefinition = "TEXT") | ||
private String descriptionNotionLink; | ||
|
||
@Enumerated(EnumType.STRING) |
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.
@Enumerated(EnumType.STRING)
필요한가요?
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.
@Embedded
인데 실수했네요 굿
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
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.
lgtm
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
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.
엔티티 필드 순서 미리 맞추는 것만 코멘트 남겼습니다
public void create( | ||
String title, | ||
String description, |
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.
혹시 static
키워드 있어야 하지 않나요?
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.
깜빡했네요 수정완
@Embedded | ||
@AttributeOverride(name = "startDate", column = @Column(name = "lesson_start_at")) | ||
@AttributeOverride(name = "endDate", column = @Column(name = "lesson_end_at")) | ||
private Period lessonPeriod; | ||
|
||
// 과제 관련 필드 | ||
|
||
@Comment("과제 명세 링크") | ||
private String assignmentDescriptionLink; |
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.
임베디드랑 일반 필드 순서 맞추는 거 새로운 엔티티에서는 미리 맞춰둬도 좋을 거 같아요
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.
그그그그 이거 하면서 좀 생각을 해봤는데, 일반 필드 -> VO는 꼭 맞춰야 하나? 싶더라고요.
요 케이스처럼 필드 성격에 따라 묶이는 게 적절할 것 같은 케이스도 있고...
물론 ID -> 상태 / 타입 -> 일반 / VO 필드 -> 참조 필드 순서는 지키되 일반/VO는 굳이 안지켜도 될 것 같은데 어떻게 생각하시나용
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.
음 저도 성격에 따라 묶는 게 더 좋은 것 같아요 그럼 이대로 가죠 ㅎㅎ
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.
굿입니다 그 필드 순서 이슈 작업 건에서도 참고해주세용
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
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.
lgtm
🌱 관련 이슈
📌 작업 내용 및 특이사항
📝 참고사항
📚 기타
Summary by CodeRabbit
새로운 기능
리팩토링
이번 릴리즈는 스터디 관리 시스템의 도메인 모델을 확장하고 개선하는 데 중점을 두었습니다.