From 63fbf68d543d3160c7f9ade018562bdc3b276bac Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Thu, 9 Nov 2017 09:06:06 -0700 Subject: [PATCH 1/3] Admin order edit: Disable state_name when switching to a country with states If you had a country without states + a manually typed-in `state_name` and then switched to a country with states, the form would still submit the old state_name alongside the new state_id. This updates the js code to disable both state inputs (`state_name` and `state_id`) by default and then re-enable whichever one ended up being used. This is the same thing we were already doing for hiding & showing the inputs. --- .../javascripts/spree/backend/views/state_select.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/backend/app/assets/javascripts/spree/backend/views/state_select.js b/backend/app/assets/javascripts/spree/backend/views/state_select.js index e43759b1c2e..d5bd3bb26ea 100644 --- a/backend/app/assets/javascripts/spree/backend/views/state_select.js +++ b/backend/app/assets/javascripts/spree/backend/views/state_select.js @@ -39,11 +39,11 @@ Spree.Views.StateSelect = Backbone.View.extend({ }, render: function() { - this.$state_select.empty().hide(); - this.$state_input.hide(); + this.$state_select.empty().hide().prop('disabled', true); + this.$state_input.hide().prop('disabled', true); if (!this.states.fetched) { - this.$state_select.show().prop("disabled", true); + this.$state_select.show(); } else if (this.states.length) { var $state_select = this.$state_select; this.states.each(function(state) { @@ -54,7 +54,7 @@ Spree.Views.StateSelect = Backbone.View.extend({ this.$state_select.val(this.model.get("state_id")) this.$state_select.show().prop("disabled", false); } else { - this.$state_input.prop('disabled', false).show(); + this.$state_input.show().prop('disabled', false); } } }) From 2d209dba9c25fe6f5a74118fc7c2e4d140258b54 Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Thu, 9 Nov 2017 09:08:21 -0700 Subject: [PATCH 2/3] Admin order edit: Clear address state_id when switching to country w/o states The admin wasn't clearing `address.state_id` from the DB when you switched from a country with states to a country without states. You would then end up with an address with both `state_name` and `state_id`, and `state_text` would end up showing the old `state_id`. This happened because we have the complicated state_name + state_id situation with addresses, and we swap out one form input with another depending on the country. We needed to provide default hidden inputs for state_name and state_id with blank values. --- backend/app/views/spree/admin/shared/_address_form.html.erb | 2 ++ 1 file changed, 2 insertions(+) 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 443b8e6662e..4dc5e060a35 100644 --- a/backend/app/views/spree/admin/shared/_address_form.html.erb +++ b/backend/app/views/spree/admin/shared/_address_form.html.erb @@ -48,9 +48,11 @@
"> <%= f.label :state_id, Spree::State.model_name.human %> + <%= f.hidden_field :state_name, value: nil %> <%= 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' %> + <%= 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?} %>
From 5b11430de18dcb4a96abe14d12a36ccee7186252 Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Thu, 9 Nov 2017 09:20:02 -0700 Subject: [PATCH 3/3] Validate Address.state matches country if present --- core/app/models/spree/address.rb | 7 +++++++ core/config/locales/en.yml | 4 ++++ core/spec/models/spree/address_spec.rb | 23 ++++++++++++++++++----- 3 files changed, 29 insertions(+), 5 deletions(-) diff --git a/core/app/models/spree/address.rb b/core/app/models/spree/address.rb index 101eb916c8f..710037c2f44 100644 --- a/core/app/models/spree/address.rb +++ b/core/app/models/spree/address.rb @@ -14,6 +14,7 @@ class Address < Spree::Base validates :phone, presence: true, if: :require_phone? validate :state_validate + validate :validate_state_matches_country alias_attribute :first_name, :firstname alias_attribute :last_name, :lastname @@ -209,5 +210,11 @@ def state_validate # ensure at least one state field is populated errors.add :state, :blank if state.blank? && state_name.blank? end + + def validate_state_matches_country + if state && state.country != country + errors.add(:state, :does_not_match_country) + end + end end end diff --git a/core/config/locales/en.yml b/core/config/locales/en.yml index c9a0c121652..9bbfc0cd6df 100644 --- a/core/config/locales/en.yml +++ b/core/config/locales/en.yml @@ -639,6 +639,10 @@ en: other: Zones errors: models: + spree/address: + attributes: + state: + does_not_match_country: does not match the country spree/calculator/tiered_flat_rate: attributes: base: diff --git a/core/spec/models/spree/address_spec.rb b/core/spec/models/spree/address_spec.rb index a38e6711f9c..5f4b05a91d7 100644 --- a/core/spec/models/spree/address_spec.rb +++ b/core/spec/models/spree/address_spec.rb @@ -68,11 +68,24 @@ expect(address.state_name).to be_nil end - it "state is entered but country does not contain that state" do - address.state = state - address.country = Spree::Country.new(states_required: true) - address.valid? - expect(address.errors["state"]).to eq(['is invalid']) + context 'when the country does not match the state' do + context 'when the country requires states' do + it 'is invalid' do + address.state = state + address.country = Spree::Country.new(states_required: true) + address.valid? + expect(address.errors["state"]).to eq(['is invalid', 'does not match the country']) + end + end + + context 'when the country does not require states' do + it 'is invalid' do + address.state = state + address.country = Spree::Country.new(states_required: false) + address.valid? + expect(address.errors["state"]).to eq(['does not match the country']) + end + end end it "both state and state_name are entered but country does not contain the state" do