-
Notifications
You must be signed in to change notification settings - Fork 662
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
Refactor voucher Authenticators into a single VoucherAuthenticator type #8448
Conversation
...s-core/src/main/java/com/stripe/android/payments/core/authentication/VoucherAuthenticator.kt
Show resolved
Hide resolved
...core/src/main/java/com/stripe/android/payments/core/authentication/WebIntentAuthenticator.kt
Outdated
Show resolved
Hide resolved
Diffuse output:
APK
DEX
|
@@ -172,6 +172,9 @@ sealed interface StripeIntent : StripeModel { | |||
} | |||
|
|||
sealed class NextActionData : StripeModel { | |||
interface DisplayVoucherDetails { |
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.
You could also add another val paymentMethodType: PaymentMethod.Type
here, and manually add it to each of the deserialized types. That would allow you to continue to have the data in the error reporter.
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.
It's a little weird though for DisplayOxxoDetails
to need to have a payment method property to know that it's related to Oxxo though, right? It's right there in the name. Maybe there's something I should be doing with reflection here
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.
It's a little weird, I'd think about the type (DisplayOxxoDetails) telling the interface (DisplayVoucherDetails) some metadata about the implementation.
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.
Took a stab at using the NextActionType
on the authenticatable
instead. Lmk what you think!
d653adf
to
ccd56c2
Compare
93622b0
to
a493dc1
Compare
Whenever we add a voucher LPM, we currently have to create a new Authenticator every time. However, we can instead make each piece of next action data conform to a common DisplayVoucherDetails interface and then delete all of the LPM-specific authenticators.
a493dc1
to
85e89d0
Compare
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.
Nice!
Summary
Deleted all LPM-specific Authenticators and replaced with a single
VoucherAuthenticator
type. Stacked on #8433Motivation
Whenever we add a voucher LPM, we currently have to create a new
Authenticator
every time. However, we can instead make each piece of next action data conform to a commonDisplayVoucherDetails
interface and then delete all of the LPM-specific authenticators.Testing