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.

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: 스터디 수료 및 철회 API 구현 #819

Merged
merged 7 commits into from
Nov 12, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
package com.gdschongik.gdsc.domain.study.api;

import com.gdschongik.gdsc.domain.study.application.MentorStudyHistoryService;
import com.gdschongik.gdsc.domain.study.dto.request.StudyCompletionRequest;
import io.swagger.v3.oas.annotations.Operation;
import io.swagger.v3.oas.annotations.tags.Tag;
import jakarta.validation.Valid;
import lombok.RequiredArgsConstructor;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.PatchMapping;
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.RestController;

@Tag(name = "Mentor Study History", description = "멘토 스터디 수강 이력 API입니다.")
@RestController
@RequestMapping("/mentor/study-history")
@RequiredArgsConstructor
public class MentorStudyHistoryController {

private final MentorStudyHistoryService mentorStudyHistoryService;

Comment on lines +14 to +21
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

보안 및 권한 검증이 필요합니다.

컨트롤러 레벨에서 다음 보안 관련 개선사항이 필요합니다:

  1. 멘토 권한 검증을 위한 @PreAuthorize("hasRole('MENTOR')") 어노테이션 추가
  2. 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.

Suggested change
@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 = "스터디 수료 처리합니다.")
@PatchMapping("/complete")
Copy link
Member

Choose a reason for hiding this comment

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

patch보다는 post가 적절할 것 같습니다.
uri 단에서 리소스가 식별 가능할 때 patch/put을 쓰는데
여기서는 리소스 식별을 request body로 처리하고 있기 때문에 '이 request uri가 지시하는 리소스 프로퍼티의 일부를 업데이트한다' 라는 patch의 정의와 그닥 일치하지 않는 것 같네요

public ResponseEntity<Void> completeStudy(
@RequestParam(name = "studyId") Long studyId, @Valid @RequestBody StudyCompletionRequest request) {
Copy link
Member

Choose a reason for hiding this comment

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

request dto 안에 studyId를 넣는게 좋을 것 같네요

mentorStudyHistoryService.completeStudy(studyId, request);
return ResponseEntity.ok().build();
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

응답 처리와 검증 로직 개선이 필요합니다.

현재 구현에서 몇 가지 개선 포인트가 있습니다:

  1. 성공 응답에 대한 명확한 메시지나 상태 정보가 없습니다.
  2. 예외 처리에 대한 명시적인 처리가 없습니다.

다음과 같은 개선을 제안드립니다:

-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.


@Operation(summary = "스터디 수료 철회", description = "스터디 수료 처리를 철회합니다.")
@PatchMapping("/withdraw-completion")
public ResponseEntity<Void> withdrawStudyCompletion(
@RequestParam(name = "studyId") Long studyId, @Valid @RequestBody StudyCompletionRequest request) {
Copy link
Member

Choose a reason for hiding this comment

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

DTO에 제약 조건이 없는데 혹시 @Valid 어노테이션이 List<Long> studentIds 에 대해 기본적으로 검증하는 동작이 있나요?

Copy link
Member Author

Choose a reason for hiding this comment

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

복붙이 이렇게 무섭습니다,,

mentorStudyHistoryService.withdrawStudyCompletion(studyId, request);
return ResponseEntity.ok().build();
}
Sangwook02 marked this conversation as resolved.
Show resolved Hide resolved
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
package com.gdschongik.gdsc.domain.study.application;

import static com.gdschongik.gdsc.global.exception.ErrorCode.*;

import com.gdschongik.gdsc.domain.member.domain.Member;
import com.gdschongik.gdsc.domain.study.dao.StudyHistoryRepository;
import com.gdschongik.gdsc.domain.study.dao.StudyRepository;
import com.gdschongik.gdsc.domain.study.domain.Study;
import com.gdschongik.gdsc.domain.study.domain.StudyHistory;
import com.gdschongik.gdsc.domain.study.domain.StudyHistoryValidator;
import com.gdschongik.gdsc.domain.study.domain.StudyValidator;
import com.gdschongik.gdsc.domain.study.dto.request.StudyCompletionRequest;
import com.gdschongik.gdsc.global.exception.CustomException;
import com.gdschongik.gdsc.global.util.MemberUtil;
import java.util.List;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

@Slf4j
@Service
@RequiredArgsConstructor
public class MentorStudyHistoryService {

private final MemberUtil memberUtil;
private final StudyValidator studyValidator;
private final StudyHistoryValidator studyHistoryValidator;
private final StudyRepository studyRepository;
private final StudyHistoryRepository studyHistoryRepository;

@Transactional
public void completeStudy(Long studyId, StudyCompletionRequest request) {
Member currentMember = memberUtil.getCurrentMember();
Study study = studyRepository.findById(studyId).orElseThrow(() -> new CustomException(STUDY_NOT_FOUND));
List<StudyHistory> studyHistories = studyHistoryRepository.findAllById(request.studentIds());

Copy link

@coderabbitai coderabbitai bot Nov 6, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

StudyHistory의 Study 일치 여부 검증 필요

현재 studyHistoriesstudentIds로만 조회하고 있습니다. 그러나 이들이 요청된 studyId와 연관된 StudyHistory인지 검증하지 않고 있습니다. 해당 검증이 없으면 다른 스터디의 StudyHistory를 처리할 위험이 있습니다.

수정 제안:

 List<StudyHistory> studyHistories = studyHistoryRepository.findAllById(request.studentIds());
+studyHistories = studyHistories.stream()
+    .filter(history -> history.getStudy().getId().equals(studyId))
+    .collect(Collectors.toList());

또는 studyIdstudentIds를 동시에 사용하는 새로운 메서드를 studyHistoryRepository에 추가하여 정확한 StudyHistory를 조회할 수 있습니다.

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Member Author

Choose a reason for hiding this comment

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

굿

Copy link

Choose a reason for hiding this comment

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

@Sangwook02, 수정 사항을 반영해 주셔서 감사합니다!


If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!

studyValidator.validateStudyMentor(currentMember, study);
studyHistoryValidator.validateAppliedToStudy(
studyHistories.size(), request.studentIds().size());

studyHistories.forEach(StudyHistory::complete);

log.info("[MentorStudyHistoryService] 스터디 수료 처리: studyId={}, studentIds={}", studyId, request.studentIds());
}

@Transactional
public void withdrawStudyCompletion(Long studyId, StudyCompletionRequest request) {
Member currentMember = memberUtil.getCurrentMember();
Study study = studyRepository.findById(studyId).orElseThrow(() -> new CustomException(STUDY_NOT_FOUND));
List<StudyHistory> studyHistories = studyHistoryRepository.findAllById(request.studentIds());

Copy link

@coderabbitai coderabbitai bot Nov 6, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

StudyHistory의 Study 일치 여부 검증 필요

withdrawStudyCompletion 메서드에서도 studyHistoriesstudentIds로만 조회하고 있으며, studyId와의 연관성을 확인하지 않고 있습니다. 앞서 언급한 방식으로 수정하여 일관된 데이터 처리를 보장해야 합니다.

수정 제안:

 List<StudyHistory> studyHistories = studyHistoryRepository.findAllById(request.studentIds());
+studyHistories = studyHistories.stream()
+    .filter(history -> history.getStudy().getId().equals(studyId))
+    .collect(Collectors.toList());

또는 studyHistoryRepositoryfindByStudyIdAndStudentIds와 같은 메서드를 구현하여 정확한 데이터를 조회하세요.

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Member Author

Choose a reason for hiding this comment

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

굿

Copy link

Choose a reason for hiding this comment

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

@Sangwook02, 감사합니다!

studyValidator.validateStudyMentor(currentMember, study);
studyHistoryValidator.validateAppliedToStudy(
studyHistories.size(), request.studentIds().size());

studyHistories.forEach(StudyHistory::withdrawCompletion);

log.info("[MentorStudyHistoryService] 스터디 수료 철회: studyId={}, studentIds={}", studyId, request.studentIds());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,13 @@ public void complete() {
studyHistoryStatus = COMPLETED;
}

/**
* 스터디 수료 철회
*/
public void withdrawCompletion() {
studyHistoryStatus = NONE;
}
Comment on lines +81 to +86
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

문서화 개선 및 상태 검증 로직 추가 필요

현재 구현은 기본적인 기능은 제공하지만, 다음 사항들을 고려해 주시기 바랍니다:

  1. JavaDoc에 메서드의 사용 조건, 제약 사항, 부작용 등을 상세히 기술해 주세요.
  2. 현재 상태가 COMPLETED인 경우에만 철회가 가능하도록 검증 로직을 추가하는 것이 좋겠습니다.
  3. 수료 철회와 같은 중요한 상태 변경 시 이벤트를 발행하여 다른 시스템에서 대응할 수 있도록 하는 것을 고려해 주세요.

다음과 같은 개선을 제안드립니다:

    /**
     * 스터디 수료 철회
+     * 
+     * @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.


// 데이터 전달 로직
public boolean isWithinApplicationAndCourse() {
return study.isWithinApplicationAndCourse();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package com.gdschongik.gdsc.domain.study.dto.request;

import java.util.List;

public record StudyCompletionRequest(List<Long> studentIds) {}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public record StudyCompletionRequest(List<Long> studentIds) {}
public record StudyCompleteRequest(List<Long> studentIds) {}

Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,27 @@ class 스터디_수료시 {
assertThat(studyHistory.getStudyHistoryStatus()).isEqualTo(COMPLETED);
}
}

@Nested
class 스터디_수료_철회시 {

@Test
void 수료상태는_NONE이다() {
// 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);
studyHistory.complete();

// when
studyHistory.withdrawCompletion();

// then
assertThat(studyHistory.getStudyHistoryStatus()).isEqualTo(NONE);
}
}
}