Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discount doesn't match items unless "All categories" toggled on #2717

Closed
jamiepittock opened this issue Mar 1, 2022 · 8 comments
Closed
Assignees

Comments

@jamiepittock
Copy link

Description

This discount doesn't match any items - 'All categories' toggled off but no categories selected.

Screenshot 2022-03-01 at 14 44 53

This discount does match items - 'All categories' toggled on.

Screenshot 2022-03-01 at 14 45 00

Maybe this is expected behaviour but it tripped us up the other day, and someone on Discord ran into it too, so I thought I'd mention it and check.

Additional info

  • Craft CMS version: 3.7.33
  • Craft Commerce version: 3.4.11
@lukeholder
Copy link
Member

If you set All categories to false, then the product in the cart must match the selected categories. If you don't select any, then it cant match and the match fails.

If you want to apply the discount to any product regardless of the categories they are related to, then you need to turn on All Categories.

How do you think we could make it more clear in the labels?

@pdaleramirez pdaleramirez added ℹ️ status: need more info When waiting for user to supply database or more information. 📣 promotions labels Mar 2, 2022
@terryupton
Copy link

terryupton commented Mar 2, 2022

I also ran into this today. I am not sure if it is the functionality that is wrong or just the messaging/clarity.
So the use case for me, was that we needed to offer a discount if a user purchased 2 or more of some specific product variants. So naturally, selected the variants this applied to and setup the rules.
My expectation is was that if it matched those products the discount would be applied.
However, I then realised we had to also turn on All Categories.

The way the screen is presented feels more like an option to either choose specific products or for a more broader match choose certain categories. It really doesn't feel like both are required.
So perhaps it needs some more messaging and clarity around this. Even your note above @lukeholder gives it more clarity.

The other issue, and perhaps a bigger discussion above and beyond this specific thread is the use of categories full stop.
Not all of the products on our sites sit inside categories. So how does this affect them? All categories I assume matches products that have no cats at all?
Some users might not use categories at all and opt for structures/channels instead -as these have more control and additional benefits over categories.
I understood that Craft was also moving away from categories entirely, and this had potentially been marked for version 4. Is this still the case and how will this affect this setup?

Finally, another useful option might be productType as an option to match against. So you can match all products in a specific type, quicker and easier than having to pick every single variant. As categories might not reflect the product types directly, so cannot be used this way.

@mattstein
Copy link
Contributor

mattstein commented Mar 3, 2022

How do you think we could make it more clear in the labels?

Maybe “All” becomes “Any” and field instructions could help reinforce what’s going on?

Any purchasable
Disable to select specific purchasables that should be available to match.

Any category
Disable to select specific categories that should be available to match.

Could be too subtle, but it seems like switching off an “Any” control feels like “not just any ____, but these ...”. Implies that further selection limits the criteria, and that you should otherwise leave that setting enabled if you don’t want to limit further.

I’d almost say there could be a soft warning if you’ve flipped a switch off and not designated any items, but that could be excessive.

@lukeholder lukeholder self-assigned this May 16, 2022
@pdaleramirez pdaleramirez added 💡 enhancement Ideas and suggestions 🧭 guide and removed ℹ️ status: need more info When waiting for user to supply database or more information. labels Dec 6, 2022
@RitterKnightCreative
Copy link

Yeh this just tripped me up today and after I spent a little while debugging after a client emailed me asking about it. Came here to find others taking about it as well.

I think the root of the problem is Commerce is treating the lightswitches as an ALL condition instead of OR. Under the hood that's what's happening but it's not especially clear to the users (who aren't programmers).

By default, both light switches are on but let's say you want to limit to just a couple products. Naturally you turn off one light switch and choose your purchasable (which is already kind of counterintuitive).

However since you're limiting a purchasable, users think that they may need to limit categories as well to make sure their logic sticks, so they ALSO flip that light switch box on. But, since there's nothing selected, you end up with Commerce doing the follow logic:

LIMIT THIS DISCOUNT to ONLY (PRODUCTS SELECTED) AND (NO CATEGORIES).

... which matches nothing.

Most users are aren't thinking like:
LIMIT THIS DISCOUNT to ONLY (PRODUCTS SELECTED) AND (ALL CATEGORIES SELECTED).

The light switches should really be reversed and the default set to OFF which means all purchasables would be included by default:

Screenshot 2023-03-28 at 1 18 29 PM

Commerce could then figure out how to do the right logic under the hood. Unchecking to limit something is counteruintutive. It's a bit like asking someone to "check this box to opt-out".

One other option would be to just get rid of the lightswitches all together. If a user puts in a limiter, Commerce could use that, otherwise assume every purchasable and/or category is included. We could also consider changing the "Matching items" tab near the top to "Limit items" might help.

Screenshot 2023-03-28 at 1 54 58 PM

Now with Commerce 4, maybe the Matching Items tab needs to go away and use the UI from conditional builder to create something else entirely?

Screenshot 2023-03-28 at 2 10 52 PM

@samueldraper
Copy link

I have also been caught out by this before. I think a simple UI and wording change could resolve. But also like @RitterKnightCreative suggestion to "Limit to" instead of "Match".

@lukeholder
Copy link
Member

The issue here is that if you only have a selector for purchasables without the 'all purchasables' guard switch, if you had a single purchasable selected to apply the discount to, and then you deleted that purchasable in the system elsewhere the relationship to that purchasable will be dropped from the discount and then it would start applying the discount to all purchasables. This would not be good obviously.

We are still thinking about this and will see if we can improve the wording in the future.

@RitterKnightCreative
Copy link

@lukeholder yeh - that's an interesting wrinkle. I wonder how other systems are handing it?

I also wonder if some defensive coding could be helpful?

For example, if you delete a purchasable that's the only purchasable tied to a discount, maybe remind the user that it has "dependencies" and not let them remove the purchasable? And/or proactively disable the discount since the whole expression basically falls apart anyway in that case?

I would assume a purchasable that still exists in the system but is disabled wouldn't be affected by this (only because it still exists)? From a user's perspective, I think most authors would think removing a one-off punchable that's no longer needed is effectively disabling it.

@lukeholder
Copy link
Member

This has been improved in 4.3 due out soon...

CleanShot 2023-08-01 at 18 48 07@2x CleanShot 2023-08-01 at 18 48 39@2x

@tommysvr tommysvr closed this as completed Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants