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