Fixes crash in STPAddCardViewController with some prefilled billing addresses #1004
Conversation
… check for valid table view inserts and removes. In cases where a zip code is required and the user's locale doesn't support zip codes but their billing address includes one (or vice-versa), this was causing a crash because we were trying to add or remove cells that were already there or were never there respectively.
|
|
||
| @implementation NSObject (STPSwizzling) | ||
|
|
||
| + (void)stp_swizzleClassMethod:(SEL)original withReplacement:(SEL)replacement |
There was a problem hiding this comment.
Did you consider using the STPAspects code instead?
This is a really simple usage, and since it's test code I don't think it needs to get fancy, mostly just curious.
There was a problem hiding this comment.
didn't even know that existed! I'll keep in mind for next time
Stripe/STPAddCardViewController.m
Outdated
| [self.tableView insertRowsAtIndexPaths:@[indexPath] withRowAnimation:UITableViewRowAnimationAutomatic]; | ||
| [self updateInputAccessoryVisiblity]; | ||
| NSInteger rowsInSection = [self.tableView numberOfRowsInSection:STPPaymentCardBillingAddressSection]; | ||
| if (rowsInSection != NSNotFound && rowsInSection < [self tableView:self.tableView numberOfRowsInSection:STPPaymentCardBillingAddressSection]) { |
There was a problem hiding this comment.
Is there a way to (cleanly) push this down into the STPAddressViewModel? It looks like it's already trying to track showingPostalCodeCell, and has logic for whether it's being added or removed based on updates to the address.
Or maybe we should move the _addressViewModel.delegate = self line to after we set the pre-filled info, because we don't care about updates to the ViewModel until after we've finished creating the views? That might allow us to avoid this complexity completely?
There was a problem hiding this comment.
Is there a way to (cleanly) push this down into the STPAddressViewModel?
I actually prefer the logic to live in the view controller because this situation is a result of layout passes/table view caching/etc. that I think the view controller is closer too than the view model (correct me if I'm wrong in my view model understanding :) ).
Or maybe we should move the _addressViewModel.delegate = self line to after we set the pre-filled info, because we don't care about updates to the ViewModel until after we've finished creating the views? That might allow us to avoid this complexity completely?
I thought about this. I believe that change would also fix the crash, but this solution is more robust
| if (rowsInSection != NSNotFound && index < (NSUInteger)rowsInSection) { | ||
| NSIndexPath *indexPath = [NSIndexPath indexPathForRow:index inSection:STPPaymentCardBillingAddressSection]; | ||
| [self.tableView deleteRowsAtIndexPaths:@[indexPath] withRowAnimation:UITableViewRowAnimationAutomatic]; | ||
| } | ||
| [self updateInputAccessoryVisiblity]; |
There was a problem hiding this comment.
Is it important that the call to updateInputAccessoryVisibility is inside the if block for add, but outside it for remove?
I don't think so, but wanted to see if there was something I'm missing.
There was a problem hiding this comment.
no, I'll move it out of the if in remove for consistency :)
I played around with the idea of a larger fix that avoids delegate calls that are too early, etc. I ended up deciding that this solution is not too hacky, pretty robust, and ultimately the best bang for our buck. Happy to reconsider/discuss if you feel differently! |
|
@danj-stripe pointed out that STPShippingAddressViewController has pretty much identical code. I'm going to try triggering the crash there and fix in this patch |
There was a problem hiding this comment.
Looks good.
I have some minor nits, where I think the tests could be better self-documenting and more robust to changes (given we just tripped over that in #1010).
| prefilledInformation:nil]; | ||
|
|
||
| [sut loadView]; | ||
| [sut viewDidLoad]; |
There was a problem hiding this comment.
WDYT about adding XCTAssertNoThrow calls around these?
I think that'd catch the NSInternalInconsistencyException that was being triggered. If so, it'd give a better failure message if it regresses, and it helps document what these tests are looking for. As it is, without context this test looks like a no-op.
Same for the matching tests of the other class.
| } | ||
|
|
||
| - (void)testPrefilledBillingAddress_addAddress { | ||
| [NSLocale stp_setCurrentLocale:[NSLocale localeWithLocaleIdentifier:@"en_ZW"]]; |
There was a problem hiding this comment.
WDYT about adding assertions (or a separate test?) that postalCodeIsRequiredForCountryCode: returns the expected values?
If ZW starts requiring a postal code, or we stop requiring it for US, we start testing a different code path, and the code could break silently.
It also helps document what's being tested, and why those countries were chosen.
| @@ -41,6 +42,53 @@ - (STPAddCardViewController *)buildAddCardViewController { | |||
| #pragma clang diagnostic push | |||
| #pragma clang diagnostic ignored "-Warc-performSelector-leaks" | |||
|
|
|||
| - (void)testPrefilledBillingAddress_removeAddress { | |||
There was a problem hiding this comment.
Do these need to be inside the #pragma clang diagnostic ignored "-Warc-performSelector-leaks" block? It might be slightly better to move them outside.
* Fix bug when shadow is nil and double apply of shadow * Hide shadow when using `StackViewWithSeparator` * Add shadow snapshots * update snapshots * resolve snapshot merge conflicts
Summary
Fixes STPAddCardViewController STPAddressViewModelDelegate methods to check for valid table view inserts and removes.
In cases where a zip code is required and the user's locale doesn't support zip codes but their billing address includes one (or vice-versa),
this was causing a crash because we were trying to add or remove cells that were already there or were never there respectively.
Motivation
#999
Testing
I wrote automated tests for the cases that were previously causing crashes