WIP remove Spree::Zone.default_tax and associated code paths #1537
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
mamhoff
force-pushed
the
remove-default-tax-zone
branch
from
08fa704 to
105cf76
Compare
mamhoff
force-pushed
the
remove-default-tax-zone
branch
from
105cf76 to
e858943
Compare
Previously, Spree and Solidus had code paths for tax reimbursements: If an orders tax address was outside the "default tax zone" and value added tax applied from that default tax zone, Solidus would create negative tax adjustments, i.e. Reimbursements. Since Solidus 1.3, the taxation system has been overhauled and we do not need this hack anymore.
mamhoff
force-pushed
the
remove-default-tax-zone
branch
from
e858943 to
b27de36
Compare
Spree::Zone.match suffers of several shortcomings: 1. It's badly named. What's matching? 2. It returns arbitrary results (why does smaller match more?) 3. It's expensive database-wise. 4. It's only used in now deprecated `Spree::Order.tax_zone`. This commit deprecates it, too.
This method checks whether a zone is wholly contained in another zone. Since zones should really be used as auxiliary constructs for finding out which shipping methods or tax rates apply to a particular address, comparing zones to zones is unnecessary.
mamhoff
force-pushed
the
remove-default-tax-zone
branch
from
33b4cec to
7ca8081
Compare
mamhoff
changed the title
mamhoff
changed the title
tvdeyen
reviewed
Nov 7, 2016
| In versions prior to Solidus 1.3, instead of changing prices for | ||
| different VAT territories, there was an assumption that backend | ||
| prices would include VAT, and the tax system was tasked with "refunding" that. This has since been changed so we could now delete a lot of code paths that performed these "refunds". | ||
|
|
There was a problem hiding this comment.
I think it should be enough the link to the 1.3 blog post.
| @@ -227,7 +227,7 @@ def backordered? | |||
| # Returns the relevant zone (if any) to be used for taxation purposes. | |||
| # Uses default tax zone unless there is a specific match | |||
There was a problem hiding this comment.
Please remove this outdated comment
| @@ -42,13 +41,17 @@ class Zone < Spree::Base | |||
| self.whitelisted_ransackable_attributes = ['description'] | |||
|
|
|||
| def self.default_tax | |||
| where(default_tax: true).first | |||
| Spree::Deprecation.warn("There is no default tax zone anymore.", caller) | |||
| all.detect { |zone| zone.try(:default_tax) } | |||
There was a problem hiding this comment.
If the column is gone, there is no need for this anymore, right? Maybe we should keep the column for at least one minor version instead?
| if Spree::Zone.where(default_tax: true).any? | ||
| raise DefaultTaxZonePresent, "Please run the the solidus:migrations:create_vat_prices rake task." | ||
| end | ||
| remove_column :spree_zones, :default_tax, :boolean, default: false |
There was a problem hiding this comment.
Maybe we keep the column for another round?
| # Returns the most specific matching zone for an address. Specific means: | ||
| # A State zone wins over a country zone, and a zone with few members wins | ||
| # over one with many members. If there is no match, returns nil. | ||
| def self.match(address) | ||
| Spree::Deprecation.warn("Spree::Zone.match is deprecated. Please use Spree::Zone.for_adress instead.", caller) |
There was a problem hiding this comment.
small typo. should be Spree::Zone.for_address
|
public API does not change right? This cleans up so nicely :) I really like it. That being said, I think these kind of changes should be introduced very defensively. |
Merged
|
Closing in favour of #1586 . |
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.
Previously, Spree and Solidus had code paths for tax reimbursements:
If an orders tax address was outside the "default tax zone" and value added tax
applied from that default tax zone, Solidus would create negative tax
adjustments, i.e. Reimbursements. Since Solidus 1.3, the taxation system
has been overhauled and we do not need this hack anymore. Taking it out is
roughly this commit. It can (and should) be broken up into smaller commits,
and we need to come up with a good deprecation plan for many of these methods.