Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure Spree::Country.available and Zone#country_list consistently return AR relations #5503

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions core/app/models/spree/country.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,11 @@ def self.default
def self.available(restrict_to_zone: Spree::Config[:checkout_zone])
checkout_zone = Zone.find_by(name: restrict_to_zone)

return checkout_zone.country_list if checkout_zone.try(:kind) == 'country'

all
if checkout_zone.try(:kind) == 'country'
checkout_zone.country_list
else
all
end
end

def <=>(other)
Expand Down
10 changes: 5 additions & 5 deletions core/app/models/spree/zone.rb
Original file line number Diff line number Diff line change
Expand Up @@ -86,11 +86,11 @@ def include?(address)

# convenience method for returning the countries contained within a zone
def country_list
@countries ||= case kind
when 'country' then zoneables
when 'state' then zoneables.collect(&:country)
else []
end.flatten.compact.uniq
case kind
when 'country' then countries
when 'state' then Spree::Country.where(id: states.select(:id))
else Spree::Country.none
end
end

def <=>(other)
Expand Down
4 changes: 4 additions & 0 deletions core/spec/models/spree/country_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,24 +43,28 @@

context 'with no arguments' do
it 'returns "Checkout Zone" countries' do
expect(described_class.available).to be_an(ActiveRecord::Relation)
expect(described_class.available).to contain_exactly(united_states, canada)
end
end

context 'setting nil as restricting zone' do
it 'returns all countries' do
expect(described_class.available(restrict_to_zone: nil)).to be_an(ActiveRecord::Relation)
expect(described_class.available(restrict_to_zone: nil)).to contain_exactly(united_states, canada, italy)
end
end

context 'setting "Custom Zone" as restricting zone' do
it 'returns "Custom Zone" countries' do
expect(described_class.available(restrict_to_zone: 'Custom Zone')).to be_an(ActiveRecord::Relation)
expect(described_class.available(restrict_to_zone: 'Custom Zone')).to contain_exactly(united_states, italy)
end
end

context 'setting "Checkout Zone" as restricting zone' do
it 'returns "Checkout Zone" countries' do
expect(described_class.available(restrict_to_zone: 'Checkout Zone')).to be_an(ActiveRecord::Relation)
expect(described_class.available(restrict_to_zone: 'Checkout Zone')).to contain_exactly(united_states, canada)
end
end
Expand Down
11 changes: 11 additions & 0 deletions core/spec/models/spree/zone_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
before { country_zone.members.create(zoneable: country) }

it 'should return a list of countries' do
expect(country_zone.country_list).to be_a(ActiveRecord::Relation)
expect(country_zone.country_list).to eq([country])
end
end
Expand All @@ -67,9 +68,19 @@
before { state_zone.members.create(zoneable: state) }

it 'should return a list of countries' do
expect(state_zone.country_list).to be_a(ActiveRecord::Relation)
expect(state_zone.country_list).to eq([state.country])
end
end

context "when zone doesn't consist of either countries nor states" do
let(:zone) { create(:zone, name: 'Zone') }

it 'should return an empty list' do
expect(zone.country_list).to be_a(ActiveRecord::Relation)
expect(zone.country_list).to be_empty
end
end
end

context "#include?" do
Expand Down