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

#315 Improve Filter Architecture #314

Closed
wants to merge 3 commits into from
Closed

#315 Improve Filter Architecture #314

wants to merge 3 commits into from

Conversation

larsroettig
Copy link
Member

@larsroettig larsroettig commented Jan 8, 2019 •
edited
Loading

Description (*)

Implement a Generic Class for GaphQl Filter

Exsample ussage:

 <virtualType name="Magento\CatalogGraphQl\Model\Resolver\Products\FilterArgument\ProductEntityAttributesForAst" type="\Magento\Framework\GraphQl\Query\Resolver\Argument\FieldEntityAttributes">	
        <arguments>	
            <argument name="configElementName" xsi:type="string">SimpleProduct</argument>	
            <argument name="additionalAttributes" xsi:type="array">	
                <item name="min_price" xsi:type="string">min_price</item>	
                <item name="max_price" xsi:type="string">max_price</item>	
                <item name="category_id" xsi:type="string">category_id</item>	
            </argument>	
        </arguments>	
    </virtualType>

Fixed Issues (if relevant)

  1. magento/magento2#<issue_number>: Issue title
  2. ...

Manual testing scenarios (*)

  1. git checkout filter_better_architecture
  2. Test the products endpoint for graphql

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-cicd2
Copy link
Contributor

magento-cicd2 commented Jan 8, 2019 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

paliarush
paliarush suggested changes Jan 8, 2019
View reviewed changes
lib/internal/Magento/Framework/GraphQl/Query/Resolver/Argument/FieldEntityAttributes.php Outdated
@@ -5,17 +5,17 @@
*/
declare(strict_types=1);

namespace Magento\CatalogGraphQl\Model\Resolver\Products\FilterArgument;
namespace Magento\Framework\GraphQl\Query\Resolver\Argument;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think Filter should still be part of the namespace because this class is specifically for filterable fields.

lib/internal/Magento/Framework/GraphQl/Query/Resolver/Argument/FieldEntityAttributes.php Outdated
{
$productTypeSchema = $this->config->getConfigElement('SimpleProduct');
$productTypeSchema = $this->config->getConfigElement($this->configElementName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please eliminate all product occurrences in this file since now it is generic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because currently we use only the array key

lib/internal/Magento/Framework/GraphQl/Query/Resolver/Argument/FieldEntityAttributes.php Outdated
@@ -55,12 +64,12 @@ public function getEntityAttributes() : array
$configElement = $this->config->getConfigElement($interface['interface']);

foreach ($configElement->getFields() as $field) {
$fields[$field->getName()] = 'String';
$fields[$field->getName()] = '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this was changed? I believe each filter value must be of a String type. Same below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because currently we use only the array key

lib/internal/Magento/Framework/GraphQl/Query/Resolver/Argument/FieldEntityAttributes.php Outdated
}

/**
* {@inheritdoc}
*/
public function getEntityAttributes() : array
public function getEntityAttributes(): array
Copy link
Contributor

Choose a reason for hiding this comment

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

For now this method will collect filterable fields from interface declarations and from additionalAttributes defined in di.xml, however it does not seem to collect fields from the entity itself.

paliarush
paliarush reviewed Jan 8, 2019
View reviewed changes
lib/internal/Magento/Framework/GraphQl/Query/Resolver/Argument/Filter/FieldEntityAttributes.php Outdated
{
$configElements = [];

if ($type->getInterfaces() === null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Fields should be read from interfaces AND from the main entity, not OR

@larsroettig larsroettig changed the title Improve Filter Architecture #315 Improve Filter Architecture Jan 9, 2019
@larsroettig larsroettig mentioned this pull request Jan 9, 2019
Closed
@larsroettig larsroettig mentioned this pull request Mar 3, 2019
Closed
@naydav
Copy link
Contributor

naydav commented Mar 6, 2019

Hello @larsroettig,

Unfortunately, we cannot accept this PR because this class is not used anywhere and is not covered by API-functional tests. So it will lead to class obsolescence.

Feel free to reopen with an alternative implementation. It could be an improvement in the current code or it could be some GraphQL scenario described in API-functional tests.

Thanks

@naydav naydav closed this Mar 6, 2019
@ghost
Copy link

ghost commented Mar 6, 2019

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

@lenaorobei lenaorobei deleted the filter_better_architecture branch September 10, 2019 19:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@paliarush paliarush paliarush requested changes

Assignees
No one assigned
Labels
Partner: TechDivision partners-contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@larsroettig @magento-cicd2 @naydav @paliarush @magento-engcom-team