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/#26 feat sns login #30

Open
wants to merge 31 commits into
base: develop
Choose a base branch
from
Open

Conversation

imseongwoo
Copy link
Contributor

@imseongwoo imseongwoo commented Dec 30, 2022

PR 개요

이슈 번호: 26

PR Checklist

  • Code convention을 잘 지켰나요?
  • 테스트 코드를 추가했나요?

작업사항

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Other

작업사항의 상세한 설명

카카오,네이버,구글의 토큰을 받아온 후 해당 정보로 로그인 및 자동로그인 기능 구현

추가내용

구글로그인 파이어베이스 인증과정에서 할당량 관련 에러가 발생해 구글로그인은 추후 구현 예정입니다.

  • Added Reviewers, Assignees, Labels, Milestone

@imseongwoo imseongwoo requested a review from wnehdals December 30, 2022 06:05
@@ -35,6 +36,7 @@ annotation class NOAUTH
@Module
@InstallIn(SingletonComponent::class)
object NetworkModule {
private val gson = GsonBuilder().setLenient().create()
Copy link
Contributor

Choose a reason for hiding this comment

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

해당 객체가 필요한 이유가 혹시 있나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use JsonReader.setLenient(true) to accept malformed JSON 이라는 에러가 발생해서 gson 객체 설정 후 retrofit 객체에 추가하였습니다.

Log.e(TAG, "카카오계정으로 로그인 실패", error)
} else if (token != null) {
Log.e(TAG, "카카오계정으로 로그인 성공 ${token.accessToken}")
CoroutineScope(Dispatchers.IO).launch {
Copy link
Contributor

Choose a reason for hiding this comment

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

비즈니스 로직은 다 viewmodel로 빼주세요

Copy link
Contributor Author

@imseongwoo imseongwoo Jan 6, 2023

Choose a reason for hiding this comment

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

713500f 6e85a9f 에서 수정했습니다.

private val _loginState = SingleLiveEvent<LoginResult>()
val loginState: SingleLiveEvent<LoginResult> get() = _loginState

private val _isNaverSignUpSuccess = SingleLiveEvent<Boolean>()
Copy link
Contributor

@wnehdals wnehdals Jan 5, 2023

Choose a reason for hiding this comment

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

로그인 화면에서 필요한것은

  1. 회원인가?
    2-1 회원이 아니라면 회원가입을 시키고 로그인 시킨다
    3-1 오류가 난다면 오류를 사용자에게 보여준다
    2-2 회원이면 로그인 시킨다
    3-2 오류가 난다면 오류를 사용자에게 보여준다
    결국 끝에는 로그인 시킨다 or 오류를 사용자에게 보여준다 입니다.
    즉 로그인 화면(Activity)는 로그인 성공, 실패만 알면 됩니다.
    이것의 내용을 담은 라이브데이터만 액티비티가 observe하면 됩니다
    그렇게 하면 변수를 줄일 수 있어 좋아보입니다.

val googleSignUpId = "google"
val naverSignUpId = "naver"

companion object {
Copy link
Contributor

Choose a reason for hiding this comment

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

companion object 는 제일 밑에 작성해주세요

@imseongwoo
Copy link
Contributor Author

피드백 반영 완료했습니다!

}
}

fun socialSignUp(id: String, email: String, nickname: String) {
Copy link
Contributor

Choose a reason for hiding this comment

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

로그인 시도시 아이디가 없다면 code값으로 3을 줍니다 code값으로 다음 할 일을 정하도록 수정해주세요
3이면 회원가입을 시키면 될 것 같습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

163ce27 에서 수정했습니다.

}
}

fun isKakaoSocialIdExist() {
Copy link
Contributor

Choose a reason for hiding this comment

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

코드값으로 구분하면 필요없어 보이는 로직과 함수입니다.

}
}

fun getCustomKakaoSignUpEmail(): String {
Copy link
Contributor

Choose a reason for hiding this comment

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

소셜로그인시 새롭게 아이디, 패스워드, 이메일을 만들어 서버로 보내는것을 서버동아리원가 협의된 내용일까요?

when (it.erroMessage) {
"3" -> {
when (socialNum) {
1 -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

1,2 이렇게 작성하면 협업하는 사람은 무슨 뜻인지 모릅니다.
enum을 활용 혹은 val NAVER = 1 이런식으로 변수로 의미를 부여해주세요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

2a69c96 에서 수정했습니다.

repository.postLogin(account, BuildConfig.social_login_password, isAutoLogin.value!!) {
_loginState.value = it
when (it.erroMessage) {
"3" -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

"3" 이렇게 작성하면 협업하는 사람은 무슨 뜻인지 모릅니다.
enum을 활용 혹은 val NEED_SING_UP = "3", api 응답값을 담는 Enum, 혹은 object 파일 하나 만들어서 변수에 의미를 부여해주세요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

마찬가지로 2a69c96 에서 수정했습니다.


fun socialSignUp(id: String, email: String, nickname: String, socialNum: Int) {
val signUpReq = SignUpReq(id, email, nickname, BuildConfig.social_login_password)
viewModelScope.launch {
Copy link
Contributor

Choose a reason for hiding this comment

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

회원가입 실패에 대한 대처가 필요해 보입니다. 만약 회원가입 실패했다면 사용자는 실패한줄도 모르고 계속 소셜로그인 버튼을 누를 수 있는 상황이 생길 것 같습니다.

viewModel.loginState.observe(this@SocialLoginActivity) {
if (it != null) {
if (it.isSuccess) {
goToMainActivity()
Copy link
Contributor

Choose a reason for hiding this comment

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

로그인 성공한 이후 뒤로가기하면 다시 로그인 화면이 나와서는 안됩니다. 뒤로가기시 로그인 화면이 나오지 않도록 처리해주세요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

960849f 에서 수정했습니다.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants