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] 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..2f5100b 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; @@ -298,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); @@ -345,11 +353,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 +385,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,37 +399,37 @@ 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'); } } - // 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([ @@ -426,20 +440,79 @@ 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. + * + * @param array $params + */ + public static function showSolutionRequirementsWarning(array $params): void + { + $item = $params['item'] ?? null; + if (!($item instanceof CommonITILObject) || !$item->canSolve()) { + return; + } + + $missing = self::getMissingCloseFields($item, true); + if (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 +551,57 @@ public static function checkTaskRequirements(CommonDBTM $item): CommonDBTM return $item; } + /** + * 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. + * + * 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 + { + $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..2c20063 --- /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. + #} + + diff --git a/tests/Units/ConfigTest.php b/tests/Units/ConfigTest.php index a0d4d1d..8ce79e3 100644 --- a/tests/Units/ConfigTest.php +++ b/tests/Units/ConfigTest.php @@ -392,6 +392,78 @@ 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). + // `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, + '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. + // '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 + $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 */