[DDING-93] 지원자 상태 수정 API 구현 #241
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
이전에 FormResponse에서 FormApplication으로 변경했을 때 미처 변경하지 못한 부분 변경했습니다.
Seooooo24
requested review from
wonjunYou,
5uhwann and
KoSeonJe
as code owners
Walkthrough이번 PR은 폼 어플리케이션 도메인 관련 변경사항을 포함합니다. 구체적으로, Changes
Sequence Diagram(s)sequenceDiagram
participant U as 사용자
participant C as CentralFormApplicationController
participant F as FacadeCentralFormApplicationService
participant R as Repository
U->>C: getFormApplication(formId, applicationId, principal)
C->>F: getFormApplication(formId, applicationId, user)
F->>R: 폼 어플리케이션 데이터 조회
R-->>F: FormApplicationQuery 반환
F-->>C: FormApplicationQuery 반환
C-->>U: FormApplicationResponse 전달
sequenceDiagram
participant U as 사용자
participant C as CentralFormApplicationController
participant F as FacadeCentralFormApplicationService
participant R as Repository
U->>C: updateFormApplicationStatus(formId, principal, request)
C->>F: updateStatus(command 생성 후 전달)
F->>R: 해당 FormApplication 조회 및 상태 업데이트
R-->>F: 업데이트된 데이터 저장 완료
F-->>C: 처리 완료 (204 No Content)
C-->>U: 204 No Content 응답
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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: 7
🧹 Nitpick comments (8)
src/test/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormApplicationServiceImplTest.java (2)
48-83: 응답 상세 조회 테스트 로직 개선 제안
현재는 지원자의 이름만 검증하고 있습니다. 응답 내용을 좀 더 철저히 확인하기 위해 응답 필드 전체(학번, 학과 등)를 추가적으로 검증해 주시면 좋을 것 같습니다.
86-130: 상태 업데이트 테스트에 DB 재확인 절차 권장
상태 업데이트 후 메모리 상의 엔티티만 확인하고 있습니다. DB에 정상 반영되었는지 확인하기 위해, 별도의 find 로직을 통해 DB에서 다시 조회하여 상태를 검증하면 테스트 신뢰도를 높일 수 있습니다.src/main/java/ddingdong/ddingdongBE/domain/formapplication/repository/FormAnswerRepository.java (1)
4-10: JPA 쿼리 메서드 추가 검토
findAllByFormApplication()메서드는 직관적인 네이밍을 통해 목적이 명확해 보입니다. 다만, 연관관계가 복잡해질 수 있으므로 페치 전략이나 추가 최적화가 필요한지 확인을 고려해 보세요.src/main/java/ddingdong/ddingdongBE/domain/formapplication/api/CentralFormApplicationApi.java (2)
37-47: API 오류 응답 문서화 추가 필요지원자 상세 조회 API에 대한 오류 응답 케이스가 문서화되어 있지 않습니다.
다음과 같이 오류 응답을 추가하는 것을 제안합니다:
@Operation(summary = "지원자 상세 조회 API") @ApiResponse(responseCode = "200", description = "지원자 상세 조회 성공", content = @Content(schema = @Schema(implementation = FormApplicationResponse.class))) +@ApiResponse(responseCode = "404", description = "지원자를 찾을 수 없음") +@ApiResponse(responseCode = "403", description = "접근 권한 없음") @ResponseStatus(HttpStatus.OK)
49-59: API 오류 응답 문서화 및 요청 유효성 검증 추가 필요상태 수정 API에 대한 오류 응답 케이스와 요청 본문 유효성 검증이 필요합니다.
다음과 같이 오류 응답을 추가하는 것을 제안합니다:
@Operation(summary = "지원자 개인 상태 수정 API") @ApiResponse(responseCode = "204", description = "지원자 상태 수정 성공") +@ApiResponse(responseCode = "400", description = "잘못된 상태 값") +@ApiResponse(responseCode = "404", description = "지원자를 찾을 수 없음") +@ApiResponse(responseCode = "403", description = "접근 권한 없음") @ResponseStatus(HttpStatus.NO_CONTENT)src/main/java/ddingdong/ddingdongBE/domain/formapplication/controller/dto/response/FormApplicationResponse.java (1)
16-29: 응답 필드에 대한 유효성 검증 어노테이션 추가 필요응답 DTO의 필드들에 대한 기본적인 유효성 검증이 누락되어 있습니다.
다음과 같이 유효성 검증 어노테이션을 추가하는 것을 제안합니다:
@Builder public record FormApplicationResponse ( @Schema(description = "제출일시", example = "2025-01-01T00:00") + @NotNull(message = "제출일시는 필수입니다") LocalDateTime submittedAt, @Schema(description = "지원자 이름", example = "김띵동") + @NotBlank(message = "이름은 필수입니다") String name, @Schema(description = "지원자 학번", example = "60201111") + @Pattern(regexp = "^\\d{8}$", message = "학번은 8자리 숫자여야 합니다") String studentNumber, // ... 나머지 필드들에 대해서도 동일하게 적용src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/dto/query/FormApplicationQuery.java (1)
64-93: 성능 최적화를 위한 리팩토링 제안
of메서드에서 여러 번의 스트림 변환과 컬렉션 생성이 발생하고 있습니다.다음과 같이 최적화하는 것을 제안합니다:
public static FormApplicationQuery of(FormApplication formApplication, List<FormField> formFields, List<FormAnswer> formAnswers) { - List<FormFieldListQuery> formFieldListQueries = formFields.stream() - .map(FormFieldListQuery::from) - .toList(); - List<FormAnswerListQuery> formAnswerListQueries = formAnswers.stream() - .map(FormAnswerListQuery::from) - .toList(); - Map<Long, FormAnswerListQuery> answerMap = formAnswerListQueries.stream() - .collect(Collectors.toMap(FormAnswerListQuery::fieldId, Function.identity())); + Map<Long, FormAnswerListQuery> answerMap = formAnswers.stream() + .map(FormAnswerListQuery::from) + .collect(Collectors.toMap(FormAnswerListQuery::fieldId, Function.identity())); List<FormFieldAnswerListQuery> formFieldAnswerListQueries = formFields.stream() .map(field -> { + FormFieldListQuery fieldQuery = FormFieldListQuery.from(field); FormAnswerListQuery answerQuery = answerMap.get(field.getId()); if (answerQuery == null) { answerQuery = FormAnswerListQuery.builder() - .fieldId(fieldQuery.id()) + .fieldId(field.getId()) .value(null) .build(); } - return FormFieldAnswerListQuery.from(fieldQuery, answerQuery); + return FormFieldAnswerListQuery.from(fieldQuery, answerQuery); }) .collect(Collectors.toList());src/test/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImplTest.java (1)
53-82: createForm 테스트 케이스의 검증을 강화할 필요가 있습니다.테스트의 기본 구조는 좋으나, 다음과 같은 개선이 필요합니다:
- 생성된 Form의 구체적인 필드 값 검증
- 생성된 FormField의 개수와 내용 검증
@Test void createForm() { // given User user = fixtureMonkey.giveMeBuilder(User.class) .set("id", 1L) .set("Role", Role.CLUB) .set("deletedAt", null) .sample(); User savedUser = userRepository.save(user); Club club = fixtureMonkey.giveMeBuilder(Club.class) .set("id", 1L) .set("user", savedUser) .set("score", null) .set("clubMembers", null) .set("deletedAt", null) .sample(); clubRepository.save(club); CreateFormCommand createFormCommand = fixtureMonkey.giveMeBuilder(CreateFormCommand.class) .set("user", savedUser) .sample(); // when facadeCentralFormService.createForm(createFormCommand); // then List<Form> forms = formRepository.findAll(); List<FormField> formFields = formFieldRepository.findAll(); assertThat(forms).isNotEmpty(); + Form createdForm = forms.get(0); + assertThat(createdForm.getTitle()).isEqualTo(createFormCommand.title()); + assertThat(createdForm.getDescription()).isEqualTo(createFormCommand.description()); + assertThat(createdForm.getClub()).isEqualTo(club); assertThat(formFields).isNotEmpty(); + assertThat(formFields).hasSameSizeAs(createFormCommand.formFieldCommands()); + assertThat(formFields.get(0).getQuestion()) + .isEqualTo(createFormCommand.formFieldCommands().get(0).question()); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/main/java/ddingdong/ddingdongBE/domain/form/service/dto/query/FormQuery.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/api/CentralFormApplicationApi.java(3 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/api/UserFormApplicationApi.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/controller/CentralFormApplicationController.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/controller/UserFormApplicationController.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/controller/dto/request/UpdateFormApplicationStatusRequest.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/controller/dto/response/FormApplicationResponse.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/entity/FormApplication.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/repository/FormAnswerRepository.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FacadeCentralFormApplicationService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FacadeCentralFormApplicationServiceImpl.java(3 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FormAnswerService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FormApplicationService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/GeneralFormAnswerService.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/GeneralFormApplicationService.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/dto/command/UpdateFormApplicationStatusCommand.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/dto/query/FormApplicationQuery.java(1 hunks)src/test/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormApplicationServiceImplTest.java(1 hunks)src/test/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImplTest.java(1 hunks)
🔇 Additional comments (16)
src/main/java/ddingdong/ddingdongBE/domain/formapplication/controller/UserFormApplicationController.java (1)
15-18: 메서드 이름 변경이 적절하게 구현되었습니다.
createFormResponse에서createFormApplication으로의 메서드 이름 변경이 API 인터페이스와 일관성 있게 구현되었으며, 기능의 목적을 더 명확하게 표현합니다.src/main/java/ddingdong/ddingdongBE/domain/formapplication/api/UserFormApplicationApi.java (2)
15-18: API 문서화가 명확하게 작성되었습니다.Swagger 어노테이션을 통한 API 문서화가 한글로 잘 작성되어 있으며, 응답 상태 코드와 설명이 적절합니다.
19-22: 메서드 시그니처가 RESTful 규칙을 잘 따르고 있습니다.
- 엔드포인트 구조가 RESTful 규칙을 잘 따르고 있습니다
@Valid어노테이션을 통한 요청 검증이 적절히 구현되어 있습니다- 메서드 이름이 기능을 더 명확하게 표현하도록 변경되었습니다
src/test/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormApplicationServiceImplTest.java (1)
11-16: 새로운 엔티티 및 DTO import 추가 확인
해당 import들이 신규 도메인 로직(FormApplication)과 잘 연동되어 보입니다. 현재로서는 충돌 없이 정상적으로 동작할 것으로 판단됩니다.src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FormAnswerService.java (1)
4-13: 특정 지원서에 대한 답변 목록 조회 기능 추가
새로 추가된getAllByApplication()메서드는 지원서와 연관된 답변을 한 번에 가져올 수 있어 유용합니다. 파라미터와 반환타입이 명확해 보이며 설계가 적절합니다.src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FormApplicationService.java (1)
13-15: 인터페이스 메서드 추가가 적절합니다!
getById와updateStatus메서드가 단일 책임 원칙을 잘 따르고 있으며, 메서드 시그니처가 명확합니다.src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FacadeCentralFormApplicationService.java (1)
12-14: 퍼사드 패턴이 잘 적용되었습니다!사용자 권한 확인을 포함한 메서드 설계가 적절하며, 퍼사드 패턴의 의도를 잘 반영하고 있습니다.
src/main/java/ddingdong/ddingdongBE/domain/formapplication/controller/dto/request/UpdateFormApplicationStatusRequest.java (2)
9-13: 검증 로직이 잘 구현되었습니다!
@NotNull검증과 한글 에러 메시지가 적절합니다- Swagger 문서화를 위한
@Schema어노테이션이 잘 적용되었습니다
14-21: Command 변환 로직이 깔끔합니다!
toCommand메서드가 모든 필요한 필드를 포함하여 명확하게 구현되었습니다.src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/GeneralFormAnswerService.java (1)
25-28: 구현이 깔끔하고 명확합니다!트랜잭션 처리가 적절하게 되어있고, 메서드 구현이 간단명료합니다.
src/main/java/ddingdong/ddingdongBE/domain/form/service/dto/query/FormQuery.java (1)
23-23: ID 필드 추가가 적절합니다!폼 필드의 식별을 위한 ID 필드 추가는 적절한 변경사항입니다.
Also applies to: 33-33
src/main/java/ddingdong/ddingdongBE/domain/formapplication/controller/CentralFormApplicationController.java (1)
19-19: 변수명 변경이 적절합니다!
facadeCentralFormService에서facadeCentralFormApplicationService로의 변경은 더 명확한 의미를 전달합니다.src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FacadeCentralFormApplicationServiceImpl.java (3)
33-34: 의존성 주입이 적절히 구성되었습니다.새로 추가된 FormAnswerService와 FormFieldService의 의존성 주입이 잘 구성되어 있습니다.
56-67: getFormApplication 메소드의 보안 검증이 잘 구현되어 있습니다.사용자의 권한 검증이 적절히 구현되어 있으며, 필요한 데이터를 효율적으로 조회하고 있습니다.
하지만 다음 사항들을 고려해보시기 바랍니다:
- formApplication이 해당 form에 속하는지 검증이 필요합니다.
- null 체크가 누락되어 있습니다.
@Override public FormApplicationQuery getFormApplication(Long formId, Long applicationId, User user) { Club club = clubService.getByUserId(user.getId()); Form form = formService.getById(formId); if (!form.getClub().equals(club)) { throw new AccessDeniedException("권한이 없습니다."); } FormApplication formApplication = formApplicationService.getById(applicationId); + if (!formApplication.getForm().equals(form)) { + throw new AccessDeniedException("해당 폼의 지원서가 아닙니다."); + } List<FormField> formFields = formFieldService.findAllByForm(form); List<FormAnswer> formAnswers = formAnswerService.getAllByApplication(formApplication); return FormApplicationQuery.of(formApplication, formFields, formAnswers); }
69-79: updateStatus 메소드의 트랜잭션 처리가 적절합니다.@transactional 어노테이션이 올바르게 적용되어 있으며, 권한 검증도 잘 구현되어 있습니다.
다만 다음 개선사항을 고려해보시기 바랍니다:
- formApplication이 해당 form에 속하는지 검증이 필요합니다.
- status 값의 유효성 검증이 필요할 수 있습니다.
@Transactional @Override public void updateStatus(UpdateFormApplicationStatusCommand command) { Club club = clubService.getByUserId(command.user().getId()); Form form = formService.getById(command.formId()); if (!form.getClub().equals(club)) { throw new AccessDeniedException("권한이 없습니다."); } FormApplication formApplication = formApplicationService.getById(command.applicationId()); + if (!formApplication.getForm().equals(form)) { + throw new AccessDeniedException("해당 폼의 지원서가 아닙니다."); + } formApplicationService.updateStatus(command.applicationId(), command.status()); }src/test/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImplTest.java (1)
51-51: FixtureMonkey 설정이 적절합니다.테스트 데이터 생성을 위한 FixtureMonkey 설정이 잘 되어 있습니다.
Comment on lines
16
to
21
| public FormApplication toEntity(Form form) { | ||
| return FormApplication.builder() | ||
| .form(form) | ||
| .status(status) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
toEntity 메서드의 구현이 불완전해 보입니다.
toEntity 메서드가 form과 status만 설정하고 있습니다. 다음 사항들을 고려해주세요:
- 다른 필수 필드들이 누락되었을 수 있습니다
- 기존 엔티티의 다른 필드들이 손실될 수 있습니다
다음과 같이 수정하는 것을 고려해보세요:
public FormApplication toEntity(Form form) {
return FormApplication.builder()
.form(form)
.status(status)
+ // 다른 필수 필드들 추가
.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
| public FormApplication toEntity(Form form) { | |
| return FormApplication.builder() | |
| .form(form) | |
| .status(status) | |
| .build(); | |
| } | |
| public FormApplication toEntity(Form form) { | |
| return FormApplication.builder() | |
| .form(form) | |
| .status(status) | |
| // 다른 필수 필드들 추가 | |
| .build(); | |
| } |
Comment on lines
45
to
47
| public void update(FormApplicationStatus status) { | ||
| this.status = status; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
상태 업데이트 시 유효성 검증이 필요합니다.
상태 변경 시 null 체크와 상태 전이 규칙 검증을 추가하는 것이 좋습니다.
다음과 같이 개선해보세요:
public void update(FormApplicationStatus status) {
+ if (status == null) {
+ throw new IllegalArgumentException("상태값은 null이 될 수 없습니다.");
+ }
+ validateStatusTransition(status);
this.status = status;
}
+private void validateStatusTransition(FormApplicationStatus newStatus) {
+ // 현재 상태에서 새로운 상태로의 전이가 가능한지 검증
+ if (!this.status.canTransitionTo(newStatus)) {
+ throw new IllegalStateException(
+ String.format("현재 상태 %s에서 %s로 변경할 수 없습니다.", this.status, newStatus)
+ );
+ }
+}📝 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
| public void update(FormApplicationStatus status) { | |
| this.status = status; | |
| } | |
| public void update(FormApplicationStatus status) { | |
| if (status == null) { | |
| throw new IllegalArgumentException("상태값은 null이 될 수 없습니다."); | |
| } | |
| validateStatusTransition(status); | |
| this.status = status; | |
| } | |
| private void validateStatusTransition(FormApplicationStatus newStatus) { | |
| // 현재 상태에서 새로운 상태로의 전이가 가능한지 검증 | |
| if (!this.status.canTransitionTo(newStatus)) { | |
| throw new IllegalStateException( | |
| String.format("현재 상태 %s에서 %s로 변경할 수 없습니다.", this.status, newStatus) | |
| ); | |
| } | |
| } |
Comment on lines
28
to
33
| @Override | ||
| public FormApplicationResponse getFormApplication(Long formId, Long applicationId, PrincipalDetails principalDetails) { | ||
| User user = principalDetails.getUser(); | ||
| FormApplicationQuery query = facadeCentralFormApplicationService.getFormApplication(formId, applicationId, user); | ||
| return FormApplicationResponse.from(query); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
요청 파라미터 검증이 필요합니다.
엔드포인트에 @Valid 또는 @validated 어노테이션을 추가하여 입력값 검증을 구현하는 것이 좋습니다.
다음과 같이 개선해보세요:
@Override
-public FormApplicationResponse getFormApplication(Long formId, Long applicationId, PrincipalDetails principalDetails) {
+public FormApplicationResponse getFormApplication(
+ @Positive(message = "폼 ID는 양수여야 합니다") @PathVariable Long formId,
+ @Positive(message = "지원서 ID는 양수여야 합니다") @PathVariable Long applicationId,
+ @Valid PrincipalDetails principalDetails) {📝 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
| @Override | |
| public FormApplicationResponse getFormApplication(Long formId, Long applicationId, PrincipalDetails principalDetails) { | |
| User user = principalDetails.getUser(); | |
| FormApplicationQuery query = facadeCentralFormApplicationService.getFormApplication(formId, applicationId, user); | |
| return FormApplicationResponse.from(query); | |
| } | |
| @Override | |
| public FormApplicationResponse getFormApplication( | |
| @Positive(message = "폼 ID는 양수여야 합니다") @PathVariable Long formId, | |
| @Positive(message = "지원서 ID는 양수여야 합니다") @PathVariable Long applicationId, | |
| @Valid PrincipalDetails principalDetails) { | |
| User user = principalDetails.getUser(); | |
| FormApplicationQuery query = facadeCentralFormApplicationService.getFormApplication(formId, applicationId, user); | |
| return FormApplicationResponse.from(query); | |
| } |
Comment on lines
35
to
39
| @Override | ||
| public void updateFormApplicationStatus(Long formId, Long applicationId, PrincipalDetails principalDetails, UpdateFormApplicationStatusRequest request) { | ||
| User user = principalDetails.getUser(); | ||
| facadeCentralFormApplicationService.updateStatus(request.toCommand(formId, applicationId, user)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
상태 업데이트 요청에 대한 검증이 필요합니다.
상태 변경 요청에 대한 유효성 검증을 추가하고, 적절한 예외 처리를 구현해야 합니다.
다음과 같이 개선해보세요:
@Override
-public void updateFormApplicationStatus(Long formId, Long applicationId, PrincipalDetails principalDetails, UpdateFormApplicationStatusRequest request) {
+public void updateFormApplicationStatus(
+ @Positive(message = "폼 ID는 양수여야 합니다") @PathVariable Long formId,
+ @Positive(message = "지원서 ID는 양수여야 합니다") @PathVariable Long applicationId,
+ @Valid PrincipalDetails principalDetails,
+ @Valid @RequestBody UpdateFormApplicationStatusRequest request) {📝 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
| @Override | |
| public void updateFormApplicationStatus(Long formId, Long applicationId, PrincipalDetails principalDetails, UpdateFormApplicationStatusRequest request) { | |
| User user = principalDetails.getUser(); | |
| facadeCentralFormApplicationService.updateStatus(request.toCommand(formId, applicationId, user)); | |
| } | |
| @Override | |
| public void updateFormApplicationStatus( | |
| @Positive(message = "폼 ID는 양수여야 합니다") @PathVariable Long formId, | |
| @Positive(message = "지원서 ID는 양수여야 합니다") @PathVariable Long applicationId, | |
| @Valid PrincipalDetails principalDetails, | |
| @Valid @RequestBody UpdateFormApplicationStatusRequest request) { | |
| User user = principalDetails.getUser(); | |
| facadeCentralFormApplicationService.updateStatus(request.toCommand(formId, applicationId, user)); | |
| } |
Comment on lines
40
to
44
| @Override | ||
| public void updateStatus(Long applicationId, FormApplicationStatus status) { | ||
| FormApplication formApplication = formApplicationRepository.findById(applicationId).orElse(null); | ||
| formApplication.update(status); | ||
| } |
There was a problem hiding this comment.
NullPointerException 방지를 위한 null 체크 추가 필요
updateStatus 메서드에서 formApplication이 null일 경우 NullPointerException이 발생할 수 있습니다.
다음과 같이 수정하는 것을 제안합니다:
@Override
public void updateStatus(Long applicationId, FormApplicationStatus status) {
FormApplication formApplication = formApplicationRepository.findById(applicationId).orElse(null);
+ if (formApplication == null) {
+ throw new IllegalArgumentException("지원서를 찾을 수 없습니다: " + applicationId);
+ }
formApplication.update(status);
}📝 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
| @Override | |
| public void updateStatus(Long applicationId, FormApplicationStatus status) { | |
| FormApplication formApplication = formApplicationRepository.findById(applicationId).orElse(null); | |
| formApplication.update(status); | |
| } | |
| @Override | |
| public void updateStatus(Long applicationId, FormApplicationStatus status) { | |
| FormApplication formApplication = formApplicationRepository.findById(applicationId).orElse(null); | |
| if (formApplication == null) { | |
| throw new IllegalArgumentException("지원서를 찾을 수 없습니다: " + applicationId); | |
| } | |
| formApplication.update(status); | |
| } |
Comment on lines
242
to
262
| @DisplayName("동아리는 폼지를 상세조회 할 수 있다.") | ||
| @Test | ||
| void getForm() { | ||
| // given | ||
| Form form = fixtureMonkey.giveMeBuilder(Form.class) | ||
| .set("id", 1L) | ||
| .set("title", "제목1") | ||
| .set("club", null) | ||
| .sample(); | ||
| Form form2 = fixtureMonkey.giveMeBuilder(Form.class) | ||
| .set("id", 2L) | ||
| .set("title", "제목2") | ||
| .set("club", null) | ||
| .sample(); | ||
| formService.create(form); | ||
| formService.create(form2); | ||
| // when | ||
| FormQuery formQuery = facadeCentralFormService.getForm(1L); | ||
| // then | ||
| assertThat(formQuery.title()).isEqualTo("제목1"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
getForm 테스트에서 예외 케이스 처리가 누락되었습니다.
존재하지 않는 Form ID를 조회하는 경우에 대한 테스트가 필요합니다.
@Test
void getForm() {
// ... existing test code ...
+
+ @Test
+ @DisplayName("존재하지 않는 폼 조회시 예외가 발생한다")
+ void getForm_NotFound() {
+ // when & then
+ assertThrows(EntityNotFoundException.class,
+ () -> facadeCentralFormService.getForm(999L));
+ }
}Committable suggestion skipped: line range outside the PR's diff.
Comment on lines
84
to
127
| @DisplayName("폼지와 폼지 질문을 수정할 수 있다.") | ||
| @Test | ||
| void updateForm() { | ||
| // given | ||
| User user = fixtureMonkey.giveMeBuilder(User.class) | ||
| .set("id", 1L) | ||
| .set("Role", Role.CLUB) | ||
| .set("deletedAt", null) | ||
| .sample(); | ||
| User savedUser = userRepository.save(user); | ||
| Club club = fixtureMonkey.giveMeBuilder(Club.class) | ||
| .set("id", 1L) | ||
| .set("user", savedUser) | ||
| .set("score", null) | ||
| .set("clubMembers", null) | ||
| .set("deletedAt", null) | ||
| .sample(); | ||
| clubRepository.save(club); | ||
| Form form = fixtureMonkey.giveMeBuilder(Form.class) | ||
| .set("club", club) | ||
| .sample(); | ||
| Form savedForm = formService.create(form); | ||
| UpdateFormCommand updateFormCommand = fixtureMonkey.giveMeBuilder(UpdateFormCommand.class) | ||
| .set("title", "수정된 제목") | ||
| .set("description", "수정된 설명") | ||
| .set("formId", savedForm.getId()) | ||
| .set("formFieldCommands", List.of( | ||
| fixtureMonkey.giveMeBuilder(UpdateFormFieldCommand.class) | ||
| .set("question", "수정된 질문") | ||
| .sample()) | ||
| ) | ||
| .sample(); | ||
| // when | ||
| facadeCentralFormService.updateForm(updateFormCommand); | ||
| // then | ||
| Form found = formRepository.findById(savedForm.getId()).orElse(null); | ||
| List<FormField> formFields = formFieldRepository.findAllByForm(found); | ||
| assertThat(found).isNotNull(); | ||
| assertThat(found.getTitle()).isEqualTo("수정된 제목"); | ||
| assertThat(found.getDescription()).isEqualTo("수정된 설명"); | ||
| assertThat(formFields).isNotEmpty(); | ||
| assertThat(formFields.get(0).getQuestion()).isEqualTo("수정된 질문"); | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
updateForm 테스트의 중복된 설정 코드를 개선해야 합니다.
테스트 설정 코드가 createForm 테스트와 중복됩니다. @beforeeach를 사용하여 공통 설정을 분리하는 것이 좋습니다.
+ private User savedUser;
+ private Club club;
+
+ @BeforeEach
+ void setUp() {
+ User user = fixtureMonkey.giveMeBuilder(User.class)
+ .set("id", 1L)
+ .set("Role", Role.CLUB)
+ .set("deletedAt", null)
+ .sample();
+ savedUser = userRepository.save(user);
+ Club clubEntity = fixtureMonkey.giveMeBuilder(Club.class)
+ .set("id", 1L)
+ .set("user", savedUser)
+ .set("score", null)
+ .set("clubMembers", null)
+ .set("deletedAt", null)
+ .sample();
+ club = clubRepository.save(clubEntity);
+ }
@Test
void updateForm() {
- // given
- User user = fixtureMonkey.giveMeBuilder(User.class)...
- User savedUser = userRepository.save(user);
- Club club = fixtureMonkey.giveMeBuilder(Club.class)...
- clubRepository.save(club);
Form form = fixtureMonkey.giveMeBuilder(Form.class)
.set("club", club)
.sample();
// ... rest of the test
}📝 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
| @DisplayName("폼지와 폼지 질문을 수정할 수 있다.") | |
| @Test | |
| void updateForm() { | |
| // given | |
| User user = fixtureMonkey.giveMeBuilder(User.class) | |
| .set("id", 1L) | |
| .set("Role", Role.CLUB) | |
| .set("deletedAt", null) | |
| .sample(); | |
| User savedUser = userRepository.save(user); | |
| Club club = fixtureMonkey.giveMeBuilder(Club.class) | |
| .set("id", 1L) | |
| .set("user", savedUser) | |
| .set("score", null) | |
| .set("clubMembers", null) | |
| .set("deletedAt", null) | |
| .sample(); | |
| clubRepository.save(club); | |
| Form form = fixtureMonkey.giveMeBuilder(Form.class) | |
| .set("club", club) | |
| .sample(); | |
| Form savedForm = formService.create(form); | |
| UpdateFormCommand updateFormCommand = fixtureMonkey.giveMeBuilder(UpdateFormCommand.class) | |
| .set("title", "수정된 제목") | |
| .set("description", "수정된 설명") | |
| .set("formId", savedForm.getId()) | |
| .set("formFieldCommands", List.of( | |
| fixtureMonkey.giveMeBuilder(UpdateFormFieldCommand.class) | |
| .set("question", "수정된 질문") | |
| .sample()) | |
| ) | |
| .sample(); | |
| // when | |
| facadeCentralFormService.updateForm(updateFormCommand); | |
| // then | |
| Form found = formRepository.findById(savedForm.getId()).orElse(null); | |
| List<FormField> formFields = formFieldRepository.findAllByForm(found); | |
| assertThat(found).isNotNull(); | |
| assertThat(found.getTitle()).isEqualTo("수정된 제목"); | |
| assertThat(found.getDescription()).isEqualTo("수정된 설명"); | |
| assertThat(formFields).isNotEmpty(); | |
| assertThat(formFields.get(0).getQuestion()).isEqualTo("수정된 질문"); | |
| } | |
| public class FacadeCentralFormServiceImplTest { | |
| // ... (other class members and imports) | |
| private User savedUser; | |
| private Club club; | |
| @BeforeEach | |
| void setUp() { | |
| User user = fixtureMonkey.giveMeBuilder(User.class) | |
| .set("id", 1L) | |
| .set("Role", Role.CLUB) | |
| .set("deletedAt", null) | |
| .sample(); | |
| savedUser = userRepository.save(user); | |
| Club clubEntity = fixtureMonkey.giveMeBuilder(Club.class) | |
| .set("id", 1L) | |
| .set("user", savedUser) | |
| .set("score", null) | |
| .set("clubMembers", null) | |
| .set("deletedAt", null) | |
| .sample(); | |
| club = clubRepository.save(clubEntity); | |
| } | |
| @DisplayName("폼지와 폼지 질문을 수정할 수 있다.") | |
| @Test | |
| void updateForm() { | |
| // given | |
| Form form = fixtureMonkey.giveMeBuilder(Form.class) | |
| .set("club", club) | |
| .sample(); | |
| Form savedForm = formService.create(form); | |
| UpdateFormCommand updateFormCommand = fixtureMonkey.giveMeBuilder(UpdateFormCommand.class) | |
| .set("title", "수정된 제목") | |
| .set("description", "수정된 설명") | |
| .set("formId", savedForm.getId()) | |
| .set("formFieldCommands", List.of( | |
| fixtureMonkey.giveMeBuilder(UpdateFormFieldCommand.class) | |
| .set("question", "수정된 질문") | |
| .sample()) | |
| ) | |
| .sample(); | |
| // when | |
| facadeCentralFormService.updateForm(updateFormCommand); | |
| // then | |
| Form found = formRepository.findById(savedForm.getId()).orElse(null); | |
| List<FormField> formFields = formFieldRepository.findAllByForm(found); | |
| assertThat(found).isNotNull(); | |
| assertThat(found.getTitle()).isEqualTo("수정된 제목"); | |
| assertThat(found.getDescription()).isEqualTo("수정된 설명"); | |
| assertThat(formFields).isNotEmpty(); | |
| assertThat(formFields.get(0).getQuestion()).isEqualTo("수정된 질문"); | |
| } | |
| // ... (other tests and methods) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FacadeCentralFormApplicationServiceImpl.java (1)
56-60: 상태 변경에 대한 로깅과 유효성 검증이 필요합니다.상태 변경은 중요한 도메인 이벤트이므로, 다음 개선사항을 고려해주세요:
- 상태 변경 전 유효성 검증
- 상태 변경 이력 로깅
- 상태 변경 이벤트 발행 고려
예시 코드:
@Transactional @Override public void updateStatus(UpdateFormApplicationStatusCommand command) { + log.info("지원서 상태 변경 시도: applicationId={}, newStatus={}", + command.applicationId(), command.status()); + + FormApplication application = formApplicationService.getById(command.applicationId()); + Status oldStatus = application.getStatus(); + formApplicationService.updateStatus(command.applicationId(), command.status()); + + log.info("지원서 상태 변경 완료: applicationId={}, oldStatus={}, newStatus={}", + command.applicationId(), oldStatus, command.status()); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FacadeCentralFormApplicationServiceImpl.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (2)
src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FacadeCentralFormApplicationServiceImpl.java (2)
3-7: 의존성 추가가 적절합니다!새로운 기능 구현을 위해 필요한 의존성들이 잘 추가되었습니다.
Also applies to: 29-30
47-54: 유효성 검증 로직 추가가 필요합니다.다음 사항들에 대한 검증이 필요해 보입니다:
- 조회된 엔티티들의 null 체크
- formApplication이 주어진 form에 속하는지 검증
예시 코드:
@Override public FormApplicationQuery getFormApplication(Long formId, Long applicationId, User user) { Form form = formService.getById(formId); FormApplication formApplication = formApplicationService.getById(applicationId); + if (!formApplication.getForm().getId().equals(formId)) { + throw new IllegalArgumentException("해당 폼에 속하지 않는 지원서입니다."); + } List<FormField> formFields = formFieldService.findAllByForm(form); List<FormAnswer> formAnswers = formAnswerService.getAllByApplication(formApplication); return FormApplicationQuery.of(formApplication, formFields, formAnswers); }
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FormApplicationService.java (1)
14-16: 메서드 문서화 추가 필요인터페이스에 새로 추가된 메서드들의 동작과 예외 상황을 명확히 하기 위해 JavaDoc 문서화가 필요합니다.
다음과 같이 수정하는 것을 제안합니다:
+ /** + * 지원서 ID로 단일 지원서를 조회합니다. + * + * @param applicationId 조회할 지원서 ID + * @return 조회된 지원서, 없을 경우 null + */ FormApplication getById(Long applicationId); + /** + * 여러 지원서 ID로 복수의 지원서를 조회합니다. + * + * @param applicationIds 조회할 지원서 ID 목록 + * @return 조회된 지원서 목록 + */ List<FormApplication> getAllById(List<Long> applicationIds);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/ddingdong/ddingdongBE/domain/formapplication/api/CentralFormApplicationApi.java(3 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/controller/CentralFormApplicationController.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/controller/dto/request/UpdateFormApplicationStatusRequest.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FacadeCentralFormApplicationServiceImpl.java(3 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/java/ddingdong/ddingdongBE/domain/formapplication/service/dto/command/UpdateFormApplicationStatusCommand.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (4)
src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/dto/command/UpdateFormApplicationStatusCommand.java (1)
18-23:⚠️ Potential issuetoEntity 메서드의 구현이 불완전합니다.
현재 구현은 form과 status만 설정하고 있어 다른 필수 필드들이 누락될 수 있습니다. 상태 업데이트만을 위한 별도의 메서드를 만드는 것이 좋습니다.
다음과 같이 수정하는 것을 제안합니다:
-public FormApplication toEntity(Form form) { - return FormApplication.builder() - .form(form) - .status(status) - .build(); -} +public FormApplication updateStatus(FormApplication application) { + return application.toBuilder() + .status(status) + .build(); +}Likely invalid or redundant comment.
src/main/java/ddingdong/ddingdongBE/domain/formapplication/controller/CentralFormApplicationController.java (1)
29-33:⚠️ Potential issue경로 변수에 대한 유효성 검증이 필요합니다.
formId와 applicationId에 대한 유효성 검증이 누락되어 있습니다.
다음과 같이 수정하는 것을 제안합니다:
@Override public FormApplicationResponse getFormApplication( - Long formId, - Long applicationId, + @Positive(message = "폼 ID는 양수여야 합니다") @PathVariable Long formId, + @Positive(message = "지원서 ID는 양수여야 합니다") @PathVariable Long applicationId, @Valid PrincipalDetails principalDetails) {Likely invalid or redundant comment.
src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FacadeCentralFormApplicationServiceImpl.java (2)
3-8: 의존성 주입이 잘 구성되어 있습니다!새로운 의존성들이 단일 책임 원칙(SRP)을 잘 따르고 있으며, 생성자 주입을 통해 적절하게 관리되고 있습니다.
Also applies to: 30-32
49-56: 폼과 지원서 간의 유효성 검증이 필요합니다.다음과 같은 개선사항을 제안드립니다:
- 폼 애플리케이션이 해당 폼에 속하는지 검증이 필요합니다.
user파라미터가 현재 사용되지 않고 있습니다. 권한 검증에 활용하거나 불필요하다면 제거해주세요.아래는 개선된 구현을 제안드립니다:
public FormApplicationQuery getFormApplication(Long formId, Long applicationId, User user) { Form form = formService.getById(formId); FormApplication formApplication = formApplicationService.getById(applicationId); + if (!formApplication.getForm().getId().equals(formId)) { + throw new IllegalArgumentException("해당 폼에 속하지 않는 지원서입니다."); + } List<FormField> formFields = formFieldService.findAllByForm(form); List<FormAnswer> formAnswers = formAnswerService.getAllByApplication(formApplication); return FormApplicationQuery.of(formApplication, formFields, formAnswers); }
Comment on lines
39
to
42
| @Override | ||
| public List<FormApplication> getAllById(List<Long> applicationIds) { | ||
| return formApplicationRepository.findAllById(applicationIds); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
입력 검증 및 예외 처리 추가 필요
applicationIds에 대한 유효성 검사와 결과가 없는 경우에 대한 처리가 필요합니다.
다음과 같이 수정하는 것을 제안합니다:
@Override
public List<FormApplication> getAllById(List<Long> applicationIds) {
+ if (applicationIds == null || applicationIds.isEmpty()) {
+ return new ArrayList<>();
+ }
+
+ // 중복 제거 및 null 값 필터링
+ applicationIds = applicationIds.stream()
+ .filter(id -> id != null)
+ .distinct()
+ .collect(Collectors.toList());
+
return formApplicationRepository.findAllById(applicationIds);
}📝 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
| @Override | |
| public List<FormApplication> getAllById(List<Long> applicationIds) { | |
| return formApplicationRepository.findAllById(applicationIds); | |
| } | |
| @Override | |
| public List<FormApplication> getAllById(List<Long> applicationIds) { | |
| if (applicationIds == null || applicationIds.isEmpty()) { | |
| return new ArrayList<>(); | |
| } | |
| // 중복 제거 및 null 값 필터링 | |
| applicationIds = applicationIds.stream() | |
| .filter(id -> id != null) | |
| .distinct() | |
| .collect(Collectors.toList()); | |
| return formApplicationRepository.findAllById(applicationIds); | |
| } |
Comment on lines
34
to
37
| @Override | ||
| public FormApplication getById(Long applicationId) { | ||
| return formApplicationRepository.findById(applicationId).orElse(null); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optional을 사용한 null 처리 개선 필요
null을 직접 반환하는 대신 Optional을 사용하거나 예외를 throw하는 것이 더 안전한 방식입니다.
다음과 같이 수정하는 것을 제안합니다:
@Override
public FormApplication getById(Long applicationId) {
- return formApplicationRepository.findById(applicationId).orElse(null);
+ return formApplicationRepository.findById(applicationId)
+ .orElseThrow(() -> new IllegalArgumentException("지원서를 찾을 수 없습니다: " + applicationId));
}📝 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
| @Override | |
| public FormApplication getById(Long applicationId) { | |
| return formApplicationRepository.findById(applicationId).orElse(null); | |
| } | |
| @Override | |
| public FormApplication getById(Long applicationId) { | |
| - return formApplicationRepository.findById(applicationId).orElse(null); | |
| + return formApplicationRepository.findById(applicationId) | |
| + .orElseThrow(() -> new IllegalArgumentException("지원서를 찾을 수 없습니다: " + applicationId)); | |
| } |
Comment on lines
13
to
14
| @Schema(description = "수정할 지원자 상태", example = "FIRST_PASS") | ||
| List<Long> applicationIds, |
There was a problem hiding this comment.
applicationIds의 스키마 설명이 잘못되었습니다.
applicationIds 필드의 @Schema 설명이 status 필드와 동일하게 설정되어 있습니다.
다음과 같이 수정하는 것을 제안합니다:
-@Schema(description = "수정할 지원자 상태", example = "FIRST_PASS")
+@Schema(description = "수정할 지원자 ID 목록", example = "[1, 2, 3]")
List<Long> applicationIds,📝 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
| @Schema(description = "수정할 지원자 상태", example = "FIRST_PASS") | |
| List<Long> applicationIds, | |
| @Schema(description = "수정할 지원자 ID 목록", example = "[1, 2, 3]") | |
| List<Long> applicationIds, |
Comment on lines
37
to
47
| @Operation(summary = "지원자 상세 조회 API") | ||
| @ApiResponse(responseCode = "200", description = "지원자 상세 조회 성공", | ||
| content = @Content(schema = @Schema(implementation = FormApplicationResponse.class))) | ||
| @ResponseStatus(HttpStatus.OK) | ||
| @SecurityRequirement(name = "AccessToken") | ||
| @GetMapping("/my/forms/{formId}/applications/{applicationId}") | ||
| FormApplicationResponse getFormApplication( | ||
| @PathVariable("formId") Long formId, | ||
| @PathVariable("applicationId") Long applicationId, | ||
| @AuthenticationPrincipal PrincipalDetails principalDetails | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
API 문서에 오류 응답에 대한 설명이 필요합니다.
현재 성공 케이스만 문서화되어 있습니다. 주요 오류 시나리오(권한 없음, 리소스 없음 등)에 대한 응답도 문서화가 필요합니다.
다음과 같이 추가하는 것을 제안합니다:
@ApiResponse(responseCode = "200", description = "지원자 상세 조회 성공",
content = @Content(schema = @Schema(implementation = FormApplicationResponse.class)))
+@ApiResponse(responseCode = "403", description = "권한 없음")
+@ApiResponse(responseCode = "404", description = "지원서를 찾을 수 없음")📝 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 = "지원자 상세 조회 API") | |
| @ApiResponse(responseCode = "200", description = "지원자 상세 조회 성공", | |
| content = @Content(schema = @Schema(implementation = FormApplicationResponse.class))) | |
| @ResponseStatus(HttpStatus.OK) | |
| @SecurityRequirement(name = "AccessToken") | |
| @GetMapping("/my/forms/{formId}/applications/{applicationId}") | |
| FormApplicationResponse getFormApplication( | |
| @PathVariable("formId") Long formId, | |
| @PathVariable("applicationId") Long applicationId, | |
| @AuthenticationPrincipal PrincipalDetails principalDetails | |
| ); | |
| @Operation(summary = "지원자 상세 조회 API") | |
| @ApiResponse(responseCode = "200", description = "지원자 상세 조회 성공", | |
| content = @Content(schema = @Schema(implementation = FormApplicationResponse.class))) | |
| @ApiResponse(responseCode = "403", description = "권한 없음") | |
| @ApiResponse(responseCode = "404", description = "지원서를 찾을 수 없음") | |
| @ResponseStatus(HttpStatus.OK) | |
| @SecurityRequirement(name = "AccessToken") | |
| @GetMapping("/my/forms/{formId}/applications/{applicationId}") | |
| FormApplicationResponse getFormApplication( | |
| @PathVariable("formId") Long formId, | |
| @PathVariable("applicationId") Long applicationId, | |
| @AuthenticationPrincipal PrincipalDetails principalDetails | |
| ); |
Comment on lines
+49
to
+58
| @Operation(summary = "지원자 상태 수정 API") | ||
| @ApiResponse(responseCode = "204", description = "지원자 상태 수정 성공") | ||
| @ResponseStatus(HttpStatus.NO_CONTENT) | ||
| @SecurityRequirement(name = "AccessToken") | ||
| @PatchMapping("/my/forms/{formId}/applications") | ||
| void updateFormApplicationStatus( | ||
| @PathVariable("formId") Long formId, | ||
| @AuthenticationPrincipal PrincipalDetails principalDetails, | ||
| @Valid @RequestBody UpdateFormApplicationStatusRequest request | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
상태 업데이트 API에도 오류 응답 문서화가 필요합니다.
상태 업데이트 API도 주요 오류 시나리오에 대한 응답 문서화가 필요합니다.
다음과 같이 추가하는 것을 제안합니다:
@ApiResponse(responseCode = "204", description = "지원자 상태 수정 성공")
+@ApiResponse(responseCode = "400", description = "잘못된 요청")
+@ApiResponse(responseCode = "403", description = "권한 없음")
+@ApiResponse(responseCode = "404", description = "지원서를 찾을 수 없음")📝 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 = "지원자 상태 수정 API") | |
| @ApiResponse(responseCode = "204", description = "지원자 상태 수정 성공") | |
| @ResponseStatus(HttpStatus.NO_CONTENT) | |
| @SecurityRequirement(name = "AccessToken") | |
| @PatchMapping("/my/forms/{formId}/applications") | |
| void updateFormApplicationStatus( | |
| @PathVariable("formId") Long formId, | |
| @AuthenticationPrincipal PrincipalDetails principalDetails, | |
| @Valid @RequestBody UpdateFormApplicationStatusRequest request | |
| ); | |
| @Operation(summary = "지원자 상태 수정 API") | |
| @ApiResponse(responseCode = "204", description = "지원자 상태 수정 성공") | |
| @ApiResponse(responseCode = "400", description = "잘못된 요청") | |
| @ApiResponse(responseCode = "403", description = "권한 없음") | |
| @ApiResponse(responseCode = "404", description = "지원서를 찾을 수 없음") | |
| @ResponseStatus(HttpStatus.NO_CONTENT) | |
| @SecurityRequirement(name = "AccessToken") | |
| @PatchMapping("/my/forms/{formId}/applications") | |
| void updateFormApplicationStatus( | |
| @PathVariable("formId") Long formId, | |
| @AuthenticationPrincipal PrincipalDetails principalDetails, | |
| @Valid @RequestBody UpdateFormApplicationStatusRequest request | |
| ); |
Comment on lines
58
to
64
| @Transactional | ||
| @Override | ||
| public void updateStatus(UpdateFormApplicationStatusCommand command) { | ||
| List<FormApplication> formApplications = formApplicationService.getAllById(command.applicationIds()); | ||
| formApplications.forEach(formApplication -> formApplication.update(command.status())); | ||
| formApplicationRepository.saveAll(formApplications); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
상태 업데이트 시 유효성 검증과 예외 처리가 필요합니다.
다음과 같은 개선사항을 제안드립니다:
- 상태값의 유효성 검증
- 존재하지 않는 지원서에 대한 예외 처리
- 벌크 업데이트 실패 시의 예외 처리
아래는 개선된 구현을 제안드립니다:
@Transactional
@Override
public void updateStatus(UpdateFormApplicationStatusCommand command) {
+ if (command.status() == null) {
+ throw new IllegalArgumentException("상태값은 null일 수 없습니다.");
+ }
List<FormApplication> formApplications = formApplicationService.getAllById(command.applicationIds());
+ if (formApplications.size() != command.applicationIds().size()) {
+ throw new IllegalArgumentException("존재하지 않는 지원서가 포함되어 있습니다.");
+ }
formApplications.forEach(formApplication -> formApplication.update(command.status()));
- formApplicationRepository.saveAll(formApplications);
+ try {
+ formApplicationRepository.saveAll(formApplications);
+ } catch (Exception e) {
+ throw new RuntimeException("상태 업데이트 중 오류가 발생했습니다.", e);
+ }
}📝 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
| @Transactional | |
| @Override | |
| public void updateStatus(UpdateFormApplicationStatusCommand command) { | |
| List<FormApplication> formApplications = formApplicationService.getAllById(command.applicationIds()); | |
| formApplications.forEach(formApplication -> formApplication.update(command.status())); | |
| formApplicationRepository.saveAll(formApplications); | |
| } | |
| @Transactional | |
| @Override | |
| public void updateStatus(UpdateFormApplicationStatusCommand command) { | |
| if (command.status() == null) { | |
| throw new IllegalArgumentException("상태값은 null일 수 없습니다."); | |
| } | |
| List<FormApplication> formApplications = formApplicationService.getAllById(command.applicationIds()); | |
| if (formApplications.size() != command.applicationIds().size()) { | |
| throw new IllegalArgumentException("존재하지 않는 지원서가 포함되어 있습니다."); | |
| } | |
| formApplications.forEach(formApplication -> formApplication.update(command.status())); | |
| try { | |
| formApplicationRepository.saveAll(formApplications); | |
| } catch (Exception e) { | |
| throw new RuntimeException("상태 업데이트 중 오류가 발생했습니다.", e); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/test/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormApplicationServiceImplTest.java (2)
1-1: 패키지 구조 업데이트 필요현재 테스트 클래스가
form.service패키지에 위치해 있지만, 클래스의 주요 기능이formapplication도메인을 다루고 있습니다. 패키지 구조를formapplication.service로 이동하는 것이 더 적절할 것 같습니다.-package ddingdong.ddingdongBE.domain.form.service; +package ddingdong.ddingdongBE.domain.formapplication.service;
49-85: 테스트 검증 범위 확장 필요현재 테스트는 지원자의 이름만 검증하고 있습니다. 응답의 완전성을 보장하기 위해 다음 필드들도 검증이 필요합니다:
- studentNumber
- department
- status
// then assertThat(formApplicationQuery.name()).isEqualTo("지원자1"); + assertThat(formApplicationQuery.studentNumber()).isEqualTo("60201115"); + assertThat(formApplicationQuery.department()).isEqualTo("융합소프트웨어학부"); + assertThat(formApplicationQuery.status()).isEqualTo(FormApplicationStatus.SUBMITTED);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormApplicationServiceImplTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
Comment on lines
87
to
143
| .set("deletedAt", null) | ||
| .sample(); | ||
| clubRepository.save(club); | ||
| User user2 = fixtureMonkey.giveMeBuilder(User.class) | ||
| .set("id", 2L) | ||
| .set("Role", Role.CLUB) | ||
| .set("deletedAt", null) | ||
| .sample(); | ||
| User savedUser2 = userRepository.save(user2); | ||
| Club club2 = fixtureMonkey.giveMeBuilder(Club.class) | ||
| .set("id", 2L) | ||
| .set("user", savedUser2) | ||
| .set("score", null) | ||
| .set("clubMembers", null) | ||
| .set("deletedAt", null) | ||
| .sample(); | ||
| clubRepository.save(club2); | ||
|
|
||
| Form form = fixtureMonkey.giveMeBuilder(Form.class) | ||
| .set("club", club) | ||
| .sample(); | ||
| Form savedForm = formService.create(form); | ||
| // when //then | ||
| assertThrows(NonHaveAuthority.class, () -> { | ||
| facadeCentralFormService.deleteForm(savedForm.getId(), user2); | ||
| }); | ||
| } | ||
|
|
||
| @DisplayName("동아리는 자신의 폼지를 전부 조회할 수 있다.") | ||
| @Test | ||
| void getAllMyForm() { | ||
| // given | ||
| User user = fixtureMonkey.giveMeBuilder(User.class) | ||
| Club savedClub = clubRepository.saveAndFlush(club); | ||
| Form form = fixture.giveMeBuilder(Form.class) | ||
| .set("id", 1L) | ||
| .set("Role", Role.CLUB) | ||
| .set("deletedAt", null) | ||
| .sample(); | ||
| User savedUser = userRepository.save(user); | ||
| Club club = fixtureMonkey.giveMeBuilder(Club.class) | ||
| .set("id", 1L) | ||
| .set("user", savedUser) | ||
| .set("score", null) | ||
| .set("clubMembers", null) | ||
| .set("deletedAt", null) | ||
| .sample(); | ||
| clubRepository.save(club); | ||
| Form form = fixtureMonkey.giveMeBuilder(Form.class) | ||
| .set("title", "제목1") | ||
| .set("club", club) | ||
| .sample(); | ||
| Form form2 = fixtureMonkey.giveMeBuilder(Form.class) | ||
| .set("title", "제목2") | ||
| .set("club", club) | ||
| .set("club", savedClub) | ||
| .sample(); | ||
| Form savedForm = formRepository.saveAndFlush(form); | ||
| FormApplication formApplication1 = FormApplication.builder() | ||
| .name("지원자1") | ||
| .studentNumber("60201115") | ||
| .department("융합소프트웨어학부") | ||
| .status(FormApplicationStatus.SUBMITTED) | ||
| .form(savedForm) | ||
| .build(); | ||
| FormApplication formApplication2 = FormApplication.builder() | ||
| .name("지원자2") | ||
| .studentNumber("602011156") | ||
| .department("디지털콘텐츠디자인학과") | ||
| .status(FormApplicationStatus.SUBMITTED) | ||
| .form(savedForm) | ||
| .build(); | ||
| FormApplication savedApplication1 = formApplicationService.create(formApplication1); | ||
| FormApplication savedApplication2 = formApplicationService.create(formApplication2); | ||
| List<Long> applicationIds = new ArrayList<>(); | ||
| applicationIds.add(savedApplication1.getId()); | ||
| applicationIds.add(savedApplication2.getId()); | ||
| UpdateFormApplicationStatusCommand command = fixture.giveMeBuilder(UpdateFormApplicationStatusCommand.class) | ||
| .set("formId", savedForm.getId()) | ||
| .set("applicationIds", applicationIds) | ||
| .set("status", FormApplicationStatus.FIRST_PASS) | ||
| .set("user", savedUser) | ||
| .sample(); | ||
| formService.create(form); | ||
| formService.create(form2); | ||
|
|
||
| // when | ||
| List<FormListQuery> queries = facadeCentralFormService.getAllMyForm(savedUser); | ||
| facadeCentralFormApplicationService.updateStatus(command); | ||
| // then | ||
| assertThat(queries).isNotEmpty(); | ||
| assertThat(queries.size()).isEqualTo(2); | ||
| assertThat(queries.get(0).title()).isEqualTo("제목1"); | ||
| assertThat(queries.get(1).title()).isEqualTo("제목2"); | ||
|
|
||
| assertThat(formApplication1.getName()).isEqualTo("지원자1"); | ||
| assertThat(formApplication1.getStatus()).isEqualTo(FormApplicationStatus.FIRST_PASS); | ||
| assertThat(formApplication2.getName()).isEqualTo("지원자2"); | ||
| assertThat(formApplication2.getStatus()).isEqualTo(FormApplicationStatus.FIRST_PASS); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
테스트 케이스 보완 필요
현재 테스트는 메모리상의 객체만 검증하고 있어, 실제 데이터베이스에 변경사항이 저장되었는지 확인할 수 없습니다. 또한 다음과 같은 엣지 케이스에 대한 테스트가 필요합니다:
- 빈 applicationIds 리스트 처리
- 존재하지 않는 applicationId 처리
- 권한이 없는 사용자의 요청 처리
데이터베이스 저장 검증을 위해 다음과 같이 수정하는 것을 제안합니다:
// when
facadeCentralFormApplicationService.updateStatus(command);
// then
- assertThat(formApplication1.getName()).isEqualTo("지원자1");
- assertThat(formApplication1.getStatus()).isEqualTo(FormApplicationStatus.FIRST_PASS);
- assertThat(formApplication2.getName()).isEqualTo("지원자2");
- assertThat(formApplication2.getStatus()).isEqualTo(FormApplicationStatus.FIRST_PASS);
+ FormApplication updatedApplication1 = formApplicationService.findById(savedApplication1.getId());
+ FormApplication updatedApplication2 = formApplicationService.findById(savedApplication2.getId());
+
+ assertThat(updatedApplication1.getStatus()).isEqualTo(FormApplicationStatus.FIRST_PASS);
+ assertThat(updatedApplication2.getStatus()).isEqualTo(FormApplicationStatus.FIRST_PASS);📝 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
| @DisplayName("동아리는 지원자의 상태를 수정할 수 있다.") | |
| @Test | |
| void validateEqualsClub() { | |
| void updateFormApplicationStatus() { | |
| // given | |
| User user = fixtureMonkey.giveMeBuilder(User.class) | |
| User user = fixture.giveMeBuilder(User.class) | |
| .set("id", 1L) | |
| .set("Role", Role.CLUB) | |
| .set("deletedAt", null) | |
| .sample(); | |
| User savedUser = userRepository.save(user); | |
| Club club = fixtureMonkey.giveMeBuilder(Club.class) | |
| User savedUser = userRepository.saveAndFlush(user); | |
| Club club = fixture.giveMeBuilder(Club.class) | |
| .set("id", 1L) | |
| .set("user", savedUser) | |
| .set("score", null) | |
| .set("clubMembers", null) | |
| .set("deletedAt", null) | |
| .sample(); | |
| clubRepository.save(club); | |
| User user2 = fixtureMonkey.giveMeBuilder(User.class) | |
| .set("id", 2L) | |
| .set("Role", Role.CLUB) | |
| .set("deletedAt", null) | |
| .sample(); | |
| User savedUser2 = userRepository.save(user2); | |
| Club club2 = fixtureMonkey.giveMeBuilder(Club.class) | |
| .set("id", 2L) | |
| .set("user", savedUser2) | |
| .set("score", null) | |
| .set("clubMembers", null) | |
| .set("deletedAt", null) | |
| .sample(); | |
| clubRepository.save(club2); | |
| Form form = fixtureMonkey.giveMeBuilder(Form.class) | |
| .set("club", club) | |
| .sample(); | |
| Form savedForm = formService.create(form); | |
| // when //then | |
| assertThrows(NonHaveAuthority.class, () -> { | |
| facadeCentralFormService.deleteForm(savedForm.getId(), user2); | |
| }); | |
| } | |
| @DisplayName("동아리는 자신의 폼지를 전부 조회할 수 있다.") | |
| @Test | |
| void getAllMyForm() { | |
| // given | |
| User user = fixtureMonkey.giveMeBuilder(User.class) | |
| Club savedClub = clubRepository.saveAndFlush(club); | |
| Form form = fixture.giveMeBuilder(Form.class) | |
| .set("id", 1L) | |
| .set("Role", Role.CLUB) | |
| .set("deletedAt", null) | |
| .sample(); | |
| User savedUser = userRepository.save(user); | |
| Club club = fixtureMonkey.giveMeBuilder(Club.class) | |
| .set("id", 1L) | |
| .set("user", savedUser) | |
| .set("score", null) | |
| .set("clubMembers", null) | |
| .set("deletedAt", null) | |
| .sample(); | |
| clubRepository.save(club); | |
| Form form = fixtureMonkey.giveMeBuilder(Form.class) | |
| .set("title", "제목1") | |
| .set("club", club) | |
| .sample(); | |
| Form form2 = fixtureMonkey.giveMeBuilder(Form.class) | |
| .set("title", "제목2") | |
| .set("club", club) | |
| .set("club", savedClub) | |
| .sample(); | |
| Form savedForm = formRepository.saveAndFlush(form); | |
| FormApplication formApplication1 = FormApplication.builder() | |
| .name("지원자1") | |
| .studentNumber("60201115") | |
| .department("융합소프트웨어학부") | |
| .status(FormApplicationStatus.SUBMITTED) | |
| .form(savedForm) | |
| .build(); | |
| FormApplication formApplication2 = FormApplication.builder() | |
| .name("지원자2") | |
| .studentNumber("602011156") | |
| .department("디지털콘텐츠디자인학과") | |
| .status(FormApplicationStatus.SUBMITTED) | |
| .form(savedForm) | |
| .build(); | |
| FormApplication savedApplication1 = formApplicationService.create(formApplication1); | |
| FormApplication savedApplication2 = formApplicationService.create(formApplication2); | |
| List<Long> applicationIds = new ArrayList<>(); | |
| applicationIds.add(savedApplication1.getId()); | |
| applicationIds.add(savedApplication2.getId()); | |
| UpdateFormApplicationStatusCommand command = fixture.giveMeBuilder(UpdateFormApplicationStatusCommand.class) | |
| .set("formId", savedForm.getId()) | |
| .set("applicationIds", applicationIds) | |
| .set("status", FormApplicationStatus.FIRST_PASS) | |
| .set("user", savedUser) | |
| .sample(); | |
| formService.create(form); | |
| formService.create(form2); | |
| // when | |
| List<FormListQuery> queries = facadeCentralFormService.getAllMyForm(savedUser); | |
| facadeCentralFormApplicationService.updateStatus(command); | |
| // then | |
| assertThat(queries).isNotEmpty(); | |
| assertThat(queries.size()).isEqualTo(2); | |
| assertThat(queries.get(0).title()).isEqualTo("제목1"); | |
| assertThat(queries.get(1).title()).isEqualTo("제목2"); | |
| assertThat(formApplication1.getName()).isEqualTo("지원자1"); | |
| assertThat(formApplication1.getStatus()).isEqualTo(FormApplicationStatus.FIRST_PASS); | |
| assertThat(formApplication2.getName()).isEqualTo("지원자2"); | |
| assertThat(formApplication2.getStatus()).isEqualTo(FormApplicationStatus.FIRST_PASS); | |
| } | |
| @DisplayName("동아리는 지원자의 상태를 수정할 수 있다.") | |
| @Test | |
| void updateFormApplicationStatus() { | |
| // given | |
| User user = fixture.giveMeBuilder(User.class) | |
| .set("id", 1L) | |
| .set("Role", Role.CLUB) | |
| .set("deletedAt", null) | |
| .sample(); | |
| User savedUser = userRepository.saveAndFlush(user); | |
| Club club = fixture.giveMeBuilder(Club.class) | |
| .set("id", 1L) | |
| .set("user", savedUser) | |
| .set("score", null) | |
| .set("clubMembers", null) | |
| .set("deletedAt", null) | |
| .sample(); | |
| Club savedClub = clubRepository.saveAndFlush(club); | |
| Form form = fixture.giveMeBuilder(Form.class) | |
| .set("id", 1L) | |
| .set("title", "제목1") | |
| .set("club", savedClub) | |
| .sample(); | |
| Form savedForm = formRepository.saveAndFlush(form); | |
| FormApplication formApplication1 = FormApplication.builder() | |
| .name("지원자1") | |
| .studentNumber("60201115") | |
| .department("융합소프트웨어학부") | |
| .status(FormApplicationStatus.SUBMITTED) | |
| .form(savedForm) | |
| .build(); | |
| FormApplication formApplication2 = FormApplication.builder() | |
| .name("지원자2") | |
| .studentNumber("602011156") | |
| .department("디지털콘텐츠디자인학과") | |
| .status(FormApplicationStatus.SUBMITTED) | |
| .form(savedForm) | |
| .build(); | |
| FormApplication savedApplication1 = formApplicationService.create(formApplication1); | |
| FormApplication savedApplication2 = formApplicationService.create(formApplication2); | |
| List<Long> applicationIds = new ArrayList<>(); | |
| applicationIds.add(savedApplication1.getId()); | |
| applicationIds.add(savedApplication2.getId()); | |
| UpdateFormApplicationStatusCommand command = fixture.giveMeBuilder(UpdateFormApplicationStatusCommand.class) | |
| .set("formId", savedForm.getId()) | |
| .set("applicationIds", applicationIds) | |
| .set("status", FormApplicationStatus.FIRST_PASS) | |
| .set("user", savedUser) | |
| .sample(); | |
| // when | |
| facadeCentralFormApplicationService.updateStatus(command); | |
| // then | |
| - assertThat(formApplication1.getName()).isEqualTo("지원자1"); | |
| - assertThat(formApplication1.getStatus()).isEqualTo(FormApplicationStatus.FIRST_PASS); | |
| - assertThat(formApplication2.getName()).isEqualTo("지원자2"); | |
| - assertThat(formApplication2.getStatus()).isEqualTo(FormApplicationStatus.FIRST_PASS); | |
| + FormApplication updatedApplication1 = formApplicationService.findById(savedApplication1.getId()); | |
| + FormApplication updatedApplication2 = formApplicationService.findById(savedApplication2.getId()); | |
| + | |
| + assertThat(updatedApplication1.getStatus()).isEqualTo(FormApplicationStatus.FIRST_PASS); | |
| + assertThat(updatedApplication2.getStatus()).isEqualTo(FormApplicationStatus.FIRST_PASS); | |
| } |
KoSeonJe
reviewed
Feb 6, 2025
Comment on lines
16
to
19
| @NotNull(message = "지원자 상태는 필수 입력 사항입니다.") | ||
| @Schema(description = "수정할 지원자 상태", example = "FIRST_PASS") | ||
| FormApplicationStatus status | ||
| ) { |
There was a problem hiding this comment.
p3)
이것도 전에 말했던 것과 같이 String값으로 받아온뒤,
변환시키는 과정에서 해당 값이 유효한지 체크해주면 좋을 것 같아요
현재 제 API도 이렇게 되어있는데 오늘 수정해서 올리겠습니다
There was a problem hiding this comment.
참고해서 수정하였습니다! 감사합니다.
| public void updateStatus(UpdateFormApplicationStatusCommand command) { | ||
| List<FormApplication> formApplications = formApplicationService.getAllById(command.applicationIds()); | ||
| formApplications.forEach(formApplication -> formApplication.update(command.status())); | ||
| formApplicationRepository.saveAll(formApplications); |
There was a problem hiding this comment.
p2)
변경감지에 대해 공부해보시면 좋을 것 같아요!
update 시 save하지 않아도 괜찮아요!
https://velog.io/@hyojhand/JPA-Dirty-Checking%EB%B3%80%EA%B2%BD%EA%B0%90%EC%A7%80-DynamicUpdate
There was a problem hiding this comment.
카우 세션에서 배웠던 내용인데 잊어버렸네요...ㅎㅎ 자료까지 주셔서 감사합니다. 수정 완료했습니다!
5uhwann
reviewed
Feb 6, 2025
Comment on lines
45
to
47
| public void update(FormApplicationStatus status) { | ||
| this.status = status; | ||
| } |
There was a problem hiding this comment.
p3)
해당 메서드가 어떤 FormApplication의 무엇을 업데이트하는 메서드인지 명확하게 표현하는것이 좋을거 같습니다!
Suggested change
| public void update(FormApplicationStatus status) { | |
| this.status = status; | |
| } | |
| public void updateFormApplicationStatus (FormApplicationStatus status) { | |
| this.status = status; | |
| } |
There was a problem hiding this comment.
updateStatus()로 변경했습니다! 감사합니다!
| } | ||
|
|
||
| @Override | ||
| public FormApplicationQuery getFormApplication(Long formId, Long applicationId, User user) { |
There was a problem hiding this comment.
p3)
해당 메서드에서 user 파라미터는 사용하지 않는데 불필요하다면 제거해도 좋을거 같습니다!
There was a problem hiding this comment.
이것 또한 추후에 해당 폼이 로그인한 유저의 소유인지 유효성 검증을 위해서 필요하다고 생각했는데, 현재 필요가 없다면 삭제하는 것이 맞을까요?
Comment on lines
18
to
23
| public FormApplication toEntity(Form form) { | ||
| return FormApplication.builder() | ||
| .form(form) | ||
| .status(status) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
p3)
FacadeCentralFormApplicationServiceImpl의 updateStatus() 메서드에서 UpdateFormApplicationStatusCommand의 applicationIds, status 두개의 필드만 사용하고 있습니다!
불필요한 필드와 toEntity 메서드는 제거해도 좋을거 같습니다!
There was a problem hiding this comment.
toEntity 메서드 삭제 완료했습니다! 감사합니다.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FacadeCentralFormApplicationServiceImpl.java (1)
54-60:⚠️ Potential issue상태 업데이트 로직 개선이 필요합니다.
- 상태값 검증이 누락되어 있습니다.
- 존재하지 않는 지원서에 대한 예외 처리가 필요합니다.
- JPA의 변경 감지(dirty checking) 기능으로 인해
saveAll호출이 불필요합니다.다음과 같이 개선해주세요:
@Transactional @Override public void updateStatus(UpdateFormApplicationStatusCommand command) { + if (command.status() == null) { + throw new IllegalArgumentException("상태값은 null일 수 없습니다."); + } List<FormApplication> formApplications = formApplicationService.getAllById(command.applicationIds()); + if (formApplications.size() != command.applicationIds().size()) { + throw new IllegalArgumentException("존재하지 않는 지원서가 포함되어 있습니다."); + } formApplications.forEach(formApplication -> formApplication.update(command.status())); - formApplicationRepository.saveAll(formApplications); }
🧹 Nitpick comments (2)
src/main/java/ddingdong/ddingdongBE/domain/formapplication/entity/FormApplicationStatus.java (1)
13-18: 메서드 문서화 및 입력값 검증 추가 제안메서드 구현이 잘 되어 있지만, 다음 개선사항들을 고려해보시면 좋을 것 같습니다:
- 공개 메서드에 대한 JavaDoc 문서화 추가
- null 또는 빈 문자열 입력에 대한 검증 로직 추가
- 대소문자 구분 없는 비교 검토
다음과 같이 개선해보시는 건 어떨까요?:
+ /** + * 문자열을 FormApplicationStatus 열거형으로 변환합니다. + * + * @param status 변환할 상태 문자열 + * @return 찾은 FormApplicationStatus + * @throws InvalidatedEnumValue 상태를 찾을 수 없거나 입력이 유효하지 않은 경우 + */ public static FormApplicationStatus findStatus(String status) { + if (status == null || status.trim().isEmpty()) { + throw new InvalidatedEnumValue("상태 값이 비어있습니다."); + } + return Arrays.stream(values()) - .filter(findStatus -> findStatus.name().equals(status)) + .filter(findStatus -> findStatus.name().equalsIgnoreCase(status.trim())) .findFirst() .orElseThrow(() -> new InvalidatedEnumValue("FormApplicationStatus (status=" + status + ")를 찾을 수 없습니다.")); }src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FacadeCentralFormApplicationServiceImpl.java (1)
29-29: 사용하지 않는 의존성을 제거해주세요.
FormFieldService가 현재 클래스의 어떤 메서드에서도 사용되지 않고 있습니다. 불필요한 의존성은 제거하는 것이 좋습니다.- private final FormFieldService formFieldService;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/ddingdong/ddingdongBE/domain/formapplication/api/CentralFormApplicationApi.java(3 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/controller/dto/request/UpdateFormApplicationStatusRequest.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/entity/FormApplicationStatus.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FacadeCentralFormApplicationServiceImpl.java(3 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/GeneralFormApplicationService.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/GeneralFormApplicationService.java
- src/main/java/ddingdong/ddingdongBE/domain/formapplication/controller/dto/request/UpdateFormApplicationStatusRequest.java
- src/main/java/ddingdong/ddingdongBE/domain/formapplication/api/CentralFormApplicationApi.java
🔇 Additional comments (1)
src/main/java/ddingdong/ddingdongBE/domain/formapplication/entity/FormApplicationStatus.java (1)
3-5: 코드가 깔끔하고 잘 구성되어 있습니다!열거형 상수들이 명확하게 정의되어 있고, 필요한 import문들이 잘 구성되어 있습니다.
Also applies to: 6-11
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🚀 작업 내용
지원자 개인 상태 수정 API 구현하였습니다.
🤔 고민했던 내용
💬 리뷰 중점사항
Summary by CodeRabbit
신규 기능
리팩토링
테스트