Fix estimated shipping bug#2792
Conversation
There was a problem hiding this comment.
I was surprised this issue hadn't been caught by a test. Turns out we didn't have a screenshot test for RewardsCollectionViewController, so I went ahead and added one.
These tests aren't quite perfect (there looks like a bit of a scrolling issue with the selected card), but I verified that they do catch this bug.
| final class RewardCardContainerViewTests: TestCase { | ||
| func testLive_BackedProject_BackedReward() { | ||
| forEachScreenshotType(withData: allRewards, languages: [.en]) { type, rewardTuple in | ||
| forEachScreenshotType(withData: Reward.allRewards, languages: [.en]) { type, rewardTuple in |
There was a problem hiding this comment.
I wanted to reuse this in my new test, so I scoped it under Reward and cleaned it up a bit.
| ("NoReward", noReward) | ||
| ] | ||
| }() | ||
| internal extension Reward { |
There was a problem hiding this comment.
Git is incorrectly reporting this as more changed than it was, due to moving it around. I made it a static var on extension Reward; added a unique id to each reward; and correctly set the unrestricted shipping rewards to have .preference = .unrestricted.
| let itemSize = self.calculateItemSize(from: layout, using: self.collectionView) | ||
|
|
||
| if itemSize != layout.itemSize { | ||
| layout.itemSize = itemSize |
There was a problem hiding this comment.
Fixed an infinite loop which caused the test to hang.
amy-at-kickstarter
requested review from
a team and
stevestreza-ksr
and removed request for
a team
| self.shippingLocationSelectedSignal.signal | ||
| ) | ||
| .map { project, rewards, location in | ||
| let context = userIsBackingProject(project) ? |
There was a problem hiding this comment.
This is the actual bug fix.
There was a problem hiding this comment.
Makes sense to me, but there were a bunch of other fixes alongside it in that original PR. Please confirm that all those other changes in this file are still necessary.
There was a problem hiding this comment.
To recap a conversation on Slack:
- This refactoring didn't really need to go out in 5.32.0. It's related to featured rewards, and it makes more sense to test all the featured rewards stuff together. Featured rewards are going out in 5.33.0.
- I'm reverting PR MBL-2998: Make some small improvements to RewardsCollectionViewModel #2775 in the release branch to fix this bug for 5.32.0
- I'll merge this fix to main.
stevestreza-ksr
left a comment
stevestreza-ksr
left a comment
There was a problem hiding this comment.
Once that one comment is addressed, LGTM 👍
| self.shippingLocationSelectedSignal.signal | ||
| ) | ||
| .map { project, rewards, location in | ||
| let context = userIsBackingProject(project) ? |
There was a problem hiding this comment.
Makes sense to me, but there were a bunch of other fixes alongside it in that original PR. Please confirm that all those other changes in this file are still necessary.
2 participants
📲 What
Always return
.pledgecontext for reward cards created inRewardsCollectionViewModel.🤔 Why
I misunderstood the meaning of this context. A rewards card can be displayed in either
RewardsCollectionViewControllerorManagePledgeViewController, and it has slightly different display behavior in each. I incorrectly set this to return.managewhen the user had already backed the project, which hid the estimated shipping section of the card.Fixing this makes sure you always see the estimated shipping section of the rewards card during the pledge flow:
