[MBL-1491] [MBL-1663] Handle 3DS authentication with informational snackbar

[stevestreza-ksr]

📲 What

This PR adds support for letting a user handle 3DS authentication via Stripe.

NOTE: This has not yet been tested end-to-end.

🤔 Why

Stripe won't process some transactions otherwise.

🛠 How

When the user taps the "authorize card" button, I added a loading indicator to the button and updated the enum cases to include a way to turn that loading indicator off.

Then I added fetching the client secret from GraphQL and passing that through the enum case for triggering 3DS authentication.

With those pieces in place, I updated PPOViewModel to call Stripe's authentication check. In debug mode, this is simulated for ease of testing. When the callback completes, the app informs the user that the call has either succeeded or failed. This was done with a touched up version of the MessageBannerView that already existed and had some basic hookups in the PPOViewModel.

If successful, the item is hidden. This was done by adding filtering capability to PPOViewModel and tying it to the existing Publisher chain for populating results.

I also added some convenience helpers for clarity, such as withEmptyValues() on Publisher, a pattern that I was using in a few places, and a way to map the values of a Paginator's Results (in this case, to facilitate filtering).

👀 See

Figma

✅ Acceptance criteria

Tests should pass.

Once there is a test project that is viable, end-to-end testing should succeed.

[amy-at-kickstarter]

LGTM.

I have some thoughts/suggestions/nits/discussions, but nothing that's blocking.

[amy-at-kickstarter]

Some questions/thoughts on the filtering:

1. Is this because it takes a while for the Kickstarter backend to find out that the Stripe payment has been confirmed? (How does that even happen? Stripe webhook, maybe?)
2. I remember filtering completed items was brought up really early in the PPO design process. We had some concerns that certain actions wouldn't propagate through the backend quickly enough. At the time, we thought filtering would add too much complexity - but If we need filtering for 3DS, can we also leverage it for other cards? i.e. if we're taking on the extra complexity, might as well make the whole UI snappy.

[amy-at-kickstarter]

Also, it would be good to sync with Android to make sure they're taking a similar approach.

[amy-at-kickstarter]

I have a slight preference for nesting an if over adding an empty case to all the switch cases, like

    case (PPOProjectCardModelConstants.authenticationRequired):
      if let secret = backing?.clientSecret {
        primaryAction = .authenticateCard(clientSecret: secret)
        secondaryAction = nil
        tierType = .authenticateCard
      } else {
        return nil
      }

but only because I'm frequently in files which have accumulated unused cases like case (foo, _, _, _,).

[ifosli]

I agree with this. It doesn't matter very much when it's just the client secret, but this pattern seems like it'll get complicated if we end up with multiple different cards that need info attached to the action (which might also be what I end up doing with confirm address)

[stevestreza-ksr]

For a simple case I get that it's easy to just add one extra if/else block, but those quickly turn into a big pile of spaghetti code, when someone comes along later and adds just one more condition, just one more, just one more. I find it hard to reason about code when I am walking through nested if statements, and by contrast a switch statement keeps every possible outcome clearly separated into its own chunk of code, all at the same level of indentation with dependencies explicitly declared.

It also helps when adding a new dependency or case later, because the compiler will force you to think about which cases need those dependencies. And those underscores are ugly but they also do tell you that those cases don't depend on those pieces of data, which can be useful when doing code cleanup/refactoring/debugging.

_{(Also the extremely-not-relevant-to-this-case reason that the compiler usually ends up producing better optimized assembly that's kinder on the branch predictor than if/else does, especially when nested. Which is a neat aspect to them but will have effectively no performance impact on anything here, and doesn't matter here.}

[amy-at-kickstarter]

I'm fine with that then! Excellent counter-argument.

[amy-at-kickstarter]

STPAuthenticationContext is a reference to the container view controller, right? It feels a little circular to me to have the view model hold onto a reference to the view controller.

In most of the checkout flow, we get around this by making all the Stripe calls directly from the view controller, like:
https://github.com/kickstarter/ios-oss/blob/main/Kickstarter-iOS/Features/PledgeView/Controllers/PledgeViewController.swift#L529-L532

You can convince me that putting Stripe logic in the view controller is the greater evil, but if not, there's a precedent for putting it elsewhere.

[amy-at-kickstarter]

(If you did go that way, you could probably keep the navigation code instead of making this a special case, too.)

[stevestreza-ksr]

I originally had done this in PPOContainerViewController, but moved it to PPOView/PPOViewModel so that we could also show the MessageBannerView inside the PPOView. With the code in PPOContainerViewController, it led to some extra back-and-forth message passing because state needs to get relayed back to both the PPOViewModel AND the PPOProjectCard's action buttons. So it ended up just being cleaner here.

Though I see that we also have a MessageBannerViewController we could use from PPOContainerViewController, so that might make more sense. I can try it.

[amy-at-kickstarter]

Nit: Add an inline comment describing the tuple mapping. If I'm reading correctly, it's ((a, b, c), d) -> (a, b, c, d)

[ifosli]

I like the banner changes - very nice! I am concerned about this approach to 3DS - I know we talked about the complication with where to present what (considering banners) before, but it feels weird to me to have a view model presenting UI instead of a view controller. I trust your judgement, though. But I do think it'd be helpful to test this end to end now, while you still have alternatives fresh in your mind, in case the 3DS UI doesn't get presented correctly or something.

[ifosli]

I agree with this. It doesn't matter very much when it's just the client secret, but this pattern seems like it'll get complicated if we end up with multiple different cards that need info attached to the action (which might also be what I end up doing with confirm address)

[ifosli]

I'm also concerned about handling 3DS in the view model itself. The 3DS challenge is a UI view (in a test env, it means typing in a code; in real life it varies). It feels really weird to me to have a view model present UI. I'm not sure if it's the confirmSetupIntent method that causes the UI to be presented to the user (if needed) or if there's some other method that's missing, though.