From 1a71840dc8b4c0fb718f94d79c11762191956414 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 17456a2061d..0e33d6900a1 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 49706375286..d7727d11d21 100644 --- a/core/spec/models/spree/variant_spec.rb +++ b/core/spec/models/spree/variant_spec.rb @@ -256,14 +256,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 @@ -754,7 +746,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 +757,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 381e7f25433be22ca965905b6e20f11d0d698384 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 21843cb8b7d65a077ec2467e2e725c73228dc91e 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 92f4be9f1de7e1d32a0ad738328cb1eaa2b5635b 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 659b8b35f5167311905784440595546932fc4d2f 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 3f91885bacc58b22ef2b88a84437489097e7110a 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 f0ae7c507eff130b0ac61288c25dea0bd196e7a2 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 d7727d11d21..244b1715f12 100644 --- a/core/spec/models/spree/variant_spec.rb +++ b/core/spec/models/spree/variant_spec.rb @@ -300,6 +300,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 99f5e9da000d244ea505eff7da99f360a1a66e01 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 34b3f9ad815bae30bd3eef417a3f4f45e14854ae 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 d84b833c2bf588eeb310431721e851762b3deda9 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 0e33d6900a1..b9fcaa81b71 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 244b1715f12..40546ea5542 100644 --- a/core/spec/models/spree/variant_spec.rb +++ b/core/spec/models/spree/variant_spec.rb @@ -256,6 +256,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