Discount system

[mamhoff]

The current promotion system has a lot of problems, the most notable one being performance - however, the performance issues are really a symptom of architectural issues. We do need to support promotions though, as they are an integral part of what ecommerce does.

Here's a couple of the problems I see:

1. Promotions allow order-level promotion adjustments. Order-level adjustments are a problem for every finance team on the planet, because it is quite difficult to calculate which line item an adjustment should be attributed to if the order contains line items with different accounting criteria or - worse - tax categories / taxation rules.
2. Promotions create "eligibility" flags on adjustments that are set to false when the promotion does not, actually, apply. This was done - I believe - in order to boost performance.
3. The promotion system checks rules WAY too often, and promotion rules can be quite expensive to check.
4. The way it is currently implemented, we have to run the PromotionHandler::Cart from OrderContents, and because that's brittle, we need to run the order updater before AND after we run the promotion handler. This leads to us having to do tax calculation TWICE every time we add something to the cart, leading to lots of unnecessary calls to - often external - services.
5. The order updater re-checks the eligibility of each adjustment, leading to undesirable polymorphism in Spree::Promotion#eligible_rules.
6. Promotion rules have a terrible API: there's eligible? (for orders, really) and actionable? (for line items), but nothing for shipments. There's also applicable? which is only a type check. I propose renaming eligible? and actionable? into order_discountable?(order), line_item_discountable?(line_item) and shipment_discountable?(shipment), so as to make the whole thing more understandable.
7. When checking line item eligibility (in Spree::Promotion#line_item_actionable?) we also check order eligibility. By the time we do that, though, we've already checked order eligibility! That check should go, as it is quite expensive!
8. Promotion actions can do all sorts of things. This adds a lot of complexity to the system. I think they should only create discounts; for making additional line items or changing quantities there are a lot of other extension points available.

This PR is a glimpse of what could be.

The general architecture is as follows:

1. The order updater calls a central place to run eligibility checks on all active and connected order promotions. Based on these eligibility checks, discounts are created or removed (!). We do not keep uneligible discounts around. This might seems like it's slower that keeping the discounts, but the removal of complications makes it faster as we only run things once.
2. We stop attaching promotions to orders that are not manually applied. Automatic promotions are checked this way or that way in (1).
3. We chip away at the adjustments model, extracting discounts from there. Discounts and taxes should not be conflated in the same table.
4. No more order-level adjustments.

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)

[tvdeyen]

Wow. What a great piece of work. I have mostly questions :)

[tvdeyen]

We should deprecate it as well.

[loicginoux]

should you not provide have it declared on the Spree::PromotionAction as well so that all actions inherit it with a default response ? Also what about Spree::PromotionAction::CreateAdjustment, what if'll call can_discount? on this class ?

[mamhoff]

Yes, I think we should declare it on the Spree::PromotionAction base class and make it return false by default. This will also be the correct response for the CreateAdjustment class, which IMO should be deprecated and replaced by CreateItemAdjustments with the DistributedAmount calculator.

[stem]

The promotion system is indeed quite complexe but powerful too and it needed some work, especialy on the performance side. Thanks for tackling it 👍
Your approach seems to be quite simple to follow yet performant and almost as powerful as the original so 👏

I've just a quick remark regarding "4. No more order-level adjustments.". Even if the legacy promotion system is not yet deleted, I think that level of discount should not be discarded that easily.
It is not perfect for all the points you mentioned, but it can also be the right tool in some contexts.

Right now, in order to keep that level of discount, a user (and by a user I mean we @epicery as we have a case where this is necessary) has to choose between being locked on the legacy, or extends the new system with custom code to get the feature back.

If not directly implemented by Solidus (which I can agree with), I cannot help but thought of how it could be extended and, IMO, it seems really hacky:

• extend Spree::Discounts::OrderDiscounter#call to add a call to a new discount_order method
• extend Spree::OrderUpdater#update_total to consider the order's discounts
• extend Spree::Promotion#discounted_orders to consider the order's discounts
• extend Spree::PromotionAction to add a has_many :order_discounts
• extend Spree::Order to add a has_many :discounts
• and probably other entry points that I missed.

Would you be open to drop the discard of order level adjustment for this refactoring?

[jarednorman]

@stem I am concerned about creating additional schisms in the community, so the compatibility decisions around any change like this are a big concern. Can you elaborate on why you say that order-level adjustments "can also be the right tool in some contexts"? I don't believe they are the right tool for anything, and I'd also like to know more about stores are using them.

[mamhoff]

Would you be open to drop the discard of order level adjustment for this refactoring?

I would not like order level discounts to be a thing. Order level adjustments are still a part of the code base, and will probably need to be supported for a long time (there can also be manual adjustments - also a terrible idea - but most importantly I'm coming to the conclusion that the migration path for this should not touch completed orders, and for completed orders with promotions to still show up correctly in the admin panel, we must keep some support).

That said: If you had a promotion that distributed a fixed amount over all line items of the order, would that fufill your use case? Can you give some more details on what kind of thing your promotion does?

[stem]

@jarednorman of course, I can try to explain why I think it's not a good idea to get rid of order level discount :)

First, we're a marketplace, so accounting is quite different for us than for a regular store (no tax on products for us as we're not selling any products...)

We're using order, shipment and line items' adjustments according to the intended discount:

• discount for a specific product: line item
• discount for the shipping fees: shipment
• general discount for a specific seller but no specific products: order but as each seller has its own shipment, it would be better handled at the shipment level if it can handles non shipping fee discount
• general discount on the marketplace level but no specific products: order

And all discounts but the one at the marketplace level can be "paid" by the seller or the marketplace, so accounting is a bit more complex as we're pushing the discounts towards our account or the sellers'.

For us, general discounts, both at the specific seller or the marketplace level, are generally X€ off and are cumulative with the line items discounts.
At least for us, it does not make sense to split this amount into smaller amounts and distribute it on all line items because it's not what was advertise to the customer. We promised a 5€ off this product, and 10€ off as a welcome gift, he/she needs to sees exactly that, not 6€ on the product and 1€ on each other 9 products in the cart.

Frontend complexity aside, spliting the amount would be difficult to correctly account for each possible situation.
First variations of the same scenario that come to mind are:

• we have a "buy 50€ or more and you'll get a free something" promotion which is done by having an action that add the something's line item in the cart if not already there and apply a 100% discount on 1 quantity. If the customer does not want a second something, this line item is completly free and cannot have another adjustment for the general discount's dispatch.
• let say that we want to create a 10€ discount, and the customer only has 10 products in its cart, some worth less than 1€, the dispatcher will need to take that into account, make a 0.5€ discount on a 0.5€ product, another 0.99€ discount on a 0.99€ product and split the rest with rounding strangeness on the other products (in this case: 7x1.06 + 1x1.09 or 5x1.06 + 3x1.07)

To be crystal clear, I love the work from @mamoff. It goes in the right direction on many things, but IMO, Solidus should offer the building blocks for the order level, even if it does not ship with promotion actions that use them.
Order level adjustment could be this building block, but I had the impression that discounts should not be handle by adjustments in the (near?) future and that adjustments will be repurposed for tax / cancelations handling only.
Maybe I was mistaken?

And for some of our use cases of order level adjustments:

• manual compensation if something went wrong with the delivery
• welcome / welcome back gifts. Eg: 10€ off your first order, or the first order after 3 months of inactivity
• referral program (yes, we've done it with the promotion system, and I know we're not the only ones). Eg: 5€ off your first order, 5€ in store credits for the person you referred you after you complete your first order
• gift on specific sellers of sellers' category. Eg: 10€ off when you buy more than 50€ from a butcher

All that being said, I might be missing something both in the old and new system.
How would you handle those promotions?

NB: sorry for the long text, I tried to be as exhaustive as possible.

[loicginoux]

From my experience, I would also prefer to keep a way to have order level discount for financial reason. Let's say you have a 15€ discount on your purchase and 7 articles in your cart. Our issues we had when trying to split this promotion was the following:

> (BigDecimal(15) / 7) * 7
14.999999999999999999

So splitting 15€ discount on 7 articles does not add up correctly. Maybe I am wrong and I don't use ruby's BigDecimal operations correctly. Maybe there is a way to do it correctly with the line item discount without losing 1 cent, but in our case, we had a hard time making invoicing and finance calculation corrects because of this.

[mamhoff]

Thank you @stem and @loicginoux for the detailed response! This is great.

Regarding fixed-amount whole-order discounts:

@loicginoux The way the DistributedAmountsHandler distributes the discount over several line items is using Money#allocate, so no cent gets lost.

@stem In the current standard frontend, we display similar adjustments by summing them up if they have the same label, so that way it appears to the user like they're getting a whole-order adjustment while, behind the scenes, individual items are adjusted.

I think about doing the same thing here; the customer doesn't really need to know how a discount is split up, they just need to know it's there and the right amount.

What's more complicated with this proposed system is cumulative discounts, because with that, the order of discounts being applied starts to matter, and we currently have no way of giving a promotion precedence over another one. It might be worth exploring acts_as_list for promotions, so that they're always executed in a configurable order, but that would mean rewriting the Discounter classes. For example: We have a 15% discount on wine, and a 20% discount all items - do we want to subtract the 15% first and then the 20% or the other way around?

I think what you're using for cumulative discounts is something that has been reported as a bug elsewhere: #2741

What's also impossible with the proposed system is free item promotions, because I limited the scope to discounts. That's probably something we'll want to revisit.

Regarding shipment adjustments: We use shipment adjustments for item taxes in our store (they depend on where things are shipped from in our jurisdiction), frequently surpassing the shipping amount, so that some of our shipments end up with a negative total. This would work, too.

I also think that we should have more flexible shipping discounts; not just free shipping, but a large subset of the calculators we allow for line item discounts on shipments, too.

[jarednorman]

@stem Those discounts can all be handled using line item adjustments. The issue with order-level discounts is that they are a perennial cause of accounting, tax calculation, and tax reporting issues. From an accounting perspective, they aren't a real thing.

You cannot calculate tax correctly on an order if there are any order-level adjustments without making (probably wrong) assumptions about how they're being used. Different items and shipments may have different tax rates. You can't just assume that the adjustment should be distributed across the items in some arbitrary way because that will always be wrong for some cases. This incorrectness trickles into ERPs, and other systems that often don't even have the concept of arbitrary adjustments that aren't actually line items.

Because we have the distributed amounts handler, using line item/shipment adjustments just forces you to say that either your $10 discount is off the items or the items and shipments and it forces you to be specific about how those amounts are distributed. This change does nothing to prevent you from having a flat discount, just makes you be more specific about how it is applied, which is a boon for both the correctness of your accounting and tax calculation.

It's a significant footgun, and (not saying you don't, just in my experience) people don't seem to understand the downstream consequences and issues using them causes.

[mamhoff]

Closing in favor of https://github.com/friendlycart/solidus_friendly_promotions