From 515fbe501741aa85d4423ddb69edd08dd6a6ea5f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marc=20Busqu=C3=A9?= <marc@lamarciana.com>
Date: Tue, 16 Mar 2021 16:28:03 +0100
Subject: [PATCH] Move currently_valid_prices to a method

Having `currently_valid_prices` as an association between `Variant` and
`Price` made it a source of inconsistencies with the undecorated
`prices` association:

```
Spree::Variant.new.tap do |v|
  v.currently_valid_prices << Spree::Price.new
end.prices.any? # => false
```
---
 .../models/concerns/spree/default_price.rb    |  7 +++++
 core/app/models/spree/variant.rb              |  8 ------
 core/lib/spree/core/search/base.rb            |  2 +-
 core/spec/models/spree/price_spec.rb          | 27 +++++++++++++++++++
 core/spec/models/spree/variant_spec.rb        |  2 --
 core/spec/support/concerns/default_price.rb   | 14 ++++++++++
 6 files changed, 49 insertions(+), 11 deletions(-)

diff --git a/core/app/models/concerns/spree/default_price.rb b/core/app/models/concerns/spree/default_price.rb
index ee232e0b07f..20d11bd4bbc 100644
--- a/core/app/models/concerns/spree/default_price.rb
+++ b/core/app/models/concerns/spree/default_price.rb
@@ -13,6 +13,13 @@ module DefaultPrice
         autosave: true
     end
 
+    # Returns `#prices` prioritized for being considered as default price
+    #
+    # @return [ActiveRecord::Relation<Spree::Price>]
+    def currently_valid_prices
+      prices.currently_valid
+    end
+
     def find_or_build_default_price
       default_price || build_default_price(Spree::Config.default_pricing_options.desired_attributes)
     end
diff --git a/core/app/models/spree/variant.rb b/core/app/models/spree/variant.rb
index 1ecf99ae592..17456a2061d 100644
--- a/core/app/models/spree/variant.rb
+++ b/core/app/models/spree/variant.rb
@@ -24,7 +24,6 @@ class Variant < Spree::Base
       stock_items.discard_all
       images.destroy_all
       prices.discard_all
-      currently_valid_prices.discard_all
     end
 
     attr_writer :rebuild_vat_prices
@@ -58,13 +57,6 @@ class Variant < Spree::Base
       inverse_of: :variant,
       autosave: true
 
-    has_many :currently_valid_prices,
-      -> { currently_valid },
-      class_name: 'Spree::Price',
-      dependent: :destroy,
-      inverse_of: :variant,
-      autosave: true
-
     before_validation :set_cost_currency
     before_validation :set_price, if: -> { product && product.master }
     before_validation :build_vat_prices, if: -> { rebuild_vat_prices? || new_record? && product }
diff --git a/core/lib/spree/core/search/base.rb b/core/lib/spree/core/search/base.rb
index 7d641ffd804..96177e060cc 100644
--- a/core/lib/spree/core/search/base.rb
+++ b/core/lib/spree/core/search/base.rb
@@ -51,7 +51,7 @@ def add_eagerload_scopes(scope)
           # separate queries most of the time but opt for a join as soon as any
           # `where` constraints affecting joined tables are added to the search;
           # which is the case as soon as a taxon is added to the base scope.
-          scope = scope.preload(master: :currently_valid_prices)
+          scope = scope.preload(master: :prices)
           scope = scope.preload(master: :images) if @properties[:include_images]
           scope
         end
diff --git a/core/spec/models/spree/price_spec.rb b/core/spec/models/spree/price_spec.rb
index f9e76f26121..d53c959cab0 100644
--- a/core/spec/models/spree/price_spec.rb
+++ b/core/spec/models/spree/price_spec.rb
@@ -96,6 +96,33 @@
         expect(price.country).to eq(country)
       end
     end
+
+    describe '.currently_valid' do
+      it 'prioritizes first those associated to a country' do
+        price_1 = create(:price, country: create(:country))
+        price_2 = create(:price, country: nil) { |price| price.touch }
+
+        result = described_class.currently_valid
+
+        expect(
+          result.index(price_1) < result.index(price_2)
+        ).to be(true)
+      end
+
+      context 'when country data is the same' do
+        it 'prioritizes first those recently updated' do
+          price_1 = create(:price, country: nil)
+          price_2 = create(:price, country: nil)
+          price_1.touch
+
+          result = described_class.currently_valid
+
+          expect(
+            result.index(price_1) < result.index(price_2)
+          ).to be(true)
+        end
+      end
+    end
   end
 
   describe "#currency" do
diff --git a/core/spec/models/spree/variant_spec.rb b/core/spec/models/spree/variant_spec.rb
index 4d11533e125..d8c4bcef4f5 100644
--- a/core/spec/models/spree/variant_spec.rb
+++ b/core/spec/models/spree/variant_spec.rb
@@ -678,14 +678,12 @@
 
       expect(variant.stock_items).not_to be_empty
       expect(variant.prices).not_to be_empty
-      expect(variant.currently_valid_prices).not_to be_empty
 
       variant.discard
 
       expect(variant.images).to be_empty
       expect(variant.stock_items.reload).to be_empty
       expect(variant.prices).to be_empty
-      expect(variant.currently_valid_prices).to be_empty
     end
 
     describe 'default_price' do
diff --git a/core/spec/support/concerns/default_price.rb b/core/spec/support/concerns/default_price.rb
index 1656e9d588c..2972fa81542 100644
--- a/core/spec/support/concerns/default_price.rb
+++ b/core/spec/support/concerns/default_price.rb
@@ -41,4 +41,18 @@
       it { is_expected.to be_falsey }
     end
   end
+
+  describe '#currently_valid_prices' do
+    it 'returns prioritized prices' do
+      price_1 = create(:price, country: create(:country))
+      price_2 = create(:price, country: nil)
+      variant = create(:variant, prices: [price_1, price_2])
+
+      result = variant.currently_valid_prices
+
+      expect(
+        result.index(price_1) < result.index(price_2)
+      ).to be(true)
+    end
+  end
 end