-
Notifications
You must be signed in to change notification settings - Fork 53
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.
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
Improve price per month accuracy for weekly subscriptions #1504
Conversation
looks like snapshots are failing but I can't see them, I imagine we had a value that was calculated off of the weekly in the snapshot? |
private val MAX_OFFSET = Offset.offset(0.0000001) | ||
private val MAX_OFFSET = Offset.offset(0.0001) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't love reducing accuracy, but having a value this precise would probably risk our tests failing from floating point precision errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! Just that little question
@@ -31,9 +31,9 @@ data class Period( | |||
private const val DAYS_PER_WEEK = 7.0 | |||
private const val DAYS_PER_MONTH = 30.0 | |||
private const val DAYS_PER_YEAR = 365.0 | |||
private const val WEEKS_PER_MONTH = 4.0 | |||
private const val WEEKS_PER_YEAR = 52.14 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also calculate this one so we have the same precision?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was shooting for "least amount of changes possible" but we could.
It'd be nice to update them all really to also take into account leap years nvmd not sure how that'd even work since this is a "per year" value not "specifically this year" value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will make that change separately.
@aboedo I generated snapshots here and noticed that the weekly price was actually more expensive than the others with this new calculation, which caused a changed in the discount percentages. I thought to merge the new snapshots, but for now I decided to slightly lower the price of the weekly product so it's not more expensive than the monthly one. I pushed that change in this branch. Lmk if you would prefer to update the snapshots and we can do that. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1504 +/- ##
=======================================
Coverage 84.46% 84.46%
=======================================
Files 217 217
Lines 7196 7196
Branches 1004 1004
=======================================
Hits 6078 6078
Misses 730 730
Partials 388 388 ☔ View full report in Codecov by Sentry. |
### Description Followup to #1504 This slightly improves precision when calculating price per year for weekly subscriptions.
**This is an automatic release.** ### RevenueCatUI * Paywalls: Add `PaywallFooterView` (#1509) via Toni Rico (@tonidero) * Paywalls: Remove `PaywallActivity` theme to pickup application's theme by default (#1511) via Toni Rico (@tonidero) * Paywalls: Auto-close paywall activity if restore grants required entitlement identifier (#1507) via Toni Rico (@tonidero) ### Bugfixes * Improve pricePerYear price calculation precision (#1515) via Toni Rico (@tonidero) * Improve price per month accuracy for weekly subscriptions (#1504) via Andy Boedo (@aboedo) ### Dependency Updates * Bump danger from 9.4.0 to 9.4.1 (#1512) via dependabot[bot] (@dependabot[bot]) ### Other Changes * Remove unnecessary appInBackground parameters (#1508) via Cesar de la Vega (@vegaro) * Create `PurchasesStateProvider` (#1502) via Cesar de la Vega (@vegaro) Co-authored-by: revenuecat-ops <ops@revenuecat.com>
We were calculating the price per month for weekly products as price * 4, but there are on average 4.345 weeks in a month, so this updates the value accordingly.