[MBL-1210] Fix logged out and login flow for post campaign checkout #1991

ifosli · 2024-03-20T22:05:35Z

📲 What

Make "continue" button launch the login flow. Payment methods sheet should be hidden in logged out case and show up again once the user is logged in.

This makes the checkout VC a delegate of the pledge CTA and implements methods other than those corresponding to the "pledge" and "apple pay" buttons.

Note: This still hasn't added the extra warning to the pledge CTA. Since that can probably be added without touching the checkout VC, I'm going to do that in another pr to minimize merge conflicts.

👀 See

Jira

✅ Acceptance criteria

Checkout screen looks good when the user is logged out
Checkout screen is back to normal when the user logs in by hitting "continue" button

login 2

payment methods respond to login

ifosli · 2024-03-20T22:07:39Z

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

+
+  // MARK: - Public functions
+
+  func cancelPaymentSheetAppearance() {


I have no idea why this function used to live in a "delegate" (it was called a delegate, but it didn't behave like one). I've updated this to just be a public function, and because it can now be called before the view model is configured, I've correspondingly updated the view model to ignore changes here until it is configured.

amy-at-kickstarter · 2024-03-21T14:36:56Z

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

+  // MARK: - Public functions
+
+  func setShouldCancelPaymentSheetAppearance(hidden: Bool) {
+    self.viewModel.inputs.shouldCancelPaymentSheetAppearance(state: hidden)


Nit: This method name isn't totally clear to me on first read. So it means the payment sheet normally would pop up, but now it won't? Appearance makes me assume it's something like a modal.

Ah, or is this more like a dismissal?

I think this name is more clear than it was before but you're right - when I named it, I didn't know what it did either 😭. You're pretty much correct, though. Basically when you add a new card to the payment sheet, there's a loading indicator for a bit before the sheet pops up. Setting this to false (the only way it's used outside of tests) cancels that pop up (usually because we've presented a different modal instead). I think it might also dismiss the sheet itself if it's already presented by the time this is called, but I'm not sure about that one since that would only happen as a race condition. If you have any ideas for a different name, though, let me know!

Hmmm...what about something like dontShowPaymentSheet?

I'm just going to name it cancelModalPresentation I think. That's clear enough while still being general enough that you don't need to understand the paymentMethodsVC to understand what the goal is.

Sounds good to me!

amy-at-kickstarter

Some nits and commentary, but nothing blocking. Otherwise LGTM.

amy-at-kickstarter · 2024-03-21T14:37:54Z

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

+  // MARK: - Public functions
+
+  func setShouldCancelPaymentSheetAppearance(hidden: Bool) {
+    self.viewModel.inputs.shouldCancelPaymentSheetAppearance(state: hidden)


Ah, or is this more like a dismissal?

amy-at-kickstarter · 2024-03-21T14:38:59Z

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

@@ -79,6 +81,10 @@ final class PostCampaignCheckoutViewController: UIViewController {
    self.viewModel.inputs.viewDidLoad()
  }

+  deinit {
+    self.sessionStartedObserver.doIfSome(NotificationCenter.default.removeObserver)


Nifty, first time I've seen this in this codebase. Some kind of fancy RAC unsubscribe?

amy-at-kickstarter · 2024-03-21T14:48:00Z

Library/ViewModels/PledgePaymentMethodsViewModel.swift

+      self.shouldCancelPaymentSheetAppearance.signal,
+      configureWithValue
+    )
+    .map(first)


Nit: Most patterns I've seen in our codebase call configureWithValue very early, like in onAppear. When does this issue happen? Just seems a bit odd to need the protection.

It happens in the logged out state! If the user is logged out, the payment sheet doesn't actually get configured, but we still say "hey, please don't show that extra modal" if the user interacts with the pledge cta (ex: if they want to log in). Leaving it as it was causes the app to crash when that happens.

Aha, that makes sense! This would be a good comment, actually.

amy-at-kickstarter · 2024-03-21T14:50:23Z

Library/ViewModels/PostCampaignCheckoutViewModel.swift

      .compactMap { data -> PledgePaymentMethodsValue? in
        guard let user = AppEnvironment.current.currentUser else { return nil }
        guard let reward = data.rewards.first else { return nil }

        return (user, data.project, reward, data.context, data.refTag)
      }

-    // TODO: Respond to login flow.
-    let isLoggedIn = initialData.ignoreValues()
+    self.goToLoginSignup = Signal.combineLatest(initialData, self.goToLoginSignupSignal)


Is Signal.combineLatest(a, b).map(first) effectively just a takeWhen? If so, I find takeWhen clearer.

Nah, you're right, it's not any clearer, then. I guess it would be nice to have an operator like take(value: someSignal, whenAnyChange: [someSignal, anotherSignal, aThirdSignal]) but that is vastly out of scope here.

Good point! I'm actually not sure if it would behave differently if b emits before a, but it's not like that should happen here, anyways. Fixed

You were right about this one, actually! I just mixed up use-cases and made us both confused 😅

amy-at-kickstarter · 2024-03-21T14:51:15Z

Library/ViewModels/PostCampaignCheckoutViewModel.swift

    }

-    self.showWebHelp = self.pledgeDisclaimerViewDidTapLearnMoreSignal.mapConst(.trust)
+    self.paymentMethodsViewHidden = Signal.combineLatest(isLoggedIn, context)
+      .map { !$0 || $1.paymentMethodsViewHidden }


Nit: Slightly hard to read with $0, would prefer that these variables were named so we had !isLoggedIn || context.paymentMethodsViewHidden

Good point, I'll update it!

amy-at-kickstarter · 2024-03-21T14:52:23Z

Library/ViewModels/PostCampaignCheckoutViewModel.swift

@@ -101,12 +124,27 @@ public class PostCampaignCheckoutViewModel: PostCampaignCheckoutViewModelType,
    self.configureWithDataProperty.value = data
  }

+  private let (goToLoginSignupSignal, goToLoginSignupObserver) = Signal<Void, Never>.pipe()


First time I've seen this, what's the goal of using Signal<Void, Never>.pipe()?

Hmm, to be honest, this is also just copy-pasted from the standard pledge version of this VC/VM. But I think the point is that we want to react to button clicks - so we want to track whenever new events happen but we don't actually need it to pass along any data.

Aah, gotcha. I think we mix Void and () throughout the codebase. Seems fine.

ifosli added 5 commits March 20, 2024 15:55

Move cancelPaymentSheetAppearance out of delegate

f40ac82

Respond to login

3118a49

login 2

Hide payment methods when logged out

f5c47f0

payment methods respond to login

Make webHelp responsive

eb85b9c

fix format

ff9505b

ifosli self-assigned this Mar 20, 2024

ifosli commented Mar 20, 2024

View reviewed changes

Pass in bool to cancelPaymentSheet method

a445140

ifosli force-pushed the loginFlowPostCampaign branch from f4f5494 to a445140 Compare March 20, 2024 22:25

Merge branch 'main' into loginFlowPostCampaign

093bd0c

ifosli marked this pull request as ready for review March 20, 2024 22:28

ifosli added the post-campaign-pledging label Mar 20, 2024

ifosli requested review from a team and amy-at-kickstarter and removed request for a team March 20, 2024 22:28

amy-at-kickstarter reviewed Mar 21, 2024

View reviewed changes

amy-at-kickstarter approved these changes Mar 21, 2024

View reviewed changes

Minor cleanup for clarity

23001d5

ifosli force-pushed the loginFlowPostCampaign branch from d689535 to 23001d5 Compare March 21, 2024 15:59

ifosli added 2 commits March 21, 2024 10:43

Make modal cancellation more clear

a5f4549

Merge branch 'main' into loginFlowPostCampaign

237ee7e

ifosli merged commit d696e55 into main Mar 21, 2024
5 checks passed

ifosli deleted the loginFlowPostCampaign branch March 21, 2024 18:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MBL-1210] Fix logged out and login flow for post campaign checkout #1991

[MBL-1210] Fix logged out and login flow for post campaign checkout #1991

ifosli commented Mar 20, 2024 •

edited

Loading

ifosli Mar 20, 2024 •

edited

Loading

amy-at-kickstarter Mar 21, 2024

amy-at-kickstarter Mar 21, 2024

ifosli Mar 21, 2024

amy-at-kickstarter Mar 21, 2024

ifosli Mar 21, 2024

amy-at-kickstarter Mar 21, 2024

amy-at-kickstarter left a comment

amy-at-kickstarter Mar 21, 2024

amy-at-kickstarter Mar 21, 2024

amy-at-kickstarter Mar 21, 2024

ifosli Mar 21, 2024

amy-at-kickstarter Mar 21, 2024

amy-at-kickstarter Mar 21, 2024

amy-at-kickstarter Mar 21, 2024

ifosli Mar 21, 2024

ifosli Mar 21, 2024

amy-at-kickstarter Mar 21, 2024

ifosli Mar 21, 2024

amy-at-kickstarter Mar 21, 2024

ifosli Mar 21, 2024

amy-at-kickstarter Mar 21, 2024


		// MARK: - Public functions

		func cancelPaymentSheetAppearance() {

[MBL-1210] Fix logged out and login flow for post campaign checkout #1991

[MBL-1210] Fix logged out and login flow for post campaign checkout #1991

Conversation

ifosli commented Mar 20, 2024 • edited Loading

📲 What

👀 See

✅ Acceptance criteria

ifosli Mar 20, 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

amy-at-kickstarter 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ifosli commented Mar 20, 2024 •

edited

Loading

ifosli Mar 20, 2024 •

edited

Loading