-
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
[feature] Google 로그인 기능 구현 #169
base: develop
Are you sure you want to change the base?
Conversation
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가 null이 아니고 로그인되지 않았다면 id가 null이 되는 방식으로 하여 로그인 기능을 null을 활용하여 처리했다고 이해했습니다!
그러다보니 null 처리를 해야하는 곳이 많아진 것 같은데 저였어도 null로 관리했을 것 같아 마땅히 더 좋은 방법은 떠오르지 않네요 ㅎㅎ,,
그리고 기존의 랜덤 아이디로 로그인하는 코드는 만약 구글 로그인이 잘 된다면 없애도 된다고 생각합니다! 지워도 커밋에 이력이 남아있으니 필요하다면 다시 찾아서 넣으면 되지 않을까욥 ㅎㅎ
if (showSignInDialogDescription != null) { | ||
SignInAlertDialog( | ||
onDismissRequest = { showSignInDialogDescription = null }, | ||
onGoogleSignInClick = { | ||
GoogleId(context).signIn( | ||
onSuccess = { credential -> | ||
accountViewModel.signIn(credential) | ||
showSignInDialogDescription = null | ||
} | ||
) | ||
}, | ||
description = showSignInDialogDescription!! | ||
) | ||
} |
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.
!!
의 사용을 피하기 위해 let 구문을 사용하는 것도 나쁘지 않을 것 같습니다!
showSignInDialogDescription?.let { description ->
SignInAlertDialog(
onDismissRequest = { showSignInDialogDescription = null },
onGoogleSignInClick = {
GoogleId(context).signIn(
onSuccess = { credential ->
accountViewModel.signIn(credential)
showSignInDialogDescription = null
}
)
},
description = description
)
}
DetailPickScreen에도 같은 코드가 있던데 혹시 변경하신다면 저기도 함께 하면 될 것 같습니당
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.
저도 이 부분을 고민하다가 showSignInDialogDescription의 null여부에 따라 SignInAlertDialog가 보여짐을 좀 더 명시적으로 나타내고 싶어서 if문으로 구현을 했었습니다! 혹시 제 의도와 무관하게 let구문을 쓰더라도 가독성이 괜찮은 것 같으면 바꾸겠습니다 :)
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.
이건 그냥 네이밍에 대한 궁금증인데 제가 만들었다면 CreateGoogleIdUser
로 했을 것 같은데 구글 아이디로 로그인된 유저를 뜻하기 위해 CreateGoogledIdUser
인 걸까요..?!
app 모듈의 account 하위에는 GoogleId라는 이름으로 클래스를 만드셨는데 use case와 FirebaseRepository에서는 GoogledId라는 이름을 사용하셔서 의도된 뜻이 있는 건지 궁금해서 여쭤봅니당!!
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.
헉 오타입니다...! 발견해주셔서 감사합니다
selectAllPicks = { pickListViewModel.selectAllPicks() }, | ||
deselectAllPicks = { pickListViewModel.deselectAllPicks() }, | ||
) | ||
if (pickListViewModel.getUserId() == userId) { |
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문은 현재 유저의 픽 보관함, 등록한 픽 화면일 때만 편집 모드 버튼을 보여주기 위한 거죵??
제가 만들 때 빠뜨렸었나 보네요 ㅎㅎ;; 감사합니다
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 (userId == currentUser.userId) { | ||
_profileUser.emit(currentUser) | ||
if (userId == currentUser?.userId) { | ||
_profileUser.emit(currentUser ?: DEFAULT_USER) | ||
} else { | ||
val otherProfileUsr = fetchUserByIdUseCase(userId).getOrDefault(DEFAULT_USER) |
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.
otherProfileUser
에 e가 빠진 것 같아요!
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.
고생하셨어요!! 👏👏👏
랜덤 아이디로 로그인하는 코드는 삭제하는게 좋겠네요
<string name="sign_in_dialog_title_favorite_picks">담은 픽을 확인하기 위해\n로그인이 필요합니다</string> | ||
<string name="sign_in_dialog_title_add_pick">픽을 등록하기 위해\n로그인이 필요합니다</string> | ||
<string name="sign_in_dialog_title_favorite">픽을 담기 위해\n로그인이 필요합니다</string> | ||
<string name="sign_in_dialog_dismiss">더 둘러보기</string> |
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.
다이얼로그 닫는 버튼 텍스트는 '취소'가 더 직관적이라 생각해요
@@ -48,6 +55,7 @@ fun MapScreen( | |||
val playerState by playerServiceViewModel.playerState.collectAsStateWithLifecycle() | |||
|
|||
val context = LocalContext.current | |||
var showSignInDialogDescription by remember { mutableStateOf<String?>(null) } |
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.
보통 show~는 Boolean 값으로 특정 UI 요소를 보일지 말지 결정하는데 쓰이지않나요?
signInDialogDescription 으로 바꾸는게 좋을 것 같습니다
(아님말고입니다 ㅎㅎ)
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.
앗 그렇게 보일 수 있겠네요! string의 null 여부로 보여주는데 boolean 변수를 하나 더 생성하지 않으려고 이 방식을 사용했었습니다.
음.. 아니면 혹시 show변수를 하나 더 생성하고 description은 따로 관리하는 것이 더 좋을까요?
accountViewModel.signInSuccess | ||
.flowWithLifecycle(lifecycleOwner.lifecycle, Lifecycle.State.STARTED) | ||
.collect { isSuccess -> | ||
if (isSuccess) pickViewModel.fetchPick(pickId) |
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.
여기서 fetchPick을 다시 하는건 픽담기 후 로그인 했을때 화면을 다시 설정하기 위함인가요? 안하면 어떻게 되나용
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.
네 맞습니다
비회원 상태일 때 기본으로 픽을 담은 여부가 false 상태입니다.
여기서 fetchPick을 하지 않으면 dialog를 통해 로그인 후, 유저의 픽 담기 여부와 상관 없이 여전히 false 상태가 유지됩니다.
override suspend fun createGoogledIdUser(userId: String, userName: String?, userProfileImage: String?): User? { | ||
return suspendCancellableCoroutine { continuation -> | ||
val documentReference = db.collection("users").document(userId) | ||
documentReference.set(FirebaseUser(name = userName, profileImage = userProfileImage)) |
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.
GoogleIdTokenCredential에서 이름을 가져오는 것 같은데 이때 displayName은 어떤 이름인가요??
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가 구글 아이디로 이메일이고, displayName은 구글 닉네임에 해당됩니다!
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.
이 유스케이스의 역할은 data store의 유저 ID를 지우는 것인데 이름은 SignOut이라 혼동이 생길 수 있어보여요
역할을 정확히 지칭하는 이름이 좋을 것 같습니다
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.
처음에는 SignOut에 관해 필요한 기능들을 모아두려고 해당 이름을 지었는데, 만들고 보니 clear만 하게 되었네요 ㅎㅎ 말씀하신대로 수정하겠습니다
#️⃣연관된 이슈
📝작업 내용 및 코드
실행 화면
1. 비회원 실행
2. 로그인 및 로그아웃
3. 이미 담은 픽화면에서 로그인 하는 경우 담기 정보 업데이트
💬리뷰 요구사항
a. google-services.json 파일 업데이트 필요합니다.
b. local.properties에 GOOGLE_CLIENT_ID 추가가 필요합니다.
참고 사항