Set set_as_default_payment_method field using checkbox value #4502

joyceqin-stripe · 2025-01-23T23:34:55Z

Summary

Added setAsDefault to IntentConfirmParams to set the set_as_default_payment_method flag on the confirm call when checkbox is checked

Motivation

MOBILESDK-2697
MOBILESDK-2699

Testing

Changelog

…om:stripe/stripe-ios into joyceqin/remove-button-matches-save-button

github-actions · 2025-01-23T23:52:19Z

🚨 New dead code detected in this PR:

UIColor+Extensions.swift:34 warning: Property 'textBrand' is unused
UIColor+Extensions.swift:38 warning: Property 'textDisabled' is unused
UIColor+Extensions.swift:42 warning: Property 'textCritical' is unused
UIColor+Extensions.swift:50 warning: Property 'textSuccess' is unused
UIColor+Extensions.swift:70 warning: Property 'borderCritical' is unused
UIColor+Extensions.swift:90 warning: Property 'neutral0' is unused
UIColor+Extensions.swift:114 warning: Property 'neutral300' is unused
UIColor+Extensions.swift:118 warning: Property 'neutral500' is unused
UIColor+Extensions.swift:138 warning: Property 'brand100' is unused
UIColor+Extensions.swift:150 warning: Property 'critical500' is unused
UIColor+Extensions.swift:158 warning: Property 'success100' is unused
UIColor+Extensions.swift:162 warning: Property 'success500' is unused
UIColor+Extensions.swift:182 warning: Function 'dynamic(light:dark:)' is unused
DropdownFieldElement.swift:10 warning: Imported module 'StripeCore' is unused
DropdownFieldElement.swift:267 warning: Parameter 'pickerView' 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.

github-actions · 2025-01-24T19:51:57Z

⚠️ Public API changes detected:

StripePayments

- public var setAsDefaultPM: Foundation.NSNumber?
- public var setAsDefaultPM: Foundation.NSNumber?

StripePaymentSheet

- case integrationError(nonPIIDebugDescription: Swift.String)

If you are adding a new public API consider the following:

Do these APIs need to be public or can they be protected with @_spi(STP)?
If these APIs need to be public, assess whether they require an API review.

If you are modifying or removing a public API:

Does this require a breaking version change?
Do these changes require API review?

If you confirm these APIs need to be added/updated and have undergone necessary review, add the label modifies public API to this PR to acknowledge and bypass this check.

ℹ️ If this comment appears to be left in error, make sure your branch is up-to-date with master.

…s into joyceqin/MOBILESDK-2697

joyceqin-stripe · 2025-01-28T23:41:28Z

StripePaymentSheet/StripePaymentSheetTests/PaymentSheet/PaymentSheet+APITest.swift

+                ) { result, _ in
+                    switch result {
+                    case .completed:
+                        sleep(10)


For some reason, querying the elementsSession immediately after confirming it, the default payment method returns nil— back end delay in setting it?

…s into joyceqin/MOBILESDK-2697

porter-stripe · 2025-01-29T16:33:47Z

StripePaymentSheet/StripePaymentSheet/Source/PaymentSheet/IntentConfirmParams.swift

+    var setAsDefaultPM: Bool?
+


Can we add a doc comment for what this does?

porter-stripe · 2025-01-29T16:34:09Z

...ymentSheet/Source/PaymentSheet/CustomerSheet/CustomerSavedPaymentMethodsViewController.swift

+                if configuration.allowsSetAsDefaultPM {
+                    // TODO: update internal default endpoint
+                } else {
+                    try await customerSheetDataSource.setSelectedPaymentOption(paymentOption: customerPaymentMethodOption)
+                }


Do we need this change in this PR? Seems like it doesn't really do much than no-op for allowsSetAsDefaultPM

I suppose not, I think this was a remnant of when I started implementing it for CustomerSheet but then we decided not to

porter-stripe · 2025-01-29T16:43:34Z

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

+            if let shouldSetAsDefaultPM = shouldSetAsDefaultPM {
+                params.setAsDefaultPM = NSNumber(value: shouldSetAsDefaultPM)
+            }


Can shorten this to params.setAsDefaultPM = NSNumber(value: shouldSetAsDefaultPM ?? false), same with in the other spot we do this.

porter-stripe · 2025-01-29T16:44:39Z

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

@@ -371,7 +371,8 @@ extension PaymentSheetFormFactory {
            isSelectedByDefault: false,
            didToggle: didToggle
        )
-        return PaymentMethodElementWrapper(element) { _, params in
+        return PaymentMethodElementWrapper(element) { checkbox, params in


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?

porter-stripe · 2025-01-29T16:45:31Z

StripePaymentSheet/StripePaymentSheetTests/PaymentSheet/PaymentSheet+APITest.swift

@@ -65,6 +65,28 @@ class PaymentSheetAPITest: STPNetworkStubbingTestCase {
        return newCardPaymentOption
    }()

+    lazy var newCardDefaultPaymentOption: PaymentSheet.PaymentOption = {


Any way we can DRY up the duplication of code between this and "newCardPaymentOption"?

porter-stripe · 2025-01-29T22:22:08Z

StripePaymentSheet/StripePaymentSheetTests/PaymentSheet/PaymentSheetLPMConfirmFlowTests.swift

@@ -985,7 +985,7 @@ extension IntentConfirmParams: Equatable {
        // Sanity check to make sure when we add new properties, we check them here
        let mirror = Mirror(reflecting: lhs)
        let propertyCount = mirror.children.count
-        XCTAssertEqual(propertyCount, 7)
+        XCTAssertEqual(propertyCount, 8)


If possible, I think it'd be valuable to add a test in here like testCard_OnlyCardInfo_WithDefaults or testCard_AllFields_WithDefaults where we set the default checkbox and confirm.

porter-stripe · 2025-01-29T22:22:53Z

...nts/StripePayments/Source/API Bindings/Models/SetupIntents/STPSetupIntentConfirmParams.swift

+    /// `@YES` to set this PaymentIntent’s PaymentMethod as the associated Customer's default
+    /// This should be a boolean NSNumber, so that it can be `nil`
+    @objc @_spi(STP) public var setAsDefaultPM: NSNumber?


Let's update STPSetupIntentConfirmParamsTest with these changes.

porter-stripe · 2025-01-29T22:23:41Z

...yments/StripePayments/Source/API Bindings/Models/PaymentIntents/STPPaymentIntentParams.swift

@@ -80,6 +80,10 @@ public class STPPaymentIntentParams: NSObject {
    /// This should be a boolean NSNumber, so that it can be `nil`
    @objc public var savePaymentMethod: NSNumber?

+    /// `@YES` to set this PaymentIntent’s PaymentMethod as the associated Customer's default
+    /// This should be a boolean NSNumber, so that it can be `nil`
+    @objc @_spi(STP) public var setAsDefaultPM: NSNumber?


Let's update STPPaymentIntentParamsTest with these changes

porter-stripe · 2025-01-29T22:24:58Z

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

+                        shouldSave: confirmParams.saveForFutureUseCheckboxState == .selected,
+                        shouldSetAsDefaultPM: confirmParams.setAsDefaultPM


Let's add a test like testDeferredConfirm_valid_new_card_and_save_checkbox_selected for the default PM flow.

I have a test testDeferredConfirm_valid_new_card_and_save_checkbox_selected_and_set_as_default already

wooj-stripe · 2025-02-01T01:10:58Z

StripePaymentSheet/StripePaymentSheetTests/PaymentSheet/STPFixtures+PaymentSheet.swift

+        if allowsSetAsDefaultPM {
+            customerSessionData = [
+                "mobile_payment_element": [
+                    "enabled": true,
+                    "features": ["payment_method_save": "enabled",
+                                 "payment_method_remove": "enabled",
+                                ],
+                ],
+                "customer_sheet": [
+                    "enabled": false,
+                ]
+            ]
+        }


shouldn't we set payment_method_set_as_default to enabled if allowsSetAsDefaultPM is true?

Also, please see _testValue on line 61 and see how customerSessionData can be used to pass in the customerSessionData. E.g.:

STPElementsSession._testValue(paymentMethodTypes: ["card"], customerSessionData: [ "mobile_payment_element": [ "enabled": false, ], "customer_sheet": [ "enabled": true, "features": ["payment_method_remove": "enabled"], ], ])

joyceqin-stripe added 10 commits January 21, 2025 15:17

set remove button height to match save button

7bc9d63

Merge branch 'master' into joyceqin/remove-button-matches-save-button

66255c3

separated RemoveButton into own class

0955579

Merge branch 'master' into joyceqin/remove-button-matches-save-button

2d27f68

Merge branch 'joyceqin/remove-button-matches-save-button' of github.c…

dd1ddb1

…om:stripe/stripe-ios into joyceqin/remove-button-matches-save-button

remove dead code

fdf24e5

use parameter

0615150

client-side set_as_default_payment_method

ae0cafd

Merge branch 'master' into joyceqin/MOBILESDK-2697

26401fe

always set set_as_default_payment_method param to a value

d7cbfdf

joyceqin-stripe added 5 commits January 23, 2025 16:14

remove unused code

3b33102

remove extra arguments

dad8ae7

removed more unused code

ddbeea9

set as default checkbox on new pm in customersheet

af21c85

remove checkbox from customersheet until we know better how to proceed

48f02d2

joyceqin-stripe added 10 commits January 27, 2025 10:55

revert customersheet changes

bcc1ff7

revert change

549aaef

Merge branch 'master' into joyceqin/MOBILESDK-2697

b7e62a7

attempt at test

4e762cb

test network stubs

e00ee59

changed property number in test

041f865

Merge branch 'master' into joyceqin/MOBILESDK-2697

9aa4e8b

revert test timeout

f28ac9f

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

2e33889

…s into joyceqin/MOBILESDK-2697

tests

030ca74

joyceqin-stripe marked this pull request as ready for review January 28, 2025 20:47

joyceqin-stripe requested review from a team as code owners January 28, 2025 20:47

joyceqin-stripe requested a review from porter-stripe January 28, 2025 20:47

joyceqin-stripe added 4 commits January 28, 2025 13:37

fix build?

2b75dcc

deferred paymentintent test attempt

53ae9a8

add timeout to deferred paymentintent test

c606817

Merge branch 'master' into joyceqin/MOBILESDK-2697

222bdf1

joyceqin-stripe commented Jan 28, 2025

View reviewed changes

joyceqin-stripe added 3 commits January 28, 2025 16:48

fix network stubs?

4aaa0ef

lower timeout

168ed36

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

855f7e1

…s into joyceqin/MOBILESDK-2697

porter-stripe reviewed Jan 29, 2025

View reviewed changes

joyceqin-stripe added 2 commits January 29, 2025 10:32

clean up

a9d9db1

Merge branch 'master' into joyceqin/MOBILESDK-2697

3bca1bc

joyceqin-stripe force-pushed the joyceqin/MOBILESDK-2697 branch 2 times, most recently from 9277438 to 3bca1bc Compare January 29, 2025 20:35

joyceqin-stripe added 2 commits January 29, 2025 12:37

revert params.setAsDefaultPM

3e7c01a

Merge branch 'master' into joyceqin/MOBILESDK-2697

07182b9

joyceqin-stripe requested a review from porter-stripe January 29, 2025 21:09

porter-stripe reviewed Jan 29, 2025

View reviewed changes

joyceqin-stripe added 2 commits January 30, 2025 11:44

Merge branch 'master' into joyceqin/MOBILESDK-2697

1846b90

added tests

145ff3a

joyceqin-stripe requested a review from porter-stripe January 30, 2025 19:45

return test api_key to ek

66bc395

porter-stripe approved these changes Jan 30, 2025

View reviewed changes

joyceqin-stripe merged commit e6d488d into master Jan 30, 2025
6 checks passed

joyceqin-stripe deleted the joyceqin/MOBILESDK-2697 branch January 30, 2025 22:49

wooj-stripe reviewed Feb 1, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set set_as_default_payment_method field using checkbox value #4502

Set set_as_default_payment_method field using checkbox value #4502

joyceqin-stripe commented Jan 23, 2025 •

edited

Loading

github-actions bot commented Jan 23, 2025 •

edited

Loading

github-actions bot commented Jan 24, 2025 •

edited

Loading

joyceqin-stripe Jan 28, 2025

porter-stripe Jan 29, 2025

porter-stripe Jan 29, 2025

joyceqin-stripe Jan 29, 2025

porter-stripe Jan 29, 2025

porter-stripe Jan 29, 2025

porter-stripe Jan 29, 2025

porter-stripe Jan 29, 2025

porter-stripe Jan 29, 2025

porter-stripe Jan 29, 2025

porter-stripe Jan 29, 2025

joyceqin-stripe Jan 30, 2025

wooj-stripe Feb 1, 2025

wooj-stripe Feb 1, 2025

		shouldSave: confirmParams.saveForFutureUseCheckboxState == .selected,
		shouldSetAsDefaultPM: confirmParams.setAsDefaultPM

Set set_as_default_payment_method field using checkbox value #4502

Set set_as_default_payment_method field using checkbox value #4502

Conversation

joyceqin-stripe commented Jan 23, 2025 • edited Loading

Summary

Motivation

Testing

Changelog

github-actions bot commented Jan 23, 2025 • edited Loading

github-actions bot commented Jan 24, 2025 • edited Loading

StripePayments

StripePaymentSheet

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 23, 2025 •

edited

Loading

github-actions bot commented Jan 23, 2025 •

edited

Loading

github-actions bot commented Jan 24, 2025 •

edited

Loading