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.

Sign up for GitHub

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

Jump to bottom

Add variant support to PropertyAdd plugin #252

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Add variant support to PropertyAdd plugin #252

Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
77f8aea
Expressions now query on product codes not IDs
iammart Sep 28, 2021
a74644e
Fix formatting
iammart Sep 28, 2021
b1f67c0
Combine properties from both products
iammart Sep 28, 2021
688a6bb
Implement feedback
iammart Sep 28, 2021
29a2fd8
Fix failing tests
Sep 28, 2021
3c25655
Fix phpcs errors using ruleset
Sep 29, 2021
5dd0830
Change type hints on method signature to iterable
Sep 29, 2021
07ebf05
Refactor
Sep 30, 2021
a6f0848
Move logic from getProperties into addAttributes
Sep 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 22 additions & 17 deletions lib/mshoplib/src/MShop/Plugin/Provider/Order/PropertyAdd.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,18 +121,13 @@ 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 = [];
$ids = map( $value )->getProductId()->toArray();
$codes = map( $value )->getProductCode()->toArray();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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

$products = $this->getProductItems( $list );
$products = $this->getProductItems( $ids, $codes );

foreach( $value as $key => $orderProduct ) {
$value[$key] = $this->addAttributes( $orderProduct, $products, $types );
Expand All @@ -157,10 +152,19 @@ protected function addAttributes( \Aimeos\MShop\Order\Item\Base\Product\Iface $o
return $orderProduct;
}

if( $products->count() > 1 ) {
$variant = $products->col( null, 'product.code' )
->get( $orderProduct->getProductCode() );
}

foreach( $types as $type )
{
$list = $product->getProperties( $type );

if( $variant ?? false ) {
$list->union( $variant->getProperties( $type ) );
}

if( !$list->isEmpty() )
{
if( ( $attrItem = $orderProduct->getAttributeItem( $type, 'product/property' ) ) === null ) {
Expand All @@ -181,18 +185,19 @@ protected function addAttributes( \Aimeos\MShop\Order\Item\Base\Product\Iface $o
/**
* Returns the product items for the given product IDs limited by the map of properties
*
* @param string[] $productIds List of product IDs
* @param string[] $productsIds List of product IDs
* @param string[] $productCodes List of product codes
* @return \Aimeos\Map List of items implementing \Aimeos\MShop\Product\Item\Iface with IDs as keys
*/
protected function getProductItems( array $productIds ) : \Aimeos\Map
protected function getProductItems( array $productIds, array $productCodes ) : \Aimeos\Map
{
$manager = \Aimeos\MShop::create( $this->getContext(), 'product' );
$manager = \Aimeos\MShop::create($this->getContext(), 'product');
$search = $manager->filter( true );
$expr = [
$search->compare( '==', 'product.id', array_unique( $productIds ) ),
$search->getConditions(),
];
$search->setConditions( $search->and( $expr ) );

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that should work

}
Expand Down