feat: 스터디 수료 및 철회 API 구현 #819
Conversation
Walkthrough이 변경 사항은 멘토의 스터디 이력 관리를 위한 REST 컨트롤러와 서비스 클래스를 추가합니다. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
|
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
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: 5
🧹 Outside diff range and nitpick comments (4)
src/main/java/com/gdschongik/gdsc/domain/study/api/MentorStudyHistoryController.java (1)
16-20: API 경로 구조 개선을 제안드립니다.현재 경로
/mentor/study-history는 리소스 중심의 RESTful 설계와 완벽하게 일치하지 않습니다. 다음과 같은 구조를 고려해보시는 것은 어떨까요?-@RequestMapping("/mentor/study-history") +@RequestMapping("/api/v1/studies/{studyId}/mentors/history")이렇게 변경하면:
- 리소스 계층 구조가 더 명확해집니다
- API 버전 관리가 용이해집니다
- 경로 변수를 통해 studyId를 더 RESTful하게 처리할 수 있습니다
src/test/java/com/gdschongik/gdsc/domain/study/domain/StudyHistoryTest.java (1)
62-79: 테스트 케이스에 대한 개선 제안테스트 구현이 기본적인 시나리오는 잘 커버하고 있습니다만, 다음 사항들을 고려해보시면 좋겠습니다:
- 수료 상태가 아닌 경우(NONE)에 대한 철회 시도
- 이미 철회된 상태에서의 중복 철회 시도
다음과 같은 테스트 케이스 추가를 제안드립니다:
@Test void 수료상태가_아닐때_철회시_예외가_발생한다() { // given Member student = fixtureHelper.createRegularMember(1L); Member mentor = fixtureHelper.createRegularMember(2L); LocalDateTime now = LocalDateTime.now(); Study study = fixtureHelper.createStudy( mentor, Period.of(now.plusDays(5), now.plusDays(10)), Period.of(now.minusDays(5), now)); StudyHistory studyHistory = StudyHistory.create(student, study); // when & then assertThatThrownBy(() -> studyHistory.withdrawCompletion()) .isInstanceOf(IllegalStateException.class); }src/main/java/com/gdschongik/gdsc/domain/study/application/MentorStudyHistoryService.java (2)
33-45: 중복된 코드의 모듈화로 유지보수성 향상
completeStudy와withdrawStudyCompletion메서드에서 유사한 로직이 반복되고 있습니다. 중복 코드를 제거하고 공통 로직을 별도의 메서드로 추출하면 코드의 가독성과 유지보수성이 향상됩니다.예시:
private List<StudyHistory> getValidatedStudyHistories(Long studyId, StudyCompletionRequest request, Member currentMember) { Study study = studyRepository.findById(studyId) .orElseThrow(() -> new CustomException(STUDY_NOT_FOUND)); List<StudyHistory> studyHistories = studyHistoryRepository.findAllById(request.studentIds()); studyValidator.validateStudyMentor(currentMember, study); studyHistoryValidator.validateAppliedToStudy( studyHistories.size(), request.studentIds().size()); studyHistories = studyHistories.stream() .filter(history -> history.getStudy().getId().equals(studyId)) .collect(Collectors.toList()); return studyHistories; }이렇게 공통 로직을 추출하여 각 메서드에서 활용할 수 있습니다.
Also applies to: 48-60
44-44: 로그 정보의 개인정보 보호 고려로그에
studentIds를 그대로 출력하면 개인정보 유출의 위험이 있습니다. 로그에 민감한 정보를 포함시키지 않도록 주의하고, 필요 시 마스킹하거나 로그 레벨을 조정하세요.수정 제안:
-log.info("[MentorStudyHistoryService] 스터디 수료 처리: studyId={}, studentIds={}", studyId, request.studentIds()); +log.info("[MentorStudyHistoryService] 스터디 수료 처리: studyId={}, 처리된 학생 수={}", studyId, studyHistories.size());Also applies to: 59-59
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/main/java/com/gdschongik/gdsc/domain/study/api/MentorStudyHistoryController.java(1 hunks)src/main/java/com/gdschongik/gdsc/domain/study/application/MentorStudyHistoryService.java(1 hunks)src/main/java/com/gdschongik/gdsc/domain/study/domain/StudyHistory.java(1 hunks)src/main/java/com/gdschongik/gdsc/domain/study/dto/request/StudyCompletionRequest.java(1 hunks)src/test/java/com/gdschongik/gdsc/domain/study/domain/StudyHistoryTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/com/gdschongik/gdsc/domain/study/dto/request/StudyCompletionRequest.java
🔇 Additional comments (4)
src/main/java/com/gdschongik/gdsc/domain/study/api/MentorStudyHistoryController.java (2)
1-15: 패키지 구조와 임포트가 잘 구성되어 있습니다!
필요한 모든 의존성이 적절하게 임포트되어 있으며, 패키지 구조가 도메인 중심으로 잘 구성되어 있습니다.
22-22: 의존성 주입이 적절하게 구현되어 있습니다!
생성자 주입 방식을 사용하여 불변성을 보장하고 있습니다.
src/test/java/com/gdschongik/gdsc/domain/study/domain/StudyHistoryTest.java (1)
59-60: 테스트 클래스 구조가 잘 구성되어 있습니다.
중첩 클래스를 사용하여 테스트 시나리오를 명확하게 구분한 점이 좋습니다. 한글 네이밍을 통해 테스트의 의도가 명확히 전달됩니다.
src/main/java/com/gdschongik/gdsc/domain/study/application/MentorStudyHistoryService.java (1)
35-35: STUDY_NOT_FOUND 예외 처리 통일성 확인
studyRepository.findById()에서 CustomException을 사용하여 예외를 처리하고 있습니다. 전체 코드베이스에서 예외 처리를 일관성 있게 사용하고 있는지 확인하고, 필요하다면 예외 메시지나 처리 방식을 표준화하세요.
Also applies to: 50-50
src/main/java/com/gdschongik/gdsc/domain/study/api/MentorStudyHistoryController.java
Outdated
Show resolved
Hide resolved
| @Operation(summary = "스터디 수료 처리", description = "스터디 수료 처리합니다.") | ||
| @PatchMapping("/complete") | ||
| public ResponseEntity<Void> completeStudy( | ||
| @RequestParam(name = "studyId") Long studyId, @Valid @RequestBody StudyCompletionRequest request) { | ||
| mentorStudyHistoryService.completeStudy(studyId, request); | ||
| return ResponseEntity.ok().build(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
응답 처리와 검증 로직 개선이 필요합니다.
현재 구현에서 몇 가지 개선 포인트가 있습니다:
- 성공 응답에 대한 명확한 메시지나 상태 정보가 없습니다.
- 예외 처리에 대한 명시적인 처리가 없습니다.
다음과 같은 개선을 제안드립니다:
-public ResponseEntity<Void> completeStudy(
+public ResponseEntity<ApiResponse> completeStudy(
@RequestParam(name = "studyId") Long studyId,
@Valid @RequestBody StudyCompletionRequest request) {
mentorStudyHistoryService.completeStudy(studyId, request);
- return ResponseEntity.ok().build();
+ return ResponseEntity.ok(new ApiResponse("스터디 수료 처리가 완료되었습니다."));
}또한 다음 사항도 고려해보시기 바랍니다:
@RequestParam대신@PathVariable사용- 요청 유효성 검증을 위한 커스텀 어노테이션 추가
Committable suggestion skipped: line range outside the PR's diff.
| /** | ||
| * 스터디 수료 철회 | ||
| */ | ||
| public void withdrawCompletion() { | ||
| studyHistoryStatus = NONE; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
문서화 개선 및 상태 검증 로직 추가 필요
현재 구현은 기본적인 기능은 제공하지만, 다음 사항들을 고려해 주시기 바랍니다:
- JavaDoc에 메서드의 사용 조건, 제약 사항, 부작용 등을 상세히 기술해 주세요.
- 현재 상태가 COMPLETED인 경우에만 철회가 가능하도록 검증 로직을 추가하는 것이 좋겠습니다.
- 수료 철회와 같은 중요한 상태 변경 시 이벤트를 발행하여 다른 시스템에서 대응할 수 있도록 하는 것을 고려해 주세요.
다음과 같은 개선을 제안드립니다:
/**
* 스터디 수료 철회
+ *
+ * @throws IllegalStateException 현재 상태가 COMPLETED가 아닌 경우
*/
public void withdrawCompletion() {
+ if (studyHistoryStatus != COMPLETED) {
+ throw new IllegalStateException("수료 상태인 경우에만 철회가 가능합니다.");
+ }
studyHistoryStatus = NONE;
+ registerEvent(new StudyCompletionWithdrawnEvent(this.study.getId(), this.student.getId()));
}Committable suggestion skipped: line range outside the PR's diff.
| List<StudyHistory> studyHistories = studyHistoryRepository.findAllById(request.studentIds()); | ||
|
|
There was a problem hiding this comment.
StudyHistory의 Study 일치 여부 검증 필요
현재 studyHistories를 studentIds로만 조회하고 있습니다. 그러나 이들이 요청된 studyId와 연관된 StudyHistory인지 검증하지 않고 있습니다. 해당 검증이 없으면 다른 스터디의 StudyHistory를 처리할 위험이 있습니다.
수정 제안:
List<StudyHistory> studyHistories = studyHistoryRepository.findAllById(request.studentIds());
+studyHistories = studyHistories.stream()
+ .filter(history -> history.getStudy().getId().equals(studyId))
+ .collect(Collectors.toList());또는 studyId와 studentIds를 동시에 사용하는 새로운 메서드를 studyHistoryRepository에 추가하여 정확한 StudyHistory를 조회할 수 있습니다.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@Sangwook02, 수정 사항을 반영해 주셔서 감사합니다!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| List<StudyHistory> studyHistories = studyHistoryRepository.findAllById(request.studentIds()); | ||
|
|
There was a problem hiding this comment.
StudyHistory의 Study 일치 여부 검증 필요
withdrawStudyCompletion 메서드에서도 studyHistories를 studentIds로만 조회하고 있으며, studyId와의 연관성을 확인하지 않고 있습니다. 앞서 언급한 방식으로 수정하여 일관된 데이터 처리를 보장해야 합니다.
수정 제안:
List<StudyHistory> studyHistories = studyHistoryRepository.findAllById(request.studentIds());
+studyHistories = studyHistories.stream()
+ .filter(history -> history.getStudy().getId().equals(studyId))
+ .collect(Collectors.toList());또는 studyHistoryRepository에 findByStudyIdAndStudentIds와 같은 메서드를 구현하여 정확한 데이터를 조회하세요.
Committable suggestion skipped: line range outside the PR's diff.
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
| public ResponseEntity<Void> withdrawStudyCompletion( | ||
| @RequestParam(name = "studyId") Long studyId, @Valid @RequestBody StudyCompletionRequest request) { |
There was a problem hiding this comment.
DTO에 제약 조건이 없는데 혹시 @Valid 어노테이션이 List<Long> studentIds 에 대해 기본적으로 검증하는 동작이 있나요?
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
|
|
||
| import java.util.List; | ||
|
|
||
| public record StudyCompletionRequest(List<Long> studentIds) {} |
There was a problem hiding this comment.
| public record StudyCompletionRequest(List<Long> studentIds) {} | |
| public record StudyCompleteRequest(List<Long> studentIds) {} |
| @Operation(summary = "스터디 수료 처리", description = "스터디 수료 처리합니다.") | ||
| @PatchMapping("/complete") | ||
| public ResponseEntity<Void> completeStudy( | ||
| @RequestParam(name = "studyId") Long studyId, @Valid @RequestBody StudyCompletionRequest request) { |
There was a problem hiding this comment.
request dto 안에 studyId를 넣는게 좋을 것 같네요
| private final MentorStudyHistoryService mentorStudyHistoryService; | ||
|
|
||
| @Operation(summary = "스터디 수료 처리", description = "스터디 수료 처리합니다.") | ||
| @PatchMapping("/complete") |
There was a problem hiding this comment.
patch보다는 post가 적절할 것 같습니다.
uri 단에서 리소스가 식별 가능할 때 patch/put을 쓰는데
여기서는 리소스 식별을 request body로 처리하고 있기 때문에 '이 request uri가 지시하는 리소스 프로퍼티의 일부를 업데이트한다' 라는 patch의 정의와 그닥 일치하지 않는 것 같네요
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
src/main/java/com/gdschongik/gdsc/domain/study/api/MentorStudyHistoryController.java (1)
1-35: API 설계 개선이 필요합니다.전반적인 API 설계 관련 개선사항입니다:
studyId를 URL 경로에 포함하는 것이 REST 원칙에 더 부합합니다- 일관된 응답 형식 적용이 필요합니다
-@RequestMapping("/mentor/study-history") +@RequestMapping("/mentor/studies/{studyId}/history") public class MentorStudyHistoryController { // ... - @PostMapping("/complete") + @PostMapping("/completion") public ResponseEntity<ApiResponse> completeStudy( + @PathVariable Long studyId, @Valid @RequestBody StudyCompleteRequest request) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/main/java/com/gdschongik/gdsc/domain/study/api/MentorStudyAchievementController.java(1 hunks)src/main/java/com/gdschongik/gdsc/domain/study/api/MentorStudyHistoryController.java(1 hunks)src/main/java/com/gdschongik/gdsc/domain/study/application/MentorStudyHistoryService.java(1 hunks)src/main/java/com/gdschongik/gdsc/domain/study/dto/request/StudyCompleteRequest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/com/gdschongik/gdsc/domain/study/dto/request/StudyCompleteRequest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/gdschongik/gdsc/domain/study/application/MentorStudyHistoryService.java
🔇 Additional comments (1)
src/main/java/com/gdschongik/gdsc/domain/study/api/MentorStudyAchievementController.java (1)
27-27: @Valid 어노테이션 제거에 대한 검토가 필요합니다.
요청 데이터 검증은 애플리케이션의 안정성과 데이터 무결성을 보장하는 중요한 요소입니다. @Valid 어노테이션이 제거됨으로써 잘못된 형식의 요청이 서비스 계층까지 도달할 수 있습니다.
다음 두 가지 방안 중 하나를 고려해주시기 바랍니다:
- @Valid 어노테이션을 다시 추가
- 서비스 계층에서 포괄적인 유효성 검사가 이루어지는지 확인
Also applies to: 35-35
| @Tag(name = "Mentor Study History", description = "멘토 스터디 수강 이력 API입니다.") | ||
| @RestController | ||
| @RequestMapping("/mentor/study-history") | ||
| @RequiredArgsConstructor | ||
| public class MentorStudyHistoryController { | ||
|
|
||
| private final MentorStudyHistoryService mentorStudyHistoryService; | ||
|
|
There was a problem hiding this comment.
보안 및 권한 검증이 필요합니다.
컨트롤러 레벨에서 다음 보안 관련 개선사항이 필요합니다:
- 멘토 권한 검증을 위한
@PreAuthorize("hasRole('MENTOR')")어노테이션 추가 - CSRF 보호를 위한 적절한 보안 설정 검토
@Tag(name = "Mentor Study History", description = "멘토 스터디 수강 이력 API입니다.")
+@PreAuthorize("hasRole('MENTOR')")
@RestController
@RequestMapping("/mentor/study-history")
@RequiredArgsConstructor
public class MentorStudyHistoryController {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Tag(name = "Mentor Study History", description = "멘토 스터디 수강 이력 API입니다.") | |
| @RestController | |
| @RequestMapping("/mentor/study-history") | |
| @RequiredArgsConstructor | |
| public class MentorStudyHistoryController { | |
| private final MentorStudyHistoryService mentorStudyHistoryService; | |
| @Tag(name = "Mentor Study History", description = "멘토 스터디 수강 이력 API입니다.") | |
| @PreAuthorize("hasRole('MENTOR')") | |
| @RestController | |
| @RequestMapping("/mentor/study-history") | |
| @RequiredArgsConstructor | |
| public class MentorStudyHistoryController { | |
| private final MentorStudyHistoryService mentorStudyHistoryService; |
| @Operation(summary = "스터디 수료 처리", description = "스터디 수료 처리합니다.") | ||
| @PostMapping("/complete") | ||
| public ResponseEntity<Void> completeStudy(@RequestBody StudyCompleteRequest request) { | ||
| mentorStudyHistoryService.completeStudy(request); | ||
| return ResponseEntity.ok().build(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
요청 검증 및 응답 처리 개선이 필요합니다.
다음 개선사항을 제안드립니다:
@Valid어노테이션으로 요청 데이터 검증- 명확한 응답 메시지 제공
- 예외 처리 추가
@Operation(summary = "스터디 수료 처리", description = "스터디 수료 처리합니다.")
@PostMapping("/complete")
-public ResponseEntity<Void> completeStudy(@RequestBody StudyCompleteRequest request) {
+public ResponseEntity<ApiResponse> completeStudy(@Valid @RequestBody StudyCompleteRequest request) {
mentorStudyHistoryService.completeStudy(request);
- return ResponseEntity.ok().build();
+ return ResponseEntity.ok(new ApiResponse("스터디 수료 처리가 완료되었습니다."));
}Committable suggestion skipped: line range outside the PR's diff.
| @Operation(summary = "스터디 수료 철회", description = "스터디 수료 처리를 철회합니다.") | ||
| @PostMapping("/withdraw-completion") | ||
| public ResponseEntity<Void> withdrawStudyCompletion(@RequestBody StudyCompleteRequest request) { | ||
| mentorStudyHistoryService.withdrawStudyCompletion(request); | ||
| return ResponseEntity.ok().build(); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
수료 철회 기능에 제한사항이 구현되지 않았습니다.
- 수료 철회 가능 기간 제한
- 철회 사유 필드 추가
- 철회 이력 관리
🔗 Analysis chain
수료 철회 기능에 대한 제한사항 검토가 필요합니다.
다음 사항들을 고려해주세요:
- 수료 철회 가능 기간 제한
- 철회 사유 필드 추가
- 철회 이력 관리
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 수료 철회 관련 비즈니스 로직 확인
ast-grep --pattern 'class StudyHistory {
$$$
withdraw$_($$$) {
$$$
}
$$$
}'
Length of output: 89
Script:
#!/bin/bash
# withdrawStudyCompletion 메소드 정의 검색
rg 'public .* withdrawStudyCompletion' --context 2
Length of output: 1433
Script:
#!/bin/bash
# withdrawStudyCompletion 메소드 구현 내용 검색
ast-grep --pattern 'class MentorStudyHistoryService {
$$$
public void withdrawStudyCompletion($_) {
$$$
}
$$$
}'
# StudyCompleteRequest 클래스 정의 검색
fd 'StudyCompleteRequest.java' | xargs cat
# 철회 이력 관리 관련 코드 검색
rg 'withdrawalHistory' --context 2
Length of output: 358
🌱 관련 이슈
📌 작업 내용 및 특이사항
📝 참고사항
📚 기타
Summary by CodeRabbit
신규 기능
StudyCompleteRequest데이터 구조 도입.버그 수정
MentorStudyAchievementController의 유효성 검사 주석 제거로 인해 요청 처리 개선.테스트