From d3b9db317bfd432508dd90cf6779c8bbfc5463ba Mon Sep 17 00:00:00 2001
From: Thomas von Deyen <thomas@vondeyen.com>
Date: Thu, 12 Sep 2024 10:15:05 +0200
Subject: [PATCH] Replace custom product ransacker with association

We introduced a custom Ransacker to allow filtering producs by
their variants option values in the new admin interface.

This ransacker is broken in latest Ransack v4.2 and overly
complicated.  By introducing a option_values association to
products, that does the same join over variants_including_master
and their distinct option_values we can use normal ransack search.

Quoting the Ransack documentation:

> "Ransackers, like scopes, are not a cure-all. Many use cases can be better solved with a standard Ransack search on a dedicated database search field, which is faster, index-able, and scales better than converting/ransacking data on the fly."
---
 .../solidus_admin/products/index/component.rb |  2 +-
 core/app/models/spree/product.rb              | 24 ++++---------------
 core/spec/models/spree/product_spec.rb        | 12 +++++-----
 3 files changed, 11 insertions(+), 27 deletions(-)

diff --git a/admin/app/components/solidus_admin/products/index/component.rb b/admin/app/components/solidus_admin/products/index/component.rb
index 029b1c2be0..6a015ebcbd 100644
--- a/admin/app/components/solidus_admin/products/index/component.rb
+++ b/admin/app/components/solidus_admin/products/index/component.rb
@@ -68,7 +68,7 @@ def filters
       {
         label: option_type.presentation,
         combinator: 'or',
-        attribute: "variants_option_values",
+        attribute: "option_values_id",
         predicate: "in",
         options: option_type.option_values.pluck(:name, :id),
       }
diff --git a/core/app/models/spree/product.rb b/core/app/models/spree/product.rb
index 107a876759..f06df96cd2 100644
--- a/core/app/models/spree/product.rb
+++ b/core/app/models/spree/product.rb
@@ -57,6 +57,8 @@ class Product < Spree::Base
     has_many :line_items, through: :variants_including_master
     has_many :orders, through: :line_items
 
+    has_many :option_values, -> { distinct }, through: :variants_including_master
+
     scope :sort_by_master_default_price_amount_asc, -> {
       with_default_price.order('spree_prices.amount ASC')
     }
@@ -127,26 +129,8 @@ def find_or_build_master
 
     alias :options :product_option_types
 
-    # The :variants_option_values ransacker filters Spree::Product based on
-    # variant option values ids.
-    #
-    # Usage:
-    # Spree::Product.ransack(
-    #   variants_option_values_in: [option_value_id1, option_value_id2]
-    # ).result
-    ransacker :variants_option_values, formatter: proc { |v|
-      if OptionValue.exists?(v)
-        joins(variants_including_master: :option_values)
-          .where(spree_option_values: { id: v })
-          .distinct
-          .select(:id).arel
-      end
-    } do |parent|
-      parent.table[:id]
-    end
-
-    self.allowed_ransackable_associations = %w[stores variants_including_master master variants]
-    self.allowed_ransackable_attributes = %w[name slug variants_option_values]
+    self.allowed_ransackable_associations = %w[stores variants_including_master master variants option_values]
+    self.allowed_ransackable_attributes = %w[name slug]
     self.allowed_ransackable_scopes = %i[available with_discarded with_all_variant_sku_cont with_kept_variant_sku_cont]
 
     # @return [Boolean] true if there are any variants
diff --git a/core/spec/models/spree/product_spec.rb b/core/spec/models/spree/product_spec.rb
index 19f6752731..561ba07c4d 100644
--- a/core/spec/models/spree/product_spec.rb
+++ b/core/spec/models/spree/product_spec.rb
@@ -627,13 +627,13 @@ class Extension < Spree::Base
     end
   end
 
-  context "ransacker :variants_option_values" do
+  describe "ransack :option_values_id_in" do
     it "filters products based on option values of their variants" do
       product_1 = create(:product)
       option_value_1 = create(:option_value)
       create(:variant, product: product_1, option_values: [option_value_1])
 
-      result = Spree::Product.ransack(variants_option_values_in: [option_value_1.id]).result
+      result = Spree::Product.ransack(option_values_id_in: [option_value_1.id]).result
       expect(result).to contain_exactly(product_1)
     end
 
@@ -644,13 +644,13 @@ class Extension < Spree::Base
       create(:variant, product: product_1, option_values: [option_value_1])
       create(:variant, product: product_2, option_values: [option_value_1])
 
-      result = Spree::Product.ransack(variants_option_values_in: [option_value_1.id]).result
+      result = Spree::Product.ransack(option_values_id_in: [option_value_1.id]).result
       expect(result).to contain_exactly(product_1, product_2)
     end
 
     it "returns no products if there is no match" do
       non_existing_option_value_id = 99999
-      result = Spree::Product.ransack(variants_option_values_in: [non_existing_option_value_id]).result
+      result = Spree::Product.ransack(option_values_id_in: [non_existing_option_value_id]).result
       expect(result).to be_empty
     end
 
@@ -662,7 +662,7 @@ class Extension < Spree::Base
       create(:variant, product: product_1, option_values: [option_value_1])
       create(:variant, product: product_2, option_values: [option_value_2])
 
-      result = Spree::Product.ransack(variants_option_values_in: [option_value_1.id, option_value_2.id]).result
+      result = Spree::Product.ransack(option_values_id_in: [option_value_1.id, option_value_2.id]).result
       expect(result).to contain_exactly(product_1, product_2)
     end
 
@@ -674,7 +674,7 @@ class Extension < Spree::Base
       create(:variant, product: product_1, option_values: [option_value_1])
       create(:variant, product: product_2, option_values: [option_value_2])
 
-      result = Spree::Product.ransack(variants_option_values_in: [option_value_1.id]).result
+      result = Spree::Product.ransack(option_values_id_in: [option_value_1.id]).result
       expect(result).not_to include(product_2)
     end
   end