-
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
feat: 투표와 투표 취소 API 생성 #57
Conversation
@@ -20,7 +22,7 @@ public class Vote extends BaseEntity { | |||
|
|||
@ManyToOne(fetch = FetchType.LAZY) | |||
@JoinColumn(name = "member_id") | |||
private Member member; | |||
private Member voter; |
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.
voter 로 네이밍 변경이 좋은 것 같네요 :)
|
||
private void deleteVote(Vote vote) { | ||
vote.removeAssociations(); | ||
voteRepository.delete(vote); |
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.
Vote
엔티티를 연관관계 cascade를 통한 저장/삭제가 아니라 (remove는 켜져있네요)
VoteRepository
를 통해서 저장/삭제한 이유는 따로 있나요??
방식이 여러가지라 궁금해서 여쭤봅니다
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.
Topic, Member에 걸려있는 cascade REMOVE의 의미는 토픽과 멤버가 삭제될때 연관된 투표를 삭제하는 거라 cascade만으로는 투표를 삭제할 수 없고,
다른 방법으로는 orphanRemoval을 통해서 association이 다 삭제가 되면 삭제되도록 할 수 있는데, 만약 하나라도 association이 삭제 안된게 있으면 delete가 안되더라구요. 지금은 다 참조가 삭제돼서 delete가 없어도 삭제되지만, 명시적으로 삭제해놓는 게 나을 것 같아서 삭제했습니다!
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 void cancelVote(final Member member, final Topic topic) { | ||
if (member.votedAlready(topic)) { | ||
member.cancelVote(topic); | ||
private static void checkTopicVotable(final Topic topic, final Member member, final LocalDateTime votedAt) { |
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.
이런 방식도 있을 것 같아요! Vote
엔티티에게 validation을 위임하는 형태요
예를 들면 (연관관계는 Topic
만으로 적을게요)
public Vote(Topic topic, LocalDateTime votedAt) {
if (!topic.isBeforeDeadline(votedAt)) {
throw new UnableToVoteException(votedAt);
}
---
this.topic = topic;
}
이런 방식도 고려해도 좋을 것 같아요~~
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.
생성자에서 throw하는게 별로 좋지않은 형태같아서 이렇게 했었는데, 투표 자체가 생성되면 안되는 형태니까 말씀해주신 것도 좋을 것 같네요! 다음 PR에 수정하겠습니다~!
리뷰 확인해주시고 굳이 수정할 필요는 없을 것 같네요! 문제 없으면 머지해주세요 |
What is this PR? 🔍
Changes 📝
To Reviewers 📢
1. API 명세서 관련
#56 Gitbook에 에러 메시지 정리하고 그대로 개발할것
그리고 API 명세서 좀더 다듬어주시면 좋을 것 같습니다!
~~Controller
→~~ API
)이런 식으로 더 보기 좋게 변경2. 투표 시 시간은 클라이언트에서 지정해서 전송
사용자가 투표 시에 투표 시간이 deadline보다 전이어야 하는데, 이 때 투표 시간을
두 가지 중에 하나로 할 수 있는데, 서버 기준으로 하면 요청이 밀릴 수도 있으니 클라에서 선택된 시간 기준으로 하는게 사용자가 가장 이상함을 못 느낄 것 같아 클라이언트에서 요청 시 body에 요청 시간을 포함하기로 했습니다. 그래서 서버에선 최소한의 validation으로 현재 서버 시간보다 미래 시간이면 예외 처리 하기로 했습니다.
votedAt
, 투표 취소 시canceledAt
참고