From 2d4c6d335154cd3afe0759dccfb121e9c0d53d9d 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 97408a1a1c8..f15baebd2f9 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 0885fc9d51b..09db12b542f 100644 --- a/core/spec/models/spree/variant_spec.rb +++ b/core/spec/models/spree/variant_spec.rb @@ -261,14 +261,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 @@ -759,7 +751,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 @@ -770,16 +762,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 f21a93b936abf1b89e276db64f2b66da57833a3d 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 a2a45f1ed3678fa60956cff8dc117fc5aed92ab9 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 8e04445020c31c0e09b1474c3b623c46d08fe54c 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 6033c9bfe5313a1bbd374638fd69985582a6e978 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 876da69b14b258b12e7b2fe3b60b0c658326e940 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 6046f7c2a69822fe67f2e72e54db6f889558757d 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 09db12b542f..36fe926d2df 100644 --- a/core/spec/models/spree/variant_spec.rb +++ b/core/spec/models/spree/variant_spec.rb @@ -305,6 +305,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 198272df926d3c8e58f9515d182f3614bc55a9f6 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 faeba544d66e9a0474b4e6c52fd81e81f6929f1a 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 a920d3f19008be26423663a0a69deb90e692f7fb 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 f15baebd2f9..c0dfd03dc6e 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 36fe926d2df..5517da5839d 100644 --- a/core/spec/models/spree/variant_spec.rb +++ b/core/spec/models/spree/variant_spec.rb @@ -261,6 +261,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