-
Notifications
You must be signed in to change notification settings - Fork 663
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
Show blocked brands as disabled in CBC drop down #9979
base: master
Are you sure you want to change the base?
Conversation
Diffuse output:
APK
DEX
ARSC
|
paymentsheet/res/values/strings.xml
Outdated
@@ -222,4 +222,6 @@ | |||
<!-- A text notice shown when the user selects an expired card. --> | |||
<string name="stripe_wallet_update_expired_card_error">This card has expired. Update your card info or choose a different payment method.</string> | |||
<string name="stripe_wallet_expand_accessibility">Change selection</string> | |||
<!-- Shown in a dropdown picker next to a card brand that is not accepted by a merchant. E.g. "Visa (not accepted)" --> | |||
<string name="stripe_card_brand_not_accepted">(not accepted)</string> |
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 there one file where I can put these rather than in two strings.xml files?
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 can access the one in payments-core-ui
in paymentsheet
module. It'll just require a different import.
@@ -105,4 +105,6 @@ | |||
<string name="stripe_setup_button_label">Set up</string> | |||
<!-- Label for UPI ID number field on form --> | |||
<string name="stripe_upi_id_label">UPI ID</string> | |||
<!-- Shown in a dropdown picker next to a card brand that is not accepted by a merchant. E.g. "Visa (not accepted)" --> | |||
<string name="stripe_card_brand_not_accepted">(not accepted)</string> |
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.
Should this be a parameterized string (I know some languages won't have the same ordering as english).
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.
Ah, that's a good idea. I should do this on iOS too.
payments-ui-core/src/main/java/com/stripe/android/ui/core/elements/CardNumberController.kt
Show resolved
Hide resolved
label = if (enabled) { | ||
brand.displayName.resolvableString | ||
} else { | ||
brand.displayName.resolvableString + |
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.
This should be pretty easy to parameterize, let me know if you want to pair on it!
paymentsheet/res/values/strings.xml
Outdated
@@ -222,4 +222,6 @@ | |||
<!-- A text notice shown when the user selects an expired card. --> | |||
<string name="stripe_wallet_update_expired_card_error">This card has expired. Update your card info or choose a different payment method.</string> | |||
<string name="stripe_wallet_expand_accessibility">Change selection</string> | |||
<!-- Shown in a dropdown picker next to a card brand that is not accepted by a merchant. E.g. "Visa (not accepted)" --> | |||
<string name="stripe_card_brand_not_accepted">(not accepted)</string> |
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 can access the one in payments-core-ui
in paymentsheet
module. It'll just require a different import.
@@ -446,6 +446,7 @@ internal class VerticalModePaymentSheetActivityTest { | |||
// Verify that the error message appears | |||
formPage.assertErrorExists("American Express is not accepted") | |||
verticalModePage.assertPrimaryButton(isNotEnabled()) | |||
formPage.fillCardNumber("") |
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.
Why is this necessary?
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 notice some flaks occasionally, where when entering an Amex the CVC field changing from CVC to CVV, then "pasting" in an entire card doesn't update it back to CVC. Seems like an unrelated bug/test quirk.
The solution here is probably to just remove CVV usage for Amex, we did this on iOS and web no longer uses CVV.
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.
Overall this looks good to me. But I do worry about the test changes.
@@ -481,7 +481,8 @@ internal class CardNumberControllerTest { | |||
val secondReported = awaitItem() | |||
assertEquals(CardBrand.MasterCard, secondReported, "MasterCard should be reported once") | |||
|
|||
// Simulate entering an invalid card number | |||
// Simulate clearing the input and entering an invalid card number | |||
cardNumberController.onValueChange("") |
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.
These test changes do worry me a bit.
Summary
Motivation
https://jira.corp.stripe.com/browse/MOBILESDK-3036
Testing
Screenshots
Changelog
N/A