Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 9 commits into from
Dec 1, 2017
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
836cb30
Defaulting missing alt-text for a product to use the product name.
Oct 10, 2017
3381ec0
Adding unit test coverage for Catalog/Block/Product/View/Gallery's ge…
Oct 10, 2017
0f1b58f
Correcting code to conform with static code testing.
Oct 10, 2017
7f44e66
Correcting code to conform with static code testing.
Oct 11, 2017
8aeb71b
Update image label values on product entity on admin product save
dmanners Nov 27, 2017
1511377
Fix code style for empty label check if statement
dmanners Nov 27, 2017
bab1a38
Extract image and lavel processing for media attributes into their ow…
dmanners Nov 29, 2017
d6fa9db
Solve some complexity issues with the save handler
dmanners Nov 29, 2017
ae5454a
Make sure there is a space between the two new methods
dmanners Nov 29, 2017
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
2 changes: 1 addition & 1 deletion app/code/Magento/Catalog/Block/Product/View/Gallery.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ public function getGalleryImagesJson()
'thumb' => $image->getData('small_image_url'),
'img' => $image->getData('medium_image_url'),
'full' => $image->getData('large_image_url'),
'caption' => $image->getLabel(),
'caption' => ($image->getLabel() ?: $this->getProduct()->getName()),
'position' => $image->getPosition(),
'isMain' => $this->isMainImage($image),
'type' => str_replace('external-', '', $image->getMediaType()),
Expand Down
103 changes: 103 additions & 0 deletions app/code/Magento/Catalog/Test/Unit/Block/Product/View/GalleryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,83 @@ protected function mockContext()
->willReturn($this->registry);
}

public function testGetGalleryImagesJsonWithLabel(){
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome, I'll give it a try!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

$this->prepareGetGalleryImagesJsonMocks();
$json = $this->model->getGalleryImagesJson();
$decodedJson = json_decode($json, true);
$this->assertEquals('product_page_image_small_url', $decodedJson[0]['thumb']);
$this->assertEquals('product_page_image_medium_url', $decodedJson[0]['img']);
$this->assertEquals('product_page_image_large_url', $decodedJson[0]['full']);
$this->assertEquals('test_label', $decodedJson[0]['caption']);
$this->assertEquals('2', $decodedJson[0]['position']);
$this->assertEquals(false, $decodedJson[0]['isMain']);
$this->assertEquals('test_media_type', $decodedJson[0]['type']);
$this->assertEquals('test_video_url', $decodedJson[0]['videoUrl']);
}

public function testGetGalleryImagesJsonWithoutLabel(){
$this->prepareGetGalleryImagesJsonMocks(false);
$json = $this->model->getGalleryImagesJson();
$decodedJson = json_decode($json, true);
$this->assertEquals('test_product_name', $decodedJson[0]['caption']);

}

private function prepareGetGalleryImagesJsonMocks($hasLabel = true){
$storeMock = $this->getMockBuilder(\Magento\Store\Model\Store::class)
->disableOriginalConstructor()
->getMock();

$productMock = $this->getMockBuilder(\Magento\Catalog\Model\Product::class)
->disableOriginalConstructor()
->getMock();

$productTypeMock = $this->getMockBuilder(\Magento\Catalog\Model\Product\Type\AbstractType::class)
->disableOriginalConstructor()
->getMock();
$productTypeMock->expects($this->any())
->method('getStoreFilter')
->with($productMock)
->willReturn($storeMock);

$productMock->expects($this->any())
->method('getTypeInstance')
->willReturn($productTypeMock);
$productMock->expects($this->any())
->method('getMediaGalleryImages')
->willReturn($this->getImagesCollectionWithPopulatedDataObject($hasLabel));
$productMock->expects($this->any())
->method('getName')
->willReturn('test_product_name');

$this->registry->expects($this->any())
->method('registry')
->with('product')
->willReturn($productMock);

$this->imageHelper->expects($this->any())
->method('init')
->willReturnMap([
[$productMock, 'product_page_image_small', [], $this->imageHelper],
[$productMock, 'product_page_image_medium_no_frame', [], $this->imageHelper],
[$productMock, 'product_page_image_large_no_frame', [], $this->imageHelper],
])
->willReturnSelf();
$this->imageHelper->expects($this->any())
->method('setImageFile')
->with('test_file')
->willReturnSelf();
$this->imageHelper->expects($this->at(2))
->method('getUrl')
->willReturn('product_page_image_small_url');
$this->imageHelper->expects($this->at(5))
->method('getUrl')
->willReturn('product_page_image_medium_url');
$this->imageHelper->expects($this->at(8))
->method('getUrl')
->willReturn('product_page_image_large_url');
}

public function testGetGalleryImages()
{
$storeMock = $this->getMockBuilder(\Magento\Store\Model\Store::class)
Expand Down Expand Up @@ -154,4 +231,30 @@ private function getImagesCollection()

return $collectionMock;
}

/**
* @return \Magento\Framework\Data\Collection
*/
private function getImagesCollectionWithPopulatedDataObject($hasLabel)
{
$collectionMock = $this->getMockBuilder(\Magento\Framework\Data\Collection::class)
->disableOriginalConstructor()
->getMock();

$items = [
new \Magento\Framework\DataObject([
'file' => 'test_file',
'label' => ($hasLabel ? 'test_label' : ''),
'position' => '2',
'media_type' => 'external-test_media_type',
"video_url" => 'test_video_url'
]),
];

$collectionMock->expects($this->any())
->method('getIterator')
->willReturn(new \ArrayIterator($items));

return $collectionMock;
}
}