-
Notifications
You must be signed in to change notification settings - Fork 661
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
Add Amazon Pay API binding #8028
Conversation
Diffuse output:
APK
DEX
|
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 activity is a more complex than necessary in that it supports selecting different payment intent types, but there's only one option (Payment) right now. This is because we'll soon be adding some other payment intent types and it will be easier to add them later on if I just keep this a little extra complicated for now.
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.
Would it make sense/be possible to refactor all of the LPMs to have a shared architecture? Obviously not on this PR, but just wondering if it's possible or makes sense.
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.
Yes, it would make sense and be possible. Maybe not for all of the LPMs, but for many of them.
A lot of the LPM example activities are of the form:
- (optionally) some number of radio buttons to select which payment type to use
- a pay button
And I think all of those activities could fairly easily use the same architecture.
Some of the LPM example activities (e.g. iDEAL) have more complex forms and I'm not sure it'd be worth the effort to generalize those ones (it seems harder and there are fewer of them, so the ROI of sharing the architecture seems lower)
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.
Makes sense, especially for the examples. Definitely worth keeping this idea in our back pocket.
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 thought there was also something we needed to do to remove the next actions from LUXE. (in lpms.json
).
example/src/main/java/com/stripe/example/activity/AmazonPayActivity.kt
Outdated
Show resolved
Hide resolved
fix incorrect merge conflict resolution
148664a
to
44d2513
Compare
0ed550e
to
ac882bf
Compare
@@ -1,6 +1,7 @@ | |||
# CHANGELOG | |||
|
|||
## XX.XX.XX - 2023-XX-XX | |||
* [Added][8028](https://github.com/stripe/stripe-android/pull/8028) Added support for Amazon Pay to API bindings. |
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 add this under a ### Payments
header?
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.
done -- thanks for reminding me about this!
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.
Would it make sense/be possible to refactor all of the LPMs to have a shared architecture? Obviously not on this PR, but just wondering if it's possible or makes sense.
c77c40b
to
6335519
Compare
Summary
Add Amazon Pay API bindings
Motivation
https://jira.corp.stripe.com/browse/MOBILESDK-1679
I set shouldRefreshIfIntentRequiresAction to true for the AmazonPay payment type. This is because we will need this set when we remove the redirect trampoline from AmazonPay and setting it now means that that change will be backwards compatible (i.e. old SDK versions will be work properly even when the redirect trampoline is removed).
Testing
Also manually verified (via debugger) that we do not currently refresh even when shouldRefreshIfIntentRequiresAction=true
Screenshots
Changelog
[Added]8028 Added support for Amazon Pay to API bindings.