Skip to content

Commit 7a592a3

Browse files
LainowRom1-B
Lainow
and
Rom1-B
authored
Fixed a solution added even though the required fields were not filled in (#7)
* Fixed a solution added even though the required fields were not filled in * Fix PHP CS * Fixed a solution added even though the required fields were not filled in * Fix PHPStan * Fix is solution * Update src/Controller.php Co-authored-by: Romain B. <8530352+Rom1-B@users.noreply.github.com> * Add suggestions * Implements suggestions and fix double call after adding solution * Check if set_solution_tech is enabled * Fix lint * Fix lint --------- Co-authored-by: Romain B. <8530352+Rom1-B@users.noreply.github.com>
1 parent e4113a7 commit 7a592a3

4 files changed

Lines changed: 156 additions & 68 deletions

File tree

‎hook.php‎

Lines changed: 0 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -31,43 +31,8 @@
3131
* -------------------------------------------------------------------------
3232
*/
3333

34-
use Glpi\Inventory\Conf;
3534
use GlpiPlugin\Moreoptions\Config;
3635

37-
/**
38-
* -------------------------------------------------------------------------
39-
* MoreOptions plugin for GLPI
40-
* -------------------------------------------------------------------------
41-
*
42-
* MIT License
43-
*
44-
* Permission is hereby granted, free of charge, to any person obtaining a copy
45-
* of this software and associated documentation files (the "Software"), to deal
46-
* in the Software without restriction, including without limitation the rights
47-
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
48-
* copies of the Software, and to permit persons to whom the Software is
49-
* furnished to do so, subject to the following conditions:
50-
*
51-
* The above copyright notice and this permission notice shall be included in all
52-
* copies or substantial portions of the Software.
53-
*
54-
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
55-
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
56-
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
57-
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
58-
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
59-
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
60-
* SOFTWARE.
61-
* -------------------------------------------------------------------------
62-
* @copyright Copyright (C) 2025 by the MoreOptions plugin team.
63-
* @copyright Copyright (C) 2022-2024 by More Options plugin team.
64-
* @license MIT https://opensource.org/licenses/mit-license.php
65-
* @license GPLv3 https://www.gnu.org/licenses/gpl-3.0.html
66-
* @link https://github.com/pluginsGLPI/moreoptions
67-
* @link https://gitlab.teclib.com/glpi-network/cancelsend/
68-
* -------------------------------------------------------------------------
69-
*/
70-
7136
function plugin_moreoptions_install(): bool
7237
{
7338
$migration = new Migration(PLUGIN_MOREOPTIONS_VERSION);

‎setup.php‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,13 @@ function plugin_init_moreoptions(): void
9696
Controller::class, 'beforeCloseITILObject',
9797
];
9898

99+
$PLUGIN_HOOKS[Hooks::PRE_ITEM_ADD]['moreoptions'][ITILSolution::class] = [
100+
Controller::class, 'beforeCloseITILObject',
101+
];
102+
$PLUGIN_HOOKS[Hooks::PRE_ITEM_UPDATE]['moreoptions'][ITILSolution::class] = [
103+
Controller::class, 'beforeCloseITILObject',
104+
];
105+
99106
$PLUGIN_HOOKS[Hooks::PRE_ITEM_UPDATE]['moreoptions'][Config::class] = [
100107
Config::class, 'preItemUpdate',
101108
];

‎src/Controller.php‎

Lines changed: 61 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,6 @@
4747
use CommonITILActor;
4848
use CommonITILObject;
4949
use CommonITILValidation;
50-
use Glpi\Form\Category;
5150
use GlpiPlugin\Moreoptions\Config;
5251
use Group_Item;
5352
use Group_Problem;
@@ -70,6 +69,7 @@ class Controller extends CommonDBTM
7069
{
7170
public $dohistory = true;
7271
public static $rightname = 'config';
72+
private static bool $solution_check_done = false;
7373
public static function getTypeName($nb = 0): string
7474
{
7575
return __s("Controller", "moreoptions");
@@ -271,27 +271,48 @@ private static function addGroupsForActorType(CommonDBTM $item, Config $moconfig
271271
}
272272
}
273273

274-
public static function beforeCloseITILObject(CommonITILObject $item): void
274+
public static function beforeCloseITILObject(CommonDBTM $item): void
275275
{
276276
if (!is_array($item->input)) {
277277
return;
278278
}
279279

280-
if (
281-
(isset($item->input['status']) && ($item->input['status'] == CommonITILObject::CLOSED || $item->input['status'] == CommonITILObject::SOLVED))
282-
|| $item->fields['status'] == CommonITILObject::CLOSED
283-
|| $item->fields['status'] == CommonITILObject::SOLVED
280+
$closed = true;
281+
282+
if ($item instanceof ITILSolution) {
283+
$itemtype = $item->input['itemtype'] ?? null;
284+
285+
$parent_item = getItemForItemType($itemtype);
286+
287+
if (!$parent_item || !$parent_item->getFromDB($item->input['items_id'])) {
288+
return;
289+
}
290+
291+
$closed = self::requireFieldsToClose($parent_item, true);
292+
$closed = self::preventClosure($parent_item) && $closed;
293+
self::$solution_check_done = true;
294+
} elseif (
295+
$item instanceof CommonITILObject
296+
&& isset($item->input['status'])
297+
&& ($item->input['status'] == CommonITILObject::CLOSED || $item->input['status'] == CommonITILObject::SOLVED)
284298
) {
285-
self::requireFieldsToClose($item);
286-
self::preventClosure($item);
299+
if (self::$solution_check_done && $item->input['status'] == CommonITILObject::SOLVED) {
300+
return;
301+
}
302+
$closed = self::requireFieldsToClose($item);
303+
$closed = self::preventClosure($item) && $closed;
304+
}
305+
306+
if (!$closed) {
307+
$item->input = false;
287308
}
288309
}
289310

290-
public static function preventClosure(CommonDBTM $item): void
311+
public static function preventClosure(CommonDBTM $item): bool
291312
{
292313
$conf = Config::getConfig();
293314
if ($conf->fields['is_active'] != 1) {
294-
return;
315+
return true;
295316
}
296317

297318
$tasks = [];
@@ -319,21 +340,24 @@ public static function preventClosure(CommonDBTM $item): void
319340
if (is_array($t) && isset($t['state']) && $t['state'] == Planning::TODO) {
320341
Session::addMessageAfterRedirect(__s('The ticket you wish to close has tasks that need to be completed.', 'moreoptions'), false, ERROR);
321342
$item->input = false;
322-
return;
343+
return false;
323344
}
324345
}
346+
return true;
325347
}
326348

327-
public static function requireFieldsToClose(CommonDBTM $item): void
349+
public static function requireFieldsToClose(CommonDBTM $item, bool $is_solution = false): bool
328350
{
329351
$conf = Config::getConfig();
330352
if ($conf->fields['is_active'] != 1) {
331-
return;
353+
return true;
332354
}
333355

334356
$message = '';
335357
$itemtype = get_class($item);
336358

359+
$data = array_merge($item->fields, is_array($item->input) ? $item->input : []);
360+
337361
// Determine the configuration suffix and actor classes based on item type
338362
$configSuffix = '_' . strtolower($itemtype);
339363
$userClass = $item->userlinkclass ?? '';
@@ -342,18 +366,20 @@ public static function requireFieldsToClose(CommonDBTM $item): void
342366

343367
// Check for required technician
344368
if ($conf->fields['require_technician_to_close' . $configSuffix] == 1) {
345-
if (is_a($userClass, CommonDBTM::class, true)) {
369+
if ($is_solution && $itemtype === 'Ticket' && !empty($_SESSION['glpiset_solution_tech'])) {
370+
// GLPI will auto-assign the solution author as technician in post_addItem
371+
} elseif (is_a($userClass, CommonDBTM::class, true)) {
346372
$tech = new $userClass();
373+
$techs = $tech->find([
374+
$itemIdField => $data['id'],
375+
'type' => CommonITILActor::ASSIGN,
376+
]);
377+
if (count($techs) == 0) {
378+
$message .= '- ' . __s('Technician') . '<br>';
379+
}
347380
} else {
348381
// If the user class is not valid, skip this check
349-
return;
350-
}
351-
$techs = $tech->find([
352-
$itemIdField => $item->fields['id'],
353-
'type' => CommonITILActor::ASSIGN,
354-
]);
355-
if (count($techs) == 0) {
356-
$message .= '- ' . __s('Technician') . '<br>';
382+
return false;
357383
}
358384
}
359385

@@ -363,10 +389,10 @@ public static function requireFieldsToClose(CommonDBTM $item): void
363389
$group = new $groupClass();
364390
} else {
365391
// If the group class is not valid, skip this check
366-
return;
392+
return false;
367393
}
368394
$groups = $group->find([
369-
$itemIdField => $item->fields['id'],
395+
$itemIdField => $data['id'],
370396
'type' => CommonITILActor::ASSIGN,
371397
]);
372398
if (count($groups) == 0) {
@@ -376,27 +402,29 @@ public static function requireFieldsToClose(CommonDBTM $item): void
376402

377403
// Check for required category
378404
if ($conf->fields['require_category_to_close' . $configSuffix] == 1) {
379-
if ((!isset($item->input['itilcategories_id']) || empty($item->input['itilcategories_id']))) {
405+
if ((!isset($data['itilcategories_id']) || empty($data['itilcategories_id']))) {
380406
$message .= '- ' . __s('Category') . '<br>';
381407
}
382408
}
383409

384410
// Check for required location
385411
if ($conf->fields['require_location_to_close' . $configSuffix] == 1) {
386-
if ((!isset($item->input['locations_id']) || empty($item->input['locations_id']))) {
412+
if ((!isset($data['locations_id']) || empty($data['locations_id']))) {
387413
$message .= '- ' . __s('Location') . '<br>';
388414
}
389415
}
390416

391417
// Check if solution exists before closing
392-
if ($conf->fields['require_solution_to_close' . $configSuffix] == 1
393-
&& is_array($item->input)
394-
&& isset($item->input['status'])
395-
&& $item->input['status'] == CommonITILObject::CLOSED) {
418+
if (
419+
!$is_solution
420+
&& $conf->fields['require_solution_to_close' . $configSuffix] == 1
421+
&& isset($data['status'])
422+
&& $data['status'] == CommonITILObject::CLOSED
423+
) {
396424
$solution = new ITILSolution();
397425
$solutions = $solution->find([
398426
'itemtype' => $itemtype,
399-
'items_id' => $item->fields['id'],
427+
'items_id' => $data['id'],
400428
'NOT' => [
401429
'status' => CommonITILValidation::REFUSED,
402430
],
@@ -411,9 +439,9 @@ public static function requireFieldsToClose(CommonDBTM $item): void
411439

412440
$message = sprintf(__s('To close this %s, you must fill in the following fields:', 'moreoptions'), $itemTypeLabel) . '<br>' . $message;
413441
Session::addMessageAfterRedirect($message, false, ERROR);
414-
$item->input = false;
415-
return;
442+
return false;
416443
}
444+
return true;
417445
}
418446

419447
public static function checkTaskRequirements(CommonDBTM $item): CommonDBTM

‎tests/Units/ConfigTest.php‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -306,6 +306,94 @@ public function testTicketMandatoryFieldsBeforeCloseTicket(): void
306306
$this->assertTrue($resetResult);
307307
}
308308

309+
/**
310+
* Test mandatory fields before adding a solution
311+
*/
312+
public function testCannotAddSolutionWhenMissingMandatoryFields(): void
313+
{
314+
$this->login();
315+
316+
$conf = $this->getCurrentConfig();
317+
318+
// Configure mandatory fields before closing (which impacts solutions too)
319+
$result = $this->updateTestConfig($conf, [
320+
'is_active' => 1,
321+
'entities_id' => 0,
322+
'require_technician_to_close_ticket' => 1,
323+
'require_category_to_close_ticket' => 1,
324+
]);
325+
$this->assertTrue($result);
326+
327+
// Create a ticket without mandatory fields
328+
$ticket = $this->createItem(
329+
\Ticket::class,
330+
[
331+
'name' => 'Test ticket solution',
332+
'content' => 'Test content',
333+
],
334+
);
335+
$tid = $ticket->getID();
336+
337+
// Attempt to add a solution (Expected to fail because missing tech and category)
338+
$solution = new \ITILSolution();
339+
$resultFields = $solution->add([
340+
'itemtype' => \Ticket::class,
341+
'items_id' => $tid,
342+
'content' => 'My test solution',
343+
'status' => \CommonITILObject::SOLVED,
344+
]);
345+
346+
$this->clearSessionMessages();
347+
$this->assertFalse($resultFields);
348+
349+
// Add technician to the ticket
350+
$user = new \User();
351+
$this->assertTrue($user->getFromDBByCrit(['name' => 'glpi']));
352+
353+
$this->createItem(
354+
\Ticket_User::class,
355+
[
356+
'tickets_id' => $tid,
357+
'users_id' => $user->getID(),
358+
'type' => \Ticket_User::ASSIGN,
359+
],
360+
);
361+
362+
// Create category and update ticket
363+
$category = $this->createItem(
364+
\ITILCategory::class,
365+
[
366+
'name' => 'Test category for solution test',
367+
],
368+
);
369+
$this->updateItem(
370+
\Ticket::class,
371+
$tid,
372+
[
373+
'itilcategories_id' => $category->getID(),
374+
],
375+
);
376+
377+
// Attempt to add solution with all mandatory fields present (Expected to succeed)
378+
$solution2 = new \ITILSolution();
379+
$resultOk = $solution2->add([
380+
'itemtype' => \Ticket::class,
381+
'items_id' => $tid,
382+
'solutiontypes_id' => 0,
383+
'content' => 'My test solution with fields ok',
384+
'status' => \CommonITILObject::SOLVED,
385+
]);
386+
$this->assertIsInt($resultOk);
387+
$this->clearSessionMessages();
388+
389+
// Reset config
390+
$resetResult = $this->updateTestConfig($conf, [
391+
'require_technician_to_close_ticket' => 0,
392+
'require_category_to_close_ticket' => 0,
393+
]);
394+
$this->assertTrue($resetResult);
395+
}
396+
309397
/**
310398
* Test mandatory fields before closing a change
311399
*/

0 commit comments

Comments
 (0)