diff --git a/app/assets/stylesheets/admin/product_import.css.scss b/app/assets/stylesheets/admin/product_import.css.scss index d8ad019af9a4..20dcc481f1ae 100644 --- a/app/assets/stylesheets/admin/product_import.css.scss +++ b/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; diff --git a/app/assets/stylesheets/admin/variables.css.scss b/app/assets/stylesheets/admin/variables.css.scss index c7f904a38a36..86ddaaf8c92f 100644 --- a/app/assets/stylesheets/admin/variables.css.scss +++ b/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; diff --git a/app/controllers/admin/product_import_controller.rb b/app/controllers/admin/product_import_controller.rb index 38ea79a97db4..eb07ccf9f93a 100644 --- a/app/controllers/admin/product_import_controller.rb +++ b/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 diff --git a/app/models/product_import/entry_validator.rb b/app/models/product_import/entry_validator.rb index 4f48c7745c3f..54dde4578adc 100644 --- a/app/models/product_import/entry_validator.rb +++ b/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 diff --git a/app/models/product_import/product_importer.rb b/app/models/product_import/product_importer.rb index 99362b8c1b4a..6cabcc9e2f87 100644 --- a/app/models/product_import/product_importer.rb +++ b/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| diff --git a/app/views/admin/product_import/_entries_table.html.haml b/app/views/admin/product_import/_entries_table.html.haml index 8ad958b1c2fc..6b0dcfdf6ed1 100644 --- a/app/views/admin/product_import/_entries_table.html.haml +++ b/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)}"}} diff --git a/app/views/admin/product_import/_import_review.html.haml b/app/views/admin/product_import/_import_review.html.haml index 9015c31bb8f8..90d8de9577a0 100644 --- a/app/views/admin/product_import/_import_review.html.haml +++ b/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 diff --git a/config/locales/en.yml b/config/locales/en.yml index 540d306a9538..b01a94b13a05 100644 --- a/config/locales/en.yml +++ b/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 diff --git a/spec/models/product_importer_spec.rb b/spec/models/product_importer_spec.rb index c0cbc6718f91..beb1b78915c3 100644 --- a/spec/models/product_importer_spec.rb +++ b/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|