Improve Discount Rules "Blocking" functionality

[MMasey]

Which Umbraco Commerce version are you using? (Please write the exact version, example: 10.1.0)

13.2.1

Bug summary

When using the Block further discounts from being applied or Block discount if discounts already apply, the Redeem(<code>) method on an order behaves in a confusing way.

Specifics

I have two discount codes, TEST10 and TEST20, which apply and 10% and 20% discount on the order total. For both of these discounts, I have tried adding both block rules, individually and together.

Based on how the Redeem method works for non-existed discount codes, e.g. it throws a validation exception error saying No discount or gift card found with the code '<code>', I would expect it to behave in the same manner based on those built in block rules.

Instead, it still applied the additional discount code, but marks the IsFulfilled value as false. This makes it a little cumbersome when providing feedback to the UI.

For not I am adding an additional check, after the SaveOrder method is called on the order, and before the uow.Complete is called to check is the discount was applied. Here's and example of the code i'm currently using as a workaround.

_commerceApi.Uow.Execute(uow =>
{
	var orderReadOnly = _commerceApi.GetCurrentOrder(store.Id);
	var order = _commerceApi.GetCurrentOrder(store.Id)
		.AsWritable(uow)
		.Redeem(model.Code);

	_commerceApi.SaveOrder(order);

	var discountApplied = order.DiscountCodes.Any(d =>
		d.IsFulfilled && d.Code.Equals(model.Code, StringComparison.InvariantCultureIgnoreCase));

	if (!discountApplied)
	{
		throw new ValidationException(new[]
			{ new ValidationError($"Unable to apply discount or gift card with the code '{model.Code}'") });
	}

	uow.Complete();
});

Based on how it throws an exception for non-existent errors, it feels like it should also throw the same sort of error, providing a clearer error message based on the type of block rule and discount / gift card code. At the moment, I can't find a clear way to get a specific type of error based on those build in block rules.

I'm not 100% sure if this is a bug, or expected behaviour.

On a related note, In the Umbraco.Commerce.Checkout package but default, if you follow the steps to reproduce, it will actually show both discount codes, even through only one is actually active. This is more to highlight the issue.

Steps to reproduce

This assumes Umbraco Commerce is already installed with configured products, and has a working checkout flow.

1. Create a TEST10 discount code, that talks 10% off the order total price and enable the Block discount if discounts already apply rule
2. Create a TEST20 discount code, that talks 20% off the order total price and enable the Block discount if discounts already apply rule.
3. Apply the TEST10 discount code to the cart
4. Apply the TEST20 discount code to the cart. There will be no clear indication that this hasn't been applied based on the block rules, other than checking the IsFulfilled value after saving the order.

Expected result / actual result

If trying to apply a second discount code, when Block discount if discounts already apply, and/or Block further discounts from being applied, the Redeem(<code>) method should return an exception with a relevant error message.

Dependencies

Umbraco.Commerce.Checkout, but this isn't necessarily required.

[mattbrailsford]

I guess at the moment this would be expected behaviour because ultimately the second code is valid (so no exception) but it doesn't apply (so it's unfulfilled because the first rule blocked it). Also bare in mind, rules are checked in the order of them within the back office, not the order in which they are applied to the order so you'll want to ensure your most dominant reward is higher up in the list.

I appreciate the discounts system isn't that logical in some places. We have been trying to address some of it over time, but it might be in future we come up with a new approach.

As it stands then, I'd suggest this falls under a feature request so I'll move it to a disucssion instead of an issue. But I appreciate the input.

Another option could be to create a custom validation event handler for ValidateOrderDiscountCodeRedeem when a code is being added to ensure it fails validation if it looks like it's going to be unfulfilled (though you'd need to check it's position in the discounts lists and check any previous discounts to see if they have the "block further discounts" flag, and also to check if there are already any discounts on the order and if the discount in question has "block if other discounts apply" set)

https://docs.umbraco.com/umbraco-commerce/key-concepts/events#validation-events

[MMasey]

Thanks for the background info @mattbrailsford. I did wonder if it was less of a bug, and more "by design". It can definitely see how the complexities around it can increase rapidly.

Another question I have then is, what is the purpose of the UnFulfilled status, other than to say the code isn't currently active on a cart? I can see it's used for the validation rules, but if the code is UnFulfilled, does adding it to the order still make sense? It could potentially create another layer of confusion, for example;

1. TEST10 is redeemed on the order
2. TEST20 is redeemed on the order, but makes TEST10 unfulfilled. In this case it would make sense to hide the TEST10 code from the UI
3. TEST20 is unredeemed, but but results in TEST10 being applied again due to the validation rules, even though this it would have been hidden

I appreciate i'm not necessarily providing any solutions here, and I know how complex it must be under the hood, but figured it might be useful to provide some other use cases to help myself and others get a firmer understanding of it. It may help to work out future functionality too.

[mattbrailsford]

Yea, I get the confusion it could cause.

I think ultimately it boils down to the fact that discounts are rules based and so the only time we know if they apply is actually by applying them and running them through the calculation process. It's in the calculation process that it runs through all the discounts and rules and checks if they apply based on the current state of the order.

We could maybe look at saying "if a discount is unfulfilled at the end of the calculation and it is a code, remove the code from the order". This should in theory be fine to do without requiring the order to be recalculated again as if it is unfulfilled then it should mean that it has not monetary effect on the order. But it's figuring out the clean way to do this as it would potentially be difficult to throw a validation error as it's now too late in the flow of things, and, really, based on the order of discounts, it might not be the code being applied that gets removed (ie, the new code could have higher priority and so removes a previously applied code).

Maybe the solution is to not throw exceptions at all and just remove the code quietly 🤷‍♂️

[MMasey]

Interesting, I see how throwing and error wouldn't work. At the moment I've updated my workaround so that it added an additional check after the first SaveOrder that looks for any unfulfilled discounts,, and checks if any match the one the was being redeems. If it finds it, it will then unredeemed the code and save again. Wasn't really sure of another way to do it for now, but it works nicely.

Maybe the SaveOrder method could return a value with information if anything was unfulfilled etc. That way you could potentially have it remove it quietly if it doesn't get fulfilled, but still provide feedback that could be used during a request?

[mattbrailsford]

Maybe just try the OrderSaving event and update the order before it gets persisted so you don't have to trigger 2 DB saves

[MMasey]

I'm not sure if that would work in the context of error handling during a request though right? For example, I have an API endpoint for applying the discount code which should return a success or error message. If I used the OrderSaving event, won't that happen separate to that request?

[mattbrailsford]

It would yes, but this was more a suggestion of how to do whatever you are currently doing better.

Another less intrusive option could be in your Redeem endpoint apply the discount code then call order.Recalculate() to force it to run the calculation process. You can then check the discounts and if they aren't valid, return a bad request from your endpoint, otherwise then call orderService.SaveOrder(order) to actually persist the changes.