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/tgyuu/#141 #145

Merged
merged 23 commits into from
Mar 3, 2024
Merged

Feature/tgyuu/#141 #145

merged 23 commits into from
Mar 3, 2024

Conversation

tgyuuAn
Copy link
Member

@tgyuuAn tgyuuAn commented Feb 27, 2024

1. 📄 관련된 이슈 및 소개

2. 🔥 변경된 점

-최초 회원가입 시, 정보를 입력한 뒤 WAP회원 코드를 입력 성공해야만 회원 가입이 완료 된다.

3. ✅ 꼭 확인해줬으면 하는 부분

새벽에 급하게 코드를 짜느라고 네이밍이 좀 허접한게 있을 수도 있어요. 확인 바랍니다!

그리고 Manager Code확인하는 로직이 GetManageMentCode라고 되어있던데, 실제 로직은 비교하는 로직에다가 Boolean을 받더라고요. 체크 부탁드립니다!!

// com.wap.wapp.core.network.source.management.ManagementDataSourceImpl..

    override suspend fun getManagementCode(code: String): Result<Boolean> = runCatching {
        val result = firebaseFirestore.collection(CODES_COLLECTION)
            .whereEqualTo("management", code)
            .get()
            .await()

        result.isEmpty.not()
    }

4. 📸 스크린샷(선택)

bandicam2024-02-2804-43-11-862-ezgif com-crop

5. 💡알게된 혹은 궁금한 사항들

Android Compose Navigatoin 이동 시, 백스택을 모두 지우고 싶을 땐,

                navController.navigateToNotice(
                    navOptions { popUpTo(navController.graph.id) { inclusive = true } },
                )

을 navOptions으로 넣어준다!!!

@tgyuuAn tgyuuAn added 🌱기능🌱 새로운 기능 두두둥장! 🧩태규🧩 ENFP 안태규 24세 labels Feb 27, 2024
@tgyuuAn tgyuuAn requested a review from jeongjaino February 27, 2024 19:21
@tgyuuAn tgyuuAn self-assigned this Feb 27, 2024
@tgyuuAn tgyuuAn marked this pull request as ready for review February 27, 2024 19:46
@tgyuuAn tgyuuAn added the 🔥리뷰 기다리는 중🔥 PR을 올리고 코드리뷰를 기다리고 있는 상태입니다. label Feb 27, 2024
Copy link
Member

@jeongjaino jeongjaino left a comment

Choose a reason for hiding this comment

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

태규상 늦어서 죄송해요 힝구

부산에 내려오는데 시간도 많이 걸리구 ,,, (주저리주저리 핑계)

몇개 코멘트 남겨봣어용 히히
의견 기다릴게용 !

@@ -0,0 +1,5 @@
package com.wap.wapp.core.data.repository.auth

interface SignUpRepository {
Copy link
Member

Choose a reason for hiding this comment

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

태규상 아마

재가 SignInRepository를 따로 만들어놔서 그거보고 새롭게 클래스를 따로 만드신걸로 추측이 되는데용 ,,!!

제가 AuthRepository가 있음에도 따로 SigninRepo를 구성한건 로그인 동작은 Fragment위에서 동작해서 ActivityScoped로 주입해서 따로 구성해놨었어요 !!

그래서 로그인을 제외한 나머지 인증 관련 로직은 authRepo로 통일해서 구성했었어요 !

여기까지가 히스토리구 그래서 하고싶은 말은

사실 따로 클래스를 파도 관계는 없는데,
AuthRepo로 코드인증 메소드를 넣는 것도 괜찮을 것 같아서요 !

태규상의 의견이 궁금합니다 ㅎㅎ

Copy link
Member Author

Choose a reason for hiding this comment

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

@jeongjaino 오호 정확합니다.

AuthRepository안에 차곡차곡 넣어보겠습니다

Copy link
Member Author

Choose a reason for hiding this comment

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

여기 작업 완료했씁니다!

dataSource쪽도 마찬가지라서 AuthDataSource로 넣었어요!

class SignUpRepositoryImpl @Inject constructor(
private val signUpDataSource: SignUpDataSource,
) : SignUpRepository {
override suspend fun validationWapCode(code: String): Result<Boolean> =
Copy link
Member

Choose a reason for hiding this comment

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

메소드 네이밍이 명사시작보다는 동사로 시작하는게 낫지 않을까요 !?

그리구 wapCode도 좋은 것 같긴한데 (왑 동아리를 인증하는거니까)
저는 동아리 구성원 인증코드라는 의미로 memeberCode 추천드립니다 히히

Copy link
Member Author

Choose a reason for hiding this comment

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

@jeongjaino 헉 왜 validation으로 되어있을까요... 수정해놓겠씁니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

작업 완료 했씁니다!

_signUpEventFlow.emit(SignUpEvent.ValidationSuccess)
}

fun postUserProfile() = viewModelScope.launch {
Copy link
Member

Choose a reason for hiding this comment

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

태규상 따로 함수를 분리해야 하지않을까요 ?

회원을 등록하는 로직과 코드를 인증하는 로직이 한번에 처리되구

그래서 메소드 네이밍도 적절치 않은것 같아요

따로 분리하시구 인증성공일때만 유저 등록하는 매소드만 호출하는 형식으로 변경하면 좋을 것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

@jeongjaino 맞씁니다 진호상 수정해놓겠씁니다!!

@jeongjaino jeongjaino added ✏️수정 요청✏️ 코드 리뷰후 코드 수정 요청 and removed 🔥리뷰 기다리는 중🔥 PR을 올리고 코드리뷰를 기다리고 있는 상태입니다. labels Mar 1, 2024
@tgyuuAn
Copy link
Member Author

tgyuuAn commented Mar 3, 2024

@jeongjaino 수정 완료 했습니다!!!!

꼭 확인해줬으면 하는 부분 << 이거 체크 부탁드려요!

@tgyuuAn tgyuuAn added 🔥리뷰 기다리는 중🔥 PR을 올리고 코드리뷰를 기다리고 있는 상태입니다. 🌟머지 해주세요🌟 코드 리뷰가 완료된 뒤 PR을 올린사람이 Merge를 하면 되는 단계입니다. and removed ✏️수정 요청✏️ 코드 리뷰후 코드 수정 요청 🔥리뷰 기다리는 중🔥 PR을 올리고 코드리뷰를 기다리고 있는 상태입니다. labels Mar 3, 2024
@tgyuuAn tgyuuAn force-pushed the feature/tgyuu/#141 branch from 9c542c0 to 896f090 Compare March 3, 2024 04:12
@tgyuuAn tgyuuAn merged commit e64ac5f into develop Mar 3, 2024
@tgyuuAn tgyuuAn linked an issue Mar 3, 2024 that may be closed by this pull request
1 task
@tgyuuAn tgyuuAn deleted the feature/tgyuu/#141 branch March 8, 2024 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌟머지 해주세요🌟 코드 리뷰가 완료된 뒤 PR을 올린사람이 Merge를 하면 되는 단계입니다. 🌱기능🌱 새로운 기능 두두둥장! 🧩태규🧩 ENFP 안태규 24세
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE]: 회원 최초 가입 시, WAP 회원 인증
2 participants