From 69ebbfc8ea34b836dec7d85ad69d2acd1d8e717e Mon Sep 17 00:00:00 2001 From: Mike Conlin Date: Wed, 24 Mar 2021 15:51:59 -0700 Subject: [PATCH 1/3] Add price_for_options and deprecate price_for The price_for method does not return a Spree::Price as described. Instead it does some frontend work and returns a Spree::Money, limiting the usability of this method, as the price record itself can have useful information on it. This deprecates the existing method in favor of one that returns a Spree::Price. --- .../models/spree/variant/price_selector.rb | 19 ++++++++-- .../spree/variant/price_selector_spec.rb | 37 ++++++++++++++----- 2 files changed, 44 insertions(+), 12 deletions(-) diff --git a/core/app/models/spree/variant/price_selector.rb b/core/app/models/spree/variant/price_selector.rb index c6c4f57fa1f..c0a645a42b4 100644 --- a/core/app/models/spree/variant/price_selector.rb +++ b/core/app/models/spree/variant/price_selector.rb @@ -26,11 +26,24 @@ def initialize(variant) # @param [Spree::Variant::PricingOptions] price_options Pricing Options to abide by # @return [Spree::Money, nil] The most specific price for this set of pricing options. def price_for(price_options) + Spree::Deprecation.warn( + "price_for is deprecated and will be removed. The price_for method + should return a Spree::Price as described. Please use + #price_for_options and adjust your frontend code to explicitly call + &.money where required" + ) + price_for_options(price_options)&.money + end + + # The variant's Spree::Price record, given a set of pricing 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| - ( price.country_iso == price_options.desired_attributes[:country_iso] || - price.country_iso.nil? + (price.country_iso == price_options.desired_attributes[:country_iso] || + price.country_iso.nil? ) && price.currency == price_options.desired_attributes[:currency] - end.try!(:money) + end end end end diff --git a/core/spec/models/spree/variant/price_selector_spec.rb b/core/spec/models/spree/variant/price_selector_spec.rb index 161a95ac714..fe0dfca5717 100644 --- a/core/spec/models/spree/variant/price_selector_spec.rb +++ b/core/spec/models/spree/variant/price_selector_spec.rb @@ -9,6 +9,7 @@ it { is_expected.to respond_to(:variant) } it { is_expected.to respond_to(:price_for) } + it { is_expected.to respond_to(:price_for_options) } describe ".pricing_options_class" do it "returns the standard pricing options class" do @@ -18,12 +19,28 @@ describe "#price_for(options)" do let(:variant) { create(:variant, price: 12.34) } + let(:pricing_options) { described_class.pricing_options_class.new(currency: "USD") } + + it "returns the correct (default) price as a Spree::Money object", :aggregate_failures do + expect(Spree::Deprecation).to receive(:warn). + with(/^price_for is deprecated and will be removed/, any_args) + expect(subject.price_for(pricing_options)).to eq(Spree::Money.new(12.34, currency: "USD")) + end + end + + describe "#price_for_options(options)" do + let(:variant) { create(:variant, price: 12.34) } context "with the default currency" do let(:pricing_options) { described_class.pricing_options_class.new(currency: "USD") } - it "returns the correct (default) price as a Spree::Money object" do - expect(subject.price_for(pricing_options)).to eq(Spree::Money.new(12.34, currency: "USD")) + it "returns the price as a Spree::Price object" do + expect(subject.price_for_options(pricing_options)).to be_a Spree::Price + end + + it "returns the correct (default) price", :aggregate_failures do + expect(subject.price_for_options(pricing_options).amount).to eq 12.34 + expect(subject.price_for_options(pricing_options).currency).to eq "USD" end context "with the another country iso" do @@ -38,8 +55,9 @@ variant.prices.create(amount: 44.44, country: country, currency: Spree::Config.currency) end - it "returns the correct price" do - expect(subject.price_for(pricing_options)).to eq(Spree::Money.new(44.44, currency: "USD")) + it "returns the correct price and currency", :aggregate_failures do + expect(subject.price_for_options(pricing_options).amount).to eq 44.44 + expect(subject.price_for_options(pricing_options).currency).to eq "USD" end end @@ -50,7 +68,7 @@ end it "returns nil" do - expect(subject.price_for(pricing_options)).to be_nil + expect(subject.price_for_options(pricing_options)).to be_nil end end @@ -60,7 +78,7 @@ end it "returns the fallback price" do - expect(subject.price_for(pricing_options)).to eq(Spree::Money.new(55.44, currency: "USD")) + expect(subject.price_for_options(pricing_options).amount).to eq 55.44 end end end @@ -75,14 +93,15 @@ variant.prices.create(amount: 99.00, currency: "EUR") end - it "returns the price in the correct currency as a Spree::Money object" do - expect(subject.price_for(pricing_options)).to eq(Spree::Money.new(99.00, currency: "EUR")) + it "returns the price in the correct currency", :aggregate_failures do + expect(subject.price_for_options(pricing_options).amount).to eq 99.00 + expect(subject.price_for_options(pricing_options).currency).to eq "EUR" end end context "when there is no price for that currency" do it "returns nil" do - expect(subject.price_for(pricing_options)).to be_nil + expect(subject.price_for_options(pricing_options)).to be_nil end end end From bd97dbe1a7c8086df33599a4ed37da28b9952018 Mon Sep 17 00:00:00 2001 From: Mike Conlin Date: Wed, 24 Mar 2021 16:07:10 -0700 Subject: [PATCH 2/3] Add custom price_for_options method to variant We need to safely ensure the new price_for_options method in the price selector will not break for those that have a custom price selector. If the new method is defined in the price selector class this will behave normally, and if their customization does not allow for it, we will instead return an unpersisted Spree::Price from the Spree::Money object that the deprecated price_for method returns. Spree::Product can now delegate price_for_options to master variant --- core/app/models/spree/product.rb | 1 + core/app/models/spree/variant.rb | 14 ++++++++++- core/spec/models/spree/variant_spec.rb | 35 ++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 1 deletion(-) diff --git a/core/app/models/spree/product.rb b/core/app/models/spree/product.rb index 6928b3f4af7..0e9abf9aaab 100644 --- a/core/app/models/spree/product.rb +++ b/core/app/models/spree/product.rb @@ -85,6 +85,7 @@ def find_or_build_master :has_default_price?, :images, :price_for, + :price_for_options, :rebuild_vat_prices=, to: :find_or_build_master diff --git a/core/app/models/spree/variant.rb b/core/app/models/spree/variant.rb index 591239bc914..9d400abc50d 100644 --- a/core/app/models/spree/variant.rb +++ b/core/app/models/spree/variant.rb @@ -276,8 +276,9 @@ def price_selector end # Chooses an appropriate price for the given pricing options + # This has been deprecated in favor of #price_for_options. # - # @see Spree::Variant::PriceSelector#price_for + # @see Spree::Variant::PriceSelector#price_for_options delegate :price_for, to: :price_selector # Returns the difference in price from the master variant @@ -293,6 +294,17 @@ def price_same_as_master?(pricing_options = Spree::Config.default_pricing_option diff && diff.zero? end + def price_for_options(price_options) + if price_selector.respond_to?(:price_for_options) + price_selector.price_for_options(price_options) + else + money = price_for(price_options) + return if money.nil? + + Spree::Price.new(amount: money.to_d, variant: self, currency: price_options.currency) + end + end + # Generates a friendly name and sku string. # # @return [String] diff --git a/core/spec/models/spree/variant_spec.rb b/core/spec/models/spree/variant_spec.rb index 2e1e5a39a12..6098de4b09f 100644 --- a/core/spec/models/spree/variant_spec.rb +++ b/core/spec/models/spree/variant_spec.rb @@ -269,6 +269,41 @@ expect(variant.price_selector).to receive(:price_for).with(price_options) variant.price_for(price_options) end + + it "returns a Spree::Money object with a deprecation warning", :aggregate_failures do + expect(Spree::Deprecation).to receive(:warn). + with(/^price_for is deprecated and will be removed/, any_args) + expect(variant.price_for(price_options)).to eq Spree::Money.new(19.99) + end + end + + context "#price_for_options(price_options)" do + subject { variant.price_for_options(price_options) } + + let(:price_options) { Spree::Config.variant_price_selector_class.pricing_options_class.new } + + it "delegates to the price_selector" do + expect(variant.price_selector).to receive(:price_for_options).with(price_options) + subject + end + + it "returns a Spree::Price object for the given pricing_options", :aggregate_failures do + expect(subject).to be_a Spree::Price + expect(subject.amount).to eq 19.99 + end + + context "when the price_selector does not implement #price_for_options" do + before do + allow(variant.price_selector).to receive(:respond_to?).with(:price_for_options).and_return false + end + + it "returns an unpersisted Spree::Price", :aggregate_failures do + expect(Spree::Deprecation).to receive(:warn). + with(/^price_for is deprecated and will be removed/, any_args) + expect(subject).to be_a Spree::Price + expect(subject.amount).to eq 19.99 + end + end end context "#price_difference_from_master" do From fe4e0a5ae3674eacd39cbfa4fa22a86b9488a3a5 Mon Sep 17 00:00:00 2001 From: Mike Conlin Date: Thu, 25 Mar 2021 10:33:16 -0700 Subject: [PATCH 3/3] Update usage of #price_for to #price_for_options The #price_for method will be deprecated in favor of the price_for_options method, which returns a Spree::Price. Converting the price to a Spree::Money object is now a frontend concern. --- .../views/spree/api/products/_product.json.jbuilder | 4 ++-- .../views/spree/api/variants/_small.json.jbuilder | 4 ++-- core/app/helpers/spree/base_helper.rb | 2 +- core/app/helpers/spree/products_helper.rb | 2 +- core/app/models/spree/line_item.rb | 4 ++-- core/app/models/spree/variant.rb | 6 +++--- core/spec/models/spree/line_item_spec.rb | 8 ++++---- core/spec/models/spree/variant_spec.rb | 12 ++++++------ .../app/views/spree/products/_cart_form.html.erb | 6 +++--- frontend/app/views/spree/shared/_products.html.erb | 2 +- 10 files changed, 25 insertions(+), 25 deletions(-) diff --git a/api/app/views/spree/api/products/_product.json.jbuilder b/api/app/views/spree/api/products/_product.json.jbuilder index 4984a4107a1..f6f3725364f 100644 --- a/api/app/views/spree/api/products/_product.json.jbuilder +++ b/api/app/views/spree/api/products/_product.json.jbuilder @@ -4,8 +4,8 @@ json.cache! [I18n.locale, @current_user_roles.include?('admin'), current_pricing_options, @product_attributes, @exclude_data, product] do json.(product, *(@product_attributes - [:total_on_hand])) json.total_on_hand(total_on_hand_for(product)) - json.price(product.price_for(current_pricing_options).try(:to_d)) - json.display_price(product.price_for(current_pricing_options).to_s) + json.price(product.price_for_options(current_pricing_options)&.amount) + json.display_price(product.price_for_options(current_pricing_options)&.money&.to_s) @exclude_data ||= {} unless @exclude_data[:variants] diff --git a/api/app/views/spree/api/variants/_small.json.jbuilder b/api/app/views/spree/api/variants/_small.json.jbuilder index d74f80e1661..d807e82ec3b 100644 --- a/api/app/views/spree/api/variants/_small.json.jbuilder +++ b/api/app/views/spree/api/variants/_small.json.jbuilder @@ -2,8 +2,8 @@ json.cache! [I18n.locale, current_pricing_options, variant] do json.(variant, *variant_attributes) - json.price(variant.price_for(current_pricing_options).try(:to_d)) - json.display_price(variant.price_for(current_pricing_options).to_s) + json.price(variant.price_for_options(current_pricing_options)&.amount) + json.display_price(variant.price_for_options(current_pricing_options)&.money&.to_s) json.options_text(variant.options_text) json.track_inventory(variant.should_track_inventory?) json.in_stock(variant.in_stock?) diff --git a/core/app/helpers/spree/base_helper.rb b/core/app/helpers/spree/base_helper.rb index f3974d1e080..e41ec1cad7c 100644 --- a/core/app/helpers/spree/base_helper.rb +++ b/core/app/helpers/spree/base_helper.rb @@ -130,7 +130,7 @@ def seo_url(taxon) end def display_price(product_or_variant) - product_or_variant.price_for(current_pricing_options).to_html + product_or_variant.price_for_options(current_pricing_options)&.money&.to_html end def pretty_time(time, format = :long) diff --git a/core/app/helpers/spree/products_helper.rb b/core/app/helpers/spree/products_helper.rb index a000b405966..18100b17ac5 100644 --- a/core/app/helpers/spree/products_helper.rb +++ b/core/app/helpers/spree/products_helper.rb @@ -38,7 +38,7 @@ def variant_full_price(variant) .with_prices(current_pricing_options) .all? { |variant_with_prices| variant_with_prices.price_same_as_master?(current_pricing_options) } - variant.price_for(current_pricing_options).to_html + variant.price_for_options(current_pricing_options)&.money&.to_html end # Converts line breaks in product description into

tags. diff --git a/core/app/models/spree/line_item.rb b/core/app/models/spree/line_item.rb index dc5c284f925..719a40ed8ec 100644 --- a/core/app/models/spree/line_item.rb +++ b/core/app/models/spree/line_item.rb @@ -123,7 +123,7 @@ def options=(options = {}) # a price for this line item, even if there is no existing price # for the associated line item in the order currency. unless options.key?(:price) || options.key?('price') - self.money_price = variant.price_for(pricing_options) + self.money_price = variant.price_for_options(pricing_options)&.money end end @@ -149,7 +149,7 @@ def set_required_attributes # Set price, cost_price and currency. def set_pricing_attributes self.cost_price ||= variant.cost_price - self.money_price = variant.price_for(pricing_options) if price.nil? + self.money_price = variant.price_for_options(pricing_options)&.money if price.nil? true end diff --git a/core/app/models/spree/variant.rb b/core/app/models/spree/variant.rb index 9d400abc50d..d4a31ee43b2 100644 --- a/core/app/models/spree/variant.rb +++ b/core/app/models/spree/variant.rb @@ -283,10 +283,10 @@ def price_selector # Returns the difference in price from the master variant def price_difference_from_master(pricing_options = Spree::Config.default_pricing_options) - master_price = product.master.price_for(pricing_options) - variant_price = price_for(pricing_options) + master_price = product.master.price_for_options(pricing_options) + variant_price = price_for_options(pricing_options) return unless master_price && variant_price - variant_price - master_price + Spree::Money.new(variant_price.amount - master_price.amount, currency: pricing_options.currency) end def price_same_as_master?(pricing_options = Spree::Config.default_pricing_options) diff --git a/core/spec/models/spree/line_item_spec.rb b/core/spec/models/spree/line_item_spec.rb index 26ee5900971..55c8b5e6d90 100644 --- a/core/spec/models/spree/line_item_spec.rb +++ b/core/spec/models/spree/line_item_spec.rb @@ -104,10 +104,10 @@ let(:variant) { Spree::Variant.new(product: Spree::Product.new) } let(:line_item) { Spree::LineItem.new(order: order, variant: variant) } - before { expect(variant).to receive(:price_for).at_least(:once).and_return(price) } + before { expect(variant).to receive(:price_for_options).at_least(:once).and_return(price) } context "when a price exists in order currency" do - let(:price) { Spree::Money.new(99.00, currency: "RUB") } + let(:price) { Spree::Price.new(amount: 99.00, currency: "RUB") } it "is a valid line item" do expect(line_item.valid?).to be_truthy @@ -140,8 +140,8 @@ it "sets price anyway, retrieving it from line item options" do expect(line_item.variant) - .to receive(:price_for) - .and_return(Spree::Money.new(123, currency: "USD")) + .to receive(:price_for_options) + .and_return(Spree::Price.new(amount: 123, currency: "USD")) line_item.options = options diff --git a/core/spec/models/spree/variant_spec.rb b/core/spec/models/spree/variant_spec.rb index 6098de4b09f..382021f24af 100644 --- a/core/spec/models/spree/variant_spec.rb +++ b/core/spec/models/spree/variant_spec.rb @@ -236,8 +236,8 @@ end it "displays default price" do - expect(variant.price_for(pricing_options_united_states).to_s).to eq("$19.99") - expect(variant.price_for(pricing_options_germany).to_s).to eq("€29.99") + 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 @@ -333,7 +333,7 @@ let(:variant) { create(:variant, product: product, price: 35) } before do - allow(product.master).to receive(:price_for).and_return(nil) + allow(product.master).to receive(:price_for_options).and_return(nil) end it { is_expected.to be_nil } @@ -344,7 +344,7 @@ let(:variant) { create(:variant, product: product, price: 35) } before do - allow(variant).to receive(:price_for).and_return(nil) + allow(variant).to receive(:price_for_options).and_return(nil) end it { is_expected.to be_nil } @@ -375,7 +375,7 @@ let(:variant) { create(:variant, price: 10, product: master.product) } before do - allow(master).to receive(:price_for).and_return(nil) + allow(master).to receive(:price_for_options).and_return(nil) end subject { variant.price_same_as_master? } @@ -388,7 +388,7 @@ let(:variant) { create(:variant, price: 10, product: master.product) } before do - allow(variant).to receive(:price_for).and_return(nil) + allow(variant).to receive(:price_for_options).and_return(nil) end subject { variant.price_same_as_master? } diff --git a/frontend/app/views/spree/products/_cart_form.html.erb b/frontend/app/views/spree/products/_cart_form.html.erb index a1d20e1d804..072b800b125 100644 --- a/frontend/app/views/spree/products/_cart_form.html.erb +++ b/frontend/app/views/spree/products/_cart_form.html.erb @@ -7,7 +7,7 @@

    <% @product.variants_and_option_values_for(current_pricing_options).each_with_index do |variant, index| %>
  • - <%= radio_button_tag "variant_id", variant.id, index == 0, 'data-price' => variant.price_for(current_pricing_options) %> + <%= radio_button_tag "variant_id", variant.id, index == 0, 'data-price' => variant.price_for_options(current_pricing_options)&.money %> <%= label_tag "variant_id_#{ variant.id }" do %> <%= variant_options variant %> @@ -28,13 +28,13 @@ <%= hidden_field_tag "variant_id", @product.master.id %> <% end %> - <% if @product.price_for(current_pricing_options) and !@product.price.nil? %> + <% if @product.price_for_options(current_pricing_options) and !@product.price.nil? %>
    <%= t('spree.price') %>
    - + <%= display_price(@product) %> diff --git a/frontend/app/views/spree/shared/_products.html.erb b/frontend/app/views/spree/shared/_products.html.erb index 2527ec4feb6..a97dcf5cf45 100644 --- a/frontend/app/views/spree/shared/_products.html.erb +++ b/frontend/app/views/spree/shared/_products.html.erb @@ -32,7 +32,7 @@
    <%= link_to truncate(product.name, length: 50), url, class: 'info', itemprop: "name", title: product.name %> - <% if price = product.price_for(current_pricing_options) %> + <% if price = product.price_for_options(current_pricing_options)&.money %> <%= price.to_html %>