feat(filters): add new IN_COLLECTION operator to allow searching cell value as Array #282
Conversation
Codecov Report
@@ Coverage Diff @@
## master #282 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 209 209
Lines 12433 12482 +49
Branches 4252 4277 +25
=========================================
+ Hits 12433 12482 +49
Continue to review full report at Codecov.
|
|
hmm I'm not sure I like this approach, it seems the goal of this is simply to remove white spaces, why don't we fix the source instead? Because if I look at your code change, I'm quite sure this will have a perf impact and I'm not willing to add that for that reason. Where do you see that it gets split by white space? Can't we just add a trim somewhere? I find you're adding lot of complexity for something that should be simple and will hit perf. |
|
Yes, I understand your concern about the performance hit. I just assumed you'd prefer me reusing existing functions. 😃 From my understanding the problem occurs when using a multiselect filter on an column that contains an array of strings, each string can contain white spaces. The array of selected options seem to get whitespace separated. When these values get send to the testFilterCondition function the 'inContains' operator splits the search string coming from the multiselect filter on white spaces. This is where our issue lies. Our search string, e.g. Group ABC, gets parted in Group and ABC. This makes that it will never get a hit on the value. Because the testFilterCondition function seems to search for Group ABC in the strings Group and ABC. |
|
Are you using the The What are you using that with? I mean is that a local grid (JSON dataset) or is it with a Backend Service (OData/GraphQL)? It's not provided in your issue, and perhaps show a print screen of the issue. It would be useful to know how to reproduce so that we could maybe add that to one of the demo to test it out.
|
|
We're using SlickGrid with json (local grid) and we're using the I'll try to add screenshots later if you want. 😄 |
|
hmm it seems that the reason you are using the case 'IN_CONTAINS':
if (value2 && Array.isArray(value2) && typeof value1 === 'string') {
return value2.some(item => value1.split(/[\s,]+/).includes(item));
}
return false;
case 'IN_COLLECTION':
if (value1 && value2 && Array.isArray(value1) && Array.isArray(value2)) {
return value2.some(item => value1.includes(item));
}
return false;
I'm not sure what you mean here? The code in that PR for that switch case is what is in master right now and it was shipped in last version |
|
An new What I meant is that the commit you're referring to in PR #262 doesn't split on whitespaces just comma but that got added in PR #267 😄 |
|
Oh I didn't know it was using a string every time, do you mean this line (on the last part)? I think I made it a string mainly to convert numbers to string, we probably need to find a better way so that it works with an array of string. Maybe just casting as a string when it's a number should be sufficient const cellValue = (options.cellValue === undefined || options.cellValue === null) ? '' : typeof options.cellValue === 'number' ? `${options.cellValue}` : options.cellValue;
Thanks for the find and so I would rather keep that then because we use the Composite Editor extensively in our project and I don't want to create new bugs by changing the regex. So I think the Would you like to contribute and test it out with your code? If so, you'll have to start adding both Operators in the following 2 files operatorType.enum.ts and operatorString.type.ts and then modify the other files which you know about. Also, if you don't know how to push this code change to your project and test it out, you can simply run the build in this project and copy the build package dist and replace it in your node_modules, I can provide more info if you need, and that is basically the steps I now do in testing Aurelia-Slickgrid before making an official version (or you can also npm links but I never tried myself, I should probably give it a try though) |
|
Yes, that's the code that makes it a combined string. I'm just wondering how other types get affected by the change in this string conversation you're proposing. Thank you for pointing me in the right direction, I'll try the proposed changes our project and update the PR if everything works as expected. 👍 |
|
ah yeah I forgot about the boolean values, it's probably better to include them as well because the Maybe we can simply go the inverse and only check for arrays const cellValue = (options.cellValue === undefined || options.cellValue === null)
? ''
: Array.isArray(options.cellValue) ? options.cellValue : `${options.cellValue}`;You might need to put it all in 1 line though because I think ESLint will complain if it's not It's a good discussion, I'm always happy to have contributions 😉 |
|
I updated the PR now, and did the following changes:
|
|
Wow that is pretty good, I got nothing to say on the PR, it's perfect 😄 Were you done with the PR, I would be ready to merge as is. EDIT Actually the Example 7 Cypress E2E tests keeps failing on the last column "Prerequisites" which is a multiple-select dropdown. I'm not sure what makes it fail though, it could be the |
|
Yes, I saw that 😢 |
ghiscoding
changed the title
|
I'm not sure why Cypress dashboard doesn't me those tests but if I look at the GitHub Actions, the failing test is the following (which is the last test on 1) should open the "Prerequisites" Filter then choose "Task 3", "Task 4" and "Task 8" from the list and expect to see 2 rows of data in the grid
30 passing (33s)
1 failingThe Cypress test code has the following steps, I suggest to try the steps manually on that Example 7 slickgrid-universal/test/cypress/integration/example07.spec.js Lines 587 to 611 in 3174c86 |
|
Yes, I'm experiencing a similar issue, I cannot seem to run the cypress tests locally. |
|
hmm I sent you the code for the Cypress tests, I thought it was readable but the title of the test should tell you what to do in a general way it('should open the "Prerequisites" Filter then choose "Task 3", "Task 4" and "Task 8" from the list and expect to see 2 rows of data in the grid')Note that a few months back, we can no longer open Cypress directly from VSCode, it throws some kind of memory heap size error of some kind. It was reported on VSCode but they never really addressed it, if that is what you're doing then simply open a separate terminal and run cypress from that terminal (outside of VSCode) and that is how I got it to work. |
ArnaudVanHerck
deleted the
fix/add-kebabcase-to-collectionsearchfiltercondition
branch
ArnaudVanHerck
restored the
fix/add-kebabcase-to-collectionsearchfiltercondition
branch
|
@ArnaudVanHerck I saw you closed/reopened the PR (maybe to trigger another test) and it still fails on the exact same Cypress test, not sure what it is yet but I'll definitely take a look at it a bit later today. At least what we know so far is that there's really something on that test and it's not a flaky test because it's always the same one that fails.... to be continued ;) |
|
@ghiscoding I tried to rename the branch in order for it to reflect the PR better but apparently that closed the PR 😋 |
| @@ -7,7 +7,7 @@ import { testFilterCondition } from './filterUtilities'; | |||
| */ | |||
| export const executeCollectionSearchFilterCondition: FilterCondition = (options: FilterConditionOption) => { | |||
| // multiple-select will always return text, so we should make our cell values text as well | |||
| const cellValue = (options.cellValue === undefined || options.cellValue === null) ? '' : `${options.cellValue}`; | |||
| const cellValue = (options.cellValue === undefined || options.cellValue === null) ? '' : ((options.operator === 'IN_COLLECTION' || options.operator === 'NOT_IN_COLLECTION') && Array.isArray(options.cellValue)) ? options.cellValue.map(value => `${value}`) : `${options.cellValue}`; | |||
There was a problem hiding this comment.
I find this line is starting to be way too lengthy, could we split this into multiple lines to increase readability. Actually, I rewrote the entire function and the unit tests are still good, could you please use the code below, I find it more readable. Thanks
export const executeCollectionSearchFilterCondition: FilterCondition = (options: FilterConditionOption) => {
// multiple-select will always return text, so we should make our cell values text as well
const filterOperator = options.operator;
let cellValue: string | string[];
if (Array.isArray(options.cellValue) && (filterOperator === 'IN_COLLECTION' || filterOperator === 'NOT_IN_COLLECTION')) {
cellValue = options.cellValue.map(value => `${value}`);
} else {
cellValue = (options.cellValue === undefined || options.cellValue === null) ? '' : `${options.cellValue}`;
}
return testFilterCondition(filterOperator || 'IN', cellValue, options.searchTerms || []);
};There was a problem hiding this comment.
Haha, yes that line got quite long now. I'm happy to change it, I'll update it soon 👍
|
Alright, so I ran all the unit tests and I also wanted to test it out in Aurelia-Slickgrid before approving this PR and all the Aurelia-Slickgrid Cypress tests are passing (all 400 tests), so I'll be happy to merge after you do the code change I asked in previous comment, the rest is all great, nice PR. Thanks |
|
I added a nullcheck to the array given that the cellValue got typed as export const executeCollectionSearchFilterCondition: FilterCondition = (options: FilterConditionOption) => {
// multiple-select will always return text, so we should make our cell values text as well
const filterOperator = options.operator;
let cellValue: string | string[];
if (Array.isArray(options.cellValue) && (filterOperator === 'IN_COLLECTION' || filterOperator === 'NOT_IN_COLLECTION')) {
cellValue = (!!options.cellValue.length ? options.cellValue.map(value => `${value}`) : []);
} else {
cellValue = (options.cellValue === undefined || options.cellValue === null) ? '' : `${options.cellValue}`;
}
return testFilterCondition(filterOperator || 'IN', cellValue, options.searchTerms || []);
};What do you think? |
|
sure that'd be ok |
|
Thanks for the great contribution, good PR, good feedback, all good 😄 |
|
Thank you for the feedback and your patience. And thank you for you hard work on these packages 🥇 |
ArnaudVanHerck
deleted the
fix/add-kebabcase-to-collectionsearchfiltercondition
branch
|
@ArnaudVanHerck Cheers ⭐ and thanks again for the contribution |
We are having issues with our implementation of SlickGrid (through Aurelia-SlickGrid) where the search terms sometimes can contain whitespaces. When the testFilterCondition function runs with the operator 'inContains' the terms get split on whitespaces. This issue got solved for us when using camelCasing or kebabCasing because this removes whitespaces from the search terms and cellValue before sending them to the testFilterCondition function. Because the 'toKebabCase' function needs a string and we know that multiple-select.js also provides a string, a toString() was added to the searchTerm without the need of worrying about possible type conficts (as far as my understanding of the code).