From aef0d28dd185e19c6fef149e2748f68b4852a833 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 1 Feb 2021 16:28:45 +0100 Subject: [PATCH 1/2] Do not load unmodified VOs from DB Closes #6727. This avoids the authorization of all the VOs of the hub, which will go through VOs that may have become invalid due to their underlying product not belonging to the supplier the hub has permissions with (or any other data integrity issue). This is utterly confusing for the user who is only given a generic error and doesn't understand what's wrong with the particular VO they changed, while it may be fine after all. What's more, this often results in a customer support request, which then may end up with a dev finding out which VO is broken. Also, there's no point in loading them from DB if the users didn't touch them. --- app/controllers/admin/variant_overrides_controller.rb | 8 ++++++++ .../admin/variant_overrides_controller_spec.rb | 7 +++++++ 2 files changed, 15 insertions(+) diff --git a/app/controllers/admin/variant_overrides_controller.rb b/app/controllers/admin/variant_overrides_controller.rb index de58a3b514..ba591bd967 100644 --- a/app/controllers/admin/variant_overrides_controller.rb +++ b/app/controllers/admin/variant_overrides_controller.rb @@ -79,6 +79,14 @@ module Admin joins(variant: :product). preload(variant: :product). for_hubs(params[:hub_id] || @hubs) + + return @variant_overrides unless params.key?(:variant_overrides) + + @variant_overrides.where(id: modified_variant_overrides_ids) + end + + def modified_variant_overrides_ids + variant_overrides_params.map { |vo| vo[:id] } end def collection_actions diff --git a/spec/controllers/admin/variant_overrides_controller_spec.rb b/spec/controllers/admin/variant_overrides_controller_spec.rb index 1dca6eeec6..ad2adcc6d7 100644 --- a/spec/controllers/admin/variant_overrides_controller_spec.rb +++ b/spec/controllers/admin/variant_overrides_controller_spec.rb @@ -36,6 +36,13 @@ describe Admin::VariantOverridesController, type: :controller do spree_put :bulk_update, format: format, variant_overrides: variant_override_params expect(response).to redirect_to unauthorized_path end + + it 'only authorizes the updated variant overrides' do + other_variant_override = create(:variant_override, hub: hub, variant: create(:variant)) + expect(controller).not_to receive(:authorize!).with(:update, other_variant_override) + + spree_put :bulk_update, format: format, variant_overrides: variant_override_params + end end context "and the producer has granted VO permission" do From a19aceae8c9c3739573164a8f5dd174bd075be42 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 1 Feb 2021 16:36:28 +0100 Subject: [PATCH 2/2] Replace spree_put with put in controller tests This removes the following annoying deprecation warnings that happen in each test. ``` DEPRECATION WARNING: You are trying to generate the URL for a named route called :main_app but no such route was found. In the future, this will result in an `ActionController::UrlGenerationError` exception. (called from process_action_with_route at /usr/s rc/app/spec/support/controller_requests_helper.rb:49) DEPRECATION WARNING: Passing the `use_route` option in functional tests are deprecated. Support for this option in the `process` method (and the related `get`, `head`, `post`, `patch`, `put` and `delete` helpers) will be removed in the next version without replacement. Functional tests are essentially unit tests for controllers and they should not require knowledge to how the application's routes are configured. Instead, you should explicitly pass the appropiate params to the `process` method. Previously th e engines guide also contained an incorrect example that recommended using this option to test an engine's controllers within the dummy application. That recommendation was incorrect and has since been corrected. Instead, you should override the `@routes` variable in the test case with `Foo::Engine.routes`. See the updated engines guide for details. (called from process_action_with_route at /usr/src/app/spec/support/controller_requests_helper.rb:49) ``` --- .../variant_overrides_controller_spec.rb | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/spec/controllers/admin/variant_overrides_controller_spec.rb b/spec/controllers/admin/variant_overrides_controller_spec.rb index ad2adcc6d7..1b3a3e3da7 100644 --- a/spec/controllers/admin/variant_overrides_controller_spec.rb +++ b/spec/controllers/admin/variant_overrides_controller_spec.rb @@ -21,7 +21,7 @@ describe Admin::VariantOverridesController, type: :controller do end it "redirects to unauthorized" do - spree_put :bulk_update, format: format, variant_overrides: variant_override_params + put :bulk_update, format: format, variant_overrides: variant_override_params expect(response).to redirect_to unauthorized_path end end @@ -33,7 +33,7 @@ describe Admin::VariantOverridesController, type: :controller do context "but the producer has not granted VO permission" do it "redirects to unauthorized" do - spree_put :bulk_update, format: format, variant_overrides: variant_override_params + put :bulk_update, format: format, variant_overrides: variant_override_params expect(response).to redirect_to unauthorized_path end @@ -41,7 +41,7 @@ describe Admin::VariantOverridesController, type: :controller do other_variant_override = create(:variant_override, hub: hub, variant: create(:variant)) expect(controller).not_to receive(:authorize!).with(:update, other_variant_override) - spree_put :bulk_update, format: format, variant_overrides: variant_override_params + put :bulk_update, format: format, variant_overrides: variant_override_params end end @@ -51,7 +51,7 @@ describe Admin::VariantOverridesController, type: :controller do end it "loads data" do - spree_put :bulk_update, format: format, variant_overrides: variant_override_params + put :bulk_update, format: format, variant_overrides: variant_override_params expect(assigns[:hubs]).to eq [hub] expect(assigns[:producers]).to eq [variant.product.supplier] expect(assigns[:hub_permissions]).to eq Hash[hub.id, [variant.product.supplier.id]] @@ -59,7 +59,8 @@ describe Admin::VariantOverridesController, type: :controller do end it "allows me to update the variant override" do - spree_put :bulk_update, format: format, variant_overrides: variant_override_params + put :bulk_update, format: format, variant_overrides: variant_override_params + variant_override.reload expect(variant_override.price).to eq 123.45 expect(variant_override.count_on_hand).to eq 321 @@ -71,7 +72,7 @@ describe Admin::VariantOverridesController, type: :controller do let(:variant_override_params) { [{ id: variant_override.id, price: "", count_on_hand: "", default_stock: nil, resettable: nil, sku: nil, on_demand: nil }] } it "destroys the variant override" do - spree_put :bulk_update, format: format, variant_overrides: variant_override_params + put :bulk_update, format: format, variant_overrides: variant_override_params expect(VariantOverride.find_by(id: variant_override.id)).to be_nil end end @@ -83,7 +84,7 @@ describe Admin::VariantOverridesController, type: :controller do before { deleted_variant.update_attribute :deleted_at, Time.zone.now } it "allows to update other variant overrides" do - spree_put :bulk_update, format: format, variant_overrides: variant_override_params + put :bulk_update, format: format, variant_overrides: variant_override_params expect(response).to_not redirect_to unauthorized_path variant_override.reload @@ -117,7 +118,7 @@ describe Admin::VariantOverridesController, type: :controller do end it "redirects to unauthorized" do - spree_put :bulk_reset, params + put :bulk_reset, params expect(response).to redirect_to unauthorized_path end end @@ -129,7 +130,7 @@ describe Admin::VariantOverridesController, type: :controller do context "where the producer has not granted create_variant_overrides permission to the hub" do it "restricts access" do - spree_put :bulk_reset, params + put :bulk_reset, params expect(response).to redirect_to unauthorized_path end end @@ -138,7 +139,7 @@ describe Admin::VariantOverridesController, type: :controller do let!(:er1) { create(:enterprise_relationship, parent: producer, child: hub, permissions_list: [:create_variant_overrides]) } it "loads data" do - spree_put :bulk_reset, params + put :bulk_reset, params expect(assigns[:hubs]).to eq [hub] expect(assigns[:producers]).to eq [producer] expect(assigns[:hub_permissions]).to eq Hash[hub.id, [producer.id]] @@ -148,7 +149,7 @@ describe Admin::VariantOverridesController, type: :controller do it "updates stock to default values where reset is enabled" do expect(variant_override1.reload.count_on_hand).to eq 5 # reset enabled expect(variant_override2.reload.count_on_hand).to eq 2 # reset disabled - spree_put :bulk_reset, params + put :bulk_reset, params expect(variant_override1.reload.count_on_hand).to eq 7 # reset enabled expect(variant_override2.reload.count_on_hand).to eq 2 # reset disabled end @@ -163,7 +164,7 @@ describe Admin::VariantOverridesController, type: :controller do it "does not reset count_on_hand for variant_overrides not in params" do expect { - spree_put :bulk_reset, params + put :bulk_reset, params }.to_not change{ variant_override3.reload.count_on_hand } end end