-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix multi select filtering (#5358) #8452
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This PR implements comprehensive multi-select filtering functionality across the frontend and backend, including GraphQL schema updates and PostgreSQL array operations.
- Added
containsAny
operator in/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/utils/compute-where-condition-parts.ts
for PostgreSQL array overlap checks - Modified array handling in
/packages/twenty-front/src/modules/object-record/graphql/types/RecordGqlOperationFilter.ts
to supportcontainsAny
filtering - Created
/packages/twenty-front/src/modules/object-record/object-filter-dropdown/constants/SelectFilterTypes.ts
to unify SELECT and MULTI_SELECT filter handling - Updated enum filter types in
/packages/twenty-server/src/engine/api/graphql/workspace-schema-builder/factories/input-type.factory.ts
to supportcontainsAny
operation - Changed multi-select field array handling in schema generation via
/packages/twenty-server/src/engine/api/graphql/workspace-schema-builder/utils/generate-fields.utils.ts
10 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile
...y-front/src/modules/object-record/record-filter/utils/computeViewRecordGqlOperationFilter.ts
Outdated
Show resolved
Hide resolved
...y-front/src/modules/object-record/record-filter/utils/computeViewRecordGqlOperationFilter.ts
Outdated
Show resolved
Hide resolved
...y-front/src/modules/object-record/record-filter/utils/computeViewRecordGqlOperationFilter.ts
Outdated
Show resolved
Hide resolved
...twenty-server/src/engine/api/graphql/workspace-schema-builder/utils/generate-fields.utils.ts
Outdated
Show resolved
Hide resolved
...ty-server/src/engine/api/graphql/graphql-query-runner/utils/compute-where-condition-parts.ts
Show resolved
Hide resolved
@@ -63,6 +63,9 @@ export const isMatchingStringFilter = ({ | |||
case stringFilter.startsWith !== undefined: { | |||
return value.startsWith(stringFilter.startsWith); | |||
} | |||
case stringFilter.containsAny !== undefined: { | |||
return stringFilter.containsAny.some((item) => value.includes(item)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not able to reproduce this. I could add a nullish check like so
return stringFilter.containsAny.some((item) => value?.includes(item));
but would be better to know why null is passed to this function in the first place. Quick screenshare call?
Could you please also take care of the Array field? I just noticed it's similar because I saw not_contains. Also could you please rename it to notContains? Since there was no filtered view I don't think there's any risk to rolling out a rename for everyone (unless you see one?). Also I saw the "not contains" filters out null which we probably don't really want. On the UI front, maybe display "Contains" / "Does not contain" instead of "Is" / "Is not"? |
…ias/twenty into fix/multi-select-filtering
packages/twenty-front/src/modules/object-record/graphql/types/RecordGqlOperationFilter.ts
Show resolved
Hide resolved
...ty-server/src/engine/api/graphql/graphql-query-runner/utils/compute-where-condition-parts.ts
Show resolved
Hide resolved
@@ -19,6 +19,11 @@ export const computeWhereConditionParts = ( | |||
const uuid = Math.random().toString(36).slice(2, 7); | |||
|
|||
switch (operator) { | |||
case 'isEmptyArray': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value of an empty multi-select can either be NULL
or an empty array.
Instead of adding isEmptyArray
operator, we could change the eq
operator, but as far as I understand GraphQL does not support union types of inputs.
In other words I think it's not possible to set the value of an eq
filter to either be an empty array or an enum like this:
{
eq: []
}
# or
{
eq: 'SOME_ENUM_VALUE'
}
To get around that, we could add a subfield under eq like this:
{
eq: { value: 'SOME_ENUM_VALUE' }
}
# or
{
eq: { emptyList: true }
}
But I think it's cleaner to just add the isEmptyArray
operator and keep eq as is.
@FelixMalfait What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess all variations of existing syntax like eq: "'{}'"
etc didn't work?
packages/twenty-front/src/modules/object-record/graphql/types/RecordGqlOperationFilter.ts
Outdated
Show resolved
Hide resolved
...twenty-front/src/modules/object-record/object-filter-dropdown/constants/SelectFilterTypes.ts
Outdated
Show resolved
Hide resolved
...y-front/src/modules/object-record/record-filter/utils/computeViewRecordGqlOperationFilter.ts
Outdated
Show resolved
Hide resolved
...y-front/src/modules/object-record/record-filter/utils/computeViewRecordGqlOperationFilter.ts
Outdated
Show resolved
Hide resolved
filter.definition, | ||
); | ||
} | ||
const stringifiedSelectValues = filter.value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we manipulated stringified values here? I feel they should be parsed as as regular values way earlier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved parsing the string to resolveFilterValue
function, it's a bit cleaner now.
Could be good to first do a bigger refactoring of filtering, especially remove the Filter
type and use ViewFilter
everywhere instead, and after that move parsing earlier. What do you think?
...y-front/src/modules/object-record/record-filter/utils/computeViewRecordGqlOperationFilter.ts
Outdated
Show resolved
Hide resolved
...y-front/src/modules/object-record/record-filter/utils/computeViewRecordGqlOperationFilter.ts
Outdated
Show resolved
Hide resolved
emptyRecordFilter = { | ||
or: [ | ||
{ | ||
[correspondingField.name]: { is: 'NULL' } as ArrayFilter, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fast folllow, we should have ArrayFilter, MultiSelectFilter, etc.
case FieldMetadataType.Text: { | ||
return isMatchingStringFilter({ | ||
stringFilter: filterValue as StringFilter, | ||
value: record[filterKey], | ||
}); | ||
} | ||
case FieldMetadataType.Select: | ||
case FieldMetadataType.MultiSelect: | ||
case FieldMetadataType.Array: { | ||
return isMatchingArrayFilter({ | ||
arrayFilter: filterValue as ArrayFilter, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks wrong to me (see comment above) why would Select checking isMatchingArrayFilter. MultiSelect is already borderline to me (as constainsILike does not make sense), Select seems wrong
Allow filtering by multi-select fields.