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.

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

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

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

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