MBL-2998: Make some small improvements to RewardsCollectionViewModel#2775
Conversation
| self.shippingLocationSelectedSignal | ||
| ) | ||
|
|
||
| let filteredByLocationRewards = Signal.combineLatest(rewards, self.shippingLocationSelectedSignal) |
There was a problem hiding this comment.
This is a little confusing, which is why I refactored it. There were two signals for rewards, which were switched on or combined. There was the rewards signal, which was sorted (but not filtered), and the filteredByLocationRewards signal, which was sorted and then filtered.
By doing some refactoring, I made this just one signal, rewards, which will be replaced with the new network fetch in a follow-up PR.
| .filter { reward in isStartDateBeforeToday(for: reward) } | ||
| .map { reward in (project, reward, .pledge, nil) } | ||
| .map { project, rewards, location in | ||
| let context = userIsBackingProject(project) ? |
There was a problem hiding this comment.
Looks like this has been wrong for a while, fixed it while I was in the neighborhood.
| let isRewardLocalOrDigital = isRewardDigital(reward) || isRewardLocalPickup(reward) | ||
| let isUnrestrictedShippingReward = reward.isUnRestrictedShippingPreference | ||
| let isRestrictedShippingReward = reward.isRestrictedShippingPreference | ||
| private func shouldShowReward( |
There was a problem hiding this comment.
This is a very light refactoring of filteredRewardsByLocation. Should be a little clearer.
| ) -> Bool { | ||
| // Check if the reward isn't available yet. | ||
| // These are usually filtered out by the backend, but may be visible if you're the project creator. | ||
| if !isStartDateBeforeToday(for: reward) { |
There was a problem hiding this comment.
This is the one filtering behavior tweak. I moved this out of self.reloadDataWithValues and into the filtering method itself, where it belongs.
n.B. we don't filter rewards that are no longer available, those are still displayed (with a disabled button).
| let isRewardLocalOrDigital = isRewardDigital(reward) || isRewardLocalPickup(reward) | ||
| let isUnrestrictedShippingReward = reward.isUnRestrictedShippingPreference | ||
| let isRestrictedShippingReward = reward.isRestrictedShippingPreference | ||
| let isNoRewardReward = reward.isNoReward |
There was a problem hiding this comment.
This was checking if the reward was first in the list. Changed to noRewardReward for clarity, sicne that's what was intended.
| guard let location else { | ||
| // This is a restricted reward, but the user hasn't selected a shipping location yet. | ||
| // Filter it out until a location is set. | ||
| return false |
There was a problem hiding this comment.
We don't actually display rewards until the shipping location is set, anyways.
| } | ||
|
|
||
| func testRewardsOrdered() { | ||
| func testRewardsOrderedAndFiltered() { |
There was a problem hiding this comment.
Expanded this test a bit to handle some under-tested cases.
| |> Reward.lens.shippingRulesExpanded .~ [] | ||
| |> Reward.lens.shipping .~ Reward.Shipping( | ||
| enabled: true, | ||
| enabled: false, |
There was a problem hiding this comment.
These mock objects were wrong. When a reward is unshippable (local or digital), shippingEnabled is false. Here's an example GraphQL response showing some test rewards:
[
{
"name": "pickup yer stickers",
"shippingEnabled": false,
"shippingPreference": "local",
"localReceiptLocation": {
"displayableName": "Brooklyn, NY"
}
},
{
"name": "Digital reward",
"shippingEnabled": false,
"shippingPreference": "none",
"localReceiptLocation": null
}
]
amy-at-kickstarter
requested review from
a team and
stevestreza-ksr
and removed request for
a team
|
|
||
| let rewards = project | ||
| .map(allowableSortedProjectRewards) | ||
| let shippingLocation = Signal.merge( |
There was a problem hiding this comment.
Just moved this up top.
amy-at-kickstarter
left a comment
amy-at-kickstarter
left a comment
There was a problem hiding this comment.
Some comments for whoever reviews this!
stevestreza-ksr
left a comment
stevestreza-ksr
left a comment
There was a problem hiding this comment.
Appreciate the comments, this one was a little hard to follow along with.
Left one note about how this reward shipping restriction is handled but that's not caused by this PR, so seems fine to me
| return false | ||
| } | ||
|
|
||
| assert(false, "A reward should either be restricted, or unrestricted. Showing in all locations.") |
There was a problem hiding this comment.
Is this a restriction imposed on us from v1 API or something? It feels like we could catch this during validation of objects at parsing, since this would seem to suggest an invalid data state. Not a blocker, just noting this pattern smells a little.
There was a problem hiding this comment.
Hrm, yeah, very fair point about the smell. Looking into the helper functions like isRewardLocal, they all seem to be checking multiple fields:
!existingReward.shipping.enabled &&
existingReward.localPickup != nil &&
existingReward.shipping
.preference.isAny(of: Reward.Shipping.Preference.local)instead of just switching on the existing enum, the Reward.Shipping.Preference.
It's not clear to me if you can end up in a state where, say, your shipping preference is local but the localPickup object isn't set. So I agree with you, I like the idea of dealing with that in parsing, not here. (And probably this should just be an enum? You can't be a locally-picked up digital object, now can you?)
I filed a follow-up ticket MBL-3104 to look into this.
amy-at-kickstarter
changed the title
amy-at-kickstarter
deleted the
feat/adyer/mbl-2998/reward-carousel-cleanup
branch
2 participants
📲 What
Make some improvements and refactors to
RewardsCollectionViewModel, all as prep for shipping Featured Rewards.initcode, combining the signalsfilteredRewardsByLocationandrewardsinto one signal,rewardscontextwas set incorrectly🤔 Why
As part of the change for featured rewards,
RewardsCollectionViewModelwill fetch a list of sorted rewards from the server. The list will be re-fetched when the shipping location changes. This refactoring makes it easier to plug in that API query.