diff --git a/api/app/controllers/spree/api/base_controller.rb b/api/app/controllers/spree/api/base_controller.rb index 5b4676ef249..6feef99b139 100644 --- a/api/app/controllers/spree/api/base_controller.rb +++ b/api/app/controllers/spree/api/base_controller.rb @@ -139,7 +139,7 @@ def product_scope end def variants_associations - [{ option_values: :option_type }, :default_price, :images] + [{ option_values: :option_type }, :prices, :images] end def product_includes diff --git a/api/app/controllers/spree/api/shipments_controller.rb b/api/app/controllers/spree/api/shipments_controller.rb index 0a6b20facdb..fbaa793ac22 100644 --- a/api/app/controllers/spree/api/shipments_controller.rb +++ b/api/app/controllers/spree/api/shipments_controller.rb @@ -186,7 +186,7 @@ def mine_includes }, variant: { product: {}, - default_price: {}, + prices: {}, option_values: { option_type: {} } diff --git a/api/app/controllers/spree/api/taxons_controller.rb b/api/app/controllers/spree/api/taxons_controller.rb index c178de1abdf..70da15d8645 100644 --- a/api/app/controllers/spree/api/taxons_controller.rb +++ b/api/app/controllers/spree/api/taxons_controller.rb @@ -69,7 +69,7 @@ def products # Products#index does not do the sorting. taxon = Spree::Taxon.find(params[:id]) @products = paginate(taxon.products.ransack(params[:q]).result) - @products = @products.includes(master: :default_price) + @products = @products.includes(master: :prices) if params[:simple] @exclude_data = { diff --git a/api/app/controllers/spree/api/variants_controller.rb b/api/app/controllers/spree/api/variants_controller.rb index c03f6147a09..28d049fa224 100644 --- a/api/app/controllers/spree/api/variants_controller.rb +++ b/api/app/controllers/spree/api/variants_controller.rb @@ -83,7 +83,7 @@ def variant_params end def include_list - [{ option_values: :option_type }, :product, :default_price, :images, { stock_items: :stock_location }] + [{ option_values: :option_type }, :product, :prices, :images, { stock_items: :stock_location }] end end end diff --git a/backend/app/controllers/spree/admin/products_controller.rb b/backend/app/controllers/spree/admin/products_controller.rb index 4bee5629044..519f4fc87b1 100644 --- a/backend/app/controllers/spree/admin/products_controller.rb +++ b/backend/app/controllers/spree/admin/products_controller.rb @@ -104,7 +104,7 @@ def update_before end def product_includes - [:variant_images, { variants: [:images], master: [:images, :default_price] }] + [:variant_images, { variants: [:images], master: [:images, :prices] }] end def clone_object_url(resource) diff --git a/backend/app/controllers/spree/admin/variants_controller.rb b/backend/app/controllers/spree/admin/variants_controller.rb index 8c1ff0861ce..149be67f70c 100644 --- a/backend/app/controllers/spree/admin/variants_controller.rb +++ b/backend/app/controllers/spree/admin/variants_controller.rb @@ -16,7 +16,7 @@ def new_before @object.attributes = @object.product.master.attributes.except('id', 'created_at', 'deleted_at', 'sku', 'is_master') # Shallow Clone of the default price to populate the price field. - @object.default_price = @object.product.master.default_price.clone + @object.prices.build(@object.product.master.default_price.attributes.except("id", "created_at", "updated_at", "deleted_at")) end def collection @@ -35,7 +35,7 @@ def load_data end def variant_includes - [{ option_values: :option_type }, :default_price] + [{ option_values: :option_type }, :prices] end def redirect_on_empty_option_values diff --git a/backend/app/views/spree/admin/products/index.html.erb b/backend/app/views/spree/admin/products/index.html.erb index 3cab1ef3d33..852a0622db5 100644 --- a/backend/app/views/spree/admin/products/index.html.erb +++ b/backend/app/views/spree/admin/products/index.html.erb @@ -78,7 +78,7 @@ <%= render 'spree/admin/shared/image', image: product.gallery.images.first, size: :mini %> <%= link_to product.try(:name), edit_admin_product_path(product) %> - <%= product.display_price.to_html %> + <%= product.display_price&.to_html %> <%= link_to_edit product, no_text: true, class: 'edit' if can?(:edit, product) && !product.deleted? %>   diff --git a/backend/app/views/spree/admin/variants/_form.html.erb b/backend/app/views/spree/admin/variants/_form.html.erb index e09fbaff561..4e03535dbed 100644 --- a/backend/app/views/spree/admin/variants/_form.html.erb +++ b/backend/app/views/spree/admin/variants/_form.html.erb @@ -65,7 +65,7 @@
<%= f.label :price %> - <%= render "spree/admin/shared/number_with_currency", f: f, amount_attr: :price, currency: @variant.find_or_build_default_price.currency %> + <%= render "spree/admin/shared/number_with_currency", f: f, amount_attr: :price, currency: @variant.default_price_or_build.currency %>
diff --git a/core/app/models/concerns/spree/default_price.rb b/core/app/models/concerns/spree/default_price.rb index 20d11bd4bbc..d6b130c7c36 100644 --- a/core/app/models/concerns/spree/default_price.rb +++ b/core/app/models/concerns/spree/default_price.rb @@ -5,12 +5,13 @@ module DefaultPrice extend ActiveSupport::Concern included do - has_one :default_price, - -> { with_discarded.currently_valid.with_default_attributes }, - class_name: 'Spree::Price', - inverse_of: :variant, - dependent: :destroy, - autosave: true + delegate :display_price, :display_amount, :price, to: :default_price, allow_nil: true + delegate :price=, to: :default_price_or_build + + # @see Spree::Variant::PricingOptions.default_price_attributes + def self.default_price_attributes + Spree::Config.default_pricing_options.desired_attributes + end end # Returns `#prices` prioritized for being considered as default price @@ -20,15 +21,60 @@ def currently_valid_prices prices.currently_valid end - def find_or_build_default_price - default_price || build_default_price(Spree::Config.default_pricing_options.desired_attributes) + # Returns {#default_price} or builds it from {Spree::Variant.default_price_attributes} + # + # @return [Spree::Price, nil] + # @see Spree::Variant.default_price_attributes + def default_price_or_build + default_price || + prices.build(self.class.default_price_attributes) end - delegate :display_price, :display_amount, :price, to: :find_or_build_default_price - delegate :price=, to: :find_or_build_default_price + # 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. + # + # 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 + # found, non-persisted records take preference. When more than one persisted + # candidate exists, the one most recently updated is taken or, in case of + # race condition, the one with higher id. + # + # @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 + ) + ) + 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/product.rb b/core/app/models/spree/product.rb index 886d911f4e3..ad8ab33d329 100644 --- a/core/app/models/spree/product.rb +++ b/core/app/models/spree/product.rb @@ -60,6 +60,17 @@ class Product < Spree::Base has_many :line_items, through: :variants_including_master has_many :orders, through: :line_items + scope :sort_by_master_default_price_amount_asc, -> { + with_default_price.order('spree_prices.amount ASC') + } + scope :sort_by_master_default_price_amount_desc, -> { + with_default_price.order('spree_prices.amount DESC') + } + scope :with_default_price, -> { + left_joins(master: :prices) + .where(master: { spree_prices: Spree::Config.default_pricing_options.desired_attributes }) + } + def find_or_build_master master || build_master end diff --git a/core/app/models/spree/product/scopes.rb b/core/app/models/spree/product/scopes.rb index f2091a8add5..cb9c6483b45 100644 --- a/core/app/models/spree/product/scopes.rb +++ b/core/app/models/spree/product/scopes.rb @@ -29,25 +29,25 @@ def self.property_conditions(property) scope :descend_by_name, -> { order(name: :desc) } add_search_scope :ascend_by_master_price do - joins(master: :default_price).select('spree_products.* , spree_prices.amount') + joins(master: :prices).select('spree_products.* , spree_prices.amount') .order(Spree::Price.arel_table[:amount].asc) end add_search_scope :descend_by_master_price do - joins(master: :default_price).select('spree_products.* , spree_prices.amount') + joins(master: :prices).select('spree_products.* , spree_prices.amount') .order(Spree::Price.arel_table[:amount].desc) end add_search_scope :price_between do |low, high| - joins(master: :default_price).where(Price.table_name => { amount: low..high }) + joins(master: :prices).where(Price.table_name => { amount: low..high }) end add_search_scope :master_price_lte do |price| - joins(master: :default_price).where("#{price_table_name}.amount <= ?", price) + joins(master: :prices).where("#{price_table_name}.amount <= ?", price) end add_search_scope :master_price_gte do |price| - joins(master: :default_price).where("#{price_table_name}.amount >= ?", price) + joins(master: :prices).where("#{price_table_name}.amount >= ?", price) end # This scope selects products in taxon AND all its descendants diff --git a/core/lib/spree/core/product_filters.rb b/core/lib/spree/core/product_filters.rb index 8480736dd9d..d22bd0ba6ae 100644 --- a/core/lib/spree/core/product_filters.rb +++ b/core/lib/spree/core/product_filters.rb @@ -61,7 +61,7 @@ module ProductFilters scope = scope.or(new_scope) end - Spree::Product.joins(master: :default_price).where(scope) + Spree::Product.joins(master: :prices).where(scope) end def self.format_price(amount) diff --git a/core/spec/models/spree/product_spec.rb b/core/spec/models/spree/product_spec.rb index cac6c32b703..01ec66c0276 100644 --- a/core/spec/models/spree/product_spec.rb +++ b/core/spec/models/spree/product_spec.rb @@ -151,8 +151,7 @@ class Extension < Spree::Base context "with currency set to JPY" do before do - product.master.default_price.currency = 'JPY' - product.master.default_price.save! + product.master.default_price.update!(currency: 'JPY') stub_spree_preferences(currency: 'JPY') end @@ -619,4 +618,26 @@ class Extension < Spree::Base expect(subject).to respond_to(:images) end end + + describe '.sort_by_master_default_price_amount_asc' do + it 'returns first those which default price is lower' do + product_1 = create(:product, price: 10) + product_2 = create(:product, price: 5) + + result = described_class.sort_by_master_default_price_amount_asc + + expect(result).to eq([product_2, product_1]) + end + end + + describe '.sort_by_master_default_price_amount_desc' do + it 'returns first those which default price is higher' do + product_1 = create(:product, price: 10) + product_2 = create(:product, price: 5) + + result = described_class.sort_by_master_default_price_amount_desc + + expect(result).to eq([product_1, product_2]) + end + end end diff --git a/core/spec/models/spree/variant_spec.rb b/core/spec/models/spree/variant_spec.rb index d8c4bcef4f5..49706375286 100644 --- a/core/spec/models/spree/variant_spec.rb +++ b/core/spec/models/spree/variant_spec.rb @@ -7,8 +7,6 @@ let!(:variant) { create(:variant) } - it_behaves_like 'default_price' - describe 'delegates' do let(:product) { build(:product) } let(:variant) { build(:variant, product: product) } @@ -26,7 +24,7 @@ context "validations" do it "should validate price is greater than 0" do - variant.price = -1 + variant = build(:variant, price: -1) expect(variant).to be_invalid end @@ -212,42 +210,125 @@ end end - context "#cost_currency" do - context "when cost currency is nil" do - before { variant.cost_currency = nil } - it "populates cost currency with the default value on save" do - variant.save! - expect(variant.cost_currency).to eql "USD" - end - end - end - - context "#default_price" do + describe '#default_price' do context "when multiple prices are present in addition to a default price" do let(:pricing_options_germany) { Spree::Config.pricing_options_class.new(currency: "EUR") } let(:pricing_options_united_states) { Spree::Config.pricing_options_class.new(currency: "USD") } + before do variant.prices.create(currency: "EUR", amount: 29.99) variant.reload end - it "the default stays the same" do + it 'the default stays the same' do expect(variant.default_price.amount).to eq(19.99) end - it "displays default price" do + it 'displays default price' do expect(variant.price_for_options(pricing_options_united_states).money.to_s).to eq("$19.99") expect(variant.price_for_options(pricing_options_germany).money.to_s).to eq("€29.99") end end - context "when adding multiple prices" do - it "it can reassign a default price" do + context 'when adding multiple prices' do + it 'it can reassign a default price' do expect(variant.default_price.amount).to eq(19.99) variant.prices.create(currency: "USD", amount: 12.12) expect(variant.reload.default_price.amount).to eq(12.12) end end + + it 'prioritizes prices recently updated' do + variant = create(:variant) + price = create(:price, variant: variant, currency: 'USD') + create(:price, variant: variant, currency: 'USD') + price.touch + variant.prices.reload + + expect(variant.default_price).to eq(price) + end + + it 'prioritizes non-persisted prices' do + variant = create(:variant) + price = build(:price, currency: 'USD', amount: 1, variant_id: variant.id) + variant.prices.build(price.attributes) + create(:price, variant: variant, currency: 'USD', amount: 2) + + 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 + context 'when default price exists' do + it 'returns it' do + price = build(:price, currency: 'USD') + variant = build(:variant, prices: [price]) + + expect(variant.default_price_or_build).to eq(price) + end + end + + context 'when default price does not exist' do + it 'builds and returns it' do + variant = build(:variant, prices: [], price: nil) + + expect( + variant.default_price_or_build.attributes.values_at(*described_class.default_price_attributes.keys.map(&:to_s)) + ).to eq(described_class.default_price_attributes.values) + end + end + end + + describe '#has_default_price?' do + context 'when default price is present and not discarded' do + it 'returns true' do + variant = create(:variant, price: 20) + + expect(variant.has_default_price?).to be(true) + end + end + + context 'when default price is discarded' do + it 'returns false' do + variant = create(:variant, price: 20) + + variant.default_price.discard + + expect(variant.has_default_price?).to be(false) + end + 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 + + context "#cost_currency" do + context "when cost currency is nil" do + before { variant.cost_currency = nil } + it "populates cost currency with the default value on save" do + variant.save! + expect(variant.cost_currency).to eql "USD" + end + end end context "#price_selector" do @@ -680,6 +761,7 @@ expect(variant.prices).not_to be_empty variant.discard + variant.reload expect(variant.images).to be_empty expect(variant.stock_items.reload).to be_empty @@ -687,25 +769,25 @@ end describe 'default_price' do - let!(:previous_variant_price) { variant.display_price } + let!(:previous_variant_price) { variant.default_price } it "should discard default_price" do variant.discard variant.reload - expect(variant.default_price).to be_discarded + expect(previous_variant_price.reload).to be_discarded end it "should keep its price if deleted" do variant.discard variant.reload - expect(variant.display_price).to eq(previous_variant_price) + expect(variant.default_price).to eq(previous_variant_price) end context 'when loading with pre-fetching of default_price' do it 'also keeps the previous price' do variant.discard - reloaded_variant = Spree::Variant.with_discarded.includes(:default_price).find_by(id: variant.id) - expect(reloaded_variant.display_price).to eq(previous_variant_price) + reloaded_variant = Spree::Variant.with_discarded.find_by(id: variant.id) + expect(reloaded_variant.default_price).to eq(previous_variant_price) end end end