WIP: Remove TaxRate.match#536
Closed
Conversation
Closed
Contributor
|
I poked @mamhoff in slack and the first thing we're going to try with this PR (which we all know is very scary) is try and pull out the general "spec improvement" commits from any serious change here. The PR itself is so massive anything we can do to reduce noise would go a long way. |
Merged
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).
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
A scope that yields all tax rates that are valid for a particular zone. This is intended to replace `Spree::TaxRate.match` in its entirety. I've also refactored the specs for better readability and reduced scope of the method.
Contributor
Author
|
Rebased on current master in order to remove some noise. Still not yet ready for prime time as I'm waiting on comments regarding #685. |
This looks like a very large commit, but it actually mostly removes lines. The goal is to make somewhat more understandable what's currently going on in the Solidus codebase: What rates apply to my order's zone? Those which have shared members with my zone. When is there a refund? Whenever the store is sending stuff to a place outside the default zone that does not have included taxes. The refactoring now implies that a zone have members to be applicable. In the real world, that is always the case. There is an expectation for a negative included tax in the reimbursement tax calculator. That expectation is IMO bogus, I have no idea why it should be there.
Contributor
Author
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
2 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 builds upon #530, and will remove two methods from TaxRate:
TaxRate.matchas well asTaxRate#potentially_applicable. I'm pushing already to get CI builds.For now, I have not added any new functionality to Solidus' taxation system, it's more a Housekeeping thing to build on: Less terse code that is hard to reason about.
The other big upside of this PR is that the tests for VAT taxation where partly expecting the wrong things. I added new tests and ported those from the old tests that weren't covered, eventually allowing me to delete part of the old tests (which were ALSO very hard to understand).
Ready for review. (Maybe do #530 first, and then I rebase, though - less lines).