-
Notifications
You must be signed in to change notification settings - Fork 992
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
Send and decode CPMs from elements/sessions #4637
Conversation
🚨 New dead code detected in this PR: STPElementsSession.swift:50 warning: Property 'customPaymentMethods' is assigned, but never used Please remove the dead code before merging. If this is intentional, you can bypass this check by adding the label ℹ️ If this comment appears to be left in error, double check that the flagged code is actually used and/or make sure your branch is up-to-date with |
@_spi(STP) import StripeCore | ||
@_spi(STP) import StripePayments | ||
|
||
struct CustomPaymentMethod: Decodable { |
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.
Noting that all these properties (besides type) are intentionally optional, see an example response here for why: https://github.com/stripe/stripe-ios/pull/4637/files#diff-c71d9097e968f7c5fd01a4cb9425c80f53998412b3026808eca160f1ca08b35fR139
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.
(Just a comment, no changes required, but something to think about for the future:)
I verified that you have modeled this correctly based on there response object, but feels like we need to add a layer of abstraction or something -- e.g. if error is nil, then i'd expect displayName
, logoUrland
isPreset` not to be nil -- and if error has a value, then i'd expect those other values to be nil.
let logoUrl: URL? | ||
|
||
/// If true, this custom payment method was created using a preset in the Stripe dashboard | ||
let isPreset: Bool? |
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 does it mean for isPreset to be nil vs false?
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.
Good question, I touched on it in this comment: #4637 (comment)
For more of an explanation though, this is optional based on the behavior of the backend. There are instances where the backend may not return us "is_preset" in the response so our model client-side needs to handle that. The case where isPreset is nil, is the case where there is no value specified by the backend. The goal of CustomPaymentMethod
is to match the backend definition/behavior.
@@ -169,6 +175,23 @@ extension STPElementsSession: STPAPIResponseDecodable { | |||
return epms | |||
}() | |||
|
|||
let customPaymentMethods: [CustomPaymentMethod] = { | |||
let customPaymentMethodDataKey = "custom_payment_method_data" | |||
guard response[customPaymentMethodDataKey] != nil, !(response[customPaymentMethodDataKey] is NSNull) else { |
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.
Why do we need to check both response[customPaymentMethodDataKey] != nil
and !(response[customPaymentMethodDataKey] is NSNull)
?
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.
If you look higher up in this function you will see we do this for external payment methods and customer. The reason is b/c:
- nil check: This checks if the key doesn't exist in the response dictionary at all.
- NSNull check: This checks if the key exists but its value is explicitly set to NSNull (null)
Here is an example:
Case 1: Key doesn't exist (nil check)
{
"other_data": "some value",
"payment_methods": ["card", "paypal"]
// "custom_payment_method_data" key is completely absent
}
Case 2:
{
"other_data": "some value",
"payment_methods": ["card", "paypal"],
"custom_payment_method_data": null
}
We need to be able to handle both scenarios just to be safe.
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.
Looks fine to me
Example/PaymentSheet Example/PaymentSheet Example/PlaygroundController.swift
Show resolved
Hide resolved
@_spi(STP) import StripeCore | ||
@_spi(STP) import StripePayments | ||
|
||
struct CustomPaymentMethod: Decodable { |
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.
(Just a comment, no changes required, but something to think about for the future:)
I verified that you have modeled this correctly based on there response object, but feels like we need to add a layer of abstraction or something -- e.g. if error is nil, then i'd expect displayName
, logoUrland
isPreset` not to be nil -- and if error has a value, then i'd expect those other values to be nil.
StripePaymentSheet/StripePaymentSheetTests/PaymentSheet/STPElementsSessionTest.swift
Show resolved
Hide resolved
|
||
let customPaymentMethodsArray: [[String: Any]] = [ | ||
[ | ||
"logo_url": "stripe.com", |
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.
should logo_url be a fully qualified url: etc: https:// -- i'd expect this fail given the type is of URL
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.
Added https!
Summary
Motivation
Testing
Changelog
N/A