-
Notifications
You must be signed in to change notification settings - Fork 976
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 createsCardSources <> didCreatePaymentResult , Apple Pay #893
Conversation
self.onTokenCreation(token, ^(NSError *tokenCreationError){ | ||
if (tokenCreationError) { | ||
self.lastError = tokenCreationError; | ||
id<STPSourceProtocol> source; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is source
used for? It kind of looks like you meant to pass source
to the onSourceCreation()
call, but it's getting result
right now, so I think this is dead code.
edit: neat, static analyzer agrees with me and failed the build 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm more confident that you meant to pass source
instead of result
: down in the definition of applePaySourceHandler
, the code used to be constructing a STPPaymentResult
with token.card
, and now it just passes source
directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh derp, good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so i think i understand why this worked (returning STPToken
instead of the child STPCard
for an apple pay source). we don't display apple pay sources in the UI, so it didn't matter that STPToken
doesn't conform to <STPPaymentMethod>
(the protocol defining how a payment method is displayed). i'll still fix for consistency with how we handle non-apple pay sources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in eb09444, re-r?
@@ -582,9 +582,10 @@ - (void)requestPayment { | |||
else if ([self requestPaymentShouldPresentShippingViewController]) { | |||
[self presentShippingViewControllerWithNewState:STPPaymentContextStateRequestingPayment]; | |||
} | |||
else if ([self.selectedPaymentMethod isKindOfClass:[STPCard class]]) { | |||
else if ([self.selectedPaymentMethod isKindOfClass:[STPCard class]] || | |||
[self.selectedPaymentMethod isKindOfClass:[STPSource class]]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT about checking [self.selectedPaymentMethod conformsToProtocol:@protocol(STPSourceProtocol)]
instead?
I don't know:
- whether there are other classes that conform to the protocol at we want excluded from this branch (making the check invalid)
- how likely it is that we'll add new classes that do conform to the protocol that should take this branch (updated check would future-proof us)
- how likely that we'll remove conformance to the protocol from these classes (which would break the check)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mm, interesting idea. I think it's unlikely that we'll create other class that conform to this protocol (the protocol is mostly a compatibility shim for card tokens to work with sources). I think we should leave it this way and explicitly reference STPCard
, which we should start treating as a legacy class (card tokens will continue to be supported for a long time, but sources are the preferred integration).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
r? @danj-stripe
This fixes two issues:
I tested Apple Pay and didCreatePaymentResult in the example app with sources. I've also updated the example app to always create card sources.