Skip to content
This repository has been archived by the owner on Feb 2, 2022. It is now read-only.

Interval calculation #41

Open
robindirksen1 opened this issue Aug 30, 2019 · 6 comments
Open

Interval calculation #41

robindirksen1 opened this issue Aug 30, 2019 · 6 comments
Assignees
Labels
enhancement New feature or request

Comments

@robindirksen1
Copy link
Contributor

So for this example I have a plan configured with an interval of 1 month and made a subscription.

What I expect:

When I start a subscription for a user on 2019-08-30, I want to bill them on the 30 off every month.

Here I made a example (the billing dates are marked with a green box):
calendar-view-expected

What the packages does:

The package billing-day is changing when the month doesn't have (in this case) 30 or more days in a month.

Here I made a example of what the package is doing (startdate = 2018-08-30) (the billing dates are marked with a green box):
calendar-view-actual

Proposal

I think we should make this as an optional item in the plan configuration, that way the enduser can decide when to use this option in their plans or use the default carbon behaviour.

Also, I've discussed this issue with @vernondegoede about how they handle this in their Subscription API.

Used code

When starting on 2019-08-30

//source how laravel/cashier-mollie handles: https://github.com/laravel/cashier-mollie/blob/b1256489456a5f1e71e3af1de150df7d4a9fb08a/src/Subscription.php#L423

$run_timer = 12;
$tmp = $now = Carbon::parse('2019-08-30');

for($x = 1; $x <= $run_timer; $x++) {
    $tmp = $tmp->copy();

    $tmp->modify('+ 1 month');

    dump($tmp);
}

image

When starting on 2018-08-30 (one year before)

//source how laravel/cashier-mollie handles: https://github.com/laravel/cashier-mollie/blob/b1256489456a5f1e71e3af1de150df7d4a9fb08a/src/Subscription.php#L423

$run_timer = 12;
$tmp = $now = Carbon::parse('2018-08-30');

for($x = 1; $x <= $run_timer; $x++) {
    $tmp = $tmp->copy();

    $tmp->modify('+ 1 month');

    dump($tmp);
}

image

@robindirksen1
Copy link
Contributor Author

cc @driesvints @bobbybouwmann as discussed at LaraconEU

@bobbybouwmann
Copy link

@robindirksen1 I was going over this and I thought it makes sense that this is broken because februari can have only 28 days. It doesn't make sense that the other months don't work and that the calculations for the other months are broken, but that's a whole other story.

What happens when you always put it on the 28th when it's 29th, 30th or 31st? This should fix in general you problem and makes sense when accounting februari as well!

@robindirksen1
Copy link
Contributor Author

What happens when you always put it on the 28th when it's 29th, 30th or 31st? This should fix in general you problem and makes sense when accounting februari as well!

Yes that would work, but then nobody can have a subscription that runs from the last days (29,30,31) in a month. When you subscribe to something, let's says Laravel Forge, your subscription runs from the day you started and not a few days before or after that.

@robindirksen1
Copy link
Contributor Author

Another proposal I've:

Add a method that returns a collection Carbon instances so you can debug/show information about when the next billing day(s) for that specific subscription are.

@sandervanhooft sandervanhooft added the enhancement New feature or request label Sep 3, 2019
@sandervanhooft sandervanhooft self-assigned this Sep 3, 2019
@sandervanhooft
Copy link
Contributor

Thanks @robindirksen1

Some good bits in here, we can work in a swappable action class (optionally override this per subscription plan) once the v1 has released.

@sandervanhooft sandervanhooft changed the title Behaviour of Carbon Interval different from what expected Interval calculation Sep 3, 2019
@mmachatschek
Copy link
Contributor

I was looking into this a little more. Currently I don't see a good solution here because the new billing cycle is calculated with ->modify() (as stated above) which basically can have any string that can be handled by the DateTime::class e.g. ->modify('+2 day first sunday''). This is a bad example but could be set as interval on integrations.

My suggestion would be to allow a more specific interval but also allow the old version e.g.

// 'interval'=> '1 month' //still valid
'interval' => [
    'value' => 1,
    'period' => 'month', //day, month, year
   // 'fixed' => true,
],

This way we could call the corresponding ->addPeriodWithoutOverflow() method, and let Carbon handle the correct interval Date. So the next cycle would not result in a completely different month.

With the fixed entry you can force the next billing cycle to be on the same day or the correspondig last day in the next month.

I made a draft PR for this #194
What do you guys think?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants