From c646eb3939fb585304dcd7ae0e28cca5c1287592 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Fri, 1 Dec 2017 19:32:37 +1100 Subject: [PATCH 1/6] Disable api auth as there is no Spree api key set Although Spree::Api::Config[:requires_authentication] is set to false by default for some unknown reason if not done explicitly Spree still returns it as false. This amends the change done in a87c89c83db9e54c58ea43e5b273d2906669c624, which introduced the bug. As there is no Spree api key set the auth fails when getting taxons. --- config/initializers/spree.rb | 2 +- .../spree/api/products_controller_spec.rb | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/config/initializers/spree.rb b/config/initializers/spree.rb index dc41aa9258..2c5ba91ac3 100644 --- a/config/initializers/spree.rb +++ b/config/initializers/spree.rb @@ -12,7 +12,7 @@ require 'spree/core/calculated_adjustments_decorator' require "#{Rails.root}/app/models/spree/payment_method_decorator" require "#{Rails.root}/app/models/spree/gateway_decorator" -Spree::Api::Config[:requires_authentication] = true +Spree::Api::Config[:requires_authentication] = false Spree.config do |config| config.shipping_instructions = true diff --git a/spec/controllers/spree/api/products_controller_spec.rb b/spec/controllers/spree/api/products_controller_spec.rb index fcc56c20a3..29d508ab9a 100644 --- a/spec/controllers/spree/api/products_controller_spec.rb +++ b/spec/controllers/spree/api/products_controller_spec.rb @@ -15,8 +15,7 @@ module Spree let(:attributes) { [:id, :name, :supplier, :price, :on_hand, :available_on, :permalink_live] } before do - stub_authentication! - Spree.user_class.stub :find_by_spree_api_key => current_api_user + allow(controller).to receive(:spree_current_user) { current_api_user } end context "as a normal user" do @@ -109,14 +108,11 @@ module Spree end describe '#clone' do - before do - spree_post :clone, product_id: product1.id, format: :json - end - context 'as a normal user' do sign_in_as_user! it 'denies access' do + spree_post :clone, product_id: product1.id, format: :json assert_unauthorized! end end @@ -125,10 +121,12 @@ module Spree sign_in_as_enterprise_user! [:supplier] it 'responds with a successful response' do + spree_post :clone, product_id: product1.id, format: :json expect(response.status).to eq(201) end it 'clones the product' do + spree_post :clone, product_id: product1.id, format: :json expect(json_response['name']).to eq("COPY OF #{product1.name}") end end @@ -137,10 +135,12 @@ module Spree sign_in_as_admin! it 'responds with a successful response' do + spree_post :clone, product_id: product1.id, format: :json expect(response.status).to eq(201) end it 'clones the product' do + spree_post :clone, product_id: product1.id, format: :json expect(json_response['name']).to eq("COPY OF #{product1.name}") end end From 5eb1fcddbbacdd6856feb78d6b6a03ea12508a49 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 4 Dec 2017 22:52:36 +1100 Subject: [PATCH 2/6] Remove dependency on TestingSupport by inlining --- .../spree/api/products_controller_spec.rb | 53 +++++++++++++++---- 1 file changed, 42 insertions(+), 11 deletions(-) diff --git a/spec/controllers/spree/api/products_controller_spec.rb b/spec/controllers/spree/api/products_controller_spec.rb index 29d508ab9a..d07d1dd270 100644 --- a/spec/controllers/spree/api/products_controller_spec.rb +++ b/spec/controllers/spree/api/products_controller_spec.rb @@ -1,9 +1,7 @@ require 'spec_helper' -require 'spree/api/testing_support/helpers' module Spree describe Spree::Api::ProductsController, type: :controller do - include Spree::Api::TestingSupport::Helpers render_views let(:supplier) { FactoryGirl.create(:supplier_enterprise) } @@ -19,7 +17,14 @@ module Spree end context "as a normal user" do - sign_in_as_user! + let(:current_api_user) do + build_stubbed(:user, enterprises: [], owned_groups: []) + end + + before do + allow(current_api_user) + .to receive(:has_spree_role?).with("admin").and_return(false) + end it "should deny me access to managed products" do spree_get :managed, { :template => 'bulk_index', :format => :json } @@ -28,10 +33,10 @@ module Spree end context "as an enterprise user" do - sign_in_as_enterprise_user! [:supplier] - - before :each do - spree_get :index, { :template => 'bulk_index', :format => :json } + let(:current_api_user) do + user = create(:user) + user.enterprise_roles.create(enterprise: supplier) + user end it "retrieves a list of managed products" do @@ -56,7 +61,14 @@ module Spree end context "as an administrator" do - sign_in_as_admin! + let(:current_api_user) do + build_stubbed(:user, enterprises: [], owned_groups: []) + end + + before do + allow(current_api_user) + .to receive(:has_spree_role?).with("admin").and_return(true) + end it "retrieves a list of managed products" do spree_get :managed, { :template => 'bulk_index', :format => :json } @@ -109,7 +121,14 @@ module Spree describe '#clone' do context 'as a normal user' do - sign_in_as_user! + let(:current_api_user) do + build_stubbed(:user, enterprises: [], owned_groups: []) + end + + before do + allow(current_api_user) + .to receive(:has_spree_role?).with("admin").and_return(false) + end it 'denies access' do spree_post :clone, product_id: product1.id, format: :json @@ -118,7 +137,11 @@ module Spree end context 'as an enterprise user' do - sign_in_as_enterprise_user! [:supplier] + let(:current_api_user) do + user = create(:user) + user.enterprise_roles.create(enterprise: supplier) + user + end it 'responds with a successful response' do spree_post :clone, product_id: product1.id, format: :json @@ -132,7 +155,15 @@ module Spree end context 'as an administrator' do - sign_in_as_admin! + let(:current_api_user) do + build_stubbed(:user, enterprises: [], owned_groups: []) + end + + before do + allow(current_api_user) + .to receive(:has_spree_role?).with("admin").and_return(true) + end + it 'responds with a successful response' do spree_post :clone, product_id: product1.id, format: :json From bb0223877c392306053fed85e8359d2002929b51 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 4 Dec 2017 23:07:44 +1100 Subject: [PATCH 3/6] Remove unused arguments and reduce object creation --- .../spree/api/products_controller_spec.rb | 36 +++++++------------ 1 file changed, 12 insertions(+), 24 deletions(-) diff --git a/spec/controllers/spree/api/products_controller_spec.rb b/spec/controllers/spree/api/products_controller_spec.rb index d07d1dd270..d952c42537 100644 --- a/spec/controllers/spree/api/products_controller_spec.rb +++ b/spec/controllers/spree/api/products_controller_spec.rb @@ -4,23 +4,19 @@ module Spree describe Spree::Api::ProductsController, type: :controller do render_views - let(:supplier) { FactoryGirl.create(:supplier_enterprise) } - let(:supplier2) { FactoryGirl.create(:supplier_enterprise) } - let!(:product1) { FactoryGirl.create(:product, supplier: supplier) } - let!(:product2) { FactoryGirl.create(:product, supplier: supplier) } - let!(:product3) { FactoryGirl.create(:product, supplier: supplier) } - let(:product_other_supplier) { FactoryGirl.create(:product, supplier: supplier2) } + let(:supplier) { create(:supplier_enterprise) } + let(:supplier2) { create(:supplier_enterprise) } + let!(:product1) { create(:product, supplier: supplier) } + let(:product_other_supplier) { create(:product, supplier: supplier2) } let(:attributes) { [:id, :name, :supplier, :price, :on_hand, :available_on, :permalink_live] } + let(:current_api_user) { build_stubbed(:user) } + before do allow(controller).to receive(:spree_current_user) { current_api_user } end context "as a normal user" do - let(:current_api_user) do - build_stubbed(:user, enterprises: [], owned_groups: []) - end - before do allow(current_api_user) .to receive(:has_spree_role?).with("admin").and_return(false) @@ -61,10 +57,6 @@ module Spree end context "as an administrator" do - let(:current_api_user) do - build_stubbed(:user, enterprises: [], owned_groups: []) - end - before do allow(current_api_user) .to receive(:has_spree_role?).with("admin").and_return(true) @@ -83,7 +75,11 @@ module Spree end it "sorts products in ascending id order" do + FactoryGirl.create(:product, supplier: supplier) + FactoryGirl.create(:product, supplier: supplier) + spree_get :index, { :template => 'bulk_index', :format => :json } + ids = json_response.map{ |product| product['id'] } ids[0].should < ids[1] ids[1].should < ids[2] @@ -101,14 +97,14 @@ module Spree it "should allow available_on to be nil" do spree_get :index, { :template => 'bulk_index', :format => :json } - json_response.size.should == 3 + json_response.size.should == 1 product5 = FactoryGirl.create(:product) product5.available_on = nil product5.save! spree_get :index, { :template => 'bulk_index', :format => :json } - json_response.size.should == 4 + json_response.size.should == 2 end it "soft deletes a product" do @@ -121,10 +117,6 @@ module Spree describe '#clone' do context 'as a normal user' do - let(:current_api_user) do - build_stubbed(:user, enterprises: [], owned_groups: []) - end - before do allow(current_api_user) .to receive(:has_spree_role?).with("admin").and_return(false) @@ -155,10 +147,6 @@ module Spree end context 'as an administrator' do - let(:current_api_user) do - build_stubbed(:user, enterprises: [], owned_groups: []) - end - before do allow(current_api_user) .to receive(:has_spree_role?).with("admin").and_return(true) From 52533fc04cc4d004c129224acd11a85011ca4891 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Tue, 5 Dec 2017 12:56:12 +1100 Subject: [PATCH 4/6] Rely on Spree's default value for requires_auth --- config/initializers/spree.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/config/initializers/spree.rb b/config/initializers/spree.rb index 2c5ba91ac3..9b62e3ad6e 100644 --- a/config/initializers/spree.rb +++ b/config/initializers/spree.rb @@ -12,8 +12,6 @@ require 'spree/core/calculated_adjustments_decorator' require "#{Rails.root}/app/models/spree/payment_method_decorator" require "#{Rails.root}/app/models/spree/gateway_decorator" -Spree::Api::Config[:requires_authentication] = false - Spree.config do |config| config.shipping_instructions = true config.address_requires_state = true From 0f0216fe7935aafc0641cf2d84fc9ebdcac674aa Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Tue, 5 Dec 2017 17:18:32 +1100 Subject: [PATCH 5/6] Upgrade spree to get our latest patch --- Gemfile | 2 +- Gemfile.lock | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Gemfile b/Gemfile index b8cad0ffaa..66e717039a 100644 --- a/Gemfile +++ b/Gemfile @@ -11,7 +11,7 @@ gem 'i18n-js', '~> 3.0.0' gem 'nokogiri', '>= 1.6.7.1' gem 'pg' -gem 'spree', github: 'openfoodfoundation/spree', branch: 'step-6a', ref: 'b1b33c7ec682e042bc939aac39dfa1f2de446227' +gem 'spree', github: 'openfoodfoundation/spree', branch: 'step-6a', ref: '86bf87f1b1e1b299edc8cd10a2486e44ba0a3987' gem 'spree_i18n', github: 'spree/spree_i18n', branch: '1-3-stable' gem 'spree_auth_devise', github: 'openfoodfoundation/spree_auth_devise', branch: 'spree-upgrade-intermediate' diff --git a/Gemfile.lock b/Gemfile.lock index 3d07ab5d93..883b658265 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -30,8 +30,8 @@ GIT GIT remote: https://github.com/openfoodfoundation/spree.git - revision: b1b33c7ec682e042bc939aac39dfa1f2de446227 - ref: b1b33c7ec682e042bc939aac39dfa1f2de446227 + revision: 86bf87f1b1e1b299edc8cd10a2486e44ba0a3987 + ref: 86bf87f1b1e1b299edc8cd10a2486e44ba0a3987 branch: step-6a specs: spree (1.3.99) @@ -807,4 +807,4 @@ RUBY VERSION ruby 2.1.5p273 BUNDLED WITH - 1.15.4 + 1.16.0 From 72889b5c365179f03886203a7473e7d48ce11164 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Wed, 6 Dec 2017 08:08:50 +1100 Subject: [PATCH 6/6] Stub current_user instead of api key's user --- app/controllers/api/enterprises_controller.rb | 7 +- .../api/enterprises_controller_spec.rb | 41 ++++++----- .../api/order_cycles_controller_spec.rb | 68 +++++++------------ .../spree/api/line_items_controller_spec.rb | 23 ++++--- .../spree/api/variants_controller_spec.rb | 3 +- 5 files changed, 71 insertions(+), 71 deletions(-) diff --git a/app/controllers/api/enterprises_controller.rb b/app/controllers/api/enterprises_controller.rb index 909f8a3567..c1f6fb3ce7 100644 --- a/app/controllers/api/enterprises_controller.rb +++ b/app/controllers/api/enterprises_controller.rb @@ -60,7 +60,12 @@ module Api def override_sells has_hub = current_api_user.owned_enterprises.is_hub.any? new_enterprise_is_producer = !!params[:enterprise][:is_primary_producer] - params[:enterprise][:sells] = (has_hub && !new_enterprise_is_producer) ? 'any' : 'unspecified' + + params[:enterprise][:sells] = if has_hub && !new_enterprise_is_producer + 'any' + else + 'unspecified' + end end def override_visible diff --git a/spec/controllers/api/enterprises_controller_spec.rb b/spec/controllers/api/enterprises_controller_spec.rb index f215062acd..61502cc4ed 100644 --- a/spec/controllers/api/enterprises_controller_spec.rb +++ b/spec/controllers/api/enterprises_controller_spec.rb @@ -7,22 +7,29 @@ module Api let(:enterprise) { create(:distributor_enterprise) } - before do - stub_authentication! - Enterprise.stub(:find).and_return(enterprise) - end - context "as an enterprise owner" do let(:enterprise_owner) { create_enterprise_user enterprise_limit: 10 } - let(:enterprise) { create(:distributor_enterprise, owner: enterprise_owner) } + let!(:enterprise) { create(:distributor_enterprise, owner: enterprise_owner) } before do - Spree.user_class.stub :find_by_spree_api_key => enterprise_owner + allow(controller).to receive(:spree_current_user) { enterprise_owner } end describe "creating an enterprise" do let(:australia) { Spree::Country.find_by_name('Australia') } - let(:new_enterprise_params) { {enterprise: {name: 'name', email: 'email@example.com', address_attributes: {address1: '123 Abc Street', city: 'Northcote', zipcode: '3070', state_id: australia.states.first, country_id: australia.id } } } } + let(:new_enterprise_params) do + { + enterprise: { + name: 'name', email: 'email@example.com', address_attributes: { + address1: '123 Abc Street', + city: 'Northcote', + zipcode: '3070', + state_id: australia.states.first, + country_id: australia.id + } + } + } + end it "creates as sells=any when it is not a producer" do spree_post :create, new_enterprise_params @@ -39,35 +46,37 @@ module Api before do enterprise_manager.enterprise_roles.build(enterprise: enterprise).save - Spree.user_class.stub :find_by_spree_api_key => enterprise_manager + allow(controller).to receive(:spree_current_user) { enterprise_manager } end describe "submitting a valid image" do before do + allow(Enterprise) + .to receive(:find_by_permalink).with(enterprise.id.to_s) { enterprise } enterprise.stub(:update_attributes).and_return(true) end it "I can update enterprise image" do - spree_post :update_image, logo: 'a logo' + spree_post :update_image, logo: 'a logo', id: enterprise.id response.should be_success end end end - describe "as an non-managing user" do + context "as an non-managing user" do let(:non_managing_user) { create_enterprise_user } before do - Spree.user_class.stub :find_by_spree_api_key => non_managing_user + allow(Enterprise) + .to receive(:find_by_permalink).with(enterprise.id.to_s) { enterprise } + allow(controller).to receive(:spree_current_user) { non_managing_user } end describe "submitting a valid image" do - before do - enterprise.stub(:update_attributes).and_return(true) - end + before { enterprise.stub(:update_attributes).and_return(true) } it "I can't update enterprise image" do - spree_post :update_image, logo: 'a logo' + spree_post :update_image, logo: 'a logo', id: enterprise.id assert_unauthorized! end end diff --git a/spec/controllers/api/order_cycles_controller_spec.rb b/spec/controllers/api/order_cycles_controller_spec.rb index 69c3acaaf3..0802bc1d58 100644 --- a/spec/controllers/api/order_cycles_controller_spec.rb +++ b/spec/controllers/api/order_cycles_controller_spec.rb @@ -14,8 +14,7 @@ module Api let(:attributes) { [:id, :name, :suppliers, :distributors] } before do - stub_authentication! - Spree.user_class.stub :find_by_spree_api_key => current_api_user + allow(controller).to receive(:spree_current_user) { current_api_user } end context "as a normal user" do @@ -77,38 +76,37 @@ module Api let!(:order_cycle) { create(:simple_order_cycle, suppliers: [oc_supplier], distributors: [oc_distributor]) } context "as the user of a supplier to an order cycle" do - before :each do - stub_authentication! - Spree.user_class.stub :find_by_spree_api_key => oc_supplier_user - spree_get :accessible, { :template => 'bulk_index', :format => :json } + before do + allow(controller).to receive(:spree_current_user) { oc_supplier_user } end it "gives me access" do + spree_get :accessible, { :template => 'bulk_index', :format => :json } + json_response.length.should == 1 json_response[0]['id'].should == order_cycle.id end end context "as the user of some other supplier" do - before :each do - stub_authentication! - Spree.user_class.stub :find_by_spree_api_key => other_supplier_user - spree_get :accessible, { :template => 'bulk_index', :format => :json } + before do + allow(controller).to receive(:spree_current_user) { other_supplier_user } end it "does not give me access" do + spree_get :accessible, { :template => 'bulk_index', :format => :json } json_response.length.should == 0 end end context "as the user of a hub for the order cycle" do - before :each do - stub_authentication! - Spree.user_class.stub :find_by_spree_api_key => oc_distributor_user - spree_get :accessible, { :template => 'bulk_index', :format => :json } + before do + allow(controller).to receive(:spree_current_user) { oc_distributor_user } end it "gives me access" do + spree_get :accessible, { :template => 'bulk_index', :format => :json } + json_response.length.should == 1 json_response[0]['id'].should == order_cycle.id end @@ -125,39 +123,32 @@ module Api let(:params) { { format: :json, as: 'distributor' } } before do - stub_authentication! - Spree.user_class.stub :find_by_spree_api_key => user + allow(controller).to receive(:spree_current_user) { user } end context "as the manager of a supplier in an order cycle" do - before do - user.enterprise_roles.create(enterprise: producer) - spree_get :accessible, params - end + before { user.enterprise_roles.create(enterprise: producer) } it "does not return the order cycle" do + spree_get :accessible, params expect(assigns(:order_cycles)).to_not include oc end end context "as the manager of a distributor in an order cycle" do - before do - user.enterprise_roles.create(enterprise: distributor) - spree_get :accessible, params - end + before { user.enterprise_roles.create(enterprise: distributor) } it "returns the order cycle" do + spree_get :accessible, params expect(assigns(:order_cycles)).to include oc end end context "as the manager of the coordinator of an order cycle" do - before do - user.enterprise_roles.create(enterprise: coordinator) - spree_get :accessible, params - end + before { user.enterprise_roles.create(enterprise: coordinator) } it "returns the order cycle" do + spree_get :accessible, params expect(assigns(:order_cycles)).to include oc end end @@ -173,39 +164,32 @@ module Api let(:params) { { format: :json, as: 'producer' } } before do - stub_authentication! - Spree.user_class.stub :find_by_spree_api_key => user + allow(controller).to receive(:spree_current_user) { user } end context "as the manager of a producer in an order cycle" do - before do - user.enterprise_roles.create(enterprise: producer) - spree_get :accessible, params - end + before { user.enterprise_roles.create(enterprise: producer) } it "returns the order cycle" do + spree_get :accessible, params expect(assigns(:order_cycles)).to include oc end end context "as the manager of a distributor in an order cycle" do - before do - user.enterprise_roles.create(enterprise: distributor) - spree_get :accessible, params - end + before { user.enterprise_roles.create(enterprise: distributor) } it "does not return the order cycle" do + spree_get :accessible, params expect(assigns(:order_cycles)).to_not include oc end end context "as the manager of the coordinator of an order cycle" do - before do - user.enterprise_roles.create(enterprise: coordinator) - spree_get :accessible, params - end + before { user.enterprise_roles.create(enterprise: coordinator) } it "returns the order cycle" do + spree_get :accessible, params expect(assigns(:order_cycles)).to include oc end end diff --git a/spec/controllers/spree/api/line_items_controller_spec.rb b/spec/controllers/spree/api/line_items_controller_spec.rb index 822e734b89..cf864aabc5 100644 --- a/spec/controllers/spree/api/line_items_controller_spec.rb +++ b/spec/controllers/spree/api/line_items_controller_spec.rb @@ -5,24 +5,27 @@ module Spree render_views before do - stub_authentication! - Spree.user_class.stub :find_by_spree_api_key => current_api_user - end - - def self.make_simple_data! - let!(:order) { FactoryGirl.create(:order, state: 'complete', completed_at: Time.zone.now) } - let!(:line_item) { FactoryGirl.create(:line_item, order: order, final_weight_volume: 500) } + allow(controller).to receive(:spree_current_user) { current_api_user } end #test that when a line item is updated, an order's fees are updated too context "as an admin user" do sign_in_as_admin! - make_simple_data! + + let(:order) { FactoryGirl.create(:order, state: 'complete', completed_at: Time.zone.now) } + let(:line_item) { FactoryGirl.create(:line_item, order: order, final_weight_volume: 500) } context "as a line item is updated" do + before { allow(controller).to receive(:order) { order } } + it "update distribution charge on the order" do - line_item_params = { order_id: order.number, id: line_item.id, line_item: { id: line_item.id, final_weight_volume: 520 }, format: :json} - allow(controller).to receive(:order) { order } + line_item_params = { + order_id: order.number, + id: line_item.id, + line_item: { id: line_item.id, final_weight_volume: 520 }, + format: :json + } + expect(order).to receive(:update_distribution_charge!) spree_post :update, line_item_params end diff --git a/spec/controllers/spree/api/variants_controller_spec.rb b/spec/controllers/spree/api/variants_controller_spec.rb index 5681b495fc..06f6185675 100644 --- a/spec/controllers/spree/api/variants_controller_spec.rb +++ b/spec/controllers/spree/api/variants_controller_spec.rb @@ -11,8 +11,7 @@ module Spree let(:attributes) { [:id, :options_text, :price, :on_hand, :unit_value, :unit_description, :on_demand, :display_as, :display_name] } before do - stub_authentication! - Spree.user_class.stub :find_by_spree_api_key => current_api_user + allow(controller).to receive(:spree_current_user) { current_api_user } end context "as a normal user" do