From 98afff7c9a4f29a6f4bbabcc2a2ae727c021a36f Mon Sep 17 00:00:00 2001 From: Rom1-B <8530352+Rom1-B@users.noreply.github.com> Date: Mon, 24 Aug 2026 12:30:52 +0200 Subject: [PATCH] Fix: simplify group reassignment via core actor diff --- CHANGELOG.md | 1 + inc/ticket.class.php | 30 ++--- tests/Units/ActorDiffReplacementTest.php | 134 +++++++++++++++++++++++ tests/Units/GroupEscalationTest.php | 10 +- tests/Units/TicketTest.php | 55 ++++++++++ 5 files changed, 200 insertions(+), 30 deletions(-) create mode 100644 tests/Units/ActorDiffReplacementTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 381d374a..ac252717 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,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()` +- Simplify group reassignment to a single actor diff, relying on GLPI core's status preservation on simultaneous actor removal/addition ## [2.10.6] - 2026-07-31 diff --git a/inc/ticket.class.php b/inc/ticket.class.php index 03b22c8f..b28a6726 100644 --- a/inc/ticket.class.php +++ b/inc/ticket.class.php @@ -535,32 +535,11 @@ public static function processAfterAddGroup(Group_Ticket $item) $tickets_id = $item->fields['tickets_id']; $groups_id = $item->fields['groups_id']; - // Fire business rules before removing old groups: pass _actors with only the new - // group so GLPI detects old groups as deleted and rules see the final state. - // 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. + // Safety net for entry points (auto assign group, category-based reassignment) + // that add a group without already removing the old one themselves. if ($_SESSION['glpi_plugins']['escalade']['config']['remove_group'] == true) { $all_actors = self::getTicketFieldsWithActors($tickets_id, $groups_id); - // Keep only the new group in the assign list (drop old ones). - $seen_new_group = false; - $all_actors['assign'] = array_values(array_filter( - $all_actors['assign'], - function (array $actor) use ($groups_id, &$seen_new_group): bool { - if ($actor['itemtype'] !== 'Group') { - return true; - } - - if ($actor['items_id'] == $groups_id && !$seen_new_group) { - $seen_new_group = true; - return true; - } - - return false; - }, - )); - $ticket_for_rules = new Ticket(); $ticket_for_rules->getFromDB($tickets_id); $ticket_for_rules->update([ @@ -1267,6 +1246,11 @@ public static function getTicketFieldsWithActors($tickets_id, $group_id) } } + // Diff-only when remove_group: lets core delete the old group(s) and add the new one in a single call. + if ($_SESSION['glpi_plugins']['escalade']['config']['remove_group'] == true) { + $ticket_actors['Group']['assign'] = []; + } + $ticket_actors['Group']['assign'][] = [ 'itemtype' => 'Group', 'items_id' => $group_id, diff --git a/tests/Units/ActorDiffReplacementTest.php b/tests/Units/ActorDiffReplacementTest.php new file mode 100644 index 00000000..ccc149d0 --- /dev/null +++ b/tests/Units/ActorDiffReplacementTest.php @@ -0,0 +1,134 @@ +. + * ------------------------------------------------------------------------- + * @copyright Copyright (C) 2015-2023 by Escalade plugin team. + * @license GPLv2 https://www.gnu.org/licenses/gpl-2.0.html + * @link https://github.com/pluginsGLPI/escalade + * ------------------------------------------------------------------------- + */ + +namespace GlpiPlugin\Escalade\Tests\Units; + +use CommonITILActor; +use Glpi\DBAL\QueryExpression; +use GlpiPlugin\Escalade\Tests\EscaladeTestCase; +use Group_Ticket; +use Notification; +use NotificationTarget; +use QueuedNotification; +use Ticket; +use User; + +// Kept isolated from GroupEscalationTest.php, which has a pre-existing state-leak flakiness. +final class ActorDiffReplacementTest extends EscaladeTestCase +{ + public function testSingleShotGroupReplacementPreservesStatusAndNotifiesOnlyNewGroup(): void + { + global $CFG_GLPI, $DB; + + $this->initConfig([ + 'remove_group' => 1, + 'show_history' => 1, + ]); + + $CFG_GLPI['use_notifications'] = true; + $CFG_GLPI['notifications_mailing'] = true; + + $DB->update(Notification::getTable(), ['is_active' => false], [new QueryExpression('true')]); + + $notification = new Notification(); + if (!$notification->getFromDBByCrit(['itemtype' => 'Ticket', 'event' => 'assign_group'])) { + $this->markTestSkipped('assign_group notification not found'); + } + + $this->assertTrue($notification->update(['id' => $notification->getID(), 'is_active' => 1])); + + $DB->delete(NotificationTarget::getTable(), ['notifications_id' => $notification->getID()]); + $this->createItem(NotificationTarget::class, [ + 'notifications_id' => $notification->getID(), + 'items_id' => Notification::ASSIGN_GROUP, + 'type' => Notification::USER_TYPE, + ]); + + [$user1, $user2] = $this->createItems(User::class, [ + ['name' => 'diff_user1_' . uniqid(), '_useremails' => [-1 => 'diff1_' . uniqid() . '@example.com']], + ['name' => 'diff_user2_' . uniqid(), '_useremails' => [-1 => 'diff2_' . uniqid() . '@example.com']], + ]); + + $group1 = $this->createGroupAndAssignUsers($user1, 'diff_group_1_' . uniqid()); + $group2 = $this->createGroupAndAssignUsers($user2, 'diff_group_2_' . uniqid()); + + $ticket = $this->createItem(Ticket::class, [ + 'name' => 'Diff replacement test', + 'content' => 'content', + 'entities_id' => $this->getTestRootEntity(true), + '_actors' => [ + 'assign' => [ + ['items_id' => $group1->getID(), 'itemtype' => 'Group'], + ], + ], + ]); + $this->assertEquals(Ticket::ASSIGNED, $ticket->fields['status']); + + $this->cleanQueuedNotifications(); + + // Diff-only: only group2 is sent, core must delete group1 and add group2 in one call. + $this->updateItem( + Ticket::class, + $ticket->getID(), + [ + '_actors' => [ + 'assign' => [ + ['items_id' => $group2->getID(), 'itemtype' => 'Group'], + ], + ], + ], + ); + + $group_ticket = new Group_Ticket(); + $this->assertFalse($group_ticket->getFromDBByCrit([ + 'tickets_id' => $ticket->getID(), + 'groups_id' => $group1->getID(), + 'type' => CommonITILActor::ASSIGN, + ]), 'The old group must have been removed'); + $this->assertTrue($group_ticket->getFromDBByCrit([ + 'tickets_id' => $ticket->getID(), + 'groups_id' => $group2->getID(), + 'type' => CommonITILActor::ASSIGN, + ]), 'The new group must have been added'); + + $ticket->getFromDB($ticket->getID()); + $this->assertEquals( + Ticket::ASSIGNED, + $ticket->fields['status'], + 'Status must be preserved despite the old group being removed in the same call as the new group addition', + ); + + $queued = new QueuedNotification(); + $recipients = array_column($queued->find(), 'recipient'); + + $this->assertContains($this->getItemEmail($user2), $recipients, "The new group's users must be notified"); + $this->assertNotContains($this->getItemEmail($user1), $recipients, "The old group's users must not be notified"); + } +} diff --git a/tests/Units/GroupEscalationTest.php b/tests/Units/GroupEscalationTest.php index 8db2e4fc..aeee145b 100644 --- a/tests/Units/GroupEscalationTest.php +++ b/tests/Units/GroupEscalationTest.php @@ -702,10 +702,7 @@ public function testHistory() $this->assertEquals(1, count($history->find(['tickets_id' => $ticket->getID(), 'groups_id' => $group1->getID()]))); } - /** - * Test that the standard target "Group in charge of the ticket" - * sends notifications to users of both groups (old and new) during an escalation - */ + // The "Group in charge of the ticket" target only notifies the newly assigned group on escalation. public function testStandardGroupNotification() { global $CFG_GLPI, $DB; @@ -799,11 +796,10 @@ public function testStandardGroupNotification() $notification_recipients[] = $notif['recipient']; } - // Check that users from both groups received notifications + // Only the new group's users should be notified; the old group was removed. $group1_user_emails = [$this->getItemEmail($user1), $this->getItemEmail($user2)]; $group2_user_emails = [$this->getItemEmail($user3), $this->getItemEmail($user4)]; - // At least one user from each group should have received a notification $group1_notified = false; $group2_notified = false; @@ -821,7 +817,7 @@ public function testStandardGroupNotification() } } - $this->assertTrue($group1_notified, "No user from the original group received a notification"); + $this->assertFalse($group1_notified, "A user from the removed group should not receive a notification"); $this->assertTrue($group2_notified, "No user from the new group received a notification"); } diff --git a/tests/Units/TicketTest.php b/tests/Units/TicketTest.php index 6170a1c7..3f760772 100644 --- a/tests/Units/TicketTest.php +++ b/tests/Units/TicketTest.php @@ -480,6 +480,61 @@ public function testStatusTicketOption() $this->assertEquals(CommonITILObject::ASSIGNED, $ticket->fields['status']); } + // Pins core's status preservation on simultaneous actor removal/addition for MANAGED_BY_CORE. + public function testStatusPreservedOnGroupReassignmentWhenManagedByCore() + { + $this->initConfig([ + 'remove_group' => 1, + 'ticket_last_status' => PluginEscaladeTicket::MANAGED_BY_CORE, + ]); + + $group1 = $this->createItem('Group', [ + 'name' => 'Group reassign 1_' . uniqid(), + 'entities_id' => 0, + 'is_recursive' => 1, + ]); + $group2 = $this->createItem('Group', [ + 'name' => 'Group reassign 2_' . uniqid(), + 'entities_id' => 0, + 'is_recursive' => 1, + ]); + + $ticket = $this->createItem('Ticket', [ + 'name' => 'Test ticket for status preservation', + 'content' => 'Content', + 'entities_id' => 0, + '_actors' => [ + 'assign' => [ + [ + 'items_id' => $group1->getID(), + 'itemtype' => 'Group', + ], + ], + ], + ]); + $ticket_id = $ticket->getID(); + $ticket->getFromDB($ticket_id); + $this->assertEquals(CommonITILObject::ASSIGNED, $ticket->fields['status']); + + $this->updateItem(Ticket::class, $ticket_id, [ + '_actors' => [ + 'assign' => [ + [ + 'items_id' => $group2->getID(), + 'itemtype' => 'Group', + ], + ], + ], + ]); + + $group_ticket = new Group_Ticket(); + $this->assertEquals(0, count($group_ticket->find(['tickets_id' => $ticket_id, 'groups_id' => $group1->getID(), 'type' => CommonITILActor::ASSIGN]))); + $this->assertEquals(1, count($group_ticket->find(['tickets_id' => $ticket_id, 'groups_id' => $group2->getID(), 'type' => CommonITILActor::ASSIGN]))); + + $ticket->getFromDB($ticket_id); + $this->assertEquals(CommonITILObject::ASSIGNED, $ticket->fields['status']); + } + private function testAssignGroupToTicketWithCategoryProvider() { yield [