Introduce a condition plugin for purchased entities

Hi Bojan,

Just an idea; the condition creation part of Drupal 7 Rules was very easy to use. You would:
1. Add a new condition
2. Select the field as tokens like: node:author:email
3. Select the values either as fixed values or as tokens.

Do you think it is applicable here? Like:

1. Add condition
2. Select product attribute
3. Select desired values

I don't think this is the direction that this issue is going to do but I'm attaching the plugin I'm using for now, I need something quick for allowing selecting variations. It's a copy of OrderItemProduct adapted for variations. A clear shortcoming is that someone could select variations, and then restrict to a product that doesn't have those variations and the coupon would never apply. Maybe this can help someone in the meantime.

+++ b/src/Plugin/Commerce/Condition/OrderItemVariation.php
@@ -0,0 +1,129 @@
+    $variation_ids = array_column($this->configuration['variations'], 'variation_id');

Should be target_id if you use this :) Argh, there are some other issues in the evaluate method - just ping me if you want the updated stuff :D

Here is another way to limit by variations. Once products are selected in autocomplete field, variations are loaded as pre-checked checboxes field.
Now product limitation only check variations, no more products.

Just figure out this patch should add a hook_update or deal with old product way to evaluate conditions

Of course, some tests are also needed, so i think we should update this to make old behavior works without hook_update (previous tests should pass) + add new tests for variations

OrderItemProduct.php has been changed on 21 June 2018 in issue Rename conditions to remove the "Limit by" prefix,
#5 can't be applied to latest commerce module, so I can't test the effect of it.

Matt wanted to be able to select individual attributes if the variation type supports them, but that can result in a potentially huge form.
I'm more inclined to just go with an autocomplete shown when the "Applies to all variations" is unchecked.

All of this is too complicated.

Consider that every condition is "or", so we can just add another condition OrderItemVariaiton/OrderVariation, more simple more better.
Just like #3 did.

Add "OrderItemVariatoin,OrderVariation,OrderItemVariationTest,OrderVariationTest"
Just some copy of "OrderItemProduct,OrderProduct,OrderItemProductTest,OrderProductTest"

Hope I'm not suggestion something impossible given the existing structure in the promotion menu.

I have great difficulties to figure out how to use the existing taxonomies used within the existing variations. It seems to me impossible in the current structure. I haven't found any use for 'categories'. In the current concept the taxonomies seems to brought into focus for use on every level irregardless if they are associated to product or variations. But why do I see no result in 'categories'?

To have a more elegant process, should it not be a nested query process to limit the query size?
And how about selecting stores?

Would this concept still be possible? I show some ideas.
First select store X (click and query follows and finds nested items - if null 'no product type found')
select product type X (same - if null 'no product variations found')
select the available (same - if null 'no taxonomies found')
select the available taxonomy
select product type Y
select the available variation
select the available taxonomy
First select store Y
select product type Z (click and query follows, multiple nested items found - if null 'no product type found', taxonomy not mentioned)
select a product type related taxonomy
select the available variation
select the available taxonomy

And separately select AND / OR discounted quantity.
And separately select AND / OR value totals

That way even a massive list can be reduce to show only what is selected / exists.
But it deviates a lot from the current structure. It's just an idea which may improve it a bit.

I think the approach in #12 is the right one, I closed #2971381: Add a product SKU condition in favor of this.

The patch needs minor tweaks though. We store the UUIDS for the "order_item_product" condition, and use the entity uuid mapper. We should do the same. Otherwise the patch looks good, especially now that #3083123: Product variation autocomplete should allow matching by SKU is in (since that allows searching by title + SKU).

The only potential problem I see is that I'm wondering if customers wouldn't expect the search to be limited to products selected (If "applies to selected products is also checked and products are selected).

So wrapping it up.

Comment fix :p.

1. +++ b/modules/product/src/Plugin/Commerce/Condition/OrderItemVariation.php
   @@ -0,0 +1,80 @@
   +    $variation_ids = $this->getVariationIds();
   +
   +    return in_array($purchased_entity->id(), $variation_ids);
   

   Couldn't we just get the UUID from the product variation and check from the configuration instead of querying for the mapped identifier?

2. +++ b/modules/product/src/Plugin/Commerce/Condition/OrderVariation.php
   @@ -0,0 +1,85 @@
   +      if (in_array($purchased_entity->id(), $variation_ids)) {
   +        return TRUE;
   +      }
   

   Again, couldn't we just use the uuid method to check, without having to map the UUIDs?

3. +++ b/modules/product/src/Plugin/Commerce/Condition/VariationTrait.php
   @@ -0,0 +1,88 @@
   +trait VariationTrait {
   

   +1 on trait, helps folks build custom conditions.

+++ b/modules/product/src/Plugin/Commerce/Condition/OrderItemVariation.php
@@ -0,0 +1,79 @@
+    return in_array($purchased_entity->uuid(), $this->getVariationUuids(), TRUE);

+++ b/modules/product/src/Plugin/Commerce/Condition/OrderVariation.php
@@ -0,0 +1,85 @@
+      if (in_array($purchased_entity->uuid(), $variation_uuids, TRUE)) {
+        return TRUE;
+      }

+++ b/modules/product/src/Plugin/Commerce/Condition/VariationTrait.php
@@ -0,0 +1,87 @@
+  protected function getVariationUuids() {
+    return array_column($this->configuration['variations'], 'variation');
+  }

At this point, do we get getVariationUuids or just an isValid that gets the UUIDs and does the in_array comparison?

I renamed getVariationUuids() to isUuidValid() (could not come up with a better name.

The problem with naming the method this way is that it could potentially conflict with someone else's code (since it's a Trait).

@mglaman: better naming in mind?

+++ b/modules/product/src/Plugin/Commerce/Condition/OrderItemVariation.php
@@ -73,7 +73,7 @@ class OrderItemVariation extends ConditionBase implements ContainerFactoryPlugin
-    return in_array($purchased_entity->uuid(), $this->getVariationUuids(), TRUE);
+    return $this->isUuidValid($purchased_entity->uuid());

+++ b/modules/product/src/Plugin/Commerce/Condition/OrderVariation.php
@@ -67,14 +67,13 @@ class OrderVariation extends ConditionBase implements ContainerFactoryPluginInte
-      if (in_array($purchased_entity->uuid(), $variation_uuids, TRUE)) {
+      if ($this->isUuidValid($purchased_entity->uuid())) {

Looks good. 🤣another thing: should we just have "isValid" and pass the purchased entity?

We actually still need getVariationIds() that is used to set the default value of the field.

@mglaman: I just realized, what about other purchasable entity types? Should we provide support for that? Using deriver perhaps? technically, the only main difference is the entity reference field that needs to target the right entity type. The rest should just work?

+++ b/modules/product/config/schema/commerce_product.schema.yml
@@ -42,6 +42,22 @@ commerce.commerce_condition.plugin.order_product_type:
+commerce.commerce_condition.plugin.order_variation:
...
+    variations:
...
+      label: 'Variations'
...
+          variation:
...
+            label: 'Product variation'
...
+commerce.commerce_condition.plugin.order_item_variation:
+  type: commerce.commerce_condition.plugin.order_variation

So we'd basically call these "purchased_entity"

We label them as "Specific @label" where @lavel is the plural label from the entity type definition.

It's a lot of tedious work (not hard, but lot of changes.) But it technically is the "right" way since we have purchasable entity from the order item not just variations.

It's a lot of tedious work (not hard, but lot of changes.) But it technically is the "right" way since we have purchasable entity from the order item not just variations.

I agree, I think we need to do this the right way and be consistent with the rest of Commerce which technically supports different purchasable entity types.

So setting this back to needs work.

Wrapping this up today

Okay, here is it converted to a purchasable entity plugin with a derived, in the Order namespace. The Product module performs an alter to "fix" the category.

1. +++ b/modules/order/config/schema/commerce_order.schema.yml
   @@ -68,6 +68,22 @@ commerce.commerce_condition.plugin.order_total_price:
   +      sequence:
   +        type: mapping
   +        mapping:
   +          entity:
   +            type: uuid
   +            label: 'Purchasable entity'
   

   I don't know why we opted for a mapping here instead of just a sequence? We use a sequence for the order_store plugin and removes the need for array_column.

2. +++ b/modules/order/src/Plugin/Commerce/Condition/OrderItemPurchasedEntity.php
   @@ -0,0 +1,86 @@
   +  use PurchasedEntityTrait;
   
   +++ b/modules/order/src/Plugin/Commerce/Condition/OrderPurchasedEntity.php
   @@ -0,0 +1,92 @@
   +  use PurchasedEntityTrait;
   
   +++ b/modules/order/src/Plugin/Commerce/Condition/PurchasedEntityTrait.php
   @@ -0,0 +1,114 @@
   +trait PurchasedEntityTrait {
   

   The trait feels kind of weird. I'd rather a shared base class. One that just sets the configuration form up, still.

3. +++ b/modules/order/src/Plugin/Commerce/Condition/PurchasedEntityTrait.php
   @@ -0,0 +1,114 @@
   +    $entity_uuids = array_column($this->configuration['entities'], 'entity');
   +    return $this->entityUuidMapper->mapToIds($this->getPurchasableEntityType(), $entity_uuids);
   

   Moving the config from a mapping to sequence simplifies this. Probably removes the need for the method, even.

4. +++ b/modules/product/commerce_product.module
   @@ -345,3 +346,12 @@ function commerce_product_config_schema_info_alter(&$definitions) {
   +
   +function commerce_product_commerce_condition_info_alter(array &$definitions) {
   

   Missing doc block

PHPCS violations

FILE: ...modules/contrib/commerce/modules/product/commerce_product.module
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 350 | ERROR | [x] Missing function doc comment
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------
FILE: .../src/Unit/Plugin/Commerce/Condition/OrderPurchasedEntityTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 10 | WARNING | [x] Unused use statement
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...ernel/Plugin/Commerce/Condition/PurchasedEntityConditionTest.php
----------------------------------------------------------------------
FOUND 2 ERRORS AND 1 WARNING AFFECTING 3 LINES
----------------------------------------------------------------------
  5 | WARNING | [x] Unused use statement
 16 | ERROR   | [ ] Parameter $expected_label is not described in
    |         |     comment
 34 | ERROR   | [x] Missing function doc comment
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

I don't know why we opted for a mapping here instead of just a sequence? We use a sequence for the order_store plugin and removes the need for array_column.

I wondered the same while working on the order_item_variation condition plugin, and I did this because it matched what was done for products, but I see no reason to not switch to a simple sequence instead.

Regarding the trait, feel free to try with a different approach (but once again, that is the pattern that was used for products, and product types.

Here is a reroll dropping the trait for a base class, it feels a bit cleaner.

Here is another go, which refactors the schema storage and evaluation method.

This looks good, few nitpicks.

1. +++ b/modules/order/src/Plugin/Commerce/Condition/PurchasedEntityConditionDeriver.php
   @@ -0,0 +1,67 @@
   +   * The entity manager.
   

   The entity type manager :).

2. +++ b/modules/order/src/Plugin/Commerce/Condition/PurchasedEntityConditionDeriver.php
   @@ -0,0 +1,67 @@
   +   * Constructs a new PackageTypeDeriver object.
   

   This should be PurchasedEntityConditionDeriver.

3. +++ b/modules/order/src/Plugin/Commerce/Condition/PurchasedEntityConditionDeriver.php
   @@ -0,0 +1,67 @@
   +    $this->entityTypeManager = $entity_manager;
   

   The parameter should be $entity_type_maanger.

Caught in the act of a bold face copy paste.

🥳 Committed!

Hi,

After update the Drupal core to Drupal 8.9.3 after that re-apply the patch and got the error as

An HTTP AJAX error has occurred. HTTP status code: 200 Debugging information below. Path: / promotion / 1 / edit? Destination = / admin / commerce / promotions & ajax_form = 1 StatusText: parsererror ResponseText: "

can you please help to solve me this asap as the site is already live and we update it and got this error.

Thank you

Hi,

Not sure if this is related to the fact that the patch works against 2.x, but even though it applies to 2.20, it doesn't work.

I had to fix L83 in PurchasedEntityConditionBase.php:

$entity_ids = $this->entityUuidMapper->mapToIds($this->getPurchasableEntityType(), $this->configuration['entities'] ? $this->configuration['entities'] : []);

Without that fix, I get an error in the logs that the second argument for mapToIds is null instead of an array.

Also, while reviewing the patch, I noticed a typo in line 90 for PurchasedEntityConditionBase.php should be

'#title' => $purchasable_entity_type->getCollectionLabel(),

instead of

'#title' => $purchasable_entity_type->getCollectionLabsel(),

see https://git.drupalcode.org/project/commerce/-/blob/8.x-2.x/modules/order...

This code needs a bit more polish re #43 to avoid errors for both new and existing promotions after this code is added.

The configuration issue needs to have the entities key returned as part of defaultConfiguration() to get rid of the errors.

The typo is obvious.

Attached is an additional patch for current dev state (essentially an interdiff) plus the full patch.

Good catch, not sure how this has been unnoticed for that long :). Thanks!

+++ b/modules/order/src/Plugin/Commerce/Condition/PurchasedEntityConditionBase.php
@@ -87,7 +96,7 @@ abstract class PurchasedEntityConditionBase extends ConditionBase implements Con
-      '#title' => $purchasable_entity_type->getCollectionLabsel(),
+      '#title' => $purchasable_entity_type->getCollectionLabel(),

🤦🏼‍♂️sorry, everyone.

I am wondering if there can be a bit of an expansion on this. In my store with Drupal 8/9 I have a Product Attribute of Department Name. Each product variation has a drop down where that product is assigned a Department Name (because some products are only available in certain departments). I want to apply a promotion with a coupon code where I can limit that code to only work for products that have that department name assigned to them. A similar example would be I also have a Product Attribute for size. If I wanted to just limit that Promotion Code by the size XXL I don't see a way to do that in the current restrictions. Other than selecting every single SKU but that would take ages with the amount of products I have. Unless I am missing something.