Skip to content
This repository has been archived by the owner on Dec 19, 2019. It is now read-only.

GraphQL - Added sort by options to Products GraphQL type #12

Merged
merged 6 commits into from
May 22, 2018

Conversation

zbigniewkuras
Copy link
Contributor

Description

Added available sort options and default sort option for product listing to page_info object of Products GraphQL object.

Fixed Issues (if relevant)

  1. Return 'Available sort options' for product listing #3: Return 'Available sort options' for product listing

Manual testing scenarios

  1. Add following object to query object page_info and run:
    sort_fields { default options { key label } }
  2. Magento should returns object sort_fields contains default and options

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\CatalogGraphQl\Model\Resolver\Layer\DataProvider\Filters $filtersDataProvider
\Magento\CatalogGraphQl\Model\Resolver\Layer\DataProvider\Filters $filtersDataProvider,
\Magento\Catalog\Model\Config $catalogConfig,
\Magento\Store\Model\StoreManagerInterface $storeManager
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add constructor doc block type hint for StoreManagerInterface

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added!

@ishakhsuvarov ishakhsuvarov added the fra-rz-cd Label for distributed contribution day between Germany and Poland label May 12, 2018
@magento-engcom-team
Copy link
Contributor

magento-engcom-team commented May 13, 2018

Closed and Re Opened PR to trigger Travis CI builds.

@@ -118,14 +134,28 @@ public function resolve(
);
}

$options = $this->catalogConfig->getAttributeUsedForSortByArray();
Copy link
Contributor

Choose a reason for hiding this comment

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

Please create a separate resolver for sort_fields. There are 2 main reasons for that:

  1. Performance. In case sort_fields is not requested, all this logic will not be executed in case of separate resolver
  2. Following single responsibility and object decomposition simplifies maintenance and understanding

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Roger, i will fix it, thanks for your feedback

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented May 15, 2018

CLA assistant check
All committers have signed the CLA.

@magento-engcom-team
Copy link
Contributor

magento-engcom-team commented May 15, 2018

Hi @zbigniewkuras
Please sign Contributor License Agreement for this Pull Request so that we could accept this Pull Request. Thank you.

Copy link
Contributor

@paliarush paliarush left a comment

Choose a reason for hiding this comment

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

@zbigniewkuras thank you for making additional changes, please see new comments. As soon as those are addressed, we can merge the PR.

@@ -25,9 +25,20 @@ input FilterTypeInput @doc(description: "FilterTypeInput specifies which action
type SearchResultPageInfo @doc(description: "SearchResultPageInfo provides navigation for the query response") {
page_size: Int @doc(description: "Specifies the maximum number of items to return")
current_page: Int @doc(description: "Specifies which page of results to return")
sort_fields: SortFields @doc(description: "An object that includes the default sort field and all available sort fields")
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be a single new resolver, just for sort_fields.

*/
public function resolve(Field $field, $context, ResolveInfo $info, array $value = null, array $args = null) : Value
{
$sortFieldsOptions = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use \Magento\Catalog\Model\Category\Attribute\Source\Sortby::getAllOptions. It has two benefits:

  • includes position, so no need to hardcode it here again
  • result is already properly formatted, no need to do foreach (assuming that key is renamed to value as suggested in another comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason i didn't use this function was the translation function called for labels in \Magento\Catalog\Model\Category\Attribute\Source\Sortby::getAllOptions casuses Internal server error and every value for label is equal null. I'm not sure exactly, but it looks like a bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about this one \Magento\Catalog\Model\Config::getAttributeUsedForSortByArray?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the same, this function also contains translation function for Position option
$options = ['position' => __('Position')];

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is that underlying GraphQL library (webonyx) expects exact match between declared type (string) and the actual value type (Magento\Framework\Phrase), and it does not try to perform type casting.

I was able to make it work by manually casting Phrase to string before returning the result:

        $sortFieldsOptions = $this->sortBy->getAllOptions();
        array_walk(
            $sortFieldsOptions,
            function (&$option) {
                $option['label'] = (string)$option['label'];
            }
        );

default
options
{
key
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rename key to value for consistency with naming in other places in Magento, for example \Magento\Catalog\Model\Category\Attribute\Source\Sortby::getAllOptions

@@ -530,6 +539,10 @@ public function testQueryProductsInCurrentPageSortedByPriceASC()
$this->assertProductItems($filteredChildProducts, $response);
$this->assertEquals(4, $response['products']['page_info']['page_size']);
$this->assertEquals(1, $response['products']['page_info']['current_page']);
$this->assertArrayHasKey('sort_fields', $response['products']['page_info']);
$this->assertArrayHasKey('options', $response['products']['page_info']['sort_fields']);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add assertion for options values, it is not validated at the moment.

@@ -25,9 +25,20 @@ input FilterTypeInput @doc(description: "FilterTypeInput specifies which action
type SearchResultPageInfo @doc(description: "SearchResultPageInfo provides navigation for the query response") {
page_size: Int @doc(description: "Specifies the maximum number of items to return")
current_page: Int @doc(description: "Specifies which page of results to return")
sort_fields: SortFields @doc(description: "An object that includes the default sort field and all available sort fields") @resolver(class: "Magento\\CatalogGraphQl\\Model\\Resolver\\Category\\SortFields")
Copy link
Contributor

Choose a reason for hiding this comment

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

sort_fields should be moved to Products type, as mentioned in the issue description, because SearchResultPageInfo is meant for storing paginating information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okey

ValueFactory $valueFactory,
\Magento\Catalog\Model\Config $catalogConfig,
\Magento\Store\Model\StoreManagerInterface $storeManager,
\Magento\Catalog\Model\Category\Attribute\Source\Sortby $ss
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like an accidental change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right. I was testing function \Magento\Catalog\Model\Category\Attribute\Source\Sortby::getAllOptions(). I will fix it

@misha-kotov
Copy link

Approved ✅

@paliarush
Copy link
Contributor

@zbigniewkuras, the PR has been merged to mainline/2.3-develop, thanks you for contribution!

@keharper
Copy link
Contributor

Added SortFields information in the Products endpoint documentation.

magento-engcom-team pushed a commit that referenced this pull request Aug 9, 2018
magento-engcom-team pushed a commit that referenced this pull request Sep 11, 2018
magento-engcom-team pushed a commit that referenced this pull request Jul 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Award: test coverage documentation-completed fra-rz-cd Label for distributed contribution day between Germany and Poland
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants