-
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
fix: 스터디 중복신청 불가능하도록 수정 #710
Conversation
Walkthrough변경 사항은 Changes
Assessment against linked issues
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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/main/java/com/gdschongik/gdsc/domain/study/application/StudentStudyService.java (3 hunks)
- src/main/java/com/gdschongik/gdsc/domain/study/domain/Study.java (1 hunks)
- src/main/java/com/gdschongik/gdsc/domain/study/domain/StudyHistoryValidator.java (1 hunks)
Files skipped from review due to trivial changes (1)
- src/main/java/com/gdschongik/gdsc/domain/study/domain/StudyHistoryValidator.java
Additional context used
Learnings (1)
src/main/java/com/gdschongik/gdsc/domain/study/application/StudentStudyService.java (1)
Learnt from: Sangwook02 PR: GDSC-Hongik/gdsc-server#431 File: src/main/java/com/gdschongik/gdsc/domain/study/application/StudyService.java:50-57 Timestamp: 2024-07-07T15:32:34.451Z Learning: Consider using Stream API for creating lists in a more concise and potentially performant manner compared to traditional for-loops.
Additional comments not posted (3)
src/main/java/com/gdschongik/gdsc/domain/study/application/StudentStudyService.java (2)
45-45
: 코드 변경 승인
StudyHistory::isStudyOngoing
를 사용하여 스터디를 필터링하는 로직이 적절하게 구현되었습니다. Stream API를 사용하여 필터링 및 매핑하는 방식이 잘 적용되었습니다.코드 변경이 승인되었습니다.
109-111
: 코드 변경 승인
StudyHistory::isStudyOngoing
를 사용하여 스터디를 필터링하는 로직이 적절하게 구현되었습니다. Stream API를 사용하여 필터링하는 방식이 잘 적용되었습니다.코드 변경이 승인되었습니다.
src/main/java/com/gdschongik/gdsc/domain/study/domain/Study.java (1)
179-181
: 코드 변경 승인현재 날짜가 신청 시작일과 기간 종료일 사이에 있는지 확인하는 로직이 적절하게 구현되었습니다. 이 변경 사항은 스터디가 지정된 기간 내에만 진행 중으로 간주되도록 보장합니다.
코드 변경이 승인되었습니다.
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
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.
근데 ongoing은 현재 스터디의 진행여부를 보는 것이고
수강신청 가능 여부를 체크하려면 별도 메서드로 뽑는게 맞지 않나요?
메서드 이름도 수정이 필요하겠네요 |
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.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- src/main/java/com/gdschongik/gdsc/domain/study/application/StudentStudyService.java (3 hunks)
- src/main/java/com/gdschongik/gdsc/domain/study/domain/Study.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/StudyHistoryValidator.java (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/gdschongik/gdsc/domain/study/application/StudentStudyService.java
- src/main/java/com/gdschongik/gdsc/domain/study/domain/Study.java
public boolean isWithinApplicationAndCourse() { | ||
return study.isWithinApplicationAndCourse(); |
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.
메소드 이름 변경 승인 및 문서화 권장
isStudyOngoing
에서 isWithinApplicationAndCourse
로 메소드 이름이 변경되었습니다. 이 변경은 메소드의 기능을 더 명확하게 반영합니다. 그러나, 새로운 메소드의 기능을 설명하는 문서화가 필요합니다.
변경 사항을 승인합니다.
메소드의 기능을 설명하는 주석을 추가해드릴까요?
boolean hasAppliedStudy = | ||
currentMemberStudyHistories.stream().anyMatch(StudyHistory::isWithinApplicationAndCourse); | ||
|
||
if (isInOngoingStudy) { | ||
if (hasAppliedStudy) { |
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.
검증 로직 변경 승인 및 테스트 강화 권장
validateApplyStudy
메소드에서 isStudyOngoing
대신 isWithinApplicationAndCourse
를 사용하여 검증 로직이 변경되었습니다. 이 변경은 스터디 신청의 적절한 시간을 보다 정확하게 판단하기 위함입니다.
변경 사항을 승인합니다.
새로운 로직이 예상대로 작동하는지 확인하기 위해 포괄적인 테스트를 추가하는 것이 좋습니다. 테스트 코드를 작성해드릴까요?
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
🌱 관련 이슈
📌 작업 내용 및 특이사항
isOngoing
메서드는 스터디개강일 ~ 종강일
에 포함되는지를 판단했지만,수강신청 시작일 ~ 종강일
을 기준으로 해야 수강신청 종료일과 개강일 사이의 기간에 대한 처리가 가능합니다.📝 참고사항
📚 기타
Summary by CodeRabbit
기능 개선
StudyHistory
클래스를 통해 진행 중인 연구를 필터링하도록 변경하여 코드의 가독성과 유지보수성을 향상시켰습니다.주석 수정
StudyHistoryValidator
클래스의 주석을 수정하여 연구 상태 확인 로직에 대한 방향성을 명확히 하였습니다.