Issue 1930/smart search min matching tags #1948
Conversation
| @@ -1,4 +1,4 @@ | |||
| import { FormEvent } from 'react'; | |||
| import { FormEvent, useState } from 'react'; | |||
| import { Box, Chip, MenuItem } from '@mui/material'; | |||
There was a problem hiding this comment.
This causes the lint error. import { Box, Chip, MenuItem } from '@mui/material'; should be on line 1
There was a problem hiding this comment.
I'll resolve this, no problem! Don't know why my own linting hook did not catch it.
| @@ -51,6 +51,10 @@ const PersonTags = ({ | |||
| tags: [], | |||
| }); | |||
|
|
|||
| const [selected, setSelected] = useState<CONDITION_OPERATOR | 'min_matching'>( | |||
| filter.config.condition | |||
There was a problem hiding this comment.
The reason why condition is 'any' when editing is that min_matching also has the condition 'any'. The key to differentiating between 'any' and 'at least' is the presence of min_matching. It would look like this:
- at least : {condition:'any', min_matching:1, tags:[1,2]}
- any : {condition:'any', tags:[1,2]}
There was a problem hiding this comment.
It's what happens on old line 163 that is causing headaches: min_matching: +e.target.value. That forces a zero to be entered when the user deletes the value. So they can never empty the input. We want to change min_matching to be able to have an empty value, but the smartsearch config is only typed for either a number or undefined for min_matching currently.
Which causes problems because we cant resolve this cleanly:
- at least : {condition:'any', min_matching: undefined, tags:[1,2]}
- any : {condition:'any', tags:[1,2]}
So we need something extra. Which is what the selected being able to have a value of 'min_matching' is for. The alternative is to extend the type for min_matching to allow a null value, but I was scared of touching that.
There was a problem hiding this comment.
I'm just throwing in two things that may be relevant here:
First, as you point out, { foo: undefined } is not the same as {}. But type-wise, I think they are. So it should be possible to delete the min_matching attribute, not just setting it to undefined:
delete obj['min_matching']; // results in object without `min_matching`
obj.min_matching = undefined; // results in object with `min_matching` attribute (value `undefined`)
But second, and more importantly, there is a mistake in the code as it exists on main, because min_matching should not be combined with the any operator, it should be combined with the some operator. That hopefully makes fixing this even easier, because the operator alone should be enough to decide whether to show "at least" or "any" in the UI.
|
I think it should limit the maximum number, otherwise the sentence can be like this |
|
The problem is that I can't think of a smart way of explaining the upper limit to the user, and the lack of that is what caused part of the bug report in #1930 to start with. You can't go from left to right - "Add", "at least", [number], [tags...] - you have to go - "Add", "at least", [tags...], [number]. We would have to add some extra note, or rearrange the sentence. |
|
Could you also add this logic to |
Description
Fixes two issues with SmartSearch form for matching tags when setting the number of tags to match "at least".
Note that the filter being specified does not seem to work correctly. Regardless of number of tags set to match against, it still seems to behave as the "any" variant of the filter.
Changes
Related issues
Resolves #1930