From cc5a335fb7456e6043a016cbd1d9fa551eab33a8 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley Date: Tue, 7 Mar 2017 14:31:44 +0000 Subject: [PATCH] Refactor and additional permissions checks Don't include non-permitted enterprises in existin product count Tweaked feedback --- .../controllers/import_options_form.js.coffee | 10 ++-- .../services/product_import_service.js.coffee | 14 ++--- app/models/product_importer.rb | 51 +++++++++++-------- .../product_import/_import_options.html.haml | 6 +-- .../product_import/_import_review.html.haml | 26 +++++----- .../product_import/_options_form.html.haml | 20 ++++---- 6 files changed, 67 insertions(+), 60 deletions(-) diff --git a/app/assets/javascripts/admin/product_import/controllers/import_options_form.js.coffee b/app/assets/javascripts/admin/product_import/controllers/import_options_form.js.coffee index e1c99a42b5..8cc4de5202 100644 --- a/app/assets/javascripts/admin/product_import/controllers/import_options_form.js.coffee +++ b/app/assets/javascripts/admin/product_import/controllers/import_options_form.js.coffee @@ -1,15 +1,15 @@ -angular.module("ofn.admin").controller "ImportOptionsFormCtrl", ($scope, $timeout, $rootScope, ProductImportService) -> +angular.module("ofn.admin").controller "ImportOptionsFormCtrl", ($scope, $rootScope, ProductImportService) -> $scope.toggleResetAbsent = () -> confirmed = confirm 'This will set stock level to zero on all products for this \n' + 'enterprise that are not present in the uploaded file.' if $scope.resetAbsent if confirmed or !$scope.resetAbsent - ProductImportService.updateResetAbsent($scope.supplierId, $scope.nonUpdated, $scope.resetAbsent) + ProductImportService.updateResetAbsent($scope.supplierId, $scope.resetCount, $scope.resetAbsent) else $scope.resetAbsent = false - $scope.resetCount = ProductImportService.resetCount + $scope.resetTotal = ProductImportService.resetTotal - $rootScope.$watch 'resetCount', (newValue) -> - $scope.resetCount = newValue if newValue || newValue == 0 + $rootScope.$watch 'resetTotal', (newValue) -> + $scope.resetTotal = newValue if newValue || newValue == 0 diff --git a/app/assets/javascripts/admin/product_import/services/product_import_service.js.coffee b/app/assets/javascripts/admin/product_import/services/product_import_service.js.coffee index d65da24e95..330e7b6cad 100644 --- a/app/assets/javascripts/admin/product_import/services/product_import_service.js.coffee +++ b/app/assets/javascripts/admin/product_import/services/product_import_service.js.coffee @@ -1,15 +1,15 @@ -angular.module("ofn.admin").factory "ProductImportService", ($rootScope, $timeout) -> +angular.module("ofn.admin").factory "ProductImportService", ($rootScope) -> new class ProductImportService suppliers: {} - resetCount: 0 + resetTotal: 0 - updateResetAbsent: (supplierId, nonUpdated, resetAbsent) -> + updateResetAbsent: (supplierId, resetCount, resetAbsent) -> if resetAbsent - @suppliers[supplierId] = nonUpdated - @resetCount += nonUpdated + @suppliers[supplierId] = resetCount + @resetTotal += resetCount else @suppliers[supplierId] = null - @resetCount -= nonUpdated + @resetTotal -= resetCount - $rootScope.resetCount = @resetCount + $rootScope.resetTotal = @resetTotal diff --git a/app/models/product_importer.rb b/app/models/product_importer.rb index 801fdd724e..c9f56dcf91 100644 --- a/app/models/product_importer.rb +++ b/app/models/product_importer.rb @@ -5,6 +5,8 @@ class ProductImporter include ActiveModel::Conversion include ActiveModel::Validations + attr_reader :total_supplier_products + def initialize(file, editable_enterprises, import_settings={}) if file.is_a?(File) @file = file @@ -25,7 +27,8 @@ class ProductImporter editable_enterprises.map { |e| @editable_enterprises[e.name] = e.id } @non_display_attributes = 'id', 'product_id', 'variant_id', 'supplier_id', 'primary_taxon_id', 'category_id', 'shipping_category_id', 'tax_category_id', 'on_hand_nil' - @supplier_products = {total: 0, by_supplier: {}} + @total_supplier_products = 0 + @products_to_reset = {} @updated_ids = [] validate_all if @sheet @@ -46,16 +49,16 @@ class ProductImporter @sheet ? @sheet.last_row - 1 : 0 end - def supplier_products - # Return indexed data about existing product count and update count per supplier - @supplier_products[:by_supplier].each do |supplier_id, supplier_data| - supplier_data[:updates_count] = 0 if supplier_data[:updates_count].blank? + def products_to_reset + # Return indexed data about existing product count, reset count, and updates count per supplier + @products_to_reset.each do |supplier_id, values| + values[:updates_count] = 0 if values[:updates_count].blank? - if supplier_data[:updates_count] and supplier_data[:existing_products] - @supplier_products[:by_supplier][supplier_id][:non_updated] = supplier_data[:existing_products] - supplier_data[:updates_count] + if values[:updates_count] and values[:existing_products] + @products_to_reset[supplier_id][:reset_count] = values[:existing_products] - values[:updates_count] end end - @supplier_products + @products_to_reset end def valid_count @@ -123,6 +126,14 @@ class ProductImporter delete_uploaded_file end + def permission_by_name?(supplier_name) + @editable_enterprises.has_key?(supplier_name) + end + + def permission_by_id?(supplier_id) + @editable_enterprises.has_value?(Integer(supplier_id)) + end + private def open_spreadsheet @@ -174,20 +185,20 @@ class ProductImporter def count_existing_products @suppliers_index.each do |supplier_name, supplier_id| - if supplier_id + if supplier_id and permission_by_id?(supplier_id) products_count = Spree::Variant.joins(:product). where('spree_products.supplier_id IN (?) AND spree_variants.is_master = false AND spree_variants.deleted_at IS NULL', supplier_id). count - if @supplier_products[:by_supplier][supplier_id] - @supplier_products[:by_supplier][supplier_id][:existing_products] = products_count + if @products_to_reset[supplier_id] + @products_to_reset[supplier_id][:existing_products] = products_count else - @supplier_products[:by_supplier][supplier_id] = {existing_products: products_count} + @products_to_reset[supplier_id] = {existing_products: products_count} end - @supplier_products[:total] += products_count + @total_supplier_products += products_count end end end @@ -210,7 +221,7 @@ class ProductImporter return end - unless permission_to_manage?(supplier_name) + unless permission_by_name?(supplier_name) mark_as_invalid(line_number, entry, "You do not have permission to manage products for \"#{supplier_name}\"") return end @@ -222,10 +233,6 @@ class ProductImporter @suppliers_index[supplier_name] end - def permission_to_manage?(supplier_name) - @editable_enterprises.has_key?(supplier_name) - end - def category_validation(line_number, entry) categories_index = @categories_index || get_categories_index category_name = entry['category'] @@ -345,7 +352,7 @@ class ProductImporter enterprises_to_reset = [] @import_settings.each do |enterprise_id, settings| - enterprises_to_reset.push enterprise_id if settings['reset_all_absent'] + enterprises_to_reset.push enterprise_id if settings['reset_all_absent'] and permission_by_id?(enterprise_id) end unless enterprises_to_reset.empty? or @updated_ids.empty? @@ -438,10 +445,10 @@ class ProductImporter end def updates_count_per_supplier(supplier_id) - if @supplier_products[:by_supplier][supplier_id] and @supplier_products[:by_supplier][supplier_id][:updates_count] - @supplier_products[:by_supplier][supplier_id][:updates_count] += 1 + if @products_to_reset[supplier_id] and @products_to_reset[supplier_id][:updates_count] + @products_to_reset[supplier_id][:updates_count] += 1 else - @supplier_products[:by_supplier][supplier_id] = {updates_count: 1} + @products_to_reset[supplier_id] = {updates_count: 1} end end diff --git a/app/views/admin/product_import/_import_options.html.haml b/app/views/admin/product_import/_import_options.html.haml index 242c0d8638..a87729a1c5 100644 --- a/app/views/admin/product_import/_import_options.html.haml +++ b/app/views/admin/product_import/_import_options.html.haml @@ -1,8 +1,8 @@ %h5 Import options and defaults %br -- @importer.suppliers_index.each do |name, id| - - if name and id +- @importer.suppliers_index.each do |name, supplier_id| + - if name and supplier_id and @importer.permission_by_id?(supplier_id) %div.panel-section.import-settings{ng: {controller: 'DropdownPanelsCtrl'}} %div.panel-header{ng: {click: 'togglePanel()', class: '{active: active}'}} %div.header-caret @@ -14,7 +14,7 @@ %div.header-description = name %div.panel-content{ng: {hide: '!active'}} - = render 'options_form', id: id, name: name + = render 'options_form', supplier_id: supplier_id, name: name %br.panels.clearfix diff --git a/app/views/admin/product_import/_import_review.html.haml b/app/views/admin/product_import/_import_review.html.haml index 4f547b0dbb..848e996db7 100644 --- a/app/views/admin/product_import/_import_review.html.haml +++ b/app/views/admin/product_import/_import_review.html.haml @@ -1,26 +1,26 @@ %h5 Import validation overview %br -%div.panel-section - %div.panel-header - %div.header-caret - %div.header-icon.info - %i.fa.fa-info-circle - %div.header-count - %strong.item-count= @importer.supplier_products[:total] - %div.header-description - Existing products in referenced enterprise(s) - -#%div.panel-content{ng: {hide: '!active'}} - -# Content goes here +-#%div.panel-section +-# %div.panel-header +-# %div.header-caret +-# %div.header-icon.info +-# %i.fa.fa-info-circle +-# %div.header-count +-# %strong.existing-count= @importer.total_supplier_products +-# %div.header-description +-# Existing products in referenced enterprise(s) +-# -#%div.panel-content{ng: {hide: '!active'}} +-# -# Content goes here -%div.panel-section{ng: {controller: 'ImportOptionsFormCtrl', hide: 'resetCount == 0'}} +%div.panel-section{ng: {controller: 'ImportOptionsFormCtrl', hide: 'resetTotal == 0'}} %div.panel-header %div.header-caret %div.header-icon.info %i.fa.fa-info-circle %div.header-count %strong.reset-count - {{resetCount}} + {{resetTotal}} %div.header-description Existing products will have their stock reset to zero -#%div.panel-content{ng: {hide: '!active'}} diff --git a/app/views/admin/product_import/_options_form.html.haml b/app/views/admin/product_import/_options_form.html.haml index c6a5c28030..da0e2df055 100644 --- a/app/views/admin/product_import/_options_form.html.haml +++ b/app/views/admin/product_import/_options_form.html.haml @@ -1,35 +1,35 @@ -%table.import-settings{ng: {controller: 'ImportOptionsFormCtrl', init: "supplierId = #{id}; nonUpdated = #{@importer.supplier_products[:by_supplier][id][:non_updated]}"}} +%table.import-settings{ng: {controller: 'ImportOptionsFormCtrl', init: "supplierId = #{supplier_id}; resetCount = #{@importer.products_to_reset[supplier_id][:reset_count]}"}} %tr %td.description Remove absent products? %td - = check_box_tag "settings[#{id}][reset_all_absent]", 1, false, :'ng-model' => 'resetAbsent', :'ng-change' => 'toggleResetAbsent()' + = check_box_tag "settings[#{supplier_id}][reset_all_absent]", 1, false, :'ng-model' => 'resetAbsent', :'ng-change' => 'toggleResetAbsent()' %td %tr %td.description Set default stock level %td - = select_tag "settings[#{id}][defaults][on_hand][mode]", options_for_select({"Don't overwrite" => :overwrite_none, "Overwrite all" => :overwrite_all, "Overwrite if empty" => :overwrite_empty}), {class: 'select2 fullwidth'} + = select_tag "settings[#{supplier_id}][defaults][on_hand][mode]", options_for_select({"Don't overwrite" => :overwrite_none, "Overwrite all" => :overwrite_all, "Overwrite if empty" => :overwrite_empty}), {class: 'select2 fullwidth'} %td - = number_field_tag "settings[#{id}][defaults][on_hand][value]", 0 + = number_field_tag "settings[#{supplier_id}][defaults][on_hand][value]", 0 %tr %td.description Set default tax category %td - = select_tag "settings[#{id}][defaults][tax_category_id][mode]", options_for_select({"Don't overwrite" => :overwrite_none, "Overwrite all" => :overwrite_all, "Overwrite if empty" => :overwrite_empty}), {class: 'select2 fullwidth'} + = select_tag "settings[#{supplier_id}][defaults][tax_category_id][mode]", options_for_select({"Don't overwrite" => :overwrite_none, "Overwrite all" => :overwrite_all, "Overwrite if empty" => :overwrite_empty}), {class: 'select2 fullwidth'} %td - = select_tag "settings[#{id}][defaults][tax_category_id][value]", options_for_select(@tax_categories.map {|tc| [tc.name, tc.id]}), {prompt: 'None', class: 'select2 fullwidth'} + = select_tag "settings[#{supplier_id}][defaults][tax_category_id][value]", options_for_select(@tax_categories.map {|tc| [tc.name, tc.id]}), {prompt: 'None', class: 'select2 fullwidth'} %tr %td.description Set default shipping category %td - = select_tag "settings[#{id}][defaults][shipping_category_id][mode]", options_for_select({"Don't overwrite" => :overwrite_none, "Overwrite all" => :overwrite_all, "Overwrite if empty" => :overwrite_empty}), {class: 'select2 fullwidth'} + = select_tag "settings[#{supplier_id}][defaults][shipping_category_id][mode]", options_for_select({"Don't overwrite" => :overwrite_none, "Overwrite all" => :overwrite_all, "Overwrite if empty" => :overwrite_empty}), {class: 'select2 fullwidth'} %td - = select_tag "settings[#{id}][defaults][shipping_category_id][value]", options_for_select(@shipping_categories.map {|sc| [sc.name, sc.id]}), {prompt: 'None', class: 'select2 fullwidth'} + = select_tag "settings[#{supplier_id}][defaults][shipping_category_id][value]", options_for_select(@shipping_categories.map {|sc| [sc.name, sc.id]}), {prompt: 'None', class: 'select2 fullwidth'} %tr %td.description Set default available date %td - = select_tag "settings[#{id}][defaults][available_on][mode]", options_for_select({"Don't overwrite" => :overwrite_none, "Overwrite all" => :overwrite_all, "Overwrite if empty" => :overwrite_empty}), {class: 'select2 fullwidth'} + = select_tag "settings[#{supplier_id}][defaults][available_on][mode]", options_for_select({"Don't overwrite" => :overwrite_none, "Overwrite all" => :overwrite_all, "Overwrite if empty" => :overwrite_empty}), {class: 'select2 fullwidth'} %td - = text_field_tag "settings[#{id}][defaults][available_on][value]", nil, {class: 'datepicker', placeholder: 'Today'} + = text_field_tag "settings[#{supplier_id}][defaults][available_on][value]", nil, {class: 'datepicker', placeholder: 'Today'}