Let the PriceSelector return a Spree::Price

[swively]

Description
Issue laid out in this discussion

The Variant::PriceSelector currently returns a Spree::Money, which is misleading and somewhat limiting.
The Spree::Price model is useful for its ability to be extended to support things like sales prices, partial prices, etc.

Checklist:

• I have followed Pull Request guidelines
• I have added a detailed description into each commit message
• I have updated Guides and README accordingly to this change (if needed)
• I have added tests to cover this change (if needed)
• I have attached screenshots to this PR for visual changes (if needed)

[jarednorman]

It also occurred to me that it's perhaps the case that the reason that this returns a Spree::Money instance is to allow for dynamic pricing when persisting the price might be undesireable. For example if you sold stickers that were priced dynamically based on the size that customer inputted and also the volume.

I don't think this is an issue, since you can just return an unpersisted price, but it should be understood that this API is allowed to return unpersisted prices to allow for that use case. 🤔

[aldesantis]

I'm a bit concerned by this approach to deprecating the method. Our approach to major upgrades is to ensure that someone who's on the latest 3.x will always be able to update to 4.0 without touching their code as long as they don't get any deprecation warnings.

Unless I'm missing something, with this change people will have to call price_for(..., return_price: true) in the latest 3.x in order not to get the deprecation warning. But then, in 4.0, we are going to remove the return_price: argument altogether, and anyone who passes it will start getting an error.

In other words, people who upgrade from the latest 3.x to 4.0 will have to change something in their code in order to complete the upgrade, which is something we should try to avoid if at all possible.

How about removing this, and only using the value of Spree::Config. price_selector_returns_price instead? It's also more straightforward.

[kennyadsl]

The idea was to deprecated return_price after the change in the next major (4.0). This kind of deprecation allows keeping the system working for example if the host application switched to returning price but some extensions don't.

I'm also ok using Spree::Config. price_selector_returns_price alone.

[jarednorman]

Interesting, I'd not really considered this. So @kennyadsl are you thinking we do this:

• In 3.x you get a deprecation warning if you use the return_price: false behaviour, but the behaviour remains.
• In 4.x we remove the return_price: false behaviour and you get a deprecation warning if you pass return_price: to the method.
• In 5.x we remove the return_price: argument.

That's not unreasonable, but 5.0 seems so far in the distance. 😅

[kennyadsl]

That's what I was thinking but, totally unrelated, I think we can change the removal deprecation process starting from Solidus 3.0 so that we are allowed to remove deprecated code in the next minor version. That's what Rails does, and also (I discovered this yesterday 😓) what we kinda state here:

We would like to also follow the Rails approach: deprecating functionality in one minor version and removing it in the next.

Anyway, I think @aldesantis has a valid proposal to avoid this parameter at all, we were discussing it yesterday and I guess he will post some comments soon.

[aldesantis]

@swively thanks, I think the change makes sense but the deprecation strategy should probably be adjusted a little bit.

Also, we'll have to address a bunch of deprecation warnings that this change causes in the Solidus codebase instead — the calling code will have to be updated in order to use the Spree::Price API instead of Spree::Money.

As an alternative, have you also considered simply implementing a new price selector class that returns a Spree::Price object, and using that as the default in all new applications?

Also, to elaborate a bit on what @jarednorman said, we use a custom price selector for one of our clients to (somewhat) cleanly implement grandfathered pricing in a subscription-based app: our price selector checks the product and the user and returns either a Spree::Money instance with the regular product price or a Spree::Money instance with the grandfathered pricing. We could still do this by returning an unpersisted Spree::Price instance, so I think this is okay, but I just wanted to provide a real example of when it would make sense to return something that is not a persisted price.

[swively]

Having seen a few client projects encounter issues with the price selector returning a Spree::Money, the typical solution has been to add a method price_record_for that returns the Spree::Price. For this reason I think having the return value be configurable by the return_price flag is useful.

To avoid forcing developer intervention when bumping to 4.0, I could remove the deprecation altogether and just let this method's return value be controlled via the config/flag, but I also think that the method in its current state is misleading and not as useful as it would be if it just returned a price object.

Happy to go either way on this, the less impactful option being to remove the deprecation and leave it up to the user to configure the method to suit their use case.

[aldesantis]

I think just using the global configuration would be better, since it doesn't create more work for us and our users in the future to also remove the new method parameter.

@jarednorman @kennyadsl what do you think? Do you see any drawbacks?

Also, @swively, would you be able to share some situations where returning a Spree::Money doesn't work? I'd like to get a better understanding of the rationale behind this change.

[swively]

@aldesantis I've encountered it on two client projects in the last month. The first was using the sales prices extension, which has the sale price for a variant on the Spree::Price. Anywhere that you would need to grab the price (using #price_for(pricing_options)) would return a Spree::Money, not allowing us to reach for the sales price.

On a different project they used partial prices for items that are part of a bundle, so each item would have a regular standalone price, as well as a partial_price that was discounted when in a bundle. Again you would ideally grab the price with #price_for(pricing_options) to get one that is accurate for the store and currency, but we were limited by not being able to access the partial_price for the given variant.

[aldesantis]

@swively after discussing the deprecation approach for this with @kennyadsl, we decided adding a new method may be the best path for moving forward. The additional parameter doesn't play well with a situation where someone has implemented their custom price selector, without knowing that it needs to comply with the return_price: true argument.

You could add a PriceSelector#price_for_options method that returns a Spree::Price instead of a Spree::Money, and update PriceSelector#price_for to raise a deprecation warning and simply call #price_for_options(...).money.

The only concern is properly handling a situation where someone has implemented their own price selector and has only implemented #price_for, but not #price_for_options.

To fix that, you might want to also implement Spree::Variant#price_for_options (and have a delegate :price_for_options, to: :find_or_build_master in Spree::Product) and deprecate Spree::Variant#price_for. Spree::Variant#price_for_options would check if the price selector responds to #price_for_options: if it does, it would simply return its value. If not, it would print a deprecation warning, then build an unpersisted Spree::Price with the value returned by #price_for and return that.

Finally, you would update all the places in Solidus where we call #price_for and call #price_for_options instead.

Something else to keep in mind: Spree::Variant#price_for was implemented as the successor to Spree::Variant#price_in and Spree::Variant#amount_in, so you need to make sure the deprecation warnings for those two methods tell people to call #price_for_options instead. Otherwise, we'll be telling people to solve a deprecation by calling another deprecated method. 😅

[kennyadsl]

@swively sorry, my fault for initially pointing you in a path that is not ideal. 🙏

[jarednorman]

Ah, that's a good catch. I had not thought of that. I don't love the name price_for_options, but I don't hate it either.

[aldesantis]

Yeah, to be honest it's the only one we could think of. 🤷 Open to better suggestions.

One thing we should keep in mind is that we're out of names for deprecating that method, so let's make really sure we get it right this time.

[jarednorman]

Yeah, I think in one app I used spree_price_for to distinguish it from price_for by making it clear it returns a Spree::Price, but I don't think we should do that in core.

[kennyadsl]

Thanks, this PR is perfect! 🙏 🙏

We are going to merge this as soon as 3.0 is released so it will be part of 3.1.

[spaghetticode]

@swively thanks!

PR approved by other core team members.