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

Unexpected behaviour with onTrial method in Billable #1128

Closed
benjamindoe opened this issue Apr 14, 2021 · 5 comments
Closed

Unexpected behaviour with onTrial method in Billable #1128

benjamindoe opened this issue Apr 14, 2021 · 5 comments

Comments

@benjamindoe
Copy link
Contributor

benjamindoe commented Apr 14, 2021

  • Cashier Version: 12.9.1
  • Laravel Version: 7.30.4
  • PHP Version: 7.4.16

Description:

Not so much a bug as unexpected behaviour from the onTrial method.

I have a snippet of code that does something similar to the following:

if ($user->onTrial()) {
    $trialEnd = $user->trialEndsAt()->format('d/m/y');
}

The behaviour I would expect is that the onTrial and trialEndsAt should both check and return the same date & time respectively. The issue in question arises when a user is on a generic trial and has a subscription that isn't on a trial.

onTrial first checks for generic trial if no arguments have been passed through. It then defaults to the subscription named default and checks that subscription for a trial.

trialEndsAt first checks the default subscription if there are no arguments passed through. If a trial doesn't exist there it falls back to generic trial.

The point I am trying to get across is that both onTrial and trialEndsAt should probably follow similar logic when it comes to checking for trials. Perhaps onTrial should call trialEndsAt?

@benjamindoe
Copy link
Contributor Author

Happy to suggest a PR

@thomasrd1
Copy link

Are you also using Laravel Spark?

@benjamindoe
Copy link
Contributor Author

No, custom build using Cashier directly

@driesvints
Copy link
Member

driesvints commented Apr 15, 2021

So I first implemented this function here: #1000 and it was later fixed here: #1015

But it indeed seems to not account for your use case. I agree this should be fixed. Although I wouldn't immediately label this as a bug. Depending on the implementation this should go to either the current stable version or master.

@benjamindoe
Copy link
Contributor Author

I'll send a PR to master then as it's, although nuanced, a breaking change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants