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.

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

Filtering interface implementation #3952

Merged
merged 42 commits into from Apr 24, 2019
Merged

Filtering interface implementation #3952

merged 42 commits into from Apr 24, 2019

Conversation

korycins
Copy link
Member

@korycins korycins commented Apr 10, 2019

Backend part - #2588

This feature introduces base filter logic that can be used in any graphQL queries.

How it works:
I've created EnumFilter which handle graphQL enums, it was needed to serve enum fields over api. Based on django-filters and EnumFilter we create new FilterSet object with all fields that we want to use in filters. Our custom FilterSet should be assigned in a derived class from FilterInputObjectType. FilterInputObjectType is responsible for converting all django-filter's fields to graphene fields (also enum fields). An object of a class that derives from FilterInputObjectType should be assigned as a new param in FilterInputConnectionField (by default it should be filters=FilterInputObjectType(), but you can also pass custom filters name - All filters will be available under this name in API).
You can also check the test: test_filter_input which. The test contains configuration step by step.

What do you think? If you like it we can think about the implementation of the filters for the rest of the queries. For now, I've added some filters only for products view.
We can also think about some small documentation chapter about filtration.

Pull Request Checklist

  1. Privileged views and APIs are guarded by proper permission checks.
  2. All visible strings are translated with proper context.
  3. All data-formatting is locale-aware (dates, numbers, and so on).
  4. Database queries are optimized and the number of queries is constant.
  5. Database migration files are up to date.
  6. The changes are tested.
  7. The code is documented (docstrings, project documentation).
  8. GraphQL schema and type definitions are up to date.
  9. Changes are mentioned in the changelog.

@korycins korycins changed the title Add initial implementation filtering interface implementation Apr 10, 2019
Copy link
Member

@NyanKiyoshi NyanKiyoshi left a comment

Choose a reason for hiding this comment

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

Whenever you have 5 minutes before switching from draft to ready for review, could you add look back at the changes and put some more comments so the code will be quicker to read when not familiar with the implementation?

@korycins
Copy link
Member Author

@NyanKiyoshi No worries :)

@korycins korycins self-assigned this Apr 10, 2019
@korycins korycins added graphql Issues related to the GraphQL API in progress labels Apr 10, 2019
@korycins korycins marked this pull request as ready for review April 11, 2019 12:31
@codecov
Copy link

codecov bot commented Apr 11, 2019

Codecov Report

Merging #3952 into master will decrease coverage by 0.02%.
The diff coverage is 90.86%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3952      +/-   ##
==========================================
- Coverage    91.3%   91.27%   -0.03%     
==========================================
  Files         267      275       +8     
  Lines       14501    14953     +452     
  Branches     1393     1448      +55     
==========================================
+ Hits        13240    13649     +409     
- Misses        894      913      +19     
- Partials      367      391      +24
Impacted Files Coverage Δ
saleor/graphql/product/resolvers.py 94.56% <100%> (+0.75%) ⬆️
saleor/graphql/discount/schema.py 100% <100%> (ø) ⬆️
saleor/graphql/order/schema.py 100% <100%> (ø) ⬆️
saleor/graphql/core/types/__init__.py 100% <100%> (ø) ⬆️
saleor/graphql/product/enums.py 90.47% <100%> (+2.59%) ⬆️
saleor/graphql/core/types/converter.py 100% <100%> (ø)
saleor/graphql/product/types/attributes.py 100% <100%> (ø)
saleor/graphql/account/schema.py 100% <100%> (ø) ⬆️
saleor/graphql/core/types/common.py 100% <100%> (ø) ⬆️
saleor/graphql/order/types.py 83.24% <100%> (+0.08%) ⬆️
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d0ba739...84881bb. Read the comment docs.

Copy link
Member

@NyanKiyoshi NyanKiyoshi left a comment

Choose a reason for hiding this comment

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

LGTM, good job! 🎉

tests/api/test_product.py Outdated Show resolved Hide resolved
saleor/graphql/core/types/filter_input.py Outdated Show resolved Hide resolved
saleor/graphql/core/filters.py Outdated Show resolved Hide resolved
saleor/graphql/core/filters.py Outdated Show resolved Hide resolved
saleor/graphql/schema.graphql Outdated Show resolved Hide resolved
saleor/graphql/core/fields.py Outdated Show resolved Hide resolved
@maarcingebala maarcingebala changed the title filtering interface implementation Filtering interface implementation Apr 15, 2019
saleor/graphql/product/types/products.py Outdated Show resolved Hide resolved
saleor/graphql/product/types/products.py Outdated Show resolved Hide resolved
saleor/graphql/product/types/products.py Outdated Show resolved Hide resolved
saleor/graphql/account/filters.py Outdated Show resolved Hide resolved
saleor/graphql/product/filters.py Outdated Show resolved Hide resolved
saleor/graphql/product/filters.py Outdated Show resolved Hide resolved
saleor/graphql/account/filters.py Outdated Show resolved Hide resolved
saleor/graphql/core/types/common.py Outdated Show resolved Hide resolved
saleor/graphql/product/types/attributes.py Outdated Show resolved Hide resolved
@maarcingebala
Copy link
Member

@korycins Looks good! I have few notes to change later but I'd really like to merge it as it is and make some improvements later. Remember to update the changelog ;)

@maarcingebala maarcingebala merged commit 49b7080 into saleor:master Apr 24, 2019
@korycins korycins deleted the 2588_filtering_interface branch October 22, 2019 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
graphql Issues related to the GraphQL API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants