Refactor VAT taxation Part I: Fix existing functionality and tests #519
Conversation
This file contains 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
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.
This class method returns all those zones that have at least one member in common (even via a state that is part of a country), as well as itself.
Both Spree::Zone.match and Spree::Zone.with_shared_members employ almost the same SQL for doing what they need to do. Moved into a named scope to reduce understanding effort. Improve documentation, follow Rubocop recommendations
TaxRate.match does some really hard-to-understand calculations at the end, only so that TaxRate.adjust gets exactly the right array for other really complex code. This only affects VAT taxation, and the code in question is, unfortunately, somewhat questionable. The tests for TaxRate.adjust test the same behaviour. They're xit'ed for now, but will be added again once I'm done refactoring TaxRate.adjust.
Half this method is now covered by Spree::Zone.with_shared_members. The other half has to come from refactoring Spree::TaxRate.adjust.
Move the comments from TaxRate#potentially_applicable to TaxRate.adjust and comment every line in the refactored adjust method.
Especially the ones for prices that include tax (aka VAT) were rather hard to work with (very long context lines, lots of instance variables instead of `let`s etc.). Changed that and `xit`ed those that fail. They fail because of bugs in the VAT code. Before, they had the right descriptions and wrong expectations.
Also refactor carefully and add much more commentary. In order for refunds to work like they should, rather than always loading the default tax zone's rates, and randomly discarding one, this commit introduces a "Do we need to refund" check, and then adds the default zone's VAT rates.
This broke the reimbursement tests.
It does almost the same thing as TaxRate.for_zone, except for an early return when the Argument is nil. Since it's only called from TaxRate.adjust, at a point where we've already made sure that passed in zone is not nil, the method is unnecessary.
|
The initial attempt at directly manipulating prices in the line item did not work, as line items can get their price from all sorts of places. So right now I've been restricting myself to a more iterative approach: Simplify Because the initial attempt failed, I force-pushed. Lateron, I realized that was a shitty idea because all of your comments got lost :( - I am deeply sorry for that. Otherwise, this would be ready to merge, and for the next piece, I'd open up a new PR. |
mamhoff
changed the title
Closed
|
I'm breaking this up into smaller PR's to make sure all commits pass the CI individually. |
|
Closing in favour of #536 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 to #461. It will be large - I hope though smaller than the one done to Spree 3.1.