-
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
Support for Klarna with SI&PIwSFU. #8132
Conversation
Diffuse output:
APK
DEX
ARSC
|
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.
Can you add a screen recording or screenshot of this? Also do we have any tests the test the mandate is shown for PI+SFU and SI, but not for PIs?
initialValues: Map<IdentifierSpec, String?> | ||
) = createSectionElement( | ||
CountryElement( | ||
this.apiPath, | ||
DropdownFieldController( | ||
CountryConfig(KlarnaHelper.getAllowedCountriesForCurrency(currencyCode)), | ||
CountryConfig(), |
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'm not familiar with how Android works, but do we need this entire class if it is just now a normal country list?
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.
Hmm, I can try to replace it with our normal country placeholder. But we can't rip out klarna_country
from most of the things, just map it to our default CountrySpec.
testParameters = testParameters.copyPlaygroundSettings { settings -> | ||
settings[CheckoutModeSettingsDefinition] = CheckoutMode.SETUP | ||
}, |
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 it possible to test PI+SFU?
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 it is tested in the testKlarnaSetupFutureUsage
method.
Klarna LGTM, can't comment on some more specific Android stuff though |
payments-ui-core/src/main/java/com/stripe/android/ui/core/elements/MandateTextUI.kt
Outdated
Show resolved
Hide resolved
) | ||
) | ||
assertThat(KlarnaDefinition.isSupported(metadata)).isFalse() | ||
assertThat(GrabPayDefinition.isSupported(metadata)).isFalse() |
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.
Not for this PR, but it would be good if we didn't have to update this test every time we add support or more support for the LPM used in the test. I believe this is tied down by PaymentMethodRegistry
though correct?
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 alternative would be to create a "fake" PaymentMethodDefinition inline that is unsupported for setup.
In theory a lot of these tests have the same problem.
Summary
iOS PR: stripe/stripe-ios#3392
Demo:
klarna.mp4