Skip to content

[GraphQl] 29168 TierPrices filtering #29675

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

Merged
magento-engcom-team merged 14 commits into from
Nov 13, 2020
Merged

[GraphQl] 29168 TierPrices filtering #29675

Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
017c44e
add filter for tier price value
Usik2203 Aug 19, 2020
603bc9f
add test coverage and some changes
Usik2203 Aug 19, 2020
257ba74
Merge branch '2.4-develop' into 29168-graphql-tier-price
Usik2203 Aug 19, 2020
4f978a4
add some changes to test
Usik2203 Aug 21, 2020
2cfd8a0
remove redundant foreach
Usik2203 Aug 21, 2020
2d9740d
minor fix
Usik2203 Aug 21, 2020
6c285ad
moved part of the logic to separate method
Usik2203 Sep 2, 2020
d535303
minor fix
Usik2203 Sep 3, 2020
f498653
add some changes in method names
Usik2203 Sep 6, 2020
b2021d5
Merge branch '2.4-develop' into 29168-graphql-tier-price
Usik2203 Nov 11, 2020
30901be
CR recommendations
Usik2203 Nov 11, 2020
6b4383b
minor changes
Usik2203 Nov 11, 2020
58eaf18
Revert changes
Usik2203 Nov 11, 2020
9f9164f
Merge branch '2.4-develop' into 29168-graphql-tier-price
Usik2203 Nov 12, 2020
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
59 changes: 50 additions & 9 deletions app/code/Magento/CatalogCustomerGraphQl/Model/Resolver/PriceTiers.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,19 @@

namespace Magento\CatalogCustomerGraphQl\Model\Resolver;

use Magento\Catalog\Api\Data\ProductTierPriceInterface;
use Magento\CatalogCustomerGraphQl\Model\Resolver\Customer\GetCustomerGroup;
use Magento\CatalogCustomerGraphQl\Model\Resolver\Product\Price\Tiers;
use Magento\CatalogCustomerGraphQl\Model\Resolver\Product\Price\TiersFactory;
use Magento\CatalogGraphQl\Model\Resolver\Product\Price\Discount;
use Magento\CatalogGraphQl\Model\Resolver\Product\Price\ProviderPool as PriceProviderPool;
use Magento\Framework\Exception\LocalizedException;
use Magento\Framework\GraphQl\Config\Element\Field;
use Magento\Framework\GraphQl\Query\Resolver\ValueFactory;
use Magento\Framework\GraphQl\Query\ResolverInterface;
use Magento\Framework\GraphQl\Schema\Type\ResolveInfo;
use Magento\Framework\Exception\LocalizedException;
use Magento\Framework\GraphQl\Query\Resolver\ValueFactory;
use Magento\Framework\Pricing\PriceCurrencyInterface;
use Magento\CatalogCustomerGraphQl\Model\Resolver\Product\Price\Tiers;
use Magento\CatalogCustomerGraphQl\Model\Resolver\Product\Price\TiersFactory;
use Magento\CatalogCustomerGraphQl\Model\Resolver\Customer\GetCustomerGroup;
use Magento\Store\Api\Data\StoreInterface;
use Magento\CatalogGraphQl\Model\Resolver\Product\Price\Discount;
use Magento\CatalogGraphQl\Model\Resolver\Product\Price\ProviderPool as PriceProviderPool;
use Magento\Catalog\Api\Data\ProductTierPriceInterface;

/**
* Resolver for price_tiers
Expand Down Expand Up @@ -120,7 +120,7 @@ function () use ($productId, $context) {

Copy link
Contributor

@cpartica cpartica Nov 11, 2020

Choose a reason for hiding this comment

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

can you fix types on $this->tiers->addProductFilter($productId) ? \Magento\CatalogCustomerGraphQl\Model\Resolver\Product\Price\Tiers::addProductFilter has no strict types
public function addProductFilter($productId): void
would be
public function addProductFilter(int $productId): void

Copy link
Contributor Author

@Usik2203 Usik2203 Nov 11, 2020

Choose a reason for hiding this comment

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

Done

$productPrice = $this->tiers->getProductRegularPrice($productId) ?? 0.0;
$tierPrices = $this->tiers->getProductTierPrices($productId) ?? [];

$tierPrices = $this->filterTierPrices($tierPrices);
return $this->formatProductTierPrices($tierPrices, $productPrice, $store);
}
);
Expand Down Expand Up @@ -158,4 +158,45 @@ private function formatProductTierPrices(array $tierPrices, float $productPrice,
}
return $tiers;
}

/**
* Select a lower price for each quantity
*
* @param ProductTierPriceInterface[] $tierPrices
*
* @return array
*/
private function filterTierPrices(array $tierPrices): array
{
$qtyCache = [];
foreach ($tierPrices as $item => &$price) {
Copy link
Contributor

@rogyar rogyar Aug 19, 2020

Choose a reason for hiding this comment

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

We are passing the &$price variable by reference but it does not seem like we modify it somehow in the loop below. But I may miss something.

Copy link
Contributor Author

@Usik2203 Usik2203 Aug 19, 2020

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

@pmarjan pmarjan Aug 21, 2020

Choose a reason for hiding this comment

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

@Usik2203
Both methods, filterTierPrices and formatProductTierPrices, which are called one after another, are using foreach to loop through the tier prices.

Given that this code executes when a query searches for products, please consider a situation where we might have large b2b catalog with plenty tiers for different customer groups and a relatively short search phrase, yielding plenty of results.
Having to loop twice through all tier prices in all search results, might lead to performance degradation.

I am wondering, could these two methods be somehow combined, so that we get the same results using foreach only once?

Copy link
Contributor Author

@Usik2203 Usik2203 Aug 22, 2020

Choose a reason for hiding this comment

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

@pmarjan Done

Copy link
Contributor

@pmarjan pmarjan Aug 26, 2020

Choose a reason for hiding this comment

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

@Usik2203
From "clarity of intent" perspective, when one reads through the "resolve" function alone, it seems as though formatProductTierPrices function does only what its name says. In fact this function's comment seems to support that.
But, that is misleading.

So, I am wondering can we have a function named like filterAndFormatProductTierPrices, that invokes a foreach loop, and inside this loop, one below the other filterTierPrices and formatProductTierPrices are called?

Check for example, when an unfortunate situation like we have, calls for two different things to be done in the same method:
League\Flysystem\Filesystem::readAndDelete()
internally first calls
$this->read()
and later
$this->delete()

Copy link
Contributor Author

@Usik2203 Usik2203 Sep 3, 2020

Choose a reason for hiding this comment

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

@pmarjan Done

$qty = $price->getQty();
if (isset($qtyCache[$qty])) {
$priceQty = $qtyCache[$qty];
if ($this->isFirstPriceBetter((float)$price->getValue(), (float)$tierPrices[$priceQty]->getValue())) {
unset($tierPrices[$priceQty]);
$qtyCache[$priceQty] = $item;
} else {
unset($tierPrices[$item]);
}
} else {
$qtyCache[$qty] = $item;
}
}

return $tierPrices;
}

/**
* Returns true if first price is better
*
* @param float $firstPrice
* @param float $secondPrice
*
* @return bool
*/
private function isFirstPriceBetter(float $firstPrice, float $secondPrice): bool
Copy link
Contributor

@rogyar rogyar Aug 19, 2020

Choose a reason for hiding this comment

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

I would not suggest creating a separate method for such a simple comparison. It makes code harder to read and introduces semantical complexity. In the future, if we will need more complex comparison we can always create a separate method. But on this stage, I would keep things simple

Copy link
Contributor Author

@Usik2203 Usik2203 Aug 19, 2020

Choose a reason for hiding this comment

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

Done

{
return $firstPrice < $secondPrice;
}
}