From 060530cda8774b52b041010433a2bbc76aaff867 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Tue, 26 Jan 2021 17:15:57 +0100 Subject: [PATCH] Do not fetch VOs with deleted variant In the line below we filter them out in Ruby so it's a waste of resources. The fundamental difference is that `#includes` and `#references` results in LEFT JOINs, whereas `#joins` results in INNER JOIN, and because there's a default scope on `deleted_at IS NULL`, these are not included in the result set. This however, requires us to move away from the current algorithm but unfortunately we can't refactor it completely yet. Before: ```sql SELECT * FROM "variant_overrides" LEFT OUTER JOIN "spree_variants" ON "spree_variants"."id" = "variant_overrides"."variant_id" AND "spree_variants"."deleted_at" IS NULL LEFT OUTER JOIN "spree_products" ON "spree_products"."id" = "spree_variants"."product_id" AND "spree_products"."deleted_at" IS NULL WHERE "variant_overrides"."permission_revoked_at" IS NULL AND "variant_overrides"."hub_id" IN ( SELECT "enterprises"."id" FROM "enterprises" INNER JOIN "enterprise_roles" ON "enterprise_roles"."enterprise_id" = "enterprises"."id" WHERE (enterprise_roles.user_id = ?) AND (sells != 'none') ORDER BY name) ``` After: ```sql SELECT "variant_overrides".* FROM "variant_overrides" INNER JOIN "spree_variants" ON "spree_variants"."id" = "variant_overrides"."variant_id" AND "spree_variants"."deleted_at" IS NULL INNER JOIN "spree_products" ON "spree_products"."id" = "spree_variants"."product_id" AND "spree_products"."deleted_at" IS NULL WHERE "variant_overrides"."permission_revoked_at" IS NULL AND "variant_overrides"."hub_id" IN ( SELECT "enterprises"."id" FROM "enterprises" INNER JOIN "enterprise_roles" ON "enterprise_roles"."enterprise_id" = "enterprises"."id" WHERE (enterprise_roles.user_id = ?) AND (sells != 'none') ORDER BY name) ``` This is covered in the test suite by spec/controllers/admin/variant_overrides_controller_spec.rb:72. It keeps passing so we're good to go. --- .../admin/variant_overrides_controller.rb | 6 +-- app/models/variant_override.rb | 9 ++++ app/services/sets/model_set.rb | 11 ++++- app/services/sets/variant_override_set.rb | 41 ++++++++++++++----- 4 files changed, 51 insertions(+), 16 deletions(-) diff --git a/app/controllers/admin/variant_overrides_controller.rb b/app/controllers/admin/variant_overrides_controller.rb index 3c6778640c..7e4359a7f4 100644 --- a/app/controllers/admin/variant_overrides_controller.rb +++ b/app/controllers/admin/variant_overrides_controller.rb @@ -75,10 +75,8 @@ module Admin def collection @variant_overrides = VariantOverride. - includes(variant: :product). - for_hubs(params[:hub_id] || @hubs). - references(:variant). - select { |vo| vo.variant.present? } + joins(variant: :product). + for_hubs(params[:hub_id] || @hubs) end def collection_actions diff --git a/app/models/variant_override.rb b/app/models/variant_override.rb index f2630d98a1..ad4369ea51 100644 --- a/app/models/variant_override.rb +++ b/app/models/variant_override.rb @@ -68,4 +68,13 @@ class VariantOverride < ActiveRecord::Base end self end + + def deletable? + price.blank? && + count_on_hand.blank? && + default_stock.blank? && + resettable.blank? && + sku.nil? && + on_demand.nil? + end end diff --git a/app/services/sets/model_set.rb b/app/services/sets/model_set.rb index 5ecd2cddab..87e2ef4655 100644 --- a/app/services/sets/model_set.rb +++ b/app/services/sets/model_set.rb @@ -29,11 +29,20 @@ module Sets if found_element.nil? @collection << @klass.new(attributes) unless @reject_if.andand.call(attributes) else - found_element.assign_attributes(attributes.except(:id)) + process(found_element, attributes) end end end + # The request params are assigned to the fetched record here, becoming dirty and ready to + # be persisted in DB as an UPDATE. + # + # This enables having different process strategies depending on the concrete model set class, if + # we choose to. This will be the default implementation. + def process(found_element, attributes) + found_element.assign_attributes(attributes.except(:id)) + end + def errors errors = ActiveModel::Errors.new self full_messages = @collection diff --git a/app/services/sets/variant_override_set.rb b/app/services/sets/variant_override_set.rb index 3240846e54..0e4e90fac2 100644 --- a/app/services/sets/variant_override_set.rb +++ b/app/services/sets/variant_override_set.rb @@ -3,23 +3,30 @@ module Sets class VariantOverrideSet < ModelSet def initialize(collection, attributes = {}) + @collection_to_delete = [] + super(VariantOverride, collection, attributes, nil, - proc { |attrs, tag_list| deletable?(attrs, tag_list) } ) + proc { |variant_override| deletable?(variant_override) } ) + end + + protected + + def process(variant_override, attributes) + variant_override.assign_attributes(attributes.except(:id)) + if deletable?(variant_override) + @collection_to_delete << variant_override + else + variant_override.assign_attributes(attributes.except(:id)) + end end 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? + def deletable?(variant_override) + variant_override.deletable? && variant_override.tag_list.empty? end # Overrides ModelSet method to check presence of a tag_list (which is not an attribute) @@ -27,12 +34,24 @@ module Sets # 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) } + + if collection.is_a?(ActiveRecord::Relation) + deleted = @collection_to_delete + else + collection.delete_if do |variant_override| + deleted << variant_override if @delete_if.andand.call(variant_override) + end + end + deleted end def collection_to_keep - collection.reject { |e| @delete_if.andand.call(e.attributes, e.tag_list) } + if collection.is_a?(ActiveRecord::Relation) + collection - @collection_to_delete + else + collection.reject { |e| @delete_if.andand.call(e.attributes, e.tag_list) } + end end end end