Simplify Variant#default_price logic

Before this work, `#default_price` was a `has_one` association between
`Variant` and `Product`. As we already have a `has_many` association
between both models, this redundancy was a source of inconsistencies.
I.g.:

```ruby
include Spree
Variant.new(price: Price.new) # price delegates to default_price
Variant.prices # => []
```

From now on, `#default_price` is a regular method that searches within
`#prices` taking into account the criteria for a price to be considered
the default one.

We have renamed previous method `#find_default_price_or_build` to
`default_price_or_build`. The latter feels less standard according to
Rails conventions, but the intention here is, precisely, to communicate
that this is not a Rails association.

No longer being a Rails association makes it impossible to use the default
ransack conventions to build the sort-by-price link on the products
listing page. For this reason, we added
`sort_by_master_default_price_amount_{asc, desc}` scopes to the `Price`
model, which is automatically picked up by ransack. As it's now
explicit, these queries ignore the `ORDER BY` clauses implicit in the
`currently_valid_prices` method, but this was also happening in the
query built by ransack from the `has_one` association:

```SQL
SELECT "spree_products".* FROM "spree_products" LEFT OUTER JOIN
"spree_variants" ON "spree_variants"."is_master" = ? AND "spree
_variants"."product_id" = "spree_products"."id" LEFT OUTER JOIN
"spree_prices" ON "spree_prices"."currency" = ? AND "spree_pric
es"."country_iso" IS NULL AND "spree_prices"."variant_id" =
"spree_variants"."id" WHERE "spree_products"."deleted_at" IS NULL O
RDER BY "spree_prices"."amount" ASC, "spree_products"."id" ASC LIMIT ?
OFFSET ?  [["is_master", 1], ["currency", "USD"], ["LIMI  T", 10],
["OFFSET", 0]]
```

However, the new query doesn't include discarded prices on the result,
but I think it's something desirable:

```SQL
SELECT "spree_products".* FROM "spree_products" LEFT OUTER JOIN
"spree_variants" "master" ON "master"."is_master" = ? AND "mast
er"."product_id" = "spree_products"."id" LEFT OUTER JOIN "spree_prices"
"prices" ON "prices"."deleted_at" IS NULL AND "prices".  "variant_id" =
"master"."id" LEFT OUTER JOIN "spree_variants" "masters_spree_products"
ON "masters_spree_products"."is_master"   = ? AND
"masters_spree_products"."product_id" = "spree_products"."id" WHERE
"spree_products"."deleted_at" IS NULL AND "prices".  "currency" = ? AND
"prices"."country_iso" IS NULL AND "prices"."deleted_at" IS NULL ORDER
BY prices.amount DESC, "spree_product  s"."id" ASC LIMIT ? OFFSET ?
[["is_master", 1], ["is_master", 1], ["currency", "USD"], ["LIMIT", 10],
["OFFSET", 0]]
```

To study this logic made me realize that the `#default_price` logic is
kind of weird. The fact that it includes discarded prices makes little
sense in my view (other than being able to get the default price that a
discarded variant had, but as far as I can see there's no GUI for
discarded models). The order criteria neither make sense. Taking the
`updated_at` attribute to prioritize could lead to undesired behavior:

- Create a product with a default price.
- Add another price in the same currency to override the previous one.
- Update the previous price for some reason.
- Now, the previous price becomes the default.

On top of that, probably it doesn't make sense to allow having different
prices for the same country & currency tuple.

As follow-up work I'd propose:

- Stop including discarded prices.
- Validate that they can't coexist two prices with the same currency/country
  tuple, and remove the ordering criteria for default prices. We can
  provide more flexibility adding an `active` flag and scoping the
  validation to the case it's `true`.

api/app/controllers/spree/api/base_controller.rb

@@ -139,7 +139,7 @@ def product_scope
       end
 
       def variants_associations
-        [{ option_values: :option_type }, :default_price, :images]
+        [{ option_values: :option_type }, :prices, :images]
       end
 
       def product_includes

api/app/controllers/spree/api/shipments_controller.rb

@@ -186,7 +186,7 @@ def mine_includes
             },
             variant: {
               product: {},
-              default_price: {},
+              prices: {},
               option_values: {
                 option_type: {}
               }

api/app/controllers/spree/api/taxons_controller.rb

@@ -69,7 +69,7 @@ def products
         # Products#index does not do the sorting.
         taxon = Spree::Taxon.find(params[:id])
         @products = paginate(taxon.products.ransack(params[:q]).result)
-        @products = @products.includes(master: :default_price)
+        @products = @products.includes(master: :prices)
 
         if params[:simple]
           @exclude_data = {

api/app/controllers/spree/api/variants_controller.rb

@@ -8,7 +8,7 @@ class VariantsController < Spree::Api::BaseController
       def create
         authorize! :create, Variant
         @variant = scope.new(variant_params)
-        @variant.inherit_prices unless @variant.any_price?
+        @variant.inherit_prices unless @variant.prices.any?
         if @variant.save
           respond_with(@variant, status: 201, default_template: :show)
         else
@@ -84,7 +84,7 @@ def variant_params
       end
 
       def include_list
-        [{ option_values: :option_type }, :product, :default_price, :images, { stock_items: :stock_location }]
+        [{ option_values: :option_type }, :product, :prices, :images, { stock_items: :stock_location }]
       end
     end
   end

backend/app/controllers/spree/admin/products_controller.rb

@@ -104,7 +104,7 @@ def update_before
       end
 
       def product_includes
-        [:variant_images, { variants: [:images], master: [:images, :default_price] }]
+        [:variant_images, { variants: [:images], master: [:images, :prices] }]
       end
 
       def clone_object_url(resource)

backend/app/controllers/spree/admin/variants_controller.rb

@@ -34,7 +34,7 @@ def load_data
       end
 
       def variant_includes
-        [{ option_values: :option_type }, :default_price]
+        [{ option_values: :option_type }, :prices]
       end
 
       def redirect_on_empty_option_values

backend/app/views/spree/admin/products/index.html.erb

@@ -78,7 +78,7 @@
               <%= render 'spree/admin/shared/image', image: product.gallery.images.first, size: :mini %>
             </td>
             <td><%= link_to product.try(:name), edit_admin_product_path(product) %></td>
-            <td class="align-right"><%= product.display_price.to_html %></td>
+            <td class="align-right"><%= product.display_price&.to_html %></td>
             <td class="actions" data-hook="admin_products_index_row_actions">
               <%= link_to_edit product, no_text: true, class: 'edit' if can?(:edit, product) && !product.deleted? %>
               &nbsp;

core/app/models/concerns/spree/default_price.rb

@@ -5,19 +5,10 @@ module DefaultPrice
     extend ActiveSupport::Concern
 
     included do
-      # TODO: Treat as a regular method to avoid in-memory inconsistencies with
-      # `prices`. e.g.:
-      #
-      # ```
-      # Variant.new(price: 25).prices.any? # => false
-      # ```
-      has_one :default_price,
-        -> { with_discarded.currently_valid.with_default_attributes },
-        class_name: 'Spree::Price',
-        inverse_of: :variant,
-        dependent: :destroy,
-        autosave: true
+      delegate :display_price, :display_amount, :price, to: :default_price, allow_nil: true
+      delegate :price=, to: :default_price_or_build
 
+      # @see Spree::Variant::PricingOptions.default_price_attributes
       def self.default_pricing
         Spree::Config.default_pricing_options.desired_attributes
       end
@@ -30,31 +21,48 @@ def currently_valid_prices
       prices.currently_valid
     end
 
-    def find_or_build_default_price
+    # Returns {#default_price} or builds it from {Spree::Price.default_pricing}
+    #
+    # @return [Spree::Price, nil]
+    # @see Spree::Price.default_pricing
+    def default_price_or_build
       default_price ||
-        default_price_from_memory ||
-        build_default_price(self.class.default_pricing)
+        prices.build(self.class.default_pricing)
     end
 
-    delegate :display_price, :display_amount, :price, to: :find_or_build_default_price
-    delegate :price=, to: :find_or_build_default_price
-
-    def has_default_price?
-      default_price.present? && !default_price.discarded?
-    end
-
-    private
-
-    # TODO: Remove when {Spree::Price.default_price} is no longer an
-    # association.
-    def default_price_from_memory
-      prices.to_a.select(&:new_record?).find do |price|
+    # Select from {#prices} the one to be considered as the default
+    #
+    # This method works with the in-memory association, so non-persisted prices
+    # are taken into account. Discarded prices are also considered.
+    #
+    # A price is a candidate to be considered as the default when it meets
+    # {Spree::Variant.default_pricing} criteria. When more than one candidate is
+    # found, non-persisted records take preference. When more than one persisted
+    # candidate exists, the one most recently updated is taken or, in case of
+    # race condition, the one with higher id.
+    #
+    # @return [Spree::Price, nil]
+    # @see Spree::Price.default_pricing
+    def default_price
+      candidates = (prices + prices.with_discarded).to_a.uniq
+      candidates.select do |price|
         price.
           attributes.
           values_at(
             *self.class.default_pricing.keys.map(&:to_s)
           ) == self.class.default_pricing.values
+      end.min do |a, b|
+        [b, a].map do |i|
+          [
+            i.updated_at || Time.zone.now,
+            i.id || Float::INFINITY
+          ]
+        end.reduce { |x, y| x <=> y }
       end
     end
+
+    def has_default_price?
+      default_price.present? && !default_price.discarded?
+    end
   end
 end

core/app/models/spree/product.rb

@@ -61,6 +61,17 @@ class Product < Spree::Base
     has_many :line_items, through: :variants_including_master
     has_many :orders, through: :line_items
 
+    scope :sort_by_master_default_price_amount_asc, -> {
+      with_default_price.order('spree_prices.amount ASC')
+    }
+    scope :sort_by_master_default_price_amount_desc, -> {
+      with_default_price.order('spree_prices.amount DESC')
+    }
+    scope :with_default_price, -> {
+      left_joins(master: :prices)
+        .where(master: { spree_prices: Spree::Config.default_pricing_options.desired_attributes })
+    }
+
     def find_or_build_master
       master || build_master
     end

core/app/models/spree/product/scopes.rb

@@ -29,25 +29,25 @@ def self.property_conditions(property)
           scope :descend_by_name, -> { order(name: :desc) }
 
           add_search_scope :ascend_by_master_price do
-            joins(master: :default_price).select('spree_products.* , spree_prices.amount')
+            joins(master: :prices).select('spree_products.* , spree_prices.amount')
                                          .order(Spree::Price.arel_table[:amount].asc)
           end
 
           add_search_scope :descend_by_master_price do
-            joins(master: :default_price).select('spree_products.* , spree_prices.amount')
+            joins(master: :prices).select('spree_products.* , spree_prices.amount')
                                          .order(Spree::Price.arel_table[:amount].desc)
           end
 
           add_search_scope :price_between do |low, high|
-            joins(master: :default_price).where(Price.table_name => { amount: low..high })
+            joins(master: :prices).where(Price.table_name => { amount: low..high })
           end
 
           add_search_scope :master_price_lte do |price|
-            joins(master: :default_price).where("#{price_table_name}.amount <= ?", price)
+            joins(master: :prices).where("#{price_table_name}.amount <= ?", price)
           end
 
           add_search_scope :master_price_gte do |price|
-            joins(master: :default_price).where("#{price_table_name}.amount >= ?", price)
+            joins(master: :prices).where("#{price_table_name}.amount >= ?", price)
           end
 
           # This scope selects products in taxon AND all its descendants

core/app/models/spree/variant.rb

@@ -358,17 +358,6 @@ def inherit_prices
       self.prices = product.master.prices.map(&:dup)
     end
 
-    # TODO: Treat {Spree::Variant.default_price} as a method
-    #
-    # Returns whether self is associated to {Spree::Price} through any of their
-    # associations.
-    #
-    # @return [Boolean]
-    def any_price?
-      prices.any? ||
-        default_price.present?
-    end
-
     private
 
     def rebuild_vat_prices?

core/lib/spree/core/product_filters.rb

@@ -61,7 +61,7 @@ module ProductFilters
           scope = scope.or(new_scope)
         end
 
-        Spree::Product.joins(master: :default_price).where(scope)
+        Spree::Product.joins(master: :prices).where(scope)
       end
 
       def self.format_price(amount)

core/spec/models/spree/product_spec.rb

@@ -152,8 +152,8 @@ class Extension < Spree::Base
       context "with currency set to JPY" do
         before do
           product.master.default_price.currency = 'JPY'
-          product.master.default_price.save!
           stub_spree_preferences(currency: 'JPY')
+          product.master.default_price.save!
         end
 
         it "displays the currency in yen" do
@@ -619,4 +619,26 @@ class Extension < Spree::Base
       expect(subject).to respond_to(:images)
     end
   end
+
+  describe '.sort_by_master_default_price_amount_asc' do
+    it 'returns first those which default price is lower' do
+      product_1 = create(:product, price: 10)
+      product_2 = create(:product, price: 5)
+
+      result = described_class.sort_by_master_default_price_amount_asc
+
+      expect(result).to eq([product_2, product_1])
+    end
+  end
+
+  describe '.sort_by_master_default_price_amount_desc' do
+    it 'returns first those which default price is higher' do
+      product_1 = create(:product, price: 10)
+      product_2 = create(:product, price: 5)
+
+      result = described_class.sort_by_master_default_price_amount_desc
+
+      expect(result).to eq([product_1, product_2])
+    end
+  end
 end