Add errors when attempting to update non-updatable fields

app/assets/stylesheets/admin/product_import.css.scss

@@ -1,15 +1,22 @@
 @import "variables";
 
-div.panel-section {
+$pi-red: $warning-red;
+$pi-green: lighten($spree-green, 10%);
+$pi-orange: $bright-orange;
+$pi-blue: lighten($spree-blue, 10%);
+$pi-light-yellow: #faffaf;
+
+// scss-lint:disable NestingDepth
 
-  .neutral {
-    color: #bfbfbf;
+div.panel-section {
+  .error {
+    color: $pi-red;
   }
   .warning {
-    color: $warning-red;
+    color: $bright-orange;
   }
   .success {
-    color: #86d83a;
+    color: $pi-green;
   }
   .info {
     color: #68b7c0;
@@ -85,21 +92,29 @@ div.panel-section {
       td, th {
         white-space: nowrap;
       }
-      tr.error {
-        color: #c84C4c;
-      }
-      tr:hover td.invalid {
-        background-color: #ed5135;
-      }
-      tr i {
-        display: block;
-        margin-bottom: -0.2em;
-        font-size: 1.4em !important;
+
+      tr {
+        &.error {
+          color: #c84C4c;
+        }
+
+        &:hover td.invalid {
+          background-color: darken(#f05c51, 5%);
+        }
+
+        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;
+
+      td {
+        &.invalid {
+          background-color: #f05c51;
+          box-shadow: inset 0px 0px 1px red;
+          color: white;
+        }
       }
     }
 
@@ -123,52 +138,6 @@ br.panels.clearfix {
   clear: both;
 }
 
-table.import-settings {
-  background-color: transparent !important;
-  width: auto;
-
-  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: 0;
-  }
-  tr:last-child td {
-    border-bottom: 0;
-  }
-  div.select2-container {
-    width: 13.5em;
-  }
-  div.select2-container.select2-container-disabled {
-    a.select2-choice, span.select2-arrow {
-      background-color: #d5d5d5;
-    }
-  }
-  input[disabled], input:disabled {
-    background-color: #d5d5d5;
-    opacity: 1;
-    border-color: transparent;
-    color: white !important;
-  }
-}
-
 .panel-section.import-settings {
 
   .header-description {
@@ -177,7 +146,7 @@ table.import-settings {
 
   span.header-error {
     font-size: 0.85em;
-    color: $warning-red;
+    color: $pi-red;
   }
 
   .select2-search {
@@ -208,10 +177,10 @@ table.import-settings {
     }
 
     i.fa-check-circle {
-      color: #86d83a;
+      color: $pi-green;
     }
     i.fa-info-circle {
-      color: #68b7c0;
+      color: $pi-blue;
     }
   }
 
@@ -234,6 +203,10 @@ form.product-import, div.post-save-results, div.import-wrapper {
 
 div.import-wrapper {
 
+  .alert-box {
+    margin: 0 0 1.75em;
+  }
+
   .ng-hide:not(.ng-hide-animate) {
     // We have to use !important here to override angular's display properties
     // scss-lint:disable ImportantRule
@@ -281,7 +254,7 @@ div.progress-bar {
 
   span.progress-track{
     display: block;
-    background: #b7ea53;
+    background: lighten($pi-green, 10%);
     height: 100%;
     border-radius: 0.3em;
     box-shadow: inset 0 0 3px rgba(0,0,0,0.3);
@@ -312,7 +285,7 @@ div.progress-bar {
 
   span.category {
     display: inline-block;
-    background-color: lighten($spree-blue, 10%);
+    background-color: $pi-blue;
     color: white;
     padding: 0.3em 0.6em;
     margin: 0 0.4em 0.5em 0;

app/assets/stylesheets/admin/variables.css.scss

@@ -6,6 +6,7 @@ $spree-light-blue: #eff5fc;
 
 $warning-red: #da5354;
 $warning-orange: #da7f52;
+$bright-orange: #ffa92e;
 $medium-grey: #919191;
 $pale-blue: #cee1f4;
 

app/controllers/admin/product_import_controller.rb

@@ -14,6 +14,7 @@ def import
       @filepath = save_uploaded_file(params[:file])
       @importer = ProductImport::ProductImporter.new(File.new(@filepath), spree_current_user, params[:settings])
       @original_filename = params[:file].try(:original_filename)
+      @non_updatable_fields = ProductImport::EntryValidator.non_updatable_fields
 
       check_file_errors @importer
       check_spreadsheet_has_data @importer

app/models/product_import/entry_validator.rb

@@ -14,6 +14,16 @@ def initialize(current_user, import_time, spreadsheet_data, editable_enterprises
       @import_settings = import_settings
     end
 
+    def self.non_updatable_fields
+      {
+        category: :primary_taxon_id,
+        unit_type: :variant_unit_scale,
+        variant_unit_name: :variant_unit_name,
+        tax_category: :tax_category_id,
+        shipping_category: :shipping_category_id
+      }
+    end
+
     def validate_all(entries)
       entries.each do |entry|
         supplier_validation(entry)
@@ -177,23 +187,25 @@ def tax_and_shipping_validation(entry, type, category, index)
 
     def product_validation(entry)
       # Find product with matching supplier and name
-      match = Spree::Product.where(supplier_id: entry.supplier_id, name: entry.name, deleted_at: nil).first
+      existing_product = 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?
+      if existing_product.nil?
         mark_as_new_product(entry)
         return
       end
 
+      product_field_errors(entry, existing_product)
+
       # Otherwise, if a variant exists with matching display_name and unit_value, update it
-      match.variants.each do |existing_variant|
+      existing_product.variants.each do |existing_variant|
         if entry_matches_existing_variant?(entry, existing_variant) && existing_variant.deleted_at.nil?
           return mark_as_existing_variant(entry, existing_variant)
         end
       end
 
       # Otherwise, a variant with sufficiently matching attributes doesn't exist; create a new one
-      mark_as_new_variant(entry, match.id)
+      mark_as_new_variant(entry, existing_product.id)
     end
 
     def mark_as_new_product(entry)
@@ -220,6 +232,25 @@ def mark_as_existing_variant(entry, existing_variant)
       end
     end
 
+    def product_field_errors(entry, existing_product)
+      non_updatable_fields.each do |display_name, attribute|
+        next if attributes_match?(attribute, existing_product, entry) || attributes_blank?(attribute, existing_product, entry)
+        mark_as_invalid(entry, attribute: display_name, error: I18n.t('admin.product_import.model.not_updatable'))
+      end
+    end
+
+    def non_updatable_fields
+      EntryValidator.non_updatable_fields
+    end
+
+    def attributes_match?(attribute, existing_product, entry)
+      existing_product.send(attribute) == entry.send(attribute)
+    end
+
+    def attributes_blank?(attribute, existing_product, entry)
+      existing_product.send(attribute).blank? && entry.send(attribute).blank?
+    end
+
     def permission_by_name?(supplier_name)
       @editable_enterprises.key?(supplier_name)
     end

app/models/product_import/product_importer.rb

@@ -57,6 +57,13 @@ def item_count
       @sheet ? @sheet.last_row - 1 : 0
     end
 
+    def product_field_errors?
+      @entries.each do |entry|
+        return true if entry.errors.messages.values.include? [I18n.t('admin.product_import.model.not_updatable')]
+      end
+      false
+    end
+
     def reset_counts
       # Return indexed data about existing product count, reset count, and updates count per supplier
       @reset_counts.each do |supplier_id, values|

app/views/admin/product_import/_entries_table.html.haml

@@ -7,7 +7,7 @@
         %th= heading
     %tr{ng: {repeat: "(line_number, entry) in (entries | entriesFilterValid:'#{entries}')"}}
       %td
-        %i{ng: {class: "{'fa fa-warning warning': (count(entry.errors) > 0), 'fa fa-check-circle success': (count(entry.errors) == 0)}"}}
+        %i{ng: {class: "{'fa fa-warning error': (count(entry.errors) > 0), 'fa fa-check-circle success': (count(entry.errors) == 0)}"}}
       %td
         {{line_number}}
       %td{ng: {repeat: "(attribute, value) in entry.attributes", class: "{'invalid': attribute_invalid(attribute, line_number)}"}}

app/views/admin/product_import/_import_review.html.haml

@@ -3,6 +3,12 @@
 
 %div{ng: {controller: 'ImportFeedbackCtrl'}}
 
+  - if @importer.product_field_errors?
+    .alert-box.warning
+      = t('.not_updatable_tip')
+      %em= @non_updatable_fields.keys.join(', ') + "."
+      = t('.fields_ignored')
+
   %div.panel-section{ng: {controller: 'DropdownPanelsCtrl'}}
     %div.panel-header{ng: {click: 'togglePanel()', class: '{active: active && count((entries | entriesFilterValid:"all"))}'}}
       %div.header-caret
@@ -21,7 +27,7 @@
     %div.panel-header{ng: {click: 'togglePanel()', class: '{active: active && count((entries | entriesFilterValid:"invalid"))}'}}
       %div.header-caret
         %i{ng: {class: "{'icon-chevron-down': active, 'icon-chevron-right': !active}", hide: 'count((entries | entriesFilterValid:"invalid")) == 0'}}
-      %div.header-icon.warning
+      %div.header-icon.error
         %i.fa.fa-warning
       %div.header-count
         %strong.invalid-count

config/locales/en.yml

@@ -489,6 +489,7 @@ en:
         conditional_blank: can't be blank if unit_type is blank
         no_product: did not match any products in the database
         not_found: not found in database
+        not_updatable: cannot be updated on existing products via product import
         blank: can't be blank
         products_no_permission: you do not have permission to manage products for this enterprise
         inventory_no_permission: you do not have permission to create inventory for this producer
@@ -547,6 +548,11 @@ en:
         inventory_to_reset: Existing inventory items will have their stock reset to zero
         line: Line
         item_line: Item line
+      import_review:
+        not_updatable_tip: "The following fields cannot be updated via bulk import for existing products:"
+        fields_ignored: These fields will be ignored when the imported products are saved.
+      entries_table:
+        not_updatable: This field is not updatable via bulk import on existing products
       save_results:
         final_results: Import final results
         products_created: Products created

spec/models/product_importer_spec.rb

@@ -16,18 +16,19 @@
 
   let!(:category) { create(:taxon, name: 'Vegetables') }
   let!(:category2) { create(:taxon, name: 'Cake') }
+  let!(:category3) { create(:taxon, name: 'Meat') }
   let!(:tax_category) { create(:tax_category) }
   let!(:tax_category2) { create(:tax_category) }
   let!(:shipping_category) { create(:shipping_category) }
 
-  let!(:product) { create(:simple_product, supplier: enterprise2, name: 'Hypothetical Cake') }
+  let!(:product) { create(:simple_product, supplier: enterprise2, name: 'Hypothetical Cake', primary_taxon_id: category2.id) }
   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', unit_value: '500') }
-  let!(:product3) { create(:simple_product, supplier: enterprise, on_hand: '100', name: 'Sprouts', unit_value: '500') }
-  let!(:product4) { create(:simple_product, supplier: enterprise, on_hand: '100', name: 'Cabbage', unit_value: '500') }
-  let!(:product5) { create(:simple_product, supplier: enterprise2, on_hand: '100', name: 'Lettuce', unit_value: '500') }
-  let!(:product6) { create(:simple_product, supplier: enterprise3, on_hand: '100', name: 'Beetroot', unit_value: '500', on_demand: true, variant_unit_scale: 1, variant_unit: 'weight') }
-  let!(:product7) { create(:simple_product, supplier: enterprise3, on_hand: '100', name: 'Tomato', unit_value: '500', variant_unit_scale: 1, variant_unit: 'weight') }
+  let!(:product2) { create(:simple_product, supplier: enterprise, on_hand: '100', name: 'Beans', unit_value: '500', primary_taxon_id: category.id) }
+  let!(:product3) { create(:simple_product, supplier: enterprise, on_hand: '100', name: 'Sprouts', unit_value: '500', primary_taxon_id: category.id) }
+  let!(:product4) { create(:simple_product, supplier: enterprise, on_hand: '100', name: 'Cabbage', unit_value: '500', primary_taxon_id: category.id) }
+  let!(:product5) { create(:simple_product, supplier: enterprise2, on_hand: '100', name: 'Lettuce', unit_value: '500', primary_taxon_id: category.id) }
+  let!(:product6) { create(:simple_product, supplier: enterprise3, on_hand: '100', name: 'Beetroot', unit_value: '500', on_demand: true, variant_unit_scale: 1, variant_unit: 'weight', primary_taxon_id: category.id) }
+  let!(:product7) { create(:simple_product, supplier: enterprise3, on_hand: '100', name: 'Tomato', unit_value: '500', variant_unit_scale: 1, variant_unit: 'weight', primary_taxon_id: category.id) }
   let!(:variant_override) { create(:variant_override, variant_id: product4.variants.first.id, hub: enterprise2, count_on_hand: 42) }
   let!(:variant_override2) { create(:variant_override, variant_id: product5.variants.first.id, hub: enterprise, count_on_hand: 96) }
 
@@ -203,6 +204,7 @@
 
     it "validates entries" do
       @importer.validate_entries
+
       entries = JSON.parse(@importer.entries_json)
 
       expect(filter('valid', entries)).to eq 2
@@ -317,6 +319,33 @@
     end
   end
 
+  describe "updating non-updatable fields on existing products" do
+    before do
+      csv_data = CSV.generate do |csv|
+        csv << ["name", "supplier", "category", "on_hand", "price", "units", "unit_type"]
+        csv << ["Beetroot", "And Another Enterprise", "Meat", "5", "3.50", "500", "g"]
+        csv << ["Tomato", "And Another Enterprise", "Vegetables", "6", "5.50", "500", "Kg"]
+      end
+      File.write('/tmp/test-m.csv', csv_data)
+      file = File.new('/tmp/test-m.csv')
+      settings = {enterprise3.id.to_s => {'import_into' => 'product_list'}}
+      @importer = ProductImport::ProductImporter.new(file, admin, start: 1, end: 100, settings: settings)
+    end
+    after { File.delete('/tmp/test-m.csv') }
+
+    it "does not allow updating" do
+      @importer.validate_entries
+      entries = JSON.parse(@importer.entries_json)
+
+      expect(filter('valid', entries)).to eq 0
+      expect(filter('invalid', entries)).to eq 2
+
+      @importer.all_entries.each do |entry|
+        expect(entry.errors.messages.values).to include [I18n.t('admin.product_import.model.not_updatable')]
+      end
+    end
+  end
+
   describe "importing items into inventory" do
     before do
       csv_data = CSV.generate do |csv|