[feat] #94 이용약관 API 연동#96
Conversation
Walkthrough이용약관 조회/동의 서버 연동과 관련된 도메인 모델, 원격 API, 저장소 구현, UI(약관 화면) 및 내비게이션/리소스 변경을 추가합니다. Changes
Sequence DiagramsequenceDiagram
participant User
participant TermScreen
participant TermViewModel
participant TermRepository
participant TermService
participant Server
User->>TermScreen: 화면 진입 (EnterTermScreen)
TermScreen->>TermViewModel: dispatch(EnterTermScreen)
TermViewModel->>TermRepository: getTerms()
TermRepository->>TermService: getTerms()
TermService->>Server: GET /api/terms
Server-->>TermService: TermsResponse
TermService-->>TermRepository: BasicResponse<TermsResponse>
TermRepository-->>TermViewModel: Result<List<Term>>
TermViewModel->>TermScreen: 상태 업데이트(terms)
User->>TermScreen: 약관 체크/전체동의
TermScreen->>TermViewModel: dispatch(ClickAgreeTerm / ClickAgreeAll)
TermViewModel->>TermScreen: 상태 업데이트(isChecked)
User->>TermScreen: ClickNext
TermScreen->>TermViewModel: dispatch(ClickNext)
TermViewModel->>TermRepository: agreeTerms(termIds)
TermRepository->>TermService: agreeTerms(request)
TermService->>Server: POST /api/terms/agreements
Server-->>TermService: BasicNullableResponse<Unit>
TermService-->>TermRepository: Result<Unit>
TermRepository-->>TermViewModel: success
TermViewModel->>TermScreen: NavigateToMain
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In
`@core/data/src/main/java/com/neki/android/core/data/repository/impl/TermRepositoryImpl.kt`:
- Around line 17-24: The agreeTerms implementation returns the raw
BasicNullableResponse<Unit> instead of extracting its .data; inside agreeTerms
(which builds TermAgreementsRequest and calls termService.agreeTerms) unwrap the
service response and return its .data (or map to Unit) so the runSuspendCatching
block yields Result<Unit>, mirroring how FolderRepositoryImpl.createFolder and
PhotoRepositoryImpl.registerPhoto extract .data from their service responses.
In
`@feature/auth/impl/src/main/kotlin/com/neki/android/feature/auth/impl/term/component/TermContent.kt`:
- Around line 83-89: Add an error field to TermState (e.g., error: String? or
Throwable?), update TermViewModel.fetchTerms() to set that error on API failure
(instead of only logging) and clear it on retry/success, and make TermScreen
observe TermState.error to show an error UI or a Toast (similar to agreeTerms()
behavior) while preserving LoadingDialog; locate and modify TermState,
TermViewModel.fetchTerms(), and TermScreen (and the AgreementSection list in
TermContent.kt) so the UI renders a user-facing error message when fetchTerms
fails.
In
`@feature/auth/impl/src/main/kotlin/com/neki/android/feature/auth/impl/term/TermContract.kt`:
- Around line 11-12: The computed property isAllRequiredChecked currently
returns true when there are no required terms because terms.filter {
it.isRequired }.all { it.isChecked } is vacuously true; update the property in
TermContract (isAllRequiredChecked) to first ensure there is at least one
required term (e.g., terms.any { it.isRequired }) and only then return whether
all required terms are checked (terms.filter { it.isRequired }.all {
it.isChecked }) so that if no required terms exist the property returns false
and the "Next" button won’t be enabled.
In
`@feature/auth/impl/src/main/kotlin/com/neki/android/feature/auth/impl/term/TermScreen.kt`:
- Around line 40-43: TermSideEffect.NavigateUrl handling currently calls
Intent(Intent.ACTION_VIEW, Uri.parse(sideEffect.url)) and
context.startActivity(intent) without guarding against invalid URIs or missing
handlers; wrap the start flow in a try-catch that catches
ActivityNotFoundException (and optionally
IllegalArgumentException/URISyntaxException) around context.startActivity,
validate or safely parse the URL before creating the Intent, and provide a
fallback (e.g., show a toast/error via viewModel/event or log the error) so
TermSideEffect.NavigateUrl does not crash the app when Uri.parse(sideEffect.url)
or startActivity fails.
In
`@feature/auth/impl/src/main/kotlin/com/neki/android/feature/auth/impl/term/TermViewModel.kt`:
- Around line 68-80: fetchTerms currently logs failures but provides no user
feedback; update the TermViewModel.fetchTerms signature to accept a
postSideEffect: (TermSideEffect) -> Unit (or the existing side-effect poster
used elsewhere) and call postSideEffect(...) inside the .onFailure block (after
reduce { copy(isLoading = false) }) to emit the same toast/error side effect
used by agreeTerms; then update all places that call fetchTerms (notably the
onIntent handler) to pass the postSideEffect parameter through so failures show
a toast to the user.
🧹 Nitpick comments (3)
feature/auth/impl/src/main/kotlin/com/neki/android/feature/auth/impl/login/LoginViewModel.kt (2)
40-59: 카카오 로그인 중복 호출 방지가 필요합니다.
signUp은viewModelScope.launch로 비동기 실행되므로, 사용자가 버튼을 빠르게 연타하면isLoading상태 반영 전에 코루틴이 중복 실행될 수 있습니다.state.isLoading이true이면 early return하는 가드를 추가하는 것을 권장합니다.♻️ 수정 제안
private fun signUp( kakaoIdToken: String, reduce: (LoginState.() -> LoginState) -> Unit, postSideEffect: (LoginSideEffect) -> Unit, - ) = viewModelScope.launch { + ) { + if (store.state.isLoading) return + viewModelScope.launch { reduce { copy(isLoading = true) } authRepository.loginWithKakao(kakaoIdToken) .onSuccess { authResult -> tokenRepository.saveTokens( accessToken = authResult.accessToken, refreshToken = authResult.refreshToken, ) checkTermAgreementState(postSideEffect) } .onFailure { exception -> Timber.e(exception) postSideEffect(LoginSideEffect.ShowToastMessage("가입에 실패했습니다. 다시 시도해주세요.")) } reduce { copy(isLoading = false) } + } }
61-76:getUserInfo실패 시 약관 화면으로 이동하는 폴백 동작을 확인해 주세요.
getUserInfo()실패 시 무조건NavigateToTerm으로 이동하는데, 네트워크 오류나 토큰 만료 등 다양한 원인이 있을 수 있습니다. 이 경우 약관 화면에서도 API 호출이 실패할 가능성이 높으므로, 사용자에게 에러 토스트를 함께 표시하거나 재시도 가능한 경로를 제공하는 것이 좋을 수 있습니다.core/model/src/main/java/com/neki/android/core/model/Term.kt (1)
1-12:isChecked는 UI 상태이므로 도메인 모델에 포함하는 것이 적절한지 검토 필요.
isChecked는 사용자의 체크박스 선택 여부를 나타내는 프레젠테이션 레이어 상태입니다. 도메인 모델(Term)에 포함하면 서버 응답 매핑과 UI 상태가 혼재됩니다. 현재 규모에서는 실용적인 접근이지만, 향후 ViewModel 내부의 별도 UI 상태(Map<Long, Boolean>등)로 분리하면 관심사 분리가 명확해집니다.
ikseong00
left a comment
ikseong00
left a comment
There was a problem hiding this comment.
기존 코드를 리팩토링하는 형태라 크게 리뷰할 사항은 없어보이네요, 좋습니다!
다만, when 절에서, -> 이후에 한 줄만 오는 경우에 대해서 2가지 케이스가 있는데,
이거는 하나로 맞추는 방향으로 가면 좋을 것 같아요!
개인적으로는 라인 수는 적게 하는 편을 선호했는데, 괄호가 있으면 가독성이 조금 더 좋아지고, 추후 Intent에 의한 작업이 추가되었을 때(ex) reduce), 수정 사항이 한 줄만 있을 것 같다는 생각도 드네요
TermIntent.EnterTermScreen -> fetchTerms(reduce)
TermIntent.ClickBack -> {
postSideEffect(TermSideEffect.NavigateBack)
}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@feature/auth/impl/src/main/kotlin/com/neki/android/feature/auth/impl/term/TermViewModel.kt`:
- Around line 21-26: fetchTerms() currently only logs errors with
Timber.e(error) and provides no user feedback; update the error branch in
fetchTerms() to emit a UI side-effect similar to agreeTerms() by dispatching
TermSideEffect.ShowToastMessage(...) (or an appropriate error message) via the
store or side-effect emitter used in this ViewModel so the user is informed when
fetching terms fails; locate the fetchTerms() method in TermViewModel and add
the side-effect emission where Timber.e(error) is called.
🧹 Nitpick comments (2)
feature/auth/impl/src/main/kotlin/com/neki/android/feature/auth/impl/login/LoginViewModel.kt (1)
61-76:getUserInfo()실패 시 약관 화면으로 이동하는 폴백 전략이 합리적입니다.사용자 정보 조회 실패 시 약관 화면으로 안내하는 것은 안전한 기본 동작입니다. 다만, 이미 약관에 동의한 사용자가 네트워크 오류 등으로 인해 불필요하게 약관 화면을 다시 보게 될 수 있습니다. 필요 시 재시도 로직 추가를 고려해 보세요.
feature/auth/impl/src/main/kotlin/com/neki/android/feature/auth/impl/term/TermViewModel.kt (1)
37-43: "약관 전체 동의"가 필수 약관만 토글합니다.
ClickAgreeAll처리 시isRequired인 약관만 토글하고 선택 약관은 변경하지 않습니다. UI에서 "약관 전체 동의"라고 표시되므로 사용자 기대와 실제 동작이 다를 수 있습니다. 의도된 동작이라면 주석을 추가하거나, 전체 약관을 토글하도록 수정을 고려해 주세요.전체 약관 토글 시 제안
TermIntent.ClickAgreeAll -> { - val shouldCheckAll = !state.isAllRequiredTermChecked + val allChecked = state.terms.all { it.isChecked } val updatedTerms = state.terms.map { term -> - if (term.isRequired) term.copy(isChecked = shouldCheckAll) else term + term.copy(isChecked = !allChecked) }.toImmutableList() reduce { copy(terms = updatedTerms) } }
| val store: MviIntentStore<TermState, TermIntent, TermSideEffect> = | ||
| mviIntentStore( | ||
| initialState = TermState(), | ||
| onIntent = ::onIntent, | ||
| initialFetchData = { store.onIntent(TermIntent.EnterTermScreen) }, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.kt" | xargs grep -l "MviIntentStoreImpl" | head -5Repository: YAPP-Github/27th-App-Team-2-Android
Length of output: 145
🏁 Script executed:
rg -n "class MviIntentStoreImpl" --type=kt -A 30Repository: YAPP-Github/27th-App-Team-2-Android
Length of output: 105
🏁 Script executed:
rg -n "initialFetchData" --type=kt -B 2 -A 5Repository: YAPP-Github/27th-App-Team-2-Android
Length of output: 105
🏁 Script executed:
cat ./core/ui/src/main/java/com/neki/android/core/ui/MviIntentStore.ktRepository: YAPP-Github/27th-App-Team-2-Android
Length of output: 2468
🏁 Script executed:
rg -n "initialFetchData" -A 10 --glob="*.kt"Repository: YAPP-Github/27th-App-Team-2-Android
Length of output: 16794
🏁 Script executed:
cat feature/auth/impl/src/main/kotlin/com/neki/android/feature/auth/impl/term/TermViewModel.ktRepository: YAPP-Github/27th-App-Team-2-Android
Length of output: 3710
fetchTerms() 에러 처리에서 사용자 피드백 부재
현재 fetchTerms() 메서드는 실패 시 Timber.e(error) 만 호출하고 있습니다. 반면 agreeTerms() 메서드는 실패 시 TermSideEffect.ShowToastMessage() 로 사용자에게 피드백을 제공합니다. 오류 발생 시 사용자에게 상태를 알릴 수 있도록 약관 조회 실패에 대한 피드백을 추가해주세요.
🤖 Prompt for AI Agents
In
`@feature/auth/impl/src/main/kotlin/com/neki/android/feature/auth/impl/term/TermViewModel.kt`
around lines 21 - 26, fetchTerms() currently only logs errors with
Timber.e(error) and provides no user feedback; update the error branch in
fetchTerms() to emit a UI side-effect similar to agreeTerms() by dispatching
TermSideEffect.ShowToastMessage(...) (or an appropriate error message) via the
store or side-effect emitter used in this ViewModel so the user is informed when
fetching terms fails; locate the fetchTerms() method in TermViewModel and add
the side-effect emission where Timber.e(error) is called.
Ojongseok
left a comment
Ojongseok
left a comment
There was a problem hiding this comment.
사실 저는 -> 이후 한줄로 작성이 가능한 경우에도 {} 괄호를 여는 것이 습관이 되었는데 익성님 코드를 보면서 괄호를 열지 않는 것이 라인이 많이 줄어드는 것 같아 한줄로 작성하려 신경을 쓰고 있었는데 해당 부분에서는 놓쳤군요..
다만, 말씀하신 것 처럼 괄호를 여는 것이 가독성은 확실히 좋은 것 같고, 이후 코드가 수정되었을 경우에 수정사항이 줄어든다는 장점이 있는 것 같습니다. 그치만 현재 저희 lint 룰에서는 {...} 중괄호 이후 다음 블럭 이전에 개행이 한줄 더 필요합니다
...
is TermIntent.ClickAgreeTerm -> {
reduce { copy(terms = updatedTerms) }
}
is TermIntent.ClickTermNavigateUrl -> {
postSideEffect(TermSideEffect.NavigateUrl(intent.term.url))
}
이런 식으로 말이죠. 코드 라인이 배로 길어지게 될 것 같아요 ㅎㅎ...
한줄로 커버가 가능한 경우에는 대부분 postSideEffect() or reduce {}가 대부분이라 한줄로 적는다고 해서 가독성이 크게 떨어지지는 않는 것 같습니다-! 한줄로 작성하는 것으로 통일하는거 어떠실까요?
통일한다면 Intent, Effect when절 모두에 적용하면 될 것 같습니다!
좋습니다!! 이 부분도 이슈업하고 클로드 룰에 추가하면 어떨까요? |
|
@ikseong00 #103 , #104 해당 PR에서 협의한 내용 이슈로 등록해 두었고, 해당 PR은 병합하도록 하겠습니다. |
2 participants
🔗 관련 이슈
📙 작업 설명
💬 추가 설명 or 리뷰 포인트 (선택)
조회/동의와 관련해 기존에 앱에서 하드코딩된 부분이 API를 통해 진행되도록 변경되었습니다. UI는 이전과 동일합니다.Summary by CodeRabbit
릴리스 노트
새로운 기능
개선사항