From 1df1ddcf66cf15848a114f1e8cdd674d8722aa91 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 27 Aug 2018 23:06:54 +0100 Subject: [PATCH 01/10] Add spec for saving product and variant simultaneously --- spec/features/admin/product_import_spec.rb | 38 ++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/spec/features/admin/product_import_spec.rb b/spec/features/admin/product_import_spec.rb index af23ddf9bf..31477f1338 100644 --- a/spec/features/admin/product_import_spec.rb +++ b/spec/features/admin/product_import_spec.rb @@ -201,6 +201,44 @@ feature "Product Import", js: true do 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" 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"] + 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: "2" + expect(page).to_not have_selector '.invalid-count' + expect(page).to have_selector '.create-count', text: "2" + expect(page).to_not have_selector '.update-count' + 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"] From 8c2a49a57c7a173226da6141322453eb1013e822 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 27 Aug 2018 23:08:19 +0100 Subject: [PATCH 02/10] Update PI model spec --- spec/models/product_importer_spec.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/models/product_importer_spec.rb b/spec/models/product_importer_spec.rb index c0cbc6718f..1714e0f88e 100644 --- a/spec/models/product_importer_spec.rb +++ b/spec/models/product_importer_spec.rb @@ -272,6 +272,8 @@ describe ProductImport::ProductImporter do 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 From 6b9b8d8b734be78acbdcb2671f06edc60566380d Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 28 Aug 2018 03:26:17 +0100 Subject: [PATCH 03/10] Match multiple product names --- app/models/product_import/entry_validator.rb | 21 ++++++++------------ 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/app/models/product_import/entry_validator.rb b/app/models/product_import/entry_validator.rb index 4f48c7745c..873e1478e0 100644 --- a/app/models/product_import/entry_validator.rb +++ b/app/models/product_import/entry_validator.rb @@ -124,16 +124,15 @@ module ProductImport 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.map(&:variants).flatten.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 @@ module ProductImport 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.map(&:variants).flatten.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) From f905284f7a2323490d81b21add1bc3e7736229b6 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 28 Aug 2018 12:45:04 +0100 Subject: [PATCH 04/10] Add spec for edge case --- spec/models/product_importer_spec.rb | 56 ++++++++++++++++++++++++++-- 1 file changed, 52 insertions(+), 4 deletions(-) diff --git a/spec/models/product_importer_spec.rb b/spec/models/product_importer_spec.rb index 1714e0f88e..d227c40d7e 100644 --- a/spec/models/product_importer_spec.rb +++ b/spec/models/product_importer_spec.rb @@ -16,6 +16,7 @@ describe ProductImport::ProductImporter do 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 @@ describe ProductImport::ProductImporter do 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 @@ describe ProductImport::ProductImporter do 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 @@ describe ProductImport::ProductImporter do 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') } @@ -286,7 +294,7 @@ describe ProductImport::ProductImporter do 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') } @@ -319,6 +327,46 @@ describe ProductImport::ProductImporter do 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 "importing items into inventory" do before do csv_data = CSV.generate do |csv| @@ -484,7 +532,7 @@ describe ProductImport::ProductImporter do @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 From 8de0355dc7884b10372b22ccc49a566973fb15cb Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 28 Aug 2018 14:05:57 +0100 Subject: [PATCH 05/10] Add spec for multiple stages during import --- spec/models/product_importer_spec.rb | 69 ++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/spec/models/product_importer_spec.rb b/spec/models/product_importer_spec.rb index d227c40d7e..b587ad8d76 100644 --- a/spec/models/product_importer_spec.rb +++ b/spec/models/product_importer_spec.rb @@ -367,6 +367,75 @@ describe ProductImport::ProductImporter do 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| From 04d50d455502405f59092ca4b0c4360ab2dce056 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 31 Aug 2018 15:00:29 +0100 Subject: [PATCH 06/10] Fix filter results bug in validation section --- .../admin/product_import/filters/filter_entries.js.coffee | 2 +- spec/features/admin/product_import_spec.rb | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/app/assets/javascripts/admin/product_import/filters/filter_entries.js.coffee b/app/assets/javascripts/admin/product_import/filters/filter_entries.js.coffee index 4a1007b3a9..efbcf5652b 100644 --- a/app/assets/javascripts/admin/product_import/filters/filter_entries.js.coffee +++ b/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' diff --git a/spec/features/admin/product_import_spec.rb b/spec/features/admin/product_import_spec.rb index 31477f1338..be6c01810c 100644 --- a/spec/features/admin/product_import_spec.rb +++ b/spec/features/admin/product_import_spec.rb @@ -201,11 +201,12 @@ feature "Product Import", js: true do 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" do + 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) @@ -215,9 +216,9 @@ feature "Product Import", js: true do import_data - expect(page).to have_selector '.item-count', text: "2" + 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: "2" + expect(page).to have_selector '.create-count', text: "3" expect(page).to_not have_selector '.update-count' expect(page).to_not have_selector '.update-count' expect(page).to_not have_selector '.inv-create-count' From 75a9ea5bfa79431bf039be30d3a942c0f7c849b1 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 31 Aug 2018 15:38:38 +0100 Subject: [PATCH 07/10] Prefer .flat_map(&foo) over .map(&foo).flatten --- app/models/product_import/entry_validator.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/product_import/entry_validator.rb b/app/models/product_import/entry_validator.rb index 873e1478e0..33a7932f4f 100644 --- a/app/models/product_import/entry_validator.rb +++ b/app/models/product_import/entry_validator.rb @@ -131,7 +131,7 @@ module ProductImport return end - products.map(&:variants).flatten.each do |existing_variant| + 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 @@ -182,7 +182,7 @@ module ProductImport return end - products.map(&:variants).flatten.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 From 4dfbbd60d44a700d1fd2ebc75861b57d6559fd4b Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 31 Aug 2018 19:13:17 +0100 Subject: [PATCH 08/10] Fix occasional StaleObjectError on variant updates --- app/models/product_import/entry_processor.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/app/models/product_import/entry_processor.rb b/app/models/product_import/entry_processor.rb index 97f9cbe55c..14cfd67f2d 100644 --- a/app/models/product_import/entry_processor.rb +++ b/app/models/product_import/entry_processor.rb @@ -115,7 +115,13 @@ module ProductImport 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 From d8bbcdc54bbc8c71327dc86f05daf248b8b2a95a Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 31 Aug 2018 22:52:44 +0100 Subject: [PATCH 09/10] Decrease batch size to reduce chance of timeouts --- .../product_import/controllers/import_form_controller.js.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/admin/product_import/controllers/import_form_controller.js.coffee b/app/assets/javascripts/admin/product_import/controllers/import_form_controller.js.coffee index 75b506fc66..d9b73cdabf 100644 --- a/app/assets/javascripts/admin/product_import/controllers/import_form_controller.js.coffee +++ b/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 From 9d05e5c97a4f5601bc0e8420735d48c36a31585b Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sat, 1 Sep 2018 14:09:07 +0100 Subject: [PATCH 10/10] Remove duplicate line in spec --- spec/features/admin/product_import_spec.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/features/admin/product_import_spec.rb b/spec/features/admin/product_import_spec.rb index be6c01810c..8043ab82a0 100644 --- a/spec/features/admin/product_import_spec.rb +++ b/spec/features/admin/product_import_spec.rb @@ -220,7 +220,6 @@ feature "Product Import", js: true do 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 '.update-count' expect(page).to_not have_selector '.inv-create-count' expect(page).to_not have_selector '.inv-update-count'