MBL-1969: Remove unused amount selector from NoShippingPledge flow #2247

amy-at-kickstarter · 2025-01-15T16:24:30Z

📲 What

Remove unused amount selector from NoShippingPledgeViewModel and NoShippingPledgeViewController.

🤔 Why

The amount selector was used to update your bonus support on the checkout page of the pledge flow. In the no-shipping flow, that selector is moved to its own page. This means that all of the UI and VM code for the amount selector can be deleted from NoShippingPledge pages.

Removing this code improves the clarity of our NoShippingPledgeViewModel by deleting a fairly sized chunk of unused code. Fewer signals means better readability!

🛠 How

I did this change in multiple stages: first tearing out the outputs, then the signal inputs, then the signals themselves. Fixing this also required cleaning up and fixing some of our checkout tests, many of which were no longer relevant to the no shipping flow.

Please see inline comments.

nativeksr · 2025-01-15T17:14:42Z

	1 Warning
⚠️	Big PR

Generated by 🚫 Danger

amy-at-kickstarter · 2025-01-16T15:11:59Z

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

@@ -41,11 +41,6 @@ final class NoShippingPledgeViewController: UIViewController,

  private var paymentSectionLabel: UILabel = { UILabel(frame: .zero) }()

-  private lazy var pledgeAmountViewController = {
-    PledgeAmountViewController.instantiate()


pledgeAmountViewController was no longer added to the view hierarchy in NoShippingPledgeViewController - it was here, wired up, but invisible and unused.

amy-at-kickstarter · 2025-01-16T15:12:46Z

Library/ViewModels/NoShippingPledgeViewModel.swift

  var pledgeAmountSummaryViewHidden: Signal<Bool, Never> { get }
  var popToRootViewController: Signal<(), Never> { get }
  var processingViewIsHidden: Signal<Bool, Never> { get }
-  var showApplePayAlert: Signal<(String, String), Never> { get }


This alert was only used to show information about pledge min/max issues. With the amount selector gone, the pledge can never be over/under on this page.

amy-at-kickstarter · 2025-01-16T15:13:24Z

Library/ViewModels/NoShippingPledgeViewModel.swift

@@ -95,7 +90,6 @@ public class NoShippingPledgeViewModel: NoShippingPledgeViewModelType, NoShippin

    let backing = project.map { $0.personalization.backing }.skipNil()

-    self.pledgeAmountViewHidden = context.map { $0.pledgeAmountViewHidden }


This did sometimes return false but as mentioned, the pledgeAmountView wasn't in the view hierarchy any more.

amy-at-kickstarter · 2025-01-16T15:13:53Z

Library/ViewModels/NoShippingPledgeViewModel.swift

@@ -445,7 +407,7 @@ public class NoShippingPledgeViewModel: NoShippingPledgeViewModelType, NoShippin
    }

    self.goToApplePayPaymentAuthorization = paymentAuthorizationData
-      .takeWhen(goToApplePayPaymentAuthorization)
+      .takeWhen(self.applePayButtonTappedSignal)


pledgeAmount is now always valid, so an extra validation signal isn't needed here.

amy-at-kickstarter · 2025-01-16T15:15:39Z

Library/ViewModels/NoShippingPledgeViewModel.swift

    let isEnabled = Signal.merge(
      self.viewDidLoadProperty.signal.mapConst(false)
        .take(until: valuesChangedAndValid.ignoreValues()),
      valuesChangedAndValid,
      self.submitButtonTappedSignal.mapConst(false),
-      createOrUpdateEvent.filter { $0.isTerminating }.mapConst(true)
+      valuesChangedAndValid.takeWhen(didCreateOrUpdateBacking)


I found this small bug while running through the tests. The bug looked like this:

Go to checkout

Don't select a payment method. The CTA is disabled.

Hit ApplePay

ApplePay fails for some reason

The CTA becomes enabled, even though there is no payment method selected still. That's because createOrUpdateEvent was always mapped to true.

This fixes that edge case.

amy-at-kickstarter · 2025-01-16T15:17:17Z

Library/ViewModels/NoShippingPledgeViewModelTests.swift

-      self.configurePledgeViewCTAContainerViewIsEnabled.assertValues([false])
+      self.configurePledgeViewCTAContainerViewIsEnabled.assertValues(
+        [true],
+        "CTA is automatically enabled when you are updating the pledge."


Updated this test to reflect the actual page behavior. When you update your reward, you land on the checkout page with the CTA enabled. There's nothing to select on the checkout page to change the CTA state.

amy-at-kickstarter · 2025-01-16T15:17:52Z

Library/ViewModels/NoShippingPledgeViewModelTests.swift

@@ -401,18 +354,23 @@ final class NoShippingPledgeViewModelTests: TestCase {
      self.configurePaymentMethodsViewControllerWithContext.assertValues([.changePaymentMethod])

      self.configurePledgeViewCTAContainerViewIsLoggedIn.assertValues([true])
-      self.configurePledgeViewCTAContainerViewIsEnabled.assertValues([false])
+      self.configurePledgeViewCTAContainerViewIsEnabled.assertLastValue(
+        false,


Per @ifosli suggestion, made a lot of these tests assertLastValue instead of assertValues.

amy-at-kickstarter · 2025-01-16T15:20:07Z

Library/ViewModels/NoShippingPledgeViewModelTests.swift


    let data = PledgeViewData(
      project: project,
      rewards: [reward],
+      bonusSupport: 5.0,


For lots of these tests, I had to add back the bonusSupport to the correct place. Before, the amount selector (even though it was hidden!) was echoing the selected bonus amount back to the view model.

…odel and ViewController

ifosli

Looks good! I like what you did with the tests! If you haven't already, though, it might be good to just verify that the pledge flow still works as it should. (Ex: I'd probably test edit reward flow when actually changing the reward, edit reward when changing add-ons only, edit reward when just changing bonus support, change payment method, starting the pledge flow as a logged-out user.) But I think you were planning to wait to merge this anyways until after the release is cut, so I'm fine with just doing extra careful pledge-related testing as part of regression testing for the release after (potentially covering this pr and more).

ifosli · 2025-01-20T23:45:21Z

Library/ViewModels/NoShippingPledgeViewModelTests.swift

@@ -2330,7 +2124,7 @@ final class NoShippingPledgeViewModelTests: TestCase {
      ])
      self.popToRootViewController.assertValueCount(1)
      self.showErrorBannerWithMessage.assertDidNotEmitValue()
-      self.configurePledgeViewCTAContainerViewIsEnabled.assertValues([false, false, true])
+      self.configurePledgeViewCTAContainerViewIsEnabled.assertValues([false])
      self.goToThanksProject.assertDidNotEmitValue()


I read through this test to understand why the true disappeared (looks like that's because of the bug you fixed; I'm a little annoyed there was a test that tested wrong behavior) and my goodness some of these old tests really like to copy paste assertDidNotEmitValue! Not an actionable thing for you (I don't think it's feasible to completely rewrite all our tests) but I didn't realize they were this tedious.

Yeah, I didn't realize that easier! Definitely a big area for improvement. I remember testability came up in our retro for Pledge Management, and it's good to know there's a lot of room for improvement there.

scottkicks

👍 and ➕1 to ingerid's comment about calling special attention to pledge regression tests in the next release.

amy-at-kickstarter · 2025-01-27T21:04:02Z

Excellent callout about testing! Yes, I'll make sure this release has some extra regression testing - since we're going to merge other checkout improvements, too.

As for this one, I've tested pledging as well as updating my pledge, both on staging and in prod, and it all looks good.

amy-at-kickstarter force-pushed the feat/adyer/mbl-1969 branch from c198beb to dc12984 Compare January 15, 2025 22:02

amy-at-kickstarter changed the title ~~Feat/adyer/mbl 1969~~ MBL-1969: Remove unused amount selector from NoShippingPledge flow Jan 16, 2025

amy-at-kickstarter commented Jan 16, 2025

View reviewed changes

amy-at-kickstarter marked this pull request as ready for review January 16, 2025 15:21

amy-at-kickstarter requested review from scottkicks and ifosli January 16, 2025 15:21

amy-at-kickstarter added 5 commits January 16, 2025 15:51

MBL-1969: Clean up unused amount selector from NoShippingPledge ViewM…

c28e1e1

…odel and ViewController

Fix a couple tests

05a4d86

Delete some lines I commented out

4806a29

Don't accidentally commit feature flag

e5a310f

Fix failing test

902d481

amy-at-kickstarter force-pushed the feat/adyer/mbl-1969 branch from b316808 to 902d481 Compare January 16, 2025 20:51

One last bit of test cleanup

a3e10d1

ifosli approved these changes Jan 20, 2025

View reviewed changes

scottkicks approved these changes Jan 21, 2025

View reviewed changes

Merge branch 'main' into feat/adyer/mbl-1969

d7de916

amy-at-kickstarter merged commit 9294c35 into main Jan 27, 2025
5 checks passed

amy-at-kickstarter deleted the feat/adyer/mbl-1969 branch January 27, 2025 21:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MBL-1969: Remove unused amount selector from NoShippingPledge flow #2247

MBL-1969: Remove unused amount selector from NoShippingPledge flow #2247

amy-at-kickstarter commented Jan 15, 2025 •

edited

Loading

nativeksr commented Jan 15, 2025

amy-at-kickstarter Jan 16, 2025

amy-at-kickstarter Jan 16, 2025

amy-at-kickstarter Jan 16, 2025

amy-at-kickstarter Jan 16, 2025

amy-at-kickstarter Jan 16, 2025

amy-at-kickstarter Jan 16, 2025

amy-at-kickstarter Jan 16, 2025

amy-at-kickstarter Jan 16, 2025

ifosli left a comment

ifosli Jan 20, 2025

amy-at-kickstarter Jan 27, 2025

scottkicks left a comment

amy-at-kickstarter commented Jan 27, 2025

		@@ -95,7 +90,6 @@ public class NoShippingPledgeViewModel: NoShippingPledgeViewModelType, NoShippin

		let backing = project.map { $0.personalization.backing }.skipNil()

		self.pledgeAmountViewHidden = context.map { $0.pledgeAmountViewHidden }

MBL-1969: Remove unused amount selector from NoShippingPledge flow #2247

MBL-1969: Remove unused amount selector from NoShippingPledge flow #2247

Conversation

amy-at-kickstarter commented Jan 15, 2025 • edited Loading

📲 What

🤔 Why

🛠 How

nativeksr commented Jan 15, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottkicks left a comment

Choose a reason for hiding this comment

amy-at-kickstarter commented Jan 27, 2025

amy-at-kickstarter commented Jan 15, 2025 •

edited

Loading