-
Notifications
You must be signed in to change notification settings - Fork 628
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 paymentMethodTypes to CustomerAdapter. #8015
Conversation
Diffuse output:
|
Still need to filter retrievePaymentMethods by the supplied paymentMethodTypes |
paymentMethodTypes?.filter { PaymentMethod.Type.fromCode(it) == null }?.takeIf { it.isNotEmpty() }?.run { | ||
val cause = IllegalStateException("Invalid payment method types provided (${joinToString()}).") | ||
return CustomerAdapter.Result.failure(cause, null) | ||
} | ||
|
||
val supportedPaymentMethodCodes = supportedPaymentMethodTypes.map { it.code }.toSet() | ||
val unsupportedPaymentMethods = paymentMethodTypes?.minus(supportedPaymentMethodCodes) ?: emptyList() | ||
if (unsupportedPaymentMethods.isNotEmpty()) { | ||
val unsupportedCodes = unsupportedPaymentMethods.joinToString() | ||
val cause = IllegalStateException("Unsupported payment method types provided ($unsupportedCodes).") | ||
return CustomerAdapter.Result.failure(cause, 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.
Two differences between iOS and Android in the impl:
- On iOS we have CustomerSheetError with specific types. If i read this correctly, everything is of type IllegalStateException, so there's no specific type
- On iOS, if a payment method deserializes to unknown, we effectively throw the same error -- unsupported.
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 don't have error types (and it doesn't look like we expose exceptions of this nature for CustomerSheet or PaymentSheet).
I think I'm ok with both of those differences between platforms, but happy to discuss a change if you feel strongly.
): Result<ElementsSessionWithMetadata> { | ||
val initializationMode = PaymentSheet.InitializationMode.DeferredIntent( | ||
PaymentSheet.IntentConfiguration( | ||
mode = PaymentSheet.IntentConfiguration.Mode.Setup(), | ||
paymentMethodTypes = customerAdapter.paymentMethodTypes ?: emptyList() |
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.
If a user passes in ["card", "klarna"] where does klarna get filtered out?
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.
Our supported payment method types are here: https://github.com/stripe/stripe-android/pull/8015/files#diff-29d4dede5084ae494929fa55b68d50e8e88be4887076b5531f70664fc85525d7R211
Which results in a failure here: https://github.com/stripe/stripe-android/pull/8015/files#diff-29d4dede5084ae494929fa55b68d50e8e88be4887076b5531f70664fc85525d7R56
There's also a test for this here: https://github.com/stripe/stripe-android/pull/8015/files#diff-3ee906c7fa8222a439ab144d5208047ef06018b7f1ec18c6a87064077b88e8d4R194
Summary
Supplying payment method types will filter available payment methods (new and saved) to the list provided.
Motivation
https://jira.corp.stripe.com/browse/MOBILESDK-1401