From 170101cbfe9581c46e60c2660b303158a7d808ae Mon Sep 17 00:00:00 2001
From: Rohan Mitchell <rohan@rohanmitchell.com>
Date: Fri, 16 Dec 2016 15:24:17 +1100
Subject: [PATCH 01/18] Avoid reloading order during checkout request, which
 clears credit card number

---
 app/models/spree/payment_decorator.rb         |  1 -
 app/models/spree/tax_rate_decorator.rb        |  3 +-
 .../consumer/shopping/checkout_spec.rb        | 54 ++++++++++---------
 3 files changed, 31 insertions(+), 27 deletions(-)

diff --git a/app/models/spree/payment_decorator.rb b/app/models/spree/payment_decorator.rb
index dbee3cfd0f..2ae08430b5 100644
--- a/app/models/spree/payment_decorator.rb
+++ b/app/models/spree/payment_decorator.rb
@@ -16,7 +16,6 @@ module Spree
         adjustment.save
       else
         payment_method.create_adjustment(adjustment_label, order, self, true)
-        reload
       end
     end
 
diff --git a/app/models/spree/tax_rate_decorator.rb b/app/models/spree/tax_rate_decorator.rb
index 121d379584..7126e8ac29 100644
--- a/app/models/spree/tax_rate_decorator.rb
+++ b/app/models/spree/tax_rate_decorator.rb
@@ -12,7 +12,8 @@ module Spree
     def adjust_with_included_tax(order)
       adjust_without_included_tax(order)
 
-      order.reload
+      order.adjustments(:reload)
+      order.line_items(:reload)
       (order.adjustments.tax + order.price_adjustments).each do |a|
         a.set_absolute_included_tax! a.amount
       end
diff --git a/spec/features/consumer/shopping/checkout_spec.rb b/spec/features/consumer/shopping/checkout_spec.rb
index 16bdb27ec1..e1b696d0da 100644
--- a/spec/features/consumer/shopping/checkout_spec.rb
+++ b/spec/features/consumer/shopping/checkout_spec.rb
@@ -328,37 +328,41 @@ feature "As a consumer I want to check out my cart", js: true, retry: 3 do
             end
           end
 
-          context "with a credit card payment method" do
-            let!(:pm1) { create(:payment_method, distributors: [distributor], name: "Roger rabbit", type: "Spree::Gateway::Bogus") }
+          describe "credit card payments" do
+            ["Spree::Gateway::Bogus", "Spree::Gateway::BogusSimple"].each do |gateway_type|
+              context "with a credit card payment method using #{gateway_type}" do
+                let!(:pm1) { create(:payment_method, distributors: [distributor], name: "Roger rabbit", type: gateway_type) }
 
-            it "takes us to the order confirmation page when submitted with a valid credit card" do
-              toggle_payment
-              fill_in 'Card Number', with: "4111111111111111"
-              select 'February', from: 'secrets.card_month'
-              select (Date.current.year+1).to_s, from: 'secrets.card_year'
-              fill_in 'Security Code', with: '123'
+                it "takes us to the order confirmation page when submitted with a valid credit card" do
+                  toggle_payment
+                  fill_in 'Card Number', with: "4111111111111111"
+                  select 'February', from: 'secrets.card_month'
+                  select (Date.current.year+1).to_s, from: 'secrets.card_year'
+                  fill_in 'Security Code', with: '123'
 
-              place_order
-              page.should have_content "Your order has been processed successfully"
+                  place_order
+                  page.should have_content "Your order has been processed successfully"
 
-              # Order should have a payment with the correct amount
-              o = Spree::Order.complete.first
-              o.payments.first.amount.should == 11.23
-            end
+                  # Order should have a payment with the correct amount
+                  o = Spree::Order.complete.first
+                  o.payments.first.amount.should == 11.23
+                end
 
-            it "shows the payment processing failed message when submitted with an invalid credit card" do
-              toggle_payment
-              fill_in 'Card Number', with: "9999999988887777"
-              select 'February', from: 'secrets.card_month'
-              select (Date.current.year+1).to_s, from: 'secrets.card_year'
-              fill_in 'Security Code', with: '123'
+                it "shows the payment processing failed message when submitted with an invalid credit card" do
+                  toggle_payment
+                  fill_in 'Card Number', with: "9999999988887777"
+                  select 'February', from: 'secrets.card_month'
+                  select (Date.current.year+1).to_s, from: 'secrets.card_year'
+                  fill_in 'Security Code', with: '123'
 
-              place_order
-              page.should have_content "Payment could not be processed, please check the details you entered"
+                  place_order
+                  page.should have_content "Payment could not be processed, please check the details you entered"
 
-              # Does not show duplicate shipping fee
-              visit checkout_path
-              page.should have_selector "th", text: "Shipping", count: 1
+                  # Does not show duplicate shipping fee
+                  visit checkout_path
+                  page.should have_selector "th", text: "Shipping", count: 1
+                end
+              end
             end
           end
         end

From 8582e6d6b465e31922d4d2401069638214c3ca09 Mon Sep 17 00:00:00 2001
From: Rohan Mitchell <rohan@rohanmitchell.com>
Date: Fri, 23 Dec 2016 13:46:01 +1100
Subject: [PATCH 02/18] Add robustness check against intermittent spec failure

---
 spec/features/admin/enterprises_spec.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/spec/features/admin/enterprises_spec.rb b/spec/features/admin/enterprises_spec.rb
index c051c9eade..ce414ed99f 100644
--- a/spec/features/admin/enterprises_spec.rb
+++ b/spec/features/admin/enterprises_spec.rb
@@ -94,6 +94,7 @@ feature %q{
 
     within (".side_menu") { click_link "Users" }
     select2_search user.email, from: 'Owner'
+    expect(page).to have_no_selector '.select2-drop-mask' # Ensure select2 has finished
 
     click_link "About"
     fill_in 'enterprise_description', :with => 'Connecting farmers and eaters'

From 2cb3da56abe873c4204ed96e61520fb87a387566 Mon Sep 17 00:00:00 2001
From: Rohan Mitchell <rohan@rohanmitchell.com>
Date: Wed, 11 Jan 2017 14:51:10 +1100
Subject: [PATCH 03/18] Fix regression: Transaction fee double-charged

---
 app/models/spree/payment_decorator.rb         |  1 +
 app/views/checkout/_summary.html.haml         | 14 +++++-----
 .../consumer/shopping/checkout_spec.rb        | 26 +++++++++++++++++--
 3 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/app/models/spree/payment_decorator.rb b/app/models/spree/payment_decorator.rb
index 2ae08430b5..98b996cce7 100644
--- a/app/models/spree/payment_decorator.rb
+++ b/app/models/spree/payment_decorator.rb
@@ -16,6 +16,7 @@ module Spree
         adjustment.save
       else
         payment_method.create_adjustment(adjustment_label, order, self, true)
+        association(:adjustment).reload
       end
     end
 
diff --git a/app/views/checkout/_summary.html.haml b/app/views/checkout/_summary.html.haml
index 0fec419556..a6bb5ca651 100644
--- a/app/views/checkout/_summary.html.haml
+++ b/app/views/checkout/_summary.html.haml
@@ -4,30 +4,30 @@
       %legend
         = t :checkout_your_order
       %table
-        %tr
+        %tr.subtotal
           %th
             = t :checkout_cart_total
           %td.cart-total.text-right= display_checkout_subtotal(@order)
 
         - checkout_adjustments_for(current_order, exclude: [:shipping, :payment, :line_item]).reject{ |a| a.amount == 0 }.each do |adjustment|
-          %tr
+          %tr.adjustment
             %th= adjustment.label
             %td.text-right= adjustment.display_amount.to_html
 
-        %tr
+        %tr.shipping
           %th
             = t :checkout_shipping_price
-          %td.shipping.text-right {{ Checkout.shippingPrice() | localizeCurrency }}
+          %td.text-right {{ Checkout.shippingPrice() | localizeCurrency }}
 
-        %tr
+        %tr.transaction-fee
           %th
             = t :payment_method_fee
           %td.text-right {{ Checkout.paymentPrice() | localizeCurrency }}
 
-        %tr
+        %tr.total
           %th
             = t :checkout_total_price
-          %td.total.text-right {{ Checkout.cartTotal() | localizeCurrency }}
+          %td.text-right {{ Checkout.cartTotal() | localizeCurrency }}
 
       //= f.submit "Purchase", class: "button", "ofn-focus" => "accordion['payment']"
       %a.button.secondary{href: cart_url}
diff --git a/spec/features/consumer/shopping/checkout_spec.rb b/spec/features/consumer/shopping/checkout_spec.rb
index e1b696d0da..d9c9e8bf8f 100644
--- a/spec/features/consumer/shopping/checkout_spec.rb
+++ b/spec/features/consumer/shopping/checkout_spec.rb
@@ -1,6 +1,5 @@
 require 'spec_helper'
 
-
 feature "As a consumer I want to check out my cart", js: true, retry: 3 do
   include AuthenticationWorkflow
   include ShopWorkflow
@@ -31,7 +30,7 @@ feature "As a consumer I want to check out my cart", js: true, retry: 3 do
     let(:sm2) { create(:shipping_method, require_ship_address: false, name: "Donkeys", description: "blue", calculator: Spree::Calculator::FlatRate.new(preferred_amount: 4.56)) }
     let(:sm3) { create(:shipping_method, require_ship_address: false, name: "Local", tag_list: "local") }
     let!(:pm1) { create(:payment_method, distributors: [distributor], name: "Roger rabbit", type: "Spree::PaymentMethod::Check") }
-    let!(:pm2) { create(:payment_method, distributors: [distributor]) }
+    let!(:pm2) { create(:payment_method, distributors: [distributor], calculator: Spree::Calculator::FlatRate.new(preferred_amount: 5.67)) }
     let!(:pm3) do
       Spree::Gateway::PayPalExpress.create!(name: "Paypal", environment: 'test', distributor_ids: [distributor.id]).tap do |pm|
         pm.preferred_login = 'devnull-facilitator_api1.rohanmitchell.com'
@@ -40,6 +39,7 @@ feature "As a consumer I want to check out my cart", js: true, retry: 3 do
       end
     end
 
+
     before do
       distributor.shipping_methods << sm1
       distributor.shipping_methods << sm2
@@ -328,6 +328,28 @@ feature "As a consumer I want to check out my cart", js: true, retry: 3 do
             end
           end
 
+          context "when we are charged a payment method fee (transaction fee)" do
+            it "creates a payment including the transaction fee" do
+              # Selecting the transaction fee, it is displayed
+              expect(page).to have_selector ".transaction-fee td", text: "$0.00"
+              expect(page).to have_selector ".total", text: "$11.23"
+
+              toggle_payment
+              choose "#{pm2.name} ($5.67)"
+
+              expect(page).to have_selector ".transaction-fee td", text: "$5.67"
+              expect(page).to have_selector ".total", text: "$16.90"
+
+              place_order
+              expect(page).to have_content "Your order has been processed successfully"
+
+              # There are two orders - our order and our new cart
+              o = Spree::Order.complete.first
+              expect(o.adjustments.payment_fee.first.amount).to eq 5.67
+              expect(o.payments.first.amount).to eq(10 + 1.23 + 5.67) # items + fees + transaction
+            end
+          end
+
           describe "credit card payments" do
             ["Spree::Gateway::Bogus", "Spree::Gateway::BogusSimple"].each do |gateway_type|
               context "with a credit card payment method using #{gateway_type}" do

From 2ad433590d69c2389b02c098ec51b99908e12500 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <visible_souls@hotmail.com>
Date: Tue, 14 Feb 2017 12:26:26 +0000
Subject: [PATCH 04/18] Add roo gem

---
 Gemfile      | 1 +
 Gemfile.lock | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/Gemfile b/Gemfile
index 45b260c4c8..2b8f6bdddc 100644
--- a/Gemfile
+++ b/Gemfile
@@ -63,6 +63,7 @@ gem 'wkhtmltopdf-binary'
 
 gem 'foreigner'
 gem 'immigrant'
+gem 'roo', '~> 2.7.0'
 
 gem 'whenever', require: false
 
diff --git a/Gemfile.lock b/Gemfile.lock
index e03e350866..20366a9107 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -558,6 +558,9 @@ GEM
     roadie-rails (1.0.3)
       rails (>= 3.0, < 4.2)
       roadie (~> 3.0)
+    roo (2.7.1)
+      nokogiri (~> 1)
+      rubyzip (~> 1.1, < 2.0.0)
     rspec (2.14.1)
       rspec-core (~> 2.14.0)
       rspec-expectations (~> 2.14.0)
@@ -576,6 +579,7 @@ GEM
     rspec-retry (0.4.2)
       rspec-core
     ruby-progressbar (1.7.1)
+    rubyzip (1.2.0)
     safe_yaml (0.9.5)
     sass (3.3.14)
     sass-rails (3.2.6)
@@ -717,6 +721,7 @@ DEPENDENCIES
   redcarpet
   representative_view
   roadie-rails (~> 1.0.3)
+  roo (~> 2.7.0)
   rspec-rails
   rspec-retry
   sass (~> 3.3)

From c0c6cd1a607947ab40f685127056822a93227d86 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <visible_souls@hotmail.com>
Date: Tue, 14 Feb 2017 12:33:53 +0000
Subject: [PATCH 05/18] Product Import feature

---
 .../admin/product_import_controller.rb        |  52 +++
 app/models/product_importer.rb                | 314 ++++++++++++++++++
 .../admin/product_import/import.html.haml     |  67 ++++
 .../admin/product_import/index.html.haml      |  14 +
 app/views/admin/product_import/save.html.haml |  24 ++
 config/routes.rb                              |   5 +-
 6 files changed, 475 insertions(+), 1 deletion(-)
 create mode 100644 app/controllers/admin/product_import_controller.rb
 create mode 100644 app/models/product_importer.rb
 create mode 100644 app/views/admin/product_import/import.html.haml
 create mode 100644 app/views/admin/product_import/index.html.haml
 create mode 100644 app/views/admin/product_import/save.html.haml

diff --git a/app/controllers/admin/product_import_controller.rb b/app/controllers/admin/product_import_controller.rb
new file mode 100644
index 0000000000..aba3a8833a
--- /dev/null
+++ b/app/controllers/admin/product_import_controller.rb
@@ -0,0 +1,52 @@
+require 'roo'
+
+class Admin::ProductImportController < Spree::Admin::BaseController
+
+  before_filter :check_upload, except: :index
+
+  def import
+    # Save uploaded file to tmp directory
+    @filepath = save_upload(params[:file])
+    @importer = ProductImporter.new(File.new(@filepath), editable_enterprises)
+
+    if @importer.errors.present?
+      flash[:notice] = @importer.errors.full_messages.to_sentence
+    end
+  end
+
+  def save
+    file = File.new(params[:filepath])
+    @importer = ProductImporter.new(file, editable_enterprises)
+    @importer.save_all_valid
+
+    if @importer.updated_count && @importer.updated_count > 0
+      File.delete(file)
+      flash[:success] = "#{@importer.updated_count} records updated successfully"
+    else
+      flash[:notice] = @importer.errors.full_messages.to_sentence
+    end
+  end
+
+  private
+
+  def check_upload
+    unless params[:file] || (params[:filepath] && File.exist?(params[:filepath]))
+      redirect_to '/admin/product_import', :notice => 'File not found or could not be opened'
+      return
+    end
+  end
+
+  def save_upload(upload)
+    filename = Time.now.strftime('%d-%m-%Y-%H-%M-%S')
+    extension = '.' + upload.original_filename.split('.').last
+    File.open(Rails.root.join('tmp', filename+extension), 'wb') do |f|
+      f.write(upload.read)
+      f.path
+    end
+  end
+
+  # Define custom model class for Cancan permissions
+  def model_class
+    ProductImporter
+  end
+end
\ No newline at end of file
diff --git a/app/models/product_importer.rb b/app/models/product_importer.rb
new file mode 100644
index 0000000000..545b903f12
--- /dev/null
+++ b/app/models/product_importer.rb
@@ -0,0 +1,314 @@
+require 'roo'
+
+class ProductImporter
+  extend ActiveModel::Naming
+  include ActiveModel::Conversion
+  include ActiveModel::Validations
+
+  def initialize(file, editable_enterprises, options={})
+    if file.is_a?(File)
+      @file = file
+      @options = options
+      @sheet = open_spreadsheet
+      @valid_entries = {}
+      @invalid_entries = {}
+
+      @products_to_create = []
+      @variants_to_create = []
+      @variants_to_update = []
+      
+      @products_created = 0
+      @variants_created = 0
+      @variants_updated = 0
+
+      @editable_enterprises = {}
+      editable_enterprises.map { |e| @editable_enterprises[e.name] = e.id }
+
+      validate_all
+    else
+      self.errors.add(:importer, "error: no file uploaded")
+    end
+  end
+
+  def persisted?
+    false #ActiveModel, not ActiveRecord
+  end
+
+  # Private methods below which only work with a valid spreadsheet object can be called publicly
+  # via here if the spreadsheet was successfully loaded, otherwise they return nil (without error).
+  def method_missing(method, *args, &block)
+    if self.respond_to?(method, include_private=true)
+      @sheet ? self.send(method, *args, &block) : nil
+    else
+      super
+    end
+  end
+
+  private
+
+  def open_spreadsheet
+    if accepted_mimetype
+      @sheet = Roo::Spreadsheet.open(@file, extension: accepted_mimetype)
+    else
+      self.errors.add(:importer, "could not proccess file: invalid filetype")
+      nil
+    end
+  end
+
+  def accepted_mimetype
+    File.extname(@file.path).in?(['.csv', '.xls', '.xlsx', '.ods']) ? @file.path.split('.').last.to_sym : false
+
+    # case @file.content_type
+    #   when "text/csv"
+    #     :csv
+    #   when "application/excel", "application/x-excel", "application/x-msexcel", "application/vnd.ms-excel"
+    #     :xls
+    #   when "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet"
+    #     :xlsx
+    #   when "application/vnd.oasis.opendocument.spreadsheet"
+    #     :ods
+    #   else
+    #     #Mimetype not compatible
+    #     false
+    # end
+  end
+
+  def headers
+    @sheet.row(1)
+  end
+
+  def rows
+    (2..@sheet.last_row).map do |i|
+      @sheet.row(i)
+    end
+  end
+
+  def entries
+    rows.map do |row|
+      Hash[[headers, row].transpose]
+    end
+  end
+
+  def validate_all
+    suppliers_index = @suppliers_index || get_suppliers_index
+    categories_index = @categories_index || get_categories_index
+
+    entries.each_with_index do |entry, i|
+
+      # Fetch/assign and validate supplier id
+      supplier_name = entry['supplier']
+      if supplier_name.blank?
+        invalidate({i+2 => {entry: entry, errors: ["Supplier name field is empty"]}}) #unless entry['supplier_id']
+      else
+        if suppliers_index[supplier_name]
+          entry['supplier_id'] = suppliers_index[supplier_name]
+        else
+          invalidate({i+2 => {entry: entry, errors: ["Supplier \"#{supplier_name}\" not found in database"]}})
+          #next
+        end
+
+        # Check enterprise permissions
+        unless @editable_enterprises[supplier_name]
+          invalidate({i+2 => {entry: entry, errors: ["You do not have permission to manage products for \"#{supplier_name}\""]}})
+          #next
+        end
+      end
+
+      # Fetch/assign and validate category id
+      category_name = entry['category']
+      if category_name.blank?
+        invalidate({i+2 => {entry: entry, errors: ["Category field is empty"]}}) #unless entry['primary_taxon_id']
+      else
+        if categories_index[category_name]
+          entry['primary_taxon_id'] = categories_index[category_name]
+        else
+          invalidate({i+2 => {entry: entry, errors: ["Category \"#{category_name}\" not found in database"]}})
+          #next
+        end
+      end
+
+      # Ensure on_hand isn't nil (because Spree::Product and Spree::Variant each validate this differently)
+      entry['on_hand'] = 0 if entry['on_hand'].nil?
+
+      # Check if entry can be updated/saved; assign updatable status
+      set_update_status(entry, i)
+
+      # Add valid entry
+      @valid_entries[i+2] = {entry: entry} unless @invalid_entries[i+2]
+    end
+  end
+
+  def invalidate(invalid_line)
+    # Update exiting errors array for this line, if it exists
+    @invalid_entries.each do |line, data|
+      if invalid_line[line]
+        @invalid_entries[line][:errors] += invalid_line[line][:errors]
+        return
+      end
+    end
+    # Otherwise add new entry
+    @invalid_entries.merge!(invalid_line)
+  end
+
+  # Minimise db queries by getting a list of suppliers to look
+  # up, instead of doing a query for each entry in the spreadsheet
+  def get_suppliers_index
+    @suppliers_index ||= {}
+    entries.each do |entry|
+      supplier_name = entry['supplier']
+      supplier_id = @suppliers_index[supplier_name] ||
+          Enterprise.find_by_name(supplier_name, :select => 'id, name').try(:id)
+      @suppliers_index[supplier_name] = supplier_id
+    end
+    @suppliers_index
+  end
+
+  def get_categories_index
+    @categories_index ||= {}
+    entries.each do |entry|
+      category_name = entry['category']
+      category_id = @categories_index[category_name] ||
+          Spree::Taxon.find_by_name(category_name, :select => 'id, name').try(:id)
+      @categories_index[category_name] = category_id
+    end
+    @categories_index
+  end
+
+  def save_all_valid
+    updated = {}
+    @products_to_create.each do |entry|
+      # If we've already added a new product with these attributes from
+      # this spreadsheet, pass this entry to @variants_to_create with
+      # the new product id, as this is a now variant of that product...
+      if updated[entry['supplier_id']] && updated[entry['supplier_id']][entry['name']]
+        product_id = updated[entry['supplier_id']][entry['name']]
+        new_variant = Spree::Variant.new(entry.except('id', 'product_id'))
+        new_variant.product_id = product_id
+        @variants_to_create.push(new_variant)
+        next
+      end
+
+      product = Spree::Product.new()
+      product.assign_attributes(entry.except('id'))
+      if product.save
+        # Ensure display_name and on_demand are copied to new variant
+        if entry['display_name'] || entry['on_demand']
+          variant = product.variants.first
+          variant.display_name = entry['display_name'] if entry['display_name']
+          variant.on_demand = entry['on_demand'] if entry['on_demand']
+          variant.save
+        end
+        @products_created += 1
+      else
+        self.errors.add(:importer, product.errors.full_messages)
+      end
+
+      updated[entry['supplier_id']] = {entry['name'] => product.id}
+    end
+
+    @variants_to_update.each do |variant|
+      if variant.save
+        @variants_updated += 1
+      else
+        self.errors.add(:importer, variant.errors.full_messages)
+      end
+    end
+
+    @variants_to_create.each do |new_variant|
+      if new_variant.save
+        @variants_created += 1
+      else
+        self.errors.add(:importer, new_variant.errors.full_messages)
+      end
+    end
+
+    self.errors.add(:importer, "did not save any products successfully") if updated_count == 0
+
+    updated_count
+  end
+
+  def set_update_status(entry, i)
+    # Find product with matching supplier and name
+    match = Spree::Product.where(supplier_id: entry['supplier_id'], name: entry['name'], deleted_at: nil).first
+
+    # If no matching product was found, create a new product
+    if match.nil?
+      # Check product validations
+      new_product = Spree::Product.new()
+      new_product.assign_attributes(entry.except('id'))
+      if new_product.valid?
+        @products_to_create.push(entry) unless @invalid_entries[i+2]
+      else
+        invalidate({i+2 => {entry: entry, errors: new_product.errors.full_messages}})
+      end
+      return
+    end
+
+    # Otherwise, if a variant exists with matching display_name and unit_value, update it
+    match.variants.each do |existing_variant|
+      if existing_variant.display_name == entry['display_name'] && existing_variant.unit_value == Float(entry['unit_value'])
+        # Check updated variant would be valid
+        existing_variant.assign_attributes(entry.except('id', 'product_id'))
+        if existing_variant.valid?
+          @variants_to_update.push(existing_variant) unless @invalid_entries[i+2]
+        else
+          invalidate({i+2 => {entry: entry, errors: existing_variant.errors.full_messages}})
+        end
+        return
+      end
+    end
+
+    # Otherwise, a variant with sufficiently matching attributes doesn't exist; create a new one
+    new_variant = Spree::Variant.new(entry.except('id', 'product_id'))
+    new_variant.product_id = match.id
+    if new_variant.valid?
+      @variants_to_create.push(new_variant) unless @invalid_entries[i+2]
+    else
+      invalidate({i+2 => {entry: entry, errors: new_variant.errors.full_messages}})
+    end
+  end
+
+  def has_valid_entries?
+    valid_count > 0
+  end
+
+  def item_count
+    @sheet.last_row - 1
+  end
+
+  def valid_count
+    @valid_entries.count
+  end
+
+  def invalid_count
+    @invalid_entries.count
+  end
+
+  def valid_entries
+    @valid_entries
+  end
+
+  def invalid_entries
+    @invalid_entries
+  end
+
+  def products_create_count
+    @products_to_create.count + @variants_to_create.count
+  end
+
+  def products_update_count
+    @variants_to_update.count
+  end
+
+  def products_created
+    @products_created + @variants_created
+  end
+
+  def products_updated
+    @variants_updated
+  end
+
+  def updated_count
+    @products_created + @variants_created + @variants_updated
+  end
+end
diff --git a/app/views/admin/product_import/import.html.haml b/app/views/admin/product_import/import.html.haml
new file mode 100644
index 0000000000..beab046b29
--- /dev/null
+++ b/app/views/admin/product_import/import.html.haml
@@ -0,0 +1,67 @@
+- content_for :page_title do
+  Product Import
+
+= render :partial => 'spree/admin/shared/product_sub_menu'
+
+- if @importer.valid_count && @importer.invalid_count
+  %h5 Products imported from spreadsheet:
+  %br
+  %p{style: "font-size: 1.15em"}
+    Valid entries found:
+    = @importer.valid_count
+  %p{style: "font-size: 1.15em"}
+    Invalid entries found:
+    = @importer.invalid_count
+  %br
+  %p{style: "font-size: 1.15em"}
+    Products to be created:
+    = @importer.products_create_count
+  %p{style: "font-size: 1.15em"}
+    Products to be updated:
+    = @importer.products_update_count
+
+- if @importer.invalid_entries && @importer.invalid_entries.count > 0
+  %br
+  %h5 Import errors:
+  %br
+  - @importer.invalid_entries.each do |line, item|
+    %p{style: "font-size: 1.15em"}
+      %strong
+        Item line #{line}:
+      %span= item[:entry]['name']
+      - if item[:entry]['display_name']
+        ( #{item[:entry]['display_name']} )
+    - item[:errors].each do |error|
+      %p{class: "red"}
+        &nbsp;-&nbsp; #{error}
+  %br
+  %h5 Review invalid entries:
+  %div{style: 'width: 100%; overflow: auto;'}
+    %table
+      %thead
+        %th Line
+        - @importer.invalid_entries.values.first[:entry].each do |key, value|
+          %th{style: 'white-space: nowrap;'}= key
+      - @importer.invalid_entries.each do |line, item|
+        %tr
+          %td= line
+          - item[:entry].each do |key, value|
+            %td{style: 'white-space: nowrap;'}= value
+
+- if @importer.has_valid_entries?
+  %br
+  - if @importer.invalid_count > 0
+    %h5 Invalid entries detected.
+    %p Save valid products for now and discard the others?
+  - else
+    %h5 No errors detected!
+    %p Save all imported products?
+  %br
+  = form_tag main_app.admin_product_import_save_path, multipart: true do
+    = hidden_field_tag :filepath, @filepath
+    = submit_tag "Save"
+    %a.button{href: main_app.admin_product_import_path} Cancel
+
+- if @importer.invalid_count && !@importer.has_valid_entries?
+  %br
+  %a.button{href: main_app.admin_product_import_path} Cancel
diff --git a/app/views/admin/product_import/index.html.haml b/app/views/admin/product_import/index.html.haml
new file mode 100644
index 0000000000..6ca97a0686
--- /dev/null
+++ b/app/views/admin/product_import/index.html.haml
@@ -0,0 +1,14 @@
+- content_for :page_title do
+  Product Import
+
+= render :partial => 'spree/admin/shared/product_sub_menu'
+
+%h5 Select a spreadsheet to upload
+%br
+= form_tag main_app.admin_product_import_path, multipart: true do
+  = file_field_tag :file
+  %br
+  %br
+  = submit_tag "Import"
+  %br
+  %br
diff --git a/app/views/admin/product_import/save.html.haml b/app/views/admin/product_import/save.html.haml
new file mode 100644
index 0000000000..bca2163805
--- /dev/null
+++ b/app/views/admin/product_import/save.html.haml
@@ -0,0 +1,24 @@
+- content_for :page_title do
+  Product Import
+
+= render :partial => 'spree/admin/shared/product_sub_menu'
+
+%h5 Import results
+%br
+
+%p{style: 'font-size: 1.15em'}
+  Products created:
+  = @importer.products_created
+%p{style: 'font-size: 1.15em'}
+  Products updated:
+  = @importer.products_updated
+
+- if @importer.errors.full_messages && !@importer.errors.full_messages.blank?
+  %br
+  %h6 Errors
+  - @importer.errors.full_messages.each do |error|
+    %p{class: "red"}
+      &nbsp;-&nbsp; #{error}
+
+%br
+  %a.button{href: main_app.admin_product_import_path} Back
diff --git a/config/routes.rb b/config/routes.rb
index 04ad87568e..90700d89b0 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -114,6 +114,10 @@ Openfoodnetwork::Application.routes.draw do
 
     get '/inventory', to: 'variant_overrides#index'
 
+    get '/product_import', to: 'product_import#index'
+    post '/product_import', to: 'product_import#import'
+    post '/product_import/save', to: 'product_import#save', as: 'product_import_save'
+
     resources :variant_overrides do
       post :bulk_update, on: :collection
       post :bulk_reset, on: :collection
@@ -237,7 +241,6 @@ Spree::Core::Engine.routes.prepend do
 
   namespace :admin do
     get '/search/known_users' => "search#known_users", :as => :search_known_users
-
     get '/search/customers' => 'search#customers', :as => :search_customers
 
     resources :products do

From 6e5c87849198cc585946f595b0dfe509750697db Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <visible_souls@hotmail.com>
Date: Tue, 14 Feb 2017 12:37:26 +0000
Subject: [PATCH 06/18] Product Import cancan permissions and ui tab

---
 app/models/spree/ability_decorator.rb                         | 2 ++
 .../_product_sub_menu/add_product_import_tab.html.haml.deface | 4 ++++
 2 files changed, 6 insertions(+)
 create mode 100644 app/overrides/spree/admin/shared/_product_sub_menu/add_product_import_tab.html.haml.deface

diff --git a/app/models/spree/ability_decorator.rb b/app/models/spree/ability_decorator.rb
index 13efc7e109..e49db570cb 100644
--- a/app/models/spree/ability_decorator.rb
+++ b/app/models/spree/ability_decorator.rb
@@ -155,6 +155,8 @@ class AbilityDecorator
     can [:admin, :index, :read, :search], Spree::Taxon
     can [:admin, :index, :read, :create, :edit], Spree::Classification
 
+    can [:admin, :index, :import, :save], ProductImporter
+
     # Reports page
     can [:admin, :index, :customers, :orders_and_distributors, :group_buys, :bulk_coop, :payments, :orders_and_fulfillment, :products_and_inventory, :order_cycle_management, :packing], :report
   end
diff --git a/app/overrides/spree/admin/shared/_product_sub_menu/add_product_import_tab.html.haml.deface b/app/overrides/spree/admin/shared/_product_sub_menu/add_product_import_tab.html.haml.deface
new file mode 100644
index 0000000000..f3029e55cf
--- /dev/null
+++ b/app/overrides/spree/admin/shared/_product_sub_menu/add_product_import_tab.html.haml.deface
@@ -0,0 +1,4 @@
+/ insert_bottom "[data-hook='admin_product_sub_tabs']"
+
+-# Commenting out for now, until product import is finished
+-# = tab :product_import, label: "Import", url: main_app.admin_product_import_path, match_path: '/product_import'

From 052d6c6b0239bca5f4ae05ad4fd2c7b5b53b6f97 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <visible_souls@hotmail.com>
Date: Tue, 14 Feb 2017 12:37:55 +0000
Subject: [PATCH 07/18] Product Import basic specs

---
 spec/features/admin/product_import_spec.rb | 96 ++++++++++++++++++++++
 spec/models/product_importer_spec.rb       | 33 ++++++++
 2 files changed, 129 insertions(+)
 create mode 100644 spec/features/admin/product_import_spec.rb
 create mode 100644 spec/models/product_importer_spec.rb

diff --git a/spec/features/admin/product_import_spec.rb b/spec/features/admin/product_import_spec.rb
new file mode 100644
index 0000000000..ebf34026c4
--- /dev/null
+++ b/spec/features/admin/product_import_spec.rb
@@ -0,0 +1,96 @@
+require 'spec_helper'
+require 'open_food_network/permissions'
+
+feature "Product Import", js: true do
+  include AuthenticationWorkflow
+  include WebHelper
+
+  let!(:admin) { create(:admin_user) }
+  let!(:user) { create_enterprise_user }
+  let!(:enterprise) { create(:enterprise, owner: user, name: "Test Enterprise") }
+  let!(:category) { create(:taxon, name: 'Vegetables') }
+  let(:permissions) { OpenFoodNetwork::Permissions.new(user) }
+
+  describe "product import" do
+    before { quick_login_as_admin }
+    after { File.delete('/tmp/test.csv') }
+
+    it "validates entries and saves them if they are all valid" do
+      csv_data = CSV.generate do |csv|
+        csv << ["name", "supplier", "category", "on_hand", "price", "unit_value", "variant_unit", "variant_unit_scale"]
+        csv << ["Carrots", "Test Enterprise", "Vegetables", "5", "3.20", "500", "weight", "1"]
+        csv << ["Potatoes", "Test Enterprise", "Vegetables", "6", "6.50", "1000", "weight", "1000"]
+      end
+      File.write('/tmp/test.csv', csv_data)
+
+      visit main_app.admin_product_import_path
+
+      expect(page).to have_content "Select a spreadsheet to upload"
+      attach_file('file', '/tmp/test.csv')
+      click_button('Import')
+
+      expect(page).to have_content("Valid entries found: 2")
+      expect(page).to have_content("Invalid entries found: 0")
+      click_button('Save')
+
+      expect(page).to have_content("Products created: 2")
+
+      potatoes = Spree::Product.find_by_name('Potatoes')
+      potatoes.supplier.should == enterprise
+      potatoes.on_hand.should == 6
+      potatoes.price.should == 6.50
+    end
+
+    it "displays info about invalid entries but still allows saving of valid entries" do
+      csv_data = CSV.generate do |csv|
+        csv << ["name", "supplier", "category", "on_hand", "price", "unit_value", "variant_unit", "variant_unit_scale"]
+        csv << ["Good Carrots", "Test Enterprise", "Vegetables", "5", "3.20", "500", "weight", "1"]
+        csv << ["Bad Potatoes", "", "Vegetables", "6", "6.50", "1000", "", "1000"]
+      end
+      File.write('/tmp/test.csv', csv_data)
+
+      visit main_app.admin_product_import_path
+
+      expect(page).to have_content "Select a spreadsheet to upload"
+      attach_file('file', '/tmp/test.csv')
+      click_button('Import')
+
+      expect(page).to have_content("Valid entries found: 1")
+      expect(page).to have_content("Invalid entries found: 1")
+      expect(page).to have_content("errors")
+
+      click_button('Save')
+
+      expect(page).to have_content("Products created: 1")
+
+      Spree::Product.find_by_name('Bad Potatoes').should == nil
+      carrots = Spree::Product.find_by_name('Good Carrots')
+      carrots.supplier.should == enterprise
+      carrots.on_hand.should == 5
+      carrots.price.should == 3.20
+    end
+
+    it "displays info about invalid entries but no save button if all invalid" do
+      csv_data = CSV.generate do |csv|
+        csv << ["name", "supplier", "category", "on_hand", "price", "unit_value", "variant_unit", "variant_unit_scale"]
+        csv << ["Bad Carrots", "Unkown Enterprise", "Mouldy vegetables", "666", "3.20", "", "weight", ""]
+        csv << ["Bad Potatoes", "", "Vegetables", "6", "6", "6", "", "1000"]
+      end
+      File.write('/tmp/test.csv', csv_data)
+
+      visit main_app.admin_product_import_path
+
+      expect(page).to have_content "Select a spreadsheet to upload"
+      attach_file('file', '/tmp/test.csv')
+      click_button('Import')
+
+      expect(page).to have_content("Valid entries found: 0")
+      expect(page).to have_content("Invalid entries found: 2")
+      expect(page).to have_content("errors")
+
+      expect(page).to_not have_selector('input[type=submit]', text: "Save")
+    end
+  end
+
+  # Test enterprise permissions with non-admin user
+end
\ No newline at end of file
diff --git a/spec/models/product_importer_spec.rb b/spec/models/product_importer_spec.rb
new file mode 100644
index 0000000000..e51bf91cea
--- /dev/null
+++ b/spec/models/product_importer_spec.rb
@@ -0,0 +1,33 @@
+require 'spec_helper'
+require 'open_food_network/permissions'
+
+describe ProductImporter do
+  include AuthenticationWorkflow
+
+  let!(:admin) { create(:admin_user) }
+  let!(:user) { create_enterprise_user }
+  let!(:enterprise) { create(:enterprise, owner: user, name: "Test Enterprise") }
+  let!(:category) { create(:taxon, name: 'Vegetables') }
+  let(:permissions) { OpenFoodNetwork::Permissions.new(user) }
+
+  describe "importing products from a spreadsheet" do
+    after { File.delete('/tmp/test-m.csv') }
+
+    it "validates the entries" do
+      csv_data = CSV.generate do |csv|
+        csv << ["name", "supplier", "category", "on_hand", "price", "unit_value", "variant_unit", "variant_unit_scale"]
+        csv << ["Carrots", "Test Enterprise", "Vegetables", "5", "3.20", "500", "weight", "1"]
+        csv << ["Potatoes", "Test Enterprise", "Vegetables", "6", "6.50", "1000", "weight", "1000"]
+      end
+      File.write('/tmp/test-m.csv', csv_data)
+      file = File.new('/tmp/test-m.csv')
+
+      importer = ProductImporter.new(file, permissions.editable_enterprises)
+
+      expect(importer.valid_count).to eq(2)
+      expect(importer.invalid_count).to eq(0)
+    end
+  end
+
+  # Test handling of filetypes
+end

From 6b7cdf3a37eec6d546af672229e15018f87b7fa5 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <visible_souls@hotmail.com>
Date: Sat, 25 Feb 2017 20:37:54 +0000
Subject: [PATCH 08/18] Product Import Refactoring

---
 app/models/product_importer.rb | 179 ++++++++++++++++-----------------
 1 file changed, 89 insertions(+), 90 deletions(-)

diff --git a/app/models/product_importer.rb b/app/models/product_importer.rb
index 545b903f12..97d67eea23 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 :valid_entries, :invalid_entries
+
   def initialize(file, editable_enterprises, options={})
     if file.is_a?(File)
       @file = file
@@ -34,8 +36,9 @@ class ProductImporter
     false #ActiveModel, not ActiveRecord
   end
 
-  # Private methods below which only work with a valid spreadsheet object can be called publicly
-  # via here if the spreadsheet was successfully loaded, otherwise they return nil (without error).
+  # Private methods below which only work with a valid spreadsheet object can
+  # be called publicly via here if the spreadsheet was successfully loaded,
+  # otherwise they return nil (without error).
   def method_missing(method, *args, &block)
     if self.respond_to?(method, include_private=true)
       @sheet ? self.send(method, *args, &block) : nil
@@ -57,20 +60,6 @@ class ProductImporter
 
   def accepted_mimetype
     File.extname(@file.path).in?(['.csv', '.xls', '.xlsx', '.ods']) ? @file.path.split('.').last.to_sym : false
-
-    # case @file.content_type
-    #   when "text/csv"
-    #     :csv
-    #   when "application/excel", "application/x-excel", "application/x-msexcel", "application/vnd.ms-excel"
-    #     :xls
-    #   when "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet"
-    #     :xlsx
-    #   when "application/vnd.oasis.opendocument.spreadsheet"
-    #     :ods
-    #   else
-    #     #Mimetype not compatible
-    #     false
-    # end
   end
 
   def headers
@@ -90,64 +79,72 @@ class ProductImporter
   end
 
   def validate_all
-    suppliers_index = @suppliers_index || get_suppliers_index
-    categories_index = @categories_index || get_categories_index
-
     entries.each_with_index do |entry, i|
+      line_number = i+2
 
-      # Fetch/assign and validate supplier id
-      supplier_name = entry['supplier']
-      if supplier_name.blank?
-        invalidate({i+2 => {entry: entry, errors: ["Supplier name field is empty"]}}) #unless entry['supplier_id']
-      else
-        if suppliers_index[supplier_name]
-          entry['supplier_id'] = suppliers_index[supplier_name]
-        else
-          invalidate({i+2 => {entry: entry, errors: ["Supplier \"#{supplier_name}\" not found in database"]}})
-          #next
-        end
+      supplier_validation(entry, line_number)
+      category_validation(entry, line_number)
 
-        # Check enterprise permissions
-        unless @editable_enterprises[supplier_name]
-          invalidate({i+2 => {entry: entry, errors: ["You do not have permission to manage products for \"#{supplier_name}\""]}})
-          #next
-        end
-      end
-
-      # Fetch/assign and validate category id
-      category_name = entry['category']
-      if category_name.blank?
-        invalidate({i+2 => {entry: entry, errors: ["Category field is empty"]}}) #unless entry['primary_taxon_id']
-      else
-        if categories_index[category_name]
-          entry['primary_taxon_id'] = categories_index[category_name]
-        else
-          invalidate({i+2 => {entry: entry, errors: ["Category \"#{category_name}\" not found in database"]}})
-          #next
-        end
-      end
-
-      # Ensure on_hand isn't nil (because Spree::Product and Spree::Variant each validate this differently)
+      # Ensure on_hand isn't nil because Spree::Product and
+      # Spree::Variant each validate this differently
       entry['on_hand'] = 0 if entry['on_hand'].nil?
 
-      # Check if entry can be updated/saved; assign updatable status
-      set_update_status(entry, i)
-
-      # Add valid entry
-      @valid_entries[i+2] = {entry: entry} unless @invalid_entries[i+2]
+      set_update_status(entry, line_number)
+      mark_as_valid(entry, line_number) unless entry_invalid?(line_number)
     end
   end
 
-  def invalidate(invalid_line)
-    # Update exiting errors array for this line, if it exists
-    @invalid_entries.each do |line, data|
-      if invalid_line[line]
-        @invalid_entries[line][:errors] += invalid_line[line][:errors]
-        return
+  def entry_invalid?(line_number)
+    !!@invalid_entries[line_number]
+  end
+
+  def supplier_validation(entry, line_number)
+    # Fetch/assign and validate supplier id
+    suppliers_index = @suppliers_index || get_suppliers_index
+    supplier_name = entry['supplier']
+    if supplier_name.blank?
+      mark_as_invalid(entry, line_number, "Supplier name field is empty")
+    else
+      if suppliers_index[supplier_name]
+        entry['supplier_id'] = suppliers_index[supplier_name]
+      else
+        mark_as_invalid(entry, line_number, "Supplier \"#{supplier_name}\" not found in database")
+      end
+
+      # Check enterprise permissions
+      unless @editable_enterprises[supplier_name]
+        mark_as_invalid(entry, line_number, "You do not have permission to manage products for \"#{supplier_name}\"")
       end
     end
-    # Otherwise add new entry
-    @invalid_entries.merge!(invalid_line)
+  end
+
+  def category_validation(entry, line_number)
+    # Fetch/assign and validate category id
+    categories_index = @categories_index || get_categories_index
+    category_name = entry['category']
+    if category_name.blank?
+      mark_as_invalid(entry, line_number, "Category field is empty")
+    else
+      if categories_index[category_name]
+        entry['primary_taxon_id'] = categories_index[category_name]
+      else
+        mark_as_invalid(entry, line_number, "Category \"#{category_name}\" not found in database")
+      end
+    end
+  end
+
+  def mark_as_valid(entry, line_number)
+    @valid_entries[line_number] = {entry: entry}
+  end
+
+  def mark_as_invalid(entry, line_number, errors)
+    errors = [errors] if errors.is_a? String
+
+    if entry_invalid?(line_number)
+      @invalid_entries[line_number][:errors] += errors
+    else
+      @invalid_entries[line_number] = {entry: entry, errors: errors}
+    end
   end
 
   # Minimise db queries by getting a list of suppliers to look
@@ -227,44 +224,54 @@ class ProductImporter
     updated_count
   end
 
-  def set_update_status(entry, i)
+  def set_update_status(entry, line_number)
     # Find product with matching supplier and name
     match = Spree::Product.where(supplier_id: entry['supplier_id'], name: entry['name'], deleted_at: nil).first
 
     # If no matching product was found, create a new product
     if match.nil?
-      # Check product validations
-      new_product = Spree::Product.new()
-      new_product.assign_attributes(entry.except('id'))
-      if new_product.valid?
-        @products_to_create.push(entry) unless @invalid_entries[i+2]
-      else
-        invalidate({i+2 => {entry: entry, errors: new_product.errors.full_messages}})
-      end
+      mark_as_new_product(entry, line_number)
       return
     end
 
     # Otherwise, if a variant exists with matching display_name and unit_value, update it
     match.variants.each do |existing_variant|
       if existing_variant.display_name == entry['display_name'] && existing_variant.unit_value == Float(entry['unit_value'])
-        # Check updated variant would be valid
-        existing_variant.assign_attributes(entry.except('id', 'product_id'))
-        if existing_variant.valid?
-          @variants_to_update.push(existing_variant) unless @invalid_entries[i+2]
-        else
-          invalidate({i+2 => {entry: entry, errors: existing_variant.errors.full_messages}})
-        end
+        mark_as_existing_variant(entry, line_number, existing_variant)
         return
       end
     end
 
     # Otherwise, a variant with sufficiently matching attributes doesn't exist; create a new one
+    mark_as_new_variant(entry, line_number)
+  end
+
+  def mark_as_new_product(entry, line_number)
+    new_product = Spree::Product.new()
+    new_product.assign_attributes(entry.except('id'))
+    if new_product.valid?
+      @products_to_create.push(entry) unless entry_invalid?(line_number)
+    else
+      mark_as_invalid(entry, line_number, new_product.errors.full_messages)
+    end
+  end
+
+  def mark_as_existing_variant(entry, line_number, existing_variant)
+    existing_variant.assign_attributes(entry.except('id', 'product_id'))
+    if existing_variant.valid?
+      @variants_to_update.push(existing_variant) unless entry_invalid?(line_number)
+    else
+      mark_as_invalid(entry, line_number, existing_variant.errors.full_messages)
+    end
+  end
+
+  def mark_as_new_variant(entry, line_number)
     new_variant = Spree::Variant.new(entry.except('id', 'product_id'))
     new_variant.product_id = match.id
     if new_variant.valid?
-      @variants_to_create.push(new_variant) unless @invalid_entries[i+2]
+      @variants_to_create.push(new_variant) unless entry_invalid?(line_number)
     else
-      invalidate({i+2 => {entry: entry, errors: new_variant.errors.full_messages}})
+      mark_as_invalid(entry, line_number, new_variant.errors.full_messages)
     end
   end
 
@@ -284,14 +291,6 @@ class ProductImporter
     @invalid_entries.count
   end
 
-  def valid_entries
-    @valid_entries
-  end
-
-  def invalid_entries
-    @invalid_entries
-  end
-
   def products_create_count
     @products_to_create.count + @variants_to_create.count
   end

From 3d0f192490f2573405b7e4f4a9ddbc2a3be44965 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <visible_souls@hotmail.com>
Date: Fri, 3 Mar 2017 21:25:54 +0000
Subject: [PATCH 09/18] Product Import update

---
 .../controllers/feedback_panels.js.coffee     |   5 +
 .../stylesheets/admin/product_import.css.scss | 104 +++++++
 .../admin/product_import_controller.rb        |  50 ++--
 app/models/product_importer.rb                | 270 +++++++++++-------
 .../product_import/_entries_table.html.haml   |  12 +
 .../product_import/_errors_list.html.haml     |  11 +
 .../product_import/_import_review.html.haml   |  62 ++++
 .../product_import/_upload_form.html.haml     |   9 +
 .../admin/product_import/import.html.haml     |  79 ++---
 .../admin/product_import/index.html.haml      |  10 +-
 app/views/admin/product_import/save.html.haml |  15 +-
 spec/features/admin/product_import_spec.rb    | 118 ++++++--
 12 files changed, 522 insertions(+), 223 deletions(-)
 create mode 100644 app/assets/javascripts/admin/product_import/controllers/feedback_panels.js.coffee
 create mode 100644 app/assets/stylesheets/admin/product_import.css.scss
 create mode 100644 app/views/admin/product_import/_entries_table.html.haml
 create mode 100644 app/views/admin/product_import/_errors_list.html.haml
 create mode 100644 app/views/admin/product_import/_import_review.html.haml
 create mode 100644 app/views/admin/product_import/_upload_form.html.haml

diff --git a/app/assets/javascripts/admin/product_import/controllers/feedback_panels.js.coffee b/app/assets/javascripts/admin/product_import/controllers/feedback_panels.js.coffee
new file mode 100644
index 0000000000..2c0a2efb59
--- /dev/null
+++ b/app/assets/javascripts/admin/product_import/controllers/feedback_panels.js.coffee
@@ -0,0 +1,5 @@
+angular.module("ofn.admin").controller "FeedbackPanelsCtrl", ($scope) ->
+  $scope.active = false
+
+  $scope.togglePanel = ->
+    $scope.active = !$scope.active
diff --git a/app/assets/stylesheets/admin/product_import.css.scss b/app/assets/stylesheets/admin/product_import.css.scss
new file mode 100644
index 0000000000..54164191b3
--- /dev/null
+++ b/app/assets/stylesheets/admin/product_import.css.scss
@@ -0,0 +1,104 @@
+div.feedback-section {
+
+  div.feedback-header {
+    width: 100%;
+    //font-size: 1.5em;
+    clear: both;
+    //border: 1px solid #ccc;
+    float: left;
+    padding: 0.5em;
+
+    div {
+      font-size: 1.25em;
+      float: left;
+    }
+
+    div.header-caret {
+      width: 2em;
+      text-align: center;
+      min-height: 0.1em; //Empty div fix
+    }
+
+    div.header-icon {
+      width: 2.5em;
+      text-align: center;
+      padding-top: 0.1em;
+
+      .fa {
+        font-size: 1.5em;
+        line-height: 0.9em;
+      }
+      .fa-warning {
+        color: #ee4728;
+      }
+      .fa-check-circle {
+        color: #86d83a;
+      }
+    }
+
+    div.header-count {
+      min-width: 2em;
+      text-align: right;
+      padding-right: 0.5em;
+    }
+
+    div.header-description {
+      width: auto;
+    }
+
+  }
+
+  div.feedback-header:hover {
+    cursor: pointer;
+    background-color: #f7f7f7;
+  }
+
+  div.feedback-header.active {
+    background-color: #efefef;
+    text-shadow: 1px 1px 0px rgba(255,255,255,0.75);
+  }
+
+  div.feedback-panel {
+    width: 100%;
+    clear: both;
+    //border: 1px solid #ccc;
+    margin-bottom: 0.5em;
+    background-color: #f9f9f9;
+    padding: 1.5em;
+
+    div.table-wrap {
+      width: 100%;
+      overflow: auto;
+      border-right: 1px solid #ceede3;
+      max-height: 23em;
+    }
+
+    table {
+      background-color: white;
+      margin-bottom: 0;
+      td, th {
+        white-space: nowrap;
+      }
+    }
+
+    div.import-errors {
+      margin-bottom: 0.85em;
+
+      p.line {
+        font-size: 1.15em;
+        margin-bottom: 0.2em;
+        color: #577084;
+      }
+      p.error {
+        color: #cb1b1b;
+        margin-bottom: 0.2em;
+      }
+    }
+
+  }
+
+}
+
+br.panels.clearfix {
+  clear: both;
+}
diff --git a/app/controllers/admin/product_import_controller.rb b/app/controllers/admin/product_import_controller.rb
index aba3a8833a..d0ddd069f1 100644
--- a/app/controllers/admin/product_import_controller.rb
+++ b/app/controllers/admin/product_import_controller.rb
@@ -2,44 +2,54 @@ require 'roo'
 
 class Admin::ProductImportController < Spree::Admin::BaseController
 
-  before_filter :check_upload, except: :index
+  before_filter :validate_upload_presence, except: :index
 
   def import
     # Save uploaded file to tmp directory
-    @filepath = save_upload(params[:file])
+    @filepath = save_uploaded_file(params[:file])
     @importer = ProductImporter.new(File.new(@filepath), editable_enterprises)
 
-    if @importer.errors.present?
-      flash[:notice] = @importer.errors.full_messages.to_sentence
-    end
+    check_file_errors @importer
+    check_spreadsheet_has_data @importer
+
+    @tax_categories = Spree::TaxCategory.order('is_default DESC, name ASC')
+    @shipping_categories = Spree::ShippingCategory.order('name ASC')
   end
 
   def save
-    file = File.new(params[:filepath])
-    @importer = ProductImporter.new(file, editable_enterprises)
-    @importer.save_all_valid
-
-    if @importer.updated_count && @importer.updated_count > 0
-      File.delete(file)
-      flash[:success] = "#{@importer.updated_count} records updated successfully"
-    else
-      flash[:notice] = @importer.errors.full_messages.to_sentence
-    end
+    @importer = ProductImporter.new(File.new(params[:filepath]), editable_enterprises)
+    @importer.save_all if @importer.has_valid_entries?
   end
 
   private
 
-  def check_upload
+  def validate_upload_presence
     unless params[:file] || (params[:filepath] && File.exist?(params[:filepath]))
-      redirect_to '/admin/product_import', :notice => 'File not found or could not be opened'
+      redirect_to '/admin/product_import', notice: 'File not found or could not be opened'
       return
     end
   end
 
-  def save_upload(upload)
-    filename = Time.now.strftime('%d-%m-%Y-%H-%M-%S')
+  def check_file_errors(importer)
+    if importer.errors.present?
+      redirect_to '/admin/product_import', notice: @importer.errors.full_messages.to_sentence
+      return
+    end
+  end
+
+  def check_spreadsheet_has_data(importer)
+    unless importer.item_count
+      redirect_to '/admin/product_import', notice: 'No data found in spreadsheet'
+      return
+    end
+  end
+
+  def save_uploaded_file(upload)
+    filename = 'import' + Time.now.strftime('%d-%m-%Y-%H-%M-%S')
     extension = '.' + upload.original_filename.split('.').last
-    File.open(Rails.root.join('tmp', filename+extension), 'wb') do |f|
+    directory = 'tmp/product_import'
+    Dir.mkdir(directory) unless File.exists?(directory)
+    File.open(Rails.root.join(directory, filename+extension), 'wb') do |f|
       f.write(upload.read)
       f.path
     end
diff --git a/app/models/product_importer.rb b/app/models/product_importer.rb
index 97d67eea23..ee50e9e9c3 100644
--- a/app/models/product_importer.rb
+++ b/app/models/product_importer.rb
@@ -5,8 +5,6 @@ class ProductImporter
   include ActiveModel::Conversion
   include ActiveModel::Validations
 
-  attr_reader :valid_entries, :invalid_entries
-
   def initialize(file, editable_enterprises, options={})
     if file.is_a?(File)
       @file = file
@@ -15,9 +13,9 @@ class ProductImporter
       @valid_entries = {}
       @invalid_entries = {}
 
-      @products_to_create = []
-      @variants_to_create = []
-      @variants_to_update = []
+      @products_to_create = {}
+      @variants_to_create = {}
+      @variants_to_update = {}
       
       @products_created = 0
       @variants_created = 0
@@ -26,9 +24,11 @@ class ProductImporter
       @editable_enterprises = {}
       editable_enterprises.map { |e| @editable_enterprises[e.name] = e.id }
 
-      validate_all
+      @non_display_attributes = 'id', 'product_id', 'variant_id', 'supplier_id', 'primary_taxon_id', 'category_id', 'shipping_category_id', 'tax_category_id'
+
+      validate_all if @sheet
     else
-      self.errors.add(:importer, "error: no file uploaded")
+      self.errors.add(:importer, 'error: no file uploaded')
     end
   end
 
@@ -36,30 +36,89 @@ class ProductImporter
     false #ActiveModel, not ActiveRecord
   end
 
-  # Private methods below which only work with a valid spreadsheet object can
-  # be called publicly via here if the spreadsheet was successfully loaded,
-  # otherwise they return nil (without error).
-  def method_missing(method, *args, &block)
-    if self.respond_to?(method, include_private=true)
-      @sheet ? self.send(method, *args, &block) : nil
-    else
-      super
+  def has_valid_entries?
+    valid_count and valid_count > 0
+  end
+
+  def item_count
+    @sheet ? @sheet.last_row - 1 : 0
+  end
+
+  def valid_count
+    @valid_entries.count
+  end
+
+  def invalid_count
+    @invalid_entries.count
+  end
+
+  def products_create_count
+    @products_to_create.count + @variants_to_create.count
+  end
+
+  def products_update_count
+    @variants_to_update.count
+  end
+
+  def suppliers_index
+    @suppliers_index || get_suppliers_index
+  end
+
+  def invalid_entries
+    entries = {}
+    @invalid_entries.each do |line_number, data|
+      entries[line_number] = {entry: data[:entry].except(*@non_display_attributes), errors: data[:errors]}
     end
+    entries
+  end
+
+  def products_to_create
+    entries = {}
+    @products_to_create.merge(@variants_to_create).each do |line_number, data|
+      entries[line_number] = {entry: data[:entry].except(*@non_display_attributes)}
+    end
+    entries
+  end
+
+  def products_to_update
+    entries = {}
+    @variants_to_update.each do |line_number, data|
+      entries[line_number] = {entry: data[:entry].except(*@non_display_attributes)}
+    end
+    entries
+  end
+
+  def products_created_count
+    @products_created + @variants_created
+  end
+
+  def products_updated_count
+    @variants_updated
+  end
+
+  def total_saved_count
+    @products_created + @variants_created + @variants_updated
+  end
+
+  def save_all
+    save_all_valid
+    delete_uploaded_file
   end
 
   private
 
   def open_spreadsheet
     if accepted_mimetype
-      @sheet = Roo::Spreadsheet.open(@file, extension: accepted_mimetype)
+      Roo::Spreadsheet.open(@file, extension: accepted_mimetype)
     else
-      self.errors.add(:importer, "could not proccess file: invalid filetype")
+      self.errors.add(:importer, 'could not process file: invalid filetype')
+      delete_uploaded_file
       nil
     end
   end
 
   def accepted_mimetype
-    File.extname(@file.path).in?(['.csv', '.xls', '.xlsx', '.ods']) ? @file.path.split('.').last.to_sym : false
+    File.extname(@file.path).in?('.csv', '.xls', '.xlsx', '.ods') ? @file.path.split('.').last.to_sym : false
   end
 
   def headers
@@ -67,6 +126,7 @@ class ProductImporter
   end
 
   def rows
+    return [] unless @sheet and @sheet.last_row
     (2..@sheet.last_row).map do |i|
       @sheet.row(i)
     end
@@ -82,62 +142,80 @@ class ProductImporter
     entries.each_with_index do |entry, i|
       line_number = i+2
 
-      supplier_validation(entry, line_number)
-      category_validation(entry, line_number)
+      supplier_validation(line_number, entry)
+      category_validation(line_number, entry)
 
       # Ensure on_hand isn't nil because Spree::Product and
-      # Spree::Variant each validate this differently
+      # Spree::Variant each validate it differently
       entry['on_hand'] = 0 if entry['on_hand'].nil?
 
-      set_update_status(entry, line_number)
-      mark_as_valid(entry, line_number) unless entry_invalid?(line_number)
+      set_update_status(line_number, entry)
+      mark_as_valid(line_number, entry) unless entry_invalid?(line_number)
     end
+
+    delete_uploaded_file if item_count.zero? or valid_count.zero?
   end
 
   def entry_invalid?(line_number)
     !!@invalid_entries[line_number]
   end
 
-  def supplier_validation(entry, line_number)
-    # Fetch/assign and validate supplier id
+  def supplier_validation(line_number, entry)
     suppliers_index = @suppliers_index || get_suppliers_index
     supplier_name = entry['supplier']
-    if supplier_name.blank?
-      mark_as_invalid(entry, line_number, "Supplier name field is empty")
-    else
-      if suppliers_index[supplier_name]
-        entry['supplier_id'] = suppliers_index[supplier_name]
-      else
-        mark_as_invalid(entry, line_number, "Supplier \"#{supplier_name}\" not found in database")
-      end
 
-      # Check enterprise permissions
-      unless @editable_enterprises[supplier_name]
-        mark_as_invalid(entry, line_number, "You do not have permission to manage products for \"#{supplier_name}\"")
-      end
+    if supplier_name.blank?
+      mark_as_invalid(line_number, entry, "Supplier name field is empty")
+      return
     end
+
+    unless supplier_exists?(supplier_name)
+      mark_as_invalid(line_number, entry, "Supplier \"#{supplier_name}\" not found in database")
+      return
+    end
+
+    unless permission_to_manage?(supplier_name)
+      mark_as_invalid(line_number, entry, "You do not have permission to manage products for \"#{supplier_name}\"")
+      return
+    end
+
+    entry['supplier_id'] = suppliers_index[supplier_name]
   end
 
-  def category_validation(entry, line_number)
-    # Fetch/assign and validate category id
+  def supplier_exists?(supplier_name)
+    @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']
+
     if category_name.blank?
-      mark_as_invalid(entry, line_number, "Category field is empty")
+      mark_as_invalid(line_number, entry, "Category field is empty")
+      entry['primary_taxon_id'] = Spree::Taxon.first.id # Removes a duplicate validation message
+      return
+    end
+
+    if category_exists?(category_name)
+      entry['primary_taxon_id'] = categories_index[category_name]
     else
-      if categories_index[category_name]
-        entry['primary_taxon_id'] = categories_index[category_name]
-      else
-        mark_as_invalid(entry, line_number, "Category \"#{category_name}\" not found in database")
-      end
+      mark_as_invalid(line_number, entry, "Category \"#{category_name}\" not found in database")
     end
   end
 
-  def mark_as_valid(entry, line_number)
+  def category_exists?(category_name)
+    @categories_index[category_name]
+  end
+
+  def mark_as_valid(line_number, entry)
     @valid_entries[line_number] = {entry: entry}
   end
 
-  def mark_as_invalid(entry, line_number, errors)
+  def mark_as_invalid(line_number, entry, errors)
     errors = [errors] if errors.is_a? String
 
     if entry_invalid?(line_number)
@@ -173,15 +251,14 @@ class ProductImporter
 
   def save_all_valid
     updated = {}
-    @products_to_create.each do |entry|
-      # If we've already added a new product with these attributes from
-      # this spreadsheet, pass this entry to @variants_to_create with
+    @products_to_create.each do |line_number, data|
+      entry = data[:entry]
+      # If we've already added a new product with these attributes
+      # from this spreadsheet, mark this entry as a new variant with
       # the new product id, as this is a now variant of that product...
       if updated[entry['supplier_id']] && updated[entry['supplier_id']][entry['name']]
         product_id = updated[entry['supplier_id']][entry['name']]
-        new_variant = Spree::Variant.new(entry.except('id', 'product_id'))
-        new_variant.product_id = product_id
-        @variants_to_create.push(new_variant)
+        mark_as_new_variant(line_number, entry, product_id)
         next
       end
 
@@ -197,117 +274,90 @@ class ProductImporter
         end
         @products_created += 1
       else
-        self.errors.add(:importer, product.errors.full_messages)
+        self.errors.add("Line #{line_number}:", product.errors.full_messages)
       end
 
       updated[entry['supplier_id']] = {entry['name'] => product.id}
     end
 
-    @variants_to_update.each do |variant|
-      if variant.save
+    @variants_to_update.each do |line_number, data|
+      variant = data[:variant]
+      if variant.valid? and variant.save
         @variants_updated += 1
       else
-        self.errors.add(:importer, variant.errors.full_messages)
+        self.errors.add("Line #{line_number}:", variant.errors.full_messages)
       end
     end
 
-    @variants_to_create.each do |new_variant|
-      if new_variant.save
+    @variants_to_create.each do |line_number, data|
+      new_variant = data[:variant]
+      if new_variant.valid? and new_variant.save
         @variants_created += 1
       else
-        self.errors.add(:importer, new_variant.errors.full_messages)
+        self.errors.add("Line #{line_number}:", new_variant.errors.full_messages)
       end
     end
 
-    self.errors.add(:importer, "did not save any products successfully") if updated_count == 0
+    self.errors.add(:importer, "did not save any products successfully") if total_saved_count == 0
 
-    updated_count
+    total_saved_count
   end
 
-  def set_update_status(entry, line_number)
+  def set_update_status(line_number, entry)
     # Find product with matching supplier and name
     match = Spree::Product.where(supplier_id: entry['supplier_id'], name: entry['name'], deleted_at: nil).first
 
     # If no matching product was found, create a new product
     if match.nil?
-      mark_as_new_product(entry, line_number)
+      mark_as_new_product(line_number, entry)
       return
     end
 
     # Otherwise, if a variant exists with matching display_name and unit_value, update it
     match.variants.each do |existing_variant|
       if existing_variant.display_name == entry['display_name'] && existing_variant.unit_value == Float(entry['unit_value'])
-        mark_as_existing_variant(entry, line_number, existing_variant)
+        mark_as_existing_variant(line_number, entry, existing_variant)
         return
       end
     end
 
     # Otherwise, a variant with sufficiently matching attributes doesn't exist; create a new one
-    mark_as_new_variant(entry, line_number)
+    mark_as_new_variant(line_number, entry, match.id)
   end
 
-  def mark_as_new_product(entry, line_number)
+  def mark_as_new_product(line_number, entry)
     new_product = Spree::Product.new()
     new_product.assign_attributes(entry.except('id'))
     if new_product.valid?
-      @products_to_create.push(entry) unless entry_invalid?(line_number)
+      @products_to_create[line_number] = {entry: entry} unless entry_invalid?(line_number)
     else
-      mark_as_invalid(entry, line_number, new_product.errors.full_messages)
+      mark_as_invalid(line_number, entry, new_product.errors.full_messages)
     end
   end
 
-  def mark_as_existing_variant(entry, line_number, existing_variant)
+  def mark_as_existing_variant(line_number, entry, existing_variant)
     existing_variant.assign_attributes(entry.except('id', 'product_id'))
     if existing_variant.valid?
-      @variants_to_update.push(existing_variant) unless entry_invalid?(line_number)
+      @variants_to_update[line_number] = {entry: entry, variant: existing_variant} unless entry_invalid?(line_number)
     else
-      mark_as_invalid(entry, line_number, existing_variant.errors.full_messages)
+      mark_as_invalid(line_number, entry, existing_variant.errors.full_messages)
     end
   end
 
-  def mark_as_new_variant(entry, line_number)
+  def mark_as_new_variant(line_number, entry, product_id)
     new_variant = Spree::Variant.new(entry.except('id', 'product_id'))
-    new_variant.product_id = match.id
+    new_variant.product_id = product_id
     if new_variant.valid?
-      @variants_to_create.push(new_variant) unless entry_invalid?(line_number)
+      @variants_to_create[line_number] = {entry: entry, variant: new_variant} unless entry_invalid?(line_number)
     else
-      mark_as_invalid(entry, line_number, new_variant.errors.full_messages)
+      mark_as_invalid(line_number, entry, new_variant.errors.full_messages)
     end
   end
 
-  def has_valid_entries?
-    valid_count > 0
-  end
-
-  def item_count
-    @sheet.last_row - 1
-  end
-
-  def valid_count
-    @valid_entries.count
-  end
-
-  def invalid_count
-    @invalid_entries.count
-  end
-
-  def products_create_count
-    @products_to_create.count + @variants_to_create.count
-  end
-
-  def products_update_count
-    @variants_to_update.count
-  end
-
-  def products_created
-    @products_created + @variants_created
-  end
-
-  def products_updated
-    @variants_updated
-  end
-
-  def updated_count
-    @products_created + @variants_created + @variants_updated
+  def delete_uploaded_file
+    # Only delete if file is in '/tmp/product_import' directory
+    if @file.path == Rails.root.join('tmp', 'product_import').to_s
+      File.delete(@file)
+    end
   end
 end
diff --git a/app/views/admin/product_import/_entries_table.html.haml b/app/views/admin/product_import/_entries_table.html.haml
new file mode 100644
index 0000000000..5385112398
--- /dev/null
+++ b/app/views/admin/product_import/_entries_table.html.haml
@@ -0,0 +1,12 @@
+- if entries && entries.count > 0 #&& entries.values.first && entries.values.first[:entry]
+  %div.table-wrap
+    %table
+      %thead
+        %th Line
+        - entries.values.first[:entry].each do |key, value|
+          %th= key
+      - entries.each do |line, item|
+        %tr
+          %td= line
+          - item[:entry].each do |key, value|
+            %td= value
\ No newline at end of file
diff --git a/app/views/admin/product_import/_errors_list.html.haml b/app/views/admin/product_import/_errors_list.html.haml
new file mode 100644
index 0000000000..4974c978df
--- /dev/null
+++ b/app/views/admin/product_import/_errors_list.html.haml
@@ -0,0 +1,11 @@
+- @importer.invalid_entries.each do |line, item|
+  %div.import-errors
+    %p.line
+      %strong
+        Item line #{line}:
+      %span= item[:entry]['name']
+      - if item[:entry]['display_name']
+        ( #{item[:entry]['display_name']} )
+    - item[:errors].each do |error|
+      %p.error
+        &nbsp;-&nbsp; #{error}
\ No newline at end of file
diff --git a/app/views/admin/product_import/_import_review.html.haml b/app/views/admin/product_import/_import_review.html.haml
new file mode 100644
index 0000000000..9422be87f0
--- /dev/null
+++ b/app/views/admin/product_import/_import_review.html.haml
@@ -0,0 +1,62 @@
+%h5 Import validation overview
+%br
+
+%div.feedback-section
+  %div.feedback-header
+    %div.header-caret
+      -#%i.icon-chevron-right{ng: {hide: 'active'}}
+      -#%i.icon-chevron-down{ng: {hide: '!active'}}
+    %div.header-icon
+      %i.fa.fa-check-circle
+    %div.header-count
+      %strong.item-count= @importer.item_count
+    %div.header-description
+      Entries found in imported file
+  -#%div.feedback-panel{ng: {hide: '!active'}}
+  -#  Content goes here
+
+%div.feedback-section{ng: {controller: 'FeedbackPanelsCtrl', init: "count = #{@importer.invalid_count}"}}
+  %div.feedback-header{ng: {click: 'togglePanel()', class: '{active: active && count}'}}
+    %div.header-caret
+      %i.icon-chevron-right{ng: {hide: 'active || count == 0'}}
+      %i.icon-chevron-down{ng: {hide: '!active || count == 0'}}
+    %div.header-icon
+      %i.fa.fa-warning
+    %div.header-count
+      %strong.invalid-count= @importer.invalid_count
+    %div.header-description
+      Items contain errors and will not be imported
+  %div.feedback-panel{ng: {hide: '!active || count == 0'}}
+    = render 'errors_list'
+    %br
+    = render 'entries_table', entries: @importer.invalid_entries
+
+%div.feedback-section{ng: {controller: 'FeedbackPanelsCtrl', init: "count = #{@importer.products_create_count}"}}
+  %div.feedback-header{ng: {click: 'togglePanel()', class: '{active: active && count}'}}
+    %div.header-caret
+      %i.icon-chevron-right{ng: {hide: 'active || count == 0'}}
+      %i.icon-chevron-down{ng: {hide: '!active || count == 0'}}
+    %div.header-icon
+      %i.fa.fa-check-circle
+    %div.header-count
+      %strong.create-count= @importer.products_create_count
+    %div.header-description
+      Products will be created
+  %div.feedback-panel{ng: {hide: '!active || count == 0'}}
+    = render 'entries_table', entries: @importer.products_to_create
+
+%div.feedback-section{ng: {controller: 'FeedbackPanelsCtrl', init: "count = #{@importer.products_update_count}"}}
+  %div.feedback-header{ng: {click: 'togglePanel()', class: '{active: active && count}'}}
+    %div.header-caret
+      %i.icon-chevron-right{ng: {hide: 'active || count == 0'}}
+      %i.icon-chevron-down{ng: {hide: '!active || count == 0'}}
+    %div.header-icon
+      %i.fa.fa-check-circle
+    %div.header-count
+      %strong.update-count= @importer.products_update_count
+    %div.header-description
+      Products will be updated
+  %div.feedback-panel{ng: {hide: '!active || count == 0'}}
+    = render 'entries_table', entries: @importer.products_to_update
+
+%br.panels.clearfix
\ No newline at end of file
diff --git a/app/views/admin/product_import/_upload_form.html.haml b/app/views/admin/product_import/_upload_form.html.haml
new file mode 100644
index 0000000000..46e05adfdb
--- /dev/null
+++ b/app/views/admin/product_import/_upload_form.html.haml
@@ -0,0 +1,9 @@
+%h5 Select a spreadsheet to upload
+%br
+= form_tag main_app.admin_product_import_path, multipart: true do
+  = file_field_tag :file
+  %br
+  %br
+  = submit_tag "Import"
+  %br
+  %br
\ No newline at end of file
diff --git a/app/views/admin/product_import/import.html.haml b/app/views/admin/product_import/import.html.haml
index beab046b29..de0a1ec20e 100644
--- a/app/views/admin/product_import/import.html.haml
+++ b/app/views/admin/product_import/import.html.haml
@@ -1,67 +1,32 @@
 - content_for :page_title do
   Product Import
 
-= render :partial => 'spree/admin/shared/product_sub_menu'
+= render partial: 'spree/admin/shared/product_sub_menu'
 
-- if @importer.valid_count && @importer.invalid_count
-  %h5 Products imported from spreadsheet:
-  %br
-  %p{style: "font-size: 1.15em"}
-    Valid entries found:
-    = @importer.valid_count
-  %p{style: "font-size: 1.15em"}
-    Invalid entries found:
-    = @importer.invalid_count
-  %br
-  %p{style: "font-size: 1.15em"}
-    Products to be created:
-    = @importer.products_create_count
-  %p{style: "font-size: 1.15em"}
-    Products to be updated:
-    = @importer.products_update_count
+= form_tag main_app.admin_product_import_save_path, {'ng-app' => 'ofn.admin'} do
 
-- if @importer.invalid_entries && @importer.invalid_entries.count > 0
-  %br
-  %h5 Import errors:
-  %br
-  - @importer.invalid_entries.each do |line, item|
-    %p{style: "font-size: 1.15em"}
-      %strong
-        Item line #{line}:
-      %span= item[:entry]['name']
-      - if item[:entry]['display_name']
-        ( #{item[:entry]['display_name']} )
-    - item[:errors].each do |error|
-      %p{class: "red"}
-        &nbsp;-&nbsp; #{error}
-  %br
-  %h5 Review invalid entries:
-  %div{style: 'width: 100%; overflow: auto;'}
-    %table
-      %thead
-        %th Line
-        - @importer.invalid_entries.values.first[:entry].each do |key, value|
-          %th{style: 'white-space: nowrap;'}= key
-      - @importer.invalid_entries.each do |line, item|
-        %tr
-          %td= line
-          - item[:entry].each do |key, value|
-            %td{style: 'white-space: nowrap;'}= value
+  - if @importer.invalid_count && !@importer.has_valid_entries?
+    %h5 No valid entries found
+    %p There are no entries that can be saved
+    %br
 
-- if @importer.has_valid_entries?
-  %br
-  - if @importer.invalid_count > 0
-    %h5 Invalid entries detected.
-    %p Save valid products for now and discard the others?
-  - else
-    %h5 No errors detected!
-    %p Save all imported products?
-  %br
-  = form_tag main_app.admin_product_import_save_path, multipart: true do
+  = render 'import_review'
+
+  - if @importer.has_valid_entries?
+    - if @importer.invalid_count > 0
+      %br
+      %h5 Imported file contains some invalid entries
+      %p Save valid entries for now and discard the others?
+    - else
+      %h5 No errors detected!
+      %p Save all imported products?
+    %br
     = hidden_field_tag :filepath, @filepath
     = submit_tag "Save"
     %a.button{href: main_app.admin_product_import_path} Cancel
 
-- if @importer.invalid_count && !@importer.has_valid_entries?
-  %br
-  %a.button{href: main_app.admin_product_import_path} Cancel
+  - else
+    %br
+    %a.button{href: main_app.admin_product_import_path} Cancel
+
+
diff --git a/app/views/admin/product_import/index.html.haml b/app/views/admin/product_import/index.html.haml
index 6ca97a0686..c904b6a70f 100644
--- a/app/views/admin/product_import/index.html.haml
+++ b/app/views/admin/product_import/index.html.haml
@@ -3,12 +3,4 @@
 
 = render :partial => 'spree/admin/shared/product_sub_menu'
 
-%h5 Select a spreadsheet to upload
-%br
-= form_tag main_app.admin_product_import_path, multipart: true do
-  = file_field_tag :file
-  %br
-  %br
-  = submit_tag "Import"
-  %br
-  %br
+= render 'upload_form'
diff --git a/app/views/admin/product_import/save.html.haml b/app/views/admin/product_import/save.html.haml
index bca2163805..2b4abdd911 100644
--- a/app/views/admin/product_import/save.html.haml
+++ b/app/views/admin/product_import/save.html.haml
@@ -8,17 +8,20 @@
 
 %p{style: 'font-size: 1.15em'}
   Products created:
-  = @importer.products_created
+  = @importer.products_created_count
 %p{style: 'font-size: 1.15em'}
   Products updated:
-  = @importer.products_updated
+  = @importer.products_updated_count
 
-- if @importer.errors.full_messages && !@importer.errors.full_messages.blank?
-  %br
-  %h6 Errors
+%br
+
+- if !@importer.errors.full_messages
+  %h5 All #{importer.total_saved_count} items saved successfully
+- else
+  %h5 Errors
   - @importer.errors.full_messages.each do |error|
     %p{class: "red"}
       &nbsp;-&nbsp; #{error}
 
 %br
-  %a.button{href: main_app.admin_product_import_path} Back
+%a.button{href: main_app.admin_product_import_path} Back
diff --git a/spec/features/admin/product_import_spec.rb b/spec/features/admin/product_import_spec.rb
index ebf34026c4..1eefb83ab3 100644
--- a/spec/features/admin/product_import_spec.rb
+++ b/spec/features/admin/product_import_spec.rb
@@ -7,11 +7,14 @@ feature "Product Import", js: true do
 
   let!(:admin) { create(:admin_user) }
   let!(:user) { create_enterprise_user }
-  let!(:enterprise) { create(:enterprise, owner: user, name: "Test Enterprise") }
+  let!(:enterprise) { create(:supplier_enterprise, owner: user, name: "Test Enterprise") }
   let!(:category) { create(:taxon, name: 'Vegetables') }
+  let!(:category2) { create(:taxon, name: 'Cake') }
+  let!(:product) { create(:simple_product, supplier: enterprise, name: 'Hypothetical Cake') }
+  let!(:variant) { create(:variant, product_id: product.id, price: '8.50', count_on_hand: '100', unit_value: '500', display_name: 'Preexisting Banana') }
   let(:permissions) { OpenFoodNetwork::Permissions.new(user) }
 
-  describe "product import" do
+  describe "when importing products from uploaded file" do
     before { quick_login_as_admin }
     after { File.delete('/tmp/test.csv') }
 
@@ -26,14 +29,16 @@ feature "Product Import", js: true do
       visit main_app.admin_product_import_path
 
       expect(page).to have_content "Select a spreadsheet to upload"
-      attach_file('file', '/tmp/test.csv')
-      click_button('Import')
+      attach_file 'file', '/tmp/test.csv'
+      click_button 'Import'
 
-      expect(page).to have_content("Valid entries found: 2")
-      expect(page).to have_content("Invalid entries found: 0")
-      click_button('Save')
+      expect(page).to have_selector '.item-count', text: "2"
+      expect(page).to have_selector '.invalid-count', text: "0"
+      expect(page).to have_selector '.create-count', text: "2"
+      expect(page).to have_selector '.update-count', text: "0"
 
-      expect(page).to have_content("Products created: 2")
+      click_button 'Save'
+      expect(page).to have_content "Products created: 2"
 
       potatoes = Spree::Product.find_by_name('Potatoes')
       potatoes.supplier.should == enterprise
@@ -53,15 +58,16 @@ feature "Product Import", js: true do
 
       expect(page).to have_content "Select a spreadsheet to upload"
       attach_file('file', '/tmp/test.csv')
-      click_button('Import')
+      click_button 'Import'
 
-      expect(page).to have_content("Valid entries found: 1")
-      expect(page).to have_content("Invalid entries found: 1")
-      expect(page).to have_content("errors")
+      expect(page).to have_selector '.item-count', text: "2"
+      expect(page).to have_selector '.invalid-count', text: "1"
+      expect(page).to have_selector '.create-count', text: "1"
+      expect(page).to have_selector '.update-count', text: "0"
 
-      click_button('Save')
-
-      expect(page).to have_content("Products created: 1")
+      expect(page).to have_selector 'input[type=submit][value="Save"]'
+      click_button 'Save'
+      expect(page).to have_content "Products created: 1"
 
       Spree::Product.find_by_name('Bad Potatoes').should == nil
       carrots = Spree::Product.find_by_name('Good Carrots')
@@ -81,14 +87,84 @@ feature "Product Import", js: true do
       visit main_app.admin_product_import_path
 
       expect(page).to have_content "Select a spreadsheet to upload"
-      attach_file('file', '/tmp/test.csv')
-      click_button('Import')
+      attach_file 'file', '/tmp/test.csv'
+      click_button 'Import'
 
-      expect(page).to have_content("Valid entries found: 0")
-      expect(page).to have_content("Invalid entries found: 2")
-      expect(page).to have_content("errors")
+      expect(page).to have_selector '.item-count', text: "2"
+      expect(page).to have_selector '.invalid-count', text: "2"
+      expect(page).to have_selector '.create-count', text: "0"
+      expect(page).to have_selector '.update-count', text: "0"
 
-      expect(page).to_not have_selector('input[type=submit]', text: "Save")
+      expect(page).to_not have_selector 'input[type=submit][value="Save"]'
+    end
+
+    it "can add new variants to existing products and update price and stock level of existing products" do
+      csv_data = CSV.generate do |csv|
+        csv << ["name", "supplier", "category", "on_hand", "price", "unit_value", "variant_unit", "variant_unit_scale", "display_name"]
+        csv << ["Hypothetical Cake", "Test Enterprise", "Cake", "5", "5.50", "500", "weight", "1", "Preexisting Banana"]
+        csv << ["Hypothetical Cake", "Test Enterprise", "Cake", "6", "3.50", "500", "weight", "1", "Emergent Coffee"]
+      end
+      File.write('/tmp/test.csv', csv_data)
+
+      visit main_app.admin_product_import_path
+      attach_file 'file', '/tmp/test.csv'
+      click_button 'Import'
+
+      expect(page).to have_selector '.item-count', text: "2"
+      expect(page).to have_selector '.invalid-count', text: "0"
+      expect(page).to have_selector '.create-count', text: "1"
+      expect(page).to have_selector '.update-count', text: "1"
+
+      click_button 'Save'
+      expect(page).to have_content "Products created: 1"
+      expect(page).to have_content "Products updated: 1"
+
+      added_coffee = Spree::Variant.find_by_display_name('Emergent Coffee')
+      added_coffee.product.name.should == 'Hypothetical Cake'
+      added_coffee.price.should == 3.50
+      added_coffee.on_hand.should == 6
+
+      updated_banana = Spree::Variant.find_by_display_name('Preexisting Banana')
+      updated_banana.product.name.should == 'Hypothetical Cake'
+      updated_banana.price.should == 5.50
+      updated_banana.on_hand.should == 5
+    end
+  end
+
+  describe "when dealing with uploaded files" do
+    before { quick_login_as_admin }
+
+    it "checks filetype on upload" do
+      File.write('/tmp/test.txt', "Wrong filetype!")
+
+      visit main_app.admin_product_import_path
+      attach_file 'file', '/tmp/test.txt'
+      click_button 'Import'
+
+      expect(page).to have_content "Importer could not process file: invalid filetype"
+      expect(page).to_not have_selector 'input[type=submit][value="Save"]'
+      expect(page).to have_content "Select a spreadsheet to upload"
+      File.delete('/tmp/test.txt')
+    end
+
+    it "returns and error if nothing was uploaded" do
+      visit main_app.admin_product_import_path
+      click_button 'Import'
+
+      expect(page).to have_content "File not found or could not be opened"
+    end
+
+    it "handles cases where no meaningful data can be read from the file" do
+      File.write('/tmp/test.csv', "A22££S\\\\\n**VA,,,AF..D")
+
+      visit main_app.admin_product_import_path
+      attach_file 'file', '/tmp/test.csv'
+      click_button 'Import'
+
+      expect(page).to have_selector '.create-count', text: "0"
+      expect(page).to have_selector '.update-count', text: "0"
+      expect(page).to_not have_selector 'input[type=submit][value="Save"]'
+      File.delete('/tmp/test.csv')
     end
   end
 

From f4511fc74dd21a556ec1c9ead9608b8c9c77c4a5 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <visible_souls@hotmail.com>
Date: Fri, 3 Mar 2017 23:43:30 +0000
Subject: [PATCH 10/18] PI permission test

---
 .../product_import/_import_options.html.haml  |  0
 spec/features/admin/product_import_spec.rb    | 45 ++++++++++++++++---
 2 files changed, 38 insertions(+), 7 deletions(-)
 create mode 100644 app/views/admin/product_import/_import_options.html.haml

diff --git a/app/views/admin/product_import/_import_options.html.haml b/app/views/admin/product_import/_import_options.html.haml
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/spec/features/admin/product_import_spec.rb b/spec/features/admin/product_import_spec.rb
index 1eefb83ab3..61aa8a4d03 100644
--- a/spec/features/admin/product_import_spec.rb
+++ b/spec/features/admin/product_import_spec.rb
@@ -7,7 +7,8 @@ feature "Product Import", js: true do
 
   let!(:admin) { create(:admin_user) }
   let!(:user) { create_enterprise_user }
-  let!(:enterprise) { create(:supplier_enterprise, owner: user, name: "Test Enterprise") }
+  let!(:enterprise) { create(:supplier_enterprise, owner: user, name: "User Enterprise") }
+  let!(:enterprise2) { create(:supplier_enterprise, owner: admin, name: "Another Enterprise") }
   let!(:category) { create(:taxon, name: 'Vegetables') }
   let!(:category2) { create(:taxon, name: 'Cake') }
   let!(:product) { create(:simple_product, supplier: enterprise, name: 'Hypothetical Cake') }
@@ -21,8 +22,8 @@ feature "Product Import", js: true do
     it "validates entries and saves them if they are all valid" do
       csv_data = CSV.generate do |csv|
         csv << ["name", "supplier", "category", "on_hand", "price", "unit_value", "variant_unit", "variant_unit_scale"]
-        csv << ["Carrots", "Test Enterprise", "Vegetables", "5", "3.20", "500", "weight", "1"]
-        csv << ["Potatoes", "Test Enterprise", "Vegetables", "6", "6.50", "1000", "weight", "1000"]
+        csv << ["Carrots", "User Enterprise", "Vegetables", "5", "3.20", "500", "weight", "1"]
+        csv << ["Potatoes", "User Enterprise", "Vegetables", "6", "6.50", "1000", "weight", "1000"]
       end
       File.write('/tmp/test.csv', csv_data)
 
@@ -49,7 +50,7 @@ feature "Product Import", js: true do
     it "displays info about invalid entries but still allows saving of valid entries" do
       csv_data = CSV.generate do |csv|
         csv << ["name", "supplier", "category", "on_hand", "price", "unit_value", "variant_unit", "variant_unit_scale"]
-        csv << ["Good Carrots", "Test Enterprise", "Vegetables", "5", "3.20", "500", "weight", "1"]
+        csv << ["Good Carrots", "User Enterprise", "Vegetables", "5", "3.20", "500", "weight", "1"]
         csv << ["Bad Potatoes", "", "Vegetables", "6", "6.50", "1000", "", "1000"]
       end
       File.write('/tmp/test.csv', csv_data)
@@ -101,8 +102,8 @@ feature "Product Import", js: true do
     it "can add new variants to existing products and update price and stock level of existing products" do
       csv_data = CSV.generate do |csv|
         csv << ["name", "supplier", "category", "on_hand", "price", "unit_value", "variant_unit", "variant_unit_scale", "display_name"]
-        csv << ["Hypothetical Cake", "Test Enterprise", "Cake", "5", "5.50", "500", "weight", "1", "Preexisting Banana"]
-        csv << ["Hypothetical Cake", "Test Enterprise", "Cake", "6", "3.50", "500", "weight", "1", "Emergent Coffee"]
+        csv << ["Hypothetical Cake", "User Enterprise", "Cake", "5", "5.50", "500", "weight", "1", "Preexisting Banana"]
+        csv << ["Hypothetical Cake", "User Enterprise", "Cake", "6", "3.50", "500", "weight", "1", "Emergent Coffee"]
       end
       File.write('/tmp/test.csv', csv_data)
 
@@ -168,5 +169,35 @@ feature "Product Import", js: true do
     end
   end
 
-  # Test enterprise permissions with non-admin user
+  describe "handling enterprise permissions" do
+    before { quick_login_as user }
+
+    it "only allows import into enterprises the user is permitted to manage" do
+      csv_data = CSV.generate do |csv|
+        csv << ["name", "supplier", "category", "on_hand", "price", "unit_value", "variant_unit", "variant_unit_scale"]
+        csv << ["My Carrots", "User Enterprise", "Vegetables", "5", "3.20", "500", "weight", "1"]
+        csv << ["Your Potatoes", "Another Enterprise", "Vegetables", "6", "6.50", "1000", "weight", "1000"]
+      end
+      File.write('/tmp/test.csv', csv_data)
+
+      visit main_app.admin_product_import_path
+
+      attach_file 'file', '/tmp/test.csv'
+      click_button 'Import'
+
+      expect(page).to have_selector '.item-count', text: "2"
+      expect(page).to have_selector '.invalid-count', text: "1"
+      expect(page).to have_selector '.create-count', text: "1"
+      expect(page).to have_selector '.update-count', text: "0"
+
+      expect(page.body).to have_content 'You do not have permission to manage products for "Another Enterprise"'
+
+      click_button 'Save'
+      expect(page).to have_content "Products created: 1"
+
+      Spree::Product.find_by_name('My Carrots').should be_a Spree::Product
+      Spree::Product.find_by_name('Your Potatoes').should == nil
+    end
+  end
+
 end
\ No newline at end of file

From 14fb40a99624fd0c88e85182c1654b8a86ecb8a5 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <visible_souls@hotmail.com>
Date: Sat, 4 Mar 2017 20:51:41 +0000
Subject: [PATCH 11/18] Product Import options and defaults

Added available_on test

Obscure case fix and extra spec
---
 ...ls.js.coffee => dropdown_panels.js.coffee} |  2 +-
 .../stylesheets/admin/product_import.css.scss | 70 +++++++++++++--
 .../admin/product_import_controller.rb        |  2 +-
 app/models/product_importer.rb                | 55 ++++++++----
 .../product_import/_import_options.html.haml  | 21 +++++
 .../product_import/_import_review.html.haml   | 33 ++++---
 .../product_import/_options_form.html.haml    | 35 ++++++++
 .../admin/product_import/import.html.haml     |  2 +
 app/views/admin/product_import/save.html.haml |  4 +-
 spec/features/admin/product_import_spec.rb    | 90 ++++++++++++++++++-
 10 files changed, 269 insertions(+), 45 deletions(-)
 rename app/assets/javascripts/admin/product_import/controllers/{feedback_panels.js.coffee => dropdown_panels.js.coffee} (59%)
 create mode 100644 app/views/admin/product_import/_options_form.html.haml

diff --git a/app/assets/javascripts/admin/product_import/controllers/feedback_panels.js.coffee b/app/assets/javascripts/admin/product_import/controllers/dropdown_panels.js.coffee
similarity index 59%
rename from app/assets/javascripts/admin/product_import/controllers/feedback_panels.js.coffee
rename to app/assets/javascripts/admin/product_import/controllers/dropdown_panels.js.coffee
index 2c0a2efb59..1403611168 100644
--- a/app/assets/javascripts/admin/product_import/controllers/feedback_panels.js.coffee
+++ b/app/assets/javascripts/admin/product_import/controllers/dropdown_panels.js.coffee
@@ -1,4 +1,4 @@
-angular.module("ofn.admin").controller "FeedbackPanelsCtrl", ($scope) ->
+angular.module("ofn.admin").controller "DropdownPanelsCtrl", ($scope) ->
   $scope.active = false
 
   $scope.togglePanel = ->
diff --git a/app/assets/stylesheets/admin/product_import.css.scss b/app/assets/stylesheets/admin/product_import.css.scss
index 54164191b3..e99cc9beea 100644
--- a/app/assets/stylesheets/admin/product_import.css.scss
+++ b/app/assets/stylesheets/admin/product_import.css.scss
@@ -1,6 +1,6 @@
-div.feedback-section {
+div.panel-section {
 
-  div.feedback-header {
+  div.panel-header {
     width: 100%;
     //font-size: 1.5em;
     clear: both;
@@ -22,7 +22,7 @@ div.feedback-section {
     div.header-icon {
       width: 2.5em;
       text-align: center;
-      padding-top: 0.1em;
+      padding-top: 0.18em;
 
       .fa {
         font-size: 1.5em;
@@ -48,17 +48,17 @@ div.feedback-section {
 
   }
 
-  div.feedback-header:hover {
+  div.panel-header:hover {
     cursor: pointer;
     background-color: #f7f7f7;
   }
 
-  div.feedback-header.active {
+  div.panel-header.active {
     background-color: #efefef;
     text-shadow: 1px 1px 0px rgba(255,255,255,0.75);
   }
 
-  div.feedback-panel {
+  div.panel-content {
     width: 100%;
     clear: both;
     //border: 1px solid #ccc;
@@ -102,3 +102,61 @@ div.feedback-section {
 br.panels.clearfix {
   clear: both;
 }
+
+table.import-settings {
+  background-color: transparent !important;
+  width: auto;
+
+  //select {
+  //  width: 100%;
+  //}
+  tr {
+
+  }
+  tbody tr:hover td {
+    background-color: #f3f3f3;
+  }
+  td {
+    border: 0;
+    border-bottom: 1px solid #eee;
+    text-align: left;
+
+    input {
+      width: 15em;
+    }
+    input[type="checkbox"] {
+      width: auto;
+    }
+
+  }
+  td.description {
+    font-weight: bold;
+    padding-right: 2.5em;
+  }
+  tr:first-child td {
+    //border-top: 1px solid #eee;
+    border-top: 0;
+  }
+  tr:last-child td {
+    //border-top: 1px solid #eee;
+    border-bottom: 0;
+  }
+
+}
+
+.panel-section.import-settings {
+  .header-icon {
+    color: #BFBFBF;
+  }
+  .header-description {
+    padding-left: 1em;
+  }
+}
+
+.select2-search {
+  display: none;
+}
+
+.select2-results {
+  margin: 0;
+}
diff --git a/app/controllers/admin/product_import_controller.rb b/app/controllers/admin/product_import_controller.rb
index d0ddd069f1..819fd3823b 100644
--- a/app/controllers/admin/product_import_controller.rb
+++ b/app/controllers/admin/product_import_controller.rb
@@ -17,7 +17,7 @@ class Admin::ProductImportController < Spree::Admin::BaseController
   end
 
   def save
-    @importer = ProductImporter.new(File.new(params[:filepath]), editable_enterprises)
+    @importer = ProductImporter.new(File.new(params[:filepath]), editable_enterprises, params[:settings])
     @importer.save_all if @importer.has_valid_entries?
   end
 
diff --git a/app/models/product_importer.rb b/app/models/product_importer.rb
index ee50e9e9c3..7bc5d0ff06 100644
--- a/app/models/product_importer.rb
+++ b/app/models/product_importer.rb
@@ -5,10 +5,9 @@ class ProductImporter
   include ActiveModel::Conversion
   include ActiveModel::Validations
 
-  def initialize(file, editable_enterprises, options={})
+  def initialize(file, editable_enterprises, import_settings={})
     if file.is_a?(File)
       @file = file
-      @options = options
       @sheet = open_spreadsheet
       @valid_entries = {}
       @invalid_entries = {}
@@ -21,10 +20,11 @@ class ProductImporter
       @variants_created = 0
       @variants_updated = 0
 
+      @import_settings = import_settings
       @editable_enterprises = {}
       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'
+      @non_display_attributes = 'id', 'product_id', 'variant_id', 'supplier_id', 'primary_taxon_id', 'category_id', 'shipping_category_id', 'tax_category_id', 'on_hand_nil'
 
       validate_all if @sheet
     else
@@ -144,12 +144,8 @@ class ProductImporter
 
       supplier_validation(line_number, entry)
       category_validation(line_number, entry)
-
-      # Ensure on_hand isn't nil because Spree::Product and
-      # Spree::Variant each validate it differently
-      entry['on_hand'] = 0 if entry['on_hand'].nil?
-
       set_update_status(line_number, entry)
+
       mark_as_valid(line_number, entry) unless entry_invalid?(line_number)
     end
 
@@ -264,14 +260,9 @@ class ProductImporter
 
       product = Spree::Product.new()
       product.assign_attributes(entry.except('id'))
+      assign_defaults(product, entry)
       if product.save
-        # Ensure display_name and on_demand are copied to new variant
-        if entry['display_name'] || entry['on_demand']
-          variant = product.variants.first
-          variant.display_name = entry['display_name'] if entry['display_name']
-          variant.on_demand = entry['on_demand'] if entry['on_demand']
-          variant.save
-        end
+        ensure_variant_updated(entry, product)
         @products_created += 1
       else
         self.errors.add("Line #{line_number}:", product.errors.full_messages)
@@ -282,6 +273,7 @@ class ProductImporter
 
     @variants_to_update.each do |line_number, data|
       variant = data[:variant]
+      assign_defaults(variant, data[:entry])
       if variant.valid? and variant.save
         @variants_updated += 1
       else
@@ -291,6 +283,7 @@ class ProductImporter
 
     @variants_to_create.each do |line_number, data|
       new_variant = data[:variant]
+      assign_defaults(new_variant, data[:entry])
       if new_variant.valid? and new_variant.save
         @variants_created += 1
       else
@@ -303,6 +296,29 @@ class ProductImporter
     total_saved_count
   end
 
+  def assign_defaults(object, entry)
+    @import_settings[entry['supplier_id'].to_s]['defaults'].each do |attribute, setting|
+      case setting['mode']
+        when 'overwrite_all'
+          object.assign_attributes(attribute => setting['value'])
+        when 'overwrite_empty'
+          if object.send(attribute).blank? or (attribute == 'on_hand' and entry['on_hand_nil'])
+            object.assign_attributes(attribute => setting['value'])
+          end
+      end
+    end
+  end
+
+  def ensure_variant_updated(entry, product)
+    # Ensure display_name and on_demand are copied to new product's variant
+    if entry['display_name'] || entry['on_demand']
+      variant = product.variants.first
+      variant.display_name = entry['display_name'] if entry['display_name']
+      variant.on_demand = entry['on_demand'] if entry['on_demand']
+      variant.save
+    end
+  end
+
   def set_update_status(line_number, entry)
     # Find product with matching supplier and name
     match = Spree::Product.where(supplier_id: entry['supplier_id'], name: entry['name'], deleted_at: nil).first
@@ -337,6 +353,7 @@ class ProductImporter
 
   def mark_as_existing_variant(line_number, entry, existing_variant)
     existing_variant.assign_attributes(entry.except('id', 'product_id'))
+    check_on_hand_nil(entry, existing_variant)
     if existing_variant.valid?
       @variants_to_update[line_number] = {entry: entry, variant: existing_variant} unless entry_invalid?(line_number)
     else
@@ -347,6 +364,7 @@ class ProductImporter
   def mark_as_new_variant(line_number, entry, product_id)
     new_variant = Spree::Variant.new(entry.except('id', 'product_id'))
     new_variant.product_id = product_id
+    check_on_hand_nil(entry, new_variant)
     if new_variant.valid?
       @variants_to_create[line_number] = {entry: entry, variant: new_variant} unless entry_invalid?(line_number)
     else
@@ -354,6 +372,13 @@ class ProductImporter
     end
   end
 
+  def check_on_hand_nil(entry, variant)
+    if entry['on_hand'].blank?
+      variant.on_hand = 0
+      entry['on_hand_nil'] = true
+    end
+  end
+
   def delete_uploaded_file
     # Only delete if file is in '/tmp/product_import' directory
     if @file.path == Rails.root.join('tmp', 'product_import').to_s
diff --git a/app/views/admin/product_import/_import_options.html.haml b/app/views/admin/product_import/_import_options.html.haml
index e69de29bb2..64f9b657ff 100644
--- a/app/views/admin/product_import/_import_options.html.haml
+++ b/app/views/admin/product_import/_import_options.html.haml
@@ -0,0 +1,21 @@
+%h5 Import options and defaults
+%br
+
+- @importer.suppliers_index.each do |name, id|
+  - if name and id
+    %div.panel-section.import-settings{ng: {controller: 'DropdownPanelsCtrl'}}
+      %div.panel-header{ng: {click: 'togglePanel()', class: '{active: active}'}}
+        %div.header-caret
+          %i{ng: {class: "{'icon-chevron-down': active, 'icon-chevron-right': !active}"}}
+        %div.header-icon
+          %i.fa.fa-edit
+        -#%div.header-count
+        -#  %strong.invalid-count= @importer.invalid_count
+        %div.header-description
+          = name
+      %div.panel-content{ng: {hide: '!active'}}
+        = render 'options_form', id: id, name: name
+
+
+%br.panels.clearfix
+%br
diff --git a/app/views/admin/product_import/_import_review.html.haml b/app/views/admin/product_import/_import_review.html.haml
index 9422be87f0..52cc334ca2 100644
--- a/app/views/admin/product_import/_import_review.html.haml
+++ b/app/views/admin/product_import/_import_review.html.haml
@@ -1,8 +1,8 @@
 %h5 Import validation overview
 %br
 
-%div.feedback-section
-  %div.feedback-header
+%div.panel-section
+  %div.panel-header
     %div.header-caret
       -#%i.icon-chevron-right{ng: {hide: 'active'}}
       -#%i.icon-chevron-down{ng: {hide: '!active'}}
@@ -12,51 +12,48 @@
       %strong.item-count= @importer.item_count
     %div.header-description
       Entries found in imported file
-  -#%div.feedback-panel{ng: {hide: '!active'}}
+  -#%div.panel-content{ng: {hide: '!active'}}
   -#  Content goes here
 
-%div.feedback-section{ng: {controller: 'FeedbackPanelsCtrl', init: "count = #{@importer.invalid_count}"}}
-  %div.feedback-header{ng: {click: 'togglePanel()', class: '{active: active && count}'}}
+%div.panel-section{ng: {controller: 'DropdownPanelsCtrl', init: "count = #{@importer.invalid_count}"}}
+  %div.panel-header{ng: {click: 'togglePanel()', class: '{active: active && count}'}}
     %div.header-caret
-      %i.icon-chevron-right{ng: {hide: 'active || count == 0'}}
-      %i.icon-chevron-down{ng: {hide: '!active || count == 0'}}
+      %i{ng: {class: "{'icon-chevron-down': active, 'icon-chevron-right': !active}", hide: 'count == 0'}}
     %div.header-icon
       %i.fa.fa-warning
     %div.header-count
       %strong.invalid-count= @importer.invalid_count
     %div.header-description
       Items contain errors and will not be imported
-  %div.feedback-panel{ng: {hide: '!active || count == 0'}}
+  %div.panel-content{ng: {hide: '!active || count == 0'}}
     = render 'errors_list'
     %br
     = render 'entries_table', entries: @importer.invalid_entries
 
-%div.feedback-section{ng: {controller: 'FeedbackPanelsCtrl', init: "count = #{@importer.products_create_count}"}}
-  %div.feedback-header{ng: {click: 'togglePanel()', class: '{active: active && count}'}}
+%div.panel-section{ng: {controller: 'DropdownPanelsCtrl', init: "count = #{@importer.products_create_count}"}}
+  %div.panel-header{ng: {click: 'togglePanel()', class: '{active: active && count}'}}
     %div.header-caret
-      %i.icon-chevron-right{ng: {hide: 'active || count == 0'}}
-      %i.icon-chevron-down{ng: {hide: '!active || count == 0'}}
+      %i{ng: {class: "{'icon-chevron-down': active, 'icon-chevron-right': !active}", hide: 'count == 0'}}
     %div.header-icon
       %i.fa.fa-check-circle
     %div.header-count
       %strong.create-count= @importer.products_create_count
     %div.header-description
       Products will be created
-  %div.feedback-panel{ng: {hide: '!active || count == 0'}}
+  %div.panel-content{ng: {hide: '!active || count == 0'}}
     = render 'entries_table', entries: @importer.products_to_create
 
-%div.feedback-section{ng: {controller: 'FeedbackPanelsCtrl', init: "count = #{@importer.products_update_count}"}}
-  %div.feedback-header{ng: {click: 'togglePanel()', class: '{active: active && count}'}}
+%div.panel-section{ng: {controller: 'DropdownPanelsCtrl', init: "count = #{@importer.products_update_count}"}}
+  %div.panel-header{ng: {click: 'togglePanel()', class: '{active: active && count}'}}
     %div.header-caret
-      %i.icon-chevron-right{ng: {hide: 'active || count == 0'}}
-      %i.icon-chevron-down{ng: {hide: '!active || count == 0'}}
+      %i{ng: {class: "{'icon-chevron-down': active, 'icon-chevron-right': !active}", hide: 'count == 0'}}
     %div.header-icon
       %i.fa.fa-check-circle
     %div.header-count
       %strong.update-count= @importer.products_update_count
     %div.header-description
       Products will be updated
-  %div.feedback-panel{ng: {hide: '!active || count == 0'}}
+  %div.panel-content{ng: {hide: '!active || count == 0'}}
     = render 'entries_table', entries: @importer.products_to_update
 
 %br.panels.clearfix
\ No newline at end of file
diff --git a/app/views/admin/product_import/_options_form.html.haml b/app/views/admin/product_import/_options_form.html.haml
new file mode 100644
index 0000000000..040a6d4f44
--- /dev/null
+++ b/app/views/admin/product_import/_options_form.html.haml
@@ -0,0 +1,35 @@
+%table.import-settings
+  %tr
+    %td.description
+      Remove absent products?
+    %td
+      = check_box_tag "settings[#{id}][products_absent]", 1, false
+    %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'}
+    %td
+      = number_field_tag "settings[#{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'}
+    %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'}
+  %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'}
+    %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'}
+  %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'}
+    %td
+      = text_field_tag "settings[#{id}][defaults][available_on][value]", nil, {class: 'datepicker', placeholder: 'Today'}
diff --git a/app/views/admin/product_import/import.html.haml b/app/views/admin/product_import/import.html.haml
index de0a1ec20e..74422c162f 100644
--- a/app/views/admin/product_import/import.html.haml
+++ b/app/views/admin/product_import/import.html.haml
@@ -10,6 +10,8 @@
     %p There are no entries that can be saved
     %br
 
+  = render 'import_options' if @importer.has_valid_entries?
+
   = render 'import_review'
 
   - if @importer.has_valid_entries?
diff --git a/app/views/admin/product_import/save.html.haml b/app/views/admin/product_import/save.html.haml
index 2b4abdd911..65c7663113 100644
--- a/app/views/admin/product_import/save.html.haml
+++ b/app/views/admin/product_import/save.html.haml
@@ -15,8 +15,8 @@
 
 %br
 
-- if !@importer.errors.full_messages
-  %h5 All #{importer.total_saved_count} items saved successfully
+- if @importer.errors.count == 0
+  %h5 All #{@importer.total_saved_count} items saved successfully
 - else
   %h5 Errors
   - @importer.errors.full_messages.each do |error|
diff --git a/spec/features/admin/product_import_spec.rb b/spec/features/admin/product_import_spec.rb
index 61aa8a4d03..28976fee43 100644
--- a/spec/features/admin/product_import_spec.rb
+++ b/spec/features/admin/product_import_spec.rb
@@ -11,9 +11,11 @@ feature "Product Import", js: true do
   let!(:enterprise2) { create(:supplier_enterprise, owner: admin, name: "Another Enterprise") }
   let!(:category) { create(:taxon, name: 'Vegetables') }
   let!(:category2) { create(:taxon, name: 'Cake') }
+  let!(:tax_category) { create(:tax_category) }
+  let!(:tax_category2) { create(:tax_category) }
+  let!(:shipping_category) { create(:shipping_category) }
   let!(:product) { create(:simple_product, supplier: enterprise, name: 'Hypothetical Cake') }
   let!(:variant) { create(:variant, product_id: product.id, price: '8.50', count_on_hand: '100', unit_value: '500', display_name: 'Preexisting Banana') }
-  let(:permissions) { OpenFoodNetwork::Permissions.new(user) }
 
   describe "when importing products from uploaded file" do
     before { quick_login_as_admin }
@@ -77,7 +79,7 @@ feature "Product Import", js: true do
       carrots.price.should == 3.20
     end
 
-    it "displays info about invalid entries but no save button if all invalid" do
+    it "displays info about invalid entries but no save button if all items are invalid" do
       csv_data = CSV.generate do |csv|
         csv << ["name", "supplier", "category", "on_hand", "price", "unit_value", "variant_unit", "variant_unit_scale"]
         csv << ["Bad Carrots", "Unkown Enterprise", "Mouldy vegetables", "666", "3.20", "", "weight", ""]
@@ -130,6 +132,37 @@ feature "Product Import", js: true do
       updated_banana.price.should == 5.50
       updated_banana.on_hand.should == 5
     end
+
+    it "can add a new product and sub-variants of that product at the same time" do
+      csv_data = CSV.generate do |csv|
+        csv << ["name", "supplier", "category", "on_hand", "price", "unit_value", "variant_unit", "variant_unit_scale", "display_name"]
+        csv << ["Potatoes", "User Enterprise", "Vegetables", "5", "3.50", "500", "weight", "1000", "Small Bag"]
+        csv << ["Potatoes", "User Enterprise", "Vegetables", "6", "5.50", "2000", "weight", "1000", "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 'Import'
+
+      expect(page).to have_selector '.item-count', text: "2"
+      expect(page).to have_selector '.invalid-count', text: "0"
+      expect(page).to have_selector '.create-count', text: "2"
+      expect(page).to have_selector '.update-count', text: "0"
+
+      click_button 'Save'
+      expect(page).to have_content "Products created: 2"
+
+      small_bag = Spree::Variant.find_by_display_name('Small Bag')
+      small_bag.product.name.should == 'Potatoes'
+      small_bag.price.should == 3.50
+      small_bag.on_hand.should == 5
+
+      big_bag = Spree::Variant.find_by_display_name('Big Bag')
+      big_bag.product.name.should == 'Potatoes'
+      big_bag.price.should == 5.50
+      big_bag.on_hand.should == 6
+    end
   end
 
   describe "when dealing with uploaded files" do
@@ -200,4 +233,57 @@ feature "Product Import", js: true do
     end
   end
 
+  describe "applying settings and defaults on import" do
+    before { quick_login_as_admin }
+
+    it "overwrites fields with selected defaults" do
+      csv_data = CSV.generate do |csv|
+        csv << ["name", "supplier", "category", "on_hand", "price", "unit_value", "variant_unit", "variant_unit_scale", "tax_category_id", "available_on"]
+        csv << ["Carrots", "User Enterprise", "Vegetables", "5", "3.20", "500", "weight", "1", tax_category.id, ""]
+        csv << ["Potatoes", "User Enterprise", "Vegetables", "6", "6.50", "1000", "weight", "1000", "", ""]
+      end
+      File.write('/tmp/test.csv', csv_data)
+
+      visit main_app.admin_product_import_path
+
+      attach_file 'file', '/tmp/test.csv'
+      click_button 'Import'
+
+      within 'div.import-settings' do
+        find('div.header-description').click  # Import settings tab
+        expect(page).to have_selector "#settings_#{enterprise.id}_defaults_on_hand_mode", visible: false
+
+        # Overwrite stock level of all items to 9000
+        select 'Overwrite all', from: "settings_#{enterprise.id}_defaults_on_hand_mode", visible: false
+        fill_in "settings_#{enterprise.id}_defaults_on_hand_value", with: '9000'
+
+        # Overwrite default tax category, but only where field is empty
+        select 'Overwrite if empty', from: "settings_#{enterprise.id}_defaults_tax_category_id_mode", visible: false
+        select tax_category2.name, from: "settings_#{enterprise.id}_defaults_tax_category_id_value", visible: false
+
+        # Set default shipping category (field not present in file)
+        select 'Overwrite all', from: "settings_#{enterprise.id}_defaults_shipping_category_id_mode", visible: false
+        select shipping_category.name, from: "settings_#{enterprise.id}_defaults_shipping_category_id_value", visible: false
+
+        # Set available_on date
+        select 'Overwrite all', from: "settings_#{enterprise.id}_defaults_available_on_mode", visible: false
+        find("input#settings_#{enterprise.id}_defaults_available_on_value").set '2020-01-01'
+      end
+
+      click_button 'Save'
+      expect(page).to have_content "Products created: 2"
+
+      carrots = Spree::Product.find_by_name('Carrots')
+      carrots.on_hand.should == 9000
+      carrots.tax_category_id.should == tax_category.id
+      carrots.shipping_category_id.should == shipping_category.id
+      carrots.available_on.should be_within(1.day).of(Time.zone.local(2020, 1, 1))
+
+      potatoes = Spree::Product.find_by_name('Potatoes')
+      potatoes.on_hand.should == 9000
+      potatoes.tax_category_id.should == tax_category2.id
+      potatoes.shipping_category_id.should == shipping_category.id
+      potatoes.available_on.should be_within(1.day).of(Time.zone.local(2020, 1, 1))
+    end
+  end
 end
\ No newline at end of file

From 24fcc3dd34b3afa912a7e12cdcd860260f3a1cd7 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <visible_souls@hotmail.com>
Date: Sun, 5 Mar 2017 21:16:07 +0000
Subject: [PATCH 12/18] PI reset absent products

---
 .../controllers/import_options_form.js.coffee |  9 +++
 .../services/product_import_service.js.coffee | 15 ++++
 .../stylesheets/admin/product_import.css.scss | 25 ++++---
 app/models/product_importer.rb                | 74 +++++++++++++++++--
 .../product_import/_import_options.html.haml  |  2 +-
 .../product_import/_import_review.html.haml   | 35 +++++++--
 .../product_import/_options_form.html.haml    |  4 +-
 spec/features/admin/product_import_spec.rb    | 42 ++++++++++-
 8 files changed, 177 insertions(+), 29 deletions(-)
 create mode 100644 app/assets/javascripts/admin/product_import/controllers/import_options_form.js.coffee
 create mode 100644 app/assets/javascripts/admin/product_import/services/product_import_service.js.coffee

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
new file mode 100644
index 0000000000..68f12079c4
--- /dev/null
+++ b/app/assets/javascripts/admin/product_import/controllers/import_options_form.js.coffee
@@ -0,0 +1,9 @@
+angular.module("ofn.admin").controller "ImportOptionsFormCtrl", ($scope, $timeout, $rootScope, ProductImportService) ->
+
+  $scope.toggleResetAbsent = () ->
+    ProductImportService.updateResetAbsent($scope.supplierId, $scope.nonUpdated, $scope.resetAbsent)
+
+  $scope.resetCount = ProductImportService.resetCount
+
+  $rootScope.$watch 'resetCount', (newValue) ->
+    $scope.resetCount = 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
new file mode 100644
index 0000000000..d65da24e95
--- /dev/null
+++ b/app/assets/javascripts/admin/product_import/services/product_import_service.js.coffee
@@ -0,0 +1,15 @@
+angular.module("ofn.admin").factory "ProductImportService", ($rootScope, $timeout) ->
+  new class ProductImportService
+    suppliers: {}
+    resetCount: 0
+
+    updateResetAbsent: (supplierId, nonUpdated, resetAbsent) ->
+      if resetAbsent
+        @suppliers[supplierId] = nonUpdated
+        @resetCount += nonUpdated
+      else
+        @suppliers[supplierId] = null
+        @resetCount -= nonUpdated
+
+      $rootScope.resetCount = @resetCount
+
diff --git a/app/assets/stylesheets/admin/product_import.css.scss b/app/assets/stylesheets/admin/product_import.css.scss
index e99cc9beea..301a91c741 100644
--- a/app/assets/stylesheets/admin/product_import.css.scss
+++ b/app/assets/stylesheets/admin/product_import.css.scss
@@ -24,16 +24,23 @@ div.panel-section {
       text-align: center;
       padding-top: 0.18em;
 
-      .fa {
+      i {
         font-size: 1.5em;
         line-height: 0.9em;
       }
-      .fa-warning {
-        color: #ee4728;
-      }
-      .fa-check-circle {
-        color: #86d83a;
-      }
+    }
+
+    .neutral {
+      color: #BFBFBF;
+    }
+    .warning {
+      color: #ee4728;
+    }
+    .success {
+      color: #86d83a;
+    }
+    .info {
+      color: #68b7c0;
     }
 
     div.header-count {
@@ -145,9 +152,7 @@ table.import-settings {
 }
 
 .panel-section.import-settings {
-  .header-icon {
-    color: #BFBFBF;
-  }
+
   .header-description {
     padding-left: 1em;
   }
diff --git a/app/models/product_importer.rb b/app/models/product_importer.rb
index 7bc5d0ff06..dea26ed720 100644
--- a/app/models/product_importer.rb
+++ b/app/models/product_importer.rb
@@ -25,6 +25,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: {}}
+      @updated_ids = []
 
       validate_all if @sheet
     else
@@ -44,6 +46,18 @@ 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?
+
+      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]
+      end
+    end
+    @supplier_products
+  end
+
   def valid_count
     @valid_entries.count
   end
@@ -140,7 +154,7 @@ class ProductImporter
 
   def validate_all
     entries.each_with_index do |entry, i|
-      line_number = i+2
+      line_number = i+2 # Roo counts "line 2" as the first line of data
 
       supplier_validation(line_number, entry)
       category_validation(line_number, entry)
@@ -149,9 +163,26 @@ class ProductImporter
       mark_as_valid(line_number, entry) unless entry_invalid?(line_number)
     end
 
+    count_existing_products
+
     delete_uploaded_file if item_count.zero? or valid_count.zero?
   end
 
+  def count_existing_products
+    @suppliers_index.each do |supplier_name, supplier_id|
+      if 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
+
+        @supplier_products[:by_supplier][supplier_id] = {existing_products: products_count}
+        @supplier_products[:total] += products_count
+      end
+    end
+  end
+
   def entry_invalid?(line_number)
     !!@invalid_entries[line_number]
   end
@@ -246,14 +277,14 @@ class ProductImporter
   end
 
   def save_all_valid
-    updated = {}
+    already_created = {}
     @products_to_create.each do |line_number, data|
       entry = data[:entry]
       # If we've already added a new product with these attributes
       # from this spreadsheet, mark this entry as a new variant with
       # the new product id, as this is a now variant of that product...
-      if updated[entry['supplier_id']] && updated[entry['supplier_id']][entry['name']]
-        product_id = updated[entry['supplier_id']][entry['name']]
+      if already_created[entry['supplier_id']] and already_created[entry['supplier_id']][entry['name']]
+        product_id = already_created[entry['supplier_id']][entry['name']]
         mark_as_new_variant(line_number, entry, product_id)
         next
       end
@@ -264,11 +295,12 @@ class ProductImporter
       if product.save
         ensure_variant_updated(entry, product)
         @products_created += 1
+        @updated_ids.push product.variants.first.id
       else
         self.errors.add("Line #{line_number}:", product.errors.full_messages)
       end
 
-      updated[entry['supplier_id']] = {entry['name'] => product.id}
+      already_created[entry['supplier_id']] = {entry['name'] => product.id}
     end
 
     @variants_to_update.each do |line_number, data|
@@ -276,6 +308,7 @@ class ProductImporter
       assign_defaults(variant, data[:entry])
       if variant.valid? and variant.save
         @variants_updated += 1
+        @updated_ids.push variant.id
       else
         self.errors.add("Line #{line_number}:", variant.errors.full_messages)
       end
@@ -286,16 +319,36 @@ class ProductImporter
       assign_defaults(new_variant, data[:entry])
       if new_variant.valid? and new_variant.save
         @variants_created += 1
+        @updated_ids.push new_variant.id
       else
         self.errors.add("Line #{line_number}:", new_variant.errors.full_messages)
       end
     end
 
-    self.errors.add(:importer, "did not save any products successfully") if total_saved_count == 0
+    self.errors.add(:importer, "did not save any products successfully") if total_saved_count.zero?
 
+    reset_absent_products
     total_saved_count
   end
 
+  def reset_absent_products
+    return if total_saved_count.zero?
+
+    enterprises_to_reset = []
+    @import_settings.each do |enterprise_id, settings|
+      enterprises_to_reset.push enterprise_id if settings['reset_all_absent']
+    end
+
+    unless enterprises_to_reset.empty? or @updated_ids.empty?
+      # Set stock to zero for all products in selected enterprises that were not
+      # present in the uploaded spreadsheet.
+      Spree::Variant.joins(:product).
+        where('spree_products.supplier_id IN (?)
+          AND spree_variants.id NOT IN (?)', enterprises_to_reset, @updated_ids).
+        update_all(count_on_hand: 0)
+    end
+  end
+
   def assign_defaults(object, entry)
     @import_settings[entry['supplier_id'].to_s]['defaults'].each do |attribute, setting|
       case setting['mode']
@@ -356,6 +409,7 @@ class ProductImporter
     check_on_hand_nil(entry, existing_variant)
     if existing_variant.valid?
       @variants_to_update[line_number] = {entry: entry, variant: existing_variant} unless entry_invalid?(line_number)
+      updates_count_per_supplier(entry['supplier_id']) unless entry_invalid?(line_number)
     else
       mark_as_invalid(line_number, entry, existing_variant.errors.full_messages)
     end
@@ -372,6 +426,14 @@ class ProductImporter
     end
   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
+    else
+      @supplier_products[:by_supplier][supplier_id] = {updates_count: 1}
+    end
+  end
+
   def check_on_hand_nil(entry, variant)
     if entry['on_hand'].blank?
       variant.on_hand = 0
diff --git a/app/views/admin/product_import/_import_options.html.haml b/app/views/admin/product_import/_import_options.html.haml
index 64f9b657ff..242c0d8638 100644
--- a/app/views/admin/product_import/_import_options.html.haml
+++ b/app/views/admin/product_import/_import_options.html.haml
@@ -7,7 +7,7 @@
       %div.panel-header{ng: {click: 'togglePanel()', class: '{active: active}'}}
         %div.header-caret
           %i{ng: {class: "{'icon-chevron-down': active, 'icon-chevron-right': !active}"}}
-        %div.header-icon
+        %div.header-icon.neutral
           %i.fa.fa-edit
         -#%div.header-count
         -#  %strong.invalid-count= @importer.invalid_count
diff --git a/app/views/admin/product_import/_import_review.html.haml b/app/views/admin/product_import/_import_review.html.haml
index 52cc334ca2..4f547b0dbb 100644
--- a/app/views/admin/product_import/_import_review.html.haml
+++ b/app/views/admin/product_import/_import_review.html.haml
@@ -4,9 +4,32 @@
 %div.panel-section
   %div.panel-header
     %div.header-caret
-      -#%i.icon-chevron-right{ng: {hide: 'active'}}
-      -#%i.icon-chevron-down{ng: {hide: '!active'}}
-    %div.header-icon
+    %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{ng: {controller: 'ImportOptionsFormCtrl', hide: 'resetCount == 0'}}
+  %div.panel-header
+    %div.header-caret
+    %div.header-icon.info
+      %i.fa.fa-info-circle
+    %div.header-count
+      %strong.reset-count
+        {{resetCount}}
+    %div.header-description
+      Existing products will have their stock reset to zero
+  -#%div.panel-content{ng: {hide: '!active'}}
+  -#  Content goes here
+
+%div.panel-section
+  %div.panel-header
+    %div.header-caret
+    %div.header-icon.success
       %i.fa.fa-check-circle
     %div.header-count
       %strong.item-count= @importer.item_count
@@ -19,7 +42,7 @@
   %div.panel-header{ng: {click: 'togglePanel()', class: '{active: active && count}'}}
     %div.header-caret
       %i{ng: {class: "{'icon-chevron-down': active, 'icon-chevron-right': !active}", hide: 'count == 0'}}
-    %div.header-icon
+    %div.header-icon.warning
       %i.fa.fa-warning
     %div.header-count
       %strong.invalid-count= @importer.invalid_count
@@ -34,7 +57,7 @@
   %div.panel-header{ng: {click: 'togglePanel()', class: '{active: active && count}'}}
     %div.header-caret
       %i{ng: {class: "{'icon-chevron-down': active, 'icon-chevron-right': !active}", hide: 'count == 0'}}
-    %div.header-icon
+    %div.header-icon.success
       %i.fa.fa-check-circle
     %div.header-count
       %strong.create-count= @importer.products_create_count
@@ -47,7 +70,7 @@
   %div.panel-header{ng: {click: 'togglePanel()', class: '{active: active && count}'}}
     %div.header-caret
       %i{ng: {class: "{'icon-chevron-down': active, 'icon-chevron-right': !active}", hide: 'count == 0'}}
-    %div.header-icon
+    %div.header-icon.success
       %i.fa.fa-check-circle
     %div.header-count
       %strong.update-count= @importer.products_update_count
diff --git a/app/views/admin/product_import/_options_form.html.haml b/app/views/admin/product_import/_options_form.html.haml
index 040a6d4f44..c6a5c28030 100644
--- a/app/views/admin/product_import/_options_form.html.haml
+++ b/app/views/admin/product_import/_options_form.html.haml
@@ -1,9 +1,9 @@
-%table.import-settings
+%table.import-settings{ng: {controller: 'ImportOptionsFormCtrl', init: "supplierId = #{id}; nonUpdated = #{@importer.supplier_products[:by_supplier][id][:non_updated]}"}}
   %tr
     %td.description
       Remove absent products?
     %td
-      = check_box_tag "settings[#{id}][products_absent]", 1, false
+      = check_box_tag "settings[#{id}][reset_all_absent]", 1, false, :'ng-model' => 'resetAbsent', :'ng-change' => 'toggleResetAbsent()'
     %td
   %tr
     %td.description
diff --git a/spec/features/admin/product_import_spec.rb b/spec/features/admin/product_import_spec.rb
index 28976fee43..23703b953c 100644
--- a/spec/features/admin/product_import_spec.rb
+++ b/spec/features/admin/product_import_spec.rb
@@ -14,8 +14,12 @@ feature "Product Import", js: true do
   let!(:tax_category) { create(:tax_category) }
   let!(:tax_category2) { create(:tax_category) }
   let!(:shipping_category) { create(:shipping_category) }
-  let!(:product) { create(:simple_product, supplier: enterprise, name: 'Hypothetical Cake') }
-  let!(:variant) { create(:variant, product_id: product.id, price: '8.50', count_on_hand: '100', unit_value: '500', display_name: 'Preexisting Banana') }
+  let!(:product) { create(:simple_product, supplier: enterprise2, name: 'Hypothetical Cake') }
+  let!(:variant) { create(:variant, product_id: product.id, price: '8.50', on_hand: '100', unit_value: '500', display_name: 'Preexisting Banana') }
+  let!(:product2) { create(:simple_product, supplier: enterprise, on_hand: '100', name: 'Beans') }
+  let!(:product3) { create(:simple_product, supplier: enterprise, on_hand: '100', name: 'Sprouts') }
+  let!(:product4) { create(:simple_product, supplier: enterprise2, on_hand: '100', name: 'Lettuce') }
+
 
   describe "when importing products from uploaded file" do
     before { quick_login_as_admin }
@@ -104,8 +108,8 @@ feature "Product Import", js: true do
     it "can add new variants to existing products and update price and stock level of existing products" do
       csv_data = CSV.generate do |csv|
         csv << ["name", "supplier", "category", "on_hand", "price", "unit_value", "variant_unit", "variant_unit_scale", "display_name"]
-        csv << ["Hypothetical Cake", "User Enterprise", "Cake", "5", "5.50", "500", "weight", "1", "Preexisting Banana"]
-        csv << ["Hypothetical Cake", "User Enterprise", "Cake", "6", "3.50", "500", "weight", "1", "Emergent Coffee"]
+        csv << ["Hypothetical Cake", "Another Enterprise", "Cake", "5", "5.50", "500", "weight", "1", "Preexisting Banana"]
+        csv << ["Hypothetical Cake", "Another Enterprise", "Cake", "6", "3.50", "500", "weight", "1", "Emergent Coffee"]
       end
       File.write('/tmp/test.csv', csv_data)
 
@@ -236,6 +240,36 @@ feature "Product Import", js: true do
   describe "applying settings and defaults on import" do
     before { quick_login_as_admin }
 
+    it "can set all products for an enterprise that are not present in the uploaded file to zero stock" do
+      csv_data = CSV.generate do |csv|
+        csv << ["name", "supplier", "category", "on_hand", "price", "unit_value", "variant_unit", "variant_unit_scale"]
+        csv << ["Carrots", "User Enterprise", "Vegetables", "5", "3.20", "500", "weight", "1"]
+        csv << ["Potatoes", "User Enterprise", "Vegetables", "6", "6.50", "1000", "weight", "1000"]
+      end
+      File.write('/tmp/test.csv', csv_data)
+
+      visit main_app.admin_product_import_path
+
+      attach_file 'file', '/tmp/test.csv'
+      click_button 'Import'
+
+      within 'div.import-settings' do
+        find('div.header-description').click  # Import settings tab
+        check "settings_#{enterprise.id}_reset_all_absent"
+      end
+
+      expect(page).to have_selector '.reset-count', text: "2"
+
+      click_button 'Save'
+      expect(page).to have_content "Products created: 2"
+
+      Spree::Product.find_by_name('Carrots').on_hand.should == 5    # Present in file
+      Spree::Product.find_by_name('Potatoes').on_hand.should == 6   # Present in file
+      Spree::Product.find_by_name('Beans').on_hand.should == 0      # In enterprise, not in file
+      Spree::Product.find_by_name('Sprouts').on_hand.should == 0    # In enterprise, not in file
+      Spree::Product.find_by_name('Lettuce').on_hand.should == 100  # In different enterprise; unchanged
+    end
+
     it "overwrites fields with selected defaults" do
       csv_data = CSV.generate do |csv|
         csv << ["name", "supplier", "category", "on_hand", "price", "unit_value", "variant_unit", "variant_unit_scale", "tax_category_id", "available_on"]

From 648753b41218e24ea0052531e1b74d801db20472 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <visible_souls@hotmail.com>
Date: Sun, 5 Mar 2017 23:28:31 +0000
Subject: [PATCH 13/18] Improved save step UI

---
 .../stylesheets/admin/product_import.css.scss | 32 +++++++++++++++
 app/views/admin/product_import/save.html.haml | 39 +++++++++++--------
 spec/features/admin/product_import_spec.rb    | 27 +++++++++----
 3 files changed, 73 insertions(+), 25 deletions(-)

diff --git a/app/assets/stylesheets/admin/product_import.css.scss b/app/assets/stylesheets/admin/product_import.css.scss
index 301a91c741..99ed52c758 100644
--- a/app/assets/stylesheets/admin/product_import.css.scss
+++ b/app/assets/stylesheets/admin/product_import.css.scss
@@ -165,3 +165,35 @@ table.import-settings {
 .select2-results {
   margin: 0;
 }
+
+.post-save-results {
+  p {
+    font-size: 1.25em;
+    margin-bottom: 0.5em;
+
+    strong {
+      margin-left: 0.5em;
+      margin-right: 0.2em;
+    }
+
+    i {
+      font-size: 1.4em;
+      vertical-align: middle;
+      position: relative;
+      //margin-bottom: -0.8em;
+    }
+
+    i.fa-check-circle {
+      color: #86d83a;
+    }
+    i.fa-info-circle {
+      color: #68b7c0;
+    }
+  }
+
+  p.save-error {
+    color: #ee4728;
+    font-size: 1.05em;
+    margin-top: 0.4em;
+  }
+}
\ No newline at end of file
diff --git a/app/views/admin/product_import/save.html.haml b/app/views/admin/product_import/save.html.haml
index 65c7663113..a3cf6b2fd3 100644
--- a/app/views/admin/product_import/save.html.haml
+++ b/app/views/admin/product_import/save.html.haml
@@ -3,25 +3,30 @@
 
 = render :partial => 'spree/admin/shared/product_sub_menu'
 
-%h5 Import results
+%h5 Import final results
 %br
 
-%p{style: 'font-size: 1.15em'}
-  Products created:
-  = @importer.products_created_count
-%p{style: 'font-size: 1.15em'}
-  Products updated:
-  = @importer.products_updated_count
+%div.post-save-results{ng: {app: 'ofn.admin'}}
 
-%br
+  %p
+    %i.fa{ng: {class: "{'fa-info-circle': #{@importer.products_created_count} == 0, 'fa-check-circle': #{@importer.products_created_count} != 0}"}}
+    %strong.created-count= @importer.products_created_count
+    Products created
 
-- if @importer.errors.count == 0
-  %h5 All #{@importer.total_saved_count} items saved successfully
-- else
-  %h5 Errors
-  - @importer.errors.full_messages.each do |error|
-    %p{class: "red"}
-      &nbsp;-&nbsp; #{error}
+  %p
+    %i.fa{ng: {class: "{'fa-info-circle': #{@importer.products_updated_count} == 0, 'fa-check-circle': #{@importer.products_updated_count} != 0}"}}
+    %strong.updated-count= @importer.products_updated_count
+    Products updated
 
-%br
-%a.button{href: main_app.admin_product_import_path} Back
+  %br
+
+  - if @importer.errors.count == 0
+    %p All #{@importer.total_saved_count} items saved successfully
+  - else
+    %h5 Save errors
+    - @importer.errors.full_messages.each do |error|
+      %p.save-error
+        &nbsp;-&nbsp; #{error}
+
+  %br
+  %a.button{href: main_app.admin_product_import_path} Back
diff --git a/spec/features/admin/product_import_spec.rb b/spec/features/admin/product_import_spec.rb
index 23703b953c..a38a32b877 100644
--- a/spec/features/admin/product_import_spec.rb
+++ b/spec/features/admin/product_import_spec.rb
@@ -45,7 +45,8 @@ feature "Product Import", js: true do
       expect(page).to have_selector '.update-count', text: "0"
 
       click_button 'Save'
-      expect(page).to have_content "Products created: 2"
+      expect(page).to have_selector '.created-count', text: '2'
+      expect(page).to have_selector '.updated-count', text: '0'
 
       potatoes = Spree::Product.find_by_name('Potatoes')
       potatoes.supplier.should == enterprise
@@ -74,7 +75,9 @@ feature "Product Import", js: true do
 
       expect(page).to have_selector 'input[type=submit][value="Save"]'
       click_button 'Save'
-      expect(page).to have_content "Products created: 1"
+
+      expect(page).to have_selector '.created-count', text: '1'
+      expect(page).to have_selector '.updated-count', text: '0'
 
       Spree::Product.find_by_name('Bad Potatoes').should == nil
       carrots = Spree::Product.find_by_name('Good Carrots')
@@ -123,8 +126,9 @@ feature "Product Import", js: true do
       expect(page).to have_selector '.update-count', text: "1"
 
       click_button 'Save'
-      expect(page).to have_content "Products created: 1"
-      expect(page).to have_content "Products updated: 1"
+
+      expect(page).to have_selector '.created-count', text: '1'
+      expect(page).to have_selector '.updated-count', text: '1'
 
       added_coffee = Spree::Variant.find_by_display_name('Emergent Coffee')
       added_coffee.product.name.should == 'Hypothetical Cake'
@@ -155,7 +159,8 @@ feature "Product Import", js: true do
       expect(page).to have_selector '.update-count', text: "0"
 
       click_button 'Save'
-      expect(page).to have_content "Products created: 2"
+      expect(page).to have_selector '.created-count', text: '2'
+      expect(page).to have_selector '.updated-count', text: '0'
 
       small_bag = Spree::Variant.find_by_display_name('Small Bag')
       small_bag.product.name.should == 'Potatoes'
@@ -230,7 +235,9 @@ feature "Product Import", js: true do
       expect(page.body).to have_content 'You do not have permission to manage products for "Another Enterprise"'
 
       click_button 'Save'
-      expect(page).to have_content "Products created: 1"
+
+      expect(page).to have_selector '.created-count', text: '1'
+      expect(page).to have_selector '.updated-count', text: '0'
 
       Spree::Product.find_by_name('My Carrots').should be_a Spree::Product
       Spree::Product.find_by_name('Your Potatoes').should == nil
@@ -261,7 +268,9 @@ feature "Product Import", js: true do
       expect(page).to have_selector '.reset-count', text: "2"
 
       click_button 'Save'
-      expect(page).to have_content "Products created: 2"
+
+      expect(page).to have_selector '.created-count', text: '2'
+      expect(page).to have_selector '.updated-count', text: '0'
 
       Spree::Product.find_by_name('Carrots').on_hand.should == 5    # Present in file
       Spree::Product.find_by_name('Potatoes').on_hand.should == 6   # Present in file
@@ -305,7 +314,9 @@ feature "Product Import", js: true do
       end
 
       click_button 'Save'
-      expect(page).to have_content "Products created: 2"
+
+      expect(page).to have_selector '.created-count', text: '2'
+      expect(page).to have_selector '.updated-count', text: '0'
 
       carrots = Spree::Product.find_by_name('Carrots')
       carrots.on_hand.should == 9000

From 91fc3f33a0ca52494e9547ca0867a6b00ed04a29 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <visible_souls@hotmail.com>
Date: Mon, 6 Mar 2017 19:21:06 +0000
Subject: [PATCH 14/18] PI reset and save step improvements

---
 .../controllers/import_options_form.js.coffee |  8 +++++-
 .../stylesheets/admin/product_import.css.scss |  5 ++--
 app/models/product_importer.rb                | 21 ++++++++++++----
 app/views/admin/product_import/save.html.haml |  6 +++++
 spec/features/admin/product_import_spec.rb    | 25 +++++++++++++------
 5 files changed, 49 insertions(+), 16 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 68f12079c4..e1c99a42b5 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,7 +1,13 @@
 angular.module("ofn.admin").controller "ImportOptionsFormCtrl", ($scope, $timeout, $rootScope, ProductImportService) ->
 
   $scope.toggleResetAbsent = () ->
-    ProductImportService.updateResetAbsent($scope.supplierId, $scope.nonUpdated, $scope.resetAbsent)
+    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)
+    else
+      $scope.resetAbsent = false
 
   $scope.resetCount = ProductImportService.resetCount
 
diff --git a/app/assets/stylesheets/admin/product_import.css.scss b/app/assets/stylesheets/admin/product_import.css.scss
index 99ed52c758..b878c63d8c 100644
--- a/app/assets/stylesheets/admin/product_import.css.scss
+++ b/app/assets/stylesheets/admin/product_import.css.scss
@@ -172,15 +172,16 @@ table.import-settings {
     margin-bottom: 0.5em;
 
     strong {
-      margin-left: 0.5em;
       margin-right: 0.2em;
+      min-width: 1.8em;
+      display: inline-block;
+      text-align: right;
     }
 
     i {
       font-size: 1.4em;
       vertical-align: middle;
       position: relative;
-      //margin-bottom: -0.8em;
     }
 
     i.fa-check-circle {
diff --git a/app/models/product_importer.rb b/app/models/product_importer.rb
index dea26ed720..801fdd724e 100644
--- a/app/models/product_importer.rb
+++ b/app/models/product_importer.rb
@@ -110,6 +110,10 @@ class ProductImporter
     @variants_updated
   end
 
+  def products_reset_count
+    @products_reset_count || 0
+  end
+
   def total_saved_count
     @products_created + @variants_created + @variants_updated
   end
@@ -177,7 +181,12 @@ class ProductImporter
           AND spree_variants.deleted_at IS NULL', supplier_id).
           count
 
-        @supplier_products[:by_supplier][supplier_id] = {existing_products: products_count}
+        if @supplier_products[:by_supplier][supplier_id]
+          @supplier_products[:by_supplier][supplier_id][:existing_products] = products_count
+        else
+          @supplier_products[:by_supplier][supplier_id] = {existing_products: products_count}
+        end
+
         @supplier_products[:total] += products_count
       end
     end
@@ -340,11 +349,13 @@ class ProductImporter
     end
 
     unless enterprises_to_reset.empty? or @updated_ids.empty?
-      # Set stock to zero for all products in selected enterprises that were not
-      # present in the uploaded spreadsheet.
-      Spree::Variant.joins(:product).
+      # For selected enterprises; set stock to zero for all products
+      # that were not present in the uploaded spreadsheet
+      @products_reset_count = Spree::Variant.joins(:product).
         where('spree_products.supplier_id IN (?)
-          AND spree_variants.id NOT IN (?)', enterprises_to_reset, @updated_ids).
+        AND spree_variants.id NOT IN (?)
+        AND spree_variants.is_master = false
+        AND spree_variants.deleted_at IS NULL', enterprises_to_reset, @updated_ids).
         update_all(count_on_hand: 0)
     end
   end
diff --git a/app/views/admin/product_import/save.html.haml b/app/views/admin/product_import/save.html.haml
index a3cf6b2fd3..06d11e5f35 100644
--- a/app/views/admin/product_import/save.html.haml
+++ b/app/views/admin/product_import/save.html.haml
@@ -18,6 +18,12 @@
     %strong.updated-count= @importer.products_updated_count
     Products updated
 
+  - if @importer.products_reset_count > 0
+    %p
+      %i.fa.fa-check-circle
+      %strong.reset-count= @importer.products_reset_count
+      Products had stock level reset to zero
+
   %br
 
   - if @importer.errors.count == 0
diff --git a/spec/features/admin/product_import_spec.rb b/spec/features/admin/product_import_spec.rb
index a38a32b877..dbe204af33 100644
--- a/spec/features/admin/product_import_spec.rb
+++ b/spec/features/admin/product_import_spec.rb
@@ -16,9 +16,10 @@ feature "Product Import", js: true do
   let!(:shipping_category) { create(:shipping_category) }
   let!(:product) { create(:simple_product, supplier: enterprise2, name: 'Hypothetical Cake') }
   let!(:variant) { create(:variant, product_id: product.id, price: '8.50', on_hand: '100', unit_value: '500', display_name: 'Preexisting Banana') }
-  let!(:product2) { create(:simple_product, supplier: enterprise, on_hand: '100', name: 'Beans') }
+  let!(:product2) { create(:simple_product, supplier: enterprise, on_hand: '100', name: 'Beans', unit_value: '500') }
   let!(:product3) { create(:simple_product, supplier: enterprise, on_hand: '100', name: 'Sprouts') }
-  let!(:product4) { create(:simple_product, supplier: enterprise2, on_hand: '100', name: 'Lettuce') }
+  let!(:product4) { create(:simple_product, supplier: enterprise, on_hand: '100', name: 'Cabbage') }
+  let!(:product5) { create(:simple_product, supplier: enterprise2, on_hand: '100', name: 'Lettuce') }
 
 
   describe "when importing products from uploaded file" do
@@ -251,7 +252,7 @@ feature "Product Import", js: true do
       csv_data = CSV.generate do |csv|
         csv << ["name", "supplier", "category", "on_hand", "price", "unit_value", "variant_unit", "variant_unit_scale"]
         csv << ["Carrots", "User Enterprise", "Vegetables", "5", "3.20", "500", "weight", "1"]
-        csv << ["Potatoes", "User Enterprise", "Vegetables", "6", "6.50", "1000", "weight", "1000"]
+        csv << ["Beans", "User Enterprise", "Vegetables", "6", "6.50", "500", "weight", "1"]
       end
       File.write('/tmp/test.csv', csv_data)
 
@@ -260,6 +261,13 @@ feature "Product Import", js: true do
       attach_file 'file', '/tmp/test.csv'
       click_button 'Import'
 
+      expect(page).to have_selector '.item-count', text: "2"
+      expect(page).to have_selector '.invalid-count', text: "0"
+      expect(page).to have_selector '.create-count', text: "1"
+      expect(page).to have_selector '.update-count', text: "1"
+
+      expect(page).to_not have_selector '.reset-count'
+
       within 'div.import-settings' do
         find('div.header-description').click  # Import settings tab
         check "settings_#{enterprise.id}_reset_all_absent"
@@ -269,13 +277,14 @@ feature "Product Import", js: true do
 
       click_button 'Save'
 
-      expect(page).to have_selector '.created-count', text: '2'
-      expect(page).to have_selector '.updated-count', text: '0'
+      expect(page).to have_selector '.created-count', text: '1'
+      expect(page).to have_selector '.updated-count', text: '1'
+      expect(page).to have_selector '.reset-count', text: '2'
 
-      Spree::Product.find_by_name('Carrots').on_hand.should == 5    # Present in file
-      Spree::Product.find_by_name('Potatoes').on_hand.should == 6   # Present in file
-      Spree::Product.find_by_name('Beans').on_hand.should == 0      # In enterprise, not in file
+      Spree::Product.find_by_name('Carrots').on_hand.should == 5    # Present in file, added
+      Spree::Product.find_by_name('Beans').on_hand.should == 6      # Present in file, updated
       Spree::Product.find_by_name('Sprouts').on_hand.should == 0    # In enterprise, not in file
+      Spree::Product.find_by_name('Cabbage').on_hand.should == 0    # In enterprise, not in file
       Spree::Product.find_by_name('Lettuce').on_hand.should == 100  # In different enterprise; unchanged
     end
 

From cc5a335fb7456e6043a016cbd1d9fa551eab33a8 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <visible_souls@hotmail.com>
Date: Tue, 7 Mar 2017 14:31:44 +0000
Subject: [PATCH 15/18] 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'}

From 5e1e4c1d194f21e3e8043209b80bc862601637fc Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <visible_souls@hotmail.com>
Date: Thu, 9 Mar 2017 20:25:13 +0000
Subject: [PATCH 16/18] Product Import UX review updates

Minor tweaks

Minor fix
---
 .../stylesheets/admin/product_import.css.scss | 57 ++++++++++++-------
 app/models/product_importer.rb                | 12 +++-
 .../product_import/_entries_table.html.haml   |  5 +-
 .../product_import/_import_options.html.haml  | 28 +++++++++
 .../product_import/_import_review.html.haml   | 37 ++++++------
 5 files changed, 99 insertions(+), 40 deletions(-)

diff --git a/app/assets/stylesheets/admin/product_import.css.scss b/app/assets/stylesheets/admin/product_import.css.scss
index b878c63d8c..5d986f51dc 100644
--- a/app/assets/stylesheets/admin/product_import.css.scss
+++ b/app/assets/stylesheets/admin/product_import.css.scss
@@ -1,5 +1,18 @@
 div.panel-section {
 
+  .neutral {
+    color: #bfbfbf;
+  }
+  .warning {
+    color: #da5354;
+  }
+  .success {
+    color: #86d83a;
+  }
+  .info {
+    color: #68b7c0;
+  }
+
   div.panel-header {
     width: 100%;
     //font-size: 1.5em;
@@ -30,19 +43,6 @@ div.panel-section {
       }
     }
 
-    .neutral {
-      color: #BFBFBF;
-    }
-    .warning {
-      color: #ee4728;
-    }
-    .success {
-      color: #86d83a;
-    }
-    .info {
-      color: #68b7c0;
-    }
-
     div.header-count {
       min-width: 2em;
       text-align: right;
@@ -86,6 +86,18 @@ div.panel-section {
       td, th {
         white-space: nowrap;
       }
+      tr.error {
+        //background-color: #ffe6e4;
+        color: #ee4728;
+      }
+      tr.error:hover td {
+        //background-color: #fecac6;
+      }
+      tr i {
+        display: block;
+        margin-bottom: -0.2em;
+        font-size: 1.4em !important;
+      }
     }
 
     div.import-errors {
@@ -156,15 +168,22 @@ table.import-settings {
   .header-description {
     padding-left: 1em;
   }
+
+  span.header-error {
+    font-size: 0.85em;
+    color: #da5354;
+  }
+
+  .select2-search {
+    display: none;
+  }
+
+  .select2-results {
+    margin: 0;
+  }
 }
 
-.select2-search {
-  display: none;
-}
 
-.select2-results {
-  margin: 0;
-}
 
 .post-save-results {
   p {
diff --git a/app/models/product_importer.rb b/app/models/product_importer.rb
index c9f56dcf91..ea53891a85 100644
--- a/app/models/product_importer.rb
+++ b/app/models/product_importer.rb
@@ -78,7 +78,12 @@ class ProductImporter
   end
 
   def suppliers_index
-    @suppliers_index || get_suppliers_index
+    index = @suppliers_index || get_suppliers_index
+    index.sort_by{ |k,v| v.to_i }.reverse.to_h
+  end
+
+  def all_entries
+    invalid_entries.merge(products_to_create).merge(products_to_update).sort.to_h
   end
 
   def invalid_entries
@@ -213,16 +218,19 @@ class ProductImporter
 
     if supplier_name.blank?
       mark_as_invalid(line_number, entry, "Supplier name field is empty")
+      entry['supplier_id'] = Enterprise.first.id # Removes a duplicate validation message TODO: proper solution
       return
     end
 
     unless supplier_exists?(supplier_name)
       mark_as_invalid(line_number, entry, "Supplier \"#{supplier_name}\" not found in database")
+      entry['supplier_id'] = Enterprise.first.id # Removes a duplicate validation message TODO: proper solution
       return
     end
 
     unless permission_by_name?(supplier_name)
       mark_as_invalid(line_number, entry, "You do not have permission to manage products for \"#{supplier_name}\"")
+      entry['supplier_id'] = Enterprise.first.id # Removes a duplicate validation message TODO: proper solution
       return
     end
 
@@ -239,7 +247,7 @@ class ProductImporter
 
     if category_name.blank?
       mark_as_invalid(line_number, entry, "Category field is empty")
-      entry['primary_taxon_id'] = Spree::Taxon.first.id # Removes a duplicate validation message
+      entry['primary_taxon_id'] = Spree::Taxon.first.id # Removes a duplicate validation message TODO: proper solution
       return
     end
 
diff --git a/app/views/admin/product_import/_entries_table.html.haml b/app/views/admin/product_import/_entries_table.html.haml
index 5385112398..80f89c2899 100644
--- a/app/views/admin/product_import/_entries_table.html.haml
+++ b/app/views/admin/product_import/_entries_table.html.haml
@@ -2,11 +2,14 @@
   %div.table-wrap
     %table
       %thead
+        %th
         %th Line
         - entries.values.first[:entry].each do |key, value|
           %th= key
       - entries.each do |line, item|
-        %tr
+        %tr{class: ('error' if item[:errors])}
+          %td
+            %i{class: (item[:errors] ? 'fa fa-warning warning' : 'fa fa-check-circle success')}
           %td= line
           - item[:entry].each do |key, value|
             %td= value
\ No newline at end of file
diff --git a/app/views/admin/product_import/_import_options.html.haml b/app/views/admin/product_import/_import_options.html.haml
index a87729a1c5..f5733c4f80 100644
--- a/app/views/admin/product_import/_import_options.html.haml
+++ b/app/views/admin/product_import/_import_options.html.haml
@@ -15,6 +15,34 @@
           = name
       %div.panel-content{ng: {hide: '!active'}}
         = render 'options_form', supplier_id: supplier_id, name: name
+  - elsif name and supplier_id
+    %div.panel-section.import-settings{ng: {controller: 'DropdownPanelsCtrl'}}
+      %div.panel-header
+        %div.header-caret
+          -#%i{ng: {class: "{'icon-chevron-down': active, 'icon-chevron-right': !active}"}}
+        %div.header-icon.error
+          %i.fa.fa-warning
+          -#%div.header-count
+          -#  %strong.invalid-count= @importer.invalid_count
+        %div.header-description
+          = name
+          %span.header-error= " - you do not have permission to manage this enterprise"
+      -#%div.panel-content{ng: {hide: '!active'}}
+      -#  = render 'options_form', supplier_id: supplier_id, name: name
+  - elsif name
+    %div.panel-section.import-settings{ng: {controller: 'DropdownPanelsCtrl'}}
+      %div.panel-header
+        %div.header-caret
+          -#%i{ng: {class: "{'icon-chevron-down': active, 'icon-chevron-right': !active}"}}
+        %div.header-icon.error
+          %i.fa.fa-warning
+          -#%div.header-count
+          -#  %strong.invalid-count= @importer.invalid_count
+        %div.header-description
+          = name
+          %span.header-error= " - enterprise could not be found in database"
+      -#%div.panel-content{ng: {hide: '!active'}}
+      -#  = 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 848e996db7..20cb48b769 100644
--- a/app/views/admin/product_import/_import_review.html.haml
+++ b/app/views/admin/product_import/_import_review.html.haml
@@ -13,30 +13,18 @@
 -#  -#%div.panel-content{ng: {hide: '!active'}}
 -#  -#  Content goes here
 
-%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
-        {{resetTotal}}
-    %div.header-description
-      Existing products will have their stock reset to zero
-  -#%div.panel-content{ng: {hide: '!active'}}
-  -#  Content goes here
-
-%div.panel-section
-  %div.panel-header
+%div.panel-section{ng: {controller: 'DropdownPanelsCtrl', init: "count = #{@importer.item_count}"}}
+  %div.panel-header{ng: {click: 'togglePanel()', class: '{active: active && count}'}}
     %div.header-caret
+      %i{ng: {class: "{'icon-chevron-down': active, 'icon-chevron-right': !active}", hide: 'count == 0'}}
     %div.header-icon.success
-      %i.fa.fa-check-circle
+      %i.fa.fa-info-circle.info
     %div.header-count
       %strong.item-count= @importer.item_count
     %div.header-description
       Entries found in imported file
-  -#%div.panel-content{ng: {hide: '!active'}}
-  -#  Content goes here
+  %div.panel-content{ng: {hide: '!active || count == 0'}}
+    = render 'entries_table', entries: @importer.all_entries
 
 %div.panel-section{ng: {controller: 'DropdownPanelsCtrl', init: "count = #{@importer.invalid_count}"}}
   %div.panel-header{ng: {click: 'togglePanel()', class: '{active: active && count}'}}
@@ -79,4 +67,17 @@
   %div.panel-content{ng: {hide: '!active || count == 0'}}
     = render 'entries_table', entries: @importer.products_to_update
 
+%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
+        {{resetTotal}}
+    %div.header-description
+      Existing products will have their stock reset to zero
+  -#%div.panel-content{ng: {hide: '!active'}}
+  -#  Content goes here
+
 %br.panels.clearfix
\ No newline at end of file

From f73fbe7f232b13a61efdc0a0d3ad26bd6af75ea2 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <visible_souls@hotmail.com>
Date: Sun, 12 Mar 2017 10:35:26 +0000
Subject: [PATCH 17/18] SpreadsheetEntry Class and PI refactor

---
 app/models/product_importer.rb                | 198 ++++++++----------
 app/models/spreadsheet_entry.rb               |  68 ++++++
 .../product_import/_entries_table.html.haml   |  14 +-
 .../product_import/_errors_list.html.haml     |  12 +-
 spec/features/admin/product_import_spec.rb    |   2 +-
 5 files changed, 175 insertions(+), 119 deletions(-)
 create mode 100644 app/models/spreadsheet_entry.rb

diff --git a/app/models/product_importer.rb b/app/models/product_importer.rb
index ea53891a85..58c0def237 100644
--- a/app/models/product_importer.rb
+++ b/app/models/product_importer.rb
@@ -11,6 +11,7 @@ class ProductImporter
     if file.is_a?(File)
       @file = file
       @sheet = open_spreadsheet
+      @entries = []
       @valid_entries = {}
       @invalid_entries = {}
 
@@ -26,12 +27,11 @@ class ProductImporter
       @editable_enterprises = {}
       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'
       @total_supplier_products = 0
       @products_to_reset = {}
       @updated_ids = []
 
-      validate_all if @sheet
+      init_product_importer if @sheet
     else
       self.errors.add(:importer, 'error: no file uploaded')
     end
@@ -78,7 +78,7 @@ class ProductImporter
   end
 
   def suppliers_index
-    index = @suppliers_index || get_suppliers_index
+    index = @suppliers_index || build_suppliers_index
     index.sort_by{ |k,v| v.to_i }.reverse.to_h
   end
 
@@ -87,27 +87,15 @@ class ProductImporter
   end
 
   def invalid_entries
-    entries = {}
-    @invalid_entries.each do |line_number, data|
-      entries[line_number] = {entry: data[:entry].except(*@non_display_attributes), errors: data[:errors]}
-    end
-    entries
+    @invalid_entries
   end
 
   def products_to_create
-    entries = {}
-    @products_to_create.merge(@variants_to_create).each do |line_number, data|
-      entries[line_number] = {entry: data[:entry].except(*@non_display_attributes)}
-    end
-    entries
+    @products_to_create.merge(@variants_to_create)
   end
 
   def products_to_update
-    entries = {}
-    @variants_to_update.each do |line_number, data|
-      entries[line_number] = {entry: data[:entry].except(*@non_display_attributes)}
-    end
-    entries
+    @variants_to_update
   end
 
   def products_created_count
@@ -141,6 +129,13 @@ class ProductImporter
 
   private
 
+  def init_product_importer
+    build_entries
+    build_categories_index
+    build_suppliers_index
+    validate_all
+  end
+
   def open_spreadsheet
     if accepted_mimetype
       Roo::Spreadsheet.open(@file, extension: accepted_mimetype)
@@ -166,25 +161,26 @@ class ProductImporter
     end
   end
 
-  def entries
-    rows.map do |row|
-      Hash[[headers, row].transpose]
+  def build_entries
+    rows.each_with_index do |row, i|
+      row_data = Hash[[headers, row].transpose]
+      entry = SpreadsheetEntry.new(row_data)
+      entry.line_number = i+2
+      @entries.push entry
     end
+    @entries
   end
 
   def validate_all
-    entries.each_with_index do |entry, i|
-      line_number = i+2 # Roo counts "line 2" as the first line of data
+    @entries.each do |entry|
+      supplier_validation(entry)
+      category_validation(entry)
+      set_update_status(entry)
 
-      supplier_validation(line_number, entry)
-      category_validation(line_number, entry)
-      set_update_status(line_number, entry)
-
-      mark_as_valid(line_number, entry) unless entry_invalid?(line_number)
+      mark_as_valid(entry) unless entry_invalid?(entry.line_number)
     end
 
     count_existing_products
-
     delete_uploaded_file if item_count.zero? or valid_count.zero?
   end
 
@@ -212,49 +208,43 @@ class ProductImporter
     !!@invalid_entries[line_number]
   end
 
-  def supplier_validation(line_number, entry)
-    suppliers_index = @suppliers_index || get_suppliers_index
-    supplier_name = entry['supplier']
+  def supplier_validation(entry)
+    supplier_name = entry.supplier
 
     if supplier_name.blank?
-      mark_as_invalid(line_number, entry, "Supplier name field is empty")
-      entry['supplier_id'] = Enterprise.first.id # Removes a duplicate validation message TODO: proper solution
+      mark_as_invalid(entry, attribute: "supplier", error: "can't be blank")
       return
     end
 
     unless supplier_exists?(supplier_name)
-      mark_as_invalid(line_number, entry, "Supplier \"#{supplier_name}\" not found in database")
-      entry['supplier_id'] = Enterprise.first.id # Removes a duplicate validation message TODO: proper solution
+      mark_as_invalid(entry, attribute: "supplier", error: "\"#{supplier_name}\" not found in database")
       return
     end
 
     unless permission_by_name?(supplier_name)
-      mark_as_invalid(line_number, entry, "You do not have permission to manage products for \"#{supplier_name}\"")
-      entry['supplier_id'] = Enterprise.first.id # Removes a duplicate validation message TODO: proper solution
+      mark_as_invalid(entry, attribute: "supplier", error: "\"#{supplier_name}\": you do not have permission to manage products for this enterprise")
       return
     end
 
-    entry['supplier_id'] = suppliers_index[supplier_name]
+    entry.supplier_id = @suppliers_index[supplier_name]
   end
 
   def supplier_exists?(supplier_name)
     @suppliers_index[supplier_name]
   end
 
-  def category_validation(line_number, entry)
-    categories_index = @categories_index || get_categories_index
-    category_name = entry['category']
+  def category_validation(entry)
+    category_name = entry.category
 
     if category_name.blank?
-      mark_as_invalid(line_number, entry, "Category field is empty")
-      entry['primary_taxon_id'] = Spree::Taxon.first.id # Removes a duplicate validation message TODO: proper solution
+      mark_as_invalid(entry, attribute: "category", error: "can't be blank")
       return
     end
 
     if category_exists?(category_name)
-      entry['primary_taxon_id'] = categories_index[category_name]
+      entry.primary_taxon_id = @categories_index[category_name]
     else
-      mark_as_invalid(line_number, entry, "Category \"#{category_name}\" not found in database")
+      mark_as_invalid(entry, attribute: "category", error: "\"#{category_name}\" not found in database")
     end
   end
 
@@ -262,26 +252,23 @@ class ProductImporter
     @categories_index[category_name]
   end
 
-  def mark_as_valid(line_number, entry)
-    @valid_entries[line_number] = {entry: entry}
+  def mark_as_valid(entry)
+    @valid_entries[entry.line_number] = entry
   end
 
-  def mark_as_invalid(line_number, entry, errors)
-    errors = [errors] if errors.is_a? String
+  def mark_as_invalid(entry, options={})
+    entry.errors.add(options[:attribute], options[:error]) if options[:attribute] and options[:error]
+    entry.product_validations = options[:product_validations] if options[:product_validations]
 
-    if entry_invalid?(line_number)
-      @invalid_entries[line_number][:errors] += errors
-    else
-      @invalid_entries[line_number] = {entry: entry, errors: errors}
-    end
+    @invalid_entries[entry.line_number] = entry
   end
 
   # Minimise db queries by getting a list of suppliers to look
   # up, instead of doing a query for each entry in the spreadsheet
-  def get_suppliers_index
-    @suppliers_index ||= {}
-    entries.each do |entry|
-      supplier_name = entry['supplier']
+  def build_suppliers_index
+    @suppliers_index = {}
+    @entries.each do |entry|
+      supplier_name = entry.supplier
       supplier_id = @suppliers_index[supplier_name] ||
           Enterprise.find_by_name(supplier_name, :select => 'id, name').try(:id)
       @suppliers_index[supplier_name] = supplier_id
@@ -289,10 +276,10 @@ class ProductImporter
     @suppliers_index
   end
 
-  def get_categories_index
-    @categories_index ||= {}
-    entries.each do |entry|
-      category_name = entry['category']
+  def build_categories_index
+    @categories_index = {}
+    @entries.each do |entry|
+      category_name = entry.category
       category_id = @categories_index[category_name] ||
           Spree::Taxon.find_by_name(category_name, :select => 'id, name').try(:id)
       @categories_index[category_name] = category_id
@@ -302,45 +289,44 @@ class ProductImporter
 
   def save_all_valid
     already_created = {}
-    @products_to_create.each do |line_number, data|
-      entry = data[:entry]
+    @products_to_create.each do |line_number, entry|
       # If we've already added a new product with these attributes
       # from this spreadsheet, mark this entry as a new variant with
       # the new product id, as this is a now variant of that product...
-      if already_created[entry['supplier_id']] and already_created[entry['supplier_id']][entry['name']]
-        product_id = already_created[entry['supplier_id']][entry['name']]
-        mark_as_new_variant(line_number, entry, product_id)
+      if already_created[entry.supplier_id] and already_created[entry.supplier_id][entry.name]
+        product_id = already_created[entry.supplier_id][entry.name]
+        mark_as_new_variant(entry, product_id)
         next
       end
 
       product = Spree::Product.new()
-      product.assign_attributes(entry.except('id'))
-      assign_defaults(product, entry)
+      product.assign_attributes(entry.attributes.except('id'))
+      assign_defaults(product, entry.attributes)
       if product.save
-        ensure_variant_updated(entry, product)
+        ensure_variant_updated(product, entry)
         @products_created += 1
         @updated_ids.push product.variants.first.id
       else
-        self.errors.add("Line #{line_number}:", product.errors.full_messages)
+        self.errors.add("Line #{line_number}:", product.errors.full_messages) #TODO: change
       end
 
-      already_created[entry['supplier_id']] = {entry['name'] => product.id}
+      already_created[entry.supplier_id] = {entry.name => product.id}
     end
 
-    @variants_to_update.each do |line_number, data|
-      variant = data[:variant]
-      assign_defaults(variant, data[:entry])
+    @variants_to_update.each do |line_number, entry|
+      variant = entry.product_object
+      assign_defaults(variant, entry.attributes)
       if variant.valid? and variant.save
         @variants_updated += 1
         @updated_ids.push variant.id
       else
-        self.errors.add("Line #{line_number}:", variant.errors.full_messages)
+        self.errors.add("Line #{line_number}:", variant.errors.full_messages) #TODO: change
       end
     end
 
-    @variants_to_create.each do |line_number, data|
-      new_variant = data[:variant]
-      assign_defaults(new_variant, data[:entry])
+    @variants_to_create.each do |line_number, entry|
+      new_variant = entry.product_object
+      assign_defaults(new_variant, entry.attributes)
       if new_variant.valid? and new_variant.save
         @variants_created += 1
         @updated_ids.push new_variant.id
@@ -388,67 +374,69 @@ class ProductImporter
     end
   end
 
-  def ensure_variant_updated(entry, product)
+  def ensure_variant_updated(product, entry)
     # Ensure display_name and on_demand are copied to new product's variant
-    if entry['display_name'] || entry['on_demand']
+    if entry.display_name || entry.on_demand
       variant = product.variants.first
-      variant.display_name = entry['display_name'] if entry['display_name']
-      variant.on_demand = entry['on_demand'] if entry['on_demand']
+      variant.display_name = entry.display_name if entry.display_name
+      variant.on_demand = entry.on_demand if entry.on_demand
       variant.save
     end
   end
 
-  def set_update_status(line_number, entry)
+  def set_update_status(entry)
     # Find product with matching supplier and name
-    match = Spree::Product.where(supplier_id: entry['supplier_id'], name: entry['name'], deleted_at: nil).first
+    match = Spree::Product.where(supplier_id: entry.supplier_id, name: entry.name, deleted_at: nil).first
 
     # If no matching product was found, create a new product
     if match.nil?
-      mark_as_new_product(line_number, entry)
+      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|
-      if existing_variant.display_name == entry['display_name'] && existing_variant.unit_value == Float(entry['unit_value'])
-        mark_as_existing_variant(line_number, entry, existing_variant)
+      if existing_variant.display_name == entry.display_name && existing_variant.unit_value == Float(entry.unit_value)
+        mark_as_existing_variant(entry, existing_variant)
         return
       end
     end
 
     # Otherwise, a variant with sufficiently matching attributes doesn't exist; create a new one
-    mark_as_new_variant(line_number, entry, match.id)
+    mark_as_new_variant(entry, match.id)
   end
 
-  def mark_as_new_product(line_number, entry)
+  def mark_as_new_product(entry)
     new_product = Spree::Product.new()
-    new_product.assign_attributes(entry.except('id'))
+    new_product.assign_attributes(entry.attributes.except('id'))
     if new_product.valid?
-      @products_to_create[line_number] = {entry: entry} unless entry_invalid?(line_number)
+      @products_to_create[entry.line_number] = entry unless entry_invalid?(entry.line_number)
     else
-      mark_as_invalid(line_number, entry, new_product.errors.full_messages)
+      mark_as_invalid(entry, product_validations: new_product.errors)
     end
   end
 
-  def mark_as_existing_variant(line_number, entry, existing_variant)
-    existing_variant.assign_attributes(entry.except('id', 'product_id'))
+  def mark_as_existing_variant(entry, existing_variant)
+    existing_variant.assign_attributes(entry.attributes.except('id', 'product_id'))
     check_on_hand_nil(entry, existing_variant)
     if existing_variant.valid?
-      @variants_to_update[line_number] = {entry: entry, variant: existing_variant} unless entry_invalid?(line_number)
-      updates_count_per_supplier(entry['supplier_id']) unless entry_invalid?(line_number)
+      entry.product_object = existing_variant
+      @variants_to_update[entry.line_number] = entry unless entry_invalid?(entry.line_number)
+      updates_count_per_supplier(entry.supplier_id) unless entry_invalid?(entry.line_number)
     else
-      mark_as_invalid(line_number, entry, existing_variant.errors.full_messages)
+      mark_as_invalid(entry, product_validations: existing_variant.errors)
     end
   end
 
-  def mark_as_new_variant(line_number, entry, product_id)
-    new_variant = Spree::Variant.new(entry.except('id', 'product_id'))
+  def mark_as_new_variant(entry, product_id)
+    new_variant = Spree::Variant.new(entry.attributes.except('id', 'product_id'))
     new_variant.product_id = product_id
     check_on_hand_nil(entry, new_variant)
     if new_variant.valid?
-      @variants_to_create[line_number] = {entry: entry, variant: new_variant} unless entry_invalid?(line_number)
+      entry.product_object = new_variant
+      @variants_to_create[entry.line_number] = entry unless entry_invalid?(entry.line_number)
     else
-      mark_as_invalid(line_number, entry, new_variant.errors.full_messages)
+      mark_as_invalid(entry, product_validations: new_variant.errors)
     end
   end
 
@@ -461,9 +449,9 @@ class ProductImporter
   end
 
   def check_on_hand_nil(entry, variant)
-    if entry['on_hand'].blank?
+    if entry.on_hand.blank?
       variant.on_hand = 0
-      entry['on_hand_nil'] = true
+      entry.on_hand_nil = true
     end
   end
 
diff --git a/app/models/spreadsheet_entry.rb b/app/models/spreadsheet_entry.rb
new file mode 100644
index 0000000000..1db6d43879
--- /dev/null
+++ b/app/models/spreadsheet_entry.rb
@@ -0,0 +1,68 @@
+# Class for defining spreadsheet entry objects for use in ProductImporter
+class SpreadsheetEntry
+  extend ActiveModel::Naming
+  include ActiveModel::Conversion
+  include ActiveModel::Validations
+
+  attr_accessor :line_number, :valid, :product_object, :product_validations, :save_type, :on_hand_nil
+
+  attr_accessor :id, :product_id, :supplier, :supplier_id, :name, :display_name, :sku,
+                :unit_value, :unit_description, :variant_unit, :variant_unit_scale, :variant_unit_name,
+                :display_as, :category, :primary_taxon_id, :price, :on_hand, :on_demand,
+                :tax_category_id, :shipping_category_id, :description
+
+  def initialize(attrs)
+    @product_validations = {}
+
+    attrs.each do |k, v|
+      if self.respond_to?("#{k}=")
+        send("#{k}=", v) unless non_product_attributes.include?(k)
+      else
+        # Trying to assign unknown attribute. Record this and give feedback or just ignore silently?
+      end
+    end
+  end
+
+  def persisted?
+    false #ActiveModel
+  end
+
+  def has_errors?
+    self.errors.count > 0 or @product_validations.count > 0
+  end
+
+  def attributes
+    attrs = {}
+    self.instance_variables.each do |var|
+      attrs[var.to_s.delete("@")] = self.instance_variable_get(var)
+    end
+    attrs.except(*non_product_attributes)
+  end
+
+  def displayable_attributes
+    # Modified attributes list for displaying in user feedback
+    attrs = {}
+    self.instance_variables.each do |var|
+      attrs[var.to_s.delete("@")] = self.instance_variable_get(var)
+    end
+    attrs.except(*non_product_attributes, *non_display_attributes)
+  end
+
+  def invalid_attributes
+    invalid_attrs = {}
+    @product_validations.messages.merge(self.errors.messages).each do |attr, message|
+      invalid_attrs[attr.to_s] = "#{attr.to_s.capitalize} #{message.first}"
+    end
+    invalid_attrs.except(*non_product_attributes, *non_display_attributes)
+  end
+
+  private
+
+  def non_display_attributes
+    ['id', 'product_id', 'variant_id', 'supplier_id', 'primary_taxon', 'primary_taxon_id', 'category_id', 'shipping_category_id', 'tax_category_id']
+  end
+
+  def non_product_attributes
+    ['line_number', 'valid', 'errors', 'product_object', 'product_validations', 'save_type', 'on_hand_nil']
+  end
+end
diff --git a/app/views/admin/product_import/_entries_table.html.haml b/app/views/admin/product_import/_entries_table.html.haml
index 80f89c2899..16c55a4fc3 100644
--- a/app/views/admin/product_import/_entries_table.html.haml
+++ b/app/views/admin/product_import/_entries_table.html.haml
@@ -1,15 +1,15 @@
-- if entries && entries.count > 0 #&& entries.values.first && entries.values.first[:entry]
+- if entries && entries.count > 0
   %div.table-wrap
     %table
       %thead
         %th
         %th Line
-        - entries.values.first[:entry].each do |key, value|
+        - entries.values.first.displayable_attributes.each do |key, value|
           %th= key
-      - entries.each do |line, item|
-        %tr{class: ('error' if item[:errors])}
+      - entries.each do |line_number, entry|
+        %tr{class: ('error' if entry.has_errors?)}
           %td
-            %i{class: (item[:errors] ? 'fa fa-warning warning' : 'fa fa-check-circle success')}
-          %td= line
-          - item[:entry].each do |key, value|
+            %i{class: (entry.has_errors? ? 'fa fa-warning warning' : 'fa fa-check-circle success')}
+          %td= line_number
+          - entry.displayable_attributes.each do |key, value|
             %td= value
\ No newline at end of file
diff --git a/app/views/admin/product_import/_errors_list.html.haml b/app/views/admin/product_import/_errors_list.html.haml
index 4974c978df..e7203a7ddd 100644
--- a/app/views/admin/product_import/_errors_list.html.haml
+++ b/app/views/admin/product_import/_errors_list.html.haml
@@ -1,11 +1,11 @@
-- @importer.invalid_entries.each do |line, item|
+- @importer.invalid_entries.each do |line_number, entry|
   %div.import-errors
     %p.line
       %strong
-        Item line #{line}:
-      %span= item[:entry]['name']
-      - if item[:entry]['display_name']
-        ( #{item[:entry]['display_name']} )
-    - item[:errors].each do |error|
+        Item line #{line_number}:
+      %span= entry.name
+      - if entry.display_name
+        ( #{entry.display_name} )
+    - entry.invalid_attributes.each do |attr, error|
       %p.error
         &nbsp;-&nbsp; #{error}
\ No newline at end of file
diff --git a/spec/features/admin/product_import_spec.rb b/spec/features/admin/product_import_spec.rb
index dbe204af33..5843330a7f 100644
--- a/spec/features/admin/product_import_spec.rb
+++ b/spec/features/admin/product_import_spec.rb
@@ -233,7 +233,7 @@ feature "Product Import", js: true do
       expect(page).to have_selector '.create-count', text: "1"
       expect(page).to have_selector '.update-count', text: "0"
 
-      expect(page.body).to have_content 'You do not have permission to manage products for "Another Enterprise"'
+      expect(page.body).to have_content 'you do not have permission'
 
       click_button 'Save'
 

From c62a044598eb37d392174267b3c4675157ac645a Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <visible_souls@hotmail.com>
Date: Fri, 10 Mar 2017 16:19:09 +0000
Subject: [PATCH 18/18] PI highlight invalid fields in feedback tables

---
 app/assets/stylesheets/admin/product_import.css.scss | 12 +++++++++---
 .../admin/product_import/_entries_table.html.haml    |  2 +-
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/app/assets/stylesheets/admin/product_import.css.scss b/app/assets/stylesheets/admin/product_import.css.scss
index 5d986f51dc..907e8b1a5c 100644
--- a/app/assets/stylesheets/admin/product_import.css.scss
+++ b/app/assets/stylesheets/admin/product_import.css.scss
@@ -88,16 +88,22 @@ div.panel-section {
       }
       tr.error {
         //background-color: #ffe6e4;
-        color: #ee4728;
+        //color: #ee4728;
+        color: #c84C4c;
       }
-      tr.error:hover td {
-        //background-color: #fecac6;
+      tr:hover td.invalid {
+        background-color: #ed5135;
       }
       tr i {
         display: block;
         margin-bottom: -0.2em;
         font-size: 1.4em !important;
       }
+      td.invalid {
+        background-color: #f05c51;
+        box-shadow: inset 0px 0px 1px red;
+        color: white;
+      }
     }
 
     div.import-errors {
diff --git a/app/views/admin/product_import/_entries_table.html.haml b/app/views/admin/product_import/_entries_table.html.haml
index 16c55a4fc3..cd293eea05 100644
--- a/app/views/admin/product_import/_entries_table.html.haml
+++ b/app/views/admin/product_import/_entries_table.html.haml
@@ -12,4 +12,4 @@
             %i{class: (entry.has_errors? ? 'fa fa-warning warning' : 'fa fa-check-circle success')}
           %td= line_number
           - entry.displayable_attributes.each do |key, value|
-            %td= value
\ No newline at end of file
+            %td{class: ('invalid' if entry.has_errors? and entry.invalid_attributes[key])}= value