Improve taxation specs #650
Conversation
| # 1) The rate's zone is the default zone, then it's always applicable. | ||
| (self.included_in_price? && self.zone.default_tax) | ||
| # 1) The rate's zone is in the default zone, then it's always applicable. | ||
| (self.included_in_price? && self.zone.contains?(Spree::Zone.default_tax)) |
There was a problem hiding this comment.
was the logic here wrong before?
There was a problem hiding this comment.
Yes. If your default zone is Germany, and you have a tax rate defined for Europe, and then you try to ship outside Europe, you want to be aware that there's a European tax rate in your price.
I deliberately chose a test setup with hairy cases like this, and reverting this to the original breaks seven specs: https://gist.github.com/mamhoff/512dae674e2437133eec
There was a problem hiding this comment.
The comment says:
The rate's zone is in the default zone
But the code is
self.zone.contains?(Spree::Zone.default_tax))
Which reads to me as the opposite, "the default zone is contained by the rate's zone"
There was a problem hiding this comment.
Yes, correct, the comment is not correct in this case.
There is this idea of a smallest possible zone (Spree::Zone.match(address)) produces just that, the smallest fitting zone for an address. Apparently, a default zone should also be like that (contain no other zones).
I'll fix the comment.
There was a problem hiding this comment.
Thanks Martin. I've cloned this down, and am trying to wrap my head around the tax system to give a proper review. I really appreciate format of the new specs here.
There was a problem hiding this comment.
There's some fun bits in the production code. If you have questions, shoot them at me, I'm reachable on slack.
Consider I ask a zone whether it contains itself - that would be true, right? `Zone#contains` is only ever called from `tax_rate.rb`, and every time it gets called there is (or should be) a safeguard for this. Plus: It removes a `create` call from the zone specs.
There's a bit of confusion around when a tax rate is applicable in the code. Sometimes the order tax zone has to be equal the tax rates zone, and sometimes it's ok if the tax rates' zone contains the order tax zone. Using the shortcut from the previous commit, this can be unified.
|
I've given the specs a more thorough read and am very 👍 |
Right now, this only includes the super-standard case. They pass.
|
Rebased on #657 in order to separate concerns. |
MOSS requires prices to change. In order to model the required behaviour, I've added an example European country, and all the things that should happen when you ship stuff there: For non-digital products, your German VAT should apply, while for digital products, the Romanian VAT should apply. Never ever should the pre tax amount change. The current code does not do this at all, so the tests are set to "pending". Some other tests fail intermittently (!!!), so I `xit`ed them.
Adds specs for the refund that applies once you ship outside a VAT jurisdiction. This refund is necessary to arrive at the correct line item totals as long as we are not able to calculate correct line item prices. Also adds support for the situation that the default tax zone is contained by a rates tax zone, and you're shipping within that. This support is particularly ugly, as it comes with a very expensive call to `Spree::Zone.contains?`.
These mimick the original specs in terms of test setup, but have the expectations aligned to the VAT specs.
Much of what they tested I had covered before, and for those case where I hadn't I moved the tests over. The main aim of the whole exercise is to have a comprehensive set of tests, and no false positives.
|
Fixups for fixed |
|
Yea, this is good by me too. Can starting to see this go in an actual reasonable direction now for the first since the beginning of the codebase, thanks a lot @mamhoff. 👍 |
This extracts the spec improvements (and only those) from #536.
What I do is I create a sort-of real-world spec set-up with understandable country and zone names and attach creative tax rates to them, then expect the order/line items to be correctly adjusted.
A number of specs are set to pending because the current tax implementation can't do them. A number of other specs are xited because they produce random failures.
There is one fix included for some failures that occur with the MOSS cases: If the zone your customer is ordering from is inside the default zone, but not the default zone, the default zone's taxes should still apply.
The global diff is, unfortunately, horrific - I basically replace an entire section of specs. Git sees that a little differently. If you go through the commits one by one, you'll have a much easier time reviewing.