From fbc3fc6a510d676a78c560dea642ca5372c90171 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Fri, 22 Mar 2019 09:18:31 +0100 Subject: [PATCH 1/3] Test spree/admin/variants_controller #destroy --- .../admin/variants_controller_decorator.rb | 2 +- .../spree/admin/variants_controller_spec.rb | 52 +++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/app/controllers/spree/admin/variants_controller_decorator.rb b/app/controllers/spree/admin/variants_controller_decorator.rb index 999ca0b22c..f93c6fcafc 100644 --- a/app/controllers/spree/admin/variants_controller_decorator.rb +++ b/app/controllers/spree/admin/variants_controller_decorator.rb @@ -15,7 +15,7 @@ Spree::Admin::VariantsController.class_eval do respond_with(@variant) do |format| format.html { redirect_to admin_product_variants_url(params[:product_id]) } - format.js { render_js_for_destroy } + format.js { render_js_for_destroy } end end diff --git a/spec/controllers/spree/admin/variants_controller_spec.rb b/spec/controllers/spree/admin/variants_controller_spec.rb index 36fd0463a2..d34444026d 100644 --- a/spec/controllers/spree/admin/variants_controller_spec.rb +++ b/spec/controllers/spree/admin/variants_controller_spec.rb @@ -35,6 +35,58 @@ module Spree assigns(:variants).should match_array [v1, v2] end end + + describe '#destroy' do + let(:variant) { create(:variant) } + + context 'when requesting with js' do + before do + allow(Spree::Variant).to receive(:find).with(variant.id.to_s) { variant } + allow(variant).to receive(:delete) + end + + it 'deletes the variant' do + spree_delete :destroy, id: variant.id, product_id: variant.product.permalink, format: 'js' + expect(variant).to have_received(:delete) + end + + it 'shows a success flash message' do + spree_delete :destroy, id: variant.id, product_id: variant.product.permalink, format: 'js' + expect(flash[:success]).to eq(I18n.t('notice_messages.variant_deleted')) + end + + it 'renders spree/admin/shared/destroy' do + spree_delete :destroy, id: variant.id, product_id: variant.product.permalink, format: 'js' + expect(response).to render_template('spree/admin/shared/_destroy') + end + end + + context 'when requesting with html' do + before do + allow(Spree::Variant).to receive(:find).with(variant.id.to_s) { variant } + allow(variant).to receive(:delete) + end + + it 'deletes the variant' do + spree_delete :destroy, id: variant.id, product_id: variant.product.permalink, format: 'html' + expect(variant).to have_received(:delete) + end + + it 'shows a success flash message' do + spree_delete :destroy, id: variant.id, product_id: variant.product.permalink, format: 'html' + expect(flash[:success]).to eq(I18n.t('notice_messages.variant_deleted')) + end + + it 'redirects to admin_product_variants_url' do + spree_delete :destroy, id: variant.id, product_id: variant.product.permalink, format: 'html' + expect(response).to redirect_to( + controller: 'spree/admin/variants', + action: :index, + product_id: variant.product.permalink + ) + end + end + end end end end From b293e00777dfbf6e1b49515b756face7278616e7 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Fri, 22 Mar 2019 09:37:20 +0100 Subject: [PATCH 2/3] Test that we refresh the cache on variant destroy These tests prove that https://github.com/openfoodfoundation/openfoodnetwork/issues/3629 is indeed is a bug because we don't refresh the cache when deleting a variant. --- .../spree/admin/variants_controller_spec.rb | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/spec/controllers/spree/admin/variants_controller_spec.rb b/spec/controllers/spree/admin/variants_controller_spec.rb index d34444026d..8846d80e22 100644 --- a/spec/controllers/spree/admin/variants_controller_spec.rb +++ b/spec/controllers/spree/admin/variants_controller_spec.rb @@ -59,6 +59,19 @@ module Spree spree_delete :destroy, id: variant.id, product_id: variant.product.permalink, format: 'js' expect(response).to render_template('spree/admin/shared/_destroy') end + + it 'refreshes the cache' do + expect(OpenFoodNetwork::ProductsCache).to receive(:variant_destroyed).with(variant) + spree_delete :destroy, id: variant.id, product_id: variant.product.permalink, format: 'js' + end + + it 'destroys all its exchanges' do + exchange = create(:exchange) + variant.exchanges << exchange + + spree_delete :destroy, id: variant.id, product_id: variant.product.permalink, format: 'js' + expect(variant.exchanges).to be_empty + end end context 'when requesting with html' do @@ -85,6 +98,19 @@ module Spree product_id: variant.product.permalink ) end + + it 'refreshes the cache' do + expect(OpenFoodNetwork::ProductsCache).to receive(:variant_destroyed).with(variant) + spree_delete :destroy, id: variant.id, product_id: variant.product.permalink, format: 'js' + end + + it 'destroys all its exchanges' do + exchange = create(:exchange) + variant.exchanges << exchange + + spree_delete :destroy, id: variant.id, product_id: variant.product.permalink, format: 'js' + expect(variant.exchanges).to be_empty + end end end end From 96b8c8ac2caa4a3ac4bf1120c3d130ac5d6f66ef Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Fri, 22 Mar 2019 09:47:35 +0100 Subject: [PATCH 3/3] Refresh products cache also on Variant#delete Note that, as explained in https://apidock.com/rails/v3.2.13/ActiveRecord/Relation/delete, `delete` does not trigger callbacks and so it skips the products cache logic. If we still want to avoid instantiating the AR object, we need to explicitly call that logic for the cache to be up-to-date. --- .../admin/variants_controller_decorator.rb | 3 ++- app/models/spree/variant_decorator.rb | 5 ++++ spec/models/spree/variant_spec.rb | 23 ++++++++++++++++++- 3 files changed, 29 insertions(+), 2 deletions(-) diff --git a/app/controllers/spree/admin/variants_controller_decorator.rb b/app/controllers/spree/admin/variants_controller_decorator.rb index f93c6fcafc..a87155b300 100644 --- a/app/controllers/spree/admin/variants_controller_decorator.rb +++ b/app/controllers/spree/admin/variants_controller_decorator.rb @@ -10,7 +10,8 @@ Spree::Admin::VariantsController.class_eval do def destroy @variant = Spree::Variant.find(params[:id]) - @variant.delete # This line changed, as well as removal of following conditional + @variant.delete_and_refresh_cache + flash[:success] = I18n.t('notice_messages.variant_deleted') respond_with(@variant) do |format| diff --git a/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb index cefb7917da..69359c6851 100644 --- a/app/models/spree/variant_decorator.rb +++ b/app/models/spree/variant_decorator.rb @@ -118,6 +118,11 @@ Spree::Variant.class_eval do end end + # Deletes the record, skipping callbacks, but it also refreshes the cache + def delete_and_refresh_cache + destruction { delete } + end + private def update_weight_from_unit_value diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb index fe4e132ae8..cc37f889ef 100644 --- a/spec/models/spree/variant_spec.rb +++ b/spec/models/spree/variant_spec.rb @@ -196,7 +196,6 @@ module Spree end it "refreshes the products cache for the entire product on destroy" do - # Does this ever happen? expect(OpenFoodNetwork::ProductsCache).to receive(:product_changed).with(product) expect(OpenFoodNetwork::ProductsCache).to receive(:variant_destroyed).never master.destroy @@ -551,4 +550,26 @@ module Spree end end end + + describe '#delete_and_refresh_cache' do + context 'when it is not the master variant' do + let(:variant) { create(:variant) } + + it 'refreshes the products cache on delete' do + expect(OpenFoodNetwork::ProductsCache).to receive(:variant_destroyed).with(variant) + variant.delete_and_refresh_cache + end + end + + context "when it is the master variant" do + let(:product) { create(:simple_product) } + let(:master) { product.master } + + it 'refreshes the products cache for the entire product on delete' do + expect(OpenFoodNetwork::ProductsCache).to receive(:product_changed).with(product) + expect(OpenFoodNetwork::ProductsCache).to receive(:variant_destroyed).never + master.delete_and_refresh_cache + end + end + end end