Several small refactors to promotions code #3416
Conversation
The previous specs file was actually testing the main Spree::Calculator::PriceSack instead. The allow_any_instance hack is required since while building the package content, no matter what is the price of the variant, the line item amount is always 10. This is due to how the factory is defined: https://github.com/solidusio/solidus/blob/b76c1dd67642245778c019b15b3a10ef22b1f50c/core/lib/spree/testing_support/factories/line_item_factory.rb#L9
The right method to be overridden is `compute` and not `compute_shipment`. Take advantage of the change to use described_class here.
We had some issues on the PriceSack specs that was testing the wrong class. Using described_class more should avoid this kind of issues in the future.
We don't need to check and raise if that method is not defined since it's not used in our codebase for that kind of calculator. This is the reason why also specs are useless. We are able to remove this method without a deprecation warning because it's an abstract method and, if users are overriding it into their own calculators that inherit from this one their application will be perfectly safe.
kennyadsl
changed the title
| # This hack is due to our factories not being so smart to understand | ||
| # that they should create line items with the price of the associated | ||
| # variant by default. | ||
| allow_any_instance_of(Spree::Stock::ContentItem).to receive(:price) { amount } |
There was a problem hiding this comment.
Do you think it would make sense (be possible, simple, and useful for other scenarios) to change the factory in order to avoid this stubbing?
There was a problem hiding this comment.
I had the same thought but I think we have to change the line item price having a dynamic price, based on the variant price. I think that this would break a lot of specs in stores that are relying on the static price of line items in our core factories, and that's why I think this is the best trade-off.
I'm open to making an attempt to create an alternative line_item factory with a dynamic price and trying to deprecate the current one. Do you think it's worth it?
There was a problem hiding this comment.
I think it is the normal scenario for line items to have prices bound to the ones of their variants... I would consider a price difference to be more of an edge case, so I think that factory could be later changed to reflect the more regular use case.
But of course I would not consider this an urgent matter at all. We may just open an issue and see how it goes from there.
There was a problem hiding this comment.
Agree, let me do that!
Description
I've worked on these refactors while working on #3355, which is closed now but I think these small preliminary changes could still be beneficial for the codebase.
Checklist: