Business rule for assets : action to add a group in charge now supports multiple groups. (11/bf)

[SebSept]

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 Can't add multiple groups in charge of an asset using Business Rules for Assets #20837 : Business rule for assets : action to add a group in charge now supports multiple groups.

[cconard96]

Shouldn't the action for "Group" be changed in addition to "Group in charge"?

[SebSept]

Shouldn't the action for "Group" be changed in addition to "Group in charge"?

good question.
groups_id_tech is used to associate asset to group with the type GROUP_TYPE_TECH
and groups_id association type is GROUP_TYPE_NORMAL (which have no clear meaning to me, is it ownership relation ?).

And, you are right, we can also add the possibility to add multiple groups for groups_id since it's possible in the UI.
I'll add it very soon. I guess it's possible to add it in this PR.

[cconard96]

and groups_id association type is GROUP_TYPE_NORMAL (which have no clear meaning to me, is it ownership relation ?).

Yes. It is just the default/regular group association type.

And, you are right, we can also add the possibility to add multiple groups for groups_id since it's possible in the UI. I'll add it very soon. I guess it's possible to add it in this PR.

The only issue I forsee is confusion around the defaultfromuser and firstgroupfromuser actions in the context of multiple groups.

[SebSept]

permitseveral is applied only to append action (for both actions), so no problem with defaultfromuser and firstgroupfromuser.

[cedric-anne]

The append action seems to remove the existing groups, instead of appending new groups to existing groups.

[SebSept]

ok, I'll check by adding a test.

[SebSept]

The append action seems to remove the existing groups, instead of appending new groups to existing groups.

This was the case. I added a test then fix the code.

[SebSept]

(sorry for the merge, I resolved the conflict in GitHub)

[cedric-anne]

There is still an issue. Existing values are correctly preserved, but if I tried to manually add a group in the form, it is dropped when the "append" rule is executed.

[AdrienClairembault]

E2E failure is related, I reproduce it with make cypress c="--spec tests/cypress/e2e/form/destination_config_fields/requester.cy.js"

The 500 error from the server logs:

[2025-11-14 08:49:37] glpi.CRITICAL:   *** Uncaught PHP Exception TypeError: "array_merge(): Argument #2 must be of type array, int given" at CommonDBTM.php line 5776
  Backtrace :
  ./src/CommonDBTM.php:5776                          
  ./src/CommonDBTM.php:5776                          array_merge()
  ./src/CommonDBTM.php:1353                          CommonDBTM->assetBusinessRules()
  ./src/Glpi/Api/API.php:1894                        CommonDBTM->add()
  ./src/Glpi/Api/APIRest.php:344                     Glpi\Api\API->createItems()
  ./src/Glpi/Controller/ApiRestController.php:59     Glpi\Api\APIRest->call()
  ./vendor/symfony/http-kernel/HttpKernel.php:101    Glpi\Controller\ApiRestController->{closure:Glpi\Controller\ApiRestController::__invoke():57}()
  ...ymfony/http-foundation/StreamedResponse.php:106 Symfony\Component\HttpKernel\HttpKernel::{closure:Symfony\Component\HttpKernel\HttpKernel::handle():98}()
  ./vendor/symfony/http-foundation/Response.php:423  Symfony\Component\HttpFoundation\StreamedResponse->sendContent()
  ./src/Glpi/Kernel/Kernel.php:295                   Symfony\Component\HttpFoundation\Response->send()
  ./public/index.php:72                              Glpi\Kernel\Kernel->sendResponse()

It seems to be this API payload that trigger the errors:

I think you missed that groups_id can be an int instead of an array (as this API call work properly and set the correct groups on the bugfixes branch).

[SebSept]

E2E failure is related, I reproduce it with make cypress c="--spec tests/cypress/e2e/form/destination_config_fields/requester.cy.js"

The 500 error from the server logs:

[2025-11-14 08:49:37] glpi.CRITICAL:   *** Uncaught PHP Exception TypeError: "array_merge(): Argument #2 must be of type array, int given" at CommonDBTM.php line 5776
  Backtrace :
  ./src/CommonDBTM.php:5776                          
  ./src/CommonDBTM.php:5776                          array_merge()
  ./src/CommonDBTM.php:1353                          CommonDBTM->assetBusinessRules()
  ./src/Glpi/Api/API.php:1894                        CommonDBTM->add()
  ./src/Glpi/Api/APIRest.php:344                     Glpi\Api\API->createItems()
  ./src/Glpi/Controller/ApiRestController.php:59     Glpi\Api\APIRest->call()
  ./vendor/symfony/http-kernel/HttpKernel.php:101    Glpi\Controller\ApiRestController->{closure:Glpi\Controller\ApiRestController::__invoke():57}()
  ...ymfony/http-foundation/StreamedResponse.php:106 Symfony\Component\HttpKernel\HttpKernel::{closure:Symfony\Component\HttpKernel\HttpKernel::handle():98}()
  ./vendor/symfony/http-foundation/Response.php:423  Symfony\Component\HttpFoundation\StreamedResponse->sendContent()
  ./src/Glpi/Kernel/Kernel.php:295                   Symfony\Component\HttpFoundation\Response->send()
  ./public/index.php:72                              Glpi\Kernel\Kernel->sendResponse()

It seems to be this API payload that trigger the errors:

I think you missed that groups_id can be an int instead of an array (as this API call work properly and set the correct groups on the bugfixes branch).

it means a phpunit test is missing, I suppose. I'll add it if possible.

[AdrienClairembault]

Given the number of unexpected issues that have been found (and the fact that this is a new feature), I think it would be safer to target the main branch instead.

[SebSept]

Given the number of unexpected issues that have been found (and the fact that this is a new feature), I think it would be safer to target the main branch instead.

A (cypress?) test is missing probably.

I agree, it's a feature more than a bugfix.

[cedric-anne]

If we are able to move the required changes in the rules class, without having to change too much the API and the CommonDBTM code, it could be OK to include this change in GLPI 11.

[cedric-anne]

I think it must be fixed somewhere else. Indeed, there are probably many places where the if ($item->can($id, CREATE, $input)) { $item->add($input); } chaining is done. It should not trigger issues.

[cedric-anne]

If possible , the related logic should be located in the rules classes. However, I do not know where the logic used for ticket actors is located.