[DDING-112] 지원자 메모하기 API 구현 #262
Conversation
Seooooo24
requested review from
wonjunYou,
5uhwann and
KoSeonJe
as code owners
Walkthrough이 PR은 폼 신청과 관련된 노트 업데이트 기능을 추가합니다. API 인터페이스와 컨트롤러에 Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant Service
participant Entity
Client->>Controller: updateFormApplicationNote(formId, applicationId, principal, request)
Controller->>Service: updateNote(command)
Service->>Entity: updateNote(note)
Entity-->>Service: 업데이트 결과 반환
Service-->>Controller: 성공 응답 반환
Controller-->>Client: 응답 전달
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: 2
🧹 Nitpick comments (5)
src/main/java/ddingdong/ddingdongBE/domain/formapplication/entity/FormApplication.java (1)
44-55: 생성자에 note 필드 초기화 로직 추가가 필요합니다.Builder 패턴을 사용하는 생성자에 note 필드도 포함시켜 일관된 객체 생성이 가능하도록 하는 것이 좋습니다.
@Builder private FormApplication(String name, String studentNumber, String department, String phoneNumber, String email, - FormApplicationStatus status, Form form) { + FormApplicationStatus status, Form form, String note) { this.name = name; this.studentNumber = studentNumber; this.department = department; this.phoneNumber = phoneNumber; this.email = email; this.status = status; this.form = form; + this.note = note; }src/main/java/ddingdong/ddingdongBE/domain/formapplication/controller/CentralFormApplicationController.java (1)
47-52: 메서드 구현이 잘 되었습니다만, 추가적인 검증이 필요할 수 있습니다.사용자 권한 검증은 잘 되어 있지만, 폼과 지원서의 존재 여부 확인이 서비스 레이어에 위임되어 있습니다. 이는 괜찮은 접근이지만, 명시적으로 문서화하면 좋을 것 같습니다.
다음과 같이 메서드에 주석을 추가하는 것을 고려해보세요:
+ /** + * 지원서 메모를 업데이트합니다. + * @throws EntityNotFoundException 폼 또는 지원서가 존재하지 않는 경우 + * @throws UnauthorizedException 사용자가 권한이 없는 경우 + */ @Override public void updateFormApplicationNote(Long formId, Long applicationId, PrincipalDetails principalDetails, UpdateFormApplicationNoteRequest request) {src/main/java/ddingdong/ddingdongBE/domain/formapplication/api/CentralFormApplicationApi.java (1)
56-66: API 정의가 잘 되어 있습니다.OpenAPI 어노테이션을 통한 문서화가 잘 되어 있으며, RESTful 규칙을 잘 따르고 있습니다.
API 문서화를 더욱 상세하게 하면 좋을 것 같습니다:
@Operation(summary = "지원자 메모하기 API") + @Operation( + summary = "지원자 메모하기 API", + description = "지원서에 대한 메모를 추가하거나 수정합니다. 메모는 관리자만 작성할 수 있습니다." + ) @ApiResponse(responseCode = "204", description = "지원자 메모하기 성공") + @ApiResponse(responseCode = "404", description = "지원서를 찾을 수 없음") + @ApiResponse(responseCode = "403", description = "권한 없음")src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/dto/query/FormApplicationQuery.java (1)
25-25: note 필드의 null 안전성을 고려해보세요.현재 구현에서 note 필드가 null일 수 있는데, 이에 대한 명시적인 처리가 없습니다.
다음과 같은 개선을 고려해보세요:
- String note, + @Schema(nullable = true) + String note, - .note(formApplication.getNote()) + .note(Optional.ofNullable(formApplication.getNote()).orElse(""))Also applies to: 79-79
src/main/java/ddingdong/ddingdongBE/domain/formapplication/controller/dto/response/FormApplicationResponse.java (1)
33-34: 메모 필드의 문서화가 잘 되어있습니다.Schema 어노테이션을 통한 문서화가 잘 되어 있습니다.
예시 값을 다른 필드들과 일관성 있게 작성하면 좋을 것 같습니다:
- @Schema(description = "메모", example = "좋은 지원자였습니다.") + @Schema(description = "지원자에 대한 메모", example = "적극적이고 열정이 있는 지원자")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/main/java/ddingdong/ddingdongBE/domain/formapplication/api/CentralFormApplicationApi.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/controller/CentralFormApplicationController.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/controller/dto/request/UpdateFormApplicationNoteRequest.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/controller/dto/response/FormApplicationResponse.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/entity/FormApplication.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FacadeCentralFormApplicationService.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FacadeCentralFormApplicationServiceImpl.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/dto/command/UpdateFormApplicationNoteCommand.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/dto/query/FormApplicationQuery.java(2 hunks)src/main/resources/db/migration/V40_alter_table_form_application_add_column.sql(1 hunks)
⏰ 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/dto/command/UpdateFormApplicationNoteCommand.java (1)
6-14: 구조가 깔끔하고 적절합니다!커맨드 패턴을 잘 활용하였고, 필요한 모든 필드가 포함되어 있습니다.
@Builder어노테이션의 사용으로 객체 생성이 용이합니다.src/main/java/ddingdong/ddingdongBE/domain/formapplication/controller/dto/request/UpdateFormApplicationNoteRequest.java (1)
8-21: 검증과 문서화가 잘 되어있습니다!
- 한글 검증 메시지를 사용한 것이 좋습니다.
- Swagger 문서화를 위한 예시가 포함되어 있습니다.
- Command 변환 메서드가 깔끔하게 구현되어 있습니다.
src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/FacadeCentralFormApplicationService.java (1)
16-17: 인터페이스 확장이 일관성 있게 이루어졌습니다!기존 메서드들과 동일한 패턴을 따르고 있으며, 메서드 시그니처가 적절합니다.
src/main/java/ddingdong/ddingdongBE/domain/formapplication/entity/FormApplication.java (1)
61-63: 메모 업데이트 메서드가 적절히 구현되었습니다!간단하고 명확한 구현입니다.
src/main/resources/db/migration/V40_alter_table_form_application_add_column.sql (1)
1-2: 마이그레이션 스크립트가 적절히 구현되었습니다.컬럼 정의가 적절합니다:
- VARCHAR(500)은 노트 필드에 적합한 크기입니다
- NULL 허용은 PR 설명에서 언급된 요구사항과 일치합니다
노트 필드의 NULL 허용 여부가 비즈니스 요구사항과 일치하는지 확인해주시기 바랍니다. 현재 구현은 NULL을 허용하고 있으며, 이는 지원자에 대한 메모가 선택적임을 의미합니다.
| @@ -39,6 +39,8 @@ public class FormApplication extends BaseEntity { | |||
| @ManyToOne(fetch = FetchType.LAZY) | |||
| private Form form; | |||
|
|
|||
| private String note; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
note 필드에 제약 조건 추가가 필요합니다.
데이터베이스 컬럼 크기 제한과 일관성을 위해 @Column 어노테이션에 length 속성을 추가하는 것이 좋습니다.
- private String note;
+ @Column(length = 1000)
+ private String note;📝 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 String note; | |
| @Column(length = 1000) | |
| private String note; |
| @Transactional | ||
| @Override | ||
| public void updateNote(UpdateFormApplicationNoteCommand command) { | ||
| FormApplication formApplication = formApplicationService.getById(command.applicationId()); | ||
| formApplication.updateNote(command.note()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
노트 업데이트 메서드의 유효성 검사 추가 필요
메서드의 기본 구현은 적절하나, 다음 사항들을 고려해 주시기 바랍니다:
- 노트 내용의 길이 제한 (500자) 검증
- 노트 내용의 XSS 방지를 위한 문자열 sanitization
- 존재하지 않는 applicationId에 대한 명확한 예외 처리
다음과 같이 수정하는 것을 제안드립니다:
@Transactional
@Override
public void updateNote(UpdateFormApplicationNoteCommand command) {
+ if (command.note() != null && command.note().length() > 500) {
+ throw new IllegalArgumentException("노트는 500자를 초과할 수 없습니다.");
+ }
+
+ String sanitizedNote = HtmlUtils.htmlEscape(command.note());
FormApplication formApplication = formApplicationService.getById(command.applicationId());
- formApplication.updateNote(command.note());
+ formApplication.updateNote(sanitizedNote);
}📝 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.
| @Transactional | |
| @Override | |
| public void updateNote(UpdateFormApplicationNoteCommand command) { | |
| FormApplication formApplication = formApplicationService.getById(command.applicationId()); | |
| formApplication.updateNote(command.note()); | |
| } | |
| @Transactional | |
| @Override | |
| public void updateNote(UpdateFormApplicationNoteCommand command) { | |
| if (command.note() != null && command.note().length() > 500) { | |
| throw new IllegalArgumentException("노트는 500자를 초과할 수 없습니다."); | |
| } | |
| String sanitizedNote = HtmlUtils.htmlEscape(command.note()); | |
| FormApplication formApplication = formApplicationService.getById(command.applicationId()); | |
| formApplication.updateNote(sanitizedNote); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/ddingdong/ddingdongBE/domain/formapplication/controller/CentralFormApplicationController.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/controller/dto/request/UpdateFormApplicationNoteRequest.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/service/dto/command/UpdateFormApplicationNoteCommand.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/ddingdong/ddingdongBE/domain/formapplication/controller/dto/request/UpdateFormApplicationNoteRequest.java
⏰ 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/dto/command/UpdateFormApplicationNoteCommand.java (1)
1-4: 패키지 구조가 잘 구성되어 있습니다!Command 패턴에 맞게 적절한 패키지 구조를 가지고 있으며, 필요한 Lombok 어노테이션만 임포트하고 있습니다.
src/main/java/ddingdong/ddingdongBE/domain/formapplication/controller/CentralFormApplicationController.java (1)
4-4: 새로운 DTO import가 적절히 추가되었습니다!UpdateFormApplicationNoteRequest DTO의 import가 다른 request DTO들과 함께 잘 배치되어 있습니다.
| @Builder | ||
| public record UpdateFormApplicationNoteCommand ( | ||
| Long applicationId, | ||
| String note | ||
| ){ | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
필드 유효성 검증 추가를 고려해보세요.
PR 설명에서 언급된 note 필드의 null 값 처리에 대한 우려와 관련하여, 다음과 같은 개선사항을 제안드립니다:
applicationId에@NotNull어노테이션 추가note필드에 대한 명확한 제약조건 정의
다음과 같이 수정해보세요:
+import jakarta.validation.constraints.NotNull;
+
@Builder
public record UpdateFormApplicationNoteCommand (
+ @NotNull(message = "신청서 ID는 필수입니다")
Long applicationId,
String note
){
}note 필드의 경우:
- null을 허용하려면 현재 상태를 유지
- null을 허용하지 않으려면
@NotNull추가 - 빈 문자열을 허용하지 않으려면
@NotBlank추가 - 길이 제한이 필요하다면
@Size(max = XXX)추가
비즈니스 요구사항에 맞는 제약조건을 선택해주세요.
📝 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.
| @Builder | |
| public record UpdateFormApplicationNoteCommand ( | |
| Long applicationId, | |
| String note | |
| ){ | |
| } | |
| import jakarta.validation.constraints.NotNull; | |
| @Builder | |
| public record UpdateFormApplicationNoteCommand ( | |
| @NotNull(message = "신청서 ID는 필수입니다") | |
| Long applicationId, | |
| String note | |
| ) { | |
| } |
| @Override | ||
| public void updateFormApplicationNote(Long formId, Long applicationId, | ||
| PrincipalDetails principalDetails, UpdateFormApplicationNoteRequest request) { | ||
| facadeCentralFormApplicationService.updateNote(request.toCommand(applicationId)); | ||
| } |
There was a problem hiding this comment.
메서드 구현에 몇 가지 개선이 필요합니다.
다음 사항들을 검토해 주시기 바랍니다:
- formId 파라미터가 사용되지 않고 있습니다. 필요하지 않다면 제거하거나, 필요하다면 검증 로직을 추가해 주세요.
- 다른 메서드들과 달리 사용자 인증 검증(User user = principalDetails.getUser())이 누락되었습니다.
- 요청 파라미터에 대한 유효성 검사가 없습니다.
- 예외 처리가 누락되었습니다.
다음과 같이 수정하는 것을 제안드립니다:
@Override
public void updateFormApplicationNote(Long formId, Long applicationId,
PrincipalDetails principalDetails, UpdateFormApplicationNoteRequest request) {
+ User user = principalDetails.getUser();
+ // formId에 대한 검증 로직 추가
+ facadeCentralFormApplicationService.validateFormAccess(formId, user);
+ // request 유효성 검사
+ if (request == null || applicationId == null) {
+ throw new IllegalArgumentException("Invalid request parameters");
+ }
facadeCentralFormApplicationService.updateNote(request.toCommand(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.
| @Override | |
| public void updateFormApplicationNote(Long formId, Long applicationId, | |
| PrincipalDetails principalDetails, UpdateFormApplicationNoteRequest request) { | |
| facadeCentralFormApplicationService.updateNote(request.toCommand(applicationId)); | |
| } | |
| @Override | |
| public void updateFormApplicationNote(Long formId, Long applicationId, | |
| PrincipalDetails principalDetails, UpdateFormApplicationNoteRequest request) { | |
| User user = principalDetails.getUser(); | |
| // formId에 대한 검증 로직 추가 | |
| facadeCentralFormApplicationService.validateFormAccess(formId, user); | |
| // request 유효성 검사 | |
| if (request == null || applicationId == null) { | |
| throw new IllegalArgumentException("Invalid request parameters"); | |
| } | |
| facadeCentralFormApplicationService.updateNote(request.toCommand(applicationId)); | |
| } |
|
🚀 작업 내용
지원자 메모하기 API 구현하였습니다.
지원자 상세조회 API 응답 dto에 note 필드를 추가하였습니다.
🤔 고민했던 내용
note 컬럼에 기본값을 지정해주지 않고 null로 둬도 괜찮을지 궁금합니다.
💬 리뷰 중점사항
Summary by CodeRabbit