Refactor VAT handling (Work in Progress) #6227
Conversation
|
I've prepared a stage to preview changes. Open stage or view logs. |
| @@ -16,21 +16,25 @@ def compute(return_item) | |||
| private | |||
|
|
|||
| def weighted_order_adjustment_amount(inventory_unit) | |||
| inventory_unit.order.adjustments.eligible.non_tax.sum(:amount) * percentage_of_order_total(inventory_unit) | |||
| round_to_two_places(inventory_unit.order.adjustments.eligible.non_tax.sum(:amount) * percentage_of_order_total(inventory_unit)) | |||
|
I like that this approach would simplify the code base. I feel these changes would be more appropriate as a Spree 4 than 3.1 though as they are breaking. We've considered doing a 3.1 release with more minor enhancements such as v2 api w/AMS & store credits, but perhaps we should just bump to 4.0 instead once this is ready & skip 3.1. |
|
🎉 nice work @mamhoff 💯 |
|
@JDutil yes 4.0 would be better I think. Although we will provide a rake task for shops currently entering prices including tax. So, this shouldn't be breaking in reality. |
| let(:india) { create :country, name: "India" } | ||
| let(:france) { create :country, name: "France" } | ||
| let(:france_zone) { create :zone_with_country, name: "France Zone" } | ||
| let(:germany_zone) { create :zone_with_country, name: "Germany Zone", default_tax: true } |
|
Please make sure to update release notes with the relevant changes, and instructions on the rake task usage: |
| # These methods are for display only, not for calculation. | ||
| # They include VAT! | ||
| def discounted_money | ||
| Spree::Money.new(discounted_amount_including_vat, { currency: currency }) |
There was a problem hiding this comment.
Redundant curly braces around a hash parameter.
ShippingRate could be reduced in complexity a little as well, since there are no more shipping rates that include VAT. Besides, I corrected a few expectations (there was one in particular that was wrong).
Because the default zone does not have to be included in all tax calculations, there's no need to load it here. This should address #6234.
Since we need net prices to be stored with high scale (five digits min), and we can't ask our users to be so exact, we let them enter gross prices and convert before saving to a net price.
End-Users in VAT countries need to be shown prices including VAT by law. This accomplishes that. For countries that do not have a VAT, the prices are shown as-is.
| end | ||
| end | ||
| return 0.0 if inventory_unit.order.pre_tax_item_amount.zero? | ||
| weighted_line_item_pre_tax_amount(inventory_unit) / inventory_unit.order.pre_tax_item_amount |
|
The current state of things is: I think this works. Please try it out, and please provide cases where it doesn't. Also: I'm grateful for anyone reviewing this and commenting (don't be shy!). Still to do:
Notable changes: Increased precision on all price fields, simplified tax rate selection, added |
|
Please make sure to add updating release notes to your todo list with the relevant changes, and instructions on the rake task usage: |
|
Uhm, in my experience this will add much complexity to EU stores. Isn't the same in Germany? /cc @kennyadsl @mtylty |
|
@mamhoff saw your awesome explanation on #6154... pretty complex topic, thanks for that. My main question is: |
|
I agree this PR simplfy codebase and it seems the most logic way to handle prices and taxes (great work @mamhoff) but as @alepore we never needed this kind of scenario for our clients. I agree with him that it could create issues for the majority of them while adding product prices. Just thinking loud: what about letting (via some preference) choose which kind of price they enter (vat included or not) and remove vat (if included) before write it on the database? I know it could make things much more complex but it's the only idea I have to support both scenarios without breaking the new proposed convention. |
|
@alepore @kennyadsl we need net prices for precise calculation. I agree that re-/entering prices in net is unpractical for our customers. That's why we automatically convert VAT prices into net prices after save. So shop owners don't need to enter net prices. We plan to add some JavaScript magic for automatically updating labels on price fields if tax category changes. Yes, in Germany (and AFAIK in all countries having VAT) prices must not be displayed without tax. They always have to be displayed including tax (That's the only difference between sales tax and VAT btw). Therefore we added new So, we tried many different scenarios in how we could achieve this complex issue (spent two two week sprints!) and finally came up with this solution. The advantage is, that existing stores having sales tax (IMHO the majority of spree users) don't need to do anything after upgrading. For existing VAT stores we will provide rake tasks and a complete documentation that will help with upgrading. Thanks for your feedback. Please keep on reviewing and discussing this. Happy easter weekend! |
|
@alepore @kennyadsl I guess you never needed to care about net prices, because you don't sell digital goods together with physical goods, right? And the laws changed with 01.01.2015 and the glorious (recognized the irony?) MOSS taxation rules. You now have to display the prices for digital goods including the VAT of the customers country. For physical goods it's the same as before: display the prices including the VAT of the stores country. Spree does some heavy lifting in calculating the net price for VAT stores behind the scenes for you. But, it only calculates the net price for the default tax rate. Besides lots of code for calculating the net price on the fly, that does not work for stores having physical and digital goods. Also spree makes a lot of rounding errors while calculating the net price on the fly and does not do this for all prices (I.e. Shipping costs). So, this looks like a big change for existing VAT stores (and it actually is), but is less complexity in calculating and therefore less rounding issues and more readable code. |
|
What @tvdeyen said, generally :) I'd like to add a little bit of detail to both of your questions: @alepore We actually modify the price setter to automagically convert the price to a net price if the default zone has VATs before saving to the database, see https://github.com/magiclabs/spree/blob/refactor-tax/core/app/models/spree/product.rb#L80 and https://github.com/magiclabs/spree/blob/refactor-tax/core/app/models/spree/product.rb#L223-L237. If you get gross prices, you should be able to create the products with the gross price and still end up with the net price in the DB. @kennyadsl I think this is exactly the kind of solution you're suggesting. There's no preference for it though, because we actually need net prices to be exactly calculated (to six digits precision). @alepore Your workflow will have to change slightly with this PR if you're in Europe. You'll want to create a default zone and TaxRate that has @alepore If VATs change (which they do really seldomly, as that is politically a little loaded...), and your shop does not adjust its gross prices, they are changing their prices. I think it's a good thing for merchants to be aware of that kind of change as well. If you want to keep prices as before - just change them back to the previous price afterwards. |
|
@alepore If you don't create a default tax rate, Spree will assume the prices you enter as net prices. If you add a tax rate after entering product prices, your prices will be inflated by your VAT (because at the time of entering prices, Spree has no knowledge of how much VAT to deduct). If your VAT rules are anything similar to the German ones though, I strongly recommend setting VAT rates correctly at the start, because only then will you be able to display all the legally required "VAT included" messages... |
|
Ok, that's clearly an hack. |
|
We are planning on changing the label on the price input field to say "net price" or "gross price" depending on whether the product's tax category has a default VAT. We can't add a warning when you enter a price when there isn't a default VAT, because that's the standard use case for Sales tax countries. I can imagine a warning when you change the default zone's VAT rate, and re-introducing the "you have to choose a default zone" validation on adding the As for the "Hack" claim - if you have a better suggestion, I'd be absolutely thrilled to hear about it :) |
|
Oh yeah, that's actually not a hack for sales tax countries, got it 👍 |
|
Well, well. So it turned out that net prices are not the right way to go - if gross prices change (as is the case with MOSS (#6154, the issue that started it all), their net prices actually do change with them. I will close this PR and open a new one that has a solution that
It is PR #6293 . |
This PR should address Issue #6154.
This PR breaks existing stores
This PR is breaking. It assumes all prices to be entered without tax, which will make prices wrong in stores that have entered their prices including tax. If this has a chance of being accepted, we'd write a rake task that converts prices from gross to net prices for existing stores.
New meaning for
Zone.default_taxThe
default_taxzone has a slightly different role: It is only there for calculating included tax on orders that don't have a zone assigned yet. This also removes the need for loading the default zone when looking for applicable tax_rates.New meaning for
included_in_taxThe
included_in_priceflag can now be set even when there is no default zone. It is not used anymore to calculate net prices, but only for tagging VATs as VATs. At a later stage, this flag will be used for displaying prices including VAT not-logged in users.New convention: All prices in the database are net prices
Adopting the convention that all prices are entered and calculated upon excluding VAT simplifies VAT handling enormously. This is the reason a lot of code (especially in tax_rate.rb and the default tax calculator) could go away. I took care to adapt Spree's tests to the new situation.
Like I said, this is still work in progress. Variants are still not shown with the default price added, and there is stuff to do for shipping rates and promotions, too.
To have an idea what this is supposed to accomplish, I set up a separate spec in
core/spec/models/moss_taxes_spec.rb. These specs fail on every other Spree version.Please please please, if anyone has a chance to look over this code or even try it out, I'd be more than grateful for Feedback. Here's a reference to some of the people who have provided notable feedback or code on taxes in issues I've read: @JDutil @peterberkenbosch @vkvelho @jdurand @gmacdougall @GeekOnCoffee @cduv @radar @Hates @simmsy. Feel free to pull more people into the conversation.
Right now, you can follow my commit history to see what I did when. For the PR to be merged, I will rebase; and there will be force pushes to this PR at some point. You've been warned 💀.