[GraphQl] 28200 fixed issue with attribute label #29180

Usik2203 · 2020-07-16T17:47:50Z

Description (*)

I am using magento 2.4 with Sample Data
Product for testing with SKU 24-UG07

Result Before Fix

I see option_id of selected product attribute

Result After Fix

I see value of selected product attribute

Related Pull Requests

Fixed Issues (if relevant)

Fixes Graphql products query returns only ids for dropdows or multiselect fields #28200>

Manual testing scenarios (*)

...
...

Questions or comments

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds are green)

m2-assistant · 2020-07-16T17:47:52Z

Hi @Usik2203. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

@magento run all tests - run or re-run all required tests against the PR changes
@magento run <test-build(s)> - run or re-run specific test build(s)
For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

Database Compare
Functional Tests CE
Functional Tests EE,
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

Usik2203 · 2020-07-17T08:13:07Z

@magento run all tests

Usik2203 · 2020-07-17T14:00:48Z

@magento run all tests

Usik2203 · 2020-07-17T14:34:33Z

@magento give me 2.4-develop instance

magento-engcom-team · 2020-07-17T14:34:38Z

Hi @Usik2203. Thank you for your request. I'm working on Magento 2.4-develop instance for you

prabhuram93 · 2020-07-17T14:36:12Z

@Usik2203 Can you please add tests to cover this scenario. Thanks!

magento-engcom-team · 2020-07-17T14:36:28Z

Hi @Usik2203, here is your Magento instance.
Admin access: https://i-29180-2-4-develop.instances.magento-community.engineering/admin_424e
Login: 43548653 Password: e2197132f81f
Instance will be terminated in up to 3 hours.

avattam06 · 2020-07-17T14:44:10Z

@Usik2203, Please check and fix the static test failures.

rogyar

Hi @Usik2203. Thank you for your collaboration. Please, check my recommendations below. Also, is there a chance to have this fix covered by a test?

app/code/Magento/CatalogGraphQl/Model/Resolver/Products/Query/Search.php

rogyar · 2020-07-18T14:36:22Z

app/code/Magento/CatalogGraphQl/Model/Resolver/Products/Query/Search.php

+            if ($attributeValue && !($attributeValue instanceof Phrase) ) {
+               return $attributeValue;
+            } else {
+                return isset($productArray[$product->getId()][$field])? $productArray[$product->getId()][$field] : null;


We may simplify this structure with null coalescing operator.

Suggested change

return isset($productArray[$product->getId()][$field])? $productArray[$product->getId()][$field] : null;

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

If we will use simplify condition we can get error "Undefined index" in case where product don't have requested attribute. isset() helps to avoid this

The null coalescing operator should not generate the "Undefined Index" notice. Please, double-check it. You may refer to the official doc for more details

https://www.php.net/manual/en/migration70.new-features.php#migration70.new-features.null-coalesce-op

Hi @rogyar , Yes you are right .
I have applied this changes
Thanks

...sts/api-functional/testsuite/Magento/GraphQl/Catalog/ProductWithMultiselectAttributeTest.php

…ductWithMultiselectAttributeTest.php

Usik2203 · 2020-10-24T19:40:46Z

@magento run WebAPI Tests

...sts/api-functional/testsuite/Magento/GraphQl/Catalog/ProductWithMultiselectAttributeTest.php

…ductWithMultiselectAttributeTest.php

app/code/Magento/CatalogGraphQl/Model/Resolver/Products/Query/Search.php

…Search.php

app/code/Magento/CatalogGraphQl/Model/Resolver/Products/Query/Search.php

…Search.php

Usik2203 · 2020-10-26T14:25:39Z

@magento run WebAPI Tests

Usik2203 · 2020-10-26T16:06:25Z

@magento run all tests

Usik2203 · 2020-10-26T18:12:15Z

@magento run Integration Tests

magento-engcom-team · 2020-11-03T17:17:56Z

Hi @rogyar, thank you for the review.
ENGCOM-8410 has been created to process this Pull Request

cpartica · 2020-11-25T21:24:55Z

@Usik2203
https://user-images.githubusercontent.com/13517181/87704280-d9270b00-c7a4-11ea-933c-0ea3856fb9f3.png

isn't it better to have an array of attributes instead of coma separated values?
In the very near we are supposed to not have these attributes in the same level as static
check this out https://github.com/magento/architecture/pull/456/files

cpartica · 2020-11-25T21:29:38Z

app/code/Magento/CatalogGraphQl/Model/Resolver/Products/Query/Search.php

+     */
+    private function getAttributeValue(Product $product, array $productArray, string $field)
+    {
+        if ($attribute = $product->getResource()->getAttribute($field)) {


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

cpartica · 2020-11-25T21:30:59Z

app/code/Magento/CatalogGraphQl/Model/Resolver/Products/Query/Search.php

+        if ($attribute = $product->getResource()->getAttribute($field)) {
+            $attributeValue =  $attribute->getFrontend()->getValue($product);
+            if ($attributeValue && !($attributeValue instanceof Phrase)) {
+                return $attributeValue;


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..

gabrieldagama · 2021-03-23T16:17:54Z

Hi @Usik2203, thanks for contributing!

Closing this one in favor of #31622 which implements a key-value approach, more suitable for this problem.

m2-assistant · 2021-03-23T16:17:56Z

Hi @Usik2203, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

fixed issue with attribute label

24ebc2e

ghost added the Progress: pending review label Jul 16, 2020

magento-engcom-team added Component: CatalogGraphQl Release Line: 2.4 Partner: Atwix Pull Request is created by partner Atwix partners-contribution Pull Request is created by Magento Partner labels Jul 16, 2020

checking attribute

0d904ab

Usik2203 force-pushed the graphql-28200 branch from 5d4b65f to 0d904ab Compare July 17, 2020 08:12

apply some fixes related with failed tests

3cfc907

Usik2203 force-pushed the graphql-28200 branch from 231bfe7 to 3cfc907 Compare July 17, 2020 13:59

Merge branch '2.4-develop' into graphql-28200

6356cfa

nrkapoor assigned Usik2203 Jul 17, 2020

nrkapoor requested review from danielrenaud and cpartica July 17, 2020 14:31

nrkapoor added this to the 2.4.1 milestone Jul 17, 2020

nrkapoor requested review from prabhuram93, avattam06 and dthampy July 17, 2020 14:31

nrkapoor added the Project: GraphQL label Jul 17, 2020

rogyar self-assigned this Jul 18, 2020

rogyar requested changes Jul 18, 2020

View reviewed changes

Usik2203 commented Oct 24, 2020

View reviewed changes

...sts/api-functional/testsuite/Magento/GraphQl/Catalog/ProductWithMultiselectAttributeTest.php Outdated Show resolved Hide resolved

Update dev/tests/api-functional/testsuite/Magento/GraphQl/Catalog/Pro…

a092ff5

…ductWithMultiselectAttributeTest.php

Usik2203 commented Oct 26, 2020

View reviewed changes

...sts/api-functional/testsuite/Magento/GraphQl/Catalog/ProductWithMultiselectAttributeTest.php Outdated Show resolved Hide resolved

Update dev/tests/api-functional/testsuite/Magento/GraphQl/Catalog/Pro…

008fb16

…ductWithMultiselectAttributeTest.php

Usik2203 commented Oct 26, 2020

View reviewed changes

app/code/Magento/CatalogGraphQl/Model/Resolver/Products/Query/Search.php Outdated Show resolved Hide resolved

Update app/code/Magento/CatalogGraphQl/Model/Resolver/Products/Query/…

6666c40

…Search.php

Usik2203 commented Oct 26, 2020

View reviewed changes

app/code/Magento/CatalogGraphQl/Model/Resolver/Products/Query/Search.php Outdated Show resolved Hide resolved

Update app/code/Magento/CatalogGraphQl/Model/Resolver/Products/Query/…

8b6a3cd

…Search.php

rogyar approved these changes Oct 30, 2020

View reviewed changes

ghost added Progress: ready for testing and removed Progress: needs update labels Oct 30, 2020

cpartica requested changes Nov 25, 2020

View reviewed changes

m2-community-project bot assigned cpartica Nov 25, 2020

m2-community-project bot added Progress: needs update and removed Progress: ready for testing labels Nov 25, 2020

gabrieldagama closed this Mar 23, 2021

m2-community-project bot removed the Progress: needs update label Mar 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GraphQl] 28200 fixed issue with attribute label #29180

[GraphQl] 28200 fixed issue with attribute label #29180

Usik2203 commented Jul 16, 2020

m2-assistant bot commented Jul 16, 2020

Usik2203 commented Jul 17, 2020

Usik2203 commented Jul 17, 2020

Usik2203 commented Jul 17, 2020

magento-engcom-team commented Jul 17, 2020

prabhuram93 commented Jul 17, 2020

magento-engcom-team commented Jul 17, 2020

avattam06 commented Jul 17, 2020

rogyar left a comment

rogyar Jul 18, 2020

Usik2203 Jul 29, 2020

rogyar Aug 3, 2020 •

edited

Loading

Usik2203 Aug 6, 2020

Usik2203 commented Oct 24, 2020

Usik2203 commented Oct 26, 2020

Usik2203 commented Oct 26, 2020

Usik2203 commented Oct 26, 2020

magento-engcom-team commented Nov 3, 2020

cpartica commented Nov 25, 2020

cpartica Nov 25, 2020

cpartica Nov 25, 2020

gabrieldagama commented Mar 23, 2021

m2-assistant bot commented Mar 23, 2021

	return isset($productArray[$product->getId()][$field])? $productArray[$product->getId()][$field] : null;
	return $productArray[$product->getId()][$field] ?? null;

[GraphQl] 28200 fixed issue with attribute label #29180

[GraphQl] 28200 fixed issue with attribute label #29180

Conversation

Usik2203 commented Jul 16, 2020

Description (*)

Result Before Fix

Result After Fix

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

Questions or comments

Contribution checklist (*)

m2-assistant bot commented Jul 16, 2020

Usik2203 commented Jul 17, 2020

Usik2203 commented Jul 17, 2020

Usik2203 commented Jul 17, 2020

magento-engcom-team commented Jul 17, 2020

prabhuram93 commented Jul 17, 2020

magento-engcom-team commented Jul 17, 2020

avattam06 commented Jul 17, 2020

rogyar left a comment

Choose a reason for hiding this comment

rogyar Jul 18, 2020

Choose a reason for hiding this comment

Usik2203 Jul 29, 2020

Choose a reason for hiding this comment

rogyar Aug 3, 2020 • edited Loading

Choose a reason for hiding this comment

Usik2203 Aug 6, 2020

Choose a reason for hiding this comment

Usik2203 commented Oct 24, 2020

Usik2203 commented Oct 26, 2020

Usik2203 commented Oct 26, 2020

Usik2203 commented Oct 26, 2020

magento-engcom-team commented Nov 3, 2020

cpartica commented Nov 25, 2020

cpartica Nov 25, 2020

Choose a reason for hiding this comment

cpartica Nov 25, 2020

Choose a reason for hiding this comment

gabrieldagama commented Mar 23, 2021

m2-assistant bot commented Mar 23, 2021

rogyar Aug 3, 2020 •

edited

Loading