Fixed magento text swatch switches product image even if attribute feature is disabled #16446 #19184

ravi-chandra3197 · 2018-11-13T12:45:52Z

Description (*)

Fixed magento text swatch switches product image even if attribute feature is disabled #16446

Fixed Issues (if relevant)

magento 2.2.2 text swatch switches product image even if attribute feature is disabled #16446: Fixed magento text swatch switches product image even if attribute feature is disabled

Manual testing scenarios (*)

N/A

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 on Travis CI are green)

magento-engcom-team · 2018-11-13T12:46:06Z

Hi @ravi-chandra3197. 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-engcom-team give me test instance - deploy test instance based on PR changes
@magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

sivaschenko

Please see the review comments. Also I think the attributes used to idetify "update_product_preview_image" should be retrived considering product context. Also, I believe it would be better to fix the issue in frontend implementation (See https://github.com/magento/magento2/blob/2.3-develop/app/code/Magento/Swatches/view/frontend/web/js/swatch-renderer.js)

sivaschenko · 2018-11-15T12:15:46Z

app/code/Magento/Swatches/Helper/Data.php

@@ -117,7 +117,8 @@ public function __construct(
        UrlBuilder $urlBuilder,
        Json $serializer = null,
        SwatchAttributesProvider $swatchAttributesProvider = null,
-        SwatchAttributeType $swatchTypeChecker = null
+        SwatchAttributeType $swatchTypeChecker = null,
+        \Magento\Catalog\Model\ResourceModel\Eav\Attribute $attributeFactory


Please add additional constructor parameters as options to keep backward compatibility

I have changed as per your suggestion

sivaschenko · 2018-11-15T12:23:17Z

app/code/Magento/Swatches/Helper/Data.php

+        $resource = $objectManager->get('Magento\Framework\App\ResourceConnection');
+        $attributeInfo = $this->_attributeFactory->getCollection();
+        if ($attributeInfo) {
+            $attributeInfo = $attributeInfo->join(array('catalog_eav_attribute' => $resource->getTableName('catalog_eav_attribute')), 'main_table.attribute_id = catalog_eav_attribute.attribute_id')


Please avoid using objectManager to keep dependencies explicit. Please use \Magento\Catalog\Api\ProductAttributeRepositoryInterface::getList to retrieve attributes.

sivaschenko · 2018-11-15T12:25:33Z

app/code/Magento/Swatches/Helper/Data.php

+                ->addFieldToFilter('catalog_eav_attribute.additional_data', array('like' => '%update_product_preview_image%'));
+
+            foreach ($attributeInfo as $attribute) {
+                $additional_data = json_decode($attribute['additional_data'], true);


Please use camel case for variable names: $additionalData

ravi-chandra3197 · 2018-11-26T10:52:02Z

Hello @sivaschenko
I have update change as per your suggestion can review it.

sivaschenko

@ravi-chandra3197 thanks for your updates, please see the additional comments. Also can you please provide your feedback to my comment on the previous review

sivaschenko · 2018-11-26T14:47:10Z

app/code/Magento/Swatches/Controller/Ajax/Media.php

+        $attributeInfo = $this->attributeRepository->getList($searchCriteria);
+        $attributeInfo = $attributeInfo->getItems();
+        foreach ($attributeInfo as $attribute) {
+            $additionalData = json_decode($attribute['additional_data'], true);


Please use serializer to decode the additional data

sivaschenko · 2018-11-26T14:48:07Z

app/code/Magento/Swatches/Controller/Ajax/Media.php

@@ -39,11 +39,15 @@ public function __construct(
        Context $context,
        \Magento\Catalog\Model\ProductFactory $productModelFactory,
        \Magento\Swatches\Helper\Data $swatchHelper,
-        \Magento\PageCache\Model\Config $config
+        \Magento\PageCache\Model\Config $config,
+        \Magento\Framework\Api\SearchCriteriaBuilder $searchCriteria,


Please add additional parameters as optional (= null)

app/code/Magento/Swatches/Controller/Ajax/Media.php

sivaschenko · 2018-12-14T16:55:38Z

Hi @ravi-chandra3197 are you still working on this pull request?

ravi-chandra3197 · 2018-12-15T04:42:04Z

Hello @sivaschenko
I have try to find front-end solution.
https://github.com/magento/magento2/blob/2.3-develop/app/code/Magento/Swatches/view/frontend/web/js/swatch-renderer.js)

sidolov · 2019-01-28T19:51:07Z

Hi @ravi-chandra3197 , will you continue progress with the PR?

ravi-chandra3197 · 2019-01-29T13:31:57Z

Hello @sidolov
I am working on it, try to find front-end solution using js.

https://github.com/magento/magento2/blob/2.3-develop/app/code/Magento/Swatches/view/frontend/web/js/swatch-renderer.js

sidolov · 2019-03-14T17:33:20Z

@ravi-chandra3197 , I am closing this PR now due to inactivity.
Please reopen and update if you wish to continue.
Thank you for the collaboration!

ghost · 2019-03-14T17:33:22Z

Hi @ravi-chandra3197, 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.

ravi-chandra3197 · 2019-04-22T05:24:52Z

Hello @sivaschenko
I have updated the code as per your suggestion and implement solution using swatch-renderer.js

https://github.com/magento/magento2/blob/2.3-develop/app/code/Magento/Swatches/view/frontend/web/js/swatch-renderer.js

sivaschenko

Hi @ravi-chandra3197 , thanks for update. Please see my review comments

app/code/Magento/Swatches/Block/Product/Renderer/Configurable.php

app/code/Magento/Swatches/view/frontend/web/js/swatch-renderer.js

ravi-chandra3197 · 2019-04-24T06:15:30Z

Hello @sivaschenko
I have updated the code as your suggestion.

sivaschenko

@ravi-chandra3197 thanks for updates, please see my review comments

sivaschenko · 2019-04-24T11:36:24Z

app/code/Magento/Swatches/Block/Product/Renderer/Configurable.php

@@ -182,6 +182,9 @@ public function getJsonSwatchConfig()
                    $attributeDataArray
                );
            }
+            if (isset($attributeDataArray['additional_data'])) {
+            $config[$attributeId]['additional_data'] =  $attributeDataArray['additional_data'];


Proper alignment makes code much easier to read

Suggested change

$config[$attributeId]['additional_data'] = $attributeDataArray['additional_data'];

$config[$attributeId]['additional_data'] = $attributeDataArray['additional_data'];

Also, what do you think about assigning only 'update_product_preview_image' key?

Hello @sivaschenko
in additional_data we get all data in serialize form
{swatch_input_type: "text", update_product_preview_image: "1", use_product_image_for_swatch: 0}

app/code/Magento/Swatches/view/frontend/web/js/swatch-renderer.js

ravi-chandra3197 · 2019-04-24T13:15:21Z

Hello @sivaschenko
I have updated the PR code as per your suggestion can you review it.

…ature is disabled

soleksii · 2019-04-25T14:54:54Z

✔️ QA Passed

Before:

After:

soleksii · 2019-04-26T13:25:53Z

✔️ QA Passed

m2-assistant · 2019-06-07T03:09:45Z

Hi @ravi-chandra3197, 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.

… attribute feature is disabled magento#16446 magento#19184

magento-engcom-team added Partner: Krish TechnoLabs partners-contribution Pull Request is created by Magento Partner Component: Swatches Release Line: 2.3 labels Nov 13, 2018

sivaschenko self-assigned this Nov 13, 2018

sivaschenko requested changes Nov 15, 2018

View reviewed changes

sivaschenko added the Progress: needs update label Nov 15, 2018

sivaschenko requested changes Nov 26, 2018

View reviewed changes

sidolov closed this Mar 14, 2019

ravi-chandra3197 reopened this Apr 19, 2019

ghost unassigned sivaschenko Apr 19, 2019

ravi-chandra3197 force-pushed the patch-16446 branch from 845dcfd to 5fd11d2 Compare April 19, 2019 09:07

sivaschenko requested changes Apr 23, 2019

View reviewed changes

app/code/Magento/Swatches/Block/Product/Renderer/Configurable.php Outdated Show resolved Hide resolved

app/code/Magento/Swatches/view/frontend/web/js/swatch-renderer.js Outdated Show resolved Hide resolved

ghost assigned sivaschenko Apr 23, 2019

sivaschenko requested changes Apr 24, 2019

View reviewed changes

sivaschenko approved these changes Apr 25, 2019

View reviewed changes

ghost added Progress: ready for testing and removed Progress: needs update labels Apr 25, 2019

magento-engcom-team added the Area: Frontend label Apr 25, 2019

ghost added the Progress: testing in progress label Apr 25, 2019

Fixed magento text swatch switches product image even if attribute fe…

dfaee8e

…ature is disabled

ravi-chandra3197 force-pushed the patch-16446 branch from 3e302cf to dfaee8e Compare April 25, 2019 13:27

ghost added Progress: extended testing and removed Progress: testing in progress labels Apr 25, 2019

p-bystritsky self-assigned this Apr 25, 2019

ghost added Progress: testing in progress and removed Progress: extended testing labels Apr 26, 2019

magento#19184: Static test fix.

6ce2026

p-bystritsky force-pushed the patch-16446 branch from c92bd91 to 6ce2026 Compare April 26, 2019 12:43

magento#19184: MFTF test fix

01e8ff1

magento-engcom-team added the Component: Catalog label Apr 26, 2019

ghost added Progress: extended testing and removed Progress: testing in progress labels Apr 26, 2019

ravi-chandra3197 mentioned this pull request Apr 26, 2019

[Backport] Fixed magento text swatch switches product image even if attribute feature is disabled #22510

Merged

4 tasks

magento#19184: Static test fix.

73ef119

p-bystritsky force-pushed the patch-16446 branch from 1fb41ac to 73ef119 Compare April 30, 2019 13:06

ghost added Progress: accept and removed Progress: extended testing labels Apr 30, 2019

magento-engcom-team added the Award: bug fix label May 1, 2019

magento-engcom-team mentioned this pull request Jun 7, 2019

magento 2.2.2 text swatch switches product image even if attribute feature is disabled #16446

Closed

magento-engcom-team merged commit 73ef119 into magento:2.3-develop Jun 7, 2019

magento-engcom-team pushed a commit to okorshenko/magento2 that referenced this pull request Jun 7, 2019

ENGCOM-4825: Fixed magento text swatch switches product image even if…

821adca

… attribute feature is disabled magento#16446 magento#19184

magento-engcom-team added this to the Release: 2.3.3 milestone Jun 7, 2019

ihor-sviziev mentioned this pull request Sep 17, 2019

Configurable associated product image not showing #24571

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed magento text swatch switches product image even if attribute feature is disabled #16446 #19184

Fixed magento text swatch switches product image even if attribute feature is disabled #16446 #19184

ravi-chandra3197 commented Nov 13, 2018 •

edited

Loading

magento-engcom-team commented Nov 13, 2018

sivaschenko left a comment

sivaschenko Nov 15, 2018

ravi-chandra3197 Nov 27, 2018

sivaschenko Nov 15, 2018

sivaschenko Nov 15, 2018

ravi-chandra3197 commented Nov 26, 2018

sivaschenko left a comment

sivaschenko Nov 26, 2018

sivaschenko Nov 26, 2018

sivaschenko commented Dec 14, 2018 •

edited

Loading

ravi-chandra3197 commented Dec 15, 2018

sidolov commented Jan 28, 2019

ravi-chandra3197 commented Jan 29, 2019

sidolov commented Mar 14, 2019

ghost commented Mar 14, 2019

ravi-chandra3197 commented Apr 22, 2019

sivaschenko left a comment

ravi-chandra3197 commented Apr 24, 2019

sivaschenko left a comment

sivaschenko Apr 24, 2019

sivaschenko Apr 24, 2019

ravi-chandra3197 Apr 24, 2019

ravi-chandra3197 commented Apr 24, 2019

soleksii commented Apr 25, 2019

soleksii commented Apr 26, 2019

m2-assistant bot commented Jun 7, 2019

	$config[$attributeId]['additional_data'] = $attributeDataArray['additional_data'];
	$config[$attributeId]['additional_data'] = $attributeDataArray['additional_data'];

Fixed magento text swatch switches product image even if attribute feature is disabled #16446 #19184

Fixed magento text swatch switches product image even if attribute feature is disabled #16446 #19184

Conversation

ravi-chandra3197 commented Nov 13, 2018 • edited Loading

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*)

Contribution checklist (*)

magento-engcom-team commented Nov 13, 2018

sivaschenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravi-chandra3197 commented Nov 26, 2018

sivaschenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sivaschenko commented Dec 14, 2018 • edited Loading

ravi-chandra3197 commented Dec 15, 2018

sidolov commented Jan 28, 2019

ravi-chandra3197 commented Jan 29, 2019

sidolov commented Mar 14, 2019

ghost commented Mar 14, 2019

ravi-chandra3197 commented Apr 22, 2019

sivaschenko left a comment

Choose a reason for hiding this comment

ravi-chandra3197 commented Apr 24, 2019

sivaschenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravi-chandra3197 commented Apr 24, 2019

soleksii commented Apr 25, 2019

soleksii commented Apr 26, 2019

m2-assistant bot commented Jun 7, 2019

ravi-chandra3197 commented Nov 13, 2018 •

edited

Loading

sivaschenko commented Dec 14, 2018 •

edited

Loading