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.

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

Description

• It fixes #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.

[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)

[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.