[MBL-1224] Round rewards up instead of down #1955
Conversation
ifosli
requested review from
a team and
amy-at-kickstarter
and removed request for
a team
| |> Project.lens.country .~ .us | ||
| |> Project.lens.stats.currency .~ Project.Country.mx.currencyCode | ||
| let reward = Reward.noReward | ||
| |> Reward.lens.convertedMinimum .~ 0.6 |
There was a problem hiding this comment.
Nit: I've stopped (or rather, never started) using Prelude operators, unless absolutely necessary, in the hopes we can prioritize it for removal some day. But this is something we might want to discuss.
There was a problem hiding this comment.
I agree that it would be nice to remove prelude! However, in this case, I at least want to use it for the reward (and then I might as well use it for both). convertedMinimum here is immutable (as it should be) so I'm not sure how else to set it (other than not using a template for the reward at all). I think moving away from prelude/lenses for our immutable properties would maybe involve making them mutable for testing or something? Either way, it's more work to not use prelude :(
Let me know if you have any other ideas here! I'm also happy to switch the mutable properties away from prelude if you prefer; I feel ambivalent about those in this context.
There was a problem hiding this comment.
Discussed in Slack and seems like this is a bigger question. I'm totally fine ignoring this for now; my desire to clean up usage of Prelude is purely opportunistic, when it's easy.
| @@ -70,7 +70,8 @@ public final class RewardCardViewModel: RewardCardViewModelType, RewardCardViewM | |||
| Format.currency( | |||
| reward.convertedMinimum, | |||
| country: project.stats.currentCountry ?? .us, | |||
| omitCurrencyCode: project.stats.omitUSCurrencyCode | |||
| omitCurrencyCode: project.stats.omitUSCurrencyCode, | |||
| roundingMode: .up | |||
There was a problem hiding this comment.
This is probably an overly philosophical point, but what if the converted amount is $1.01? In that case, rounding it up to $2 seems unfair. Would it make more sense to just add a floor of $1?
There was a problem hiding this comment.
Kind of, but I care more about being consistent here. (Both with web and across our rewards.) For context, here's web's rounding up ticket from back in the day: https://kickstarter.atlassian.net/browse/NT-148 (this is the value that's sent to us as convertedAmount that we use for the other rewards) and here's the ticket for rounding up for add-ons: https://kickstarter.atlassian.net/browse/NT-1508. I'm not sure about this, but I think the "no reward" case slipped through the cracks when we switched from v1 to graphQL (since I think it was returned by the backend in v1). So my goal with this pr isn't to change the way we do rounding but rather to apply the scheme we're theoretically using to all rewards.
(I personally think that rounding up to the next integer is a lot, and we should consider using two decimals the way we do in the pledge summary - but that would be a big change that we should synchronize across all our clients, and is probably a question for UX instead of us.)
There was a problem hiding this comment.
Legitimate. I think consistency is a great reason to do it this way, then.
📲 What
Round up when formatting the converted amount string for rewards.
🤔 Why
Rewards should all be rounded up to be consistent with our rounding for add-ons and the rounding done on web. Note that this change only affects the "no reward" option (ie "pledge because you believe in it"), since the converted value we use for actual rewards is rounded by the backend before we see it.
With this change, we'll no longer see "About $0" pledge options.
👀 See
Jira
Spike for more details
✅ Acceptance criteria