-
Notifications
You must be signed in to change notification settings - Fork 1.1k
CheckoutSessions — PMO SFU #6353
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
Changes from all commits
7191e83
94a3a8b
57c705c
1f30639
20d9f32
3713ea7
4c0c181
c2eee6e
8f31016
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 |
|---|---|---|
|
|
@@ -166,9 +166,8 @@ enum Intent { | |
| return !setupFutureUsageValues.isEmpty | ||
| } | ||
| return nil | ||
| case .checkoutSession: | ||
|
Collaborator
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. Referring to line 133 here, can't select it. For payment mode, it returns the top-level setupFutureUsage only. The setupFutureUsageString property is used in analytics. Is it intentional that this reports only the top-level value? I think this is consistent with others but just want to double check.
Collaborator
Author
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. Yes, I intentionally made it only report the top-level to align with intent flows |
||
| // TODO(gbirch): implement during PMO SFU work | ||
| return nil | ||
| case .checkoutSession(let checkoutSession): | ||
| return checkoutSession.isPaymentMethodOptionsSetupFutureUsageSet | ||
| case .setupIntent: | ||
| return nil | ||
| } | ||
|
|
@@ -195,7 +194,7 @@ enum Intent { | |
| case .checkoutSession(let checkoutSession): | ||
| switch checkoutSession.mode { | ||
| case .payment: | ||
| guard let setupFutureUsage = checkoutSession.setupFutureUsage else { | ||
| guard let setupFutureUsage = checkoutSession.setupFutureUsage(for: paymentMethodType) else { | ||
| return false | ||
| } | ||
| return setupFutureUsage != "none" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -537,6 +537,42 @@ final class PaymentSheetAPIMockTest: APIStubbedTestCase { | |
|
|
||
| waitForExpectations(timeout: 10) | ||
| } | ||
|
|
||
| func testCheckoutSessionConfirmWithNonCardPaymentMethodIncludesSavePaymentMethod() { | ||
|
Collaborator
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. The tests in this PR are quite light considering what it's adding. Let's add |
||
| var checkoutSessionJSON = MockJson.checkoutSession | ||
| checkoutSessionJSON["payment_method_types"] = ["paypal"] | ||
| let checkoutSession = STPCheckoutSession.decodedObject(fromAPIResponse: checkoutSessionJSON)! | ||
| let elementsSession = STPElementsSession._testValue(paymentMethodTypes: ["paypal"]) | ||
| let confirmParams = IntentConfirmParams(type: .stripe(.payPal)) | ||
| confirmParams.saveForFutureUseCheckboxState = .selected | ||
|
|
||
| stubCreatePaymentMethodExpecting(allowRedisplay: "always") | ||
| stubCheckoutSessionConfirm( | ||
| sessionId: checkoutSession.stripeId, | ||
| savePaymentMethod: true | ||
| ) | ||
|
|
||
| let configuration = MockParams.configuration(pk: MockParams.publicKey) | ||
| let exp = expectation(description: "confirm completed") | ||
| let paymentHandler = STPPaymentHandler(apiClient: configuration.apiClient) | ||
|
|
||
| PaymentSheet.confirm( | ||
| configuration: configuration, | ||
| authenticationContext: self, | ||
| intent: .checkoutSession(checkoutSession), | ||
| elementsSession: elementsSession, | ||
| paymentOption: .new(confirmParams: confirmParams), | ||
| paymentHandler: paymentHandler, | ||
| analyticsHelper: ._testValue(), | ||
| completion: { result, _ in | ||
| XCTAssertEqual(result, .completed) | ||
| exp.fulfill() | ||
| } | ||
| ) | ||
|
|
||
| waitForExpectations(timeout: 10) | ||
| } | ||
|
|
||
| } | ||
|
|
||
| extension PaymentSheetAPIMockTest: STPAuthenticationContext { | ||
|
|
||
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.
Nit/question: The existing
STPPaymentMethodOptions.isSetupFutureUsageSet(line 45-52 of STPPaymentMethodOptions.swift) checks whether any PMO entry contains the key setup_future_usage. Might be a bit safer to align withSTPPaymentMethodOptions.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.
Are you suggesting that we use the
payment_method_optionsproperty on the checkout session as the data source for PMO SFU rather thansetup_future_usage_for_payment_method_typeor something else? If the former, I have a question about which to use down in the API team questions list, but for now my intuition is to use thesetup_future_usage_for_payment_method_typeproperty since it is specific to checkout sessions and what is used by web: https://stripe.sourcegraphcloud.com/deepsearch/c4c1b6ed-ecad-4140-a808-600a15fa1da9#answer-169850