Add variant support to PropertyAdd plugin #252

iammart · 2021-09-28T09:52:40Z

Following on from the discussion on the below thread, I have updated the plugin to query on product code(s) not product ID.

This enables product attributes to be stored on order products that have types of selection or bundle.

https://aimeos.org/help/viewtopic.php?p=15713#p15713

coveralls · 2021-09-28T09:56:59Z

Coverage decreased (-2.3%) to 85.886% when pulling a6f0848 on iammart:master into f04beb8 on aimeos:master.

aimeos · 2021-09-28T10:18:43Z

Using codes only leads to the situation that it now works for variant articles but not for selection products any more. You need to fetch all products by their ID or their code.

iammart · 2021-09-28T13:21:28Z

If I update the existing expression to following:

$expr = [
    $search->combine('||', [
        $search->compare('==', 'product.id', array_unique($productIds)),
        $search->compare('==', 'product.code', array_unique($productCodes)),
    ]),
    $search->getConditions()
];
$search->setConditions($search->combine('&&', $expr));

And the getProductItems signature to accept an additional property $productCodes, I am now returned two products in the list.

The addAttributes filters the product from the array list using the productId stored on the order. Do we need to check here on the product type and switch between code and ID in order to get the correct product reference to grab the properties we are attaching to the order product?

Or, would you just filter the list of products by code first, followed by id if null is returned?

aimeos · 2021-09-28T13:44:09Z

It's expected that it returns two products for ordered variant articles (the article and the selection product) and we want to get properties from both if they exists.

You can shorten the code a bit:

$search->add( $search->or( [
    $search->is( 'product.id', '==', array_unique( $productIds ) ),
    $search->is( 'product.code', '==', array_unique( $productCodes ) ),
] );
$manager->search( $search );

iammart · 2021-09-28T15:07:18Z

That makes sense now, I was ignoring the possibility the article could still have properties that might need to be stored against the order product. This now introduces more questions :) Which properties should take priority?

For eg. an article and selection will have the same property options. If one of these is a radio group, either yes/no then there is always a value. This would be stored as two json objects, and result in two comma separated values when printed out in the back office under the order.

I'll make the changes for now and we can review.

aimeos · 2021-09-28T15:13:31Z

If both, selection products and variant articles has the same property type assigned, then article properties should have priority - but I wouldn't care much about that scenario yet

aimeos · 2021-09-28T16:02:53Z

lib/mshoplib/src/MShop/Plugin/Provider/Order/PropertyAdd.php

@@ -121,18 +121,19 @@ public function update( \Aimeos\MW\Observer\Publisher\Iface $order, string $acti
 		if( !is_array( $value ) )
 		{
 			\Aimeos\MW\Common\Base::checkClass( \Aimeos\MShop\Order\Item\Base\Product\Iface::class, $value );
-			return $this->addAttributes( $value, $this->getProductItems( [$value->getProductId()] ), $types );
+			return $this->addAttributes( $value, $this->getProductItems( [$value->getProductId()], [$value->getProductCode()]), $types );
 		}

 		$list = [];


Would be better to use $ids and $codes instead of a two-dimensional array

aimeos · 2021-09-28T16:06:27Z

lib/mshoplib/src/MShop/Plugin/Provider/Order/PropertyAdd.php

 		foreach( $value as $orderProduct )
 		{
-			\Aimeos\MW\Common\Base::checkClass( \Aimeos\MShop\Order\Item\Base\Product\Iface::class, $orderProduct );
-			$list[] = $orderProduct->getProductId();
-		}
+			\Aimeos\MW\Common\Base::checkClass(\Aimeos\MShop\Order\Item\Base\Product\Iface::class, $orderProduct);
+			$list['code'][] = $orderProduct->getProductCode();
+			$list['id'][] = $orderProduct->getProductId();
+        	}


You can shorten this to:

$ids = map( $value )->getProductId(); $codes = map( $value )->getProductCode();

Do we still need to keep the checks for orderProduct(s)? ie. can do you want me to also remove the loop?

Don't think they are really necessary

You can shorten this to:

$ids = map( $value )->getProductId(); $codes = map( $value )->getProductCode();

This is sorcery...it's pretty cool to be able to call a method on an array of objects directly from the list.

jQuery style ;-)

aimeos · 2021-09-28T16:07:58Z

lib/mshoplib/src/MShop/Plugin/Provider/Order/PropertyAdd.php

+	protected function getProductItems( array $productCodes ) : \Aimeos\Map
 	{
 		$manager = \Aimeos\MShop::create( $this->getContext(), 'product' );
 		$search = $manager->filter( true );
 		$expr = [
-			$search->compare( '==', 'product.id', array_unique( $productIds ) ),
+			$search->compare( '==', 'product.code', array_unique( $productCodes ) ),
 			$search->getConditions(),
 		];
 		$search->setConditions( $search->and( $expr ) );


You don't use both, IDs and codes

My bad, I never copied my changes over, im copying my edits from editor to the browser.

Im trying to find more info on how I can run the test suite using phing, I just setup the environment so it has some data to work against.

aimeos · 2021-09-28T16:11:17Z

lib/mshoplib/src/MShop/Plugin/Provider/Order/PropertyAdd.php

+		if($products->count() > 1) {
+			$variant = $products->find(
+				function ( \Aimeos\MShop\Product\Item\Iface $item ) use ( $orderProduct ) : bool {
+					return $item->getCode() === $orderProduct->getProductCode();
+				}
+			);
+		}
+


It's more efficient if you use $map = $products->col( null, 'product.code' ); first and then access the products by code

Nice! Im used to using laravel collections and find myself digging through your docs for functions that are similar that I can use.

The Map methods are very close to Laravel, only if standard PHP uses another name (array_column() instead of Laravel's pluck()), then the Map methods follow the PHP naming.

Im commiting blind in the browser again, if there is anything I miss im really going to need to spend some time to understand and setup phing.

aimeos · 2021-09-29T10:11:14Z

lib/mshoplib/src/MShop/Plugin/Provider/Order/PropertyAdd.php

+		$ids = map( $value )->getProductId()->toArray();
+		$codes = map( $value )->getProductCode()->toArray();


You can use ->unique() already here and pass the Map objects if you change the method signature of getProductsItems() to iterable instead of array

I did do that initially, but my mistake was type hinting as Aimeos\Map, which would have failed for the call we make on the addAttributes method outside of the loop.

aimeos · 2021-09-29T10:45:32Z

lib/mshoplib/src/MShop/Plugin/Provider/Order/PropertyAdd.php

+		$search->add( $search->or( [
+			$search->is( 'product.id', '==', array_unique( $productIds ) ),
+			$search->is( 'product.code', '==', array_unique( $productCodes ) ),
+		] ) );

 		return $manager->search( $search, ['product/property'] );


We could save a lot of $products->col( null, 'product.code' ) mappings in addAttributes() if we already return the merged properties already here. For that, we would need an array of product.code/product.id pairs, search for both like now and return a product.id/propertiy items map. What do you think?

The method should probably change to reflect what it returns then.

getProductItems should probably become getProductProperties.

I guess we would also want to filter the attributes by the types configured here also, which means passing the types to method.

Do you not think passing an array of code/ids into getProductProperties is less explicit than the current approach, as the filter will need to extract the keys and values for the relevant columns to filter on?

We can also pass IDs and codes separately and return a map of product.code as keys and property items as values.
Your are right, we should rename the method, getProperties() is shorter and would fit too

Do we need to return the map keyed as you described?

The changes ive made locally within the now labelled getProperties function, are to accept the types.

I map over the types and return a new map that includes the properties.

It returns something like the following:

Aimeos\Map {#1780 #list: array:3 [ "package-weight" => Aimeos\Map {#1778 #list: array:1 [ 182 => "0.4" ] } "package-net-weight" => Aimeos\Map {#1777 #list: array:1 [ 190 => "0.4" ] } "package-dangerous" => Aimeos\Map {#1772 #list: array:2 [ 191 => "0" 192 => "0" ] } ] }

This is then passed to addAttributes replacing the existing $products parameter, and I iterate over this and attach to the $orderProduct.

The tests are still passing for me with these changes - but could be a false positive due to coverage. I can publish my changes If you like?

Yes, that should work

aimeos · 2021-09-30T09:09:45Z

lib/mshoplib/src/MShop/Plugin/Provider/Order/PropertyAdd.php

+			if ( ($attrItem = $orderProduct->getAttributeItem($type, 'product/property')) === null ) {
+				$attrItem = $this->orderAttrManager->create();


You can shorten this to:

$attrItem = $orderProduct->getAttributeItem( $type, 'product/property' ) ?: $this->orderAttrManager->create();

aimeos · 2021-09-30T09:10:17Z

lib/mshoplib/src/MShop/Plugin/Provider/Order/PropertyAdd.php

-		foreach( $types as $type )
-		{
-			$list = $product->getProperties( $type );
+		$properties->each(function ( $attributes, $type ) use ( &$orderProduct ) {


Where is $type comming from? & isn't necessary for objects

$type is the key in the properties map, it's the type the user configures in the admin for the plugin.

An example of the properties passed into addAttributes is below.

Array ( [package-width] => Aimeos\Map Object ( [list:protected] => Array ( [7] => 15.0 ) [sep:protected] => / ) )

aimeos · 2021-09-30T09:13:44Z

lib/mshoplib/src/MShop/Plugin/Provider/Order/PropertyAdd.php

+		$items = $manager->search( $search, ['product/property'] );
+
+		return map( $types )->map( function ( $type ) use ( $items ) {
+			if( !( $properties = $items->getProperties($type)->collapse() )->empty() ) {
+				return [ $type => $properties ];
+			}
+		} )->filter()->collapse(1);


This should be sufficient to get a product code index and the properties as list of items:

return $manager->search( $search, ['product/property'] )->col( null, 'product.code' )->getProperties( $types );

getProperties requires a string not an array, does it accept comma separated values? If so we would need to implode $types.

aimeos · 2021-10-01T15:32:38Z

$manager->search( $search, ['product/property'] )
->col( null, 'product.code' )
->map( function( $product ) use( $types ) {
foreach ( $types as $type ) {
return $product->getProperties( $type );
}
});

That should work.

iammart · 2021-10-07T08:24:27Z

$manager->search( $search, ['product/property'] )
->col( null, 'product.code' )
->map( function( $product ) use( $types ) {
foreach ( $types as $type ) {
return $product->getProperties( $type );
}
});

That should work.

I thought so too initially, but after I thought about it - it couldn't be :)

We're mapping over the products, and returning properties by type, but this will return early if there is more than a single type as we are not returning a collection of properties by the types given.

For eg. the below would only return a map of properties belonging to the type 'one'

$types = ['one', 'two'];

$properties = $manager->search( $search, ['product/property'] )
    ->col( null, 'product.code' )
    ->map( function( $product ) use( $types ) {
        foreach ( $types as $type ) {
            return $product->getProperties( $type );
        }
    });

I will come back to this when I have some more time, for me what I originally wrote before the refactoring was working and has enabled me to continue.

aimeos · 2022-10-13T08:55:40Z

@iammart Any news on this topic?

Expressions now query on product codes not IDs

77f8aea

Fix formatting

a74644e

Combine properties from both products

b1f67c0

aimeos reviewed Sep 28, 2021

View reviewed changes

iammart and others added 3 commits September 28, 2021 17:55

Implement feedback

688a6bb

Im commiting blind in the browser again, if there is anything I miss im really going to need to spend some time to understand and setup phing.

Fix failing tests

29a2fd8

Fix phpcs errors using ruleset

3c25655

aimeos reviewed Sep 29, 2021

View reviewed changes

Change type hints on method signature to iterable

5dd0830

aimeos reviewed Sep 29, 2021

View reviewed changes

Refactor

07ebf05

aimeos reviewed Sep 30, 2021

View reviewed changes

Move logic from getProperties into addAttributes

a6f0848

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add variant support to PropertyAdd plugin #252

Add variant support to PropertyAdd plugin #252

iammart commented Sep 28, 2021

coveralls commented Sep 28, 2021 •

edited

aimeos commented Sep 28, 2021

iammart commented Sep 28, 2021 •

edited

aimeos commented Sep 28, 2021

iammart commented Sep 28, 2021

aimeos commented Sep 28, 2021

aimeos Sep 28, 2021

aimeos Sep 28, 2021

iammart Sep 28, 2021

aimeos Sep 28, 2021

iammart Sep 28, 2021

aimeos Sep 28, 2021

aimeos Sep 28, 2021

iammart Sep 28, 2021

aimeos Sep 28, 2021

iammart Sep 28, 2021

aimeos Sep 28, 2021

aimeos Sep 29, 2021

iammart Sep 29, 2021

aimeos Sep 29, 2021

iammart Sep 29, 2021

aimeos Sep 29, 2021

iammart Sep 29, 2021

aimeos Sep 30, 2021

aimeos Sep 30, 2021

aimeos Sep 30, 2021

iammart Sep 30, 2021

aimeos Sep 30, 2021

iammart Sep 30, 2021

aimeos commented Oct 1, 2021

iammart commented Oct 7, 2021

aimeos commented Oct 13, 2022

		$ids = map( $value )->getProductId()->toArray();
		$codes = map( $value )->getProductCode()->toArray();

		if ( ($attrItem = $orderProduct->getAttributeItem($type, 'product/property')) === null ) {
		$attrItem = $this->orderAttrManager->create();

Add variant support to PropertyAdd plugin #252

Are you sure you want to change the base?

Add variant support to PropertyAdd plugin #252

Conversation

iammart commented Sep 28, 2021

coveralls commented Sep 28, 2021 • edited

aimeos commented Sep 28, 2021

iammart commented Sep 28, 2021 • edited

aimeos commented Sep 28, 2021

iammart commented Sep 28, 2021

aimeos commented Sep 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aimeos commented Oct 1, 2021

iammart commented Oct 7, 2021

aimeos commented Oct 13, 2022

coveralls commented Sep 28, 2021 •

edited

iammart commented Sep 28, 2021 •

edited