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

feature: add Catalog query filters #5126

Merged
merged 10 commits into from
Apr 19, 2019
Merged

Conversation

willopez
Copy link
Member

@willopez willopez commented Apr 16, 2019

Impact: minor
Type: feature

Feature

Adds Boolean filtering support for the catalogItems query. Filters are specified as a key value pair of a supported filter field,(see below) and a boolean value. A booleanFilters param will need to be sent with requests to the catalogItems query. These filters are optional.

Example query

query catalogItemsQuery($shopId: ID!, $tagIds: [ID], $booleanFilters: [CatalogBooleanFilter], $first: ConnectionLimitInt, $last:  ConnectionLimitInt, $before: ConnectionCursor, $after: ConnectionCursor, $sortBy: CatalogItemSortByField, $sortByPriceCurrencyCode: String, $sortOrder: SortOrder) {
  catalogItems(shopIds: [$shopId], tagIds: $tagIds, booleanFilters: $booleanFilters, first: $first, last: $last, before: $before, after: $after, sortBy: $sortBy, sortByPriceCurrencyCode: $sortByPriceCurrencyCode, sortOrder: $sortOrder) {
.
.
.
  }
}

Example filters:

[
        {
          name: "isSoldOut",
          value: false
        },
        {
          name: "isBackorder",
          value: false
        }
]

Currently supported fields

isDeleted | Boolean
isVisible | Boolean
isLowQuantity | Boolean
isSoldOut | Boolean
isBackorder | Boolean

Testing

  1. In the storefront, add filters as specified above
  2. confirm products are correctly filtered

Signed-off-by: Will Lopez <will.lopez77@gmail.com>
Signed-off-by: Will Lopez <will.lopez77@gmail.com>
Signed-off-by: Will Lopez <will.lopez77@gmail.com>
@willopez willopez marked this pull request as ready for review April 16, 2019 18:27
@willopez
Copy link
Member Author

@mikemurray FYI, there are some tests failing that were already failing. I fixed the one related to this PR.

@willopez willopez changed the title feature[WIP]: add Catalog collection filters feature: add Catalog collection filters Apr 16, 2019
@willopez willopez changed the title feature: add Catalog collection filters feature[WIP]: add Catalog collection filters Apr 17, 2019
@willopez willopez changed the title feature[WIP]: add Catalog collection filters feature: add Catalog collection filters Apr 18, 2019
@willopez
Copy link
Member Author

@mikemurray this is now ready for review, the underlying inventory issue has been resolved.

Copy link
Member

@mikemurray mikemurray left a comment

Choose a reason for hiding this comment

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

@willopez Allowing strings of JSON for filters is not the way to go because you're exposing mongo functionality in an API that needs to be agnostic of its underlying database. This also opens the door to random code injection and has security/performance concerns.

These filters need to be defined in the GraphQL schema so they are limited to only what's necessary, reducing the possibility of code injection, and the benefit of being introspectable.

Signed-off-by: Will Lopez <will.lopez77@gmail.com>
@willopez
Copy link
Member Author

@mikemurray I have refactored the filters to be of Boolean type and restricted the props on which to filter on with an enum.

Copy link
Contributor

@aldeed aldeed left a comment

Choose a reason for hiding this comment

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

I left a few inline comments.

Big picture, I think this is generally fine but we can't do it quite like this. Three of the filters, isLowQuantity, isSoldOut, and isBackorder are fields added and controlled by the inventory plugin, so catalog code should not reference them. So you'll need to add a way for any plugin to add supported filters, and then move these to the inventory plugin.

I think functionsByType is a good choice here. It could be called xformCatalogFilters, same as you have. Rather than calling just the one from this plugin, you'd loop over all functions registered as that type in series, and for each one that returns a Mongo filter object, merge them all together to build the final catalogFilters object. So each plugin can handle whatever filters it handles.

You'll also need to move those GraphQL schema enum values into the inventory plugin using extend enum there.

… changes

to Catalog schema

Signed-off-by: Will Lopez <will.lopez77@gmail.com>
@willopez willopez changed the title feature: add Catalog collection filters feature: add Catalog query filters Apr 19, 2019
Signed-off-by: Will Lopez <will.lopez77@gmail.com>
@willopez
Copy link
Member Author

@aldeed I have resolved all your previous comments. Wondering if I should created indices for

isDeleted
isVisible
isLowQuantity
isSoldOut
isBackorder

@aldeed
Copy link
Contributor

aldeed commented Apr 19, 2019

I'm approving the code but I did not test. @willopez Indexes are generally not very useful for fields with boolean values. The more possible values a field has, the more an index helps. But by their nature boolean fields only have 2 possible values. (ref: https://docs.mongodb.com/manual/tutorial/create-queries-that-ensure-selectivity/) So I think we can hold off adding indexes for these unless we find out they are indeed helpful in the wild.

Signed-off-by: Will Lopez <will.lopez77@gmail.com>
@mikemurray mikemurray merged commit b2bf4f0 into develop Apr 19, 2019
@mikemurray mikemurray deleted the willopez-add-catalog-filters branch April 19, 2019 20:56
@jeffcorpuz jeffcorpuz mentioned this pull request Jul 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants