From c7c70252d02e640bb2dc84da255c805f83e029ac Mon Sep 17 00:00:00 2001
From: Rohan Mitchell <rohan@rohanmitchell.com>
Date: Tue, 11 Feb 2014 19:13:11 +1100
Subject: [PATCH 01/21] ofnTrackProduct and ofnTrackVariant accept nested
 properties as arguments

---
 .../admin/bulk_product_update.js.coffee       | 25 +++++++++----------
 .../unit/bulk_product_update_spec.js.coffee   | 13 +++++++---
 2 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/app/assets/javascripts/admin/bulk_product_update.js.coffee b/app/assets/javascripts/admin/bulk_product_update.js.coffee
index 81ad95f9ca..ce198c4a74 100644
--- a/app/assets/javascripts/admin/bulk_product_update.js.coffee
+++ b/app/assets/javascripts/admin/bulk_product_update.js.coffee
@@ -20,30 +20,31 @@ productEditModule.directive "ofnDecimal", ->
       viewValue
 
 
-productEditModule.directive "ofnTrackProduct", ->
+productEditModule.directive "ofnTrackProduct", ['$parse', ($parse) ->
   require: "ngModel"
   link: (scope, element, attrs, ngModel) ->
-    property_name = attrs.ofnTrackProduct
     ngModel.$parsers.push (viewValue) ->
       if ngModel.$dirty
-        addDirtyProperty scope.dirtyProducts, scope.product.id, property_name, viewValue
+        parsedPropertyName = $parse(attrs.ofnTrackProduct)
+        addDirtyProperty scope.dirtyProducts, scope.product.id, parsedPropertyName, viewValue
         scope.displayDirtyProducts()
       viewValue
+  ]
 
 
-productEditModule.directive "ofnTrackVariant", ->
+productEditModule.directive "ofnTrackVariant", ['$parse', ($parse) ->
   require: "ngModel"
   link: (scope, element, attrs, ngModel) ->
-    property_name = attrs.ofnTrackVariant
     ngModel.$parsers.push (viewValue) ->
       dirtyVariants = {}
       dirtyVariants = scope.dirtyProducts[scope.product.id].variants  if scope.dirtyProducts.hasOwnProperty(scope.product.id) and scope.dirtyProducts[scope.product.id].hasOwnProperty("variants")
       if ngModel.$dirty
-        addDirtyProperty dirtyVariants, scope.variant.id, property_name, viewValue
-        addDirtyProperty scope.dirtyProducts, scope.product.id, "variants", dirtyVariants
+        parsedPropertyName = $parse(attrs.ofnTrackVariant)
+        addDirtyProperty dirtyVariants, scope.variant.id, parsedPropertyName, viewValue
+        addDirtyProperty scope.dirtyProducts, scope.product.id, $parse("variants"), dirtyVariants
         scope.displayDirtyProducts()
       viewValue
-
+  ]
 
 productEditModule.directive "ofnToggleVariants", ->
   link: (scope, element, attrs) ->
@@ -515,13 +516,11 @@ filterSubmitProducts = (productsToFilter) ->
   filteredProducts
 
 
-addDirtyProperty = (dirtyObjects, objectID, propertyName, propertyValue) ->
-  if dirtyObjects.hasOwnProperty(objectID)
-    dirtyObjects[objectID][propertyName] = propertyValue
-  else
+addDirtyProperty = (dirtyObjects, objectID, parsedPropertyName, propertyValue) ->
+  if !dirtyObjects.hasOwnProperty(objectID)
     dirtyObjects[objectID] = {}
     dirtyObjects[objectID]["id"] = objectID
-    dirtyObjects[objectID][propertyName] = propertyValue
+  parsedPropertyName.assign(dirtyObjects[objectID], propertyValue)
 
 
 removeCleanProperty = (dirtyObjects, objectID, propertyName) ->
diff --git a/spec/javascripts/unit/bulk_product_update_spec.js.coffee b/spec/javascripts/unit/bulk_product_update_spec.js.coffee
index 9fe79278d4..4381bec9e1 100644
--- a/spec/javascripts/unit/bulk_product_update_spec.js.coffee
+++ b/spec/javascripts/unit/bulk_product_update_spec.js.coffee
@@ -202,10 +202,17 @@ describe "filtering products for submission to database", ->
 
 
 describe "Maintaining a live record of dirty products and properties", ->
+  parse = null
+  beforeEach ->
+    module "ofn.bulk_product_edit"
+  beforeEach inject(($parse) ->
+    parse = $parse
+  )
+
   describe "adding product properties to the dirtyProducts object", -> # Applies to both products and variants
     it "adds the product and the property to the list if property is dirty", ->
       dirtyProducts = {}
-      addDirtyProperty dirtyProducts, 1, "name", "Product 1"
+      addDirtyProperty dirtyProducts, 1, parse("name"), "Product 1"
       expect(dirtyProducts).toEqual 1:
         id: 1
         name: "Product 1"
@@ -216,7 +223,7 @@ describe "Maintaining a live record of dirty products and properties", ->
         id: 1
         notaname: "something"
 
-      addDirtyProperty dirtyProducts, 1, "name", "Product 3"
+      addDirtyProperty dirtyProducts, 1, parse("name"), "Product 3"
       expect(dirtyProducts).toEqual 1:
         id: 1
         notaname: "something"
@@ -228,7 +235,7 @@ describe "Maintaining a live record of dirty products and properties", ->
         id: 1
         name: "Product 1"
 
-      addDirtyProperty dirtyProducts, 1, "name", "Product 2"
+      addDirtyProperty dirtyProducts, 1, parse("name"), "Product 2"
       expect(dirtyProducts).toEqual 1:
         id: 1
         name: "Product 2"

From 0b255ed1e954bba06ca55c2150022c74c067a51f Mon Sep 17 00:00:00 2001
From: Rohan Mitchell <rohan@rohanmitchell.com>
Date: Tue, 11 Feb 2014 19:19:12 +1100
Subject: [PATCH 02/21] Pack the master variant before sending to the server

---
 .../admin/bulk_product_update.js.coffee       | 10 ++++--
 .../unit/bulk_product_update_spec.js.coffee   | 31 ++++++++++++++++---
 2 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/app/assets/javascripts/admin/bulk_product_update.js.coffee b/app/assets/javascripts/admin/bulk_product_update.js.coffee
index ce198c4a74..3d13e0423c 100644
--- a/app/assets/javascripts/admin/bulk_product_update.js.coffee
+++ b/app/assets/javascripts/admin/bulk_product_update.js.coffee
@@ -216,8 +216,13 @@ productEditModule.controller "AdminProductEditCtrl", [
 
       if product.variants
         for variant in product.variants
-          unit_value = $scope.variantUnitValue product, variant
-          variant.unit_value_with_description = "#{unit_value || ''} #{variant.unit_description || ''}".trim()
+          $scope.loadVariantVariantUnit product, variant
+      $scope.loadVariantVariantUnit product, product.master if product.master
+
+
+    $scope.loadVariantVariantUnit = (product, variant) ->
+      unit_value = $scope.variantUnitValue product, variant
+      variant.unit_value_with_description = "#{unit_value || ''} #{variant.unit_description || ''}".trim()
 
 
     $scope.variantUnitValue = (product, variant) ->
@@ -369,6 +374,7 @@ productEditModule.controller "AdminProductEditCtrl", [
         else
           product.variant_unit = product.variant_unit_with_scale
           product.variant_unit_scale = null
+      $scope.packVariant product, product.master if product.master
       if product.variants
         for id, variant of product.variants
           $scope.packVariant product, variant
diff --git a/spec/javascripts/unit/bulk_product_update_spec.js.coffee b/spec/javascripts/unit/bulk_product_update_spec.js.coffee
index 4381bec9e1..769f6cf8c8 100644
--- a/spec/javascripts/unit/bulk_product_update_spec.js.coffee
+++ b/spec/javascripts/unit/bulk_product_update_spec.js.coffee
@@ -427,12 +427,24 @@ describe "AdminProductEditCtrl", ->
         scope.loadVariantUnit product
         expect(product.variant_unit_with_scale).toEqual "items"
 
+    it "loads data for variants (inc. master)", ->
+      spyOn scope, "loadVariantVariantUnit"
+
+      product =
+        variant_unit_scale: 1.0
+        master: {id: 1, unit_value: 1, unit_description: '(one)'}
+        variants: [{id: 2, unit_value: 2, unit_description: '(two)'}]
+      scope.loadVariantUnit product
+
+      expect(scope.loadVariantVariantUnit).toHaveBeenCalledWith product, product.variants[0]
+      expect(scope.loadVariantVariantUnit).toHaveBeenCalledWith product, product.master
+
     describe "setting variant unit_value_with_description", ->
       it "sets by combining unit_value and unit_description", ->
         product =
           variant_unit_scale: 1.0
           variants: [{id: 1, unit_value: 1, unit_description: '(bottle)'}]
-        scope.loadVariantUnit product
+        scope.loadVariantVariantUnit product, product.variants[0]
         expect(product.variants[0]).toEqual
           id: 1
           unit_value: 1
@@ -443,21 +455,21 @@ describe "AdminProductEditCtrl", ->
         product =
           variant_unit_scale: 1.0
           variants: [{id: 1, unit_value: 1}]
-        scope.loadVariantUnit product
+        scope.loadVariantVariantUnit product, product.variants[0]
         expect(product.variants[0].unit_value_with_description).toEqual '1'
 
       it "uses unit_description when value is missing", ->
         product =
           variant_unit_scale: 1.0
           variants: [{id: 1, unit_description: 'Small'}]
-        scope.loadVariantUnit product
+        scope.loadVariantVariantUnit product, product.variants[0]
         expect(product.variants[0].unit_value_with_description).toEqual 'Small'
 
       it "converts values from base value to chosen unit", ->
         product =
           variant_unit_scale: 1000.0
           variants: [{id: 1, unit_value: 2500}]
-        scope.loadVariantUnit product
+        scope.loadVariantVariantUnit product, product.variants[0]
         expect(product.variants[0].unit_value_with_description).toEqual '2.5'
 
 
@@ -617,6 +629,17 @@ describe "AdminProductEditCtrl", ->
           variant_unit_scale: null
           variant_unit_with_scale: 'items'
 
+      it "packs the master variant", ->
+        spyOn scope, "packVariant"
+        testVariant = {id: 1}
+        testProduct =
+          id: 1
+          master: testVariant
+
+        scope.packProduct(testProduct)
+
+        expect(scope.packVariant).toHaveBeenCalledWith(testProduct, testVariant)
+
       it "packs each variant", ->
         spyOn scope, "packVariant"
         testVariant = {id: 1}

From e845c0dc0647517e122b5ccc7420b29998925d18 Mon Sep 17 00:00:00 2001
From: Rohan Mitchell <rohan@rohanmitchell.com>
Date: Tue, 11 Feb 2014 19:20:46 +1100
Subject: [PATCH 03/21] Remove derived attributes from master variant

---
 .../javascripts/admin/bulk_product_update.js.coffee      | 3 +++
 spec/javascripts/unit/bulk_product_update_spec.js.coffee | 9 +++++++++
 2 files changed, 12 insertions(+)

diff --git a/app/assets/javascripts/admin/bulk_product_update.js.coffee b/app/assets/javascripts/admin/bulk_product_update.js.coffee
index 3d13e0423c..734b3d0db0 100644
--- a/app/assets/javascripts/admin/bulk_product_update.js.coffee
+++ b/app/assets/javascripts/admin/bulk_product_update.js.coffee
@@ -401,6 +401,9 @@ productEditModule.controller "AdminProductEditCtrl", [
               delete variant.unit_value_with_description
               # If we end up live-updating this field, we might want to reinstate its verification here
               delete variant.options_text
+          if product.master
+            delete product.master.unit_value_with_description
+            delete product.master.options_text
       products_filtered
 
 
diff --git a/spec/javascripts/unit/bulk_product_update_spec.js.coffee b/spec/javascripts/unit/bulk_product_update_spec.js.coffee
index 769f6cf8c8..90d80dd295 100644
--- a/spec/javascripts/unit/bulk_product_update_spec.js.coffee
+++ b/spec/javascripts/unit/bulk_product_update_spec.js.coffee
@@ -850,6 +850,15 @@ describe "AdminProductEditCtrl", ->
           }
         ]
 
+      it "returns master variant without the unit_value_with_description field", ->
+        scope.products = [{id: 123, master: {id: 234, unit_value_with_description: 'foo'}}]
+        expect(scope.productsWithoutDerivedAttributes(scope.products)).toEqual [
+          {
+            id: 123
+            master: {id: 234}
+          }
+        ]
+
 
   describe "deep copying products", ->
     it "copies products", ->

From b2ad6c7d577fe55fc0fed9d52e02deede6500884 Mon Sep 17 00:00:00 2001
From: Rohan Mitchell <rohan@rohanmitchell.com>
Date: Tue, 11 Feb 2014 19:23:27 +1100
Subject: [PATCH 04/21] Filter products for submit includes master variant

---
 .../admin/bulk_product_update.js.coffee       | 48 ++++++++++++-------
 .../unit/bulk_product_update_spec.js.coffee   |  8 ++++
 2 files changed, 39 insertions(+), 17 deletions(-)

diff --git a/app/assets/javascripts/admin/bulk_product_update.js.coffee b/app/assets/javascripts/admin/bulk_product_update.js.coffee
index 734b3d0db0..14ff9557b4 100644
--- a/app/assets/javascripts/admin/bulk_product_update.js.coffee
+++ b/app/assets/javascripts/admin/bulk_product_update.js.coffee
@@ -466,6 +466,7 @@ productEditModule.filter "rangeArray", ->
     input.push(i) for i in [start..end]
     input
 
+
 filterSubmitProducts = (productsToFilter) ->
   filteredProducts = []
   if productsToFilter instanceof Object
@@ -475,23 +476,16 @@ filterSubmitProducts = (productsToFilter) ->
         filteredVariants = []
         if product.hasOwnProperty("variants")
           angular.forEach product.variants, (variant) ->
-            if not variant.deleted_at? and variant.hasOwnProperty("id")
-              hasUpdateableProperty = false
-              filteredVariant = {}
-              filteredVariant.id = variant.id
-              if variant.hasOwnProperty("on_hand")
-                filteredVariant.on_hand = variant.on_hand
-                hasUpdatableProperty = true
-              if variant.hasOwnProperty("price")
-                filteredVariant.price = variant.price
-                hasUpdatableProperty = true
-              if variant.hasOwnProperty("unit_value")
-                filteredVariant.unit_value = variant.unit_value
-                hasUpdatableProperty = true
-              if variant.hasOwnProperty("unit_description")
-                filteredVariant.unit_description = variant.unit_description
-                hasUpdatableProperty = true
-              filteredVariants.push filteredVariant  if hasUpdatableProperty
+            result = filterSubmitVariant variant
+            filteredVariant = result.filteredVariant
+            hasUpdatableProperty = result.hasUpdatableProperty
+            filteredVariants.push filteredVariant  if hasUpdatableProperty
+
+        if product.hasOwnProperty("master")
+          result = filterSubmitVariant product.master
+          filteredMaster = result.filteredVariant
+          hasUpdatableProperty = result.hasUpdatableProperty
+          filteredProduct.master = filteredMaster if hasUpdatableProperty
 
         hasUpdatableProperty = false
         filteredProduct.id = product.id
@@ -525,6 +519,26 @@ filterSubmitProducts = (productsToFilter) ->
   filteredProducts
 
 
+filterSubmitVariant = (variant) ->
+  hasUpdatableProperty = false
+  filteredVariant = {}
+  if not variant.deleted_at? and variant.hasOwnProperty("id")
+    filteredVariant.id = variant.id
+    if variant.hasOwnProperty("on_hand")
+      filteredVariant.on_hand = variant.on_hand
+      hasUpdatableProperty = true
+    if variant.hasOwnProperty("price")
+      filteredVariant.price = variant.price
+      hasUpdatableProperty = true
+    if variant.hasOwnProperty("unit_value")
+      filteredVariant.unit_value = variant.unit_value
+      hasUpdatableProperty = true
+    if variant.hasOwnProperty("unit_description")
+      filteredVariant.unit_description = variant.unit_description
+      hasUpdatableProperty = true
+  {filteredVariant: filteredVariant, hasUpdatableProperty: hasUpdatableProperty}
+
+
 addDirtyProperty = (dirtyObjects, objectID, parsedPropertyName, propertyValue) ->
   if !dirtyObjects.hasOwnProperty(objectID)
     dirtyObjects[objectID] = {}
diff --git a/spec/javascripts/unit/bulk_product_update_spec.js.coffee b/spec/javascripts/unit/bulk_product_update_spec.js.coffee
index 90d80dd295..bec7e8b8d3 100644
--- a/spec/javascripts/unit/bulk_product_update_spec.js.coffee
+++ b/spec/javascripts/unit/bulk_product_update_spec.js.coffee
@@ -171,6 +171,10 @@ describe "filtering products for submission to database", ->
       group_buy: null
       group_buy_unit_size: null
       on_demand: false
+      master:
+        id: 2
+        unit_value: 250
+        unit_description: "foo"
       variants: [
         id: 1
         on_hand: 2
@@ -191,6 +195,10 @@ describe "filtering products for submission to database", ->
       variant_unit_scale: 1
       variant_unit_name: 'loaf'
       available_on: available_on
+      master:
+        id: 2
+        unit_value: 250
+        unit_description: "foo"
       variants_attributes: [
         id: 1
         on_hand: 2

From 4aa43cfbe039d12b7af161b56d66bf1c49593cbd Mon Sep 17 00:00:00 2001
From: Rohan Mitchell <rohan@rohanmitchell.com>
Date: Wed, 12 Feb 2014 13:57:49 +1100
Subject: [PATCH 05/21] Set the master unit value for a product without
 variants

---
 .../admin/bulk_product_update.js.coffee       | 30 +++++++------
 app/models/spree/product_decorator.rb         |  3 +-
 .../spree/admin/products/bulk_edit.html.haml  |  4 +-
 .../spree/api/products/bulk_show.v1.rabl      |  7 ++-
 .../admin/bulk_product_update_spec.rb         | 44 ++++++++++++++++++-
 .../unit/bulk_product_update_spec.js.coffee   |  9 ++--
 6 files changed, 73 insertions(+), 24 deletions(-)

diff --git a/app/assets/javascripts/admin/bulk_product_update.js.coffee b/app/assets/javascripts/admin/bulk_product_update.js.coffee
index 14ff9557b4..6ef5b2a267 100644
--- a/app/assets/javascripts/admin/bulk_product_update.js.coffee
+++ b/app/assets/javascripts/admin/bulk_product_update.js.coffee
@@ -348,6 +348,10 @@ productEditModule.controller "AdminProductEditCtrl", [
           $scope.resetProducts data
           $timeout -> $scope.displaySuccess()
         else
+          # console.log angular.toJson($scope.productsWithoutDerivedAttributes($scope.products))
+          # console.log "---"
+          # console.log angular.toJson($scope.productsWithoutDerivedAttributes(data))
+          # console.log "---"
           $scope.displayFailure "Product lists do not match."
       ).error (data, status) ->
         $scope.displayFailure "Server returned with error status: " + status
@@ -365,6 +369,7 @@ productEditModule.controller "AdminProductEditCtrl", [
       else
         $scope.setMessage $scope.updateStatusMessage, "No changes to update.", color: "grey", 3000
 
+
     $scope.packProduct = (product) ->
       if product.variant_unit_with_scale
         match = product.variant_unit_with_scale.match(/^([^_]+)_([\d\.]+)$/)
@@ -401,9 +406,7 @@ productEditModule.controller "AdminProductEditCtrl", [
               delete variant.unit_value_with_description
               # If we end up live-updating this field, we might want to reinstate its verification here
               delete variant.options_text
-          if product.master
-            delete product.master.unit_value_with_description
-            delete product.master.options_text
+          delete product.master
       products_filtered
 
 
@@ -472,23 +475,24 @@ filterSubmitProducts = (productsToFilter) ->
   if productsToFilter instanceof Object
     angular.forEach productsToFilter, (product) ->
       if product.hasOwnProperty("id")
-        filteredProduct = {}
+        filteredProduct = {id: product.id}
         filteredVariants = []
+        hasUpdatableProperty = false
+
         if product.hasOwnProperty("variants")
           angular.forEach product.variants, (variant) ->
             result = filterSubmitVariant variant
             filteredVariant = result.filteredVariant
-            hasUpdatableProperty = result.hasUpdatableProperty
-            filteredVariants.push filteredVariant  if hasUpdatableProperty
+            variantHasUpdatableProperty = result.hasUpdatableProperty
+            filteredVariants.push filteredVariant  if variantHasUpdatableProperty
 
-        if product.hasOwnProperty("master")
-          result = filterSubmitVariant product.master
-          filteredMaster = result.filteredVariant
-          hasUpdatableProperty = result.hasUpdatableProperty
-          filteredProduct.master = filteredMaster if hasUpdatableProperty
+        if product.master?.hasOwnProperty("unit_value")
+          filteredProduct.unit_value = product.master.unit_value
+          hasUpdatableProperty = true
+        if product.master?.hasOwnProperty("unit_description")
+          filteredProduct.unit_description = product.master.unit_description
+          hasUpdatableProperty = true
 
-        hasUpdatableProperty = false
-        filteredProduct.id = product.id
         if product.hasOwnProperty("name")
           filteredProduct.name = product.name
           hasUpdatableProperty = true
diff --git a/app/models/spree/product_decorator.rb b/app/models/spree/product_decorator.rb
index 3f570bdd2f..33ccf73a81 100644
--- a/app/models/spree/product_decorator.rb
+++ b/app/models/spree/product_decorator.rb
@@ -11,8 +11,9 @@ Spree::Product.class_eval do
   has_many :distributors, :through => :product_distributions
 
   accepts_nested_attributes_for :product_distributions, :allow_destroy => true
+  delegate_belongs_to :master, :unit_value, :unit_description
 
-  attr_accessible :supplier_id, :distributor_ids, :product_distributions_attributes, :group_buy, :group_buy_unit_size, :variant_unit, :variant_unit_scale, :variant_unit_name, :notes
+  attr_accessible :supplier_id, :distributor_ids, :product_distributions_attributes, :group_buy, :group_buy_unit_size, :variant_unit, :variant_unit_scale, :variant_unit_name, :unit_value, :unit_description, :notes
 
   validates_presence_of :supplier
 
diff --git a/app/views/spree/admin/products/bulk_edit.html.haml b/app/views/spree/admin/products/bulk_edit.html.haml
index e2ae2dcee4..68e73e5da3 100644
--- a/app/views/spree/admin/products/bulk_edit.html.haml
+++ b/app/views/spree/admin/products/bulk_edit.html.haml
@@ -117,7 +117,9 @@
             %input{ 'ng-model' => "product.name", :name => 'product_name', 'ofn-track-product' => 'name', :type => 'text' }
           %td.unit{ 'ng-show' => 'columns.unit.visible' }
             %select.select2{ 'ng-model' => 'product.variant_unit_with_scale', :name => 'variant_unit_with_scale', 'ofn-track-product' => 'variant_unit_with_scale', 'ng-options' => 'unit[1] as unit[0] for unit in variant_unit_options' }
-            %input{ 'ng-model' => 'product.variant_unit_name', :name => 'variant_unit_name', 'ofn-track-product' => 'variant_unit_name', 'ng-show' => "product.variant_unit_with_scale == 'items'", :type => 'text' }
+              %option{'value' => '', 'ng-hide' => "hasVariants(product)"}
+            %input{ 'ng-model' => 'product.master.unit_value_with_description', :name => 'master_unit_value_with_description', 'ofn-track-product' => 'master.unit_value_with_description', :type => 'text', :placeholder => 'value', 'ng-show' => "product.variant_unit_with_scale != undefined && !hasVariants(product)" }
+            %input{ 'ng-model' => 'product.variant_unit_name', :name => 'variant_unit_name', 'ofn-track-product' => 'variant_unit_name', :placeholder => 'unit', 'ng-show' => "product.variant_unit_with_scale == 'items'", :type => 'text' }
           %td{ 'ng-show' => 'columns.price.visible' }
             %input{ 'ng-model' => 'product.price', 'ofn-decimal' => :true, :name => 'price', 'ofn-track-product' => 'price', :type => 'text' }
           %td{ 'ng-show' => 'columns.on_hand.visible' }
diff --git a/app/views/spree/api/products/bulk_show.v1.rabl b/app/views/spree/api/products/bulk_show.v1.rabl
index 1f07d3d91f..6219bc193e 100644
--- a/app/views/spree/api/products/bulk_show.v1.rabl
+++ b/app/views/spree/api/products/bulk_show.v1.rabl
@@ -7,8 +7,11 @@ node( :on_hand ) { |p| p.on_hand.to_f.finite? ? p.on_hand : "On demand" }
 node( :available_on ) { |p| p.available_on.blank? ? "" : p.available_on.strftime("%F %T") }
 node( :permalink_live ) { |p| p.permalink }
 node( :supplier ) do |p|
-	partial 'spree/api/enterprises/bulk_show', :object => p.supplier
+  partial 'spree/api/enterprises/bulk_show', :object => p.supplier
 end
 node( :variants ) do |p|
-	partial 'spree/api/variants/bulk_index', :object => p.variants.order('id ASC')
+  partial 'spree/api/variants/bulk_index', :object => p.variants.order('id ASC')
+end
+node( :master ) do |p|
+  partial 'spree/api/variants/bulk_show', :object => p.master
 end
diff --git a/spec/features/admin/bulk_product_update_spec.rb b/spec/features/admin/bulk_product_update_spec.rb
index 352c3d14c7..fb57fa7254 100644
--- a/spec/features/admin/bulk_product_update_spec.rb
+++ b/spec/features/admin/bulk_product_update_spec.rb
@@ -295,7 +295,7 @@ feature %q{
     page.should have_field "on_hand", with: "18"
   end
   
-  scenario "updating a product with an items variant unit" do
+  scenario "updating a product with a variant unit of 'items'" do
     p = FactoryGirl.create(:product, variant_unit: 'weight', variant_unit_scale: 1000)
 
     login_to_admin_section
@@ -340,6 +340,48 @@ feature %q{
     page.should have_field "variant_unit_value_with_description", with: "123 abc"
   end
 
+  describe "setting the master unit value for a product without variants" do
+    it "sets the master unit value" do
+      p = FactoryGirl.create(:product, variant_unit: nil, variant_unit_scale: nil)
+
+      login_to_admin_section
+
+      visit '/admin/products/bulk_edit'
+
+      page.should have_select "variant_unit_with_scale", selected: ''
+      page.should_not have_field "master_unit_value_with_description", visible: true
+
+      select "Weight (kg)", from: "variant_unit_with_scale"
+      fill_in "master_unit_value_with_description", with: '123 abc'
+
+      click_button 'Update'
+      page.find("span#update-status-message").should have_content "Update complete"
+
+      visit '/admin/products/bulk_edit'
+
+      page.should have_select "variant_unit_with_scale", selected: "Weight (kg)"
+      page.should have_field "master_unit_value_with_description", with: "123 abc"
+
+      p.reload
+      p.variant_unit.should == 'weight'
+      p.variant_unit_scale.should == 1000
+      p.master.unit_value.should == 123000
+      p.master.unit_description.should == 'abc'
+    end
+
+    it "does not show the field when the product has variants" do
+      p = FactoryGirl.create(:product, variant_unit: nil, variant_unit_scale: nil)
+      v = FactoryGirl.create(:variant, product: p, unit_value: nil, unit_description: nil)
+
+      login_to_admin_section
+
+      visit '/admin/products/bulk_edit'
+
+      select "Weight (kg)", from: "variant_unit_with_scale"
+      page.should_not have_field "master_unit_value_with_description", visible: true
+    end
+  end
+
 
   scenario "updating a product with variants" do
     s1 = FactoryGirl.create(:supplier_enterprise)
diff --git a/spec/javascripts/unit/bulk_product_update_spec.js.coffee b/spec/javascripts/unit/bulk_product_update_spec.js.coffee
index bec7e8b8d3..604fb4e9b4 100644
--- a/spec/javascripts/unit/bulk_product_update_spec.js.coffee
+++ b/spec/javascripts/unit/bulk_product_update_spec.js.coffee
@@ -194,11 +194,9 @@ describe "filtering products for submission to database", ->
       variant_unit: 'volume'
       variant_unit_scale: 1
       variant_unit_name: 'loaf'
+      unit_value: 250
+      unit_description: "foo"
       available_on: available_on
-      master:
-        id: 2
-        unit_value: 250
-        unit_description: "foo"
       variants_attributes: [
         id: 1
         on_hand: 2
@@ -858,12 +856,11 @@ describe "AdminProductEditCtrl", ->
           }
         ]
 
-      it "returns master variant without the unit_value_with_description field", ->
+      it "removes the master variant", ->
         scope.products = [{id: 123, master: {id: 234, unit_value_with_description: 'foo'}}]
         expect(scope.productsWithoutDerivedAttributes(scope.products)).toEqual [
           {
             id: 123
-            master: {id: 234}
           }
         ]
 

From 4d24fec6fb32cf99367ac48c4459ca9485478306 Mon Sep 17 00:00:00 2001
From: Rohan Mitchell <rohan@rohanmitchell.com>
Date: Thu, 13 Feb 2014 09:26:07 +1100
Subject: [PATCH 06/21] BPE: Add a variant. Do not show edit on unsaved
 variants. Delete unsaved variants.

---
 .../admin/bulk_product_update.js.coffee       |  41 +++-
 .../spree/admin/products/bulk_edit.html.haml  |   8 +-
 .../admin/bulk_product_update_spec.rb         |  42 +++-
 .../unit/bulk_product_update_spec.js.coffee   | 223 ++++++++++++------
 4 files changed, 230 insertions(+), 84 deletions(-)

diff --git a/app/assets/javascripts/admin/bulk_product_update.js.coffee b/app/assets/javascripts/admin/bulk_product_update.js.coffee
index 6ef5b2a267..eb809d084f 100644
--- a/app/assets/javascripts/admin/bulk_product_update.js.coffee
+++ b/app/assets/javascripts/admin/bulk_product_update.js.coffee
@@ -285,6 +285,17 @@ productEditModule.controller "AdminProductEditCtrl", [
         window.location = "/admin/products/" + product.permalink_live + ((if variant then "/variants/" + variant.id else "")) + "/edit"
 
 
+    $scope.addVariant = (product) ->
+      product.variants.push {id: $scope.nextVariantId()}
+      $scope.displayProperties[product.id].showVariants = true
+
+
+    $scope.nextVariantId = ->
+      $scope.variantIdCounter = 0 unless $scope.variantIdCounter?
+      $scope.variantIdCounter -= 1
+      $scope.variantIdCounter
+
+
     $scope.deleteProduct = (product) ->
       if confirm("Are you sure?")
         $http(
@@ -297,14 +308,20 @@ productEditModule.controller "AdminProductEditCtrl", [
 
 
     $scope.deleteVariant = (product, variant) ->
-      if confirm("Are you sure?")
-        $http(
-          method: "DELETE"
-          url: "/api/products/" + product.id + "/variants/" + variant.id
-        ).success (data) ->
-          product.variants.splice product.variants.indexOf(variant), 1
-          delete $scope.dirtyProducts[product.id].variants[variant.id]  if $scope.dirtyProducts.hasOwnProperty(product.id) and $scope.dirtyProducts[product.id].hasOwnProperty("variants") and $scope.dirtyProducts[product.id].variants.hasOwnProperty(variant.id)
-          $scope.displayDirtyProducts()
+      if !$scope.variantSaved(variant)
+        $scope.removeVariant(product, variant)
+      else
+        if confirm("Are you sure?")
+          $http(
+            method: "DELETE"
+            url: "/api/products/" + product.id + "/variants/" + variant.id
+          ).success (data) ->
+            $scope.removeVariant(product, variant)
+
+    $scope.removeVariant = (product, variant) ->
+      product.variants.splice product.variants.indexOf(variant), 1
+      delete $scope.dirtyProducts[product.id].variants[variant.id]  if $scope.dirtyProducts.hasOwnProperty(product.id) and $scope.dirtyProducts[product.id].hasOwnProperty("variants") and $scope.dirtyProducts[product.id].variants.hasOwnProperty(variant.id)
+      $scope.displayDirtyProducts()
 
 
     $scope.cloneProduct = (product) ->
@@ -325,6 +342,14 @@ productEditModule.controller "AdminProductEditCtrl", [
       Object.keys(product.variants).length > 0
 
 
+    $scope.hasUnit = (product) ->
+      product.variant_unit_with_scale?
+
+
+    $scope.variantSaved = (variant) ->
+      variant.hasOwnProperty('id') && variant.id > 0
+
+
     $scope.hasOnDemandVariants = (product) ->
       (variant for id, variant of product.variants when variant.on_demand).length > 0
 
diff --git a/app/views/spree/admin/products/bulk_edit.html.haml b/app/views/spree/admin/products/bulk_edit.html.haml
index 68e73e5da3..28cefe564f 100644
--- a/app/views/spree/admin/products/bulk_edit.html.haml
+++ b/app/views/spree/admin/products/bulk_edit.html.haml
@@ -111,6 +111,7 @@
         %tr.product
           %td.left-actions
             %a{ 'ofn-toggle-variants' => 'true', :class => "view-variants icon-chevron-right", 'ng-show' => 'hasVariants(product)' }
+            %a{ :class => "add-variant icon-plus-sign", 'ng-click' => "addVariant(product)", 'ng-show' => "!hasVariants(product) && hasUnit(product)" }
           %td.supplier{ 'ng-show' => 'columns.supplier.visible' }
             %select.select2{ 'ng-model' => 'product.supplier', :name => 'supplier', 'ofn-track-product' => 'supplier', 'ng-options' => 's.name for s in suppliers' }
           %td{ 'ng-show' => 'columns.name.visible' }
@@ -118,7 +119,7 @@
           %td.unit{ 'ng-show' => 'columns.unit.visible' }
             %select.select2{ 'ng-model' => 'product.variant_unit_with_scale', :name => 'variant_unit_with_scale', 'ofn-track-product' => 'variant_unit_with_scale', 'ng-options' => 'unit[1] as unit[0] for unit in variant_unit_options' }
               %option{'value' => '', 'ng-hide' => "hasVariants(product)"}
-            %input{ 'ng-model' => 'product.master.unit_value_with_description', :name => 'master_unit_value_with_description', 'ofn-track-product' => 'master.unit_value_with_description', :type => 'text', :placeholder => 'value', 'ng-show' => "product.variant_unit_with_scale != undefined && !hasVariants(product)" }
+            %input{ 'ng-model' => 'product.master.unit_value_with_description', :name => 'master_unit_value_with_description', 'ofn-track-product' => 'master.unit_value_with_description', :type => 'text', :placeholder => 'value', 'ng-show' => "!hasVariants(product) && hasUnit(product)" }
             %input{ 'ng-model' => 'product.variant_unit_name', :name => 'variant_unit_name', 'ofn-track-product' => 'variant_unit_name', :placeholder => 'unit', 'ng-show' => "product.variant_unit_with_scale == 'items'", :type => 'text' }
           %td{ 'ng-show' => 'columns.price.visible' }
             %input{ 'ng-model' => 'product.price', 'ofn-decimal' => :true, :name => 'price', 'ofn-track-product' => 'price', :type => 'text' }
@@ -135,7 +136,8 @@
             %a{ 'ng-click' => 'deleteProduct(product)', :class => "delete-product icon-trash no-text" }
         %tr.variant{ 'ng-repeat' => 'variant in product.variants', 'ng-show' => 'displayProperties[product.id].showVariants', 'ng-class-even' => "'even'", 'ng-class-odd' => "'odd'" }
           %td.left-actions
-            %a{ :class => "variant-item icon-caret-right" }
+            %a{ :class => "variant-item icon-caret-right", 'ng-hide' => "$last" }
+            %a{ :class => "add-variant icon-plus-sign", 'ng-click' => "addVariant(product)", 'ng-show' => "$last" }
           %td{ 'ng-show' => 'columns.supplier.visible' }
           %td{ 'ng-show' => 'columns.name.visible' }
             {{ variant.options_text }}
@@ -148,7 +150,7 @@
             %span{ 'ng-bind' => 'variant.on_hand', :name => 'variant_on_hand', 'ng-show' => 'variant.on_demand' }
           %td{ 'ng-show' => 'columns.available_on.visible' }
           %td.actions
-            %a{ 'ng-click' => 'editWarn(product,variant)', :class => "edit-variant icon-edit no-text" }
+            %a{ 'ng-click' => 'editWarn(product,variant)', :class => "edit-variant icon-edit no-text", 'ng-show' => "variantSaved(variant)" }
           %td.actions
           %td.actions
             %a{ 'ng-click' => 'deleteVariant(product,variant)', :class => "delete-variant icon-trash no-text" }
diff --git a/spec/features/admin/bulk_product_update_spec.rb b/spec/features/admin/bulk_product_update_spec.rb
index fb57fa7254..0ff07ca0ab 100644
--- a/spec/features/admin/bulk_product_update_spec.rb
+++ b/spec/features/admin/bulk_product_update_spec.rb
@@ -229,7 +229,8 @@ feature %q{
     end
   end
 
-  scenario "create a new product" do
+
+  scenario "creating a new product" do
     s = FactoryGirl.create(:supplier_enterprise)
     d = FactoryGirl.create(:distributor_enterprise)
 
@@ -253,6 +254,44 @@ feature %q{
     page.should have_field "product_name", with: 'Big Bag Of Apples'
   end
 
+
+  scenario "creating new variants" do
+    # Given a product without variants or a unit
+    p = FactoryGirl.create(:product, variant_unit: nil, variant_unit_scale: nil)
+    login_to_admin_section
+    visit '/admin/products/bulk_edit'
+
+    # I should not see an add variant button
+    page.should_not have_selector 'a.add-variant', visible: true
+
+    # When I set the unit
+    select "Weight (kg)", from: "variant_unit_with_scale"
+
+    # I should see an add variant button
+    page.should have_selector 'a.add-variant', visible: true
+
+    # When I add three variants
+    page.find('a.add-variant', visible: true).click
+    page.find('a.add-variant', visible: true).click
+    page.find('a.add-variant', visible: true).click
+
+    # They should be added, and should see no edit buttons
+    page.all("tr.variant").count.should == 3
+    page.should_not have_selector "a.edit-variant", visible: true
+
+    # When I remove one, it should be removed
+    page.all('a.delete-variant').first.click
+    page.all("tr.variant").count.should == 2
+
+    # When I fill out variant details and hit update
+
+    # Then the variants should be saved
+
+    # And I should see edit buttons for the new variants
+
+  end
+
+
   scenario "updating a product with no variants (except master)" do
     s1 = FactoryGirl.create(:supplier_enterprise)
     s2 = FactoryGirl.create(:supplier_enterprise)
@@ -340,6 +379,7 @@ feature %q{
     page.should have_field "variant_unit_value_with_description", with: "123 abc"
   end
 
+
   describe "setting the master unit value for a product without variants" do
     it "sets the master unit value" do
       p = FactoryGirl.create(:product, variant_unit: nil, variant_unit_scale: nil)
diff --git a/spec/javascripts/unit/bulk_product_update_spec.js.coffee b/spec/javascripts/unit/bulk_product_update_spec.js.coffee
index 604fb4e9b4..f87c13a21a 100644
--- a/spec/javascripts/unit/bulk_product_update_spec.js.coffee
+++ b/spec/javascripts/unit/bulk_product_update_spec.js.coffee
@@ -603,6 +603,43 @@ describe "AdminProductEditCtrl", ->
       expect(scope.hasOnDemandVariants(product)).toBe(false)
 
 
+  describe "determining whether a product has variants", ->
+    it "returns true when it does", ->
+      product =
+        variants: [{id: 1}, {id: 2}]
+      expect(scope.hasVariants(product)).toBe(true)
+
+    it "returns false when it does not", ->
+      product =
+        variants: []
+      expect(scope.hasVariants(product)).toBe(false)
+
+
+  describe "determining whether a product has a unit", ->
+    it "returns true when it does", ->
+      product =
+        variant_unit_with_scale: 'weight_1000'
+      expect(scope.hasUnit(product)).toBe(true)
+
+    it "returns false when its unit is undefined", ->
+      product = {}
+      expect(scope.hasUnit(product)).toBe(false)
+
+
+  describe "determining whether a variant has been saved", ->
+    it "returns true when it has a positive id", ->
+      variant = {id: 1}
+      expect(scope.variantSaved(variant)).toBe(true)
+
+    it "returns false when it has no id", ->
+      variant = {}
+      expect(scope.variantSaved(variant)).toBe(false)
+
+    it "returns false when it has a negative id", ->
+      variant = {id: -1}
+      expect(scope.variantSaved(variant)).toBe(false)
+
+
   describe "submitting products to be updated", ->
     describe "packing products", ->
       it "extracts variant_unit_with_scale into variant_unit and variant_unit_scale", ->
@@ -889,6 +926,31 @@ describe "AdminProductEditCtrl", ->
       expect(scope.findProduct(123)).toBeNull()
 
 
+  describe "adding variants", ->
+    beforeEach ->
+      scope.displayProperties ||= {123: {}}
+
+    it "adds the first variant", ->
+      product = {id: 123, variants: []}
+      scope.addVariant(product)
+      expect(product).toEqual
+        id: 123
+        variants: [{id: -1}]
+
+    it "adds subsequent variants", ->
+      product = {id: 123, variants: []}
+      scope.addVariant(product)
+      scope.addVariant(product)
+      expect(product).toEqual
+        id: 123
+        variants: [{id: -1}, {id: -2}]
+
+    it "shows the variant(s)", ->
+      product = {id: 123, variants: []}
+      scope.addVariant(product)
+      expect(scope.displayProperties[123].showVariants).toBe(true)
+
+
   describe "deleting products", ->
     it "deletes products with a http delete request to /api/products/id", ->
       spyOn(window, "confirm").andReturn true
@@ -942,83 +1004,100 @@ describe "AdminProductEditCtrl", ->
 
 
   describe "deleting variants", ->
-    it "deletes variants with a http delete request to /api/products/product_id/variants/(variant_id)", ->
-      spyOn(window, "confirm").andReturn true
-      scope.products = [
-        {
-          id: 9
-          permalink_live: "apples"
-          variants: [
-            id: 3
-            price: 12
-          ]
-        }
-        {
-          id: 13
-          permalink_live: "oranges"
-        }
-      ]
-      scope.dirtyProducts = {}
-      httpBackend.expectDELETE("/api/products/9/variants/3").respond 200, "data"
-      scope.deleteVariant scope.products[0], scope.products[0].variants[0]
-      httpBackend.flush()
+    describe "when the variant has not been saved", ->
+      it "removes the variant from products and dirtyProducts", ->
+        spyOn(window, "confirm").andReturn true
+        scope.products = [
+          {id: 1, variants: [{id: -1}]}
+        ]
+        scope.dirtyProducts =
+          1: {id: 1, variants: {'-1': {id: -1}}}
+        scope.deleteVariant scope.products[0], scope.products[0].variants[0]
+        expect(scope.products).toEqual([
+          {id: 1, variants: []}
+        ])
+        expect(scope.dirtyProducts).toEqual
+          1: {id: 1, variants: {}}
 
-    it "removes the specified variant from both the variants object and scope.dirtyProducts (if it exists there)", ->
-      spyOn(window, "confirm").andReturn true
-      scope.products = [
-        {
-          id: 9
-          permalink_live: "apples"
-          variants: [
-            {
+  
+    describe "when the variant has been saved", ->
+      it "deletes variants with a http delete request to /api/products/product_id/variants/(variant_id)", ->
+        spyOn(window, "confirm").andReturn true
+        scope.products = [
+          {
+            id: 9
+            permalink_live: "apples"
+            variants: [
               id: 3
-              price: 12.0
-            }
-            {
-              id: 4
-              price: 6.0
-            }
-          ]
-        }
-        {
-          id: 13
-          permalink_live: "oranges"
-        }
-      ]
-      scope.dirtyProducts =
-        9:
-          id: 9
-          variants:
-            3:
-              id: 3
-              price: 12.0
+              price: 12
+            ]
+          }
+          {
+            id: 13
+            permalink_live: "oranges"
+          }
+        ]
+        scope.dirtyProducts = {}
+        httpBackend.expectDELETE("/api/products/9/variants/3").respond 200, "data"
+        scope.deleteVariant scope.products[0], scope.products[0].variants[0]
+        httpBackend.flush()
 
-            4:
-              id: 4
-              price: 6.0
+      it "removes the specified variant from both the variants object and scope.dirtyProducts (if it exists there)", ->
+        spyOn(window, "confirm").andReturn true
+        scope.products = [
+          {
+            id: 9
+            permalink_live: "apples"
+            variants: [
+              {
+                id: 3
+                price: 12.0
+              }
+              {
+                id: 4
+                price: 6.0
+              }
+            ]
+          }
+          {
+            id: 13
+            permalink_live: "oranges"
+          }
+        ]
+        scope.dirtyProducts =
+          9:
+            id: 9
+            variants:
+              3:
+                id: 3
+                price: 12.0
+  
+              4:
+                id: 4
+                price: 6.0
+  
+          13:
+            id: 13
+            name: "P1"
 
-        13:
-          id: 13
-          name: "P1"
+        httpBackend.expectDELETE("/api/products/9/variants/3").respond 200, "data"
+        scope.deleteVariant scope.products[0], scope.products[0].variants[0]
+        httpBackend.flush()
+        expect(scope.products[0].variants).toEqual [
+          id: 4
+          price: 6.0
+        ]
+        expect(scope.dirtyProducts).toEqual
+          9:
+            id: 9
+            variants:
+              4:
+                id: 4
+                price: 6.0
 
-      httpBackend.expectDELETE("/api/products/9/variants/3").respond 200, "data"
-      scope.deleteVariant scope.products[0], scope.products[0].variants[0]
-      httpBackend.flush()
-      expect(scope.products[0].variants).toEqual [
-        id: 4
-        price: 6.0
-      ]
-      expect(scope.dirtyProducts).toEqual
-        9:
-          id: 9
-          variants:
-            4:
-              id: 4
-              price: 6.0
-
-        13:
-          id: 13
-          name: "P1"
+          13:
+            id: 13
+            name: "P1"
 
 
 

From 701896be95fcf4c48b1e1dc16845b6ef20c39552 Mon Sep 17 00:00:00 2001
From: Rohan Mitchell <rohan@rohanmitchell.com>
Date: Thu, 13 Feb 2014 11:52:44 +1100
Subject: [PATCH 07/21] BPE: Add variant and save it to server. Edit the saved
 variant.

---
 .../admin/bulk_product_update.js.coffee       | 56 +++++++++++++------
 app/models/spree/product_set.rb               |  6 +-
 .../admin/bulk_product_update_spec.rb         | 21 +++++--
 .../unit/bulk_product_update_spec.js.coffee   | 53 +++++++++++++++---
 4 files changed, 106 insertions(+), 30 deletions(-)

diff --git a/app/assets/javascripts/admin/bulk_product_update.js.coffee b/app/assets/javascripts/admin/bulk_product_update.js.coffee
index eb809d084f..a9735aca4e 100644
--- a/app/assets/javascripts/admin/bulk_product_update.js.coffee
+++ b/app/assets/javascripts/admin/bulk_product_update.js.coffee
@@ -286,7 +286,13 @@ productEditModule.controller "AdminProductEditCtrl", [
 
 
     $scope.addVariant = (product) ->
-      product.variants.push {id: $scope.nextVariantId()}
+      product.variants.push
+        id: $scope.nextVariantId()
+        price: null
+        unit_value: null
+        unit_description: null
+        on_demand: false
+        on_hand: null
       $scope.displayProperties[product.id].showVariants = true
 
 
@@ -354,6 +360,19 @@ productEditModule.controller "AdminProductEditCtrl", [
       (variant for id, variant of product.variants when variant.on_demand).length > 0
 
 
+    $scope.submitProducts = ->
+      # Pack pack $scope.products, so they will match the list returned from the server,
+      # then pack $scope.dirtyProducts, ensuring that the correct product info is sent to the server.
+      $scope.packProduct product for id, product of $scope.products
+      $scope.packProduct product for id, product of $scope.dirtyProducts
+
+      productsToSubmit = filterSubmitProducts($scope.dirtyProducts)
+      if productsToSubmit.length > 0
+        $scope.updateProducts productsToSubmit # Don't submit an empty list
+      else
+        $scope.setMessage $scope.updateStatusMessage, "No changes to update.", color: "grey", 3000
+
+
     $scope.updateProducts = (productsToSubmit) ->
       $scope.displayUpdating()
       $http(
@@ -369,7 +388,7 @@ productEditModule.controller "AdminProductEditCtrl", [
         #       doing things. TODO: Review together and decide on strategy here. -- Rohan, 14-1-2014
         #if subset($scope.productsWithoutDerivedAttributes(), data)
 
-        if angular.toJson($scope.productsWithoutDerivedAttributes($scope.products)) == angular.toJson($scope.productsWithoutDerivedAttributes(data))
+        if $scope.productListsMatch $scope.products, data
           $scope.resetProducts data
           $timeout -> $scope.displaySuccess()
         else
@@ -382,19 +401,6 @@ productEditModule.controller "AdminProductEditCtrl", [
         $scope.displayFailure "Server returned with error status: " + status
 
 
-    $scope.submitProducts = ->
-      # Pack pack $scope.products, so they will match the list returned from the server,
-      # then pack $scope.dirtyProducts, ensuring that the correct product info is sent to the server.
-      $scope.packProduct product for id, product of $scope.products
-      $scope.packProduct product for id, product of $scope.dirtyProducts
-
-      productsToSubmit = filterSubmitProducts($scope.dirtyProducts)
-      if productsToSubmit.length > 0
-        $scope.updateProducts productsToSubmit # Don't submit an empty list
-      else
-        $scope.setMessage $scope.updateStatusMessage, "No changes to update.", color: "grey", 3000
-
-
     $scope.packProduct = (product) ->
       if product.variant_unit_with_scale
         match = product.variant_unit_with_scale.match(/^([^_]+)_([\d\.]+)$/)
@@ -420,6 +426,24 @@ productEditModule.controller "AdminProductEditCtrl", [
           variant.unit_description = match[3]
 
 
+    $scope.productListsMatch = (clientProducts, serverProducts) ->
+      $scope.copyNewVariantIds clientProducts, serverProducts
+      angular.toJson($scope.productsWithoutDerivedAttributes(clientProducts)) == angular.toJson($scope.productsWithoutDerivedAttributes(serverProducts))
+
+
+    # When variants are created clientside, they are given a negative id. The server
+    # responds with a real id, which would cause the productListsMatch() check to fail.
+    # To avoid that false negative, we copy the server variant id to the client for any
+    # negative ids.
+    $scope.copyNewVariantIds = (clientProducts, serverProducts) ->
+      if clientProducts?
+        for product, i in clientProducts
+          if product.variants?
+            for variant, j in product.variants
+              if variant.id < 0
+                variant.id = serverProducts[i].variants[j].id
+
+
     $scope.productsWithoutDerivedAttributes = (products) ->
       products_filtered = []
       if products
@@ -552,7 +576,7 @@ filterSubmitVariant = (variant) ->
   hasUpdatableProperty = false
   filteredVariant = {}
   if not variant.deleted_at? and variant.hasOwnProperty("id")
-    filteredVariant.id = variant.id
+    filteredVariant.id = variant.id unless variant.id <= 0
     if variant.hasOwnProperty("on_hand")
       filteredVariant.on_hand = variant.on_hand
       hasUpdatableProperty = true
diff --git a/app/models/spree/product_set.rb b/app/models/spree/product_set.rb
index db3095187e..e79408d8c5 100644
--- a/app/models/spree/product_set.rb
+++ b/app/models/spree/product_set.rb
@@ -20,7 +20,11 @@ class Spree::ProductSet < ModelSet
   def update_variants_attributes(product, variants_attributes)
     variants_attributes.each do |attributes|
       e = product.variants.detect { |e| e.id.to_s == attributes[:id].to_s && !e.id.nil? }
-      e.update_attributes(attributes.except(:id)) if e.present?
+      if e.present?
+        e.update_attributes(attributes.except(:id))
+      else
+        product.variants.create attributes
+      end
     end
   end
 
diff --git a/spec/features/admin/bulk_product_update_spec.rb b/spec/features/admin/bulk_product_update_spec.rb
index 0ff07ca0ab..4644aae8c1 100644
--- a/spec/features/admin/bulk_product_update_spec.rb
+++ b/spec/features/admin/bulk_product_update_spec.rb
@@ -279,16 +279,29 @@ feature %q{
     page.all("tr.variant").count.should == 3
     page.should_not have_selector "a.edit-variant", visible: true
 
-    # When I remove one, it should be removed
+    # When I remove two, they should be removed
     page.all('a.delete-variant').first.click
-    page.all("tr.variant").count.should == 2
+    page.all('a.delete-variant').first.click
+    page.all("tr.variant").count.should == 1
 
     # When I fill out variant details and hit update
+    fill_in "variant_unit_value_with_description", with: "4000 (12x250 mL bottles)"
+    fill_in "variant_price", with: "4.0"
+    fill_in "variant_on_hand", with: "10"
+    click_button 'Update'
+    page.find("span#update-status-message").should have_content "Update complete"
 
-    # Then the variants should be saved
+    # Then I should see edit buttons for the new variant
+    page.should have_selector "a.edit-variant", visible: true
 
-    # And I should see edit buttons for the new variants
+    # And the variants should be saved
+    visit '/admin/products/bulk_edit'
+    page.should have_selector "a.view-variants"
+    first("a.view-variants").click
 
+    page.should have_field "variant_unit_value_with_description", with: "4000 (12x250 mL bottles)"
+    page.should have_field "variant_price", with: "4.0"
+    page.should have_field "variant_on_hand", with: "10"
   end
 
 
diff --git a/spec/javascripts/unit/bulk_product_update_spec.js.coffee b/spec/javascripts/unit/bulk_product_update_spec.js.coffee
index f87c13a21a..e0d3f33660 100644
--- a/spec/javascripts/unit/bulk_product_update_spec.js.coffee
+++ b/spec/javascripts/unit/bulk_product_update_spec.js.coffee
@@ -121,6 +121,27 @@ describe "filtering products for submission to database", ->
       ]
     ]
 
+  it "returns variants with a negative id without that id", ->
+    testProduct =
+      id: 1
+      variants: [
+        id: -1
+        on_hand: 5
+        price: 12.0
+        unit_value: 250
+        unit_description: "(bottle)"
+      ]
+
+    expect(filterSubmitProducts([testProduct])).toEqual [
+      id: 1
+      variants_attributes: [
+        on_hand: 5
+        price: 12.0
+        unit_value: 250
+        unit_description: "(bottle)"
+      ]
+    ]
+
   it "does not return variants_attributes property if variants is an empty array", ->
     testProduct =
       id: 1
@@ -862,6 +883,24 @@ describe "AdminProductEditCtrl", ->
         expect(scope.displayFailure).toHaveBeenCalled()
 
 
+  describe "copying new variant ids from server to client", ->
+    it "copies server ids to the client where the client id is negative", ->
+      clientProducts = [
+        {
+          id: 123
+          variants: [{id: 1}, {id: -2}, {id: -3}]
+        }
+      ]
+      serverProducts = [
+        {
+          id: 123
+          variants: [{id: 1}, {id: 4534}, {id: 3453}]
+        }
+      ]
+      scope.copyNewVariantIds(clientProducts, serverProducts)
+      expect(clientProducts).toEqual(serverProducts)
+
+
   describe "fetching products without derived attributes", ->
     it "returns products without the variant_unit_with_scale field", ->
       scope.products = [{id: 123, variant_unit_with_scale: 'weight_1000'}]
@@ -930,20 +969,16 @@ describe "AdminProductEditCtrl", ->
     beforeEach ->
       scope.displayProperties ||= {123: {}}
 
-    it "adds the first variant", ->
-      product = {id: 123, variants: []}
-      scope.addVariant(product)
-      expect(product).toEqual
-        id: 123
-        variants: [{id: -1}]
-
-    it "adds subsequent variants", ->
+    it "adds first and subsequent variants", ->
       product = {id: 123, variants: []}
       scope.addVariant(product)
       scope.addVariant(product)
       expect(product).toEqual
         id: 123
-        variants: [{id: -1}, {id: -2}]
+        variants: [
+          {id: -1, price: null, unit_value: null, unit_description: null, on_demand: false, on_hand: null}
+          {id: -2, price: null, unit_value: null, unit_description: null, on_demand: false, on_hand: null}
+        ]
 
     it "shows the variant(s)", ->
       product = {id: 123, variants: []}

From 134d9831fe6df1d805e0da0e863b8efda2354712 Mon Sep 17 00:00:00 2001
From: Will Marshall <will@willmarshall.me>
Date: Fri, 14 Feb 2014 15:34:55 +1100
Subject: [PATCH 08/21] Fixing bug #271

---
 app/views/shop/_variant.html.haml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/app/views/shop/_variant.html.haml b/app/views/shop/_variant.html.haml
index 0adb5a2421..9a71ac8b39 100644
--- a/app/views/shop/_variant.html.haml
+++ b/app/views/shop/_variant.html.haml
@@ -1,5 +1,6 @@
 
-%td{colspan: 2}
+%td
+%td.notes
 %td {{variant.options_text}}
 %td 
   %input{type: :number, 

From 502dba1b3f7e1718d53289b693d433d08fd7142c Mon Sep 17 00:00:00 2001
From: Will Marshall <will@willmarshall.me>
Date: Fri, 14 Feb 2014 15:35:40 +1100
Subject: [PATCH 09/21] Fixing bug #281

---
 app/assets/stylesheets/darkswarm/shop.css.sass | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/assets/stylesheets/darkswarm/shop.css.sass b/app/assets/stylesheets/darkswarm/shop.css.sass
index 3781b380f8..825d720cbf 100644
--- a/app/assets/stylesheets/darkswarm/shop.css.sass
+++ b/app/assets/stylesheets/darkswarm/shop.css.sass
@@ -122,7 +122,7 @@ shop
         //&.notes
           //width: 140px
         &.variant
-          width: 100px
+          width: 180px
         &.quantity, &.bulk, &.price
           width: 90px
       .notes

From 19e31a264d8da2ee2e3b6080caedae2d6d8157c8 Mon Sep 17 00:00:00 2001
From: Will Marshall <will@willmarshall.me>
Date: Fri, 14 Feb 2014 15:49:16 +1100
Subject: [PATCH 10/21] Tidying footer contact details

---
 app/assets/stylesheets/darkswarm/footer.sass |  4 ++++
 app/views/shop/_contact_us.html.haml         | 22 ++++++++++++++------
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/app/assets/stylesheets/darkswarm/footer.sass b/app/assets/stylesheets/darkswarm/footer.sass
index ba2424e94b..df649a8d59 100644
--- a/app/assets/stylesheets/darkswarm/footer.sass
+++ b/app/assets/stylesheets/darkswarm/footer.sass
@@ -10,3 +10,7 @@
     img
       display: block
       margin: 0px auto 8px
+
+  .contact
+    strong
+      padding-right: 1em
diff --git a/app/views/shop/_contact_us.html.haml b/app/views/shop/_contact_us.html.haml
index 2cb4d65984..58a69ed38f 100644
--- a/app/views/shop/_contact_us.html.haml
+++ b/app/views/shop/_contact_us.html.haml
@@ -1,8 +1,18 @@
 .contact.small-2.large-3.columns
   %h3 Contact
-  %ul
-    %li= @distributor.email
-    %li= @distributor.website
-  = @distributor.address.address1
-  = @distributor.address.address2
-  = @distributor.address.city
+
+  %p
+    %strong E
+    %a{href: "mailto:#{@distributor.email}"}= @distributor.email
+  
+  - unless @distributor.website.blank?
+    %p
+      %strong W
+      %a{href: @distributor.website}= @distributor.website
+
+  %p
+    = @distributor.address.address1
+    %br
+    = @distributor.address.address2
+    %br
+    = @distributor.address.city

From 547f46fbc9d73ba1985b0f17bea698e7f67efab8 Mon Sep 17 00:00:00 2001
From: Rohan Mitchell <rohan@rohanmitchell.com>
Date: Tue, 18 Feb 2014 10:32:36 +1100
Subject: [PATCH 11/21] Deal with unit_value of zero correctly - do not treat
 as nil

---
 .../admin/bulk_product_update.js.coffee       |  8 ++++---
 .../unit/bulk_product_update_spec.js.coffee   | 21 +++++++++++++++++++
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/app/assets/javascripts/admin/bulk_product_update.js.coffee b/app/assets/javascripts/admin/bulk_product_update.js.coffee
index a9735aca4e..83590ba45d 100644
--- a/app/assets/javascripts/admin/bulk_product_update.js.coffee
+++ b/app/assets/javascripts/admin/bulk_product_update.js.coffee
@@ -222,11 +222,12 @@ productEditModule.controller "AdminProductEditCtrl", [
 
     $scope.loadVariantVariantUnit = (product, variant) ->
       unit_value = $scope.variantUnitValue product, variant
-      variant.unit_value_with_description = "#{unit_value || ''} #{variant.unit_description || ''}".trim()
+      unit_value = if unit_value? then unit_value else ''
+      variant.unit_value_with_description = "#{unit_value} #{variant.unit_description || ''}".trim()
 
 
     $scope.variantUnitValue = (product, variant) ->
-      if variant.unit_value
+      if variant.unit_value?
         if product.variant_unit_scale
           variant.unit_value / product.variant_unit_scale
         else
@@ -421,7 +422,8 @@ productEditModule.controller "AdminProductEditCtrl", [
         match = variant.unit_value_with_description.match(/^([\d\.]+|)( |)(.*)$/)
         if match
           product = $scope.findProduct(product.id)
-          variant.unit_value  = parseFloat(match[1]) || null
+          variant.unit_value  = parseFloat(match[1])
+          variant.unit_value  = null if isNaN(variant.unit_value)
           variant.unit_value *= product.variant_unit_scale if variant.unit_value && product.variant_unit_scale
           variant.unit_description = match[3]
 
diff --git a/spec/javascripts/unit/bulk_product_update_spec.js.coffee b/spec/javascripts/unit/bulk_product_update_spec.js.coffee
index e0d3f33660..d88bb1e34e 100644
--- a/spec/javascripts/unit/bulk_product_update_spec.js.coffee
+++ b/spec/javascripts/unit/bulk_product_update_spec.js.coffee
@@ -499,6 +499,13 @@ describe "AdminProductEditCtrl", ->
         scope.loadVariantVariantUnit product, product.variants[0]
         expect(product.variants[0].unit_value_with_description).toEqual '2.5'
 
+      it "displays a unit_value of zero", ->
+        product =
+          variant_unit_scale: 1.0
+          variants: [{id: 1, unit_value: 0}]
+        scope.loadVariantVariantUnit product, product.variants[0]
+        expect(product.variants[0].unit_value_with_description).toEqual '0'
+
 
   describe "calculating the scaled unit value for a variant", ->
     it "returns the scaled value when variant has a unit_value", ->
@@ -511,6 +518,11 @@ describe "AdminProductEditCtrl", ->
       variant = {unit_value: 5}
       expect(scope.variantUnitValue(product, variant)).toEqual 5
 
+    it "returns zero when the value is zero", ->
+      product = {}
+      variant = {unit_value: 0}
+      expect(scope.variantUnitValue(product, variant)).toEqual 0
+
     it "returns null when the variant has no unit_value", ->
       product = {}
       variant = {}
@@ -761,6 +773,15 @@ describe "AdminProductEditCtrl", ->
         scope.packVariant(testProduct, testVariant)
         expect(testVariant).toEqual {}
 
+      it "sets zero when the field is zero", ->
+        testProduct = {id: 123, variant_unit_scale: 1.0}
+        testVariant = {unit_value_with_description: "0"}
+        scope.packVariant(testProduct, testVariant)
+        expect(testVariant).toEqual
+          unit_value: 0
+          unit_description: ''
+          unit_value_with_description: "0"
+
       it "converts value from chosen unit to base unit", ->
         testProduct = {id: 123, variant_unit_scale: 1000}
         testVariant = {unit_value_with_description: "250.5"}

From 4ee4ea7c605a92b8b3c25e4387a5d9e5fdd17da2 Mon Sep 17 00:00:00 2001
From: Rohan Mitchell <rohan@rohanmitchell.com>
Date: Tue, 18 Feb 2014 14:02:17 +1100
Subject: [PATCH 12/21] Fix mismatch between client and server variants
 (ordering issue)

---
 app/views/spree/api/products/bulk_show.v1.rabl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/views/spree/api/products/bulk_show.v1.rabl b/app/views/spree/api/products/bulk_show.v1.rabl
index 6219bc193e..a220737007 100644
--- a/app/views/spree/api/products/bulk_show.v1.rabl
+++ b/app/views/spree/api/products/bulk_show.v1.rabl
@@ -10,7 +10,7 @@ node( :supplier ) do |p|
   partial 'spree/api/enterprises/bulk_show', :object => p.supplier
 end
 node( :variants ) do |p|
-  partial 'spree/api/variants/bulk_index', :object => p.variants.order('id ASC')
+  partial 'spree/api/variants/bulk_index', :object => p.variants.reorder('spree_variants.id ASC')
 end
 node( :master ) do |p|
   partial 'spree/api/variants/bulk_show', :object => p.master

From b0cb19e3702894c5b2eee96f05632009d77faff3 Mon Sep 17 00:00:00 2001
From: Rohan Mitchell <rohan@rohanmitchell.com>
Date: Tue, 18 Feb 2014 16:45:57 +1100
Subject: [PATCH 13/21] Do not show master options text (eg. '1kg') when
 product has variants

---
 app/views/shop/_products.html.haml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/views/shop/_products.html.haml b/app/views/shop/_products.html.haml
index 98b6a7ecf1..4a29a23170 100644
--- a/app/views/shop/_products.html.haml
+++ b/app/views/shop/_products.html.haml
@@ -21,7 +21,7 @@
               {{ product.supplier.name }}
           %td.notes {{ product.notes | truncate:80 }}
           %td 
-            {{ product.master.options_text }}
+            %span{"ng-hide" => "product.variants.length > 0"} {{ product.master.options_text }}
             %span{"ng-show" => "product.variants.length > 0"}
               %img.collapse{src: "/assets/collapse.png",
               "ng-show" => "product.show_variants",

From 7b8051862114bfdce65c1898c51b6f983e8a3981 Mon Sep 17 00:00:00 2001
From: Rohan Mitchell <rohan@rohanmitchell.com>
Date: Wed, 19 Feb 2014 11:29:26 +1100
Subject: [PATCH 14/21] On admin variant edit page, do not show option values
 for unit-related option types

---
 .../admin/variants_controller_decorator.rb    |  3 ++
 .../spree/products_helper_decorator.rb        |  5 +++
 app/models/spree/variant_decorator.rb         |  3 --
 .../hide_unit_option_types.html.haml.deface   |  9 +++++
 spec/features/admin/variants_spec.rb          | 34 +++++++++++++++++++
 5 files changed, 51 insertions(+), 3 deletions(-)
 create mode 100644 app/controllers/spree/admin/variants_controller_decorator.rb
 create mode 100644 app/overrides/spree/admin/variants/_form/hide_unit_option_types.html.haml.deface
 create mode 100644 spec/features/admin/variants_spec.rb

diff --git a/app/controllers/spree/admin/variants_controller_decorator.rb b/app/controllers/spree/admin/variants_controller_decorator.rb
new file mode 100644
index 0000000000..a567f0a8c9
--- /dev/null
+++ b/app/controllers/spree/admin/variants_controller_decorator.rb
@@ -0,0 +1,3 @@
+Spree::Admin::VariantsController.class_eval do
+  helper 'spree/products'
+end
diff --git a/app/helpers/spree/products_helper_decorator.rb b/app/helpers/spree/products_helper_decorator.rb
index 8c7b7fbe54..d83b2ad95b 100644
--- a/app/helpers/spree/products_helper_decorator.rb
+++ b/app/helpers/spree/products_helper_decorator.rb
@@ -4,5 +4,10 @@ module Spree
     def variant_price_diff(variant)
       "(#{number_to_currency variant.price})"
     end
+
+
+    def variant_unit_option_type?(option_type)
+      Spree::Product.all_variant_unit_option_types.include? option_type
+    end
   end
 end
diff --git a/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb
index de9a9fa9d0..12a4ca1a0e 100644
--- a/app/models/spree/variant_decorator.rb
+++ b/app/models/spree/variant_decorator.rb
@@ -16,14 +16,11 @@ Spree::Variant.class_eval do
     price + fees_for(distributor, order_cycle)
   end
 
-  # TODO: This method seems a little redundant. Though perhaps a useful interface.
-  # Consider removing.
   def fees_for(distributor, order_cycle)
     order_cycle.fees_for(self, distributor)
   end
 
 
-
   # Copied and modified from Spree::Variant
   def options_text
     values = self.option_values.joins(:option_type).order("#{Spree::OptionType.table_name}.position asc")
diff --git a/app/overrides/spree/admin/variants/_form/hide_unit_option_types.html.haml.deface b/app/overrides/spree/admin/variants/_form/hide_unit_option_types.html.haml.deface
new file mode 100644
index 0000000000..8facf38294
--- /dev/null
+++ b/app/overrides/spree/admin/variants/_form/hide_unit_option_types.html.haml.deface
@@ -0,0 +1,9 @@
+/ replace_contents "[data-hook='presentation']"
+
+= label :new_variant, option.option_type.presentation
+- if @variant.new_record?
+  = select(:new_variant, option.option_type.presentation, option.option_type.option_values.collect {|ov| [ ov.presentation, ov.id ] }, {}, {:class => 'select2 fullwidth'})
+- else
+  - opt = @variant.option_values.detect {|o| o.option_type == option.option_type }.try(:presentation)
+  - if opt && !variant_unit_option_type?(option.option_type)
+    = text_field(:new_variant,  option.option_type.presentation, :value => opt, :disabled => 'disabled', :class => 'fullwidth')
diff --git a/spec/features/admin/variants_spec.rb b/spec/features/admin/variants_spec.rb
new file mode 100644
index 0000000000..5380333c61
--- /dev/null
+++ b/spec/features/admin/variants_spec.rb
@@ -0,0 +1,34 @@
+require "spec_helper"
+
+feature %q{
+    As an admin
+    I want to manage product variants
+} do
+  include AuthenticationWorkflow
+  include WebHelper
+
+  describe "units and values" do
+    it "does not show traditional option value fields for unit-related option types" do
+      # Given a product with unit-related option types, with a variant
+      p = create(:simple_product, variant_unit: "weight", variant_unit_scale: "1")
+      v = create(:variant, product: p, unit_value: 1, unit_description: 'foo')
+
+      # And the product has option types for the unit-related and non-unit-related option values
+      p.option_types << v.option_values.first.option_type
+
+      # When I view the variant
+      login_to_admin_section
+      click_link 'Products'
+      within('#sub_nav') { click_link 'Products' }
+      click_link p.name
+      click_link 'Variants'
+
+      page.find('table.index .icon-edit').click
+
+      # Then I should not see a traditional option value field for the unit-related option value
+      page.all("div[data-hook='presentation'] input").count.should == 1
+    end
+
+    it "shows unit value and description fields when the variant's product has associated option types set"
+  end
+end

From fb427244300747b3a7bfea81220ee43ca733b67a Mon Sep 17 00:00:00 2001
From: Rohan Mitchell <rohan@rohanmitchell.com>
Date: Wed, 19 Feb 2014 11:56:10 +1100
Subject: [PATCH 15/21] Remove entire field div instead of just the text field

---
 .../hide_unit_option_types.html.haml.deface     | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/app/overrides/spree/admin/variants/_form/hide_unit_option_types.html.haml.deface b/app/overrides/spree/admin/variants/_form/hide_unit_option_types.html.haml.deface
index 8facf38294..16a0e572b4 100644
--- a/app/overrides/spree/admin/variants/_form/hide_unit_option_types.html.haml.deface
+++ b/app/overrides/spree/admin/variants/_form/hide_unit_option_types.html.haml.deface
@@ -1,9 +1,10 @@
-/ replace_contents "[data-hook='presentation']"
+/ replace "[data-hook='presentation']"
 
-= label :new_variant, option.option_type.presentation
-- if @variant.new_record?
-  = select(:new_variant, option.option_type.presentation, option.option_type.option_values.collect {|ov| [ ov.presentation, ov.id ] }, {}, {:class => 'select2 fullwidth'})
-- else
-  - opt = @variant.option_values.detect {|o| o.option_type == option.option_type }.try(:presentation)
-  - if opt && !variant_unit_option_type?(option.option_type)
-    = text_field(:new_variant,  option.option_type.presentation, :value => opt, :disabled => 'disabled', :class => 'fullwidth')
+- unless variant_unit_option_type?(option.option_type)
+  .field{"data-hook" => "presentation"}
+    = label :new_variant, option.option_type.presentation
+    - if @variant.new_record?
+      = select(:new_variant, option.option_type.presentation, option.option_type.option_values.collect {|ov| [ ov.presentation, ov.id ] }, {}, {:class => 'select2 fullwidth'})
+    - else
+      - if opt = @variant.option_values.detect {|o| o.option_type == option.option_type }.try(:presentation)
+        = text_field(:new_variant,  option.option_type.presentation, :value => opt, :disabled => 'disabled', :class => 'fullwidth')

From 3024bbbeb59404195ec8c075721625f8f6c788a5 Mon Sep 17 00:00:00 2001
From: Rohan Mitchell <rohan@rohanmitchell.com>
Date: Wed, 19 Feb 2014 11:56:37 +1100
Subject: [PATCH 16/21] Edit unit value and description of variant

---
 ...nit_value_and_description.html.haml.deface |  9 ++++
 spec/features/admin/variants_spec.rb          | 48 ++++++++++++-------
 2 files changed, 39 insertions(+), 18 deletions(-)
 create mode 100644 app/overrides/spree/admin/variants/_form/add_unit_value_and_description.html.haml.deface

diff --git a/app/overrides/spree/admin/variants/_form/add_unit_value_and_description.html.haml.deface b/app/overrides/spree/admin/variants/_form/add_unit_value_and_description.html.haml.deface
new file mode 100644
index 0000000000..8fbd99d4e3
--- /dev/null
+++ b/app/overrides/spree/admin/variants/_form/add_unit_value_and_description.html.haml.deface
@@ -0,0 +1,9 @@
+/ insert_top "[data-hook='admin_variant_form_fields']"
+
+.field{"data-hook" => "unit_value"}
+  = f.label :unit_value, "Unit Value"
+  = f.text_field :unit_value, class: "fullwidth"
+
+.field{"data-hook" => "unit_description"}
+  = f.label :unit_description, "Unit Description"
+  = f.text_field :unit_description, class: "fullwidth"
diff --git a/spec/features/admin/variants_spec.rb b/spec/features/admin/variants_spec.rb
index 5380333c61..c0bd305f15 100644
--- a/spec/features/admin/variants_spec.rb
+++ b/spec/features/admin/variants_spec.rb
@@ -7,28 +7,40 @@ feature %q{
   include AuthenticationWorkflow
   include WebHelper
 
-  describe "units and values" do
-    it "does not show traditional option value fields for unit-related option types" do
-      # Given a product with unit-related option types, with a variant
-      p = create(:simple_product, variant_unit: "weight", variant_unit_scale: "1")
-      v = create(:variant, product: p, unit_value: 1, unit_description: 'foo')
+  scenario "editing unit value and description for a variant" do
+    # Given a product with unit-related option types, with a variant
+    p = create(:simple_product, variant_unit: "weight", variant_unit_scale: "1")
+    v = create(:variant, product: p, unit_value: 1, unit_description: 'foo')
 
-      # And the product has option types for the unit-related and non-unit-related option values
-      p.option_types << v.option_values.first.option_type
+    # And the product has option types for the unit-related and non-unit-related option values
+    p.option_types << v.option_values.first.option_type
 
-      # When I view the variant
-      login_to_admin_section
-      click_link 'Products'
-      within('#sub_nav') { click_link 'Products' }
-      click_link p.name
-      click_link 'Variants'
+    # When I view the variant
+    login_to_admin_section
+    click_link 'Products'
+    within('#sub_nav') { click_link 'Products' }
+    click_link p.name
+    click_link 'Variants'
+    page.find('table.index .icon-edit').click
 
-      page.find('table.index .icon-edit').click
+    # Then I should not see a traditional option value field for the unit-related option value
+    page.all("div[data-hook='presentation'] input").count.should == 1
 
-      # Then I should not see a traditional option value field for the unit-related option value
-      page.all("div[data-hook='presentation'] input").count.should == 1
-    end
+    # And I should see unit value and description fields for the unit-related option value
+    page.should have_field "variant_unit_value", with: "1"
+    page.should have_field "variant_unit_description", with: "foo"
 
-    it "shows unit value and description fields when the variant's product has associated option types set"
+    # When I update the fields and save the variant
+    fill_in "variant_unit_value", with: "123"
+    fill_in "variant_unit_description", with: "bar"
+    click_button 'Update'
+    page.should have_content %Q(Variant "#{p.name}" has been successfully updated!)
+
+    # Then the unit value and description should have been saved
+    v.reload
+    v.unit_value.should == 123
+    v.unit_description.should == 'bar'
   end
+
+  it "should not show unit value or description fields when the product does not have a unit-related option type"
 end

From 3f9f24157c927720c97708ef8040423e858e9a1a Mon Sep 17 00:00:00 2001
From: Rohan Mitchell <rohan@rohanmitchell.com>
Date: Wed, 19 Feb 2014 12:07:53 +1100
Subject: [PATCH 17/21] Do not show unit value or description when product does
 not have a unit-related option type

---
 .../spree/products_helper_decorator.rb        |  5 +++++
 ...nit_value_and_description.html.haml.deface | 13 ++++++------
 spec/features/admin/variants_spec.rb          | 21 ++++++++++++++++++-
 3 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/app/helpers/spree/products_helper_decorator.rb b/app/helpers/spree/products_helper_decorator.rb
index d83b2ad95b..99596a73e9 100644
--- a/app/helpers/spree/products_helper_decorator.rb
+++ b/app/helpers/spree/products_helper_decorator.rb
@@ -6,6 +6,11 @@ module Spree
     end
 
 
+    def product_has_variant_unit_option_type?(product)
+      product.option_types.any? { |option_type| variant_unit_option_type? option_type }
+    end
+
+
     def variant_unit_option_type?(option_type)
       Spree::Product.all_variant_unit_option_types.include? option_type
     end
diff --git a/app/overrides/spree/admin/variants/_form/add_unit_value_and_description.html.haml.deface b/app/overrides/spree/admin/variants/_form/add_unit_value_and_description.html.haml.deface
index 8fbd99d4e3..8696134177 100644
--- a/app/overrides/spree/admin/variants/_form/add_unit_value_and_description.html.haml.deface
+++ b/app/overrides/spree/admin/variants/_form/add_unit_value_and_description.html.haml.deface
@@ -1,9 +1,10 @@
 / insert_top "[data-hook='admin_variant_form_fields']"
 
-.field{"data-hook" => "unit_value"}
-  = f.label :unit_value, "Unit Value"
-  = f.text_field :unit_value, class: "fullwidth"
+- if product_has_variant_unit_option_type?(@product)
+  .field{"data-hook" => "unit_value"}
+    = f.label :unit_value, "Unit Value"
+    = f.text_field :unit_value, class: "fullwidth"
 
-.field{"data-hook" => "unit_description"}
-  = f.label :unit_description, "Unit Description"
-  = f.text_field :unit_description, class: "fullwidth"
+  .field{"data-hook" => "unit_description"}
+    = f.label :unit_description, "Unit Description"
+    = f.text_field :unit_description, class: "fullwidth"
diff --git a/spec/features/admin/variants_spec.rb b/spec/features/admin/variants_spec.rb
index c0bd305f15..7e178ab443 100644
--- a/spec/features/admin/variants_spec.rb
+++ b/spec/features/admin/variants_spec.rb
@@ -42,5 +42,24 @@ feature %q{
     v.unit_description.should == 'bar'
   end
 
-  it "should not show unit value or description fields when the product does not have a unit-related option type"
+  it "does not show unit value or description fields when the product does not have a unit-related option type" do
+    # Given a product without unit-related option types, with a variant
+    p = create(:simple_product, variant_unit: nil, variant_unit_scale: nil)
+    v = create(:variant, product: p, unit_value: nil, unit_description: nil)
+
+    # And the product has option types for the variant's option values
+    p.option_types << v.option_values.first.option_type
+
+    # When I view the variant
+    login_to_admin_section
+    click_link 'Products'
+    within('#sub_nav') { click_link 'Products' }
+    click_link p.name
+    click_link 'Variants'
+    page.find('table.index .icon-edit').click
+
+    # Then I should not see unit value and description fields
+    page.should_not have_field "variant_unit_value"
+    page.should_not have_field "variant_unit_description"
+  end
 end

From acbf1ed680245710d527f4a972ce9d3fb5aa99f3 Mon Sep 17 00:00:00 2001
From: Rohan Mitchell <rohan@rohanmitchell.com>
Date: Wed, 19 Feb 2014 13:12:52 +1100
Subject: [PATCH 18/21] Refactor fragile spec to use has_field? and has_select?
 in an attempt to make it more reliable in CI

---
 spec/features/admin/order_cycles_spec.rb | 32 +++++++++---------------
 1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/spec/features/admin/order_cycles_spec.rb b/spec/features/admin/order_cycles_spec.rb
index 8f2c1b2a34..5cadd11a29 100644
--- a/spec/features/admin/order_cycles_spec.rb
+++ b/spec/features/admin/order_cycles_spec.rb
@@ -189,24 +189,20 @@ feature %q{
     end
 
     # And the suppliers should have fees
-    page.find('#order_cycle_incoming_exchange_0_enterprise_fees_0_enterprise_id option[selected=selected]').
-      text.should == oc.suppliers.first.name
-    page.find('#order_cycle_incoming_exchange_0_enterprise_fees_0_enterprise_fee_id option[selected=selected]').
-      text.should == oc.suppliers.first.enterprise_fees.first.name
+    page.should have_select 'order_cycle_incoming_exchange_0_enterprise_fees_0_enterprise_id', selected: oc.suppliers.first.name
+    page.should have_select 'order_cycle_incoming_exchange_0_enterprise_fees_0_enterprise_fee_id', selected: oc.suppliers.first.enterprise_fees.first.name
 
-    page.find('#order_cycle_incoming_exchange_1_enterprise_fees_0_enterprise_id option[selected=selected]').
-      text.should == oc.suppliers.last.name
-    page.find('#order_cycle_incoming_exchange_1_enterprise_fees_0_enterprise_fee_id option[selected=selected]').
-      text.should == oc.suppliers.last.enterprise_fees.first.name
+    page.should have_select 'order_cycle_incoming_exchange_1_enterprise_fees_0_enterprise_id', selected: oc.suppliers.last.name
+    page.should have_select 'order_cycle_incoming_exchange_1_enterprise_fees_0_enterprise_fee_id', selected: oc.suppliers.last.enterprise_fees.first.name
 
     # And I should see the distributors
     page.should have_selector 'td.distributor_name', :text => oc.distributors.first.name
     page.should have_selector 'td.distributor_name', :text => oc.distributors.last.name
 
-    page.find('#order_cycle_outgoing_exchange_0_pickup_time').value.should == 'time 0'
-    page.find('#order_cycle_outgoing_exchange_0_pickup_instructions').value.should == 'instructions 0'
-    page.find('#order_cycle_outgoing_exchange_1_pickup_time').value.should == 'time 1'
-    page.find('#order_cycle_outgoing_exchange_1_pickup_instructions').value.should == 'instructions 1'
+    page.should have_field 'order_cycle_outgoing_exchange_0_pickup_time', with: 'time 0'
+    page.should have_field 'order_cycle_outgoing_exchange_0_pickup_instructions', with: 'instructions 0'
+    page.should have_field 'order_cycle_outgoing_exchange_1_pickup_time', with: 'time 1'
+    page.should have_field 'order_cycle_outgoing_exchange_1_pickup_instructions', with: 'instructions 1'
 
     # And the distributors should have products
     page.all('table.exchanges tbody tr.distributor').each do |row|
@@ -219,15 +215,11 @@ feature %q{
     end
 
     # And the distributors should have fees
-    page.find('#order_cycle_outgoing_exchange_0_enterprise_fees_0_enterprise_id option[selected=selected]').
-      text.should == oc.distributors.first.name
-    page.find('#order_cycle_outgoing_exchange_0_enterprise_fees_0_enterprise_fee_id option[selected=selected]').
-      text.should == oc.distributors.first.enterprise_fees.first.name
+    page.should have_select 'order_cycle_outgoing_exchange_0_enterprise_fees_0_enterprise_id', selected: oc.distributors.first.name
+    page.should have_select 'order_cycle_outgoing_exchange_0_enterprise_fees_0_enterprise_fee_id', selected: oc.distributors.first.enterprise_fees.first.name
 
-    page.find('#order_cycle_outgoing_exchange_1_enterprise_fees_0_enterprise_id option[selected=selected]').
-      text.should == oc.distributors.last.name
-    page.find('#order_cycle_outgoing_exchange_1_enterprise_fees_0_enterprise_fee_id option[selected=selected]').
-      text.should == oc.distributors.last.enterprise_fees.first.name
+    page.should have_select 'order_cycle_outgoing_exchange_1_enterprise_fees_0_enterprise_id', selected: oc.distributors.last.name
+    page.should have_select 'order_cycle_outgoing_exchange_1_enterprise_fees_0_enterprise_fee_id', selected: oc.distributors.last.enterprise_fees.first.name
   end
 
 

From f88b930137a7c5609bd5f09a5fa13e5ecc050c8d Mon Sep 17 00:00:00 2001
From: Rohan Mitchell <rohan@rohanmitchell.com>
Date: Wed, 19 Feb 2014 13:32:17 +1100
Subject: [PATCH 19/21] Extract unit_value_with_description into
 unit_description only when a string starting with a number is provided

---
 .../javascripts/admin/bulk_product_update.js.coffee       | 2 +-
 spec/javascripts/unit/bulk_product_update_spec.js.coffee  | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/app/assets/javascripts/admin/bulk_product_update.js.coffee b/app/assets/javascripts/admin/bulk_product_update.js.coffee
index 83590ba45d..42e727bb6f 100644
--- a/app/assets/javascripts/admin/bulk_product_update.js.coffee
+++ b/app/assets/javascripts/admin/bulk_product_update.js.coffee
@@ -419,7 +419,7 @@ productEditModule.controller "AdminProductEditCtrl", [
 
     $scope.packVariant = (product, variant) ->
       if variant.hasOwnProperty("unit_value_with_description")
-        match = variant.unit_value_with_description.match(/^([\d\.]+|)( |)(.*)$/)
+        match = variant.unit_value_with_description.match(/^([\d\.]+(?= |$)|)( |)(.*)$/)
         if match
           product = $scope.findProduct(product.id)
           variant.unit_value  = parseFloat(match[1])
diff --git a/spec/javascripts/unit/bulk_product_update_spec.js.coffee b/spec/javascripts/unit/bulk_product_update_spec.js.coffee
index d88bb1e34e..bd85419ac9 100644
--- a/spec/javascripts/unit/bulk_product_update_spec.js.coffee
+++ b/spec/javascripts/unit/bulk_product_update_spec.js.coffee
@@ -760,6 +760,14 @@ describe "AdminProductEditCtrl", ->
           unit_description: 'Medium'
           unit_value_with_description: "Medium"
 
+      it "extracts into unit_description when a string starting with a number is provided", ->
+        testVariant = {unit_value_with_description: "1kg"}
+        scope.packVariant(testProduct, testVariant)
+        expect(testVariant).toEqual
+          unit_value: null
+          unit_description: '1kg'
+          unit_value_with_description: "1kg"
+
       it "sets blank values when no value provided", ->
         testVariant = {unit_value_with_description: ""}
         scope.packVariant(testProduct, testVariant)

From 6d1a20280097a4d7f4019c93ac30edb3d22962d4 Mon Sep 17 00:00:00 2001
From: Rohan Mitchell <rohan@rohanmitchell.com>
Date: Wed, 19 Feb 2014 13:32:57 +1100
Subject: [PATCH 20/21] Fix grammar

---
 spec/features/admin/products_spec.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/spec/features/admin/products_spec.rb b/spec/features/admin/products_spec.rb
index 3399785a3c..58d9d57775 100644
--- a/spec/features/admin/products_spec.rb
+++ b/spec/features/admin/products_spec.rb
@@ -1,8 +1,8 @@
 require "spec_helper"
 
 feature %q{
-    As a supplier
-    I want set a supplier and distributor(s) for a product
+    As an admin
+    I want to set a supplier and distributor(s) for a product
 } do
   include AuthenticationWorkflow
   include WebHelper

From 97a6a812b833c0204ded88f650970cbefef22420 Mon Sep 17 00:00:00 2001
From: Rohan Mitchell <rohan@rohanmitchell.com>
Date: Wed, 19 Feb 2014 13:44:31 +1100
Subject: [PATCH 21/21] Fix product listing appearing on RHS of page on Firefox

---
 app/assets/stylesheets/admin/products.css.scss | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/app/assets/stylesheets/admin/products.css.scss b/app/assets/stylesheets/admin/products.css.scss
index 026e4cf83b..20f9c49a84 100644
--- a/app/assets/stylesheets/admin/products.css.scss
+++ b/app/assets/stylesheets/admin/products.css.scss
@@ -106,6 +106,8 @@ ul.column-list {
 }
 
 table#listing_products.bulk {
+  clear: both;
+
   td.supplier {
     select {
       width: 125px;