From bf084cd8dfc14e2e1baca791b7fbc50ca73c542c Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Fri, 25 Jan 2019 00:28:56 +0800 Subject: [PATCH 1/5] Move import batch size outside method --- .../controllers/import_form_controller.js.coffee | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 95e6a67a5d..e645970a4f 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 @@ -10,6 +10,7 @@ angular.module("admin.productImport").controller "ImportFormCtrl", ($scope, $htt $scope.updated_ids = [] $scope.update_errors = [] + $scope.batchSize = 50 $scope.step = 'settings' $scope.chunks = 0 $scope.completed = 0 @@ -51,14 +52,13 @@ angular.module("admin.productImport").controller "ImportFormCtrl", ($scope, $htt $scope.start = () -> $scope.started = true total = ams_data.item_count - size = 50 - $scope.chunks = Math.ceil(total / size) + $scope.chunks = Math.ceil(total / $scope.batchSize) i = 0 while i < $scope.chunks - start = (i*size)+1 - end = (i+1)*size + start = (i * $scope.batchSize) + 1 + end = (i + 1) * $scope.batchSize if $scope.step == 'import' $scope.processImport(start, end) if $scope.step == 'save' From b67ba89aca2bfa2ccff5b61400717e727ca3bb8f Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Fri, 25 Jan 2019 00:32:31 +0800 Subject: [PATCH 2/5] Process batches of the import CSV one after another Start the HTTP request for processing next batches only after the previous batch has completed. A con of this approach is that the user needs to keep their browser open for later parts of the large CSV to be processed, but this is better than a higher chance of requests timing out. --- .../import_form_controller.js.coffee | 26 ++++++++++++------- 1 file changed, 17 insertions(+), 9 deletions(-) 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 e645970a4f..d1454307dc 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 @@ -54,16 +54,24 @@ angular.module("admin.productImport").controller "ImportFormCtrl", ($scope, $htt total = ams_data.item_count $scope.chunks = Math.ceil(total / $scope.batchSize) - i = 0 + # Process only the first batch. + $scope.processBatch($scope.step, 0, $scope.chunks) - while i < $scope.chunks - start = (i * $scope.batchSize) + 1 - end = (i + 1) * $scope.batchSize - if $scope.step == 'import' - $scope.processImport(start, end) - if $scope.step == 'save' - $scope.processSave(start, end) - i++ + $scope.processBatch = (step, batchIndex, batchCount) -> + start = (batchIndex * $scope.batchSize) + 1 + end = (batchIndex + 1) * $scope.batchSize + withNextBatch = batchIndex + 1 < batchCount + + promise = if step == 'import' + $scope.processImport(start, end) + else if step == 'save' + $scope.processSave(start, end) + + processNextBatch = -> + $scope.processBatch(step, batchIndex + 1, batchCount) + + # Process next batch whether or not processing of the current batch succeeds. + promise.then(processNextBatch, processNextBatch) if withNextBatch $scope.processImport = (start, end) -> $http( From b0efc3a2fa112538c1e399128ec406dcc6bfa9b2 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Fri, 25 Jan 2019 04:09:02 +0800 Subject: [PATCH 3/5] Add feature spec for large file import --- spec/features/admin/product_import_spec.rb | 72 +++++++++++++++++++++- 1 file changed, 71 insertions(+), 1 deletion(-) diff --git a/spec/features/admin/product_import_spec.rb b/spec/features/admin/product_import_spec.rb index d196a686e0..6649397e1c 100644 --- a/spec/features/admin/product_import_spec.rb +++ b/spec/features/admin/product_import_spec.rb @@ -361,6 +361,58 @@ feature "Product Import", js: true do end end + describe "handling a large file (120 data rows)" do + let!(:producer) { enterprise } + + let(:tmp_csv_path) { "/tmp/test.csv" } + + before do + quick_login_as admin + visit main_app.admin_product_import_path + end + + context "when importing to product list" do + def write_tmp_csv_file + CSV.open(tmp_csv_path, "w") do |csv| + csv << ["name", "producer", "category", "on_hand", "price", "units", "unit_type", + "tax_category", "shipping_category"] + 120.times do |i| + csv << ["Imported Product #{i + 1}", producer.name, category.name, 1, "1.00", "500", + "g", tax_category.name, shipping_category.name] + end + end + end + + before { write_tmp_csv_file } + + it "validates and saves all batches" do + # Upload and validate file. + attach_file "file", tmp_csv_path + click_button I18n.t("admin.product_import.index.upload") + proceed_to_validation + + # Check that all rows are validated. + heading = "120 #{I18n.t("admin.product_import.import.products_to_create")}" + find(".panel-header", text: heading).click + expect(page).to have_content "Imported Product 10" + expect(page).to have_content "Imported Product 60" + expect(page).to have_content "Imported Product 110" + + # Save file. + proceed_with_save + + # Be extra patient. + expect_progress_percentages "33%", "67%", "100%" + expect_import_completed + + # Check that all rows are saved. + expect(producer.supplied_products.find_by_name("Imported Product 10")).to be_present + expect(producer.supplied_products.find_by_name("Imported Product 60")).to be_present + expect(producer.supplied_products.find_by_name("Imported Product 110")).to be_present + end + end + end + private def import_data @@ -372,8 +424,26 @@ feature "Product Import", js: true do def save_data expect(page).to have_selector 'a.button.proceed', visible: true - click_link I18n.t('admin.product_import.import.save') + proceed_with_save expect(page).to have_selector 'div.save-results', visible: true + expect_import_completed + end + + def expect_progress_percentages(*percentages) + percentages.each do |percentage| + expect(page).to have_selector ".progress-interface", text: percentage + end + end + + def proceed_to_validation + import_data + end + + def proceed_with_save + click_link I18n.t("admin.product_import.import.save") + end + + def expect_import_completed expect(page).to have_content I18n.t('admin.product_import.save_results.final_results') end end From 4425b0416d021c958a8eaa4ec8edd61d37b05e63 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Fri, 25 Jan 2019 04:12:56 +0800 Subject: [PATCH 4/5] Rename import_data feature test method --- spec/features/admin/product_import_spec.rb | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/spec/features/admin/product_import_spec.rb b/spec/features/admin/product_import_spec.rb index 6649397e1c..eefa33d007 100644 --- a/spec/features/admin/product_import_spec.rb +++ b/spec/features/admin/product_import_spec.rb @@ -46,7 +46,7 @@ feature "Product Import", js: true do attach_file 'file', '/tmp/test.csv' click_button 'Upload' - import_data + proceed_to_validation expect(page).to have_selector '.item-count', text: "2" expect(page).to have_no_selector '.invalid-count' @@ -89,7 +89,7 @@ feature "Product Import", js: true do attach_file 'file', '/tmp/test.csv' click_button 'Upload' - import_data + proceed_to_validation expect(page).to have_selector '.item-count', text: "2" expect(page).to have_selector '.invalid-count', text: "2" @@ -112,7 +112,7 @@ feature "Product Import", js: true do attach_file 'file', '/tmp/test.csv' click_button 'Upload' - import_data + proceed_to_validation expect(page).to have_selector '.item-count', text: "1" expect(page).to have_selector '.create-count', text: "1" @@ -142,7 +142,7 @@ feature "Product Import", js: true do attach_file 'file', '/tmp/test.csv' click_button 'Upload' - import_data + proceed_to_validation save_data @@ -188,7 +188,7 @@ feature "Product Import", js: true do click_button 'Upload' - import_data + proceed_to_validation save_data @@ -213,7 +213,7 @@ feature "Product Import", js: true do attach_file 'file', '/tmp/test.csv' click_button 'Upload' - import_data + proceed_to_validation expect(page).to have_selector '.item-count', text: "3" expect(page).to_not have_selector '.invalid-count' @@ -252,7 +252,7 @@ feature "Product Import", js: true do attach_file 'file', '/tmp/test.csv' click_button 'Upload' - import_data + proceed_to_validation expect(page).to have_selector '.item-count', text: "3" expect(page).to have_no_selector '.invalid-count' @@ -349,7 +349,7 @@ feature "Product Import", js: true do attach_file 'file', '/tmp/test.csv' click_button 'Upload' - import_data + proceed_to_validation expect(page).to have_content I18n.t('admin.product_import.import.validation_overview') expect(page).to have_selector '.item-count', text: "2" @@ -415,7 +415,7 @@ feature "Product Import", js: true do private - def import_data + def proceed_to_validation expect(page).to have_selector 'a.button.proceed', visible: true click_link I18n.t('admin.product_import.import.import') expect(page).to have_selector 'form.product-import', visible: true @@ -435,10 +435,6 @@ feature "Product Import", js: true do end end - def proceed_to_validation - import_data - end - def proceed_with_save click_link I18n.t("admin.product_import.import.save") end From b6d8611359ed2cd701ee7997f095487c4bdcba92 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Fri, 25 Jan 2019 09:06:25 +0800 Subject: [PATCH 5/5] Return early in JS if last batch --- .../controllers/import_form_controller.js.coffee | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) 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 d1454307dc..174d0049d9 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 @@ -60,18 +60,20 @@ angular.module("admin.productImport").controller "ImportFormCtrl", ($scope, $htt $scope.processBatch = (step, batchIndex, batchCount) -> start = (batchIndex * $scope.batchSize) + 1 end = (batchIndex + 1) * $scope.batchSize - withNextBatch = batchIndex + 1 < batchCount + isLastBatch = batchCount == batchIndex + 1 promise = if step == 'import' $scope.processImport(start, end) else if step == 'save' $scope.processSave(start, end) + return if isLastBatch + processNextBatch = -> $scope.processBatch(step, batchIndex + 1, batchCount) # Process next batch whether or not processing of the current batch succeeds. - promise.then(processNextBatch, processNextBatch) if withNextBatch + promise.then(processNextBatch, processNextBatch) $scope.processImport = (start, end) -> $http(