From f6653f93a00997a8b30404d28d2bea657a5aaaa8 Mon Sep 17 00:00:00 2001 From: Lainow Date: Tue, 15 Sep 2026 17:07:46 +0200 Subject: [PATCH 1/8] Fix solution content disapeared --- setup.php | 11 ++ src/Controller.php | 135 ++++++++++++++++-- templates/timeline_solution_warning.html.twig | 114 +++++++++++++++ .../timeline_task_mandatory_fields.html.twig | 81 +++++++++++ 4 files changed, 330 insertions(+), 11 deletions(-) create mode 100644 templates/timeline_solution_warning.html.twig create mode 100644 templates/timeline_task_mandatory_fields.html.twig diff --git a/setup.php b/setup.php index 22928a6..2ad112c 100644 --- a/setup.php +++ b/setup.php @@ -84,6 +84,17 @@ function plugin_init_moreoptions(): void Controller::class, 'requireFieldsToClose', ]; + // Both hooks below are called by GLPI core with an array of parameters (not an item + // instance), so they must be registered without an itemtype key: the callback filters + // on $params['item'] itself. + $PLUGIN_HOOKS[Hooks::TIMELINE_ACTIONS]['moreoptions'] = [ + Controller::class, 'showSolutionRequirementsWarning', + ]; + + $PLUGIN_HOOKS[Hooks::POST_ITEM_FORM]['moreoptions'] = [ + Controller::class, 'markMandatoryTaskFields', + ]; + $PLUGIN_HOOKS[Hooks::PRE_ITEM_UPDATE]['moreoptions'][Ticket::class] = [ Controller::class, 'beforeCloseITILObject', ]; diff --git a/src/Controller.php b/src/Controller.php index 70ad728..2dead56 100644 --- a/src/Controller.php +++ b/src/Controller.php @@ -47,6 +47,7 @@ use CommonITILActor; use CommonITILObject; use CommonITILValidation; +use Glpi\Application\View\TemplateRenderer; use GlpiPlugin\Moreoptions\Config; use Group_Item; use Group_Problem; @@ -345,11 +346,17 @@ public static function preventClosure(CommonDBTM $item): bool return true; } - public static function requireFieldsToClose(CommonDBTM $item, bool $is_solution = false): bool + /** + * Determine which fields configured as required to close are missing on the given item. + * + * @return string[]|null Labels of the missing fields, an empty array if none are missing, + * or null if the check could not be performed (invalid actor class). + */ + private static function getMissingCloseFields(CommonDBTM $item, bool $is_solution): ?array { $conf = Config::getConfig(); - $message = ''; + $missing = []; $itemtype = get_class($item); $data = array_merge($item->fields, is_array($item->input) ? $item->input : []); @@ -371,11 +378,11 @@ public static function requireFieldsToClose(CommonDBTM $item, bool $is_solution 'type' => CommonITILActor::ASSIGN, ]); if (count($techs) == 0) { - $message .= '- ' . __s('Technician') . '
'; + $missing[] = __s('Technician'); } } else { // If the user class is not valid, skip this check - return false; + return null; } } @@ -385,28 +392,28 @@ public static function requireFieldsToClose(CommonDBTM $item, bool $is_solution $group = new $groupClass(); } else { // If the group class is not valid, skip this check - return false; + return null; } $groups = $group->find([ $itemIdField => $data['id'], 'type' => CommonITILActor::ASSIGN, ]); if (count($groups) == 0) { - $message .= '- ' . __s('Technician group') . '
'; + $missing[] = __s('Technician group'); } } // Check for required category if ($conf->fields['require_category_to_close' . $configSuffix] == 1) { if ((!isset($data['itilcategories_id']) || empty($data['itilcategories_id']))) { - $message .= '- ' . __s('Category') . '
'; + $missing[] = __s('Category'); } } // Check for required location if ($conf->fields['require_location_to_close' . $configSuffix] == 1) { if ((!isset($data['locations_id']) || empty($data['locations_id']))) { - $message .= '- ' . __s('Location') . '
'; + $missing[] = __s('Location'); } } @@ -426,20 +433,77 @@ public static function requireFieldsToClose(CommonDBTM $item, bool $is_solution ], ]); if (count($solutions) == 0) { - $message .= '- ' . __s('Solution') . '
'; + $missing[] = __s('Solution'); } } - if (!empty($message)) { + return $missing; + } + + public static function requireFieldsToClose(CommonDBTM $item, bool $is_solution = false): bool + { + $missing = self::getMissingCloseFields($item, $is_solution); + + if ($missing === null) { + return false; + } + + if (!empty($missing)) { $itemTypeLabel = $item->getTypeName(); - $message = sprintf(__s('To close this %s, you must fill in the following fields:', 'moreoptions'), $itemTypeLabel) . '
' . $message; + $message = sprintf(__s('To close this %s, you must fill in the following fields:', 'moreoptions'), $itemTypeLabel) . '
'; + foreach ($missing as $field) { + $message .= '- ' . $field . '
'; + } Session::addMessageAfterRedirect($message, false, ERROR); return false; } return true; } + /** + * Hooked on {@link \Glpi\Plugin\Hooks::TIMELINE_ACTIONS}. Renders, into the ticket/change/ + * problem timeline footer, a script that mutes the "Add a solution" action, adds a lock icon + * to it, and attaches a popover listing the missing fields, as soon as one of the fields + * required to close the item (technician, group, category, location...) is missing. + * + * This is purely client-side: it does not replace the server-side block already performed by + * {@see self::beforeCloseITILObject()} on actual submission, it just gives the user a visual + * hint before they even open the solution form. + */ + public static function showSolutionRequirementsWarning(array $params): void + { + $item = $params['item'] ?? null; + if (!($item instanceof CommonITILObject) || !$item->canSolve()) { + return; + } + + $missing = self::getMissingCloseFields($item, true); + if ($missing === null || empty($missing)) { + // Nothing configured as required, or everything is already filled: let the + // normal "Add a solution" action be usable. + return; + } + + $count = count($missing); + $header = sprintf( + _n( + '%1$d required field is missing, so this %2$s can\'t be solved yet.', + '%1$d required fields are missing, so this %2$s can\'t be solved yet.', + $count, + 'moreoptions' + ), + $count, + $item->getTypeName(1) + ); + + TemplateRenderer::getInstance()->display('@moreoptions/timeline_solution_warning.html.twig', [ + 'marker_id' => 'moreoptions-solution-warning-' . $item->getType() . '-' . $item->getID(), + 'header' => $header, + 'missing_fields' => $missing, + ]); + } + public static function checkTaskRequirements(CommonDBTM $item): CommonDBTM { $conf = Config::getConfig(); @@ -478,6 +542,55 @@ public static function checkTaskRequirements(CommonDBTM $item): CommonDBTM return $item; } + /** + * Hooked on {@link \Glpi\Plugin\Hooks::POST_ITEM_FORM}. Echoes, into the task creation/edit + * form, a script that marks the fields configured as mandatory in moreoptions (category, + * duration, technician, technician group) with the usual red "required" marker and blocks + * client-side submission of the form until they are filled. + * + * The server-side block already performed by {@see self::checkTaskRequirements()} on actual + * submission (PRE_ITEM_ADD) is kept as-is; this only prevents the user from submitting an + * incomplete task in the first place. + */ + public static function markMandatoryTaskFields(array $params): void + { + $item = $params['item'] ?? null; + if ( + !($item instanceof TicketTask) + && !($item instanceof ChangeTask) + && !($item instanceof ProblemTask) + ) { + return; + } + + $conf = Config::getConfig(); + + $labels = []; + if ($conf->fields['mandatory_task_category'] == 1) { + $labels['taskcategories_id'] = __('Category'); + } + if ($conf->fields['mandatory_task_duration'] == 1) { + $labels['actiontime'] = __('Duration'); + } + if ($conf->fields['mandatory_task_user'] == 1) { + $labels['users_id_tech'] = __('User'); + } + if ($conf->fields['mandatory_task_group'] == 1) { + $labels['groups_id_tech'] = __('Group'); + } + + if (empty($labels)) { + return; + } + + TemplateRenderer::getInstance()->display('@moreoptions/timeline_task_mandatory_fields.html.twig', [ + // Unique per-call anchor: lets the injected script find its own
reliably. + 'marker_id' => 'moreoptions-task-mandatory-' . bin2hex(random_bytes(6)), + 'field_labels' => $labels, + 'error_message' => __('To create this task, you must fill in the following fields:', 'moreoptions'), + ]); + } + public static function updateItemActors(CommonITILObject $item): CommonITILObject { $conf = Config::getConfig(); diff --git a/templates/timeline_solution_warning.html.twig b/templates/timeline_solution_warning.html.twig new file mode 100644 index 0000000..91064ff --- /dev/null +++ b/templates/timeline_solution_warning.html.twig @@ -0,0 +1,114 @@ +{# + # ------------------------------------------------------------------------- + # MoreOptions plugin for GLPI + # ------------------------------------------------------------------------- + # + # MIT License + # + # Permission is hereby granted, free of charge, to any person obtaining a copy + # of this software and associated documentation files (the "Software"), to deal + # in the Software without restriction, including without limitation the rights + # to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + # copies of the Software, and to permit persons to whom the Software is + # furnished to do so, subject to the following conditions: + # + # The above copyright notice and this permission notice shall be included in all + # copies or substantial portions of the Software. + # + # THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + # IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + # FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + # AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + # LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + # SOFTWARE. + # ------------------------------------------------------------------------- + # @copyright Copyright (C) 2025 by the MoreOptions plugin team. + # @license MIT https://opensource.org/licenses/mit-license.php + # @link https://github.com/pluginsGLPI/moreoptions + # ------------------------------------------------------------------------- + #} + +{# + # Rendered through the core `Hooks::TIMELINE_ACTIONS` hook (see Controller::showSolutionRequirementsWarning). + # We cannot alter the "Add a solution" action markup itself (rendered by GLPI core), so this + # only drops an invisible anchor + script that finds it and adjusts it client-side: mute its + # label, append a lock icon, and attach a tooltip listing the missing fields. + #} +{# + # Bootstrap's tooltip sanitizer strips `style` attributes from HTML content (its default + # allowlist has no entry for it), so text alignment has to go through an actual CSS class + # instead of an inline style, which would silently be removed. + #} + + +{% set tooltip_content %} +
+ +
+
{{ header }}
+
    + {% for field in missing_fields %} +
  • {{ field }}
    • + {% endfor %} +
+
+
+{% endset %} + + + diff --git a/templates/timeline_task_mandatory_fields.html.twig b/templates/timeline_task_mandatory_fields.html.twig new file mode 100644 index 0000000..fe1b43f --- /dev/null +++ b/templates/timeline_task_mandatory_fields.html.twig @@ -0,0 +1,81 @@ +{# + # ------------------------------------------------------------------------- + # MoreOptions plugin for GLPI + # ------------------------------------------------------------------------- + # + # MIT License + # + # Permission is hereby granted, free of charge, to any person obtaining a copy + # of this software and associated documentation files (the "Software"), to deal + # in the Software without restriction, including without limitation the rights + # to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + # copies of the Software, and to permit persons to whom the Software is + # furnished to do so, subject to the following conditions: + # + # The above copyright notice and this permission notice shall be included in all + # copies or substantial portions of the Software. + # + # THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + # IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + # FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + # AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + # LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + # SOFTWARE. + # ------------------------------------------------------------------------- + # @copyright Copyright (C) 2025 by the MoreOptions plugin team. + # @license MIT https://opensource.org/licenses/mit-license.php + # @link https://github.com/pluginsGLPI/moreoptions + # ------------------------------------------------------------------------- + #} + +{# + # Rendered through the core `Hooks::POST_ITEM_FORM` hook (see Controller::markMandatoryTaskFields), + # right before the task form's closing tag. Marks the configured fields as required (red asterisk + # on their label, matching GLPI's own convention) and blocks client-side submission until they + # are filled. + #} + + From 8a53dab9a260353ef83625498bee6e9db2b50af2 Mon Sep 17 00:00:00 2001 From: Lainow Date: Wed, 16 Sep 2026 10:54:23 +0200 Subject: [PATCH 2/8] Fix solve ticket without solution --- src/Controller.php | 4 ++-- templates/timeline_solution_warning.html.twig | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Controller.php b/src/Controller.php index 2dead56..df733c3 100644 --- a/src/Controller.php +++ b/src/Controller.php @@ -417,12 +417,12 @@ private static function getMissingCloseFields(CommonDBTM $item, bool $is_solutio } } - // Check if solution exists before closing + // Check if solution exists before resolving/closing. if ( !$is_solution && $conf->fields['require_solution_to_close' . $configSuffix] == 1 && isset($data['status']) - && $data['status'] == CommonITILObject::CLOSED + && in_array($data['status'], [CommonITILObject::SOLVED, CommonITILObject::CLOSED], true) ) { $solution = new ITILSolution(); $solutions = $solution->find([ diff --git a/templates/timeline_solution_warning.html.twig b/templates/timeline_solution_warning.html.twig index 91064ff..2c20063 100644 --- a/templates/timeline_solution_warning.html.twig +++ b/templates/timeline_solution_warning.html.twig @@ -48,7 +48,7 @@ {% set tooltip_content %}
- +
{{ header }}
    From b0900dd0a99fc6ac4431d03b93cc4b44251b4485 Mon Sep 17 00:00:00 2001 From: Lainow Date: Wed, 16 Sep 2026 10:58:39 +0200 Subject: [PATCH 3/8] Update CHangelog --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d8ad23..0693377 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,4 +5,9 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/) and this project adheres to [Semantic Versioning](http://semver.org/). +## [unreleased] + +### Fixed +- Fixed the issue where a ticket could be solved without a solution + ## [1.0.0-rc1] From 6489284f59864236a3537aaa46e9ffda83bab7a8 Mon Sep 17 00:00:00 2001 From: Lainow Date: Wed, 16 Sep 2026 11:17:14 +0200 Subject: [PATCH 4/8] Fix lint --- src/Controller.php | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/Controller.php b/src/Controller.php index df733c3..e794379 100644 --- a/src/Controller.php +++ b/src/Controller.php @@ -470,6 +470,8 @@ public static function requireFieldsToClose(CommonDBTM $item, bool $is_solution * This is purely client-side: it does not replace the server-side block already performed by * {@see self::beforeCloseITILObject()} on actual submission, it just gives the user a visual * hint before they even open the solution form. + * + * @param array $params */ public static function showSolutionRequirementsWarning(array $params): void { @@ -491,10 +493,10 @@ public static function showSolutionRequirementsWarning(array $params): void '%1$d required field is missing, so this %2$s can\'t be solved yet.', '%1$d required fields are missing, so this %2$s can\'t be solved yet.', $count, - 'moreoptions' + 'moreoptions', ), $count, - $item->getTypeName(1) + $item->getTypeName(1), ); TemplateRenderer::getInstance()->display('@moreoptions/timeline_solution_warning.html.twig', [ @@ -543,7 +545,7 @@ public static function checkTaskRequirements(CommonDBTM $item): CommonDBTM } /** - * Hooked on {@link \Glpi\Plugin\Hooks::POST_ITEM_FORM}. Echoes, into the task creation/edit + * Hooked on {@link \Glpi\Plugin\Hooks::POST_ITEM_FORM}. Renders, into the task creation/edit * form, a script that marks the fields configured as mandatory in moreoptions (category, * duration, technician, technician group) with the usual red "required" marker and blocks * client-side submission of the form until they are filled. @@ -551,6 +553,8 @@ public static function checkTaskRequirements(CommonDBTM $item): CommonDBTM * The server-side block already performed by {@see self::checkTaskRequirements()} on actual * submission (PRE_ITEM_ADD) is kept as-is; this only prevents the user from submitting an * incomplete task in the first place. + * + * @param array $params */ public static function markMandatoryTaskFields(array $params): void { From 374c70fa4c4770726cdde4830481da66e2e83529 Mon Sep 17 00:00:00 2001 From: Lainow Date: Wed, 16 Sep 2026 11:26:54 +0200 Subject: [PATCH 5/8] Add units tests --- tests/Units/ConfigTest.php | 66 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/tests/Units/ConfigTest.php b/tests/Units/ConfigTest.php index a0d4d1d..bc9d178 100644 --- a/tests/Units/ConfigTest.php +++ b/tests/Units/ConfigTest.php @@ -392,6 +392,72 @@ public function testCannotAddSolutionWhenMissingMandatoryFields(): void $this->assertTrue($resetResult); } + /** + * Test that a ticket cannot be resolved (status set to Solved) without an existing + * solution, but that adding a solution - which resolves the ticket as a side effect - + * is not itself blocked by that same requirement + */ + public function testCannotResolveTicketWithoutSolution(): void + { + $this->login(); + + $conf = $this->getCurrentConfig(); + + // Require a solution before resolving/closing a ticket + $result = $this->updateTestConfig($conf, [ + 'entities_id' => 0, + 'require_solution_to_close_ticket' => 1, + ]); + $this->assertTrue($result); + + // Create a ticket + $ticket = $this->createItem( + \Ticket::class, + [ + 'name' => 'Test ticket resolve without solution', + 'content' => 'Test content', + ], + ); + $tid = $ticket->getID(); + + // Directly set the status to Solved, without any solution (Expected to fail) + $ticket = new \Ticket(); + $result = $ticket->update([ + 'id' => $tid, + 'status' => \Ticket::SOLVED, + ]); + $this->assertFalse($result); + $this->clearSessionMessages(); + + // The status must not actually have changed in DB + $ticket = new \Ticket(); + $this->assertTrue($ticket->getFromDB($tid)); + $this->assertNotEquals(\Ticket::SOLVED, $ticket->fields['status']); + + // Add a solution (Expected to succeed): the parent ticket is resolved as a side + // effect of this, and that resulting status change must not be blocked + $solution = new \ITILSolution(); + $resultSolution = $solution->add([ + 'itemtype' => \Ticket::class, + 'items_id' => $tid, + 'content' => 'My test solution', + 'status' => \CommonITILObject::SOLVED, + ]); + $this->assertIsInt($resultSolution); + $this->clearSessionMessages(); + + // The ticket must now actually be Solved + $ticket = new \Ticket(); + $this->assertTrue($ticket->getFromDB($tid)); + $this->assertEquals(\Ticket::SOLVED, $ticket->fields['status']); + + // Reset config + $resetResult = $this->updateTestConfig($conf, [ + 'require_solution_to_close_ticket' => 0, + ]); + $this->assertTrue($resetResult); + } + /** * Test mandatory fields before closing a change */ From 179914d188a3ca182d5d34e4d4e551310578fe72 Mon Sep 17 00:00:00 2001 From: Lainow Date: Wed, 16 Sep 2026 11:41:35 +0200 Subject: [PATCH 6/8] Fix units tests --- src/Controller.php | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/Controller.php b/src/Controller.php index e794379..40cc113 100644 --- a/src/Controller.php +++ b/src/Controller.php @@ -299,7 +299,14 @@ public static function beforeCloseITILObject(CommonDBTM $item): void && isset($item->input['status']) && ($item->input['status'] == CommonITILObject::CLOSED || $item->input['status'] == CommonITILObject::SOLVED) ) { - if (self::$solution_check_done && $item->input['status'] == CommonITILObject::SOLVED) { + // Consume the one-shot bypass as soon as it is read: it is only meant to let + // through the single status update that ITILSolution::post_addItem() performs + // on its parent right after the solution itself was validated and saved. Leaving + // it at `true` would silently skip this check for every later status change too. + $bypass = self::$solution_check_done; + self::$solution_check_done = false; + + if ($bypass && $item->input['status'] == CommonITILObject::SOLVED) { return; } $closed = self::requireFieldsToClose($item); From 258a13a6374a67423e118f9d57beaa0668055e39 Mon Sep 17 00:00:00 2001 From: Lainow Date: Wed, 16 Sep 2026 16:59:45 +0200 Subject: [PATCH 7/8] Fix tests --- tests/Units/ConfigTest.php | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/tests/Units/ConfigTest.php b/tests/Units/ConfigTest.php index bc9d178..8ce79e3 100644 --- a/tests/Units/ConfigTest.php +++ b/tests/Units/ConfigTest.php @@ -420,7 +420,9 @@ public function testCannotResolveTicketWithoutSolution(): void ); $tid = $ticket->getID(); - // Directly set the status to Solved, without any solution (Expected to fail) + // Directly set the status to Solved, without any solution (Expected to fail). + // `updateItem()` cannot be used here: it asserts the update succeeds, which is + // exactly what this step must NOT do. $ticket = new \Ticket(); $result = $ticket->update([ 'id' => $tid, @@ -435,15 +437,19 @@ public function testCannotResolveTicketWithoutSolution(): void $this->assertNotEquals(\Ticket::SOLVED, $ticket->fields['status']); // Add a solution (Expected to succeed): the parent ticket is resolved as a side - // effect of this, and that resulting status change must not be blocked - $solution = new \ITILSolution(); - $resultSolution = $solution->add([ - 'itemtype' => \Ticket::class, - 'items_id' => $tid, - 'content' => 'My test solution', - 'status' => \CommonITILObject::SOLVED, - ]); - $this->assertIsInt($resultSolution); + // effect of this, and that resulting status change must not be blocked. + // 'content' and 'status' are skipped from createItem()'s post-add field check, + // as ITILSolution may transform/recompute them (rich text, auto-acceptance...). + $this->createItem( + \ITILSolution::class, + [ + 'itemtype' => \Ticket::class, + 'items_id' => $tid, + 'content' => 'My test solution', + 'status' => \CommonITILObject::SOLVED, + ], + ['content', 'status'], + ); $this->clearSessionMessages(); // The ticket must now actually be Solved From 475bfe668d6c03fbac4470ed1a9bd64651444dc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Langlois=20Ga=C3=ABtan?= <64356364+MyvTsv@users.noreply.github.com> Date: Wed, 16 Sep 2026 17:22:16 +0200 Subject: [PATCH 8/8] Update src/Controller.php --- src/Controller.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Controller.php b/src/Controller.php index 40cc113..2f5100b 100644 --- a/src/Controller.php +++ b/src/Controller.php @@ -488,7 +488,7 @@ public static function showSolutionRequirementsWarning(array $params): void } $missing = self::getMissingCloseFields($item, true); - if ($missing === null || empty($missing)) { + if (empty($missing)) { // Nothing configured as required, or everything is already filled: let the // normal "Add a solution" action be usable. return;