#price_for doesn't return a Spree::Price

[swively]

Solidus 3.0 will stop supporting the deprecated method #price_in(currency), in favour of #price_for(pricing_options)
Handling pricing with the pricing_options is useful as it allows finer and more contextual control, specifically for multiple countries using the same currency.

The Spree::Price model is useful as it can be extended to support things like sales prices, or partial prices for items that are part of a bundle.

While #price_in returned a Spree::Price object, #price_for ignores the return value its name suggests, instead giving us a Spree::Money object.

I started to refactor solidus_sale_prices to bring it into the #price_for world, but it is so closely tied to the Spree::Price model, it is not viable unless the PriceSelector can provide us a price.

Suggested action would be to have the PriceSelector provide an extra method #spree_price_for that returns a Spree::Price. A more destructive route would be to remove the .money on the current #price_for method.

[jarednorman]

Yeah, this would definitely be a breaking change unless we did something fun with the API for the price selector, but I'm on the same page. Converting to a money shouldn't be the price selector's job. If you look through the codebase, Spree::Money objects are mostly just used for frontend stuff.

[kennyadsl]

I think it's a good idea even if I don't have a lot of experience with Solidus pricing. Calling .money doesn't seem something that belongs to that call to me as well.

I think we can think of a way to deprecate returning a money object, like setting a parameter in the options, if not set means that price_for is being called in apps or extension expecting the old money return value.

Something like this, it's just an idea and I didn't actually test it so sorry if there's something wrong in the logic:

def price_for(price_options)
  price = variant.currently_valid_prices.detect do |price|
    ( price.country_iso == price_options.desired_attributes[:country_iso] ||
      price.country_iso.nil?
    ) && price.currency == price_options.desired_attributes[:currency]
  end

  return_price = Spree::Config.price_selector_returns_price || price_option.delete(:return_price)
  return price if return_price

  Spree::Deprecation.warn('price_for will stop returning a Spree::Money, please change your code so that it accepts a Spree::Price istead. When done, you can suppress this message by passing `return_price: true` to this method or setting `Spree::Config.price_selector_returns_price = true` globally.')
  price.try!(:money)
end

The global preference will probably allow switching price_for usage in the whole codebase without passing return_price every time we use it. What do you think?

[jarednorman]

We're probably best off adding an optional keyword argument or something, since price_options is an instance of whatever the price options class is for the app and configured to be something other than the price options class we provide, but overall I agree. Ever so slightly tweaked yours:

def price_for(price_options, return_price: false)
  price = variant.currently_valid_prices.detect do |price|
    ( price.country_iso == price_options.desired_attributes[:country_iso] ||
      price.country_iso.nil?
    ) && price.currency == price_options.desired_attributes[:currency]
  end

  return_price = Spree::Config.price_selector_returns_price || return_price
  return price if return_price

  Spree::Deprecation.warn('price_for will stop returning a Spree::Money, please change your code so that it accepts a Spree::Price instead. When done, you can suppress this message by passing `return_price: true` to this method or setting `Spree::Config.price_selector_returns_price = true` globally.')
  price.try!(:money)
end

[softr8]

I spent some time trying to fix the same issue in the sales price extension, and I was thinking, what about offering a 'stack' of price processors to modify the price as needed ?

Spree::Config.price_processors << SaleTaxPrice
Spree::Config.price_processors << SalePrice

def price_for(price_options)
  price = variant.currently_valid_prices.detect do |price|
    ( price.country_iso == price_options.desired_attributes[:country_iso] ||
      price.country_iso.nil?
    ) && price.currency == price_options.desired_attributes[:currency]
  end

  Spree::Config.price_processors.inject(price) {|processed_price, processor| processor.new(price_options).call(processed_price) }
end

Then as a 'middleware' stack, modify a price object as needed in the queue

class PriceProcessorsBase
  attr_reader :options #just an example
  def initialize(options)
    @options = options
  end
end

class SaleTaxPrice < PriceProcessorsBase
  def call(price)
    price.adjustments << { amount:  price.amount * options.magic_to_get_default_tax * -1 } # just an example
    # or
    price.amount *= options.magic_to_get_default_tax 
    price
  end
end

class SalePrice < PriceProcessorsBase
  def call(price)
    price.adjustments << { amount:  price.amount * 0.25 * -1 } # 25% off
    # or
    price.amount *= variant.active_sale.calculator.percentage # just pseudocode 
    price
  end
end

[jarednorman]

I'm unsure about this design, but I also like that it's possible to implement this kind of middleware-based price selector without a change to core to see how well it works.

[jarednorman]

I might like it better if it worked more like middleware in Rack (using the call stack) than processors (iteratively invoked) so that earlier ones might return early or invoke other different ones. I'd need to see more use cases I think though. Certainly a neat idea.

[kennyadsl]

For the record, this has been addressed with #3925.