From 29da7da0e3d2767ebd347da9b4e527c2f357ab7c Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Fri, 20 Nov 2015 19:44:33 +0100 Subject: [PATCH 1/3] Add Spree::Zone.with_shared_members(zone) 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). --- core/app/models/spree/zone.rb | 15 ++++++ core/spec/models/spree/zone_spec.rb | 77 +++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+) diff --git a/core/app/models/spree/zone.rb b/core/app/models/spree/zone.rb index 68b51dd6456..b5ccab9fa20 100644 --- a/core/app/models/spree/zone.rb +++ b/core/app/models/spree/zone.rb @@ -40,6 +40,21 @@ def self.match(address) matches.first end + # Returns all zones that also contain any of the zone members of the zone + # passed in. This also includes any country zones that contain any states of + # the current zone, if it's a state zone. If the zone passed in has members, + # those will also be returned. + def self.with_shared_members(zone) + joins(:zone_members) + .where("(spree_zone_members.zoneable_type = 'Spree::State' AND + spree_zone_members.zoneable_id IN (?)) + OR (spree_zone_members.zoneable_type = 'Spree::Country' AND + spree_zone_members.zoneable_id IN (?))", + zone.states.pluck(:id), + zone.states.pluck(:country_id) + zone.countries.pluck(:id) + ).uniq + end + def kind if members.any? && !members.any? { |member| member.try(:zoneable_type).nil? } members.last.zoneable_type.demodulize.underscore diff --git a/core/spec/models/spree/zone_spec.rb b/core/spec/models/spree/zone_spec.rb index e6f7d43ce88..1850122f1f7 100644 --- a/core/spec/models/spree/zone_spec.rb +++ b/core/spec/models/spree/zone_spec.rb @@ -336,4 +336,81 @@ end end end + + context ".with_shared_members" do + let!(:country) { create(:country) } + let!(:country2) { create(:country, name: 'OtherCountry') } + let!(:country3) { create(:country, name: 'TaxCountry') } + + subject(:zones_with_shared_members) { Spree::Zone.with_shared_members(zone) } + + context 'when passing a zone with no members' do + let!(:zone) { create :zone } + + it 'will return an empty set' do + expect(subject).to eq([]) + end + end + + context "finding potential matches for a country zone" do + let!(:zone) do + create(:zone).tap do |z| + z.members.create(zoneable: country) + z.members.create(zoneable: country2) + z.save! + end + end + + let!(:zone2) do + create(:zone).tap { |z| z.members.create(zoneable: country) && z.save! } + end + + let!(:zone3) do + create(:zone).tap { |z| z.members.create(zoneable: country3) && z.save! } + end + + it "will find all zones with countries covered by the passed in zone" do + expect(zones_with_shared_members).to include(zone, zone2) + end + + it "will not return zones with countries not covered in the passed in zone" do + expect(zones_with_shared_members).not_to include(zone3) + end + + it "only returns each zone once" do + expect(zones_with_shared_members.select { |z| z == zone }.size).to be 1 + end + end + + context "finding potential matches for a state zone" do + let!(:state) { create(:state, country: country) } + let!(:state2) { create(:state, country: country2, name: 'OtherState') } + let!(:state3) { create(:state, country: country2, name: 'State') } + let!(:zone) do + create(:zone).tap do |z| + z.members.create(zoneable: state) + z.members.create(zoneable: state2) + z.save! + end + end + let!(:zone2) do + create(:zone).tap { |z| z.members.create(zoneable: state) && z.save! } + end + let!(:zone3) do + create(:zone).tap { |z| z.members.create(zoneable: state2) && z.save! } + end + + it "will find all zones which share states covered by passed in zone" do + expect(zones_with_shared_members).to include(zone, zone2) + end + + it "will find zones that share countries with any states of the passed in zone" do + expect(zones_with_shared_members).to include(zone3) + end + + it "only returns each zone once" do + expect(zones_with_shared_members.select { |z| z == zone }.size).to be 1 + end + end + end end From 3402ee831b1348e013a2156bd1e9ae65f3836553 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Fri, 20 Nov 2015 20:10:08 +0100 Subject: [PATCH 2/3] Spree::Zone: Move SQL query into scope, linting, docs 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 --- core/app/models/spree/zone.rb | 36 ++++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/core/app/models/spree/zone.rb b/core/app/models/spree/zone.rb index b5ccab9fa20..dfd64d14324 100644 --- a/core/app/models/spree/zone.rb +++ b/core/app/models/spree/zone.rb @@ -15,6 +15,18 @@ class Zone < Spree::Base after_save :remove_defunct_members after_save :remove_previous_default + scope :with_member_ids, + ->(state_ids, country_ids) do + joins(:zone_members).where( + "(spree_zone_members.zoneable_type = 'Spree::State' AND + spree_zone_members.zoneable_id IN (?)) + OR (spree_zone_members.zoneable_type = 'Spree::Country' AND + spree_zone_members.zoneable_id IN (?))", + state_ids, + country_ids + ).uniq + end + alias :members :zone_members accepts_nested_attributes_for :zone_members, allow_destroy: true, reject_if: proc { |a| a['zoneable_id'].blank? } @@ -24,12 +36,13 @@ def self.default_tax where(default_tax: true).first end - # Returns the matching zone with the highest priority zone type (State, Country, Zone.) - # Returns nil in the case of no matches. + # 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) - return unless address and matches = self.includes(:zone_members). + return unless address and matches = self. + with_member_ids(address.state_id, address.country_id). order(:zone_members_count, :created_at, :id). - where("(spree_zone_members.zoneable_type = 'Spree::Country' AND spree_zone_members.zoneable_id = ?) OR (spree_zone_members.zoneable_type = 'Spree::State' AND spree_zone_members.zoneable_id = ?)", address.country_id, address.state_id). references(:zones) ['state', 'country'].each do |zone_kind| @@ -40,16 +53,13 @@ def self.match(address) matches.first end - # Returns all zones that also contain any of the zone members of the zone - # passed in. This also includes any country zones that contain any states of - # the current zone, if it's a state zone. If the zone passed in has members, - # those will also be returned. + + # Returns all zones that contain any of the zone members of the zone passed + # in. This also includes any country zones that contain the state of the + # current zone, if it's a state zone. If the passed-in zone has members, it + # will also be in the result set. def self.with_shared_members(zone) - joins(:zone_members) - .where("(spree_zone_members.zoneable_type = 'Spree::State' AND - spree_zone_members.zoneable_id IN (?)) - OR (spree_zone_members.zoneable_type = 'Spree::Country' AND - spree_zone_members.zoneable_id IN (?))", + with_member_ids( zone.states.pluck(:id), zone.states.pluck(:country_id) + zone.countries.pluck(:id) ).uniq From b77fb331dc121aa61b01928e76d2086728bd6a16 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 13 Jan 2016 14:13:25 -0600 Subject: [PATCH 3/3] Reduce number of queries in Zone.with_shared_members to three --- core/app/models/spree/zone.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/core/app/models/spree/zone.rb b/core/app/models/spree/zone.rb index dfd64d14324..671d0a22fcd 100644 --- a/core/app/models/spree/zone.rb +++ b/core/app/models/spree/zone.rb @@ -59,10 +59,12 @@ def self.match(address) # current zone, if it's a state zone. If the passed-in zone has members, it # will also be in the result set. def self.with_shared_members(zone) - with_member_ids( - zone.states.pluck(:id), - zone.states.pluck(:country_id) + zone.countries.pluck(:id) - ).uniq + states_and_state_country_ids = zone.states.pluck(:id, :country_id).to_a + state_ids = states_and_state_country_ids.map(&:first) + state_country_ids = states_and_state_country_ids.map(&:second) + country_ids = zone.countries.pluck(:id).to_a + + with_member_ids(state_ids, country_ids + state_country_ids).uniq end def kind