Skip to content
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 postal validation #5456

Merged
merged 3 commits into from Aug 26, 2022
Merged

Conversation

jameswoo-stripe
Copy link
Contributor

@jameswoo-stripe jameswoo-stripe commented Aug 24, 2022

Summary

Validate the postal code in payment sheet

Motivation

Testing

  • Added tests
  • Modified tests
  • Manually verified

Screenshots

Changelog

  • [Added] Postal code validation in PaymentSheet for CA, US zip codes

Copy link
Collaborator

@tillh-stripe tillh-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well! Just a few comments with UX improvement ideas.

SimpleTextFieldController(
val textFieldConfig = when (it) {
FieldType.PostalCode -> {
addressField.schema?.nameType
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be removed?

object CA : CountryPostalFormat(
minimumLength = 6,
maximumLength = 6,
regexPattern = Regex("([a-zA-Z]\\d)+")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we allow spaces for Canadian postal codes? Currently, it’s not possible to enter them.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And another 🇨🇦 question: Should we auto-capitalize the postal code? I find a1b 2c3 looks odd, and A1B 2C3 looks much nicer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added spacing regex, won't fix the auto-capitalization

Added through visual transformation

Comment on lines 36 to 43
override fun filter(userTyped: String): String =
if (
setOf(KeyboardType.Number, KeyboardType.NumberPassword).contains(keyboard)
) {
userTyped.filter { it.isDigit() }
} else {
userTyped
}.replace(Regex("\\s*"), "")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On iOS, it looks like we restrict the input length to the country’s maximum length. Can we do the same here?

maximumLength = 9,
regexPattern = Regex("\\d+")
)
object Other : CountryPostalFormat(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this copied from stripe.js or somewhere else? Just making sure we're not being too strict on the requirements. Better to not validate than to validate it wrong and end up not accepting a valid value, which frustrates the user.

val maximumLength: Int,
val regexPattern: Regex
) {
object CA : CountryPostalFormat(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're accepting the space, should it take up to 7 characters now?

maximumLength = 6,
regexPattern = Regex("[a-zA-Z]\\d[a-zA-Z][\\s-]?\\d[a-zA-Z]\\d")
)
object US : CountryPostalFormat(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually a 9-digit zip code is formatted as 12345-6789.
This and the spaces for CA would be better handled by a VisualTransformation, like we have for the card number and others. For these two it would be a pretty simple one, I think it's worth taking a look.

@jameswoo-stripe jameswoo-stripe merged commit 190f799 into master Aug 26, 2022
@jameswoo-stripe jameswoo-stripe deleted the jameswoo/paymentsheet-postalcode branch August 26, 2022 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants