Read the default_payment_method field from elements session and use that as the default PM #4313

joyceqin-stripe · 2024-11-27T21:29:11Z

Summary

Added allowsSetAsDefaultPM flag. Only shown if we're using customer session.
If allowsSetAsDefaultPM is on, then instead of reading from the locally stored default, we read from ElementsSession.

Motivation

MOBILESDK-2799

Testing

customer_session, allowsSetAsDefaultPM on
Hardcoded the value that was "read" from default_payment_method (set to the 4242 Visa stripe id)
After changing the selected payment method and paying with it, upon reload the PaymentSheet default is back to our hardcoded default.

Simulator.Screen.Recording.-.iPhone.16.-.2024-12-02.at.08.05.00.mp4

Changelog

github-actions · 2024-11-27T21:41:22Z

🚨 New dead code detected in this PR:

String+Localized.swift:352 warning: Property 'remove_payment_method' is unused
SavedPaymentMethodCollectionView.swift:113 warning: Property 'cbcEligible' is assigned, but never used
SavedPaymentMethodRowButton.swift:16 warning: Function 'didSelectRemoveButton(_:with:)' is unused
SavedPaymentMethodRowButton.swift:70 warning: Property 'canRemove' is unused
SavedPaymentMethodRowButton.swift:117 warning: Function 'handleRemoveButtonTapped()' is unused
VerticalSavedPaymentMethodsViewController.swift:99 warning: Property 'hasCoBrandedCards' is unused
VerticalSavedPaymentMethodsViewController.swift:312 warning: Function 'didSelectRemoveButton(_:with:)' is unused
PaymentSheetUIKitAdditions.swift:164 warning: Function 'makeRowButtonContentStackView(arrangedSubviews:)' is unused

Please remove the dead code before merging.

If this is intentional, you can bypass this check by adding the label skip dead code check to this PR.

ℹ️ 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 master.

…s into joyceqin-MOBILESDK-2799

porter-stripe

There is a lot of duplicated logic that is very similar, we should try to consolidate that into one place as much as possible.

porter-stripe · 2024-12-05T16:08:42Z

Example/PaymentSheet Example/PaymentSheet Example/CustomerSheetTestPlayground.swift

@@ -53,6 +53,9 @@ struct CustomerSheetTestPlayground: View {
                                SettingPickerView(setting: $playgroundController.settings.paymentMethodRemove)
                                SettingPickerView(setting: $playgroundController.settings.paymentMethodRemoveLast)
                                SettingPickerView(setting: $playgroundController.settings.paymentMethodAllowRedisplayFilters)
+                                if playgroundController.settings.alternateUpdatePaymentMethodNavigation == .on {


When can we remove alternateUpdatePaymentMethodNavigation?

I suppose if we're trying to release this is the default flow, it should be removed now

I do anticipate possibly having to update a lot of tests though, so might be changing a lot of files

porter-stripe · 2024-12-05T16:10:26Z

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

+        // 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


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.

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")

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.

porter-stripe · 2024-12-05T16:15:31Z

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

@@ -107,7 +107,13 @@ extension CustomerSessionAdapter {
        return stripePaymentMethodId
    }

-    func fetchSelectedPaymentOption(for customerId: String) -> CustomerPaymentOption? {
+    func fetchSelectedPaymentOption(for customerId: String, elementsSession: STPElementsSession? = nil) -> CustomerPaymentOption? {
+        if configuration.allowsSetAsDefaultPM,


Nit; I think a guard here is more appropriate as it enforces that you return a value:

guard configuration.allowsSetAsDefaultPM, let elementsSession = elementsSession, let customer = elementsSession.customer, let defaultPaymentMethod = customer.defaultPaymentMethod else { return CustomerPaymentOption.defaultPaymentMethod(for: customerId) } return CustomerPaymentOption.stripeId(defaultPaymentMethod)

Also, it seems unnecessary to pass in the entire STPElementsSession here, we only really need the customer.

porter-stripe · 2024-12-05T16:17:09Z

...tSheet/StripePaymentSheet/Source/PaymentSheet/CustomerSheet/CustomerSheetConfiguration.swift

@@ -86,6 +86,11 @@ extension CustomerSheet {
        /// If false (default), only card brand choice eligible cards can be edited and users can remove payment methods from the list screen.
        @_spi(AlternateUpdatePaymentMethodNavigation) public var alternateUpdatePaymentMethodNavigation = false

+        /// This is an experimental feature that may be removed at any time.
+        /// If true, users can set a payment method as default.


We should add that this also influences where we read the default payment method from, not just allow setting.

What about "users can set a payment method as default and sync their default payment method across web and mobile"

porter-stripe · 2024-12-05T16:18:55Z

StripePaymentSheet/StripePaymentSheet/Source/PaymentSheet/CustomerSheet/CustomerSheet.swift

+
+            var selectedPaymentOption: CustomerPaymentOption?
+
+            // read from back end


I think this comment could be better!

Changed it to "get default payment method from elements session"— is that better?

porter-stripe · 2024-12-05T16:21:04Z

...et/StripePaymentSheet/Source/PaymentSheet/Embedded/EmbeddedPaymentElementConfiguration.swift

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


Same comment from about in the CustomerSheetConfiguration. Also same in PaymentSheetConfiguration.

porter-stripe · 2024-12-05T16:22:23Z

...tSheet/StripePaymentSheet/Source/PaymentSheet/Embedded/EmbeddedPaymentElement+Internal.swift

+            // read from back end
+            if configuration.allowsSetAsDefaultPM,
+               let customer =  loadResult.elementsSession.customer {
+                let defaultPaymentMethod = customer.paymentMethods.filter {
+                    $0.stripeId == customer.defaultPaymentMethod
+                }.first
+                if let defaultPaymentMethod = defaultPaymentMethod {
+                    return .saved(paymentMethod: defaultPaymentMethod)
+                }
+
+            }
+


Somewhat similar comment to above, about the "read from back end" comment and the nested if's

Maybe:

if configuration.allowsSetAsDefaultPM, let customer = loadResult.elementsSession.customer, let defaultPaymentMethodId = customer.defaultPaymentMethod, let defaultPaymentMethod = customer.paymentMethods.first(where: { $0.stripeId == defaultPaymentMethodId }) { return .saved(paymentMethod: defaultPaymentMethod) }

porter-stripe · 2024-12-05T16:25:20Z

StripePaymentSheet/StripePaymentSheet/Source/PaymentSheet/PaymentSheetLoader.swift

+            // read from back end
+            if configuration.allowsSetAsDefaultPM,
+               let customer =  elementsSession.customer {


Similar here w/ the comment and nested if's and multiple branches leading to the same CustomerPaymentOption.defaultPaymentMethod(for: customerID). I think there's a cleaner way to write this.

porter-stripe · 2024-12-05T16:28:57Z

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

@@ -438,9 +444,22 @@ class SavedPaymentOptionsViewController: UIViewController {

    /// Creates the list of viewmodels to display in the "saved payment methods" carousel e.g. `["+ Add", "Apple Pay", "Link", "Visa 4242"]`
    /// - Returns defaultSelectedIndex: The index of the view model that is the default e.g. in the above list, if "Visa 4242" is the default, the index is 3.
-    static func makeViewModels(savedPaymentMethods: [STPPaymentMethod], customerID: String?, showApplePay: Bool, showLink: Bool) -> (defaultSelectedIndex: Int, viewModels: [Selection]) {
+    static func makeViewModels(savedPaymentMethods: [STPPaymentMethod], customerID: String?, showApplePay: Bool, showLink: Bool, allowsSetAsDefaultPM: Bool, elementsSession: STPElementsSession) -> (defaultSelectedIndex: Int, viewModels: [Selection]) {


Similar to how I don't think we need to pass in the entire ElementsSession, maybe just the customer or default PM. That way we can consolidate all this unwrapping/duplicate code.

porter-stripe · 2024-12-05T16:29:38Z

...ipePaymentSheet/Source/PaymentSheet/ViewControllers/PaymentSheetVerticalViewController.swift

+            // read from back end
+            if configuration.allowsSetAsDefaultPM,
+               let customer =  elementsSession.customer {
+                let defaultPaymentMethod = customer.paymentMethods.filter {
+                    $0.stripeId == customer.defaultPaymentMethod
+                }.first
+                if let defaultPaymentMethod = defaultPaymentMethod {
+                    return .saved(paymentMethod: defaultPaymentMethod)
+                }
+            }


This code is duplicated in a few spots, I think we should consolidate this logic into one spot and pass around the defaultPM or something.

…entMethods

…from elements session logic

joyceqin-stripe · 2024-12-05T19:46:22Z

There is a lot of duplicated logic that is very similar, we should try to consolidate that into one place as much as possible.

I've made a static func getDefaultPaymentMethod(from customer: ElementsCustomer?) -> STPPaymentMethod? in ElementsCustomer.swift

…odnavigation-flag

…flag' of github.com:stripe/stripe-ios into joyceqin-remove-alternateupdatepaymentmethodnavigation-flag

…flag' into joyceqin-MOBILESDK-2799

…odnavigation-flag

…vedPaymentMethods_verticalMode

…flag' into joyceqin-MOBILESDK-2799

joyceqin-stripe · 2024-12-06T23:31:06Z

Okay, I've extracted reading the default payment method from the elements to a function in ElementsCustomer, edited some of the comments, and changed some parameters from ElementsSession to ElementsCustomer

porter-stripe

Let's make sure we are also adding tests for this new functionality.

porter-stripe · 2024-12-06T23:44:19Z

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

+        // 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


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.

porter-stripe · 2024-12-06T23:45:30Z

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

+
+    static func getDefaultPaymentMethod(from customer: ElementsCustomer?) -> STPPaymentMethod? {
+        guard let customer = customer else { return nil }
+        return customer.paymentMethods.first { $0.stripeId == customer.defaultPaymentMethod }
+    }


Why static? Why not an instance function on ElementsCustomer with tests?

I can't really remember any reasoning of mine, you're right that it should be an instance function on ElementsCustomer

porter-stripe · 2024-12-09T15:40:56Z

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

+        guard configuration.allowsSetAsDefaultPM,
+              let customer = customer,
+              let defaultPaymentMethod = customer.defaultPaymentMethod else {
+            return CustomerPaymentOption.defaultPaymentMethod(for: customerId)
+        }
+
+        return CustomerPaymentOption.stripeId(defaultPaymentMethod)


This behavior seems inconsistent with other places where we read the default payment method. If the user is opted into allowsSetAsDefaultPM but the customer does not have a default payment method we will fall through and read it from the stripeId on line 117. In other places we do not fall through. My understanding is that if we are opted into this feature we will use the customer default payment method as the single source of truth for their default. Let's add tests here too.

Correct me if I'm wrong.

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

porter-stripe · 2024-12-09T15:43:09Z

...tSheet/StripePaymentSheet/Source/PaymentSheet/Embedded/EmbeddedPaymentElement+Internal.swift

+            // get default payment method from elements session
+            if configuration.allowsSetAsDefaultPM,
+               let defaultPaymentMethod = ElementsCustomer.getDefaultPaymentMethod(from: loadResult.elementsSession.customer) {
+                    return .saved(paymentMethod: defaultPaymentMethod)
+            }
+


This seems to be in too high in the function, we should use previousPaymentOption instead of the default on the customer. See line 63 where we try to read the customer default.

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

porter-stripe · 2024-12-09T15:45:37Z

...ipePaymentSheet/Source/PaymentSheet/ViewControllers/PaymentSheetVerticalViewController.swift

+            // get default payment method from elements session
+            if configuration.allowsSetAsDefaultPM,
+               let defaultPaymentMethod = ElementsCustomer.getDefaultPaymentMethod(from: elementsSession.customer) {
+                return .saved(paymentMethod: defaultPaymentMethod)
+            }


Same comment here as above about being too high in the function, it should not take precedence over previousPaymentOption. See below for where we look for the saved PM.

…in saved pm list

…s into joyceqin-MOBILESDK-2799

porter-stripe

Almost there, one small clean up

porter-stripe · 2024-12-09T22:54:42Z

...tSheet/StripePaymentSheet/Source/PaymentSheet/Embedded/EmbeddedPaymentElement+Internal.swift

+                else {
+                    customerDefault = nil
+                }


We should be able to remove this, customerDefault is implied nil from it's definition. Same with other similar code paths in the PR.

…s into joyceqin-MOBILESDK-2799

joyceqin-stripe added 2 commits November 27, 2024 13:28

reading default_payment_method from back end?

1403094

revert accidental space changes;

71c5516

joyceqin-stripe added 8 commits November 27, 2024 14:13

customersheet default from back end

e3ecc2c

removed unused value

fb86faa

Merge branch 'master' into joyceqin-MOBILESDK-2799

b283446

remove unused property

9ed5ed2

Merge branch 'master' into joyceqin-MOBILESDK-2799

e689449

fix build issue

3c36c97

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

c749e6e

…s into joyceqin-MOBILESDK-2799

fix test

aac6ae1

joyceqin-stripe marked this pull request as ready for review December 2, 2024 16:52

joyceqin-stripe requested review from a team as code owners December 2, 2024 16:52

joyceqin-stripe requested a review from porter-stripe December 2, 2024 16:52

joyceqin-stripe added 2 commits December 4, 2024 09:49

merged with master

b25eb4b

Merge branch 'master' into joyceqin-MOBILESDK-2799

5936f96

porter-stripe reviewed Dec 5, 2024

View reviewed changes

joyceqin-stripe added 2 commits December 5, 2024 10:46

check default pm against savedPaymentMethods instead of customer.paym…

0fca0dc

…entMethods

removed alternateUpdatePaymentMethodNavigation, consolidated reading …

c226e09

…from elements session logic

joyceqin-stripe added 9 commits December 5, 2024 11:48

Merge branch 'master' into joyceqin-MOBILESDK-2799

4d146b6

remove alternateUpdatePaymentMethodNavigation flag and updated tests

f858536

update snapshot tests

8d1c90b

update row button chevron condition

c70c6a1

remove removeonly mode, update tests

d999b9e

fixing tests

ac5189a

remove wait for chevron existence in tests for last card

ffcf6e4

localizable string

403fdcb

edit a test

c8fbfef

joyceqin-stripe added 14 commits December 6, 2024 00:39

merged with remove alternateupdatepaymentmethodnavigation flag

2f515cd

remove removeOnly snapshots

0a92c87

Merge branch 'master' into joyceqin-remove-alternateupdatepaymentmeth…

a89890c

…odnavigation-flag

changelog

bd51b31

Merge branch 'master' into joyceqin-remove-alternateupdatepaymentmeth…

3c69384

…odnavigation-flag

update changelog and tests

f239d20

Merge branch 'joyceqin-remove-alternateupdatepaymentmethodnavigation-…

5fad2d3

…flag' of github.com:stripe/stripe-ios into joyceqin-remove-alternateupdatepaymentmethodnavigation-flag

Merge branch 'master' into joyceqin-MOBILESDK-2799

4da3034

Merge branch 'joyceqin-remove-alternateupdatepaymentmethodnavigation-…

05407cc

…flag' into joyceqin-MOBILESDK-2799

Merge branch 'master' into joyceqin-remove-alternateupdatepaymentmeth…

eaae456

…odnavigation-flag

sleep for 1 second to allow label animation to finish testRemovalOfSa…

7022a11

…vedPaymentMethods_verticalMode

Merge branch 'joyceqin-remove-alternateupdatepaymentmethodnavigation-…

f5fbd58

…flag' into joyceqin-MOBILESDK-2799

Merge branch 'master' into joyceqin-MOBILESDK-2799

cb12138

Revert space change

79f2a94

joyceqin-stripe requested a review from porter-stripe December 6, 2024 23:33

porter-stripe reviewed Dec 9, 2024

View reviewed changes

joyceqin-stripe added 5 commits December 9, 2024 10:50

fallback when opted in to set as default not local default but first …

b592b55

…in saved pm list

added tests

74c9db8

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

dbc2beb

…s into joyceqin-MOBILESDK-2799

Merge branch 'master' into joyceqin-MOBILESDK-2799

40574a6

no saved pms customersessionadapter test

2e8b49e

porter-stripe reviewed Dec 9, 2024

View reviewed changes

joyceqin-stripe added 3 commits December 9, 2024 14:59

removed explicit nil assignments when already implied

a2a37e6

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

0b6781f

…s into joyceqin-MOBILESDK-2799

Merge branch 'master' into joyceqin-MOBILESDK-2799

7d3f1c4

porter-stripe approved these changes Dec 9, 2024

View reviewed changes

joyceqin-stripe enabled auto-merge (squash) December 9, 2024 23:02

joyceqin-stripe merged commit 171edb3 into master Dec 9, 2024
6 checks passed

joyceqin-stripe deleted the joyceqin-MOBILESDK-2799 branch December 9, 2024 23:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read the default_payment_method field from elements session and use that as the default PM #4313

Read the default_payment_method field from elements session and use that as the default PM #4313

joyceqin-stripe commented Nov 27, 2024 •

edited

Loading

github-actions bot commented Nov 27, 2024 •

edited

Loading

porter-stripe left a comment

porter-stripe Dec 5, 2024

joyceqin-stripe Dec 5, 2024

joyceqin-stripe Dec 5, 2024

porter-stripe Dec 5, 2024

joyceqin-stripe Dec 5, 2024

porter-stripe Dec 6, 2024

porter-stripe Dec 5, 2024

porter-stripe Dec 5, 2024

porter-stripe Dec 5, 2024

joyceqin-stripe Dec 5, 2024

porter-stripe Dec 5, 2024

joyceqin-stripe Dec 5, 2024

porter-stripe Dec 5, 2024

porter-stripe Dec 5, 2024

porter-stripe Dec 5, 2024

porter-stripe Dec 5, 2024

porter-stripe Dec 5, 2024

joyceqin-stripe commented Dec 5, 2024

joyceqin-stripe commented Dec 6, 2024

porter-stripe left a comment

porter-stripe Dec 6, 2024

porter-stripe Dec 6, 2024

joyceqin-stripe Dec 9, 2024

porter-stripe Dec 9, 2024

joyceqin-stripe Dec 9, 2024

porter-stripe Dec 9, 2024

joyceqin-stripe Dec 9, 2024

porter-stripe Dec 9, 2024

porter-stripe left a comment

porter-stripe Dec 9, 2024

		// 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


		var selectedPaymentOption: CustomerPaymentOption?

		// read from back end

Read the default_payment_method field from elements session and use that as the default PM #4313

Read the default_payment_method field from elements session and use that as the default PM #4313

Conversation

joyceqin-stripe commented Nov 27, 2024 • edited Loading

Summary

Motivation

Testing

Changelog

github-actions bot commented Nov 27, 2024 • edited Loading

porter-stripe left a 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 Dec 5, 2024

joyceqin-stripe commented Dec 6, 2024

porter-stripe left a 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

porter-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyceqin-stripe commented Nov 27, 2024 •

edited

Loading

github-actions bot commented Nov 27, 2024 •

edited

Loading