Defaulting missing alt-text for a product to use the product name. #11323

brobie · 2017-10-10T04:06:20Z

Description

The code in app/code/Magento/Catalog/Block/Product/View/Gallery.php's getGalleryImagesJson() function was setting the image's label to $image->getLabel() regardless of whether the image's label was null or empty. The fix was to test the $image->getLabel() response and if it was empty to use the product's name instead.

Fixed Issues (if relevant)

Empty image alt-text & missing alt attribute on product detail page #9931: Empty image alt-text & missing alt attribute on product detail page

Manual testing scenarios

Manually create a product with images.
Intentionally leave the Image Label blank.
Navigate to that product's PDP and you should see the alt attribute for the image be populated with the product's name.
You should also test images that have Image Labels and ensure that those labels show in the alt attribute

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#9931

magento-cicd2 · 2017-10-10T04:06:22Z

All committers have signed the CLA.

dmanners · 2017-10-10T08:09:40Z

Thanks for this PR @brobie I will review and process this now for you. If I need any further information I will let you know.

dmanners · 2017-10-10T08:59:34Z

Hi @brobie the fix looks good to me so far but it would be great if you could cover it with a test. Ideally an integration test but a unit test would also be of some benefit here as the method getGalleryImagesJson currently has no coverage. Let me know if you need any assistance with this. Thanks.

…tGalleryImagesJson() function magento#9931

brobie · 2017-10-10T14:00:26Z

@dmanners The unit tests have been added. I'm not sure how to kick them off as a part of this new addition to the pull request.

dmanners · 2017-10-10T14:02:41Z

@brobie thanks, they will be picked up by travis automatically.

dmanners · 2017-10-10T14:31:28Z

app/code/Magento/Catalog/Test/Unit/Block/Product/View/GalleryTest.php

@@ -77,6 +77,83 @@ protected function mockContext()
            ->willReturn($this->registry);
    }

+    public function testGetGalleryImagesJsonWithLabel(){


The static tests will complain that the { needs to be on a new line. You should be able to run the static tests over your changes and see all the issues otherwise you can wait until Travis has finished. Though I think there will be nothing more than:

FILE: ...ode/Magento/Catalog/Test/Unit/Block/Product/View/GalleryTest.php ---------------------------------------------------------------------- FOUND 4 ERRORS AFFECTING 4 LINES ---------------------------------------------------------------------- 80 | ERROR | [x] Opening brace should be on a new line 94 | ERROR | [x] Opening brace should be on a new line 100 | ERROR | [x] Function closing brace must go on the next line | | following the body; found 1 blank lines before | | brace 102 | ERROR | [x] Opening brace should be on a new line ---------------------------------------------------------------------- PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY ----------------------------------------------------------------------

@dmanners - I have fixed the above issues, but am curious if you can point me to the correct configuration for running the static code analysis. When I run the below command, I get more issues than what you have said.

vendor/squizlabs/php_codesniffer/bin/phpcs app/code/Magento/Catalog/Test/Unit/Block/Product/View/GalleryTest.php

So those results came from the build system. Not 100% sure how it was run but there are a couple of things I have done locally before.

./vendor/bin/phpcs app/code/Magento/Catalog/Test/Unit/Block/Product/View/GalleryTest.php dev/tests/static/framework/Magento - This will run the framework/Magento standards when running phpcs.
You could also try bin/magento dev:tests:run static but that runs them all.
Finally there is a way of running them only on a diff (though I have as yet got that running....) From what I understand you run git diff origin/2.2-develop HEAD --name-only > dev/tests/static/testsuite/Magento/Test/_files/changed_files_local.txt and this will create a list of changed files from your branch compared to another branch (in this case origin/2.2-develop). Then after that when running the suite it should only run on those files you changes. Maybe @okorshenko knows the full command for the static tests.

There is one other option which is to setup Travis against your fork. This would then mean you could run the travis checks before providing a PR.

@brobie ok so if you run the git diff as described above followed by ./vendor/bin/phpunit --config dev/tests/static/phpunit.xml.dist that should show you the details you are looking for.

Awesome, I'll give it a try!

Here is the request that mimics the rules used by Magento for Code Sniffer (I think)
vendor/squizlabs/php_codesniffer/bin/phpcs --standard=dev/tests/static/framework/Magento/ruleset.xml app/code/Magento/Catalog/Test/Unit/Block/Product/View/GalleryTest.php

magento#9931

dmanners

Thanks for this PR. It looks good. I will start to process this now.

dmanners · 2017-11-04T10:29:36Z

@brobie so we have the following issue during internal testing.

1. Create a simple product that will have an image with an alt text.
2. Export this product to a CSV file.
3. Delete the product from Website
4. Import the previously exported product (CSV) back to the website
5. Open the imported product page (on a backend)

Expected result:
The product still has the alt text, that we created in the first step

Actual result:
The alt text that we had is gone from the product (it was lost)

Would you be up for having a look at fixing this also here. This is part of #9931 which has been linked to this PR.

Thanks

brobie · 2017-11-05T01:51:47Z

I can try to have a look at it in the next few days.

…

On Sat, Nov 4, 2017 at 5:29 AM David Manners ***@***.***> wrote: @brobie <https://github.com/brobie> so we have the following issue during internal testing. 1. Create a simple product that will have an image with an alt text. 2. Export this product to a CSV file. 3. Delete the product from Website 4. Import the previously exported product (CSV) back to the website 5. Open the imported product page (on a backend) Expected result: The product still has the alt text, that we created in the first step Actual result: The alt text that we had is gone from the product (it was lost) Would you be up for having a look at fixing this also here. This is part of #9931 <#9931> which has been linked to this PR. Thanks — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11323 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAND5oqPl7kRHTM8kDCEbyFW-VS5Vmldks5szDySgaJpZM4PzWXJ> .

dmanners · 2017-11-22T14:27:17Z

Hi @brobie have you had any chance to look into this issue?

dmanners · 2017-11-27T14:34:06Z

@brobie I think I found the issue with the labels being saved. Will check the fix provided and let you know.

…n method

…roduct name. #11323

Defaulting missing alt-text for a product to use the product name.

836cb30

magento#9931

dmanners added SQUASHTOBERFEST Release Line: 2.2 labels Oct 10, 2017

dmanners self-assigned this Oct 10, 2017

dmanners added this to the October 2017 milestone Oct 10, 2017

dmanners added Progress: needs update Component: Catalog labels Oct 10, 2017

vkublytskyi added the partner contribution label Oct 10, 2017

Adding unit test coverage for Catalog/Block/Product/View/Gallery's ge…

3381ec0

…tGalleryImagesJson() function magento#9931

magento-team removed the Progress: needs update label Oct 10, 2017

dmanners suggested changes Oct 10, 2017

View reviewed changes

Ben Robie added 2 commits October 10, 2017 10:01

Correcting code to conform with static code testing.

0f1b58f

magento#9931

Correcting code to conform with static code testing.

7f44e66

magento#9931

dmanners approved these changes Oct 11, 2017

View reviewed changes

dmanners added the Progress: accept label Oct 11, 2017

okorshenko modified the milestones: October 2017, November 2017 Nov 1, 2017

magento-engcom-team added 2.2.x Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release labels Nov 7, 2017

magento-engcom-team added Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on develop Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release labels Nov 7, 2017

okorshenko removed the Reproduced on develop label Nov 7, 2017

magento-engcom-team added the Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release label Nov 22, 2017

Update image label values on product entity on admin product save

8aeb71b

dmanners added 4 commits November 27, 2017 14:44

Fix code style for empty label check if statement

1511377

Extract image and lavel processing for media attributes into their ow…

bab1a38

…n method

Solve some complexity issues with the save handler

d6fa9db

Make sure there is a space between the two new methods

ae5454a

okorshenko merged commit ae5454a into magento:2.2-develop Dec 1, 2017

okorshenko pushed a commit that referenced this pull request Dec 1, 2017

MAGETWO-81368: Defaulting missing alt-text for a product to use the p…

9bc20c4

…roduct name. #11323

vkublytskyi added the Award: test coverage label Dec 14, 2017

vkublytskyi added the port-to-2.3 label Jan 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Defaulting missing alt-text for a product to use the product name. #11323

Defaulting missing alt-text for a product to use the product name. #11323

brobie commented Oct 10, 2017 •

edited by dmanners

Loading

magento-cicd2 commented Oct 10, 2017 •

edited

Loading

dmanners commented Oct 10, 2017

dmanners commented Oct 10, 2017

brobie commented Oct 10, 2017

dmanners commented Oct 10, 2017

dmanners Oct 10, 2017

brobie Oct 10, 2017

dmanners Oct 10, 2017

dmanners Oct 10, 2017

dmanners Oct 10, 2017

brobie Oct 10, 2017

brobie Oct 11, 2017

dmanners left a comment

dmanners commented Nov 4, 2017

brobie commented Nov 5, 2017 via email

dmanners commented Nov 22, 2017

dmanners commented Nov 27, 2017

Defaulting missing alt-text for a product to use the product name. #11323

Defaulting missing alt-text for a product to use the product name. #11323

Conversation

brobie commented Oct 10, 2017 • edited by dmanners Loading

Description

Fixed Issues (if relevant)

Manual testing scenarios

Contribution checklist

magento-cicd2 commented Oct 10, 2017 • edited Loading

dmanners commented Oct 10, 2017

dmanners commented Oct 10, 2017

brobie commented Oct 10, 2017

dmanners commented Oct 10, 2017

dmanners Oct 10, 2017

Choose a reason for hiding this comment

brobie Oct 10, 2017

Choose a reason for hiding this comment

dmanners Oct 10, 2017

Choose a reason for hiding this comment

dmanners Oct 10, 2017

Choose a reason for hiding this comment

dmanners Oct 10, 2017

Choose a reason for hiding this comment

brobie Oct 10, 2017

Choose a reason for hiding this comment

brobie Oct 11, 2017

Choose a reason for hiding this comment

dmanners left a comment

Choose a reason for hiding this comment

dmanners commented Nov 4, 2017

brobie commented Nov 5, 2017 via email

dmanners commented Nov 22, 2017

dmanners commented Nov 27, 2017

brobie commented Oct 10, 2017 •

edited by dmanners

Loading

magento-cicd2 commented Oct 10, 2017 •

edited

Loading