[NT-286][NT-360] SCA Support #902

justinswart · 2019-10-18T16:51:09Z

📲 What

Adds SCA support to our create and update pledge flows.

🤔 Why

From the Stripe docs:

Strong Customer Authentication (SCA), a rule in effect as of September 14, 2019, as part of PSD2 regulation in Europe, requires changes to how your European customers authenticate online payments. Card payments require a different user experience, namely 3D Secure, in order to meet SCA requirements. Transactions that don’t follow the new authentication guidelines may be declined by your customers’ banks.

🛠 How

Added an output to PledgeViewModel to trigger the SCA flow.
Added an input to PledgeViewModel to handle the response (succeeded/failed/canceled).
During this process I noticed that we weren't being very DRY about handling the create and update signals. Their patterns are exactly the same so I refactored things to be more reusable. Tests passed without any changes except for the SCA stuff that was added.
Also introduced StripePaymentHandlerActionStatus (an enum) and StripePaymentHandlerActionStatusType (a protocol) that allows us to more easily mock the Stripe response enum in tests.

Note: I've renamed createApplePayBackingEvent to deprecatedCreateApplePayBackingEvent in the expectation that it will be removed soon when we've updated our CreateBacking mutation to accept ApplePayParams.

✅ Acceptance criteria

Using the test cards listed here:

Note: Please be aware of the caveats mentioned in this thread when testing.

Create a new pledge, the SCA test modal should be displayed.
Update a pledge, the SCA test modal should be displayed.
Change payment method from a non-SCA card to an SCA card, the SCA test modal should be displayed.
When creating a pledge, confirming in the SCA modal should dismiss and take you to the thanks page.
When updating a pledge, confirming in the SCA modal should dismiss with success and return to manage plege.
In any of the above scenarios producing an error from the modal (by way of an error test card), should dismiss and display the error in the error banner.
In any of the above scenarios dismissing the modal shouldn't do anything.
No Apple Pay pledges should trigger this SCA flow.

…update tests

justinswart · 2019-10-18T16:53:18Z

Library/StripeTypes.swift

+  }
+}
+
+public enum StripePaymentHandlerActionStatus {


This type maps to STPPaymentHandler.ActionStatus and makes it easier for us to mock in tests without having to link the Stripe SDK to our Library framework.

justinswart · 2019-10-18T16:53:32Z

Library/ViewModels/PledgeViewModel.swift

@@ -33,6 +33,7 @@ public protocol PledgeViewModelInputs {
  )
  func paymentAuthorizationViewControllerDidFinish()
  func pledgeAmountViewControllerDidUpdate(with data: PledgeAmountData)
+  func scaFlowCompleted(with result: StripePaymentHandlerActionStatusType, error: Error?)


We pass in our protocol type here.

justinswart · 2019-10-18T16:53:59Z

Library/StripeTypes.swift

+import Foundation
+import KsApi
+
+public protocol StripeSCARequiring {


This protocol helps to merge the signals in PledgeViewModel for simplification.

justinswart · 2019-10-18T16:54:36Z

Library/ViewModels/PledgeViewModel.swift

-    let updateBackingDidCompleteApplePay = updateApplePayBackingSuccess
-      .ignoreValues()
+    #warning("To be removed once new CreateBacking is implemented to handle Apple Pay")
+    let deprecatedCreateApplePayBackingCompleted = Signal.combineLatest(


Self-explanatory but this should be removed once we're handling Apple Pay through CreateBacking.

justinswart · 2019-10-18T16:55:14Z

Library/ViewModels/PledgeViewModel.swift

@@ -636,17 +667,23 @@ public class PledgeViewModel: PledgeViewModelType, PledgeViewModelInputs, Pledge
  public let popViewController: Signal<(), Never>
  public let sectionSeparatorsHidden: Signal<Bool, Never>
  public let shippingLocationViewHidden: Signal<Bool, Never>
+  public let showErrorBannerWithMessage: Signal<String, Never>


Errors are all emitted through one output now.

Library/ViewModels/PledgeViewModelTests.swift

ksr-ci-bot · 2019-10-18T17:14:05Z

	1 Warning
⚠️	Big PR

Generated by 🚫 Danger

ifbarrera · 2019-10-21T16:21:12Z

Library/ViewModels/PledgeViewModel.swift

@@ -318,7 +315,7 @@ public class PledgeViewModel: PledgeViewModelType, PledgeViewModelInputs, Pledge
      )
    }

-    let createApplePayBackingEvent = createApplePayBackingData.map(
+    let deprecatedCreateApplePayBackingEvent = createApplePayBackingData.map(


This is deprecated cuz we'll be merging it with createBacking soon right?

That's correct, we should be able to remove this completely once that work is done.

ifbarrera · 2019-10-22T15:23:08Z

Kickstarter-iOS/Views/Controllers/PledgeViewController.swift

+    STPPaymentHandler.shared().confirmSetupIntent(
+      withParams: .init(clientSecret: secret),
+      authenticationContext: self
+    ) { [weak self] status, _, error in


Just curious what's this middle parameter here that we're ignoring?

That's the SetupIntent object. I don't believe we need it for anything.

ifbarrera · 2019-10-22T15:23:36Z

Kickstarter-iOS/Views/Controllers/PledgeViewController.swift

+  }
+}
+
+// MARK: STPAuthenticationContext


Can you add the - here

ifbarrera · 2019-10-22T15:29:04Z

Library/ViewModels/PledgeViewModel.swift

+    let createOrUpdateApplePayBackingCompleted = Signal.combineLatest(
+      Signal.merge(
+        updateBackingEvent.filter { $0.isTerminating }.ignoreValues(),
+        createBackingEvent.filter { $0.isTerminating }.ignoreValues()


Should this be deprecatedCreateApplePayBackingEvent? 🤔

That is handled on its own so that it'll be easy to remove later. This future-proofs for when CreateBacking supports Apple Pay.

ifbarrera · 2019-10-22T15:32:50Z

Library/ViewModels/PledgeViewModel.swift

-    self.createBackingError = Signal.merge(
-      createApplePayBackingError,
-      createBackingEventError
+    let createOrUpdateApplePayBackingCompleted = Signal.combineLatest(


Wondering if this should be a zip to account for multiple groups of events?:

User opens payment sheet and pledges - payment sheet dismisses, but pledge fails.

User opens up payment sheet a second time and pledges - because the createBackingEvent.filter event has already fired once before, the next paymentAuthorizationDidFinish event will cause the createOrUpdateApplePayBackingCompleted to fire without waiting for the next request to terminate.

🤔

I'll try to write a failing test for it and then prove that scenario 👍

Ok @ifbarrera @Scollaco I've taken a new approach which clears the values for the create and update backing events each time a new flow is started. I think this gives us more confidence that we won't have overlapping values between flows being interrupted and new ones starting.

The tests are passing (all several thousand lines of them 😓).

Note right now I don't think the deprecatedCreateApplePayBacking needs this treatment because it's somewhat separate, but I didn't want to spend too much time on that given that we should remove it in the next PR.

ifbarrera · 2019-10-22T15:40:18Z

Library/ViewModels/PledgeViewModel.swift

-        .ignoreValues(),
-      updatePaymentAuthorizationDidFinishSignal
-    )
+    let didCompleteApplePayBacking = createOrUpdateBackingEventValues


Curious why do we need to take createOrUpdateBackingEventValues here? That seems to require either createBackingEvent.values() or updateBackingEvent.values().

Yes this is future proof for when createBacking supports Apple Pay.

ifbarrera · 2019-10-22T15:47:49Z

Library/ViewModels/PledgeViewModelTests.swift

+      self.submitButtonEnabled.assertValues([false, true, false, true])
+      self.goToThanks.assertDidNotEmitValue()
+      self.showErrorBannerWithMessage.assertValues(
+        ["The operation couldn’t be completed. (KsApi.GraphError error 5.)"]


This is a strange error message. Shouldn't this be "Something went wrong." which is the localizedDescription of GraphError.invalidInput?

ifbarrera · 2019-10-22T15:49:36Z

Library/ViewModels/PledgeViewModelTests.swift

+        with: MockStripePaymentHandlerActionStatus(status: .succeeded), error: nil
+      )
+
+      self.beginSCAFlowWithClientSecret.assertValues(["client-secret"])


This should probably be above the call to inputs.scaFlowCompleted no?

Oh I see it's already on line 1205, apologies!

ifbarrera · 2019-10-22T15:52:07Z

Library/ViewModels/PledgeViewModel.swift


-    let updateBackingEventErrors = updateBackingEvent.errors()
+    let createUpdateBackingEventErrors = Signal.merge(
+      deprecatedCreateApplePayBackingEvent.errors().map { $0 as Error },


I don't think you should have to do this casting to Error 🤔

I think that's because self.showErrorBannerWithMessage merges different types of errors (GraphError and Error). What if here we simplify to:

let createUpdateBackingEventErrors = Signal.merge( deprecatedCreateApplePayBackingEvent.errors(), createBackingEvent.errors(), updateBackingEvent.errors() ).map { $0 as Error }

Moved the mapping to after the merge. I think I was moving things around and did this to satisfy the compiler at some point.

Once we've removed deprecatedCreateApplePayBackingCompletedError this should be a little simpler.

Moved this around to better support Graph vs SCA errors 👍

Scollaco · 2019-10-22T15:56:40Z

KsApi/models/Checkout.swift

@@ -12,7 +12,7 @@ public struct Checkout: Decodable {
  }

  public struct Backing: Decodable {
-    let requiresAction: Bool
-    let clientSecret: String?
+    public let requiresAction: Bool


Could you alpha here?

Scollaco · 2019-10-22T15:56:48Z

Library/StripeTypes.swift

+
+public protocol StripeSCARequiring {
+  var requiresSCAFlow: Bool { get }
+  var clientSecret: String? { get }


Scollaco · 2019-10-22T15:58:25Z

Library/StripeTypes.swift

+public enum StripePaymentHandlerActionStatus {
+  case succeeded
+  case canceled
+  case failed


Scollaco · 2019-10-22T17:28:29Z

Library/ViewModels/PledgeViewModel.swift

-      self.submitButtonTappedSignal.mapConst(true),
-      willUpdateApplePayBacking.mapConst(false)
+    let deprecatedCreateApplePayBackingCompletedError = deprecatedCreateApplePayBackingCompleted
+      .withLatest(from: deprecatedCreateApplePayBackingEvent.errors())


For curiosity, why are we using withLatest(from:) and not zip here? Shouldn't deprecatedCreateApplePayBackingCompletedError only emit when both deprecatedCreateApplePayBackingCompleted and deprecatedCreateApplePayBackingEvent.errors() emit?

I wanted to avoid zip just because it can often cause sneaky problems, but that's possible true for this case.

I'll look into this along with #902 (comment).

I don't think this is going to be necessary - #902 (comment).

Also we did withLatest(from:) here because the errors emit but we have a race condition between the Apple Pay sheet dismissing and the events emitting, which is why wait for deprecatedCreateApplePayBackingCompleted, which is a combined signal to emit and then take the most recently emitted errors.

Scollaco · 2019-10-22T17:35:36Z

Library/ViewModels/PledgeViewModel.swift


-    let updateBackingEventErrors = updateBackingEvent.errors()
+    let createUpdateBackingEventErrors = Signal.merge(


Could you rename this to createUpdateBackingEventErrors to match the createOrUpdateBacking... convention adopted above?

Scollaco · 2019-10-22T17:44:56Z

Library/ViewModels/PledgeViewModel.swift


-    let updateBackingEventErrors = updateBackingEvent.errors()
+    let createUpdateBackingEventErrors = Signal.merge(
+      deprecatedCreateApplePayBackingEvent.errors().map { $0 as Error },


I think that's because self.showErrorBannerWithMessage merges different types of errors (GraphError and Error). What if here we simplify to:

let createUpdateBackingEventErrors = Signal.merge( deprecatedCreateApplePayBackingEvent.errors(), createBackingEvent.errors(), updateBackingEvent.errors() ).map { $0 as Error }

Scollaco · 2019-10-22T17:45:48Z

Library/ViewModels/PledgeViewModel.swift


-    let updateApplePayBackingError = updateApplePayBackingCompleted
-      .withLatest(from: updateBackingEventErrors)
+    let createUpdateApplePayBackingError = createOrUpdateApplePayBackingCompleted


Could you rename this variable too?

Scollaco · 2019-10-22T17:45:57Z

Library/ViewModels/PledgeViewModel.swift

      .map(second)

-    let updateBackingError = submitButtonTapped
-      .takePairWhen(updateBackingEventErrors)
+    let createUpdateBackingError = submitButtonTapped


# Conflicts: # Library/ViewModels/PledgeViewModel.swift # Library/ViewModels/PledgeViewModelTests.swift

ifbarrera

All my comments are either minor changes or suggestions. Awesome work on this, I know it was tricky one! 🎉

ifbarrera · 2019-10-23T20:45:41Z

Library/ViewModels/PledgeViewModel.swift

@@ -443,6 +440,7 @@ public class PledgeViewModel: PledgeViewModelType, PledgeViewModelInputs, Pledge
      createBackingEvent.filter { $0.isTerminating }.mapConst(true),
      updateBackingEvent.filter { $0.isTerminating }.mapConst(true)
    )
+    .skipRepeats()


ifbarrera · 2019-10-23T20:52:58Z

Library/ViewModels/PledgeViewModel.swift

@@ -459,90 +457,139 @@ public class PledgeViewModel: PledgeViewModelType, PledgeViewModelInputs, Pledge
      updateButtonIsLoading
    )

-    // MARK: - Success/Failure Create
+    let submitButtonTapped = Signal.merge(


The naming of this signal is a little misleading, because really what you're capturing here is whether a normal transaction has begun (to differentiate between a normal transaction and an apple pay transaction). I wonder if something like isCreateOrUpdateBacking would be more appropriate considering the usage below on line 508:

let createOrUpdateBackingDidCompleteNoSCA = submitButtonTapped .takeWhen(createOrUpdateBackingEventValuesNoSCA) .filter(isTrue) .ignoreValues()

would become

let createOrUpdateBackingDidCompleteNoSCA = isCreateOrUpdateBacking .takeWhen(createOrUpdateBackingEventValuesNoSCA) .filter(isTrue) .ignoreValues()

Just a thought.

ifbarrera · 2019-10-23T20:57:28Z

Library/ViewModels/PledgeViewModel.swift

+      deprecatedCreateApplePayBackingEvent.values()
+        .combineLatest(with: deprecatedCreateApplePayBackingCompleted)
+        .ignoreValues(),
+      didCompleteApplePayBacking.combineLatest(with: willCreateApplePayBacking).ignoreValues(),


I have a hunch that the combineLatest here isn't needed because createOrUpdateApplePayBackingCompleted zips with didInitiateApplePayBacking which merges with willCreateApplePayBacking (phew 😅). Fine to leave it if you want, but I would just try removing it and see if any tests break.

Yeah naming is hard. In this particular case the combined signal is to differentiate between whether this updated or created an Apple Pay Backing 🤕 .

ifbarrera · 2019-10-23T21:02:48Z

Library/ViewModels/PledgeViewModelTests.swift

-    self.submitButtonEnabled.assertValues(
-      [false, false, false, false, true, false, true, false], "Amount and shipping rule unchanged"
-    )
+    self.submitButtonEnabled.assertValues([false, true, false, true, false], "Shipping rule changed")


Is the comment a little off here?

ifbarrera · 2019-10-23T21:05:15Z

Library/ViewModels/PledgeViewModelTests.swift

+    }
+  }
+
+  func testApplePayBackingFails_ThenSucceeds_SignalsDoNotOverlap() {


Looks like this is testing with the .update context. Can we update (see what I did there) the name of this test to reflect that?

ifbarrera · 2019-10-23T21:06:15Z

Library/ViewModels/PledgeViewModelTests.swift

      self.goToThanks.assertDidNotEmitValue()
    }
  }
+
+  func testApplePayBackingFails_ThenStoredCardSucceeds_SignalsDoNotOverlap() {


Same here the context seems to be .update.

Scollaco

Well done on this one, Justin! 🎉 🎉

justinswart added 2 commits October 18, 2019 06:24

Add SCA support, refactor signals in PledgeViewModel to be more DRY, …

36ae036

…update tests

Add tests

24d0857

justinswart requested review from ifbarrera and Scollaco October 18, 2019 16:51

justinswart assigned Scollaco Oct 18, 2019

justinswart commented Oct 18, 2019

View reviewed changes

Merge branch 'master' into sca-support

b7dc2ce

ksr-ci-bot reviewed Oct 18, 2019

View reviewed changes

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

ksr-ci-bot reviewed Oct 18, 2019

View reviewed changes

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

ksr-ci-bot reviewed Oct 18, 2019

View reviewed changes

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

justinswart added 3 commits October 18, 2019 10:25

Fix line lengths

86038a7

Improve signal naming

b1f3897

Remove type specification

7409321

ifbarrera reviewed Oct 22, 2019

View reviewed changes

Add dash

c0d0099

Scollaco reviewed Oct 22, 2019

View reviewed changes

justinswart added 7 commits October 22, 2019 11:18

Merge branch 'master' into sca-support

ee0fdcf

# Conflicts: # Library/ViewModels/PledgeViewModel.swift # Library/ViewModels/PledgeViewModelTests.swift

Rename signals to createOrUpdate

d7f1d7b

Fix errors

453907e

Alphabetize

0a1a2ab

Fix args

d8ef631

Fix and add tests for possibly overlapping signals

47d03b4

Move things around to try to improve diff

1f8314c

ifbarrera approved these changes Oct 23, 2019

View reviewed changes

Scollaco approved these changes Oct 23, 2019

View reviewed changes

justinswart changed the title ~~[NT-286] SCA Support~~ [NT-286][NT-360] SCA Support Oct 23, 2019

justinswart added 2 commits October 23, 2019 14:50

Improve signal naming

9262c14

Add context to test name

73b513c

justinswart merged commit b09379c into master Oct 23, 2019

justinswart deleted the sca-support branch October 23, 2019 22:19


		let updateBackingEventErrors = updateBackingEvent.errors()
		let createUpdateBackingEventErrors = Signal.merge(

[NT-286][NT-360] SCA Support #902

[NT-286][NT-360] SCA Support #902

Conversation

justinswart commented Oct 18, 2019 • edited by Scollaco Loading

📲 What

🤔 Why

🛠 How

✅ Acceptance criteria

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

ksr-ci-bot commented Oct 18, 2019

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

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

ifbarrera 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

Scollaco left a comment

Choose a reason for hiding this comment

justinswart commented Oct 18, 2019 •

edited by Scollaco

Loading