Extend UpdateCardViewController to bank accounts and sepa debit #4280

joyceqin-stripe · 2024-11-18T23:42:08Z

Summary

Refactored UpdateCardViewController to UpdatePaymentMethodViewController and extended its functionality to include US bank accounts and SEPA debit. Created an UpdatePaymentMethodViewModel to determine which form should be shown. Conditionally displays "Save" button and "_ details cannot be changed." label.

Motivation

MOBILESDK-2743
MOBILESDK-2662

Testing

https://github.com/user-attachments/assets/49331f2f-1fe7-45bf-91eb-7554c22eecdf

Changelog

…ripe-ios into joyceqin-updatecard-cvc-expiry

…Sheet. Edit icon shows for all cards regardless of cbc eligibility.

…reorganized code for card details section, and added conditionally rendered label

…tion and updated tests

…pe-ios into joyceqin-update-bank-account

StripePaymentSheet/StripePaymentSheet/Source/Categories/String+Localized.swift

porter-stripe · 2024-11-20T17:49:18Z

.../Source/PaymentSheet/CustomerSheet/CustomerSavedPaymentMethodsCollectionViewController.swift

                                              isTestMode: configuration.isTestMode,
-                                              cardBrandFilter: savedPaymentMethodsConfiguration.cardBrandFilter)
+                                              cardBrandFilter: savedPaymentMethodsConfiguration.cardBrandFilter,
+                                              viewModel: UpdatePaymentMethodViewModel(paymentMethod: paymentMethod, canEdit: paymentMethod.isCoBrandedCard && cbcEligible, canRemove: configuration.paymentMethodRemove && (savedPaymentMethods.count > 1 || configuration.allowsRemovalOfLastSavedPaymentMethod)))


Let's break the view model instantiation into it's own line, it's difficult to read. Same for wherever this is done.

That's fair, done.

porter-stripe · 2024-11-20T17:53:23Z

...tSheet/StripePaymentSheet/Source/PaymentSheet/Elements/TextField/TextFieldElement+IBAN.swift

@@ -179,6 +179,24 @@ extension TextFieldElement {
            }
        }
    }
+
+    struct LastFourIBANConfiguration: TextFieldElementConfiguration {
+        let label: String = STPLocalizedString("IBAN", "Label for an IBAN field")


We should moved this into where Localized.bank_account lives.

However, I'm not really sure it' makes senes to localize "IBAN", I don't think it'll localize into anything. Can you confirm in the project channel?

I removed the STPLocalizedString part and just have it as a hardcoded String "IBAN" now.

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

porter-stripe · 2024-11-20T18:26:50Z

...tSheet/StripePaymentSheet/Source/PaymentSheet/Elements/TextField/TextFieldElement+Card.swift

@@ -361,4 +361,23 @@ extension TextFieldElement {
            return TextFieldElement.PANConfiguration(cardBrandDropDown: cardBrandDropDown).accessoryView(for: lastFourFormatted, theme: theme)
        }
    }
+    struct USBankNumberConfiguration: TextFieldElementConfiguration {


Let's put this in it's own file, it doesn't belong in the +Card file.

Created a new file, TextFieldElement+Card.swift specifically for USBankNumberConfiguration

...ripePaymentSheet/Source/PaymentSheet/ViewControllers/UpdatePaymentMethodViewController.swift

porter-stripe · 2024-11-20T18:38:55Z

...ripePaymentSheet/Source/PaymentSheet/ViewControllers/UpdatePaymentMethodViewController.swift

+        return stackView
+    }()
+
+    private lazy var sepaDebitSection: UIStackView = {


I think it would be good to move all the form creation for card, us bank account and sepa into it's own class.

Then it can have a function where you pass it in a view model and it just returns a view.

Or inside of the view model.

porter-stripe · 2024-11-20T18:40:58Z

...et/StripePaymentSheet/Source/PaymentSheet/ViewControllers/UpdatePaymentMethodViewModel.swift

+
+class UpdatePaymentMethodViewModel {
+    let paymentMethod: STPPaymentMethod
+    static let supportedPaymentMethods: [STPPaymentMethodType] = [.card, .USBankAccount, .SEPADebit]


Nit: On the ordering here, let's put all static variables up on their own and not in between more "normal" variables for readability.

class UpdatePaymentMethodViewModel { static let supportedPaymentMethods: [STPPaymentMethodType] = [.card, .USBankAccount, .SEPADebit] let paymentMethod: STPPaymentMethod }

Also does it need to be static? Do we ever reference this property where we don't have an instance of UpdatePaymentMethodViewModel?

It's used in VerticalSavedPaymentMethodsViewController and SavedPaymentMethodCollectionView when checking if a payment method should allow editint

porter-stripe · 2024-11-20T18:43:01Z

StripeUICore/StripeUICore/Source/Elements/Factories/TextFieldElement+Factory.swift

        public let disallowedCharacters: CharacterSet = .whitespacesAndNewlines
        let invalidError = Error.invalid(
            localizedDescription: String.Localized.invalid_email
        )

-        init(defaultValue: String? = nil, isOptional: Bool = false) {
+        public init(defaultValue: String? = nil, isOptional: Bool = false, isEditable: Bool = true) {


Why does this need to be public now?

It doesn't— I don't remember what led me to add that in. I've reverted that part

Oh wait actually, I think it was because I was getting inaccessible due to '@_spi' protection level errors

…level

…pe-ios into joyceqin-update-bank-account

porter-stripe · 2024-11-21T17:38:38Z

...Sheet/StripePaymentSheet.xcodeproj/project.xcworkspace/xcshareddata/IDEWorkspaceChecks.plist

@@ -0,0 +1,8 @@
+<?xml version="1.0" encoding="UTF-8"?>


Don't think we should commit this, but not sure why it's not being ignored.

porter-stripe · 2024-11-21T17:38:45Z

...Sheet/StripePaymentSheet.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved

@@ -0,0 +1,24 @@
+{


Same here, let's not commit this.

porter-stripe · 2024-11-21T17:44:15Z

...tSheet/StripePaymentSheet/Source/PaymentSheet/Elements/TextField/TextFieldElement+IBAN.swift

+        let isEditable = false
+
+        private var lastFourFormatted: String {
+            "•••• \(lastFour)"


In the figma I'm seeing the last four of IBAN in SEPA having a space between the 2 and 3rd digit. Looking at the snapshots that doesn't appear to be the case here.

I remember being told that the IBAN shouldn't have the space between those digits, but I'll verify in the channel

Bella confirmed there shouldn't be a space there.

porter-stripe · 2024-11-21T17:51:42Z

...ePaymentSheet/StripePaymentSheet/Source/PaymentSheet/ViewControllers/PaymentMethodForm.swift

@@ -0,0 +1,148 @@
+//
+//  PaymentMethodForm.swift


Now that this is a standalone class and not a variable nested within the update screen, the name "PaymentMethodForm" seems ambiguous. Maybe "SavedPaymentMethodFormFactory" or something.

porter-stripe · 2024-11-21T17:53:15Z

...ePaymentSheet/StripePaymentSheet/Source/PaymentSheet/ViewControllers/PaymentMethodForm.swift

+        case .SEPADebit:
+            return sepaDebitSection
+        default:
+            fatalError("Updating payment method has not been implemented for \(viewModel.paymentMethod.type)")


Nit: This error message doesn't make much sense in the context of this class

porter-stripe · 2024-11-21T17:57:11Z

...ripePaymentSheet/Source/PaymentSheet/ViewControllers/UpdatePaymentMethodViewController.swift

+        stackView.addArrangedSubview(updateButton)
+        stackView.addArrangedSubview(removeButton)
+        stackView.addArrangedSubview(errorLabel)


Why not just add these to the array on line 50?

I don't know how to insert footnoteLabel at an index into the stackView array— addArrangedSubview adds to the end of the array. If I add these to the array on line 50, then when I add footnoteLabel, it will show up below the remove button.

Never mind, found out that there's insertArrangedSubview(view, at)

porter-stripe · 2024-11-21T18:01:08Z

StripePaymentSheet/StripePaymentSheet/Source/PaymentSheet/Views/CircularButton.swift

@@ -103,7 +103,7 @@ class CircularButton: UIControl {
            accessibilityIdentifier = "CircularButton.Remove"
        case .edit:
            imageView.image = Image.icon_edit.makeImage(template: true)
-            accessibilityLabel = String.Localized.update_card_brand


I was wrong here, sorry! I got this mixed up w/ accessibilityIdentifier! Let's localize this, apologies for the churn on this.

…added Update payment method localized string, changed error message

…pe-ios into joyceqin-update-bank-account

joyceqin-stripe added 30 commits November 13, 2024 12:23

added cvc and expiry to updatecardviewcontroller

f5752ae

make it look like the figma and update tests

dc09a75

removed unused

9791ce3

localizable

9aa250a

Merge branch 'master' into joyceqin-updatecard-cvc-expiry

203c866

revert a space

f6740cf

Merge branch 'joyceqin-updatecard-cvc-expiry' of github.com:stripe/st…

3bd0c85

…ripe-ios into joyceqin-updatecard-cvc-expiry

Merge branch 'master' into joyceqin-updatecard-cvc-expiry

acf866e

Merge branch 'master' into joyceqin-updatecard-cvc-expiry

c9c6441

applied changes

d92004f

localizable

7ca5bfd

Merge branch 'master' into joyceqin-updatecard-cvc-expiry

864f405

fix a test

9ea21cf

changes

f793251

changes

3c55c81

Merge branch 'master' into joyceqin-updatecard-cvc-expiry

a59a64f

Merge branch 'master' into joyceqin-updatecard-cvc-expiry

07dcd21

Added a flag for default spm navigation for PaymentSheet and Customer…

21a311c

…Sheet. Edit icon shows for all cards regardless of cbc eligibility.

revert snapshot test

13e4f86

updated snapshots

136da06

removed unused string in this pr

c251929

Added US Bank Account and SEPA Debit UI to UpdateCardViewController, …

3e12769

…reorganized code for card details section, and added conditionally rendered label

merged with joyceqin-updatecard-cvc-expiry and resolved conflicts

56315a3

Merge branch 'master' into joyceqin-default-spm-navigation

011f7c9

refactored deleteButton to removeButton

2421272

refactored defaultSPMNavigation to alternateUpdatePaymentMethodNaviga…

9b74677

…tion and updated tests

resolve merge conflict

debee92

Merge branch 'master' into joyceqin-default-spm-navigation

3d72712

Card brand choice dropdown updates updateButton state

95bb746

merge conflict

656e44a

joyceqin-stripe added 5 commits November 19, 2024 15:06

removed unused params

125f972

update comment

ddb1e04

remove unused param

d25fc27

fix ui test

1dd9636

Merge branch 'master' into joyceqin-update-bank-account

4a38362

joyceqin-stripe marked this pull request as ready for review November 20, 2024 17:37

joyceqin-stripe requested review from a team as code owners November 20, 2024 17:37

joyceqin-stripe requested a review from porter-stripe November 20, 2024 17:37

joyceqin-stripe added 2 commits November 20, 2024 10:09

change error message

4f96bcc

Merge branch 'joyceqin-update-bank-account' of github.com:stripe/stri…

6bbb2e9

…pe-ios into joyceqin-update-bank-account

porter-stripe added the ship without translations label Nov 20, 2024

porter-stripe reviewed Nov 20, 2024

View reviewed changes

joyceqin-stripe added 7 commits November 20, 2024 19:53

extracted form creation and applied suggested changes

b0404ce

conditional spacing depending on existence of footnote;

d5a5a28

made EmailConfiguration init public again due to internal protection …

5318956

…level

update snapshots

a10899f

Merge branch 'master' into joyceqin-update-bank-account

4b0b0db

unused parameter

68b6000

Merge branch 'joyceqin-update-bank-account' of github.com:stripe/stri…

3fd9277

…pe-ios into joyceqin-update-bank-account

joyceqin-stripe requested a review from porter-stripe November 21, 2024 04:48

porter-stripe reviewed Nov 21, 2024

View reviewed changes

joyceqin-stripe added 3 commits November 21, 2024 11:19

removed .xcshareddata from git, renamed PaymentMethodForm

bfac23c

renamed PaymentMethodForm.swift to SavedPaymentMethodFormFactory, re-…

aee04e5

…added Update payment method localized string, changed error message

Merge branch 'master' into joyceqin-update-bank-account

22706e9

joyceqin-stripe requested a review from porter-stripe November 21, 2024 19:43

joyceqin-stripe added 3 commits November 21, 2024 12:00

insertArrangedSubview

852fdf4

Merge branch 'joyceqin-update-bank-account' of github.com:stripe/stri…

d76c8c2

…pe-ios into joyceqin-update-bank-account

Merge branch 'master' into joyceqin-update-bank-account

26e4351

joyceqin-stripe enabled auto-merge (squash) November 21, 2024 21:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend UpdateCardViewController to bank accounts and sepa debit #4280

Extend UpdateCardViewController to bank accounts and sepa debit #4280

joyceqin-stripe commented Nov 18, 2024 •

edited

Loading

porter-stripe Nov 20, 2024

joyceqin-stripe Nov 21, 2024

porter-stripe Nov 20, 2024

joyceqin-stripe Nov 21, 2024

porter-stripe Nov 20, 2024

joyceqin-stripe Nov 21, 2024

porter-stripe Nov 20, 2024

porter-stripe Nov 20, 2024

porter-stripe Nov 20, 2024

porter-stripe Nov 20, 2024

joyceqin-stripe Nov 20, 2024

porter-stripe Nov 20, 2024

joyceqin-stripe Nov 21, 2024

joyceqin-stripe Nov 21, 2024

porter-stripe Nov 21, 2024

porter-stripe Nov 21, 2024

porter-stripe Nov 21, 2024

joyceqin-stripe Nov 21, 2024

joyceqin-stripe Nov 21, 2024

porter-stripe Nov 21, 2024

porter-stripe Nov 21, 2024

porter-stripe Nov 21, 2024

joyceqin-stripe Nov 21, 2024

joyceqin-stripe Nov 21, 2024

porter-stripe Nov 21, 2024

Extend UpdateCardViewController to bank accounts and sepa debit #4280

Are you sure you want to change the base?

Extend UpdateCardViewController to bank accounts and sepa debit #4280

Conversation

joyceqin-stripe commented Nov 18, 2024 • edited Loading

Summary

Motivation

Testing

Changelog

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

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 Nov 18, 2024 •

edited

Loading