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.
E.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]]
```

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

@@ -83,7 +83,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

@@ -16,7 +16,7 @@ def new_before
         @object.attributes = @object.product.master.attributes.except('id', 'created_at', 'deleted_at',
                                                                       'sku', 'is_master')
         # Shallow Clone of the default price to populate the price field.
-        @object.default_price = @object.product.master.default_price.clone
+        @object.prices.build(@object.product.master.default_price.attributes.except("id", "created_at", "updated_at", "deleted_at"))
       end
 
       def collection
@@ -35,7 +35,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;

backend/app/views/spree/admin/variants/_form.html.erb

@@ -65,7 +65,7 @@
       <div class="col-3">
         <div class="field" data-hook="price">
           <%= f.label :price %>
-          <%= render "spree/admin/shared/number_with_currency", f: f, amount_attr: :price, currency: @variant.find_or_build_default_price.currency %>
+          <%= render "spree/admin/shared/number_with_currency", f: f, amount_attr: :price, currency: @variant.default_price_or_build.currency %>
         </div>
       </div>
 

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

@@ -5,12 +5,13 @@ module DefaultPrice
     extend ActiveSupport::Concern
 
     included do
-      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_price_attributes
+        Spree::Config.default_pricing_options.desired_attributes
+      end
     end
 
     # Returns `#prices` prioritized for being considered as default price
@@ -20,15 +21,60 @@ 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)
+    # Returns {#default_price} or builds it from {Spree::Variant.default_price_attributes}
+    #
+    # @return [Spree::Price, nil]
+    # @see Spree::Variant.default_price_attributes
+    def default_price_or_build
+      default_price ||
+        prices.build(self.class.default_price_attributes)
     end
 
-    delegate :display_price, :display_amount, :price, to: :find_or_build_default_price
-    delegate :price=, to: :find_or_build_default_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_price_attributes} 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::Variant.default_price_attributes
+    def default_price
+      prioritized_default(
+        prices_meeting_criteria_to_be_default(
+          (prices + prices.with_discarded).uniq
+        )
+      )
+    end
 
     def has_default_price?
       default_price.present? && !default_price.discarded?
     end
+
+    private
+
+    def prices_meeting_criteria_to_be_default(prices)
+      criteria = self.class.default_price_attributes.transform_keys(&:to_s)
+      prices.select do |price|
+        contender = price.attributes.slice(*criteria.keys)
+        criteria == contender
+      end
+    end
+
+    def prioritized_default(prices)
+      prices.min do |prev, succ|
+        contender_one, contender_two = [succ, prev].map do |item|
+          [
+            item.updated_at || Time.zone.now,
+            item.id || Float::INFINITY
+          ]
+        end
+        contender_one <=> contender_two
+      end
+    end
   end
 end

core/app/models/spree/product.rb

@@ -60,6 +60,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/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

@@ -151,8 +151,7 @@ class Extension < Spree::Base
 
       context "with currency set to JPY" do
         before do
-          product.master.default_price.currency = 'JPY'
-          product.master.default_price.save!
+          product.master.default_price.update!(currency: 'JPY')
           stub_spree_preferences(currency: 'JPY')
         end
 
@@ -619,4 +618,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