[NEP-12629] Ability to use "OR" between filters (+ filter groups) #108

footnoise · 2022-01-17T04:48:24Z

Original ticket: https://jobready.atlassian.net/browse/NEP-12629

Working on new version which allows using OR and AND operators with filters.

So, before we had registered filters in a hash and it was represented in FilterBarStore like that:

{
  "some_filter_key1": {
    "uid" : "some_filter_key1",
    "value" : "",
    "name" : "Some Filter Key 1",
    ...
  }, {
    "some_filter_key2" : {
    "uid" : "some_filter_key2",
    "value" : "",
    "name" : "Some Filter Key 2",
    ...
  }
}

and if a User selected some filters, for example with the key some_filter_key1 we needed mark it somehow and don't allow user to select that filter again (it should be removed from dropdown) and to make it we should call method this.context.filterBarStore.getDisabled() and pass it as disabledFilters property into component.

The new logic works in completely different way, we still have original registered hash of filters (as I described above) but data and values we have in the same FilterBarStore but in activeFilters array. In this case I use groupKey and inputKey properties to manage values data. It's a double array, which has groupKey as ID of the first array and inputKey as ID for the second array.

So, that means we should hide filters from dropdown and we don't need to set property disabledFilters anymore.

SyntheticDave

Amazing work, @footnoise!
I've left some questions here mostly for my own understanding of the code. I primarily want to make sure:

Existing saved searches, quick filters, and links to filtered index pages (e.g. from reports) still work as previously (if I've read the code right, this should be the case)
We use inline styles as little as possible, so anything can be overwritten at the application level

SyntheticDave · 2022-01-28T02:21:08Z

bower.json

@@ -1,7 +1,7 @@
 {
  "name": "react-filterbar",
  "homepage": "https://github.com/jobready/react-filterbar",
-  "version": "1.7.1",
+  "version": "2.1",


Odd that this was out of sync with the package file.

Would this be better represented by a major upgrade 3.0 since there may not exist backward compatibility with previous design?

SyntheticDave · 2022-01-28T02:21:32Z

docs/react-components-structure.md

@@ -0,0 +1,29 @@
+React Components Structure


Good to see this here ✨

SyntheticDave · 2022-01-28T02:29:54Z

src/components/FilterBar/FilterBar.react.js

-            <FilterList
-              disabledFilters={this.context.filterBarStore.getDisabled()}
-            />


What was the purpose of this previously?

Yeah @SyntheticDave sorry, potentially I should put that architectural changes in PR description, because it's a huge changes. Definitely will do it before moving PR into Release stage.

So, before we had registered filters in a hash and it was represented in FilterBarStore like that:

{ "some_filter_key1": { "uid" : "some_filter_key1", "value" : "", "name" : "Some Filter Key 1", ... }, { "some_filter_key2" : { "uid" : "some_filter_key2", "value" : "", "name" : "Some Filter Key 2", ... } }

and if a User selected some filters, for example with the key some_filter_key1 we needed mark it somehow and don't allow user to select that filter again (it should be removed from dropdown) and to make it we should call method this.context.filterBarStore.getDisabled() and pass it as disabledFilters property into component.

The new logic works in completely different way, we still have original registered hash of filters (as I described above) but data and values we have in the same FilterBarStore but in activeFilters array. In this case I use groupKey and inputKey properties to manage values data. It's a double array, which has groupKey as ID of the first array and inputKey as ID for the second array.

So, that means we should hide filters from dropdown and we don't need to set property disabledFilters anymore.

Thanks for the explanation. I definitely like the change, but you're right that it's a big one.

SyntheticDave · 2022-01-28T02:38:39Z

src/components/FilterBar/FilterDisplay/FilterDisplay.react.js

+      filters.push(
+        (<FilterGroup
+          key={ idx }
+          groupKey={ idx }
+          filters={ groupFilters }
+        />)


So does this mean if there's no group specified, all filters get automatically added to a new first group?
If so, it seems like existing saved searches and existing links to filters will still work correctly? Would be amazing if so.

Well, yeah, that's a good point about SavedSearches. My thinking was that SavedSearches should be presented as is with OLD format (w/o groups) and should be converted to NEW format (w/ groups) when new react component loads it the first time and SavedSearch should be saved in NEW format after if user decides to save it.

We'll definitely want to make sure we don't break existing saved searches or links in the application that direct to filtered index pages (e.g. reports often link to an index page with the same filters as is used by that cell in the report), so we'll need to make sure this is backwards compatible with those.

SyntheticDave · 2022-01-28T02:41:34Z

src/components/FilterBar/FilterDisplay/FilterDisplay.react.js


    if (filters.length === 0) {
-      filters = (<div>No Filters Enabled!</div>);
+      filters.push((
+        <div style={ { marginTop: 'auto', marginBottom: 'auto', padding: '10px'} }>


There's a bunch of inline styling here. Is it instead worth creating classes for this so they can be overridden at the application level if needed?

Yeah, currently thinking how to make it better and replace inline styles with reusable css.

Don't stress too much about it. If it's too big a job to do so, I'd create a tech debt ticket to add that in later, as inline styles should be okay in the short term.

SyntheticDave · 2022-01-28T02:58:42Z

src/components/FilterBar/FilterDisplay/Inputs/DateInput.react.js

@@ -24,7 +24,7 @@ export class DateInput extends React.Component {
  }

  onBlur() {
-    this.context.filterBarActor.updateFilter(this.props.filterUid, "value", this.state.value);
+    this.context.filterBarActor.updateFilter(this.props.groupKey, this.props.inputKey, this.state.value);


So inputKey used to just be "value"?

Yes, groupKey and inputKey here are just IDs where exactly values should be set. Object filterBarActor has activeFIlters array with filters values which can be presented like that:

activeFIlters = [ ["value1", "value2"], ["value3", "value4"] ]

and if we need to update value for example for filter2 ("value2" is located in 1st group) we should pass groupKey as 0 (first index in array) and inputKey as 1 (second index in array)

SyntheticDave · 2022-01-28T02:59:30Z

src/components/FilterBar/FilterDisplay/Inputs/LazyMultiSelectInput.react.js

@@ -72,6 +72,7 @@ export class LazyMultiSelectInput extends React.Component {
    } else {
      filter.value = event.target.value.split(",");
    }
+    this.context.filterBarActor.updateFilter(this.props.groupKey, this.props.inputKey, filter.value);


Do you know why this was not needed for this filter type previously?

Yes, because react-component and filters had different architecture & structure before (see my first comment above) but now we need to set selected data to activeFilters array.

SyntheticDave · 2022-01-28T04:28:16Z

src/stores/FilterBarStore.js

-    for (var filterUid in enabledFilters) {
-      this.disableFilter(filterUid);
-    }
+    this.activeFilters = []


So we don't need to call disable filter on each individually now?

Yes, because architecture and data-structure changed.

SyntheticDave · 2022-01-28T04:28:37Z

src/stores/FilterBarStore.js

+  }
+
+  updateFilter(groupKey, inputKey, value) {
+    //this.deactivateQuickFiltersBasedOnFilterValue(filterUid, propValue, this.activeQuickFilters());


Should this be removed?

Oh yes, definitely!

SyntheticDave · 2022-01-28T04:29:45Z

src/stores/FilterBarStore.js

+    this.activeFilters[groupKey][inputKey].value = value;
+  }
+
+  addGroupFilter(groupKey, filterUid) {


As mentioned elsewhere, changing the order of these params would allow you to pass groupKey optionally, then just check for undefined.

Yep, that's a good point! I will follow your suggestion 👌

msxavi

This is massive and neat. Great effort @footnoise !

msxavi · 2022-02-06T21:58:30Z

bower.json

@@ -1,7 +1,7 @@
 {
  "name": "react-filterbar",
  "homepage": "https://github.com/jobready/react-filterbar",
-  "version": "1.7.1",
+  "version": "2.1",


Would this be better represented by a major upgrade 3.0 since there may not exist backward compatibility with previous design?

msxavi · 2022-02-06T22:01:00Z

package.json

@@ -1,6 +1,6 @@
 {
  "name": "react-filterbar",
-  "version": "2.0",
+  "version": "2.1",


As mentioned above, major upgrade perhaps?

@msxavi I'm happy with any version actually and if you think 3.0 better reflects current changes and improvements we can change it 👌

msxavi · 2022-02-06T22:06:22Z

src/actors/FilterBarActor.js

    this.filterBarStore.enableQuickFilter(quickFilterName, blockName);
-    this.enableFilter(filterName, value);


Do you still reference enableFilter() in this file?

msxavi · 2022-02-06T22:13:44Z

src/components/FilterBar/FilterDisplay/Inputs/DateInput.react.js

+    if (this.props.onBlur) {
+      this.props.onBlur();
+    } else {
+      this.context.filterBarActor.updateFilter(this.props.groupKey, this.props.inputKey, this.state.value);
+    }


I didn't quite follow this change. Why filter types are being affected by groups? Is there any reason where a Filter needs to know the group it belongs to?

@msxavi yes it's a bit tricky and most complicated part of the code.
As I mentioned in PR descripute now component has new data strucure it two-sized array activeFilters in FilterBarStore.
It can be presented like this:

this.activeFilters = [ [Filter1, Filter2], // <- Group1 [Filter3], // <- Group2 [Filter4, Filter5] // <- Group3 ]

As you can see the external array presents groups and interal array presents filters inside each group.
Groups/Filters rendering based on that activeFitlers variable and to identify correct pathway to manage data-values we heed groupKey which represents group index in external array and we need inputKey which represents filter index in internal array. When we change values in filter component we should update data in Store and set updated values in activeFilters and specially in this case we should use groupKey and inputKey like this:

this.activeFilters[groupKey][inputKey] = newFilterValue;

thoran

I'm not Mr. React exactly, but apart from one file which Github refused to show me it looks fine.

thoran

Much tidier. The comments are generally inconsequential.

thoran · 2022-03-02T23:18:36Z

dist/react-filterbar.js

-        filtersArr = filters;
-      } else {
-        filtersArr = Object.keys(filters).map(function (name) {
+      if (!Array.isArray(savedSearchFilters)) {


For instance, why is this better than say if (savedSearchFilters instanceof Array)?

thoran · 2022-03-02T23:25:50Z

dist/react-filterbar.js

          return {
            uid: name
          };
        });
      }

-      return new _FilterVerificator.FilterVerificator(this.filterBarStore.getFilters(), filtersArr).verify();
+      if (!Array.isArray(filters[0])) {


Is this overly defensive?

thoran · 2022-03-02T23:35:32Z

dist/react-filterbar.js

+      return filters;
+    }
+  }, {
+    key: "verifySavedFilters",


I see. You've moved the filter 'grooming' to a new method.

thoran · 2022-03-02T23:37:37Z

dist/react-filterbar.js

-      _iterator.e(err);
-    } finally {
-      _iterator.f();
+    if (Array.isArray(activeFilters[0])) {


Might be slightly easier on the eye if it was if (!Array.isArray(activeFilters[0])) { then it would ...parseQueryVersion1... then ...parseQueryVersion2.

thoran · 2022-03-02T23:57:38Z

example/server.rb

+    field, type, value = nil
+
+    if needle.is_a? Array
+      # Case 1:  Filters with new format


I like that these are sign-posted.

footnoise force-pushed the feature/NEP-12629 branch 3 times, most recently from bd08153 to 37b8d05 Compare January 20, 2022 22:44

[NEP-12629] Rework filters logic by adding AND/OR conditions.

e6ad99e

footnoise force-pushed the feature/NEP-12629 branch from 37b8d05 to e6ad99e Compare January 24, 2022 06:23

Sergei Aksiutin added 2 commits January 24, 2022 16:31

[NEP-12629] Bump version to 2.1

2a41da1

[NEP-12629] Fix "X" icon position

affdf0e

footnoise marked this pull request as ready for review January 24, 2022 22:22

[NEP-12629] Dist

95206e3

SyntheticDave reviewed Jan 28, 2022

View reviewed changes

Sergei Aksiutin added 2 commits February 1, 2022 17:07

[NEP-12629] Update example on server

d629f0f

[NEP-12629] Initial URL filters

0ad2738

footnoise force-pushed the feature/NEP-12629 branch from 27c78de to 0ad2738 Compare February 6, 2022 14:13

msxavi reviewed Feb 6, 2022

View reviewed changes

[NEP-12629] Change version to 3.0

df27902

nrankin approved these changes Feb 8, 2022

View reviewed changes

footnoise force-pushed the feature/NEP-12629 branch from 1489a93 to eab2c74 Compare February 14, 2022 11:59

msxavi approved these changes Feb 14, 2022

View reviewed changes

footnoise force-pushed the feature/NEP-12629 branch 2 times, most recently from 106b84b to 2b9579b Compare February 20, 2022 14:02

[NEP-12629] Fix filters default value.

828327a

footnoise force-pushed the feature/NEP-12629 branch from 2b9579b to 828327a Compare February 20, 2022 14:49

jbat approved these changes Feb 20, 2022

View reviewed changes

thoran approved these changes Feb 20, 2022

View reviewed changes

footnoise force-pushed the feature/NEP-12629 branch from 4d12e5b to d194b8c Compare March 1, 2022 14:25

jbat approved these changes Mar 1, 2022

View reviewed changes

footnoise force-pushed the feature/NEP-12629 branch from d194b8c to e0d3e29 Compare March 2, 2022 15:12

thoran approved these changes Mar 3, 2022

View reviewed changes

footnoise force-pushed the feature/NEP-12629 branch from e0d3e29 to 4987cb3 Compare March 6, 2022 16:12

[NEP-12629] Fix default filters URLs with old format for SavedSearches.

d52e253

footnoise force-pushed the feature/NEP-12629 branch from 4987cb3 to d52e253 Compare March 6, 2022 16:49

Sergei Aksiutin added 2 commits March 15, 2022 01:20

[NEP-12629] Fix filter labels for SavedSearch

a4b3b44

[NEP-12629] Add debug version to console.log

b012d4f

		this.filterBarStore.enableQuickFilter(quickFilterName, blockName);
		this.enableFilter(filterName, value);

[NEP-12629] Ability to use "OR" between filters (+ filter groups) #108

Are you sure you want to change the base?

[NEP-12629] Ability to use "OR" between filters (+ filter groups) #108

Conversation

footnoise commented Jan 17, 2022 • edited Loading

SyntheticDave left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

footnoise Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msxavi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

footnoise Feb 7, 2022 • edited Loading

Choose a reason for hiding this comment

thoran left a comment

Choose a reason for hiding this comment

thoran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

footnoise commented Jan 17, 2022 •

edited

Loading

footnoise Jan 31, 2022 •

edited

Loading

footnoise Feb 7, 2022 •

edited

Loading