read from customersession setting instead of config #4517

joyceqin-stripe · 2025-01-30T19:50:10Z

Summary

Reads whether the feature is enabled from ElementsSession.customer.customerSession instead of PaymentSheet/CustomerSheet Configuration

Motivation

MOBILESDK-2666

Testing

Changelog

emerge-tools · 2025-01-30T20:02:48Z

1 build decreased size, 7 builds had no size change

Name	Version	Download	Change	Install	Change	Approval
StripeSize com.stripe.StripeSize	1.0 (1)	2.1 MB	⬆️ 6 B	7.0 MB	-	N/A
StripePaymentsSize com.stripe.StripePaymentsSize	1.0 (1)	1.2 MB	-	4.2 MB	-	N/A
StripePaymentsUISize com.stripe.StripePaymentsUISize	1.0 (1)	1.9 MB	⬇️ 6 B	6.4 MB	-	N/A
StripePaymentSheetSize com.stripe.StripePaymentSheetSize	1.0 (1)	3.8 MB	⬆️ 835 B (0.02%)	11.2 MB	⬇️ 388 B	N/A
StripeIdentitySize com.stripe.StripeIdentitySize	1.0 (1)	1.4 MB	⬆️ 2 B	4.4 MB	-	N/A
StripeApplePaySize com.stripe.StripeApplePaySize	1.0 (1)	489.1 kB	-	1.7 MB	-	N/A
StripeFinancialConnectionsSize com.stripe.StripeFinancialConnectionsSize	1.0 (1)	1.5 MB	⬆️ 2 B	4.8 MB	-	N/A
StripeConnectSize com.stripe.StripeConnectSize	1.0 (1)	1.7 MB	⬆️ 1 B	5.4 MB	-	N/A

StripeSize 1.0 (1)
com.stripe.StripeSize

No changes to report

StripePaymentsSize 1.0 (1)
com.stripe.StripePaymentsSize

No changes to report

StripePaymentsUISize 1.0 (1)
com.stripe.StripePaymentsUISize

No changes to report

StripePaymentSheetSize 1.0 (1)
com.stripe.StripePaymentSheetSize

⚖️ Compare build
⏱️ Analyze build performance

Total install size change: ⬇️ 388 B
Total download size change: ⬆️ 835 B (0.02%)

Largest size changes

Item	Install Size Change
🗑 StripePaymentSheet.PaymentSheetFormFactory.makeCard(cardBrandChoi...	⬇️ -13.7 kB
📝 StripePaymentSheet.PaymentSheetFormFactory.makeCard(cardBrandChoi...	⬆️ 13.0 kB
🗑 StripePaymentSheet.PaymentSheetFormFactory.makeUSBankAccount(merc...	⬇️ -8.1 kB
📝 StripePaymentSheet.PaymentSheetFormFactory.makeUSBankAccount(merc...	⬆️ 7.4 kB
📝 StripePaymentSheet.SavedPaymentOptionsViewController.makeViewMode...	⬆️ 1.7 kB

View Treemap

StripeIdentitySize 1.0 (1)
com.stripe.StripeIdentitySize

No changes to report

StripeApplePaySize 1.0 (1)
com.stripe.StripeApplePaySize

No changes to report

StripeFinancialConnectionsSize 1.0 (1)
com.stripe.StripeFinancialConnectionsSize

No changes to report

StripeConnectSize 1.0 (1)
com.stripe.StripeConnectSize

No changes to report

🛸 Powered by Emerge Tools

…t for customersheet, fix some tests

…s into joyceqin/MOBILESDK-2666

porter-stripe · 2025-02-10T16:02:59Z

StripePaymentSheet/StripePaymentSheet/Source/PaymentSheet/PaymentSheetConfiguration.swift

-        /// This is an experimental feature that may be removed at any time.
-        /// If true, users can set a payment method as default and sync their default payment method across web and mobile
-        /// If false (default), users cannot set default payment methods.
-        @_spi(AllowsSetAsDefaultPM) public var allowsSetAsDefaultPM = false


Do a search in the codebase for I see 3 instances that need to be removed/cleaned up. (AllowsSetAsDefaultPM)

porter-stripe · 2025-02-10T16:19:31Z

...t/StripePaymentSheet/Source/Internal/API Bindings/v1-elements-sessions/CustomerSession.swift

@@ -42,6 +42,7 @@ struct CustomerSession: Equatable, Hashable {
                return nil
            }
            let paymentMethodRemoveLast = mobilePaymentElementFeaturesDict["payment_method_remove_last"] as? String ?? "enabled"
+            let paymentMethodSetAsDefault = mobilePaymentElementFeaturesDict["payment_method_set_as_default"] as? String ?? "disabled"


Surprised we don't have any tests for this class? If so can we file a run ticket to get some tests created for this class.

Created a ticket: RUN_MOBILESDK-3836

We do have tests. They are in STPElementsSessionTest.swift

Ah I see, it's a child of ElementsSession. Kinda hard to tell it's being tested might still be good to have standalone tests.

porter-stripe · 2025-02-10T16:21:13Z

StripePaymentSheet/StripePaymentSheetTests/PaymentSheet/STPElementsSessionTest.swift

+    }
+    func testSetAsDefault_disabled() {


Super nit: Our linter on CI isn't working but let's always have a new line between functions

porter-stripe · 2025-02-10T16:25:50Z

StripePaymentSheet/StripePaymentSheetTests/PaymentSheet/STPElementsSessionTest.swift

@@ -262,6 +262,41 @@ class STPElementsSessionTest: XCTestCase {

        XCTAssertEqual(.legacy, savePaymentMethodConsentBehavior)
    }
+    func testSetAsDefault_enabled() {


Can we update testDecodedObjectFromAPIResponseMapping to make sure that paymentMethodSetAsDefaultForPaymentSheet defaults to false?

porter-stripe · 2025-02-10T16:28:30Z

...t/StripePaymentSheet/Source/PaymentSheet/CustomerSessionAdapter/CustomerSessionAdapter.swift

@@ -107,10 +107,10 @@ extension CustomerSessionAdapter {
        return stripePaymentMethodId
    }

-    func fetchSelectedPaymentOption(for customerId: String, customer: ElementsCustomer? = nil) -> CustomerPaymentOption? {
+    func fetchSelectedPaymentOption(for customerId: String, elementsSession: STPElementsSession? = nil) -> CustomerPaymentOption? {


Similarly surprised we don't have tests for this, can you mention this class in the JIRA ticket you create.

porter-stripe · 2025-02-10T16:31:02Z

...mentSheet/StripePaymentSheet/Source/PaymentSheet/CustomerSheet/CustomerSheetDataSource.swift

@@ -42,7 +42,7 @@ class CustomerSheetDataSource {
                await loadFormSpecs()
                let customerId = try await customerSessionClientSecret.customerId
                let elementSession = try await elementsSessionResult
-                let paymentOption = customerSessionAdapter.fetchSelectedPaymentOption(for: customerId, customer: elementSession.customer)


Hm same, this entire file has no tests :(

Not having tests for this class was intentional for most functions in this class as they are just switching on legacy ephemeral keys vs customersession. For loadPaymentMethodInfo it's debatable whether we want to add tests it seems like we'd end up with a lot of mocked responses so it's unclear how helpful it would be.

porter-stripe · 2025-02-10T16:33:50Z

StripePaymentSheet/StripePaymentSheet/Source/PaymentSheet/PaymentSheet+API.swift

@@ -200,7 +200,7 @@ extension PaymentSheet {
                    params,
                    with: authenticationContext,
                    completion: { actionStatus, paymentIntent, error in
-                        if let paymentIntent {
+                        if let paymentIntent, !elementsSession.paymentMethodSetAsDefaultForPaymentSheet {


This reads backwards to me, why are we setting default if necessary only if NOT paymentMethodSetAsDefaultForPaymentSheet. Is setDefaultPaymentMethodIfNecessary setting the default in the local storage? If so let's make this more clear there is a difference between where we are saving defaults.

Maybe pass this flag into setDefaultPaymentMethodIfNecessary rather than checking it here? Seems like the wrong place to do it.

Yeah, setDefaultPaymentMethodIfNecessary writes to local storage

porter-stripe · 2025-02-10T16:34:34Z

StripePaymentSheet/StripePaymentSheet/Source/PaymentSheet/PaymentSheet+API.swift

@@ -222,7 +222,7 @@ extension PaymentSheet {
                    setupIntentParams,
                    with: authenticationContext,
                    completion: { actionStatus, setupIntent, error in
-                        if let setupIntent {
+                        if let setupIntent, !elementsSession.paymentMethodSetAsDefaultForPaymentSheet {


porter-stripe · 2025-02-10T16:35:15Z

StripePaymentSheet/StripePaymentSheet/Source/PaymentSheet/PaymentSheet+DeferredAPI.swift

+                    if let paymentOrSetupIntent, !allowsSetAsDefaultPM {
                        setDefaultPaymentMethodIfNecessary(actionStatus: status, intent: paymentOrSetupIntent, configuration: configuration)


Same comment here as above

porter-stripe · 2025-02-10T16:36:26Z

StripePaymentSheet/StripePaymentSheet/Source/PaymentSheet/PaymentSheetLoader.swift

@@ -319,7 +318,7 @@ final class PaymentSheetLoader {
        if let customerID = configuration.customer?.id {
            var defaultPaymentMethodOption: CustomerPaymentOption?
            // if opted in to the "set as default" feature, try to get default payment method from elements session


Is this comment still true?

Yes, we read from the CustomerSession setting to see if the feature is on, and if so, it tries to get the default payment method from the ElementsSession, but otherwise, it reads from the local storage

…s into joyceqin/MOBILESDK-2666

wooj-stripe · 2025-02-11T00:24:35Z

...tripePaymentSheet/Source/Internal/API Bindings/v1-elements-sessions/STPElementsSession.swift

@@ -211,6 +211,10 @@ extension STPElementsSession {
        return customer?.customerSession.mobilePaymentElementComponent.features?.paymentMethodRemoveLast ?? true
    }

+    var paymentMethodSetAsDefaultForPaymentSheet: Bool {


Doesn't need to be in this PR, but we should probably rename xForPaymentSheet to xForMPE

wooj-stripe · 2025-02-11T00:29:20Z

...StripePaymentSheet/Source/PaymentSheet/PaymentSheetFormFactory/PaymentSheetFormFactory.swift

@@ -526,10 +530,10 @@ extension PaymentSheetFormFactory {
        )
    }

-    func makeUSBankAccount(merchantName: String) -> PaymentMethodElement {
+    func makeUSBankAccount(merchantName: String, showSetAsDefaultCheckbox: Bool = false) -> PaymentMethodElement {


There doesn't seem to be a good reason to default to false.

wooj-stripe · 2025-02-11T00:29:41Z

...ePaymentSheet/Source/PaymentSheet/PaymentSheetFormFactory/PaymentSheetFormFactory+Card.swift

@@ -13,10 +13,10 @@ import StripePayments
 import UIKit

 extension PaymentSheetFormFactory {
-    func makeCard(cardBrandChoiceEligible: Bool = false) -> PaymentMethodElement {
+    func makeCard(cardBrandChoiceEligible: Bool = false, showSetAsDefaultCheckbox: Bool = false) -> PaymentMethodElement {


Is there a good reason to default showSetAsDefaultCheckbox to false?

wooj-stripe · 2025-02-11T00:33:42Z

...heet/Source/PaymentSheet/Saved Payment Method Screen/SavedPaymentOptionsViewController.swift

+        if let elementsSession = elementsSession,
+           let customer = elementsSession.customer,
+           let features = customer.customerSession.mobilePaymentElementComponent.features,
+           features.paymentMethodSetAsDefault,
+           let defaultPaymentMethod = customer.getDefaultOrFirstPaymentMethod() {


If we go with this pattern, it could be more simply written:

if let elementsSession, elementsSession.paymentMethodSetAsDefaultForPaymentSheet, let defaultPaymentMethod = elementsSession.customer?.getDefaultOrFirstPaymentMethod() {

But, taking a step back, have we considered passing in the elementsSession instance into CustomerPaymentOption so that we can just simply write something like:

CustomerPaymentOption.defaultPaymentMethod(for: customerID, elementsSession: elementsSession, surface: .paymentSheet)

read from customersession setting instead of config

a149588

joyceqin-stripe added 19 commits January 30, 2025 13:18

changed naming to better match others

4660d4f

merged with master

2057cb2

elementssession test

cf5fd54

remove canUpdateCardBrand (removed in master)

b6cfbe9

fix build

cb9d01d

fix test

907a363

merge with main

08087f6

fixing build

9aa6307

replace payment_method_set_as_default with payment_method_sync_defaul…

23358dd

…t for customersheet, fix some tests

Merge branch 'master' into joyceqin/MOBILESDK-2666

782ffec

Merge branch 'master' into joyceqin/MOBILESDK-2666

d43d73e

remove @testable import from STPTestingAPIClient

68ae80e

Merge branch 'master' into joyceqin/MOBILESDK-2666

56b7ff8

read from elementsSession in makeViewModels instead of passing it in

9c268e7

Merge branch 'joyceqin/MOBILESDK-2666' of github.com:stripe/stripe-io…

ac0fdf1

…s into joyceqin/MOBILESDK-2666

mock network for tests with updated ci test backend

3aef64e

revert timeout

ea32e91

do not show apple pay in customersheet when adding first saved pm

2fc6c05

Merge branch 'master' into joyceqin/MOBILESDK-2666

4f2f131

joyceqin-stripe marked this pull request as ready for review February 10, 2025 15:39

joyceqin-stripe requested review from a team as code owners February 10, 2025 15:39

joyceqin-stripe requested a review from porter-stripe February 10, 2025 15:39

porter-stripe reviewed Feb 10, 2025

View reviewed changes

joyceqin-stripe added 3 commits February 10, 2025 10:49

cleanup

1480972

Merge branch 'joyceqin/MOBILESDK-2666' of github.com:stripe/stripe-io…

f88219a

…s into joyceqin/MOBILESDK-2666

Merge branch 'master' into joyceqin/MOBILESDK-2666

69d0483

joyceqin-stripe requested a review from porter-stripe February 10, 2025 19:53

Merge branch 'master' into joyceqin/MOBILESDK-2666

f9bd545

wooj-stripe reviewed Feb 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

read from customersession setting instead of config #4517

read from customersession setting instead of config #4517

joyceqin-stripe commented Jan 30, 2025 •

edited

Loading

emerge-tools bot commented Jan 30, 2025 •

edited

Loading

porter-stripe Feb 10, 2025

porter-stripe Feb 10, 2025

joyceqin-stripe Feb 10, 2025

wooj-stripe Feb 10, 2025

porter-stripe Feb 11, 2025

porter-stripe Feb 10, 2025

porter-stripe Feb 10, 2025

porter-stripe Feb 10, 2025

porter-stripe Feb 10, 2025

wooj-stripe Feb 11, 2025

porter-stripe Feb 10, 2025

porter-stripe Feb 10, 2025

joyceqin-stripe Feb 10, 2025

porter-stripe Feb 10, 2025

porter-stripe Feb 10, 2025

porter-stripe Feb 10, 2025

joyceqin-stripe Feb 10, 2025

wooj-stripe Feb 11, 2025

wooj-stripe Feb 11, 2025

wooj-stripe Feb 11, 2025

wooj-stripe Feb 11, 2025

		if let paymentOrSetupIntent, !allowsSetAsDefaultPM {
		setDefaultPaymentMethodIfNecessary(actionStatus: status, intent: paymentOrSetupIntent, configuration: configuration)

read from customersession setting instead of config #4517

Are you sure you want to change the base?

read from customersession setting instead of config #4517

Conversation

joyceqin-stripe commented Jan 30, 2025 • edited Loading

Summary

Motivation

Testing

Changelog

emerge-tools bot commented Jan 30, 2025 • edited Loading

1 build decreased size, 7 builds had no size change

StripeSize 1.0 (1)com.stripe.StripeSize

StripePaymentsSize 1.0 (1)com.stripe.StripePaymentsSize

StripePaymentsUISize 1.0 (1)com.stripe.StripePaymentsUISize

StripePaymentSheetSize 1.0 (1)com.stripe.StripePaymentSheetSize

StripeIdentitySize 1.0 (1)com.stripe.StripeIdentitySize

StripeApplePaySize 1.0 (1)com.stripe.StripeApplePaySize

StripeFinancialConnectionsSize 1.0 (1)com.stripe.StripeFinancialConnectionsSize

StripeConnectSize 1.0 (1)com.stripe.StripeConnectSize

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyceqin-stripe commented Jan 30, 2025 •

edited

Loading

emerge-tools bot commented Jan 30, 2025 •

edited

Loading

StripeSize 1.0 (1)
com.stripe.StripeSize

StripePaymentsSize 1.0 (1)
com.stripe.StripePaymentsSize

StripePaymentsUISize 1.0 (1)
com.stripe.StripePaymentsUISize

StripePaymentSheetSize 1.0 (1)
com.stripe.StripePaymentSheetSize

StripeIdentitySize 1.0 (1)
com.stripe.StripeIdentitySize

StripeApplePaySize 1.0 (1)
com.stripe.StripeApplePaySize

StripeFinancialConnectionsSize 1.0 (1)
com.stripe.StripeFinancialConnectionsSize

StripeConnectSize 1.0 (1)
com.stripe.StripeConnectSize