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.

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

Merged
merged 5 commits into from
Dec 5, 2023

Conversation

aboedo
Copy link
Member

@aboedo aboedo commented Nov 29, 2023

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.

@aboedo aboedo self-assigned this Nov 29, 2023
@aboedo aboedo added the pr:fix A bug fix label Nov 29, 2023
@aboedo aboedo marked this pull request as ready for review November 29, 2023 20:50
@aboedo
Copy link
Member Author

aboedo commented Nov 29, 2023

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?

@aboedo aboedo requested a review from a team November 29, 2023 20:50
private val MAX_OFFSET = Offset.offset(0.0000001)
private val MAX_OFFSET = Offset.offset(0.0001)
Copy link
Member Author

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

Copy link
Contributor

@vegaro vegaro left a 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
Copy link
Contributor

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?

Copy link
Member Author

@aboedo aboedo Nov 29, 2023

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

Copy link
Contributor

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.

@tonidero
Copy link
Contributor

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?

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

Copy link

codecov bot commented Nov 30, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f5d74b0) 84.46% compared to head (8323803) 84.46%.

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.
📢 Have feedback on the report? Share it here.

@tonidero tonidero merged commit 27c08e2 into main Dec 5, 2023
@tonidero tonidero deleted the andy/fix_price_per_month branch December 5, 2023 13:04
tonidero added a commit that referenced this pull request Dec 5, 2023
### Description
Followup to #1504 

This slightly improves precision when calculating price per year for
weekly subscriptions.
This was referenced Dec 5, 2023
vegaro pushed a commit that referenced this pull request Dec 5, 2023
**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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:fix A bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants