diff --git a/backend/app/views/spree/admin/products/_form.html.erb b/backend/app/views/spree/admin/products/_form.html.erb index 5286d689e00..793221ae7db 100644 --- a/backend/app/views/spree/admin/products/_form.html.erb +++ b/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, diff --git a/core/app/models/concerns/spree/default_price.rb b/core/app/models/concerns/spree/default_price.rb index d6b130c7c36..f638d54bb84 100644 --- a/core/app/models/concerns/spree/default_price.rb +++ b/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] 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 diff --git a/core/app/models/spree/variant.rb b/core/app/models/spree/variant.rb index 17456a2061d..b9fcaa81b71 100644 --- a/core/app/models/spree/variant.rb +++ b/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, diff --git a/core/app/models/spree/variant/price_selector.rb b/core/app/models/spree/variant/price_selector.rb index c0a645a42b4..cb9566f2e86 100644 --- a/core/app/models/spree/variant/price_selector.rb +++ b/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] + 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 diff --git a/core/spec/helpers/products_helper_spec.rb b/core/spec/helpers/products_helper_spec.rb index bfe0cc9fcd8..2edbb51abe8 100644 --- a/core/spec/helpers/products_helper_spec.rb +++ b/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 diff --git a/core/spec/models/spree/variant_spec.rb b/core/spec/models/spree/variant_spec.rb index 49706375286..40546ea5542 100644 --- a/core/spec/models/spree/variant_spec.rb +++ b/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 diff --git a/core/spec/support/concerns/default_price.rb b/core/spec/support/concerns/default_price.rb deleted file mode 100644 index 2972fa81542..00000000000 --- a/core/spec/support/concerns/default_price.rb +++ /dev/null @@ -1,58 +0,0 @@ -# frozen_string_literal: true - -RSpec.shared_examples_for "default_price" do - let(:model) { described_class } - subject(:instance) { FactoryBot.build(model.name.demodulize.downcase.to_sym) } - - describe '.has_one :default_price' do - let(:default_price_association) { model.reflect_on_association(:default_price) } - - it 'should be a has one association' do - expect(default_price_association.macro).to eql :has_one - end - - it 'should have a dependent destroy' do - expect(default_price_association.options[:dependent]).to eql :destroy - end - - it 'should have the class name of Spree::Price' do - expect(default_price_association.options[:class_name]).to eql 'Spree::Price' - end - end - - describe '#default_price' do - subject { instance.default_price } - - describe '#class' do - subject { super().class } - it { is_expected.to eql Spree::Price } - end - end - - describe '#has_default_price?' do - subject { super().has_default_price? } - it { is_expected.to be_truthy } - - context 'when default price is discarded' do - before do - instance.default_price.discard - end - - 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