Fix issue with authenticators launching activity in background #5667
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Diffuse output: APKDEX |
tillh-stripe
force-pushed
the
tillh/5508-fix-issue-launch-activity-in-background
branch
2 times, most recently
from
1284244 to
e37e132
Compare
tillh-stripe
commented
Oct 7, 2022
Comment on lines
+34
to
+38
| val lifecycleOwner = host.lifecycleOwner | ||
| lifecycleOwner.lifecycleScope.launch { | ||
| lifecycleOwner.whenResumed { | ||
| performAuthentication(host, authenticatable, requestOptions) | ||
| } | ||
| } |
There was a problem hiding this comment.
Using this in the public authenticate() method and calling a protected method that subclasses can override. Seemed better to me than making every single authenticator implement the lifecycle observation. Thoughts?
tillh-stripe
commented
Oct 7, 2022
| @@ -43,6 +49,8 @@ sealed class AuthActivityStarterHost { | |||
| override val statusBarColor: Int? | |||
| ) : AuthActivityStarterHost() { | |||
|
|
|||
| override val lifecycleOwner: LifecycleOwner = fragment.requireActivity() | |||
There was a problem hiding this comment.
Any concerns with this as the lifecycle owner?
There was a problem hiding this comment.
Can we guarantee that the activity is a lifecycle owner?
There was a problem hiding this comment.
Yes, because every activity is. I was just wondering if there are any downsides to using the activity lifecycle instead of the fragment lifecycle. Since we call fragment.startActivityForResult() below, I’ll just change the lifecycleOwner to also point to the fragment.
tillh-stripe
commented
Oct 7, 2022
|
|
||
| @RunWith(RobolectricTestRunner::class) | ||
| @OptIn(ExperimentalCoroutinesApi::class) | ||
| class GenericAuthenticatorTest { |
There was a problem hiding this comment.
Using this to test the lifecycle observation behavior. The tests for the authenticator subclasses stay (mostly) unchanged.
tillh-stripe
requested review from
brnunes-stripe,
ccen-stripe and
jameswoo-stripe
as code owners
tillh-stripe
force-pushed
the
tillh/5508-fix-issue-launch-activity-in-background
branch
from
e1d0d71 to
4b70ade
Compare
| } | ||
|
|
||
| @VisibleForTesting(otherwise = VisibleForTesting.PROTECTED) | ||
| abstract suspend fun performAuthentication( |
There was a problem hiding this comment.
Can this be protected?
There was a problem hiding this comment.
I made this public so that I could test it in unit tests, and added the VisibleForTesting annotation to limit its visibility outside of tests.
But now I’m thinking that I should instead call authenticate() in these tests and just mock host.lifecycleOwner. Will update the pull request!
There was a problem hiding this comment.
Pull request updated, so this is now protected
Instead of each authenticator awaiting the desired lifecycle state, we're now doing this in a single place in PaymentAuthenticator.
tillh-stripe
force-pushed
the
tillh/5508-fix-issue-launch-activity-in-background
branch
from
fe5ed6e to
8f4412a
Compare
tillh-stripe
force-pushed
the
tillh/5508-fix-issue-launch-activity-in-background
branch
from
8f4412a to
1742afc
Compare
brnunes-stripe
approved these changes
Oct 17, 2022
ccen-stripe
approved these changes
Oct 17, 2022
tillh-stripe
deleted the
tillh/5508-fix-issue-launch-activity-in-background
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Resolves: #5508
This pull request fixes an issue where launching a
PaymentAuthenticatorwhile in the background resulted in the payment being considered a failure in the SDK, even if the payment was successful. The problem seems to be limited to Android 10 and 11.Instead of launching the authentication activity right away, we now observe the lifecycle of
AuthActivityStarterHostand only start the activity when it’s resumed.Motivation
Bugfix.
Testing
Screenshots
N/A.
Changelog
Completed payments are no longer incorrectly reported as having failed if the app is backgrounded during confirmation on Android 10 and 11.