-
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
Set set_as_default_payment_method field using checkbox value #4502
Changes from 29 commits
7bc9d63
66255c3
0955579
2d27f68
dd1ddb1
fdf24e5
0615150
ae0cafd
26401fe
d7cbfdf
3b33102
dad8ae7
ddbeea9
af21c85
48f02d2
bcc1ff7
549aaef
b7e62a7
4e762cb
e00ee59
041f865
9aa4e8b
f28ac9f
2e33889
030ca74
2b75dcc
53ae9a8
c606817
222bdf1
4aaa0ef
168ed36
855f7e1
a9d9db1
3bca1bc
3e7c01a
07182b9
1846b90
145ff3a
66bc395
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 |
---|---|---|
|
@@ -45,6 +45,8 @@ final class IntentConfirmParams { | |
} | ||
} | ||
|
||
var setAsDefaultPM: Bool? | ||
|
||
Comment on lines
+49
to
+50
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. Can we add a doc comment for what this does? |
||
func makeIcon(updateImageHandler: DownloadManager.UpdateImageHandler?) -> UIImage { | ||
if let bankName = (financialConnectionsLinkedBank?.bankName ?? instantDebitsLinkedBank?.bankName) { | ||
return PaymentSheetImageLibrary.bankIcon(for: PaymentSheetImageLibrary.bankIconCode(for: bankName)) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -190,7 +190,8 @@ extension PaymentSheet { | |
confirmPaymentMethodType: .new( | ||
params: confirmParams.paymentMethodParams, | ||
paymentOptions: confirmParams.confirmPaymentMethodOptions, | ||
shouldSave: confirmParams.saveForFutureUseCheckboxState == .selected | ||
shouldSave: confirmParams.saveForFutureUseCheckboxState == .selected, | ||
shouldSetAsDefaultPM: confirmParams.setAsDefaultPM | ||
Comment on lines
+193
to
+194
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. Let's add a test like testDeferredConfirm_valid_new_card_and_save_checkbox_selected for the default PM flow. 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 have a test testDeferredConfirm_valid_new_card_and_save_checkbox_selected_and_set_as_default already |
||
), | ||
paymentIntent: paymentIntent, | ||
configuration: configuration | ||
|
@@ -211,7 +212,8 @@ extension PaymentSheet { | |
confirmPaymentMethodType: .new( | ||
params: confirmParams.paymentMethodParams, | ||
paymentOptions: confirmParams.confirmPaymentMethodOptions, | ||
shouldSave: false | ||
shouldSave: false, | ||
shouldSetAsDefaultPM: confirmParams.setAsDefaultPM | ||
), | ||
setupIntent: setupIntent, | ||
configuration: configuration | ||
|
@@ -232,7 +234,8 @@ extension PaymentSheet { | |
confirmType: .new( | ||
params: confirmParams.paymentMethodParams, | ||
paymentOptions: confirmParams.confirmPaymentMethodOptions, | ||
shouldSave: confirmParams.saveForFutureUseCheckboxState == .selected | ||
shouldSave: confirmParams.saveForFutureUseCheckboxState == .selected, | ||
shouldSetAsDefaultPM: confirmParams.setAsDefaultPM | ||
), | ||
configuration: configuration, | ||
intentConfig: intentConfig, | ||
|
@@ -570,7 +573,9 @@ extension PaymentSheet { | |
intent.isSetupFutureUsageSet, | ||
let paymentMethod = intent.paymentMethod, | ||
// Can it appear in the list of saved PMs? | ||
PaymentSheet.supportedSavedPaymentMethods.contains(paymentMethod.type) | ||
PaymentSheet.supportedSavedPaymentMethods.contains(paymentMethod.type), | ||
// Should we be writing to local storage? | ||
!configuration.allowsSetAsDefaultPM | ||
else { | ||
return | ||
} | ||
|
@@ -592,12 +597,12 @@ extension PaymentSheet { | |
enum ConfirmPaymentMethodType { | ||
case saved(STPPaymentMethod, paymentOptions: STPConfirmPaymentMethodOptions?) | ||
/// - paymentMethod: Pass this if you created a PaymentMethod already (e.g. for the deferred flow). | ||
case new(params: STPPaymentMethodParams, paymentOptions: STPConfirmPaymentMethodOptions, paymentMethod: STPPaymentMethod? = nil, shouldSave: Bool) | ||
case new(params: STPPaymentMethodParams, paymentOptions: STPConfirmPaymentMethodOptions, paymentMethod: STPPaymentMethod? = nil, shouldSave: Bool, shouldSetAsDefaultPM: Bool? = nil) | ||
var shouldSave: Bool { | ||
switch self { | ||
case .saved: | ||
return false | ||
case .new(_, _, _, let shouldSave): | ||
case .new(_, _, _, let shouldSave, _): | ||
return shouldSave | ||
} | ||
} | ||
|
@@ -619,7 +624,7 @@ extension PaymentSheet { | |
params = STPPaymentIntentParams(clientSecret: paymentIntent.clientSecret, paymentMethodType: paymentMethod.type) | ||
params.paymentMethodOptions = paymentMethodOptions | ||
params.paymentMethodId = paymentMethod.stripeId | ||
case let .new(paymentMethodParams, paymentMethodoptions, paymentMethod, _shouldSave): | ||
case let .new(paymentMethodParams, paymentMethodoptions, paymentMethod, _shouldSave, shouldSetAsDefaultPM): | ||
shouldSave = _shouldSave | ||
if let paymentMethod = paymentMethod { | ||
paymentMethodType = paymentMethod.type | ||
|
@@ -632,7 +637,9 @@ extension PaymentSheet { | |
params.paymentMethodOptions = paymentMethodoptions | ||
paymentMethodType = paymentMethodParams.type | ||
} | ||
|
||
if let shouldSetAsDefaultPM = shouldSetAsDefaultPM { | ||
params.setAsDefaultPM = NSNumber(value: shouldSetAsDefaultPM) | ||
} | ||
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. Can shorten this to |
||
let requiresMandateData: [STPPaymentMethodType] = [.payPal, .cashApp, .revolutPay, .amazonPay, .klarna] | ||
if requiresMandateData.contains(paymentMethodType) && paymentIntent.setupFutureUsage == .offSession | ||
{ | ||
|
@@ -652,7 +659,6 @@ extension PaymentSheet { | |
params.paymentMethodOptions = paymentOptions | ||
params.returnURL = configuration.returnURL | ||
params.shipping = makeShippingParams(for: paymentIntent, configuration: configuration) | ||
|
||
return params | ||
} | ||
|
||
|
@@ -671,7 +677,7 @@ extension PaymentSheet { | |
) | ||
params.paymentMethodID = paymentMethod.stripeId | ||
|
||
case let .new(paymentMethodParams, _, paymentMethod, _): | ||
case let .new(paymentMethodParams, _, paymentMethod, _, shouldSetAsDefaultPM): | ||
if let paymentMethod { | ||
params = STPSetupIntentConfirmParams( | ||
clientSecret: setupIntent.clientSecret, | ||
|
@@ -682,6 +688,9 @@ extension PaymentSheet { | |
params = STPSetupIntentConfirmParams(clientSecret: setupIntent.clientSecret) | ||
params.paymentMethodParams = paymentMethodParams | ||
} | ||
if let shouldSetAsDefaultPM = shouldSetAsDefaultPM { | ||
params.setAsDefaultPM = NSNumber(value: shouldSetAsDefaultPM) | ||
} | ||
// Paypal & revolut requires mandate_data if setting up | ||
if params.paymentMethodType == .payPal || params.paymentMethodType == .revolutPay { | ||
params.mandateData = .makeWithInferredValues() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -371,7 +371,8 @@ extension PaymentSheetFormFactory { | |
isSelectedByDefault: false, | ||
didToggle: didToggle | ||
) | ||
return PaymentMethodElementWrapper(element) { _, params in | ||
return PaymentMethodElementWrapper(element) { checkbox, params in | ||
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. Nit: There can be two checkboxes, or at least it seems unclear what checkbox this refers to, can we make it something more specific to what checkbox it is? |
||
params.setAsDefaultPM = checkbox.isSelected | ||
return params | ||
} | ||
} | ||
|
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.
Do we need this change in this PR? Seems like it doesn't really do much than no-op for allowsSetAsDefaultPM
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 suppose not, I think this was a remnant of when I started implementing it for CustomerSheet but then we decided not to