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

Feature/mz 150 kakao login #19

Merged
merged 60 commits into from
Jan 3, 2024
Merged

Conversation

yangsooplus
Copy link
Member

@yangsooplus yangsooplus commented Dec 26, 2023

💡 Issue

🌱 Key changes

  • 카카오 SDK를 이용한 카카오 가입/로그인
  • 카카오 accessToken을 이용한 수수 가입/로그인
  • Encrypted-DataStore로 토큰 저장소 교체
  • 적당히 뼈대만 구현한 UI (그러나 겁나 많은 파일)

✅ To Reviewers

코드를 실행하기 전에

  1. Kakao 키 해시 등록
    1-1. Kakao SDK Debug Key 발급방법을 참고하여 디버그 키를 발급합니다
    1-2. 카카오 디벨롭스에 수수 노션 - 보안 페이지의 카카오 개발자 계정으로 로그인하여 SUSU - 플랫폼 - Android에 디버그 키를 등록합니다
    스크린샷 2023-12-26 오후 7 01 41

  2. local.property 내용 추가
    내용은 슬랙에 메세지 드리겠습니다~

코드 읽기 전에

  • UI는 기능 동작을 실험하기 위한 최소치라고 봐주세요
  • Feature/mz 154 navigation setting #18 PR이 머지되고 난 develop에 rebase 했습니다.
  • 설명이 필요할만한 부분에 코멘트를 달았습니다.

📸 스크린샷

KakaoTalk_20240102_153056748.mp4

수수 토큰 만료 여부 체크해야 함
mypage module에서도 로그아웃을 위해 접근 가능해야 함
feature 모듈에서 hiltViewModel() 사용을 위해 추가했습니다.
data module에 액세스 불가하여 Repository를 주입하지 못하는 문제
@yangsooplus yangsooplus requested review from jinukeu and syb8200 January 2, 2024 07:45
@yangsooplus yangsooplus force-pushed the feature/MZ-150-kakao-login branch from af0a8f1 to d4e1d00 Compare January 2, 2024 07:57
Copy link
Member

@jinukeu jinukeu left a 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(
Copy link
Member

Choose a reason for hiding this comment

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

toDomain 대신 toModel 어떠신가용??
Domain 모듈과는 좀 다른 느낌이라 ... ㅎㅎ

Comment on lines +13 to +15
tokenRepository.saveTokens(
token = loginRepository.login(oauthAccessToken = oauthAccessToken),
)
Copy link
Member

Choose a reason for hiding this comment

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

token 저장하는 로직을 loginRepository에서 처리하는건 어떻게 생각하시나요?

이유는 다음과 같습니다.

  1. token 저장 로직은 도메인보단 데이터 영역에 더 가깝다고 생각합니다. 도메인에는 앱의 핵심 비즈니스 규칙이 들어가야하는데, 토큰 저장은 데이터 영역에 가깝다고 생각하기 때문이에요.
  2. token 저장 로직을 domain에 넣은 경우, 추후 token 방식이 아닌 session 방식으로 변경이 되었을때 data layer와 domain layer 모두 수정을 해줘야합니다. / 만약 token 저장 로직이 data layer에만 존재한다면 session 방식으로 변경되었을 때 data layer만 변경하면 되므로 유지보수에 더 유리하다 생각해요.

Comment on lines +25 to +31
loginUseCase(oauthAccessToken = oauthAccessToken)
.onSuccess {
postSideEffect(LoginContract.LoginEffect.NavigateToReceived)
}
.onFailure {
postSideEffect(LoginContract.LoginEffect.ShowToast(it.message ?: "수수 로그인 실패"))
}
Copy link
Member

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 {
Copy link
Member

Choose a reason for hiding this comment

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

sealed interface로 한번 더 감싼 이유가 뭔가요?

Copy link
Member Author

Choose a reason for hiding this comment

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

그...그냥 MVI 참고한 포스트나 레포에서 묶어쓰길래 국룰인갑다 하고 썼어요

Copy link
Member Author

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 {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sealed class SignUpEffect : SideEffect {
sealed interface SignUpEffect : SideEffect {

viewModel: SignUpViewModel = hiltViewModel(),
navigateToReceived: () -> Unit,
) {
val uiState by viewModel.uiState.collectAsState()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
val uiState by viewModel.uiState.collectAsState()
val uiState by viewModel.uiState.collectAsStateWithLifecycle()

Comment on lines +23 to +24
LaunchedEffect(key1 = viewModel.sideEffect) {
viewModel.sideEffect.collect { sideEffect ->
Copy link
Member

Choose a reason for hiding this comment

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

collectWithLifecycle

Comment on lines +78 to +89
fun getAccessToken(
callback: (String?) -> Unit,
) {
UserApiClient.instance.accessTokenInfo { _, error ->
if (error != null) {
callback(null)
} else {
// 토큰 유효성 체크 성공(필요 시 토큰 갱신됨)
callback(TokenManagerProvider.instance.manager.getToken()?.accessToken)
}
}
}
Copy link
Member

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를 사용하는 이유가 없어보여요.

Copy link
Member Author

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를 반환하기 위함입니다

Copy link
Member

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
Copy link
Member

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()) {
Copy link
Member

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 ->
Copy link
Member

Choose a reason for hiding this comment

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

앗 다시 보니 UserApiClient 관련 코드만 Screen 쪽에서 실행한다면 의존성 제거 가능할거같아요

Comment on lines +36 to +38
name = uiState.value.name,
gender = uiState.value.gender,
birth = uiState.value.birth.toInt(),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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() {
Copy link
Member

@jinukeu jinukeu Jan 2, 2024

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가 너무 많아서 가독성이 떨어지는 것 같아요.

(다른 코드에서도 얼리 리턴 쓰면 더 보기 좋을거같아요~! (제 개인 취향 같긴합니다만 ...)

Copy link
Contributor

@syb8200 syb8200 left a comment

Choose a reason for hiding this comment

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

merge 가시죱!

@yangsooplus yangsooplus force-pushed the feature/MZ-150-kakao-login branch 4 times, most recently from 61baaa5 to 25ef406 Compare January 3, 2024 17:56
@yangsooplus yangsooplus force-pushed the feature/MZ-150-kakao-login branch from 25ef406 to d6f69ac Compare January 3, 2024 18:03
@yangsooplus yangsooplus merged commit 89dd447 into develop Jan 3, 2024
1 check passed
@jinukeu jinukeu deleted the feature/MZ-150-kakao-login branch January 15, 2024 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feat] 카카오 소셜 로그인
3 participants