-
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/LS-6: 카카오, 구글 OAuth2 #16
Conversation
…into feat/LS-6
layer-api/src/main/java/org/layer/auth/dto/service/SignUpServiceResponse.java
Outdated
Show resolved
Hide resolved
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.
고생하셨습니다 궁금한 부분은 질문 남겼습니다!!
추가로 PR에 대한 제 생각 공유해드리겠습니다!
- "회원이 아닌 유저가 로그인하는 상황에 대해"
- 이 부분은 클라와 합의가 된 방식이라면 좋은 것 같습니다!!
- "회원 탈퇴의 방식"
- 회원탈퇴는 soft delete 방식 생각하신게 맞을까요?? 좋은 것 같습니다! 거기까진 고려가 안돼서 컬럼 추가가 필요할 것 같아요! 그렇다면 나중에 batch 작업하는 것도 추가해야 겠네요!
- BaseEntity
- 이 부분은 제가 놓친 것 같습니다..! 추가하는게 좋을 것 같은데 모든 엔티티에 추가하는게 좋을까요?!
layer-domain/src/main/java/org/layer/domain/member/repository/MemberRepository.java
Show resolved
Hide resolved
dependencies { | ||
implementation project(path: ':layer-domain') | ||
} |
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.
여기 build.gradle은 일부러 안지우신건가요??
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.
앗 저거는 external에서 domain 의존성이 필요한 경우가 있었는데, option + enter
쳤더니 인텔리제이가 알아서 만들어 준것 같아요
다시 작업 할 때 파일 삭제하고 전체 build.gradle에 추가하겠습니다
layer-common/src/main/java/org/layer/common/exception/MemberExceptionType.java
Show resolved
Hide resolved
layer-api/src/main/java/org/layer/member/service/MemberService.java
Outdated
Show resolved
Hide resolved
} catch(Exception e) { | ||
throw new BaseCustomException(INVALID_TOKEN); | ||
} |
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.
엇 여기 BaseCustomException 은 TokenException extends BaseCustomException
식으로 상속해서 사용하는 것을 의도했었는데요! 혹시 어떻게 생각하시나요?!
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 switch (socialType) { | ||
case KAKAO -> kakaoService.getMemberInfo(socialAccessToken); | ||
case GOOGLE -> googleService.getMemberInfo(socialAccessToken); | ||
default -> throw new BaseCustomException(INVALID_SOCIAL_TYPE); |
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.
여기도 AuthException extends BaseCustomExcepiton
으로 의도했는데 어떻게 생각하시나요?!
if(memberOpt.isPresent()) { | ||
throw new BaseCustomException(NOT_A_NEW_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.
요기도 같은 맥락입니다!
throw new TokenException(); | ||
throw new BaseCustomException(INVALID_REFRESH_TOKEN); | ||
} |
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(!currentMember.getId().equals(memberId)) { | ||
throw new BaseCustomException(FORBIDDEN); | ||
} |
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 도메인 내 함수로 하는 건 어떠신가요? 그렇게 하면 장점으로 설계가 더 도메인의 역할이 중요해지고, 테스트 코드 작성도 용이해지기에 말씀드려봤습니다!
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.
그게 더 좋을 것 같습니다
그렇게 수정하겠습니다!
refactor/LS-14: 파일 위치 변경, 미사용 코드 제거
📝 PR 타입
📢 변경 사항
구현 내용 노션에 간단하게 문서화 하여 내일(7/8) 중으로 공유해드리겠습니다!
❗️To Reviewer
회원이 아닌 유저가 로그인하는 상황에 대해
회원이 아닌 유저가 로그인하면, 이름을 입력받아서 회원 가입을 할 수 있도록 하는 상황을 가정하고 구현했습니다.
따라서 소셜로그인 한 유저가 DB에 없다면 Exception을 발생시키는 상황입니다.
이런 방식이 괜찮은지 의견 여쭙고 싶습니다!
회원 탈퇴의 방식
현재 "회원 탈퇴" api는 구현하지 않은 상황입니다.
탈퇴 기능을 구현할 때,
Member
엔티티의 삭제 여부 필드(예를 들면delYn
)를true
로 바꿈으로써 구현할 생각이었는데,Member
엔티티에 삭제 여부 필드가 없어서 아직은 구현하지 않았습니다.@mikekks 혹시
BaseEntity
를 안만드신 이유가 있으신 건지 궁금합니다.또한 탈퇴를 어떻게 처리하면 좋을지 의견 여쭙고 싶습니다!
⚙️ 테스트 결과
회원 가입 (유저에게서 이름을 입력받음)
회원 가입이 된 유저가 소셜로그인
👉 반영 브랜치