fix(form): support validation of nested table answers

Check table questions when searching the form structure.

caluma/caluma_form/jexl.py

@@ -6,7 +6,7 @@
 from pyjexl.evaluator import Context
 
 from ..caluma_core.jexl import JEXL, ExtractTransformReferenceAnalyzer
-from .models import Question
+from .structure import Field
 
 
 class QuestionMissing(Exception):
@@ -58,19 +58,18 @@ def __init__(self, validation_context=None, **kwargs):
         )
 
     def answer_transform(self, question_slug):
-        question = self._question(question_slug)
+        fields = self._structure.get_fields(question_slug)
 
-        if self.is_hidden(question):
-            if question.type in [
-                Question.TYPE_MULTIPLE_CHOICE,
-                Question.TYPE_DYNAMIC_MULTIPLE_CHOICE,
-                Question.TYPE_TABLE,
-            ]:
-                return []
+        if len(fields) == 0 or len(fields) > 1:  # pragma: no cover
+            raise RuntimeError(
+                "Answer transform cannot reference row_form questions from outside row context"
+            )
 
-            return None
+        field = fields[0]
+        if self.is_hidden(field):
+            return field.question.empty_value()
 
-        return self._structure.get_field(question_slug).value()
+        return field.value()
 
     def validate(self, expression, **kwargs):
         return super().validate(expression, QuestionValidatingAnalyzer)
@@ -81,17 +80,8 @@ def extract_referenced_questions(self, expr):
             expr, partial(ExtractTransformReferenceAnalyzer, transforms=transforms)
         )
 
-    def _question(self, slug):
-        field = self._structure.get_field(slug)
-        if field:
-            return field.question
-
-        raise QuestionMissing(
-            f"Question `{slug}` could not be found in form {self.context['form']}"
-        )
-
     @contextmanager
-    def use_question_context(self, question_slug):
+    def use_field_context(self, field: Field):
         """Context manger to temporarily overwrite self._structure.
 
         This is used so we can evaluate each JEXL expression in the context
@@ -103,99 +93,97 @@ def use_question_context(self, question_slug):
 
         # field's parent is the fieldset - which is a valid structure object
         old_structure = self._structure
-        self._structure = self._structure.get_field(question_slug).parent()
+        self._structure = field.parent() or self._structure
         yield
         self._structure = old_structure
 
-    def _all_containers_hidden(self, question):
-        """Check whether all containers of the given question are hidden.
+    def _get_referenced_fields(self, field: Field, expr: str):
+        deps = list(self.extract_referenced_questions(expr))
+        referenced_fields = [
+            ref_field for slug in deps for ref_field in self._structure.get_fields(slug)
+        ]
 
-        The question could be used in multiple sub-forms in the given
-        document structure. This goes through all paths from the main form
-        to the given question, and checks if they're hidden.
+        referenced_slugs = [ref.question.slug for ref in referenced_fields]
 
-        If all subforms are hidden where the question shows up,
-        the question shall be hidden as well.
-        """
-        paths = self._structure.root().paths_to_question(question.slug)
-
-        res = bool(paths) and all(
-            # the "inner" check here represents a single path. If any
-            # question is hidden, the question is not reachable via
-            # this path
-            bool(path) and any(self.is_hidden(fq) for fq in path if fq != question)
-            for path in paths
-        )
-        # The outer check verifies if all paths are "hidden" (ie have a hidden
-        # question in them). If all paths from the root form to the
-        # question are hidden, the question must indeed be hidden itself.
-
-        return res
+        for slug in deps:
+            if slug not in referenced_slugs:
+                raise QuestionMissing(
+                    f"Question `{slug}` could not be found in form {field.form}"
+                )
 
-    def _cache_key(self, question_slug):
-        field = self._structure.get_field(question_slug)
-        return (field.document.pk, question_slug)
+        return deps, referenced_fields
 
-    def is_hidden(self, question):
-        """Return True if the given question is hidden.
+    def is_hidden(self, field: Field):
+        """Return True if the given field is hidden.
 
         This checks whether the dependency questions are hidden, then
-        evaluates the question's is_hidden expression itself.
+        evaluates the field's is_hidden expression itself.
         """
-        cache_key = self._cache_key(question.pk)
+        cache_key = (field.document.pk, field.question.pk)
 
         if cache_key in self._cache["hidden"]:
             return self._cache["hidden"][cache_key]
+
         # Check visibility of dependencies before actually evaluating the `is_hidden`
         # expression. If all dependencies are hidden,
         # there is no way to evaluate our own visibility, so we default to
         # hidden state as well.
-
-        deps = list(self.extract_referenced_questions(question.is_hidden))
+        deps, referenced_fields = self._get_referenced_fields(
+            field, field.question.is_hidden
+        )
 
         # all() returns True for the empty set, thus we need to
         # check that we have some deps at all first
         all_deps_hidden = bool(deps) and all(
-            self.is_hidden(self._question(dep)) for dep in deps
+            self.is_hidden(ref_field) for ref_field in referenced_fields
         )
         if all_deps_hidden:
             self._cache["hidden"][cache_key] = True
             return True
 
         # Also check if the question is hidden indirectly,
         # for example via parent formquestion.
-        if self._all_containers_hidden(question):
+        parent = field.parent()
+        if parent and parent.question and self.is_hidden(parent):
             # no way this is shown somewhere
             self._cache["hidden"][cache_key] = True
             return True
+
         # if the question is visible-in-context and not hidden by invisible dependencies,
         # we can evaluate it's own is_hidden expression
+        with self.use_field_context(field):
+            self._cache["hidden"][cache_key] = self.evaluate(field.question.is_hidden)
 
-        with self.use_question_context(question.pk):
-            self._cache["hidden"][cache_key] = self.evaluate(question.is_hidden)
         return self._cache["hidden"][cache_key]
 
-    def is_required(self, question_field):
-        cache_key = (question_field.document.pk, question_field.question.pk)
-        question = question_field.question
+    def is_required(self, field: Field):
+        cache_key = (field.document.pk, field.question.pk)
+        question = field.question
 
         if cache_key in self._cache["required"]:
             return self._cache["required"][cache_key]
 
-        deps = list(self.extract_referenced_questions(question.is_required))
+        deps, referenced_fields = self._get_referenced_fields(
+            field, question.is_required
+        )
 
-        if bool(deps) and all(self.is_hidden(self._question(dep)) for dep in deps):
-            # all dependent questions are hidden. cannot evaluate,
-            # so assume requiredness to be False
+        # all() returns True for the empty set, thus we need to
+        # check that we have some deps at all first
+        all_deps_hidden = bool(deps) and all(
+            self.is_hidden(ref_field) for ref_field in referenced_fields
+        )
+        if all_deps_hidden:
             ret = False
         else:
-            with self.use_question_context(question.pk):
+            with self.use_field_context(field):
                 ret = self.evaluate(question.is_required)
         self._cache["required"][cache_key] = ret
         return ret
 
-    def evaluate_calc_value(self, question_field):
+    def evaluate(self, expr, raise_on_error=True):
         try:
-            return self.evaluate(question_field.question.calc_expression)
+            return super().evaluate(expr)
         except TypeError:
+            if raise_on_error:
+                raise
             return None

caluma/caluma_form/signals.py

@@ -81,28 +81,23 @@ def _update_or_create_calc_answer(question, document):
 
     struc = structure.FieldSet(root_doc, root_doc.form)
 
-    for path in struc.paths_to_question(question.slug):
-        element = struc
-        for formquestion in path:
-            element = element.get_field(formquestion.slug)
-
+    for field in struc.get_fields(question.slug):
         jexl = QuestionJexl(
             {
-                "form": element.form,
-                "document": element.document,
-                "structure": element.parent(),
+                "form": field.form,
+                "document": field.document,
+                "structure": field.parent(),
             }
         )
-        value = jexl.evaluate_calc_value(element)
+
+        value = jexl.evaluate(field.question.calc_expression, raise_on_error=False)
 
         try:
-            ans = models.Answer.objects.get(
-                question=question, document=element.document
-            )
+            ans = models.Answer.objects.get(question=question, document=field.document)
             update_model(ans, {"value": value})
         except models.Answer.DoesNotExist:
             models.Answer.objects.create(
-                question=question, document=element.document, value=value
+                question=question, document=field.document, value=value
             )
 
 

caluma/caluma_form/structure.py

@@ -1,7 +1,7 @@
 """Hierarchical representation of a document / form."""
 import weakref
 from functools import singledispatch
-from typing import Optional
+from typing import List
 
 from .models import AnswerDocument, Question
 
@@ -140,35 +140,40 @@ def fields(self):
         return self._fields
 
     @property
-    def sub_forms(self):
+    def sub_forms(self) -> List[Field]:
         if self._sub_forms is None:
-            self._sub_forms = {
-                field.question.slug: field
+            self._sub_forms = [
+                field
                 for field in self.children()
                 if field.question.type == Question.TYPE_FORM
-            }
+            ] + [
+                child
+                for field in self.children()
+                for child in field.children()
+                if field.question.type == Question.TYPE_TABLE
+            ]
         return self._sub_forms
 
-    def get_field(
-        self, question_slug: str, check_parent: bool = True
-    ) -> Optional[Field]:
+    def get_fields(self, question_slug: str, check_parent: bool = True) -> List[Field]:
 
         field = self.fields.get(question_slug)
 
-        if not field and check_parent:
-            field = self.parent().get_field(question_slug) if self.parent() else None
         if field:
-            return field
+            return [field]
+        elif check_parent:
+            fields = self.parent().get_fields(question_slug) if self.parent() else []
+            if fields:
+                return fields
 
         # OK start looking in subforms / row forms below our level.
         # Since we're looking down, we're disallowing recursing to outer context
         # to avoid recursing back to where we are
-        for subform in self.sub_forms.values():
-            sub_field = subform.get_field(question_slug, check_parent=False)
-            if sub_field:
-                return sub_field
+        for subform in self.sub_forms:
+            sub_fields = subform.get_fields(question_slug, check_parent=False)
+            if sub_fields:
+                return sub_fields
         # if we reach this line, we didn't find the question
-        return None
+        return []
 
     @object_local_memoise
     def children(self):
@@ -184,27 +189,6 @@ def children(self):
             for question in self.form.questions.all()
         ]
 
-    @object_local_memoise
-    def paths_to_question(self, slug):
-        res = []
-        prefix = [self.question] if self.question else []
-
-        if slug in self.fields:
-            res.append(prefix + [self.fields[slug].question])
-        for field in self.children():
-
-            if field.question.type == Question.TYPE_FORM:
-                for sub_path in field.paths_to_question(slug):
-                    res.append(prefix + sub_path)
-
-            elif field.question.type == Question.TYPE_TABLE:
-                for child in field.children():
-                    for sub_path in child.paths_to_question(slug):
-                        # TODO: This should be covered, but is a case that will probably
-                        # never happen in real life
-                        res.append(prefix + sub_path)  # pragma: no cover
-        return res
-
     def __repr__(self):
         q_slug = self.question.slug if self.question else None
         if q_slug:

caluma/caluma_form/tests/test_jexl.py

@@ -82,8 +82,9 @@ def test_all_deps_hidden(db, form, document_factory, form_question_factory):
             "structure": structure.FieldSet(document, document.form),
         }
     )
-    assert qj.is_hidden(q2)
-    assert not qj.is_required(structure.Field(document, document.form, q2))
+    field = structure.Field(document, document.form, q2)
+    assert qj.is_hidden(field)
+    assert not qj.is_required(field)
 
 
 @pytest.mark.parametrize("fq_is_hidden", ["true", "false"])
@@ -320,16 +321,26 @@ def test_answer_transform_on_hidden_question(info, form_and_document):
     ],
 )
 def test_answer_transform_on_hidden_question_types(
-    info, form_and_document, question_type, expected_value
+    info,
+    form_and_document,
+    document_factory,
+    answer_factory,
+    question_type,
+    expected_value,
 ):
     form, document, questions, answers = form_and_document(
         use_table=True, use_subform=True
     )
-
-    questions[
-        "form"
-    ].is_hidden = (
-        f"'top_question'|answer == {expected_value} && 'table'|answer|mapby('column')"
+    table = questions["table"]
+    row_form = table.row_form
+    row_doc = document_factory(form=row_form)
+    answer_factory(document=row_doc, question_id="column")
+    answers["table"].documents.add(row_doc)
+
+    questions["form"].is_hidden = (
+        f"'top_question'|answer == {expected_value}"
+        " && 'table'|answer|mapby('column')[0]"
+        " && 'table'|answer|mapby('column')[1]"
     )
     questions["form"].save()
 
@@ -346,7 +357,8 @@ def test_answer_transform_on_hidden_question_types(
         }
     )
 
-    assert qj.is_hidden(questions["form"])
+    field = structure.Field(document, form, questions["form"])
+    assert qj.is_hidden(field)
 
 
 @pytest.mark.parametrize(

caluma/caluma_form/tests/test_validators.py

@@ -295,7 +295,7 @@ def test_validate_empty_answers(
 ):
 
     struct = structure.FieldSet(document, document.form)
-    field = struct.get_field(question.slug)
+    field = struct.get_fields(question.slug)[0]
     answer_value = field.value() if field else field
     assert answer_value == expected_value