-
Notifications
You must be signed in to change notification settings - Fork 644
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 PaymentConfirmationMediator
& PaymentConfirmationRegistry
#9335
base: master
Are you sure you want to change the base?
Conversation
paymentsheet/src/main/java/com/stripe/android/paymentsheet/PaymentConfirmationRegistry.kt
Show resolved
Hide resolved
Diffuse output:
APK
|
b06b901
to
0c8c521
Compare
0c8c521
to
5134d5b
Compare
PaymentConfirmationMediator
& PaymentConfirmationRegistry
PaymentConfirmationMediator
& PaymentConfirmationRegistry
@@ -200,7 +200,7 @@ internal class IntentConfirmationFlowTest { | |||
assertThat(failAction.cause).isInstanceOf(IllegalStateException::class.java) | |||
assertThat(failAction.cause.message).isEqualTo("An error occurred!") | |||
assertThat(failAction.message).isEqualTo(R.string.stripe_something_went_wrong.resolvableString) | |||
assertThat(failAction.errorType).isEqualTo(PaymentConfirmationErrorType.Internal) | |||
assertThat(failAction.errorType).isEqualTo(PaymentConfirmationErrorType.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.
The handler used to return Payment
for IntentConfirmationDefinition
which is the more accurate error type to return. Updated the definition to do this as well.
intent = PAYMENT_INTENT, | ||
confirmationOption = PaymentConfirmationOption.PaymentMethod.Saved( | ||
initializationMode = ARGS_CUSTOMER_WITH_GOOGLEPAY.initializationMode, | ||
paymentMethod = CARD_PAYMENT_METHOD, | ||
optionsParams = null, | ||
shippingDetails = null, | ||
) | ||
), | ||
deferredIntentConfirmationType = 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.
In reality, PaymentSheet
should be unaware of this. When we create a fake handler, we should update all the tests to test the interactions with the handler rather than the whole confirmation flow. That should be tested in a more integrated style.
This is a pretty big PR that primarily from the added tests (test code accounts for ~600 lines). I can look into breaking it up into multiple PRs (one for the mediator, one for the registry) if that's easier to read but would just be adding placeholder code that would be immediately removed after merging the registry. There are some smaller changes that I can pull out of this PR but won't make of a dent to this PR size. |
data class Complete( | ||
val intent: StripeIntent, | ||
val confirmationOption: PaymentConfirmationOption, | ||
val deferredIntentConfirmationType: DeferredIntentConfirmationType? = 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.
@jaynewstrom-stripe I think you mentioned potentially injecting this attribute. I could have another mediator object that is shared between PaymentSheet
and IntentConfirmationDefinition
that is able to push this value into the object for PaymentSheet
to grab afterwards.
Other option is to have PaymentConfirmationMetadata
where we can store attributes like this and use.
Summary
Adds
PaymentConfirmationMediator
to mediate payment confirmation types and an accompanyingPaymentConfirmationRegistry
type to define all the payment confirmation types and create mediators from.Motivation
Helps move all the
ConfirmationOption
andLauncher
abstractions into a mediator to manage the definitions. Following this PR, we can start slowly moving all the internal definitions of the handler into their own definitions then make the registry a parameter of the handler.Testing