Filter Feature #6684

sujithvn · 2022-12-02T19:06:06Z

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 · 2022-12-02T19:06:09Z

🦋 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>

brent-hoover

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

packages/api-core/src/graphql/schema.graphql

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

aldeed

I didn't test it but it looks like a nice flexible pattern in general.

packages/api-core/src/graphql/schema.graphql

vanpho93

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.

brent-hoover

I think it still needs a little polish

packages/api-core/src/graphql/schema.graphql

brent-hoover · 2022-12-12T03:36:36Z

packages/api-plugin-accounts/src/queries/filterSearchAccounts.js

+  }
+  await context.validatePermissions("reaction:legacy:accounts", "read", { shopId });
+
+  const { filterQuery } = generateFilterQuery(context, "Account", conditions, shopId);


The fact that this is Account and not Accounts seems worrisome

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

brent-hoover · 2022-12-12T03:39:40Z

packages/api-plugin-accounts/src/resolvers/Query/filterSearchAccounts.js

+import wasFieldRequested from "@reactioncommerce/api-utils/graphql/wasFieldRequested.js";
+
+/**
+ * @name Query/accounts


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

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.

Can we not extend it while not breaking the existing?

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.

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?

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.

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

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

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

packages/api-plugin-products/README.md

packages/api-utils/docs/generateFilterQuery.md

packages/api-utils/package.json

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

sujithvn · 2022-12-16T07:24:52Z

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.

brent-hoover · 2022-12-19T09:43:37Z

packages/api-plugin-accounts/src/resolvers/Query/filterAccounts.js

+ * @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


missing - in JSDoc

feat: filterSearch initial version

e5a287a

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

fix: graphql linter errors and reversal sampledata

d59ee6f

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

sujithvn changed the title ~~feat: filterSearch initial version~~ Filter Feature (initial version) Dec 3, 2022

sujithvn requested review from vanpho93, aldeed, tedraykov and brent-hoover December 3, 2022 03:09

brent-hoover requested changes Dec 5, 2022

View reviewed changes

packages/api-core/src/graphql/schema.graphql Outdated Show resolved Hide resolved

packages/api-core/src/graphql/schema.graphql Outdated Show resolved Hide resolved

packages/api-core/src/graphql/schema.graphql Outdated Show resolved Hide resolved

sujithvn added 3 commits December 6, 2022 18:38

fix: review comment fixes

566714e

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

fix: review comment fixes 2

21c62f1

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

fix: add changeset and unit test

315bb97

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

brent-hoover changed the title ~~Filter Feature (initial version)~~ Filter Feature Dec 8, 2022

fix: update README of plugins - utils & products

cde07fd

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

sujithvn requested a review from brent-hoover December 8, 2022 10:36

aldeed reviewed Dec 8, 2022

View reviewed changes

packages/api-core/src/graphql/schema.graphql Outdated Show resolved Hide resolved

packages/api-core/src/graphql/schema.graphql Show resolved Hide resolved

aldeed previously approved these changes Dec 11, 2022

View reviewed changes

vanpho93 requested changes Dec 12, 2022

View reviewed changes

brent-hoover requested changes Dec 12, 2022

View reviewed changes

fix: more review comment fixes

9dc4fe9

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

sujithvn dismissed aldeed’s stale review via 9dc4fe9 December 14, 2022 06:11

sujithvn added 5 commits December 14, 2022 11:46

fix: fixed 1 missed entry in test case

8c72369

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

fix: upgrade version of api-util and simp-schema

0bc6279

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

Merge branch 'trunk' into feat-filter-search

0e4b0dc

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

fix: update api-util and pnpm-lock

f16ad92

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

fix: revert simpl-schema version to 1.12

5c86ead

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

sujithvn requested review from brent-hoover and vanpho93 December 16, 2022 07:46

vanpho93 approved these changes Dec 16, 2022

View reviewed changes

brent-hoover approved these changes Dec 19, 2022

View reviewed changes

brent-hoover merged commit 924e7f1 into trunk Dec 19, 2022

brent-hoover deleted the feat-filter-search branch December 19, 2022 09:47

github-actions bot mentioned this pull request Feb 6, 2023

Version Packages (next) tedraykov/reaction#6

Merged

github-actions bot mentioned this pull request Jun 13, 2023

Version Packages #6852

Merged

Filter Feature #6684

Filter Feature #6684

Uh oh!

Conversation

sujithvn commented Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Solution

Breaking changes

Testing

Uh oh!

changeset-bot bot commented Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

brent-hoover left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aldeed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vanpho93 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brent-hoover left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sujithvn commented Dec 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sujithvn commented Dec 2, 2022 •

edited

Loading

changeset-bot bot commented Dec 2, 2022 •

edited

Loading

vanpho93 left a comment •

edited

Loading