Skip to content

Fix - Table question column validation - #48

Open
RomainLvr wants to merge 2 commits into
mainfrom
fix/table-question-column-validation
Open

Fix - Table question column validation#48
RomainLvr wants to merge 2 commits into
mainfrom
fix/table-question-column-validation

Conversation

@RomainLvr

Copy link
Copy Markdown
Contributor

Checklist before requesting a review

Please delete options that are not relevant.

  • I have performed a self-review of my code.
  • I have added tests (when available) that prove my fix is effective or that my feature works.
  • I have updated the CHANGELOG with a short functional description of the fix or new feature.
  • This change requires a documentation update.

Description

Problems

  • A column's pattern could be applied to another column: with a pattern on columns 0 and 2, column 1 inherited column 2's pattern and column 2 lost its check. This is the reported bug by the customer.
  • Patterns that were not anchored at both ends, or carried a flag such as /…/i, rejected values they should accept.
  • Setting a pattern on a typed column silenced that type's own format check, so an e-mail column with a pattern stopped validating e-mail addresses.
  • Server-side errors were repeated once per cell.
  • Row bounds were handled by a mechanism specific to this plugin, and were never actually checked on submission.

Fixes

  • The required columns and patterns sent to the browser are now built in PHP, from the same rules the server validates against, and keyed like the submitted field names. They were built in Twig with the merge filter, which renumbers integer keys and shifted a pattern onto the neighbouring column.
  • The HTML pattern attribute is no longer emitted. It is implicitly anchored and ignores flags, so it disagreed with both the PHP and the JS check.
  • Matching is delegated to GLPI's RegexConditionHandler, and each column's question type keeps its own format check.
  • Server-side errors are gathered into a single list below the table.
  • Min rows / Max rows are replaced by a native condition criterion on the number of filled rows.

Breaking change

The Min rows / Max rows settings are removed. The row count is now a native criterion, usable for validation (Length is greater than…) and for visibility.
Bounds configured in 1.2.0 are ignored and must be redeclared as validation conditions on the question.

@RomainLvr
RomainLvr requested review from Rom1-B and ccailly August 4, 2026 09:12
@RomainLvr RomainLvr self-assigned this Aug 4, 2026
@@ -109,6 +170,11 @@ export class AfTableQuestion {
// stale messages from a previous attempt next to our fresh ones.
table.querySelectorAll('.invalid-tooltip').forEach(el => el.remove());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cleanup also removes messages already relocated into #errors, since they keep the invalid-tooltip class and #errors sits inside table. Resubmitting without fixing anything briefly wipes the message.

Suggested change
table.querySelectorAll('.invalid-tooltip').forEach(el => el.remove());
// Clears core's stale server errors before a blocked resubmit; skip the
// ones already relocated below the table so they survive.
table.querySelectorAll('.invalid-tooltip').forEach(el => {
if (!el.closest('[data-af-table-errors]')) { el.remove(); }
});

Comment thread CHANGELOG.md
@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changelog only lists the bug fix; the breaking Min rows/Max rows removal and its mandatory-question caveat aren't documented.

Suggested change
### Changed
- **Breaking**: `Min rows`/`Max rows` removed; use a `Length is greater/less than...` validation condition instead (also mark the question mandatory to enforce a minimum, since an empty table skips validation)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants