Negate by nid should override a general match.

Thanks, this looks like a good addition, though it appears to be rolled against an older version of the module than the current HEAD.

Perhaps the terms/negate_terms test should also work in the same way, so you can remove entire categories as well as specific products?

I think I am facing this issue as well. One of my site admins configured a coupon to apply to the class "product", then to apply only to a product of a particular nid. The coupon is currently applying to all products.

I am hesitant to apply a patch like this to a production store though until it receives more testing.

Seems like this would be useful, but it doesn't apply to HEAD any more.

I took a look at this - the attached patch updates the OP's patch to HEAD and generalizes to apply to terms as well.

However, the logic for resolving conflicting restrictions seems a bit haphazard to me. For example, if you limit to a certain class, but allow terms, then items with those terms will be included even if they don't belong to the specified class. Again, if you include a product and also list applicable sku's, then all models of that product will be included regardless of whether they are listed in the sku section.

The confusion lies in the fact that some restrictions are ORed and some are ANDed. The logic should all be the same, or perhaps the user should be able to specify? I'd be happy to look into this further if you'd like.

Had a quick look at your patch but what will happen if "negate products" or "negate terms" is selected? It seems that no product or term restrictions will apply at all, making the product/term selection useless?

And I agree that the logic for restrictions is somewhat of a mess - made more complicated by the "negate" features - and I haven't had time to try and sort this out. If you want to attempt a patch to fix it all up then please do so!

If one of the "negate" options is selected and the item matches a product/term in the list, then the item will be excluded regardless of other restrictions (isn't that the desired behavior?). If the item doesn't match a product/term in the list, then that restriction will be ignored, and the next in sequence will be applied, regardless of whether the "negate" option is selected. At least I think that's how it works - have tested it a bit and it seems to be ok, but it should certainly have further testing.

I will look at sorting the rules. I am leaning towards simply ANDing all specified restrictions but want another opinion before i implement. This would mean, effectively, that the most restrictive rules would apply. Does that sound reasonable?

Also I'm worried about breaking setups that depend on the existing logic. Maybe there needs to be a "legacy mode" option which keeps things working the old way for administrators who don't want to update their coupons?

But if the negate option is selected and the item *doesn't* match, then it should be included - the negate option is labelled "Apply coupon to all products except those listed.". This is what the original code tried to achieve with "negate XOR in_array" - either negate is set, or it's in the array, but not both. The point of the original issue is that it isn't obvious or intuitive how "negate" is applied when other restrictions are included on top of this.

Perhaps we need an option for both AND and OR, as people might need a coupon that covers "all products in this taxonomy term, and some other specific products as well" which would be an OR condition. But the more complex we make this, the more we are just reinventing Rules or Conditional Actions...

I see, yes, of course, you're right. I'd been thinking that itemsi were included by default when in fact they are excluded by default. I'll fix it.

As to the OR option, it does make things much more complicated - how common do you think that use-case would be? Is it rare enough that it could be covered by two identical coupons one applying to each product set?

OK - attached are 2 patches - the first (very small) one simply update's the OP's patch (this time correctly, i hope) and generalizes it to apply to taxonomy (which is what i originally set out to do...). This is still not really intuitive - if you specify a class restriction AND a list of negated products, the negated products list will override the class restriction in all cases (i.e. if you limit to class "widgets" and then say include all products except "widget 1" and "widget 2", not only will the rest of the widgets be included, but the doohickies will as well. This is contrary to the form instructions which state that product class restrictions override the rest.

Anyway, rather than trying to sort that out piecemeal, I've submitted the second (larger) patch, which attempts to implement better logic for applying multiple criteria generally. It allows you to specify an "all" or "any" mode which applies to all criteria - if you specify "all", then tests for each of the criteria are ANDed; if you specify "any" then they're ORed (only non-blank criteria are considered). There is also a "legacy" mode which keeps things exactly as they were, and any already existing coupon defaults to legacy mode to avoid breaking existing sites.

I know ideally you'd be able to specify whether each individual criterion was to be ANDed or ORed, but this seemed a reasonable compromise for the sake of reducing complexity.

I've tested both patches a bit, but they need more, for sure. My site doesn't make use of very complicated coupon restrictions, so it's probably not the best testbed.

I committed the first patch, and updated the product class restriction message to note that exclusions will still apply. I need more time to review the second patch; I'm wary of adding yet more options, especially a "legacy" mode option which will need to be explained somewhere.

I understand - it's not a big deal for me as it's not a feature I will be using. I agree about adding too many options, but I don't see any way of revamping the validation logic without providing some kind of backwards compatibility. One thought I'd had was to hide the "legacy" mode for all new coupons - only showing it as an option when editing an old coupon that requires it. One could even generate a message saying "the restrictions for this coupon need to be updated to work with the latest version of uc_coupon" or something like that.

But if you decide you want to leave it alone, that's fine - in that case, I'd be happy to have a go at documenting the way it works now, because it is counterintuitive in some places. And the statement Selecting one or more product classes will apply this coupon to all products of this class, overriding other restrictions (except for specific products or terms explicitly excluded below) is still not quite accurate - if you include a specific product, term or SKU, then an item matching that criterion will be included whether or not it's product class is excluded. Probably not a common situation for new coupon entries, but if you're editing an old coupon which had specific products included, you might think you'd be able to exclude them just by changing the product class, which is not the case.

Marking this as a feature request, since things now work as they always did. Also note that the 2nd patch at #10 is severely out of date.