From d23d5bf5147101435a294e4cd23bb9a0a25050f5 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Sun, 7 Feb 2016 17:51:19 +0100 Subject: [PATCH 01/10] Add a new preference for default country ISO code Right now, the default country for address drop downs is defined using that addresses `id`, which is error-prone and makes Solidus' startup depend on the database. This commit adds a preference for the country's ISO code, which is more understandable and does not depend on the database. --- core/app/models/spree/app_configuration.rb | 7 ++++++- core/spec/models/spree/app_configuration_spec.rb | 6 ++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/core/app/models/spree/app_configuration.rb b/core/app/models/spree/app_configuration.rb index 03ba57d9e75..885b886b018 100644 --- a/core/app/models/spree/app_configuration.rb +++ b/core/app/models/spree/app_configuration.rb @@ -117,10 +117,15 @@ class AppConfiguration < Preferences::Configuration preference :currency, :string, default: "USD" # @!attribute [rw] default_country_id - # @deprecated + # @deprecated Use the default country ISO preference instead # @return [Integer,nil] id of {Country} to be selected by default in dropdowns (default: nil) preference :default_country_id, :integer + # @!attribute [rw] default_country_iso + # Default customer country + # @return [String] id of {Country} to be selected by default in dropdowns (default: "US") + preference :default_country_iso, :string, default: 'US' + # @!attribute [rw] expedited_exchanges # Kicks off an exchange shipment upon return authorization save. # charge customer if they do not return items within timely manner. diff --git a/core/spec/models/spree/app_configuration_spec.rb b/core/spec/models/spree/app_configuration_spec.rb index 37ede21a976..41960ccfc3c 100644 --- a/core/spec/models/spree/app_configuration_spec.rb +++ b/core/spec/models/spree/app_configuration_spec.rb @@ -24,4 +24,10 @@ subject { prefs.stock } it { is_expected.to be_a Spree::Core::StockConfiguration } end + + describe '@default_country_iso_code' do + it 'is the USA by default' do + expect(prefs[:default_country_iso]).to eq("US") + end + end end From e0e9ddf0ecc93cf42e40d8f7d1b94474ce304ae1 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Sun, 7 Feb 2016 18:05:36 +0100 Subject: [PATCH 02/10] Make Spree::Country.default use default ISO instead Also, raise an ActiveRecord::RecordNotFound if the default country is not present. In this case, a number of core functionalities (especially Customer returns) will not work as expected. --- .../spree/admin/stock_locations_controller.rb | 10 +++----- .../admin/orders/customer_details_spec.rb | 6 ++--- core/app/models/spree/country.rb | 4 +-- core/spec/models/spree/country_spec.rb | 25 +++++++++++++++++++ 4 files changed, 32 insertions(+), 13 deletions(-) create mode 100644 core/spec/models/spree/country_spec.rb diff --git a/backend/app/controllers/spree/admin/stock_locations_controller.rb b/backend/app/controllers/spree/admin/stock_locations_controller.rb index 4c5d240e939..900ce49926e 100644 --- a/backend/app/controllers/spree/admin/stock_locations_controller.rb +++ b/backend/app/controllers/spree/admin/stock_locations_controller.rb @@ -6,15 +6,11 @@ class StockLocationsController < ResourceController private def set_country - if Spree::Config[:default_country_id].present? - @stock_location.country = Spree::Country.find(Spree::Config[:default_country_id]) - else - @stock_location.country = Spree::Country.find_by!(iso: 'US') - end + @stock_location.country = Spree::Country.default rescue ActiveRecord::RecordNotFound - flash[:error] = Spree.t(:stock_locations_need_a_default_country) - redirect_to(admin_stock_locations_path) && return + flash[:error] = Spree.t(:stock_locations_need_a_default_country) + redirect_to(admin_stock_locations_path) && return end end end diff --git a/backend/spec/features/admin/orders/customer_details_spec.rb b/backend/spec/features/admin/orders/customer_details_spec.rb index b040a2e2f95..c9194e277e2 100644 --- a/backend/spec/features/admin/orders/customer_details_spec.rb +++ b/backend/spec/features/admin/orders/customer_details_spec.rb @@ -48,7 +48,7 @@ context "editing an order" do before do configure_spree_preferences do |config| - config.default_country_id = country.id + config.default_country_iso = country.iso config.company = true end @@ -111,12 +111,12 @@ end context "country associated was removed" do - let(:brazil) { create(:country, iso: "BRA", name: "Brazil") } + let(:brazil) { create(:country, iso: "BR", name: "Brazil") } before do order.bill_address.country.destroy configure_spree_preferences do |config| - config.default_country_id = brazil.id + config.default_country_iso = brazil.iso end end diff --git a/core/app/models/spree/country.rb b/core/app/models/spree/country.rb index ba37d2a7952..88a8f8009c0 100644 --- a/core/app/models/spree/country.rb +++ b/core/app/models/spree/country.rb @@ -13,9 +13,7 @@ def self.states_required_by_country_id end def self.default - if default_country_id = Spree::Config[:default_country_id] - find_by_id(default_country_id) - end || first + find_by!(iso: Spree::Config.default_country_iso) end def <=>(other) diff --git a/core/spec/models/spree/country_spec.rb b/core/spec/models/spree/country_spec.rb new file mode 100644 index 00000000000..9bf00fa10eb --- /dev/null +++ b/core/spec/models/spree/country_spec.rb @@ -0,0 +1,25 @@ +require 'spec_helper' + +describe Spree::Country, type: :model do + describe '.default' do + before do + create(:country, iso: "DE", id: 1) + create(:country, id: 2) + end + + context 'with the configuration setting an existing ISO code' do + it 'is a country with the configurations ISO code' do + expect(described_class.default).to be_a(Spree::Country) + expect(described_class.default.iso).to eq('US') + end + end + + context 'with the configuration setting an non-existing ISO code' do + before { Spree::Config[:default_country_iso] = "ZZ" } + + it 'raises a Record not Found error' do + expect { described_class.default }.to raise_error(ActiveRecord::RecordNotFound) + end + end + end +end From bd80a65aa5a034b3a7a2e3ce2cde4ea36c786a22 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Mon, 8 Feb 2016 10:49:19 +0100 Subject: [PATCH 03/10] Use default country ISO code for API endpoint The only configuration value actually emitted via the API is the default country id. This commit adds another value to be emitted, the default country ISO, and feeds that from the actual default country found at runtime. --- api/app/views/spree/api/config/show.v1.rabl | 3 ++- api/spec/controllers/spree/api/config_controller_spec.rb | 4 +++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/api/app/views/spree/api/config/show.v1.rabl b/api/app/views/spree/api/config/show.v1.rabl index 4e28ef4c345..3e7121af74c 100644 --- a/api/app/views/spree/api/config/show.v1.rabl +++ b/api/app/views/spree/api/config/show.v1.rabl @@ -1,2 +1,3 @@ object false -node(:default_country_id) { Spree::Config[:default_country_id] } \ No newline at end of file +node(:default_country_id) { Spree::Country.default.id } +node(:default_country_iso) { Spree::Config[:default_country_iso] } diff --git a/api/spec/controllers/spree/api/config_controller_spec.rb b/api/spec/controllers/spree/api/config_controller_spec.rb index b5730dfae0d..6f0e39bae16 100644 --- a/api/spec/controllers/spree/api/config_controller_spec.rb +++ b/api/spec/controllers/spree/api/config_controller_spec.rb @@ -2,6 +2,7 @@ module Spree describe Api::ConfigController, type: :controller do + let!(:default_country) { create :country, iso: "US"} render_views before do @@ -17,7 +18,8 @@ module Spree it "returns some configuration settings" do api_get :show expect(response).to be_success - expect(json_response["default_country_id"]).to eq(Spree::Config[:default_country_id]) + expect(json_response["default_country_iso"]).to eq("US") + expect(json_response["default_country_id"]).to eq(default_country.id) end end end From df6b797ec58c74b0427815d759eec02d6b19c000 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Mon, 8 Feb 2016 10:53:53 +0100 Subject: [PATCH 04/10] Adapt Specs for Admin::StockLocationsController Before, this would rely on the default country id. Now it uses the default country ISO. --- .../spree/admin/stock_locations_controller_spec.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/backend/spec/controllers/spree/admin/stock_locations_controller_spec.rb b/backend/spec/controllers/spree/admin/stock_locations_controller_spec.rb index d9fe5596291..d3469b50911 100644 --- a/backend/spec/controllers/spree/admin/stock_locations_controller_spec.rb +++ b/backend/spec/controllers/spree/admin/stock_locations_controller_spec.rb @@ -14,10 +14,11 @@ module Admin end end - context "with a default country present" do + context "with a default country other than the US present" do + let(:country) { create :country, iso: "BR" } + before do - country = FactoryGirl.create(:country) - Spree::Config[:default_country_id] = country.id + Spree::Config[:default_country_iso] = country.iso end it "can create a new stock location" do From 6c7e8080b0b80ab2933fd0eecc7efa5088215bfa Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Mon, 8 Feb 2016 10:56:59 +0100 Subject: [PATCH 05/10] Use default country ISO in configurations menu --- .../app/views/spree/admin/shared/_configuration_menu.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/app/views/spree/admin/shared/_configuration_menu.html.erb b/backend/app/views/spree/admin/shared/_configuration_menu.html.erb index d575477b38c..cdf0fbbd9df 100644 --- a/backend/app/views/spree/admin/shared/_configuration_menu.html.erb +++ b/backend/app/views/spree/admin/shared/_configuration_menu.html.erb @@ -26,7 +26,7 @@ <% end %> <% if can?(:display, Spree::State) %> - <% if country = Spree::Country.find_by_id(Spree::Config[:default_country_id]) || Spree::Country.first %> + <% if country = Spree::Country.find_by(iso: Spree::Config[:default_country_iso]) || Spree::Country.first %> <%= configurations_sidebar_menu_item Spree::State.model_name.human(count: :other), admin_country_states_path(country) %> <% end %> <% end %> From c75d7564d3208ce230b7a133d7c4f75ed38518f6 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Mon, 8 Feb 2016 10:58:00 +0100 Subject: [PATCH 06/10] Do not set default country id in seeds By default, preferences are not stored anyway, and with the default country ISO code present, we do not need to first find the US and then store the ID. --- core/db/default/spree/countries.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/core/db/default/spree/countries.rb b/core/db/default/spree/countries.rb index 70dd06be305..c33b4ec850b 100644 --- a/core/db/default/spree/countries.rb +++ b/core/db/default/spree/countries.rb @@ -15,5 +15,3 @@ ActiveRecord::Base.transaction do Spree::Country.create!(countries) end - -Spree::Config[:default_country_id] ||= Spree::Country.find_by(iso: "US").id From df57194f5bb5a0a65c2886c0182c053fe5a79f6e Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Mon, 8 Feb 2016 11:02:48 +0100 Subject: [PATCH 07/10] Change behaviour of Spree::Address.build_default Instead of the first country found, please raise an error if the default country ISO code does not have an associated country. The issue referenced in the spec, https://github.com/spree/spree/issues/1142, is people complaining about no states found for Nil, which only ever happened because Spree::Country.default would silently fail before. --- core/spec/models/spree/address_spec.rb | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/core/spec/models/spree/address_spec.rb b/core/spec/models/spree/address_spec.rb index 8b51f690bd8..dbd2fe7421f 100644 --- a/core/spec/models/spree/address_spec.rb +++ b/core/spec/models/spree/address_spec.rb @@ -165,18 +165,20 @@ context 'has a default country' do before do - Spree::Config[:default_country_id] = default_country.id + Spree::Config[:default_country_iso] = default_country.iso end - it "sets up a new record with Spree::Config[:default_country_id]" do + it "sets up a new record with Spree::Config[:default_country_iso]" do expect(Spree::Address.build_default.country).to eq default_country end end # Regression test for https://github.com/spree/spree/issues/1142 - it "uses the first available country if :default_country_id is set to an invalid value" do - Spree::Config[:default_country_id] = "0" - expect(Spree::Address.build_default.country).to eq default_country + it "raises ActiveRecord::RecordNotFound if :default_country_iso is set to an invalid value" do + Spree::Config[:default_country_iso] = "00" + expect { + Spree::Address.build_default.country + }.to raise_error(ActiveRecord::RecordNotFound) end end end From 8f00f895b5dee8798352e9635440d08f0c38d218 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Mon, 8 Feb 2016 11:06:56 +0100 Subject: [PATCH 08/10] Adapt address feature specs to use default country ISO Also, make sure the countries created actually have correct data by providing the two-letter ISO code to the factories. --- frontend/spec/features/address_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frontend/spec/features/address_spec.rb b/frontend/spec/features/address_spec.rb index 8892e5fccee..af53b348b78 100644 --- a/frontend/spec/features/address_spec.rb +++ b/frontend/spec/features/address_spec.rb @@ -22,7 +22,7 @@ let!(:canada) { create(:country, name: "Canada", states_required: true, iso: "CA") } let!(:uk) { create(:country, name: "United Kingdom", states_required: true, iso: "GB") } - before { Spree::Config[:default_country_id] = uk.id } + before { Spree::Config[:default_country_iso] = uk.iso } context "but has no state" do it "shows the state input field" do @@ -47,7 +47,7 @@ end context "user changes to country without states required" do - let!(:france) { create(:country, name: "France", states_required: false, iso: "FRA") } + let!(:france) { create(:country, name: "France", states_required: false, iso: "FR") } it "clears the state name" do click_button "Checkout" @@ -63,7 +63,7 @@ end context "country does not require state", js: true do - let!(:france) { create(:country, name: "France", states_required: false, iso: "FRA") } + let!(:france) { create(:country, name: "France", states_required: false, iso: "FR") } it "shows a disabled state input field" do click_button "Checkout" From bb08d2857c97c7c6727a0709b6c1d12eac34ab1d Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Mon, 8 Feb 2016 20:19:40 +0100 Subject: [PATCH 09/10] Improve YARD docs for new parameter --- core/app/models/spree/app_configuration.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/app/models/spree/app_configuration.rb b/core/app/models/spree/app_configuration.rb index 885b886b018..fdb254b83fe 100644 --- a/core/app/models/spree/app_configuration.rb +++ b/core/app/models/spree/app_configuration.rb @@ -122,8 +122,8 @@ class AppConfiguration < Preferences::Configuration preference :default_country_id, :integer # @!attribute [rw] default_country_iso - # Default customer country - # @return [String] id of {Country} to be selected by default in dropdowns (default: "US") + # Default customer country ISO code + # @return [String] Two-letter ISO code of a {Spree::Country} to assumed as the country of an unidentified customer (default: "US") preference :default_country_iso, :string, default: 'US' # @!attribute [rw] expedited_exchanges From b6b74a3d6fc7a1c15f8a5a84dbbea5181e114aba Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 10 Feb 2016 10:30:56 +0100 Subject: [PATCH 10/10] Re-enable finding default country by ID Previously, this PR broke existing shops with a default country ID with no Warning. With this commit, we get a warning, and still load the default country by ID. The fallback will do exactly what the default code is doing. --- core/app/models/spree/country.rb | 7 ++++++- core/spec/models/spree/country_spec.rb | 27 ++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/core/app/models/spree/country.rb b/core/app/models/spree/country.rb index 88a8f8009c0..2635489d797 100644 --- a/core/app/models/spree/country.rb +++ b/core/app/models/spree/country.rb @@ -13,7 +13,12 @@ def self.states_required_by_country_id end def self.default - find_by!(iso: Spree::Config.default_country_iso) + if Spree::Config.default_country_id + ActiveSupport::Deprecation.warn("Setting your default country via its ID is deprecated. Please set your default country via the `default_country_iso` setting.", caller) + find_by(id: Spree::Config.default_country_id) || find_by!(iso: Spree::Config.default_country_iso) + else + find_by!(iso: Spree::Config.default_country_iso) + end end def <=>(other) diff --git a/core/spec/models/spree/country_spec.rb b/core/spec/models/spree/country_spec.rb index 9bf00fa10eb..24753382c04 100644 --- a/core/spec/models/spree/country_spec.rb +++ b/core/spec/models/spree/country_spec.rb @@ -7,6 +7,33 @@ create(:country, id: 2) end + subject(:default_country) { described_class.default } + + context 'with the configuration setting an existing legacy default country ID' do + before do + Spree::Config[:default_country_id] = 2 + end + + it 'emits a deprecation warning' do + expect(ActiveSupport::Deprecation).to receive(:warn) + default_country + end + + it 'is the country with that ID' do + expect(default_country).to eq(Spree::Country.find(2)) + end + end + + context 'with the configuration setting a non-existing legacy default country ID' do + before do + Spree::Config[:default_country_id] = 0 + end + + it 'loads the country configured by the ISO code' do + expect(default_country).to eq(Spree::Country.find(2)) + end + end + context 'with the configuration setting an existing ISO code' do it 'is a country with the configurations ISO code' do expect(described_class.default).to be_a(Spree::Country)