diff --git a/CHANGELOG.md b/CHANGELOG.md index 39b1625..077e51e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,8 +7,13 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## [Unreleased] +### Changed + +- **Breaking**: the `Table` question's `Min rows` / `Max rows` settings are removed. Row bounds are now declared as validation conditions on the question (`Length is greater than…` / `Length is less than…`, applied to the number of filled rows), and are also usable as visibility criteria. Bounds configured in 1.2.0 are ignored and must be redeclared + ### Fixed +- Fixed the `Table` question's column validation: each column is now checked against its own pattern only, values the pattern accepts are no longer rejected, the column type's own format check still applies, and errors are listed once below the table - Fix string condition operators (equals, contains, length) not being available on hidden questions ## [1.2.0] - 2026-07-28 diff --git a/public/css/advancedforms.css b/public/css/advancedforms.css index 2ba303c..f8f9427 100644 --- a/public/css/advancedforms.css +++ b/public/css/advancedforms.css @@ -74,3 +74,22 @@ [data-af-table-question] td:has(select.is-invalid) .select2-container--default .select2-selection { border-color: var(--tblr-form-invalid-border-color) !important; } + +/* The core renderer appends a question's error message next to every input it + * finds, so a table receives one copy per cell. AfTableQuestion moves the + * distinct messages into [data-af-table-errors]; hide the copies still sitting + * in the cells so they never flash. The class is set by the module itself: if it + * fails to load, core's own rendering stays visible rather than silently gone. */ +.af-table-errors-managed td > .invalid-tooltip { + display: none; +} +[data-af-table-errors]:empty { + display: none; +} +[data-af-table-errors] .invalid-tooltip { + position: static; + display: block; + width: 100%; + max-width: none; + margin-top: 0; +} diff --git a/public/js/modules/AfTableQuestion.js b/public/js/modules/AfTableQuestion.js index d0abb10..038890c 100644 --- a/public/js/modules/AfTableQuestion.js +++ b/public/js/modules/AfTableQuestion.js @@ -36,21 +36,21 @@ export class AfTableQuestion { #body; #template; #addBtn; - #minRows; - #maxRows; + #errors; constructor(tableElement) { this.#table = tableElement; this.#body = tableElement.querySelector('[data-af-table-body]'); this.#template = tableElement.querySelector('[data-af-table-row-template]'); this.#addBtn = tableElement.querySelector('[data-af-table-add-row]'); - this.#minRows = parseInt(tableElement.dataset.afMinRows, 10) || 1; - this.#maxRows = parseInt(tableElement.dataset.afMaxRows, 10) || 50; + this.#errors = tableElement.querySelector('[data-af-table-errors]'); if (!this.#body || !this.#template || !this.#addBtn) { return; } + this.#watchServerErrors(); + this.#addBtn.addEventListener('click', () => this.addRow()); this.#body.addEventListener('click', e => { const btn = e.target.closest('[data-af-table-remove-row]'); @@ -72,6 +72,69 @@ export class AfTableQuestion { AfTableQuestion.#registerSubmitGuard(); } + /** + * The core renderer reports validation errors per question, but attaches each + * message next to every input it finds inside that question. A table has one + * input per cell, so a single error ends up repeated in every cell, and every + * cell gets flagged whether or not it is at fault. + * + * Watch for those injections, keep one copy of each distinct message in a list + * below the table, and let the client-side rules decide which cells to flag. + * The relocated nodes keep their class so the renderer still clears them on + * the next round. + */ + #watchServerErrors() { + if (!this.#errors) { return; } + + // Only hide the in-cell copies once we know we can relocate them: without + // this class, a module that failed to load leaves core's output visible. + this.#table.classList.add('af-table-errors-managed'); + + new MutationObserver(mutations => { + const injected = []; + mutations.forEach(mutation => { + mutation.addedNodes.forEach(node => { + if (node.nodeType !== Node.ELEMENT_NODE) { return; } + if (!node.classList.contains('invalid-tooltip')) { return; } + // Ignore the ones we just moved ourselves. + if (this.#errors.contains(node)) { return; } + injected.push(node); + }); + }); + + if (injected.length) { this.#relocateServerErrors(injected); } + }).observe(this.#table, { childList: true, subtree: true }); + } + + /** @param {Element[]} injected */ + #relocateServerErrors(injected) { + // Core empties the whole question before injecting a new round, so the + // list is already clear here; do it explicitly anyway, as #validateTable + // deliberately leaves this container alone. + this.#errors.replaceChildren(); + + const seen = []; + injected.forEach(node => { + const message = (node.textContent ?? '').trim(); + if (message !== '' && !seen.some(kept => kept.textContent.trim() === message)) { + seen.push(node); + } + node.remove(); + }); + + // Flag the cells the browser can judge on its own; the detail stays in + // the list below the table. + AfTableQuestion.#validateTable(this.#table, false); + + seen.forEach((node, index) => { + // Core gives every copy the same id; keep it on the first one only so + // the inputs' aria-errormessage still resolves to a unique element. + if (index > 0) { node.removeAttribute('id'); } + node.classList.add('d-block'); + this.#errors.appendChild(node); + }); + } + static #registerSubmitGuard() { if (AfTableQuestion.#submitGuardRegistered) { return; } AfTableQuestion.#submitGuardRegistered = true; @@ -98,17 +161,30 @@ export class AfTableQuestion { } /** + * @param {Element} table + * @param {boolean} withMessages Flag the cells only, when the detail already + * sits in the server error list below the table. * @returns {Element|null} the first invalid control of the table, or null. */ - static #validateTable(table) { + static #validateTable(table, withMessages = true) { // Skip tables hidden by step-by-step navigation or conditional sections. if (table.offsetParent === null) { return null; } // Core only clears its own server-rendered errors when a new request // round-trips; if we block the submit below, that never happens, leaving - // stale messages from a previous attempt next to our fresh ones. - table.querySelectorAll('.invalid-tooltip').forEach(el => el.remove()); + // stale messages from a previous attempt next to our fresh ones. The + // ones already relocated below the table are spared: they are the only + // rendering left for a rule the browser cannot judge on its own, and + // blocking the submit means no round-trip will bring them back. + table.querySelectorAll('.invalid-tooltip').forEach(el => { + if (!el.closest('[data-af-table-errors]')) { el.remove(); } + }); + const message = text => (withMessages ? text : ''); + + // Both payloads are keyed by the "col_N" token found in the field names, + // and are built server-side from the rules the server itself validates + // against, so a column can never pick up its neighbour's rule. const requiredCols = (table.dataset.afRequiredCols ?? '') .split(',') .filter(value => value !== ''); @@ -120,9 +196,9 @@ export class AfTableQuestion { patternCols = {}; } const patternRegexes = {}; - Object.entries(patternCols).forEach(([colIndex, pattern]) => { + Object.entries(patternCols).forEach(([colKey, pattern]) => { const regex = AfTableQuestion.#toRegExp(pattern); - if (regex) { patternRegexes[colIndex] = regex; } + if (regex) { patternRegexes[colKey] = regex; } }); let firstInvalid = null; @@ -133,32 +209,32 @@ export class AfTableQuestion { const rowHasValue = controls.some(control => AfTableQuestion.#hasValue(control) || control.validity?.badInput); controls.forEach(control => { - const colIndex = AfTableQuestion.#columnIndex(control); + const colKey = AfTableQuestion.#columnKey(control); if (control.validity?.badInput) { - AfTableQuestion.#setCellError(control, table.dataset.afPatternMsg ?? ''); + AfTableQuestion.#setCellError(control, message(table.dataset.afPatternMsg ?? '')); if (!firstInvalid) { firstInvalid = control; } return; } const hasValue = AfTableQuestion.#hasValue(control); - if (rowHasValue && requiredCols.includes(colIndex) && !hasValue) { - AfTableQuestion.#setCellError(control, table.dataset.afRequiredMsg ?? ''); + if (rowHasValue && requiredCols.includes(colKey) && !hasValue) { + AfTableQuestion.#setCellError(control, message(table.dataset.afRequiredMsg ?? '')); if (!firstInvalid) { firstInvalid = control; } return; } - const regex = patternRegexes[colIndex]; + const regex = patternRegexes[colKey]; if (hasValue && regex && !regex.test(control.value)) { - AfTableQuestion.#setCellError(control, table.dataset.afPatternMsg ?? ''); + AfTableQuestion.#setCellError(control, message(table.dataset.afPatternMsg ?? '')); if (!firstInvalid) { firstInvalid = control; } return; } // Native constraint from the column's type (number, email...); "missing" is handled above. if (hasValue && control.validity && !control.validity.valid && !control.validity.valueMissing) { - AfTableQuestion.#setCellError(control, table.dataset.afPatternMsg ?? ''); + AfTableQuestion.#setCellError(control, message(table.dataset.afPatternMsg ?? '')); if (!firstInvalid) { firstInvalid = control; } return; } @@ -172,16 +248,25 @@ export class AfTableQuestion { /** * Parses a PHP-style `/regex/flags` string into a RegExp, or a bare pattern - * with no delimiters. Only JS-supported flags (gimsuy) are kept. + * with no delimiters. + * + * Only flags shared by PCRE and JS are accepted. A flag PCRE understands but + * JS does not (`x`, for one) makes this return null rather than be dropped, + * leaving the server as sole judge — which it is anyway. `g` and `y` do not + * exist in PCRE at all, and would make `test()` stateful across cells. * - * @returns {RegExp|null} null if the pattern is empty or invalid. + * @returns {RegExp|null} null if the pattern is empty or unusable here. */ static #toRegExp(pattern) { if (typeof pattern !== 'string' || pattern === '') { return null; } const match = /^\/(.*)\/([a-z]*)$/s.exec(pattern); const body = match ? match[1] : pattern; - const flags = (match ? match[2] : '').split('').filter(f => 'gimsuy'.includes(f)).join(''); + const flags = match ? match[2] : ''; + + // Dropping a flag we cannot honour would silently evaluate a different + // regex than the server does, so give up instead and let it decide. + if (flags.split('').some(f => !'imsu'.includes(f))) { return null; } try { return new RegExp(body, flags); @@ -204,18 +289,24 @@ export class AfTableQuestion { return (control.value ?? '').trim() !== ''; } - /** Extracts the "col_N" index from a cell control name, as a string. */ - static #columnIndex(control) { - const match = /\[col_(\d+)\]/.exec(control.name ?? ''); + /** Extracts the "col_N" key from a cell control name. */ + static #columnKey(control) { + const match = /\[(col_\d+)\]/.exec(control.name ?? ''); return match ? match[1] : ''; } + /** An empty message flags the cell without writing any text under it. */ static #setCellError(control, message) { control.classList.add('is-invalid'); const td = control.closest('td') ?? control.parentElement; if (!td) { return; } + if (message === '') { + td.querySelector('[data-af-cell-error]')?.remove(); + return; + } + let feedback = td.querySelector('[data-af-cell-error]'); if (!feedback) { feedback = document.createElement('div'); @@ -234,9 +325,6 @@ export class AfTableQuestion { addRow() { const rowCount = this.#rowCount(); - if (rowCount >= this.#maxRows) { - return; - } const clone = this.#template.content.cloneNode(true); clone.querySelectorAll('[name]').forEach(el => { el.name = el.name.replace('__ROW__', rowCount); @@ -267,7 +355,9 @@ export class AfTableQuestion { } removeRow(rowElement) { - if (!rowElement || this.#rowCount() <= this.#minRows) { + // Keeping one row on screen is presentation only: acceptable row counts + // are enforced by the form's validation conditions. + if (!rowElement || this.#rowCount() <= 1) { return; } rowElement.remove(); @@ -284,16 +374,11 @@ export class AfTableQuestion { } #updateButtonStates() { - const count = this.#rowCount(); - const atMax = count >= this.#maxRows; - const atMin = count <= this.#minRows; - - this.#addBtn.classList.toggle('opacity-25', atMax); - this.#addBtn.classList.toggle('pe-none', atMax); + const lastRow = this.#rowCount() <= 1; this.#body.querySelectorAll('[data-af-table-remove-row]').forEach(icon => { - icon.classList.toggle('opacity-25', atMin); - icon.classList.toggle('pe-none', atMin); + icon.classList.toggle('opacity-25', lastRow); + icon.classList.toggle('pe-none', lastRow); }); } diff --git a/src/Model/Condition/TableRowCountConditionHandler.php b/src/Model/Condition/TableRowCountConditionHandler.php new file mode 100644 index 0000000..1a1dbba --- /dev/null +++ b/src/Model/Condition/TableRowCountConditionHandler.php @@ -0,0 +1,131 @@ +} */ + #[Override] + public function getTemplateParameters(ConditionData $condition): array + { + return [ + 'attributes' => [ + 'type' => 'number', + 'min' => '0', + 'step' => '1', + ], + ]; + } + + #[Override] + public function applyValueOperator( + mixed $a, + ValueOperator $operator, + mixed $b, + ): bool { + $rows = $this->countFilledRows($a); + $threshold = (int) (is_scalar($b) ? $b : 0); + + return match ($operator) { + ValueOperator::LENGTH_GREATER_THAN => $rows > $threshold, + ValueOperator::LENGTH_GREATER_THAN_OR_EQUALS => $rows >= $threshold, + ValueOperator::LENGTH_LESS_THAN => $rows < $threshold, + ValueOperator::LENGTH_LESS_THAN_OR_EQUALS => $rows <= $threshold, + + // Unsupported operators + default => false, + }; + } + + /** + * Counts the rows that carry at least one value. Entirely empty rows are + * dropped when the answer is saved, so counting them would compare against + * something that never reaches storage. + */ + private function countFilledRows(mixed $answer): int + { + if (!is_array($answer)) { + return 0; + } + + $count = 0; + foreach ($answer as $row) { + if (!is_array($row)) { + continue; + } + + foreach ($row as $cell) { + if ($cell !== '' && $cell !== null) { + $count++; + continue 2; + } + } + } + + return $count; + } +} diff --git a/src/Model/QuestionType/TableColumnRule.php b/src/Model/QuestionType/TableColumnRule.php new file mode 100644 index 0000000..7bf5ece --- /dev/null +++ b/src/Model/QuestionType/TableColumnRule.php @@ -0,0 +1,182 @@ +getInputType() === 'number'; + + if (!$required && $pattern === '' && $validator === null && !$numeric) { + return null; + } + + // Delegating the match itself to the native handler keeps a single regex + // implementation across GLPI. A pattern is only ever set on a scalar + // column, so the type is guaranteed here. + $pattern_handler = ($pattern !== '' && $scalar_type instanceof AbstractQuestionTypeShortAnswer) + ? new RegexConditionHandler($scalar_type, null) + : null; + + return new self( + name: (string) ($column[TableQuestionConfig::COL_NAME] ?? ''), + required: $required, + pattern: $pattern, + validator: $validator, + numeric: $numeric, + pattern_handler: $pattern_handler, + ); + } + + /** + * A configured pattern is usable when it is a `/…/flags` PCRE that actually + * compiles. The delimited form is the one the JS side parses, so anything + * else would end up evaluated differently on each side. + * + * Core has no primitive for this — its handlers only ever answer "does this + * value match" — and a malformed regex raises a warning, hence the silenced + * call, exactly as RegexConditionHandler does. + */ + public static function isUsablePattern(string $pattern): bool + { + // @phpstan-ignore theCodingMachineSafe.function + if (@preg_match('/^\/.*\/[a-z]*$/s', $pattern) !== 1) { + return false; + } + + // @phpstan-ignore theCodingMachineSafe.function + return @preg_match($pattern, '') !== false; + } + + /** + * Runs the format check owned by the column's question type. + * + * @return string|null The type's own message, or null when it is satisfied. + */ + public function validateNatively(Question $question, string $value): ?string + { + if ($this->validator instanceof QuestionTypeValidationInterface) { + $result = $this->validator->validateAnswer($question, $value); + if ($result->isValid()) { + return null; + } + + $error = $result->getErrors()[0] ?? null; + $message = is_array($error) ? ($error['message'] ?? '') : ''; + + return is_string($message) && $message !== '' + ? $message + : __('the value is not valid', 'advancedforms'); + } + + if ($this->numeric && !is_numeric($value)) { + return __('the value is not a number', 'advancedforms'); + } + + return null; + } + + /** + * True when no pattern is configured, or when the value satisfies it. A + * malformed stored pattern matches everything rather than locking the user + * out of the form. + */ + public function matchesPattern(string $value): bool + { + if (!$this->pattern_handler instanceof RegexConditionHandler) { + return true; + } + + return $this->pattern_handler->applyValueOperator( + $value, + ValueOperator::MATCH_REGEX, + $this->pattern, + ); + } +} diff --git a/src/Model/QuestionType/TableQuestion.php b/src/Model/QuestionType/TableQuestion.php index 84889cf..be46ac6 100644 --- a/src/Model/QuestionType/TableQuestion.php +++ b/src/Model/QuestionType/TableQuestion.php @@ -40,7 +40,6 @@ use Glpi\Form\QuestionType\QuestionTypeUrgency; use CommonItilObject_Item; use Dropdown; -use GLPIMailer; use Glpi\Application\View\TemplateRenderer; use Glpi\Form\Question; use Glpi\Form\QuestionType\AbstractQuestionType; @@ -55,22 +54,22 @@ use Glpi\Form\QuestionType\QuestionTypeUserDevice; use Glpi\Form\QuestionType\QuestionTypesManager; use Glpi\Form\Condition\ConditionHandler\EmptyConditionHandler; +use Glpi\Form\Condition\ConditionHandler\RegexConditionHandler; use Glpi\Form\Condition\ConditionHandler\VisibilityConditionHandler; use Glpi\Form\Condition\ConditionValueTransformerInterface; use Glpi\Form\QuestionType\QuestionTypeValidationInterface; use Glpi\Form\QuestionType\RawAnswerIsHtmlInterface; use Glpi\Form\ValidationResult; use Glpi\DBAL\JsonFieldInterface; +use GlpiPlugin\Advancedforms\Model\Condition\TableRowCountConditionHandler; use GlpiPlugin\Advancedforms\Model\Config\ConfigurableItemInterface; use GlpiPlugin\Advancedforms\Model\QuestionType\LdapQuestion; use Override; -use Safe\Exceptions\PcreException; use Session; use User; use function Safe\json_decode; use function Safe\json_encode; -use function Safe\preg_match; final class TableQuestion extends AbstractQuestionType implements ConfigurableItemInterface, @@ -138,25 +137,15 @@ public function validateExtraDataInput(array $input): bool return false; } - if ($pattern !== '') { - // Only `/…/flags`: matches the JS and HTML `pattern` attribute delimiter stripping. - if (preg_match('/^\/.*\/[a-z]*$/s', $pattern) !== 1) { - return false; - } - - try { - @preg_match($pattern, ''); // malformed regex logs a PHP warning, silence it - } catch (PcreException) { - return false; - } + // Same notion of a usable pattern as the one enforced at validation + // time, so a configuration that is accepted here is never silently + // ignored later. + if ($pattern !== '' && !TableColumnRule::isUsablePattern($pattern)) { + return false; } } - $min_raw = $input[TableQuestionConfig::MIN_ROWS] ?? 1; - $max_raw = $input[TableQuestionConfig::MAX_ROWS] ?? 50; - $min = is_numeric($min_raw) ? (int) $min_raw : 1; - $max = is_numeric($max_raw) ? (int) $max_raw : 50; - return $min >= 1 && $max >= $min && $max <= 50; + return true; } /** @param array $input */ @@ -181,15 +170,8 @@ static function (mixed $col): array { array_filter((array) ($input[TableQuestionConfig::COLUMNS] ?? []), is_array(...)), )); - $min_raw = $input[TableQuestionConfig::MIN_ROWS] ?? 1; - $max_raw = $input[TableQuestionConfig::MAX_ROWS] ?? 50; - $min = max(1, is_numeric($min_raw) ? (int) $min_raw : 1); - $max = min(50, max($min, is_numeric($max_raw) ? (int) $max_raw : 50)); - return [ - TableQuestionConfig::COLUMNS => $columns, - TableQuestionConfig::MIN_ROWS => $min, - TableQuestionConfig::MAX_ROWS => $max, + TableQuestionConfig::COLUMNS => $columns, ]; } @@ -219,39 +201,8 @@ public function validateAnswer(Question $question, mixed $answer): ValidationRes return $result; } - $type_instances = []; - foreach (QuestionTypesManager::getInstance()->getQuestionTypes() as $type) { - $type_instances[$type::class] = $type; - } - - $required_columns = []; - $pattern_columns = []; - $native_columns = []; - foreach ($this->loadConfig($question)->getColumns() as $index => $col) { - if ($col[TableQuestionConfig::COL_REQUIRED]) { - $required_columns[$index] = $col[TableQuestionConfig::COL_NAME]; - } - - $pattern = $col[TableQuestionConfig::COL_PATTERN] ?? ''; - $fqcn = $col[TableQuestionConfig::COL_QUESTION_TYPE]; - if ($pattern !== '' && is_a($fqcn, AbstractQuestionTypeShortAnswer::class, true)) { - $pattern_columns[$index] = [$col[TableQuestionConfig::COL_NAME], $pattern]; - continue; - } - - // Text columns rely on the (optional) pattern above; Email/Number - // enforce their own native format server-side, since core does not - // expose them as a portable regex. - $native_type = $type_instances[$fqcn] ?? null; - if ($native_type instanceof AbstractQuestionTypeShortAnswer) { - $input_type = $native_type->getInputType(); - if ($input_type === 'email' || $input_type === 'number') { - $native_columns[$index] = [$col[TableQuestionConfig::COL_NAME], $input_type]; - } - } - } - - if ($required_columns === [] && $pattern_columns === [] && $native_columns === []) { + $rules = $this->buildColumnRules($question); + if ($rules === []) { return $result; } @@ -268,82 +219,104 @@ public function validateAnswer(Question $question, mixed $answer): ValidationRes continue; } - foreach ($required_columns as $index => $name) { - $value = $row['col_' . $index] ?? ''; - if (!is_scalar($value) || (string) $value === '') { - $result->addError($question, sprintf( - __('Row %1$s: the column "%2$s" is required.', 'advancedforms'), - $row_number, - $name, - )); + foreach ($rules as $index => $rule) { + $error = $this->validateCell($question, $rule, $row['col_' . $index] ?? '', $row_number); + if ($error !== null) { + $result->addError($question, $error); } } + } - foreach ($pattern_columns as $index => [$name, $pattern]) { - $value = $row['col_' . $index] ?? ''; - if (!is_scalar($value) || (string) $value === '') { - continue; - } + return $result; + } - try { - $matches_pattern = preg_match($pattern, (string) $value) === 1; - } catch (PcreException) { - continue; // malformed stored pattern: don't block submission on it - } + /** + * Collects the columns that carry at least one rule, keyed by column index. + * + * @return array + */ + private function buildColumnRules(Question $question): array + { + $type_instances = []; + foreach (QuestionTypesManager::getInstance()->getQuestionTypes() as $type) { + $type_instances[$type::class] = $type; + } - if (!$matches_pattern) { - $result->addError($question, sprintf( - __('Row %1$s: the column "%2$s" does not match the expected format.', 'advancedforms'), - $row_number, - $name, - )); - } + $rules = []; + foreach ($this->loadConfig($question)->getColumns() as $index => $col) { + $type = $type_instances[$col[TableQuestionConfig::COL_QUESTION_TYPE]] ?? null; + $rule = TableColumnRule::fromColumn($col, $type); + if ($rule instanceof TableColumnRule) { + $rules[(int) $index] = $rule; } + } - foreach ($native_columns as $index => [$name, $input_type]) { - $value = $row['col_' . $index] ?? ''; - if (!is_scalar($value) || (string) $value === '') { - continue; - } + return $rules; + } + + /** + * Applies a column's rules to a single cell and returns the error to report, + * or null when the cell is acceptable. At most one message is produced per + * cell: telling the user that a value is both badly formatted and not a valid + * e-mail address helps nobody. + */ + private function validateCell( + Question $question, + TableColumnRule $rule, + mixed $value, + int $row_number, + ): ?string { + if (!is_scalar($value) || (string) $value === '') { + if (!$rule->required) { + return null; + } + + return sprintf( + __('Row %1$s: the column "%2$s" is required.', 'advancedforms'), + $row_number, + $rule->name, + ); + } - $is_valid = $input_type === 'email' - ? GLPIMailer::validateAddress((string) $value) - : is_numeric($value); + $value = (string) $value; - if (!$is_valid) { - $result->addError($question, sprintf( - __('Row %1$s: the column "%2$s" does not match the expected format.', 'advancedforms'), - $row_number, - $name, - )); - } - } + // The column's own question type knows its format best, so let it speak + // first: its message is more specific than a generic format complaint. + $native_message = $rule->validateNatively($question, $value); + if ($native_message !== null) { + return sprintf( + __('Row %1$s: the column "%2$s" is invalid: %3$s', 'advancedforms'), + $row_number, + $rule->name, + $native_message, + ); } - return $result; + if (!$rule->matchesPattern($value)) { + return sprintf( + __('Row %1$s: the column "%2$s" does not match the expected format.', 'advancedforms'), + $row_number, + $rule->name, + ); + } + + return null; } #[Override] public function getConditionHandlers(?JsonFieldInterface $question_config): array { - // No regex handler here: it targets the whole value, not a column. See validateAnswer(). return [ new VisibilityConditionHandler(), new EmptyConditionHandler($this, $question_config), + // Targets every cell of the table at once, as a complement to the + // per-column patterns enforced by validateAnswer(). + new RegexConditionHandler($this, $question_config), + // Row bounds, declared as conditions instead of a hardcoded limit. + new TableRowCountConditionHandler(), ]; } - /** - * Strips `/regex/flags` down to `regex` for the HTML `pattern` attribute, which - * takes no delimiters or flags. JS and server-side validation apply the real check. - */ - private function stripRegexDelimiters(string $pattern): string - { - preg_match('/^\/(.*)\/[a-z]*$/s', $pattern, $matches); - - return $matches[1] ?? $pattern; - } - /** * @param array $row */ @@ -723,8 +696,6 @@ public function renderAdvancedConfigurationTemplate(?Question $question): string 'COL_REQUIRED' => TableQuestionConfig::COL_REQUIRED, 'COL_ITEMTYPE' => TableQuestionConfig::COL_ITEMTYPE, 'COL_PATTERN' => TableQuestionConfig::COL_PATTERN, - 'MIN_ROWS' => TableQuestionConfig::MIN_ROWS, - 'MAX_ROWS' => TableQuestionConfig::MAX_ROWS, 'itemtype_options' => $itemtype_options, 'short_answer_fqcns' => $short_answer_fqcns, 'short_answer_fqcns_json' => json_encode($short_answer_fqcns), @@ -768,10 +739,22 @@ public function renderEndUserTemplate(Question $question): string } else { $cell_map[$index] = $this->getCellInfo($fqcn, $type); } + } - $pattern = $col[TableQuestionConfig::COL_PATTERN] ?? ''; - if ($pattern !== '' && ($cell_map[$index]['mode'] ?? '') === 'input') { - $cell_map[$index]['pattern'] = $this->stripRegexDelimiters($pattern); + // Built from the very same rules validateAnswer() uses, and keyed like + // the submitted field names. Both points matter: deriving these lists + // separately, with numeric keys, is what once let a column inherit its + // neighbour's pattern. + $required_cols = []; + $pattern_cols = []; + foreach ($this->buildColumnRules($question) as $index => $rule) { + $key = 'col_' . $index; + if ($rule->required) { + $required_cols[] = $key; + } + + if ($rule->pattern !== '') { + $pattern_cols[$key] = $rule->pattern; } } @@ -779,10 +762,12 @@ public function renderEndUserTemplate(Question $question): string return $twig->render( '@advancedforms/table_end_user.html.twig', [ - 'question' => $question, - 'config' => $config, - 'column_cell_map' => $cell_map, - 'ajax_limit_count' => $this->ajaxLimitCount(), + 'question' => $question, + 'config' => $config, + 'column_cell_map' => $cell_map, + 'ajax_limit_count' => $this->ajaxLimitCount(), + 'required_cols' => $required_cols, + 'pattern_cols_json' => json_encode($pattern_cols, JSON_FORCE_OBJECT), ], ); } diff --git a/src/Model/QuestionType/TableQuestionConfig.php b/src/Model/QuestionType/TableQuestionConfig.php index 03d90f1..da27c27 100644 --- a/src/Model/QuestionType/TableQuestionConfig.php +++ b/src/Model/QuestionType/TableQuestionConfig.php @@ -40,10 +40,6 @@ { public const COLUMNS = 'columns'; - public const MIN_ROWS = 'min_rows'; - - public const MAX_ROWS = 'max_rows'; - // Column sub-keys public const COL_NAME = 'name'; @@ -59,16 +55,16 @@ * @param array $columns */ public function __construct( - private array $columns = [], - private int $min_rows = 1, - private int $max_rows = 50, + private array $columns = [], ) {} /** + * Row bounds used to live here as `min_rows` / `max_rows`. They are now + * declared as native validation conditions, so those keys are ignored when + * they are found in data written by version 1.2.0. + * * @param array{ - * columns?: array, - * min_rows?: int, - * max_rows?: int + * columns?: array * } $data */ #[Override] @@ -85,30 +81,19 @@ public static function jsonDeserialize(array $data): self array_filter($data[self::COLUMNS] ?? [], is_array(...)), )); - $min_rows = min(50, max(1, (int) ($data[self::MIN_ROWS] ?? 1))); - $max_rows = min(50, max($min_rows, (int) ($data[self::MAX_ROWS] ?? 50))); - - return new self( - columns: $columns, - min_rows: $min_rows, - max_rows: $max_rows, - ); + return new self(columns: $columns); } /** * @return array{ - * columns: array, - * min_rows: int, - * max_rows: int + * columns: array * } */ #[Override] public function jsonSerialize(): array { return [ - self::COLUMNS => $this->columns, - self::MIN_ROWS => $this->min_rows, - self::MAX_ROWS => $this->max_rows, + self::COLUMNS => $this->columns, ]; } @@ -117,14 +102,4 @@ public function getColumns(): array { return $this->columns; } - - public function getMinRows(): int - { - return $this->min_rows; - } - - public function getMaxRows(): int - { - return $this->max_rows; - } } diff --git a/templates/editor/question_types/table_config.html.twig b/templates/editor/question_types/table_config.html.twig index f5e519c..af9bb0c 100644 --- a/templates/editor/question_types/table_config.html.twig +++ b/templates/editor/question_types/table_config.html.twig @@ -257,36 +257,9 @@ data-af-table-column-add="{{ rand }}" > -
- -
- - -
+ {# How many rows are acceptable is declared through the form's own + # validation conditions ("Length is greater than…"), which also + # makes the row count usable as a visibility criterion. #} @@ -308,6 +281,6 @@ diff --git a/templates/table_end_user.html.twig b/templates/table_end_user.html.twig index 13ec991..e1fbaaa 100644 --- a/templates/table_end_user.html.twig +++ b/templates/table_end_user.html.twig @@ -29,32 +29,20 @@ # ------------------------------------------------------------------------- #} +{# `required_cols` and `pattern_cols_json` are built by the PHP renderer, from the + # same rules the server validates against. They must not be recomputed here: the + # Twig `merge` filter renumbers integer keys, which used to shift a pattern onto + # the neighbouring column. #} {% set rand_eu = random() %} {% set input_base = question.getEndUserInputName() %} -{% set min_rows = config.getMinRows() %} -{% set max_rows = config.getMaxRows() %} {% set columns = config.getColumns() %} -{% set required_cols = [] %} -{% for col_index, col in columns %} - {% if col.required %}{% set required_cols = required_cols|merge([col_index]) %}{% endif %} -{% endfor %} - -{% set pattern_cols = {} %} -{% for col_index, col in columns %} - {% if col.pattern is defined and col.pattern != '' %} - {% set pattern_cols = pattern_cols|merge({(col_index): col.pattern}) %} - {% endif %} -{% endfor %} -
@@ -70,60 +58,61 @@ - {% for row_index in 0..(min_rows - 1) %} - - {% for col_index, col in columns %} - {% set cell = column_cell_map[col_index] %} - + {% for col_index, col in columns %} + {% set cell = column_cell_map[col_index] %} + - {% endfor %} - - - {% endfor %} + {% endfor %} + +
- {% if cell.mode == 'checkbox' %} -
- -
- {% elseif cell.mode == 'select' %} - {% set sel_html %} - {% do call('Dropdown::showFromArray', [ - input_base ~ '[' ~ row_index ~ '][col_' ~ col_index ~ ']', - cell.options, - { - 'class' : 'form-select form-select-sm', - 'width' : '100%', - 'display_emptychoice': false, - } - ]) %} - {% endset %} - {{ sel_html|raw }} - {% else %} + {# A single row to start with: how many rows are acceptable is now a + # validation condition, not a rendering concern. Its remove button + # starts disabled, since it is also the last one. #} + {% set row_index = 0 %} +
+ {% if cell.mode == 'checkbox' %} +
- {% endif %} -
- + + {% elseif cell.mode == 'select' %} + {% set sel_html %} + {% do call('Dropdown::showFromArray', [ + input_base ~ '[' ~ row_index ~ '][col_' ~ col_index ~ ']', + cell.options, + { + 'class' : 'form-select form-select-sm', + 'width' : '100%', + 'display_emptychoice': false, + } + ]) %} + {% endset %} + {{ sel_html|raw }} + {% else %} + + {% endif %}
+ +
@@ -161,7 +150,6 @@ name="{{ input_base }}[__ROW__][col_{{ col_index }}]" placeholder="{{ col.name }}" {% if col.required %} required {% endif %} - {% if cell.pattern is defined and cell.pattern != '' %} pattern="{{ cell.pattern }}" {% endif %} {% for attr_name, attr_value in cell.attributes|default({}) %} {{ attr_name }}="{{ attr_value }}" {% endfor %} > {% endif %} @@ -181,9 +169,14 @@ + {# Core reports one error per question but appends a copy next to every input + # it finds, which for a table means one per cell. AfTableQuestion relocates + # the distinct messages here. #} +
+ diff --git a/tests/Model/Condition/TableRowCountConditionHandlerTest.php b/tests/Model/Condition/TableRowCountConditionHandlerTest.php new file mode 100644 index 0000000..23fcee6 --- /dev/null +++ b/tests/Model/Condition/TableRowCountConditionHandlerTest.php @@ -0,0 +1,203 @@ +handler = new TableRowCountConditionHandler(); + } + + public function testOnlyLengthOperatorsAreSupported(): void + { + $this->assertSame( + [ + ValueOperator::LENGTH_GREATER_THAN, + ValueOperator::LENGTH_GREATER_THAN_OR_EQUALS, + ValueOperator::LENGTH_LESS_THAN, + ValueOperator::LENGTH_LESS_THAN_OR_EQUALS, + ], + $this->handler->getSupportedValueOperators(), + ); + } + + /** + * Every supported operator is usable as a validation criterion, which is the + * whole point of picking this family over GREATER_THAN/LESS_THAN. + */ + public function testEverySupportedOperatorCanBeUsedForValidation(): void + { + foreach ($this->handler->getSupportedValueOperators() as $operator) { + $this->assertTrue( + $operator->canBeUsedForValidation(), + $operator->value . ' must be usable as a validation criterion', + ); + } + } + + public function testTheAdminInputIsANumberField(): void + { + $parameters = $this->handler->getTemplateParameters( + $this->conditionData(ValueOperator::LENGTH_GREATER_THAN, '3'), + ); + + $this->assertSame('number', $parameters['attributes']['type'] ?? null); + } + + /** + * @param list> $rows + */ + #[DataProvider('provideRowCounts')] + public function testRowCountComparison( + array $rows, + ValueOperator $operator, + string $value, + bool $expected, + ): void { + $this->assertSame( + $expected, + $this->handler->applyValueOperator($rows, $operator, $value), + ); + } + + /** @return iterable>, ValueOperator, string, bool}> */ + public static function provideRowCounts(): iterable + { + $one = [['col_0' => 'a']]; + $three = [['col_0' => 'a'], ['col_0' => 'b'], ['col_0' => 'c']]; + + yield '3 rows > 2' => [$three, ValueOperator::LENGTH_GREATER_THAN, '2', true]; + yield '3 rows > 3' => [$three, ValueOperator::LENGTH_GREATER_THAN, '3', false]; + yield '3 rows > 4' => [$three, ValueOperator::LENGTH_GREATER_THAN, '4', false]; + yield '3 rows >= 3' => [$three, ValueOperator::LENGTH_GREATER_THAN_OR_EQUALS, '3', true]; + yield '3 rows >= 4' => [$three, ValueOperator::LENGTH_GREATER_THAN_OR_EQUALS, '4', false]; + yield '1 row < 2' => [$one, ValueOperator::LENGTH_LESS_THAN, '2', true]; + yield '3 rows < 3' => [$three, ValueOperator::LENGTH_LESS_THAN, '3', false]; + yield '3 rows <= 3' => [$three, ValueOperator::LENGTH_LESS_THAN_OR_EQUALS, '3', true]; + yield '3 rows <= 2' => [$three, ValueOperator::LENGTH_LESS_THAN_OR_EQUALS, '2', false]; + yield 'no row < 1' => [[], ValueOperator::LENGTH_LESS_THAN, '1', true]; + yield 'no row > 0' => [[], ValueOperator::LENGTH_GREATER_THAN, '0', false]; + yield 'no row >= 0' => [[], ValueOperator::LENGTH_GREATER_THAN_OR_EQUALS, '0', true]; + } + + /** + * Empty rows are dropped when the answer is saved, so they must not be + * counted either. + */ + public function testEmptyRowsAreNotCounted(): void + { + $rows = [ + ['col_0' => 'filled', 'col_1' => ''], + ['col_0' => '', 'col_1' => ''], + ['col_0' => null, 'col_1' => null], + ]; + + $this->assertTrue( + $this->handler->applyValueOperator($rows, ValueOperator::LENGTH_LESS_THAN_OR_EQUALS, '1'), + ); + } + + public function testWhitespaceOnlyRowIsCounted(): void + { + // Not trimmed: a cell holding a space is a value as far as storage goes. + $rows = [['col_0' => ' ']]; + + $this->assertTrue( + $this->handler->applyValueOperator($rows, ValueOperator::LENGTH_GREATER_THAN_OR_EQUALS, '1'), + ); + } + + public function testNonArrayRowsAreIgnored(): void + { + $rows = ['not-a-row', ['col_0' => 'a'], 42]; + + $this->assertTrue( + $this->handler->applyValueOperator($rows, ValueOperator::LENGTH_LESS_THAN_OR_EQUALS, '1'), + ); + } + + public function testUnansweredTableCountsAsZeroRows(): void + { + $this->assertTrue( + $this->handler->applyValueOperator(null, ValueOperator::LENGTH_LESS_THAN, '1'), + ); + } + + public function testUnsupportedOperatorIsRejected(): void + { + $this->assertFalse( + $this->handler->applyValueOperator( + [['col_0' => 'a']], + ValueOperator::EQUALS, + '1', + ), + ); + } + + public function testNonNumericThresholdIsTreatedAsZero(): void + { + $this->assertTrue( + $this->handler->applyValueOperator( + [['col_0' => 'a']], + ValueOperator::LENGTH_GREATER_THAN, + 'not-a-number', + ), + ); + } + + private function conditionData(ValueOperator $operator, string $value): ConditionData + { + return new ConditionData( + item_uuid: 'uuid', + item_type: Type::QUESTION->value, + value_operator: $operator->value, + value: $value, + ); + } +} diff --git a/tests/Model/Condition/TableRowCountConditionTest.php b/tests/Model/Condition/TableRowCountConditionTest.php new file mode 100644 index 0000000..23181a7 --- /dev/null +++ b/tests/Model/Condition/TableRowCountConditionTest.php @@ -0,0 +1,323 @@ +type = new TableQuestion(); + $this->login(); + $this->enableConfigurableItem($this->type); + } + + public function testInvalidIfRowCountGreaterThanRejectsTooManyRows(): void + { + $form = $this->formWithRowCountValidation( + ValidationStrategy::INVALID_IF, + ValueOperator::LENGTH_GREATER_THAN, + '2', + ); + + $this->assertFalse($this->validate($form, $this->rows(3))->isValid()); + } + + public function testInvalidIfRowCountGreaterThanAcceptsTheExactLimit(): void + { + $form = $this->formWithRowCountValidation( + ValidationStrategy::INVALID_IF, + ValueOperator::LENGTH_GREATER_THAN, + '2', + ); + + $this->assertTrue($this->validate($form, $this->rows(2))->isValid()); + } + + public function testInvalidIfRowCountLessThanRejectsTooFewRows(): void + { + $form = $this->formWithRowCountValidation( + ValidationStrategy::INVALID_IF, + ValueOperator::LENGTH_LESS_THAN, + '3', + ); + + $this->assertFalse($this->validate($form, $this->rows(2))->isValid()); + } + + public function testInvalidIfRowCountLessThanAcceptsEnoughRows(): void + { + $form = $this->formWithRowCountValidation( + ValidationStrategy::INVALID_IF, + ValueOperator::LENGTH_LESS_THAN, + '3', + ); + + $this->assertTrue($this->validate($form, $this->rows(3))->isValid()); + } + + public function testValidIfRowCountLessThanOrEqualsAcceptsWithinBounds(): void + { + $form = $this->formWithRowCountValidation( + ValidationStrategy::VALID_IF, + ValueOperator::LENGTH_LESS_THAN_OR_EQUALS, + '5', + ); + + $this->assertTrue($this->validate($form, $this->rows(5))->isValid()); + } + + public function testValidIfRowCountLessThanOrEqualsRejectsBeyondBounds(): void + { + $form = $this->formWithRowCountValidation( + ValidationStrategy::VALID_IF, + ValueOperator::LENGTH_LESS_THAN_OR_EQUALS, + '5', + ); + + $this->assertFalse($this->validate($form, $this->rows(6))->isValid()); + } + + public function testValidIfRowCountGreaterThanOrEqualsRejectsTooFewRows(): void + { + $form = $this->formWithRowCountValidation( + ValidationStrategy::VALID_IF, + ValueOperator::LENGTH_GREATER_THAN_OR_EQUALS, + '2', + ); + + $this->assertFalse($this->validate($form, $this->rows(1))->isValid()); + } + + public function testValidIfRowCountGreaterThanOrEqualsAcceptsEnoughRows(): void + { + $form = $this->formWithRowCountValidation( + ValidationStrategy::VALID_IF, + ValueOperator::LENGTH_GREATER_THAN_OR_EQUALS, + '2', + ); + + $this->assertTrue($this->validate($form, $this->rows(2))->isValid()); + } + + public function testEmptyRowsDoNotCountTowardsTheLimit(): void + { + $form = $this->formWithRowCountValidation( + ValidationStrategy::INVALID_IF, + ValueOperator::LENGTH_GREATER_THAN, + '2', + ); + + // Five submitted rows, but only two carry a value. + $answer = [ + ['col_0' => 'a'], + ['col_0' => ''], + ['col_0' => 'b'], + ['col_0' => ''], + ['col_0' => ''], + ]; + + $this->assertTrue($this->validate($form, $answer)->isValid()); + } + + public function testTheErrorMessageComesFromTheNativeOperator(): void + { + $form = $this->formWithRowCountValidation( + ValidationStrategy::INVALID_IF, + ValueOperator::LENGTH_GREATER_THAN, + '2', + ); + + $errors = $this->validate($form, $this->rows(3))->getErrors(); + + $this->assertCount(1, $errors); + $this->assertStringContainsString('2', $errors[0]['message']); + } + + /** + * AnswersHandler skips the validation conditions of a question it received + * no answer for. A table only reaches that state when the browser posts + * nothing at all for it, which the next test rules out for any column that + * is not a lone checkbox. + */ + public function testAnAbsentAnswerSkipsRowCountValidation(): void + { + $form = $this->formWithRowCountValidation( + ValidationStrategy::VALID_IF, + ValueOperator::LENGTH_GREATER_THAN_OR_EQUALS, + '2', + ); + + $this->assertTrue($this->validate($form, [])->isValid()); + } + + /** + * An untouched table still posts its rendered row, with empty cells, so a + * minimum row count is enforced without the question having to be mandatory. + */ + public function testAnUntouchedTableStillFailsAMinimumRowCount(): void + { + $form = $this->formWithRowCountValidation( + ValidationStrategy::VALID_IF, + ValueOperator::LENGTH_GREATER_THAN_OR_EQUALS, + '2', + ); + + $this->assertFalse($this->validate($form, [['col_0' => '']])->isValid()); + } + + public function testRowCountDrivesVisibilityOfAnotherQuestion(): void + { + $builder = new FormBuilder('Row count visibility form'); + $builder->addQuestion( + 'Table', + TableQuestion::class, + extra_data: $this->extraData(), + ); + $builder->addQuestion('Follow up', QuestionTypeShortText::class); + $builder->setQuestionVisibility( + 'Follow up', + VisibilityStrategy::VISIBLE_IF, + [ + [ + 'logic_operator' => LogicOperator::AND, + 'item_name' => 'Table', + 'item_type' => Type::QUESTION, + 'value_operator' => ValueOperator::LENGTH_GREATER_THAN, + 'value' => '2', + ], + ], + ); + $form = $this->createForm($builder); + + $table_id = $this->getQuestionId($form, 'Table'); + $follow_up = $this->getQuestionId($form, 'Follow up'); + + $this->assertTrue($this->isVisible($form, [$table_id => $this->rows(3)], $follow_up)); + $this->assertFalse($this->isVisible($form, [$table_id => $this->rows(2)], $follow_up)); + } + + /** @param array $answers */ + private function isVisible(Form $form, array $answers, int $question_id): bool + { + return (new Engine($form, new EngineInput($answers))) + ->computeVisibility() + ->isQuestionVisible($question_id); + } + + private function formWithRowCountValidation( + ValidationStrategy $strategy, + ValueOperator $operator, + string $value, + ): Form { + $builder = new FormBuilder('Row count validation form'); + $builder->addQuestion( + 'Table', + TableQuestion::class, + extra_data: $this->extraData(), + ); + $builder->setQuestionValidation( + 'Table', + $strategy, + [ + [ + 'logic_operator' => LogicOperator::AND, + 'item_name' => 'Table', + 'item_type' => Type::QUESTION, + 'value_operator' => $operator, + 'value' => $value, + ], + ], + ); + + return $this->createForm($builder); + } + + /** @param array $answer */ + private function validate(Form $form, array $answer): ValidationResult + { + return AnswersHandler::getInstance()->validateAnswers($form, [ + $this->getQuestionId($form, 'Table') => $answer, + ]); + } + + /** @return list */ + private function rows(int $count): array + { + $rows = []; + for ($i = 0; $i < $count; $i++) { + $rows[] = ['col_0' => 'value ' . $i]; + } + + return $rows; + } + + private function extraData(): string + { + return json_encode(new TableQuestionConfig(columns: [ + [ + TableQuestionConfig::COL_NAME => 'Name', + TableQuestionConfig::COL_QUESTION_TYPE => QuestionTypeShortText::class, + TableQuestionConfig::COL_REQUIRED => false, + TableQuestionConfig::COL_ITEMTYPE => '', + TableQuestionConfig::COL_PATTERN => '', + ], + ])); + } +} diff --git a/tests/Model/QuestionType/TableQuestionConfigTest.php b/tests/Model/QuestionType/TableQuestionConfigTest.php index 76c2ed7..9849972 100644 --- a/tests/Model/QuestionType/TableQuestionConfigTest.php +++ b/tests/Model/QuestionType/TableQuestionConfigTest.php @@ -44,8 +44,6 @@ public function testDefaultValues(): void { $config = new TableQuestionConfig(); $this->assertSame([], $config->getColumns()); - $this->assertSame(1, $config->getMinRows()); - $this->assertSame(50, $config->getMaxRows()); } public function testJsonRoundtrip(): void @@ -55,14 +53,17 @@ public function testJsonRoundtrip(): void ['name' => 'Source IP', 'question_type' => QuestionTypeShortText::class, 'required' => true, 'itemtype' => '', 'pattern' => '/^172\\.23\\./'], ['name' => 'Port', 'question_type' => QuestionTypeNumber::class, 'required' => false, 'itemtype' => '', 'pattern' => ''], ], - min_rows: 2, - max_rows: 20, ); $serialized = $original->jsonSerialize(); $deserialized = TableQuestionConfig::jsonDeserialize($serialized); $this->assertSame($original->getColumns(), $deserialized->getColumns()); - $this->assertSame($original->getMinRows(), $deserialized->getMinRows()); - $this->assertSame($original->getMaxRows(), $deserialized->getMaxRows()); + } + + public function testSerializedFormCarriesNoRowBounds(): void + { + $serialized = (new TableQuestionConfig())->jsonSerialize(); + + $this->assertSame([TableQuestionConfig::COLUMNS], array_keys($serialized)); } public function testColumnPatternDefaultsToEmptyStringWhenAbsent(): void @@ -79,35 +80,29 @@ public function testColumnPatternDefaultsToEmptyStringWhenAbsent(): void public function testJsonDeserializeFiltersNonArrayColumns(): void { $config = TableQuestionConfig::jsonDeserialize([ - 'columns' => ['not_array', ['name' => 'Valid', 'question_type' => 'SomeFqcn', 'required' => false]], - 'min_rows' => 1, - 'max_rows' => 50, + 'columns' => ['not_array', ['name' => 'Valid', 'question_type' => 'SomeFqcn', 'required' => false]], ]); $this->assertCount(1, $config->getColumns()); $this->assertSame('Valid', $config->getColumns()[0]['name']); } - public function testJsonDeserializeEnforcesMinRow(): void + /** + * Row bounds moved to native validation conditions. Configurations written by + * 1.2.0 still carry them, and must deserialize without complaint. + */ + public function testLegacyRowBoundsAreIgnored(): void { - $config = TableQuestionConfig::jsonDeserialize(['min_rows' => 0, 'max_rows' => 10]); - $this->assertSame(1, $config->getMinRows()); - } - - public function testJsonDeserializeEnforcesMaxRowNotZero(): void - { - $config = TableQuestionConfig::jsonDeserialize(['min_rows' => 1, 'max_rows' => 0]); - $this->assertSame(1, $config->getMaxRows()); - } - - public function testJsonDeserializeEnforcesMaxRowNotLessThanMin(): void - { - $config = TableQuestionConfig::jsonDeserialize(['min_rows' => 10, 'max_rows' => 5]); - $this->assertSame(10, $config->getMaxRows()); - } + $config = TableQuestionConfig::jsonDeserialize([ + 'columns' => [ + ['name' => 'Source IP', 'question_type' => QuestionTypeShortText::class, 'required' => true], + ], + 'min_rows' => 2, + 'max_rows' => 20, + ]); - public function testJsonDeserializePreservesMaxRowAtCap(): void - { - $config = TableQuestionConfig::jsonDeserialize(['min_rows' => 1, 'max_rows' => 50]); - $this->assertSame(50, $config->getMaxRows()); + $this->assertCount(1, $config->getColumns()); + $this->assertSame('Source IP', $config->getColumns()[0][TableQuestionConfig::COL_NAME]); + $this->assertArrayNotHasKey('min_rows', $config->jsonSerialize()); + $this->assertArrayNotHasKey('max_rows', $config->jsonSerialize()); } } diff --git a/tests/Model/QuestionType/TableQuestionIntegrationTest.php b/tests/Model/QuestionType/TableQuestionIntegrationTest.php index e168046..d1edac1 100644 --- a/tests/Model/QuestionType/TableQuestionIntegrationTest.php +++ b/tests/Model/QuestionType/TableQuestionIntegrationTest.php @@ -67,8 +67,6 @@ protected function getDefaultExtraDataForQuestionType(QuestionTypeInterface $typ TableQuestionConfig::COL_REQUIRED => false, ], ], - min_rows: 1, - max_rows: 50, )); } @@ -97,11 +95,12 @@ protected function validateHelpdeskRenderingWhenEnabled(Crawler $html): void $container = $html->filter('[data-af-table-question]'); $this->assertNotEmpty($container); - // Required columns must be exposed to the client validation layer. - // The default config marks the first column ("Source IP") as required. - $this->assertSame('0', $container->attr('data-af-required-cols')); + // Required columns must be exposed to the client validation layer, keyed + // like the submitted field names. The default config marks the first + // column ("Source IP") as required. + $this->assertSame('col_0', $container->attr('data-af-required-cols')); - // At least one input row rendered (min_rows = 1) + // At least one input row rendered $rows = $html->filter('[data-af-table-body] [data-af-table-row]'); $this->assertGreaterThanOrEqual(1, $rows->count()); diff --git a/tests/Model/QuestionType/TableQuestionRenderingTest.php b/tests/Model/QuestionType/TableQuestionRenderingTest.php new file mode 100644 index 0000000..5e7098b --- /dev/null +++ b/tests/Model/QuestionType/TableQuestionRenderingTest.php @@ -0,0 +1,336 @@ +type = new TableQuestion(); + $this->login(); + } + + /** + * Regression test for the reported bug: with a pattern on the first and last + * of three columns, the middle column was reported as badly formatted while + * the last column silently lost its client-side check. + */ + public function testPatternsAreMappedToTheirOwnColumn(): void + { + $patterns = $this->renderedPatternColumns([ + $this->column('SRC IP', QuestionTypeShortText::class, pattern: '/^[0-9.]+$/'), + $this->column('description', QuestionTypeShortText::class), + $this->column('DST IP', QuestionTypeShortText::class, pattern: '/^[a-f:]+$/'), + ]); + + $this->assertSame( + ['col_0' => '/^[0-9.]+$/', 'col_2' => '/^[a-f:]+$/'], + $patterns, + ); + } + + public function testColumnWithoutPatternIsAbsentFromThePayload(): void + { + $patterns = $this->renderedPatternColumns([ + $this->column('SRC IP', QuestionTypeShortText::class, pattern: '/^[0-9.]+$/'), + $this->column('description', QuestionTypeShortText::class), + $this->column('DST IP', QuestionTypeShortText::class, pattern: '/^[0-9.]+$/'), + ]); + + $this->assertArrayNotHasKey('col_1', $patterns); + } + + public function testTableWithoutAnyPatternExposesAnEmptyPayload(): void + { + $patterns = $this->renderedPatternColumns([ + $this->column('Name', QuestionTypeShortText::class), + $this->column('Comment', QuestionTypeShortText::class), + ]); + + $this->assertSame([], $patterns); + } + + public function testEveryColumnCanCarryItsOwnPattern(): void + { + $patterns = $this->renderedPatternColumns([ + $this->column('A', QuestionTypeShortText::class, pattern: '/^a$/'), + $this->column('B', QuestionTypeShortText::class, pattern: '/^b$/'), + $this->column('C', QuestionTypeShortText::class, pattern: '/^c$/'), + ]); + + $this->assertSame( + ['col_0' => '/^a$/', 'col_1' => '/^b$/', 'col_2' => '/^c$/'], + $patterns, + ); + } + + public function testOnlyTheLastColumnCarriesAPattern(): void + { + $patterns = $this->renderedPatternColumns([ + $this->column('A', QuestionTypeShortText::class), + $this->column('B', QuestionTypeShortText::class), + $this->column('C', QuestionTypeShortText::class, pattern: '/^c$/'), + ]); + + $this->assertSame(['col_2' => '/^c$/'], $patterns); + } + + public function testRequiredColumnsAreMappedToTheirOwnColumn(): void + { + $required = $this->renderedRequiredColumns([ + $this->column('A', QuestionTypeShortText::class, required: true), + $this->column('B', QuestionTypeShortText::class), + $this->column('C', QuestionTypeShortText::class, required: true), + ]); + + $this->assertSame(['col_0', 'col_2'], $required); + } + + public function testTableWithoutAnyRequiredColumnExposesAnEmptyList(): void + { + $required = $this->renderedRequiredColumns([ + $this->column('A', QuestionTypeShortText::class), + $this->column('B', QuestionTypeShortText::class), + ]); + + $this->assertSame([], $required); + } + + /** + * The HTML `pattern` attribute is implicitly anchored and drops flags, so it + * disagrees with both the PHP and the JS check. It must not be emitted. + */ + public function testNoHtmlPatternAttributeIsEmitted(): void + { + $html = $this->render([ + $this->column('Prefix', QuestionTypeShortText::class, pattern: '/^172\.23\./'), + ]); + + $crawler = new Crawler($html); + $inputs = $crawler->filter('[data-af-table-body] input'); + + $this->assertGreaterThan(0, $inputs->count()); + foreach ($inputs as $input) { + $this->assertFalse( + $input->hasAttribute('pattern'), + 'The end-user table must not rely on the anchored HTML pattern attribute.', + ); + } + } + + public function testRowTemplateAlsoOmitsTheHtmlPatternAttribute(): void + { + $html = $this->render([ + $this->column('Prefix', QuestionTypeShortText::class, pattern: '/^172\.23\./'), + ]); + + // The cloned-row template is not part of the DOM tree, inspect the markup. + $this->assertStringNotContainsString('pattern="', $html); + } + + public function testCheckboxColumnPatternIsNotExposedToTheClient(): void + { + // The config UI only offers a pattern on text columns; a hand-crafted one + // on a checkbox column has no input to validate and must be dropped. + $patterns = $this->renderedPatternColumns([ + $this->column('Flag', QuestionTypeCheckbox::class, pattern: '/^yes$/'), + ]); + + $this->assertSame([], $patterns); + } + + public function testEmailColumnKeepsItsNativeInputType(): void + { + $html = $this->render([$this->column('Contact', QuestionTypeEmail::class)]); + $crawler = new Crawler($html); + + $this->assertSame( + 'email', + $crawler->filter('[data-af-table-body] input')->first()->attr('type'), + ); + } + + public function testASingleRowIsRenderedByDefault(): void + { + $html = $this->render([$this->column('Name', QuestionTypeShortText::class)]); + $crawler = new Crawler($html); + + $this->assertSame(1, $crawler->filter('[data-af-table-body] [data-af-table-row]')->count()); + } + + /** + * Row bounds are now declared as native validation conditions, so the + * renderer must not ship any min/max row hint to the client. + */ + public function testNoRowBoundsAreExposedToTheClient(): void + { + $html = $this->render([$this->column('Name', QuestionTypeShortText::class)]); + $crawler = new Crawler($html); + $table = $crawler->filter('[data-af-table-question]')->first(); + + $this->assertNull($table->attr('data-af-min-rows')); + $this->assertNull($table->attr('data-af-max-rows')); + } + + /** + * Server-side errors are reported per question but injected by the core + * renderer next to every input, so a table needs somewhere to gather them. + */ + public function testASingleContainerIsProvidedForServerErrors(): void + { + $html = $this->render([$this->column('Name', QuestionTypeShortText::class)]); + $crawler = new Crawler($html); + + $this->assertSame(1, $crawler->filter('[data-af-table-errors]')->count()); + } + + /** + * The module must be imported through its import map key. A `?v=` of our own + * would not match that key, and would cost both the content-based cache + * busting and the root_doc prefix. + */ + public function testTheJsModuleIsImportedThroughTheImportMap(): void + { + $html = $this->render([$this->column('Name', QuestionTypeShortText::class)]); + + $this->assertStringContainsString( + "from '" . self::MODULE_PATH . "'", + $html, + ); + } + + public function testTheImportMapVersionsTheModuleOnItsContent(): void + { + $imports = ImportMapGenerator::getInstance()->generate()['imports']; + + $this->assertArrayHasKey( + self::MODULE_PATH, + $imports, + 'setup.php must register the plugin modules directory.', + ); + $this->assertMatchesRegularExpression( + '#' . preg_quote(self::MODULE_PATH, '#') . '\?v=[0-9a-f]+$#', + $imports[self::MODULE_PATH], + ); + } + + /** + * @param array $columns + * @return array Decoded `data-af-pattern-cols` payload. + */ + private function renderedPatternColumns(array $columns): array + { + $crawler = new Crawler($this->render($columns)); + $raw = $crawler->filter('[data-af-table-question]')->first()->attr('data-af-pattern-cols'); + + $decoded = json_decode((string) $raw, associative: true); + $this->assertIsArray($decoded, 'data-af-pattern-cols must hold a JSON object.'); + + return $decoded; + } + + /** + * @param array $columns + * @return list Parsed `data-af-required-cols` payload. + */ + private function renderedRequiredColumns(array $columns): array + { + $crawler = new Crawler($this->render($columns)); + $raw = $crawler->filter('[data-af-table-question]')->first()->attr('data-af-required-cols'); + + return array_values(array_filter(explode(',', (string) $raw), fn(string $v): bool => $v !== '')); + } + + /** + * @param array $columns + */ + private function render(array $columns): string + { + $this->enableConfigurableItem($this->type); + + $builder = new FormBuilder('Rendering form'); + $builder->addQuestion( + 'Table', + TableQuestion::class, + extra_data: json_encode(new TableQuestionConfig(columns: $columns)), + ); + $form = $this->createForm($builder); + + $question = Question::getById($this->getQuestionId($form, 'Table')); + + return $this->type->renderEndUserTemplate($question); + } + + /** + * @return array{name: string, question_type: string, required: bool, itemtype: string, pattern: string} + */ + private function column( + string $name, + string $fqcn, + bool $required = false, + string $pattern = '', + ): array { + return [ + TableQuestionConfig::COL_NAME => $name, + TableQuestionConfig::COL_QUESTION_TYPE => $fqcn, + TableQuestionConfig::COL_REQUIRED => $required, + TableQuestionConfig::COL_ITEMTYPE => '', + TableQuestionConfig::COL_PATTERN => $pattern, + ]; + } +} diff --git a/tests/Model/QuestionType/TableQuestionTest.php b/tests/Model/QuestionType/TableQuestionTest.php index 24c8e99..ea22748 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\Condition\ValueOperator; use Glpi\Form\QuestionType\QuestionTypeCheckbox; use Glpi\Form\QuestionType\QuestionTypeEmail; use Glpi\Form\QuestionType\QuestionTypeFile; @@ -69,8 +70,6 @@ public function testPrepareExtraDataReindexesColumns(): void $result = $this->type->prepareExtraData($input); $this->assertArrayHasKey(0, $result[TableQuestionConfig::COLUMNS]); $this->assertArrayHasKey(1, $result[TableQuestionConfig::COLUMNS]); - $this->assertSame(2, $result[TableQuestionConfig::MIN_ROWS]); - $this->assertSame(20, $result[TableQuestionConfig::MAX_ROWS]); } public function testPrepareExtraDataCoercesRequiredToBool(): void @@ -170,44 +169,91 @@ public function testGetExtraDataConfigClass(): void $this->assertSame(TableQuestionConfig::class, $this->type->getExtraDataConfigClass()); } - public function testValidateExtraDataInputAcceptsMaxRows50(): void + public function testValidateExtraDataInputAcceptsASingleColumn(): void { $result = $this->type->validateExtraDataInput([ - 'columns' => [['name' => 'A', 'question_type' => QuestionTypeShortText::class]], - 'min_rows' => 1, - 'max_rows' => 50, + 'columns' => [['name' => 'A', 'question_type' => QuestionTypeShortText::class]], ]); $this->assertTrue($result); } - public function testValidateExtraDataInputRejectsMaxRowsAbove50(): void + public function testValidateExtraDataInputRejectsAColumnLessTable(): void { - $result = $this->type->validateExtraDataInput([ - 'columns' => [['name' => 'A', 'question_type' => QuestionTypeShortText::class]], - 'min_rows' => 1, - 'max_rows' => 51, - ]); - $this->assertFalse($result); + $this->assertFalse($this->type->validateExtraDataInput(['columns' => []])); } - public function testValidateExtraDataInputRejectsCraftedLargeMaxRows(): void + /** + * Row bounds are now validation conditions; leftover keys from 1.2.0 must + * neither be rejected nor carried over. + */ + public function testLegacyRowBoundsAreAccepted(): void { $result = $this->type->validateExtraDataInput([ 'columns' => [['name' => 'A', 'question_type' => QuestionTypeShortText::class]], 'min_rows' => 1, 'max_rows' => 99999, ]); - $this->assertFalse($result); + $this->assertTrue($result); } - public function testPrepareExtraDataClampsMaxRowsTo50(): void + public function testPrepareExtraDataDropsLegacyRowBounds(): void { $result = $this->type->prepareExtraData([ 'columns' => [['name' => 'A', 'question_type' => QuestionTypeShortText::class, 'required' => false]], 'min_rows' => 1, 'max_rows' => 99999, ]); - $this->assertSame(50, $result[TableQuestionConfig::MAX_ROWS]); + $this->assertSame([TableQuestionConfig::COLUMNS], array_keys($result)); + } + + /** + * Engine::applyValueOperator() demands exactly one handler per operator and + * throws a LogicException otherwise — but only once an admin actually builds + * such a condition, so the clash has to be caught here. + */ + public function testNoValueOperatorIsClaimedByTwoConditionHandlers(): void + { + $seen = []; + foreach ($this->type->getConditionHandlers(null) as $handler) { + foreach ($handler->getSupportedValueOperators() as $operator) { + $this->assertArrayNotHasKey( + $operator->value, + $seen, + sprintf( + 'Operator "%s" is claimed by both %s and %s', + $operator->value, + $seen[$operator->value] ?? '', + $handler::class, + ), + ); + $seen[$operator->value] = $handler::class; + } + } + } + + public function testRowCountOperatorsAreAvailableAsConditionCriteria(): void + { + $operators = []; + foreach ($this->type->getConditionHandlers(null) as $handler) { + foreach ($handler->getSupportedValueOperators() as $operator) { + $operators[] = $operator; + } + } + + $this->assertContains(ValueOperator::LENGTH_GREATER_THAN, $operators); + $this->assertContains(ValueOperator::LENGTH_LESS_THAN_OR_EQUALS, $operators); + } + + public function testWholeTableRegexIsAvailableAsAConditionCriterion(): void + { + $operators = []; + foreach ($this->type->getConditionHandlers(null) as $handler) { + foreach ($handler->getSupportedValueOperators() as $operator) { + $operators[] = $operator; + } + } + + $this->assertContains(ValueOperator::MATCH_REGEX, $operators); } public function testTransformConditionValueFlattensRowsToScalars(): void diff --git a/tests/Model/QuestionType/TableQuestionValidationTest.php b/tests/Model/QuestionType/TableQuestionValidationTest.php index c70f2e9..80e7178 100644 --- a/tests/Model/QuestionType/TableQuestionValidationTest.php +++ b/tests/Model/QuestionType/TableQuestionValidationTest.php @@ -272,6 +272,35 @@ public function testNonSlashDelimitedPatternIsRejected(): void ])); } + public function testPatternWithASharedFlagIsAccepted(): void + { + $this->assertTrue($this->type->validateExtraDataInput([ + 'columns' => [ + $this->column('Env', QuestionTypeShortText::class, pattern: '/^prod$/i'), + ], + ])); + } + + public function testPatternWithAJsOnlyFlagIsRejected(): void + { + // `g` is not a PCRE modifier, and it would make the client-side `test()` + // stateful from one cell to the next. + $this->assertFalse($this->type->validateExtraDataInput([ + 'columns' => [ + $this->column('Env', QuestionTypeShortText::class, pattern: '/prod/g'), + ], + ])); + } + + public function testPatternWithoutDelimitersIsRejected(): void + { + $this->assertFalse($this->type->validateExtraDataInput([ + 'columns' => [ + $this->column('Env', QuestionTypeShortText::class, pattern: '^prod$'), + ], + ])); + } + public function testPatternIsIgnoredOnNonShortAnswerColumn(): void { // Config UI never exposes pattern for non-short-answer columns; a hand-crafted one must be ignored. @@ -381,6 +410,363 @@ public function testAnswersHandlerReportsMissingRequiredColumn(): void $this->assertFalse($result->isValid()); } + public function testUnanchoredPatternMatchesAPrefix(): void + { + // The HTML `pattern` attribute would anchor this and reject the value. + $question = $this->makeTableQuestion([ + $this->column('Source IP', QuestionTypeShortText::class, pattern: '/^172\.23\./'), + ]); + + $result = $this->type->validateAnswer($question, [['col_0' => '172.23.0.1']]); + + $this->assertTrue($result->isValid()); + } + + public function testUnanchoredPatternMatchesAnywhereInTheValue(): void + { + $question = $this->makeTableQuestion([ + $this->column('Label', QuestionTypeShortText::class, pattern: '/PROD/'), + ]); + + $result = $this->type->validateAnswer($question, [['col_0' => 'srv-PROD-01']]); + + $this->assertTrue($result->isValid()); + } + + public function testCaseInsensitiveFlagIsHonoured(): void + { + $question = $this->makeTableQuestion([ + $this->column('Env', QuestionTypeShortText::class, pattern: '/^prod$/i'), + ]); + + $result = $this->type->validateAnswer($question, [['col_0' => 'PROD']]); + + $this->assertTrue($result->isValid()); + } + + public function testCaseSensitivePatternStillRejectsWrongCase(): void + { + $question = $this->makeTableQuestion([ + $this->column('Env', QuestionTypeShortText::class, pattern: '/^prod$/'), + ]); + + $result = $this->type->validateAnswer($question, [['col_0' => 'PROD']]); + + $this->assertFalse($result->isValid()); + } + + public function testDotAllFlagIsHonoured(): void + { + $question = $this->makeTableQuestion([ + $this->column('Blob', QuestionTypeShortText::class, pattern: '/^a.b$/s'), + ]); + + $result = $this->type->validateAnswer($question, [['col_0' => "a\nb"]]); + + $this->assertTrue($result->isValid()); + } + + public function testUnicodeValueIsMatchedWithTheUnicodeFlag(): void + { + $question = $this->makeTableQuestion([ + $this->column('City', QuestionTypeShortText::class, pattern: '/^\p{L}+$/u'), + ]); + + $result = $this->type->validateAnswer($question, [['col_0' => 'Besançon']]); + + $this->assertTrue($result->isValid()); + } + + public function testMalformedStoredPatternDoesNotBlockSubmission(): void + { + // validateExtraDataInput() rejects these, so it has to be injected as + // already-stored data: a pattern that predates a rule change must never + // lock a user out of the form. + $question = $this->makeTableQuestionWithStoredConfig([ + $this->column('Broken', QuestionTypeShortText::class, pattern: '/[/'), + ]); + + $result = $this->type->validateAnswer($question, [['col_0' => 'anything']]); + + $this->assertTrue($result->isValid()); + } + + public function testUnknownStoredColumnTypeDoesNotBlockSubmission(): void + { + $question = $this->makeTableQuestionWithStoredConfig([ + $this->column('Ghost', 'GlpiPlugin\\Removed\\QuestionType\\Gone'), + ]); + + $result = $this->type->validateAnswer($question, [['col_0' => 'anything']]); + + $this->assertTrue($result->isValid()); + } + + public function testMiddleColumnWithoutPatternIsNeverFlagged(): void + { + // Exact shape of the customer report: regex on the outer columns only. + $question = $this->makeTableQuestion([ + $this->column('SRC IP', QuestionTypeShortText::class, pattern: '/^[0-9.]+$/'), + $this->column('description', QuestionTypeShortText::class), + $this->column('DST IP', QuestionTypeShortText::class, pattern: '/^[0-9.]+$/'), + ]); + + $result = $this->type->validateAnswer($question, [ + ['col_0' => '10.0.0.1', 'col_1' => 'asdasd', 'col_2' => '10.0.0.2'], + ]); + + $this->assertTrue($result->isValid()); + } + + public function testPatternOnTheLastColumnOnlyLeavesTheFirstColumnFree(): void + { + $question = $this->makeTableQuestion([ + $this->column('Free text', QuestionTypeShortText::class), + $this->column('Digits', QuestionTypeShortText::class, pattern: '/^\d+$/'), + ]); + + $result = $this->type->validateAnswer($question, [ + ['col_0' => 'anything goes', 'col_1' => '42'], + ]); + + $this->assertTrue($result->isValid()); + } + + public function testPatternOnTheLastColumnOnlyStillFlagsThatColumn(): void + { + $question = $this->makeTableQuestion([ + $this->column('Free text', QuestionTypeShortText::class), + $this->column('Digits', QuestionTypeShortText::class, pattern: '/^\d+$/'), + ]); + + $errors = $this->type->validateAnswer($question, [ + ['col_0' => 'anything goes', 'col_1' => 'not digits'], + ])->getErrors(); + + $this->assertCount(1, $errors); + $this->assertStringContainsString('Digits', $errors[0]['message']); + } + + public function testEveryColumnPatternIsEnforcedIndependently(): void + { + $question = $this->makeTableQuestion([ + $this->column('A', QuestionTypeShortText::class, pattern: '/^a+$/'), + $this->column('B', QuestionTypeShortText::class, pattern: '/^b+$/'), + $this->column('C', QuestionTypeShortText::class, pattern: '/^c+$/'), + ]); + + // Only the middle column is wrong. + $errors = $this->type->validateAnswer($question, [ + ['col_0' => 'aaa', 'col_1' => 'xxx', 'col_2' => 'ccc'], + ])->getErrors(); + + $this->assertCount(1, $errors); + $this->assertStringContainsString('"B"', $errors[0]['message']); + } + + public function testAllColumnPatternsCanFailAtOnce(): void + { + $question = $this->makeTableQuestion([ + $this->column('A', QuestionTypeShortText::class, pattern: '/^a+$/'), + $this->column('B', QuestionTypeShortText::class, pattern: '/^b+$/'), + $this->column('C', QuestionTypeShortText::class, pattern: '/^c+$/'), + ]); + + $result = $this->type->validateAnswer($question, [ + ['col_0' => 'x', 'col_1' => 'y', 'col_2' => 'z'], + ]); + + $this->assertCount(3, $result->getErrors()); + } + + public function testRequiredColumnWithPatternReportsOnlyTheMissingValue(): void + { + $question = $this->makeTableQuestion([ + $this->column('Source IP', QuestionTypeShortText::class, required: true, pattern: '/^[0-9.]+$/'), + $this->column('Comment', QuestionTypeShortText::class), + ]); + + $errors = $this->type->validateAnswer($question, [ + ['col_0' => '', 'col_1' => 'a comment'], + ])->getErrors(); + + $this->assertCount(1, $errors); + $this->assertStringContainsString('required', $errors[0]['message']); + } + + public function testRequiredColumnWithPatternReportsTheFormatWhenFilled(): void + { + $question = $this->makeTableQuestion([ + $this->column('Source IP', QuestionTypeShortText::class, required: true, pattern: '/^[0-9.]+$/'), + ]); + + $errors = $this->type->validateAnswer($question, [['col_0' => 'nope']])->getErrors(); + + $this->assertCount(1, $errors); + $this->assertStringContainsString('format', $errors[0]['message']); + } + + public function testPatternOnAnEmailColumnDoesNotDisableEmailValidation(): void + { + $question = $this->makeTableQuestion([ + $this->column('Contact', QuestionTypeEmail::class, pattern: '/@example\.com$/'), + ]); + + // Satisfies the pattern, yet is not a valid address: only the native + // check can catch it. + $result = $this->type->validateAnswer($question, [['col_0' => 'bob bob@example.com']]); + + $this->assertFalse($result->isValid(), 'A pattern must not shadow the native email check.'); + } + + public function testEmailColumnPatternRejectsAWrongDomain(): void + { + $question = $this->makeTableQuestion([ + $this->column('Contact', QuestionTypeEmail::class, pattern: '/@example\.com$/'), + ]); + + $result = $this->type->validateAnswer($question, [['col_0' => 'bob@other.com']]); + + $this->assertFalse($result->isValid()); + } + + public function testEmailColumnAcceptsAnAddressMatchingBothRules(): void + { + $question = $this->makeTableQuestion([ + $this->column('Contact', QuestionTypeEmail::class, pattern: '/@example\.com$/'), + ]); + + $result = $this->type->validateAnswer($question, [['col_0' => 'bob@example.com']]); + + $this->assertTrue($result->isValid()); + } + + public function testInvalidEmailIsReportedOnceWithTheNativeMessage(): void + { + $question = $this->makeTableQuestion([ + $this->column('Contact', QuestionTypeEmail::class, pattern: '/@example\.com$/'), + ]); + + // Fails the native check and the pattern; the user gets one message, and + // the type-specific one is the most helpful of the two. + $errors = $this->type->validateAnswer($question, [['col_0' => 'not-an-email']])->getErrors(); + + $this->assertCount(1, $errors); + $this->assertStringContainsString('email', $errors[0]['message']); + } + + public function testPatternOnANumberColumnDoesNotDisableNumberValidation(): void + { + $question = $this->makeTableQuestion([ + $this->column('Port', QuestionTypeNumber::class, pattern: '/^[0-9a-f]+$/'), + ]); + + // Hexadecimal letters satisfy the pattern but are not a number. + $result = $this->type->validateAnswer($question, [['col_0' => 'abc']]); + + $this->assertFalse($result->isValid(), 'A pattern must not shadow the native number check.'); + } + + public function testNumberColumnWithPatternRejectsAnOutOfShapeNumber(): void + { + $question = $this->makeTableQuestion([ + $this->column('Port', QuestionTypeNumber::class, pattern: '/^\d{1,4}$/'), + ]); + + $this->assertFalse( + $this->type->validateAnswer($question, [['col_0' => '99999']])->isValid(), + ); + $this->assertTrue( + $this->type->validateAnswer($question, [['col_0' => '8080']])->isValid(), + ); + } + + public function testErrorMessageCarriesTheSubmittedRowNumber(): void + { + $question = $this->makeTableQuestion([ + $this->column('Digits', QuestionTypeShortText::class, pattern: '/^\d+$/'), + ]); + + $errors = $this->type->validateAnswer($question, [ + ['col_0' => '1'], + ['col_0' => '2'], + ['col_0' => 'bad'], + ])->getErrors(); + + $this->assertCount(1, $errors); + $this->assertStringContainsString('3', $errors[0]['message']); + } + + public function testEachFaultyRowIsReportedSeparately(): void + { + $question = $this->makeTableQuestion([ + $this->column('Digits', QuestionTypeShortText::class, pattern: '/^\d+$/'), + ]); + + $result = $this->type->validateAnswer($question, [ + ['col_0' => 'bad'], + ['col_0' => '2'], + ['col_0' => 'worse'], + ]); + + $this->assertCount(2, $result->getErrors()); + } + + public function testManyValidRowsProduceNoError(): void + { + $question = $this->makeTableQuestion([ + $this->column('Digits', QuestionTypeShortText::class, required: true, pattern: '/^\d+$/'), + $this->column('Comment', QuestionTypeShortText::class), + ]); + + $rows = []; + for ($i = 1; $i <= 10; $i++) { + $rows[] = ['col_0' => (string) $i, 'col_1' => 'row ' . $i]; + } + + $this->assertTrue($this->type->validateAnswer($question, $rows)->isValid()); + } + + public function testTableWithoutAnyRuleAcceptsAnything(): void + { + $question = $this->makeTableQuestion([ + $this->column('A', QuestionTypeShortText::class), + $this->column('B', QuestionTypeShortText::class), + ]); + + $result = $this->type->validateAnswer($question, [ + ['col_0' => '!!!', 'col_1' => '???'], + ['col_0' => '', 'col_1' => ''], + ]); + + $this->assertTrue($result->isValid()); + } + + public function testNoColumnConfiguredAcceptsAnyAnswer(): void + { + // A column-less table cannot be configured, but stored data could end up + // that way; validateAnswer() must not choke on it. + $question = $this->makeTableQuestionWithStoredConfig([]); + + $result = $this->type->validateAnswer($question, [['col_0' => 'orphan value']]); + + $this->assertTrue($result->isValid()); + } + + public function testCellForAnUnknownColumnIsIgnored(): void + { + $question = $this->makeTableQuestion([ + $this->column('A', QuestionTypeShortText::class, pattern: '/^a+$/'), + ]); + + // col_9 has no matching column; it must not be validated against col_0. + $result = $this->type->validateAnswer($question, [ + ['col_0' => 'aaa', 'col_9' => 'zzz'], + ]); + + $this->assertTrue($result->isValid()); + } + /** * @param array $columns */ @@ -399,11 +785,28 @@ private function makeTableQuestion(array $columns): Question return Question::getById($this->getQuestionId($form, 'Table')); } + /** + * Builds a question whose stored configuration would be refused by + * validateExtraDataInput(), to exercise the tolerance of validateAnswer() + * towards data written by an older version. + * + * @param array $columns + */ + private function makeTableQuestionWithStoredConfig(array $columns): Question + { + $question = $this->makeTableQuestion([ + $this->column('Placeholder', QuestionTypeShortText::class), + ]); + $question->fields['extra_data'] = json_encode(new TableQuestionConfig(columns: $columns)); + + return $question; + } + /** * @return array{name: string, question_type: string, required: bool, itemtype: string, pattern: string} */ - private function column(string $name, string $fqcn, bool $required, string $pattern = ''): array + private function column(string $name, string $fqcn, bool $required = false, string $pattern = ''): array { return [ TableQuestionConfig::COL_NAME => $name, diff --git a/tests/e2e/specs/table_question.spec.ts b/tests/e2e/specs/table_question.spec.ts index f5a0994..2c49914 100644 --- a/tests/e2e/specs/table_question.spec.ts +++ b/tests/e2e/specs/table_question.spec.ts @@ -133,11 +133,12 @@ test.describe('Advanced forms - Table question', () => { await profile.set(Profiles.SuperAdmin); const form = new FormPage(page); const table = new AdvancedFormsTablePage(page); + const form_name = `E2E table pattern - ${randomUUID()}`; await table.enableTableQuestionType(); const form_id = await api.createItem('Glpi\\Form\\Form', { - name: `E2E table pattern - ${randomUUID()}`, + name: form_name, entities_id: getWorkerEntityId(), }); await form.goto(form_id); @@ -161,8 +162,138 @@ test.describe('Advanced forms - Table question', () => { await expect(eu_table.getByText('This value does not match the expected format.')).toBeVisible(); await expect(table.getEndUserCell(eu_table, 0, 0)).toHaveClass(/is-invalid/); - // Filling the cell with a matching value clears the error. + // A value the pattern accepts must actually go through. The pattern is + // unanchored on purpose: the anchored HTML `pattern` attribute used to + // reject this and there was no way past it. await table.getEndUserCell(eu_table, 0, 0).fill('172.23.0.1'); await expect(table.getEndUserCell(eu_table, 0, 0)).not.toHaveClass(/is-invalid/); + + await page.getByRole('button', { name: 'Submit' }).click(); + await expect(page.getByRole('link', { name: form_name })).toBeVisible(); + }); + + /** + * Regression test for the reported bug: with a pattern on the outer columns + * only, the middle column was reported as badly formatted and the last column + * lost its client-side check. + */ + test('a column without a pattern is never flagged by its neighbours', async ({ page, profile, api }) => { + await profile.set(Profiles.SuperAdmin); + const form = new FormPage(page); + const table = new AdvancedFormsTablePage(page); + const form_name = `E2E table pattern isolation - ${randomUUID()}`; + + await table.enableTableQuestionType(); + + const form_id = await api.createItem('Glpi\\Form\\Form', { + name: form_name, + entities_id: getWorkerEntityId(), + }); + await form.goto(form_id); + + const question = await form.addQuestion('Flows'); + await form.doChangeQuestionType(question, 'Table'); + + await table.openColumnConfig(question); + await table.addColumn(question, { name: 'SRC IP', type: 'Text', pattern: '/^[0-9.]+$/' }); + await table.addColumn(question, { name: 'description', type: 'Text' }); + await table.addColumn(question, { name: 'DST IP', type: 'Text', pattern: '/^[0-9.]+$/' }); + + await form.doSaveFormEditor(); + await form.doPreviewForm(); + + const eu_table = table.getEndUserTable(); + + // Free text in the middle column, valid addresses on both sides. + await table.getEndUserCell(eu_table, 0, 0).fill('10.0.0.1'); + await table.getEndUserCell(eu_table, 0, 1).fill('asdasd'); + await table.getEndUserCell(eu_table, 0, 2).fill('10.0.0.2'); + + await page.getByRole('button', { name: 'Submit' }).click(); + + await expect(page.getByRole('link', { name: form_name })).toBeVisible(); + }); + + /** + * The core renderer attaches a question's error next to every input it finds, + * so a table used to show every message once per cell. They must end up in a + * single list instead. + */ + test('server-side errors are listed once, not repeated in every cell', async ({ page, profile, api }) => { + await profile.set(Profiles.SuperAdmin); + const form = new FormPage(page); + const table = new AdvancedFormsTablePage(page); + + await table.enableTableQuestionType(); + + const form_id = await api.createItem('Glpi\\Form\\Form', { + name: `E2E table server errors - ${randomUUID()}`, + entities_id: getWorkerEntityId(), + }); + await form.goto(form_id); + + const question = await form.addQuestion('Flows'); + await form.doChangeQuestionType(question, 'Table'); + + await table.openColumnConfig(question); + // `x` is a PCRE flag with no JS equivalent, so the browser declines to + // judge this column and the server gets to reject the value: the shortest + // route to a genuine server-side error. + await table.addColumn(question, { name: 'Code', type: 'Text', pattern: '/^abc$/x' }); + await table.addColumn(question, { name: 'description', type: 'Text' }); + await table.addColumn(question, { name: 'Comment', type: 'Text' }); + + await form.doSaveFormEditor(); + await form.doPreviewForm(); + + const eu_table = table.getEndUserTable(); + + await table.getEndUserCell(eu_table, 0, 0).fill('zzz'); + await table.getEndUserCell(eu_table, 0, 1).fill('free text'); + + await page.getByRole('button', { name: 'Submit' }).click(); + + // One visible message, gathered below the table rather than in the cells. + const messages = eu_table.getByTestId('validation-error-message'); + await expect(messages.filter({ visible: true })).toHaveCount(1); + await expect( + eu_table.locator('[data-af-table-errors] [data-testid="validation-error-message"]'), + ).toHaveCount(1); + }); + + test('the last column keeps its own pattern', async ({ page, profile, api }) => { + await profile.set(Profiles.SuperAdmin); + const form = new FormPage(page); + const table = new AdvancedFormsTablePage(page); + + await table.enableTableQuestionType(); + + const form_id = await api.createItem('Glpi\\Form\\Form', { + name: `E2E table last column pattern - ${randomUUID()}`, + entities_id: getWorkerEntityId(), + }); + await form.goto(form_id); + + const question = await form.addQuestion('Flows'); + await form.doChangeQuestionType(question, 'Table'); + + await table.openColumnConfig(question); + await table.addColumn(question, { name: 'description', type: 'Text' }); + await table.addColumn(question, { name: 'DST IP', type: 'Text', pattern: '/^[0-9.]+$/' }); + + await form.doSaveFormEditor(); + await form.doPreviewForm(); + + const eu_table = table.getEndUserTable(); + + await table.getEndUserCell(eu_table, 0, 0).fill('anything goes here'); + await table.getEndUserCell(eu_table, 0, 1).fill('not-an-ip'); + + await page.getByRole('button', { name: 'Submit' }).click(); + + // Only the column that carries the pattern is flagged. + await expect(eu_table.getByText('This value does not match the expected format.')).toBeVisible(); + await expect(table.getEndUserCell(eu_table, 0, 1)).toHaveClass(/is-invalid/); + await expect(table.getEndUserCell(eu_table, 0, 0)).not.toHaveClass(/is-invalid/); }); });