-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Filter Feature #6684
Conversation
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
🦋 Changeset detectedLatest commit: 5c86ead The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
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>
There was a problem hiding this 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>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
There was a problem hiding this 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.
There was a problem hiding this 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.
There was a problem hiding this 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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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: {},
...
packages/api-plugin-accounts/src/resolvers/Query/filterSearchAccounts.js
Outdated
Show resolved
Hide resolved
import wasFieldRequested from "@reactioncommerce/api-utils/graphql/wasFieldRequested.js"; | ||
|
||
/** | ||
* @name Query/accounts |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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>
@vanpho93 |
* @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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing - in JSDoc
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 functionpackages/api-plugin-xxx/src/resolvers/Query/filterSearchxxx.js
. Also the correspondingschema.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.