Shipping discount applied multiple times to the same shipping service

I have the same issue. In our configuration we have 4 different shipping methods and commerce_discount_commerce_shipping_method_collect_rates($method, $order) is being called for every of these methods. And we have shipping price discounted 4 times in a row. So if we set 50% discount we get 93.75% discount (1-0.5^4).
So we probably need to add parameter $method_name to "commerce_discount_percent_off_shipping" event and check it when deciding if we need to apply discount to the shipping rate of the order.

Does this title more accurately reflect the issue?

Does this title more accurately reflect the issue?

Yes!

Ok, cool - will try to reproduce and resolve this ASAP.

Added test for this case.
Unfortunately couldn't find any tests for shipping neither in Discount module nor in Shipping module.
So this test is a first of a kind and needs review anyway (for example we can assert directly shipping_rate's prices but it would look more synthetic)

Setting the status to "needs review" see what the testbot will say. Thanks for writing tests @potop

Just a quick review and it looks like it may not have been created against the right branch.

1. +++ b/tests/commerce_discount.test
   @@ -452,4 +452,4 @@ class CommerceDiscountTest extends CommerceDiscountTestBase {
   -}
   +  }
   

   This looks like an accidental change?

2. +++ b/tests/commerce_discount_base.test
   @@ -54,9 +54,10 @@ class CommerceDiscountTestBase extends CommerceBaseTestCase {
   +    // Enable all commerce modules + commerce_shipping + commerce_discount.
   ...
   +    $modules[] = 'commerce_shipping';
   

   This should be probably done in your new commerce_discount_shipping.test

Let's see if this works.

1. This looks like an accidental change?

Yes. You are right.

2. This should be probably done in your new commerce_discount_shipping.test

Commerce Shipping module should be enabled before Discount module otherwise no shipping-related offers are initialized and cannot be tested. So we need to modify CommerceDiscountTestBase::setUp() to be able to do it.
I just made it the easiest way without adding a parameter to pass more modules to it. And as Discount has so many offers related to Shipping it just seemed ok to me.

This test is supposed to fail now as the reported issue exists, but without exception. The only its' assertion should fail.

Ah interesting, well let's make that more possible, and I made a typo and enabled commerce_discount twice with a bad copy/paste.

I made a mistake on the last posting with my array_merge(). But where does 'commerce_shipping_dummy' come from? There is an example one maybe it should have been 'commerce_shipping_example'?

Running the code locally seems to confirm example is correct, but I could be wrong still, LMK.

This passes locally, the assertion didn't fail. Maybe there is something else in the scenario that causes this to fail. Always good to have more tests though, feel free to iterate.

And of course I post the wrong files.

But where does 'commerce_shipping_dummy' come from? There is an example one maybe it should have been 'commerce_shipping_example'?

Shipping Example contains only one shipping method while we need two to reproduce the issue so another module was added for testing purposes only.
And I managed to miss it in my patch.

Now it seems to fail as expected!

This new patch contains fix to the issue so tests should pass this time.

The fix also needs review.
It contains following changes:

• 'Shipping method' parameter added to '% off of a shipping service' event
• Unused (?) parameter 'Shipping service' changed to 'Shipping method' in '% off of shipping' action
• parameter 'shipping_method' added to commerce_discount_percent_off_shipping_service() and shipping services are checked for it before applying the discount

Patch #22 works for the case where you do not select a shipping service. In the case that we have a shipping service selected in the discount, that shipping service gets discounted by the rule multiple times. I've added a check to prevent this. Please review.

It seems I've used a wrong patch as my base for the patch in the previous commit. This should hopefully work now.

Could you create an interdiff with your changes and try again with the patch?
https://www.drupal.org/documentation/git/interdiff

This is the interdiff, between the patch from #22 and #26, it adds a check if the shipping service exists, to prevent any fatal errors.

Sorry for the late reply. Re-uploading the patch. No idea what happened with the last one.

@simeon157: Important changes related to free shipping & % off shipping will soon be committed to the codebase.
Could you confirm the bug still occurs with the patch from #2910160: Fix the "free shipping" offer type? This patch is supposed to fix various issues related to free shipping.