Merge pull request #2604 from Matt-Yorkley/pi/updating_variants_bug

Pi/updating variants bug

app/assets/javascripts/admin/product_import/controllers/import_form_controller.js.coffee

@@ -51,7 +51,7 @@ angular.module("admin.productImport").controller "ImportFormCtrl", ($scope, $htt
   $scope.start = () ->
     $scope.started = true
     total = ams_data.item_count
-    size = 100
+    size = 50
     $scope.chunks = Math.ceil(total / size)
 
     i = 0

app/assets/javascripts/admin/product_import/filters/filter_entries.js.coffee

@@ -10,7 +10,7 @@ angular.module("admin.productImport").filter 'entriesFilterValid', ->
 
       if type == 'valid' and validates_as != '' \
       or type == 'invalid' and validates_as == '' \
-      or type == 'create_product' and validates_as == 'new_product' or validates_as == 'new_variant' \
+      or type == 'create_product' and (validates_as == 'new_product' or validates_as == 'new_variant') \
       or type == 'update_product' and validates_as == 'existing_variant' \
       or type == 'create_inventory' and validates_as == 'new_inventory_item' \
       or type == 'update_inventory' and validates_as == 'existing_inventory_item'

app/models/product_import/entry_processor.rb

@@ -115,7 +115,13 @@ def save_to_product_list(entry)
 
       return unless entry.validates_as? 'existing_variant'
 
-      save_variant entry
+      begin
+        save_variant entry
+      rescue ActiveRecord::StaleObjectError
+        entry.product_object.reload
+        save_variant entry
+      end
+
       @variants_updated += 1
     end
 

app/models/product_import/entry_validator.rb

@@ -124,16 +124,15 @@ def producer_validation(entry)
     end
 
     def inventory_validation(entry)
-      # Checks a potential inventory item corresponds to a valid variant
-      match = Spree::Product.where(supplier_id: entry.producer_id, name: entry.name, deleted_at: nil).first
+      products = Spree::Product.where(supplier_id: entry.producer_id, name: entry.name, deleted_at: nil)
 
-      if match.nil?
+      if products.empty?
         mark_as_invalid(entry, attribute: 'name', error: I18n.t('admin.product_import.model.no_product'))
         return
       end
 
-      match.variants.each do |existing_variant|
-        unit_scale = match.variant_unit_scale
+      products.flat_map(&:variants).each do |existing_variant|
+        unit_scale = existing_variant.product.variant_unit_scale
         unscaled_units = entry.unscaled_units || 0
         entry.unit_value = unscaled_units * unit_scale
 
@@ -176,24 +175,20 @@ def tax_and_shipping_validation(entry, type, category, index)
     end
 
     def product_validation(entry)
-      # Find product with matching supplier and name
-      match = Spree::Product.where(supplier_id: entry.supplier_id, name: entry.name, deleted_at: nil).first
+      products = Spree::Product.where(supplier_id: entry.supplier_id, name: entry.name, deleted_at: nil)
 
-      # If no matching product was found, create a new product
-      if match.nil?
+      if products.empty?
         mark_as_new_product(entry)
         return
       end
 
-      # Otherwise, if a variant exists with matching display_name and unit_value, update it
-      match.variants.each do |existing_variant|
+      products.flat_map(&:variants).each do |existing_variant|
         if entry_matches_existing_variant?(entry, existing_variant) && existing_variant.deleted_at.nil?
           return mark_as_existing_variant(entry, existing_variant)
         end
       end
 
-      # Otherwise, a variant with sufficiently matching attributes doesn't exist; create a new one
-      mark_as_new_variant(entry, match.id)
+      mark_as_new_variant(entry, products.first.id)
     end
 
     def mark_as_new_product(entry)

spec/features/admin/product_import_spec.rb

@@ -201,6 +201,44 @@
       expect(Spree::Product.find_by_name('Beans').on_hand).to eq 0
     end
 
+    it "can save a new product and variant of that product at the same time, add variant to existing product" do
+      csv_data = CSV.generate do |csv|
+        csv << ["name", "supplier", "category", "on_hand", "price", "units", "unit_type", "display_name"]
+        csv << ["Potatoes", "User Enterprise", "Vegetables", "5", "3.50", "500", "g", "Small Bag"]
+        csv << ["Potatoes", "User Enterprise", "Vegetables", "6", "5.50", "2", "kg", "Big Bag"]
+        csv << ["Beans", "User Enterprise", "Vegetables", "7", "2.50", "250", "g", nil]
+      end
+      File.write('/tmp/test.csv', csv_data)
+
+      visit main_app.admin_product_import_path
+      attach_file 'file', '/tmp/test.csv'
+      click_button 'Upload'
+
+      import_data
+
+      expect(page).to have_selector '.item-count', text: "3"
+      expect(page).to_not have_selector '.invalid-count'
+      expect(page).to have_selector '.create-count', text: "3"
+      expect(page).to_not have_selector '.update-count'
+      expect(page).to_not have_selector '.inv-create-count'
+      expect(page).to_not have_selector '.inv-update-count'
+
+      save_data
+
+      small_bag = Spree::Variant.find_by_display_name('Small Bag')
+      expect(small_bag.product.name).to eq 'Potatoes'
+      expect(small_bag.price).to eq 3.50
+      expect(small_bag.on_hand).to eq 5
+
+      big_bag = Spree::Variant.find_by_display_name('Big Bag')
+      expect(big_bag.product.name).to eq 'Potatoes'
+      expect(big_bag.price).to eq 5.50
+      expect(big_bag.on_hand).to eq 6
+
+      expect(big_bag.product.id).to eq small_bag.product.id
+    end
+
+
     it "can import items into inventory" do
       csv_data = CSV.generate do |csv|
         csv << ["name", "supplier", "producer", "category", "on_hand", "price", "units"]

spec/models/product_importer_spec.rb

@@ -16,6 +16,7 @@
 
   let!(:category) { create(:taxon, name: 'Vegetables') }
   let!(:category2) { create(:taxon, name: 'Cake') }
+  let!(:category3) { create(:taxon, name: 'Cereal') }
   let!(:tax_category) { create(:tax_category) }
   let!(:tax_category2) { create(:tax_category) }
   let!(:shipping_category) { create(:shipping_category) }
@@ -28,6 +29,13 @@
   let!(:product5) { create(:simple_product, supplier: enterprise2, on_hand: '100', name: 'Lettuce', unit_value: '500') }
   let!(:product6) { create(:simple_product, supplier: enterprise3, on_hand: '100', name: 'Beetroot', unit_value: '500', on_demand: true, variant_unit_scale: 1, variant_unit: 'weight') }
   let!(:product7) { create(:simple_product, supplier: enterprise3, on_hand: '100', name: 'Tomato', unit_value: '500', variant_unit_scale: 1, variant_unit: 'weight') }
+
+  let!(:product8) { create(:simple_product, supplier: enterprise, on_hand: '100', name: 'Oats', unit_value: '500', variant_unit_scale: 1, variant_unit: 'weight', primary_taxon_id: category3.id) }
+  let!(:product9) { create(:simple_product, supplier: enterprise, on_hand: '100', name: 'Oats', unit_value: '500', variant_unit_scale: 1, variant_unit: 'weight', primary_taxon_id: category3.id) }
+  let!(:variant2) { create(:variant, product_id: product8.id, price: '4.50', on_hand: '100', unit_value: '500', display_name: 'Porridge Oats') }
+  let!(:variant3) { create(:variant, product_id: product8.id, price: '5.50', on_hand: '100', unit_value: '500', display_name: 'Rolled Oats') }
+  let!(:variant4) { create(:variant, product_id: product9.id, price: '6.50', on_hand: '100', unit_value: '500', display_name: 'Flaked Oats') }
+
   let!(:variant_override) { create(:variant_override, variant_id: product4.variants.first.id, hub: enterprise2, count_on_hand: 42) }
   let!(:variant_override2) { create(:variant_override, variant_id: product5.variants.first.id, hub: enterprise, count_on_hand: 96) }
 
@@ -196,7 +204,7 @@
       end
       File.write('/tmp/test-m.csv', csv_data)
       file = File.new('/tmp/test-m.csv')
-      settings = {enterprise2.id.to_s => {'import_into' => 'product_list'}}
+      settings = {'import_into' => 'product_list'}
       @importer = ProductImport::ProductImporter.new(file, admin, start: 1, end: 100, settings: settings)
     end
     after { File.delete('/tmp/test-m.csv') }
@@ -242,7 +250,7 @@
       end
       File.write('/tmp/test-m.csv', csv_data)
       file = File.new('/tmp/test-m.csv')
-      settings = {enterprise.id.to_s => {'import_into' => 'product_list'}}
+      settings = {'import_into' => 'product_list'}
       @importer = ProductImport::ProductImporter.new(file, admin, start: 1, end: 100, settings: settings)
     end
     after { File.delete('/tmp/test-m.csv') }
@@ -272,6 +280,8 @@
       expect(big_bag.product.name).to eq 'Potatoes'
       expect(big_bag.price).to eq 5.50
       expect(big_bag.on_hand).to eq 6
+
+      expect(big_bag.product.id).to eq small_bag.product.id
     end
   end
 
@@ -284,7 +294,7 @@
       end
       File.write('/tmp/test-m.csv', csv_data)
       file = File.new('/tmp/test-m.csv')
-      settings = {enterprise3.id.to_s => {'import_into' => 'product_list'}}
+      settings = {'import_into' => 'product_list'}
       @importer = ProductImport::ProductImporter.new(file, admin, start: 1, end: 100, settings: settings)
     end
     after { File.delete('/tmp/test-m.csv') }
@@ -317,6 +327,115 @@
     end
   end
 
+  describe "when more than one product of the same name already exists with multiple variants each" do
+    before do
+      csv_data = CSV.generate do |csv|
+        csv << ["name", "supplier", "category", "on_hand", "price", "units", "unit_type", "display_name"]
+        csv << ["Oats", "User Enterprise", "Cereal", "50", "3.50", "500", "g", "Rolled Oats"]   # Update
+        csv << ["Oats", "User Enterprise", "Cereal", "80", "3.75", "500", "g", "Flaked Oats"]   # Update
+        csv << ["Oats", "User Enterprise", "Cereal", "60", "5.50", "500", "g", "Magic Oats"]    # Add
+        csv << ["Oats", "User Enterprise", "Cereal", "70", "8.50", "500", "g", "French Oats"]   # Add
+        csv << ["Oats", "User Enterprise", "Cereal", "70", "8.50", "500", "g", "Scottish Oats"] # Add
+      end
+      File.write('/tmp/test-m.csv', csv_data)
+      file = File.new('/tmp/test-m.csv')
+      settings = {'import_into' => 'product_list'}
+      @importer = ProductImport::ProductImporter.new(file, admin, start: 1, end: 100, settings: settings)
+    end
+    after { File.delete('/tmp/test-m.csv') }
+
+    it "validates entries" do
+      @importer.validate_entries
+      entries = JSON.parse(@importer.entries_json)
+
+      expect(filter('valid', entries)).to eq 5
+      expect(filter('invalid', entries)).to eq 0
+      expect(filter('create_product', entries)).to eq 3
+      expect(filter('update_product', entries)).to eq 2
+      expect(filter('create_inventory', entries)).to eq 0
+      expect(filter('update_inventory', entries)).to eq 0
+    end
+
+    it "saves and updates" do
+      @importer.save_entries
+
+      expect(@importer.products_created_count).to eq 3
+      expect(@importer.products_updated_count).to eq 2
+      expect(@importer.inventory_created_count).to eq 0
+      expect(@importer.inventory_updated_count).to eq 0
+      expect(@importer.updated_ids.count).to eq 5
+    end
+  end
+
+  describe "when importer processes create and update across multiple stages" do
+    before do
+      csv_data = CSV.generate do |csv|
+        csv << ["name", "supplier", "category", "on_hand", "price", "units", "unit_type", "display_name"]
+        csv << ["Bag of Oats", "User Enterprise", "Cereal", "60", "5.50", "500", "g", "Magic Oats"]     # Add
+        csv << ["Bag of Oats", "User Enterprise", "Cereal", "70", "8.50", "500", "g", "French Oats"]    # Add
+        csv << ["Bag of Oats", "User Enterprise", "Cereal", "80", "9.50", "500", "g", "Organic Oats"]   # Add
+        csv << ["Bag of Oats", "User Enterprise", "Cereal", "90", "7.50", "500", "g", "Scottish Oats"]  # Add
+        csv << ["Bag of Oats", "User Enterprise", "Cereal", "30", "6.50", "500", "g", "Breakfast Oats"] # Add
+      end
+      File.write('/tmp/test-m.csv', csv_data)
+      @file = File.new('/tmp/test-m.csv')
+      @settings = {'import_into' => 'product_list'}
+    end
+    after { File.delete('/tmp/test-m.csv') }
+
+    it "processes the validation in stages" do
+      # Using settings of start: 1, end: 2 to simulate import over multiple stages
+      @importer = ProductImport::ProductImporter.new(@file, admin, start: 1, end: 3, settings: @settings)
+
+      @importer.validate_entries
+      entries = JSON.parse(@importer.entries_json)
+
+      expect(filter('valid', entries)).to eq 3
+      expect(filter('invalid', entries)).to eq 0
+      expect(filter('create_product', entries)).to eq 3
+      expect(filter('update_product', entries)).to eq 0
+      expect(filter('create_inventory', entries)).to eq 0
+      expect(filter('update_inventory', entries)).to eq 0
+
+      @importer = ProductImport::ProductImporter.new(@file, admin, start: 4, end: 6, settings: @settings)
+
+      @importer.validate_entries
+      entries = JSON.parse(@importer.entries_json)
+
+      expect(filter('valid', entries)).to eq 2
+      expect(filter('invalid', entries)).to eq 0
+      expect(filter('create_product', entries)).to eq 2
+      expect(filter('update_product', entries)).to eq 0
+      expect(filter('create_inventory', entries)).to eq 0
+      expect(filter('update_inventory', entries)).to eq 0
+    end
+
+    it "processes saving in stages" do
+      @importer = ProductImport::ProductImporter.new(@file, admin, start: 1, end: 3, settings: @settings)
+      @importer.save_entries
+
+      expect(@importer.products_created_count).to eq 3
+      expect(@importer.products_updated_count).to eq 0
+      expect(@importer.inventory_created_count).to eq 0
+      expect(@importer.inventory_updated_count).to eq 0
+      expect(@importer.updated_ids.count).to eq 3
+
+      @importer = ProductImport::ProductImporter.new(@file, admin, start: 4, end: 6, settings: @settings)
+      @importer.save_entries
+
+      expect(@importer.products_created_count).to eq 2
+      expect(@importer.products_updated_count).to eq 0
+      expect(@importer.inventory_created_count).to eq 0
+      expect(@importer.inventory_updated_count).to eq 0
+      expect(@importer.updated_ids.count).to eq 2
+
+      products = Spree::Product.find_all_by_name('Bag of Oats')
+
+      expect(products.count).to eq 1
+      expect(products.first.variants.count).to eq 5
+    end
+  end
+
   describe "importing items into inventory" do
     before do
       csv_data = CSV.generate do |csv|
@@ -482,7 +601,7 @@
       @importer = ProductImport::ProductImporter.new(file, admin, start: 1, end: 100, updated_ids: updated_ids, enterprises_to_reset: [enterprise.id], settings: settings)
       @importer.reset_absent(updated_ids)
 
-      expect(@importer.products_reset_count).to eq 2
+      expect(@importer.products_reset_count).to eq 7
 
       expect(Spree::Product.find_by_name('Carrots').on_hand).to eq 5    # Present in file, added
       expect(Spree::Product.find_by_name('Beans').on_hand).to eq 6      # Present in file, updated