-
Notifications
You must be signed in to change notification settings - Fork 995
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
Read the default_payment_method field from elements session and use that as the default PM #4313
Changes from 37 commits
1403094
71c5516
e3ecc2c
fb86faa
b283446
9ed5ed2
e689449
3c36c97
c749e6e
aac6ae1
b25eb4b
5936f96
0fca0dc
c226e09
4d146b6
f858536
8d1c90b
c70c6a1
d999b9e
ac5189a
ffcf6e4
403fdcb
c8fbfef
2f515cd
0a92c87
a89890c
bd51b31
3c69384
f239d20
5fad2d3
4da3034
05407cc
eaae456
7022a11
f5fbd58
cb12138
79f2a94
b592b55
74c9db8
dbc2beb
40574a6
2e8b49e
a2a37e6
0b6781f
7d3f1c4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,7 +36,14 @@ struct ElementsCustomer: Equatable, Hashable { | |
} | ||
|
||
// Optional | ||
// to test default payment methods reading from back end, hard-code a valid default payment method | ||
// later, when API calls to get and update default payment method are available, that will no longer be needed | ||
let defaultPaymentMethod = response["default_payment_method"] as? String | ||
return ElementsCustomer(paymentMethods: paymentMethods, defaultPaymentMethod: defaultPaymentMethod, customerSession: customerSession) | ||
} | ||
|
||
static func getDefaultPaymentMethod(from customer: ElementsCustomer?) -> STPPaymentMethod? { | ||
guard let customer = customer else { return nil } | ||
return customer.paymentMethods.first { $0.stripeId == customer.defaultPaymentMethod } | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why static? Why not an instance function on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't really remember any reasoning of mine, you're right that it should be an instance function on ElementsCustomer |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -107,8 +107,14 @@ extension CustomerSessionAdapter { | |
return stripePaymentMethodId | ||
} | ||
|
||
func fetchSelectedPaymentOption(for customerId: String) -> CustomerPaymentOption? { | ||
return CustomerPaymentOption.defaultPaymentMethod(for: customerId) | ||
func fetchSelectedPaymentOption(for customerId: String, customer: ElementsCustomer? = nil) -> CustomerPaymentOption? { | ||
guard configuration.allowsSetAsDefaultPM, | ||
let customer = customer, | ||
let defaultPaymentMethod = customer.defaultPaymentMethod else { | ||
return CustomerPaymentOption.defaultPaymentMethod(for: customerId) | ||
} | ||
|
||
return CustomerPaymentOption.stripeId(defaultPaymentMethod) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This behavior seems inconsistent with other places where we read the default payment method. If the user is opted into Correct me if I'm wrong. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure why I used guard here instead of if/else because it seems more confusing here, but in this scenario, if the user is opted in but the customer does not have a default, it reads from defaultPaymentMethod on line 114, which returns the local default. I saw the thread with Bella where she said that it should fall back to first saved payment method in the list instead, so I will have to change this to reflect that |
||
} | ||
|
||
func detachPaymentMethod(paymentMethodId: String) async throws { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,12 @@ extension EmbeddedPaymentElement { | |
isFlatCheckmarkStyle: configuration.appearance.embeddedPaymentElement.row.style == .flatWithCheckmark | ||
) | ||
let initialSelection: EmbeddedPaymentMethodsView.Selection? = { | ||
// get default payment method from elements session | ||
if configuration.allowsSetAsDefaultPM, | ||
let defaultPaymentMethod = ElementsCustomer.getDefaultPaymentMethod(from: loadResult.elementsSession.customer) { | ||
return .saved(paymentMethod: defaultPaymentMethod) | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to be in too high in the function, we should use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, I didn't consider that the view shouldn't automatically initially select the default because there could have been previous user input. I'll fix that |
||
// Select the previous payment option | ||
switch previousPaymentOption { | ||
case .applePay: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -288,6 +288,11 @@ class PaymentSheetVerticalViewController: UIViewController, FlowControllerViewCo | |
if let selection { | ||
return selection | ||
} | ||
// get default payment method from elements session | ||
if configuration.allowsSetAsDefaultPM, | ||
let defaultPaymentMethod = ElementsCustomer.getDefaultPaymentMethod(from: elementsSession.customer) { | ||
return .saved(paymentMethod: defaultPaymentMethod) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment here as above about being too high in the function, it should not take precedence over |
||
|
||
switch previousPaymentOption { | ||
case .applePay: | ||
|
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.
This comment doesn't make much sense to me, I don't see any code changes near this comment indicating we are hard coding a default PM.
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 I was trying to say is that the only way I knew to test it was to add a payment method, get its stripe ID, and then hardcode that here so that the elements session will set that as its default. And then after adding other cards, even after paying with them, when the payment sheet is reloaded, it will have the hardcoded default selected, which shows that it's reading from the elements session instead of the local default.
Right now, there's a duping process where when you read from "default_payment_method", it returns the locally saved default that you pass into the API call— it's not actually retrieving a default from the back end. When the API endpoints are ready, then we should be able to actually get the value stored in the back end, but until then, the only way I could think to test that it's reading from the elementsSession value instead of the local default was to hard-code a default (e.g. let defaultPaymentMethod = "pm_1QSOUILu5o3P18ZpN4PY49SE")
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.
Hrm, I see what you mean, but don't think we need a comment for that. I don't think anyone else will need this comment as it shouldn't be relevant once this feature is implemented. If you want to leave it let's add a TODO to remove it for yourself.