-
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
Feature/mz 150 kakao login #19
Conversation
수수 토큰 만료 여부 체크해야 함
mypage module에서도 로그아웃을 위해 접근 가능해야 함
feature 모듈에서 hiltViewModel() 사용을 위해 추가했습니다.
data module에 액세스 불가하여 Repository를 주입하지 못하는 문제
사용하는 플러그인들에서 자꾸 파일이 생성되어서 추가합니다. 이전 프로젝트들에서는 항상 idea 폴더를 통으로 뺐어요!
af0a8f1
to
d4e1d00
Compare
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.
중복되는 내용은 제외했어요~!
한번 얘기해보면 좋을거같습니다!
val refreshToken: String, | ||
) | ||
|
||
fun TokenResponse.toDomain() = 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.
toDomain 대신 toModel 어떠신가용??
Domain 모듈과는 좀 다른 느낌이라 ... ㅎㅎ
tokenRepository.saveTokens( | ||
token = loginRepository.login(oauthAccessToken = oauthAccessToken), | ||
) |
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.
token 저장하는 로직을 loginRepository에서 처리하는건 어떻게 생각하시나요?
이유는 다음과 같습니다.
- token 저장 로직은 도메인보단 데이터 영역에 더 가깝다고 생각합니다. 도메인에는 앱의 핵심 비즈니스 규칙이 들어가야하는데, 토큰 저장은 데이터 영역에 가깝다고 생각하기 때문이에요.
- token 저장 로직을 domain에 넣은 경우, 추후 token 방식이 아닌 session 방식으로 변경이 되었을때 data layer와 domain layer 모두 수정을 해줘야합니다. / 만약 token 저장 로직이 data layer에만 존재한다면 session 방식으로 변경되었을 때 data layer만 변경하면 되므로 유지보수에 더 유리하다 생각해요.
loginUseCase(oauthAccessToken = oauthAccessToken) | ||
.onSuccess { | ||
postSideEffect(LoginContract.LoginEffect.NavigateToReceived) | ||
} | ||
.onFailure { | ||
postSideEffect(LoginContract.LoginEffect.ShowToast(it.message ?: "수수 로그인 실패")) | ||
} |
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.
요놈은 별도의 함수로 한번 더 분리하는거 어떤가요? onSuccess가 2번 있으니까 가독성이 떨어지는 것 같아요. (개인 취향)
import com.susu.core.ui.base.SideEffect | ||
import com.susu.core.ui.base.UiState | ||
|
||
sealed interface SignUpContract { |
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.
sealed interface로 한번 더 감싼 이유가 뭔가요?
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.
그...그냥 MVI 참고한 포스트나 레포에서 묶어쓰길래 국룰인갑다 하고 썼어요
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.
지난 리뷰에서 sealed interface면 충분하다 말씀하셨던게 Contract 라인이 아니라 Effect 라인이엇군요
import com.susu.core.ui.base.UiState | ||
|
||
sealed interface SignUpContract { | ||
sealed class SignUpEffect : SideEffect { |
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.
sealed class SignUpEffect : SideEffect { | |
sealed interface SignUpEffect : SideEffect { |
viewModel: SignUpViewModel = hiltViewModel(), | ||
navigateToReceived: () -> Unit, | ||
) { | ||
val uiState by viewModel.uiState.collectAsState() |
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.
val uiState by viewModel.uiState.collectAsState() | |
val uiState by viewModel.uiState.collectAsStateWithLifecycle() |
LaunchedEffect(key1 = viewModel.sideEffect) { | ||
viewModel.sideEffect.collect { sideEffect -> |
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.
collectWithLifecycle
fun getAccessToken( | ||
callback: (String?) -> Unit, | ||
) { | ||
UserApiClient.instance.accessTokenInfo { _, error -> | ||
if (error != null) { | ||
callback(null) | ||
} else { | ||
// 토큰 유효성 체크 성공(필요 시 토큰 갱신됨) | ||
callback(TokenManagerProvider.instance.manager.getToken()?.accessToken) | ||
} | ||
} | ||
} |
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.
val accessToken = TokenManagerProvider.instance.manager.getToken()?.accessToken
그냥 이렇게하면 콜백 안써도 되지 않나요?
UserApiClient.instance.accessTokenInfo를 사용하는 이유가 없어보여요.
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.
UserApiClient.instance.accessTokenInfo를 사용하면 Kakao SDK에서 accessToken 만료시 토큰 갱신 후에 callback을 실행합니다
그런데 accessTokenInfo에서 제공하는 데이터 중에는 accessToken이 없어요! (최대 의문)
오로지 TokenManagerProvider를 통해서만 accessToken에 접근할 수 있습니다!
정리하자면, accessToken 만료 시 갱신하고, 갱신된 accessToken를 반환하기 위함입니다
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.
아하 이런 사정이 있었군요 ... 어쩐지 ...
확인했슴다!
package com.susu.feature.mypage | ||
|
||
import androidx.lifecycle.viewModelScope | ||
import com.kakao.sdk.user.UserApiClient |
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.
조금 복잡하지만 callback 형식으로 만들면 kakao sdk 의존성 제거 가능하지 않을려나요?
카카오 sdk 의존성이 viewmodel에 있으면 나중에 테스트 코드 짜기가 어려울 것 같아요. 물론 이 프로젝트에서 테스트코드를 짜진 않을테지만 테스트 코드 작성이 유리하게 ViewModel을 만드는 연습을 해보면 어떨까 하는 생각입니다!
private val checkCanRegisterUseCase: CheckCanRegisterUseCase, | ||
) : BaseViewModel<MainContract.MainState, MainContract.MainEffect>(MainContract.MainState()) { | ||
init { | ||
if (!KakaoLoginHelper.hasKakaoLoginHistory()) { |
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.
KakaoLoginHelper 의존성 끊는거 어때요??
) : BaseViewModel<MyPageContract.MyPageState, MyPageContract.MyPageEffect>(MyPageContract.MyPageState) { | ||
|
||
fun logout() { | ||
UserApiClient.instance.logout { error -> |
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.
앗 다시 보니 UserApiClient 관련 코드만 Screen 쪽에서 실행한다면 의존성 제거 가능할거같아요
name = uiState.value.name, | ||
gender = uiState.value.gender, | ||
birth = uiState.value.birth.toInt(), |
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.
name = uiState.value.name, | |
gender = uiState.value.gender, | |
birth = uiState.value.birth.toInt(), | |
name = currentState.name, | |
gender = currentState.gender, | |
birth = currentState.birth.toInt(), |
intent { copy(birth = birth) } | ||
} | ||
|
||
fun 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.
https://devscb.tistory.com/130
얼리 리턴 써보시는거 어때요? indent가 너무 많아서 가독성이 떨어지는 것 같아요.
(다른 코드에서도 얼리 리턴 쓰면 더 보기 좋을거같아요~! (제 개인 취향 같긴합니다만 ...)
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.
merge 가시죱!
61baaa5
to
25ef406
Compare
25ef406
to
d6f69ac
Compare
💡 Issue
🌱 Key changes
✅ To Reviewers
코드를 실행하기 전에
Kakao 키 해시 등록
1-1. Kakao SDK Debug Key 발급방법을 참고하여 디버그 키를 발급합니다
1-2. 카카오 디벨롭스에 수수 노션 - 보안 페이지의 카카오 개발자 계정으로 로그인하여
SUSU
- 플랫폼 - Android에 디버그 키를 등록합니다local.property 내용 추가
내용은 슬랙에 메세지 드리겠습니다~
코드 읽기 전에
📸 스크린샷
KakaoTalk_20240102_153056748.mp4