+
Skip to content

Filter Feature #6684

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

Merged
merged 12 commits into from
Dec 19, 2022
Merged

Filter Feature #6684

merged 12 commits into from
Dec 19, 2022

Conversation

sujithvn
Copy link
Contributor

@sujithvn sujithvn commented Dec 2, 2022

Signed-off-by: Sujith mail.sujithvn@gmail.com

Resolves #6528
Impact: major
Type: feature

Issue

As mentioned in the ticket, requirement is to provide filtering/search functionality and provide a consistent pattern/interface across all Data objects where it makes sense, but for now focused on Products/Customers/Orders.

Solution

The format of the input data (conditions) to perform the filter is described in the fle packages/api-core/src/graphql/schema.graphql
A new generic function to handle the incoming conditions is defined in packages/api-utils/lib/generateFilterQuery.js

For each of the collection where filter feature has to be enabled, we basically need two new files packages/api-plugin-xxx/src/queries/filterSearchxxx.js and the corresponding resolver function packages/api-plugin-xxx/src/resolvers/Query/filterSearchxxx.js. Also the corresponding schema.graphql also has to be updated to add the new query endpoint.

We have implemented this currently in Orders, Products, Accounts/Customers collections and the same set of changes could be used as template for other collections.

Breaking changes

No breaking change expected as all the files and changes are new.

Testing

Unit test file for the generateFilterQuery.js updated.

Signed-off-by: Sujith <mail.sujithvn@gmail.com>
@changeset-bot
Copy link

changeset-bot bot commented Dec 2, 2022

🦋 Changeset detected

Latest commit: 5c86ead

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@reactioncommerce/api-core Minor
@reactioncommerce/api-plugin-accounts Minor
@reactioncommerce/api-plugin-orders Minor
@reactioncommerce/api-plugin-products Minor
@reactioncommerce/api-utils Minor
reaction Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Signed-off-by: Sujith <mail.sujithvn@gmail.com>
@sujithvn sujithvn changed the title feat: filterSearch initial version Filter Feature (initial version) Dec 3, 2022
Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

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

This needs some really good documentation and examples before I think anyone is going to be able to figure out how to use this. Probably emphasis on the examples

Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
@brent-hoover brent-hoover changed the title Filter Feature (initial version) Filter Feature Dec 8, 2022
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
@sujithvn sujithvn requested a review from brent-hoover December 8, 2022 10:36
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 didn't test it but it looks like a nice flexible pattern in general.

aldeed
aldeed previously approved these changes Dec 11, 2022
Copy link
Member

@vanpho93 vanpho93 left a comment

Choose a reason for hiding this comment

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

plugin orders will only works with the latest api-utils after this?
If that, in plugin orders package.json, we should change "@reactioncommerce/api-utils": "^1.16.5" to "1.17.0". The same with accounts / product.

Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

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

I think it still needs a little polish

}
await context.validatePermissions("reaction:legacy:accounts", "read", { shopId });

const { filterQuery } = generateFilterQuery(context, "Account", conditions, shopId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The fact that this is Account and not Accounts seems worrisome

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 is 'Account' in SimpleSchema.

  Account: SimpleSchema {
    pick: [Function: pickOrOmit],
    omit: [Function: pickOrOmit],
    _constructorOptions: { humanizeAutoLabels: true, requiredByDefault: true },
    _cleanOptions: {
      autoConvert: true,
      extendAutoValueContext: {},
      ...

import wasFieldRequested from "@reactioncommerce/api-utils/graphql/wasFieldRequested.js";

/**
* @name Query/accounts
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason this has to be a separate endpoint rather than functionality added on to the existing queries?

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 existing query endpoint is using a fixed input structure like so
const { groupIds, notInAnyGroups } = input;

The new filter has an entirely different input format to handle multiple conditions. So I assume it will break any existing implementation of the query if we change it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we not extend it while not breaking the existing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking that we keep the implementation of the new 'filter' feature as a standard/similar implementation across all the selected collections. Something like a common format including the name of the query endpoint.

All the existing end-points (for accounts/customers/orders/products) accept input in different formats and we would have to make different changes in each of these to accept both the old & new format of input.

Note: I have not checked, but we may again hit the earlier problem of graphql not flexible in allowing different input formats.

Please let me know if we want to make these changes for all the collections or selected ones.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can see the logic of having consistent naming of the endpoint across collections but I also think it's confusing to have two endpoints that do basically the same thing but differently. What do you suggest to resolve this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you be okay to use 'filterXYZ' as the go-forward endpoint across all collections and mark the old ones as 'deprecated'? If so, we can add the new filter to all collections where there is something similar currently in place.

Else, as you mentioned initially, we have to update the existing query end-points. We can start & test with a simple one like accounts (to confirm graphql does not create issues) and then implement that for the remaining 3 (customers/orders/products). In this case also, we may have to implement this update to other collections where there is something similar currently in place.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I'm fine with the "create new and deprecate" old approach

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I shall find other collections that are using 'filter' like query and add new filter along with the promotions-filter ticket

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, let's just stick with the three for now

Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
@sujithvn
Copy link
Contributor Author

plugin orders will only works with the latest api-utils after this? If that, in plugin orders package.json, we should change "@reactioncommerce/api-utils": "^1.16.5" to "1.17.0". The same with accounts / product.

@vanpho93
As discussed, updated the api-util version to ~1.17.1 in accounts/products/orders.

* @param {String} args.shopId - id of shop to query
* @param {Object} args.conditions - object containing the filter conditions
* @param {Object} context - an object containing the per-request state
* @param {Object} info Info about the GraphQL request
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing - in JSDoc

@brent-hoover brent-hoover merged commit 924e7f1 into trunk Dec 19, 2022
@brent-hoover brent-hoover deleted the feat-filter-search branch December 19, 2022 09:47
@github-actions github-actions bot mentioned this pull request Jun 13, 2023
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.

SPIKE: Doing Filtering/Search across Products/Customers/Orders
4 participants
点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载