Calculate pre tax amounts on the fly #941
Conversation
| @@ -3,8 +3,11 @@ | |||
| require 'spec_helper' | |||
|
|
|||
| describe Spree::ShippingRate, type: :model do | |||
| let(:address) { create(:address) } | |||
| let(:order) { create(:order, ship_address: address) } | |||
| let(:tax_category) { create(:tax_category)} | |||
This takes away some caching and slows down some of the tax calculations in VAT countries, but it also speeds up tax calculation for Sales tax countries. It also makes sure that for line items and shipments, the following always holds true: ``` discounted_amount - included_tax_total = pre_tax_amount ``` The slowdown is significant, but will be remedied by two planned improvements in the very near future: * Looking up rates by address * Separating handling of default taxes vs. order taxes Yet again, spec setups hat to be adjusted.
This column is no longer needed, as we do the calculation of the pre tax amount on the fly whenever needed.
|
Despite the horrid performance implications, I really want this in. It rids |
|
Did you ran any benchmarks? Is this really so much slower? |
|
The taxation integration test suite, which is really exactly testing this lots and lots of times, goes from ~45s to ~53 seconds. Considering that much of that time is spent doing spec setup, yes, this must be a lot slower. The real reason why is that in the adjustments callback cycle, every adjustments amount get recalculated an insane number of times, which is why the caching (on the This is a huge priority, but I think before this was the wrong spot in which to do the caching. If it's really necessary (I believe the more sanity we add to this code, the faster it will run). |
|
👍 I think the simplification here is worth the temporary slowdown, especially since @mamhoff has already done so much of the work to speed things up in upcoming commits. I feel like this commit is helping detangle the code and helps me be able understand the existing code more clearly. |
|
We'd probably want some sort of changelog note about removing the columns yes? |
|
Yep, I'm getting on the 👍 train too, excellent work as always @mamhoff. Temporary slowdown for vat is a small price to pay for moving this forward. |
|
👍 |
1 similar comment
|
👍 |
Calculate pre tax amounts on the fly
This PR has a close relationship with solidusio#706, and solidusio#706 was still missing a changelog entry.
This takes away some caching and slows down some of the tax calculations
in VAT countries, but it also speeds up tax calculation for Sales tax countries.
It also makes sure that for line items and shipments, the following always
holds true:
The slowdown is significant, but will be remedied by two planned improvements
in the very near future:
I had to adjust the spec setup for the default tax calculator spec and realised the order (!) calculator code has a bug: It takes the item's
totalrather than itsdiscounted_amount, applying tax rates twice. Yay.