Shipping discount coupon resulting in negative value exception

Siddharth Gupta

Vote:

Hi,

I am using Commerce 12.10.0 and below is the scenario:

I added an item into the cart and then applied a shipping coupon (Free shipping). Upon applying the coupon I get the error mentioned below (step 2)

1) Added a weight based shipping method: https://www.screencast.com/t/trWQxz7zJ

2) Below is the stack trace for the error: https://www.screencast.com/t/TRpqw9MgFR6D

I also had a look into the reflector based on the stack trace and I see the code mentioned below:

public Money GetDiscountedShippingAmount(IShipment shipment, IMarket market, Mediachase.Commerce.Currency currency)
    {
      Validator.ThrowIfNull(nameof (shipment), (object) shipment);
      Validator.ThrowIfNull(nameof (market), (object) market);
      Money money = this.GetShippingCost(shipment, market, currency) - shipment.GetShipmentDiscountPrice(currency);
      this.ValidateShippingCostForShipment(money);
      return money;
    }

Looking at this I feel the issue is where you subtract GetShippingCost with GetShipmentDiscountPrice. And GetShipmentDiscountPrice being the culprit which leads to a negative value and then throws the error.

#198790

Nov 06, 2018 23:00

Jafet Valdez

Vote:

Right before running the calculators in your own code.

If you do:

shipment.GetShipmentDiscount()

What does that give you?

#198807

Edited, Nov 07, 2018 8:23

Siddharth Gupta

Vote:

#198868

Nov 07, 2018 23:08

Jafet Valdez

Vote:

Yep, as I suspected. The dreaded rounding error. :P

The underlying cause is that the PromotionEngine rounds the resulting saved amount before setting it on the Shipment.SavedAmount field.

So either you avoid using shipment costs with more than X number of decimal places (X in your case being 2).

Or......

... if you can't do that I'm hard pressed to find a good solution to this.

But technically you can modify to how many decimal places the promotion engine rounds off to. You can try doing this before running the promotion engine. Like so:

var oldCurrencyDecimalDigits = orderGroup.Currency.Format.CurrencyDecimalDigits,
orderGroup.Currency.Format.CurrencyDecimalDigits = 5;
// Run PromotionEngine
orderGroup.Currency.Format.CurrencyDecimalDigits = oldCurrencyDecimalDigits;

This should (I hope, I haven't tested it :D) mean that the PromotionEngine rounds off correctly in your case.

But I'm assuming that this approach will come with other issues since all calculations made based on the ordergroup in the Promotion Engine will be rounded off the same way, which may be incorrect in another case.

#198911

Edited, Nov 08, 2018 12:55

Siddharth Gupta

Vote:

@Jafet Valdez: Thank you for the response, I see this as a bug with the promotions engine then. It should not round the saved amount right? I appreciate your suggestion to try and modify the number of decimal places rounding. But since the effect of this change might cause other issues I dont want to move forward with this. Would you suggest to file a bug in this case then? The project I am working on is approaching the go-live date.

#198915

Nov 08, 2018 16:06

Jafet Valdez

Vote:

I'm not so sure that it can be considered a bug per sé. It's rounding according to the defaults for currencies in C#. Like Episerver Commerce does in most other places.

It needs to round to be able to work with various types of percentages or you'd have other calculation issues instead.

Personally I'd say that doing the same rounding in your ShippingMethod would be the best solution in general, but I understand that this might not be possible for business reasons.

So contact Developer Support to get a path forward on this.

#198916

Nov 08, 2018 17:25

Jafet Valdez

Vote:

It looks like modifying Currency.Format.CurrencyDecimalDigit is the way to go for now until Episerver gets a fix out in the next major release

Take a look at this blog post:

https://world.episerver.com/blogs/Quan-Mai/Dates/2018/11/control-the-rounding-level-of-currencies-/

#199076

Edited, Nov 14, 2018 20:32

Siddharth Gupta

Vote:

@Jafet Valdez: Thanks the response. Marking your reply as the answer :)

#199078

Nov 14, 2018 20:53