From 8e65d29b023e165c816b96a2e8a06ff936e6a87d Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Sat, 31 Oct 2020 21:12:24 +0000 Subject: [PATCH 1/7] Move sets to specific services namespace --- app/{models => services/sets}/column_preference_set.rb | 0 app/{models => services/sets}/enterprise_fee_set.rb | 0 app/{models => services/sets}/enterprise_set.rb | 0 app/{models => services/sets}/model_set.rb | 0 app/{models => services/sets}/order_cycle_set.rb | 0 app/{models/spree => services/sets}/product_set.rb | 0 app/{models => services/sets}/variant_override_set.rb | 0 spec/{models => services/sets}/model_set_spec.rb | 0 spec/{models/spree => services/sets}/product_set_spec.rb | 0 9 files changed, 0 insertions(+), 0 deletions(-) rename app/{models => services/sets}/column_preference_set.rb (100%) rename app/{models => services/sets}/enterprise_fee_set.rb (100%) rename app/{models => services/sets}/enterprise_set.rb (100%) rename app/{models => services/sets}/model_set.rb (100%) rename app/{models => services/sets}/order_cycle_set.rb (100%) rename app/{models/spree => services/sets}/product_set.rb (100%) rename app/{models => services/sets}/variant_override_set.rb (100%) rename spec/{models => services/sets}/model_set_spec.rb (100%) rename spec/{models/spree => services/sets}/product_set_spec.rb (100%) diff --git a/app/models/column_preference_set.rb b/app/services/sets/column_preference_set.rb similarity index 100% rename from app/models/column_preference_set.rb rename to app/services/sets/column_preference_set.rb diff --git a/app/models/enterprise_fee_set.rb b/app/services/sets/enterprise_fee_set.rb similarity index 100% rename from app/models/enterprise_fee_set.rb rename to app/services/sets/enterprise_fee_set.rb diff --git a/app/models/enterprise_set.rb b/app/services/sets/enterprise_set.rb similarity index 100% rename from app/models/enterprise_set.rb rename to app/services/sets/enterprise_set.rb diff --git a/app/models/model_set.rb b/app/services/sets/model_set.rb similarity index 100% rename from app/models/model_set.rb rename to app/services/sets/model_set.rb diff --git a/app/models/order_cycle_set.rb b/app/services/sets/order_cycle_set.rb similarity index 100% rename from app/models/order_cycle_set.rb rename to app/services/sets/order_cycle_set.rb diff --git a/app/models/spree/product_set.rb b/app/services/sets/product_set.rb similarity index 100% rename from app/models/spree/product_set.rb rename to app/services/sets/product_set.rb diff --git a/app/models/variant_override_set.rb b/app/services/sets/variant_override_set.rb similarity index 100% rename from app/models/variant_override_set.rb rename to app/services/sets/variant_override_set.rb diff --git a/spec/models/model_set_spec.rb b/spec/services/sets/model_set_spec.rb similarity index 100% rename from spec/models/model_set_spec.rb rename to spec/services/sets/model_set_spec.rb diff --git a/spec/models/spree/product_set_spec.rb b/spec/services/sets/product_set_spec.rb similarity index 100% rename from spec/models/spree/product_set_spec.rb rename to spec/services/sets/product_set_spec.rb From 187b4a1fc2852634a741d4f4a379240ac3ba2965 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Sat, 31 Oct 2020 21:16:14 +0000 Subject: [PATCH 2/7] Add Sets namespace to each set file --- app/services/sets/column_preference_set.rb | 8 +- app/services/sets/enterprise_fee_set.rb | 12 +- app/services/sets/enterprise_set.rb | 8 +- app/services/sets/model_set.rb | 102 ++++----- app/services/sets/order_cycle_set.rb | 8 +- app/services/sets/product_set.rb | 248 +++++++++++---------- app/services/sets/variant_override_set.rb | 50 +++-- 7 files changed, 225 insertions(+), 211 deletions(-) diff --git a/app/services/sets/column_preference_set.rb b/app/services/sets/column_preference_set.rb index 24508d7f9d..006ca9afe8 100644 --- a/app/services/sets/column_preference_set.rb +++ b/app/services/sets/column_preference_set.rb @@ -1,5 +1,7 @@ -class ColumnPreferenceSet < ModelSet - def initialize(collection, attributes = {}) - super(ColumnPreference, collection, attributes, nil, nil ) +module Sets + class ColumnPreferenceSet < ModelSet + def initialize(collection, attributes = {}) + super(ColumnPreference, collection, attributes, nil, nil ) + end end end diff --git a/app/services/sets/enterprise_fee_set.rb b/app/services/sets/enterprise_fee_set.rb index ae7ff692ad..9fdb64432c 100644 --- a/app/services/sets/enterprise_fee_set.rb +++ b/app/services/sets/enterprise_fee_set.rb @@ -1,7 +1,9 @@ -class EnterpriseFeeSet < ModelSet - def initialize(attributes = {}) - super(EnterpriseFee, EnterpriseFee.all, - attributes, - proc { |attrs| attrs[:name].blank? }) +module Sets + class EnterpriseFeeSet < ModelSet + def initialize(attributes = {}) + super(EnterpriseFee, EnterpriseFee.all, + attributes, + proc { |attrs| attrs[:name].blank? }) + end end end diff --git a/app/services/sets/enterprise_set.rb b/app/services/sets/enterprise_set.rb index 20da64b083..b482da0875 100644 --- a/app/services/sets/enterprise_set.rb +++ b/app/services/sets/enterprise_set.rb @@ -1,5 +1,7 @@ -class EnterpriseSet < ModelSet - def initialize(collection, attributes = {}) - super(Enterprise, collection, attributes) +module Sets + class EnterpriseSet < ModelSet + def initialize(collection, attributes = {}) + super(Enterprise, collection, attributes) + end end end diff --git a/app/services/sets/model_set.rb b/app/services/sets/model_set.rb index 8a599b4214..d1ac510d52 100644 --- a/app/services/sets/model_set.rb +++ b/app/services/sets/model_set.rb @@ -1,65 +1,67 @@ # Tableless model to handle updating multiple models at once from a single form -class ModelSet - include ActiveModel::Conversion - extend ActiveModel::Naming +module Sets + class ModelSet + include ActiveModel::Conversion + extend ActiveModel::Naming - attr_accessor :collection + attr_accessor :collection - def initialize(klass, collection, attributes = {}, reject_if = nil, delete_if = nil) - @klass, @collection, @reject_if, @delete_if = klass, collection, reject_if, delete_if + def initialize(klass, collection, attributes = {}, reject_if = nil, delete_if = nil) + @klass, @collection, @reject_if, @delete_if = klass, collection, reject_if, delete_if - # Set here first, to ensure that we apply collection_attributes to the right collection - @collection = attributes[:collection] if attributes[:collection] + # Set here first, to ensure that we apply collection_attributes to the right collection + @collection = attributes[:collection] if attributes[:collection] - attributes.each do |name, value| - public_send("#{name}=", value) - end - end - - def collection_attributes=(collection_attributes) - collection_attributes.each do |_k, attributes| - # attributes == {:id => 123, :next_collection_at => '...'} - found_element = @collection.detect do |element| - element.id.to_s == attributes[:id].to_s && !element.id.nil? - end - - if found_element.nil? - @collection << @klass.new(attributes) unless @reject_if.andand.call(attributes) - else - found_element.assign_attributes(attributes.except(:id)) + attributes.each do |name, value| + public_send("#{name}=", value) end end - end - def errors - errors = ActiveModel::Errors.new self - full_messages = @collection - .map { |model| model.errors.full_messages } - .flatten + def collection_attributes=(collection_attributes) + collection_attributes.each do |_k, attributes| + # attributes == {:id => 123, :next_collection_at => '...'} + found_element = @collection.detect do |element| + element.id.to_s == attributes[:id].to_s && !element.id.nil? + end - full_messages.each { |message| errors.add(:base, message) } - errors - end + if found_element.nil? + @collection << @klass.new(attributes) unless @reject_if.andand.call(attributes) + else + found_element.assign_attributes(attributes.except(:id)) + end + end + end - def save - collection_to_delete.each(&:destroy) - collection_to_keep.all?(&:save) - end + def errors + errors = ActiveModel::Errors.new self + full_messages = @collection + .map { |model| model.errors.full_messages } + .flatten - def collection_to_delete - # Remove all elements to be deleted from collection and return them - # Allows us to render @model_set.collection without deleted elements - deleted = [] - @collection = collection.to_a - collection.delete_if { |e| deleted << e if @delete_if.andand.call(e.attributes) } - deleted - end + full_messages.each { |message| errors.add(:base, message) } + errors + end - def collection_to_keep - collection.reject { |e| @delete_if.andand.call(e.attributes) } - end + def save + collection_to_delete.each(&:destroy) + collection_to_keep.all?(&:save) + end - def persisted? - false + def collection_to_delete + # Remove all elements to be deleted from collection and return them + # Allows us to render @model_set.collection without deleted elements + deleted = [] + @collection = collection.to_a + collection.delete_if { |e| deleted << e if @delete_if.andand.call(e.attributes) } + deleted + end + + def collection_to_keep + collection.reject { |e| @delete_if.andand.call(e.attributes) } + end + + def persisted? + false + end end end diff --git a/app/services/sets/order_cycle_set.rb b/app/services/sets/order_cycle_set.rb index 9f4fb4433e..6459cbf0b3 100644 --- a/app/services/sets/order_cycle_set.rb +++ b/app/services/sets/order_cycle_set.rb @@ -1,5 +1,7 @@ -class OrderCycleSet < ModelSet - def initialize(collection, attributes = {}) - super(OrderCycle, collection, attributes) +module Sets + class OrderCycleSet < ModelSet + def initialize(collection, attributes = {}) + super(OrderCycle, collection, attributes) + end end end diff --git a/app/services/sets/product_set.rb b/app/services/sets/product_set.rb index 4731c44bf7..dc53224d9b 100644 --- a/app/services/sets/product_set.rb +++ b/app/services/sets/product_set.rb @@ -1,138 +1,140 @@ -class Spree::ProductSet < ModelSet - def initialize(attributes = {}) - super(Spree::Product, [], attributes) - end - - def save - @collection_hash.each_value.all? do |product_attributes| - update_product_attributes(product_attributes) +module Sets + class Spree::ProductSet < ModelSet + def initialize(attributes = {}) + super(Spree::Product, [], attributes) end - end - def collection_attributes=(attributes) - @collection = Spree::Product - .where(id: attributes.each_value.map { |product| product[:id] }) - @collection_hash = attributes - end - - private - - # A separate method of updating products was required due to an issue with - # the way Rails' assign_attributes and update behave when - # delegated attributes of a nested object are updated via the parent object - # (ie. price of variants). Updating such attributes by themselves did not - # work using: - # - # product.update(variants_attributes: [{ id: y, price: xx.x }]) - # - # and so an explicit call to update on each individual variant was - # required. ie: - # - # variant.update( { price: xx.x } ) - # - def update_product_attributes(attributes) - split_taxon_ids!(attributes) - - product = find_model(@collection, attributes[:id]) - return if product.nil? - - update_product(product, attributes) - end - - def split_taxon_ids!(attributes) - attributes[:taxon_ids] = attributes[:taxon_ids].split(',') if attributes[:taxon_ids].present? - end - - def update_product(product, attributes) - original_supplier = product.supplier_id - return false unless update_product_only_attributes(product, attributes) - - ExchangeVariantDeleter.new.delete(product) if original_supplier != product.supplier_id - - update_product_variants(product, attributes) && - update_product_master(product, 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) - return true if product_related_attrs.blank? - - product.assign_attributes(product_related_attrs) - - validate_presence_of_unit_value_in_product(product) - - product.errors.empty? && product.save - end - - def validate_presence_of_unit_value_in_product(product) - product.variants.each do |variant| - validate_presence_of_unit_value_in_variant(product, variant) + def save + @collection_hash.each_value.all? do |product_attributes| + update_product_attributes(product_attributes) + end end - end - def validate_presence_of_unit_value_in_variant(product, variant) - return unless %w(weight volume).include?(product.variant_unit) - return if variant.unit_value.present? - - product.errors.add(:unit_value, "can't be blank") - end - - def update_product_variants(product, attributes) - return true unless attributes[:variants_attributes] - - update_variants_attributes(product, attributes[:variants_attributes]) - end - - def update_product_master(product, attributes) - return true unless attributes[:master_attributes] - - create_or_update_variant(product, attributes[:master_attributes]) - end - - def update_variants_attributes(product, variants_attributes) - variants_attributes.each do |attributes| - create_or_update_variant(product, attributes) + def collection_attributes=(attributes) + @collection = Spree::Product + .where(id: attributes.each_value.map { |product| product[:id] }) + @collection_hash = attributes end - end - def create_or_update_variant(product, variant_attributes) - variant = find_model(product.variants_including_master, variant_attributes[:id]) - if variant.present? - variant.update(variant_attributes.except(:id)) - else - create_variant(product, variant_attributes) + private + + # A separate method of updating products was required due to an issue with + # the way Rails' assign_attributes and update behave when + # delegated attributes of a nested object are updated via the parent object + # (ie. price of variants). Updating such attributes by themselves did not + # work using: + # + # product.update(variants_attributes: [{ id: y, price: xx.x }]) + # + # and so an explicit call to update on each individual variant was + # required. ie: + # + # variant.update( { price: xx.x } ) + # + def update_product_attributes(attributes) + split_taxon_ids!(attributes) + + product = find_model(@collection, attributes[:id]) + return if product.nil? + + update_product(product, attributes) end - end - def create_variant(product, variant_attributes) - on_hand = variant_attributes.delete(:on_hand) - on_demand = variant_attributes.delete(:on_demand) - - variant = product.variants.create(variant_attributes) - - begin - variant.on_demand = on_demand if on_demand.present? - variant.on_hand = on_hand.to_i if on_hand.present? - rescue StandardError => e - notify_bugsnag(e, product, variant, variant_attributes) - raise e + def split_taxon_ids!(attributes) + attributes[:taxon_ids] = attributes[:taxon_ids].split(',') if attributes[:taxon_ids].present? 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? + def update_product(product, attributes) + original_supplier = product.supplier_id + return false unless update_product_only_attributes(product, attributes) + + ExchangeVariantDeleter.new.delete(product) if original_supplier != product.supplier_id + + update_product_variants(product, attributes) && + update_product_master(product, attributes) end - end - def find_model(collection, model_id) - collection.find do |model| - model.id.to_s == model_id.to_s && model.persisted? + def update_product_only_attributes(product, attributes) + variant_related_attrs = [:id, :variants_attributes, :master_attributes] + product_related_attrs = attributes.except(*variant_related_attrs) + return true if product_related_attrs.blank? + + product.assign_attributes(product_related_attrs) + + validate_presence_of_unit_value_in_product(product) + + product.errors.empty? && product.save + end + + def validate_presence_of_unit_value_in_product(product) + product.variants.each do |variant| + validate_presence_of_unit_value_in_variant(product, variant) + end + end + + def validate_presence_of_unit_value_in_variant(product, variant) + return unless %w(weight volume).include?(product.variant_unit) + return if variant.unit_value.present? + + product.errors.add(:unit_value, "can't be blank") + end + + def update_product_variants(product, attributes) + return true unless attributes[:variants_attributes] + + update_variants_attributes(product, attributes[:variants_attributes]) + end + + def update_product_master(product, attributes) + return true unless attributes[:master_attributes] + + create_or_update_variant(product, attributes[:master_attributes]) + end + + def update_variants_attributes(product, variants_attributes) + variants_attributes.each do |attributes| + create_or_update_variant(product, attributes) + end + end + + def create_or_update_variant(product, variant_attributes) + variant = find_model(product.variants_including_master, variant_attributes[:id]) + if variant.present? + variant.update(variant_attributes.except(:id)) + else + create_variant(product, variant_attributes) + end + end + + def create_variant(product, variant_attributes) + on_hand = variant_attributes.delete(:on_hand) + on_demand = variant_attributes.delete(:on_demand) + + variant = product.variants.create(variant_attributes) + + begin + variant.on_demand = on_demand if on_demand.present? + variant.on_hand = on_hand.to_i if on_hand.present? + rescue StandardError => e + notify_bugsnag(e, product, variant, variant_attributes) + raise e + 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 + + def find_model(collection, model_id) + collection.find do |model| + model.id.to_s == model_id.to_s && model.persisted? + end end end end diff --git a/app/services/sets/variant_override_set.rb b/app/services/sets/variant_override_set.rb index d380df4144..b16d9b9e4c 100644 --- a/app/services/sets/variant_override_set.rb +++ b/app/services/sets/variant_override_set.rb @@ -1,30 +1,32 @@ -class VariantOverrideSet < ModelSet - def initialize(collection, attributes = {}) - super(VariantOverride, collection, attributes, nil, proc { |attrs, tag_list| deletable?(attrs, tag_list) } ) - end +module Sets + class VariantOverrideSet < ModelSet + def initialize(collection, attributes = {}) + super(VariantOverride, collection, attributes, nil, proc { |attrs, tag_list| deletable?(attrs, tag_list) } ) + end - private + private - def deletable?(attrs, tag_list) - attrs['price'].blank? && - attrs['count_on_hand'].blank? && - attrs['default_stock'].blank? && - attrs['resettable'].blank? && - attrs['sku'].nil? && - attrs['on_demand'].nil? && - tag_list.empty? - end + def deletable?(attrs, tag_list) + attrs['price'].blank? && + attrs['count_on_hand'].blank? && + attrs['default_stock'].blank? && + attrs['resettable'].blank? && + attrs['sku'].nil? && + attrs['on_demand'].nil? && + tag_list.empty? + end - # Override of ModelSet method to allow us to check presence of a tag_list (which is not an attribute) - # This method will delete VariantOverrides that have no values (see deletable? above) - # If the user sets all values to nil in the UI the VO will be deleted from the DB - def collection_to_delete - deleted = [] - collection.delete_if { |e| deleted << e if @delete_if.andand.call(e.attributes, e.tag_list) } - deleted - end + # Override of ModelSet method to allow us to check presence of a tag_list (which is not an attribute) + # This method will delete VariantOverrides that have no values (see deletable? above) + # If the user sets all values to nil in the UI the VO will be deleted from the DB + def collection_to_delete + deleted = [] + collection.delete_if { |e| deleted << e if @delete_if.andand.call(e.attributes, e.tag_list) } + deleted + end - def collection_to_keep - collection.reject { |e| @delete_if.andand.call(e.attributes, e.tag_list) } + def collection_to_keep + collection.reject { |e| @delete_if.andand.call(e.attributes, e.tag_list) } + end end end From 96a351ad0e293d1ecfe1cdef42a79bebf455be91 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Sat, 31 Oct 2020 21:20:35 +0000 Subject: [PATCH 3/7] Adapt usage of Sets to their new location --- app/controllers/admin/column_preferences_controller.rb | 2 +- app/controllers/admin/enterprise_fees_controller.rb | 4 ++-- app/controllers/admin/enterprises_controller.rb | 4 ++-- app/controllers/admin/order_cycles_controller.rb | 2 +- app/controllers/admin/variant_overrides_controller.rb | 2 +- app/controllers/spree/admin/products_controller.rb | 2 +- app/services/sets/product_set.rb | 2 +- .../enterprise_fees/_calculator_settings.html.haml | 2 +- spec/controllers/admin/enterprises_controller_spec.rb | 2 +- spec/services/sets/model_set_spec.rb | 10 +++++----- spec/services/sets/product_set_spec.rb | 2 +- 11 files changed, 17 insertions(+), 17 deletions(-) diff --git a/app/controllers/admin/column_preferences_controller.rb b/app/controllers/admin/column_preferences_controller.rb index d8a5ca3e6a..90ba1bd37e 100644 --- a/app/controllers/admin/column_preferences_controller.rb +++ b/app/controllers/admin/column_preferences_controller.rb @@ -29,7 +29,7 @@ module Admin collection_hash = Hash[permitted_params[:column_preferences]. each_with_index.map { |cp, i| [i, cp] }] collection_hash.select!{ |_i, cp| cp[:action_name] == permitted_params[:action_name] } - @cp_set = ColumnPreferenceSet.new @column_preferences, collection_attributes: collection_hash + @cp_set = Sets::ColumnPreferenceSet.new @column_preferences, collection_attributes: collection_hash end def collection diff --git a/app/controllers/admin/enterprise_fees_controller.rb b/app/controllers/admin/enterprise_fees_controller.rb index 0aaa2957be..9d1536856e 100644 --- a/app/controllers/admin/enterprise_fees_controller.rb +++ b/app/controllers/admin/enterprise_fees_controller.rb @@ -27,7 +27,7 @@ module Admin end def bulk_update - @enterprise_fee_set = EnterpriseFeeSet.new(enterprise_fee_bulk_params) + @enterprise_fee_set = Sets::EnterpriseFeeSet.new(enterprise_fee_bulk_params) if @enterprise_fee_set.save redirect_to redirect_path, notice: I18n.t(:enterprise_fees_update_notice) @@ -40,7 +40,7 @@ module Admin private def load_enterprise_fee_set - @enterprise_fee_set = EnterpriseFeeSet.new collection: collection + @enterprise_fee_set = Sets::EnterpriseFeeSet.new collection: collection end def load_data diff --git a/app/controllers/admin/enterprises_controller.rb b/app/controllers/admin/enterprises_controller.rb index c61ab9f30e..b62bb9ddf5 100644 --- a/app/controllers/admin/enterprises_controller.rb +++ b/app/controllers/admin/enterprises_controller.rb @@ -81,7 +81,7 @@ module Admin end def bulk_update - @enterprise_set = EnterpriseSet.new(collection, bulk_params) + @enterprise_set = Sets::EnterpriseSet.new(collection, bulk_params) if @enterprise_set.save flash[:success] = I18n.t(:enterprise_bulk_update_success_notice) @@ -129,7 +129,7 @@ module Admin private def load_enterprise_set - @enterprise_set = EnterpriseSet.new(collection) if spree_current_user.admin? + @enterprise_set = Sets::EnterpriseSet.new(collection) if spree_current_user.admin? end def load_countries diff --git a/app/controllers/admin/order_cycles_controller.rb b/app/controllers/admin/order_cycles_controller.rb index f7de30662a..3482be399f 100644 --- a/app/controllers/admin/order_cycles_controller.rb +++ b/app/controllers/admin/order_cycles_controller.rb @@ -223,7 +223,7 @@ module Admin end def order_cycle_set - @order_cycle_set ||= OrderCycleSet.new(@order_cycles, order_cycle_bulk_params) + @order_cycle_set ||= Sets::OrderCycleSet.new(@order_cycles, order_cycle_bulk_params) end def require_order_cycle_set_params diff --git a/app/controllers/admin/variant_overrides_controller.rb b/app/controllers/admin/variant_overrides_controller.rb index 333455c57b..c4fd99a766 100644 --- a/app/controllers/admin/variant_overrides_controller.rb +++ b/app/controllers/admin/variant_overrides_controller.rb @@ -67,7 +67,7 @@ module Admin def load_collection collection_hash = Hash[variant_overrides_params.each_with_index.map { |vo, i| [i, vo] }] - @vo_set = VariantOverrideSet.new @variant_overrides, collection_attributes: collection_hash + @vo_set = Sets::VariantOverrideSet.new @variant_overrides, collection_attributes: collection_hash end def collection diff --git a/app/controllers/spree/admin/products_controller.rb b/app/controllers/spree/admin/products_controller.rb index 3f9cdd92ac..1cc3f5ae6d 100644 --- a/app/controllers/spree/admin/products_controller.rb +++ b/app/controllers/spree/admin/products_controller.rb @@ -163,7 +163,7 @@ module Spree def product_set_from_params(_params) collection_hash = Hash[products_params.each_with_index.map { |p, i| [i, p] }] - Spree::ProductSet.new(collection_attributes: collection_hash) + Sets::ProductSet.new(collection_attributes: collection_hash) end def products_params diff --git a/app/services/sets/product_set.rb b/app/services/sets/product_set.rb index dc53224d9b..b6054e7125 100644 --- a/app/services/sets/product_set.rb +++ b/app/services/sets/product_set.rb @@ -1,5 +1,5 @@ module Sets - class Spree::ProductSet < ModelSet + class ProductSet < ModelSet def initialize(attributes = {}) super(Spree::Product, [], attributes) end diff --git a/app/views/admin/enterprise_fees/_calculator_settings.html.haml b/app/views/admin/enterprise_fees/_calculator_settings.html.haml index f505f0c38a..4936ee35b2 100644 --- a/app/views/admin/enterprise_fees/_calculator_settings.html.haml +++ b/app/views/admin/enterprise_fees/_calculator_settings.html.haml @@ -1,5 +1,5 @@ -# Render only the calculator settings and not the surrounding form -- enterprise_fee_set = ModelSet.new(EnterpriseFee, EnterpriseFee.where(:id => enterprise_fee.id)) +- enterprise_fee_set = Sets::ModelSet.new(EnterpriseFee, EnterpriseFee.where(:id => enterprise_fee.id)) - calculator_form_string = nil - form_for enterprise_fee_set, :as => :enterprise_fee_set, :url => '' do |form| - form.fields_for :collection do |f| diff --git a/spec/controllers/admin/enterprises_controller_spec.rb b/spec/controllers/admin/enterprises_controller_spec.rb index 3b4cd1428c..8343ca0d9e 100644 --- a/spec/controllers/admin/enterprises_controller_spec.rb +++ b/spec/controllers/admin/enterprises_controller_spec.rb @@ -397,7 +397,7 @@ describe Admin::EnterprisesController, type: :controller do end it "cuts down the list of enterprises displayed when error received on bulk update" do - allow_any_instance_of(EnterpriseSet).to receive(:save) { false } + allow_any_instance_of(Sets::EnterpriseSet).to receive(:save) { false } profile_enterprise1.enterprise_roles.build(user: new_owner).save allow(controller).to receive_messages spree_current_user: new_owner bulk_enterprise_params = { enterprise_set: { collection_attributes: { '0' => { id: profile_enterprise1.id, visible: 'false' } } } } diff --git a/spec/services/sets/model_set_spec.rb b/spec/services/sets/model_set_spec.rb index 2dd5e12c8e..00b71e5d19 100644 --- a/spec/services/sets/model_set_spec.rb +++ b/spec/services/sets/model_set_spec.rb @@ -2,13 +2,13 @@ require 'spec_helper' -describe ModelSet do +describe Sets::ModelSet do describe "updating" do it "creates new models" do attrs = { collection_attributes: { '1' => { name: 's1' }, '2' => { name: 's2' } } } - ms = ModelSet.new(EnterpriseRelationshipPermission, EnterpriseRelationshipPermission.all, attrs) + ms = Sets::ModelSet.new(EnterpriseRelationshipPermission, EnterpriseRelationshipPermission.all, attrs) expect { ms.save }.to change(EnterpriseRelationshipPermission, :count).by(2) @@ -22,7 +22,7 @@ describe ModelSet do attrs = { collection_attributes: { '1' => { id: e1.id, name: 'e1zz', description: 'foo' }, '2' => { id: e2.id, name: 'e2yy', description: 'bar' } } } - ms = ModelSet.new(EnterpriseGroup, EnterpriseGroup.all, attrs) + ms = Sets::ModelSet.new(EnterpriseGroup, EnterpriseGroup.all, attrs) expect { ms.save }.to change(EnterpriseGroup, :count).by(0) @@ -36,7 +36,7 @@ describe ModelSet do attributes = { collection_attributes: { '1' => { id: e1.id, name: 'deleteme' }, '2' => { id: e2.id, name: 'e2' } } } - ms = ModelSet.new(Enterprise, Enterprise.all, attributes, nil, + ms = Sets::ModelSet.new(Enterprise, Enterprise.all, attributes, nil, proc { |attrs| attrs['name'] == 'deleteme' }) expect { ms.save }.to change(Enterprise, :count).by(-1) @@ -48,7 +48,7 @@ describe ModelSet do it "ignores deletable new records" do attributes = { collection_attributes: { '1' => { name: 'deleteme' } } } - ms = ModelSet.new(Enterprise, Enterprise.all, attributes, nil, + ms = Sets::ModelSet.new(Enterprise, Enterprise.all, attributes, nil, proc { |attrs| attrs['name'] == 'deleteme' }) expect { ms.save }.to change(Enterprise, :count).by(0) diff --git a/spec/services/sets/product_set_spec.rb b/spec/services/sets/product_set_spec.rb index c296ed7a73..a5576d9caa 100644 --- a/spec/services/sets/product_set_spec.rb +++ b/spec/services/sets/product_set_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -describe Spree::ProductSet do +describe Sets::ProductSet do describe '#save' do context 'when passing :collection_attributes' do let(:product_set) do From 5d6d7f7ad07ca306a256d0f8d57832e2eb379fbf Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Sun, 1 Nov 2020 12:01:03 +0000 Subject: [PATCH 4/7] Adapt enterprise fees code and specs to new namespace of Sets::EnterpriseFeeSet --- .../admin/enterprise_fees_controller.rb | 2 +- app/helpers/enterprise_fees_helper.rb | 4 +- .../_calculator_settings.html.haml | 2 +- spec/features/admin/enterprise_fees_spec.rb | 60 +++++++++---------- 4 files changed, 34 insertions(+), 34 deletions(-) diff --git a/app/controllers/admin/enterprise_fees_controller.rb b/app/controllers/admin/enterprise_fees_controller.rb index 9d1536856e..73880badf4 100644 --- a/app/controllers/admin/enterprise_fees_controller.rb +++ b/app/controllers/admin/enterprise_fees_controller.rb @@ -80,7 +80,7 @@ module Admin end def enterprise_fee_bulk_params - params.require(:enterprise_fee_set).permit( + params.require(:sets_enterprise_fee_set).permit( collection_attributes: [ :id, :enterprise_id, :fee_type, :name, :tax_category_id, :inherits_tax_category, :calculator_type, diff --git a/app/helpers/enterprise_fees_helper.rb b/app/helpers/enterprise_fees_helper.rb index cc22f07326..55647b6413 100644 --- a/app/helpers/enterprise_fees_helper.rb +++ b/app/helpers/enterprise_fees_helper.rb @@ -1,10 +1,10 @@ module EnterpriseFeesHelper def angular_name(method) - "enterprise_fee_set[collection_attributes][{{ $index }}][#{method}]" + "sets_enterprise_fee_set[collection_attributes][{{ $index }}][#{method}]" end def angular_id(method) - "enterprise_fee_set_collection_attributes_{{ $index }}_#{method}" + "sets_enterprise_fee_set_collection_attributes_{{ $index }}_#{method}" end def enterprise_fee_type_options diff --git a/app/views/admin/enterprise_fees/_calculator_settings.html.haml b/app/views/admin/enterprise_fees/_calculator_settings.html.haml index 4936ee35b2..be05330d6c 100644 --- a/app/views/admin/enterprise_fees/_calculator_settings.html.haml +++ b/app/views/admin/enterprise_fees/_calculator_settings.html.haml @@ -1,7 +1,7 @@ -# Render only the calculator settings and not the surrounding form - enterprise_fee_set = Sets::ModelSet.new(EnterpriseFee, EnterpriseFee.where(:id => enterprise_fee.id)) - calculator_form_string = nil -- form_for enterprise_fee_set, :as => :enterprise_fee_set, :url => '' do |form| +- form_for enterprise_fee_set, :as => :sets_enterprise_fee_set, :url => '' do |form| - form.fields_for :collection do |f| - calculator_form_string = capture do - if !enterprise_fee.new_record? diff --git a/spec/features/admin/enterprise_fees_spec.rb b/spec/features/admin/enterprise_fees_spec.rb index 862a338f15..057840525b 100644 --- a/spec/features/admin/enterprise_fees_spec.rb +++ b/spec/features/admin/enterprise_fees_spec.rb @@ -18,11 +18,11 @@ feature ' login_as_admin_and_visit spree.edit_admin_general_settings_path click_link 'Enterprise Fees' - expect(page).to have_select "enterprise_fee_set_collection_attributes_0_enterprise_id" - expect(page).to have_select "enterprise_fee_set_collection_attributes_0_fee_type", selected: 'Packing fee' + expect(page).to have_select "sets_enterprise_fee_set_collection_attributes_0_enterprise_id" + expect(page).to have_select "sets_enterprise_fee_set_collection_attributes_0_fee_type", selected: 'Packing fee' expect(page).to have_selector "input[value='$0.50 / kg']" - expect(page).to have_select "enterprise_fee_set_collection_attributes_0_tax_category_id", selected: 'GST' - expect(page).to have_select "enterprise_fee_set_collection_attributes_0_calculator_type", selected: 'Flat Rate (per item)' + expect(page).to have_select "sets_enterprise_fee_set_collection_attributes_0_tax_category_id", selected: 'GST' + expect(page).to have_select "sets_enterprise_fee_set_collection_attributes_0_calculator_type", selected: 'Flat Rate (per item)' expect(page).to have_selector "input[value='#{amount}']" end @@ -34,11 +34,11 @@ feature ' login_as_admin_and_visit admin_enterprise_fees_path # And I fill in the fields for a new enterprise fee and click update - select 'Feedme', from: 'enterprise_fee_set_collection_attributes_0_enterprise_id' - select 'Admin', from: 'enterprise_fee_set_collection_attributes_0_fee_type' - fill_in 'enterprise_fee_set_collection_attributes_0_name', with: 'Hello!' - select 'GST', from: 'enterprise_fee_set_collection_attributes_0_tax_category_id' - select 'Flat Percent', from: 'enterprise_fee_set_collection_attributes_0_calculator_type' + select 'Feedme', from: 'sets_enterprise_fee_set_collection_attributes_0_enterprise_id' + select 'Admin', from: 'sets_enterprise_fee_set_collection_attributes_0_fee_type' + fill_in 'sets_enterprise_fee_set_collection_attributes_0_name', with: 'Hello!' + select 'GST', from: 'sets_enterprise_fee_set_collection_attributes_0_tax_category_id' + select 'Flat Percent', from: 'sets_enterprise_fee_set_collection_attributes_0_calculator_type' click_button 'Update' # Then I should see my fee and fields for the calculator @@ -46,11 +46,11 @@ feature ' expect(page).to have_selector "input[value='Hello!']" # When I fill in the calculator fields and click update - fill_in 'enterprise_fee_set_collection_attributes_0_calculator_attributes_preferred_flat_percent', with: '12.34' + fill_in 'sets_enterprise_fee_set_collection_attributes_0_calculator_attributes_preferred_flat_percent', with: '12.34' click_button 'Update' # Then I should see the correct values in my calculator fields - expect(page).to have_selector "#enterprise_fee_set_collection_attributes_0_calculator_attributes_preferred_flat_percent[value='12.34']" + expect(page).to have_selector "#sets_enterprise_fee_set_collection_attributes_0_calculator_attributes_preferred_flat_percent[value='12.34']" end scenario "editing an enterprise fee" do @@ -62,18 +62,18 @@ feature ' login_as_admin_and_visit admin_enterprise_fees_path # And I update the fields for the enterprise fee and click update - select 'Foo', from: 'enterprise_fee_set_collection_attributes_0_enterprise_id' - select 'Admin', from: 'enterprise_fee_set_collection_attributes_0_fee_type' - fill_in 'enterprise_fee_set_collection_attributes_0_name', with: 'Greetings!' - select 'Inherit From Product', from: 'enterprise_fee_set_collection_attributes_0_tax_category_id' - select 'Flat Percent', from: 'enterprise_fee_set_collection_attributes_0_calculator_type' + select 'Foo', from: 'sets_enterprise_fee_set_collection_attributes_0_enterprise_id' + select 'Admin', from: 'sets_enterprise_fee_set_collection_attributes_0_fee_type' + fill_in 'sets_enterprise_fee_set_collection_attributes_0_name', with: 'Greetings!' + select 'Inherit From Product', from: 'sets_enterprise_fee_set_collection_attributes_0_tax_category_id' + select 'Flat Percent', from: 'sets_enterprise_fee_set_collection_attributes_0_calculator_type' click_button 'Update' # Then I should see the updated fields for my fee - expect(page).to have_select "enterprise_fee_set_collection_attributes_0_enterprise_id", selected: 'Foo' - expect(page).to have_select "enterprise_fee_set_collection_attributes_0_fee_type", selected: 'Admin fee' + expect(page).to have_select "sets_enterprise_fee_set_collection_attributes_0_enterprise_id", selected: 'Foo' + expect(page).to have_select "sets_enterprise_fee_set_collection_attributes_0_fee_type", selected: 'Admin fee' expect(page).to have_selector "input[value='Greetings!']" - expect(page).to have_select 'enterprise_fee_set_collection_attributes_0_tax_category_id', selected: 'Inherit From Product' + expect(page).to have_select 'sets_enterprise_fee_set_collection_attributes_0_tax_category_id', selected: 'Inherit From Product' expect(page).to have_selector "option[selected]", text: 'Flat Percent (per item)' fee.reload @@ -123,17 +123,17 @@ feature ' within(".side_menu") { click_link 'Enterprise Fees' } click_link "Create One Now" - select distributor1.name, from: 'enterprise_fee_set_collection_attributes_0_enterprise_id' - select 'Packing', from: 'enterprise_fee_set_collection_attributes_0_fee_type' - fill_in 'enterprise_fee_set_collection_attributes_0_name', with: 'foo' - select 'GST', from: 'enterprise_fee_set_collection_attributes_0_tax_category_id' - select 'Flat Percent', from: 'enterprise_fee_set_collection_attributes_0_calculator_type' + select distributor1.name, from: 'sets_enterprise_fee_set_collection_attributes_0_enterprise_id' + select 'Packing', from: 'sets_enterprise_fee_set_collection_attributes_0_fee_type' + fill_in 'sets_enterprise_fee_set_collection_attributes_0_name', with: 'foo' + select 'GST', from: 'sets_enterprise_fee_set_collection_attributes_0_tax_category_id' + select 'Flat Percent', from: 'sets_enterprise_fee_set_collection_attributes_0_calculator_type' click_button 'Update' expect(flash_message).to eq('Your enterprise fees have been updated.') # After saving, we should be redirected to the fees for our chosen enterprise - expect(page).not_to have_select 'enterprise_fee_set_collection_attributes_1_enterprise_id', selected: 'Second Distributor' + expect(page).not_to have_select 'sets_enterprise_fee_set_collection_attributes_1_enterprise_id', selected: 'Second Distributor' enterprise_fee = EnterpriseFee.find_by name: 'foo' expect(enterprise_fee.enterprise).to eq(distributor1) @@ -146,14 +146,14 @@ feature ' visit edit_admin_enterprise_path(distributor1) within(".side_menu") { click_link 'Enterprise Fees' } click_link "Manage Enterprise Fees" - expect(page).to have_field 'enterprise_fee_set_collection_attributes_0_name', with: 'One' - expect(page).not_to have_field 'enterprise_fee_set_collection_attributes_1_name', with: 'Two' + expect(page).to have_field 'sets_enterprise_fee_set_collection_attributes_0_name', with: 'One' + expect(page).not_to have_field 'sets_enterprise_fee_set_collection_attributes_1_name', with: 'Two' visit edit_admin_enterprise_path(distributor2) within(".side_menu") { click_link 'Enterprise Fees' } click_link "Manage Enterprise Fees" - expect(page).not_to have_field 'enterprise_fee_set_collection_attributes_0_name', with: 'One' - expect(page).to have_field 'enterprise_fee_set_collection_attributes_0_name', with: 'Two' + expect(page).not_to have_field 'sets_enterprise_fee_set_collection_attributes_0_name', with: 'One' + expect(page).to have_field 'sets_enterprise_fee_set_collection_attributes_0_name', with: 'Two' end it "only allows me to select enterprises I have access to" do @@ -164,7 +164,7 @@ feature ' visit edit_admin_enterprise_path(distributor2) within(".side_menu") { click_link 'Enterprise Fees' } click_link "Manage Enterprise Fees" - expect(page).to have_select('enterprise_fee_set_collection_attributes_0_enterprise_id', + expect(page).to have_select('sets_enterprise_fee_set_collection_attributes_0_enterprise_id', selected: 'Second Distributor', options: ['', 'First Distributor', 'Second Distributor']) end From 19b12092a0692d8115f439995480e4ce130e927a Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Sun, 1 Nov 2020 12:13:32 +0000 Subject: [PATCH 5/7] Fix rubocop issues and adapt exceptions file --- .rubocop_manual_todo.yml | 6 ++---- .rubocop_todo.yml | 8 -------- app/services/sets/column_preference_set.rb | 2 ++ app/services/sets/enterprise_fee_set.rb | 2 ++ app/services/sets/enterprise_set.rb | 2 ++ app/services/sets/model_set.rb | 2 ++ app/services/sets/order_cycle_set.rb | 2 ++ app/services/sets/product_set.rb | 2 ++ app/services/sets/variant_override_set.rb | 10 ++++++++-- spec/services/sets/model_set_spec.rb | 8 +++++--- spec/services/sets/product_set_spec.rb | 6 +++++- 11 files changed, 32 insertions(+), 18 deletions(-) diff --git a/.rubocop_manual_todo.yml b/.rubocop_manual_todo.yml index cc08ddc7e0..67d1f8a600 100644 --- a/.rubocop_manual_todo.yml +++ b/.rubocop_manual_todo.yml @@ -71,7 +71,6 @@ Layout/LineLength: - app/models/spree/variant.rb - app/models/subscription.rb - app/models/variant_override.rb - - app/models/variant_override_set.rb - app/serializers/api/admin/subscription_line_item_serializer.rb - app/services/cart_service.rb - app/services/checkout/post_checkout_actions.rb @@ -254,7 +253,6 @@ Layout/LineLength: - spec/models/enterprise_relationship_spec.rb - spec/models/enterprise_spec.rb - spec/models/exchange_spec.rb - - spec/models/model_set_spec.rb - spec/models/order_cycle_spec.rb - spec/models/product_importer_spec.rb - spec/models/product_import/reset_absent_spec.rb @@ -385,7 +383,6 @@ Metrics/AbcSize: - app/models/enterprise_group.rb - app/models/enterprise.rb - app/models/enterprise_relationship.rb - - app/models/model_set.rb - app/models/product_import/entry_processor.rb - app/models/product_import/entry_validator.rb - app/models/product_import/product_importer.rb @@ -413,6 +410,7 @@ Metrics/AbcSize: - app/serializers/api/variant_serializer.rb - app/services/cart_service.rb - app/services/create_order_cycle.rb + - app/services/sets/model_set.rb - app/services/order_cycle_form.rb - app/services/order_syncer.rb - app/services/variant_units/option_value_namer.rb @@ -538,8 +536,8 @@ Metrics/CyclomaticComplexity: - app/models/spree/product.rb - app/models/spree/return_authorization.rb - app/models/spree/zone.rb - - app/models/variant_override_set.rb - app/services/cart_service.rb + - app/services/sets/variant_override_set.rb - engines/order_management/app/services/order_management/reports/bulk_coop/bulk_coop_report.rb - engines/order_management/app/services/order_management/stock/estimator.rb - lib/active_merchant/billing/gateways/stripe_payment_intents.rb diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 2713d9b86d..95485b26e2 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -398,7 +398,6 @@ Style/ClassAndModuleChildren: - 'app/models/calculator/flat_percent_per_item.rb' - 'app/models/spree/gateway/migs.rb' - 'app/models/spree/gateway/pin.rb' - - 'app/models/spree/product_set.rb' - 'app/models/tag_rule/discount_order.rb' - 'app/models/tag_rule/filter_order_cycles.rb' - 'app/models/tag_rule/filter_payment_methods.rb' @@ -575,7 +574,6 @@ Style/FrozenStringLiteralComment: - 'app/models/calculator/flat_percent_per_item.rb' - 'app/models/calculator/weight.rb' - 'app/models/column_preference.rb' - - 'app/models/column_preference_set.rb' - 'app/models/concerns/address_display.rb' - 'app/models/concerns/adjustment_scopes.rb' - 'app/models/concerns/line_item_based_adjustment_handling.rb' @@ -588,17 +586,13 @@ Style/FrozenStringLiteralComment: - 'app/models/customer.rb' - 'app/models/distributor_shipping_method.rb' - 'app/models/enterprise_fee.rb' - - 'app/models/enterprise_fee_set.rb' - 'app/models/enterprise_relationship_permission.rb' - 'app/models/enterprise_role.rb' - - 'app/models/enterprise_set.rb' - 'app/models/exchange.rb' - 'app/models/exchange_fee.rb' - 'app/models/exchange_variant.rb' - 'app/models/inventory_item.rb' - - 'app/models/model_set.rb' - 'app/models/order_cycle.rb' - - 'app/models/order_cycle_set.rb' - 'app/models/preference_sections/footer_and_external_links_section.rb' - 'app/models/preference_sections/group_signup_page_section.rb' - 'app/models/preference_sections/header_section.rb' @@ -623,7 +617,6 @@ Style/FrozenStringLiteralComment: - 'app/models/spree/gateway/pin.rb' - 'app/models/spree/gateway/stripe_connect.rb' - 'app/models/spree/preferences/file_configuration.rb' - - 'app/models/spree/product_set.rb' - 'app/models/spree/property.rb' - 'app/models/spree/user.rb' - 'app/models/stripe_account.rb' @@ -636,7 +629,6 @@ Style/FrozenStringLiteralComment: - 'app/models/tag_rule/filter_products.rb' - 'app/models/tag_rule/filter_shipping_methods.rb' - 'app/models/variant_override.rb' - - 'app/models/variant_override_set.rb' - 'app/serializers/api/address_serializer.rb' - 'app/serializers/api/adjustment_serializer.rb' - 'app/serializers/api/cached_enterprise_serializer.rb' diff --git a/app/services/sets/column_preference_set.rb b/app/services/sets/column_preference_set.rb index 006ca9afe8..13cba40957 100644 --- a/app/services/sets/column_preference_set.rb +++ b/app/services/sets/column_preference_set.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Sets class ColumnPreferenceSet < ModelSet def initialize(collection, attributes = {}) diff --git a/app/services/sets/enterprise_fee_set.rb b/app/services/sets/enterprise_fee_set.rb index 9fdb64432c..d651649232 100644 --- a/app/services/sets/enterprise_fee_set.rb +++ b/app/services/sets/enterprise_fee_set.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Sets class EnterpriseFeeSet < ModelSet def initialize(attributes = {}) diff --git a/app/services/sets/enterprise_set.rb b/app/services/sets/enterprise_set.rb index b482da0875..bb0b2bdc3c 100644 --- a/app/services/sets/enterprise_set.rb +++ b/app/services/sets/enterprise_set.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Sets class EnterpriseSet < ModelSet def initialize(collection, attributes = {}) diff --git a/app/services/sets/model_set.rb b/app/services/sets/model_set.rb index d1ac510d52..5ecd2cddab 100644 --- a/app/services/sets/model_set.rb +++ b/app/services/sets/model_set.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + # Tableless model to handle updating multiple models at once from a single form module Sets class ModelSet diff --git a/app/services/sets/order_cycle_set.rb b/app/services/sets/order_cycle_set.rb index 6459cbf0b3..663f684062 100644 --- a/app/services/sets/order_cycle_set.rb +++ b/app/services/sets/order_cycle_set.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Sets class OrderCycleSet < ModelSet def initialize(collection, attributes = {}) diff --git a/app/services/sets/product_set.rb b/app/services/sets/product_set.rb index b6054e7125..4563bce0bb 100644 --- a/app/services/sets/product_set.rb +++ b/app/services/sets/product_set.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Sets class ProductSet < ModelSet def initialize(attributes = {}) diff --git a/app/services/sets/variant_override_set.rb b/app/services/sets/variant_override_set.rb index b16d9b9e4c..3240846e54 100644 --- a/app/services/sets/variant_override_set.rb +++ b/app/services/sets/variant_override_set.rb @@ -1,7 +1,13 @@ +# frozen_string_literal: true + module Sets class VariantOverrideSet < ModelSet def initialize(collection, attributes = {}) - super(VariantOverride, collection, attributes, nil, proc { |attrs, tag_list| deletable?(attrs, tag_list) } ) + super(VariantOverride, + collection, + attributes, + nil, + proc { |attrs, tag_list| deletable?(attrs, tag_list) } ) end private @@ -16,7 +22,7 @@ module Sets tag_list.empty? end - # Override of ModelSet method to allow us to check presence of a tag_list (which is not an attribute) + # Overrides ModelSet method to check presence of a tag_list (which is not an attribute) # This method will delete VariantOverrides that have no values (see deletable? above) # If the user sets all values to nil in the UI the VO will be deleted from the DB def collection_to_delete diff --git a/spec/services/sets/model_set_spec.rb b/spec/services/sets/model_set_spec.rb index 00b71e5d19..d9154c5bf1 100644 --- a/spec/services/sets/model_set_spec.rb +++ b/spec/services/sets/model_set_spec.rb @@ -8,7 +8,9 @@ describe Sets::ModelSet do attrs = { collection_attributes: { '1' => { name: 's1' }, '2' => { name: 's2' } } } - ms = Sets::ModelSet.new(EnterpriseRelationshipPermission, EnterpriseRelationshipPermission.all, attrs) + ms = Sets::ModelSet.new(EnterpriseRelationshipPermission, + EnterpriseRelationshipPermission.all, + attrs) expect { ms.save }.to change(EnterpriseRelationshipPermission, :count).by(2) @@ -37,7 +39,7 @@ describe Sets::ModelSet do '2' => { id: e2.id, name: 'e2' } } } ms = Sets::ModelSet.new(Enterprise, Enterprise.all, attributes, nil, - proc { |attrs| attrs['name'] == 'deleteme' }) + proc { |attrs| attrs['name'] == 'deleteme' }) expect { ms.save }.to change(Enterprise, :count).by(-1) @@ -49,7 +51,7 @@ describe Sets::ModelSet do attributes = { collection_attributes: { '1' => { name: 'deleteme' } } } ms = Sets::ModelSet.new(Enterprise, Enterprise.all, attributes, nil, - proc { |attrs| attrs['name'] == 'deleteme' }) + proc { |attrs| attrs['name'] == 'deleteme' }) expect { ms.save }.to change(Enterprise, :count).by(0) end diff --git a/spec/services/sets/product_set_spec.rb b/spec/services/sets/product_set_spec.rb index a5576d9caa..36aaf9fefb 100644 --- a/spec/services/sets/product_set_spec.rb +++ b/spec/services/sets/product_set_spec.rb @@ -88,7 +88,11 @@ describe Sets::ProductSet do end let(:distributor) { create(:distributor_enterprise) } - let!(:order_cycle) { create(:simple_order_cycle, variants: [product.variants.first], coordinator: distributor, distributors: [distributor]) } + let!(:order_cycle) { + create(:simple_order_cycle, variants: [product.variants.first], + coordinator: distributor, + distributors: [distributor]) + } it 'updates the product and removes the product from order cycles' do product_set.save From ed0441dc41957e272231dd6b1d59e610b32d5a4d Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Sun, 1 Nov 2020 12:19:29 +0000 Subject: [PATCH 6/7] Fix a few more rubocop issues --- .rubocop_manual_todo.yml | 1 - .../admin/column_preferences_controller.rb | 3 ++- app/controllers/admin/variant_overrides_controller.rb | 11 +++++++---- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/.rubocop_manual_todo.yml b/.rubocop_manual_todo.yml index 67d1f8a600..934e12057c 100644 --- a/.rubocop_manual_todo.yml +++ b/.rubocop_manual_todo.yml @@ -33,7 +33,6 @@ Layout/LineLength: - app/controllers/admin/product_import_controller.rb - app/controllers/admin/schedules_controller.rb - app/controllers/admin/subscriptions_controller.rb - - app/controllers/admin/variant_overrides_controller.rb - app/controllers/api/enterprise_attachment_controller.rb - app/controllers/api/product_images_controller.rb - app/controllers/spree/paypal_controller_decorator.rb diff --git a/app/controllers/admin/column_preferences_controller.rb b/app/controllers/admin/column_preferences_controller.rb index 90ba1bd37e..90b4cb850c 100644 --- a/app/controllers/admin/column_preferences_controller.rb +++ b/app/controllers/admin/column_preferences_controller.rb @@ -29,7 +29,8 @@ module Admin collection_hash = Hash[permitted_params[:column_preferences]. each_with_index.map { |cp, i| [i, cp] }] collection_hash.select!{ |_i, cp| cp[:action_name] == permitted_params[:action_name] } - @cp_set = Sets::ColumnPreferenceSet.new @column_preferences, collection_attributes: collection_hash + @cp_set = Sets::ColumnPreferenceSet.new(@column_preferences, + collection_attributes: collection_hash) end def collection diff --git a/app/controllers/admin/variant_overrides_controller.rb b/app/controllers/admin/variant_overrides_controller.rb index c4fd99a766..3c6778640c 100644 --- a/app/controllers/admin/variant_overrides_controller.rb +++ b/app/controllers/admin/variant_overrides_controller.rb @@ -60,14 +60,17 @@ module Admin for_hubs(editable_enterprises.collect(&:id)) options = [{ id: '0', name: 'All' }] - import_dates.collect(&:import_date).map { |i| options.push(id: i.to_date, name: i.to_date.to_formatted_s(:long)) } + import_dates.collect(&:import_date).map { |i| + options.push(id: i.to_date, name: i.to_date.to_formatted_s(:long)) + } options end def load_collection collection_hash = Hash[variant_overrides_params.each_with_index.map { |vo, i| [i, vo] }] - @vo_set = Sets::VariantOverrideSet.new @variant_overrides, collection_attributes: collection_hash + @vo_set = Sets::VariantOverrideSet.new(@variant_overrides, + collection_attributes: collection_hash) end def collection @@ -82,8 +85,8 @@ module Admin [:index, :bulk_update, :bulk_reset] end - # This has been pulled from ModelSet as it is useful for compiling a list of errors on any generic collection (not necessarily a ModelSet) - # Could be pulled down into a lower level controller if it is useful in other high level controllers + # This method is also present in ModelSet + # This is useful for compiling a list of errors on any generic collection def collection_errors errors = ActiveModel::Errors.new self full_messages = @collection.map { |element| element.errors.full_messages }.flatten From fd0bba19a7c3f7c1a3d3a884594b4f8d3c9fb377 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Sun, 1 Nov 2020 13:47:59 +0000 Subject: [PATCH 7/7] Adapt enterprises code and specs to new namespace Sets::EnterpriseSet --- app/controllers/admin/enterprises_controller.rb | 6 +++--- .../admin/enterprises_controller_spec.rb | 8 ++++---- spec/features/admin/enterprises/index_spec.rb | 16 ++++++++-------- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/app/controllers/admin/enterprises_controller.rb b/app/controllers/admin/enterprises_controller.rb index b62bb9ddf5..99ae12761f 100644 --- a/app/controllers/admin/enterprises_controller.rb +++ b/app/controllers/admin/enterprises_controller.rb @@ -235,7 +235,7 @@ module Admin def check_can_change_bulk_sells unless spree_current_user.admin? - params[:enterprise_set][:collection_attributes].each do |_i, enterprise_params| + params[:sets_enterprise_set][:collection_attributes].each do |_i, enterprise_params| unless spree_current_user == Enterprise.find_by(id: enterprise_params[:id]).owner enterprise_params.delete :sells end @@ -269,7 +269,7 @@ module Admin def check_can_change_bulk_owner unless spree_current_user.admin? - params[:enterprise_set][:collection_attributes].each do |_i, enterprise_params| + params[:sets_enterprise_set][:collection_attributes].each do |_i, enterprise_params| enterprise_params.delete :owner_id end end @@ -321,7 +321,7 @@ module Admin end def bulk_params - params.require(:enterprise_set).permit( + params.require(:sets_enterprise_set).permit( collection_attributes: PermittedAttributes::Enterprise.attributes ) end diff --git a/spec/controllers/admin/enterprises_controller_spec.rb b/spec/controllers/admin/enterprises_controller_spec.rb index 8343ca0d9e..a19cada4d4 100644 --- a/spec/controllers/admin/enterprises_controller_spec.rb +++ b/spec/controllers/admin/enterprises_controller_spec.rb @@ -385,7 +385,7 @@ describe Admin::EnterprisesController, type: :controller do profile_enterprise1.enterprise_roles.build(user: new_owner).save profile_enterprise2.enterprise_roles.build(user: new_owner).save allow(controller).to receive_messages spree_current_user: new_owner - bulk_enterprise_params = { enterprise_set: { collection_attributes: { '0' => { id: profile_enterprise1.id, sells: 'any', owner_id: new_owner.id }, '1' => { id: profile_enterprise2.id, sells: 'any', owner_id: new_owner.id } } } } + bulk_enterprise_params = { sets_enterprise_set: { collection_attributes: { '0' => { id: profile_enterprise1.id, sells: 'any', owner_id: new_owner.id }, '1' => { id: profile_enterprise2.id, sells: 'any', owner_id: new_owner.id } } } } spree_put :bulk_update, bulk_enterprise_params profile_enterprise1.reload @@ -400,7 +400,7 @@ describe Admin::EnterprisesController, type: :controller do allow_any_instance_of(Sets::EnterpriseSet).to receive(:save) { false } profile_enterprise1.enterprise_roles.build(user: new_owner).save allow(controller).to receive_messages spree_current_user: new_owner - bulk_enterprise_params = { enterprise_set: { collection_attributes: { '0' => { id: profile_enterprise1.id, visible: 'false' } } } } + bulk_enterprise_params = { sets_enterprise_set: { collection_attributes: { '0' => { id: profile_enterprise1.id, visible: 'false' } } } } spree_put :bulk_update, bulk_enterprise_params expect(assigns(:enterprise_set).collection).to eq [profile_enterprise1] end @@ -409,7 +409,7 @@ describe Admin::EnterprisesController, type: :controller do context "as the owner of an enterprise" do it "allows 'sells' and 'owner' to be changed" do allow(controller).to receive_messages spree_current_user: original_owner - bulk_enterprise_params = { enterprise_set: { collection_attributes: { '0' => { id: profile_enterprise1.id, sells: 'any', owner_id: new_owner.id }, '1' => { id: profile_enterprise2.id, sells: 'any', owner_id: new_owner.id } } } } + bulk_enterprise_params = { sets_enterprise_set: { collection_attributes: { '0' => { id: profile_enterprise1.id, sells: 'any', owner_id: new_owner.id }, '1' => { id: profile_enterprise2.id, sells: 'any', owner_id: new_owner.id } } } } spree_put :bulk_update, bulk_enterprise_params profile_enterprise1.reload @@ -426,7 +426,7 @@ describe Admin::EnterprisesController, type: :controller do profile_enterprise1.enterprise_roles.build(user: new_owner).save profile_enterprise2.enterprise_roles.build(user: new_owner).save allow(controller).to receive_messages spree_current_user: admin_user - bulk_enterprise_params = { enterprise_set: { collection_attributes: { '0' => { id: profile_enterprise1.id, sells: 'any', owner_id: new_owner.id }, '1' => { id: profile_enterprise2.id, sells: 'any', owner_id: new_owner.id } } } } + bulk_enterprise_params = { sets_enterprise_set: { collection_attributes: { '0' => { id: profile_enterprise1.id, sells: 'any', owner_id: new_owner.id }, '1' => { id: profile_enterprise2.id, sells: 'any', owner_id: new_owner.id } } } } spree_put :bulk_update, bulk_enterprise_params profile_enterprise1.reload diff --git a/spec/features/admin/enterprises/index_spec.rb b/spec/features/admin/enterprises/index_spec.rb index a79fbaab3c..2709dd2201 100644 --- a/spec/features/admin/enterprises/index_spec.rb +++ b/spec/features/admin/enterprises/index_spec.rb @@ -15,7 +15,7 @@ feature 'Enterprises Index' do within("tr.enterprise-#{s.id}") do expect(page).to have_content s.name - expect(page).to have_select "enterprise_set_collection_attributes_1_sells" + expect(page).to have_select "sets_enterprise_set_collection_attributes_1_sells" expect(page).to have_content "Settings" expect(page).to have_content "Delete" expect(page).to have_no_content "Payment Methods" @@ -25,7 +25,7 @@ feature 'Enterprises Index' do within("tr.enterprise-#{d.id}") do expect(page).to have_content d.name - expect(page).to have_select "enterprise_set_collection_attributes_0_sells" + expect(page).to have_select "sets_enterprise_set_collection_attributes_0_sells" expect(page).to have_content "Settings" expect(page).to have_content "Delete" expect(page).to have_content "Payment Methods" @@ -51,10 +51,10 @@ feature 'Enterprises Index' do it "updates the enterprises" do within("tr.enterprise-#{d.id}") do - expect(page).to have_checked_field "enterprise_set_collection_attributes_0_visible" - uncheck "enterprise_set_collection_attributes_0_visible" - select 'any', from: "enterprise_set_collection_attributes_0_sells" - select d_manager.email, from: 'enterprise_set_collection_attributes_0_owner_id' + expect(page).to have_checked_field "sets_enterprise_set_collection_attributes_0_visible" + uncheck "sets_enterprise_set_collection_attributes_0_visible" + select 'any', from: "sets_enterprise_set_collection_attributes_0_sells" + select d_manager.email, from: 'sets_enterprise_set_collection_attributes_0_owner_id' end click_button "Update" expect(flash_message).to eq('Enterprises updated successfully') @@ -83,12 +83,12 @@ feature 'Enterprises Index' do it "does not update the enterprises and displays errors" do d_row_index = enterprise_row_index(d.name) within("tr.enterprise-#{d.id}") do - select d_manager.email, from: "enterprise_set_collection_attributes_#{d_row_index}_owner_id" + select d_manager.email, from: "sets_enterprise_set_collection_attributes_#{d_row_index}_owner_id" end second_distributor_row_index = enterprise_row_index(second_distributor.name) within("tr.enterprise-#{second_distributor.id}") do - select d_manager.email, from: "enterprise_set_collection_attributes_#{second_distributor_row_index}_owner_id" + select d_manager.email, from: "sets_enterprise_set_collection_attributes_#{second_distributor_row_index}_owner_id" end click_button "Update" expect(flash_message).to eq('Update failed')