Skip to content
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

refactor(user): 좋아요 취소 로직 변경 및 user cascade 옵션 변경 #697

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

NewWisdom
Copy link
Collaborator

@NewWisdom NewWisdom commented Nov 13, 2021

  • 🧪 테스트 전체를 실행해봤나요?
  • 💯 테스트가 잘 통과했나요?
  • 🏗️ 빌드는 성공했나요?
  • 🧹 안쓰는 코드가 남아있지 않나요?
  • 🎟️ PR에 대한 이슈는 등록됐나요?
  • 🏷️ PR 라벨 등록 했나요?
  • 🍠 행복한 고구마인가요?

작업 내용

베이스는 #694로 해당 pr에 다 설명하기는 좀 구구절절이라 체리픽해서 작성했습니다

1. user에 있는 feeds, likes에 있는 cascade 옵션을 remove로 변경
놀토에서 user가 생성될 때 feed와 like가 함께 생성되는 경우는 없고
원하는 것은 유저가 삭제될 때 feed와 like가 삭제되는 것이니 comments, commentsLikes에
cascade.remove만 걸어준 옵션을 동일하게 가져갈 수 있을 것 같습니다 !

2. 각 likeService에 있는 user.addLike~와 같은 로직 삭제
#694에서 논의한 것 처럼 댓글 삭제를 이제
서비스의 역할에서 영속화 된 데이터를 지운다라는 측면에서 바라봄, 연관관계를 끊어서 삭제가 아닌 직접 삭제
repository로 삭제하는 로직을 가져간다면, 이를 피드 좋아요와 댓글 좋아요에도 동일히 적용해 보는 것은 으떨까요?
이렇게 되면 기존 likeService에서 추가는 user.addLike로 추가, 삭제는 repository로 직접 삭제로
서로 달랐던 구현방식을 일관성있게 가져갈 수 있을 것 같습니다.(commentLikeService랑두요!)

이 두 사항을 반영하면 터지는 테스트 코드들이 있는데, 이는 프로덕션 로직이 잘못된 것이 아니라
em.clear()로 영속성 컨텍스트를 비우고
검증에 사용할 대상 객체들을 db에서 새롭게 조회하지 않고 영속화 되지 않은 객체를 사용해서 발생하는 문제였습니다 😅

주의사항

  • 요런식으로 무작정 사용했던 cascade.all을 개선할 수 있을 것 같다는 생각 공유 차원
  • #694를 체리픽해왔기에 이 pr이 머지되어도 충돌은 안날겝니당

@NewWisdom NewWisdom added ❓ question Further information is requested 🔨 refactor 🚀 backend backend work labels Nov 13, 2021
@NewWisdom NewWisdom self-assigned this Nov 13, 2021
@netlify
Copy link

netlify bot commented Nov 13, 2021

✔️ Deploy Preview for nolto canceled.

🔨 Explore the source changes: 2be853e

🔍 Inspect the deploy log: https://app.netlify.com/sites/nolto/deploys/618f723d07003d0007dea04d

Comment on lines 33 to 38
em.clear();

// then
찰리가_쓴_피드에_찰리가_쓴_댓글 = commentRepository.getById(찰리가_쓴_피드에_찰리가_쓴_댓글.getId());
assertThat(찰리가_쓴_피드에_찰리가_쓴_댓글.getLikes()).hasSize(1);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이전에는 db에 반영된 찰리가_쓴_피드에_찰리가_쓴_댓글을 검증한 것이 아닌 현재 테스트 메서드에 있는
찰리가_쓴_피드에_찰리가_쓴_댓글 객체를 검증하고 있었어요.
해당 서비스 테스트에서는 addCommentLike후 db에 반영된 내용을 검증하니
새롭게 getById()로 찰리가_쓴_피드에_찰리가_쓴_댓글을 찾아 검증해야할 것 같숩니다 !

Comment on lines 321 to 323
em.clear();
조엘 = userRepository.getById(조엘.getId());
FeedResponse feedResponse = feedService.viewFeed(조엘, feedId1, true);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 viewFeed 로직 안에서 user.isLiked(feed);로 좋아요 여부를 반환하는데,
이 때도 실제 db에 반영된 조엘 을 넘겨주지 않고 있었어요 !

@Gomding Gomding self-requested a review November 13, 2021 08:32
@bosl95
Copy link
Collaborator

bosl95 commented Nov 13, 2021

오오 이야기 나눈 부분을 다시 수정해주셨군요! 👍👍👍
우선

  • Comment#addCommentLike
  • User#deleteComment

사용되지 않고 있는 메서드가 있는데 확인 부탁드려요~~
천천히 읽어보면서 다시 코멘트 남기겠습니다!!!

@bosl95 bosl95 self-requested a review November 13, 2021 08:41
@Gomding
Copy link
Collaborator

Gomding commented Nov 13, 2021

서비스의 역할에서 영속화 된 데이터를 지운다라는 측면에서 바라봄, 연관관계를 끊어서 삭제가 아닌 직접 삭제

이전 조엘 PR에서 제가 이렇게 말했지만 뻔뻔하게 번복을 해보면!

계층의 관점에서 보면 서비스에서 영속화 된 데이터를 지운다 라는건 반은 맞고 반은 틀린말이라 생각합니다!
(계층 구조에서 다음 계층만 의존해야한다고 했을때 Service Layer는 Domain Layer를 의존해야함)

그래서 영속화 된 데이터를 지운다 보다 객체의 Collection에서 지운다라고 해야될것같아용

우리가 domain 패키지에 작성하는 Repository interface는 객체의 Collection 이지만 숨겨진 구현체는 영속화된 데이터의 Collection 이라고 이해하고 있습니당 :)

Copy link
Collaborator

@Gomding Gomding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

깔끔하게 수정해주셨네요!

수정내용은 전부 동의해서 어프로브 할게요!👍

Copy link
Collaborator

@joelonsw joelonsw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋네요! Cascade.ALL, OrphanRemoval 드디어 탈출!
심플이즈더베스트!

@@ -42,10 +42,10 @@
@JoinColumn(name = "parent_id")
private Comment parentComment;

@OneToMany(mappedBy = "comment", cascade = CascadeType.REMOVE, orphanRemoval = true)
@OneToMany(mappedBy = "comment", cascade = CascadeType.REMOVE)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CascadeType.REMOVE만 가져가고 orphanRemoval 없애주는 거 좋네요!

@@ -49,16 +49,16 @@

private String bio = "";

@OneToMany(mappedBy = "author", cascade = CascadeType.ALL)
@OneToMany(mappedBy = "author", cascade = CascadeType.REMOVE)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ALL을 쓰는건 생각과 다르게 동작할 여지가 있는 것 같아요!
REMOVE 전환 찬성입니다~!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚀 backend backend work ❓ question Further information is requested 🔨 refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants