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 구현 #800

Merged
merged 10 commits into from
Oct 11, 2024
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package com.gdschongik.gdsc.domain.study.api;

import com.gdschongik.gdsc.domain.study.application.MentorStudyAchievementService;
import com.gdschongik.gdsc.domain.study.dto.request.OutstandingStudentRequest;
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.DeleteMapping;
import org.springframework.web.bind.annotation.PostMapping;
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 StudyAchievement", description = "멘토 스터디 우수 스터디원 관리 API입니다.")
@RestController
@RequestMapping("/mentor/study-achievements")
@RequiredArgsConstructor
public class MentorStudyAchievementController {

private final MentorStudyAchievementService mentorStudyAchievementService;

@Operation(summary = "우수 스터디원 지정", description = "우수 스터디원으로 지정합니다.")
@PostMapping
public ResponseEntity<Void> designateOutstandingStudent(
@RequestParam(name = "studyId") Long studyId, @Valid @RequestBody OutstandingStudentRequest request) {
mentorStudyAchievementService.designateOutstandingStudent(studyId, request);
return ResponseEntity.ok().build();
}
Comment on lines +25 to +31
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. 성공 시 201 Created 응답을 반환하는 것이 더 적절할 수 있습니다.
  2. 예외 처리를 추가하여 다양한 오류 상황에 대응하는 것이 좋습니다.

다음과 같이 코드를 수정해 보세요:

 @PostMapping
 public ResponseEntity<Void> designateOutstandingStudent(
         @RequestParam(name = "studyId") Long studyId, @Valid @RequestBody OutstandingStudentRequest request) {
     mentorStudyAchievementService.designateOutstandingStudent(studyId, request);
-    return ResponseEntity.ok().build();
+    return ResponseEntity.status(HttpStatus.CREATED).build();
 }

또한, 전역 예외 처리기를 구현하여 다양한 예외 상황에 대응하는 것을 고려해 보세요.

📝 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
@Operation(summary = "우수 스터디원 지정", description = "우수 스터디원으로 지정합니다.")
@PostMapping
public ResponseEntity<Void> designateOutstandingStudent(
@RequestParam(name = "studyId") Long studyId, @Valid @RequestBody OutstandingStudentRequest request) {
mentorStudyAchievementService.designateOutstandingStudent(studyId, request);
return ResponseEntity.ok().build();
}
@Operation(summary = "우수 스터디원 지정", description = "우수 스터디원으로 지정합니다.")
@PostMapping
public ResponseEntity<Void> designateOutstandingStudent(
@RequestParam(name = "studyId") Long studyId, @Valid @RequestBody OutstandingStudentRequest request) {
mentorStudyAchievementService.designateOutstandingStudent(studyId, request);
return ResponseEntity.status(HttpStatus.CREATED).build();
}


@Operation(summary = "우수 스터디원 철회", description = "우수 스터디원 지정을 철회합니다.")
@DeleteMapping
public ResponseEntity<Void> withdrawOutstandingStudent(
@RequestParam(name = "studyId") Long studyId, @Valid @RequestBody OutstandingStudentRequest request) {
mentorStudyAchievementService.withdrawOutstandingStudent(studyId, request);
return ResponseEntity.ok().build();
}
Comment on lines +33 to +39
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. 성공적인 삭제 후 204 No Content 응답을 반환하는 것이 더 적절할 수 있습니다.
  2. 예외 처리를 추가하여 다양한 오류 상황에 대응하는 것이 좋습니다.

다음과 같이 코드를 수정해 보세요:

 @DeleteMapping
 public ResponseEntity<Void> withdrawOutstandingStudent(
         @RequestParam(name = "studyId") Long studyId, @Valid @RequestBody OutstandingStudentRequest request) {
     mentorStudyAchievementService.withdrawOutstandingStudent(studyId, request);
-    return ResponseEntity.ok().build();
+    return ResponseEntity.noContent().build();
 }

또한, 전역 예외 처리기를 구현하여 다양한 예외 상황(예: 존재하지 않는 studyId)에 대응하는 것을 고려해 보세요.

📝 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
@Operation(summary = "우수 스터디원 철회", description = "우수 스터디원 지정을 철회합니다.")
@DeleteMapping
public ResponseEntity<Void> withdrawOutstandingStudent(
@RequestParam(name = "studyId") Long studyId, @Valid @RequestBody OutstandingStudentRequest request) {
mentorStudyAchievementService.withdrawOutstandingStudent(studyId, request);
return ResponseEntity.ok().build();
}
@Operation(summary = "우수 스터디원 철회", description = "우수 스터디원 지정을 철회합니다.")
@DeleteMapping
public ResponseEntity<Void> withdrawOutstandingStudent(
@RequestParam(name = "studyId") Long studyId, @Valid @RequestBody OutstandingStudentRequest request) {
mentorStudyAchievementService.withdrawOutstandingStudent(studyId, request);
return ResponseEntity.noContent().build();
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
package com.gdschongik.gdsc.domain.study.application;

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

import com.gdschongik.gdsc.domain.member.dao.MemberRepository;
import com.gdschongik.gdsc.domain.member.domain.Member;
import com.gdschongik.gdsc.domain.study.dao.StudyAchievementRepository;
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.StudyAchievement;
import com.gdschongik.gdsc.domain.study.domain.StudyHistoryValidator;
import com.gdschongik.gdsc.domain.study.domain.StudyValidator;
import com.gdschongik.gdsc.domain.study.dto.request.OutstandingStudentRequest;
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 MentorStudyAchievementService {

private final MemberUtil memberUtil;
private final StudyValidator studyValidator;
private final StudyHistoryValidator studyHistoryValidator;
private final StudyRepository studyRepository;
private final StudyHistoryRepository studyHistoryRepository;
private final StudyAchievementRepository studyAchievementRepository;
private final MemberRepository memberRepository;

@Transactional
public void designateOutstandingStudent(Long studyId, OutstandingStudentRequest request) {
Member currentMember = memberUtil.getCurrentMember();
Study study = studyRepository.findById(studyId).orElseThrow(() -> new CustomException(STUDY_NOT_FOUND));
Long countByStudyIdAndStudentIds =
studyHistoryRepository.countByStudyIdAndStudentIds(studyId, request.studentIds());

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

List<Member> outstandingStudents = memberRepository.findAllById(request.studentIds());
List<StudyAchievement> studyAchievements = outstandingStudents.stream()
.map(member -> StudyAchievement.create(member, study, request.achievementType()))
.toList();
studyAchievementRepository.saveAll(studyAchievements);

log.info(
"[MentorStudyAchievementService] 우수 스터디원 지정: studyId={}, studentIds={}", studyId, request.studentIds());
}

@Transactional
public void withdrawOutstandingStudent(Long studyId, OutstandingStudentRequest request) {
Member currentMember = memberUtil.getCurrentMember();
Study study = studyRepository.findById(studyId).orElseThrow(() -> new CustomException(STUDY_NOT_FOUND));
Long countByStudyIdAndStudentIds =
studyHistoryRepository.countByStudyIdAndStudentIds(studyId, request.studentIds());

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

studyAchievementRepository.deleteByStudyAndAchievementTypeAndMemberIds(
studyId, request.achievementType(), request.studentIds());

log.info(
"[MentorStudyAchievementService] 우수 스터디원 철회: studyId={}, studentIds={}", studyId, request.studentIds());
}
}
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
package com.gdschongik.gdsc.domain.study.dao;

import com.gdschongik.gdsc.domain.study.domain.AchievementType;
import com.gdschongik.gdsc.domain.study.domain.StudyAchievement;
import java.util.List;

public interface StudyAchievementCustomRepository {
List<StudyAchievement> findByStudyIdAndMemberIds(Long studyId, List<Long> memberIds);

void deleteByStudyAndAchievementTypeAndMemberIds(
Long studyId, AchievementType achievementType, List<Long> memberIds);
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import static com.gdschongik.gdsc.domain.study.domain.QStudyAchievement.*;

import com.gdschongik.gdsc.domain.study.domain.AchievementType;
import com.gdschongik.gdsc.domain.study.domain.StudyAchievement;
import com.querydsl.core.types.dsl.BooleanExpression;
import com.querydsl.jpa.impl.JPAQueryFactory;
Expand All @@ -21,6 +22,18 @@ public List<StudyAchievement> findByStudyIdAndMemberIds(Long studyId, List<Long>
.fetch();
}

@Override
public void deleteByStudyAndAchievementTypeAndMemberIds(
Long studyId, AchievementType achievementType, List<Long> memberIds) {
queryFactory
.delete(studyAchievement)
.where(
eqStudyId(studyId),
studyAchievement.achievementType.eq(achievementType),
studyAchievement.student.id.in(memberIds))
.execute();
}

private BooleanExpression eqStudyId(Long studyId) {
return studyId != null ? studyAchievement.study.id.eq(studyId) : null;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package com.gdschongik.gdsc.domain.study.dao;

import java.util.List;

public interface StudyHistoryCustomRepository {

Long countByStudyIdAndStudentIds(Long studyId, 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.

굳이 nullable한 Long을 쓸 필요는 없을 것 같습니다

Copy link
Member Author

Choose a reason for hiding this comment

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

이거 아마 카운트 쓰면 long말고 Long 반환해서 이렇게 했던 것 같은데 내일 다시 확인해볼게요

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package com.gdschongik.gdsc.domain.study.dao;

import static com.gdschongik.gdsc.domain.study.domain.QStudyHistory.*;

import com.querydsl.core.types.dsl.BooleanExpression;
import com.querydsl.jpa.impl.JPAQueryFactory;
import java.util.List;
import lombok.RequiredArgsConstructor;

@RequiredArgsConstructor
public class StudyHistoryCustomRepositoryImpl implements StudyHistoryCustomRepository {

private final JPAQueryFactory queryFactory;

@Override
public Long countByStudyIdAndStudentIds(Long studyId, List<Long> studentIds) {
return queryFactory
.select(studyHistory.count())
.from(studyHistory)
.where(eqStudyId(studyId), studyHistory.student.id.in(studentIds))
.fetchOne();
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

메서드 구현 개선 제안

countByStudyIdAndStudentIds 메서드의 구현이 전반적으로 잘 되어 있습니다. 다음과 같은 개선 사항을 고려해 보시기 바랍니다:

  1. 메서드 수준의 JavaDoc을 추가하여 메서드의 목적, 매개변수, 반환값을 명확히 설명하세요.
  2. studyIdstudentIds에 대한 null 체크를 추가하여 견고성을 높이세요.
  3. 대량의 studentIds를 처리할 때의 성능을 고려하세요. 필요하다면 배치 처리를 구현할 수 있습니다.

다음과 같은 개선된 구현을 고려해보세요:

/**
 * 주어진 스터디 ID와 학생 ID 목록에 해당하는 스터디 히스토리 레코드 수를 반환합니다.
 *
 * @param studyId 조회할 스터디의 ID
 * @param studentIds 조회할 학생들의 ID 목록
 * @return 매칭되는 스터디 히스토리 레코드의 수
 * @throws IllegalArgumentException studyId가 null이거나 studentIds가 null 또는 비어있는 경우
 */
@Override
public Long countByStudyIdAndStudentIds(Long studyId, List<Long> studentIds) {
    if (studyId == null || studentIds == null || studentIds.isEmpty()) {
        throw new IllegalArgumentException("studyId와 studentIds는 null이 아니어야 하며, studentIds는 비어있지 않아야 합니다.");
    }

    // 대량의 studentIds를 처리하기 위한 배치 처리 (예: 1000개씩)
    final int batchSize = 1000;
    long totalCount = 0;

    for (int i = 0; i < studentIds.size(); i += batchSize) {
        List<Long> batch = studentIds.subList(i, Math.min(studentIds.size(), i + batchSize));
        Long batchCount = queryFactory
                .select(studyHistory.count())
                .from(studyHistory)
                .where(eqStudyId(studyId), studyHistory.student.id.in(batch))
                .fetchOne();
        totalCount += batchCount != null ? batchCount : 0;
    }

    return totalCount;
}

이 구현은 null 체크를 추가하고, 대량의 studentIds를 배치로 처리하여 성능을 개선합니다.


private BooleanExpression eqStudyId(Long studyId) {
return studyHistory.study.id.eq(studyId);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import org.springframework.data.domain.Pageable;
import org.springframework.data.jpa.repository.JpaRepository;

public interface StudyHistoryRepository extends JpaRepository<StudyHistory, Long> {
public interface StudyHistoryRepository extends JpaRepository<StudyHistory, Long>, StudyHistoryCustomRepository {

List<StudyHistory> findAllByStudent(Member member);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,4 +51,10 @@ public void validateUpdateRepository(
throw new CustomException(STUDY_HISTORY_REPOSITORY_NOT_UPDATABLE_OWNER_MISMATCH);
}
}

public void validateAppliedToStudy(Long countStudyHistory, int studentCount) {
if (countStudyHistory != studentCount) {
throw new CustomException(STUDY_HISTORY_NOT_APPLIED_STUDENT_EXISTS);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package com.gdschongik.gdsc.domain.study.dto.request;

import com.gdschongik.gdsc.domain.study.domain.AchievementType;
import java.util.List;

public record OutstandingStudentRequest(List<Long> studentIds, AchievementType achievementType) {}
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ public enum ErrorCode {
STUDY_HISTORY_REPOSITORY_NOT_UPDATABLE_ASSIGNMENT_ALREADY_SUBMITTED(
HttpStatus.CONFLICT, "이미 제출한 과제가 있으므로 레포지토리를 수정할 수 없습니다."),
STUDY_HISTORY_REPOSITORY_NOT_UPDATABLE_OWNER_MISMATCH(HttpStatus.CONFLICT, "레포지토리 소유자가 현재 멤버와 다릅니다."),
STUDY_HISTORY_NOT_APPLIED_STUDENT_EXISTS(HttpStatus.CONFLICT, "해당 스터디에 신청하지 않은 멤버가 있습니다."),

// StudyAnnouncement
STUDY_ANNOUNCEMENT_NOT_FOUND(HttpStatus.NOT_FOUND, "존재하지 않는 스터디 공지입니다."),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
package com.gdschongik.gdsc.domain.study.application;

import static com.gdschongik.gdsc.domain.study.domain.AchievementType.*;
import static org.assertj.core.api.Assertions.*;

import com.gdschongik.gdsc.domain.member.domain.Member;
import com.gdschongik.gdsc.domain.recruitment.domain.vo.Period;
import com.gdschongik.gdsc.domain.study.domain.Study;
import com.gdschongik.gdsc.domain.study.domain.StudyAchievement;
import com.gdschongik.gdsc.domain.study.dto.request.OutstandingStudentRequest;
import com.gdschongik.gdsc.helper.IntegrationTest;
import java.time.LocalDateTime;
import java.util.List;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;

public class MentorStudyAchievementServiceTest extends IntegrationTest {

@Autowired
private MentorStudyAchievementService mentorStudyAchievementService;

@Nested
class 우수_스터디원_지정시 {

@Test
void 성공한다() {
// given
LocalDateTime now = LocalDateTime.now();
Member mentor = createMentor();
Study study = createStudy(
mentor,
Period.createPeriod(now.plusDays(5), now.plusDays(10)),
Period.createPeriod(now.minusDays(5), now));

Member student = createRegularMember();
createStudyHistory(student, study);

logoutAndReloginAs(mentor.getId(), mentor.getRole());
OutstandingStudentRequest request =
new OutstandingStudentRequest(List.of(student.getId()), FIRST_ROUND_OUTSTANDING_STUDENT);

// when
mentorStudyAchievementService.designateOutstandingStudent(study.getId(), request);

// then
List<StudyAchievement> studyAchievements =
studyAchievementRepository.findByStudyIdAndMemberIds(study.getId(), request.studentIds());
assertThat(studyAchievements).hasSize(request.studentIds().size());
}
}

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

@Test
void 성공한다() {
// given
Member student = createRegularMember();
LocalDateTime now = LocalDateTime.now();
Member mentor = createMentor();
Study study = createStudy(
mentor,
Period.createPeriod(now.plusDays(5), now.plusDays(10)),
Period.createPeriod(now.minusDays(5), now));
createStudyHistory(student, study);
createStudyAchievement(student, study, FIRST_ROUND_OUTSTANDING_STUDENT);

logoutAndReloginAs(mentor.getId(), mentor.getRole());
OutstandingStudentRequest request =
new OutstandingStudentRequest(List.of(student.getId()), FIRST_ROUND_OUTSTANDING_STUDENT);

// when
mentorStudyAchievementService.withdrawOutstandingStudent(study.getId(), request);

// then
List<StudyAchievement> studyAchievements =
studyAchievementRepository.findByStudyIdAndMemberIds(study.getId(), request.studentIds());
assertThat(studyAchievements).isEmpty();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -124,4 +124,21 @@ class 레포지토리_입력시 {
.hasMessage(STUDY_HISTORY_REPOSITORY_NOT_UPDATABLE_OWNER_MISMATCH.getMessage());
}
}

@Nested
class 스터디_수강신청_여부_확인시 {

@Test
void 해당_스터디를_신청하지_않은_멤버가_있다면_실패한다() {
// given
Long countStudyHistory = 1L;
int requestStudentCount = 2;

// when & then
assertThatThrownBy(
() -> studyHistoryValidator.validateAppliedToStudy(countStudyHistory, requestStudentCount))
.isInstanceOf(CustomException.class)
.hasMessage(STUDY_HISTORY_NOT_APPLIED_STUDENT_EXISTS.getMessage());
}
}
}
21 changes: 21 additions & 0 deletions src/test/java/com/gdschongik/gdsc/helper/IntegrationTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,15 @@
import com.gdschongik.gdsc.domain.recruitment.domain.RecruitmentRound;
import com.gdschongik.gdsc.domain.recruitment.domain.RoundType;
import com.gdschongik.gdsc.domain.recruitment.domain.vo.Period;
import com.gdschongik.gdsc.domain.study.dao.StudyAchievementRepository;
import com.gdschongik.gdsc.domain.study.dao.StudyDetailRepository;
import com.gdschongik.gdsc.domain.study.dao.StudyHistoryRepository;
import com.gdschongik.gdsc.domain.study.dao.StudyRepository;
import com.gdschongik.gdsc.domain.study.domain.AchievementType;
import com.gdschongik.gdsc.domain.study.domain.Study;
import com.gdschongik.gdsc.domain.study.domain.StudyAchievement;
import com.gdschongik.gdsc.domain.study.domain.StudyDetail;
import com.gdschongik.gdsc.domain.study.domain.StudyHistory;
import com.gdschongik.gdsc.global.security.PrincipalDetails;
import com.gdschongik.gdsc.infra.feign.payment.client.PaymentClient;
import com.gdschongik.gdsc.infra.github.client.GithubClient;
Expand Down Expand Up @@ -81,6 +86,12 @@ public abstract class IntegrationTest {
@Autowired
protected StudyDetailRepository studyDetailRepository;

@Autowired
protected StudyHistoryRepository studyHistoryRepository;

@Autowired
protected StudyAchievementRepository studyAchievementRepository;

@MockBean
protected OnboardingRecruitmentService onboardingRecruitmentService;

Expand Down Expand Up @@ -262,6 +273,16 @@ protected StudyDetail createNewStudyDetail(Long week, Study study, LocalDateTime
return studyDetailRepository.save(studyDetail);
}

protected StudyHistory createStudyHistory(Member member, Study study) {
StudyHistory studyHistory = StudyHistory.create(member, study);
return studyHistoryRepository.save(studyHistory);
}

protected StudyAchievement createStudyAchievement(Member member, Study study, AchievementType achievementType) {
StudyAchievement studyAchievement = StudyAchievement.create(member, study, achievementType);
return studyAchievementRepository.save(studyAchievement);
}

protected StudyDetail publishAssignment(StudyDetail studyDetail) {
studyDetail.publishAssignment(ASSIGNMENT_TITLE, studyDetail.getPeriod().getEndDate(), DESCRIPTION_LINK);
return studyDetailRepository.save(studyDetail);
Expand Down