Incorporate the Activity result APIs to resolve loadPaymentData calls #8381
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
…o google-pay-activity-result # Conflicts: # payments-core/src/main/java/com/stripe/android/googlepaylauncher/GooglePayLauncherActivity.kt
…ty result launcher
…-pay-activity-result # Conflicts: # payments-core/src/main/java/com/stripe/android/googlepaylauncher/GooglePayLauncherActivity.kt # payments-core/src/main/java/com/stripe/android/googlepaylauncher/GooglePayLauncherViewModel.kt
|
Diffuse output: APKDEX |
jaynewstrom-stripe
changed the title
jaynewstrom-stripe
requested review from
tillh-stripe,
samer-stripe and
amk-stripe
amk-stripe
reviewed
Apr 30, 2024
| @@ -39,10 +40,6 @@ internal class GooglePayLauncherActivity : AppCompatActivity() { | |||
| GooglePayLauncherViewModel.Factory(args) | |||
| } | |||
|
|
|||
| private val errorReporter: ErrorReporter by lazy { | |||
There was a problem hiding this comment.
I think as part of fixing the merge conflicts, the actual error reporting for some of these errors was removed. Can we fix that? Or if there are error cases that no longer exist, can we remove those cases from the ErrorReporter ErrorEvent enum?
There was a problem hiding this comment.
(could be fixed by sharing more code with GooglePayPaymentMethodLauncherActivity, see my comment there)
There was a problem hiding this comment.
Yeah, I thought I fixed these, but looks like I missed some. Good callout. I'll follow up.
There was a problem hiding this comment.
I didn't realize we had two classes that were so similar. So when I saw this missing after the rebase, I thought I fixed it. But I just added it to the other one 🤦.
I think it's probably worth having in both. And it's also probably worth consolidating these.
| private fun onGooglePayResult(taskResult: ApiTaskResult<PaymentData>) { | ||
| when (taskResult.status.statusCode) { | ||
| CommonStatusCodes.SUCCESS -> { | ||
| val paymentDataJson = JSONObject(taskResult.result!!.toJson()) |
There was a problem hiding this comment.
what's the impact of result being null? Will we crash?
There was a problem hiding this comment.
(could be fixed by sharing more code with GooglePayPaymentMethodLauncherActivity, see my comment there)
| when (taskResult.status.statusCode) { | ||
| CommonStatusCodes.SUCCESS -> { | ||
| val result = taskResult.result | ||
| if (result != null) { |
There was a problem hiding this comment.
ah it actually looks like this activity handles the taskResult.result being null and it reports errors, both of which are not present in GooglePayLauncherActivity. If that's not intentional, should we try to share more code between these two classes?
| val statusMessage = status.statusMessage.orEmpty() | ||
| errorReporter.report( | ||
| ErrorReporter.ExpectedErrorEvent.GOOGLE_PAY_FAILED, | ||
| additionalNonPiiParams = mapOf("status_message" to statusMessage) |
There was a problem hiding this comment.
should we include the status code here too?
|
After digging through this more, I think we need to rewrite quite a bit of our google pay integrations to work more seamlessly with the Android lifecycle. During that refactor, we could also attempt to reuse more of the google pay integration logic between the two. Created a backlog task to do this: https://jira.corp.stripe.com/browse/MOBILESDK-2011 |
amk-stripe
approved these changes
May 1, 2024
samer-stripe
approved these changes
May 1, 2024
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Not necessary for this PR, but I added kotlinx-coroutines-play-services for GooglePayRepository in my refactor for that class. Should we align and use that instead or vice versa by moving this extension to a core lib?
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
Finishing the work started in #7895
Motivation
Upgrade to the latest playServicesWallet version.
Testing