-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add 'Matches' Search Option for Better Usability #20928
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
Add 'Matches' Search Option for Better Usability #20928
Conversation
…ed correctly for name fields
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 added some cases to the SearchTest::testAddWhere() test. I expected them to pass, but they are failing:
diff --git a/phpunit/functional/SearchTest.php b/phpunit/functional/SearchTest.php
index 87c19a6a90..782dcc5e23 100644
--- a/phpunit/functional/SearchTest.php
+++ b/phpunit/functional/SearchTest.php
@@ -2608,6 +2608,16 @@ class SearchTest extends DbTestCase
'meta' => false,
'expected' => "(`glpi_users_users_id_supervisor`.`id` = '5')",
],
+ [
+ 'link' => ' ',
+ 'nott' => 0,
+ 'itemtype' => User::class,
+ 'ID' => 99,
+ 'searchtype' => 'matches',
+ 'val' => 'glpi',
+ 'meta' => false,
+ 'expected' => "(`glpi_users_users_id_supervisor`.`name` = 'glpi')",
+ ],
[
'link' => ' AND ',
'nott' => 0,
@@ -2688,6 +2698,16 @@ class SearchTest extends DbTestCase
'meta' => false,
'expected' => "AND (INET_ATON(`glpi_ipaddresses`.`name`) < INET_ATON('192.168.1.10'))",
],
+ [
+ 'link' => ' AND ',
+ 'nott' => 0,
+ 'itemtype' => \NetworkName::class,
+ 'ID' => 13, // Search ID 13 (IPAddress name field)
+ 'searchtype' => 'matches',
+ 'val' => '192.168.1.10',
+ 'meta' => false,
+ 'expected' => "AND (`glpi_ipaddresses`.`name` = '192.168.1.10')",
+ ],
[
'link' => ' AND ',
'nott' => 0,
@@ -2708,6 +2728,16 @@ class SearchTest extends DbTestCase
'meta' => false,
'expected' => "((`glpi_computers`.`name` = '') OR `glpi_computers`.`name` IS NULL)",
],
+ [
+ 'link' => ' ',
+ 'nott' => 0,
+ 'itemtype' => Computer::class,
+ 'ID' => 1,
+ 'searchtype' => 'matches',
+ 'val' => 'My computer',
+ 'meta' => false,
+ 'expected' => "(`glpi_computers`.`name` = 'My computer')",
+ ],
];
}$ vendor/bin/phpunit phpunit/functional/SearchTest.php --filter=testAddWhere
PHPUnit 11.5.35 by Sebastian Bergmann and contributors.
Runtime: PHP 8.4.10
Configuration: /var/www/glpi/phpunit.xml.dist
.F........F..F 14 / 14 (100%)
Time: 00:00.634, Memory: 56.50 MB
There were 3 failures:
1) tests\units\SearchTest::testAddWhere with data set #1 (' ', 0, 'User', 99, 'matches', 'glpi', false, '(`glpi_users_users_id_supervi...glpi')')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'...rs_id_supervisor`.`name` = 'glpi')'
+'...rs_id_supervisor`.`name` LIKE '%glpi%')'
/var/www/glpi/phpunit/functional/SearchTest.php:2748
2) tests\units\SearchTest::testAddWhere with data set #10 (' AND ', 0, 'NetworkName', 13, 'matches', '192.168.1.10', false, 'AND `glpi_ipaddresses`.`name`....1.10'')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'AND (`glpi_ipaddresses`.`name` = '192.168.1.10')'
+'AND (`glpi_ipaddresses`.`name` LIKE '%192.168.1.10%')'
/var/www/glpi/phpunit/functional/SearchTest.php:2748
3) tests\units\SearchTest::testAddWhere with data set #13 (' ', 0, 'Computer', 1, 'matches', 'My computer', false, '`glpi_computers`.`name` = 'My...puter'')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'(`glpi_computers`.`name` = 'My computer')'
+'(`glpi_computers`.`name` LIKE '%My computer%')'
/var/www/glpi/phpunit/functional/SearchTest.php:2748
FAILURES!
Tests: 14, Assertions: 296, Failures: 3.
|
@HyunahJang can you look on failing tests please? |
|
@trasher Sure, I will check the failing tests and get back to you. |
|
@trasher It seems that the "matches" search option was processed as a "LIKE" query instead of an exact match (=) in SQL. This happened because I didn’t specify the searchtype in $search_params['criteria'], causing the system to default to "contains". I apologize for the confusion caused by my code. The code below is the corrected version, and I have also pushed the same changes to my branch. I would greatly appreciate it if you could take another look. |
|
@HyunahJang I've applied your patch, and added tests proposed by @cedric-anne; tests are still failing :/ |
|
@trasher Thank you for letting me know. Could you please confirm if the failing tests are showing the same errors as before, or if they are different this time? |
|
You should be able to see results in current PR; it's still the same error: |
|
I try to dig a bit into the feature; your changes are not in cause, it's working as expected. But there are several specific cases tahat will override (see |
|
I've added a commit that (almost) fix failing test cases; but I'm not sure it's correct (Search has always be obscure to me). Also, I've just fixed the 3 failing tests cases; it seems there are many others :/ |
|
@trasher Thank you so much for working on fixing the failing test cases. I know the Search part is quite complex, and I really appreciate your efforts. I’ll keep following the updates and check what I can. |
|
When I opened this PR, I had in mind there were just a few tests cases to fix. But unfortunately, it seems there is more work left. Initial change does really cause issue, mainly because it's override most of the time (that's why tests added by Cédric were failing). So I close this one. You can cherry-pick the work I've done here in your original branch to finalize work; I can eventually help, but I'm not going to develop the feature. |
|
Thank you for your support and for sharing your fixes. |
Replaces #20795 to target 11.0/bf branch and integrate latests tests fixes