Skip to content

Commit

Permalink
Fix duplicate variants on product page
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
mamhoff committed Mar 16, 2018
1 parent 9d408ef commit 409c68f
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 1 deletion.
14 changes: 13 additions & 1 deletion core/app/models/spree/variant.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 10 additions & 0 deletions core/spec/models/spree/product_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
14 changes: 14 additions & 0 deletions core/spec/models/spree/variant/scopes_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 409c68f

Please sign in to comment.