Let the PriceSelector return a Spree::Price

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]

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?)

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)

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 <p> tags.

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
 

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
 

core/app/models/spree/variant.rb

@@ -276,23 +276,35 @@ 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
     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 = price_difference_from_master(pricing_options)
       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]

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

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
 

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

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
 
@@ -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
@@ -298,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 }
@@ -309,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 }
@@ -340,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? }
@@ -353,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? }

frontend/app/views/spree/products/_cart_form.html.erb

@@ -7,7 +7,7 @@
         <ul>
           <% @product.variants_and_option_values_for(current_pricing_options).each_with_index do |variant, index| %>
             <li>
-              <%= 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 %>
                 <span class="variant-description">
                   <%= 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? %>
       <div data-hook="product_price" class="columns five <%= !@product.has_variants? ? 'alpha' : 'omega' %>">
 
         <div id="product-price">
           <h6 class="product-section-title"><%= t('spree.price') %></h6>
           <div>
-            <span class="price selling" itemprop="price" content="<%= @product.price_for(current_pricing_options).to_d %>">
+            <span class="price selling" itemprop="price" content="<%= @product.price_for_options(current_pricing_options).amount %>">
               <%= display_price(@product) %>
             </span>
             <span itemprop="priceCurrency" content="<%= current_pricing_options.currency %>"></span>

frontend/app/views/spree/shared/_products.html.erb

@@ -32,7 +32,7 @@
           </div>
           <%= link_to truncate(product.name, length: 50), url, class: 'info', itemprop: "name", title: product.name %>
           <span itemprop="offers" itemscope itemtype="http://schema.org/Offer">
-            <% if price = product.price_for(current_pricing_options) %>
+            <% if price = product.price_for_options(current_pricing_options)&.money %>
               <span class="price selling" itemprop="price" content="<%= price.to_d %>">
                 <%= price.to_html %>
               </span>