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/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 40e527dc3b5..00000000000 --- a/core/app/models/spree/current_store_selector.rb +++ /dev/null @@ -1,28 +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 - if store_key - Spree::Store.current(store_key) - else - Spree::Store.default - end - end - - private - - def store_key - @request.headers['HTTP_SPREE_STORE'] || @request.env['SERVER_NAME'] - 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/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..ff61730f31a --- /dev/null +++ b/core/app/models/spree/store_selector/by_server_name.rb @@ -0,0 +1,30 @@ +# 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 + 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/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 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/store_selector/by_server_name_spec.rb b/core/spec/models/spree/store_selector/by_server_name_spec.rb new file mode 100644 index 00000000000..0aa97f16296 --- /dev/null +++ b/core/spec/models/spree/store_selector/by_server_name_spec.rb @@ -0,0 +1,26 @@ +require 'spec_helper' + +describe Spree::StoreSelector::ByServerName 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 + end + end + end +end diff --git a/core/spec/models/spree/current_store_selector_spec.rb b/core/spec/models/spree/store_selector/legacy_spec.rb similarity index 74% rename from core/spec/models/spree/current_store_selector_spec.rb rename to core/spec/models/spree/store_selector/legacy_spec.rb index dbac5a538c8..87b6a84bb5f 100644 --- a/core/spec/models/spree/current_store_selector_spec.rb +++ b/core/spec/models/spree/store_selector/legacy_spec.rb @@ -1,8 +1,8 @@ require 'spec_helper' -describe Spree::CurrentStoreSelector do +describe Spree::StoreSelector::Legacy 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: {}) } @@ -21,6 +21,14 @@ 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" } 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)