diff --git a/CHANGELOG.md b/CHANGELOG.md index 8111ba9..29e2f87 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ 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 + +- Fix Table question column type edge cases + ## [1.3.0] - 2026-08-11 ### Changed diff --git a/public/js/modules/AfTableQuestion.js b/public/js/modules/AfTableQuestion.js index 038890c..df5929b 100644 --- a/public/js/modules/AfTableQuestion.js +++ b/public/js/modules/AfTableQuestion.js @@ -51,6 +51,11 @@ export class AfTableQuestion { this.#watchServerErrors(); + // The first row is server-rendered, not cloned from the template, so + // its ajax-backed selects (unlike the static 'adapt' ones, which + // self-init through Dropdown::showFromArray) need the same wiring. + this.#initSelectsInRow(this.#body.querySelector('[data-af-table-row]')); + this.#addBtn.addEventListener('click', () => this.addRow()); this.#body.addEventListener('click', e => { const btn = e.target.closest('[data-af-table-remove-row]'); @@ -335,23 +340,49 @@ export class AfTableQuestion { } #initSelectsInRow(row) { - if (!row || !window.setupAdaptDropdown) { return; } - const limit = parseInt(this.#table.dataset.afS2Limit, 10) || 100; - row.querySelectorAll('[data-af-needs-s2]').forEach(select => { - const id = 'dropdown_af_eu_' + Date.now() + '_' + Math.random().toString(36).slice(2, 7); - select.id = id; - const config = { - type: 'adapt', - field_id: id, - width: '100%', - dropdown_css_class: '', - placeholder: '', - ajax_limit_count: limit, - }; - window.select2_configs = window.select2_configs || {}; - window.select2_configs[id] = config; - window.setupAdaptDropdown(config); - }); + if (!row) { return; } + + if (window.setupAdaptDropdown) { + const limit = parseInt(this.#table.dataset.afS2Limit, 10) || 100; + row.querySelectorAll('[data-af-needs-s2]').forEach(select => { + const id = AfTableQuestion.#newFieldId(select); + const config = { + type: 'adapt', + field_id: id, + width: '100%', + dropdown_css_class: '', + placeholder: '', + ajax_limit_count: limit, + }; + window.select2_configs = window.select2_configs || {}; + window.select2_configs[id] = config; + window.setupAdaptDropdown(config); + }); + } + + if (window.setupAjaxDropdown) { + row.querySelectorAll('[data-af-needs-ajax-s2]').forEach(select => { + let config; + try { + config = JSON.parse(select.dataset.afS2Config ?? ''); + } catch { + config = null; + } + if (!config || typeof config !== 'object') { return; } + + const id = AfTableQuestion.#newFieldId(select); + const full_config = { ...config, field_id: id }; + window.select2_configs = window.select2_configs || {}; + window.select2_configs[id] = full_config; + window.setupAjaxDropdown(full_config); + }); + } + } + + static #newFieldId(select) { + const id = 'dropdown_af_eu_' + Date.now() + '_' + Math.random().toString(36).slice(2, 7); + select.id = id; + return id; } removeRow(rowElement) { diff --git a/src/Model/QuestionType/TableQuestion.php b/src/Model/QuestionType/TableQuestion.php index be46ac6..d4cc892 100644 --- a/src/Model/QuestionType/TableQuestion.php +++ b/src/Model/QuestionType/TableQuestion.php @@ -714,9 +714,9 @@ public function renderEndUserTemplate(Question $question): string } $cell_map = []; - $user_options = null; + $user_ajax_config = null; $device_options = null; - $glpi_item_options = []; // keyed by itemtype FQCN to avoid duplicate DB queries + $item_ajax_configs = []; // keyed by itemtype FQCN to avoid duplicate IDOR tokens foreach ($config->getColumns() as $index => $col) { $fqcn = $col[TableQuestionConfig::COL_QUESTION_TYPE]; @@ -724,15 +724,15 @@ public function renderEndUserTemplate(Question $question): string $itemtype = $col[TableQuestionConfig::COL_ITEMTYPE] ?? ''; if (is_a($fqcn, AbstractQuestionTypeActors::class, true)) { - $user_options ??= $this->buildUserOptions(); - $cell_map[$index] = ['mode' => 'select', 'options' => $user_options]; + $user_ajax_config ??= $this->buildAjaxDropdownConfig(User::class, ['is_active' => 1, 'is_deleted' => 0]); + $cell_map[$index] = ['mode' => 'select-ajax', 'config' => $user_ajax_config]; } elseif (is_a($fqcn, QuestionTypeUserDevice::class, true)) { $device_options ??= $this->buildUserDeviceOptions(); $cell_map[$index] = ['mode' => 'select', 'options' => $device_options]; } elseif (is_a($fqcn, QuestionTypeItem::class, true)) { if ($itemtype !== '' && class_exists($itemtype)) { - $glpi_item_options[$itemtype] ??= $this->buildGlpiItemtypeOptions($itemtype); - $cell_map[$index] = ['mode' => 'select', 'options' => $glpi_item_options[$itemtype]]; + $item_ajax_configs[$itemtype] ??= $this->buildAjaxDropdownConfig($itemtype); + $cell_map[$index] = ['mode' => 'select-ajax', 'config' => $item_ajax_configs[$itemtype]]; } else { $cell_map[$index] = ['mode' => 'input', 'input_type' => 'text']; } @@ -804,6 +804,7 @@ public function getCompatibleQuestionTypes(): array HostnameQuestion::class, HiddenQuestion::class, LdapQuestion::class, + ReservationQuestion::class, self::class, ]; @@ -816,6 +817,11 @@ public function getCompatibleQuestionTypes(): array } } + // Exclude question types with a sub-type selector (Fields plugin types) + if (!is_a($fqcn, QuestionTypeItem::class, true) && $type->getSubTypes() !== []) { + continue; + } + $types[$fqcn] = $type->getName(); } @@ -863,43 +869,6 @@ public function getCellInfo(string $fqcn, ?QuestionTypeInterface $type = null): return ['mode' => 'input', 'input_type' => 'text']; } - /** - * Builds a [value => label] options array for actor-type columns. - * Loads up to 200 active users from the database. - * - * @return array - */ - private function buildUserOptions(): array - { - global $DB; - - $options = ['' => Dropdown::EMPTY_VALUE]; - - $rows = $DB->request([ - 'SELECT' => ['id', 'name', 'realname', 'firstname'], - 'FROM' => User::getTable(), - 'WHERE' => ['is_active' => 1, 'is_deleted' => 0], - 'ORDER' => ['realname', 'firstname', 'name'], - 'LIMIT' => 200, - ]); - - foreach ($rows as $row) { - if (!is_array($row)) { - continue; - } - - $id = is_numeric($row['id']) ? (int) $row['id'] : 0; - $options[(string) $id] = formatUserName( - $id, - is_string($row['name'] ?? null) ? $row['name'] : null, - is_string($row['realname'] ?? null) ? $row['realname'] : null, - is_string($row['firstname'] ?? null) ? $row['firstname'] : null, - ); - } - - return $options; - } - /** * Builds an optgroup-keyed options array for the User Device column type. * Keys at the top level are group labels; inner keys are "Itemtype_id" strings. @@ -917,57 +886,38 @@ private function buildUserDeviceOptions(): array } /** - * Builds a [id => name] options array for a GLPI itemtype (used by Item/ItemDropdown columns). - * Applies entity and soft-delete filters when applicable. + * Builds a select2 "ajax" widget config for a GLPI itemtype-backed column + * (Item/ItemDropdown columns, and the User picker behind Actor columns). * * @param class-string $itemtype - * @return array + * @param array $condition + * @return array */ - private function buildGlpiItemtypeOptions(string $itemtype): array + private function buildAjaxDropdownConfig(string $itemtype, array $condition = []): array { - global $DB; - - $options = ['' => Dropdown::EMPTY_VALUE]; - $item = getItemForItemtype($itemtype); - if ($item === false) { - return $options; - } - - $where = []; - - if ($item->maybeDeleted()) { - $where['is_deleted'] = 0; - } + global $CFG_GLPI; - if ($item->isEntityAssign()) { - $where = array_merge($where, getEntitiesRestrictCriteria( - $item->getTable(), - '', - '', - $item->maybeRecursive(), - )); - } + $condition_key = $condition !== [] ? Dropdown::addNewCondition($condition) : ''; + $root_doc = is_string($CFG_GLPI['root_doc'] ?? null) ? $CFG_GLPI['root_doc'] : ''; + $dropdown_max = $CFG_GLPI['dropdown_max'] ?? 50; - $criteria = [ - 'SELECT' => ['id', 'name'], - 'FROM' => $item->getTable(), - 'ORDER' => 'name', - 'LIMIT' => 200, + return [ + 'url' => $root_doc . '/ajax/getDropdownValue.php', + 'params' => [ + 'itemtype' => $itemtype, + 'condition' => $condition_key, + '_idor_token' => Session::getNewIDORToken($itemtype, ['condition' => $condition_key]), + ], + 'dropdown_max' => is_numeric($dropdown_max) ? (int) $dropdown_max : 50, + 'ajax_limit_count' => $this->ajaxLimitCount(), + 'width' => '100%', + 'container_css_class' => '', + 'multiple' => false, + 'placeholder' => Dropdown::EMPTY_VALUE, + 'allowclear' => false, + 'parent_id_field' => '', + 'on_change' => '', ]; - - if ($where !== []) { - $criteria['WHERE'] = $where; - } - - foreach ($DB->request($criteria) as $row) { - if (!is_array($row)) { - continue; - } - - $options[(string) (is_numeric($row['id']) ? (int) $row['id'] : 0)] = is_string($row['name']) ? $row['name'] : ''; - } - - return $options; } private function loadConfig(Question $question): TableQuestionConfig diff --git a/templates/table_end_user.html.twig b/templates/table_end_user.html.twig index e1fbaaa..e000dfd 100644 --- a/templates/table_end_user.html.twig +++ b/templates/table_end_user.html.twig @@ -89,6 +89,16 @@ ]) %} {% endset %} {{ sel_html|raw }} + {% elseif cell.mode == 'select-ajax' %} + {% else %} {{ lbl }} {% endfor %} + {% elseif cell.mode == 'select-ajax' %} + {% else %} initDropdownDefinition('Test1'); + $test2_definition = $this->initDropdownDefinition('Test2'); + + $test1_class = $test1_definition->getDropdownClassName(); + $test2_class = $test2_definition->getDropdownClassName(); + + Dropdown::resetItemtypesStaticCache(); + + $html = $this->render([ + $this->column('Col1', QuestionTypeItemDropdown::class, itemtype: $test1_class), + $this->column('Col2', QuestionTypeItemDropdown::class, itemtype: $test2_class), + ]); + $configs = $this->renderedAjaxConfigs($html); + + $this->assertCount(2, $configs); + $this->assertSame($test1_class, $configs[0]['params']['itemtype']); + $this->assertSame($test2_class, $configs[1]['params']['itemtype']); + $this->assertNotSame( + $configs[0]['params']['_idor_token'], + $configs[1]['params']['_idor_token'], + "Each column must get its own IDOR token, or one column could query the other's scope.", + ); + } + + public function testGlpiObjectColumnIsBackedByTheAjaxDropdownEndpoint(): void + { + $html = $this->render([$this->column('Asset', QuestionTypeItem::class, itemtype: Computer::class)]); + $configs = $this->renderedAjaxConfigs($html); + + $this->assertCount(1, $configs); + $this->assertSame(Computer::class, $configs[0]['params']['itemtype']); + $this->assertNotEmpty($configs[0]['params']['_idor_token']); + $this->assertStringEndsWith('/ajax/getDropdownValue.php', $configs[0]['url']); + } + + public function testGlpiObjectColumnHasNoPreFetchedOptions(): void + { + for ($i = 1; $i <= 3; $i++) { + $this->createItem(Computer::class, ['name' => 'Computer ' . $i, 'entities_id' => Session::getActiveEntity()]); + } + + $html = $this->render([$this->column('Asset', QuestionTypeItem::class, itemtype: Computer::class)]); + $crawler = new Crawler($html); + $select = $crawler->filter('[data-af-table-body] [data-af-table-row] select[data-af-needs-ajax-s2]'); + + $this->assertSame(1, $select->count()); + + $options = $select->filter('option'); + $this->assertLessThanOrEqual(1, $options->count()); + if ($options->count() === 1) { + $this->assertSame('', $options->attr('value')); + $this->assertNotNull($options->attr('disabled')); + } + } + + public function testActorColumnIsBackedByTheAjaxDropdownEndpointRestrictedToActiveUsers(): void + { + $html = $this->render([$this->column('Owner', QuestionTypeRequester::class)]); + $configs = $this->renderedAjaxConfigs($html); + + $this->assertCount(1, $configs); + $this->assertSame(User::class, $configs[0]['params']['itemtype']); + + $condition_key = $configs[0]['params']['condition']; + $this->assertNotSame('', $condition_key); + $this->assertSame( + ['is_active' => 1, 'is_deleted' => 0], + $_SESSION['glpicondition'][$condition_key] ?? null, + ); + } + + public function testAjaxColumnConfigIsAlsoPresentInTheRowCloneTemplate(): void + { + $html = $this->render([$this->column('Asset', QuestionTypeItem::class, itemtype: Computer::class)]); + + $this->assertSame(2, substr_count($html, 'data-af-needs-ajax-s2')); + } + + /** + * @return list> Decoded `data-af-s2-config` payload + * for each ajax-backed select in the visible row, in column order. + */ + private function renderedAjaxConfigs(string $html): array + { + $crawler = new Crawler($html); + $selects = $crawler->filter('[data-af-table-body] [data-af-table-row] select[data-af-needs-ajax-s2]'); + + return $selects->each(function (Crawler $n): array { + $decoded = json_decode((string) $n->attr('data-af-s2-config'), associative: true); + $this->assertIsArray($decoded, 'data-af-s2-config must hold a JSON object.'); + + return $decoded; + }); + } + /** * @param array $columns * @return array Decoded `data-af-pattern-cols` payload. @@ -324,12 +428,13 @@ private function column( string $fqcn, bool $required = false, string $pattern = '', + string $itemtype = '', ): array { return [ TableQuestionConfig::COL_NAME => $name, TableQuestionConfig::COL_QUESTION_TYPE => $fqcn, TableQuestionConfig::COL_REQUIRED => $required, - TableQuestionConfig::COL_ITEMTYPE => '', + TableQuestionConfig::COL_ITEMTYPE => $itemtype, TableQuestionConfig::COL_PATTERN => $pattern, ]; } diff --git a/tests/Model/QuestionType/TableQuestionTest.php b/tests/Model/QuestionType/TableQuestionTest.php index ea22748..9c7d94b 100644 --- a/tests/Model/QuestionType/TableQuestionTest.php +++ b/tests/Model/QuestionType/TableQuestionTest.php @@ -33,6 +33,7 @@ namespace GlpiPlugin\Advancedforms\Tests\Model\QuestionType; +use Glpi\Form\Question; use Glpi\Form\Condition\ValueOperator; use Glpi\Form\QuestionType\QuestionTypeCheckbox; use Glpi\Form\QuestionType\QuestionTypeEmail; @@ -45,7 +46,15 @@ use GlpiPlugin\Advancedforms\Model\QuestionType\TreeCascadeDropdownQuestion; use GlpiPlugin\Advancedforms\Model\QuestionType\TableQuestion; use GlpiPlugin\Advancedforms\Model\QuestionType\TableQuestionConfig; +use GlpiPlugin\Advancedforms\Model\QuestionType\ReservationQuestion; use GlpiPlugin\Advancedforms\Tests\AdvancedFormsTestCase; +use Glpi\Form\QuestionType\AbstractQuestionType; +use Glpi\Form\QuestionType\QuestionTypeCategoryInterface; +use Glpi\Form\QuestionType\QuestionTypeItem; +use Glpi\Form\QuestionType\QuestionTypeItemDropdown; +use Glpi\Form\QuestionType\QuestionTypesManager; +use GlpiPlugin\Advancedforms\Model\QuestionType\AdvancedCategory; +use Override; final class TableQuestionTest extends AdvancedFormsTestCase { @@ -154,6 +163,63 @@ public function testCompatibleTypesExcludesTreeCascadeDropdown(): void $this->assertArrayNotHasKey(TreeCascadeDropdownQuestion::class, $types); } + public function testCompatibleTypesExcludesReservation(): void + { + $types = $this->type->getCompatibleQuestionTypes(); + $this->assertArrayNotHasKey(ReservationQuestion::class, $types); + } + + /** + * Regression test for types with custom sub-type selectors, which cannot + * be represented as flat table column types and thus must be excluded. + */ + public function testCompatibleTypesExcludesTypesWithSubTypes(): void + { + $fake_type = new class extends AbstractQuestionType { + #[Override] + public function getCategory(): QuestionTypeCategoryInterface + { + return new AdvancedCategory(); + } + + #[Override] + public function getSubTypes(): array + { + return ['fake' => 'Fake sub type']; + } + + #[Override] + public function renderAdministrationTemplate(?Question $question): string + { + return ''; + } + + #[Override] + public function renderEndUserTemplate(?Question $question, mixed $answer = null): string + { + return ''; + } + }; + + QuestionTypesManager::getInstance()->registerPluginQuestionType($fake_type); + + $types = $this->type->getCompatibleQuestionTypes(); + $this->assertArrayNotHasKey($fake_type::class, $types); + } + + /** + * QuestionTypeItem and QuestionTypeItemDropdown both declare a non-empty + * getSubTypes() but must stay selectable: Table + * already renders them through its own dedicated itemtype picker + * (TableQuestionConfig::COL_ITEMTYPE), independent of getSubTypes(). + */ + public function testCompatibleTypesIncludesItemAndItemDropdownDespiteSubTypes(): void + { + $types = $this->type->getCompatibleQuestionTypes(); + $this->assertArrayHasKey(QuestionTypeItem::class, $types); + $this->assertArrayHasKey(QuestionTypeItemDropdown::class, $types); + } + public function testGetConfigKey(): void { $this->assertSame('enable_question_type_table', TableQuestion::getConfigKey());