MBL-1488 & MBL-1360: Refactor Payment Sheet #2075

amy-at-kickstarter · 2024-05-31T14:39:35Z

📲 What

Refactor PledgePaymentMethodsViewController and PledgePaymentMethodsViewModel so that we always use a SetupIntent and store the card immediately, regardless of which pledge flow we're using.

🤔 Why

Late Pledges use a PaymentIntent in Stripe. When I implemented Late Pledges for the payment method sheet, I added a lot of plumbing to make the sheet use a PaymentIntent or a SetupIntent depending on context.

However, we have since learned that this behavior leads to bugs - PaymentIntent and SetupIntent lead to subtly different behavior with the payment sheet. For example, when you confirm a payment sheet made with a PaymentIntent it actually charges the card, ultimately leading to an issue with "dangling" Stripe charges that aren't attached to a pledge.

This refactor not only undoes all that, but greatly simplifies the payment sheet by making it always save a card and always returned a card that has been saved to the Kickstarter backend.

🛠 How

Each commit should be fairly atomic (although they can't be shipped separately as a functioning feature). I recommend reviewing this PR commit-by-commit.

✅ Acceptance criteria

I will run through all this acceptance criteria before merging this change.

Verify that adding a card in both pledge flow succeeds, the pledge is marked with the correct card on the KSR backend, and the card is saved to the KSR server:

Verify that the late pledge "double charge" is fixed

Add a new card in the late pledge flow. Before hitting 'pledge', tap Back, then confirm again. You should be able to select a card and complete the pledge; you should not see an error

Verify context:

Verify that the setup intent pledge context is still set correctly for a live pledge
Verify that the setup intent pledge context is still set correctly for a late pledge

Verify that the payment sheet works correctly:

Able to select an existing card
Able to select a new card
Able to add multiple new cards

Verify that the payment sheet works correctly when modifying a pledge:

Change payment method for a pledge, adding a new card
Change payment method for a pledge, selecting an existing card
Previously selected payment method cannot be selected when modifying the pledge

Verify that manage card works correctly:

Manage card settings, add a new card
Manage card settings, delete a card

nativeksr · 2024-05-31T15:28:28Z

	1 Warning
⚠️	Big PR

Generated by 🚫 Danger

amy-at-kickstarter · 2024-06-05T21:33:50Z

Kickstarter-iOS/Features/PaymentMethods/Controller/PaymentMethodSettingsViewController.swift

        strongSelf.viewModel.inputs
-          .paymentSheetDidAdd(newCard: paymentDisplayData, setupIntent: clientSecret)
+          .paymentSheetDidAdd(setupIntent: clientSecret)


This data was already unused; PaymentMethodSettingsViewController never read paymentDisplayData. It just used the clientSecret.

amy-at-kickstarter · 2024-06-05T21:34:32Z

...tarter-iOS/Features/PledgePaymentMethods/Controller/PledgePaymentMethodsViewController.swift

@@ -49,7 +49,6 @@ final class PledgePaymentMethodsViewController: UIViewController {
      |> ksr_addSubviewToParent()

    self.tableView.registerCellClass(PledgePaymentMethodCell.self)
-    self.tableView.registerCellClass(PledgePaymentSheetPaymentMethodCell.self)


n.B. this view controller still displays the list of payment methods in two sections, with the top section being 'new' cards and the bottom section being 'existing' cards. However, they use the same cell type and same backing data (a UserCreditCards.CreditCard).

amy-at-kickstarter · 2024-06-05T21:34:46Z

...tarter-iOS/Features/PledgePaymentMethods/Controller/PledgePaymentMethodsViewController.swift

@@ -80,8 +79,8 @@ final class PledgePaymentMethodsViewController: UIViewController {
      .observeValues { [weak self] data in
        guard let self = self else { return }

-        let cards = data.paymentMethodsCellData
-        let paymentSheetCards = data.paymentSheetPaymentMethodsCellData
+        let cards = data.existingPaymentMethods


Renamed these properties for (hopeful) clarity.

amy-at-kickstarter · 2024-06-05T21:35:58Z

...tarter-iOS/Features/PledgePaymentMethods/Controller/PledgePaymentMethodsViewController.swift

-        )
-        strongSelf.viewModel.inputs
-          .paymentSheetDidAdd(newCard: paymentDisplayData, clientSecret: clientSecret)
+        // Fetch the stripe payment method ID


This is a somewhat annoying workaround. When the view model calls addPaymentSheetPaymentSource to store the credit card, the Kickstarter backend does not return a stripeCardId in the UserCreditCard.CreditCard in the payload. Payments has opened a ticket about this - but for now, we fetch the stripeCardId directly from the Stripe API, and pass it back up to the view model.

amy-at-kickstarter · 2024-06-05T21:37:25Z

...tarter-iOS/Features/PledgePaymentMethods/Controller/PledgePaymentMethodsViewController.swift

      case .canceled:
-        // User cancelled intentionally so do nothing.
-        break


huh, guess our linter strips break statements

amy-at-kickstarter · 2024-06-05T21:37:48Z

...r-iOS/Features/PledgePaymentMethods/Controller/PledgePaymentMethodsViewControllerTests.swift

@@ -51,9 +51,7 @@ final class PledgePaymentMethodsViewControllerTests: TestCase {
          checkoutId: "checkoutId",
          reward: reward,
          context: .pledge,
-          refTag: nil,
-          pledgeTotal: Double.nan,
-          paymentSheetType: .setupIntent


n.B. that this stuff doesn't need to be passed around any more, because the payment sheet no longer needs a total or a type.

amy-at-kickstarter · 2024-06-05T21:38:10Z

...arter-iOS/Features/PledgePaymentMethods/Datasource/PledgePaymentMethodsDataSourceTests.swift

@@ -42,18 +42,18 @@ final class PledgePaymentMethodsDataSourceTests: XCTestCase {
  func testLoad_PaymentSheetCardValues() {
    let paymentSheetData = [
      (
-        image: UIImage(),


Data source now uses the same data type for new cards and existing cards, as mentioned.

amy-at-kickstarter · 2024-06-05T21:38:27Z

Kickstarter-iOS/Features/PledgeView/Controllers/PostCampaignCheckoutViewController.swift

-    default:
-      break
-    }
+    self.viewModel.inputs.creditCardSelected(source: paymentSource)


Now the payment sheet only returns ONE kind of payment source

Much nicer!

amy-at-kickstarter · 2024-06-05T21:39:00Z

KsApi/models/templates/graphql/GraphCreditCardTemplate.swift

  )

  public static let discover = UserCreditCards.CreditCard(
    expirationDate: "2022-03-12",
    id: "5",
    lastFour: "4242",
-    type: .discover
+    type: .discover,
+    stripeCardId: "pm_fake_discover"


We need these or else some of my new asserts will get triggered.

amy-at-kickstarter · 2024-06-05T21:39:16Z

Library/ViewModels/PaymentMethodSettingsViewModel.swift

@@ -86,25 +85,6 @@ public final class PaymentMethodSettingsViewModel: PaymentMethodsViewModelType,
    )

    let newSetupIntentCards = self.newSetupIntentCreditCardProperty.signal.skipNil()
-      .map { data -> PaymentSheetPaymentMethodCellData? in


This was all unused!

amy-at-kickstarter · 2024-06-05T21:39:47Z

Library/ViewModels/PaymentMethodSettingsViewModelTests.swift

@@ -127,7 +127,6 @@ internal final class PaymentMethodSettingsViewModelTests: TestCase {

      self.vm.inputs
        .paymentSheetDidAdd(
-          newCard: paymentOptionsDisplayData,
          setupIntent: "seti_1LVlHO4VvJ2PtfhK43R6p7FI_secret_MEDiGbxfYVnHGsQy8v8TbZJTQhlNKLZ"


@ifosli I hope these are test client secrets, otherwise we may have another thing to clean up 😅

We should check on that with the payments team, just in case? My guess is that anything that's copy-pasted would've been copied from staging and I'd be surprised if the secrets are identical between prod and staging. Want me to check with them?

amy-at-kickstarter · 2024-06-05T21:40:14Z

Library/ViewModels/PaymentSourceSelected.swift

+ */
+
+public typealias StripePaymentMethodID = String
+public typealias KSRCreditCardId = String


Just trying to make things clearer than case savedCreditCard(String, String?) which is what it really is.

amy-at-kickstarter · 2024-06-05T21:41:32Z

Library/ViewModels/PledgePaymentMethodsViewModel.swift

+      } else {
+        assert(false, "Expected stripeCardId to be set. Late pledges may fail if this value is missing.")
+        updatePaymentMethodData
+          .selectedPaymentMethod = .savedCreditCard(newestPaymentSheetPaymentMethod.card.id, "")


My goal was to make sure if stripeCardId failed anywhere, the code would still mostly work, since stripeCardId is only important for late pledges.

I like that! nit: I do think it'd be nice to pull this out into a helper function, though, considering we're doing basically the same checks in about 5 places.

Good suggestion, fixed!

amy-at-kickstarter · 2024-06-05T21:42:11Z

Library/ViewModels/PledgePaymentMethodsViewModel.swift

-    let newSetupIntentCards = self.newStripeIntentCreditCardProperty.signal.skipNil()
-      .map { data -> [PaymentSheetPaymentMethodCellData] in
-        let (displayData, setupIntent) = data
+    let newlyAddedCardProducer = self.newStripeIntentCreditCardProperty.signal.skipNil()


This is the beefiest part of the change - when a new card is selected, add it to the Kickstarter backend, then populate the payment sheet with the actual UserCreditCards.CreditCard returned by that mutation.

amy-at-kickstarter · 2024-06-05T21:43:04Z

Library/ViewModels/PledgePaymentMethodsViewModel.swift

+            return card
+          }
+          .skipNil()
+          .materialize()


One slightly annoying thing is now this extra call means the spinner on the new payment method button goes away before the new card appears. I struggled to figure out how to plumb together the reloading code to make that loading button work. Thoughts/suggestions? I don't think it's a blocking issue, but would be a nicer experience.

File a followup ticket? If I remember correctly, we're telling the spinner to hide when payment sheet dismisses. We should be able to do that when we get a result from newCards instead, but we'd need to handle the cancellation case the same as we do now, so it might get too complicated to be worth doing. I don't have any useful suggestions, at least not without digging into the code.

Library/ViewModels/PledgePaymentMethodsViewModel.swift

amy-at-kickstarter · 2024-06-05T21:44:15Z

Library/ViewModels/PostCampaignCheckoutViewModel.swift

          requiresConfirmation: true
        )
      }

-    // MARK: - Validate New Cards
-
-    let validateCheckoutNewCardInput = Signal.combineLatest(checkoutId, selectedCard)


All of this can go away, because all cards are now "existing" cards (i.e. stored to the KSR backend)

amy-at-kickstarter · 2024-06-06T15:39:02Z

Library/ViewModels/PledgePaymentMethodsViewModel.swift

@@ -126,15 +126,20 @@ public final class PledgePaymentMethodsViewModel: PledgePaymentMethodsViewModelT
          .materialize()
      }

+    let allNewlyAddedCards: Signal<[UserCreditCards.CreditCard], Never> = newlyAddedCardProducer.values()
+      .scan(into: []) { array, card in
+        array.insert(card, at: 0)


I forgot the scan at first - this turns a signal like [a, b, c] into a signal like [[a], [b, a], [c, b, a]].

…ntroller

…aymentSourceSelected Completely removes the possibility of Pledge PaymentMethodsViewController returning a SetupIntent or PaymentIntent

…er to Pledge/LatePledge controllers

…ViewModel

ifosli

I've only reviewed 4/7 commits (hence the "comment" instead of "approve") but I'll get to the rest on monday, but since you're probably starting work before me I figured I'd give you something to work with if you'd like. So far I think it looks good - just some nits that you're welcome to ignore if you disagree.

ifosli · 2024-06-06T20:43:48Z

Library/ViewModels/PledgePaymentMethodsViewModel.swift

+            return card
+          }
+          .skipNil()
+          .materialize()


File a followup ticket? If I remember correctly, we're telling the spinner to hide when payment sheet dismisses. We should be able to do that when we get a result from newCards instead, but we'd need to handle the cancellation case the same as we do now, so it might get too complicated to be worth doing. I don't have any useful suggestions, at least not without digging into the code.

ifosli · 2024-06-06T21:10:24Z

Library/ViewModels/PledgePaymentMethodsViewModel.swift

@@ -519,45 +509,49 @@ private func pledgePaymentSheetMethodCellDataAndSelectedCardSetupIntent(
  }()

  // First ensure all existing payment sheet payment methods are not selected.
-  let preexistingPaymentSheetCardDataUnselected: [PaymentSheetPaymentMethodCellData] = {
-    var updatedPaymentSheetPaymentMethodData = paymentMethodData.paymentSheetPaymentMethodsCellData
+  let preexistingPaymentSheetCardDataUnselected: [PledgePaymentMethodCellData] = {


If I understand this correctly, can we rename this to newCardDataUnselected instead of preexistingPaymentSheetCardDataUnselected?

ifosli · 2024-06-06T21:52:28Z

Library/ViewModels/PledgePaymentMethodsViewModelTests.swift

+        paymentSheetCard?.card.id,
+        addPaymentSheetResponse.createPaymentSource.paymentSource.id
+      )
+      XCTAssertFalse(paymentSheetCard?.isEnabled ?? false)


nit: since this is within a test, I think I'd rather use paymentSheetCard!.isEnabled to make sure it's false; defaulting to false if the card is nil and asserting that the bool is false seems like a bit of a dangerous pattern. Same applies to the next line

ifosli · 2024-06-06T22:20:57Z

Kickstarter-iOS/Features/PledgeView/Controllers/PostCampaignCheckoutViewController.swift

-    default:
-      break
-    }
+    self.viewModel.inputs.creditCardSelected(source: paymentSource)


Much nicer!

ifosli · 2024-06-06T22:45:19Z

...tarter-iOS/Features/PledgePaymentMethods/Controller/PledgePaymentMethodsViewController.swift

-        // User cancelled intentionally so do nothing.
-        break
+        strongSelf.viewModel.inputs.shouldCancelPaymentSheetAppearance(state: true)
+      // User cancelled intentionally so do nothing.


nit: Indentation for this comment looks off

ifosli · 2024-06-06T22:50:30Z

Library/ViewModels/PaymentMethodSettingsViewModelTests.swift

@@ -127,7 +127,6 @@ internal final class PaymentMethodSettingsViewModelTests: TestCase {

      self.vm.inputs
        .paymentSheetDidAdd(
-          newCard: paymentOptionsDisplayData,
          setupIntent: "seti_1LVlHO4VvJ2PtfhK43R6p7FI_secret_MEDiGbxfYVnHGsQy8v8TbZJTQhlNKLZ"


We should check on that with the payments team, just in case? My guess is that anything that's copy-pasted would've been copied from staging and I'd be surprised if the secrets are identical between prod and staging. Want me to check with them?

ifosli · 2024-06-06T23:01:38Z

Library/ViewModels/PledgePaymentMethodsViewModel.swift

+            }
+
+            // There is a backend bug where the stripeCardId isn't being refreshed
+            // after the add card mutation is called. This adds it back in.


nit: It'd be nice to link to a ticket here so we can check if the bug still exists the next time we touch this class, especially if we want to remove this code.

ifosli · 2024-06-06T23:05:29Z

Library/ViewModels/PledgePaymentMethodsViewModel.swift

+          if let stripeCardId = selectedCard?.stripeCardId {
+            selectedPaymentMethod = .savedCreditCard(selectedCardId, stripeCardId)
+          } else {
+            assert(false, "Expected stripeCardId to be set. Late pledges may fail if this value is missing.")


Just confirming: this assert only crashes debug builds? Would we want to log this as an error to firebase for release builds? Up to you if you think that's worth doing or not. (Probably out of scope for this, but I think it'd be nice to have an assert helper function that crashed debug builds and logged to firebase for beta/release builds.)

To the best of my knowledge, yes - we have assertions disabled at the compiler level for production builds:

I loooove the idea of something that has both an assertion and a log to firebase! Let me think about that.

ifosli · 2024-06-06T23:13:15Z

Library/ViewModels/PledgePaymentMethodsViewModel.swift

+      } else {
+        assert(false, "Expected stripeCardId to be set. Late pledges may fail if this value is missing.")
+        updatePaymentMethodData
+          .selectedPaymentMethod = .savedCreditCard(newestPaymentSheetPaymentMethod.card.id, "")


I like that! nit: I do think it'd be nice to pull this out into a helper function, though, considering we're doing basically the same checks in about 5 places.

ifosli · 2024-06-06T23:19:29Z

Library/ViewModels/PostCampaignCheckoutViewModel.swift

+      .switchMap { checkoutId, selectedCard, paymentIntentClientSecret in
+
+        assert(
+          selectedCard.stripePaymentMethodId != nil,


nit: Maybe check that the string is non-empty here instead of just non-nil (just in case we default to empty string ever in the future)?

ifosli

Looked through the last 3 commits and they look good!

amy-at-kickstarter force-pushed the feat/adyer/mbl-1488/clean-fix branch from 29f8aef to 6381fc5 Compare May 31, 2024 15:18

amy-at-kickstarter force-pushed the feat/adyer/mbl-1488/clean-fix branch 2 times, most recently from f14fa2b to 19ffab7 Compare May 31, 2024 20:42

amy-at-kickstarter changed the title ~~Feat/adyer/mbl 1488/clean fix~~ MBL-1488, MBL-1360: Refactor Payment Sheet May 31, 2024

amy-at-kickstarter force-pushed the feat/adyer/mbl-1488/clean-fix branch 3 times, most recently from 1d824e6 to ffd1eb6 Compare June 5, 2024 20:37

amy-at-kickstarter changed the title ~~MBL-1488, MBL-1360: Refactor Payment Sheet~~ MBL-1488 & MBL-1360: Refactor Payment Sheet Jun 5, 2024

amy-at-kickstarter force-pushed the feat/adyer/mbl-1488/clean-fix branch from ffd1eb6 to d169b98 Compare June 5, 2024 21:18

amy-at-kickstarter requested a review from ifosli June 5, 2024 21:32

amy-at-kickstarter self-assigned this Jun 5, 2024

amy-at-kickstarter marked this pull request as ready for review June 5, 2024 21:32

amy-at-kickstarter commented Jun 5, 2024

View reviewed changes

Library/ViewModels/PledgePaymentMethodsViewModel.swift Outdated Show resolved Hide resolved

amy-at-kickstarter commented Jun 5, 2024

View reviewed changes

amy-at-kickstarter force-pushed the feat/adyer/mbl-1488/clean-fix branch from d169b98 to 7f7092c Compare June 6, 2024 15:37

amy-at-kickstarter commented Jun 6, 2024

View reviewed changes

amy-at-kickstarter added 7 commits June 6, 2024 11:43

Save new cards as soon as they are added in PledgePaymentMethodViewCo…

b3472e2

…ntroller

Only use SetupIntent in PledgePaymentMethodsViewController

7d49cda

Remove .paymentIntentClientSecret and .setupIntentClientSecret from P…

fc9e55e

…aymentSourceSelected Completely removes the possibility of Pledge PaymentMethodsViewController returning a SetupIntent or PaymentIntent

Pass stripeCardId directly back from PledgePaymentMethodsViewControll…

eb8b9e3

…er to Pledge/LatePledge controllers

Remove unused code

cd7a7e2

Correctly handle errors when adding new cards in PledgePaymentMethods…

75bbed6

…ViewModel

Handle multiple new cards in PledgePaymentMethodsViewModel

e7dad22

amy-at-kickstarter force-pushed the feat/adyer/mbl-1488/clean-fix branch from 7f7092c to e7dad22 Compare June 6, 2024 15:43

ifosli reviewed Jun 6, 2024

View reviewed changes

ifosli approved these changes Jun 10, 2024

View reviewed changes

Address feedback from review #2075

e09039d

amy-at-kickstarter force-pushed the feat/adyer/mbl-1488/clean-fix branch from 0fa7ce2 to e09039d Compare June 10, 2024 18:13

amy-at-kickstarter merged commit ea4fb98 into main Jun 10, 2024
5 checks passed

amy-at-kickstarter deleted the feat/adyer/mbl-1488/clean-fix branch June 10, 2024 19:22

MBL-1488 & MBL-1360: Refactor Payment Sheet #2075

MBL-1488 & MBL-1360: Refactor Payment Sheet #2075

Conversation

amy-at-kickstarter commented May 31, 2024 • edited Loading

📲 What

🤔 Why

🛠 How

✅ Acceptance criteria

nativeksr commented May 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amy-at-kickstarter Jun 5, 2024 • edited Loading

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

amy-at-kickstarter Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

ifosli 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

ifosli left a comment

Choose a reason for hiding this comment

amy-at-kickstarter commented May 31, 2024 •

edited

Loading

amy-at-kickstarter Jun 5, 2024 •

edited

Loading

amy-at-kickstarter Jun 6, 2024 •

edited

Loading