[MBL-2062] Cleanup NoShippingAtCheckout Feature Flag #2269

scottkicks · 2025-01-30T16:09:18Z

📲 What

1st step in cleaning up legacy checkout code. Code that isn't prefaced with NoShipping)

Because we're defaulting behavior to this No Shipping flow, and removing the legacy classes in my next PR, you'll find that the legacy view controller snapshots have changes. This is because they are reacting to the No Shipping flow behavior. Since, those will be deleted in my next PR it's no big deal, but it does make the # of file changes a lot here.

🤔 Why

Part of our effort to cleanup our checkout code and remove the now legacy checkout classes.

🛠 How

Deletes featureNoShippingEnabled() from RemoteConfigFeature and cleans up all uses of it.

✅ Acceptance criteria

No Shipping At Checkout is now the only version of checkout
No Shipping Flow experiences no change functionally or visually
No Shipping Feature Flag has been removed from Remote Config Feature Flags menu
There's no way to enter the non "no shipping" flow
All tests pass

…s it

nativeksr · 2025-01-30T16:13:22Z

	1 Warning
⚠️	Big PR

Generated by 🚫 Danger

scottkicks · 2025-01-30T21:16:25Z

Library/SharedFunctions.swift

@@ -367,7 +367,7 @@ public func rewardsCarouselCanNavigateToReward(_ reward: Reward, in project: Pro

  let isBacking = userIsBacking(reward: reward, inProject: project)
  let isAvailableForNewBacker = rewardIsAvailable(reward) && !isBacking
-  let isAvailableForExistingBackerToEdit = (isBacking && (reward.hasAddOns || featureNoShippingAtCheckout()))
+  let isAvailableForExistingBackerToEdit = isBacking


In the new “no shipping” flow that is now the standard, backers should be able to edit rewards regardless of whether that reward hasAddOns. This is because backers need access to change their bonus or pledge amount.

Can you throw this in as a comment? It'd be also good to have it as a test somewhere, if that's not already covered in NoShippingPledgeViewModelTests.

scottkicks · 2025-02-03T14:25:07Z

Library/ViewModels/ManagePledgeViewModel.swift

updatePledge is not available in the no shipping flow, so we can remove it completely now

amy-at-kickstarter · 2025-02-03T14:52:53Z

.../Features/PledgePaymentMethods/Controller/PledgeRewardsSummaryTotalViewControllerTests.swift

@@ -50,17 +50,12 @@ final class PledgeRewardsSummaryTotalViewControllerTests: TestCase {
      }


Nice, good to see these tests cleaned up. Did any other tests change or break now that the feature flag is permanently on?

amy-at-kickstarter

WOW, that's a lotta deleted code! This LGTM. I've got two non-blocking comments:

First: are you confident that the critical paths for backing and updating in the no-shipping pathway are covered by the remaining tests? It's a little hard to tell just from the deletes, but now that this is the "real" pledge flow it would be good to double-check. Anywhere we were testing update should also have tests for updateReward and changePaymentMethod, for example.

Second: Wow, this ended up being a bigger change than I expected! Too late now, but in the future, I'd submit a change like this view-controller-by-view-controller. My guess is that you started by deleting the flag and figuring out which files needed to change, right? A nice way around that would be to split up your changes by commit like

commit1 Remove feature flag
commit2 Delete feature flag from FooViewModel
commit3 Delete feature flag from BarViewModel

Then you could cherry-pick commit2 and commit3 into their own individual branches, and they can be reviewed separately. Deleting the feature flag (commit1) could be submitted for review last, and would just be a one or two line change.

Thanks for fighting the good fight, looking forward to seeing this go out 🫡

amy-at-kickstarter · 2025-02-03T14:55:06Z

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

@@ -31,17 +31,12 @@ final class PostCampaignCheckoutViewControllerTests: TestCase {
    let mockService = MockService(fetchGraphUserResult: .success(response))
    let project = Project.template
      |> \.availableCardTypes .~ [CreditCardType.discover.rawValue]
-    let mockConfigClient = MockRemoteConfigClient()
-    mockConfigClient.features = [
-      RemoteConfigFeature.noShippingAtCheckout.rawValue: true


Huh, why were there tests with noShippingAtCheckout in PostCampaignCheckoutViewControllerTests? This VC should only ever have been used in the flow with shipping.

Yeah I'm not sure how this ended up here either. Glad it's being removed though.

amy-at-kickstarter · 2025-02-03T14:56:27Z

Library/RemoteConfig/RemoteConfigFeature+Helpers.swift

@@ -19,10 +19,6 @@ public func featureDarkModeEnabled() -> Bool {
  return featureEnabled(feature: .darkModeEnabled)
 }

-public func featureNoShippingAtCheckout() -> Bool {


🎉 🙌 🎉

amy-at-kickstarter · 2025-02-03T14:57:22Z

Library/SharedFunctions.swift

@@ -367,7 +367,7 @@ public func rewardsCarouselCanNavigateToReward(_ reward: Reward, in project: Pro

  let isBacking = userIsBacking(reward: reward, inProject: project)
  let isAvailableForNewBacker = rewardIsAvailable(reward) && !isBacking
-  let isAvailableForExistingBackerToEdit = (isBacking && (reward.hasAddOns || featureNoShippingAtCheckout()))
+  let isAvailableForExistingBackerToEdit = isBacking


Can you throw this in as a comment? It'd be also good to have it as a test somewhere, if that's not already covered in NoShippingPledgeViewModelTests.

amy-at-kickstarter · 2025-02-03T14:58:05Z

Library/ViewModels/ManagePledgeViewModel.swift

@@ -531,11 +523,6 @@ private func actionSheetMenuOptionsFor(

  var actions = ManagePledgeAlertAction.allCases.filter { $0 != .viewRewards }

-  // TODO: Remove 'update pledge' from ManagePledgeAlertAction after feature rollout.


amy-at-kickstarter · 2025-02-03T14:58:57Z

Library/ViewModels/ManagePledgeViewModelTests.swift

@@ -679,103 +670,6 @@ internal final class ManagePledgeViewModelTests: TestCase {
    }
  }

-  func testGoToUpdatePledge() {


Are there tests for the other goTo... methods in this file? Specifically updateReward and changePaymentMethod.

Yea! There are existing tests that cover these outputs...

When it comes to updating rewards/pledge and bonus amounts and changing payment methods, the No Shipping feature work didn't change much except removing the "Update Pledge" menu option. We did that because the pledge/bonus amount selectors were removed from the checkout screen, rendering that menu option unnecessary.

Backed vs. unbacked reward card availability and the rest of the "Edit Reward" and "Change Payment" cases are still covered. I've also spent a decent amount of time testing various edge cases with the changes in this PR.

Library/ViewModels/RewardAddOnSelectionContinueCTAViewModelTests.swift

Library/ViewModels/RewardCardContainerViewModelTests.swift

amy-at-kickstarter · 2025-02-03T15:53:47Z

Library/ViewModels/RewardCardContainerViewModelTests.swift

+      "Continue",
+      "Continue"
+    ])
+  }


ifosli

Overall, very exciting! I left a bunch of nits - up to you what you want to bother dealing with at this point. My main comment for future prs like this - make them smaller, please! If you go class by class (ex hardcode the flag to "true" first instead of deleting it entirely), it'll be easier to just delete tests that we don't need anymore, instead of accidentally modifying them, the way that we see in this pr.

ifosli · 2025-02-03T19:58:35Z

Kickstarter-iOS/Features/ManagePledge/Controllers/ManagePledgeViewController.swift

-      let vc = PledgeViewController.instantiate()
-      vc.configure(with: data)
-      vc.delegate = self
+    /// Render rewards carousel that has the shipping location dropdown


Nit: This comment doesn't seem relevant anymore

ifosli · 2025-02-03T20:01:56Z

...PledgeRewardsSummaryTotalViewControllerTests/testViewWithPledgeInFull.lang_en_device_pad.png

Nit: These snapshots are only new because the test names changed. It would be nice to mark them as moved instead of added (I don't think that's worth doing now but definitely keep it in mind when you rename files!). Do please delete the old snapshots, though!

ifosli · 2025-02-03T20:04:12Z

...ledgeViewControllerTests/testView_HasAddOns_ShippingSelected.lang_de_device_pad_LoggedIn.png

Looks like the shipping location got deleted again... That's not ideal but I don't think it's worth fixing if we're deleting the entire class anyways.

ifosli · 2025-02-03T20:07:42Z

Kickstarter-iOS/Features/ProjectPage/Controller/ProjectPageViewController.swift

-      let vc = RewardsCollectionViewController.controller(with: project, refTag: refTag)
-      self.present(vc, animated: true)
-    }
+    /// Render rewards carousel that has the shipping location dropdown


Same here; this comment doesn't seem helpful anymore

ifosli · 2025-02-03T20:11:04Z

...__/RewardAddOnSelectionViewControllerTests/testView_EmptyState.lang_de_device_phone4inch.png

This is another example of tests that would've been easier to review if they'd been deleted immediately, instead of being changed now and deleted later.

ifosli · 2025-02-03T20:15:02Z

Kickstarter-iOS/Features/RewardsCollection/Views/RewardCardView.swift

-    if featureNoShippingAtCheckout() {
-      stackViews.insert(self.estimatedShippingStackView, at: 4)
-    }
+    stackViews.insert(self.estimatedShippingStackView, at: 4)


Same here; no need to insert after list has been created.

ifosli · 2025-02-03T20:15:23Z

Kickstarter-iOS/Features/RewardsCollection/Views/RewardCardView.swift

-    if featureNoShippingAtCheckout() {
-      baseSubviews.insert(self.estimatedShippingStackView, at: 4)
-    }
+    baseSubviews.insert(self.estimatedShippingStackView, at: 4)


Same here; this could've been added to the initial list

ifosli · 2025-02-03T20:21:36Z

Library/ViewModels/PledgeShippingLocationViewModel.swift

@@ -54,18 +54,11 @@ public final class PledgeShippingLocationViewModel: PledgeShippingLocationViewMo
      .map { $0.3 }

    let shippingShouldBeginLoading = reward
-      .map { featureNoShippingAtCheckout() ? true : $0.shipping.enabled }
+      .mapConst(true)


Nit: Can we get rid of this signal entirely now? Probably in a separate pr?

ifosli · 2025-02-03T20:25:10Z

Library/ViewModels/RewardCardContainerViewModelTests.swift

    ])
    self.pledgeButtonEnabled.assertValues(
-      [true, false, false, false, false, false, false, false, false, false]
+      [true, true, true, true, true, true, true, true, true, true]


Nit: All of the assertValues in this test are super repetitive; it'd be nice to replace them with assertLastValue instead.

ifosli · 2025-02-03T20:26:15Z

Library/ViewModels/RewardCardContainerViewModelTests.swift

-      "Selected"
-    ])
-  }
+//  func testLive_BackedProject_BackedReward_Errored() {


Why is this commented out? I'm guessing this is an accident, but if it's on purpose, please add a comment explaining why/link a ticket to fix/update/delete the test!

scottkicks and others added 2 commits January 30, 2025 11:03

commit remove featureNoShippingAtCheckout() and cleanup code that use…

e2da269

…s it

Merge branch 'main' into scott/mbl-2062

8436cab

scottkicks self-assigned this Jan 30, 2025

scottkicks added 5 commits January 30, 2025 11:39

update PledgeViewControllerTests

d4fcec5

update RewardAddOnSelectionViewControllerTests

beee147

update RewardCardContainerViewTests

7d528f5

more snapshot updates

944794e

fix view model tests

c58d41c

scottkicks force-pushed the scott/mbl-2062 branch from 45ce859 to c58d41c Compare January 30, 2025 19:36

scottkicks commented Jan 30, 2025

View reviewed changes

remove updatePledge from manage pledge menu

0b0915d

scottkicks commented Feb 3, 2025

View reviewed changes

scottkicks marked this pull request as ready for review February 3, 2025 14:31

scottkicks requested review from amy-at-kickstarter and ifosli February 3, 2025 14:32

amy-at-kickstarter reviewed Feb 3, 2025

View reviewed changes

amy-at-kickstarter approved these changes Feb 3, 2025

View reviewed changes

fix commented out tests and add an inline comment

914ca91

amy-at-kickstarter reviewed Feb 3, 2025

View reviewed changes

Library/ViewModels/RewardCardContainerViewModelTests.swift

"Continue",

"Continue"

])

}

Copy link

Contributor

amy-at-kickstarter Feb 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

jovaniks merged commit 45775a2 into main Feb 3, 2025
5 checks passed

jovaniks deleted the scott/mbl-2062 branch February 3, 2025 19:31

ifosli reviewed Feb 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MBL-2062] Cleanup NoShippingAtCheckout Feature Flag #2269

[MBL-2062] Cleanup NoShippingAtCheckout Feature Flag #2269

scottkicks commented Jan 30, 2025 •

edited

Loading

nativeksr commented Jan 30, 2025

scottkicks Jan 30, 2025 •

edited

Loading

amy-at-kickstarter Feb 3, 2025

scottkicks Feb 3, 2025

amy-at-kickstarter Feb 3, 2025

amy-at-kickstarter left a comment •

edited

Loading

amy-at-kickstarter Feb 3, 2025

scottkicks Feb 3, 2025

amy-at-kickstarter Feb 3, 2025

amy-at-kickstarter Feb 3, 2025

amy-at-kickstarter Feb 3, 2025

amy-at-kickstarter Feb 3, 2025

scottkicks Feb 3, 2025

scottkicks Feb 3, 2025

amy-at-kickstarter Feb 3, 2025

amy-at-kickstarter Feb 3, 2025

ifosli left a comment

ifosli Feb 3, 2025

ifosli Feb 3, 2025

ifosli Feb 3, 2025

ifosli Feb 3, 2025

ifosli Feb 3, 2025

ifosli Feb 3, 2025

ifosli Feb 3, 2025

ifosli Feb 3, 2025

ifosli Feb 3, 2025

ifosli Feb 3, 2025

		@@ -50,17 +50,12 @@ final class PledgeRewardsSummaryTotalViewControllerTests: TestCase {
		}

		@@ -531,11 +523,6 @@ private func actionSheetMenuOptionsFor(

		var actions = ManagePledgeAlertAction.allCases.filter { $0 != .viewRewards }

		// TODO: Remove 'update pledge' from ManagePledgeAlertAction after feature rollout.

[MBL-2062] Cleanup NoShippingAtCheckout Feature Flag #2269

[MBL-2062] Cleanup NoShippingAtCheckout Feature Flag #2269

Conversation

scottkicks commented Jan 30, 2025 • edited Loading

📲 What

🤔 Why

🛠 How

✅ Acceptance criteria

nativeksr commented Jan 30, 2025

scottkicks Jan 30, 2025 • 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

amy-at-kickstarter left a comment • 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

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

scottkicks commented Jan 30, 2025 •

edited

Loading

scottkicks Jan 30, 2025 •

edited

Loading

amy-at-kickstarter left a comment •

edited

Loading