-
Notifications
You must be signed in to change notification settings - Fork 658
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 konbini to PaymentSheet. #7308
Conversation
Diffuse output:
APK
DEX
ARSC
|
813b805
to
cceea17
Compare
@@ -52,6 +53,11 @@ class FieldValuesToParamsMapConverter { | |||
blikCode | |||
) | |||
} | |||
} else if (code == PaymentMethod.Type.Konbini.code) { |
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 special casing kills me. But it seemed out of scope to refactor this while adding it. We should discuss what it would take to make this better as a follow up.
<!-- Label for Konbini Payment Method --> | ||
<string name="stripe_paymentsheet_payment_method_konbini">Konbini</string> |
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.
Should this be in the no-translate file?
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 is translated differently in Japanese.
@@ -102,6 +102,9 @@ internal class ConfirmPaymentIntentParamsFactory( | |||
PaymentMethod.Type.Blik.code -> { | |||
optionsParams | |||
} | |||
PaymentMethod.Type.Konbini.code -> { | |||
optionsParams |
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.
Marking this for a future refactor.
/** | ||
* URL of a webpage containing the voucher for this payment. | ||
*/ |
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.
/** | |
* URL of a webpage containing the voucher for this payment. | |
*/ |
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.
In theory we could expose this for bindings. I just copied what the others did.
// nextActionData.hostedVoucherUrl will never be null as AuthenticatorRegistry won't direct it here | ||
authUrl = nextActionData.hostedVoucherUrl.takeIf { it!!.isNotEmpty() } | ||
?: throw IllegalArgumentException("null hostedVoucherUrl for DisplayKonbiniDetails") | ||
returnUrl = null |
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.
Providing a return URL would cause this to be opened in a Chrome Custom Tab instead of a web view. Should we do that?
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 is what the other vouchers do, so I just kept with it.
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.
We should probably start using CCT.
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.
Are you peaceful if I leave it like this for this PR and submit a new PR to update all voucher based LPMs to switch to CCT?
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.
Or rather, what's the motivation?
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 for one-click auth to be enabled, we need CCT.
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.
Also, I think some payment methods are using CCT while others are using webview right? For example, bancontact uses CCT.
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.
These wouldn't have auth though, you'd need to take them to a different app, or to a physical location, so they wouldn't benefit from that.
37afea9
to
d2e9350
Compare
|
||
internal val KonbiniRequirement = PaymentMethodRequirements( | ||
piRequirements = setOf(Delayed), | ||
siRequirements = setOf(Delayed), |
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.
Is this correct? I thought SI is not supported?
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.
Good catch. Thanks!
internal val KonbiniRequirement = PaymentMethodRequirements( | ||
piRequirements = setOf(Delayed), | ||
siRequirements = null, | ||
confirmPMFromCustomer = true, |
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 we also don't want this to be true
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.
🤦 thank you
Summary
Note both Checkout and web PE don't use their normal 'phone number' field to collect it - it's just a normal text field (11 digits max, numbers only) and doesn't interact with merchant configuration related to the collection of a phone number (e.g. Checkout shows a separate phone field if you tell it to collect a phone number).
Motivation
LPM Sprint
Screen_recording_20230914_093653.webm