Closed
Conversation
Contributor
There was a problem hiding this comment.
unnecessary : ) The tests below would fail anyhow.
The BigDecimal one is also superfluous imo
For the time being, these two POROs (one for orders, another for items) call
the already defined methods on `Spree::TaxRate`.
The idea is, though, that the responsibilities between `Spree::Tax::{Item,Order}Adjuster` and
`Spree::TaxRate` are divided as follows: `Spree::TaxRate` stores and selects rates,
while `Spree::Tax::{Item,Order}Adjuster` create tax adjustments.
The code in `Spree::TaxRate.adjust!` is hard to understand, and the adjusting should happen in dedicated adjuster classes. This commit ports the functionality of `TaxRate.match` over to two classes: * An `OrderAdjuster` which iterates over all items (line items and shipments) in an order and runs the * `ItemAdjuster`, which actually creates adjustments for every item. The code should read a lot easier now. In order to reduce database load, the tax rate array for an order can be passed into the `ItemAdjuster.new`. Considering the performance benefits, I think that's justifiable amount of shared knowledge between the two Adjuster classes.
TaxRate.for_zone will return all tax rates that are valid for a given zone. It does not care about the default VAT zone, as that should be handled by the Adjuster classes.
When using .with_shared_members, sometimes you pass nil - and that should just yield an empty ActiveRecord relation. Same goes for Spree::TaxRate.for_zone(nil).
This is really an intermediary step. I'm using for_zone already in Spree::TaxRate.match, which should result in a constant eight DB calls per .match call. Previously, the number of DB calls would depend on how many tax rates there are, and how these are configured (as Spree::Zone.contains? would be run for every tax rate in the system). A side effect of this refactoring is that the reimbursement specs had to be amended a little: The zones had to be created `:with_country`, because otherwise there'd be no shared members and the tax rates wouldn't apply. This actually makes the tests slightly more realistic (what is a zone without members, anyway?). Also, I had to change an expectation: The included tax total of the reimbursement was previously expected to be negative (which is something that IMO should never happen, as negative included taxes at the moment become positive additional taxes).
The order adjuster and the item adjuster share code, because I need to run some methods on the order level and cache them. This extracts these methods into a module. Inheritance would've also worked, but in this case I find a mixin more fitting as the behaviour of the two classes is still quite different.
TaxRate.match does some pretty weird things, and I want to change them. This commit moves that code to a temporary `applicable_rates` helper.
Before this commit, the taxation system would handle default VATs and rates that *actually* apply to the order's item with the same code. However, they do fundamentally different things: For example, the `pre_tax_amount` should only be calculated from default VAT rates. This commit introduces a new private method on the ItemAdjuster to handle "deductable" VAT rates. This fixes some of the taxation specs, because now the pre tax amount is calculated correctly, and it opens the path for a cleverer handling of default VATs (namely, changing the line item's price). Additionally, this commit introduces cached options for the entire order to be passed into the item adjuster.
The "initial price" is the price in the default tax zone. It will be used when calculating net amounts and new prices for foreign VAT zones. This has been discussed in #532
This is the simplest thing that could possibly work. It does.
Since now there can be no VAT refunds anymore, we can get rid of all the code in tax_rate.rb that exclusively deals with refunds.
Which tax rates are applicable is for the moment determined by the tax category of the item. There is cases though where more things apply (the customer, for example, might be a business customer). Besides, putting this into a method on TaxRate unifies that check.
With the new VAT taxation system, a default zone is no longer mandatory. It is perfectly valid to have taxes that are included in price only for specific foreign zones.
Contributor
Author
|
Rebased on #685, and removed the work on shipping rates present in RfC Make shipping rates taxable like line items or shipments (on which this will have to be rebased as well). |
Contributor
Author
|
After chat on slack with @jhawthorn reverted the instance variable names to names without an underscore (as they're actually used by the classes that use the |
Member
|
That's reasonable and I'm fine with it. Although Great work. Thanks, @mamhoff |
Contributor
Author
|
I'm closing this. Much of it is already merged into master, and for all the following work, #989 is the guide. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the PR that will actually bring about behaviour changes. Notably, taxable items (line items, shipping rates, and shipments) will have their price adjusted to the correct price including any applicable VAT. There shall be no refunds.
This is still work in progress. It depends on #685 (which it includes).