-
Notifications
You must be signed in to change notification settings - Fork 0
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
✨ create comment on recipe #167
Conversation
|
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.
아토 고생 많으셨어요!👏👏
import jakarta.validation.constraints.Min; | ||
import jakarta.validation.constraints.NotBlank; | ||
|
||
public record CreateCommentRequest(@Min(1) long recipeId, @NotBlank String message) { |
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.
일관성을 위해 recipeId 검증에 대한 논의가 있으면 좋을 것 같아요!
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.
모든 Request 필드에 @min(1) 정의해주기 vs 기존 로직에서 존재하지 않는 id 값이라고 알려주는걸로 처리하기
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.
그러네요 이미 터지네요
후자가 좋다고 생각합니다
지울게요!
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.
엇 일찍 끝냈는데 리뷰 반영이 느렸네요! ㅠㅠ
여럿 테스트 다 깔끔하게 잘 진행된 것 같아요~ 굿굿!
@@ -22,4 +28,10 @@ public class CommentController { | |||
public List<CommentOfRecipeResponse> readComments(@PathVariable long recipeId, @LoginUser UserInfo userInfo) { | |||
return commentService.readComments(recipeId, userInfo); | |||
} | |||
|
|||
@PostMapping | |||
@ResponseStatus(HttpStatus.CREATED) |
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.
응답 상태코드를 CREATED
로 정의하셨다면 location
헤더를 추가해보는 건 어떠신가요>?
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.
나중에 고려할 필요성이 생길 수 있겠네요!
좋은 의견입니닷
참고하고 있다가 필요해지면 수정하겠습니다!
.body(request) | ||
.when().post("api/comments") | ||
.then().log().all() | ||
.statusCode(201); |
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.
마찬가지로 여기에서 헤더 검증을 할 수 있겠네요!
- move validation position - make and use custom exception
|
레시피에 댓글을 등록합니다!