[BugFix] Fix area for image placeholder in graphql response. #245

VoronoyAlexandr · 2018-11-11T14:18:11Z

Description (*)

PR for issue #239 Fix resolving of image placeholder for Product without images.
Changed area to "frontend" instead "graphql" for placeholder asset.

Fixed Issues (if relevant)

[BugFix] Fix resolving of image placeholder for Product without images #239: Fix resolving of image placeholder for Product without images

Manual testing scenarios (*)

Request:

{
  products (search: "Test") {
    
    items {
        id
        name
      small_image {
        url
      }
    }
  }
}

Response:

{
  "data": {
    "products": {
      "items": [
        {
          "id": 1201,
          "name": "Test",
          "small_image": {
            "url": "http://graphql-ce.loc/static/version1541944966/frontend/Magento/luma/en_US/Magento_Catalog/images/product/placeholder/small_image.jpg"
          }
        }
      ]
    }
  }
}

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)

paliarush · 2018-11-12T17:48:32Z

app/code/Magento/CatalogGraphQl/Model/Resolver/Product/ProductImage/Url.php

@@ -71,7 +79,15 @@ private function getImageUrl(string $imageType, ?string $imagePath): string
        $image = $this->productImageFactory->create();
        $image->setDestinationSubdir($imageType)
            ->setBaseFile($imagePath);
-        $imageUrl = $image->getUrl();
+
+        $imageUrl = $image->isBaseFilePlaceholder()


@naydav could we get someone to test this solution please?

We already have test, need just unskip it and add a few asserts

naydav · 2018-11-15T13:52:31Z

Also, need to unskip test
\Magento\GraphQl\Catalog\ProductImageTest::testProductWithoutBaseImage
and add a few asserts

naydav · 2018-11-19T19:14:31Z

@VoronoyAlexandr
Pls, need to add additional asserts to unskiped API functional test

naydav · 2018-11-19T19:14:57Z

app/code/Magento/CatalogGraphQl/Model/Resolver/Product/ProductImage/Url.php

+    /**
+     * @var PlaceholderProvider
+     */
+    private $_placeholderProvider;


Don't use prefix _ for variables

naydav · 2018-11-19T19:15:07Z

app/code/Magento/CatalogGraphQl/Model/Resolver/Product/ProductImage/Url.php


    /**
+     * Url constructor.


Extra comment

naydav · 2018-11-19T19:15:32Z

app/code/Magento/CatalogGraphQl/Model/Resolver/Product/ProductImage/Url.php

-        $imageUrl = $image->getUrl();
-        return $imageUrl;
+
+        if ($image->isBaseFilePlaceholder()) {


Need to cover this logic with API-functional test

naydav · 2018-11-19T19:15:51Z

app/code/Magento/CatalogGraphQl/Model/Resolver/Products/DataProvider/Image/Placeholder.php

+use Magento\Store\Model\StoreManagerInterface;
+
+/**
+ * Class Placeholder


Provide clear dockblocks for all of classes

naydav · 2018-11-19T19:16:43Z

app/code/Magento/CatalogGraphQl/Model/Resolver/Products/DataProvider/Image/Placeholder.php

+ * Class Placeholder
+ * @package Magento\CatalogGraphQl\Model\Resolver\Products\DataProvider\Image
+ */
+class Placeholder


Looks like this class has a lot of responsibility (like helper in Magento 1)

naydav · 2018-12-03T15:05:19Z

dev/tests/api-functional/testsuite/Magento/GraphQl/Catalog/ProductImageTest.php

@@ -51,7 +51,6 @@ public function testProductWithBaseImage()
     */
    public function testProductWithoutBaseImage()
    {
-        $this->markTestIncomplete('https://github.com/magento/graphql-ce/issues/239');


Need to add additional logic for checking of placeholders

naydav · 2018-12-03T15:06:45Z

app/code/Magento/CatalogGraphQl/Model/Resolver/Products/DataProvider/Image/Placeholder.php

+        }
+
+        return $this->_assetRepository->createAsset(
+            "Magento_Catalog::images/product/placeholder/{$imageType}.jpg",


Need to check behavior if I changed default placeholder on some custom value via configuration

It's done above
if ($imageAsset->getFilePath()) {
return $imageAsset->getUrl();
}

We need config fixture to check this

naydav · 2019-01-23T21:28:13Z

Hello @VoronoyAlexandr,
We still need some changes to finish the PR, will you continue progress?
Thank you

Please let me know if you need any assistance.

VitaliyBoyko · 2019-02-12T19:01:19Z

Closing this one due to inactivity.

ghost · 2019-02-12T19:01:21Z

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

Fix area for image placeholder in graphql response.

990f8ea

VoronoyAlexandr requested a review from naydav November 11, 2018 14:19

paliarush reviewed Nov 12, 2018

View reviewed changes

paliarush approved these changes Nov 12, 2018

View reviewed changes

added logic if placeholder is overridden, unblock test.

8867a57

naydav suggested changes Nov 19, 2018

View reviewed changes

naydav added the in-progress label Nov 20, 2018

naydav reviewed Dec 3, 2018

View reviewed changes

naydav changed the title ~~Fix area for image placeholder in graphql response.~~ [BugFix] Fix area for image placeholder in graphql response. Dec 12, 2018

naydav modified the milestone: Release: 2.3.1 Dec 12, 2018

naydav added Progress: needs update and removed in-progress labels Jan 23, 2019

VitaliyBoyko closed this Feb 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BugFix] Fix area for image placeholder in graphql response. #245

[BugFix] Fix area for image placeholder in graphql response. #245

VoronoyAlexandr commented Nov 11, 2018

paliarush Nov 12, 2018

naydav Nov 15, 2018 •

edited

Loading

naydav commented Nov 15, 2018 •

edited

Loading

naydav commented Nov 19, 2018

naydav Nov 19, 2018

naydav Nov 19, 2018

naydav Nov 19, 2018

naydav Nov 19, 2018

naydav Nov 19, 2018

naydav Dec 3, 2018

naydav Dec 3, 2018

VitaliyBoyko Feb 12, 2019

VitaliyBoyko Feb 12, 2019

naydav commented Jan 23, 2019 •

edited

Loading

VitaliyBoyko commented Feb 12, 2019

ghost commented Feb 12, 2019

[BugFix] Fix area for image placeholder in graphql response. #245

[BugFix] Fix area for image placeholder in graphql response. #245

Conversation

VoronoyAlexandr commented Nov 11, 2018

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*)

Contribution checklist (*)

Choose a reason for hiding this comment

naydav Nov 15, 2018 • edited Loading

Choose a reason for hiding this comment

naydav commented Nov 15, 2018 • edited Loading

naydav commented Nov 19, 2018

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

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

naydav commented Jan 23, 2019 • edited Loading

VitaliyBoyko commented Feb 12, 2019

ghost commented Feb 12, 2019

naydav Nov 15, 2018 •

edited

Loading

naydav commented Nov 15, 2018 •

edited

Loading

naydav commented Jan 23, 2019 •

edited

Loading