💲[Native Checkout] Load Shipping Rules

[ifbarrera]

📲 What

Makes a network request for the shipping rules associated with a reward, if the reward has shipping enabled. Then selects the default shipping rule based on the user's countryCode from the config.

This PR does not include:

• loading animation visual behaviour
• shipping amount & currency formatting
• UI interaction behaviour with the "Default shipping location" button

🤔 Why

So we can display a default shipping location and facilitate shipping location selection (not part of this PR).

🛠 How

A network request for the selected reward's shipping rules is made immediately on viewDidLoad only if the reward is designated as reward.shipping.enabled. Once the request completes, the shipping rules are used to determine a default shipping location, which is emitted on the selectedShippingRuleData signal using combineLatest with reloadDataProperty. This guarantees that PledgeDataSource has been populated with data (via thereloadWithData output) before we attempt to set the new default shipping location and reload the shipping location cell.

Something to note is that since we're using a table view to display content in PledgeViewController, the order in which signals fire is important. In this PR, both selectedShippingRuleData and shippingIsLoading are dependent on at least one emission from the reloadDataProperty signal (not to be confused with reloadWithData). Once we add the behavior of actually displaying a loading state in the PledgeShippingLocationCell, we may have to add an input for pledgeShippingLocationCellDidDisplay to guarantee that the cell has been rendered before we emit any shippingIsLoading signals that require a cached cell to be present.

👀 See

♿️ Accessibility

• Tap targets use minimum of 44x44 pts dimensions
• Works with VoiceOver
• Supports Dynamic Type

🏎 Performance

• Optimized Blended Layers (screenshots)

✅ Acceptance criteria

In order to see the PledgeViewController, copy and paste the following code into line 131 of ProjectPamphletContentViewController:

    let pledgeVC = PledgeViewController.instantiate()
    pledgeVC.configureWith(project: project, reward: reward)

    let navigationController = UINavigationController(rootViewController: pledgeVC)

    self.present(navigationController, animated: true)

(You'll have to comment out the existing function body).

Rewards without shipping

• Log out from any account. Navigate to a project, select "Make Pledge without reward". You should not see the shipping location cell.
  Rewards with shipping
• Log out from any account. Navigate to a project, select a reward that has a "Ships to" label (these are rewards with shipping). You should see a default location populated after a second.

[ifbarrera]

[Checkout] Pledge Screen - Load Shipping Locations

[justinswart]

Some thoughts!

[justinswart]

Not as part of this PR, but would be nice if we could do something like self.indexPath(where: ...) and give it a condition to match on. Feels like us keeping track of the position of things can become problematic. Although I guess it's locked in with tests 🤷‍♂

[justinswart]

Maybe we should only allow this to be configured with the values we expect? That could also be a typealias in the associated value like PledgeShippingLocationData?

[ifbarrera]

Yeah I think this is a fair point. We share data source value types in ProjectPamphletContentDataSource (see ProjectPamphletSubpage), but the key difference is that the value configures two instances of the same cell (ProjectPamphletSubpageCell). In this case there are two different cells, which might be an argument for different typealiased value types. What I liked about using the same enum type though is being able to easily identify the type of cell at any given row, but I'll see if there's another way to do that.

[ifbarrera]

After chatting with @dusi we're thinking it makes more sense for these cells to have their own view models, to which we will directly pipe this values via a configure(with: value) input. In the view model, we can retrieve the associated values. This work will be part of a separate PR, so line 85 here is temporary, as are lines 89-92.

[justinswart]

Perhaps a helper function on the dataSource like containsShippingCell() -> Bool which you could also filter on to avoid the guard

.observeForUI()
.filter { [weak self] _ in self?.dataSource.containsShippingCell() == .some(false) }

[ifbarrera]

IndexPath still needs to be optional as a return type from self?.dataSource.shippingCellIndexPath(), so we'll still need to unwrap it somehow even if we filter the signal against it not containing the shipping cell. I don't think the filter really does much in that case 🤔

[justinswart]

I thought we could just do return self == .shippingLocation 🤔 but I see the compiler isn't happy doing that.

[justinswart]

Why the empty empty values initially?

[justinswart]

I guess I mean, why not nils?

[ifbarrera]

That's a good question, I think visually it's less jarring to see some values that then update than to see nothing and then something 🤷‍♀. In the case of the location string, I think you're right that nil would probably be better. I'll update that one 👍

[ifbarrera]

Going to revisit this one once we do more work on the PledgeShippingLocationCell's view model.

[justinswart]

Did we need this in the end? I thought that other solution worked 😓

[justinswart]

If we have no alternative I'd call this didReloadData just for some additional clarity.

[ifbarrera]

This was the only way to get it reliably working 😢 the other solution we had only worked sometimes - it was a race condition after all. This feels like a better solution anyway, because we can guarantee that tableView.reloadData() has been called.

[dusi]

Should this test also cover both logged in & logged out state to be exhausting?

[dusi]

Should it be the responsibility of the cell to have this condition? Shouldn't the cell simply get data and do whatever it wants with it? I wonder if it should be the call site that has this condition not the "dumb" view layer that's not concerned about it at all (unless it has a VM)? Not sure if we can better destructure the data in the data source before we call configureWith(value:) on this cell 🤔

[dusi]

Or maybe PledgeDataSource.PledgeInputRow should be able to provide optional values for all cases?

PledgeInputRow {
  case1
  case2
  ...

  // optional or non-optional depending on what's better for our use case
  var amount: String? { .. } // switch over
  var currency: String? { .. } // switch over
}

then we would hopefully not need to use the guard and can simply configure AmountInputView with these optionals (I guess) ... we can simply change the configureWith to take optionals since the internal logic of that on the AmountInputView would be fine with optionals

[ifbarrera]

Since we've decided to add individual view models to the input cells, I think we should pipe in the value of PledgeDataSource.PledgeInputRow directly into the cell's view model and allow it to check that the enum case is the correct one for the cell's needs.

[dusi]

So the output of the VM is feeding the input of the VM again? Is this necessary? Can't reloadWithData trigger selectedShippingRulesData?

[dusi]

Or is this because it needs to happen after the table view is reloaded?

[dusi]

I think I finally see it - it was because of https://github.com/kickstarter/ios-oss/pull/682/files#r287428427 wasn't it?

[dusi]

Since this is now shared between new and deprecated code should we maybe extract it to SharedFunctions or other helper file in KsApi? Would also allow us to test it since it's now not private function

[ifbarrera]

The intention is to move this function to be private in PledgeViewModel once we delete DeprecatedRewardPledgeViewModel. It's currently behaving as if it were private to both the PledgeViewModel and the DeprecatedRewardPledgeViewModel, and it's functionality is tested twice, both in DeprecatedRewardPledgeViewModelTests and PledgeViewModelTests. We should leave it as is to minimize the amount of cleanup work when we remove the deprecated files.

[ksr-ci-bot]

	1 Warning
⚠️	Big PR

SwiftFormat found issues:

File	Rules
Kickstarter-iOS/Views/Cells/PledgeShippingLocationCell.swift	blankLinesAroundMark, indent, wrapArguments

Generated by 🚫 Danger

[ifbarrera]

This is temporary. The intention is to pipe this value directly into a view model for this cell:

  func configureWith(value: PledgeDataSource.PledgeInputRow) {
    self.viewModel.configure(with: value)
  }

And allow the cell's view model to match against the desired PledgeInputRow case.

[ifbarrera]

Add //TODO: here to move this function to the future PledgeShippingLocationCellViewModel

[justinswart]

I think this is gtg!

My feeling is that we may be able to simplify this a fair bit by switching it to a UIStackView instead of a UITableView, let's catch up with @dusi when he's back to decide if we want to pursue that.