Refactor and additional permissions checks

Don't include non-permitted enterprises in existin product count

Tweaked feedback

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

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
 

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
 

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

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'}}

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'}