Merge pull request #4691 from solidusio/backport/v3.1/pr-4639

[v3.1] Merge pull request #4639 from mamhoff/do-not-discard-variant-prices-on-variant-discard

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

@@ -33,7 +33,7 @@
         <%= f.field_container :price do %>
           <%= f.label :price, class: Spree::Config.require_master_price ? 'required' : '' %>
 
-          <% if f.object.new_record? || f.object.has_default_price? %>
+          <% if (f.object.new_record? || f.object.has_default_price?) && !f.object.discarded? %>
             <%= render "spree/admin/shared/number_with_currency",
                        f: f,
                        amount_attr: :price,

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

@@ -16,10 +16,12 @@ def self.default_price_attributes
 
     # Returns `#prices` prioritized for being considered as default price
     #
+    # @deprecated
     # @return [ActiveRecord::Relation<Spree::Price>]
     def currently_valid_prices
       prices.currently_valid
     end
+    deprecate :currently_valid_prices, deprecator: Spree::Deprecation
 
     # Returns {#default_price} or builds it from {Spree::Variant.default_price_attributes}
     #
@@ -33,7 +35,7 @@ def default_price_or_build
     # 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.
+    # are taken into account.
     #
     # 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
@@ -44,37 +46,11 @@ def default_price_or_build
     # @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
-        )
-      )
+      price_selector.price_for_options(Spree::Config.default_pricing_options)
     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/variant.rb

@@ -23,7 +23,6 @@ class Variant < Spree::Base
     after_discard do
       stock_items.discard_all
       images.destroy_all
-      prices.discard_all
     end
 
     attr_writer :rebuild_vat_prices
@@ -52,6 +51,7 @@ class Variant < Spree::Base
     has_many :images, -> { order(:position) }, as: :viewable, dependent: :destroy, class_name: "Spree::Image"
 
     has_many :prices,
+      -> { with_discarded },
       class_name: 'Spree::Price',
       dependent: :destroy,
       inverse_of: :variant,

core/app/models/spree/variant/price_selector.rb

@@ -39,12 +39,29 @@ def price_for(price_options)
       # @param [Spree::Variant::PricingOptions] price_options Pricing Options to abide by
       # @return [Spree::Price, nil] The most specific price for this set of pricing options.
       def price_for_options(price_options)
-        variant.currently_valid_prices.detect do |price|
+        sorted_prices_for(variant).detect do |price|
           (price.country_iso == price_options.desired_attributes[:country_iso] ||
            price.country_iso.nil?
           ) && price.currency == price_options.desired_attributes[:currency]
         end
       end
+
+      private
+
+      # Returns `#prices` prioritized for being considered as default price
+      #
+      # @return [Array<Spree::Price>]
+      def sorted_prices_for(variant)
+        variant.prices.select do |price|
+          variant.discarded? || price.kept?
+        end.sort_by do |price|
+          [
+            price.country_iso.nil? ? 0 : 1,
+            price.updated_at || Time.zone.now,
+            price.id || Float::INFINITY,
+          ]
+        end.reverse
+      end
     end
   end
 end

core/spec/helpers/products_helper_spec.rb

@@ -51,8 +51,8 @@ module Spree
         let(:currency) { 'JPY' }
 
         before do
-          variant
-          product.prices.update_all(currency: currency)
+          variant.prices.each { |p| p.update(currency: currency) }
+          product.master.prices.each { |p| p.update(currency: currency) }
         end
 
         context "when variant is more than master" do
@@ -92,8 +92,8 @@ module Spree
         let(:variant_price) { 150 }
 
         before do
-          variant
-          product.prices.update_all(currency: currency)
+          variant.prices.each { |p| p.update(currency: currency) }
+          product.master.prices.each { |p| p.update(currency: currency) }
         end
 
         it "should return the variant price if the price is different than master" do

core/spec/models/spree/variant_spec.rb

@@ -257,12 +257,20 @@
       expect(variant.default_price.attributes).to eq(price.attributes)
     end
 
-    it 'includes discarded prices' do
-      variant = create(:variant)
-      price = create(:price, variant: variant, currency: 'USD')
-      price.discard
+    context "when the variant and the price are both soft-deleted" do
+      it "will use a deleted price as the default price" do
+        variant = create(:variant, deleted_at: 1.day.ago)
+        variant.prices.each { |price| price.discard }
+        expect(variant.reload.price).to be_present
+      end
+    end
 
-      expect(variant.default_price).to eq(price)
+    context "when the variant is not soft-deleted, but its price is" do
+      it "will not use a deleted price as the default price" do
+        variant = create(:variant)
+        variant.prices.each { |price| price.discard }
+        expect(variant.reload.price).not_to be_present
+      end
     end
   end
 
@@ -308,6 +316,12 @@
   end
 
   describe '#currently_valid_prices' do
+    around do |example|
+      Spree::Deprecation.silence do
+        example.run
+      end
+    end
+
     it 'returns prioritized prices' do
       price_1 = create(:price, country: create(:country))
       price_2 = create(:price, country: nil)
@@ -754,7 +768,7 @@
   end
 
   describe "#discard" do
-    it "discards related associations" do
+    it "discards related stock items and images, but not prices" do
       variant.images = [create(:image)]
 
       expect(variant.stock_items).not_to be_empty
@@ -765,16 +779,16 @@
 
       expect(variant.images).to be_empty
       expect(variant.stock_items.reload).to be_empty
-      expect(variant.prices).to be_empty
+      expect(variant.prices).not_to be_empty
     end
 
     describe 'default_price' do
       let!(:previous_variant_price) { variant.default_price }
 
-      it "should discard default_price" do
+      it "should not discard default_price" do
         variant.discard
         variant.reload
-        expect(previous_variant_price.reload).to be_discarded
+        expect(previous_variant_price.reload).not_to be_discarded
       end
 
       it "should keep its price if deleted" do