Pdtvt 1725 filter duplicates

[jamiek-acl]

https://aclgrc.atlassian.net/browse/PDTVT-1725

Background

Before this PR, if someone added two filters: goals = 100 and goals = 200 and submitted, that would get sent to the server like this: &filter[goals]=100&filter[goals]=200. What do you think should be returned?

• players with 100 goals?
• players with 200 goals?
• no players? (since no one can have both 100 and 200 goals)

In our case, it is Rails that decides to return the players with 200 goals (the second query param overwrites the first). Using a different framework could have a different result.

And generally, it is weird that we allow the user to add an illogical/duplicate filter like this in the first place.

Purpose 🚀

1. Don't list columns more than once if doing an AND Filter, and if one of the rules is a boolean rule (IS, IS NOT, etc).
2. Hide the Add filter button once they have applied all the filters (with same rules as above).
3. If they switch from 'OR' to 'AND', remove any illogical filters
4. Don't list rules that are illogical (eg cant choose EQUALS if already filtering by a field when doing AND)

Examples

This is allowed: goals = 100 OR goals = 200
This is allowed: goals > 100 AND goals < 200
This is not: goals = 100 AND goals = 200
This is not: goals > 100 AND goals = 200

To prevent it, hide the goals field if:

• they are doing an AND Filter, and
• they already have a filter on the goals field, and
• the comparator is binary (equals, not equals)

Edge case 1

Imagine I do this in this order:

1. set it to OR
2. add a filter where goals = 100
3. add a filter where goals = 200
4. change it from OR to AND

In this case the component has to delete the goals = 200 filter automatically (jonathan wants a prompt in a perfect world, but this is a lot of work and this is just an edge case)

Edge case 2

Imagine I do this in this order:

1. set it to AND
2. add a filter where goals > 100
3. try add a filter where goals = 200 (goals is still an option because there is no other goals = filter yet)

In this case, the = comparator should not be an option for the second goals filter.

Storybook 📕

http://storybooks.highbond-s3.com/paprika/pdtvt-1725--filter-duplicates

Here I am already filtering on Goals = 1. When I add another filter, notice how Goals is not an option again:

Here I have these filters: Goals = 1 OR Goals = 2 OR Name = joe. When I switch to And, the conflicting filter (Goals = 2) is deleted automatically:

Here I already have a filter Goals > 1 AND .... Notice how I can add another filter on the Goals column, but I can't do a boolean rule (e.g. =, !=) because that would be an illogical filter.

[changeset-bot]

🦋 Changeset detected

Latest commit: d8dc3fd

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

This PR includes changesets to release 1 package

Name	Type
@paprika/filter	Minor

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

[tristanjasper]

@jamiek-acl I may have fixed the flicker you noticed in the Panel with #1166

[jamiek-acl]

@jamiek-acl I may have fixed the flicker you noticed in the Panel with #1166

I updated component in my app but still seeing the flicker

[AndreyChernykh]

Can't understand the logic of this method. Why we do processedColumnIds.push(filter.columnId) and then return false?

[jamiek-acl]

return false is within a .filter(), so it gets removed from the array (i.e. it is not an illogical filter).

The function first sorts all the the filters that exist (to simplify the following code), then goes over each.
If the current filter has a boolean rule (=, !=, etc) and is on a column that was already used by a filter, this one is illogical (so it will be removed).

For example, if the current one is goals = 1 and there was already a filter on goals, goals = 1 is illogical and will be removed.

[jamiek-acl]

Oh, and the .push() is because we keep a tally of the columnIds that already have filters... so if we see another filter on that column, we know to look into it more deeply.

[AndreyChernykh]

The function is called removeIllogicalRules but it doesn't remove anything

[jamiek-acl]

Yeah I struggled with some names. I thought it made sense from the context of where it is used (in a filter()):

{rulesByType[selectedColumnType]
  .filter(rule => removeIllogicalRules(...))
  .map()
}

Suggestions?

[jamiek-acl]

i went with isIllogicalRule

[jamiek-acl]

Leaving for @AndreyChernykh to finish this off, then update in projects (thanks Andrey!). We discussed and a few changes are to be made:

1. dont call it boolean/binary, call it equality
2. sub-group all operators into equality and inequality operators.
3. for equality operators any AND doesn’t make sense (e.g. x === 3 AND x === 4)
4. for inequality operators any OR doesnt make sense (x !=2 OR x != 3)

Andrey wrote:

So, instead of doing this:

> return filtersClone.sort(putFiltersWithBooleanRulesLast).filter(filter => {

I’d first do something like:

const filtersByColumn = {}

filtersClone.sort(putFiltersWithBooleanRulesLast).forEach(filter => {
  if (!filtersByColumn[filter.columnId]) {
    filtersByColumn[filter.columnId] = [];
  }

  filtersByColumn[filter.columnId].push(filter);
})

and then process filters for each column in isolation, applying the rules above.