Skip to content

Conversation

HyunahJang
Copy link

@HyunahJang HyunahJang commented Aug 27, 2025

Checklist before requesting a review

Please delete options that are not relevant.

  • I have read the CONTRIBUTING document.
  • I have performed a self-review of my code.
  • I have added tests that prove my fix is effective or that my feature works.
  • This change requires a documentation update.

Description

  • It fixes nothing, this upddate just adds a new function
    GLPI 11.0.0-rc2 - Add 'Matches' Search Option for Better Usability #20713
  • Here is a brief description of what this PR does
    A new matches search operator has been developed and proposed to enable faster and more flexible ticket searches using simple text.
    In details, this contribution adds a new 'matches' search condition to complement the existing 'is' operator.
    While the current UI relies on dropdowns for selecting values, this new option allows users to enter text directly when the exact value is known. It improves search speed and simplifies the process by avoiding dropdown loading.
    This enhancement offers a more efficient and user-friendly way to perform targeted searches.

Screenshots (if appropriate):

スクリーンショット 2025-07-25 105259

@orthagh
Copy link
Contributor

orthagh commented Aug 27, 2025

Hello.

Thank your for the feature.
While agreeing with the need, I'm not in favor to make it the default option. For the general usage of GLPI, most of our users will prefers the "contains" option.
But it may be in contradiction of the speed improvement you are seeking.

@HyunahJang
Copy link
Author

Hello.

Thank your for the feature. While agreeing with the need, I'm not in favor to make it the default option. For the general usage of GLPI, most of our users will prefers the "contains" option. But it may be in contradiction of the speed improvement you are seeking.

Thank you for your feedback regarding the "contains" operator.
To reflect this, I will update the PR so that "matches" is in the middle of the list. This way it stays available as a useful complement without disrupting the usual workflow.

@HyunahJang
Copy link
Author

I noticed the need unit tests label was added.
Could you please clarify if unit tests are required for this change?

@trasher
Copy link
Contributor

trasher commented Sep 1, 2025

Yes, unit tests are required for every new feature added

@HyunahJang
Copy link
Author

Thank you for the explanation.
Just to confirm, my understanding is that the checks run by GitHub Actions are the unit tests, correct?
I see that 3 out of 8 tests have failed — should I work on fixing those failing tests?

image

@trasher
Copy link
Contributor

trasher commented Sep 1, 2025

Currently, this is the Coding Standards checks that are failing. Running vendor/bin/php-cs-fixer fix should fix them for you.

@trasher
Copy link
Contributor

trasher commented Sep 1, 2025

For tests, Search clas is already tested in the phpunit/functional/SearchTest.php file. You have to add a test case that covers your change in that file (testing Search class is not the more simple one, tell us if you have problems).

@HyunahJang
Copy link
Author

HyunahJang commented Sep 5, 2025

Thank you very much for your explanation.
Before writing the test code, I would like to confirm something regarding the modification of other files.

The test "GLPI CI / E2E and web tests using late PHP and MariaDB versions (pull_request)" failed on my PR, so I am currently working on fixing it.
In this case, would it be acceptable for me to also modify the APIRestTest.php and Computer_SoftwareVersion.json files?

I appreciate your guidance.

@HyunahJang
Copy link
Author

I wrote a test case and tried to run it using the following command: vendor/bin/phpunit phpunit/functional/SearchTest.php
However, it displayed the message: run: php bin/console database:install --env=testing ...
When I executed that command, I encountered an error. Could you please let me know if the database name should be something other than 'glpi-db'? Or is it possible that I am running the test in the wrong way?
I have already pushed the modified test case code in the SearchTest.php file, so I would greatly appreciate it if you could review it.
Thank you very much.

スクリーンショット 2025-09-05 133817

@trasher
Copy link
Contributor

trasher commented Sep 5, 2025

The test "GLPI CI / E2E and web tests using late PHP and MariaDB versions (pull_request)" failed on my PR, so I am currently working on fixing it. In this case, would it be acceptable for me to also modify the APIRestTest.php and Computer_SoftwareVersion.json files?

You can modify every file you need to add yout test case.

Bu, be careful about "E2E" tests failures... We have issues with that test suite, it often break for completely unrelated reasons.
Make sure fail is related to your changes before spending time on it ;)

@trasher
Copy link
Contributor

trasher commented Sep 5, 2025

I wrote a test case and tried to run it using the following command: vendor/bin/phpunit phpunit/functional/SearchTest.php However, it displayed the message: run: php bin/console database:install --env=testing ... When I executed that command, I encountered an error. Could you please let me know if the database name should be something other than 'glpi-db'? Or is it possible that I am running the test in the wrong way? I have already pushed the modified test case code in the SearchTest.php file, so I would greatly appreciate it if you could review it. Thank you very much.

Since GLPI v11; it is required to specify the environment used to run tests. So you have to add the --env=testing option when you run the tests, but also on database install.

A readme file to explain how to setup and run tests is missing in the phpunit directory, I've opened #20913 to fix that.

Your new test case does not seems incorrect after a very quick review.
Tests workflow is running (we have to run it manually because it's your first contribution) - we'll see if that works :)

@HyunahJang
Copy link
Author

You can modify every file you need to add yout test case.

Bu, be careful about "E2E" tests failures... We have issues with that test suite, it often break for completely unrelated reasons. Make sure fail is related to your changes before spending time on it ;)

Thank you very much for your explanation. I will review my code once again, and if any modifications are needed, I will make the changes accordingly.

Since GLPI v11; it is required to specify the environment used to run tests. So you have to add the --env=testing option when you run the tests, but also on database install.

A readme file to explain how to setup and run tests is missing in the phpunit directory, I've opened #20913 to fix that.

Your new test case does not seems incorrect after a very quick review. Tests workflow is running (we have to run it manually because it's your first contribution) - we'll see if that works :)

Thank you for the clarification. I will retry with the --env=testing option. Also, thanks for opening the documentation fix (#20913). I really appreciate your help on my first contribution.

@trasher
Copy link
Contributor

trasher commented Sep 5, 2025

In the "E2E" workflow, the "web tests" part is failing because of the add of the new option; that should not be hard to fix.

Your new added test case seems to pass; that seems OK to me.

@trasher
Copy link
Contributor

trasher commented Sep 8, 2025

I've tried to fix remaning issues; but I do not have rights to push on your repository. Patch is attached.

0001-Remaining-test-fix.patch

@trasher trasher requested a review from orthagh September 8, 2025 12:51
orthagh
orthagh previously approved these changes Sep 8, 2025
trasher
trasher previously approved these changes Sep 8, 2025
@trasher
Copy link
Contributor

trasher commented Sep 8, 2025

I've opened another PR based on your branch + my test fix to target GLPI v11.

Thanks for your contribution.

@HyunahJang
Copy link
Author

I sincerely apologize for my late reply. I also tried running the tests on my side, but they kept failing and I wasn’t sure how to proceed, so I couldn’t get back to you sooner. Thank you so much for opening another PR and helping me get my first contribution finalized. I’m really grateful, and I’m very happy that my work could be part of GLPI.

@cedric-anne cedric-anne dismissed stale reviews from orthagh and trasher September 10, 2025 12:15

tests are failing

@cedric-anne cedric-anne added this to the 11.1.0 milestone Sep 17, 2025
@HyunahJang
Copy link
Author

hi, @trasher
it’s been a little while since my last update, but I’ve been looking into this issue.
I have investigated the code so far, and I believe the issue comes from the test cases.
So, I’ve made the changes and pushed them to my branch. I’d appreciate it if you could review them again.

@knibm knibm force-pushed the feature/match_function branch from cffbb48 to 9b672d5 Compare October 1, 2025 13:05
@knibm
Copy link
Contributor

knibm commented Oct 1, 2025

This feature branch was rebased to the main branch also with squashing the multiple commits into a single commit, since the changes on main branch caused conflict.

@knibm knibm force-pushed the feature/match_function branch from 9b672d5 to de53b03 Compare October 7, 2025 12:23
@knibm knibm force-pushed the feature/match_function branch from de53b03 to ee12a08 Compare October 7, 2025 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants