diff --git a/questionpy_sdk/webserver/question_ui/__init__.py b/questionpy_sdk/webserver/question_ui/__init__.py index 12dd346..3c958fc 100644 --- a/questionpy_sdk/webserver/question_ui/__init__.py +++ b/questionpy_sdk/webserver/question_ui/__init__.py @@ -14,8 +14,12 @@ from pydantic import BaseModel from questionpy_sdk.webserver.question_ui.errors import ( + ConversionError, InvalidAttributeValueError, + InvalidCleanOptionError, + PlaceholderReferenceError, RenderErrorCollection, + UnknownElementError, XMLSyntaxError, ) @@ -69,7 +73,7 @@ def _set_element_value(element: etree._Element, value: str, name: str, xpath: et element.set("value", value) -def _replace_shuffled_indices(element: etree._Element, index: int) -> None: +def _replace_shuffled_indices(element: etree._Element, index: int, error_collection: RenderErrorCollection) -> None: for index_element in _assert_element_list( element.xpath(".//qpy:shuffled-index", namespaces={"qpy": QuestionUIRenderer.QPY_NAMESPACE}) ): @@ -86,7 +90,9 @@ def _replace_shuffled_indices(element: etree._Element, index: int) -> None: elif format_style == "III": index_str = _int_to_roman(index).upper() else: - index_str = str(index) + error_collection.insert(InvalidAttributeValueError(index_element, "format", format_style)) + _remove_preserving_tail(index_element) + continue # Replace the index element with the new index string new_text_node = etree.Element("span") # Using span to replace the custom element @@ -142,10 +148,14 @@ def _require_parent(node: etree._Element) -> etree._Element: return parent +def _remove_element(node: etree._Element) -> None: + _require_parent(node).remove(node) + + def _remove_preserving_tail(node: etree._Element) -> None: if node.tail is not None: _add_text_before(node, node.tail) - _require_parent(node).remove(node) + _remove_element(node) class QuestionMetadata: @@ -236,6 +246,41 @@ def _replace_qpy_urls(self, xml: str) -> str: """Replace QPY-URLs to package files with SDK-URLs.""" return re.sub(r"qpy://(static|static-private)/((?:[a-z_][a-z0-9_]{0,126}/){2})", r"/worker/\2file/\1/", xml) + def _validate_placeholder(self, p_instruction: etree._Element) -> tuple[str, str] | None: + """Collects potential render errors for the placeholder PIs. + + Returns: + If no error occurred, a tuple consisting of the placeholder key and the cleaning option. + value. Else, None. + """ + parsing_error = False + if not p_instruction.text: + reference_error = PlaceholderReferenceError( + element=p_instruction, placeholder=None, available=self._placeholders + ) + self._errors.insert(reference_error) + return None + + parts = p_instruction.text.strip().split(maxsplit=1) + key = parts[0] + clean_option = parts[1].lower() if len(parts) == 2 else "clean" # noqa: PLR2004 + expected = ("plain", "clean", "noclean") + if clean_option not in expected: + option_error = InvalidCleanOptionError(element=p_instruction, option=clean_option, expected=expected) + self._errors.insert(option_error) + parsing_error = True + + if key not in self._placeholders: + reference_error = PlaceholderReferenceError( + element=p_instruction, placeholder=key, available=self._placeholders + ) + self._errors.insert(reference_error) + parsing_error = True + + if parsing_error: + return None + return key, clean_option + def _resolve_placeholders(self) -> None: """Replace placeholder PIs such as `` with the appropriate value from `self.placeholders`. @@ -243,20 +288,12 @@ def _resolve_placeholders(self) -> None: last. """ for p_instruction in _assert_element_list(self._xpath("//processing-instruction('p')")): - if not p_instruction.text: - continue - parts = p_instruction.text.strip().split() - key = parts[0] - clean_option = parts[1].lower() if len(parts) > 1 else "clean" - - parent = p_instruction.getparent() - if parent is None: - continue - - if key not in self._placeholders: - parent.remove(p_instruction) + data = self._validate_placeholder(p_instruction) + if data is None: + _remove_element(p_instruction) continue + key, clean_option = data raw_value = self._placeholders[key] if clean_option == "plain": @@ -288,9 +325,7 @@ def _hide_unwanted_feedback(self) -> None: (feedback_type == "general" and self._options.general_feedback) or (feedback_type == "specific" and self._options.feedback) ): - parent = element.getparent() - if parent is not None: - parent.remove(element) + _remove_element(element) expected = ("general", "specific") if feedback_type not in expected: @@ -307,6 +342,18 @@ def _hide_if_role(self) -> None: for element in _assert_element_list(self._xpath("//*[@qpy:if-role]")): if attr := element.get(f"{{{self.QPY_NAMESPACE}}}if-role"): allowed_roles = [role.upper() for role in re.split(r"[\s|]+", attr)] + expected = list(QuestionDisplayRole) + if unexpected := [role for role in allowed_roles if role not in expected]: + error = InvalidAttributeValueError( + element=element, + attribute="qpy:if-role", + value=unexpected, + expected=expected, + ) + self._errors.insert(error) + _remove_element(element) + continue + has_role = any(role in allowed_roles and role in self._options.roles for role in QuestionDisplayRole) if not has_role and (parent := element.getparent()) is not None: @@ -394,16 +441,16 @@ def _shuffle_contents(self) -> None: # Reinsert shuffled elements, preserving non-element nodes for i, child in enumerate(child_elements): - _replace_shuffled_indices(child, i + 1) + _replace_shuffled_indices(child, i + 1, self._errors) # Move each child element back to its parent at the correct position element.append(child) def _clean_up(self) -> None: """Removes remaining QuestionPy elements and attributes as well as comments and xmlns declarations.""" for element in _assert_element_list(self._xpath("//qpy:*")): - parent = element.getparent() - if parent is not None: - parent.remove(element) + error = UnknownElementError(element=element) + self._errors.insert(error) + _remove_element(element) # Remove attributes in the QuestionPy namespace for element in _assert_element_list(self._xpath("//*")): @@ -413,9 +460,7 @@ def _clean_up(self) -> None: # Remove comments for comment in _assert_element_list(self._xpath("//comment()")): - parent = comment.getparent() - if parent is not None: - parent.remove(comment) + _remove_element(comment) # Remove namespaces from all elements. (QPy elements should all have been consumed previously anyhow.) for element in _assert_element_list(self._xpath("//*")): @@ -458,6 +503,63 @@ def _add_styles(self) -> None: for element in _assert_element_list(self._xpath("//xhtml:input[@type = 'checkbox' or @type = 'radio']")): self._add_class_names(element, "qpy-input") + def _validate_format_float_element(self, element: etree._Element) -> tuple[float, int | None, str] | None: + """Collects potential render errors for the `qpy:format-float` element. + + Returns: + If no error occurred, a tuple consisting of the float value, the precision, and the thousands separator + value. Else, None. + """ + parsing_error = False + + if element.text is None: + # TODO: Show an error message? + return None + + # As PHP parses floats and integers differently than Python, we enforce a stricter format. + # E.g. parsing '20_000' or '1d1' results in: + # Python -> 20000 Error + # PHP -> 20 1 + if re.match(r"^\s*((\d+\.?\d*)|(\d*\.\d+)|(\d+e\d+))\s*$", element.text) is None: + float_error = ConversionError(element=element, value=element.text, to_type=float) + self._errors.insert(float_error) + parsing_error = True + + precision_text: str | None = element.get("precision") + precision = None + if precision_text is not None: + if not precision_text or (precision_text[0] == "-" and precision_text[1:].isnumeric()): + # Empty or negative value. + precision_error = InvalidAttributeValueError( + element=element, attribute="precision", value=precision_text + ) + self._errors.insert(precision_error) + parsing_error = True + elif precision_text.isnumeric(): + # We disallow the usage of underscores to separate numeric literals, see above for an explanation. + precision = int(precision_text) + else: + conversion_error = ConversionError( + element=element, value=precision_text, to_type=int, attribute="precision" + ) + self._errors.insert(conversion_error) + parsing_error = True + else: + precision = None + + thousands_sep_attr = element.get("thousands-separator", "no") + expected = ("yes", "no") + if thousands_sep_attr not in expected: + thousands_sep_error = InvalidAttributeValueError( + element=element, attribute="thousands-separator", value=thousands_sep_attr, expected=expected + ) + self._errors.insert(thousands_sep_error) + parsing_error = True + + if parsing_error: + return None + return float(element.text), precision, thousands_sep_attr + def _format_floats(self) -> None: """Handles `qpy:format-float`. @@ -467,19 +569,20 @@ def _format_floats(self) -> None: decimal_sep = "." # Placeholder for decimal separator for element in _assert_element_list(self._xpath("//qpy:format-float")): - if element.text is None: + data = self._validate_format_float_element(element) + if data is None: + _remove_element(element) continue - float_val = float(element.text) - precision = int(element.get("precision", -1)) + float_val, precision, thousands_sep_attr = data + strip_zeroes = "strip-zeros" in element.attrib - formatted_str = f"{float_val:.{precision}f}" if precision >= 0 else str(float_val) + formatted_str = f"{float_val:.{precision}f}" if precision is not None else str(float_val) if strip_zeroes: formatted_str = formatted_str.rstrip("0").rstrip(decimal_sep) if "." in formatted_str else formatted_str - thousands_sep_attr = element.get("thousands-separator", "no") if thousands_sep_attr == "yes": parts = formatted_str.split(decimal_sep) integral_part = parts[0] @@ -495,7 +598,7 @@ def _format_floats(self) -> None: parent = element.getparent() new_text.tail = element.tail - if parent: + if parent is not None: parent.insert(parent.index(element), new_text) parent.remove(element) diff --git a/questionpy_sdk/webserver/question_ui/errors.py b/questionpy_sdk/webserver/question_ui/errors.py index d0f9d16..9f142eb 100644 --- a/questionpy_sdk/webserver/question_ui/errors.py +++ b/questionpy_sdk/webserver/question_ui/errors.py @@ -5,8 +5,8 @@ import logging from abc import ABC, abstractmethod from bisect import insort -from collections.abc import Iterable, Iterator, Sized -from dataclasses import dataclass +from collections.abc import Collection, Iterable, Iterator, Mapping, Sized +from dataclasses import dataclass, field from operator import attrgetter from typing import TypeAlias @@ -15,10 +15,23 @@ _log = logging.getLogger(__name__) +def _format_human_readable_list(values: Collection[str], opening: str, closing: str) -> str: + *values, last_value = values + last_value = f"{opening}{last_value}{closing}" + if not values: + return last_value + + return opening + f"{closing}, {opening}".join(values) + f"{closing} and {last_value}" + + @dataclass(frozen=True) class RenderError(ABC): """Represents a generic error which occurred during rendering.""" + @property + def type(self) -> str: + return self.__class__.__name__ + @property @abstractmethod def line(self) -> int | None: @@ -41,10 +54,47 @@ def html_message(self) -> str: @dataclass(frozen=True) class RenderElementError(RenderError, ABC): + """A generic element error which occurred during rendering. + + Attributes: + element: The element where the error occurred. + template: A template string that defines the structure of the error message. + It can contain placeholders corresponding to the keys in `template_kwargs`. + These placeholders are identified by braces ('{' and '}'), similar to `str.format`. + The '{element}' placeholder is predefined and resolves to a human-readable representation of `element`. + Providing a value with the key 'element' in `template_kwargs` will overwrite this behaviour. + template_kwargs: A mapping containing the values of the placeholders in `template`. + If a value is of type `Collection[str]`, it will be formatted as a human-readable list. + """ + element: etree._Element + template: str + template_kwargs: Mapping[str, str | Collection[str]] = field(default_factory=dict) + + def _message(self, *, as_html: bool) -> str: + (opening, closing) = ("", "") if as_html else ("'", "'") + template_kwargs = {"element": f"{opening}{self.element_representation}{closing}"} + + for key, values in self.template_kwargs.items(): + collection = {values} if isinstance(values, str) else values + template_kwargs[key] = _format_human_readable_list(collection, opening, closing) + + return self.template.format_map(template_kwargs) + + @property + def message(self) -> str: + return self._message(as_html=False) + + @property + def html_message(self) -> str: + return self._message(as_html=True) @property def element_representation(self) -> str: + # Return the whole element if it is a PI. + if isinstance(self.element, etree._ProcessingInstruction): + return str(self.element) + # Create the prefix of an element. We do not want to keep 'html' as a prefix. prefix = f"{self.element.prefix}:" if self.element.prefix and self.element.prefix != "html" else "" return prefix + etree.QName(self.element).localname @@ -57,36 +107,89 @@ def line(self) -> int | None: @dataclass(frozen=True) class InvalidAttributeValueError(RenderElementError): - """Invalid attribute value.""" + """Invalid attribute value(s).""" + + def __init__( + self, + element: etree._Element, + attribute: str, + value: str | Collection[str], + expected: Collection[str] | None = None, + ): + template_kwargs = {"value": value, "attribute": attribute} + expected_str = "" + if expected: + template_kwargs["expected"] = expected + expected_str = " Expected values are {expected}." + + s = "" if isinstance(value, str) or len(value) <= 1 else "" + super().__init__( + element=element, + template=f"Invalid value{s} {{value}} for attribute {{attribute}} on element {{element}}.{expected_str}", + template_kwargs=template_kwargs, + ) - attribute: str - value: str - expected: Iterable[str] | None = None - def _message(self, *, as_html: bool) -> str: - if as_html: - (opening, closing) = ("", "") - value = html.escape(self.value) +@dataclass(frozen=True) +class ConversionError(RenderElementError): + """Could not convert a value to another type.""" + + def __init__(self, element: etree._Element, value: str, to_type: type, attribute: str | None = None): + template_kwargs = {"value": value, "type": to_type.__name__} + + in_attribute = "" + if attribute: + template_kwargs["attribute"] = attribute + in_attribute = " in attribute {attribute}" + + template = f"Unable to convert {{value}} to {{type}}{in_attribute} at element {{element}}." + super().__init__(element=element, template=template, template_kwargs=template_kwargs) + + +@dataclass(frozen=True) +class PlaceholderReferenceError(RenderElementError): + """An unknown or no placeholder was referenced.""" + + def __init__(self, element: etree._Element, placeholder: str | None, available: Collection[str]): + if placeholder is None: + template = "No placeholder was referenced." + template_kwargs = {} else: - (opening, closing) = ("'", "'") - value = self.value + if len(available) == 0: + provided = "No placeholders were provided." + else: + provided = "These are the provided placeholders: {available}." + template = f"Referenced placeholder {{placeholder}} was not found. {provided}" + template_kwargs = {"placeholder": placeholder, "available": available} + + super().__init__( + element=element, + template=template, + template_kwargs=template_kwargs, + ) - expected = "" - if self.expected: - expected = f" Expected one of [{opening}" + f"{closing}, {opening}".join(self.expected) + f"{closing}]." - return ( - f"Invalid value {opening}{value}{closing} for attribute {opening}{self.attribute}{closing} " - f"on element {opening}{self.element_representation}{closing}.{expected}" +@dataclass(frozen=True) +class InvalidCleanOptionError(RenderElementError): + """Invalid clean option.""" + + def __init__(self, element: etree._Element, option: str, expected: Collection[str]): + super().__init__( + element=element, + template="Invalid cleaning option {option}. Available options are {expected}.", + template_kwargs={"option": option, "expected": expected}, ) - @property - def message(self) -> str: - return self._message(as_html=False) - @property - def html_message(self) -> str: - return self._message(as_html=True) +@dataclass(frozen=True) +class UnknownElementError(RenderElementError): + """Unknown element with qpy-namespace.""" + + def __init__(self, element: etree._Element): + super().__init__( + element=element, + template="Unknown element {element}.", + ) @dataclass(frozen=True) @@ -106,11 +209,11 @@ def order(self) -> int: @property def message(self) -> str: - return f"Syntax error: {self.error.msg}" + return f"{self.error.msg}" @property def html_message(self) -> str: - return f"Invalid syntax: {html.escape(self.error.msg)}" + return f"{html.escape(self.error.msg)}" class RenderErrorCollection(Iterable, Sized): @@ -143,5 +246,7 @@ def log_render_errors(render_errors: RenderErrorCollections) -> None: errors_string = "" for error in errors: line = f"Line {error.line}: " if error.line else "" - errors_string += f"\n\t- {line}{error.message}" - _log.warning(f"{len(errors)} error(s) occurred while rendering {section}:{errors_string}") + errors_string += f"\n\t- {line}{error.type} - {error.message}" + error_count = len(errors) + s = "s" if error_count > 1 else "" + _log.warning(f"{error_count} error{s} occurred while rendering {section}:{errors_string}") diff --git a/questionpy_sdk/webserver/static/styles.css b/questionpy_sdk/webserver/static/styles.css index 517673a..e3e6f55 100644 --- a/questionpy_sdk/webserver/static/styles.css +++ b/questionpy_sdk/webserver/static/styles.css @@ -260,23 +260,36 @@ fieldset { text-decoration: underline; } -.container-render-errors table tr td:first-child, -.container-render-errors table tr th:first-child:not([scope="rowgroup"]) { - border-left: 0; - padding-right: 0.5rem; -} - .container-render-errors table tr td:first-child { vertical-align: top; text-align: right; font-variant-numeric: lining-nums tabular-nums; } +.container-render-errors table tr td:first-child, +.container-render-errors table tr th:first-child:not([scope="rowgroup"]) { + border-left: 0; + padding-right: 0.5rem; +} + .container-render-errors table tr td:last-child, .container-render-errors table tr th:last-child:not([scope="rowgroup"]) { border-right: 0; padding-left: 0.5rem; +} + +.container-render-errors table tr td:not(:first-child):not(:last-child), +.container-render-errors table tr th:not(:first-child):not(:last-child):not([scope="rowgroup"]) { + padding: 0 0.5rem; + vertical-align: top; +} +/* If the screen is to small, hide every column expect for the first and last (-> line and error message). */ +@media only screen and (max-width: 60rem) { + .container-render-errors table tr td:not(:first-child):not(:last-child), + .container-render-errors table tr th:not(:first-child):not(:last-child):not([scope="rowgroup"]) { + display: none; + } } .container-render-errors table tbody th { diff --git a/questionpy_sdk/webserver/templates/attempt.html.jinja2 b/questionpy_sdk/webserver/templates/attempt.html.jinja2 index c72a391..dbff95d 100644 --- a/questionpy_sdk/webserver/templates/attempt.html.jinja2 +++ b/questionpy_sdk/webserver/templates/attempt.html.jinja2 @@ -22,11 +22,13 @@ Line + Type Error-Message {% for error in errors %} {{ error.line or '' }} + {{ error.type }} {{ error.html_message|safe }} {% endfor %} diff --git a/ruff_defaults.toml b/ruff_defaults.toml index 5500045..afa2956 100644 --- a/ruff_defaults.toml +++ b/ruff_defaults.toml @@ -106,6 +106,7 @@ pydocstyle = { convention = "google" } [lint.per-file-ignores] "**/scripts/*" = ["INP001", "T201"] "**/tests/**/*" = ["PLC1901", "PLC2701", "PLR2004", "S", "TID252", "FBT"] +"**/question_ui/errors.py" = ["RUF027"] # Allow f-string without an `f` prefix for our custom error formatter. [lint.flake8-builtins] # help: (guess it's ok since built-in `help()` is for interactive use and no collisions are expected) diff --git a/tests/questionpy_sdk/webserver/test_data/faulty.xhtml b/tests/questionpy_sdk/webserver/test_data/faulty.xhtml index f2bd137..9c5e981 100644 --- a/tests/questionpy_sdk/webserver/test_data/faulty.xhtml +++ b/tests/questionpy_sdk/webserver/test_data/faulty.xhtml @@ -1,5 +1,15 @@
Unknown feedback type. + +
Unknown roles.
+ Unknown value. +
+ +
+
Missing placeholder.
+
Empty placeholder.
Missing attribute value. - Unknown feedback type.
diff --git a/tests/questionpy_sdk/webserver/test_question_ui.py b/tests/questionpy_sdk/webserver/test_question_ui.py index 0050727..bc31349 100644 --- a/tests/questionpy_sdk/webserver/test_question_ui.py +++ b/tests/questionpy_sdk/webserver/test_question_ui.py @@ -1,9 +1,8 @@ # This file is part of the QuestionPy SDK. (https://questionpy.org) # The QuestionPy SDK is free software released under terms of the MIT license. See LICENSE.md. # (c) Technische Universität Berlin, innoCampus -import re from importlib import resources -from typing import TYPE_CHECKING, Any +from typing import Any import pytest @@ -16,11 +15,15 @@ QuestionUIRenderer, XMLSyntaxError, ) +from questionpy_sdk.webserver.question_ui.errors import ( + ConversionError, + InvalidCleanOptionError, + PlaceholderReferenceError, + RenderError, + UnknownElementError, +) from tests.questionpy_sdk.webserver.conftest import assert_html_is_equal -if TYPE_CHECKING: - from questionpy_sdk.webserver.question_ui.errors import RenderError - @pytest.fixture def xml_content(request: pytest.FixtureRequest) -> str | None: @@ -320,7 +323,6 @@ def test_should_replace_shuffled_index(renderer: QuestionUIRenderer) -> None: @pytest.mark.render_params( xml="""
- Text Content Normal Content @@ -361,27 +363,33 @@ def test_should_replace_qpy_urls(renderer: QuestionUIRenderer) -> None: def test_errors_should_be_collected(renderer: QuestionUIRenderer) -> None: expected = """
+
+
Missing placeholder.
+
Empty placeholder.
Missing attribute value.
""" html, errors = renderer.render() - assert len(errors) == 3 - - invalid_attribute_error_message = re.compile( - r"Invalid value .+unknown.+ for attribute .+qpy:feedback.+ on element " r".+span.+. Expected one of \[.+]." - ) - expected_errors: list[tuple[type[RenderError], re.Pattern, int]] = [ - # Even though the syntax error occurs after an invalid attribute value error, it should be listed first. - (XMLSyntaxError, re.compile(".+"), 3), - (InvalidAttributeValueError, invalid_attribute_error_message, 2), - (InvalidAttributeValueError, invalid_attribute_error_message, 4), + assert len(errors) == 11 + + expected_errors: list[tuple[type[RenderError], int]] = [ + # Even though the syntax error occurs after all the other errors, it should be listed first. + (XMLSyntaxError, 14), + (InvalidAttributeValueError, 2), + (UnknownElementError, 3), + (InvalidAttributeValueError, 4), + (ConversionError, 5), + (ConversionError, 5), + (InvalidAttributeValueError, 5), + (InvalidAttributeValueError, 9), + (InvalidCleanOptionError, 12), + (PlaceholderReferenceError, 12), + (PlaceholderReferenceError, 13), ] for actual_error, expected_error in zip(errors, expected_errors, strict=True): - error_type, message, line = expected_error - assert actual_error.line == line + error_type, line = expected_error assert isinstance(actual_error, error_type) - assert message.match(actual_error.message) is not None - assert message.match(actual_error.html_message) is not None + assert actual_error.line == line assert_html_is_equal(html, expected)