From e4f6a8374a414e372e7a7b5e5280ae1edc9097c5 Mon Sep 17 00:00:00 2001 From: Dan Jackson Date: Wed, 5 Sep 2018 13:05:56 -0700 Subject: [PATCH 1/3] Adding test that exposes the bug, `defaultSource` isn't updating as it should --- Tests/Tests/STPCustomerContextTest.m | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/Tests/Tests/STPCustomerContextTest.m b/Tests/Tests/STPCustomerContextTest.m index f61c07ce615..cc5f44b1b56 100644 --- a/Tests/Tests/STPCustomerContextTest.m +++ b/Tests/Tests/STPCustomerContextTest.m @@ -203,29 +203,40 @@ - (void)testAttachSourceToCustomerCallsAPIClientCorrectly { - (void)testSelectDefaultCustomerSourceCallsAPIClientCorrectly { STPEphemeralKey *customerKey = [STPFixtures ephemeralKey]; - STPCustomer *expectedCustomer = [STPFixtures customerWithSingleCardTokenSource]; + STPCustomer *initialCustomer = [STPFixtures customerWithSourcesFromJSONKeys:@[STPTestJSONCard, + STPTestJSONSourceCard] + defaultSource:STPTestJSONCard]; + id initialSource = initialCustomer.defaultSource; + id changedSource = [initialCustomer.sources lastObject]; + id mockAPIClient = OCMClassMock([STPAPIClient class]); [self stubRetrieveCustomerUsingKey:customerKey - returningCustomer:expectedCustomer + returningCustomer:initialCustomer expectedCount:1 mockAPIClient:mockAPIClient]; - STPSource *expectedSource = [STPFixtures cardSource]; + + XCTAssertNotEqual(initialSource, changedSource, @"ensure call to selectDefaultCustomerSource: is changing the defaultSource"); XCTestExpectation *exp = [self expectationWithDescription:@"updateCustomer"]; - NSDictionary *expectedParams = @{@"default_source": expectedSource.stripeID}; + NSDictionary *expectedParams = @{@"default_source": changedSource.stripeID}; OCMStub([mockAPIClient updateCustomerWithParameters:[OCMArg isEqual:expectedParams] usingKey:[OCMArg isEqual:customerKey] completion:[OCMArg any]]) .andDo(^(NSInvocation *invocation) { STPCustomerCompletionBlock completion; [invocation getArgument:&completion atIndex:4]; - completion([STPFixtures customerWithSingleCardTokenSource], nil); + completion([STPFixtures customerWithSourcesFromJSONKeys:@[STPTestJSONCard, + STPTestJSONSourceCard] + defaultSource:STPTestJSONSourceCard], + nil); [exp fulfill]; }); id mockKeyManager = [self mockKeyManagerWithKey:customerKey]; XCTestExpectation *exp2 = [self expectationWithDescription:@"selectDefaultSource"]; STPCustomerContext *sut = [[STPCustomerContext alloc] initWithKeyManager:mockKeyManager]; - [sut selectDefaultCustomerSource:expectedSource completion:^(NSError *error) { + XCTAssertEqualObjects(sut.customer.defaultSource, initialSource, @"defaultSource should be the defaultSource of the Customer returned by the API"); + [sut selectDefaultCustomerSource:changedSource completion:^(NSError *error) { XCTAssertNil(error); + XCTAssertEqualObjects(sut.customer.defaultSource, changedSource, @"defaultSource should be the new source"); [exp2 fulfill]; }]; From 2f14e151c60edba0ae48e47e1387223d01a9ad69 Mon Sep 17 00:00:00 2001 From: Dan Jackson Date: Wed, 5 Sep 2018 13:19:34 -0700 Subject: [PATCH 2/3] Use response from new `STPCustomer` object instead of previous one Looks like a copy/paste error from f3b2d7250a50c1705c1776cb312f7a30676a86d2 in #864. `STPCustomerContext` holds onto a `STPCustomer`. There are three places where it retrieves a new `STPCustomer` - and when it does it needs to update the `sources` of that customer based on the `STPCustomerContext.includeApplePaySources` boolean. The bug is that we were using the *old* `STPCustomer.allResponseFields` (via `self.customer`) instead of the `allResponseFields` property on the newly retrieved `STPCustomer` object. In isolation, it's a pretty obvious bug (once you know to look for it). There's no reason we'd want to retrieve a Customer, and then use the response data for the previous object to update the `sources`/`defaultSource` properties of the new one. There is a fourth place that updates the sources, and that's inside `setIncludeApplePaySources:`. In that case, we're updating the existing customer, and using the existing customer's `allResponseFields`, so `self.customer.allResponseFields` is correct there. --- Stripe/STPCustomerContext.m | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Stripe/STPCustomerContext.m b/Stripe/STPCustomerContext.m index 057332d048f..2ba3d1d9f35 100644 --- a/Stripe/STPCustomerContext.m +++ b/Stripe/STPCustomerContext.m @@ -87,7 +87,7 @@ - (void)retrieveCustomer:(STPCustomerCompletionBlock)completion { } [STPAPIClient retrieveCustomerUsingKey:ephemeralKey completion:^(STPCustomer *customer, NSError *error) { if (customer) { - [customer updateSourcesWithResponse:self.customer.allResponseFields + [customer updateSourcesWithResponse:customer.allResponseFields filteringApplePay:!self.includeApplePaySources]; self.customer = customer; } @@ -138,7 +138,7 @@ - (void)selectDefaultCustomerSource:(id)source completion:(ST usingKey:ephemeralKey completion:^(STPCustomer *customer, NSError *error) { if (customer) { - [customer updateSourcesWithResponse:self.customer.allResponseFields + [customer updateSourcesWithResponse:customer.allResponseFields filteringApplePay:!self.includeApplePaySources]; self.customer = customer; } @@ -168,7 +168,7 @@ - (void)updateCustomerWithShippingAddress:(STPAddress *)shipping completion:(STP usingKey:ephemeralKey completion:^(STPCustomer *customer, NSError *error) { if (customer) { - [customer updateSourcesWithResponse:self.customer.allResponseFields + [customer updateSourcesWithResponse:customer.allResponseFields filteringApplePay:!self.includeApplePaySources]; self.customer = customer; } From 4f01c931b506d19ae15c4ad31b10ddada308a3fa Mon Sep 17 00:00:00 2001 From: Dan Jackson Date: Wed, 5 Sep 2018 14:52:03 -0700 Subject: [PATCH 3/3] Remove `response` parameter from `STPCustomer updateSources...` method, use `allResponseFields` directly Better fix than the previous commit. Instead of allowing the caller to pass in the response dictionary (it might be the wrong one!) use the one in `allResponseFields`. --- Stripe/STPCustomer+Private.h | 9 ++++----- Stripe/STPCustomer.m | 6 +++--- Stripe/STPCustomerContext.m | 12 ++++-------- 3 files changed, 11 insertions(+), 16 deletions(-) diff --git a/Stripe/STPCustomer+Private.h b/Stripe/STPCustomer+Private.h index 025ca83ed31..f764f513b08 100644 --- a/Stripe/STPCustomer+Private.h +++ b/Stripe/STPCustomer+Private.h @@ -13,14 +13,13 @@ NS_ASSUME_NONNULL_BEGIN @interface STPCustomer () /** - Replaces the customer's sources and defaultSource with the contents of a - Customer API response. + Replaces the customer's `sources` and `defaultSource` based on whether or not + they should include Apple Pay sources. More details on documentation for + `STPCustomerContext includeApplePaySources` - @param response The Customer API response @param filterApplePay If YES, Apple Pay sources will be ignored */ -- (void)updateSourcesWithResponse:(NSDictionary *)response - filteringApplePay:(BOOL)filterApplePay; +- (void)updateSourcesFilteringApplePay:(BOOL)filterApplePay; @end diff --git a/Stripe/STPCustomer.m b/Stripe/STPCustomer.m index 87bb6d9b41b..c9e00121eab 100644 --- a/Stripe/STPCustomer.m +++ b/Stripe/STPCustomer.m @@ -82,13 +82,13 @@ + (nullable instancetype)decodedObjectFromAPIResponse:(nullable NSDictionary *)r } customer.sources = @[]; customer.defaultSource = nil; - [customer updateSourcesWithResponse:dict filteringApplePay:YES]; customer.allResponseFields = dict; + [customer updateSourcesFilteringApplePay:YES]; return customer; } -- (void)updateSourcesWithResponse:(NSDictionary *)response - filteringApplePay:(BOOL)filterApplePay { +- (void)updateSourcesFilteringApplePay:(BOOL)filterApplePay { + NSDictionary *response = self.allResponseFields; NSArray *data; NSDictionary *sourcesDict = [response stp_dictionaryForKey:@"sources"]; if (sourcesDict) { diff --git a/Stripe/STPCustomerContext.m b/Stripe/STPCustomerContext.m index 2ba3d1d9f35..3a2f01cde88 100644 --- a/Stripe/STPCustomerContext.m +++ b/Stripe/STPCustomerContext.m @@ -55,8 +55,7 @@ - (void)setCustomer:(STPCustomer *)customer { - (void)setIncludeApplePaySources:(BOOL)includeApplePaySources { _includeApplePaySources = includeApplePaySources; - [self.customer updateSourcesWithResponse:self.customer.allResponseFields - filteringApplePay:!includeApplePaySources]; + [self.customer updateSourcesFilteringApplePay:!includeApplePaySources]; } - (BOOL)shouldUseCachedCustomer { @@ -87,8 +86,7 @@ - (void)retrieveCustomer:(STPCustomerCompletionBlock)completion { } [STPAPIClient retrieveCustomerUsingKey:ephemeralKey completion:^(STPCustomer *customer, NSError *error) { if (customer) { - [customer updateSourcesWithResponse:customer.allResponseFields - filteringApplePay:!self.includeApplePaySources]; + [customer updateSourcesFilteringApplePay:!self.includeApplePaySources]; self.customer = customer; } if (completion) { @@ -138,8 +136,7 @@ - (void)selectDefaultCustomerSource:(id)source completion:(ST usingKey:ephemeralKey completion:^(STPCustomer *customer, NSError *error) { if (customer) { - [customer updateSourcesWithResponse:customer.allResponseFields - filteringApplePay:!self.includeApplePaySources]; + [customer updateSourcesFilteringApplePay:!self.includeApplePaySources]; self.customer = customer; } if (completion) { @@ -168,8 +165,7 @@ - (void)updateCustomerWithShippingAddress:(STPAddress *)shipping completion:(STP usingKey:ephemeralKey completion:^(STPCustomer *customer, NSError *error) { if (customer) { - [customer updateSourcesWithResponse:customer.allResponseFields - filteringApplePay:!self.includeApplePaySources]; + [customer updateSourcesFilteringApplePay:!self.includeApplePaySources]; self.customer = customer; } if (completion) {