Move the strange refunding logic from Spree::TaxRate into the default tax calculator#980
Move the strange refunding logic from Spree::TaxRate into the default tax calculator#980gmacdougall merged 3 commits intomasterfrom unknown repository
Conversation
mamhoff
changed the title
|
I'd like to see the unit tests for default_tax.rb improved, covering the new functionality and providing better tests for the existing logic. We also need to make the default_tax handle currencies that do not have two decimal places, but I'm fine with leaving that for another PR. |
|
Currencies with more than two decimal places are not supported at all in Solidus, I think that's an entirely different issue. And yeah, I'll provide unit tests for default tax. |
|
I was thinking more currencies without decimal places like JPY or KRW which are generally supported, aside from some bad rounding logic here and there. |
|
Ha. Yeah, I hear you. |
There was a problem hiding this comment.
Indent the first parameter one step more than the start of the previous line.
|
Refactored the specs and added specs for some of the refunding logic @gmacdougall |
|
The failing spec on 6990e65 is due to a PhantomJS crash, I'm quite sure. |
There was a problem hiding this comment.
I don't like stubbing methods on the file that we're writing the spec for. I don't think that this test provides value.
When reviewing #904, we realized that we can not change the logic of Spree::TaxRate#compute_amount if we want the migration included there to always to the right thing. Because that method modifies the tax amount of things, it should actually be in a calculator. I now moved that logic to the default tax calculator, with the intention of deprecating use of that calculator entirely. There is code for pre-Spree-2.2 order-based taxation in there, as well as code for generating tax refunds, which in the future we won't need either.
This improves the default tax calculator spec in the following ways: - Remove the number of `let!` calls so that the specs do not leak into each other and do not create unnecessary objects. - Reorder specs so that they make some more sense.
Previously, there was no specs for when to refund VATs. This adds a spec for turning a VAT into a discount when the order's tax zone has nothing in common with the default or rates VAT zone. Furthermore, this breaks out the specs for how to tax a taxable item into a `shared_examples_for` block and tests it with line items, shipments and shipping rates.
|
Rebased and added specs for the actual behaviour of the calculator when called with line item, shipment or shipping rate. I think this is good to go now. @gmacdougall @jhawthorn |
|
👍 Looks good to me. |
|
@gmacdougall If you have a minute, this should be good to go. |
|
Please please review this, it's base work for #904, which is also ripe. |
|
👍 Looks good to me |
|
👍 |
5 participants
When reviewing #904, we realized that we can not change the logic of
Spree::TaxRate#compute_amount if we want the migration included there to
always to the right thing. Because that method modifies the tax amount of things,
it should actually be in a calculator. I now moved that logic to the default tax
calculator, with the intention of deprecating use of that calculator entirely.
There is code for pre-Spree-2.2 order-based taxation in there, as well as code for
generating tax refunds, which in the future we won't need either.