-
Notifications
You must be signed in to change notification settings - Fork 645
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
Link Verification VM #9542
Link Verification VM #9542
Conversation
Diffuse output:
APK
DEX
|
value = run { | ||
val linkAccount = linkAccountManager.linkAccount.value | ||
if (linkAccount == null) { | ||
dismissWithResult(LinkActivityResult.Failed(NoLinkAccountFoundForVerification())) |
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.
There should be a link account (unverified) in the link account manager at this point
link/src/main/java/com/stripe/android/link/repositories/LinkRepository.kt
Outdated
Show resolved
Hide resolved
link/src/main/java/com/stripe/android/link/ui/verification/VerificationViewModel.kt
Outdated
Show resolved
Hide resolved
viewModelScope.launch { | ||
linkAccountManager.confirmVerification(code).fold( |
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.
I think having a launch here means that we effectively have some code like:
viewModelScope.launch {
otpCode.collect {
viewModelScope.launch { ...
Should we instead just have the second launch also be on the top-level? That seems better than having them nested (from a clarity perspective and also efficiency)
link/src/main/java/com/stripe/android/link/ui/verification/VerificationViewModel.kt
Outdated
Show resolved
Hide resolved
link/src/main/java/com/stripe/android/link/ui/verification/VerificationViewModel.kt
Outdated
Show resolved
Hide resolved
link/src/main/java/com/stripe/android/link/ui/verification/VerificationViewModel.kt
Outdated
Show resolved
Hide resolved
link/src/main/java/com/stripe/android/link/ui/verification/VerificationViewState.kt
Show resolved
Hide resolved
955d61f
to
113c7d7
Compare
113c7d7
to
e5f964b
Compare
for (i in 0 until otpElement.controller.otpLength) { | ||
otpElement.controller.onValueChanged(i, "") | ||
} |
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.
Can you use otpElement.controller.reset here instead of needing to call onValueChanged for each index? It looks like the reset function is doing this same thing
} | ||
|
||
@Test | ||
fun `init sends analytics event`() = runTest(dispatcher) { |
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.
This test name doesn't seem accurate anymore - it looks like this is testing that start verification is called, but doesn't actually test analytics
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.
there are several other tests which mention analytics in their names but which don't actually test analytics anymore. Can you update them please?
Summary
View model to handle link account verification
Motivation
JIRA
Testing
Screenshots
Screen.Recording.2024-10-30.at.1.58.45.PM.mov
Changelog