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

Proration Invoice after subscription swap do not include tax, if tax is used #384

Closed
tobiasvielmetter opened this issue Feb 21, 2017 · 23 comments
Labels

Comments

@tobiasvielmetter
Copy link

Turns out the proration invoice created by cashier does not include tax when a tax amount is defined.
This should be changed. Here is an example how to change the invoice() method in Billable.php:

/**
     * Invoice the billable entity outside of regular billing cycle.
     *
     * @return StripeInvoice|bool
     */
    public function invoice()
    {
        if ($this->stripe_id) {
            try {
                return StripeInvoice::create(['customer' => $this->stripe_id, 'tax_percent' => $this->taxPercentage()], $this->getStripeKey())->pay();
            } catch (StripeErrorInvalidRequest $e) {
                return false;
            }
        }

        return true;
    }
@Mr-Anonymous
Copy link

Is there a way to add this fix without editing the Billable.php source file directly? You are right with your report here, when the taxPercentage() is added in the user.php model and when swapping the plan using $user->subscription('main')->updateQuantity($quantity)->swap($newPlanId);, the Tax percentage is not added when the customer gets charged during the swap after the first invoice creation. However, the Tax is added in the following month's invoice though. I don't like editing the Billable.php file since the fix will be overlapped during updates. Is there another way to fix this issue? Can I override this method somewhere else? If yes, how?

Thanks for sharing this fix.

@tobiasvielmetter
Copy link
Author

@Mr-Anonymous Well, technically you don't have to do this in the Billable Trait itself, but you can just define your own swap method in the Model that is billable. In my instance I have a custom swap method in my Tenant model, because I exclusively bill tenants:

public function swapWLPlan($plan)
    {
        $subscription = $this->asStripeCustomer()->subscriptions->retrieve($this->subscription('main')->stripe_id);

        $subscription->plan = $plan;

        $subscription->prorate = true;

        if (! is_null($this->subscription('main')->billingCycleAnchor)) {
            $subscription->billingCycleAnchor = $this->subscription('main')->billingCycleAnchor;
        }

        // If no specific trial end date has been set, the default behavior should be
        // to maintain the current trial state, whether that is "active" or to run
        // the swap out with the exact number of days left on this current plan.
        if ($this->onTrial()) {
            $subscription->trial_end = $this->subscription('main')->trial_ends_at->getTimestamp();
        } else {
            $subscription->trial_end = 'now';
        }

        // Again, if no explicit quantity was set, the default behaviors should be to
        // maintain the current quantity onto the new plan. This is a sensible one
        // that should be the expected behavior for most developers with Stripe.
        if ($this->subscription('main')->quantity) {
            $subscription->quantity = $this->subscription('main')->quantity;
        }

        $subscription->tax_percent = $this->taxPercentage();

        $subscription->save();

        if ($this->stripe_id) {
            try {
                $proration_invoice = StripeInvoice::create(['customer' => $this->stripe_id, 'tax_percent' => $subscription->tax_percent], $this->getStripeKey())->pay();
            } catch (StripeErrorInvalidRequest $e) {
                $proration_invoice = false;
            }
        }

        $this->subscription('main')->fill([
            'stripe_plan' => $plan,
            'ends_at' => null,
        ])->save();

        return $this;
    }

Obviously this needs to be maintained when an update breaks with the previous Billable Trait. Thats why I opened this issue, so it could be fixed with the next version.

@Mr-Anonymous
Copy link

Mr-Anonymous commented Jul 6, 2017

Thank you so much @tobiasvielmetter Your reply HUGELY helped!! I added your code into my User Model and then call this custom swap method to change the plan and it worked perfectly. It now adds the taxPercentage() properly on the new invoice that is generated. This is so much better than editing the Billable source file directly. If your suggested fix gets added to the source in the future, it will be great. But until then, your answer with custom method works well.

I was stressed on this since I had to complete my project by this weekend. Your timely response helped me a great deal. I am truely grateful. Thank you so much for taking the time to help me out!!

Thanks,
Neel.

@tobiasvielmetter
Copy link
Author

@Mr-Anonymous Glad my answer was of help, good luck with your project!

@Mr-Anonymous
Copy link

Hi @tobiasvielmetter Sorry to get back on this again.

When I use this Swap method to change a Daily Plan to a Monthly plan, I end up getting the error:

InvalidRequest in ApiRequestor.php line 110:
Nothing to invoice for customer

However, it appears the Stripe has received the request, the upgrade was done successfully, the new invoice was generated and the invoice was paid. I can see these in the Stripe Log. However, my App API throws this errors and it fails to proceed with the remaining code which is to update the subscription table after that. The code this error shows is in these lines of the swap method:

if ($this->stripe_id) {
              try {
                  $proration_invoice = StripeInvoice::create(['customer' => $this->stripe_id, 'tax_percent' => $subscription->tax_percent], $this->getStripeKey())->pay();
              } catch (StripeErrorInvalidRequest $e) {
                  $proration_invoice = false;
              }
}

The customer id is valid, the invoice is being generated so I am not sure why this error shows up. Do you have an idea on what may cause this issue here please?

Here is my full Swap method that I am using:

      public function swapPlan($plan, $quantity) {
          $subscription = $this->asStripeCustomer()->subscriptions->retrieve($this->subscription('main')->stripe_id);
          $subscription->plan = $plan;
          $subscription->prorate = true;

          if (! is_null($this->subscription('main')->billingCycleAnchor)) {
              $subscription->billingCycleAnchor = $this->subscription('main')->billingCycleAnchor;
          }

          // If no specific trial end date has been set, the default behavior should be
          // to maintain the current trial state, whether that is "active" or to run
          // the swap out with the exact number of days left on this current plan.
          if ($this->onTrial()) {
              $subscription->trial_end = $this->subscription('main')->trial_ends_at->getTimestamp();
          } else {
              $subscription->trial_end = 'now';
          }

          // Again, if no explicit quantity was set, the default behaviors should be to
          // maintain the current quantity onto the new plan. This is a sensible one
          // that should be the expected behavior for most developers with Stripe.
          if (!empty($quantity)) {
              $subscription->quantity = $quantity;
          }
          elseif ($this->subscription('main')->quantity) {
              $subscription->quantity = $this->subscription('main')->quantity;
          }

          $subscription->tax_percent = $this->taxPercentage();

          $subscription->save();

          if ($this->stripe_id) {
              try {
                  $proration_invoice = StripeInvoice::create(['customer' => $this->stripe_id, 'tax_percent' => $subscription->tax_percent], $this->getStripeKey())->pay();
              } catch (StripeErrorInvalidRequest $e) {
                  $proration_invoice = false;
              }
          }

          if (!empty($quantity)) {
              $this->subscription('main')->fill([
                  'stripe_plan' => $plan,
                  'quantity'    => $quantity,
                  'ends_at'     => null
              ])->save();
          }
          else {
              $this->subscription('main')->fill([
                  'stripe_plan' => $plan,
                  'ends_at'     => null
              ])->save();
          }

          return $this;
      }

@tobiasvielmetter
Copy link
Author

@Mr-Anonymous Sorry, don't have time to debug this. I'd just properly debug the code, dd() out all the values, check if everything is set correctly etc. Good luck.

@Mr-Anonymous
Copy link

@tobiasvielmetter I understand. Thanks for the input. When I look at the log, I can see the customer number is right and all the params are right so I am not sure why stripe is throwing this error. While I am still debugging this issue, I do have a quick question. What will happen if I remove this $proration_invoice code block completely in my swap method?

if ($this->stripe_id) {
              try {
                  $proration_invoice = StripeInvoice::create(['customer' => $this->stripe_id, 'tax_percent' => $subscription->tax_percent], $this->getStripeKey())->pay();
              } catch (StripeErrorInvalidRequest $e) {
                  $proration_invoice = false;
              }
          }

Is my understand correct that if I remove the above block, the following happens:

  1. The Cashier API updates the plan change to Stripe when $subscription->save() is called.
  2. Stripe creates the adjustments such as pro-rata credit on old plan, pro-rata balance on new plan.
  3. If I don't call the pay() method, then these due amount will be included in the next invoice.

Is that correct? In other words, if I don't include the above code block, then customer will not be charged straight away after plan change but instead any due or credit amount will be added to the next month's invoice. Is that right? Can I then safely remove this block if that is the behaviour?

@tobiasvielmetter
Copy link
Author

@Mr-Anonymous Once again, no time for this, as giving a proper answer would require for me to dig into the code/docs to validate how it works exactly, which you can easily do yourself. Also this is getting off topic, this is not a help forum but a issue tracker for the class.
I'd recommend you to study the Subscription Class of Cashier were the swap method derives from. Also check the Stripe docs to understand how they handle proration and how this affects your workflow. Its possible that this can be handled entirely on the stripe side, but I am not sure about it. Check with them. Good luck!

@Mr-Anonymous
Copy link

Noted and I understand. Thank you though and I apologise if it went off topic.

@IvanBernatovic
Copy link

Just joining the conversation to confirm that this is problem with client's project too (which uses Spark) so it would be nice to have it fixed. Do maintainers/contributers need any help on this?

@tobiasvielmetter I only added your modified invoice() method to my user model (so it overrides original method from Billable trait) and it seems it's working correctly now. Thanks for help! If there is any issue caused by that let me know.

@apeisa
Copy link

apeisa commented Feb 15, 2018

Also having this issue (using spark). When someone makes changes to plan, then there is no mention of taxes on receipt.

@driesvints
Copy link
Member

driesvints commented Jun 15, 2018

Hey everyone 👋

Just wanted to share my own findings because I did quite some research on this. It's indeed important to update the tax_percent value on the subscription manually because otherwise Stripe will continue to invoice for that subscription with the old tax_percent value.

It is however not necessary to overwrite the invoice() method. If the tax_percent value on the subscription is correctly set, the invoice method will work as intended. It's also not necessary to create your own swap method or overwrite Cashier's method.

What you'll need to do is manually update the subscription tax_percent value like below:

        if ($billable->subscribed()) {
            $subscription = $billable->subscription()->asStripeSubscription();
            $subscription->tax_percent = $billable->taxPercentage();
            $subscription->save();
        }

This will apply and save the new tax_percent value on the current subscription. This should happen each time the taxPercentage value changes. If you hardcode it, you should run a command or something that will update the current subscriptions in Stripe.

The reason for this is that if you auto-bill your customers, Stripe will continue to use the old value until you've updated the value itself. It's therefor very important that you update the subscriptions as soon as the tax value changes.

If you take this further and apply a package like https://github.com/mpociot/vat-calculator then you should update the subscription value every time the customer's billing details change. This is the only way to make sure Stripe correctly applies the intended Tax.

@murph133
Copy link

murph133 commented Jul 3, 2018

If proration isn't very important to you, disable the proration feature. Yes, this is not a solution to the problem, rather avoidance of it, but for those who don't want to spend too much time resolving the issue, it's a quick solution.

For those users using Laravel Spark. This is achieved by adding Spark::noProrate(); to App\Providers\SparkServiceProvider

@emergingdzns
Copy link

I'll throw my hat in the ring on this one as well. We have some clients that are taxable for the subscriptions and some that are not. In my testing I discovered that using ->swap() is charging through Stripe but only the difference between the original plan ($299) and the new plan ($699). However there's an 8% tax on the original subscription (which is passed to stripe upon initial purchase). I can view the original invoice in Stripe as well showing that it has a line entry in the invoice totals showing "Tax (8.00%)". So clearly, simply having the tax attached to the original subscription does NOT force Stripe to apply tax to the new subscription. Would be good to have this fixed.

@driesvints
Copy link
Member

driesvints commented Oct 11, 2018

I've sent in a PR with a convenience method so you can easily sync the tax percentage of the billable model with the subscription. Please read my remarks on the PR and above.

What it comes down to: when the value of the taxPercentage method changes YOU need to update the tax on any existing subscriptions as well if you want that to happen. This can differ from use case so just adding it to the swap method isn't feasible in every situation.

With the new method you could for example call it right before the swap method:

$subscription->syncTaxPercentage();

$subscription->swap('new-plan');

Hope this helps. Feel free to provide any other feedback.

@emergingdzns
Copy link

Thanks @driesvints but the tax isn't changing. The initial subscription is set to 8%. The swap should also be 8%. It seems that the cashier module isn't passing in the tax_percent at all. There appears to be somewhat of a bug in the Stripe system and I'm working with them to fix this because according to their documentation you only need to pass in the tax_percent when the tax is changed (in which case your above PR and instruction would be applicable). I tried modifying it to pass the tax and it still didn't charge the tax, but the future invoice does in fact have tax applied. So clearly there's a problem with Stripe.

@driesvints
Copy link
Member

Hey @emergingdzns, thanks for letting us know. Seems like this is indeed a problem on Stripe's end. Definitely let us know if you or they find a solution.

@emergingdzns
Copy link

Ok working with Stripe we found the problem! It's actually an issue in Cashier. It's an odd issue, but in the Billable.php you have:

    public function invoice()
    {
        if ($this->stripe_id) {
            try {
                return StripeInvoice::create(['customer' => $this->stripe_id], $this->getStripeKey())->pay();
            } catch (StripeErrorInvalidRequest $e) {
                return false;
            }
        }

        return true;
    }

But, if you look at their documentation: https://stripe.com/docs/api#create_invoice it shows that subscription_id needs to be passed for it to tie the invoice to the subscription, which will cause them to apply the tax from the subscription. So that code should be:

    public function invoice($subscription)
    {
        if ($this->stripe_id) {
            try {
                return StripeInvoice::create(['customer' => $this->stripe_id,'subscription' => $subscription->id], $this->getStripeKey())->pay();
            } catch (StripeErrorInvalidRequest $e) {
                return false;
            }
        }

        return true;
    }

and in Subscription.php you need to change:

$this->user->invoice();

to:

$this->user->invoice($subscription);

Hopefully that makes sense. I don't have the ability at the moment to do a PR. Hopefully you can fix based on this info.

@driesvints
Copy link
Member

driesvints commented Oct 15, 2018

I see. Thanks for researching this. The proper way to solve this is to create an invoice method on the Subscription model I believe. And let the swap and incrementAndInvoice methods call those instead. This will be the cleanest solution.

I can send a PR for this later, otherwise feel free to send one in already.

@driesvints driesvints reopened this Oct 15, 2018
@driesvints driesvints added the bug label Oct 15, 2018
@jlmmns
Copy link

jlmmns commented Nov 2, 2018

@driesvints Just found out about this as well, when dealing with one-off charges / manual invoices.
This discussion seems to be about subscriptions, but when dealing with manual invoices, the tax_percent option should definitely be used in the Billable invoice method, and preferably configurable by passing an options array to the invoice method.

I guess the new invoice method on the Subscription model, would then call the invoice method on the Billable trait with the necessary options, like subscription as well as tax_percent.

@driesvints
Copy link
Member

@jlmmns there's a note about this in the docs:

The taxPercentage method only applies to subscription charges. If you use Cashier to make "one off" charges, you will need to manually specify the tax rate at that time.

But I see that there's no way to pass it with the invoiceFor method at the moment. I'll send in a different PR for this as well.

@driesvints
Copy link
Member

I've sent in a PR which should fix the issue from @emergingdzns and provide @jlmmns with a way to add the tax_percent to one off invoices: #598

@driesvints
Copy link
Member

Cashier 9.0 has been released which fixes these issues.

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

No branches or pull requests

8 participants