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:

- e6c7acdff3
- 2b47c9145a
- b9f19d5777 (diff-412c5af2ec1ba9f6643f6df5a673c1d4R105)

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]) }

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

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
 

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

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

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)

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