From 9d14acf28e9b5276fdcb7756ac8a02f3bd765cb9 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Fri, 23 Jun 2017 15:56:33 -0700 Subject: [PATCH 1/4] Simplify current store selection Previously we selected the current store by pretty hacky means. We took the value of either the SPREE_STORE HTTP header or the domain name, and then selected a store either matching that code or had that value contained anywhere in its url attribute. Failing to find these it falls back to the default store. Using SPREE_STORE and finding by code is intended to be used with a reverse proxy like nginx, and is a good and powerful way to configure Solidus' multi domain. But it does require extra and very specific configuration, and isn't overly suitable as a default. Selecting by url which contains the (header or domain name) value is slow, surprising, and error-prone. This commit removes support for the SPREE_STORE header, and allows only one url per-store. Users needing this functionality can replicate it by implementing their own store selector class. This commit also allows the store selector to perform only a single query to find the current store. --- core/app/models/spree/current_store_selector.rb | 16 +++++++--------- core/app/models/spree/store.rb | 5 +++++ core/spec/lib/spree/core/current_store_spec.rb | 10 ---------- .../models/spree/current_store_selector_spec.rb | 10 ---------- core/spec/models/spree/store_spec.rb | 12 ++++++++++-- 5 files changed, 22 insertions(+), 31 deletions(-) diff --git a/core/app/models/spree/current_store_selector.rb b/core/app/models/spree/current_store_selector.rb index 40e527dc3b5..c5491595bba 100644 --- a/core/app/models/spree/current_store_selector.rb +++ b/core/app/models/spree/current_store_selector.rb @@ -12,17 +12,15 @@ def initialize(request) # looking up by the requesting server's name. # @return [Spree::Store] def store - if store_key - Spree::Store.current(store_key) - else - Spree::Store.default - end - end + server_name = @request.env['SERVER_NAME'] - private + # We select a store which either matches our server name, or is default. + # We sort by `default ASC` so that a store matching SERVER_NAME will come + # first, and we will find that instead of the default. + store = Spree::Store.where(url: server_name).or(Store.where(default: true)).order(default: :asc).first - def store_key - @request.headers['HTTP_SPREE_STORE'] || @request.env['SERVER_NAME'] + # Provide a fallback, mostly for legacy/testing purposes + store || Spree::Store.new end end end diff --git a/core/app/models/spree/store.rb b/core/app/models/spree/store.rb index ed985837717..273b104828a 100644 --- a/core/app/models/spree/store.rb +++ b/core/app/models/spree/store.rb @@ -21,7 +21,12 @@ class Store < Spree::Base scope :by_url, lambda { |url| where("url like ?", "%#{url}%") } + class << self + deprecate :by_url, "Spree::Store.by_url is DEPRECATED", deprecator: Spree::Deprecation + end + def self.current(store_key) + Spree::Deprecation.warn "Spree::Store.current is DEPRECATED" current_store = Store.find_by(code: store_key) || Store.by_url(store_key).first if store_key current_store || Store.default end diff --git a/core/spec/lib/spree/core/current_store_spec.rb b/core/spec/lib/spree/core/current_store_spec.rb index 3644d01a9f6..b1260696173 100644 --- a/core/spec/lib/spree/core/current_store_spec.rb +++ b/core/spec/lib/spree/core/current_store_spec.rb @@ -20,16 +20,6 @@ it "returns the store with the matching domain" do expect(subject).to eq(store_2) end - - context "with headers" do - let(:request) { double(headers: { "HTTP_SPREE_STORE" => headers_code }, env: {}) } - let(:headers_code) { "HEADERS" } - let!(:store_3) { create :store, code: headers_code, default: false } - - it "returns the store with the matching code" do - expect(subject).to eq(store_3) - end - end end end diff --git a/core/spec/models/spree/current_store_selector_spec.rb b/core/spec/models/spree/current_store_selector_spec.rb index dbac5a538c8..81bb4e36c79 100644 --- a/core/spec/models/spree/current_store_selector_spec.rb +++ b/core/spec/models/spree/current_store_selector_spec.rb @@ -20,16 +20,6 @@ it "returns the store with the matching domain" do expect(subject).to eq(store_2) end - - context "with headers" do - let(:request) { double(headers: { "HTTP_SPREE_STORE" => headers_code }, env: {}) } - let(:headers_code) { "HEADERS" } - let!(:store_3) { create :store, code: headers_code, default: false } - - it "returns the store with the matching code" do - expect(subject).to eq(store_3) - end - end end end end diff --git a/core/spec/models/spree/store_spec.rb b/core/spec/models/spree/store_spec.rb index a82c7bb0ff9..820ceb55710 100644 --- a/core/spec/models/spree/store_spec.rb +++ b/core/spec/models/spree/store_spec.rb @@ -3,10 +3,14 @@ describe Spree::Store, type: :model do it { is_expected.to respond_to(:cart_tax_country_iso) } - describe ".by_url" do + describe ".by_url (deprecated)" do let!(:store) { create(:store, url: "website1.com\nwww.subdomain.com") } let!(:store_2) { create(:store, url: 'freethewhales.com') } + around do |example| + Spree::Deprecation.silence { example.run } + end + it "should find stores by url" do by_domain = Spree::Store.by_url('www.subdomain.com') @@ -15,7 +19,7 @@ end end - describe '.current' do + describe '.current (deprecated)' do let!(:store_1) { create(:store) } let!(:store_default) { create(:store, name: 'default', default: true) } let!(:store_2) { create(:store, default: false, url: 'www.subdomain.com') } @@ -23,6 +27,10 @@ delegate :current, to: :described_class + around do |example| + Spree::Deprecation.silence { example.run } + end + context "with no match" do it 'should return the default domain' do expect(current('foobar.com')).to eql(store_default) From ad333c90db494ff787f6ba35fa5722f07502e58f Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Thu, 6 Jul 2017 09:27:35 -0700 Subject: [PATCH 2/4] Add legacy store_selector --- core/app/models/spree/app_configuration.rb | 2 +- .../models/spree/current_store_selector.rb | 26 ---------- .../spree/store_selector/by_server_name.rb | 26 ++++++++++ .../app/models/spree/store_selector/legacy.rb | 48 +++++++++++++++++++ .../by_server_name_spec.rb} | 4 +- .../spree/store_selector/legacy_spec.rb | 44 +++++++++++++++++ 6 files changed, 121 insertions(+), 29 deletions(-) delete mode 100644 core/app/models/spree/current_store_selector.rb create mode 100644 core/app/models/spree/store_selector/by_server_name.rb create mode 100644 core/app/models/spree/store_selector/legacy.rb rename core/spec/models/spree/{current_store_selector_spec.rb => store_selector/by_server_name_spec.rb} (86%) create mode 100644 core/spec/models/spree/store_selector/legacy_spec.rb diff --git a/core/app/models/spree/app_configuration.rb b/core/app/models/spree/app_configuration.rb index 3277a980570..9ab7b8111cc 100644 --- a/core/app/models/spree/app_configuration.rb +++ b/core/app/models/spree/app_configuration.rb @@ -394,7 +394,7 @@ def tax_calculator_class # Spree::CurrentStoreSelector attr_writer :current_store_selector_class def current_store_selector_class - @current_store_selector_class ||= Spree::CurrentStoreSelector + @current_store_selector_class ||= Spree::StoreSelector::ByServerName end # Allows providing your own class instance for generating order numbers. diff --git a/core/app/models/spree/current_store_selector.rb b/core/app/models/spree/current_store_selector.rb deleted file mode 100644 index c5491595bba..00000000000 --- a/core/app/models/spree/current_store_selector.rb +++ /dev/null @@ -1,26 +0,0 @@ -# Default class for deciding what the current store is, given an HTTP request -# This is an extension point used in Spree::Core::ControllerHelpers::Store -# Custom versions of this class must respond to a store instance method -module Spree - class CurrentStoreSelector - def initialize(request) - @request = request - end - - # Chooses the current store based on a request. - # Checks request headers for HTTP_SPREE_STORE and falls back to - # looking up by the requesting server's name. - # @return [Spree::Store] - def store - server_name = @request.env['SERVER_NAME'] - - # We select a store which either matches our server name, or is default. - # We sort by `default ASC` so that a store matching SERVER_NAME will come - # first, and we will find that instead of the default. - store = Spree::Store.where(url: server_name).or(Store.where(default: true)).order(default: :asc).first - - # Provide a fallback, mostly for legacy/testing purposes - store || Spree::Store.new - end - end -end diff --git a/core/app/models/spree/store_selector/by_server_name.rb b/core/app/models/spree/store_selector/by_server_name.rb new file mode 100644 index 00000000000..697deeb5bd0 --- /dev/null +++ b/core/app/models/spree/store_selector/by_server_name.rb @@ -0,0 +1,26 @@ +# Default class for deciding what the current store is, given an HTTP request +# This is an extension point used in Spree::Core::ControllerHelpers::Store +# Custom versions of this class must respond to a store instance method +module Spree + module StoreSelector + class ByServerName + def initialize(request) + @request = request + end + + # Chooses the current store based on a request. + # @return [Spree::Store] + def store + server_name = @request.env['SERVER_NAME'] + + # We select a store which either matches our server name, or is default. + # We sort by `default ASC` so that a store matching SERVER_NAME will come + # first, and we will find that instead of the default. + store = Spree::Store.where(url: server_name).or(Store.where(default: true)).order(default: :asc).first + + # Provide a fallback, mostly for legacy/testing purposes + store || Spree::Store.new + end + end + end +end diff --git a/core/app/models/spree/store_selector/legacy.rb b/core/app/models/spree/store_selector/legacy.rb new file mode 100644 index 00000000000..11c068041b7 --- /dev/null +++ b/core/app/models/spree/store_selector/legacy.rb @@ -0,0 +1,48 @@ +# This class provides the old behaviour for finding a matching Spree::Store +# based on a request. +# +# To enable this, somewhere inside config/initializers/ add +# +# Spree::Config.current_store_selector_class = Spree::StoreSelector::Legacy +# +# This classes behaviour is somewhat complicated and has issues, which is why +# it has been replaced with Spree::StoreSelector::ByServerName by default. +# +# It will: +# * Find a "store_key" +# * from the HTTP_SPREE_STORE header, if it exists +# * or the server's domain name if HTTP_SPREE_STORE isn't set +# * Find a store, using the first match of: +# * having a code matching the store_key exactly +# * having a url which contains the store_key anywhere as a substring +# * has default set to true +# +module Spree + module StoreSelector + class Legacy + def initialize(request) + @request = request + end + + # Chooses the current store based on a request. + # Checks request headers for HTTP_SPREE_STORE and falls back to + # looking up by the requesting server's name. + # @return [Spree::Store] + def store + current_store = + if store_key + Spree::Store.find_by(code: store_key) || + Store.where("url like ?", "%#{store_key}%").first + end + + current_store || Spree::Store.default + end + + private + + def store_key + @request.headers['HTTP_SPREE_STORE'] || @request.env['SERVER_NAME'] + end + end + end +end diff --git a/core/spec/models/spree/current_store_selector_spec.rb b/core/spec/models/spree/store_selector/by_server_name_spec.rb similarity index 86% rename from core/spec/models/spree/current_store_selector_spec.rb rename to core/spec/models/spree/store_selector/by_server_name_spec.rb index 81bb4e36c79..0aa97f16296 100644 --- a/core/spec/models/spree/current_store_selector_spec.rb +++ b/core/spec/models/spree/store_selector/by_server_name_spec.rb @@ -1,8 +1,8 @@ require 'spec_helper' -describe Spree::CurrentStoreSelector do +describe Spree::StoreSelector::ByServerName do describe "#store" do - subject { Spree::CurrentStoreSelector.new(request).store } + subject { described_class.new(request).store } context "with a default" do let(:request) { double(headers: {}, env: {}) } diff --git a/core/spec/models/spree/store_selector/legacy_spec.rb b/core/spec/models/spree/store_selector/legacy_spec.rb new file mode 100644 index 00000000000..87b6a84bb5f --- /dev/null +++ b/core/spec/models/spree/store_selector/legacy_spec.rb @@ -0,0 +1,44 @@ +require 'spec_helper' + +describe Spree::StoreSelector::Legacy do + describe "#store" do + subject { described_class.new(request).store } + + context "with a default" do + let(:request) { double(headers: {}, env: {}) } + let!(:store_1) { create :store, default: true } + + it "returns the default store" do + expect(subject).to eq(store_1) + end + + context "with a domain match" do + let(:request) { double(headers: {}, env: { "SERVER_NAME" => url } ) } + let(:url) { "server-name.org" } + let!(:store_2) { create :store, default: false, url: url } + + it "returns the store with the matching domain" do + expect(subject).to eq(store_2) + end + + context 'the store has multiple URLs' do + let!(:store_2) { create :store, default: false, url: "foo\n#{url}\nbar" } + + it "returns the store with the matching domain" do + expect(subject).to eq(store_2) + end + end + + context "with headers" do + let(:request) { double(headers: { "HTTP_SPREE_STORE" => headers_code }, env: {}) } + let(:headers_code) { "HEADERS" } + let!(:store_3) { create :store, code: headers_code, default: false } + + it "returns the store with the matching code" do + expect(subject).to eq(store_3) + end + end + end + end + end +end From 94a0ca6b7d722a6add628cc2bb32dd341bea398e Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Thu, 6 Jul 2017 15:03:10 -0700 Subject: [PATCH 3/4] Update deprecation message inside CurrentStore --- core/lib/spree/core/current_store.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/lib/spree/core/current_store.rb b/core/lib/spree/core/current_store.rb index 3ae11af9ead..0d85a61a451 100644 --- a/core/lib/spree/core/current_store.rb +++ b/core/lib/spree/core/current_store.rb @@ -7,7 +7,7 @@ class CurrentStore def initialize(request) @request = request @current_store_selector = Spree::Config.current_store_selector_class.new(request) - Spree::Deprecation.warn "Using Spree::Core::CurrentStore is deprecated. Use Spree::CurrentStoreSelector instead", caller + Spree::Deprecation.warn "Using Spree::Core::CurrentStore is deprecated. Use Spree::Config.current_store_selector_class instead", caller end # Delegate store selection to Spree::Config.current_store_selector_class From 29aa49410a907b273b55888a96b60cac7734a479 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Tue, 11 Jul 2017 16:41:17 -0700 Subject: [PATCH 4/4] Add CHANGELOG entry for ByServerName --- CHANGELOG.md | 2 ++ core/app/models/spree/store_selector/by_server_name.rb | 10 +++++++--- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 48b386a2489..613e9ec448a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ - Rails 5.1 [\#1895](https://github.com/solidusio/solidus/pull/1895) ([jhawthorn](https://github.com/jhawthorn)) +- The default behaviour for selecting the current store has changed. Stores are now only returned if their url matches the current domain exactly (falling back to the default store) [\#2041](https://github.com/solidusio/solidus/pull/2041) [\#1993](https://github.com/solidusio/solidus/pull/1993) ([jhawthorn](https://github.com/jhawthorn), [kennyadsl](https://github.com/kennyadsl)) + - Remove dependency on premailer gem [\#2061](https://github.com/solidusio/solidus/pull/2061) ([cbrunsdon](https://github.com/cbrunsdon)) - Order#outstanding_balance now uses reimbursements instead of refunds to calculate the amount that should be paid on an order. [#2002](https://github.com/solidusio/solidus/pull/2002) (many contributors :heart:) diff --git a/core/app/models/spree/store_selector/by_server_name.rb b/core/app/models/spree/store_selector/by_server_name.rb index 697deeb5bd0..ff61730f31a 100644 --- a/core/app/models/spree/store_selector/by_server_name.rb +++ b/core/app/models/spree/store_selector/by_server_name.rb @@ -1,6 +1,10 @@ -# Default class for deciding what the current store is, given an HTTP request -# This is an extension point used in Spree::Core::ControllerHelpers::Store -# Custom versions of this class must respond to a store instance method +# Default implementation for finding the current store is given an HTTP request +# +# This is the new default behaviour, starting in Solidus 2.3.0. For the old +# behaviour see Spree::StoreSelector::Legacy. +# +# This attempts to find a Spree::Store with a URL matching the domain name of +# the request exactly. Failing that it will return the store marked as default. module Spree module StoreSelector class ByServerName