add includeApplePaySources to CustomerContext #864

bg-stripe · 2017-12-18T19:05:11Z

Addresses #842 by adding a new includeApplePaySources property to CustomerContext

danj-stripe

I think you'll want to make some minor changes based on my feedback, but not necessarily everything.

danj-stripe · 2017-12-18T20:32:29Z

Stripe/PublicHeaders/STPCustomerContext.h

@@ -46,6 +46,15 @@ NS_ASSUME_NONNULL_BEGIN
 */
 - (void)clearCachedCustomer;

+/**
+ By default, `STPCustomerContext` will filter Apple Pay sources when it retrieves
+ a Customer object. If you are using `STPCustomerContext` to back your own UI and


Is it worth mentioning why we filter? Something like "because the Apple Pay sources should not be re-used and shouldn't be offered to users as a new payment source"? (I think)

Ah yep, I'll add a note.

danj-stripe · 2017-12-18T20:48:02Z

Tests/Tests/STPCustomerContextTest.m

+                         expectedCount:2];
+    id mockKeyManager = [self mockKeyManagerWithKey:customerKey];
+    STPCustomerContext *sut = [[STPCustomerContext alloc] initWithKeyManager:mockKeyManager];
+    [sut clearCachedCustomer];


I was confused, and had to figure out why expectedCount = 2. Could you change it to 1 and just delete this call to clearCachedCustomer, and test the same things? Or is there something that's being added by this that I've missed?

Maybe this is for parity with the test below?

ah yeah, I'll add a comment explaining the 2. CustomerContext fetches a customer on initialization, but there's no callback we can use for assertions, so I just used another retrieveCustomer call.

Oh actually, after changing the behavior of includeApplePaySources per below, we can just use the cached customer and change this to 1

danj-stripe · 2017-12-18T20:49:34Z

Tests/Tests/STPCustomerContextTest.m

+    id mockKeyManager = [self mockKeyManagerWithKey:customerKey];
+    STPCustomerContext *sut = [[STPCustomerContext alloc] initWithKeyManager:mockKeyManager];
+    sut.includeApplePaySources = YES;
+    [sut clearCachedCustomer];


I wonder if changing the includeApplePaySources property should clear the cached customer automatically?

Or maybe it just calls the updateSourcesWithResponse:filteringApplePay: method, instead of hitting the server again?

ah interesting, it does seem reasonable for includeApplePaySources to update the current customer too. I'll update.

danj-stripe · 2017-12-18T21:17:11Z

Stripe/STPCustomerContext.m

@@ -48,6 +49,10 @@ - (void)clearCachedCustomer {
 }

 - (void)setCustomer:(STPCustomer *)customer {
+    if (self.includeApplePaySources) {
+        [customer updateSourcesWithResponse:customer.allResponseFields


This is kind of funky/subtle/non-obvious.

This mutates the STPCustomer object passed into the setter, which the caller still has a reference to.

I didn't understand why you changed customer -> self.customer on L92 below.

I'm guessing because setCustomer: has side effects, but because they're just references to the same object, I don't think that change actually changes the behavior?

I see that this is a private property, and I think there're only three places that call the setter and none of them use the customer afterwords, so it's fine as-is (although maybe a well-placed & well-worded comment could help?).

I think it might be easier to understand & maintain if this was (duplicated and) moved up a level in the call stack to those places that receive STPCustomer objects from STPAPI. In each of them it's more clear that the STPCustomerContext completely owns the object & mutating it is fine, as well as not having (potentially) surprising behavior "hidden" in the setter.

Ah yeah that's good feedback, putting it in the setter was kinda lazy. I'll move it up in the stack.

danj-stripe · 2017-12-18T21:23:15Z

Stripe/STPCustomer.m

+    NSMutableArray *sources = [NSMutableArray new];
+    for (id contents in data) {
+        if ([contents isKindOfClass:[NSDictionary class]]) {
+            if ([contents[@"object"] isEqualToString:@"card"]) {


I recognize this is just the old code, but this line makes an assumption about the type of contents[@"object]" by using an NSString-only method (isEqualToString:), where the parsing code so far has been diligently checking types.

Maybe -> isEqual:@"card"

side note: reading this method makes me want as?, flatMap, filter & find :)

danj-stripe · 2017-12-18T21:45:55Z

Stripe/STPCustomer.m

+    if (![data isKindOfClass:[NSArray class]]) {
+        return;
+    }
+    NSString *defaultSourceId;


I think you should probably reset self.defaultSource = nil.

ex: calling with filterApplePay = NO and then with YES, for a customer whose default source is ApplePay, will still have the apple pay defaultSource on the customer object.

ah yep, good catch!

danj-stripe · 2017-12-18T21:47:10Z

Stripe/STPCustomer+Private.h

+@interface STPCustomer ()
+
+/**
+ Updates the customer's sources and defaultSource with a Customer API response.


I wonder if it's worth changing this from "Updates" to "Replaces", since you're getting all new objects?

bg-stripe · 2017-12-18T22:39:42Z

updated, re-r? @danj-stripe

danj-stripe

Those changes look great!

I also like the updates to property attributes, but I think they've highlighted an unhandled nullability corner case.

danj-stripe · 2017-12-18T22:47:34Z

Stripe/STPCustomer.m

+        data = response[@"sources"][@"data"];
+    }
+    if (![data isKindOfClass:[NSArray class]]) {
+        return;


When called from decodedObjectFromAPIResponse:, this will return a customer with sources = nil (if sources.data is not an NSArray), but the property declaration for sources is nonnull

I wasn't (and still am not) sure how we want to handle a malformed response. I like that we're not crashing! :)

I also thought I liked the idea that you're not resetting sources and defaultSources if this is not the initialization pass. However, this is private, and it looks like you're always just using the cached response, so maybe it'd be easier to just reset to empty array & nil?

Another option would be to set sources to the empty array in decodedObjectFromAPIResponse: before calling this method. I think I like this option slightly better.

ah good catch, I agree that the second option is better – I'll update. Working on some related guards against malformed responses in my next PR :)

updated in ffb5ff6

danj-stripe

Sweet!

Looks like a copy/paste error from f3b2d72 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.

add includeApplePaySources switch to CustomerContext

c3b03a2

bg-stripe assigned danj-stripe Dec 18, 2017

bg-stripe requested a review from danj-stripe December 18, 2017 19:05

danj-stripe suggested changes Dec 18, 2017

View reviewed changes

stripe-ci assigned bg-stripe and unassigned danj-stripe Dec 18, 2017

bg-stripe added 3 commits December 18, 2017 17:23

elaborate on why apple pay sources are filtered

effaea0

update docstring for updateSourcesWithResponse

f6ea528

move updateSourcesWithResponse out of setter

f3b2d72

bg-stripe assigned danj-stripe and unassigned bg-stripe Dec 18, 2017

danj-stripe reviewed Dec 18, 2017

View reviewed changes

danj-stripe assigned bg-stripe and unassigned danj-stripe Dec 18, 2017

set sources + defaultSource to default values when decoding

ffb5ff6

bg-stripe force-pushed the bg-applepay-filtering branch from ef387e5 to ffb5ff6 Compare December 18, 2017 23:33

bg-stripe assigned danj-stripe and unassigned bg-stripe Dec 18, 2017

danj-stripe approved these changes Dec 18, 2017

View reviewed changes

stripe-ci added the approved label Dec 18, 2017

stripe-ci assigned bg-stripe and unassigned danj-stripe Dec 18, 2017

bg-stripe merged commit 7046189 into master Dec 19, 2017

bg-stripe deleted the bg-applepay-filtering branch December 19, 2017 17:07

bg-stripe mentioned this pull request Dec 20, 2017

Subscribing using Apple Pay #873

Closed

danj-stripe mentioned this pull request Sep 5, 2018

Remove response parameter from STPCustomer method to update Sources with/without Apple Pay #1020

Merged

mludowise-stripe pushed a commit that referenced this pull request Mar 24, 2022

Launch connections redirect API implementation (#864)

543f655

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add includeApplePaySources to CustomerContext #864

add includeApplePaySources to CustomerContext #864

bg-stripe commented Dec 18, 2017 •

edited

Loading

danj-stripe left a comment

danj-stripe Dec 18, 2017

bg-stripe Dec 18, 2017

danj-stripe Dec 18, 2017

danj-stripe Dec 18, 2017

bg-stripe Dec 18, 2017

bg-stripe Dec 18, 2017

danj-stripe Dec 18, 2017

danj-stripe Dec 18, 2017

bg-stripe Dec 18, 2017

danj-stripe Dec 18, 2017

bg-stripe Dec 18, 2017

danj-stripe Dec 18, 2017

danj-stripe Dec 18, 2017

danj-stripe Dec 18, 2017

bg-stripe Dec 18, 2017

danj-stripe Dec 18, 2017

bg-stripe Dec 18, 2017

bg-stripe commented Dec 18, 2017

danj-stripe left a comment

danj-stripe Dec 18, 2017

bg-stripe Dec 18, 2017

bg-stripe Dec 18, 2017

danj-stripe left a comment

add includeApplePaySources to CustomerContext #864

add includeApplePaySources to CustomerContext #864

Conversation

bg-stripe commented Dec 18, 2017 • edited Loading

danj-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bg-stripe commented Dec 18, 2017

danj-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danj-stripe left a comment

Choose a reason for hiding this comment

bg-stripe commented Dec 18, 2017 •

edited

Loading