Add fallback for Product_links position attribute if not set in request #12650

mohammedsalem · 2017-12-12T16:04:57Z

By setting product_links index on a product via API call, only the first two of each link type are shown in the backend or a in the response of a GET request, although they are correctly added to the database.
The problem was that if the position wasn't set there's no default value or a fallback set in the table "catalog_product_link_attribute_int" for the position attribute.

Description

in SaveHandler class from Magento\Catalog\Model\Product\Link
A check goes in all Products link by type to check if the position were set for all products of this type, else if any wasn't set then do a fallback by array position.

Manual testing scenarios

Before applying PR:

POST/PUT request to following API endpoint with more than 2 linked products (product_links) per link_type without a position: http://dev.domian.com/rest/V1/products/{sku}
Perform a GET request to http://dev.domian.com/rest/V1/products/{sku} - you should only see the first two linked products per link type

After applying PR:

POST/PUT request to following API endpoint with more than 2 linked products (product_links) per link_type without a position: http://dev.domian.com/rest/V1/products/{sku}
Perform a GET request to http://dev.domian.com/rest/V1/products/{sku} - you should see all linked products per link type with a position set.

If the position is set manually in the POST/PUT request for all products of a specific link type, this positions should be taken into account.

… was not set in API request

magento-cicd2 · 2017-12-12T16:04:59Z

All committers have signed the CLA.

…on the same line

…:mohammedsalem/magento2 into bug-fix-product-links-position-attribute

dmanners · 2017-12-13T09:21:56Z

@mohammedsalem looks like @mzeis is going to process this pull request for you. If you need anything else just drop me or Matthias a message here. Thanks.

mohammedsalem · 2017-12-13T09:23:55Z

@dmanners many thanks, for sure gonna do :)

mzeis

Hi @mohammedsalem,

thank you very much for your PR! Please have a look at my code review, there is a minor change I requested.

mzeis · 2017-12-13T19:38:06Z

app/code/Magento/Catalog/Model/Product/Link/SaveHandler.php

@@ -18,7 +18,7 @@ class SaveHandler
    /**
     * @var ProductLinkRepositoryInterface
     */
-    protected $productLinkRepository;
+    protected $productLinkRepo;


Please don't rename a protected property as this breaks backwards compatibility (source). I know you get a warning regarding the variable name length, but backwards compatibility is more important than code style.

mzeis · 2017-12-13T19:49:12Z

app/code/Magento/Catalog/Model/Product/Link/SaveHandler.php

+     * @param $linksByType
+     * @return array
+     */
+    private function isPositionsSet($linksByType)


Please rename this to isPositionSet() for consistency.

@mzeis Changes made
the idea of the plural positions naming instead of singular was that the function returns an array of multi product link type, on the same time wanted to keep the bool naming convention using 'is' or 'has'.

mzeis

I have the feeling this code could be a little bit more simplified. I'm wondering whether we really need so many iterations to find the links that need their position set.

Please can you consider this change? Also see my comments regarding variable naming.

Thanks!

mzeis · 2017-12-20T17:08:29Z

app/code/Magento/Catalog/Model/Product/Link/SaveHandler.php

+     * @param $linksByType
+     * @return array
+     */
+    private function isPositionSet($linksByType)


Thinking some more about it, it's confusing that a method name starting with is returns an array. It suggests the result should be boolean.

Naming convention will be improved

mzeis · 2017-12-20T17:09:30Z

app/code/Magento/Catalog/Model/Product/Link/SaveHandler.php

+        }
+
+        // Do check
+        $hasPositionLinkType = $this->isPositionSet($linksByType);


It's confusing that a variable name starting with has contains an array. The name suggests that it contains a boolean value.

it contains an array of boolean values per link type, I had the case that the positions were set for the products in "Related" but not set for products under "Cross sell" and so on,

the same that under on Link type got the case of some products had position attribute and others don't, therefor I had to check if at least one product doesn't have a position under a product Link type all positions under this type will be ignored and user array index instead.

regarding the question whether we need that amount of iterations,
This came from too many use cases and variants of sending data, as I said about some link types aren't used at all others have positions but not for all products, another type with full defined products positions.

mohammedsalem · 2017-12-25T17:04:59Z

I'll continue working in January because of the holidays,
Merry Christmas 🎄🌟

mzeis · 2018-01-20T18:00:28Z

Hi @mohammedsalem, please can you provide a way to re-produce the initial problem by posting the calls starting from a fresh 2.2 install? Sorry I only come up with this now.

mzeis

Thanks for your refactoring! I have two more recommendations, please have a look at the review.

mzeis · 2018-01-20T18:47:45Z

app/code/Magento/Catalog/Model/Product/Link/SaveHandler.php

+
+        // Build links per type
+        $linksByType = [];
+        foreach ($productLinks as $link) {


After your last refactoring, we don't need the variable $productLinks in line 72 anymore. Call $entity->getProductLinks() directly in line 72. By doing this, we adhere to the best practice that a variable shouldn't be changed anymore after initialisation.

mzeis · 2018-01-20T18:48:08Z

app/code/Magento/Catalog/Model/Product/Link/SaveHandler.php

+     * @param $links
+     * @return bool
+     */
+    public function hasPosition($links)


Please change the scope - methods should be private whenever possible. Also, we now that $links will be an array so we can use type hints.

mohammedsalem · 2018-01-21T11:48:55Z

PR-usecase.json.txt
@mzeis here you are sample request of adding product using Rest API.

magento-engcom-team · 2018-01-22T19:45:31Z

@mohammedsalem thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

…ot set in request #12650

ihor-sviziev · 2018-01-31T07:07:15Z

FYI In this PR introduced new dependency to constructor, that wasn't used. I removed it inside #13436

if position index was not set in API request Original PR: magento#12650

mzeis · 2018-01-31T07:59:20Z

@mohammedsalem Thanks again for your contribution! Your PR has been merged into 2.2-develop.

@ihor-sviziev Thanks for catching this! The dependency was needed first, but not anymore after the refactoring.

🐛 add fallback for Product_links position attribute if position index…

944adbf

… was not set in API request

Mohammad HAJ SALEM added 6 commits December 13, 2017 08:13

👌 shortening variable name

aa5bc4f

🐛 fix variable re-naming

828f18b

👌 drop multiple empty lines

005e3eb

👌 closing parenthesis and the opening brace of a multi-line function …

27cc0b6

…on the same line

👌 closing parenthesis and the opening brace of a multi-line function …

12653a1

…on the same line

Merge branch 'bug-fix-product-links-position-attribute' of github.com…

6e93513

…:mohammedsalem/magento2 into bug-fix-product-links-position-attribute

mzeis self-assigned this Dec 13, 2017

mzeis added this to the December 2017 milestone Dec 13, 2017

mzeis added the Release Line: 2.2 label Dec 13, 2017

mzeis suggested changes Dec 13, 2017

View reviewed changes

mzeis added Component: Catalog Progress: needs update labels Dec 13, 2017

mzeis reviewed Dec 13, 2017

View reviewed changes

Mohammad HAJ SALEM added 2 commits December 13, 2017 22:10

👌 reset protected variable naming

9e51c51

👌 rename isPositionsSet function to isPositionSet to keep consistency

ccaf748

mzeis approved these changes Dec 20, 2017

View reviewed changes

mzeis removed the Progress: needs update label Dec 20, 2017

magento-team added the Progress: needs update label Dec 20, 2017

mzeis removed the Progress: needs update label Dec 20, 2017

Fix typo

0b14b4e

mzeis suggested changes Dec 20, 2017

View reviewed changes

mzeis added the Progress: needs update label Dec 20, 2017

okorshenko modified the milestones: December 2017, January 2018 Jan 8, 2018

👌 apply code enhancement

da1833a

magento-team removed the Progress: needs update label Jan 20, 2018

mzeis suggested changes Jan 20, 2018

View reviewed changes

👌 apply code enhancement

3d87303

Add type hint

30b494b

mzeis approved these changes Jan 22, 2018

View reviewed changes

magento-engcom-team added the Progress: accept label Jan 30, 2018

magento-engcom-team merged commit 30b494b into magento:2.2-develop Jan 31, 2018

magento-engcom-team pushed a commit that referenced this pull request Jan 31, 2018

MAGETWO-85575: Add fallback for Product_links position attribute if n…

6c0fbaf

…ot set in request #12650

ihor-sviziev mentioned this pull request Jan 31, 2018

Product Link Save Handler - Remove not used constructor dependency #13436

Merged

4 tasks

ihor-sviziev pushed a commit to ihor-sviziev/magento2 that referenced this pull request Jan 31, 2018

🐛 add fallback for Product_links position attribute

e765098

if position index was not set in API request Original PR: magento#12650

ihor-sviziev mentioned this pull request Jan 31, 2018

[2.3] Add fallback for product links position attribute #13437

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fallback for Product_links position attribute if not set in request #12650

Add fallback for Product_links position attribute if not set in request #12650

mohammedsalem commented Dec 12, 2017

magento-cicd2 commented Dec 12, 2017 •

edited

Loading

dmanners commented Dec 13, 2017

mohammedsalem commented Dec 13, 2017

mzeis left a comment

mzeis Dec 13, 2017

mzeis Dec 13, 2017

mohammedsalem Dec 13, 2017

mzeis left a comment

mzeis Dec 20, 2017

mohammedsalem Dec 21, 2017

mzeis Dec 20, 2017

mohammedsalem Dec 21, 2017 •

edited

Loading

mohammedsalem commented Dec 25, 2017 •

edited

Loading

mzeis commented Jan 20, 2018

mzeis left a comment

mzeis Jan 20, 2018

mzeis Jan 20, 2018 •

edited

Loading

mohammedsalem commented Jan 21, 2018

magento-engcom-team commented Jan 22, 2018

ihor-sviziev commented Jan 31, 2018 •

edited

Loading

mzeis commented Jan 31, 2018

Add fallback for Product_links position attribute if not set in request #12650

Add fallback for Product_links position attribute if not set in request #12650

Conversation

mohammedsalem commented Dec 12, 2017

Description

Manual testing scenarios

magento-cicd2 commented Dec 12, 2017 • edited Loading

dmanners commented Dec 13, 2017

mohammedsalem commented Dec 13, 2017

mzeis left a comment

Choose a reason for hiding this comment

mzeis Dec 13, 2017

Choose a reason for hiding this comment

mzeis Dec 13, 2017

Choose a reason for hiding this comment

mohammedsalem Dec 13, 2017

Choose a reason for hiding this comment

mzeis left a comment

Choose a reason for hiding this comment

mzeis Dec 20, 2017

Choose a reason for hiding this comment

mohammedsalem Dec 21, 2017

Choose a reason for hiding this comment

mzeis Dec 20, 2017

Choose a reason for hiding this comment

mohammedsalem Dec 21, 2017 • edited Loading

Choose a reason for hiding this comment

mohammedsalem commented Dec 25, 2017 • edited Loading

mzeis commented Jan 20, 2018

mzeis left a comment

Choose a reason for hiding this comment

mzeis Jan 20, 2018

Choose a reason for hiding this comment

mzeis Jan 20, 2018 • edited Loading

Choose a reason for hiding this comment

mohammedsalem commented Jan 21, 2018

magento-engcom-team commented Jan 22, 2018

ihor-sviziev commented Jan 31, 2018 • edited Loading

mzeis commented Jan 31, 2018

magento-cicd2 commented Dec 12, 2017 •

edited

Loading

mohammedsalem Dec 21, 2017 •

edited

Loading

mohammedsalem commented Dec 25, 2017 •

edited

Loading

mzeis Jan 20, 2018 •

edited

Loading

ihor-sviziev commented Jan 31, 2018 •

edited

Loading