Validate unit value when updating variant_unit

Variants whose product's variant_unit is weight or volume require
a unit_value.

app/models/spree/product_set.rb

@@ -35,15 +35,28 @@ class Spree::ProductSet < ModelSet
   end
 
   def update_product_only_attributes(product, attributes)
-    if attributes.except(:id, :variants_attributes, :master_attributes).present?
-      product.update_attributes(
-        attributes.except(:id, :variants_attributes, :master_attributes)
-      )
+    variant_related_attrs = [:id, :variants_attributes, :master_attributes]
+
+    if attributes.except(*variant_related_attrs).present?
+      product.assign_attributes(attributes.except(*variant_related_attrs))
+
+      product.variants.each do |variant|
+        validate_presence_of_unit_value(product, variant)
+      end
+
+      product.save if errors.empty?
     else
       true
     end
   end
 
+  def validate_presence_of_unit_value(product, variant)
+    return unless %w(weight volume).include?(product.variant_unit)
+    return if variant.unit_value.present?
+
+    product.errors.add(:unit_value, "can't be blank")
+  end
+
   def update_product_variants(product, attributes)
     if attributes[:variants_attributes]
       update_variants_attributes(product, attributes[:variants_attributes])

spec/controllers/spree/admin/products_controller_spec.rb

@@ -1,22 +1,75 @@
 require 'spec_helper'
 
 describe Spree::Admin::ProductsController, type: :controller do
-  describe "updating a product we do not have access to" do
-    let(:s_managed) { create(:enterprise) }
-    let(:s_unmanaged) { create(:enterprise) }
-    let(:p) { create(:simple_product, supplier: s_unmanaged, name: 'Peas') }
+  describe 'bulk_update' do
+    context "updating a product we do not have access to" do
+      let(:s_managed) { create(:enterprise) }
+      let(:s_unmanaged) { create(:enterprise) }
+      let(:product) do
+        create(:simple_product, supplier: s_unmanaged, name: 'Peas')
+      end
 
-    before do
-      login_as_enterprise_user [s_managed]
-      spree_post :bulk_update, {"products" => [{"id" => p.id, "name" => "Pine nuts"}]}
+      before do
+        login_as_enterprise_user [s_managed]
+        spree_post :bulk_update, {
+          "products" => [{"id" => product.id, "name" => "Pine nuts"}]
+        }
+      end
+
+      it "denies access" do
+        response.should redirect_to spree.unauthorized_url
+      end
+
+      it "does not update any product" do
+        product.reload.name.should_not == "Pine nuts"
+      end
     end
 
-    it "denies access" do
-      response.should redirect_to spree.unauthorized_url
-    end
+    context "when changing a product's variant_unit" do
+      let(:producer) { create(:enterprise) }
+      let!(:product) do
+        create(
+          :simple_product,
+          supplier: producer,
+          variant_unit: 'items',
+          variant_unit_scale: nil,
+          variant_unit_name: 'bunches',
+          unit_value: nil,
+          unit_description: 'some description'
+        )
+      end
 
-    it "does not update any product" do
-      p.reload.name.should_not == "Pine nuts"
+      before { login_as_enterprise_user([producer]) }
+
+      it 'fails' do
+        spree_post :bulk_update, {
+          "products" => [
+            {
+              "id" => product.id,
+              "variant_unit" => "weight",
+              "variant_unit_scale" => 1
+            }
+          ]
+        }
+
+        expect(response).to have_http_status(400)
+      end
+
+      it 'does not redirect to bulk_products' do
+        spree_post :bulk_update, {
+          "products" => [
+            {
+              "id" => product.id,
+              "variant_unit" => "weight",
+              "variant_unit_scale" => 1
+            }
+          ]
+        }
+
+        expect(response).not_to redirect_to(
+          '/api/products/bulk_products?page=1;per_page=500;'
+        )
+      end
     end
   end
 

spec/models/spree/product_set_spec.rb

@@ -32,35 +32,50 @@ describe Spree::ProductSet do
       end
 
       context 'when the product does exist' do
-        let!(:product) do
-          create(
-            :simple_product,
-            variant_unit: 'items',
-            variant_unit_scale: nil,
-            variant_unit_name: 'bunches'
-          )
-        end
+        context 'when a different varian_unit is passed' do
+          let!(:product) do
+            create(
+              :simple_product,
+              variant_unit: 'items',
+              variant_unit_scale: nil,
+              variant_unit_name: 'bunches',
+              unit_value: nil,
+              unit_description: 'some description'
+            )
+          end
 
-        let(:collection_hash) do
-          {
-            0 => {
-              id: product.id,
-              variant_unit: 'weight',
-              variant_unit_scale: 1
+          let(:collection_hash) do
+            {
+              0 => {
+                id: product.id,
+                variant_unit: 'weight',
+                variant_unit_scale: 1
+              }
             }
-          }
-        end
+          end
 
-        it 'updates all the specified product attributes' do
-          product_set.save
+          it 'does not update the product' do
+            product_set.save
 
-          expect(product.reload.attributes).to include(
-            'variant_unit' => 'weight',
-            'variant_unit_scale' => 1
-          )
+            expect(product.reload.attributes).to include(
+              'variant_unit' => 'items'
+            )
+          end
+
+          it 'adds an error' do
+            product_set.save
+            expect(product_set.errors.get(:base))
+              .to include("Unit value can't be blank")
+          end
+
+          it 'returns false' do
+            expect(product_set.save).to eq(false)
+          end
         end
 
         context 'when :master_attributes is passed' do
+          let!(:product) { create(:simple_product) }
+          let(:collection_hash) { { 0 => { id: product.id } } }
           let(:master_attributes) { { sku: '123' } }
 
           before do