From 1a710e5a9c1e243cca44e9bc25a3339d96fc05d9 Mon Sep 17 00:00:00 2001 From: wyzula-jan Date: Tue, 13 Oct 2026 14:04:51 +0200 Subject: [PATCH] feat(forms): show metadata validation errors under each field --- bec_widgets/utils/forms_from_types/forms.py | 141 +++++++++++-- .../device_item/device_config_dialog.py | 2 +- tests/end-2-end/test_with_plugins_e2e.py | 7 +- tests/unit_tests/test_scan_control.py | 4 +- tests/unit_tests/test_scan_metadata.py | 197 ++++++++++++++---- 5 files changed, 286 insertions(+), 65 deletions(-) diff --git a/bec_widgets/utils/forms_from_types/forms.py b/bec_widgets/utils/forms_from_types/forms.py index ea0385ce..850bddf0 100644 --- a/bec_widgets/utils/forms_from_types/forms.py +++ b/bec_widgets/utils/forms_from_types/forms.py @@ -1,20 +1,12 @@ from __future__ import annotations from types import GenericAlias, NoneType, UnionType -from typing import NamedTuple +from typing import NamedTuple, Union, get_args, get_origin from bec_lib.logger import bec_logger from pydantic import BaseModel, ValidationError -from qtpy.QtCore import Signal # type: ignore -from qtpy.QtWidgets import ( - QApplication, - QGridLayout, - QHBoxLayout, - QLabel, - QSizePolicy, - QVBoxLayout, - QWidget, -) +from qtpy.QtCore import Qt, Signal # type: ignore +from qtpy.QtWidgets import QApplication, QGridLayout, QLabel, QSizePolicy, QVBoxLayout, QWidget from bec_widgets.utils.bec_widget import BECWidget from bec_widgets.utils.colors import get_accent_colors @@ -36,6 +28,15 @@ class GridRow(NamedTuple): widget: DynamicFormItem +class ValidationIssue(NamedTuple): + """One invalid field of a form, ready for display.""" + + field: str + title: str + problem: str + entered: str + + class TypedForm(BECWidget, QWidget): PLUGIN = True ICON_NAME = "list_alt" @@ -129,7 +130,10 @@ class TypedForm(BECWidget, QWidget): which the field name is attached as a property "_model_field_name"), and the entry widget""" grid: QGridLayout = self._form_grid.layout() # type: ignore for i in range(grid.rowCount() - 1): # One extra row for stretch - yield GridRow(i, grid.itemAtPosition(i, 0).widget(), grid.itemAtPosition(i, 1).widget()) + label, widget = grid.itemAtPosition(i, 0), grid.itemAtPosition(i, 1) + if label is None or widget is None: # e.g. the error line under a field + continue + yield GridRow(i, label.widget(), widget.widget()) def _dict_from_grid(self) -> dict[str, DynamicFormItemType]: return { @@ -203,6 +207,7 @@ class PydanticModelForm(TypedForm): """ self._pretty_display = pretty_display self._md_schema = data_model + self._field_errors: dict[str, QLabel] = {} super().__init__( parent=parent, form_item_specs=self._form_item_specs(), @@ -211,13 +216,13 @@ class PydanticModelForm(TypedForm): **kwargs, ) - self._validity = QWidget(self) - validity_layout = QHBoxLayout(self._validity) - validity_layout.setContentsMargins(0, 0, 0, 0) - self._validity_message = QLabel("Not yet validated", self._validity) + self._validity_message = QLabel("Not yet validated", self) self._validity_message.setWordWrap(True) - validity_layout.addWidget(self._validity_message) + self._validity_message.setTextFormat(Qt.TextFormat.PlainText) + self._validity = self._validity_message self._validity_state: bool | None = None + self._validity_summary = "Not yet validated" + self._validity_issues: list[ValidationIssue] = [] self._layout.addWidget(self._validity) self.value_changed.connect(self.validate_form) @@ -266,19 +271,106 @@ class PydanticModelForm(TypedForm): def populate(self): self.update_items_from_schema() + self._field_errors = {} super().populate() + def _add_griditem(self, item: FormItemSpec, row: int): + # every field takes two grid rows: the field itself and its (hidden) error line below + super()._add_griditem(item, 2 * row) + error = QLabel(parent=self._form_grid) + error.setWordWrap(True) + error.setVisible(False) + self._form_grid.layout().addWidget(error, 2 * row + 1, 1) + self._field_errors[item.name] = error + def get_form_data(self): """Get the entered metadata as a dict.""" return self._dict_from_grid() - def _set_validity(self, valid: bool, message: str): - """Show the validation result inline, coloured by the theme's accent colours.""" + def _set_validity(self, valid: bool, message: str, details: str = ""): + """Show a short validation result under the form. + + Args: + valid (bool): whether the form data is valid. + message (str): the one-line summary. + details (str): readable list of the problems, used as tooltip. + """ self._validity_state = valid + self._validity_summary = message accents = get_accent_colors() color = accents.success if valid else accents.emergency self._validity_message.setStyleSheet(f"color: {color.name()};") self._validity_message.setText(message) + self._validity_message.setToolTip(details) + + def _show_field_errors(self, issues: list[ValidationIssue]): + """Show each field's problem directly under its input; hide the lines of valid fields.""" + problems = {issue.field: issue.problem for issue in issues} + color = get_accent_colors().emergency.name() + for name, label in self._field_errors.items(): + problem = problems.get(name) + label.setVisible(problem is not None) + if problem is not None: + label.setStyleSheet(f"color: {color};") + label.setText(problem) + + def _describe_errors(self, error: ValidationError) -> list[ValidationIssue]: + """One issue per invalid top-level field, without pydantic's error codes and links. + + Errors inside a field (list entries, dict keys, nested models) keep their location, e.g. + ``[1]: Input should be a valid integer``; every failing entry of a field is listed.""" + problems: dict[str, list[str]] = {} + entered: dict[str, str] = {} + for err in error.errors(): + loc = tuple(err["loc"]) + name = str(loc[0]) if loc else "form" + path = self._error_path(name, loc[1:]) + value = err.get("input") + if err["type"] == "missing" or value is None: + message = "Required - please fill in" + else: + message = err["msg"] + line = f"{path}: {message}" if path else message + lines = problems.setdefault(name, []) + if line not in lines: + lines.append(line) + if name not in entered: + if value is None: + entered[name] = "empty" + else: + entered[name] = repr(value) if isinstance(value, str) else str(value) + issues = [] + for name, lines in problems.items(): + field = self._md_schema.model_fields.get(name) + title = field.title if field is not None and field.title else name + issues.append(ValidationIssue(name, title, "\n".join(lines), entered[name])) + return issues + + def _error_path(self, name: str, sub_loc: tuple) -> str: + """Readable location inside a field, e.g. ``[1]``, ``key`` or ``inner[0].name``. + + For fields with several possible types (``int | float``), pydantic reports one error per + type, tagged with the type name; that tag is not a location and is dropped.""" + field = self._md_schema.model_fields.get(name) + annotation = field.annotation if field is not None else None + if get_origin(annotation) in (Union, UnionType): + members = [arg for arg in get_args(annotation) if arg is not NoneType] + if len(members) > 1 and sub_loc and isinstance(sub_loc[0], str): + sub_loc = sub_loc[1:] + path = "" + for part in sub_loc: + path += f"[{part}]" if isinstance(part, int) else f".{part}" + return path.lstrip(".") + + @staticmethod + def _summarize_issues(issues: list[ValidationIssue]) -> str: + if len(issues) == 1: + return f"Invalid field: {issues[0].title}" + return f"{len(issues)} invalid fields: {', '.join(issue.title for issue in issues)}" + + def _summarize_errors(self, error: ValidationError) -> str: + """One short line naming the invalid fields, so the form keeps its size.""" + return self._summarize_issues(self._describe_errors(error)) def validate_form(self, *_) -> bool: """validate the currently entered metadata against the pydantic schema. @@ -287,12 +379,21 @@ class PydanticModelForm(TypedForm): try: metadata_dict = self.get_form_data() self._md_schema.model_validate(metadata_dict) + self._validity_issues = [] + self._show_field_errors([]) self._set_validity(True, "No errors!") self.form_data_updated.emit(metadata_dict) self.validity_proc.emit(True) return True except ValidationError as e: - self._set_validity(False, str(e)) + self._validity_issues = self._describe_errors(e) + self._show_field_errors(self._validity_issues) + details = "\n".join( + f"{issue.title}: {line}" + for issue in self._validity_issues + for line in issue.problem.split("\n") + ) + self._set_validity(False, self._summarize_issues(self._validity_issues), details) self.form_data_cleared.emit(None) self.validity_proc.emit(False) return False diff --git a/bec_widgets/widgets/services/device_browser/device_item/device_config_dialog.py b/bec_widgets/widgets/services/device_browser/device_item/device_config_dialog.py index 704997b4..5aee0005 100644 --- a/bec_widgets/widgets/services/device_browser/device_item/device_config_dialog.py +++ b/bec_widgets/widgets/services/device_browser/device_item/device_config_dialog.py @@ -135,7 +135,7 @@ class DeviceConfigDialog(QDialog): # TODO: replace when https://github.com/bec-project/bec/issues/528 https://github.com/bec-project/bec/issues/547 are resolved for button in [self.button_box.button(b) for b in [_StdBtn.Apply, _StdBtn.Ok]]: button.setEnabled(valid) - button.setToolTip(self._form._validity_message.text()) + button.setToolTip(self._form._validity_message.toolTip()) def _process_action(self): self.accepted_data.emit(self._form.get_form_data()) diff --git a/tests/end-2-end/test_with_plugins_e2e.py b/tests/end-2-end/test_with_plugins_e2e.py index 15633b79..d006c6d0 100644 --- a/tests/end-2-end/test_with_plugins_e2e.py +++ b/tests/end-2-end/test_with_plugins_e2e.py @@ -7,7 +7,6 @@ try: except ImportError: pytest.skip(reason="Requires plugin repo!", allow_module_level=True) -from qtpy.QtWidgets import QGridLayout from bec_widgets.utils.widget_io import WidgetIO from bec_widgets.widgets.control.scan_control import ScanControl @@ -68,11 +67,9 @@ def test_scan_metadata_for_custom_scan( def do_test(): # Set the metadata - grid: QGridLayout = scan_control._metadata_form._form_grid.layout() - for i in range(grid.rowCount() - 1): # type: ignore - field_name = grid.itemAtPosition(i, 0).widget().property("_model_field_name") + for field_name, widget in scan_control._metadata_form.widget_dict.items(): if (value_to_set := md.pop(field_name, None)) is not None: - grid.itemAtPosition(i, 1).widget().setValue(value_to_set) + widget.setValue(value_to_set) # all values should be used assert md == {} assert scan_control.button_run_scan.isEnabled() diff --git a/tests/unit_tests/test_scan_control.py b/tests/unit_tests/test_scan_control.py index c052cc03..1fb9e6af 100644 --- a/tests/unit_tests/test_scan_control.py +++ b/tests/unit_tests/test_scan_control.py @@ -1772,7 +1772,7 @@ def test_scan_metadata_is_connected(scan_control): assert scan_control._metadata_form._scan_name == "line_scan" scan_control.comboBox_scan_selection.setCurrentText("grid_scan") assert scan_control._metadata_form._scan_name == "grid_scan" - sample_name = scan_control._metadata_form._form_grid.layout().itemAtPosition(2, 1).widget() + sample_name = scan_control._metadata_form.widget_dict["sample_name"] assert isinstance(sample_name, StrFormItem) sample_name._main_widget.setText("Test Sample") @@ -1784,7 +1784,7 @@ def test_scan_metadata_is_connected(scan_control): def test_scan_metadata_is_passed_to_scan_function(scan_control: ScanControl): scan_control.comboBox_scan_selection.setCurrentText("grid_scan") - sample_name = scan_control._metadata_form._form_grid.layout().itemAtPosition(2, 1).widget() + sample_name = scan_control._metadata_form.widget_dict["sample_name"] sample_name._main_widget.setText("Test Sample") scan_control._metadata_form._additional_metadata._table_model._data = TEST_TABLE_ENTRY scan_control._metadata_form.validate_form() diff --git a/tests/unit_tests/test_scan_metadata.py b/tests/unit_tests/test_scan_metadata.py index 83951e78..d1ef776a 100644 --- a/tests/unit_tests/test_scan_metadata.py +++ b/tests/unit_tests/test_scan_metadata.py @@ -3,7 +3,7 @@ from typing import Set import pytest from bec_lib.metadata_schema import BasicScanMetadata -from pydantic import Field +from pydantic import BaseModel, Field, ValidationError from pydantic.types import Json from qtpy.QtCore import QItemSelectionModel, QPoint, Qt @@ -77,40 +77,7 @@ def metadata_widget(empty_metadata_widget: ScanMetadata): widget._md_schema = ExampleSchema widget.populate() - scan_name = widget._form_grid.layout().itemAtPosition(0, 1).widget() - comment = widget._form_grid.layout().itemAtPosition(1, 1).widget() - sample_name = widget._form_grid.layout().itemAtPosition(2, 1).widget() - str_optional = widget._form_grid.layout().itemAtPosition(3, 1).widget() - str_required = widget._form_grid.layout().itemAtPosition(4, 1).widget() - bool_optional = widget._form_grid.layout().itemAtPosition(5, 1).widget() - bool_required_default = widget._form_grid.layout().itemAtPosition(6, 1).widget() - bool_required_nodefault = widget._form_grid.layout().itemAtPosition(7, 1).widget() - int_default = widget._form_grid.layout().itemAtPosition(8, 1).widget() - int_nodefault_optional = widget._form_grid.layout().itemAtPosition(9, 1).widget() - float_nodefault = widget._form_grid.layout().itemAtPosition(10, 1).widget() - decimal_dp_limits_nodefault = widget._form_grid.layout().itemAtPosition(11, 1).widget() - dict_default = widget._form_grid.layout().itemAtPosition(12, 1).widget() - unsupported_class = widget._form_grid.layout().itemAtPosition(13, 1).widget() - - yield ( - widget, - { - "scan_name": scan_name, - "comment": comment, - "sample_name": sample_name, - "str_optional": str_optional, - "str_required": str_required, - "bool_optional": bool_optional, - "bool_required_default": bool_required_default, - "bool_required_nodefault": bool_required_nodefault, - "int_default": int_default, - "int_nodefault_optional": int_nodefault_optional, - "float_nodefault": float_nodefault, - "decimal_dp_limits_nodefault": decimal_dp_limits_nodefault, - "dict_default": dict_default, - "unsupported_class": unsupported_class, - }, - ) + yield widget, widget.widget_dict def fill_components(components: dict[str, DynamicFormItem]): @@ -166,15 +133,171 @@ def test_validation(metadata_widget: tuple[ScanMetadata, dict[str, DynamicFormIt components["bool_required_nodefault"]._main_widget.clear() widget.validate_form() assert widget._validity_state is False - assert "Input should be a valid boolean" in widget._validity_message.text() + assert widget._validity_message.text() == "Invalid field: bool_required_nodefault" + assert ( + widget._validity_message.toolTip() == "bool_required_nodefault: Required - please fill in" + ) components["bool_required_nodefault"].setValue(True) components["float_nodefault"]._main_widget.clear() widget.validate_form() - assert "Input should be a valid number" in widget._validity_message.text() + assert widget._validity_message.text() == "Invalid field: float_nodefault" + assert widget._validity_message.toolTip() == "float_nodefault: Required - please fill in" components["float_nodefault"].setValue(True) +def test_validation_summary_stays_short( + metadata_widget: tuple[ScanMetadata, dict[str, DynamicFormItem]], +): + widget, components = metadata_widget + fill_components(components) + components["bool_required_nodefault"]._main_widget.clear() + components["float_nodefault"]._main_widget.clear() + widget.validate_form() + summary = widget._validity_summary + assert summary == "2 invalid fields: bool_required_nodefault, float_nodefault" + assert "\n" not in summary + assert "errors.pydantic.dev" not in widget._validity_message.toolTip() + + +def test_validation_summary_uses_field_titles( + metadata_widget: tuple[ScanMetadata, dict[str, DynamicFormItem]], +): + widget, _ = metadata_widget + with pytest.raises(ValidationError) as excinfo: + ExampleSchema.model_validate(TEST_DICT | {"str_optional": "x" * 30}) + assert "Optional string" in widget._summarize_errors(excinfo.value) + + +def test_validation_summary_groups_union_and_list_errors( + metadata_widget: tuple[ScanMetadata, dict[str, DynamicFormItem]], +): + class UnionListSchema(BasicScanMetadata): + u: int | float = Field(title="Union") + lst: list[int] = Field(default_factory=list, title="List") + + widget, _ = metadata_widget + with pytest.raises(ValidationError) as excinfo: + UnionListSchema.model_validate({"u": "x", "lst": [1, "a", "b"]}) + widget._md_schema = UnionListSchema + assert widget._summarize_errors(excinfo.value) == "2 invalid fields: Union, List" + + +def test_validation_issues_are_readable( + metadata_widget: tuple[ScanMetadata, dict[str, DynamicFormItem]], +): + class ReadableSchema(BasicScanMetadata): + name: str = Field(max_length=3, title="Name") + count: int = Field(title="Count") + value: Decimal = Field(decimal_places=1, title="Value") + + widget, _ = metadata_widget + widget._md_schema = ReadableSchema + with pytest.raises(ValidationError) as excinfo: + ReadableSchema.model_validate({"name": "toolong", "count": None, "value": Decimal("1.25")}) + issues = widget._describe_errors(excinfo.value) + assert [(i.field, i.title, i.entered) for i in issues] == [ + ("name", "Name", "'toolong'"), + ("count", "Count", "empty"), + ("value", "Value", "1.25"), + ] + assert issues[0].problem == "String should have at most 3 characters" + assert issues[1].problem == "Required - please fill in" + + +def test_validation_issues_keep_nested_locations( + metadata_widget: tuple[ScanMetadata, dict[str, DynamicFormItem]], +): + class Detector(BaseModel): + prefix: str + gain: int + + class NestedSchema(BasicScanMetadata): + positions: list[int] = Field(default_factory=list, title="Positions") + offsets: dict[str, int] = Field(default_factory=dict, title="Offsets") + detector: Detector | None = Field(None, title="Detector") + value: int | float = Field(0, title="Value") + + widget, _ = metadata_widget + widget._md_schema = NestedSchema + with pytest.raises(ValidationError) as excinfo: + NestedSchema.model_validate( + { + "positions": [1, "a", 3, "b"], + "offsets": {"x": 1, "y": "bad"}, + "detector": {"gain": "hi"}, + "value": "x", + } + ) + issues = {issue.field: issue for issue in widget._describe_errors(excinfo.value)} + positions = issues["positions"].problem.split("\n") + assert [line.split(":")[0] for line in positions] == ["[1]", "[3]"] + assert issues["offsets"].problem.startswith("y: Input should be a valid integer") + assert issues["detector"].problem.split("\n") == [ + "prefix: Required - please fill in", + "gain: Input should be a valid integer, unable to parse string as an integer", + ] + # union members are tagged with their type name, which is not a location + assert not any( + line.startswith(("int:", "float:")) for line in issues["value"].problem.split("\n") + ) + assert len(issues["value"].problem.split("\n")) == 2 + + +def test_validation_errors_under_each_field( + metadata_widget: tuple[ScanMetadata, dict[str, DynamicFormItem]], +): + widget, components = metadata_widget + errors = widget._field_errors + assert set(errors) == set(components) + assert not any(label.isVisibleTo(widget) for label in errors.values()) + + fill_components(components) + components["bool_required_nodefault"]._main_widget.clear() + components["float_nodefault"]._main_widget.clear() + widget.validate_form() + shown = {name for name, label in errors.items() if label.isVisibleTo(widget)} + assert shown == {"bool_required_nodefault", "float_nodefault"} + assert errors["float_nodefault"].text() == "Required - please fill in" + assert "errors.pydantic.dev" not in errors["float_nodefault"].text() + + components["bool_required_nodefault"].setValue(True) + widget.validate_form() + shown = {name for name, label in errors.items() if label.isVisibleTo(widget)} + assert shown == {"float_nodefault"} + + components["float_nodefault"].setValue(1.5) + widget.validate_form() + assert not any(label.isVisibleTo(widget) for label in errors.values()) + + +def test_validation_error_line_sits_under_its_field( + metadata_widget: tuple[ScanMetadata, dict[str, DynamicFormItem]], +): + widget, components = metadata_widget + grid = widget._form_grid.layout() + rows = list(widget.enumerate_form_widgets()) + assert len(rows) == len(components) # error lines are not form rows + for row in rows: + name = row.label.property("_model_field_name") + index = grid.indexOf(widget._field_errors[name]) + assert grid.getItemPosition(index)[:2] == (row.i + 1, 1) + + +def test_validation_error_lines_follow_schema_change( + metadata_widget: tuple[ScanMetadata, dict[str, DynamicFormItem]], +): + class OtherSchema(BasicScanMetadata): + count: int = Field(title="Count") + + widget, _ = metadata_widget + widget.set_schema(OtherSchema) + assert set(widget._field_errors) == {"scan_name", "comment", "sample_name", "count"} + widget.validate_form() + assert widget._field_errors["count"].isVisibleTo(widget) + assert widget._validity_message.text() == "Invalid field: Count" + + def test_numbers_clipped_to_limits( metadata_widget: tuple[ScanMetadata, dict[str, DynamicFormItem]], ):