Merge pull request #2041 from jhawthorn/select_store_single_query

Simplify current store selection

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:)

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.

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

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

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

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

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
 

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

core/spec/models/spree/current_store_selector_spec.rb → 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" }

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,14 +19,18 @@
     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') }
     let!(:store_3) { create(:store, default: false, url: 'www.another.com', code: 'CODE') }
 
     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)