From 2b8ebba233699e087ffce3cf0860fd0178b4db19 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Fri, 16 Aug 2019 11:03:41 +0100 Subject: [PATCH] Fix some rubocop issues in product_set and admin/products_controller --- .rubocop_manual_todo.yml | 5 -- .../admin/products_controller_decorator.rb | 72 ++++++++++++------- app/models/spree/product_set.rb | 36 ++++++---- 3 files changed, 68 insertions(+), 45 deletions(-) diff --git a/.rubocop_manual_todo.yml b/.rubocop_manual_todo.yml index 921fcf4049..7cd2a0a15b 100644 --- a/.rubocop_manual_todo.yml +++ b/.rubocop_manual_todo.yml @@ -49,7 +49,6 @@ Metrics/LineLength: - app/controllers/spree/admin/orders_controller_decorator.rb - app/controllers/spree/admin/payments_controller_decorator.rb - app/controllers/spree/admin/payment_methods_controller_decorator.rb - - app/controllers/spree/admin/products_controller_decorator.rb - app/controllers/spree/admin/reports_controller_decorator.rb - app/controllers/spree/api/products_controller_decorator.rb - app/controllers/spree/credit_cards_controller.rb @@ -437,7 +436,6 @@ Metrics/AbcSize: - app/models/spree/order_decorator.rb - app/models/spree/payment_decorator.rb - app/models/spree/product_decorator.rb - - app/models/spree/product_set.rb - app/models/spree/taxon_decorator.rb - app/serializers/api/admin/enterprise_serializer.rb - app/serializers/api/product_serializer.rb @@ -566,7 +564,6 @@ Metrics/CyclomaticComplexity: - app/models/spree/ability_decorator.rb - app/models/spree/payment_decorator.rb - app/models/spree/product_decorator.rb - - app/models/spree/product_set.rb - app/models/variant_override_set.rb - app/services/cart_service.rb - lib/discourse/single_sign_on.rb @@ -594,7 +591,6 @@ Metrics/PerceivedComplexity: - app/models/spree/ability_decorator.rb - app/models/spree/order_decorator.rb - app/models/spree/product_decorator.rb - - app/models/spree/product_set.rb - lib/discourse/single_sign_on.rb - lib/open_food_network/bulk_coop_report.rb - lib/open_food_network/enterprise_issue_validator.rb @@ -650,7 +646,6 @@ Metrics/MethodLength: - app/models/spree/payment_decorator.rb - app/models/spree/payment_method_decorator.rb - app/models/spree/product_decorator.rb - - app/models/spree/product_set.rb - app/serializers/api/admin/order_cycle_serializer.rb - app/serializers/api/cached_enterprise_serializer.rb - app/services/order_cycle_form.rb diff --git a/app/controllers/spree/admin/products_controller_decorator.rb b/app/controllers/spree/admin/products_controller_decorator.rb index 1697cc5dbc..0007c0c59e 100644 --- a/app/controllers/spree/admin/products_controller_decorator.rb +++ b/app/controllers/spree/admin/products_controller_decorator.rb @@ -48,19 +48,13 @@ Spree::Admin::ProductsController.class_eval do end def bulk_update - collection_hash = Hash[params[:products].each_with_index.map { |p, i| [i, p] }] - product_set = Spree::ProductSet.new(collection_attributes: collection_hash) - - params[:filters] ||= {} - bulk_index_query = params[:filters].reduce("") do |string, filter| - "#{string}q[#{filter[:property][:db_column]}_#{filter[:predicate][:predicate]}]=#{filter[:value]};" - end + product_set = product_set_from_params(params) # Ensure we're authorised to update all products product_set.collection.each { |p| authorize! :update, p } if product_set.save - redirect_to "/api/products/bulk_products?page=1;per_page=500;#{bulk_index_query}" + redirect_to "/api/products/bulk_products?page=1;per_page=500;#{bulk_index_query(params)}" else if product_set.errors.present? render json: { errors: product_set.errors }, status: :bad_request @@ -73,7 +67,8 @@ Spree::Admin::ProductsController.class_eval do protected def collection - # This method is copied directly from the spree product controller, except where we narrow the search below with the managed_by search to support + # This method is copied directly from the spree product controller + # except where we narrow the search below with the managed_by search to support # enterprise users. # TODO: There has to be a better way!!! return @collection if @collection.present? @@ -108,14 +103,38 @@ Spree::Admin::ProductsController.class_eval do private + def product_set_from_params(params) + collection_hash = Hash[params[:products].each_with_index.map { |p, i| [i, p] }] + Spree::ProductSet.new(collection_attributes: collection_hash) + end + + def bulk_index_query(params) + params[:filters] ||= {} + params[:filters].reduce("") do |string, filter| + filter_db_column = filter[:property][:db_column] + filter_predicate = filter[:predicate][:predicate] + filter_value = filter[:value] + "#{string}q[#{filter_db_column}_#{filter_predicate}]=#{filter_value};" + end + end + def load_form_data - @producers = OpenFoodNetwork::Permissions.new(spree_current_user).managed_product_enterprises.is_primary_producer.by_name + @producers = OpenFoodNetwork::Permissions.new(spree_current_user). + managed_product_enterprises.is_primary_producer.by_name @taxons = Spree::Taxon.order(:name) @import_dates = product_import_dates.uniq.to_json end def product_import_dates - import_dates = Spree::Variant. + options = [{ id: '0', name: '' }] + product_import_dates_query.collect(&:import_date). + map { |i| options.push(id: i.to_date, name: i.to_date.to_formatted_s(:long)) } + + options + end + + def product_import_dates_query + Spree::Variant. select('DISTINCT spree_variants.import_date'). joins(:product). where('spree_products.supplier_id IN (?)', editable_enterprises.collect(&:id)). @@ -123,18 +142,14 @@ Spree::Admin::ProductsController.class_eval do where(spree_variants: { is_master: false }). where(spree_variants: { deleted_at: nil }). order('spree_variants.import_date DESC') - - options = [{ id: '0', name: '' }] - import_dates.collect(&:import_date).map { |i| options.push(id: i.to_date, name: i.to_date.to_formatted_s(:long)) } - - options end def strip_new_properties - unless spree_current_user.admin? || params[:product][:product_properties_attributes].nil? - names = Spree::Property.pluck(:name) - params[:product][:product_properties_attributes].each do |key, property| - params[:product][:product_properties_attributes].delete key unless names.include? property[:property_name] + return if spree_current_user.admin? || params[:product][:product_properties_attributes].nil? + names = Spree::Property.pluck(:name) + params[:product][:product_properties_attributes].each do |key, property| + unless names.include? property[:property_name] + params[:product][:product_properties_attributes].delete key end end end @@ -155,17 +170,20 @@ Spree::Admin::ProductsController.class_eval do variant.on_demand = on_demand if on_demand.present? variant.on_hand = on_hand.to_i if on_hand.present? rescue StandardError => error - Bugsnag.notify(error) do |report| - report.add_tab(:product, product.attributes) - report.add_tab(:product_error, product.errors.first) unless product.valid? - report.add_tab(:variant_attributes, variant_attributes) - report.add_tab(:variant, variant.attributes) - report.add_tab(:variant_error, variant.errors.first) unless variant.valid? - end + notify_bugsnag(error, product, variant) raise error end end + def notify_bugsnag(error, product, variant) + Bugsnag.notify(error) do |report| + report.add_tab(:product, product.attributes) + report.add_tab(:product_error, product.errors.first) unless product.valid? + report.add_tab(:variant, variant.attributes) + report.add_tab(:variant_error, variant.errors.first) unless variant.valid? + end + end + def product_variant(product) if product.variants.any? product.variants.first diff --git a/app/models/spree/product_set.rb b/app/models/spree/product_set.rb index 2dee265466..f96a1afc2b 100644 --- a/app/models/spree/product_set.rb +++ b/app/models/spree/product_set.rb @@ -17,9 +17,7 @@ class Spree::ProductSet < ModelSet # variant.update_attributes( { price: xx.x } ) # def update_attributes(attributes) - if attributes[:taxon_ids].present? - attributes[:taxon_ids] = attributes[:taxon_ids].split(',') - end + split_taxon_ids!(attributes) found_model = @collection.find do |model| model.id.to_s == attributes[:id].to_s && model.persisted? @@ -28,12 +26,20 @@ class Spree::ProductSet < ModelSet if found_model.nil? @klass.new(attributes).save unless @reject_if.andand.call(attributes) else - update_product_only_attributes(found_model, attributes) && - update_product_variants(found_model, attributes) && - update_product_master(found_model, attributes) + update_product(found_model, attributes) end end + def split_taxon_ids!(attributes) + attributes[:taxon_ids] = attributes[:taxon_ids].split(',') if attributes[:taxon_ids].present? + end + + def update_product(found_model, attributes) + update_product_only_attributes(found_model, attributes) && + update_product_variants(found_model, attributes) && + update_product_master(found_model, attributes) + end + def update_product_only_attributes(product, attributes) variant_related_attrs = [:id, :variants_attributes, :master_attributes] product_related_attrs = attributes.except(*variant_related_attrs) @@ -94,13 +100,17 @@ class Spree::ProductSet < ModelSet variant.on_demand = on_demand if on_demand.present? variant.on_hand = on_hand.to_i if on_hand.present? rescue StandardError => error - Bugsnag.notify(error) do |report| - report.add_tab(:product, product.attributes) - report.add_tab(:product_error, product.errors.first) unless product.valid? - report.add_tab(:variant_attributes, variant_attributes) - report.add_tab(:variant, variant.attributes) - report.add_tab(:variant_error, variant.errors.first) unless variant.valid? - end + notify_bugsnag(error, product, variant, variant_attributes) + end + end + + def notify_bugsnag(error, product, variant, variant_attributes) + Bugsnag.notify(error) do |report| + report.add_tab(:product, product.attributes) + report.add_tab(:product_error, product.errors.first) unless product.valid? + report.add_tab(:variant_attributes, variant_attributes) + report.add_tab(:variant, variant.attributes) + report.add_tab(:variant_error, variant.errors.first) unless variant.valid? end end