-
Notifications
You must be signed in to change notification settings - Fork 682
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
[12.x] Fix proration behaviour being forced when syncing tax rates #1028
Conversation
This allows for synchronising tax rates for subscriptions and subscription items without automatically prorating them.
Forgive me but I'm a bit confused here. Isn't the problem that the proration behavior wasn't set properly when the subscription was created? Afaik syncing tax rates with existing subscriptions doesn't alter's the subscription's proration behavior. |
@driesvints Hopefully I'm understanding your comment correctly... From what I can see, proration behaviour doesn't "persist". Whether a subscription was created with prorating or not, any future updates to it will always prorate, unless explicitly disabled with each subsequent API request. I might be wrong, but here's why I think this is the case...
Basically, this PR just lets you move subscriptions from one Tax Rate to another, without creating pairs of unwanted proration adjustments. I imagine you would usually want to prorate tax changes (hence it's still the default), but there's two examples I can see where you would not want proration:
If I'm misunderstanding, let me know! But hopefully that explains it a bit better 😄 |
@jackwh I've send a message to my Stripe contacts about this. In the API docs there's no mention of proration being altered when a subscription's default tax rates are updated: https://stripe.com/docs/api/subscriptions/update#update_subscription-proration_behavior
|
@jackwh any chance you can share the request ID for that change? I've not been able to reproduce this proration with a few different configurations. |
@cjavilla-stripe I just replicated this with test customer Cashier made two requests, the first was
Let me know if there's anything else I can provide. Thanks! |
Marked this as a draft until Stripe has gotten back to us. |
@jackwh I finally have more info here from Stripe. It turns out that you're indeed very right and that proration does apply when migrating from Now I have one final question I want to check off with you and Stripe before going ahead with this PR: shouldn't we just apply |
@driesvints Thanks for the update! One example where you would want to prorate would be when a tax rate changes, whilst subscription charges are being accrued on a daily basis. Here in the UK, on January 4th 2011 the rate of VAT changed from 17.5% to 20%. It doesn't change often, but should this happen again you would want the ability to prorate. As you can't update the percentages of an existing Tax Rate object (only archive it), you would need to create a new Tax Rate (e.g. for VAT now at 20%), and assign that new rate to subscriptions by syncing it. Charges incurred up to the day the new tax came in to effect would still be charged at the old rate of 17.5%, and costs incurred from that day forward would be at the new rate. The next invoice would show these charges prorated correctly. |
@jackwh FYI: I'm waiting for a reply from @cjavilla-stripe on this before we continue. |
I've talked to @cjavilla-stripe and we both agree with your points. I've marked this PR as ready now. Thanks a lot for your patience with this! We've decided to keep the default behavior as in your PR with @taylorotwell this one can be merged now. You can ignore the tests which are failing because this is a fork. |
I recently updated my Cashier implementation to migrate customers from the
tax_percent
property to Stripe's Tax Rates.I tried using the
syncTaxRates()
function to update existing customer subscriptions, but found this would always prorate by default. The screenshot below from Stripe's dashboard shows what was happening to existing customers with existing subscriptions when$user->subscription('default')->syncTaxRates();
was being called. A pair of "Remaining time" invoice items were being added unexpectedly:This change now allows users to call
$user->subscription('default')->noProrate()->syncTaxRates();
, should they wish, to prevent prorating when syncing tax rates.This shouldn't break existing code, as the default behaviour is to still prorate. This just allows proration behaviour to be overridden when needed.