Match tax zones by address instead of by zone #862
Closed
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
This was referenced Feb 13, 2016
|
Rebased on current master to remove merge conflicts. All commits have been passing before. |
| end | ||
| end | ||
|
|
||
|
|
|
Rebased on current master, and added a refactoring of Spree::ShippingRate, allowing multiple tax rates to be displayed along a shipping rate without having it go through the item adjuster. |
In order to not have to manually reset configuration values between specs only if they are of the cached instance variable type, add one line to the `reset_spree_preferences` method that resets all instance variables between specs. Bonus: Specs for a spec helper method!
With the parent commit in place, we don't need these after hooks.
The current `default_tax_zone` system is flawed: It conflates tax and shipping zones (Spree::Order#tax_zone can return a shipping zone if that happens to be smaller than the smalles matching tax zone), and it creates a large number of complex queries that won't be necessary once we start finding tax rates by the order's tax address. This commit introduces a default tax address preference, defaulting to `nil`. In case you need VAT taxation in your default country, set the preference to Spree::Country.build_default (it just needs to respond to `:country_id` and `:state_id` for the current taxation functionality).
This scope finds all relevant tax rates for an address in one SQL query.
I'm using the new TaxRate.for_address scope to find tax rates for the configured default address. I had to adjust a lot of specs, in which the default_tax flag on zones would be abused to just apply to anything (which, I think is the source of some of the confusion regarding that flag). Some spec setups have now gotten a bit longer, but there's an upside: They've also become more understandable. For the time being, I've left the `default_tax` boolean in the factories and have not endeavoured to remove all the code that possibly could be. Return the configured default tax address if the order's tax address is nil Instead of first running `Spree::Zone.match`, which is an expensive method to call, we can just return the configured default address (which might well be `nil` for our American friends) as the fallback on `Spree::Order.tax_address`.
We now have a default address; `Spree::Zone.default_tax` should not be called from the order model anymore. This made changing the shipping rate specs necessary.
This really just fixes some descriptions and some spec setup.
This simplifies some code and also solves bugs where shipping rates tax zones would not be the same but contained within the rates zone. Unfortunately, it also unearthes a rounding bug with the current taxation system.
With the default tax address in place, there is no need for the Boolean to even be there anymore. In case anyone needs the default tax zone, it can now be obtained by running the a newly deprecated method: Spree::Zone.match.
…ates Refactor Spree::ShippingRate to a) Use all tax rates that match its order address and its tax category b) Use i18n properly to display the shipping rates Having the tax helpers as a module here helped tremendously.
This greens a lot of tests and opens up refactoring Spree::TaxRate.store_pre_tax_amount.
With the common API for shipments in place, this method can be simplified a lot. We also get rid of an unnecessary `.any?` check here.
When rounding the pre tax amount for VAT calculations, we run into rounding errors on creating the adjustment. Example: We want the tax amount for $16.00 at 19% VAT 16.00 / 1.19 = 13.4453 ~ 13.45 --> 2.55 VAT 13.45 * 1.19 = 16.0055 ~ 16.01 --> 2.56 VAT (wrong wrong wrong) Not rounding the pre tax amount fixes this rounding error. As usual with rounding, there will still be cases where this does not work, depending on how many digits our database stores. They will be much rarer, though.
There was a calculation mistake on both of these specs. This commit corrects that.
This scope has not been around for long at all. It's inefficient. The documentation goes as well, because it's partly wrong and confusing.
This method is intended to DRY away the question whether a rate is applicable to a particular item. Right now, we use tax category identity, but that might be something else in the future.
This class is intended to behave like an address when passed into Spree::Zone.for_address, but not have any of the complex behaviour of Spree::Address.
Use the new Spree::TaxLocation class with the country defined by Spree::Config.default_vat_country_iso to specify the area for which taxes are calculated when an order is in `cart` state. This relieves us from the following issues: - We don't have to assume the database to be present on app boot time - We can tighten the contract for the tax adjusters, because now we know that the tax address will always be there, but could be a Spree::TaxLocation.
With Spree::TaxLocation.empty? we can now early return for the US case, in which we have no state id or country id set in the default tax location.
| end | ||
|
|
||
| context 'with a state object' do | ||
| let(:args) { {state: state} } |
There was a problem hiding this comment.
Space inside { missing.
Space inside } missing.
| end | ||
|
|
||
| context 'with a country object' do | ||
| let(:args) { {country: country} } |
There was a problem hiding this comment.
Space inside { missing.
Space inside } missing.
|
Rebased again, and added an object to fill in for an address in the configuration. I disagree with Hound's comments in this particular case. Is this configurable? @jhawthorn, @cbrunsdon This is now ready for review, and includes everything I can do before actual schema changes for taxation I think. Sorry it's large, but commit by commit should be understandable. I'm of course available for questions. |
|
This is a now stale, and will be superseded by smaller PRs. |
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 PR introduces a new preference,
default_tax_addressto replace the functionality of thedefault_taxboolean onSpree::Zone.The reasoning behind is that it is much easier and more efficient to find all zones which include an address than it is to find zones that entirely fall within another zone (have a look at
Spree::Zone#contains?if you want to know more). A good analogy is: It's easier to find out if one dot is in a dot cloud than to find out if all dots in a dot cloud fall within another dot cloud.Since the
default_taxboolean is being used in a number of places for "easing" spec setups, you'll see a decent number of spec setups being changed. They look longer now, but are also more obvious (Now you know why some tax rate applies to an order!).Because I replace all occurrences of
Spree::TaxRate.for_zone,Spree::Zone.matchandSpree::Zone.contains?with calls toSpree::TaxRate.for_address, which is an ActiveRecord scope that allows AR to actually cache queries, this should be a little faster than the previous code.One thing I'm not sure about is using a Spree::Address in the
initializers/spree.rbin the future. This might mean you need database access on boot-up, which is not always present. I'm entirely open to suggestions.This PR is pretty large, and best reviewed commit by commit. I've tried to keep the commits as small as humanly possible, but e835e66, the core commit of this PR, couldn't be reduced.