From 5d8531d5076e4094338c6c32a992005f2094874e Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Fri, 23 Sep 2022 16:12:36 +0200 Subject: [PATCH 01/10] Do not discard prices upon variant deletion When we soft-delete a variant, we also soft-delete the variant's prices. In my opinion, this is not necessary, as the association between prices and variants includes discarded prices, and harmful, as it leads to price inconsistencies between a variant before discarding and after undiscarding (if the variant has a newer, but discarded, price, we have no way of knowing that that price should not be un-discarded along with the variant). --- core/app/models/concerns/spree/default_price.rb | 4 ++-- core/app/models/spree/variant.rb | 1 - core/spec/models/spree/variant_spec.rb | 16 ++++------------ 3 files changed, 6 insertions(+), 15 deletions(-) diff --git a/core/app/models/concerns/spree/default_price.rb b/core/app/models/concerns/spree/default_price.rb index d6b130c7c36..afb079bb542 100644 --- a/core/app/models/concerns/spree/default_price.rb +++ b/core/app/models/concerns/spree/default_price.rb @@ -33,7 +33,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 @@ -46,7 +46,7 @@ def default_price_or_build def default_price prioritized_default( prices_meeting_criteria_to_be_default( - (prices + prices.with_discarded).uniq + prices ) ) end diff --git a/core/app/models/spree/variant.rb b/core/app/models/spree/variant.rb index affebf0fc3c..a5bdc8f0325 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 diff --git a/core/spec/models/spree/variant_spec.rb b/core/spec/models/spree/variant_spec.rb index c8f600b20d8..d1cd0d9492d 100644 --- a/core/spec/models/spree/variant_spec.rb +++ b/core/spec/models/spree/variant_spec.rb @@ -295,14 +295,6 @@ 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 - - expect(variant.default_price).to eq(price) - end end describe '#default_price_or_build' do @@ -793,7 +785,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 @@ -804,16 +796,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 From 761192332f19288c0afab435934ed45e28dc50ae Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Fri, 23 Sep 2022 17:41:29 +0200 Subject: [PATCH 02/10] Sort `currently_valid_prices` in Ruby Using the `currently_valid` scope in this instance method leads to an "n+1" issue when getting prices for more than one variant. By sorting these in memory rather than in the database, we can save a lot of database round-trips. --- core/app/models/concerns/spree/default_price.rb | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/core/app/models/concerns/spree/default_price.rb b/core/app/models/concerns/spree/default_price.rb index afb079bb542..2d079ba6e47 100644 --- a/core/app/models/concerns/spree/default_price.rb +++ b/core/app/models/concerns/spree/default_price.rb @@ -16,9 +16,15 @@ def self.default_price_attributes # Returns `#prices` prioritized for being considered as default price # - # @return [ActiveRecord::Relation] + # @return [Array] def currently_valid_prices - prices.currently_valid + prices.sort_by do |price| + [ + price.country_iso.nil? ? 0 : 1, + price.updated_at || Time.zone.now, + price.id || Float::INFINITY, + ] + end.reverse end # Returns {#default_price} or builds it from {Spree::Variant.default_price_attributes} From cb89fc46054901727aa92d23e82a3a1fdb6d3034 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Fri, 23 Sep 2022 17:43:30 +0200 Subject: [PATCH 03/10] Use `currently_valid_prices` method for detecting default price This uses the redefined public method from the previous commit to detect the default price. Note that through using `reverse_each.detect` rather than `min` with a block we should be able to save some iterations. --- .../models/concerns/spree/default_price.rb | 28 ++----------------- 1 file changed, 3 insertions(+), 25 deletions(-) diff --git a/core/app/models/concerns/spree/default_price.rb b/core/app/models/concerns/spree/default_price.rb index 2d079ba6e47..d94fd0d2d2b 100644 --- a/core/app/models/concerns/spree/default_price.rb +++ b/core/app/models/concerns/spree/default_price.rb @@ -50,37 +50,15 @@ 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 - ) - ) - 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| + currently_valid_prices.detect 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 + def has_default_price? + default_price.present? && !default_price.discarded? end end end From bb132ee0b6a887054696c2fd7a714682ed9b6c33 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Fri, 23 Sep 2022 19:13:04 +0200 Subject: [PATCH 04/10] Fix products helper spec setup --- core/spec/helpers/products_helper_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 From 46fe34d465d1bdbc3e6d274aa28a423080901959 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Sat, 24 Sep 2022 11:06:39 +0200 Subject: [PATCH 05/10] Do not render price form for discarded products This was the previous behavior. --- backend/app/views/spree/admin/products/_form.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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, From 5f4e3cbc6c7f418b064259d4c947dee1e5325c5b Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 5 Oct 2022 12:01:24 +0200 Subject: [PATCH 06/10] Sort prices in-memory in Variant::PriceSelector We don't need to be using the weirdly named `currently_valid_prices` method. As this is the only spot in core where we use that method, we can now deprecate it. --- core/app/models/spree/variant/price_selector.rb | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/core/app/models/spree/variant/price_selector.rb b/core/app/models/spree/variant/price_selector.rb index c0a645a42b4..2cc76dad715 100644 --- a/core/app/models/spree/variant/price_selector.rb +++ b/core/app/models/spree/variant/price_selector.rb @@ -39,12 +39,27 @@ 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.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 From 23cff1ce4a0acdab8836ec28d1ae75a8045ea9d4 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 5 Oct 2022 12:18:56 +0200 Subject: [PATCH 07/10] Return to original `currently_valid_prices` and deprecate it --- core/app/models/concerns/spree/default_price.rb | 12 ++++-------- core/spec/models/spree/variant_spec.rb | 6 ++++++ 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/core/app/models/concerns/spree/default_price.rb b/core/app/models/concerns/spree/default_price.rb index d94fd0d2d2b..6f6451e8e99 100644 --- a/core/app/models/concerns/spree/default_price.rb +++ b/core/app/models/concerns/spree/default_price.rb @@ -16,16 +16,12 @@ def self.default_price_attributes # Returns `#prices` prioritized for being considered as default price # - # @return [Array] + # @deprecated + # @return [ActiveRecord::Relation] def currently_valid_prices - prices.sort_by do |price| - [ - price.country_iso.nil? ? 0 : 1, - price.updated_at || Time.zone.now, - price.id || Float::INFINITY, - ] - end.reverse + prices.currently_valid end + deprecate :currently_valid_prices, deprecator: Spree::Deprecation # Returns {#default_price} or builds it from {Spree::Variant.default_price_attributes} # diff --git a/core/spec/models/spree/variant_spec.rb b/core/spec/models/spree/variant_spec.rb index d1cd0d9492d..f3d58eb01de 100644 --- a/core/spec/models/spree/variant_spec.rb +++ b/core/spec/models/spree/variant_spec.rb @@ -339,6 +339,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) From afbf64f80358c59defe808065eea1fdb61c8e5b9 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 5 Oct 2022 12:19:25 +0200 Subject: [PATCH 08/10] Remove unused shared examples for default price --- core/spec/support/concerns/default_price.rb | 58 --------------------- 1 file changed, 58 deletions(-) delete mode 100644 core/spec/support/concerns/default_price.rb 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 From e273da99b35793bd7913c3b7f3701986885af1c2 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 5 Oct 2022 12:19:44 +0200 Subject: [PATCH 09/10] Use Price Selector in Spree::Variant#default_price This uses the price selector in order to find the default price rather than having to re-implement the default price selector's logic. --- core/app/models/concerns/spree/default_price.rb | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/core/app/models/concerns/spree/default_price.rb b/core/app/models/concerns/spree/default_price.rb index 6f6451e8e99..f638d54bb84 100644 --- a/core/app/models/concerns/spree/default_price.rb +++ b/core/app/models/concerns/spree/default_price.rb @@ -46,11 +46,7 @@ def default_price_or_build # @return [Spree::Price, nil] # @see Spree::Variant.default_price_attributes def default_price - criteria = self.class.default_price_attributes.transform_keys(&:to_s) - currently_valid_prices.detect do |price| - contender = price.attributes.slice(*criteria.keys) - criteria == contender - end + price_selector.price_for_options(Spree::Config.default_pricing_options) end def has_default_price? From 3f4578aba47fbfdebcba7cbdcb84507f142ae93d Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Thu, 6 Oct 2022 09:43:49 +0200 Subject: [PATCH 10/10] Maintain prices for previously soft-deleted variants Previous Solidus behavior included soft-deleted prices when looking at the default price. The intention of this behavior was that a a variant that is soft-deleted, and has its prices soft-deleted as a consequence, still has a `default_price` record. However, for a `kept` variant, we do not want to see a discarded `default_price` - otherwise what sense does it make to even add the delete action to the price? Especially given that deletion touches the variant and will then move it forward in the array of sorted prices to be considered for a given set of pricing attributes. This commit changes the `prices` association to include discarded prices, but then filters out discarded prices for non-discarded variants in the price selector. --- core/app/models/spree/variant.rb | 1 + core/app/models/spree/variant/price_selector.rb | 4 +++- core/spec/models/spree/variant_spec.rb | 16 ++++++++++++++++ 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/core/app/models/spree/variant.rb b/core/app/models/spree/variant.rb index a5bdc8f0325..0e2f54972c4 100644 --- a/core/app/models/spree/variant.rb +++ b/core/app/models/spree/variant.rb @@ -51,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 2cc76dad715..cb9566f2e86 100644 --- a/core/app/models/spree/variant/price_selector.rb +++ b/core/app/models/spree/variant/price_selector.rb @@ -52,7 +52,9 @@ def price_for_options(price_options) # # @return [Array] def sorted_prices_for(variant) - variant.prices.sort_by do |price| + 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, diff --git a/core/spec/models/spree/variant_spec.rb b/core/spec/models/spree/variant_spec.rb index f3d58eb01de..05f06cf00d2 100644 --- a/core/spec/models/spree/variant_spec.rb +++ b/core/spec/models/spree/variant_spec.rb @@ -295,6 +295,22 @@ expect(variant.default_price.attributes).to eq(price.attributes) end + + 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 + + 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 describe '#default_price_or_build' do