Adding STPAddress containsContentFor...Fields: methods to check for any useful data
#834
Conversation
… any useful data These are like an `isEmpty` check for the address, but they check the fields needed for a particular billing or shipping address setting. This addresses #831, which was introduced when `STPShippingAddressViewController.billingAddress` started receiving an empty STPAddress from STPCard instead of a nil one. I had trouble deciding on the semantics of `containsContentForFields: .None` (should it return true or false?). I chose to always return NO. If the question is "Does this address have useful data for me, and I'm interested in none of the data from this address?", then false/NO makes sense. It also parallels with the behavior when messaging nil. Unit tests to come, and I'm not in love with the method names.
Stripe/STPAddCardViewController.m
Outdated
| BOOL buttonVisible = (needsAddress && self.shippingAddress != nil && !self.hasUsedShippingAddress); | ||
| BOOL requiredFields = self.configuration.requiredBillingAddressFields; | ||
| BOOL needsAddress = requiredFields != STPBillingAddressFieldsNone && !self.addressViewModel.isValid; | ||
| BOOL buttonVisible = (needsAddress && |
There was a problem hiding this comment.
There wasn't actually a bug here (shippingAddress was still nil), but I chose to make a parallel change to keep things consistent and guard against future changes.
There was a problem hiding this comment.
Looks good I think. Left a few comments but otherwise I think this all makes sense.
I think we should possibly also revisit our overwriting logic for these autofills and hide the button if the user has manually entered text into the fields, but that can be a separate task.
Stripe/PublicHeaders/STPAddress.h
Outdated
| @parameter desiredFields The billing address information the caller is interested in. | ||
| @return YES if there is any data in this STPAddress that's relevant for those fields. | ||
| */ | ||
| - (BOOL)containsContentForFields:(STPBillingAddressFields)desiredFields; |
There was a problem hiding this comment.
I think this should be billingAddressFields. It doesn't match the other one (which was written before billing address existed) but I think new methods going forward should make clear whether they are doing billing or shipping fields and at some point we can deprecate/rename the old ones. This is what we did for the newer Apple Pay billing address related method further down.
| BOOL needsAddress = self.configuration.requiredShippingAddressFields & PKAddressFieldPostalAddress && !self.addressViewModel.isValid; | ||
| BOOL buttonVisible = (needsAddress && self.billingAddress != nil && !self.hasUsedBillingAddress); | ||
| PKAddressField requiredFields = self.configuration.requiredShippingAddressFields; | ||
| BOOL needsAddress = requiredFields & PKAddressFieldPostalAddress && !self.addressViewModel.isValid; |
There was a problem hiding this comment.
For clarity I think we should put parens around the expression that is a bitwise AND here because it's easy to misinterpret what's happening.
danj-stripe
changed the title
containsContent...ForFields: methods to check for any useful datacontainsContent...ForFields: methods to check for any useful data
danj-stripe
changed the title
containsContent...ForFields: methods to check for any useful datacontainsContentFor...Fields: methods to check for any useful data
|
|
||
| // Test every property that contributes to the full address | ||
| // This is *not* refactoring-safe, but I think it's better than a bunch more duplicated code | ||
| for (NSString *propertyName in @[@"line1", @"line2", @"city", @"state", @"postalCode", @"country"]) { |
There was a problem hiding this comment.
Maybe do something like NSStringFromSelector(@selector(line1)) instead of @"line1" for some more compiler safety?
This matches `testContainsRequiredShippingAddressFields`
I made a dumb mistake in STPAddCardViewController, declared the wrong type for the `requiredFields` variable. The snapshot tests for Add Card and Shipping Address were apparently relying on the using a non-nil & empty Address to display the "Use <> address" button. Put a piece of data into the address to keep triggering the button.
* Sync strings * Add snapshot tests for localized mandate
Summary
These are like an
isEmptycheck for the address, but they check the fields neededfor a particular billing or shipping address setting.
Motivation
This addresses #831, which was introduced when
STPShippingAddressViewController.billingAddressstarted receiving an empty STPAddressfrom STPCard instead of a nil one.
Testing
Unit tests, and manual testing through the standard integration app.
Other Notes
I'm not in love with the method names.
I had trouble deciding on the semantics of
containsContentFor...Fields: .None(should itreturn true or false?). I chose to always return NO. If the question is "Does this address
have useful data for me, and I'm interested in none of the data from this address?", then
false/NO makes sense. It also parallels with the behavior when messaging nil.