From 409c68f92c399d3f176358403991a085d1bd8dbf Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@gmail.com>
Date: Thu, 15 Mar 2018 18:18:08 +0100
Subject: [PATCH] Fix duplicate variants on product page

This fixes issue #1654. The default pricing options class has a
`search_arguments` which adds `nil` as a matching possibility for the
price's country field (in order to accomodate for the fallback price).

In Spree::Variant.with_prices, Spree::Price.where(search_arguments) is
merged into a Variant query. This results in duplicate variants being
returned.

From the standard product page, `Spree::Variant.with_prices` is called
by `Spree::Product#variants_and_option_values_for(current_pricing_options)`.
This replaces the 'merge' call in Spree::Variant.with_prices with an
SQL EXISTS subquery.

There are quirks with ActiveRecord here:
`.where(pricing_options.search_arguments)` - which translates to
`.where(currency: "EUR", country_iso: ["FR", nil])` and does pretty
much the same as the expression in this commit. However, that one
just simply does not work in this context, which is why I took the decision
to write the SQL by hand.
---
 core/app/models/spree/variant.rb              | 14 +++++++++++++-
 core/spec/models/spree/product_spec.rb        | 10 ++++++++++
 core/spec/models/spree/variant/scopes_spec.rb | 14 ++++++++++++++
 3 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/core/app/models/spree/variant.rb b/core/app/models/spree/variant.rb
index db4060b6e08..e2f0a55ef5a 100644
--- a/core/app/models/spree/variant.rb
+++ b/core/app/models/spree/variant.rb
@@ -138,11 +138,23 @@ def self.active(currency = nil)
     end
 
     # Returns variants that have a price for the given pricing options
+    # If you have modified the pricing options class, you might want to modify this scope too.
     #
     # @param pricing_options A Pricing Options object as defined on the price selector class
     # @return [ActiveRecord::Relation]
     def self.with_prices(pricing_options = Spree::Config.default_pricing_options)
-      joins(:prices).merge(Spree::Price.currently_valid.where(pricing_options.search_arguments))
+      where(
+        Spree::Price.
+          where(Spree::Variant.arel_table[:id].eq(Spree::Price.arel_table[:variant_id])).
+          # This next clause should just be `where(pricing_options.search_arguments)`, but ActiveRecord
+          # generates invalid SQL, so the SQL here is written manually.
+          where(
+            "spree_prices.currency = ? AND (spree_prices.country_iso IS NULL OR spree_prices.country_iso = ?)",
+            pricing_options.search_arguments[:currency],
+            pricing_options.search_arguments[:country_iso].compact
+          ).
+          arel.exists
+      )
     end
 
     # @return [Spree::TaxCategory] the variant's tax category
diff --git a/core/spec/models/spree/product_spec.rb b/core/spec/models/spree/product_spec.rb
index e791223cb12..61c9ac81dbc 100644
--- a/core/spec/models/spree/product_spec.rb
+++ b/core/spec/models/spree/product_spec.rb
@@ -219,6 +219,16 @@ class Extension < Spree::Base
           expect(product.variants_and_option_values_for(pricing_options)).to contain_exactly(low)
         end
       end
+
+      context 'when a variant has a fallback price' do
+        before do
+          low.prices.create(country_iso: nil)
+        end
+
+        it "returns that variant once" do
+          expect(product.variants_and_option_values_for.length).to eq(2)
+        end
+      end
     end
 
     describe "#variant_option_values_by_option_type" do
diff --git a/core/spec/models/spree/variant/scopes_spec.rb b/core/spec/models/spree/variant/scopes_spec.rb
index 7c1a37206ca..3bdd10b9f82 100644
--- a/core/spec/models/spree/variant/scopes_spec.rb
+++ b/core/spec/models/spree/variant/scopes_spec.rb
@@ -28,6 +28,20 @@
         end
       end
     end
+
+    context 'when searching for a variant that has two eligible prices (one fallback)' do
+      let(:france) { create(:country, iso: "FR") }
+      let(:pricing_options) { Spree::Variant::PricingOptions.new(country_iso: "FR", currency: "EUR") }
+
+      subject { Spree::Variant.with_prices(pricing_options) }
+
+      before do
+        variant_1.prices.create!(currency: "EUR", country: france, amount: 10)
+        variant_1.prices.create!(currency: "EUR", country: nil, amount: 10)
+      end
+
+      it { is_expected.to eq([variant_1]) }
+    end
   end
 
   it ".descend_by_popularity" do