Fixed the "Is blank"/"Is not blank" filters for the tags field type #3637

jskupsik · 2024-04-22T22:39:37Z

Hoist P/R Checklist

Pull request authors: Review and check off the below. Items that do not apply can also be
checked off to indicate they have been considered. If unclear if a step is relevant, please leave
unchecked and note in comments.

Caught up with develop branch as of last change.
Added CHANGELOG entry, or determined not required.
Reviewed for breaking changes, added breaking-change label + CHANGELOG if so.
Updated doc comments / prop-types, or determined not required.
Reviewed and tested on Mobile, or determined not required.
Created Toolbox branch / PR, or determined not required.

If your change is still a WIP, please use the "Create draft pull request" option in the split
button below to indicate it is not ready yet for a final review.

Pull request reviewers: when merging this P/R, please consider using a squash commit to
collapse multiple intermediate commits into a single commit representing the overall feature
change. This helps keep the commit log clean and easy to scan across releases. PRs containing a
single commit should be rebased when possible.

jskupsik · 2024-04-22T22:43:30Z

Client has complained that the "Is blank"/"Is not blank" filter options present in the custom filter on the grid column filter do not work as an "is empty"/"is non-empty" when set on a tag/array object. I have fixed this by making filter op/values of "= null" and "!= null" also succeed for the cases of "empty array" and "non-empty array".

… option

…alue

jskupsik · 2024-04-23T19:00:30Z

Fixed bug where opening the grid filter for a tags column would cause error + clearing of that field.
Fixed bug where selecting 'is (not) blank' for a tag would introduce an erroneous selectable value '[blank]' in the Value filter and the Custom filter's 'Includes'/'Excludes', which would result in a bogus 'includes undefined'.
Fixed bug where switching between the Value and Custom tab did not initialize the filter value.

jskupsik · 2024-04-23T19:01:30Z

Toolbox PR: https://github.com/xh/toolbox/pull/712/commits

…ld is/is not empty

lbwexler · 2024-07-06T10:41:40Z

cmp/grid/filter/GridFilterFieldSpec.ts

+                    })
+                    // Filter out the null tag value as it isn't a valid value to display in value lists.
+                    // It is set by 'is blank'/'is not blank' filters.
+                    .filter(it => isEqual(it, [null]))


seems safe enough, independent of whether we continue to put null in as filter values.

lbwexler · 2024-07-06T10:48:51Z

desktop/cmp/grid/impl/filter/ColumnHeaderFilter.ts

@@ -119,6 +119,6 @@ const switcherButton = hoistCmp.factory<ColumnHeaderFilterModel>(({model, id, ti
        text: title,
        active: activeTabId === id,
        outlined: true,
-        onClick: () => tabContainerModel.activateTab(id)
+        onClick: () => model.activateTab(id)


tabContainerModel reference no longer needed on line 114

lbwexler · 2024-07-06T11:01:10Z

data/filter/FieldFilter.ts

@@ -128,14 +130,14 @@ export class FieldFilter extends Filter {
        switch (op) {


would rValue and fValue be more helpful here? Very hard to keep track of which is which.

Great suggestion! Might I suggest we are even more explicit and call them recordVal and filterVal (i.e. more characters for the source than the word 'value')

lbwexler · 2024-07-06T11:08:06Z

desktop/cmp/grid/impl/filter/ColumnHeaderFilterModel.ts

-        return columnFilters.some(it => !['=', '!=', 'includes'].includes(it.op));
+        return columnFilters.some(
+            it =>
+                !['=', '!=', 'includes'].includes(it.op) ||


this code (line 75 and 77) would be easier to grok if we expanded out to equals checks, and did not try to use the javascript 'includes' function . We are already dealing with an 'includes' filter and it is confusing!

lbwexler · 2024-07-06T11:12:01Z

desktop/cmp/grid/impl/filter/custom/CustomRowModel.ts

@@ -82,7 +82,7 @@ export class CustomRowModel extends HoistModel {
        makeObservable(this);

        let newOp = op as OperatorOptionValue;
-        if (isNil(value)) {
+        if (isNil(value) || (isArray(value) && isEmpty(value))) {


might add a util isEmptyArray() -- we do this check 4 times in this PR -- might be others in hoist

TomTirapani

Thanks for this Jakub - looks and works great! Thanks also for setting up the Toolbox branch, made it very easy to reproduce the issues against develop before testing your fixes in this branch.

TomTirapani · 2024-07-08T11:16:19Z

data/filter/FieldFilter.ts

@@ -128,14 +130,14 @@ export class FieldFilter extends Filter {
        switch (op) {


Great suggestion! Might I suggest we are even more explicit and call them recordVal and filterVal (i.e. more characters for the source than the word 'value')

TomTirapani · 2024-07-08T11:20:33Z

cmp/grid/filter/GridFilterFieldSpec.ts

-                return filterModel.toDisplayValue(value);
-            });
+            // The parseVal of tag will castArray the value a second time, so make sure to flatten at the end.
+            const newValues = flatten(


Minor thing, but I think this would be more readable if we flatten() on line 96 as we push to filterValues

TomTirapani · 2024-07-08T11:25:09Z

I noticed a minor issue when playing around with this - here's how to reproduce:

Set a filter using the values tab (e.g. tag1 & tag2)
Switch to the custom filter tab and replace the "includes" filter with an "is blank" / "is not blank" filter
Switch back to the values tab and it will say "Custom filter active". If you click Clear the custom filter will be removed, but tag1 and tag2 will still be selected despite there being no filter.

I think the fix is probably as simple as syncing the values tab to the filter after clicking the central clear button.

amcclain · 2024-08-30T20:35:23Z

@jskupsik - wanted to ensure that you saw the comments above from earlier in the summer. It would be good to catch this up and have another look.

Fixed the "Is blank"/"Is not blank" filters for the tags field type

32446cd

jskupsik added bug small client-request labels Apr 22, 2024

jskupsik added 3 commits April 23, 2024 11:07

Fixed bug with GridFilter and tags

fbc5741

Fixed "is blank" resulting in a broken "[blank]"/"includes undefined"…

3e48929

… option

Switching between Values and Custom column filter tabs syncs filter v…

e384044

…alue

Updated CHANGELOG.md

19c4196

jskupsik requested review from haynesjm42 and TomTirapani April 23, 2024 20:29

jskupsik added 2 commits May 6, 2024 18:17

FieldFilters with = or != and value of [] will correctly check if fie…

abfa491

…ld is/is not empty

Tweak to getTestFn()

faf2ca7

lbwexler reviewed Jul 6, 2024

View reviewed changes

TomTirapani added 2 commits July 8, 2024 12:47

Merge branch 'refs/heads/develop' into tagsIsBlankFilterFix

c233c4c

Update CHANGELOG

a5631a7

TomTirapani approved these changes Jul 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed the "Is blank"/"Is not blank" filters for the tags field type #3637

Fixed the "Is blank"/"Is not blank" filters for the tags field type #3637

jskupsik commented Apr 22, 2024 •

edited

Loading

jskupsik commented Apr 22, 2024 •

edited

Loading

jskupsik commented Apr 23, 2024

jskupsik commented Apr 23, 2024

lbwexler Jul 6, 2024

lbwexler Jul 6, 2024

lbwexler Jul 6, 2024

TomTirapani Jul 8, 2024

lbwexler Jul 6, 2024 •

edited

Loading

lbwexler Jul 6, 2024

TomTirapani left a comment

TomTirapani Jul 8, 2024

TomTirapani Jul 8, 2024

TomTirapani commented Jul 8, 2024

amcclain commented Aug 30, 2024

		@@ -128,14 +130,14 @@ export class FieldFilter extends Filter {
		switch (op) {

Fixed the "Is blank"/"Is not blank" filters for the tags field type #3637

Are you sure you want to change the base?

Fixed the "Is blank"/"Is not blank" filters for the tags field type #3637

Conversation

jskupsik commented Apr 22, 2024 • edited Loading

Hoist P/R Checklist

jskupsik commented Apr 22, 2024 • edited Loading

jskupsik commented Apr 23, 2024

jskupsik commented Apr 23, 2024

lbwexler Jul 6, 2024

Choose a reason for hiding this comment

lbwexler Jul 6, 2024

Choose a reason for hiding this comment

lbwexler Jul 6, 2024

Choose a reason for hiding this comment

TomTirapani Jul 8, 2024

Choose a reason for hiding this comment

lbwexler Jul 6, 2024 • edited Loading

Choose a reason for hiding this comment

lbwexler Jul 6, 2024

Choose a reason for hiding this comment

TomTirapani left a comment

Choose a reason for hiding this comment

TomTirapani Jul 8, 2024

Choose a reason for hiding this comment

TomTirapani Jul 8, 2024

Choose a reason for hiding this comment

TomTirapani commented Jul 8, 2024

amcclain commented Aug 30, 2024

jskupsik commented Apr 22, 2024 •

edited

Loading

jskupsik commented Apr 22, 2024 •

edited

Loading

lbwexler Jul 6, 2024 •

edited

Loading