-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix(forms): Ensure consistent dropdown labels for Item questions in f… #23507
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 11.0/bugfixes
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -308,18 +308,20 @@ public function getSubTypeDefaultValue(?Question $question): ?string | |
| #[Override] | ||
| public function renderAdministrationTemplate(?Question $question): string | ||
| { | ||
| $default_itemtype = $this->getDefaultValueItemtype($question); | ||
| $twig = TemplateRenderer::getInstance(); | ||
| return $twig->render( | ||
| 'pages/admin/form/question_type/item/administration_template.html.twig', | ||
| [ | ||
| 'init' => $question != null, | ||
| 'question' => $question, | ||
| 'question_type' => $this::class, | ||
| 'default_itemtype' => $this->getDefaultValueItemtype($question), | ||
| 'default_itemtype' => $default_itemtype, | ||
| 'default_items_id' => $this->getDefaultValueItemId($question), | ||
| 'itemtypes' => $this->getAllowedItemtypes(), | ||
| 'aria_label' => $this->items_id_aria_label, | ||
| 'advanced_config' => $this->renderAdvancedConfigurationTemplate($question), | ||
| 'displaywith' => $this->getDisplayWith($default_itemtype), | ||
| ] | ||
| ); | ||
| } | ||
|
|
@@ -355,35 +357,52 @@ public function renderAdvancedConfigurationTemplate(?Question $question): string | |
| ); | ||
| } | ||
|
|
||
| #[Override] | ||
| public function renderEndUserTemplate(Question $question): string | ||
| /** | ||
| * Compute the list of additional fields to display alongside item names in dropdowns. | ||
| * | ||
| * @param string|null $itemtype The itemtype to compute displaywith for. | ||
| * @return array<string> | ||
| */ | ||
| public function getDisplayWith(?string $itemtype): array | ||
| { | ||
| global $CFG_GLPI; | ||
|
|
||
| $itemtype = $this->getDefaultValueItemtype($question) ?? '0'; | ||
| if ($itemtype === null || $itemtype === '0') { | ||
| return []; | ||
| } | ||
|
|
||
| $displaywith = []; | ||
| $is_itil_type = in_array($itemtype, $CFG_GLPI['itil_types']); | ||
| $id_already_visible = isset($_SESSION['glpiis_ids_visible']) && $_SESSION['glpiis_ids_visible']; | ||
|
|
||
| $displaywith = []; | ||
| if ($is_itil_type && !$id_already_visible) { | ||
| $displaywith[] = 'id'; | ||
| } | ||
|
|
||
| if (in_array($itemtype, $CFG_GLPI['asset_types'])) { | ||
| $item = getItemForItemtype($itemtype); | ||
| if ($item) { | ||
| if ($item->isField('serial')) { | ||
| $displaywith[] = 'serial'; | ||
| } | ||
| if ($item->isField('otherserial')) { | ||
| $displaywith[] = 'otherserial'; | ||
| } | ||
| if ($item->isField('serial')) { | ||
| $displaywith[] = 'serial'; | ||
| } | ||
|
Comment on lines
+388
to
+390
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see you changed this for consistency with the ajax dropdown endpoint but maybe this order should be the one being kept? I think the main serial number is more important and should be displayed first. Or we keep it like that to prevent changes on a bugfix and we use serial -> serialnumber everywhere on main. |
||
| if ($item->isField('users_id')) { | ||
| $displaywith[] = 'users_id'; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return $displaywith; | ||
| } | ||
|
|
||
| #[Override] | ||
| public function renderEndUserTemplate(Question $question): string | ||
| { | ||
| $itemtype = $this->getDefaultValueItemtype($question) ?? '0'; | ||
| $displaywith = $this->getDisplayWith($itemtype); | ||
|
|
||
| $twig = TemplateRenderer::getInstance(); | ||
| return $twig->render( | ||
| 'pages/admin/form/question_type/item/end_user_template.html.twig', | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,98 @@ | ||||||
| /** | ||||||
| * --------------------------------------------------------------------- | ||||||
| * | ||||||
| * GLPI - Gestionnaire Libre de Parc Informatique | ||||||
| * | ||||||
| * http://glpi-project.org | ||||||
| * | ||||||
| * @copyright 2015-2026 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/>. | ||||||
| * | ||||||
| * --------------------------------------------------------------------- | ||||||
| */ | ||||||
|
|
||||||
| import { test, expect } from '../../../fixtures/glpi_fixture'; | ||||||
| import { FormPage } from '../../../pages/FormPage'; | ||||||
| import { Profiles } from '../../../utils/Profiles'; | ||||||
| import { getWorkerEntityId } from '../../../utils/WorkerEntities'; | ||||||
|
|
||||||
| test.describe('Item form question type', () => { | ||||||
| let form: FormPage; | ||||||
| let form_id: number; | ||||||
| let entity_id: number; | ||||||
|
|
||||||
| test.beforeEach(async ({ page, profile, entity, api, formImporter }) => { | ||||||
| await profile.set(Profiles.SuperAdmin); | ||||||
| form = new FormPage(page); | ||||||
|
|
||||||
| // Create new entity | ||||||
| entity_id = await api.createItem('Entity', { | ||||||
| name: `Entity ${Date.now()}`, | ||||||
| entities_id: getWorkerEntityId(), | ||||||
| }); | ||||||
|
|
||||||
| // Switch to the new entity and refresh session to ensure the new entity is taken into account in the session | ||||||
| entity.switchToWithoutRecursion(entity_id); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| api.refreshSession(); | ||||||
|
|
||||||
| const info = await formImporter.importForm('question_types/item-editor-test.json'); | ||||||
| form_id = info.getId(); | ||||||
| await form.goto(form_id); | ||||||
|
|
||||||
| }); | ||||||
|
|
||||||
| test.afterEach(async ({ entity, api }) => { | ||||||
| // Reset entity to default one to avoid issues with other tests in the same worker | ||||||
| entity.resetToDefaultWorkerEntity(); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| api.refreshSession(); | ||||||
| }); | ||||||
|
|
||||||
| test('Adding new item option and compare select option labels', async ({ api }) => { | ||||||
| // Add two computers | ||||||
| await api.createItem('Computer', { | ||||||
| name: 'Computer 1', | ||||||
| entities_id: entity_id | ||||||
| }); | ||||||
| await api.createItem('Computer', { | ||||||
| name: 'Computer 2', | ||||||
| entities_id: entity_id, | ||||||
| serial: '123456', | ||||||
| otherserial: '654321' | ||||||
| }); | ||||||
|
|
||||||
| const persisted_question = form.getLastQuestion(); | ||||||
| await persisted_question.click({ position: { x: 0, y: 0 } }); | ||||||
|
|
||||||
| form.getDropdownByLabel('Select an item', persisted_question).click(); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| await expect(form.page.getByRole('option', { name: 'Computer 1' })).toBeVisible(); | ||||||
| await expect(form.page.getByRole('option', { name: 'Computer 2 - 654321 - 123456' })).toBeVisible(); | ||||||
|
|
||||||
| const new_question = await form.addQuestion('Item question'); | ||||||
| await form.setQuestionType(new_question, 'Item'); | ||||||
| await form.setSubQuestionType(new_question, 'GLPI Objects'); | ||||||
| await form.setItemTypeForItemQuestion(new_question, 'Computers'); | ||||||
|
|
||||||
| form.getDropdownByLabel('Select an item', new_question).click(); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| await expect(form.page.getByRole('option', { name: 'Computer 1' })).toBeVisible(); | ||||||
| await expect(form.page.getByRole('option', { name: 'Computer 2 - 654321 - 123456' })).toBeVisible(); | ||||||
| }); | ||||||
| }); | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| { | ||
| "version": 1, | ||
| "forms": [ | ||
| { | ||
| "id": 1, | ||
| "uuid": "dd000001-0001-0001-0001-000000000001", | ||
| "name": "Tests form for the item form question type suite", | ||
| "header": null, | ||
| "description": null, | ||
| "entity_name": "Root entity", | ||
| "is_recursive": true, | ||
| "is_active": false, | ||
| "submit_button_visibility_strategy": "always_visible", | ||
| "illustration": "", | ||
| "submit_button_conditions": [], | ||
| "sections": [ | ||
| { | ||
| "id": 1, | ||
| "uuid": "dd000001-0001-0001-0001-000000000010", | ||
| "name": "First section", | ||
| "description": null, | ||
| "rank": 0, | ||
| "visibility_strategy": "always_visible", | ||
| "conditions": [] | ||
| } | ||
| ], | ||
| "comments": [], | ||
| "questions": [ | ||
| { | ||
| "id": 1, | ||
| "uuid": "dd000001-0001-0001-0001-000000000100", | ||
| "name": "Test item question", | ||
| "type": "Glpi\\Form\\QuestionType\\QuestionTypeItem", | ||
| "is_mandatory": false, | ||
| "vertical_rank": 0, | ||
| "horizontal_rank": null, | ||
| "description": "", | ||
| "default_value": "", | ||
| "extra_data": { | ||
| "root_items_id": "0", | ||
| "subtree_depth": "0", | ||
| "selectable_tree_root": "0", | ||
| "itemtype": "Computer" | ||
| }, | ||
| "section_id": 1, | ||
| "visibility_strategy": "always_visible", | ||
| "validation_strategy": "no_validation", | ||
| "conditions": [], | ||
| "validation_conditions": [] | ||
| } | ||
| ], | ||
| "policies": [ | ||
| { | ||
| "strategy": "Glpi\\Form\\AccessControl\\ControlType\\AllowList", | ||
| "config": { | ||
| "user_ids": [ | ||
| "all" | ||
| ], | ||
| "group_ids": [], | ||
| "profile_ids": [] | ||
| }, | ||
| "is_active": true | ||
| } | ||
| ], | ||
| "destinations": [], | ||
| "translations": [], | ||
| "data_requirements": [ | ||
| { | ||
| "itemtype": "Entity", | ||
| "name": "Root entity" | ||
| } | ||
| ], | ||
| "custom_types_requirements": [], | ||
| "plugin_requirements": [] | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method is public but is only used internally, is it normal?