-
Notifications
You must be signed in to change notification settings - Fork 709
[Identity] Fail out of verification flow on analyzer failure #12223
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
base: master
Are you sure you want to change the base?
Conversation
|
Diffuse output: APKDEX |
identity/src/main/java/com/stripe/android/identity/viewmodel/IdentityScanViewModel.kt
Show resolved
Hide resolved
| ) | ||
| } | ||
|
|
||
| override fun onAnalyzerFailure(t: Throwable): Boolean { |
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.
Why are we overriding here an not in CameraViewModel?
Also, are there any other places where we log an error (using the logger) but don't actually send it to the backend?
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.
Why are we overriding here an not in CameraViewModel?
IdentityScanViewModel is where verificationFlowFinishable is, which is where we can react properly to the analyzer error.
Also, are there any other places where we log an error (using the logger) but don't actually send it to the backend?
Yes, likely many many dozens, unfortunately. We can tackle that, but like we chatted about yesterday in our sync that should be it's own effort and not tied to this if at all possible - that way we can make sure this is covered and schedule that work in line with other priorities if needed
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.
Could we just log in CameraViewModel and also add a timeout in case the user cannot complete document capture after a set time (20 seconds for example) and show the error screen after a timeout? I believe the timeout matches the web experience.
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 believe this is resolved after this discussion, but do let me know if there are still open questions https://hubble.corp.stripe.com/saved-queries/t/30457
luisv-stripe
left a comment
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.
Thanks!
Simply fails out of the verification flow if we see a bug like we saw recently where R8 stripped out parts of Tensorflow lite.
I tested this out by removing the newly added Proguard consumer rules, and verified that trying to go through the verification flow will fail you out. I also made sure this does not trigger in any other circumstance.