chore: Added phpunit for the filtering function in Row2Mapper::findAll - #2019
Conversation
26aec92 to
46d29ed
Compare
| /* currently not work (frontend filter settings not support multiple values) | ||
| 'Skills is equal to "Management", "Communication"' => [ | ||
| [[['columnId' => 'skills', 'operator' => 'is-equal', 'value' => ['@selection-id-9', '@selection-id-10']]], | ||
| ['Bob'], // Person with skills "Management" (id: 9) and "Communication" (id: 10) | ||
| 'Filter by skills exactly equal to "Management" and "Communication" - should include 1 rows, exclude 4' | ||
| ],*/ | ||
| /* currently work only frontend | ||
| 'Skills contains "Pyt"' => [ | ||
| [[['columnId' => 'skills', 'operator' => 'contains', 'value' => 'Pyt']]], | ||
| ['Alice', 'Charlie'], // People with skills "Python" (id: 3) | ||
| 'Filter by skills containing "Python" - should include 2 rows, exclude 3' | ||
| ],*/ |
There was a problem hiding this comment.
If these don't work, no need keeping them? Might as well remove?
There was a problem hiding this comment.
I think it makes sense to keep them.
Because we need to synchronize filters with the front end in order to paginate on the server in the future.
There was a problem hiding this comment.
I prefer removing them in the meantime. Who knows when in the future we'll do anything, and it could get messy if we start commenting things like this
| /** | ||
| * Converts selection values to IDs based on selection_options | ||
| */ | ||
| protected function convertSelectionValuesToIds(int $columnId, $value) { |
There was a problem hiding this comment.
I wonder if this still works with #1887. Gonna merge that shortly
There was a problem hiding this comment.
I did a rebase, it works
| ], | ||
| 'Experience years is greater than 5' => [ | ||
| [[['columnId' => 'experience_years', 'operator' => 'is-greater-than', 'value' => 5]]], | ||
| ['Eve'], // People with experience 8, 7 years |
There was a problem hiding this comment.
Should be this?
| ['Eve'], // People with experience 8, 7 years | |
| ['Eve'], // Person with experience 7 years |
| ['Alice', 'Eve'], // People with experience 5, 8, 7 years | ||
| 'Filter by experience years greater than or equal to 5 - should include 3 rows, exclude 2' |
There was a problem hiding this comment.
Are we including 3 or 2? We have just Alice and Eve?
| 'score' => 92.0, | ||
| 'status' => 'Inactive', | ||
| 'skills' => ['Management', 'Communication'], | ||
| 'is_available' => '"false"', |
There was a problem hiding this comment.
Is it intentional not to have experience_years for Bob?
| ['Alice', 'Bob', 'Charlie', 'Diana'], // People with experience 2, 3, 5 years | ||
| 'Filter by experience years lower than or equal to 5 - should include 3 rows, exclude 2' |
| ['Bob'], // No person with empty experience years | ||
| 'Filter by experience years is empty - should include 1 rows, exclude 4' |
There was a problem hiding this comment.
Oh, so it's intentional for Bob not to have experience? So the comment saying No person is not correct?
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
46d29ed to
62f2d6d
Compare
|
@enjeck I did a git rebase and made changes according to the comments. |
|
Is it possible that |
|
@enjeck @blizzz I don't understand what to do with this PR. I can see now that another commit ( e4f0753 ) has been made that overlaps this PR by more than 50%. It's not as simple as rebasing these commits, as they are incompatible. And this PR was only the first part. |
62f2d6d to
cf884d9
Compare
|
@AIlkiv So I rebased this branch onto master and kept only what is still unique here which are your selection support in DatabaseTestCase, and the selection filter cases, which now live in the existing Row2MapperFilterTest data provider instead of a separate block. I also had to load subtype in setupRealColumnMapper, otherwise multi-selection filtering is not exercised. Commented-out cases are removed for now. |
Signed-off-by: ailkiv <a.ilkiv.ye@gmail.com>
cf884d9 to
5df13ba
Compare
Signed-off-by: Enjeck C. <patrathewhiz@gmail.com>
3 participants
These filters are mainly used for View.
In this PR, I covered the following column types with tests:
In the future, I will try to add tests for other column types and check AND/OR operators.