From f73fbe7f232b13a61efdc0a0d3ad26bd6af75ea2 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley Date: Sun, 12 Mar 2017 10:35:26 +0000 Subject: [PATCH] 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  -  #{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'