From d724ad5b4d532012dd37cc8718e25b182160a43d Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 6 Jan 2016 19:11:12 +0100 Subject: [PATCH 1/3] Zone#contains?: Add shortcut for identical zones Consider I ask a zone whether it contains itself - that would be true, right? `Zone#contains` is only ever called from `tax_rate.rb`, and every time it gets called there is (or should be) a safeguard for this. Plus: It removes a `create` call from the zone specs. --- core/app/models/spree/zone.rb | 1 + core/spec/models/spree/zone_spec.rb | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/core/app/models/spree/zone.rb b/core/app/models/spree/zone.rb index 36251871ddb..68b51dd6456 100644 --- a/core/app/models/spree/zone.rb +++ b/core/app/models/spree/zone.rb @@ -111,6 +111,7 @@ def state_ids=(ids) # Indicates whether the specified zone falls entirely within the zone performing # the check. def contains?(target) + return true if self == target return false if kind == 'state' && target.kind == 'country' return false if zone_members.empty? || target.zone_members.empty? diff --git a/core/spec/models/spree/zone_spec.rb b/core/spec/models/spree/zone_spec.rb index 19f383684b0..16dcbc4502d 100644 --- a/core/spec/models/spree/zone_spec.rb +++ b/core/spec/models/spree/zone_spec.rb @@ -155,7 +155,6 @@ context "when both zones are the same zone" do before do - @source.members.create(zoneable: country1) @target = @source end From 9fcc2d1b8837a356d24faba5743cd54390393ba9 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 6 Jan 2016 19:26:49 +0100 Subject: [PATCH 2/3] Always use Zone#contains? for checking tax rate applicability There's a bit of confusion around when a tax rate is applicable in the code. Sometimes the order tax zone has to be equal the tax rates zone, and sometimes it's ok if the tax rates' zone contains the order tax zone. Using the shortcut from the previous commit, this can be unified. --- core/app/models/spree/tax_rate.rb | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/core/app/models/spree/tax_rate.rb b/core/app/models/spree/tax_rate.rb index 12da7a38490..39edb1fccd3 100644 --- a/core/app/models/spree/tax_rate.rb +++ b/core/app/models/spree/tax_rate.rb @@ -145,12 +145,10 @@ def self.adjust(order_tax_zone, items) # # Those rates should never come into play at all and only the French rates should apply. def potentially_applicable?(order_tax_zone) - # If the rate's zone matches the order's tax zone, then it's applicable. - self.zone == order_tax_zone || # If the rate's zone *contains* the order's tax zone, then it's applicable. self.zone.contains?(order_tax_zone) || - # 1) The rate's zone is the default zone, then it's always applicable. - (self.included_in_price? && self.zone.default_tax) + # The rate is a VAT and its zone contains the default zone, then it's applicable. + (self.included_in_price? && self.zone.contains?(Spree::Zone.default_tax)) end # Creates necessary tax adjustments for the order. @@ -188,8 +186,7 @@ def compute_amount(item) end def default_zone_or_zone_match?(order_tax_zone) - default_tax = Zone.default_tax - (default_tax && default_tax.contains?(order_tax_zone)) || order_tax_zone == self.zone + Zone.default_tax.try!(:contains?, order_tax_zone) || self.zone.contains?(order_tax_zone) end private From 298f9a96249c528cb43ff459060cce0c95aaa953 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Thu, 7 Jan 2016 16:34:49 -0800 Subject: [PATCH 3/3] Add spec to test zone.contains?(zone) --- core/spec/models/spree/zone_spec.rb | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/core/spec/models/spree/zone_spec.rb b/core/spec/models/spree/zone_spec.rb index 16dcbc4502d..e6f7d43ce88 100644 --- a/core/spec/models/spree/zone_spec.rb +++ b/core/spec/models/spree/zone_spec.rb @@ -137,6 +137,16 @@ @target = create(:zone, name: 'target', zone_members: []) end + it "should contain itself" do + expect(@source.contains?(@source)).to be true + end + + context "when both source and target have no members" do + it "should be false" do + expect(@source.contains?(@target)).to be false + end + end + context "when the target has no members" do before { @source.members.create(zoneable: country1) }