Improve Order discount products inline condition and rule

This badboy needs to be marked CNR.

code looks ok, when I read it.
Somebody else should read it too. And we should test it after being applied if all the logic works indeed AND 2. if changing the rule definition would NOT break existing user sites.

+++ b/commerce_discount.rules.inc
@@ -384,20 +396,49 @@ function commerce_order_has_owner_build(EntityDrupalWrapper $wrapper, $account)
+  $products_sku = preg_split('#(,|;) *#', (string) $products);

Why does semi-colons need to be an option? Maybe we can keep it simple with just commas but the space graces sounds like a much better plan.

@nvahalik I'm saying that because there is no precedent for semi-colons. It adds unnecessary complexity that needs to be supported for the minimum viable code for the feature which likely could inhibit a committer from accepting the patch. And there is no documentation that that is an option to use semi-colons in the help so it's a hidden feature that if accidently dropped could make the users peeved. Why not pipe symbols| too, or ~ or *... too easy to also bikeshed;)

Wanted to try this out, but for the life of me I (and patch command) can't locate commerce_discount.inline_conditions.inc file. Is this specific to dev branch?

Tried looking for it there though, also no sign of it.

@Alienpruts yes it's specific to dev branch. Most patches are as that is where development usually happens before the next stable release.

+++ b/commerce_discount.rules.inc
@@ -384,20 +396,49 @@ function commerce_order_has_owner_build(EntityDrupalWrapper $wrapper, $account)
-  $products_sku = explode(', ', (string) $products);
...
+  // Split by comma or semi-colon. Allow some grace for spaces.
+  $products_sku = preg_split('#(,|;) *#', (string) $products);

I think this should stay AS IS, as it's outside of the issue's scope.

100% for opening a new issue to add new separator items. But that way we keep the patch to just logic operators, not how it find SKUs.

Tagging for weekly sprint. Looks like a big hitter for order discount + product UX.

Tested the patch and it's working beautifully for me. In fact it may help solve the issues we were having with product-level discounts disappearing, because (at least referring to the proposal at #2621680: Remove Product Display discounts and only use order discounts for 1.2), product discounts tend to cause issues such as discounts disappearing from checkouts, and to combat that, I changed a discount to be order-level. This patch helps facilitate a much simpler (and better!) means of creating product conditions on an order-level discount. It's more robust as a result and I think it makes sense to commit this.

I'll set this to RTBC for now, but I haven't given it a totally thorough review yet; I can only speak for our own experience. But so far this patch is very promising! Others may want to give this patch a deeper review.

Forgot to set RTBC, but as I mentioned further review by others (especially anyone currently having problems with product-level discounts) should take it for a spin.

1. This feature could really use some tests (my inner @alexpott) to ensure we are getting what we want.
2. Also curious to know if it's defaulting to the existing behaviour or do we need an update hook.

Agree, tests would be good. It's a promising start to be sure. Thanks @joelpittet!

Committed this to -dev. Fixed a couple coding standards nitpicks on there too. elseif instead of else if Thanks @nvahalik

This make bug #2655084: Improperly implemented rules options list :(