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

Fix STPAddressViewModel.isValid to use country when requiredBillingFields = .Zip #853

Merged
merged 3 commits into from
Dec 12, 2017

Conversation

danj-stripe
Copy link
Contributor

Summary

We had a bug, where STPAddressViewModel objects that only require the Postal Code did
not use the country when calculating isValid, but did use the country when deciding
whether or not to show the postal code field.

This resulted in a stuck user: they couldn't add a credit card, because the
STPAddCardViewController was waiting for them to enter a postal code, but didn't allow
them to.

Additionally, in this case we were also incorrectly showing the inputAccessoryToolbar
with a 'Next' button that did nothing.

Motivation

Fixes #840

Testing

Augmented STPAddressViewModelTest to capture this case, and protect against regressions.

Further, manual testing in the Standard Integration app, both US & Macau (req'd postal &
non-req'd postal code), and all three values of STPBillingAddressFields (none, zip, full)

Why does the last assert of `testIsValid_Zip` fail?
An `STPAddressViewModel` with required billing fields == `Zip` pass a `nil` country to
determine if it `isValid`, and therefore will *always* incorrectly report that countries
without postal codes are invalid.
… `STPAddressViewModel`

When there's no country table cell/text field, the country information is stored
in `addressFieldTableViewCountryCode`. This fixes #840, because the address used to
check `isValid` is created through this method.

I think this is a desirable change overall. When `setAddress:` is called, it extracts the
`STPAddress.country` value, and stores it into `addressFieldTableViewCountryCode`. It
makes a lot of sense to me to reverse that when reading the `STPAddress` back out of the
object.
…rent country

I observed this bug while testing. The toolbar with 'Next' (above the number
pad) was visible for countries without postal codes. When the paymentCell was completely
filled, the 'Next' button enabled, but did nothing when tapped.

Instead of special-casing on the `configuration.requiredBillingAddressFields`, just
use the logic already built into `STPAddressViewModel` to know whether there'll be
a field to change to.
Copy link
Contributor

@bdorfman-stripe bdorfman-stripe left a comment

Choose a reason for hiding this comment

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

LGTM!

@danj-stripe danj-stripe merged commit 2459cb7 into master Dec 12, 2017
@danj-stripe danj-stripe deleted the danj/bugfix/840-optional-zip branch December 12, 2017 19:00
danj-stripe added a commit that referenced this pull request Dec 15, 2017
danj-stripe added a commit that referenced this pull request Dec 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants