diff --git a/CHANGELOG.md b/CHANGELOG.md index c29b56e..a3ff56e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed - Fix infinite loop / Gateway Timeout when a ticket's assigned technician and assigned group are changed simultaneously, caused by a synchronous actor removal during `pre_item_update()` +- Fixed group reassignment to remove previously assigned groups only when using the Escalade reassignment action ## [2.10.6] - 2026-07-31 diff --git a/front/ticket.form.php b/front/ticket.form.php index cfd9ad4..4382657 100644 --- a/front/ticket.form.php +++ b/front/ticket.form.php @@ -44,7 +44,14 @@ throw new AccessDeniedHttpException(); } - PluginEscaladeTicket::timelineClimbAction($group_id, $tickets_id, $_POST); + // Mark this operation as a real escalation from the Escalade form. + $_SESSION['plugin_escalade']['is_escalation'] = true; + + try { + PluginEscaladeTicket::timelineClimbAction($group_id, $tickets_id, $_POST); + } finally { + unset($_SESSION['plugin_escalade']['is_escalation']); + } $track = new Ticket(); diff --git a/inc/history.class.php b/inc/history.class.php index f2e7784..53600d7 100644 --- a/inc/history.class.php +++ b/inc/history.class.php @@ -160,12 +160,39 @@ public static function getHistory($tickets_id, $full_history = false) $group = new Group(); $history = new self(); - $found = $history->find(['tickets_id' => $tickets_id], "date_mod DESC"); - $nb_histories = count($found); + $found = $history->find( + ['tickets_id' => $tickets_id], + ['date_mod DESC', 'id DESC'], + ); //remove first line (current assign) $first_group = array_shift($found); + if (!$full_history) { + // Hide still-assigned groups from the compact widget only: the full popup stays a complete audit trail. + $group_ticket = new Group_Ticket(); + $currently_assigned = $group_ticket->find([ + 'tickets_id' => $tickets_id, + 'type' => CommonITILActor::ASSIGN, + ]); + + $currently_assigned_ids = array_map( + static fn(array $actor): int => (int) $actor['groups_id'], + $currently_assigned, + ); + + $found = array_filter( + $found, + static fn(array $history_entry): bool => !in_array( + (int) $history_entry['groups_id'], + $currently_assigned_ids, + true, + ), + ); + } + + $nb_histories = count($found) + 1; + if ($full_history) { //show 1st group echo "
"; diff --git a/inc/ticket.class.php b/inc/ticket.class.php index 96f7c3b..55bdfbc 100644 --- a/inc/ticket.class.php +++ b/inc/ticket.class.php @@ -540,7 +540,16 @@ public static function processAfterAddGroup(Group_Ticket $item) // getFromDB() is required first so isNewItem() returns false and deleted-actor // detection runs. _plugin_escalade_rules_only skips escalade logic in pre_item_update. // Safety net in case updateActors() above did not already remove old groups. - if ($_SESSION['glpi_plugins']['escalade']['config']['remove_group'] == true) { + if ( + $_SESSION['glpi_plugins']['escalade']['config']['remove_group'] == true + && ( + !empty($_SESSION['plugin_escalade']['is_escalation']) + || !empty($_SESSION['plugin_escalade']['climb_group']) + || !empty($_SESSION['plugin_escalade']['auto_group_assignment']) + || !empty($_SESSION['plugin_escalade']['category_group_reassignment']) + ) + ) { + $all_actors = self::getTicketFieldsWithActors($tickets_id, $groups_id); // Keep only the new group in the assign list (drop old ones). @@ -579,6 +588,7 @@ function (array $actor) use ($groups_id, &$seen_new_group): bool { unset($_SESSION['plugin_escalade']['keep_new_assign_users'][$tickets_id]); self::removeAssignUsers($item, $keep_users_id); + if ($_SESSION['glpi_plugins']['escalade']['config']['ticket_last_status'] != self::MANAGED_BY_CORE) { $ticket = new Ticket(); $ticket->update([ @@ -707,12 +717,22 @@ public static function climb_group($tickets_id, $groups_id, $no_redirect = false // and wipes them from the ticket regardless of the // "Remove requester(s) on escalation" plugin config. $ticket = new Ticket(); - $ticket->update([ - 'id' => $tickets_id, - '_actors' => self::getTicketFieldsWithActors($tickets_id, $groups_id), - 'actortype' => CommonITILActor::ASSIGN, - 'groups_id' => $groups_id, - ]); + + // Mark this assignment as an actual Escalade reassignment. + // processAfterAddGroup() also runs for normal GLPI group assignments, + // so old groups must only be removed for this specific action. + $_SESSION['plugin_escalade']['climb_group'] = true; + + try { + $ticket->update([ + 'id' => $tickets_id, + '_actors' => self::getTicketFieldsWithActors($tickets_id, $groups_id), + 'actortype' => CommonITILActor::ASSIGN, + 'groups_id' => $groups_id, + ]); + } finally { + unset($_SESSION['plugin_escalade']['climb_group']); + } } if (!$no_redirect) { @@ -891,12 +911,19 @@ public static function item_add_user(Ticket_User $item, $type = CommonITILActor: //prevent user removal $_SESSION['plugin_escalade']['keep_users'][$item->fields['users_id']] = $item->fields['users_id']; - //add new group to ticket - $group_ticket->add([ - 'tickets_id' => $tickets_id, - 'groups_id' => $groups_id, - 'type' => CommonITILActor::ASSIGN, - ]); + + // Mark this group assignment as an automatic Escalade assignment. + $_SESSION['plugin_escalade']['auto_group_assignment'] = true; + + try { + $group_ticket->add([ + 'tickets_id' => $tickets_id, + 'groups_id' => $groups_id, + 'type' => CommonITILActor::ASSIGN, + ]); + } finally { + unset($_SESSION['plugin_escalade']['auto_group_assignment']); + } } elseif ($_SESSION['glpi_plugins']['escalade']['config']['remove_tech']) { self::removeAssignGroups($tickets_id); } @@ -992,7 +1019,14 @@ public static function qualification(CommonDBTM $item) $group_found = $group_ticket->find($group_condition); if (empty($group_found)) { //add group to ticket - $group_ticket->add($group_condition); + $_SESSION['plugin_escalade']['category_group_reassignment'] = true; + + try { + $group_ticket->add($group_condition); + } finally { + unset($_SESSION['plugin_escalade']['category_group_reassignment']); + } + //remove old group if needed if ($_SESSION['glpi_plugins']['escalade']['config']['remove_group'] && isset($item->input['_groups_id_assign'])) { foreach ($item->input['_groups_id_assign'] as $idActor => $actor) { diff --git a/tests/EscaladeTestCase.php b/tests/EscaladeTestCase.php index d831dda..6421a12 100644 --- a/tests/EscaladeTestCase.php +++ b/tests/EscaladeTestCase.php @@ -176,7 +176,19 @@ public function escalateWithTimelineButton(Ticket $ticket, Group $group, array $ ], ); $_POST['comment'] = $options['comment'] ?? 'Default comment'; - PluginEscaladeTicket::timelineClimbAction($group->getID(), $ticket->getID(), $options); + + $_SESSION['plugin_escalade']['is_escalation'] = true; + + try { + PluginEscaladeTicket::timelineClimbAction( + $group->getID(), + $ticket->getID(), + $options, + ); + } finally { + unset($_SESSION['plugin_escalade']['is_escalation']); + } + $ticketgroup = new Group_Ticket(); $is_escalate = $ticketgroup->getFromDBByCrit([ 'tickets_id' => $ticket->getID(), diff --git a/tests/Units/GroupEscalationTest.php b/tests/Units/GroupEscalationTest.php index 8db2e4f..c3eab8b 100644 --- a/tests/Units/GroupEscalationTest.php +++ b/tests/Units/GroupEscalationTest.php @@ -37,6 +37,7 @@ use Notification; use NotificationTarget; use PluginEscaladeHistory; +use PluginEscaladeTicket; use PluginEscaladeNotification; use QueuedNotification; use Ticket; @@ -45,6 +46,154 @@ final class GroupEscalationTest extends EscaladeTestCase { + /** + * Standard GLPI group assignment must not remove previously assigned groups. + */ + public function testStandardGroupAssignmentKeepsExistingGroups(): void + { + $this->initConfig([ + 'remove_group' => 1, + 'show_history' => 1, + ]); + + $group1 = $this->createGroup('standard_group_1_' . uniqid()); + $group2 = $this->createGroup('standard_group_2_' . uniqid()); + + $ticket = $this->createItem(Ticket::class, [ + 'name' => 'Standard group assignment regression test', + 'content' => '', + '_actors' => [ + 'assign' => [ + [ + 'items_id' => $group1->getID(), + 'itemtype' => 'Group', + ], + ], + ], + ]); + + // Simulate adding another group from the standard GLPI actors field. + $this->createItem(Group_Ticket::class, [ + 'tickets_id' => $ticket->getID(), + 'groups_id' => $group2->getID(), + 'type' => CommonITILActor::ASSIGN, + ]); + + $this->assertEquals(2, countElementsInTable(Group_Ticket::getTable(), [ + 'tickets_id' => $ticket->getID(), + 'type' => CommonITILActor::ASSIGN, + ])); + + $this->assertEquals(1, countElementsInTable(Group_Ticket::getTable(), [ + 'tickets_id' => $ticket->getID(), + 'groups_id' => $group1->getID(), + 'type' => CommonITILActor::ASSIGN, + ])); + + $this->assertEquals(1, countElementsInTable(Group_Ticket::getTable(), [ + 'tickets_id' => $ticket->getID(), + 'groups_id' => $group2->getID(), + 'type' => CommonITILActor::ASSIGN, + ])); + } + + /** + * A real Escalade reassignment must remove old groups while preserving + * every previously assigned group in the visual assignment history. + */ + public function testEscaladeReassignmentPreservesAllGroupsInHistory(): void + { + $this->initConfig([ + 'remove_group' => 1, + 'show_history' => 1, + ]); + + $group1 = $this->createGroup('history_group_1_' . uniqid()); + $group2 = $this->createGroup('history_group_2_' . uniqid()); + $group3 = $this->createGroup('history_group_3_' . uniqid()); + $group4 = $this->createGroup('history_destination_' . uniqid()); + + $ticket = $this->createItem(Ticket::class, [ + 'name' => 'Multiple group history regression test', + 'content' => '', + '_actors' => [ + 'assign' => [ + [ + 'items_id' => $group1->getID(), + 'itemtype' => 'Group', + ], + ], + ], + ]); + + // Add groups through the normal GLPI assignment mechanism. + foreach ([$group2, $group3] as $group) { + $this->createItem(Group_Ticket::class, [ + 'tickets_id' => $ticket->getID(), + 'groups_id' => $group->getID(), + 'type' => CommonITILActor::ASSIGN, + ]); + } + + $group_ticket = new Group_Ticket(); + + $this->assertEquals(3, countElementsInTable(Group_Ticket::getTable(), [ + 'tickets_id' => $ticket->getID(), + 'type' => CommonITILActor::ASSIGN, + ])); + + // Simulate the real Escalade button. + $_SESSION['plugin_escalade']['is_escalation'] = true; + $_POST['comment'] = 'Regression test escalation'; + + try { + PluginEscaladeTicket::timelineClimbAction( + $group4->getID(), + $ticket->getID(), + [ + 'ticket_details' => [ + 'id' => $ticket->getID(), + ], + ], + ); + } finally { + unset($_SESSION['plugin_escalade']['is_escalation']); + unset($_POST['comment']); + } + + // Only the destination group must remain assigned. + $this->assertEquals(1, countElementsInTable(Group_Ticket::getTable(), [ + 'tickets_id' => $ticket->getID(), + 'type' => CommonITILActor::ASSIGN, + ])); + + $assigned_groups = $group_ticket->find([ + 'tickets_id' => $ticket->getID(), + 'type' => CommonITILActor::ASSIGN, + ]); + + $assigned_group = reset($assigned_groups); + $this->assertEquals($group4->getID(), $assigned_group['groups_id']); + + // All groups involved in the reassignment must remain visible + // in Escalade history. + $history = new PluginEscaladeHistory(); + + foreach ([$group1, $group2, $group3, $group4] as $group) { + $this->assertGreaterThanOrEqual( + 1, + count($history->find([ + 'tickets_id' => $ticket->getID(), + 'groups_id' => $group->getID(), + ])), + sprintf( + 'Group %d is missing from Escalade history', + $group->getID(), + ), + ); + } + } + public function testTechGroupAttributionUpdateTicket() { $this->initConfig([ @@ -80,8 +229,9 @@ public function testTechGroupAttributionUpdateTicket() ); // Check no group linked to the ticket - $ticket_group = new Group_Ticket(); - $this->assertEquals(0, count($ticket_group->find(['tickets_id' => $ticket->getID()]))); + $this->assertEquals(0, countElementsInTable(Group_Ticket::getTable(), [ + 'tickets_id' => $ticket->getID(), + ])); // Update ticket with a technician $this->updateItem(Ticket::class, $ticket->getID(), [ @@ -102,8 +252,10 @@ public function testTechGroupAttributionUpdateTicket() ]); $ticket_group2 = new Group_Ticket(); - // Check only one groupe linked to the ticket - $this->assertEquals(1, count($ticket_group2->find(['tickets_id' => $ticket->getID()]))); + // Check only one group linked to the ticket + $this->assertEquals(1, countElementsInTable(Group_Ticket::getTable(), [ + 'tickets_id' => $ticket->getID(), + ])); $ticket_group2->getFromDBByCrit([ 'tickets_id' => $ticket->getID(), diff --git a/tests/Units/TicketTest.php b/tests/Units/TicketTest.php index 6170a1c..f4d7f29 100644 --- a/tests/Units/TicketTest.php +++ b/tests/Units/TicketTest.php @@ -355,14 +355,14 @@ public function testTicketUpdateDoesNotChangeITILCategoryAssignedGroup() ], ]); - $this->assertEquals(0, count($group_ticket->find(['tickets_id' => $ticket_id, 'groups_id' => $group1_id, 'type' => CommonITILActor::ASSIGN]))); + $this->assertEquals(1, count($group_ticket->find(['tickets_id' => $ticket_id, 'groups_id' => $group1_id, 'type' => CommonITILActor::ASSIGN]))); $this->assertEquals(1, count($group_ticket->find(['tickets_id' => $ticket_id, 'groups_id' => $group2_id, 'type' => CommonITILActor::ASSIGN]))); $this->updateItem('Ticket', $ticket_id, [ 'status' => CommonITILObject::WAITING, ]); - $this->assertEquals(0, count($group_ticket->find(['tickets_id' => $ticket_id, 'groups_id' => $group1_id, 'type' => CommonITILActor::ASSIGN]))); + $this->assertEquals(1, count($group_ticket->find(['tickets_id' => $ticket_id, 'groups_id' => $group1_id, 'type' => CommonITILActor::ASSIGN]))); $this->assertEquals(1, count($group_ticket->find(['tickets_id' => $ticket_id, 'groups_id' => $group2_id, 'type' => CommonITILActor::ASSIGN]))); }