Apply suggestions from code review

ccailly · ccailly · commit b4427ca87f12 · 2025-04-16T15:22:15.000+02:00
diff --git a/js/modules/Forms/RendererController.js b/js/modules/Forms/RendererController.js
@@ -124,7 +124,11 @@ export class GlpiFormRendererController
         });
 
         // Handle delegation form update
-        this.#initDelegationEventHandlers();
+        $(this.#target).on(
+            'change',
+            '[data-glpi-form-renderer-delegation-container] select[name="delegation_users_id"]',
+            (e) => this.#renderDelegation(e)
+        );
     }
 
     /**
@@ -452,15 +456,6 @@ export class GlpiFormRendererController
         ;
     }
 
-    #initDelegationEventHandlers()
-    {
-        $(this.#target)
-            .find('[data-glpi-form-renderer-delegation-container]')
-            .find('select[name="delegation_users_id"]')
-            .on('change', (e) => this.#renderDelegation(e))
-        ;
-    }
-
     async #renderDelegation()
     {
         const selected_user_id = $(this.#target)
@@ -472,17 +467,9 @@ export class GlpiFormRendererController
             'selected_user_id': selected_user_id,
         });
 
-        // Create a temporary element to parse the AJAX response
-        const $temp = $('<div>').html(response);
-        // Extract the inner content of the delegation container from the response
-        const innerContent = $temp.find('[data-glpi-form-renderer-delegation-container]').html();
-
         // Replace only the inner content of the delegation container
         $(this.#target)
             .find('[data-glpi-form-renderer-delegation-container]')
-            .html(innerContent);
-
-        // Re-init event handlers
-        this.#initDelegationEventHandlers();
+            .html(response);
     }
 }
diff --git a/src/Glpi/Controller/Form/DelegationController.php b/src/Glpi/Controller/Form/DelegationController.php
@@ -8,7 +8,6 @@
  * http://glpi-project.org
  *
  * @copyright 2015-2025 Teclib' and contributors.
- * @copyright 2003-2014 by the INDEPNET Development Team.
  * @licence   https://www.gnu.org/licenses/gpl-3.0.html
  *
  * ---------------------------------------------------------------------
diff --git a/src/Glpi/Controller/Form/SubmitAnswerController.php b/src/Glpi/Controller/Form/SubmitAnswerController.php
@@ -41,6 +41,7 @@
 use Glpi\Exception\Http\NotFoundHttpException;
 use Glpi\Form\AnswersHandler\AnswersHandler;
 use Glpi\Form\AnswersSet;
+use Glpi\Form\DelegationData;
 use Glpi\Form\EndUserInputNameProvider;
 use Glpi\Form\Form;
 use Glpi\Http\Firewall;
@@ -96,11 +97,11 @@ private function saveSubmittedAnswers(
         $post = $request->request->all();
         $provider = new EndUserInputNameProvider();
 
-        $delegation = [
-            'users_id'          => $post['delegation_users_id'] ?? null,
-            'use_notification'  => $post['delegation_use_notification'] ?? null,
-            'alternative_email' => $post['delegation_alternative_email'] ?? null,
-        ];
+        $delegation = new DelegationData(
+            $request->request->getInt('delegation_users_id', 0) ?: null,
+            $request->request->getBoolean('delegation_use_notification', false) ?: null,
+            $request->request->getString('delegation_alternative_email', '') ?: null
+        );
         $answers    = $provider->getAnswers($post);
         $files      = $provider->getFiles($post, $answers);
         if (empty($answers)) {
@@ -112,8 +113,8 @@ private function saveSubmittedAnswers(
             $form,
             $answers,
             Session::getLoginUserID(),
-            $files,
-            $delegation
+            $delegation,
+            $files
         );
 
         return $answers;
diff --git a/src/Glpi/Form/AnswersHandler/AnswersHandler.php b/src/Glpi/Form/AnswersHandler/AnswersHandler.php
@@ -40,6 +40,7 @@
 use Glpi\Form\AnswersSet;
 use Glpi\Form\Condition\Engine;
 use Glpi\Form\Condition\EngineInput;
+use Glpi\Form\DelegationData;
 use Glpi\Form\Destination\AnswersSet_FormDestinationItem;
 use Glpi\Form\Destination\FormDestination;
 use Glpi\Form\Form;
@@ -92,21 +93,21 @@ public function saveAnswers(
         Form $form,
         array $answers,
         int $users_id,
+        DelegationData $delegation,
         array $files = [],
-        array $delegation = []
     ): AnswersSet {
         /** @var \DBmysql $DB */
         global $DB;
 
         if ($DB->inTransaction()) {
-            return $this->doSaveAnswers($form, $answers, $users_id, $files, $delegation);
+            return $this->doSaveAnswers($form, $answers, $users_id, $delegation, $files);
         } else {
             // We do not want to commit the answers unless everything was processed
             // correctly
             $DB->beginTransaction();
 
             try {
-                $answers_set = $this->doSaveAnswers($form, $answers, $users_id, $files, $delegation);
+                $answers_set = $this->doSaveAnswers($form, $answers, $users_id, $delegation, $files);
                 $DB->commit();
                 return $answers_set;
             } catch (\Throwable $e) {
@@ -141,8 +142,8 @@ protected function doSaveAnswers(
         Form $form,
         array $answers,
         int $users_id,
+        DelegationData $delegation,
         array $files = [],
-        array $delegation = []
     ): AnswersSet {
         // Save answers
         $answers_set = $this->createAnswserSet(
diff --git a/src/Glpi/Form/AnswersSet.php b/src/Glpi/Form/AnswersSet.php
@@ -52,8 +52,7 @@ final class AnswersSet extends CommonDBChild
 
     public array $files = [];
 
-    /** @var array<string, ?string> $delegation */
-    public array $delegation = [];
+    public DelegationData $delegation;
 
     #[Override]
     public static function getTypeName($nb = 0)
@@ -271,19 +270,19 @@ public function setSubmittedFiles(array $files): void
     /**
      * Get delegation data
      *
-     * @return array<string, ?string>
+     * @return DelegationData
      */
-    public function getDelegation(): array
+    public function getDelegation(): DelegationData
     {
         return $this->delegation;
     }
 
     /**
      * Set delegation data
      *
-     * @param array<string, ?string> $delegation
+     * @param DelegationData $delegation
      */
-    public function setDelegation(array $delegation): void
+    public function setDelegation(DelegationData $delegation): void
     {
         $this->delegation = $delegation;
     }
diff --git a/src/Glpi/Form/DelegationData.php b/src/Glpi/Form/DelegationData.php
@@ -0,0 +1,45 @@
+<?php
+
+/**
+ * ---------------------------------------------------------------------
+ *
+ * GLPI - Gestionnaire Libre de Parc Informatique
+ *
+ * http://glpi-project.org
+ *
+ * @copyright 2015-2025 Teclib' and contributors.
+ * @licence   https://www.gnu.org/licenses/gpl-3.0.html
+ *
+ * ---------------------------------------------------------------------
+ *
+ * LICENSE
+ *
+ * This file is part of GLPI.
+ *
+ * This program 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 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program 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 this program.  If not, see <https://www.gnu.org/licenses/>.
+ *
+ * ---------------------------------------------------------------------
+ */
+
+namespace Glpi\Form;
+
+final class DelegationData
+{
+    public function __construct(
+        public readonly ?int $users_id,
+        public readonly ?bool $use_notification,
+        public readonly ?string $alternative_email
+    ) {
+    }
+}
diff --git a/src/Glpi/Form/Destination/CommonITILField/ITILActorFieldStrategy.php b/src/Glpi/Form/Destination/CommonITILField/ITILActorFieldStrategy.php
@@ -123,13 +123,13 @@ private function getActorsFromCurrentUser(AnswersSet $answers_set): ?array
         }
 
         $delegation = $answers_set->getDelegation();
-        if (Ticket::canDelegateeCreateTicket($delegation['users_id'] ?? $user_id)) {
+        if (Ticket::canDelegateeCreateTicket($delegation->users_id ?? $user_id)) {
             return [
                 [
                     'itemtype' => User::class,
-                    'items_id' => $delegation['users_id'] ?? $user_id,
-                    'use_notification' => $delegation['use_notification'] ?? 0,
-                    'alternative_email' => $delegation['alternative_email'] ?? '',
+                    'items_id' => $delegation->users_id ?? $user_id,
+                    'use_notification' => $delegation->use_notification ?? 0,
+                    'alternative_email' => $delegation->alternative_email ?? '',
                 ]
             ];
         } else {
@@ -144,7 +144,7 @@ private function getActorsFromCurrentUser(AnswersSet $answers_set): ?array
 
     private function getActorsFromSupervisorOfCurrentUser(AnswersSet $answers_set): ?array
     {
-        $users_id = $answers_set->getDelegation()['users_id'] ?? Session::getLoginUserID();
+        $users_id = $answers_set->getDelegation()->users_id ?? Session::getLoginUserID();
         if (!is_numeric($users_id)) {
             return null;
         }
diff --git a/src/User.php b/src/User.php
@@ -3697,6 +3697,36 @@ public static function getDelegateGroupsForUser($entities_id = '')
         return $groups;
     }
 
+    /**
+     * Get all users from groups where the current user have delegating, plus the current user.
+     *
+     * @param integer|string $entities_id ID of the entity to restrict
+     *
+     * @return array<int, string> Array of user IDs mapped to their friendly names, sorted alphabetically, with "Myself" first.
+     */
+    public static function getUsersFromDelegatedGroups($entities_id = ''): array
+    {
+        $groups_ids = self::getDelegateGroupsForUser($entities_id);
+        $users_data = [];
+        foreach ($groups_ids as $groups_id) {
+            $users_data = array_merge($users_data, Group_User::getGroupUsers($groups_id));
+        }
+
+        // Get unique user IDs from the collected data
+        $user_ids = array_unique(array_column($users_data, 'id'));
+
+        $formatted_users = [];
+        foreach ($user_ids as $user_id) {
+            // Avoid adding the current user if they are in the delegated groups, will be added later
+            if ($user_id !== Session::getLoginUserID()) {
+                $formatted_users[$user_id] = User::getFriendlyNameById($user_id);
+            }
+        }
+
+        uasort($formatted_users, 'strcasecmp');
+
+        return [Session::getLoginUserID() => __('Myself')] + $formatted_users;
+    }
 
     /**
      * Execute the query to select box with all glpi users where select key = name
diff --git a/templates/components/helpdesk_forms/delegation_alert.html.twig b/templates/components/helpdesk_forms/delegation_alert.html.twig
@@ -38,36 +38,26 @@
         {% set selected_user = get_current_user() %}
     {% endif %}
 
-    {% set users = get_current_user().getDelegateGroupsForUser()|map(
-        groups_id => call('Group_User::getGroupUsers', [groups_id])
-    )|reduce(
-        (carry, group_users) => carry|merge(group_users), []
-    ) %}
+    {% set users = get_current_user().getUsersFromDelegatedGroups() %}
+    {% set user_dropdown_html %}
+        {{ fields.dropdownArrayField(
+            'delegation_users_id',
+            selected_user.getID(),
+            users,
+            '',
+            {
+                'no_label'      : true,
+                'width'         : 'auto',
+                'field_class'   : '',
+                'mb'            : '',
+                'aria_label'    : __('Select the user to delegate'),
+                'wrapper_class' : 'd-inline-block',
+            }
+        ) }}
+    {% endset %}
 
-    <div
-        class="alert d-flex align-items-center justify-content-start"
-        data-glpi-form-renderer-delegation-container
-    >
-        {% if users is not empty %}
-            <span class="text-nowrap text-muted mx-2">This ticket is for</span>
-            {{ fields.dropdownArrayField(
-                'delegation_users_id',
-                selected_user.getID(),
-                {
-                    (get_current_user().getID()): __('Myself'),
-                } + users|reduce((carry, user) => carry + {(user.id): call('User::getFriendlyNameById', [user.id])}, {}),
-                '',
-                {
-                    'no_label'   : true,
-                    'width'      : 'auto',
-                    'field_class': '',
-                    'mb'         : '',
-                    'aria_label' : __('Select the user to delegate'),
-                }
-            ) }}
-            <span class="text-nowrap text-muted mx-2">and</span>
-        {% endif %}
-        <span class="me-2 input-group d-inline-flex w-auto">
+    {% set notification_part_html %}
+        <span class="input-group d-inline-flex w-auto">
             {% if selected_user.getID() == get_current_user().getID() %}
                 {{ fields.dropdownArrayField(
                     'delegation_use_notification',
@@ -78,11 +68,12 @@
                     },
                     '',
                     {
-                        'no_label'   : true,
-                        'width'      : 'auto',
-                        'field_class': '',
-                        'mb'         : '',
-                        'aria_label' : __('Do you want to be notified of future events of this ticket'),
+                        'no_label'      : true,
+                        'width'         : 'auto',
+                        'field_class'   : '',
+                        'mb'            : '',
+                        'aria_label'    : __('Do you want to be notified of future events of this ticket'),
+                        'wrapper_class' : 'd-inline-block',
                     }
                 ) }}
             {% else %}
@@ -95,11 +86,12 @@
                     },
                     '',
                     {
-                        'no_label'   : true,
-                        'width'      : 'auto',
-                        'field_class': '',
-                        'mb'         : '',
-                        'aria_label' : __('Do you want to be notified of future events of this ticket'),
+                        'no_label'      : true,
+                        'width'         : 'auto',
+                        'field_class'   : '',
+                        'mb'            : '',
+                        'aria_label'    : __('Do you want to be notified of future events of this ticket'),
+                        'wrapper_class' : 'd-inline-block',
                     }
                 ) }}
             {% endif %}
@@ -126,6 +118,18 @@
                 ) }}
             </div>
         </span>
-        <span class="text-nowrap text-muted ms-1">to be notified of future events of this ticket</span><span></span>
+    {% endset %}
+
+    <div class="alert d-flex align-items-center justify-content-start flex-wrap">
+        {% if users|length > 1 %}
+            {{ __('This ticket is for %1$s and %2$s to be notified of future events of this ticket.')|format(
+                '<span class="mx-2">' ~ user_dropdown_html|raw ~ '</span>',
+                '<span class="mx-2">' ~ notification_part_html|raw ~ '</span>'
+            )|raw }}
+        {% else %}
+            {{ __('%1$s to be notified of future events of this ticket.')|format(
+                '<span class="mx-2">' ~ notification_part_html|raw ~ '</span>'
+            )|raw }}
+        {% endif %}
     </div>
 {% endif %}
diff --git a/templates/pages/form_renderer.html.twig b/templates/pages/form_renderer.html.twig

Original file line number	Diff line number	Diff line change
`@@ -8,7 +8,6 @@`
`8`	`8`	`* http://glpi-project.org`
`9`	`9`	`*`
`10`	`10`	`* @copyright 2015-2025 Teclib' and contributors.`
`11`		`- * @copyright 2003-2014 by the INDEPNET Development Team.`
`12`	`11`	`* @licence https://www.gnu.org/licenses/gpl-3.0.html`
`13`	`12`	`*`
`14`	`13`	`* ---------------------------------------------------------------------`
Original file line number	Diff line number	Diff line change
`@@ -52,8 +52,7 @@ final class AnswersSet extends CommonDBChild`
`52`	`52`
`53`	`53`	`public array $files = [];`
`54`	`54`
`55`		`- /** @var array<string, ?string> $delegation */`
`56`		`- public array $delegation = [];`
	`55`	`+ public DelegationData $delegation;`
`57`	`56`
`58`	`57`	`#[Override]`
`59`	`58`	`public static function getTypeName($nb = 0)`
`@@ -271,19 +270,19 @@ public function setSubmittedFiles(array $files): void`
`271`	`270`	`/**`
`272`	`271`	`* Get delegation data`
`273`	`272`	`*`
`274`		`- * @return array<string, ?string>`
	`273`	`+ * @return DelegationData`
`275`	`274`	`*/`
`276`		`- public function getDelegation(): array`
	`275`	`+ public function getDelegation(): DelegationData`
`277`	`276`	`{`
`278`	`277`	`return $this->delegation;`
`279`	`278`	`}`
`280`	`279`
`281`	`280`	`/**`
`282`	`281`	`* Set delegation data`
`283`	`282`	`*`
`284`		`- * @param array<string, ?string> $delegation`
	`283`	`+ * @param DelegationData $delegation`
`285`	`284`	`*/`
`286`		`- public function setDelegation(array $delegation): void`
	`285`	`+ public function setDelegation(DelegationData $delegation): void`
`287`	`286`	`{`
`288`	`287`	`$this->delegation = $delegation;`
`289`	`288`	`}`
Original file line number	Diff line number	Diff line change
`@@ -123,13 +123,13 @@ private function getActorsFromCurrentUser(AnswersSet $answers_set): ?array`
`123`	`123`	`}`
`124`	`124`
`125`	`125`	`$delegation = $answers_set->getDelegation();`
`126`		`- if (Ticket::canDelegateeCreateTicket($delegation['users_id'] ?? $user_id)) {`
	`126`	`+ if (Ticket::canDelegateeCreateTicket($delegation->users_id ?? $user_id)) {`
`127`	`127`	`return [`
`128`	`128`	`[`
`129`	`129`	`'itemtype' => User::class,`
`130`		`- 'items_id' => $delegation['users_id'] ?? $user_id,`
`131`		`- 'use_notification' => $delegation['use_notification'] ?? 0,`
`132`		`- 'alternative_email' => $delegation['alternative_email'] ?? '',`
	`130`	`+ 'items_id' => $delegation->users_id ?? $user_id,`
	`131`	`+ 'use_notification' => $delegation->use_notification ?? 0,`
	`132`	`+ 'alternative_email' => $delegation->alternative_email ?? '',`
`133`	`133`	`]`
`134`	`134`	`];`
`135`	`135`	`} else {`
`@@ -144,7 +144,7 @@ private function getActorsFromCurrentUser(AnswersSet $answers_set): ?array`
`144`	`144`
`145`	`145`	`private function getActorsFromSupervisorOfCurrentUser(AnswersSet $answers_set): ?array`
`146`	`146`	`{`
`147`		`- $users_id = $answers_set->getDelegation()['users_id'] ?? Session::getLoginUserID();`
	`147`	`+ $users_id = $answers_set->getDelegation()->users_id ?? Session::getLoginUserID();`
`148`	`148`	`if (!is_numeric($users_id)) {`
`149`	`149`	`return null;`
`150`	`150`	`}`