Adding new PaymentSheet FormElement for Blik payment method. #7062
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
…tsheet-example-pln-currency
fionnbarrett-stripe
force-pushed
the
fionnbarrett/BLIK-form-element
branch
from
eb3e384 to
1f6a11e
Compare
|
Diffuse output: APKDEXARSC |
Comment on lines
103
to
105
| PaymentMethod.Type.Blik.code -> { | ||
| paymentMethodOptionsParams | ||
| } |
There was a problem hiding this comment.
We’re only passing a single value setupFutureUsage for the other payment method types, but are now using the entire PaymentMethodOptionsParams for Blik. Would it make sense to align the two?
This would mean that transformToPaymentMethodOptionsParams doesn’t only produce PaymentMethodOptionsParams.Blik, but also any other PaymentMethodOptionsParams, and we’d then pass them into here.
I’m not quite sure yet about which other changes (if any) we’d need to make for this to work correctly, but happy to help figure it out.
payments-ui-core/src/main/java/com/stripe/android/ui/core/FieldValuesToParamsMapConverter.kt
Outdated
Show resolved
Hide resolved
stripe-ui-core/src/main/java/com/stripe/android/uicore/elements/IdentifierSpec.kt
Outdated
Show resolved
Hide resolved
stripe-ui-core/src/main/java/com/stripe/android/uicore/elements/IdentifierSpec.kt
Outdated
Show resolved
Hide resolved
paymentsheet/src/main/java/com/stripe/android/paymentsheet/ui/AddPaymentMethod.kt
Outdated
Show resolved
Hide resolved
paymentsheet/src/main/java/com/stripe/android/paymentsheet/IntentConfirmationInterceptor.kt
Outdated
Show resolved
Hide resolved
| /** | ||
| * This function will convert fieldValuePairs to PaymentMethodOptionsParams. | ||
| */ | ||
| fun transformToPaymentMethodOptionsParams( |
There was a problem hiding this comment.
Can you annotate this with @RestrictTo(RestrictTo.Scope.LIBRARY_GROUP) so we hide it from the public API?
There was a problem hiding this comment.
The companion object that wraps this function is already annotated with @RestrictTo(RestrictTo.Scope.LIBRARY_GROUP) I believe this implies that all functions and properties within it are also restricted to the library group. I could be wrong though.
There was a problem hiding this comment.
Unfortunately, only the class is annotated, but not the companion object.
payments-ui-core/src/main/java/com/stripe/android/ui/core/elements/BlikConfig.kt
Outdated
Show resolved
Hide resolved
payments-ui-core/src/main/java/com/stripe/android/ui/core/elements/BlikConfig.kt
Outdated
Show resolved
Hide resolved
payments-ui-core/src/main/java/com/stripe/android/ui/core/elements/BlikSpec.kt
Outdated
Show resolved
Hide resolved
payments-ui-core/src/test/java/com/stripe/android/ui/core/elements/BlikConfigTest.kt
Show resolved
Hide resolved
paymentsheet/src/test/java/com/stripe/android/paymentsheet/PaymentSheetViewModelTest.kt
Outdated
Show resolved
Hide resolved
Comment on lines
+129
to
+130
| public final fun getBlik ()Lcom/stripe/android/uicore/elements/IdentifierSpec; | ||
| public final fun getBlikCode ()Lcom/stripe/android/uicore/elements/IdentifierSpec; |
There was a problem hiding this comment.
Can we mark these with @RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)?
For context: The entire companion object should have been marked with it, but I guess the original author forgot. Let’s at least make sure that new fields don’t leak.
There was a problem hiding this comment.
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
val Blik = IdentifierSpec("blik", requestDestination = RequestDestination.Options)
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
val BlikCode = IdentifierSpec("blik[code]", requestDestination = RequestDestination.Options)
I've added the @RestrictTo(RestrictTo.Scope.LIBRARY_GROUP) but apiDump and apiCheck don't seem to care and keep trying to add them back or failing
There was a problem hiding this comment.
Hmm, not sure about it then 🤔 Feel free to leave as-is and we can take care of that later.
| @@ -5,6 +5,11 @@ import androidx.annotation.RestrictTo | |||
| import kotlinx.parcelize.Parcelize | |||
| import kotlinx.serialization.Serializable | |||
|
|
|||
| enum class RequestDestination { | |||
There was a problem hiding this comment.
I know this is my name suggestion, but maybe there’s something more descriptive 🙈
There was a problem hiding this comment.
how about ApiParameterDestination?
payments-core/src/main/java/com/stripe/android/ConfirmStripeIntentParamsFactory.kt
Outdated
Show resolved
Hide resolved
…ripe-android into fionnbarrett/BLIK-form-element
…tMethodOptionsParams and fix apiCheck
tillh-stripe
approved these changes
Aug 31, 2023
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
Adding payment method Blik to mobile PaymentSheet.
Motivation
Testing
Screenshots
Changelog