-
Notifications
You must be signed in to change notification settings - Fork 1
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 : 멤버 도메인 준회원 승급 조건 추가 - 기본 정보 작성 #349
Conversation
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
커밋 메세지 컨벤션 체크해주세요~ |
bfc79e8
to
7133165
Compare
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
7133165
to
2d42dd1
Compare
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
fd5ad85
to
9dfa392
Compare
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
@Component | ||
@RequiredArgsConstructor | ||
public class MemberAssociateEventHandler { | ||
public void handle(Object context) { |
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.
SpringEventHandler
보시고 이렇게 하신 것 같은데, 디스코드 패키지 핸들러들은 디스코드 클라이언트와 통신하면서 발생하는 예외를 aop로 잡으려고 인터페이스로 추출한 거라 일반적인 스프링 이벤트 핸들러는 Object가 아닌 이벤트 타입 그대로 받는게 좋을 것 같습니다~
@@ -28,7 +28,7 @@ public class MemberCustomRepositoryImpl extends MemberQueryMethod implements Mem | |||
public Page<Member> findAllGrantable(MemberQueryOption queryOption, Pageable pageable) { |
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.
수동 grant 대신 준회원 자동승급으로 변경되었으니 grant 가능한 대상 조회하는 메서드도 제거되는 게 좋지 않을까요?
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.
grant가능한 대상 말고 Associate인 대상을 find하는 것으로 바꾸는 것은 어떻게 생각하실까용?
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.
grant가능한 대상 말고 Associate인 대상을 find하는 것으로 바꾸는 것은 어떻게 생각하실까용?
이 부분은 기획팀이랑 피그마에서 논의한 내용이 있어요.
피그마 2차 MVP(개발용)에서 코멘트 달려있는 부분 한번 찾아보시고 별도 이슈에서 처리하시면 좋을 것 같습니다.
노션에 링크 달아두겠습니다.
https://vladmihalcea.com/best-spring-data-jparepository/ -> 저기서 말하고자 하는 바는 hibernate가 변경감지를 지원하는데도 명시적으로 save를 호출하면 안된다는 이야기인 것 같고, 저희는 이벤트 발행하려면 save를 호출해야 하니 어쩔 수 없는 부분인 것 같아요. merge event가 발생하니 불필요하게 하지 말라는 이야기이지 필요한 상황에서 굳이 안쓰기 위해서 회피하는 것보다는 나으니깐요. AbstractAggregateRoot와 JPARepository save() 안티 패턴에 대해 -> 최범균님 jpa 강의(링크)를 보면 이런 내용이 나옵니다.
위 글에서 불필요한 MergeEvent가 발생했을 때 엔티티가 일대다 관계를 가지고 있으면 문제가 발생할 수 있다고 합니다. 하지만 애초부터 저희 서비스는 모든 연관관계에서 일대다 관계를 사용하고 있기 때문에 성능적 오버헤드가 그리 심각하게 발생할 것 같다는 생각은 들지 않네요. 이벤트 발행을 위한 명시적 save가 어색할 수 있지만 어쨌든 AbstractAggregateRoot를 사용하여 이벤트를 발행하기 위한 '공식적인' 방법이고, 제가 알기로 비슷하게 data jpa 팀에게 "쓰기 지연이 있는데도, 왜 이벤트 발행을 위해서 명시적 save를 호출해야 하냐" 라는 이슈를 남긴 경우도 봤었는데, data jpa 팀 측에서 구현상 어쩔 수 없었고, 이게 최선이었다 라는 답변을 본 것도 기억이 나네요. 정리하자면 성능상 이슈는 충분히 견딜만하다고 생각하고, 불편함이 있다면 이벤트 발행하는 경우 쓰기 지연을 가정하고 작성했던 서비스 로직에 매번 명시적으로 save를 호출해야 한다... 라는 건데, "명시적 save 호출의 불편함" vs "aop로 이벤트 발행해주는 기능 만들고, 관리하기" 의 비용 비교했을 때는 아직까지는 전자가 더 적은 것 같고, 굳이 후자가 그만한 리소스 들일 정도로 중요한 기능도 아니라고 생각해서요. |
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.
확인했습니다~
|
||
this.studentId = studentId; | ||
this.name = name; | ||
this.phone = phone; | ||
this.department = department; | ||
this.email = email; | ||
|
||
registerEvent(new MemberAssociateEvent(this)); |
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.
제가 이벤트 쪽은 경험이 없어서 모르는 걸 수도 있는데,
verifyInfoStatus() 내부에서도 부르던데 반복해서 호출하는 이유가 있나요?
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.
준회원 승급 조건 하나할 때마다 조건 충족했는지 여부를 확인할때 registerEvent를 사용했습니다!
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.
네 그건 알고 있는데, 이렇게 되면 updateBasicMemberInfo
메서드에서 registerEvent(new MemberAssociateEvent(this));
를 두번 호출하는거 아닌가요?
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.
verifyInfoStatus에서 제거하기보다 이 메서드에서 지워야 하지 않나요?
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.
준회원으로 승급 가능한지 확인하는 이벤트를 발행하는 것은 기본 회원 정보를 작성했기 때문이 아니라 infoStatus가 verified로 바뀌었기 때문 아닌가요?
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.
아 전 단순히 정보 작성에 꽂혀있었네요!
지적해주셔서 감사합니다
@@ -28,7 +28,7 @@ public class MemberCustomRepositoryImpl extends MemberQueryMethod implements Mem | |||
public Page<Member> findAllGrantable(MemberQueryOption queryOption, Pageable pageable) { |
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.
grant가능한 대상 말고 Associate인 대상을 find하는 것으로 바꾸는 것은 어떻게 생각하실까용?
이 부분은 기획팀이랑 피그마에서 논의한 내용이 있어요.
피그마 2차 MVP(개발용)에서 코멘트 달려있는 부분 한번 찾아보시고 별도 이슈에서 처리하시면 좋을 것 같습니다.
노션에 링크 달아두겠습니다.
} | ||
|
||
@Test | ||
void 기본_회원정보_작성_디스코드인증_Bevy인증_재학생인증하면_isAllVerified는_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.
void 기본_회원정보_작성_디스코드인증_Bevy인증_재학생인증하면_isAllVerified는_true이다() { | |
void 준회원_가입조건을_모두_충족했다면_isAllVerified는_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.
이름에 대해 고민하고 있었는데 이게 좋은것같네용 감삼당
import org.springframework.stereotype.Component; | ||
import org.springframework.transaction.event.TransactionalEventListener; | ||
|
||
@Slf4j |
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.
@Slf4j |
지금 보니 제가 만든 DiscordIdBatchCommandListener
랑 재현님이 만드신 MemberGrantEventListener
에도 안 쓰면서 달아두긴 했는데 아마 복붙 과정에서 여기저기 붙은 것 같네요..😅😅
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
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.
확인했습니다.
member.completeUnivEmailVerification(UNIV_EMAIL); | ||
member.verifyDiscord(DISCORD_USERNAME, NICKNAME); | ||
|
||
System.out.println(member.getRequirement().isInfoVerified()); |
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.
System.out.println(member.getRequirement().isInfoVerified()); |
member.completeUnivEmailVerification(UNIV_EMAIL); | ||
member.verifyDiscord(DISCORD_USERNAME, NICKNAME); | ||
member.verifyBevy(); | ||
member.advanceToAssociate(); |
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.
line 176은 when에 해당하지 않나요?
++ test 이름도 수정이 필요해 보입니다.
member.verifyDiscord(DISCORD_USERNAME, NICKNAME); | ||
member.verifyBevy(); | ||
|
||
// then |
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.
// then | |
// when & then |
@@ -247,13 +250,18 @@ public void updateMemberInfo( | |||
public void completeUnivEmailVerification(String univEmail) { | |||
this.univEmail = univEmail; | |||
requirement.updateUnivStatus(RequirementStatus.VERIFIED); | |||
registerEvent(new MemberAssociateEvent(this)); |
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.
다른 준회원 조건처럼 line 252~253를 verify 메서드로 분리하지 않으신 이유가 있나요?
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.
completeUnivEmailVerification이라는 메서드가 이미 존재해서 메서드 이름명 차이인 것같은데 vreifyEmail이런식으로 바꾸면 기존에 사용되던것들을 다른 클래스에서도 다 바꿔야 해서 고려를 하지 않았습니다!
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.
이미 존재하던 거라서 통일시킬 생각을 못했다는 것입니다!
필요하다고 판단되면 추가하겠습니다
registerEvent(new MemberAssociateEvent(this)); | ||
} | ||
|
||
public void verifyInfoStatus() { |
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.
다른 verify 메소드들처럼 네이밍 수정해주세요~
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.
어떤건 Status가 붙었고 또 어떤건 안 붙어 있지 않나요?
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.
아 그러네요 이번 기회에 그냥 싹 다 통일하겠습니다 위에 주신 리뷰 포함해서!
registerEvent(new MemberAssociateEvent(this)); | ||
} | ||
|
||
public boolean isAllVerified() { |
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문 대신 아래와 같이 활용 가능할 것 같습니다.
return isAssociateAvailable();
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
@@ -0,0 +1,3 @@ | |||
package com.gdschongik.gdsc.domain.member.domain; | |||
|
|||
public record MemberAssociateEvent(Member member) {} |
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 등에 대한 정보만 저장하고, 나머지는 핸들러에서 조회 후 구현해야 합니다.
@Component | ||
@RequiredArgsConstructor | ||
public class MemberAssociateEventHandler { | ||
public void handle(MemberAssociateEvent context) { |
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.
handle보다 더 의미가 잘 드러나는 이름을 사용해주세요.
마찬가지로 context 파라미터 네임도 적절하게 변경해주세요.
return false; | ||
} | ||
|
||
private boolean isAtLeastAssociate(MemberRole role) { |
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 advanceToAssociate(Member member) { |
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.
분리한 이유는 딱히 없습니다! 한 줄 정도는 분리할 필요가 없겠네요
registerEvent(new MemberAssociateEvent(this)); | ||
} | ||
|
||
public boolean isAllVerified() { |
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.
isAllVerified
메서드를 Requirement
vo가 아닌 멤버가 가지게 만드신 이유가 뭔가요?
전체 verifiy status에 대한 조작 및 리턴 책임을 해당 vo에서 가지는데, 전체 status에 대한 verified 값 리턴에 대한 책임은 vo가 아닌 멤버 엔티티에서 가지게 만든 이유가 궁금합니다.
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.
this.discordame == null이런 member에만 존재하는 것들이 있어서 멤버 도메인에다가 했습니다!
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.
음 verified로만 검증해도 될 것 같아요.
이렇게 했던 이유가 기존에 infoStatus에 해당하는 부분이 signup으로 들어가 있어서 그랬던 거라서요~
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.
그러면 requirement에다가 수정해서 올리겠숨당~
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
…into refactor/348-member-basic-info
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
private final MemberRepository memberRepository; | ||
|
||
public void advanceToAssociate(MemberAssociateEvent memberAssociateEvent) { | ||
MemberAssociateEvent event = memberAssociateEvent; |
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 boolean validateAdvanceAvailable(Member member) { | ||
if (member.isAtLeastAssociate(member.getRole())) { |
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 isAssociateAvailable() { | ||
if (!this.isInfoVerified()) { |
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;
을 없애면 좋을 것 같습니다.
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
private boolean isAssociateAvailable() { | ||
if (!this.isInfoVerified() || !this.isDiscordVerified() || !this.isBevyVerified() || !this.isUnivVerified()) { | ||
return false; | ||
} | ||
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.
private boolean isAssociateAvailable() { | |
if (!this.isInfoVerified() || !this.isDiscordVerified() || !this.isBevyVerified() || !this.isUnivVerified()) { | |
return false; | |
} | |
return true; | |
} | |
private boolean isAssociateAvailable() { | |
return this.isInfoVerified() && this.isDiscordVerified() && this.isBevyVerified() && this.isUnivVerified(); | |
} |
} | ||
} | ||
|
||
private boolean validateAdvanceAvailable(Member member) { |
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.
전부 멤버 내부 상태에 대한 검사인데 굳이 응용 레이어에서 수행할 필요가 있나요?
그리고 member.advanceToAssociate()
내부에서 이미 검사되는 로직인데 중복해서 핸들러에 넣을 필요는 없을 것 같습니다.
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.
음 제가 판단한거는 일단 이미 내부에
validateGrantAvailable이 있어도 그건 exception을 던지는 것이라서 boolean값으로 handler에서 이 member는 모든 조건을 충족시켰는지에 대한 여부를 boolean으로 검증하고 advanceToAssociate를 호출하는 로직을 생각한것이었습니답
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.
재현님 말처럼 하려면 저 validateAdvanceAvailable을 member내부에 두고 member.AdvanceToAssociate메서드 안에 exception던지는 것을 대신해서 boolean으로 검증하는 메서드가 들어가야 된다고 생각하는데
그렇게 되면 또 exception을 던질수가 없어지니
두 메서드 다 넣어도 되는데 중복처리 느낌이 들어서...음 네..흠..
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.
재현님 말처럼 저 validate 부분을 멤버 도메인 내부로 옮기는것이 적절하다고 생각하는데
저 exception던지는 로직 or boolean으로 검증 or 둘다 가져가기 중에 뭐가 제일 나은 선택일까요..
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.
validate 로직을 try-catch로 감싸서 boolean을 리턴하는 isAssociateAvailable 메서드를 만드는 것도 생각해볼 수 있겠네요.
아마 멤버 내부에 다른 로직도 그렇게 처리하고 있을 거에요.
그리고 핸들러에서는 if (member.isAdvancableToAssociate()) member.advanceToAssociate()
호출하면 되고요.
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.
isAdvanceToAssociate()
에서 사용되는 로직이 advanceToAssociate()
내부에서 쓰는 validate 메서드이니, 로직이 중복돼서 ssot 어길 일도 없고 괜찮지 않을까 싶네요.
굳이 불편한 점을 찾는다면 외부에 만약 X가 가능하다면, X를 수행한다
라는 컨텍스트를 노출하기 위해서 굳이 X가 가능한가?
에 대한 불린 메서드를 public으로 열어두는 것인데, 이게 싫다면 그냥 핸들러에서 승급 가능 여부 체크 없이 바로 advanceToAssociate
호출하고 try-catch 감싼 다음 예외 터트리지 않고 로그만 남기면 되겠죠
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으로 열어두는 방향이 맞다고 생각하긴 합니다! (사실 지금 그렇게 되면 수정+삭제해야될 테스트 코드가 가늠이 안돼서 ㅎㅎ...)
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.
그리고 이렇게 수정하면 또 이제 굳이 requirement에 새로 만든 isAllIVerified이런건 다 필요가 없어지네요..!
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.
근데 지금 코드 짜면서 생각이 난건데..음 만약에 그러면 기본 작성 정보중인데 다른 조건들이 만족 안하니까 결국 save하면 exception이 터질텐데, exception을 터트리는 것보다 로그를 남기는게 갑자기 더 낫지 않나 라는생각이 들기 시작하긴 헀습니다
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
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.
lgtm
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.
lgtm
🌱 관련 이슈
📌 작업 내용 및 특이사항
📝 참고사항
📚 기타