-
Notifications
You must be signed in to change notification settings - Fork 0
feat: QnA 기능 구현 #25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: QnA 기능 구현 #25
Conversation
Uralauah
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다!
몇가지 의견이 있어서 남깁니다~
|
|
||
| @Override | ||
| @PostMapping("/{inquiryId}/answer") | ||
| @PreAuthorize("hasRole('ADMIN')") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PreAuthorize("hasRole('ADMIN')")이 어노테이션은 현재 불필요해 보입니다.
SecurityConfig에서 이미 아래와 같이 경로 기반으로 ADMIN 권한을 체크하고 있기 때문에
http
.authorizeHttpRequests()
.requestMatchers(ADMIN_ENDPOINTS).hasAnyRole(String.valueOf(Role.ADMIN))해당 API(/api/v1/admin/**)는 접근 자체가 ADMIN 권한 없이는 불가능한 상태입니다.
개인적으로 컨트롤러 메서드에 @PreAuthorize("hasRole('ADMIN')")를 중복으로 지정하는 건
불필요한 오버헤드라고 생각됩니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네 수정해두겠습니다!
| if(inquiry.getIsSolved()) | ||
| throw new CustomException(InquiryExceptionCode.ALREADY_SOLVED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 Inquiry 엔티티 내부에 메서드로 추출하는게 더 좋을 것 같습니다.
해결 여부 판단은 도메인의 책임에 가깝고 DDD 관점에서도 객체가 스스로 판단하도록 하는 게 자연스럽고 같은 검증 로직이 다른 곳에서도 필요해질 수 있으니 재사용 측면에서도 이점이 있을 것 같습니다.
//Inquiry.java
public void validateNotSolved() {
if (this.isSolved) throw new CustomException(ALREADY_SOLVED);
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
너무 맞는 말인 것 같네요 수정해두었습니다!
| private Boolean isSecret; | ||
|
|
||
| private Boolean isSolved = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그리고 이 부분은 제가 처음 설계에서 좀 잘못한 것 같은데
큰 문제는 없겠지만 Boolean보다는 boolean으로 하는게 더 안정적일 것 같네요
이 부분은 제가 나중에 한번에 다 수정하도록 하겠습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네 알겠습니다!
| @NotBlank(message = "답변 내용은 필수 입력 사항입니다.") | ||
| String content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이건 큰 부분은 아니지만 클라이언트가 요청에 사용하는 DTO에는 @Schema 어노테이션으로 Swagger 문서를 잘 정의해두면 프론트 입장에서 훨씬 개발이 수월해진다고 합니다.
백엔드 입장에서는 직관적으로 이해할 수 있을 거라 생각하지만 의외로 잘 파악하지 못하는 경우도 있는 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수정해두겠습니다! 공지사항 Req DTO에도 @Schema 어노테이션 추가해두었습니다.
jung-min-ju
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다!
| User getValidUser(Long userId){ | ||
| return userRepository.findById(userId) | ||
| .orElseThrow(() -> new CustomException(UserExceptionCode.INVALID_USERID)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 코드는 이미 UserService에 정의되어 있습니다! 차라리 Controller에서 UserService를 호출하여 writer를 찾고, InquiryService 의 인자로 해당 user 객체를 넘겨주는 것은 어떤가요?
추후 테스트 코드를 짠다면 해당 user 객체는 Mock으로 대체도 가능하기에 좀 더 편하게 테스트 할 수 있습니다. 또한 User의 정보를 찾는 로직이 UserService 내에서만 구현되어 있어 srp 관점에서도 올바른 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
맞는 말인 것 같네요 수정해두었습니다!
|
|
||
| private final UserRepository userRepository; | ||
|
|
||
| private final Integer INQUIRIES_PER_PAGE = 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 상수 변수의 static이 빠진 것 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다!! 코드 깔끔한 것 같습니다👍👍 사소한 것들에 대해 코멘트 달아놨습니다.
QnA 기능 구현
📝 개요
Inquiry, Answer 엔터티를 사용하여 QnA 기능을 구현하였습니다.
⚙️ 구현 내용
관리자 권한이 필요한 답변(Answer) 작성 기능은
/admin/qnas엔드포인트에,관리자 권한이 필요하지 않은 QnA 작성, 조회, 수정, 삭제 기능은
/qnas엔드포인트에 구현하였습니다.📎 기타
Inquiry 단건 조회 시 아래와 같이 Answer 엔터티와 함께 조회되도록 작성하였습니다.
이 때, 답변이 아직 작성되지 않은 경우 Answer == null 이므로 아래와 같이 null로 반환되도록 작성했습니다.
답변 작성 메소드 호출 시 아래와 같이 해당 답변이 작성될 질문(Inquiry)과 연관관계를 연결해주도록 작성하였습니다.
이 때, AnswerService에서 InquiryId 유효성 검증이 필요했는데, 이를 위해 아래와 같이 InquiryService의 검증 코드를 재사용했습니다.
또한 QnA에 대한 답변 작성은 한 번만 작성될 수 있도록, isSolved = true면 다시 작성할 수 없도록 하였습니다. 하지만 수정은 가능하게 하는게 어떤지 싶습니다. (노션 API 명세서에는 QnA 답변 수정 API가 작성되어있지 않음)
🧪 테스트 결과
QnA 생성 성공
QnA 생성 실패(입력값 검증)
QnA 목록 조회 성공
QnA 목록 조회 성공(isSolved = true)
QnA 목록 조회 성공(sort = DATE_ASC)
QnA 목록 조회 성공(keyword = "게임")
QnA 단건 조회 성공
QnA 단건 조회 실패(존재하지 않는 QnA)
내가 작성한 QnA 조회
QnA 수정 성공
QnA 수정 후 다시 조회
QnA 수정 실패(필드 검증 오류)
QnA 수정 실패(작성자 불일치)
QnA 수정 실패(존재하지 않는 QnA)
QnA 삭제 성공
QnA 삭제 실패(작성자 불일치)
QnA 삭제 실패(존재하지 않는 QnA)
답변 작성 성공
답변 작성 실패(관리자 권한 없음)
답변 작성 실패(이미 작성된 답변)