From cdb49f88b0b7bb4bd509dba6a098c4dbf207a95b Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 25 Jan 2019 15:57:52 +1100 Subject: [PATCH] Move Variant deletion into its own service This keeps the override of Spree's model leaner. More importantly, it prepares us for using `destroy` instead of `delete`. In the past, `Product#delete` soft-deleted the product, but didn't delete the variants. When we use `Product#destroy` to soft-delete the product, it will also call destroy on the variants. If the model doesn't allow the deletion of the last variant, it will fail. So when a product is deleted we want to allow the deletion of all variants. But the user should not be allowed to delete the last variant. That's why I'm moving the check to the controller level. Related commits: - https://github.com/openfoodfoundation/openfoodnetwork/commit/e6c7acdff356d02233d5f993cb93c2d1af824548 - https://github.com/openfoodfoundation/openfoodnetwork/commit/2b47c9145a9b18f346dd186ef4254dd062eb0648 - https://github.com/openfoodfoundation/openfoodnetwork/commit/b9f19d5777ed5034297dd3725c17bccffd39cf5d#diff-412c5af2ec1ba9f6643f6df5a673c1d4R105 --- .../admin/variants_controller_decorator.rb | 7 +++++-- .../spree/api/variants_controller_decorator.rb | 2 +- app/models/spree/variant_decorator.rb | 13 ++++--------- app/services/variant_deleter.rb | 17 +++++++++++++++++ .../spree/api/variants_controller_spec.rb | 10 ++++++++++ spec/models/spree/product_spec.rb | 9 --------- spec/models/spree/variant_spec.rb | 17 ----------------- 7 files changed, 37 insertions(+), 38 deletions(-) create mode 100644 app/services/variant_deleter.rb diff --git a/app/controllers/spree/admin/variants_controller_decorator.rb b/app/controllers/spree/admin/variants_controller_decorator.rb index 999ca0b22c..cabc7e5eec 100644 --- a/app/controllers/spree/admin/variants_controller_decorator.rb +++ b/app/controllers/spree/admin/variants_controller_decorator.rb @@ -10,8 +10,11 @@ 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 - flash[:success] = I18n.t('notice_messages.variant_deleted') + if VariantDeleter.new.delete(@variant) # This line changed + flash[:success] = Spree.t('notice_messages.variant_deleted') + else + flash[:success] = Spree.t('notice_messages.variant_not_deleted') + end respond_with(@variant) do |format| format.html { redirect_to admin_product_variants_url(params[:product_id]) } diff --git a/app/controllers/spree/api/variants_controller_decorator.rb b/app/controllers/spree/api/variants_controller_decorator.rb index 3bc8b1c511..0c5a1dd2a6 100644 --- a/app/controllers/spree/api/variants_controller_decorator.rb +++ b/app/controllers/spree/api/variants_controller_decorator.rb @@ -3,7 +3,7 @@ Spree::Api::VariantsController.class_eval do @variant = scope.find(params[:variant_id]) authorize! :delete, @variant - @variant.delete + VariantDeleter.new.delete(@variant) respond_with @variant, status: 204 end end diff --git a/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb index de8f151c2c..3c2076820c 100644 --- a/app/models/spree/variant_decorator.rb +++ b/app/models/spree/variant_decorator.rb @@ -106,15 +106,10 @@ Spree::Variant.class_eval do end def delete - if product.variants == [self] # Only variant left on product - errors.add :product, I18n.t(:spree_variant_product_error) - false - else - transaction do - self.update_column(:deleted_at, Time.zone.now) - ExchangeVariant.where(variant_id: self).destroy_all - self - end + transaction do + self.update_column(:deleted_at, Time.zone.now) + ExchangeVariant.where(variant_id: self).destroy_all + self end end diff --git a/app/services/variant_deleter.rb b/app/services/variant_deleter.rb new file mode 100644 index 0000000000..e76bb8d91a --- /dev/null +++ b/app/services/variant_deleter.rb @@ -0,0 +1,17 @@ +# Checks the validity of a soft-delete call. +class VariantDeleter + def delete(variant) + if only_variant_on_product?(variant) + variant.errors.add :product, I18n.t(:spree_variant_product_error) + return false + end + + variant.delete + end + + private + + def only_variant_on_product?(variant) + variant.product.variants == [variant] + end +end diff --git a/spec/controllers/spree/api/variants_controller_spec.rb b/spec/controllers/spree/api/variants_controller_spec.rb index 698a3e7926..af50d783e3 100644 --- a/spec/controllers/spree/api/variants_controller_spec.rb +++ b/spec/controllers/spree/api/variants_controller_spec.rb @@ -69,6 +69,16 @@ module Spree lambda { variant.reload }.should_not raise_error variant.deleted_at.should_not be_nil end + + it "doesn't delete the only variant of the product" do + product = create(:product) + variant = product.variants.first + + spree_delete :soft_delete, {variant_id: variant.to_param, product_id: product.to_param, format: :json} + + expect(variant.reload).to_not be_deleted + expect(assigns(:variant).errors[:product]).to include "must have at least one variant" + end end end end diff --git a/spec/models/spree/product_spec.rb b/spec/models/spree/product_spec.rb index 2d4243eae2..a060e34a09 100644 --- a/spec/models/spree/product_spec.rb +++ b/spec/models/spree/product_spec.rb @@ -55,15 +55,6 @@ module Spree end end - - it "does not allow the last variant to be deleted" do - product = create(:simple_product) - expect(product.variants(:reload).length).to eq 1 - v = product.variants.last - v.delete - expect(v.errors[:product]).to include "must have at least one variant" - end - context "when the product has variants" do let(:product) do product = create(:simple_product) diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb index d48d16d636..5894b4e12d 100644 --- a/spec/models/spree/variant_spec.rb +++ b/spec/models/spree/variant_spec.rb @@ -535,22 +535,5 @@ module Spree v.destroy e.reload.variant_ids.should be_empty end - - context "as the last variant of a product" do - let!(:extra_variant) { create(:variant) } - let!(:product) { extra_variant.product } - let!(:first_variant) { product.variants.first } - - before { product.reload } - - it "cannot be deleted" do - expect(product.variants.length).to eq 2 - expect(extra_variant.delete).to eq extra_variant - expect(product.variants(:reload).length).to eq 1 - expect(first_variant.delete).to be false - expect(product.variants(:reload).length).to eq 1 - expect(first_variant.errors[:product]).to include "must have at least one variant" - end - end end end