From cf848323919f0e2b498cc06ae15ccb4eeed7987a Mon Sep 17 00:00:00 2001 From: Ryan Woods Date: Tue, 15 Feb 2022 12:14:57 +0000 Subject: [PATCH 1/4] Fix Country factory states_required attribute Resolves issue: https://github.com/solidusio/solidus/issues/4270 Some tests were modified to give a state with the address parameters as they used a country which will now get the states_required attribute of true, meaning a state must be present on the address. --- api/spec/requests/spree/api/orders_spec.rb | 5 +++-- api/spec/requests/spree/api/users_spec.rb | 12 ++++++++---- .../testing_support/factories/country_factory.rb | 3 +-- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/api/spec/requests/spree/api/orders_spec.rb b/api/spec/requests/spree/api/orders_spec.rb index ce7d5190d16..e1671123c48 100644 --- a/api/spec/requests/spree/api/orders_spec.rb +++ b/api/spec/requests/spree/api/orders_spec.rb @@ -523,14 +523,15 @@ module Spree::Api let(:billing_address) { { name: "Tiago Motta", address1: "Av Paulista", city: "Sao Paulo", zipcode: "01310-300", phone: "12345678", - country_id: country.id } + country_id: country.id, state_id: state.id } } let(:shipping_address) { { name: "Tiago Motta", address1: "Av Paulista", city: "Sao Paulo", zipcode: "01310-300", phone: "12345678", - country_id: country.id } + country_id: country.id, state_id: state.id } } let(:country) { create(:country, { name: "Brazil", iso_name: "BRAZIL", iso: "BR", iso3: "BRA", numcode: 76 }) } + let(:state) { create(:state, country_iso: 'BR') } before { allow_any_instance_of(Spree::Order).to receive_messages user: current_api_user } diff --git a/api/spec/requests/spree/api/users_spec.rb b/api/spec/requests/spree/api/users_spec.rb index 8908695fab1..4fa5eb4022f 100644 --- a/api/spec/requests/spree/api/users_spec.rb +++ b/api/spec/requests/spree/api/users_spec.rb @@ -44,6 +44,8 @@ module Spree::Api it "can update own details" do country = create(:country) + state = create(:state) + put spree.api_user_path(user.id), params: { token: user.spree_api_key, user: { email: "mine@example.com", bill_address_attributes: { @@ -51,7 +53,7 @@ module Spree::Api address1: '1 Test Rd', city: 'City', country_id: country.id, - state_id: 1, + state_id: state.id, zipcode: '55555', phone: '5555555555' }, @@ -60,7 +62,7 @@ module Spree::Api address1: '1 Test Rd', city: 'City', country_id: country.id, - state_id: 1, + state_id: state.id, zipcode: '55555', phone: '5555555555' } @@ -72,6 +74,8 @@ module Spree::Api it "can update own details in JSON with unwrapped parameters (Rails default)" do country = create(:country) + state = create(:state) + put spree.api_user_path(user.id), headers: { "CONTENT_TYPE": "application/json" }, params: { @@ -82,7 +86,7 @@ module Spree::Api address1: '1 Test Rd', city: 'City', country_id: country.id, - state_id: 1, + state_id: state.id, zipcode: '55555', phone: '5555555555' }, @@ -91,7 +95,7 @@ module Spree::Api address1: '1 Test Rd', city: 'City', country_id: country.id, - state_id: 1, + state_id: state.id, zipcode: '55555', phone: '5555555555' } diff --git a/core/lib/spree/testing_support/factories/country_factory.rb b/core/lib/spree/testing_support/factories/country_factory.rb index fef80feae87..bfb4e12c72c 100644 --- a/core/lib/spree/testing_support/factories/country_factory.rb +++ b/core/lib/spree/testing_support/factories/country_factory.rb @@ -20,7 +20,6 @@ iso3 { carmen_country.alpha_3_code } numcode { carmen_country.numeric_code } - # FIXME: We should set states required, but it causes failing tests - # states_required { carmen_country.subregions? } + states_required { carmen_country.subregions? } end end From 31dd14f4a503405e3f5de0c6f32c6bbc8eb031b2 Mon Sep 17 00:00:00 2001 From: Ryan Woods Date: Wed, 6 Jul 2022 12:22:11 +0200 Subject: [PATCH 2/4] Wrap address_factory_spec in a context Improves the readability of the next commit. --- .../factories/address_factory_spec.rb | 24 ++++++++++--------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/core/spec/lib/spree/core/testing_support/factories/address_factory_spec.rb b/core/spec/lib/spree/core/testing_support/factories/address_factory_spec.rb index 18d27274439..3899bf07ce6 100644 --- a/core/spec/lib/spree/core/testing_support/factories/address_factory_spec.rb +++ b/core/spec/lib/spree/core/testing_support/factories/address_factory_spec.rb @@ -31,17 +31,19 @@ end end - describe 'when passing in a state and country' do - subject { build(:address, country_iso_code: country_iso_code, state_code: state_code) } - - context 'when the country has a state with proper code' do - let(:country_iso_code) { "US" } - let(:state_code) { "NY" } - - it 'works' do - expect(subject).to be_valid - expect(subject.state.abbr).to eq("NY") - expect(subject.country.iso).to eq("US") + context 'states and countries' do + describe 'when passing in a state and country' do + subject { build(:address, country_iso_code: country_iso_code, state_code: state_code) } + + context 'when the country has a state with proper code' do + let(:country_iso_code) { "US" } + let(:state_code) { "NY" } + + it 'works' do + expect(subject).to be_valid + expect(subject.state.abbr).to eq("NY") + expect(subject.country.iso).to eq("US") + end end end end From c3dd6222600289aa3f7849a14d520bb8e2717a79 Mon Sep 17 00:00:00 2001 From: Ryan Woods Date: Wed, 6 Jul 2022 16:03:56 +0200 Subject: [PATCH 3/4] Use nested Carmen regions if necessary for state factory For some countries such as Italy, its states are nested within regions, and not at the first level of subregions like 'US'. `country > region > states` `Carmen::Country.coded('IT').subregions.first.type` Now nested subregions exist, it will use this instead. This will be useful for when using the state factory, but also necessary for the upcoming commit, where if a state is not given, but a country is, for the address factory, it can automatically use/create the first state of that country. --- .../factories/state_factory.rb | 10 ++++++++-- .../factories/state_factory_spec.rb | 20 +++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/core/lib/spree/testing_support/factories/state_factory.rb b/core/lib/spree/testing_support/factories/state_factory.rb index 9fe2e36f76d..12fa0c4c1cf 100644 --- a/core/lib/spree/testing_support/factories/state_factory.rb +++ b/core/lib/spree/testing_support/factories/state_factory.rb @@ -17,9 +17,15 @@ carmen_subregion do carmen_country = Carmen::Country.coded(country.iso) - carmen_country.subregions.coded(state_code) || - carmen_country.subregions.sort_by(&:name).first || + unless carmen_country.subregions? fail("Country #{country.iso} has no subregions") + end + + carmen_regions = carmen_country.subregions + carmen_regions = carmen_regions.flat_map(&:subregions) if carmen_regions.first.subregions? + region_collection = Carmen::RegionCollection.new(carmen_regions) + + region_collection.coded(state_code) || region_collection.sort_by(&:name).first end end diff --git a/core/spec/lib/spree/core/testing_support/factories/state_factory_spec.rb b/core/spec/lib/spree/core/testing_support/factories/state_factory_spec.rb index b1fa4e5a43c..2378fb27f5e 100644 --- a/core/spec/lib/spree/core/testing_support/factories/state_factory_spec.rb +++ b/core/spec/lib/spree/core/testing_support/factories/state_factory_spec.rb @@ -33,6 +33,26 @@ end end + describe 'for a country with nested carmen states' do + context 'when not given a state_iso' do + let(:state) { build(:state, country_iso: "IT") } + + it 'creates the first state for that country it finds in carmen' do + expect(state.abbr).to eq("AL") + expect(state.name).to eq("Alessandria") + end + end + + context 'when given a state_iso' do + let(:state) { build(:state, country_iso: "IT", state_code: 'PE' ) } + + it 'finds the corresponding state' do + expect(state.abbr).to eq("PE") + expect(state.name).to eq("Pescara") + end + end + end + describe 'when given a country record' do let(:country) { build(:country, iso: "DE") } let(:state) { build(:state, country: country) } From bd8ad623b0aa6f807be84a9ffa88c005f700eaf7 Mon Sep 17 00:00:00 2001 From: Ryan Woods Date: Wed, 6 Jul 2022 16:04:10 +0200 Subject: [PATCH 4/4] Automatically create a state if necessary for address factory Two rationales for this change: 1. Now that the states_required attribute is now functioning in the country factory, some specs for codebases might start breaking, due to them using an address factory, not giving a state, but using a country that requires a state. This should prevent many breaking tests * 2. Less setup is now needed for specs. Only a state or country is needed for a valid non-US (default) address * But not all, for example address API endpoint specs might break, which happened for Solidus: cf848323919f0e2b498cc06ae15ccb4eeed7987a --- .../factories/address_factory.rb | 20 ++++---- .../factories/address_factory_spec.rb | 48 ++++++++++++++++++- 2 files changed, 56 insertions(+), 12 deletions(-) diff --git a/core/lib/spree/testing_support/factories/address_factory.rb b/core/lib/spree/testing_support/factories/address_factory.rb index 86567470cd0..44533605f50 100644 --- a/core/lib/spree/testing_support/factories/address_factory.rb +++ b/core/lib/spree/testing_support/factories/address_factory.rb @@ -24,21 +24,19 @@ phone { '555-555-0199' } alternative_phone { '555-555-0199' } - state do |address| - Spree::State.joins(:country).where('spree_countries.iso = (?)', country_iso_code).find_by(abbr: state_code) || - address.association( - :state, - strategy: :create, - country_iso: country_iso_code, - state_code: state_code - ) - end - country do |address| if address.state address.state.country else - address.association(:country, strategy: :create, iso: country_iso_code) + Spree::Country.find_by(iso: country_iso_code) || + address.association(:country, strategy: :create, iso: country_iso_code) + end + end + + after(:build) do |address, evaluator| + if address&.country&.states_required? && address.state.nil? && address.state_name.nil? + address.state = address.country.states.find_by(abbr: evaluator.state_code) || + create(:state, country_iso: address.country.iso, state_code: evaluator.state_code) end end end diff --git a/core/spec/lib/spree/core/testing_support/factories/address_factory_spec.rb b/core/spec/lib/spree/core/testing_support/factories/address_factory_spec.rb index 3899bf07ce6..f5a0d93a429 100644 --- a/core/spec/lib/spree/core/testing_support/factories/address_factory_spec.rb +++ b/core/spec/lib/spree/core/testing_support/factories/address_factory_spec.rb @@ -32,7 +32,53 @@ end context 'states and countries' do - describe 'when passing in a state and country' do + describe 'country requiring a state' do + let(:state) { create(:state, country_iso: 'IT', state_code: 'PE' )} + let(:country) { create(:country, iso: 'IT' )} + + context 'when given a state but no country' do + subject { build(:address, state: state) } + + it 'infers the country from the state' do + expect(subject).to be_valid + expect(subject.state.abbr).to eq("PE") + expect(subject.country.iso).to eq("IT") + end + end + + context 'when given a country but no state' do + subject { build(:address, country: country) } + + it 'automatically finds or creates an appropriate state' do + expect(subject).to be_valid + expect(subject.state.abbr).to eq("AL") + expect(subject.country.iso).to eq("IT") + end + end + + context 'when given a country, no state but a state_name' do + subject { build(:address, country: country, state_name: 'Bogus state') } + + it 'does not automatically find or create an appropriate state' do + expect(subject).to be_valid + expect(subject.state).to be_nil + expect(subject.state_name).to eq('Bogus state') + end + end + end + + describe 'country not requiring a state' do + subject { build(:address, country: country) } + let(:country) { create(:country, iso: 'AI' )} + + it 'does not automatically find or create an appropriate state' do + expect(subject).to be_valid + expect(subject.state).to be_nil + expect(subject.country.iso).to eq("AI") + end + end + + describe 'when passing in a state and country ISO' do subject { build(:address, country_iso_code: country_iso_code, state_code: state_code) } context 'when the country has a state with proper code' do