From 8814b86b1293eb9148415f99decfc34c5a78c11b Mon Sep 17 00:00:00 2001 From: Maurizio De Santis Date: Wed, 9 Jan 2019 15:13:15 +0100 Subject: [PATCH] Skip setting default state when not included in available countries Closes #1205. In the case that default country is not included within available countries, this commit leaves the states select empty, where previously was filled in with default country states which didn't match with any available country. The state select is later filled in by Select2, which knows which country option is selected. Server side we can't assume that the first country option is selected, because it doesn't play well with client side features, e.g. autocomplete. --- .../orders/customer_details_controller.rb | 9 +- .../spree/admin/shared/_address_form.html.erb | 13 +- .../customer_details_controller_spec.rb | 52 +++++++- .../features/admin/orders/new_order_spec.rb | 117 ++++++++++++++++++ core/app/helpers/spree/base_helper.rb | 8 +- core/app/models/spree/country.rb | 8 ++ core/spec/models/spree/country_spec.rb | 76 ++++++++++++ 7 files changed, 270 insertions(+), 13 deletions(-) diff --git a/backend/app/controllers/spree/admin/orders/customer_details_controller.rb b/backend/app/controllers/spree/admin/orders/customer_details_controller.rb index 78046aba2d1..37f74502cc0 100644 --- a/backend/app/controllers/spree/admin/orders/customer_details_controller.rb +++ b/backend/app/controllers/spree/admin/orders/customer_details_controller.rb @@ -13,7 +13,8 @@ def show end def edit - country_id = Spree::Country.default.id + country_id = default_country_id + @order.build_bill_address(country_id: country_id) if @order.bill_address.nil? @order.build_ship_address(country_id: country_id) if @order.ship_address.nil? @@ -43,6 +44,12 @@ def update private + def default_country_id + country_id = Spree::Country.default.id + + country_id if Spree::Country.available.pluck(:id).include?(country_id) + end + def order_params params.require(:order).permit( :email, diff --git a/backend/app/views/spree/admin/shared/_address_form.html.erb b/backend/app/views/spree/admin/shared/_address_form.html.erb index 4dc5e060a35..cb1e1dd9293 100644 --- a/backend/app/views/spree/admin/shared/_address_form.html.erb +++ b/backend/app/views/spree/admin/shared/_address_form.html.erb @@ -49,11 +49,18 @@ <%= f.label :state_id, Spree::State.model_name.human %> <%= f.hidden_field :state_name, value: nil %> + <% states = f.object.country.try(:states).nil? ? [] : f.object.country.states %> <%= f.text_field :state_name, - style: "display: #{f.object.country.states.empty? ? 'block' : 'none' };", - disabled: !f.object.country.states.empty?, class: 'fullwidth state_name js-state_name' %> + style: "display: #{states.empty? ? 'block' : 'none' };", + disabled: !states.empty?, class: 'fullwidth state_name js-state_name' %> <%= f.hidden_field :state_id, value: nil %> - <%= f.collection_select :state_id, f.object.country.states.sort, :id, :name, {include_blank: true}, {class: 'custom-select fullwidth js-state_id', style: "display: #{f.object.country.states.empty? ? 'none' : 'block' };", disabled: f.object.country.states.empty?} %> + <%= f.collection_select :state_id, + states.sort, + :id, :name, + { include_blank: true }, + { class: 'custom-select fullwidth js-state_id', + style: "display: #{states.empty? ? 'none' : 'block' };", + disabled: states.empty? } %> diff --git a/backend/spec/controllers/spree/admin/orders/customer_details_controller_spec.rb b/backend/spec/controllers/spree/admin/orders/customer_details_controller_spec.rb index 642bc1d6081..83e1fbed2ee 100644 --- a/backend/spec/controllers/spree/admin/orders/customer_details_controller_spec.rb +++ b/backend/spec/controllers/spree/admin/orders/customer_details_controller_spec.rb @@ -7,11 +7,59 @@ context "with authorization" do stub_authorization! - let(:order) { create(:order, number: "R123456789") } + context '#edit' do + context 'when order has no shipping nor billing address' do + let(:order) { create(:order, number: "R123456789", ship_address: nil, bill_address: nil) } - before { allow(Spree::Order).to receive_message_chain(:includes, :find_by!) { order } } + context "with a checkout_zone set as the country of Canada" do + let!(:united_states) { create(:country, iso: 'US', states_required: true) } + let!(:canada) { create(:country, iso: 'CA', states_required: true) } + let!(:checkout_zone) { create(:zone, name: "Checkout Zone", countries: [canada]) } + + before do + Spree::Config.checkout_zone = checkout_zone.name + end + + context "and default_country_iso of the Canada" do + before do + Spree::Config.default_country_iso = Spree::Country.find_by!(iso: "CA").iso + end + + it 'defaults the shipping address country to Canada' do + get :edit, params: { order_id: order.number } + expect(assigns(:order).shipping_address.country_id).to eq canada.id + end + + it 'defaults the billing address country to Canada' do + get :edit, params: { order_id: order.number } + expect(assigns(:order).billing_address.country_id).to eq canada.id + end + end + + context "and default_country_iso of the United States" do + before do + Spree::Config.default_country_iso = Spree::Country.find_by!(iso: "US").iso + end + + it 'defaults the shipping address country to nil' do + get :edit, params: { order_id: order.number } + expect(assigns(:order).shipping_address.country_id).to be_nil + end + + it 'defaults the billing address country to nil' do + get :edit, params: { order_id: order.number } + expect(assigns(:order).billing_address.country_id).to be_nil + end + end + end + end + end context "#update" do + let(:order) { create(:order, number: "R123456789") } + + before { allow(Spree::Order).to receive_message_chain(:includes, :find_by!) { order } } + it "updates + progresses the order" do expect(order).to receive(:update_attributes) { true } expect(order).to receive(:next) { false } diff --git a/backend/spec/features/admin/orders/new_order_spec.rb b/backend/spec/features/admin/orders/new_order_spec.rb index 80096c46881..a885897cad2 100644 --- a/backend/spec/features/admin/orders/new_order_spec.rb +++ b/backend/spec/features/admin/orders/new_order_spec.rb @@ -201,6 +201,123 @@ end end + context 'with a checkout_zone set as the country of Canada' do + let!(:canada) { create(:country, iso: 'CA', states_required: true) } + let!(:canada_state) { create(:state, country: canada) } + let!(:checkout_zone) { create(:zone, name: 'Checkout Zone', countries: [canada]) } + + before do + Spree::Country.update_all(states_required: true) + Spree::Config.checkout_zone = checkout_zone.name + end + + context 'and default_country_iso of the United States' do + before do + Spree::Config.default_country_iso = Spree::Country.find_by!(iso: 'US').iso + end + + it 'the shipping address country select includes only options for Canada' do + visit spree.new_admin_order_path + click_link 'Customer' + within '#shipping' do + expect(page).to have_select( + 'Country', + options: ['Canada'] + ) + end + end + + it 'does not show any shipping address state' do + visit spree.new_admin_order_path + click_link 'Customer' + within '#shipping' do + expect(page).to have_select( + 'State', + disabled: true, + visible: false, + options: [''] + ) + end + end + + it 'the billing address country select includes only options for Canada' do + visit spree.new_admin_order_path + click_link 'Customer' + within '#billing' do + expect(page).to have_select( + 'Country', + options: ['Canada'] + ) + end + end + + it 'does not show any billing address state' do + visit spree.new_admin_order_path + click_link 'Customer' + within '#billing' do + expect(page).to have_select( + 'State', + disabled: true, + visible: false, + options: [''] + ) + end + end + end + + context 'and default_country_iso of Canada' do + before do + Spree::Config.default_country_iso = Spree::Country.find_by!(iso: 'CA').iso + end + + it 'defaults the shipping address country to Canada' do + visit spree.new_admin_order_path + click_link 'Customer' + within '#shipping' do + expect(page).to have_select( + 'Country', + selected: 'Canada', + options: ['Canada'] + ) + end + end + + it 'shows relevant shipping address states' do + visit spree.new_admin_order_path + click_link 'Customer' + within '#shipping' do + expect(page).to have_select( + 'State', + options: [''] + canada.states.map(&:name) + ) + end + end + + it 'defaults the billing address country to Canada' do + visit spree.new_admin_order_path + click_link 'Customer' + within '#billing' do + expect(page).to have_select( + 'Country', + selected: 'Canada', + options: ['Canada'] + ) + end + end + + it 'shows relevant billing address states' do + visit spree.new_admin_order_path + click_link 'Customer' + within '#billing' do + expect(page).to have_select( + 'State', + options: [''] + canada.states.map(&:name) + ) + end + end + end + end + def fill_in_address fill_in "First Name", with: "John 99" fill_in "Last Name", with: "Doe" diff --git a/core/app/helpers/spree/base_helper.rb b/core/app/helpers/spree/base_helper.rb index 9969463683b..0c8bf5c011b 100644 --- a/core/app/helpers/spree/base_helper.rb +++ b/core/app/helpers/spree/base_helper.rb @@ -111,13 +111,7 @@ def taxons_tree(root_taxon, current_taxon, max_level = 1) end def available_countries(restrict_to_zone: Spree::Config[:checkout_zone]) - checkout_zone = Zone.find_by(name: restrict_to_zone) - - if checkout_zone && checkout_zone.kind == 'country' - countries = checkout_zone.country_list - else - countries = Country.all - end + countries = Spree::Country.available(restrict_to_zone: restrict_to_zone) country_names = Carmen::Country.all.map do |country| [country.code, country.name] diff --git a/core/app/models/spree/country.rb b/core/app/models/spree/country.rb index 9f57ff6b243..b6989622007 100644 --- a/core/app/models/spree/country.rb +++ b/core/app/models/spree/country.rb @@ -17,6 +17,14 @@ def self.default end end + 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 + end + def <=>(other) name <=> other.name end diff --git a/core/spec/models/spree/country_spec.rb b/core/spec/models/spree/country_spec.rb index efa1c56bdc3..98d1182ee33 100644 --- a/core/spec/models/spree/country_spec.rb +++ b/core/spec/models/spree/country_spec.rb @@ -60,6 +60,82 @@ end end + describe '.available' do + let!(:united_states) { create(:country, iso: 'US') } + let!(:canada) { create(:country, iso: 'CA') } + let!(:italy) { create(:country, iso: 'IT') } + let!(:custom_zone) { create(:zone, name: 'Custom Zone', countries: [united_states, italy]) } + + context 'with a checkout zone defined' do + context 'when checkout zone is of type country' do + let!(:checkout_zone) { create(:zone, name: 'Checkout Zone', countries: [united_states, canada]) } + + before do + Spree::Config.checkout_zone = checkout_zone.name + end + + context 'with no arguments' do + it 'returns "Checkout Zone" countries' do + 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 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 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 contain_exactly(united_states, canada) + end + end + end + + context 'when checkout zone is of type state' do + let!(:state) { create(:state, country: united_states) } + let!(:checkout_zone) { create(:zone, name: 'Checkout Zone', states: [state]) } + + before do + Spree::Config[:checkout_zone] = checkout_zone.name + end + + context 'with no arguments' do + it 'returns all countries' do + expect(described_class.available(restrict_to_zone: nil)).to contain_exactly(united_states, canada, italy) + end + end + end + end + + context 'with no checkout zone defined' do + context 'with no arguments' do + it 'returns all countries' do + expect(described_class.available).to contain_exactly(united_states, canada, italy) + end + end + + context 'setting nil as restricting zone' do + it 'returns all countries' do + 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 contain_exactly(united_states, italy) + end + end + end + end + describe '#prices' do let(:country) { create(:country) } subject { country.prices }