Skip to content

[GraphQl] 28200 fixed issue with attribute label #29180

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

Closed
wants to merge 25 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
24ebc2e
fixed issue with attribute label
Usik2203 Jul 16, 2020
0d904ab
checking attribute
Usik2203 Jul 17, 2020
3cfc907
apply some fixes related with failed tests
Usik2203 Jul 17, 2020
6356cfa
Merge branch '2.4-develop' into graphql-28200
Usik2203 Jul 17, 2020
71132b1
fixed some issues
Usik2203 Jul 28, 2020
21b8964
Merge branch '2.4-develop' into graphql-28200
Usik2203 Jul 28, 2020
19bb551
Merge branch '2.4-develop' into graphql-28200
Usik2203 Jul 28, 2020
e736f16
revert some changes due to faild tests
Usik2203 Jul 29, 2020
dac9483
minor fix
Usik2203 Jul 29, 2020
de49f18
added test
Usik2203 Jul 29, 2020
04198e8
fix static issue
Usik2203 Jul 29, 2020
660c9bc
Merge branch '2.4-develop' into graphql-28200
Usik2203 Jul 29, 2020
18f2b08
added changes to test
Usik2203 Jul 30, 2020
9e85947
Merge branch '2.4-develop' into graphql-28200
Usik2203 Jul 30, 2020
61deae7
add minor fix
Usik2203 Aug 5, 2020
7e1e74e
Merge branch '2.4-develop' into graphql-28200
Usik2203 Aug 5, 2020
392b096
Merge branch '2.4-develop' into graphql-28200
Usik2203 Aug 6, 2020
c6b3577
CR recommendations
Usik2203 Oct 24, 2020
4f38034
Merge branch '2.4-develop' into graphql-28200
Usik2203 Oct 24, 2020
9aa76d2
clean cache fix
Usik2203 Oct 24, 2020
ec8e1d2
Update dev/tests/api-functional/testsuite/Magento/GraphQl/Catalog/Pro…
Usik2203 Oct 24, 2020
a092ff5
Update dev/tests/api-functional/testsuite/Magento/GraphQl/Catalog/Pro…
Usik2203 Oct 24, 2020
008fb16
Update dev/tests/api-functional/testsuite/Magento/GraphQl/Catalog/Pro…
Usik2203 Oct 26, 2020
6666c40
Update app/code/Magento/CatalogGraphQl/Model/Resolver/Products/Query/…
Usik2203 Oct 26, 2020
8b6a3cd
Update app/code/Magento/CatalogGraphQl/Model/Resolver/Products/Query/…
Usik2203 Oct 26, 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
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,23 @@

namespace Magento\CatalogGraphQl\Model\Resolver\Products\Query;

use Magento\Catalog\Model\Product;
use Magento\CatalogGraphQl\DataProvider\Product\SearchCriteriaBuilder;
use Magento\CatalogGraphQl\Model\Resolver\Products\DataProvider\ProductSearch;
use Magento\CatalogGraphQl\Model\Resolver\Products\SearchResult;
use Magento\CatalogGraphQl\Model\Resolver\Products\SearchResultFactory;
use Magento\Framework\Api\Search\SearchCriteriaInterface;
use Magento\Framework\Exception\InputException;
use Magento\Framework\GraphQl\Schema\Type\ResolveInfo;
use Magento\Framework\Phrase;
use Magento\GraphQl\Model\Query\ContextInterface;
use Magento\Search\Api\SearchInterface;
use Magento\Search\Model\Search\PageSizeProvider;

/**
* Full text search for catalog using given search criteria.
*
* @SuppressWarnings(PHPMD.CouplingBetweenObjects)
*/
class Search implements ProductQueryInterface
{
Expand Down Expand Up @@ -115,10 +119,17 @@ public function getResult(
$totalPages = $realPageSize ? ((int)ceil($searchResults->getTotalCount() / $realPageSize)) : 0;

$productArray = [];
/** @var \Magento\Catalog\Model\Product $product */
/** @var Product $product */
foreach ($searchResults->getItems() as $product) {
$productArray[$product->getId()] = $product->getData();
$productArray[$product->getId()]['model'] = $product;
foreach ($queryFields as $field) {
$productArray[$product->getId()][$field] = $this->getAttributeValue(
$product,
$productArray,
$field
);
}
}

return $this->searchResultFactory->create(
Expand Down Expand Up @@ -148,4 +159,27 @@ private function buildSearchCriteria(array $args, ResolveInfo $info): SearchCrit

return $searchCriteria;
}

/**
* Get product attribute value
*
* @param Product $product
* @param array $productArray
* @param string $field
*
* @return string|null|int|bool
*/
private function getAttributeValue(Product $product, array $productArray, string $field)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest specifying the return type

Suggested change
private function getAttributeValue(Product $product, array $productArray, string $field)
private function getAttributeValue(Product $product, array $productArray, string $field): ?string

Copy link
Contributor Author

@Usik2203 Usik2203 Oct 26, 2020

Choose a reason for hiding this comment

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

Hi @rogyar
I can't apply this change because attribute value can have int and bool values
I am providing this test as example
Screenshot 2020-10-25 at 01 06 52

Thanks

{
if ($attribute = $product->getResource()->getAttribute($field)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder what performance impact will this have
Ideally we would have a collection of all attributes needed in the query, load it once and use it for any product that needs it, like we do with products where first we add to collection and we return a function instead of loading at the same time getResource()->getAttribute($field))
See this code how to do this pattern, it's the same concept: \Magento\ConfigurableProductGraphQl\Model\Resolver\Options::resolve

Also we tried to stay away from $product->getResource()
instead we would inject the resource - this way it will make the performance problem more obvious
I can try to run this to see how much degradation it has, though I don't think we have by default such attributes that we test

$attributeValue = $attribute->getFrontend()->getValue($product);
if ($attributeValue && !($attributeValue instanceof Phrase)) {
return $attributeValue;
Copy link
Contributor

Choose a reason for hiding this comment

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

is this how we translate the label? or what does $attribute->getFrontend()->getValue($product) do?
it's best to inject frontend class ..whatever that class is...
Hope is not luma related..

} else {
return $productArray[$product->getId()][$field] ?? null;
}
}

return "";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -813,14 +813,12 @@ private function assertOptions($product, $actualResponse)
*/
private function assertBaseFields($product, $actualResponse)
{

$assertionMap = [
['response_field' => 'attribute_set_id', 'expected_value' => $product->getAttributeSetId()],
['response_field' => 'created_at', 'expected_value' => $product->getCreatedAt()],
['response_field' => 'id', 'expected_value' => $product->getId()],
['response_field' => 'name', 'expected_value' => $product->getName()],
['response_field' => 'price', 'expected_value' =>
[
['response_field' => 'price', 'expected_value' => [
'minimalPrice' => [
'amount' => [
'value' => $product->getSpecialPrice(),
Expand Down Expand Up @@ -920,7 +918,9 @@ private function assertEavAttributes($product, $actualResponse)
'expected_value' => $expectedAttribute ? $expectedAttribute->getValue() : null
];
}

if ($assertionMap[4]['expected_value'] === "US") {
$assertionMap[4]['expected_value'] = 'United States';
}
$this->assertResponseFields($actualResponse, $assertionMap);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types=1);

namespace Magento\GraphQl\Catalog;

use Magento\Framework\Exception\LocalizedException;
use Magento\TestFramework\Helper\Bootstrap;
use Magento\TestFramework\Helper\CacheCleaner;
use Magento\TestFramework\TestCase\GraphQlAbstract;

/**
* Test class for product with multiselect attributes
*/
class ProductWithMultiselectAttributeTest extends GraphQlAbstract
{
/**
* Check correct displaying for multiselect attributes
*
* @magentoApiDataFixture Magento/Catalog/_files/products_with_multiselect_attribute.php
*/
public function testQueryProductWithMultiselectAttribute()
{
$this->reIndexAndCleanCache();
$query = <<<QUERY
{
products(filter: {sku: {eq: "simple_ms_2" }}) {
items {
name
multiselect_attribute
}
}
}
QUERY;
$response = $this->graphQlQuery($query);
$this->assertStringContainsString(
'Option 2, Option 3, Option 4 "!@#$%^&*',
$response['products']['items'][0]['multiselect_attribute']
);
}

/**
* Reindex and clean cache
*
* @return void
* @throws LocalizedException
*/
private function reIndexAndCleanCache() : void
{
$appDir = dirname(Bootstrap::getInstance()->getAppTempDir());
$out = '';
// phpcs:ignore Magento2.Security.InsecureFunction
exec("php -f {$appDir}/bin/magento indexer:reindex", $out);
$this->cleanCache();
}
}