-
Notifications
You must be signed in to change notification settings - Fork 6
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
[BE] Feat/#587 핀 댓글 기능 추가 구현 #601
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.
고생했어요 메주
핸드폰으로 보니까 힘드네여
그래서 테스트 코드는 안 봤습니다.
한 가지 건의드리고 싶은게 있는데요.
Cascade로 안 지워져서 onDelete를 사용하셨다고 말씀 하신 부분이 이해가 잘 안 되는데, cascade를 썼을때 어떤 방식으로 코드를 작성하셨나요 ?
납득이 잘 안 되서..
throw new PinCommentForbiddenException(FORBIDDEN_PIN_COMMENT_CREATE); | ||
} | ||
|
||
private PinComment findParentPinComment(Long parentPinComment) { |
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.
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.
반영 완!
private PinComment findParentPinComment(Long parentPinComment) { | ||
if (Objects.isNull(parentPinComment)) { | ||
return null; | ||
} | ||
|
||
return pinCommentRepository.findById(parentPinComment) | ||
.orElseThrow(() -> new PinCommentNotFoundException(PIN_COMMENT_NOT_FOUND, parentPinComment)); | ||
} |
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.
Null값을 그대로 넘겨주기보단, 상황에 따라 다르게 초기화된 pinComment를 넘겨주는건 어떤가요 ? 생성자에 null넣기 좀 그러면, 정팩메로 분리해도 되구요
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.
반영을 완료하긴 했지만 쥬니가 말씀하신 방법이 이게 맞는지는 모르겠어요! 한번 확인 부탁드려용!
public void deletePinComment(AuthMember member, Long pinCommentId) { | ||
PinComment pinComment = findPinComment(pinCommentId); | ||
|
||
validatePinCommentDelete(member, pinComment); |
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.
ValidatePinCommentUpdate를 재사용하면 안 되는건가요 ?
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.
예외 내에 들어가는 Error Code가 달라서 이렇게 하긴 했어요! Error Code를 받아서 처리하는 방식으로 할까요?
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.
근데 이거와 별개로 생각해보니까 canPinCommentCreate 를 기존 코드의 canRead 로 바꿀 수 있더라고요 허허허 이것도 같이 바꿀게용
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.
아니네 허허허 canRead 로 하면 Guest 를 못거르네 허허허
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.
아니네 안거르네
@@ -56,4 +62,35 @@ public List<PinResponse> findAllPinsByMemberId(AuthMember authMember, Long membe | |||
.map(PinResponse::from) | |||
.toList(); | |||
} | |||
|
|||
public List<PinCommentResponse> findAllPinCommentByPinId(AuthMember member, Long pinId) { |
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.
findAllPinComment's'
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.
껄껄껄껄
.orElseThrow(() -> new PinNotFoundException(PIN_NOT_FOUND, pinId)); | ||
} | ||
|
||
private PinCommentResponse pinCommentToResponse(AuthMember member, PinComment pinComment) { |
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.
ㅋㅋㅋㅋㅋ 어떻게 할 수 있을까요??
public ResponseEntity<Void> addPinComment(AuthMember member, @RequestBody PinCommentCreateRequest request) { | ||
Long commentId = pinCommandService.savePinComment(member, request); | ||
|
||
return ResponseEntity.created(URI.create("/pins/comments/" + commentId)) |
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.
어떤 핀의 댓글인지는 몰라도 될까요 ?
ex) pins/pinId/comments/commentId
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.
PinCommentId 가 있으면 따로 pinId 는 필요 없을거라 생각했어요! 쥬니는 필요할 것 같은가용?)
.build(); | ||
} | ||
|
||
@GetMapping("/comments/{pinId}") |
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.
pins/pinId/comments
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.
PinId는 pins 바로 뒤에 오는게 rest 한거군요
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.
반영 완료!
@ManyToOne(cascade = CascadeType.REMOVE)
@JoinColumn(name = "parent_pin_comment_id")
private PinComment parentPinComment;
잘못 적용하거나, 혹은 제가 잘못 테스트 한 것일까요? |
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.
빠르게 잘 구현해주셨네요!!! 바쁜 중에 기능 구현 수고 많으셨습니다 !
빠른 확인을 위해 변경 요청 위주로 코멘트 달았습니다.
P2, P3 위주로 확인해주셔요!
) { | ||
this.pinRepository = pinRepository; | ||
this.locationRepository = locationRepository; | ||
this.topicRepository = topicRepository; | ||
this.memberRepository = memberRepository; | ||
this.pinImageRepository = pinImageRepository; | ||
this.imageService = imageService; | ||
this.pinCommentRepository = pinCommentRepository; | ||
} |
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.
P3. 필드 순서, 생성자 파라미터 순서에서 Service와 Repository를 분류하면 어떨까요?
Repository가 다음에 추가될 확률이 더 높으니 ImageService를 첫번째 파라미터, 필드로 올려도 좋을 것 같아요.
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.
반영하겠습니다~!
public PinComment( | ||
Pin pin, | ||
PinComment parentPinComment, | ||
Member creator, | ||
String content | ||
) { | ||
this.pin = pin; | ||
this.parentPinComment = parentPinComment; | ||
this.creator = creator; | ||
this.content = 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.
P3. 정팩메도 쓰고 기본 생성자도 쓰는 걸까요 ?!
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 으로 안바꿨군용
public static PinComment of( | ||
Pin pin, | ||
PinComment parentPinComment, | ||
Member creator, | ||
String content | ||
) { | ||
validateContent(content); | ||
|
||
return new PinComment(pin, parentPinComment, creator, 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.
P2.
혹시 제가 잘못 생각하고 있으면 알려주시면 감사하겠습니다!
- parentPinComment인 PinComment가 ParentPinComment를 가지는 경우도 허용하나요? (대대댓글)
- parentPinComment와 현재 PinComment가 서로를 참조하게 되는 경우도 검증할 필요가 있지 않을까요? 오버플로우 나지 않으려나용 (ex. id가 1인 pinComment의 parentPinCommentId가 1)
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.
1번은 맞습니다!
현재는 대대댓글이 가능하고, 웹에서는 아이크와 합의하에 이 부분을 막고있긴합니다.
빠른 구현을 위해 넘어가고 있었는데 이 부분을 막아야하겠죠?
2번도 고려해서 반영하겠습니당
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.
오우 2번은 발생하지 않는 상황인 것 같네요!
public boolean canPinCommentCreate(Topic topic) { | ||
Publicity publicity = topic.getPublicity(); | ||
|
||
return publicity == Publicity.PUBLIC || isGroup(topic.getId()); |
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.
P4. 이건 이번 PR 해당하는 내용만은 아닌데 제가 코드 다시 보니까 이해가 안가서 질문드립니당..
isGroup 이랑 hasPermission이랑 어떻게 다른건가요..???
createdTopic.contains(topicId)랑 isCreator(topicId)랑 같은거 아닌가염?
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.
코드 다시 보니까 그렇네요 허허허허허
publicity == Publicity.PUBLIC -> publicity.isPublic()
으로 바꾸는 것과
도이가 말씀해주신 부분들 수정해서 올리도록 할게요!
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.
isGroup -> hasPermission 일괄적으로 적용했습니다.
public static PinCommentResponse ofParentComment(PinComment pinComment, boolean canChange) { | ||
MemberInfo memberInfo = pinComment.getCreator().getMemberInfo(); | ||
|
||
return new PinCommentResponse( | ||
pinComment.getId(), | ||
pinComment.getContent(), | ||
memberInfo.getNickName(), | ||
memberInfo.getImageUrl(), | ||
null, | ||
canChange, | ||
pinComment.getUpdatedAt() | ||
); | ||
} | ||
|
||
public static PinCommentResponse ofChildComment(PinComment pinComment, boolean canChange) { | ||
MemberInfo memberInfo = pinComment.getCreator().getMemberInfo(); | ||
|
||
return new PinCommentResponse( | ||
pinComment.getId(), | ||
pinComment.getContent(), | ||
memberInfo.getNickName(), | ||
memberInfo.getImageUrl(), | ||
pinComment.getParentPinComment().getId(), | ||
canChange, | ||
pinComment.getUpdatedAt() | ||
); | ||
} |
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.
P3.분기로 인한 중복 코드를 줄이기 위해
Optional getParentPinCommentId();를 정의하고 한 메서드에서 처리하는 건 별로일까요 ?
public static PinCommentResponse of(PinComment pinComment, boolean canChange) {
MemberInfo memberInfo = pinComment.getCreator().getMemberInfo();
Optional<Long> parentCommentId = pinComment.getParentCommentId();
return new PinCommentResponse(
pinComment.getId(),
pinComment.getContent(),
memberInfo.getNickName(),
memberInfo.getImageUrl(),
parentCommentId.orElse(null),
canChange,
pinComment.getUpdatedAt()
);
}
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.
조금 더 의미를 명확하게 하고 싶어서 위와 같이 parentComment, childComment 로 해주긴 했는데, 도이 말씀대로 하는게 나을 것 같습니다 반영할게용~~
} | ||
|
||
@ParameterizedTest | ||
@MethodSource("publicAndPrivateTopicsStatus") |
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.
구욷
|
||
@ParameterizedTest | ||
@MethodSource("publicAndPrivateTopicsStatus") | ||
@DisplayName("Topic 이 PUBLIC 인 경우와 PRIVATE 이면서 Permission 을 가진 일반 유저가 핀 댓글을 조회에 성공한다.") |
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.
P3. 하지만 유저
는 회원으로 통일하는거 어떨까유 그리고 저는 DisplayName이 쪼끔 헷갈렸어요!
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.
하지만 유저
} | ||
|
||
@ParameterizedTest | ||
@MethodSource("validCommentContent") |
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.
MethodSource 야무지게 잘 활용하시는군요 👍
|
||
@ParameterizedTest | ||
@MethodSource("invalidCommentContent") | ||
@DisplayName("Pin 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.
P4. 유효하지 않은 댓글 내용인 경우
를 붙이는 거어떤가유
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.
반영하겠슴돠
) { | ||
return Topic.createTopicAssociatedWithCreator( | ||
"토픽", | ||
"토픽의 Publicity, PermissionType 이 동적으로 정해집니다.", |
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.
친절하군요
2번은 정확히 모르겠네요 훈련받으면서 생각한거라.. |
현재 댓글을 hard delete 가 아닌 soft delete 로 구현했기 때문에 모두 삭제되는 것이 맞다고 판단했어요!
저도 이게 너무 헷갈리더라구요. 근데 이게 사실 논리적으로 이상하다고 생각이 들어, 위와 같이 판단했었어용 쥬니짱 |
저는 상위 댓글이 지워지면, 해당 댓글의 content만 블러처리되고 하위 댓글은 그대로 남아있는 경우가 더 친숙한거 같아서 1번 얘길 한거였어요. 만약 함께 삭제할거라면, comment 객체에 oneToMany로 자식들 관리해주는게 자연스러울 것 같아요. 대댓글이 너무 많아지면, 바로 댓글을 다 보여주는게 아니라 대댓글 개수만 보여주고, 더보기(?) 누르면 getChildComments로 가져오는식 ? Casecade를 확인해보고싶은데 지금 테스트 못해보는게 답답하네열 ㅠㅠ |
맞아요 저도 그게 더 친숙한 것 같긴해요! 처음에, 저도 쥬니가 말씀해주신 방식으로 빠르게 구현하느라 너무 킹체지향적으로 생각하지 않은 것 같기도 합니다. 그리고 ManyToOne Cascade 안되는 이유는 찾은 것 같아요! 쥬니짱 그리고... 오늘은 동대장한테 혼나면 안돼용~~! |
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.
쥬니랑 도이가 리뷰를 잘 남겨주셨네요 👍👍
중간에 마지막 줄 개행이 되지 않아 마크된 부분만 의견 남겨놨습니다.
다른 분들 피드백 반영할 때 한번 확인 부탁드려요~
return 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.
POSIX!
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.
오 덕분에 용어 알아갑니다
return true; | ||
} | ||
|
||
} |
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.
POSIX!
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.
수고하셨습니다 !!!
기존에 프론트에서 직접 요청 보내보면서 작업하셨던 것 같아서,
API URI 바뀐 거 프론트에도 공유된 걸로 알고있을게용~
) { | ||
validatePinCommentDepth(parentPinComment); | ||
validateContent(content); | ||
|
||
|
||
return new PinComment(pin, parentPinComment, creator, content); | ||
} | ||
|
||
private static void validatePinCommentDepth(PinComment parentPinComment) { | ||
if (parentPinComment.isParentComment()) { | ||
return; | ||
} | ||
|
||
throw new PinCommentBadRequestException(ILLEGAL_PIN_COMMENT_DEPTH); | ||
} | ||
|
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.
👍👍
작업 대상
사용성 개선에 있어 이전부터 계속 얘기가 나왔던
댓글 기능
을 간단하게 구현하였습니다.📄 작업 내용
보이는 바와 같이 기본적인 댓글
CRUD
기능을 구현하였습니다.🙋🏻 주의 사항
주의할 사항
이라기 보다는PR
을 보시기 이전에 미리 봐주셨으면 하는 내용들이니 편하게 봐주시면 감사할 것 같아용~너무 빈약한 Comment 구조
현재
Comment
는 아래와 같은 구조를 가지고 있습니다.여기서 설명을 드릴법한 부분은
parentPinComment
라고 생각이 들어, 무엇을 의미하는지와 해당 방식으로 구현한 이유를 간단하게 설명드리겠습니다.해당 컬럼은 같은
PinComment
를 참조하는데요.이는 본인의
부모 댓글
을 의미합니다.즉
대댓글
인 경우에만 해당 컬럼의 값이 들어있어,@JoinColumn nullable
기본 설정인true
를 유지하였습니다.그리고, 해당 방식으로 구현한 이유는 단지 빠른 구현을 위해
아이크
가 이미 구현해놓은 코드에 맞춰 설계했기 때문입니다.근데 이렇게
Entity
를 설계하고나니,댓글
,대댓글
을 입력받는 것이 굉장히 쉬워졌습니다.parentPinComment
부분을null
로 받으면댓글
,null
로 받지 않고pinComment
를 받으면대댓글
인 것이기 때문입니다.아 또 마지막으로
부모 댓글
이 지워졌을 때, 그에 따른자식 댓글
들도 모두 지우기 위해@OnDelete
어노테이션을 붙여주었습니다.@OnDelete
는DDL
자체로 외래키 제약조건에다가On Delete Cascade
걸 수 있게 해주는annotation
이어서 사용하였습니다.이상하게
cascade = CascadeType.REMOVE
를 쓰면 안지워지더라구요.정답을 알려줘!
방대한 테스트 코드 양
최대한 모든 경우에 대처하는 테스트 코드를 한번 짜보고 싶어서, 제가 생각하는 거의 모든 경우에 대해서 테스트 코드를 짰습니다.
그래도 읽으실 때는 테스트 코드 대부분이 비슷한 맥락이어서 쑥쑥 넘길 수 있을 것 같긴합니다.
왜 그냥 Comment 아니고 Pin Comment?
현재는
Pin
에만댓글
을 달 수 있지만, 추후 확장될 수 있는 일말의 여지라도 있다고 생각하여 위와 같이네이밍
하였고, 그렇기 때문에PinController
에 이어서 작성하였습니다.그러다 보니
PinController
,PinQueryService
,PinCommandService
가 비대해진 감이 있는데..나누는 게 좋을까요?
좋다고 생각이 드시면 의견도 주시면 감사할 것 같습니다~~
스크린샷
📎 관련 이슈
closed #587
레퍼런스