[DDING-107] 최종 합격 지원자 동아리원 명단 등록 API 구현 #247
Conversation
5uhwann
requested review from
wonjunYou,
KoSeonJe and
Seooooo24
as code owners
Walkthrough이번 PR은 클럽 회원 관리 및 폼 신청 관련 기능 강화를 위해 여러 엔티티와 API에서 수정 및 신규 메소드를 추가합니다.
Changes
Sequence Diagram(s)sequenceDiagram
participant 사용자 as User
participant Controller as CentralFormController
participant Service as FacadeCentralFormServiceImpl
participant Repository as FormApplicationRepository
participant Club as Club 엔티티
사용자->>Controller: registerMembers(formId) 호출
Controller->>Service: registerApplicantAsMember(formId) 호출
Service->>Repository: findAllFinalPassedByFormId(formId) 호출
Repository-->>Service: 최종 통과 신청 목록 반환
loop 각 신청 처리
Service->>Club: addClubMember(ClubMember) 호출
Club-->>Service: 회원 추가 완료
end
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormService.java (1)
21-22: 메서드 이름을 복수형으로 변경하는 것이 좋겠습니다.현재 메서드는 여러 지원자를 등록하는 기능을 수행하므로, 메서드 이름을
registerApplicantsAsMembers로 변경하는 것이 더 명확할 것 같습니다.src/main/java/ddingdong/ddingdongBE/domain/formapplication/repository/FormApplicationRepository.java (1)
25-26: 쿼리에서 상태값을 문자열 대신 enum 참조를 사용하는 것이 좋겠습니다.현재 'FINAL_PASS' 문자열을 직접 사용하고 있는데, 이는 오타나 상태값 변경 시 문제가 될 수 있습니다. 대신
FormApplicationStatus.FINAL_PASS를 사용하는 것이 더 안전합니다.- @Query(value = "select fa from FormApplication fa where fa.form.id = :formId and fa.status = 'FINAL_PASS'") + @Query(value = "select fa from FormApplication fa where fa.form.id = :formId and fa.status = :#{T(ddingdong.ddingdongBE.domain.formapplication.entity.FormApplicationStatus).FINAL_PASS}")src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java (1)
90-103: 최종 합격자 등록 로직이 적절하게 구현되었습니다.다음 사항들이 잘 구현되었습니다:
- 최종 합격한 지원서만 필터링
- ClubMember 생성 시 필요한 정보를 모두 포함
- Position을 MEMBER로 적절하게 설정
- Club과 ClubMember 간의 관계 설정
하지만 다음 사항들을 고려해보시기 바랍니다:
- 트랜잭션 실패 시 롤백 처리 검토
- 대량의 데이터 처리 시 성능 최적화 고려
@Override @Transactional public void registerApplicantAsMember(Long formId) { + // 트랜잭션 실패 시 롤백을 위해 try-catch 추가 고려 List<FormApplication> finalPassedFormApplications = formApplicationService.getAllFinalPassedByFormId(formId); + // 대량 데이터 처리 시 배치 처리 고려 finalPassedFormApplications.forEach(formApplication -> { Club club = formApplication.getForm().getClub(); ClubMember clubMember = ClubMember.builder() .name(formApplication.getName()) .studentNumber(formApplication.getStudentNumber()) .department(formApplication.getDepartment()) .phoneNumber(formApplication.getPhoneNumber()) .position(MEMBER) .build(); club.addClubMember(clubMember); }); }src/test/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImplTest.java (1)
274-341: 서비스 계층 테스트가 상세하게 구현되었습니다.다음 사항들이 잘 구현되었습니다:
- 테스트 데이터 설정이 명확하고 포괄적
- FINAL_PASS 상태의 지원자만 회원으로 등록되는지 검증
- 등록된 회원의 상세 정보까지 검증
하나의 개선사항을 제안드립니다:
실패 케이스도 테스트에 포함하면 좋을 것 같습니다. 예를 들어:
- 존재하지 않는 formId
- 빈 지원서 목록
- 최종 합격자가 없는 경우
@Test void registerApplicantAsMember_WithNoFinalPass() { // given Form form = ... // 최종 합격자가 없는 폼 설정 // when facadeCentralFormService.registerApplicantAsMember(form.getId()); // then Club club = clubRepository.findById(form.getClub().getId()).orElseThrow(); assertThat(club.getClubMembers()).isEmpty(); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/main/java/ddingdong/ddingdongBE/domain/club/entity/Club.java(3 hunks)src/main/java/ddingdong/ddingdongBE/domain/clubmember/entity/ClubMember.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/api/CentralFormApi.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/controller/CentralFormController.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java(4 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/entity/FormApplication.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/repository/FormApplicationRepository.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FormApplicationService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/GeneralFormApplicationService.java(1 hunks)src/main/resources/db/migration/V37__alter_table_form_application_add_column.sql(1 hunks)src/test/java/ddingdong/ddingdongBE/domain/form/repository/FormApplicationRepositoryTest.java(3 hunks)src/test/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImplTest.java(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (6)
src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FormApplicationService.java (1)
17-18: LGTM! 메서드 추가가 적절합니다.최종 합격한 지원자 목록을 조회하는 메서드가 잘 정의되어 있습니다.
src/main/java/ddingdong/ddingdongBE/domain/club/entity/Club.java (2)
44-44: cascade 옵션 추가가 적절합니다.Club과 ClubMember 간의 부모-자식 관계에서 영속성 전이를 위해 cascade = CascadeType.ALL 옵션을 추가한 것이 적절합니다.
127-130: 양방향 관계 설정이 올바르게 구현되었습니다.addClubMember 메서드에서 ClubMember를 추가할 때 양방향 관계를 올바르게 설정하고 있습니다.
src/test/java/ddingdong/ddingdongBE/domain/form/repository/FormApplicationRepositoryTest.java (1)
153-212: 테스트 케이스가 잘 구현되었습니다.다음 사항들이 잘 구현되었습니다:
- 여러 상태의 지원서를 포함한 테스트 데이터 준비
- FINAL_PASS 상태의 지원서만 정확하게 필터링되는지 검증
- 적절한 assertion 사용
src/main/resources/db/migration/V37__alter_table_form_application_add_column.sql (2)
1-2:⚠️ Potential issueDB 칼럼 정의 불일치 주의
FormApplication 엔티티에서는 email 필드가 필수(non-nullable)로 선언된 것으로 보입니다. 그런데 이 마이그레이션에서는 email 칼럼을 NULL 허용(VARCHAR(255) NULL)으로 추가하고 있습니다. 이로 인해 애플리케이션과 DB 스키마 간 제약 조건이 불일치할 수 있으니, 의도된 변경사항인지 재검토해 주세요.
4-5:⚠️ Potential issueDB 칼럼 정의 일관성 검토 요청
phone_number 칼럼 역시 현재 NULL 허용으로 정의되어 있습니다. 만약 FormApplication 엔티티에서 phoneNumber 필드가 non-nullable로 설정되어 있다면, DB 스키마와의 제약 조건 불일치로 인해 문제가 발생할 수 있습니다. 해당 칼럼의 제약 조건이 의도한 바와 일치하는지 확인 부탁드립니다.
| private FormApplication(String name, String studentNumber, String department, String phoneNumber, String email, | ||
| FormApplicationStatus status, Form form) { | ||
| this.name = name; | ||
| this.studentNumber = studentNumber; | ||
| this.department = department; | ||
| this.phoneNumber = phoneNumber; | ||
| this.email = email; | ||
| this.status = status; | ||
| this.form = form; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
생성자에 유효성 검증 로직 추가가 필요합니다.
생성자에서 전화번호와 이메일의 유효성을 검증하는 로직을 추가하면 좋겠습니다.
@Builder
private FormApplication(String name, String studentNumber, String department, String phoneNumber, String email,
FormApplicationStatus status, Form form) {
+ validatePhoneNumber(phoneNumber);
+ validateEmail(email);
this.name = name;
this.studentNumber = studentNumber;
this.department = department;
this.phoneNumber = phoneNumber;
this.email = email;
this.status = status;
this.form = form;
}
+ private void validatePhoneNumber(String phoneNumber) {
+ if (!phoneNumber.matches("^\\d{3}-\\d{3,4}-\\d{4}$")) {
+ throw new IllegalArgumentException("올바른 전화번호 형식이 아닙니다.");
+ }
+ }
+
+ private void validateEmail(String email) {
+ if (!email.matches("^[A-Za-z0-9+_.-]+@(.+)$")) {
+ throw new IllegalArgumentException("올바른 이메일 형식이 아닙니다.");
+ }
+ }📝 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.
| private FormApplication(String name, String studentNumber, String department, String phoneNumber, String email, | |
| FormApplicationStatus status, Form form) { | |
| this.name = name; | |
| this.studentNumber = studentNumber; | |
| this.department = department; | |
| this.phoneNumber = phoneNumber; | |
| this.email = email; | |
| this.status = status; | |
| this.form = form; | |
| } | |
| @Builder | |
| private FormApplication(String name, String studentNumber, String department, String phoneNumber, String email, | |
| FormApplicationStatus status, Form form) { | |
| validatePhoneNumber(phoneNumber); | |
| validateEmail(email); | |
| this.name = name; | |
| this.studentNumber = studentNumber; | |
| this.department = department; | |
| this.phoneNumber = phoneNumber; | |
| this.email = email; | |
| this.status = status; | |
| this.form = form; | |
| } | |
| private void validatePhoneNumber(String phoneNumber) { | |
| if (!phoneNumber.matches("^\\d{3}-\\d{3,4}-\\d{4}$")) { | |
| throw new IllegalArgumentException("올바른 전화번호 형식이 아닙니다."); | |
| } | |
| } | |
| private void validateEmail(String email) { | |
| if (!email.matches("^[A-Za-z0-9+_.-]+@(.+)$")) { | |
| throw new IllegalArgumentException("올바른 이메일 형식이 아닙니다."); | |
| } | |
| } |
| @Column(nullable = false) | ||
| private String phoneNumber; | ||
|
|
||
| @Column(nullable = false) | ||
| private String email; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
전화번호와 이메일 필드에 유효성 검증이 필요합니다.
전화번호와 이메일 형식에 대한 유효성 검증이 없습니다. 잘못된 형식의 데이터가 저장될 수 있으므로 다음과 같은 검증을 추가하는 것이 좋겠습니다:
+ @Pattern(regexp = "^\\d{3}-\\d{3,4}-\\d{4}$", message = "올바른 전화번호 형식이 아닙니다.")
@Column(nullable = false)
private String phoneNumber;
+ @Email(message = "올바른 이메일 형식이 아닙니다.")
@Column(nullable = false)
private String email;📝 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.
| @Column(nullable = false) | |
| private String phoneNumber; | |
| @Column(nullable = false) | |
| private String email; | |
| @Pattern(regexp = "^\\d{3}-\\d{3,4}-\\d{4}$", message = "올바른 전화번호 형식이 아닙니다.") | |
| @Column(nullable = false) | |
| private String phoneNumber; | |
| @Email(message = "올바른 이메일 형식이 아닙니다.") | |
| @Column(nullable = false) | |
| private String email; |
| public void setClubFormConvenience(Club club) { | ||
| this.club = club; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
메서드 이름과 유효성 검사 개선 필요
메서드 이름이 목적을 명확하게 표현하지 않으며, club 매개변수에 대한 null 검사가 누락되었습니다.
다음과 같이 개선하는 것을 제안합니다:
- public void setClubFormConvenience(Club club) {
+ public void setClub(Club club) {
+ if (club == null) {
+ throw new IllegalArgumentException("Club cannot be null");
+ }
this.club = club;
}📝 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.
| public void setClubFormConvenience(Club club) { | |
| this.club = club; | |
| } | |
| public void setClub(Club club) { | |
| if (club == null) { | |
| throw new IllegalArgumentException("Club cannot be null"); | |
| } | |
| this.club = club; | |
| } |
| @Override | ||
| public List<FormApplication> getAllFinalPassedByFormId(Long formId) { | ||
| return formApplicationRepository.findAllFinalPassedByFormId(formId); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
메서드 문서화 및 유효성 검사 추가 필요
메서드에 대한 JavaDoc 문서화가 누락되었으며, formId에 대한 유효성 검사가 필요합니다.
다음과 같이 개선하는 것을 제안합니다:
+ /**
+ * 주어진 폼 ID에 해당하는 최종 합격된 지원서 목록을 조회합니다.
+ *
+ * @param formId 조회할 폼의 ID
+ * @return 최종 합격된 지원서 목록
+ * @throws IllegalArgumentException formId가 null이거나 0 이하인 경우
+ */
@Override
public List<FormApplication> getAllFinalPassedByFormId(Long formId) {
+ if (formId == null || formId <= 0) {
+ throw new IllegalArgumentException("Invalid formId: " + formId);
+ }
return formApplicationRepository.findAllFinalPassedByFormId(formId);
}📝 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.
| @Override | |
| public List<FormApplication> getAllFinalPassedByFormId(Long formId) { | |
| return formApplicationRepository.findAllFinalPassedByFormId(formId); | |
| } | |
| /** | |
| * 주어진 폼 ID에 해당하는 최종 합격된 지원서 목록을 조회합니다. | |
| * | |
| * @param formId 조회할 폼의 ID | |
| * @return 최종 합격된 지원서 목록 | |
| * @throws IllegalArgumentException formId가 null이거나 0 이하인 경우 | |
| */ | |
| @Override | |
| public List<FormApplication> getAllFinalPassedByFormId(Long formId) { | |
| if (formId == null || formId <= 0) { | |
| throw new IllegalArgumentException("Invalid formId: " + formId); | |
| } | |
| return formApplicationRepository.findAllFinalPassedByFormId(formId); | |
| } |
| @Override | ||
| public void registerMember(Long formId) { | ||
| facadeCentralFormService.registerApplicantAsMember(formId); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
에러 처리 및 로깅 추가 필요
중요한 작업에 대한 로깅이 누락되었으며, 예외 처리가 미흡합니다.
다음과 같이 개선하는 것을 제안합니다:
+ private static final Logger log = LoggerFactory.getLogger(CentralFormController.class);
+
@Override
public void registerMember(Long formId) {
+ log.info("Starting member registration process for form ID: {}", formId);
+ try {
facadeCentralFormService.registerApplicantAsMember(formId);
+ log.info("Successfully registered members for form ID: {}", formId);
+ } catch (Exception e) {
+ log.error("Failed to register members for form ID: {}", formId, e);
+ throw e;
+ }
}| @Operation(summary = "동아리 최종 합격 지원자 동아리원 명단 등록API") | ||
| @ApiResponse(responseCode = "200", description = "최종 합격 지원자 동아리원 명단 등록 성공") | ||
| @ResponseStatus(HttpStatus.CREATED) | ||
| @SecurityRequirement(name = "AccessToken") | ||
| @GetMapping("/my/forms/{formId}/members/register-applicants") | ||
| void registerMember(@PathVariable("formId") Long formId); |
There was a problem hiding this comment.
HTTP 메서드 및 응답 상태 코드 수정 필요
현재 구현에는 다음과 같은 문제가 있습니다:
- GET 메서드를 사용하여 리소스를 생성하는 것은 REST 원칙에 위배됩니다.
- 응답 상태 코드가 일관성이 없습니다 (설명은 200이지만 @ResponseStatus는 201).
다음과 같이 수정하는 것을 제안합니다:
@Operation(summary = "동아리 최종 합격 지원자 동아리원 명단 등록API")
- @ApiResponse(responseCode = "200", description = "최종 합격 지원자 동아리원 명단 등록 성공")
+ @ApiResponse(responseCode = "201", description = "최종 합격 지원자 동아리원 명단 등록 성공")
@ResponseStatus(HttpStatus.CREATED)
@SecurityRequirement(name = "AccessToken")
- @GetMapping("/my/forms/{formId}/members/register-applicants")
+ @PostMapping("/my/forms/{formId}/members/register-applicants")
void registerMember(@PathVariable("formId") Long formId);📝 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.
| @Operation(summary = "동아리 최종 합격 지원자 동아리원 명단 등록API") | |
| @ApiResponse(responseCode = "200", description = "최종 합격 지원자 동아리원 명단 등록 성공") | |
| @ResponseStatus(HttpStatus.CREATED) | |
| @SecurityRequirement(name = "AccessToken") | |
| @GetMapping("/my/forms/{formId}/members/register-applicants") | |
| void registerMember(@PathVariable("formId") Long formId); | |
| @Operation(summary = "동아리 최종 합격 지원자 동아리원 명단 등록API") | |
| @ApiResponse(responseCode = "201", description = "최종 합격 지원자 동아리원 명단 등록 성공") | |
| @ResponseStatus(HttpStatus.CREATED) | |
| @SecurityRequirement(name = "AccessToken") | |
| @PostMapping("/my/forms/{formId}/members/register-applicants") | |
| void registerMember(@PathVariable("formId") Long formId); |
src/main/java/ddingdong/ddingdongBE/domain/form/api/CentralFormApi.java
Outdated
Show resolved
Hide resolved
src/main/java/ddingdong/ddingdongBE/domain/form/api/CentralFormApi.java
Outdated
Show resolved
Hide resolved
src/main/java/ddingdong/ddingdongBE/domain/form/api/CentralFormApi.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| @Transactional | ||
| public void registerApplicantAsMember(Long formId) { | ||
| List<FormApplication> finalPassedFormApplications = formApplicationService.getAllFinalPassedByFormId(formId); |
There was a problem hiding this comment.
p2)
현재 hasInterView라는 필드를 통해 서류만 볼 수 있도록 해놨고,
서류 통과는 FINAL_PASS가 아닌 FIRST_PASS인데, 이 부분도 고려해야할 것 같습니다.
제 의견은 상태 수정 API에서 hasInterview가 false일 경우 서류 통과시 First pass가 아닌 final pass로 변경되는게 좋아보입니다
There was a problem hiding this comment.
hasInterview가 false일 경우 합격 처리가 되면 FINAL_PASS로 변경이 아닌 FIRST_PASS라는 뜻인가요???
최종 합격 상태
- hasInterview = true인 경우: FINAL_PASS가 최종 합격
- hasInterview = false인 경우: FIRST_PASS가 최종 합격
이게 맞을까요??
There was a problem hiding this comment.
네 최종합격을 판단하는 부분에서 위처럼 해도 좋을 것 같습니다.
# Conflicts: # src/main/java/ddingdong/ddingdongBE/domain/form/api/CentralFormApi.java # src/main/java/ddingdong/ddingdongBE/domain/form/controller/CentralFormController.java # src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormService.java # src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java # src/main/java/ddingdong/ddingdongBE/domain/formapplication/entity/FormApplication.java # src/main/java/ddingdong/ddingdongBE/domain/formapplication/repository/FormApplicationRepository.java # src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FormApplicationService.java # src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/GeneralFormApplicationService.java # src/test/java/ddingdong/ddingdongBE/domain/form/repository/FormApplicationRepositoryTest.java # src/test/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImplTest.java
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/ddingdong/ddingdongBE/domain/form/api/CentralFormApi.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/controller/CentralFormController.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java(4 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/repository/FormApplicationRepository.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/GeneralFormApplicationService.java(2 hunks)src/main/resources/db/migration/V38__alter_table_form_application_add_column.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/ddingdong/ddingdongBE/domain/form/controller/CentralFormController.java
- src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormService.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (5)
src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/GeneralFormApplicationService.java (1)
42-45: 메서드 문서화 및 유효성 검사 추가 필요메서드에 대한 JavaDoc 문서화가 누락되었으며, formId에 대한 유효성 검사가 필요합니다.
src/main/java/ddingdong/ddingdongBE/domain/formapplication/repository/FormApplicationRepository.java (1)
66-71: LGTM! 최종 합격자 조회 로직이 적절히 구현되었습니다.최종 합격 상태를 다음과 같이 올바르게 처리하고 있습니다:
- hasInterview = true인 경우: FINAL_PASS가 최종 합격
- hasInterview = false인 경우: FIRST_PASS가 최종 합격
src/main/java/ddingdong/ddingdongBE/domain/form/api/CentralFormApi.java (1)
93-98: LGTM! API 엔드포인트가 REST 원칙을 잘 준수하고 있습니다.HTTP 메소드와 응답 상태 코드가 적절하게 구현되었습니다:
- POST 메소드 사용으로 리소스 생성 의도가 명확함
- 응답 코드 201이 문서화 및 구현과 일치함
src/main/resources/db/migration/V38__alter_table_form_application_add_column.sql (2)
1-2: 검증: email 컬럼의 NULL 허용 여부 확인 필요
현재 마이그레이션 스크립트에서는VARCHAR(255) NULL로 추가되고 있습니다. 그러나 도메인 모델(FormApplication 클래스)에서는 이 필드가 non-nullable로 정의되어 있다면, DB 스키마와 애플리케이션 모델 간의 일관성이 깨질 수 있습니다. 양측 정의를 다시 확인해주시기 바랍니다.
4-5: 검증: phone_number 컬럼의 NULL 허용 여부 재검토
phone_number컬럼 또한VARCHAR(255) NULL로 추가되고 있습니다. 만약 애플리케이션 측에서 이 필드를 필수(non-nullable)로 기대하고 있다면, 스키마와 일관성을 맞추기 위해 검토가 필요합니다.
| @Override | ||
| @Transactional | ||
| public void registerApplicantAsMember(Long formId) { | ||
| List<FormApplication> finalPassedFormApplications = formApplicationService.getAllFinalPassedByFormId(formId); | ||
| finalPassedFormApplications.forEach(formApplication -> { | ||
| Club club = formApplication.getForm().getClub(); | ||
| ClubMember clubMember = ClubMember.builder() | ||
| .name(formApplication.getName()) | ||
| .studentNumber(formApplication.getStudentNumber()) | ||
| .department(formApplication.getDepartment()) | ||
| .phoneNumber(formApplication.getPhoneNumber()) | ||
| .position(MEMBER) | ||
| .build(); | ||
| club.addClubMember(clubMember); | ||
| }); | ||
| } |
There was a problem hiding this comment.
보안 및 유효성 검사 추가 필요
다음 사항들을 고려해야 합니다:
- 현재 사용자가 해당 폼의 클럽에 대한 권한이 있는지 확인이 필요합니다
- 이미 등록된 동아리원인지 중복 검사가 필요합니다
다음과 같이 수정하는 것을 제안합니다:
@Override
@Transactional
public void registerApplicantAsMember(Long formId) {
+ Form form = formService.getById(formId);
+ Club club = form.getClub();
+ User currentUser = SecurityContextHolder.getContext().getAuthentication().getPrincipal();
+ validateEqualsClub(club, form);
List<FormApplication> finalPassedFormApplications = formApplicationService.getAllFinalPassedByFormId(formId);
finalPassedFormApplications.forEach(formApplication -> {
- Club club = formApplication.getForm().getClub();
+ if (isDuplicateClubMember(club, formApplication.getStudentNumber())) {
+ throw new DuplicateClubMemberException(formApplication.getStudentNumber());
+ }
ClubMember clubMember = ClubMember.builder()
.name(formApplication.getName())
.studentNumber(formApplication.getStudentNumber())
.department(formApplication.getDepartment())
.phoneNumber(formApplication.getPhoneNumber())
.position(MEMBER)
.build();
club.addClubMember(clubMember);
});
}
+ private boolean isDuplicateClubMember(Club club, String studentNumber) {
+ return club.getClubMembers().stream()
+ .anyMatch(member -> member.getStudentNumber().equals(studentNumber));
+ }Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/java/ddingdong/ddingdongBE/domain/club/service/FacadeAdminClubServiceImplTest.java (1)
26-88: 최종 합격 지원자 등록 기능에 대한 테스트 케이스가 필요합니다.PR의 주요 목적인 "최종 합격 지원자를 동아리원으로 등록하는 API" 기능에 대한 테스트가 누락되어 있습니다. 다음과 같은 테스트 케이스 추가를 제안드립니다:
- FINAL_PASS 상태인 지원자들이 성공적으로 동아리원으로 등록되는지 검증
- 전화번호와 이메일 필드가 올바르게 저장되는지 검증
- 잘못된 상태의 지원자 등록 시도 시 예외 처리 검증
테스트 케이스 구현에 도움이 필요하시다면 제가 예시 코드를 생성해드릴 수 있습니다.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/java/ddingdong/ddingdongBE/domain/club/service/FacadeAdminClubServiceImplTest.java(2 hunks)
🔇 Additional comments (1)
src/test/java/ddingdong/ddingdongBE/domain/club/service/FacadeAdminClubServiceImplTest.java (1)
73-80: 테스트 데이터 설정이 적절합니다.
clubMembers를 빈 ArrayList로 초기화하여 NPE를 방지하고 Club 엔티티의 변경사항을 잘 반영하였습니다. 테스트 데이터 설정이 견고해졌습니다.
|
🚀 작업 내용
FINAL_PASS상태인 지원자를 ClubMember로 등록🤔 고민했던 내용
💬 리뷰 중점사항
Summary by CodeRabbit
새로운 기능
버그 수정