[DataGrid] Fix filter with object as value and value getter #1665
Conversation
| @@ -115,6 +115,31 @@ describe('<DataGrid /> - Filter', () => { | |||
| }); | |||
| expect(getColumnValues()).to.deep.equal(['Nike']); | |||
| }); | |||
|
|
|||
| it('should allow object as value and work with valueGetter', () => { | |||
There was a problem hiding this comment.
Could you replicate this test case for the other operators? Only "contains" is covered. The new test cases can be dynamically generated iterating over a [operator, value, expected] array to not flood this file with duplicated code.
There was a problem hiding this comment.
Would it run fast enough? We might have many combinations to test. Would it make sense to test the comparator unitarily?
There was a problem hiding this comment.
It will add some ms to run everything, but there're not too many combinations. We have 3 string operators, so 3 test cases. With valueGetter tests it will be just more 3 test cases. It's kinda related to #1665 (comment). The idea to cover everything is to prevent us from accidentally using it inside a filter operator.
For another PR: We should add some tests for numeric and date operators. We just have for string and boolean operators.
| const rowValue = column.valueGetter ? column.valueGetter(params) : params.value; | ||
| return Boolean(rowValue) === valueAsBoolean; | ||
| return ({ value }): boolean => { | ||
| return Boolean(value) === valueAsBoolean; |
There was a problem hiding this comment.
Should we change the type of params to not allow it to be passed to valueGetter? This is just to prevent us and users from making the same mistake in the future.
There was a problem hiding this comment.
I don't think so, as the issue was within the filters and not in the user's demo.
There was a problem hiding this comment.
@dtassone but can't developers write custom operators and fall into this trap?
There was a problem hiding this comment.
What trap? I'm not sure I understand.
there is a type
export interface GridFilterOperator {
label?: string;
value: string;
getApplyFilterFn: (
filterItem: GridFilterItem,
column: any,
) => null | ((params: GridCellParams) => boolean);
InputComponent: React.JSXElementConstructor<GridFilterInputValueProps>;
InputComponentProps?: Record<string, any>;
}There was a problem hiding this comment.
{
field: "country", // field exists in data
headerName: "Country",
valueGetter: (cellParams) => cellParams.value.name,
flex: 0.5
}{
field: "country2", // field doesn't exist in data
headerName: "Country",
valueGetter: (cellParams) => cellParams.row.country.name,
flex: 0.5
}The trap is that if the valueGetter relays on value (first example), it can't be called inside the filter operator because cellParams.value will be already the resolved value (a string). If valueGetter relays on row (second example) there's no trap, but we don't know a priori what is used.
There was a problem hiding this comment.
we just have to know that cellParams consider valueGetter, which should be expected as the value wouldn't be correct otherwise.
oliviertassinari
added
bug 🐛
dtassone
force-pushed
the
filterValueGetter
branch
from
2ed468a
to
3970c48
Compare
| before(function before() { | ||
| if (isEdge) { | ||
| // We need to skip edge as it does not handle the date the same way as other browsers. | ||
| this.skip(); |
There was a problem hiding this comment.
This problem is not with Edge but with BrowserStack. All Edge versions on it are with locale set to en_GB instead of en_US. Unfortunately, the karma launcher doesn't have an option to change the locale. Related to karma-runner/karma-browserstack-launcher#187.
There was a problem hiding this comment.
The logic should be timezone invariant, we don't control the timezone of the end users.
So it sounds very much like a bug with the filtering logic.
fix #1615