From 8edf7512f89ccffdcdf5e97a0875a857211a0db7 Mon Sep 17 00:00:00 2001 From: Herafia Date: Thu, 22 Oct 2026 10:26:53 +0200 Subject: [PATCH 1/2] fix/fields massive action multiple tab blocks --- CHANGELOG.md | 1 + inc/container.class.php | 49 +++ ...MassiveActionMultipleTabContainersTest.php | 289 ++++++++++++++++++ 3 files changed, 339 insertions(+) create mode 100644 tests/Units/MassiveActionMultipleTabContainersTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index f443e1aa..8295c701 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Fix mandatory fields of a block hidden by its display conditions blocking the save of an item - Fix fields values updates not applying the expected checks in some cases - Fix `Yes/No` field values returned as strings instead of integers in search results +- Fix massive update of fields of a tab block not saved when the item has several tab blocks ## [1.24.6] - 2026-10-06 diff --git a/inc/container.class.php b/inc/container.class.php index 5f1f615c..2683a713 100644 --- a/inc/container.class.php +++ b/inc/container.class.php @@ -2005,6 +2005,51 @@ public static function findContainer($itemtype, $type = 'tab', $subtype = '') return $id; } + /** + * Find the active tab containers of the item's type having at least one field submitted in the item input + * + * @param CommonDBTM $item Item instance + * + * @return int[] + */ + private static function findSubmittedTabContainers(CommonDBTM $item): array + { + $container = new self(); + $containers = $container->find([ + 'is_active' => 1, + 'type' => ['!=', 'dom'], + ]); + + $c_ids = []; + foreach ($containers as $data) { + if ( + !in_array($item::class, PluginFieldsToolbox::decodeJSONItemtypes($data['itemtypes'])) + || !Session::haveAccessToEntity($data['entities_id'], $data['is_recursive']) + ) { + continue; + } + + $fields = (new PluginFieldsField())->find([ + 'plugin_fields_containers_id' => $data['id'], + 'is_active' => 1, + 'type' => ['!=', 'header'], + ]); + foreach ($fields as $field) { + $input_keys = [ + $field['name'], + sprintf('plugin_fields_%sdropdowns_id', $field['name']), + sprintf('items_id_%s', $field['name']), + ]; + if (array_intersect($input_keys, array_keys($item->input)) !== []) { + $c_ids[] = (int) $data['id']; + break; + } + } + } + + return $c_ids; + } + public static function findAllContainers($itemtype) { $condition = ['is_active' => 1]; @@ -2123,6 +2168,10 @@ public static function preItem(CommonDBTM $item) [self::findContainer($item::class, 'dom'), self::findContainer($item::class, 'tab')], static fn($id) => $id !== false, ); + + // other tab containers whose fields are submitted (ex: massive action on a GLPI item + // dropdown field for which the core never displays the plugin field nor its c_id input) + $c_ids = array_values(array_unique(array_merge($c_ids, self::findSubmittedTabContainers($item)))); } if ($c_ids === []) { diff --git a/tests/Units/MassiveActionMultipleTabContainersTest.php b/tests/Units/MassiveActionMultipleTabContainersTest.php new file mode 100644 index 00000000..ca52ad49 --- /dev/null +++ b/tests/Units/MassiveActionMultipleTabContainersTest.php @@ -0,0 +1,289 @@ +. + * ------------------------------------------------------------------------- + * @copyright Copyright (C) 2013-2023 by Fields plugin team. + * @license GPLv2 https://www.gnu.org/licenses/gpl-2.0.html + * @link https://github.com/pluginsGLPI/fields + * ------------------------------------------------------------------------- + */ + +declare(strict_types=1); + +namespace GlpiPlugin\Field\Tests\Units; + +use Override; +use Computer; +use Entity; +use Glpi\Search\SearchOption; +use Glpi\Tests\DbTestCase; +use Glpi\Tests\GLPITestCase; +use GlpiPlugin\Field\Tests\FieldTestTrait; +use MassiveAction; +use PHPUnit\Framework\Attributes\DataProvider; +use PluginFieldsContainer; +use PluginFieldsField; +use PluginFieldsProfile; +use ReflectionClass; +use Ticket; + +require_once __DIR__ . '/../FieldTestCase.php'; + +/** + * Massive actions on fields of an item having several tab containers. + * + * The core displays its own input (without the plugin `c_id` hidden input) for GLPI item + * dropdown fields, and the `c_id` input is also lost when a long massive action is reloaded, + * so the plugin has to find the targeted container from the submitted fields. + */ +final class MassiveActionMultipleTabContainersTest extends DbTestCase +{ + use FieldTestTrait; + + /** @var array */ + private array $containers = []; + + /** @var array */ + private array $fields = []; + + #[Override] + public function setUp(): void + { + GLPITestCase::setUp(); + $this->login(); + } + + #[Override] + public function tearDown(): void + { + unset($_REQUEST['massiveaction'], $_REQUEST['c_id'], $_POST); + $this->tearDownFieldTest(); + GLPITestCase::tearDown(); + } + + /** + * Same configuration as the one reported: two tab containers and a "dom" one on computers. + */ + private function createContainers(): void + { + $common = [ + 'itemtypes' => [Computer::class], + 'is_active' => 1, + 'entities_id' => 0, + 'is_recursive' => 1, + ]; + $this->containers = [ + 'first_tab' => $this->createFieldContainer(['label' => 'Reconditionnement', 'type' => 'tab'] + $common), + 'last_tab' => $this->createFieldContainer(['label' => 'Achat et vente', 'type' => 'tab'] + $common), + 'dom' => $this->createFieldContainer(['label' => 'Infos Supps', 'type' => 'dom'] + $common), + ]; + + $definitions = [ + 'first_tab_ticket' => ['first_tab', 'Ticket Lot', 'dropdown-Ticket'], + 'first_tab_entity' => ['first_tab', 'Entite origine', 'dropdown-Entity'], + 'first_tab_text' => ['first_tab', 'Destination', 'text'], + 'last_tab_ticket' => ['last_tab', 'Ticket Vente', 'dropdown-Ticket'], + 'dom_ticket' => ['dom', 'Ticket Lot', 'dropdown-Ticket'], + ]; + $ranking = 0; + foreach ($definitions as $key => [$container_key, $label, $type]) { + $this->fields[$key] = $this->createField([ + 'label' => $label, + 'type' => $type, + PluginFieldsContainer::getForeignKeyField() => $this->containers[$container_key]->getID(), + 'ranking' => ++$ranking, + 'is_active' => 1, + 'is_readonly' => 0, + ]); + } + } + + private function buildMassiveAction(array $post, string $itemtype, int $id): MassiveAction + { + $ref = new ReflectionClass(MassiveAction::class); + $ma = $ref->newInstanceWithoutConstructor(); + $ma->POST = $post; + + $set = static function (string $name, $value) use ($ref, $ma): void { + $ref->getProperty($name)->setValue($ma, $value); + }; + + $set('action', 'update'); + $set('done', []); + $set('nb_done', 0); + $set('current_itemtype', null); + $set('remainings', [$itemtype => [$id => $id]]); + $set('results', ['ok' => 0, 'noaction' => 0, 'ko' => 0, 'noright' => 0, 'messages' => []]); + $set('start_time', microtime(true)); + + return $ma; + } + + /** + * Run the core "update" massive action on a single computer. + * + * @return array{ok: int, noaction: int, ko: int, noright: int, messages: array} + */ + private function runMassiveUpdate(Computer $computer, string $field_name, mixed $value, ?int $c_id = null): array + { + $so_index = null; + foreach (SearchOption::getOptionsForItemtype(Computer::class) as $idx => $opt) { + if (is_array($opt) && ($opt['linkfield'] ?? null) === $field_name) { + $so_index = $idx; + } + } + + $this->assertNotNull($so_index, sprintf('Search option not found for field "%s"', $field_name)); + + $_REQUEST['massiveaction'] = 1; + $_POST = [ + 'common_options' => [Computer::class . ':' . $so_index => Computer::class . ':' . $so_index], + 'search_options' => [Computer::class => $so_index], + 'id_field' => Computer::class . ':' . $so_index, + 'field' => $field_name, + $field_name => $value, + ]; + if ($c_id !== null) { + $_REQUEST['c_id'] = $c_id; + $_POST['c_id'] = $c_id; + } + + $ma = $this->buildMassiveAction($_POST, Computer::class, $computer->getID()); + MassiveAction::processMassiveActionsForOneItemtype($ma, $computer, [$computer->getID()]); + + return (new ReflectionClass(MassiveAction::class))->getProperty('results')->getValue($ma); + } + + /** + * @return array|null + */ + private function getStoredValues(string $container_key, Computer $computer): ?array + { + $classname = PluginFieldsContainer::getClassname(Computer::class, $this->containers[$container_key]->fields['name']); + $values = new $classname(); + + return $values->getFromDBByCrit(['items_id' => $computer->getID()]) ? $values->fields : null; + } + + private function setRightOnContainer(string $container_key, int $right): void + { + $profile_right = new PluginFieldsProfile(); + $this->assertTrue($profile_right->getFromDBByCrit([ + 'profiles_id' => $_SESSION['glpiactiveprofile']['id'], + 'plugin_fields_containers_id' => $this->containers[$container_key]->getID(), + ])); + $this->updateItem(PluginFieldsProfile::class, $profile_right->getID(), ['right' => $right]); + } + + public static function fieldProvider(): iterable + { + // GLPI item dropdowns: the core displays its own input, without the plugin `c_id` + yield 'ticket field in the first tab container' => ['first_tab_ticket', 'first_tab']; + yield 'entity field in the first tab container' => ['first_tab_entity', 'first_tab']; + yield 'ticket field in the last tab container' => ['last_tab_ticket', 'last_tab']; + yield 'ticket field in the dom container' => ['dom_ticket', 'dom']; + // plugin input, `c_id` lost when the massive action is reloaded to refresh the progress bar + yield 'text field in the first tab container' => ['first_tab_text', 'first_tab']; + } + + #[DataProvider('fieldProvider')] + public function testMassiveUpdateWithoutContainerId(string $field_key, string $container_key): void + { + $this->createContainers(); + + $entities_id = $this->getTestRootEntity(true); + $computer = $this->createItem(Computer::class, ['name' => 'PC', 'entities_id' => $entities_id]); + $field_name = $this->fields[$field_key]->fields['name']; + $value = match ($this->fields[$field_key]->fields['type']) { + 'dropdown-Ticket' => $this->createItem(Ticket::class, ['name' => 'Lot', 'content' => 'Lot', 'entities_id' => $entities_id])->getID(), + // an entity visible from the computer one, to pass the core entity compatibility check + 'dropdown-Entity' => getItemByTypeName(Entity::class, '_test_child_1', true), + default => 'Atelier', + }; + + $results = $this->runMassiveUpdate($computer, $field_name, $value); + $this->assertSame(1, $results['ok'], json_encode($results)); + + $stored = $this->getStoredValues($container_key, $computer); + $this->assertNotNull($stored, 'Value has not been stored in the targeted container'); + $this->assertEquals($value, $stored[$field_name]); + + // nothing must be stored in the other containers + foreach (array_keys($this->containers) as $other_key) { + if ($other_key !== $container_key) { + $this->assertNull($this->getStoredValues($other_key, $computer), sprintf('Unexpected values stored in "%s" container', $other_key)); + } + } + } + + public function testMassiveUpdateWithContainerId(): void + { + $this->createContainers(); + + $computer = $this->createItem(Computer::class, ['name' => 'PC', 'entities_id' => $this->getTestRootEntity(true)]); + $field_name = $this->fields['first_tab_text']->fields['name']; + + $results = $this->runMassiveUpdate($computer, $field_name, 'Atelier', $this->containers['first_tab']->getID()); + $this->assertSame(1, $results['ok'], json_encode($results)); + + $this->assertSame('Atelier', $this->getStoredValues('first_tab', $computer)[$field_name] ?? null); + } + + public function testMassiveUpdateWithoutUpdateRightOnContainer(): void + { + $this->createContainers(); + $this->setRightOnContainer('first_tab', READ); + + $entities_id = $this->getTestRootEntity(true); + $computer = $this->createItem(Computer::class, ['name' => 'PC', 'entities_id' => $entities_id]); + $ticket = $this->createItem(Ticket::class, ['name' => 'Lot', 'content' => 'Lot', 'entities_id' => $entities_id]); + + $this->runMassiveUpdate($computer, $this->fields['first_tab_ticket']->fields['name'], $ticket->getID()); + + $this->assertNull($this->getStoredValues('first_tab', $computer)); + } + + public function testUpdateOfInactiveTabContainerField(): void + { + $this->createContainers(); + $this->updateItem(PluginFieldsContainer::class, $this->containers['first_tab']->getID(), ['is_active' => 0]); + + $computer = $this->createItem(Computer::class, ['name' => 'PC', 'entities_id' => $this->getTestRootEntity(true)]); + $field_name = $this->fields['first_tab_text']->fields['name']; + $this->updateItem(Computer::class, $computer->getID(), [$field_name => 'Atelier'], [$field_name]); + + $this->assertNull($this->getStoredValues('first_tab', $computer)); + } + + public function testUpdateWithoutFieldsDoesNotStoreTabContainersValues(): void + { + $this->createContainers(); + + $computer = $this->createItem(Computer::class, ['name' => 'PC', 'entities_id' => $this->getTestRootEntity(true)]); + $this->updateItem(Computer::class, $computer->getID(), ['comment' => 'updated']); + + foreach (array_keys($this->containers) as $container_key) { + $this->assertNull($this->getStoredValues($container_key, $computer), sprintf('Unexpected values stored in "%s" container', $container_key)); + } + } +} From c62b76e59431a0fb8643a0188fa7398028519ffe Mon Sep 17 00:00:00 2001 From: Herafia Date: Mon, 26 Oct 2026 09:42:01 +0100 Subject: [PATCH 2/2] Save fields of all submitted tab containers in a single update --- inc/container.class.php | 123 +++++++++++------- ...MassiveActionMultipleTabContainersTest.php | 23 ++++ 2 files changed, 97 insertions(+), 49 deletions(-) diff --git a/inc/container.class.php b/inc/container.class.php index ec760ec2..6773fe98 100644 --- a/inc/container.class.php +++ b/inc/container.class.php @@ -2027,31 +2027,36 @@ private static function findSubmittedTabContainers(CommonDBTM $item): array $c_ids = []; foreach ($containers as $data) { if ( - !in_array($item::class, PluginFieldsToolbox::decodeJSONItemtypes($data['itemtypes'])) - || !Session::haveAccessToEntity($data['entities_id'], $data['is_recursive']) + in_array($item::class, PluginFieldsToolbox::decodeJSONItemtypes($data['itemtypes'])) + && Session::haveAccessToEntity($data['entities_id'], $data['is_recursive']) ) { - continue; + $c_ids[] = (int) $data['id']; } + } - $fields = (new PluginFieldsField())->find([ - 'plugin_fields_containers_id' => $data['id'], - 'is_active' => 1, - 'type' => ['!=', 'header'], - ]); - foreach ($fields as $field) { - $input_keys = [ - $field['name'], - sprintf('plugin_fields_%sdropdowns_id', $field['name']), - sprintf('items_id_%s', $field['name']), - ]; - if (array_intersect($input_keys, array_keys($item->input)) !== []) { - $c_ids[] = (int) $data['id']; - break; - } + if ($c_ids === []) { + return []; + } + + $fields = (new PluginFieldsField())->find([ + 'plugin_fields_containers_id' => $c_ids, + 'is_active' => 1, + 'type' => ['!=', 'header'], + ], 'plugin_fields_containers_id'); + + $submitted = []; + foreach ($fields as $field) { + $input_keys = [ + $field['name'], + sprintf('plugin_fields_%sdropdowns_id', $field['name']), + sprintf('items_id_%s', $field['name']), + ]; + if (array_intersect($input_keys, array_keys($item->input)) !== []) { + $submitted[] = (int) $field['plugin_fields_containers_id']; } } - return $c_ids; + return array_values(array_unique($submitted)); } public static function findAllContainers($itemtype) @@ -2096,18 +2101,19 @@ public static function postItemAdd(CommonDBTM $item) PluginFieldsDestinationField::registerCreatedItem($item); if (array_key_exists('_plugin_fields_data', $item->input)) { - $data = $item->input['_plugin_fields_data']; - $data['itemtype'] = $item::class; - $data['items_id'] = $item->getID(); - $data['entities_id'] = $item->isEntityAssign() ? $item->getEntityID() : 0; - //update data $container = new self(); - if ($container->updateFieldsValues($data, $item->getType(), isset($_REQUEST['massiveaction']))) { - return true; + foreach (self::getSubmittedContainersData($item) as $data) { + $data['itemtype'] = $item::class; + $data['items_id'] = $item->getID(); + $data['entities_id'] = $item->isEntityAssign() ? $item->getEntityID() : 0; + //update data + if (!$container->updateFieldsValues($data, $item->getType(), isset($_REQUEST['massiveaction']))) { + $item->input = []; + return $item; + } } - $item->input = []; - return $item; + return true; } return true; @@ -2140,26 +2146,43 @@ public static function preItemUpdate(CommonDBTM $item) return true; } - $data = $item->input['_plugin_fields_data']; - $data['itemtype'] = $item::class; - $data['entities_id'] = $item->isEntityAssign() ? $item->getEntityID() : 0; - //update data $container = new self(); - if ( - count($data) === 0 - || $container->updateFieldsValues($data, $item->getType(), isset($_REQUEST['massiveaction'])) - ) { - $item->input['date_mod'] = $_SESSION['glpi_currenttime']; - - return true; + foreach (self::getSubmittedContainersData($item) as $data) { + $data['itemtype'] = $item::class; + $data['entities_id'] = $item->isEntityAssign() ? $item->getEntityID() : 0; + //update data + if (!$container->updateFieldsValues($data, $item->getType(), isset($_REQUEST['massiveaction']))) { + return false; + } } - return false; + $item->input['date_mod'] = $_SESSION['glpi_currenttime']; + + return true; } return true; } + /** + * Get the data to store for each submitted container + * + * @param CommonDBTM $item Item instance + * + * @return array> + */ + private static function getSubmittedContainersData(CommonDBTM $item): array + { + $data = $item->input['_plugin_fields_data']; + + // data of a single container + if (array_key_exists('plugin_fields_containers_id', $data)) { + return [$data]; + } + + return $data; + } + /** * Pre item hook for add and update * Validates and store plugin data in item object @@ -2185,14 +2208,16 @@ public static function preItem(CommonDBTM $item) // generic add/update: both the "dom" and "tab" containers can carry // mandatory fields that must be enforced, even though only "dom" // fields are actually submitted inline with the main form - $c_ids = array_filter( - [self::findContainer($item::class, 'dom'), self::findContainer($item::class, 'tab')], - static fn($id) => $id !== false, - ); - // other tab containers whose fields are submitted (ex: massive action on a GLPI item // dropdown field for which the core never displays the plugin field nor its c_id input) - $c_ids = array_values(array_unique(array_merge($c_ids, self::findSubmittedTabContainers($item)))); + $c_ids = array_values(array_unique(array_filter( + [ + self::findContainer($item::class, 'dom'), + ...self::findSubmittedTabContainers($item), + self::findContainer($item::class, 'tab'), + ], + static fn($id) => $id !== false, + ))); } if ($c_ids === []) { @@ -2203,7 +2228,7 @@ public static function preItem(CommonDBTM $item) $item->fields = $item->input; } - $submitted_data = null; + $submitted_data = []; foreach ($c_ids as $c_id) { $loc_c = new PluginFieldsContainer(); $loc_c->getFromDB($c_id); @@ -2238,11 +2263,11 @@ public static function preItem(CommonDBTM $item) } if ($result !== []) { - $submitted_data = $result; + $submitted_data[] = $result; } } - if ($submitted_data !== null) { + if ($submitted_data !== []) { $item->input['_plugin_fields_data'] = $submitted_data; return true; diff --git a/tests/Units/MassiveActionMultipleTabContainersTest.php b/tests/Units/MassiveActionMultipleTabContainersTest.php index ca52ad49..fe92e72e 100644 --- a/tests/Units/MassiveActionMultipleTabContainersTest.php +++ b/tests/Units/MassiveActionMultipleTabContainersTest.php @@ -286,4 +286,27 @@ public function testUpdateWithoutFieldsDoesNotStoreTabContainersValues(): void $this->assertNull($this->getStoredValues($container_key, $computer), sprintf('Unexpected values stored in "%s" container', $container_key)); } } + + public function testUpdateOfTwoTabContainersFields(): void + { + $this->createContainers(); + + $entities_id = $this->getTestRootEntity(true); + $computer = $this->createItem(Computer::class, ['name' => 'PC', 'entities_id' => $entities_id]); + $ticket = $this->createItem(Ticket::class, ['name' => 'Lot', 'content' => 'Lot', 'entities_id' => $entities_id]); + $first_name = $this->fields['first_tab_text']->fields['name']; + $last_name = $this->fields['last_tab_ticket']->fields['name']; + + $this->updateItem( + Computer::class, + $computer->getID(), + [$first_name => 'Atelier', $last_name => $ticket->getID()], + [$first_name, $last_name], + ); + + // both tab containers are saved + $this->assertSame('Atelier', $this->getStoredValues('first_tab', $computer)[$first_name] ?? null); + $this->assertEquals($ticket->getID(), $this->getStoredValues('last_tab', $computer)[$last_name] ?? null); + $this->assertNull($this->getStoredValues('dom', $computer)); + } }