Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MBL-1631] Remove Shipping Dropdown From Add-Ons #2097

Merged
merged 10 commits into from
Jul 22, 2024

Conversation

scottkicks
Copy link
Contributor

@scottkicks scottkicks commented Jul 18, 2024

📲 What

Removes the shipping drop-down from our AddOns screen

  • Mostly deleting code and updating tests

🤔 Why

The shipping dropdown will only be used on the Rewards screen to filter rewards shown based on location. This is the first step in removing the dropdown from the other places in the app where it's displayed.

🛠 How

  1. Remove from RewardAddOnsSelectionViewController
  2. In RewardsCollectionViewModel make sure the selected ShippingRule is passed to the AddOns VM so that the correct AddOns are still fetched for the chosen reward.
  3. To pass the ShippingRule along from the Rewards screen, I've added it as a property in our PledgeViewData struct. This is used to configure the AddOns screen and final Pledge screen.
  4. Update snapshot tests. This includes updating RewardAddOnsSelectionViewControllerTests to use our newew orthogonalCombos approach. This is why so many snapshots have been deleted.

👀 See

Crowdfunding (Local/No shipping) Crowdfunding (Shipping Enabled) Late Pledge Manage Pledge
remove-add-ons-local remove-add-ons-with-shipping late-pledge-remove-add-ons-shipping Simulator Screen Recording - iPhone 15 Pro Max - 2024-07-18 at 13 39 08

✅ Acceptance criteria

  • The shipping dropdown no longer shows on AddOns screen in normal crowdfunding, late, or manage pledge flow.
  • AddOns screen still shows the correct and available AddOns for the selected reward
  • Late Pledge and Checkout flows are still complete successfully; the shipping total will be hardcoded to 0.

⏰ TODO

  • Next up is removing from Crowdfunding Checkout screen

@scottkicks scottkicks self-assigned this Jul 18, 2024
@scottkicks scottkicks force-pushed the scott/mbl-1616/remove-shipping-dropdown branch from 52e21c9 to aeb9a21 Compare July 18, 2024 18:59
@kickstarter kickstarter deleted a comment from nativeksr Jul 18, 2024
@scottkicks scottkicks force-pushed the scott/mbl-1616/remove-shipping-dropdown branch 2 times, most recently from ca55f48 to 299efac Compare July 18, 2024 19:42
@scottkicks scottkicks force-pushed the scott/mbl-1616/remove-shipping-dropdown branch from 299efac to 6e7eac6 Compare July 18, 2024 20:27
@kickstarter kickstarter deleted a comment from nativeksr Jul 18, 2024
@nativeksr
Copy link
Collaborator

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

@scottkicks scottkicks changed the title [MBL-1616] Remove Shipping Dropdown From Add-Ons [MBL-1600] Remove Shipping Dropdown From Add-Ons Jul 18, 2024
@scottkicks scottkicks changed the title [MBL-1600] Remove Shipping Dropdown From Add-Ons [MBL-1631] Remove Shipping Dropdown From Add-Ons Jul 18, 2024
@scottkicks scottkicks marked this pull request as ready for review July 18, 2024 20:48
@scottkicks scottkicks requested review from ifosli and Arkariang July 18, 2024 20:49
@@ -39,6 +39,7 @@ public typealias PKPaymentData = (displayName: String, network: String, transact
public struct PledgeViewData: Equatable {
public let project: Project
public let rewards: [Reward]
public let selectedShippingRule: ShippingRule?
Copy link
Contributor Author

@scottkicks scottkicks Jul 22, 2024

Choose a reason for hiding this comment

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

I am adding this so that the ShippingRule will be passed along to Confirm Details (late pledge flow) or PledgeViewController (crowdfunding flow) and not break any existing behavior.

This is why you see updates in the other view models and most of the tests. They get configured with this PledgeViewData struct and, therefore, need this new update.


self.configurePledgeShippingLocationViewControllerWithData = fetchShippingLocations
.map { project, reward, initialLocationId in (project, reward, false, initialLocationId) }
let selectedShippingRule = configData.map(\.selectedShippingRule)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now being passed in, from Rewards screen, via the configuration object

let checkoutPropertiesData = checkoutProperties(
from: project,
baseReward: baseReward,
addOnRewards: selectedRewards,
selectedQuantities: configData.selectedQuantities,
additionalPledgeAmount: additionalPledgeAmount,
pledgeTotal: pledgeTotal,
shippingTotal: shippingTotal,
shippingTotal: 0,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Defaulting the shipping total to 0 so that everything can remain backward compatible. Eventually, the aim is to remove shipping from checkout API calls.

@@ -100,6 +100,7 @@ final class RewardsCollectionViewController: UICollectionViewController {
self.setupConstraints()

self.viewModel.inputs.viewDidLoad()
self.viewModel.inputs.shippingRuleSelected(nil)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a workaround that I'm not sold on. The selected ShippingRule needs to emit a signal for the view model outputs that trigger navigation to the add-ons or pledge screens to complete. However, in the case where there are only rewards without shipping (Digital or Local pickup only), the Shipping Dropdown will not appear on the screen. Therefore, a ShippingRule will not be selected, and the navigation outputs will not complete.

So the solution here is to emit nil on viewDidLoad so they can. I imagine there is a way to handle this in ReactiveSwift that I'm missing, but for now, I'm moving forward with this approach to keep the development going.

@@ -285,7 +243,7 @@ public final class RewardAddOnSelectionViewModel: RewardAddOnSelectionViewModelT

let combinedPledgeTotal = Signal.combineLatest(
additionalPledgeAmount,
allRewardsShippingTotal,
additionalPledgeAmount.mapConst(0),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

mapping this to a const of 0. explanation in this comment

@scottkicks scottkicks merged commit b9398aa into main Jul 22, 2024
5 checks passed
@scottkicks scottkicks deleted the scott/mbl-1616/remove-shipping-dropdown branch July 22, 2024 16:13
scottkicks added a commit that referenced this pull request Jul 23, 2024
* WIP: populate shipping countries using the project's rewards

* No longer using individually passed in Rewards

* remove shipping dropdown from Add-Ons View Controller

* select reward with selected ShippingRule, or nil if no shippable rewards

* update RewardsCollectionViewModel Tests

* update RewardAddOnSelectionViewModel Tests

* update PledgeViewModel Tests

* update ConfirmDetailsViewModel Tests

* use passed in selectedShippingRule, if not nil, so that the manage pledge flow knows about the correct shipping rule

* this is temporary since shipping will be removed from checkout during this milestone

* update ManagePledgeViewModel Tests

* update snapshot tests
scottkicks added a commit that referenced this pull request Jul 23, 2024
scottkicks added a commit that referenced this pull request Jul 24, 2024
* Revert "[MBL-1631] Remove Shipping Dropdown From Add-Ons (#2097)"

This reverts commit b9398aa.

* Revert "[MBL-1599] Adds the Shipping Dropdown Selector to the Rewards Carousel. (#2095)"

This reverts commit e495aca.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants