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

Fix monthStart #177

Merged
merged 5 commits into from
Apr 4, 2022
Merged

Fix monthStart #177

merged 5 commits into from
Apr 4, 2022

Conversation

ro-tex
Copy link
Contributor

@ro-tex ro-tex commented Mar 31, 2022

PULL REQUEST

Overview

There was a flaw of monthStart that was only manifesting at certain periods of the year with certain subscribedUntil times. For example when a user was subscribed until 31st of a given month, they would get bad results around the first of March due to February not having a 31st.

This PR removes that issue and adds table tests which ensure that we return the expected result.

Note: The main thing to review in this PR are the table tests and, more specifically, the dates in them. If you can come up with more test cases which explore currently unaddressed situations, please suggest them.

Example for Visual Changes

Checklist

Review and complete the checklist to ensure that the PR is complete before assigned to an approver.

  • All new methods or updated methods have clear docstrings
  • Testing added or updated for new methods
  • Verify if any changes impact the WebPortal Health Checks
  • Approriate documentation updated
  • Changelog file created

Issues Closed

@ro-tex ro-tex self-assigned this Mar 31, 2022
Copy link
Contributor

@kwypchlo kwypchlo left a comment

Choose a reason for hiding this comment

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

shouldn't the subscription period start at the day when you start paying for it and last for exact month ? or does it work like that and I am misunderstanding

@ro-tex
Copy link
Contributor Author

ro-tex commented Mar 31, 2022

shouldn't the subscription period start at the day when you start paying for it and last for exact month ? or does it work like that and I am misunderstanding

How long is a month? If I sign up on 2nd of February and pay for just one month, I expect that to expire on 1st of March and last 28 days.

The reason we have this function is because we need to know when to reset the statistics for the user for the current month. We are not using this in order to promote/demote users from paid tiers, just for calculating current month statistics. But since "current month" is not the same as "calendar month", we need to rest the stats on the day the user's sub refreshes.

@kwypchlo
Copy link
Contributor

kwypchlo commented Mar 31, 2022

How long is a month? If I sign up on 2nd of February and pay for just one month, I expect that to expire on 1st of March and last 28 days.

Well my thinking is that month starts the day I subscribe so let's say it's 28th March and since March has 31 days, the plan should expire in 31 days. If user pays for next period, next period starts in April and since April has 29 days it will last for 29 days.

@ro-tex
Copy link
Contributor Author

ro-tex commented Mar 31, 2022

How long is a month? If I sign up on 2nd of February and pay for just one month, I expect that to expire on 1st of March and last 28 days.

Well my thinking is that month starts the day I subscribe so let's say it's 28th March and since March has 31 days, the plan should expire in 31 days. If user pays for next period, next period starts in April and since April has 29 days it will last for 29 days.

The issues I see with this approach are:

  1. It's more complicated to calculate than the current approach.
  2. When it works "as expected", it works exactly like the current approach.
  3. When it doesn't work like the current approach, it's not what users would have expected.

Example:
So, if I sub to your service on 28th February, I will get 28 days of service and those will expire on 28th March? That makes sense to me.
But if I sub on 31st of January, I will get 31 days of service and those will expire on 3rd March. I wouldn't expect that and I don't think anyone would. This is not how any monthly subscriptions have ever worked for me.

My expectations:
If I sub on any of 28th/29th/30th/31st January for a month, I expect to be charged again on 28th February (non-leap year assumed). Then I expect to be charged again on the same day of March that I was charged on in January.

This was my way of thinking about it when I wrote this code. A month being a fluid concept, some adjustments were necessary. The definition "from day X to day X, shortened when the day is missing from the month" seemed the most common one.

@kwypchlo
Copy link
Contributor

How long is a month? If I sign up on 2nd of February and pay for just one month, I expect that to expire on 1st of March and last 28 days.

Well my thinking is that month starts the day I subscribe so let's say it's 28th March and since March has 31 days, the plan should expire in 31 days. If user pays for next period, next period starts in April and since April has 29 days it will last for 29 days.

The issues I see with this approach are:

  1. It's more complicated to calculate than the current approach.
  2. When it works "as expected", it works exactly like the current approach.
  3. When it doesn't work like the current approach, it's not what users would have expected.

Example: So, if I sub to your service on 28th February, I will get 28 days of service and those will expire on 28th March? That makes sense to me. But if I sub on 31st of January, I will get 31 days of service and those will expire on 3rd March. I wouldn't expect that and I don't think anyone would. This is not how any monthly subscriptions have ever worked for me.

My expectations: If I sub on any of 28th/29th/30th/31st January for a month, I expect to be charged again on 28th February (non-leap year assumed). Then I expect to be charged again on the same day of March that I was charged on in January.

This was my way of thinking about it when I wrote this code. A month being a fluid concept, some adjustments were necessary. The definition "from day X to day X, shortened when the day is missing from the month" seemed the most common one.

in all honesty, I never pay attention to the exact day my services expire, I am just confused here hence my question, if I subscribe in the middle of the month I pay just a specific fraction of the subscription fee and I have just a fraction of the quota till the end of that month and then I get billed on 1st of next month and my quota resets too and I get till the end of that calendar month?

@ro-tex ro-tex requested a review from kwypchlo March 31, 2022 14:40
@ro-tex ro-tex enabled auto-merge March 31, 2022 14:40
@kwypchlo kwypchlo requested a review from peterjan March 31, 2022 14:50
@kwypchlo kwypchlo removed their request for review March 31, 2022 16:27
@kwypchlo
Copy link
Contributor

ok after a discussion on discord the change seems good but I will defer to @peterjan and @ChrisSchinnerl to review the code

database/user.go Show resolved Hide resolved
@ro-tex ro-tex merged commit 3803ca7 into main Apr 4, 2022
@ro-tex ro-tex deleted the ivo/fix_monthStart branch April 4, 2022 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants