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.

Sign up for GitHub

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

Jump to bottom

[MBL-1224] Round rewards up instead of down #1955

Merged
merged 3 commits into from
Feb 23, 2024
Merged

[MBL-1224] Round rewards up instead of down #1955

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f5e5600
Round rewards up instead of down
ifosli Feb 22, 2024
d118731
Merge branch 'main' into roundUpRewards
ifosli Feb 23, 2024
53c0e7d
Fix format
ifosli Feb 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion Library/ViewModels/RewardCardViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Legitimate. I think consistency is a great reason to do it this way, then.

)
}
.map(Strings.About_reward_amount(reward_amount:))
Expand Down
15 changes: 15 additions & 0 deletions Library/ViewModels/RewardCardViewModelTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,21 @@ final class RewardCardViewModelTests: TestCase {
)
}
}

func testConversionLabel() {
let project = Project.template
|> Project.lens.country .~ .us
|> Project.lens.stats.currency .~ Project.Country.mx.currencyCode
let reward = Reward.noReward
|> Reward.lens.convertedMinimum .~ 0.6
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


withEnvironment(countryCode: "US") {
self.vm.inputs.configure(with: (project, reward, .pledge))

self.conversionLabelText.assertValues(["About $1"],
"No-reward min is rounded up.")
}
}

// MARK: - Included Items

Expand Down