Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
30 changes: 7 additions & 23 deletions inc/ticket.class.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Comment thread
stonebuzz marked this conversation as resolved.
// 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([
Expand Down Expand Up @@ -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,
Expand Down
134 changes: 134 additions & 0 deletions tests/Units/ActorDiffReplacementTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
<?php

/**
* -------------------------------------------------------------------------
* Escalade plugin for GLPI
* -------------------------------------------------------------------------
*
* LICENSE
*
* This file is part of Escalade.
*
* Escalade is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* Escalade is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with Escalade. If not, see <http://www.gnu.org/licenses/>.
* -------------------------------------------------------------------------
* @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");
}
}
10 changes: 3 additions & 7 deletions tests/Units/GroupEscalationTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand All @@ -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");
}

Expand Down
55 changes: 55 additions & 0 deletions tests/Units/TicketTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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 [
Expand Down