Add an option to Buy X Get Y to auto-add the "Get" product variation

Here's a first pass at this. It's missing the new configuration option for enabling the auto-add behavior and test coverage, but it's otherwise ready for manual testing.

Note that the expected behavior for the case of "buy 2 get 1 free" of the same product is to _not_ increase the quantity automatically when the "buy" conditions are met (i.e. when adding a product with quantity of 2 to the cart, the quantity is not increased to 3 automatically).

The only case when the "get" product is added (and quantity increased) automatically is when the "buy" conditions are met by other products.

+++ b/modules/promotion/src/Plugin/Commerce/PromotionOffer/BuyXGetY.php
@@ -76,7 +89,8 @@ class BuyXGetY extends OrderPromotionOfferBase {
       $container->get('commerce_cart.cart_manager'),

You're injecting the cart manager but not using it? Besides, the promotion module has no dependency on the cart module.

+++ b/modules/promotion/src/Plugin/Commerce/PromotionOffer/BuyXGetY.php
@@ -368,6 +409,86 @@ class BuyXGetY extends OrderPromotionOfferBase {
+          $purchasable_entities = $condition->getPurchasableEntities();

Should we maybe just add a return statement here, from the loop?

Something like return reset($condition->getPurchasableEntities()).

and we'd just return NULL at the end of the method?

No credit here, just removed the cart manager, started adding comments to several methods introduced (I wasn't sure what to write for calculateExpectedGetQuantity()).

I just want to see if the tests are passing now.

I've been manually testing the patch, and the functionality seems to work as expected! (Tried manually adjusting the buy quantities back and forth to endure the right quantity gets discounted), tried removing the get item from the cart and it automatically gets re-added).

As mentioned in the comments we're missing a setting to enable automatically adding the get quantity, and tests, but the functionality seems to work as expected.

We'll probably need to do some bikeshedding for the new methods introduced and comments. but the new interface looks good.

It also seems we have "duplicate" (or at least very similar logic present in calculateExpectedGetQuantity()), I wonder if we could refactor some of it so it gets reused in the apply() method as well.

Also, we might have to remove the following line:

$order_item->set('order_id', $order->id());

because of https://www.drupal.org/project/commerce/issues/2656818#comment-12926347.

Basically, setting the order on the order item will trigger logic in the OrderItem::postSave() which will save the order (which can be very problematic from within the order refresh).

I think this needs work as the BuyXGetY kernel test is failing (which is probably expected more or less since we're auto-adding the get quantity and the tests don't expect that.

But I'm suspecting that regressions were introduced as well. since testNotApplicable() is failing although this should probably not have been affected.

This patch adds the new setting, validation rules for it, and fixes the existing test coverage. All the failures from #7 were in test cases that did not expect the 'offered' item to be added to the order.

And here's some initial test coverage too! I'm keeping the 'Needs tests' tag in case we want to add more test scenarios.

Expanded the tests a bit. I think they're pretty much complete now so I'm removing the tag.

I started testing the patch and noticed a small bug with the #states, when checking the "Specific products" condition and then the "Specific product variation" condition, if one of the 2 conditions is then unchecked, the auto add setting disappears, it could be a Drupal core bug, so we could probably leave it as is for now...

Additionally, UX wise, I wonder what we could do to outline the "Automatically add the offer product to the cart if it isn't in it already" setting so it does not look the same as all the checkboxes for each condition plugin, perhaps by setting a #prefix? (See the screenshot below):

Perhaps "Advanced" is not the right term, but added that just to demonstrate what I was referring to.

Besides that, found issues with the validation code. With valid conditions selected, I got the following error message: "The "auto-add" offer can only be enabled when a compatible "get" condition is used." (See screenshot below):

Also, when an invalid configuration is submitted, I also got the error message except that you end up being stuck since the "auto-add" checkbox is not present in the form (because of the #states) and there's simply no way to uncheck it. I believe we should simply set the auto get value to FALSE in this case.

The rest of the code looks good to me! I wish we could come up with a better name for findOrCreateOrderItem(), but I have no better idea at this point, perhaps we shouldn't overthink it since it's not a public method...

The attached patch is adding "Behavior" as an help text and addressing coding standard issues as well.

Thanks @jsacksick, your changes look great to me! I assume you're ok with the rest of the code, so let's get this in :)

Ok, realized we needed to call the chain price resolver on the newly created order item to ensure the right price is resolved (in case somebody is using the price list module for example...), so adding that.

Also performed additional testing and wondering if the following behavior is correct (I'm suspecting a bug).
I have a promotion configured with Buy quantity 2 of any, get 2 of a specific product (with "auto add").

In my cart, I have 2 order items:

1. The first order item has a quantity of 3
2. The second order item is created/added to the cart (for the configured get product), with a quantity of 2.
3. If I adjust the second order item quantity to 3, it gets automatically updated to 4, 3 are discounted, 1 quantity is charged.

Is that the expected behavior?

Also, my bad for suggesting to not set the order reference on the order item as it's causing a fatal error (i.e TaxTypeBase::resolveCustomerProfile() explodes because it's trying to access the order and it returns a NULL reference.

Also, shouldn't we add an option to limit the number of times the get offer applies? Let's say you want to restrict the offer to a given quantity per order... (i.e buy 2 get 1, but only once...), I can see that being useful when the product being offered has limited quantity available (Can be implemented in a followup issue.

This patch introduces a new offer_limit option to cover #19.

This update adds support for locking auto-added order items, so customers can no longer edit their quantity or remove them from the cart. The support for locked order items is added in #3184272: Provide a way to lock order items, so this patch won't work by itself anymore, so I'm posting a combined patch as well.

@rszrama: Should we assume the auto-added product needs to be automatically removed if the conditions are no longer met? Or does that need to be a promotion setting?

This is really a small logic problem with a fix that may be more complicated. Basically, I think we need to reverse apply the auto-add in a broader scope - e.g. in PromotionOrderProcessor::process() when we recheck coupon validity.

It makes sense for that logic to live in the Promotion order processor. Wondering if we shouldn't add a promotions reference field to orders (similar to the coupons reference, so we could copy what we're doing for coupons (i.e if a promotion is no longer available, call a method on the offer plugin to do the necessary cleanup (i.e remove the auto-added order item for example in that case?).

The new method we'd introduce could be implemented by PromotionOfferBase to ensure we don't introduce a breaking change, then this new offer plugin could implement the necessary logic.
We could call it clear() or clearPromotion()...

That'd be a generic fix, another less clean approach could be to loop on order items (prior to checking the promotion availability, and always set the quantity to 0, for auto added order items...

The big question in my mind is if we can manipulate quantities at that point to 0 without creating a lot of extra deletes / saves in the same way we did by manipulating quantities in BuyXGetY::apply().

The order refresh logic already takes care of saving order items that have changed during the refresh process, so we can safely update the quantity without necessarily resaving the order item. But updating the quantity to 0 is not going to remove the order item from the order?

The combined patch in #21 is failing a test because we haven't updated the new test coverage to the expectation that we should delete the auto-added order items, so here's a fix for that.

Also, I tried to implement the suggestion for cleaning up auto-added order items in OromotionOrderProcessor::process() (see additional interdiff attached), but this approach can't work because, when refreshing the order, Order::clearAdjustments() (which removes the discount applied to the auto-added order item) is called before the promotion order processor, therefore the "running" order total (the value between the calls to Order::clearAdjustments() and OromotionOrderProcessor::process()) might be higher than the value used for the condition on the order total, so the promotion would still apply.

Here's a quick steps to reproduce:
- use a product with 2 variations, the first with a price of $10 and the second with a price of $15
- configure a BXGY condition like this:
- buy 1 "first variation" (the $10 one)
- get 1 "second variation" (the $15 one)
- auto-add the get product to the cart
- add condition on "Current order total" to be equal or greater than $15

And what happens is this:
- add 1 "first variation" to the cart -> the promotion doesn't apply (order total is $10)
- increase the quantity to 2 -> the promotion applies and it adds two "second variation" order items, 100% discounted (order total is $20)
- decrease the quantity back to 1 -> at the end of Order::clearAdjustments() the order total is $40 (!! $10 for the "first variation" item and $15 + $15 for the two now non-discounted "second variation" items)
- because the promotion can't remove its auto-added order items before Order::clearAdjustments(), it will still apply based on the "intermediary" total price of $40, and it will only remove only one "second variation" order item

All that to say.. it still needs a bit more thought, but I wanted to share this WIP anyway. Maybe we could somehow clear the auto-added order items *before* trying to apply the promotions in PromotionOrderProcessor::process().

I've been working extensively on this today and had several discussions with @amateescu.

I rerolled the patch and wrote a failing test reproducing the following:

- use a product with 2 variations, the first with a price of $10 and the second with a price of $15
- configure a BXGY condition like this:
- buy 1 "first variation" (the $10 one)
- get 1 "second variation" (the $15 one)
- auto-add the get product to the cart
- add condition on "Current order total" to be equal or greater than $15

So as explained in #26, the main problem is that the auto-added order item is taken into account in the order total calculation, which is causing the promotion to apply after the promotion adjustments are cleared by $order->clearAdjustments().

There's no need for the "combined" patch anymore since #3184272: Provide a way to lock order items got committed.

The clear() method approach "almost" works, but not entirely due to the issue with the order total calculation.

I have an idea that could make this work which would consist in "locking" the promotion adjustment, in order for it to not be removed when $order->clearAdjustments()is called.

Attaching a first patch that's failing, and will attempt a second approach that locks the adjustments.

I believe the adjustment should remain otherwise that could cause issues with any promotions testing the order total...

The attached patch seems to do the trick (tests are passing for me locally). The adjustments added by the "buy x get y" offer are now "locked", and are cleaned at the beginning of the apply() method.

This way, if a promotion no longer applies, the apply() method won't even get called.

The remaining auto added order item is correctly cleaned by the logic in the promotion order processor.

Note that we're probably missing a setting to toggle whether the auto-added order item gets automatically removed....

@rszrama, @amateescu: Should we implement that now? Or make do that in a followup issue?

Shoot, only ran the BuyXGetYTest locally, other promotions tests are failing.

Now that we're locking adjustments, we can actually rely on $order->collectAdjustments()to detect whether a previously applied promotion no longer applies. Note that this means that potential other promotion offers would need to lock adjustments in a similar fashion to have their clear() method invoked.

This makes me wonder whether we should remove the introduced clear()method and have specific code this just for the "buy x get y" offer...

We need to clear the locked adjustments in a different place than from the apply() method, otherwise locked adjustments will remain...

Perhaps from the clear method? Additionally, we're not clearing them properly right now (my latest code for clearing adjustments is incorrect).

Definitely not a trivial issue :), I'm attaching my WIP that unfortunately breaks Promotion compatibility checks... (which is expected).

I also introduced the auto remove setting which has tests coverage.

Keeping the locked adjustments is causing issues, but on the other hand not keeping it affects the order total, causing any condition on the order total to now work as expected...

To make this simpler, I'm wondering if we shouldn't set the order item unit price of the order item we're auto adding to 0. The problem is the promotion is no longer going to appear in the total summary...

Will probably try a different approach later today.

The attached patch should fix the remaining issues (Although we probably need more tests).

We now clear the promotion adjustments really early in the process, via a newly introduced EarlyOrderProcessor that has a high priority.

This clears the promotion adjustments, and also reduces the quantity (i.e removes the auto added quantity).

OrderRefresh is now taking care of the order item removal in case the order item quantity at the end of the refresh process is 0, we probably need tests for that, although BuyXGetY::testAutoRemoveOrderItem() kind of test that.

Also, after discussing this with @amateescu, we've decided there was no point in a new setting for toggling the auto-removal of the automatically added quantity (this should always happen).

Removed the cosmetic changes to PromotionOrderProcessor which got committed separately.

Forgot to commit the EarlyOrderProcessor (explaining the amount of failing tests) :).

Did a bunch of cleanups after reviewing the recent changes, and I think this is finally ready!

Saving credits for @rszrama :)

Edit: or not, since I'm not a maintainer :P

You removed too much code from the clear() method :).

Oops :D

Tests are still failing :).

Committed!!! Good work guys!

I just committed a followup that requires tests coverage.