Fix: Complex jexl issues (refactor(forms): rewrite structure and jexl evaluator)

.pre-commit-config.yaml

@@ -2,19 +2,19 @@ repos:
 -   repo: local
     hooks:
     - id: ruff-format
-      stages: [commit]
+      stages: [pre-commit]
       name: format code
       language: system
       entry: ruff format .
       types: [python]
     - id: ruff-check
-      stages: [commit]
+      stages: [pre-commit]
       name: check format,import
       language: system
-      entry: ruff check .
+      entry: ruff check --fix .
       types: [python]
     - id: reuse
-      stages: [commit]
+      stages: [pre-commit]
       name: reuse
       description: Check licenses
       entry: reuse lint

.reuse/dep5

@@ -33,6 +33,7 @@ License: CC0-1.0
 Files:
     caluma/*.py
     caluma/*.ambr
+    caluma/*.json
 Copyright: 2019 Adfinis AG <info@adfinis.com>
 License: GPL-3.0-or-later
 

caluma/caluma_core/exceptions.py

@@ -0,0 +1,16 @@
+# Some common exceptions
+
+
+class ConfigurationError(Exception):
+    """Invalid configuration detected.
+
+    Use this exception type if a configuration does not make
+    sense or is generally un-processable.
+
+    For example: circular dependencies in JEXL expressions,
+    invalid form hierarchies etc
+    """
+
+
+class QuestionMissing(Exception):
+    pass

caluma/caluma_core/jexl.py

@@ -243,18 +243,3 @@ def visit_Transform(self, transform):
                         yield arg.value
 
         yield from self.generic_visit(transform)
-
-
-class ExtractTransformSubjectAndArgumentsAnalyzer(CalumaAnalyzer):
-    """
-    Extract all referenced subjects and arguments of a given transforms.
-
-    If no transforms are given all references of all transforms will be extracted.
-    """
-
-    def visit_Transform(self, transform):
-        if not self.transforms or transform.name in self.transforms:
-            if not isinstance(transform.subject, type(transform)):
-                yield (transform.subject.value, transform.args)
-
-        yield from self.generic_visit(transform)

caluma/caluma_form/domain_logic.py

@@ -1,4 +1,5 @@
 from graphlib import TopologicalSorter
+from logging import getLogger
 from typing import Optional
 
 from django.db import transaction
@@ -8,10 +9,14 @@
 from caluma.caluma_core.models import BaseModel
 from caluma.caluma_core.relay import extract_global_id
 from caluma.caluma_form import models, structure, utils, validators
-from caluma.caluma_form.utils import update_or_create_calc_answer
+from caluma.caluma_form.utils import (
+    recalculate_field,
+)
 from caluma.caluma_user.models import BaseUser
 from caluma.utils import update_model
 
+log = getLogger(__name__)
+
 
 class BaseLogic:
     @staticmethod
@@ -153,17 +158,48 @@ def post_save(answer: models.Answer) -> models.Answer:
         return answer
 
     @staticmethod
-    def update_calc_dependents(answer):
-        if not answer.question.calc_dependents:
+    def update_calc_dependents(answer, update_info):
+        is_table = update_info and answer.question.type == models.Question.TYPE_TABLE
+        if not is_table and not answer.question.calc_dependents:
+            return
+        if not answer.document:
+            # Default answer
             return
+        log.debug("update_calc_dependents(%s)", answer)
 
         root_doc = utils.prefetch_document(answer.document.family_id)
-        struc = structure.FieldSet(root_doc, root_doc.form)
-
-        for question in models.Question.objects.filter(
-            pk__in=answer.question.calc_dependents
-        ):
-            update_or_create_calc_answer(question, root_doc, struc)
+        struc = structure.FieldSet(root_doc)
+
+        field = struc.find_field_by_answer(answer)
+        if is_table:
+            # We treat all children of the table as "changed"
+            # Find all children, and if any are of type calc (and are in the "created"
+            # update info bit), recalculate them.
+            for child in field.get_all_fields():
+                # TODO: if we're in a table row, how do we know that a dependant is
+                # *only* inside the table and therefore only *our* row needs
+                # *recalculating?
+                if (
+                    child.question.type == models.Question.TYPE_CALCULATED_FLOAT
+                    and child.parent._document.pk in update_info["created"]
+                ):
+                    recalculate_field(child)
+
+        for dep_slug in field.question.calc_dependents or []:
+            # ... maybe this is enough? Cause this will find the closest "match",
+            # going only to the outer context from a table if the field is not found
+            # inside of it
+            for dep_field in struc.find_all_fields_by_slug(dep_slug):
+                # Need to iterate, because a calc question could reside *inside*
+                # a table row, so there could be multiple of them, all needing to
+                # be updated because of "our" change
+
+                log.debug(
+                    "update_calc_dependents(%s): updating question %s",
+                    answer,
+                    dep_field.question.pk,
+                )
+                recalculate_field(dep_field)
 
     @classmethod
     @transaction.atomic
@@ -179,10 +215,11 @@ def create(
         if validated_data["question"].type == models.Question.TYPE_FILES:
             cls.update_answer_files(answer, files)
 
+        update_info = None
         if answer.question.type == models.Question.TYPE_TABLE:
-            answer.create_answer_documents(documents)
+            update_info = answer.create_answer_documents(documents)
 
-        cls.update_calc_dependents(answer)
+        cls.update_calc_dependents(answer, update_info)
 
         return answer
 
@@ -198,10 +235,11 @@ def update(cls, answer, validated_data, user: Optional[BaseUser] = None):
 
         BaseLogic.update(answer, validated_data, user)
 
+        update_info = None
         if answer.question.type == models.Question.TYPE_TABLE:
-            answer.create_answer_documents(documents)
+            update_info = answer.create_answer_documents(documents)
 
-        cls.update_calc_dependents(answer)
+        cls.update_calc_dependents(answer, update_info)
 
         answer.refresh_from_db()
         return answer
@@ -303,13 +341,15 @@ def _initialize_calculated_answers(document):
         """
         Initialize all calculated questions in the document.
 
-        In order to do this efficiently, we get all calculated questions with their dependents,
-        sort them topoligically, and then update their answer.
+        In order to do this efficiently, we get all calculated questions with
+        their dependents, sort them topoligically, and then update their answer.
         """
         root_doc = utils.prefetch_document(document.family_id)
-        struc = structure.FieldSet(root_doc, root_doc.form)
+        struc = structure.FieldSet(root_doc)
 
         calculated_questions = (
+            # TODO we could fetch those as fields from the structure. Minus a DB query,
+            # and we'd already have the fields as well
             models.Form.get_all_questions([(document.family or document).form_id])
             .filter(type=models.Question.TYPE_CALCULATED_FLOAT)
             .values("slug", "calc_dependents")
@@ -323,14 +363,10 @@ def _initialize_calculated_answers(document):
         # just reverse the resulting order.
         sorted_question_slugs = list(reversed(list(ts.static_order())))
 
-        # fetch all related questions in one query, but iterate according
-        # to pre-established sorting
-        _questions = models.Question.objects.in_bulk(sorted_question_slugs)
         for slug in sorted_question_slugs:
             print("question", slug)
-            update_or_create_calc_answer(
-                _questions[slug], document, struc, update_dependents=False
-            )
+            for field in struc.find_all_fields_by_slug(slug):
+                recalculate_field(field, update_recursively=False)
 
         return document