From bb83dc02ec29011126ef0a522533360e51d77290 Mon Sep 17 00:00:00 2001 From: Alberto Vena Date: Thu, 15 Jun 2017 17:18:51 +0200 Subject: [PATCH 1/2] Always use the default store This commit avoids doing 2 extra queries for every request (#1971) even when solidus store is not using solidus_multi_domain extension. This logic has now been moved to the solidus_multi_domain extension since it is needed only when multiple stores are expected (solidusio/solidus_multi_domain#67). --- .../models/spree/current_store_selector.rb | 23 +++++++------------ .../spec/lib/spree/core/current_store_spec.rb | 22 +----------------- .../spree/current_store_selector_spec.rb | 22 +----------------- 3 files changed, 10 insertions(+), 57 deletions(-) diff --git a/core/app/models/spree/current_store_selector.rb b/core/app/models/spree/current_store_selector.rb index 40e527dc3b5..d31ea74c324 100644 --- a/core/app/models/spree/current_store_selector.rb +++ b/core/app/models/spree/current_store_selector.rb @@ -1,5 +1,8 @@ # Default class for deciding what the current store is, given an HTTP request -# This is an extension point used in Spree::Core::ControllerHelpers::Store +# +# To use a custom version of this class just set the preference: +# Spree::Config.current_store_selector_class = CustomCurrentStoreSelector +# # Custom versions of this class must respond to a store instance method module Spree class CurrentStoreSelector @@ -7,22 +10,12 @@ 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. + # Select the store to be used. In this basic implementation the + # default store will be always selected. + # # @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'] + Spree::Store.default end end 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..48e607a6ef4 100644 --- a/core/spec/lib/spree/core/current_store_spec.rb +++ b/core/spec/lib/spree/core/current_store_spec.rb @@ -5,32 +5,12 @@ subject { Spree::Deprecation.silence { Spree::Core::CurrentStore.new(request).store } } context "with a default" do - let(:request) { double(headers: {}, env: {}) } + let(:request) { double('any request') } 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 "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 it 'is deprecated' do diff --git a/core/spec/models/spree/current_store_selector_spec.rb b/core/spec/models/spree/current_store_selector_spec.rb index dbac5a538c8..0e95bd42936 100644 --- a/core/spec/models/spree/current_store_selector_spec.rb +++ b/core/spec/models/spree/current_store_selector_spec.rb @@ -5,32 +5,12 @@ subject { Spree::CurrentStoreSelector.new(request).store } context "with a default" do - let(:request) { double(headers: {}, env: {}) } + let(:request) { double('any request') } 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 "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 01ae9b605d32d8d46f4c5b6776064d381bf580ac Mon Sep 17 00:00:00 2001 From: Alberto Vena Date: Fri, 16 Jun 2017 09:23:42 +0200 Subject: [PATCH 2/2] Remove api orders specs that expects multiple stores They'll be moved in solidus_multi_domain. This commit also reorganizes a bit api orders specs for the displaying orders part. --- .../spree/api/orders_controller_spec.rb | 71 +++++++++---------- 1 file changed, 34 insertions(+), 37 deletions(-) diff --git a/api/spec/controllers/spree/api/orders_controller_spec.rb b/api/spec/controllers/spree/api/orders_controller_spec.rb index 26deeaf920d..dbeb3ce3dcb 100644 --- a/api/spec/controllers/spree/api/orders_controller_spec.rb +++ b/api/spec/controllers/spree/api/orders_controller_spec.rb @@ -128,27 +128,32 @@ module Spree end end - it "cannot view all orders" do - api_get :index - assert_unauthorized! - end + context 'displaying orders' do + let(:current_api_user) { order.user } + let(:order) { create(:order, line_items: [line_item], store: Spree::Store.default) } - context "the current api user does not exist" do - let(:current_api_user) { nil } + it 'is not authorized to view an order of another user' do + another_user_order = create(:order, store: Spree::Store.default) + api_get :show, id: another_user_order.to_param - it "returns a 401" do - api_get :mine expect(response.status).to eq(401) end - end - context "the current api user is authenticated" do - let(:current_api_user) { order.user } - let(:store) { create(:store) } - let(:order) { create(:order, line_items: [line_item], store: store) } + it "can view one of its own order" do + api_get :show, id: order.to_param + + expect(response.status).to eq(200) + expect(json_response).to have_attributes(attributes) + expect(json_response["adjustments"]).to be_empty + end + + it "is not authorized to view all orders" do + api_get :index + + expect(response.status).to eq(401) + end - it "can view all of their own orders for the current store" do - request.env['SERVER_NAME'] = store.url + it "can view all of their own orders" do api_get :mine expect(response.status).to eq(200) @@ -160,16 +165,7 @@ module Spree expect(json_response["orders"].first["line_items"].first["id"]).to eq(line_item.id) end - it "cannot view orders for a different store" do - request.env['SERVER_NAME'] = "foo" - api_get :mine - - expect(response.status).to eq(200) - expect(json_response["orders"].length).to eq(0) - end - it "can filter the returned results" do - request.env['SERVER_NAME'] = store.url api_get :mine, q: { completed_at_not_null: 1 } expect(response.status).to eq(200) @@ -178,16 +174,15 @@ module Spree it "returns orders in reverse chronological order by completed_at" do order.update_columns completed_at: Time.current, created_at: 3.days.ago - - order2 = Order.create user: order.user, completed_at: Time.current - 1.day, created_at: 2.day.ago, store: store + order2 = Order.create user: order.user, completed_at: Time.current - 1.day, created_at: 2.day.ago expect(order2.created_at).to be > order.created_at - order3 = Order.create user: order.user, completed_at: nil, created_at: 1.day.ago, store: store + order3 = Order.create user: order.user, completed_at: nil, created_at: 1.day.ago expect(order3.created_at).to be > order2.created_at - order4 = Order.create user: order.user, completed_at: nil, created_at: 0.days.ago, store: store + order4 = Order.create user: order.user, completed_at: nil, created_at: 0.days.ago expect(order4.created_at).to be > order3.created_at - request.env['SERVER_NAME'] = store.url api_get :mine + expect(response.status).to eq(200) expect(json_response["pages"]).to eq(1) orders = json_response["orders"] @@ -196,6 +191,16 @@ module Spree expect(orders[1]["number"]).to eq(order2.number) expect([orders[2]["number"], orders[3]["number"]]).to match_array([order3.number, order4.number]) end + + context "when the current api user does not exist" do + let(:current_api_user) { nil } + + it "is not authorized to list orders" do + api_get :mine + + expect(response.status).to eq(401) + end + end end describe 'current' do @@ -208,14 +213,6 @@ module Spree end end - it "can view their own order" do - allow_any_instance_of(Order).to receive_messages user: current_api_user - api_get :show, id: order.to_param - expect(response.status).to eq(200) - expect(json_response).to have_attributes(attributes) - expect(json_response["adjustments"]).to be_empty - end - describe 'GET #show' do let(:order) { create :order_with_line_items } let(:adjustment) { FactoryGirl.create(:adjustment, adjustable: order, order: order) }