From b1e9f5165954d1b3779b7c0be94275a6acee3186 Mon Sep 17 00:00:00 2001 From: David Vogt Date: Wed, 15 Jan 2025 15:21:00 +0100 Subject: [PATCH 1/6] chore: update pre-commit config format/structure --- .pre-commit-config.yaml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index de6cc6370..a8c1d09c7 100644 --- a/.pre-commit-config.yaml +++ b/.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 From 641d6125fbecb074da03a8d9d5127eb40ea177ce Mon Sep 17 00:00:00 2001 From: David Vogt Date: Tue, 14 Jan 2025 16:55:25 +0100 Subject: [PATCH 2/6] fix(structure): correctly sort questions and table rows Table rows were sorted, but backwards; questions were not sorted at all, and thus might have lead to unpredictable behaviour. We noe explicitly sort this correctly, therefore making things a bit more testable. --- caluma/caluma_form/structure.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/caluma/caluma_form/structure.py b/caluma/caluma_form/structure.py index 687b1b090..503af7216 100644 --- a/caluma/caluma_form/structure.py +++ b/caluma/caluma_form/structure.py @@ -4,7 +4,7 @@ from functools import singledispatch from typing import List, Optional -from .models import Question +from .models import FormQuestion, Question def object_local_memoise(method): @@ -123,7 +123,7 @@ def children(self): # for the answerdocument_set. Sorting in DB would re-issue the query rows = sorted( self.answer.answerdocument_set.all(), - key=lambda answer_document: answer_document.sort, + key=lambda answer_document: -answer_document.sort, ) return [ FieldSet( @@ -243,15 +243,17 @@ def get_field( @object_local_memoise def children(self): answers = {ans.question_id: ans for ans in self.document.answers.all()} + formquestions = FormQuestion.objects.filter(form=self.form).order_by("-sort") + return [ Field.factory( document=self.document, form=self.form, - question=question, - answer=answers.get(question.slug), + question=fq.question, + answer=answers.get(fq.question.slug), parent=self, ) - for question in self.form.questions.all() + for fq in formquestions ] def set_answer(self, question_slug, answer): From f58a5a8cd2fa08a1df543b9f088726f5c3476b9c Mon Sep 17 00:00:00 2001 From: David Vogt Date: Tue, 14 Jan 2025 16:51:15 +0100 Subject: [PATCH 3/6] chore(test): add test case that demonstrates jexl calculation problems Calculated questions do not work correctly when located inside a table row: The recalculation is currently triggered on the root document, which will only find one of the rows, and update that - while likely ignoring the row where the actual dependency is located. This test is intended to demonstrate the problem, and thus will currently fail. --- .reuse/dep5 | 1 + caluma/caluma_form/tests/test_complex_jexl.py | 192 ++++++++ .../tests/test_data/complex_jexl_context.json | 446 ++++++++++++++++++ 3 files changed, 639 insertions(+) create mode 100644 caluma/caluma_form/tests/test_complex_jexl.py create mode 100644 caluma/caluma_form/tests/test_data/complex_jexl_context.json diff --git a/.reuse/dep5 b/.reuse/dep5 index adcb533dc..20e89b1a4 100644 --- a/.reuse/dep5 +++ b/.reuse/dep5 @@ -33,6 +33,7 @@ License: CC0-1.0 Files: caluma/*.py caluma/*.ambr + caluma/*.json Copyright: 2019 Adfinis AG License: GPL-3.0-or-later diff --git a/caluma/caluma_form/tests/test_complex_jexl.py b/caluma/caluma_form/tests/test_complex_jexl.py new file mode 100644 index 000000000..638d74edf --- /dev/null +++ b/caluma/caluma_form/tests/test_complex_jexl.py @@ -0,0 +1,192 @@ +# Test cases for the (NEW!) structure utility class +from functools import singledispatch +from pathlib import Path + +import pytest +from django.core.management import call_command + +from caluma.caluma_form import api, structure +from caluma.caluma_form.models import AnswerDocument, Document, Form, Question + + +def get_doc_structure(document): + """Return a list of strings that represent the given document's structure.""" + + ind = {"i": 0} + res = [] + + @singledispatch + def visit(vis): + raise Exception(f"generic visit(): {vis}") + + @visit.register(structure.FieldSet) + def _(vis): + res.append(" " * ind["i"] + f"FieldSet({vis.form.slug})") + ind["i"] += 1 + for c in vis.children(): + visit(c) + ind["i"] -= 1 + + @visit.register(structure.Element) + def _(vis): + res.append( + " " * ind["i"] + + f"Field({vis.question.slug}, {vis.answer.value if vis.answer else None})" + ) + ind["i"] += 1 + for c in vis.children(): + visit(c) + ind["i"] -= 1 + + struc = structure.FieldSet(document, document.form) + visit(struc) + return res + + +@pytest.fixture +def complex_jexl_form(): + # Complex JEXL evaluation tests: + # * Recalculation witin table rows + # * Visibility checks in "outer" form with indirect calc question evaluation + data_file = Path(__file__).parent / "test_data/complex_jexl_context.json" + assert data_file.exists() + call_command("loaddata", str(data_file)) + return Form.objects.get(slug="demo-formular-1") + + +@pytest.fixture +def complex_jexl_doc(complex_jexl_form): + # -> root_doc, row1, row2 as tuple + doc = Document.objects.create(form=complex_jexl_form) + + api.save_answer( + Question.objects.get(pk="demo-outer-question-1"), + doc, + value="demo-outer-question-1-outer-option-a", + ) + table_ans = api.save_answer( + Question.objects.get(pk="demo-outer-table-question-1"), + doc, + ) + + row1 = AnswerDocument.objects.create( + answer=table_ans, + document=Document.objects.create(form=table_ans.question.row_form, family=doc), + sort=2, + ).document + row2 = AnswerDocument.objects.create( + answer=table_ans, + document=Document.objects.create(form=table_ans.question.row_form, family=doc), + sort=1, + ).document + + api.save_answer( + Question.objects.get(pk="demo-table-question-1"), document=row1, value=3 + ) + + api.save_answer( + Question.objects.get(pk="demo-table-question-1"), document=row2, value=20 + ) + return doc, row1, row2 + + +def test_evaluating_calc_inside_table( + transactional_db, complex_jexl_form, complex_jexl_doc +): + doc, *_ = complex_jexl_doc + + assert get_doc_structure(doc) == [ + "FieldSet(demo-formular-1)", + " Field(demo-outer-table-question-1, None)", + " FieldSet(demo-table-form-1)", + " Field(demo-table-question-1, 3)", + " Field(demo-table-question-2, 1)", + " FieldSet(demo-table-form-1)", + " Field(demo-table-question-1, 20)", + " Field(demo-table-question-2, 100)", + " Field(demo-outer-question-1, demo-outer-question-1-outer-option-a)", + " Field(demo-outer-table-question-2, None)", + ] + + +def test_update_calc_dependency_inside_table( + transactional_db, complex_jexl_form, complex_jexl_doc +): + doc, row1, row2 = complex_jexl_doc + + assert get_doc_structure(doc) == [ + "FieldSet(demo-formular-1)", + " Field(demo-outer-table-question-1, None)", + " FieldSet(demo-table-form-1)", + " Field(demo-table-question-1, 3)", + " Field(demo-table-question-2, 1)", + " FieldSet(demo-table-form-1)", + " Field(demo-table-question-1, 2)", + " Field(demo-table-question-2, 1)", + " Field(demo-outer-question-1, demo-outer-question-1-outer-option-a)", + " Field(demo-outer-table-question-2, None)", + ] + + api.save_answer( + Question.objects.get(pk="demo-table-question-1"), document=row1, value=30 + ) + assert get_doc_structure(doc) == [ + "FieldSet(demo-formular-1)", + " Field(demo-outer-table-question-1, None)", + " FieldSet(demo-table-form-1)", + " Field(demo-table-question-1, 30)", + " Field(demo-table-question-2, 100)", + " FieldSet(demo-table-form-1)", + " Field(demo-table-question-1, 2)", + " Field(demo-table-question-2, 1)", + " Field(demo-outer-question-1, demo-outer-question-1-outer-option-a)", + " Field(demo-outer-table-question-2, None)", + ] + + api.save_answer( + Question.objects.get(pk="demo-table-question-1"), document=row1, value=3 + ) + assert get_doc_structure(doc) == [ + "FieldSet(demo-formular-1)", + " Field(demo-outer-table-question-1, None)", + " FieldSet(demo-table-form-1)", + " Field(demo-table-question-1, 3)", + " Field(demo-table-question-2, 1)", + " FieldSet(demo-table-form-1)", + " Field(demo-table-question-1, 2)", + " Field(demo-table-question-2, 1)", + " Field(demo-outer-question-1, demo-outer-question-1-outer-option-a)", + " Field(demo-outer-table-question-2, None)", + ] + + api.save_answer( + Question.objects.get(pk="demo-table-question-1"), document=row2, value=20 + ) + assert get_doc_structure(doc) == [ + "FieldSet(demo-formular-1)", + " Field(demo-outer-table-question-1, None)", + " FieldSet(demo-table-form-1)", + " Field(demo-table-question-1, 3)", + " Field(demo-table-question-2, 1)", + " FieldSet(demo-table-form-1)", + " Field(demo-table-question-1, 20)", + " Field(demo-table-question-2, 100)", + " Field(demo-outer-question-1, demo-outer-question-1-outer-option-a)", + " Field(demo-outer-table-question-2, None)", + ] + + api.save_answer( + Question.objects.get(pk="demo-table-question-1"), document=row2, value=2 + ) + assert get_doc_structure(doc) == [ + "FieldSet(demo-formular-1)", + " Field(demo-outer-table-question-1, None)", + " FieldSet(demo-table-form-1)", + " Field(demo-table-question-1, 3)", + " Field(demo-table-question-2, 1)", + " FieldSet(demo-table-form-1)", + " Field(demo-table-question-1, 2)", + " Field(demo-table-question-2, 1)", + " Field(demo-outer-question-1, demo-outer-question-1-outer-option-a)", + " Field(demo-outer-table-question-2, None)", + ] diff --git a/caluma/caluma_form/tests/test_data/complex_jexl_context.json b/caluma/caluma_form/tests/test_data/complex_jexl_context.json new file mode 100644 index 000000000..34dcb4135 --- /dev/null +++ b/caluma/caluma_form/tests/test_data/complex_jexl_context.json @@ -0,0 +1,446 @@ +[ +{ + "model": "caluma_form.form", + "pk": "demo-formular-1", + "fields": { + "created_at": "2024-02-16T09:46:57.788Z", + "modified_at": "2024-02-16T09:46:57.788Z", + "created_by_user": null, + "created_by_group": null, + "modified_by_user": null, + "modified_by_group": null, + "name": "{\"en\": \"Formular-1\"}", + "description": "{\"en\": \"\"}", + "meta": {}, + "is_published": true, + "is_archived": false, + "source": null + } +}, +{ + "model": "caluma_form.option", + "pk": "demo-outer-question-1-outer-option-b", + "fields": { + "created_at": "2025-01-13T16:44:16.611Z", + "modified_at": "2025-01-13T16:44:16.611Z", + "created_by_user": null, + "created_by_group": null, + "modified_by_user": null, + "modified_by_group": null, + "label": "{\"en\": \"Outer Option B\"}", + "is_archived": false, + "meta": {}, + "source": null + } +}, +{ + "model": "caluma_form.option", + "pk": "demo-outer-question-1-outer-option-a", + "fields": { + "created_at": "2025-01-13T16:44:16.611Z", + "modified_at": "2025-01-13T16:44:16.611Z", + "created_by_user": null, + "created_by_group": null, + "modified_by_user": null, + "modified_by_group": null, + "label": "{\"en\": \"Outer Option A\"}", + "is_archived": false, + "meta": {}, + "source": null + } +}, +{ + "model": "caluma_form.question", + "pk": "demo-outer-question-1", + "fields": { + "created_at": "2025-01-13T16:44:16.671Z", + "modified_at": "2025-01-14T07:58:30.499Z", + "created_by_user": null, + "created_by_group": null, + "modified_by_user": null, + "modified_by_group": null, + "label": "{\"en\": \"Outer Question 1\"}", + "type": "choice", + "is_required": "true", + "is_hidden": "false", + "is_archived": false, + "placeholder": "{\"en\": null}", + "info_text": "{\"en\": null}", + "hint_text": "{\"en\": null}", + "static_content": "{\"en\": \"\"}", + "configuration": {}, + "meta": {}, + "data_source": null, + "row_form": null, + "sub_form": null, + "source": null, + "format_validators": "[]", + "default_answer": null, + "calc_expression": null, + "calc_dependents": "[]" + } +}, +{ + "model": "caluma_form.question", + "pk": "demo-outer-table-question-1", + "fields": { + "created_at": "2025-01-14T07:39:16.987Z", + "modified_at": "2025-01-14T07:39:16.987Z", + "created_by_user": null, + "created_by_group": null, + "modified_by_user": null, + "modified_by_group": null, + "label": "{\"en\": \"Outer Table Question 1\"}", + "type": "table", + "is_required": "true", + "is_hidden": "false", + "is_archived": false, + "placeholder": "{\"en\": null}", + "info_text": "{\"en\": null}", + "hint_text": "{\"en\": null}", + "static_content": "{\"en\": \"\"}", + "configuration": {}, + "meta": {}, + "data_source": null, + "row_form": "demo-table-form-1", + "sub_form": null, + "source": null, + "format_validators": "[]", + "default_answer": null, + "calc_expression": null, + "calc_dependents": "[]" + } +}, +{ + "model": "caluma_form.question", + "pk": "demo-outer-table-question-2", + "fields": { + "created_at": "2025-01-14T07:48:17.961Z", + "modified_at": "2025-01-14T07:48:17.961Z", + "created_by_user": null, + "created_by_group": null, + "modified_by_user": null, + "modified_by_group": null, + "label": "{\"en\": \"Outer Table Question 2\"}", + "type": "table", + "is_required": "true", + "is_hidden": "false", + "is_archived": false, + "placeholder": "{\"en\": null}", + "info_text": "{\"en\": null}", + "hint_text": "{\"en\": null}", + "static_content": "{\"en\": \"\"}", + "configuration": {}, + "meta": {}, + "data_source": null, + "row_form": "demo-table-form-2", + "sub_form": null, + "source": null, + "format_validators": "[]", + "default_answer": null, + "calc_expression": null, + "calc_dependents": "[]" + } +}, +{ + "model": "caluma_form.questionoption", + "pk": "demo-outer-question-1.demo-outer-question-1-outer-option-a", + "fields": { + "created_at": "2025-01-13T16:44:16.686Z", + "modified_at": "2025-01-13T16:44:16.686Z", + "created_by_user": null, + "created_by_group": null, + "modified_by_user": null, + "modified_by_group": null, + "question": "demo-outer-question-1", + "option": "demo-outer-question-1-outer-option-a", + "sort": 2 + } +}, +{ + "model": "caluma_form.questionoption", + "pk": "demo-outer-question-1.demo-outer-question-1-outer-option-b", + "fields": { + "created_at": "2025-01-13T16:44:16.680Z", + "modified_at": "2025-01-13T16:44:16.680Z", + "created_by_user": null, + "created_by_group": null, + "modified_by_user": null, + "modified_by_group": null, + "question": "demo-outer-question-1", + "option": "demo-outer-question-1-outer-option-b", + "sort": 1 + } +}, +{ + "model": "caluma_form.formquestion", + "pk": "demo-formular-1.demo-outer-table-question-1", + "fields": { + "created_at": "2025-01-14T07:39:17.045Z", + "modified_at": "2025-01-14T07:39:17.045Z", + "created_by_user": null, + "created_by_group": null, + "modified_by_user": null, + "modified_by_group": null, + "form": "demo-formular-1", + "question": "demo-outer-table-question-1", + "sort": 3 + } +}, +{ + "model": "caluma_form.formquestion", + "pk": "demo-formular-1.demo-outer-question-1", + "fields": { + "created_at": "2025-01-13T16:44:16.725Z", + "modified_at": "2025-01-13T16:44:16.725Z", + "created_by_user": null, + "created_by_group": null, + "modified_by_user": null, + "modified_by_group": null, + "form": "demo-formular-1", + "question": "demo-outer-question-1", + "sort": 2 + } +}, +{ + "model": "caluma_form.formquestion", + "pk": "demo-formular-1.demo-outer-table-question-2", + "fields": { + "created_at": "2025-01-14T07:48:18.049Z", + "modified_at": "2025-01-14T07:48:18.049Z", + "created_by_user": null, + "created_by_group": null, + "modified_by_user": null, + "modified_by_group": null, + "form": "demo-formular-1", + "question": "demo-outer-table-question-2", + "sort": 1 + } +}, +{ + "model": "caluma_form.form", + "pk": "demo-table-form-1", + "fields": { + "created_at": "2025-01-14T07:36:10.888Z", + "modified_at": "2025-01-14T07:36:18.005Z", + "created_by_user": null, + "created_by_group": null, + "modified_by_user": null, + "modified_by_group": null, + "name": "{\"en\": \"Table Form 1\"}", + "description": "{\"en\": \"\"}", + "meta": {}, + "is_published": false, + "is_archived": false, + "source": null + } +}, +{ + "model": "caluma_form.form", + "pk": "demo-table-form-2", + "fields": { + "created_at": "2025-01-14T07:45:59.594Z", + "modified_at": "2025-01-14T07:45:59.594Z", + "created_by_user": null, + "created_by_group": null, + "modified_by_user": null, + "modified_by_group": null, + "name": "{\"en\": \"Table Form 2\"}", + "description": "{\"en\": \"\"}", + "meta": {}, + "is_published": false, + "is_archived": false, + "source": null + } +}, +{ + "model": "caluma_form.question", + "pk": "demo-table-question-1", + "fields": { + "created_at": "2025-01-14T07:37:00.748Z", + "modified_at": "2025-01-14T07:38:25.075Z", + "created_by_user": null, + "created_by_group": null, + "modified_by_user": null, + "modified_by_group": null, + "label": "{\"en\": \"Table Question 1\"}", + "type": "integer", + "is_required": "true", + "is_hidden": "false", + "is_archived": false, + "placeholder": "{\"en\": null}", + "info_text": "{\"en\": null}", + "hint_text": "{\"en\": null}", + "static_content": "{\"en\": \"\"}", + "configuration": { + "max_value": null, + "min_value": null + }, + "meta": {}, + "data_source": null, + "row_form": null, + "sub_form": null, + "source": null, + "format_validators": "[]", + "default_answer": null, + "calc_expression": null, + "calc_dependents": "[\"demo-table-question-2\"]" + } +}, +{ + "model": "caluma_form.question", + "pk": "demo-table-question-2", + "fields": { + "created_at": "2025-01-14T07:38:25.084Z", + "modified_at": "2025-01-14T07:41:42.007Z", + "created_by_user": null, + "created_by_group": null, + "modified_by_user": null, + "modified_by_group": null, + "label": "{\"en\": \"Table Question 2\"}", + "type": "calculated_float", + "is_required": "false", + "is_hidden": "false", + "is_archived": false, + "placeholder": "{\"en\": null}", + "info_text": "{\"en\": \"\"}", + "hint_text": "{\"en\": \"\"}", + "static_content": "{\"en\": \"\"}", + "configuration": {}, + "meta": {}, + "data_source": null, + "row_form": null, + "sub_form": null, + "source": null, + "format_validators": "[]", + "default_answer": null, + "calc_expression": "'demo-table-question-1'|answer > 10 ? 100 : 1", + "calc_dependents": "[]" + } +}, +{ + "model": "caluma_form.question", + "pk": "demo-table-question-outer-ref-hidden", + "fields": { + "created_at": "2025-01-14T07:52:50.053Z", + "modified_at": "2025-01-14T10:01:38.803Z", + "created_by_user": null, + "created_by_group": null, + "modified_by_user": null, + "modified_by_group": null, + "label": "{\"en\": \"Table Question Outer Ref Hidden\"}", + "type": "integer", + "is_required": "true", + "is_hidden": "!'demo-outer-question-1'|answer", + "is_archived": false, + "placeholder": "{\"en\": \"\"}", + "info_text": "{\"en\": \"\"}", + "hint_text": "{\"en\": \"\"}", + "static_content": "{\"en\": \"\"}", + "configuration": { + "max_value": null, + "min_value": null + }, + "meta": {}, + "data_source": null, + "row_form": null, + "sub_form": null, + "source": null, + "format_validators": "[]", + "default_answer": null, + "calc_expression": null, + "calc_dependents": "[\"demo-table-question-outer-ref-calc\"]" + } +}, +{ + "model": "caluma_form.question", + "pk": "demo-table-question-outer-ref-calc", + "fields": { + "created_at": "2025-01-14T07:47:42.242Z", + "modified_at": "2025-01-14T10:01:38.827Z", + "created_by_user": null, + "created_by_group": null, + "modified_by_user": null, + "modified_by_group": null, + "label": "{\"en\": \"Table Question Outer Ref Calc\"}", + "type": "calculated_float", + "is_required": "false", + "is_hidden": "false", + "is_archived": false, + "placeholder": "{\"en\": null}", + "info_text": "{\"en\": \"\"}", + "hint_text": "{\"en\": \"\"}", + "static_content": "{\"en\": \"\"}", + "configuration": {}, + "meta": {}, + "data_source": null, + "row_form": null, + "sub_form": null, + "source": null, + "format_validators": "[]", + "default_answer": null, + "calc_expression": "'demo-table-question-outer-ref-hidden'|answer", + "calc_dependents": "[]" + } +}, +{ + "model": "caluma_form.formquestion", + "pk": "demo-table-form-1.demo-table-question-1", + "fields": { + "created_at": "2025-01-14T07:37:00.792Z", + "modified_at": "2025-01-14T07:37:00.792Z", + "created_by_user": null, + "created_by_group": null, + "modified_by_user": null, + "modified_by_group": null, + "form": "demo-table-form-1", + "question": "demo-table-question-1", + "sort": 2 + } +}, +{ + "model": "caluma_form.formquestion", + "pk": "demo-table-form-2.demo-table-question-outer-ref-hidden", + "fields": { + "created_at": "2025-01-14T07:52:50.098Z", + "modified_at": "2025-01-14T07:52:50.098Z", + "created_by_user": null, + "created_by_group": null, + "modified_by_user": null, + "modified_by_group": null, + "form": "demo-table-form-2", + "question": "demo-table-question-outer-ref-hidden", + "sort": 2 + } +}, +{ + "model": "caluma_form.formquestion", + "pk": "demo-table-form-1.demo-table-question-2", + "fields": { + "created_at": "2025-01-14T07:38:25.131Z", + "modified_at": "2025-01-14T07:38:25.131Z", + "created_by_user": null, + "created_by_group": null, + "modified_by_user": null, + "modified_by_group": null, + "form": "demo-table-form-1", + "question": "demo-table-question-2", + "sort": 1 + } +}, +{ + "model": "caluma_form.formquestion", + "pk": "demo-table-form-2.demo-table-question-outer-ref-calc", + "fields": { + "created_at": "2025-01-14T07:47:42.314Z", + "modified_at": "2025-01-14T07:47:42.314Z", + "created_by_user": null, + "created_by_group": null, + "modified_by_user": null, + "modified_by_group": null, + "form": "demo-table-form-2", + "question": "demo-table-question-outer-ref-calc", + "sort": 1 + } +} +] From 21f0a361ad383c5613086f31cbaf7d3a6f722723 Mon Sep 17 00:00:00 2001 From: Alana Luyten Date: Thu, 16 Jan 2025 10:12:13 +0100 Subject: [PATCH 4/6] chore(tests): add test case for calculated questions in tables --- caluma/caluma_form/tests/test_complex_jexl.py | 83 +++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/caluma/caluma_form/tests/test_complex_jexl.py b/caluma/caluma_form/tests/test_complex_jexl.py index 638d74edf..f842160fa 100644 --- a/caluma/caluma_form/tests/test_complex_jexl.py +++ b/caluma/caluma_form/tests/test_complex_jexl.py @@ -190,3 +190,86 @@ def test_update_calc_dependency_inside_table( " Field(demo-outer-question-1, demo-outer-question-1-outer-option-a)", " Field(demo-outer-table-question-2, None)", ] + + +def test_update_calc_dependency_inside_table_with_outer_reference( + transactional_db, complex_jexl_form, complex_jexl_doc +): + doc, _, _ = complex_jexl_doc + + assert get_doc_structure(doc) == [ + "FieldSet(demo-formular-1)", + " Field(demo-outer-table-question-1, None)", + " FieldSet(demo-table-form-1)", + " Field(demo-table-question-1, 3)", + " Field(demo-table-question-2, 1)", + " FieldSet(demo-table-form-1)", + " Field(demo-table-question-1, 20)", + " Field(demo-table-question-2, None)", # TODO: Should be 100 + " Field(demo-outer-question-1, demo-outer-question-1-outer-option-a)", + " Field(demo-outer-table-question-2, None)", + ] + + # TODO: This implementation corresponds to the current frontend logic, this + # might change so that table row documents are attached on creation + new_table_doc = api.save_document(form=Form.objects.get(pk="demo-table-form-2")) + + assert get_doc_structure(new_table_doc) == [ + " FieldSet(demo-table-form-2)", + " Field(demo-table-question-outer-ref-hidden, None)", + " Field(demo-table-question-outer-ref-calc, None)", + ] + + api.save_answer( + Question.objects.get(pk="demo-table-question-outer-ref-hidden"), + document=new_table_doc, + value=30, + ) + + assert get_doc_structure(new_table_doc) == [ + " FieldSet(demo-table-form-2)", + " Field(demo-table-question-outer-ref-hidden, 30)", + " Field(demo-table-question-outer-ref-calc, 30)", + ] + + api.save_answer( + question="demo-outer-table-question-2", document=doc, value=[new_table_doc.pk] + ) + + assert get_doc_structure(doc) == [ + "FieldSet(demo-formular-1)", + " Field(demo-outer-table-question-1, None)", + " FieldSet(demo-table-form-1)", + " Field(demo-table-question-1, 3)", + " Field(demo-table-question-2, 1)", + " FieldSet(demo-table-form-1)", + " Field(demo-table-question-1, 20)", + " Field(demo-table-question-2, 100)", + " Field(demo-outer-question-1, demo-outer-question-1-outer-option-a)", + " Field(demo-outer-table-question-2, None)", + " FieldSet(demo-table-form-2)", + " Field(demo-table-question-outer-ref-hidden, 30)", + " Field(demo-table-question-outer-ref-calc, 30)", + ] + + api.save_answer( + Question.objects.get(pk="demo-table-question-outer-ref-hidden"), + document=new_table_doc, + value=20, + ) + + assert get_doc_structure(doc) == [ + "FieldSet(demo-formular-1)", + " Field(demo-outer-table-question-1, None)", + " FieldSet(demo-table-form-1)", + " Field(demo-table-question-1, 3)", + " Field(demo-table-question-2, 1)", + " FieldSet(demo-table-form-1)", + " Field(demo-table-question-1, 20)", + " Field(demo-table-question-2, 100)", + " Field(demo-outer-question-1, demo-outer-question-1-outer-option-a)", + " Field(demo-outer-table-question-2, None)", + " FieldSet(demo-table-form-2)", + " Field(demo-table-question-outer-ref-hidden, 20)", + " Field(demo-table-question-outer-ref-calc, 20)", + ] From 350ab96ab0089fe89e8e89843f30ad49f77d9196 Mon Sep 17 00:00:00 2001 From: Your Name Date: Tue, 18 Feb 2025 14:25:02 +0100 Subject: [PATCH 5/6] refactor(forms): rewrite structure / jexl evaluator The new structure / jexl evaluator works a bit differently: Instead of trying to replace evaluation contexts during recursive evaluation (for example `is_hidden` checks), we now have a local JEXL runtime for each field. Also, the JEXL expressions (or their results, rather) are heavily cached and should speed things up significantly. Test cases: We're trying to keep the test cases' meaning 100% unchanged - the only modifications currently are some improved assertion messages, so debugging becomes easier, as well as refactoring some for better readability. Some tests are extended, and some are now better documented, to cover more aspects and explain in more detail what our assumptions and expectations actually are. BREAKING CHANGE: Code that uses the form jexl and / or structure code most likely will need to be rewritten. The changes are small-ish, but still semantically not exactly equal. --- caluma/caluma_core/exceptions.py | 16 + caluma/caluma_core/jexl.py | 15 - caluma/caluma_form/jexl.py | 205 ++--- caluma/caluma_form/serializers.py | 2 +- caluma/caluma_form/structure.py | 860 +++++++++++++----- caluma/caluma_form/tests/test_complex_jexl.py | 338 +++---- caluma/caluma_form/tests/test_document.py | 243 +++-- caluma/caluma_form/tests/test_jexl.py | 187 +++- caluma/caluma_form/tests/test_structure.py | 245 +++++ caluma/caluma_form/tests/test_validators.py | 38 +- caluma/caluma_form/validators.py | 290 +++--- caluma/conftest.py | 5 + 12 files changed, 1594 insertions(+), 850 deletions(-) create mode 100644 caluma/caluma_core/exceptions.py create mode 100644 caluma/caluma_form/tests/test_structure.py diff --git a/caluma/caluma_core/exceptions.py b/caluma/caluma_core/exceptions.py new file mode 100644 index 000000000..c40e2e9e7 --- /dev/null +++ b/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 diff --git a/caluma/caluma_core/jexl.py b/caluma/caluma_core/jexl.py index 27c797bc9..b6439e90b 100644 --- a/caluma/caluma_core/jexl.py +++ b/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) diff --git a/caluma/caluma_form/jexl.py b/caluma/caluma_form/jexl.py index 79c3a718b..44a0dd85b 100644 --- a/caluma/caluma_form/jexl.py +++ b/caluma/caluma_form/jexl.py @@ -1,22 +1,30 @@ -from collections import defaultdict -from contextlib import contextmanager +import weakref +from collections import ChainMap from functools import partial from pyjexl.analysis import ValidatingAnalyzer -from pyjexl.evaluator import Context + +from caluma.caluma_core.exceptions import QuestionMissing from ..caluma_core.jexl import ( JEXL, ExtractTransformArgumentAnalyzer, ExtractTransformSubjectAnalyzer, - ExtractTransformSubjectAndArgumentsAnalyzer, ) -from .models import Question -from .structure import Field +""" +Form JEXL handling + +Design principles: -class QuestionMissing(Exception): - pass +* The JEXL classes do not deal with context switching between questions anymore +* The QuestionJexl class only sets up the "runtime", any context is used from the + structure code +* We only deal with the *evaluation*, no transform/extraction is happening here. + that code is mostly fine and doesn't need a rewrite +* Caching is done by the structure code, not here +* JEXL evaluation happens lazily, but the results are cached. +""" class QuestionValidatingAnalyzer(ValidatingAnalyzer): @@ -28,58 +36,44 @@ def visit_Transform(self, transform): class QuestionJexl(JEXL): - def __init__(self, validation_context=None, **kwargs): - if validation_context: - if "jexl_cache" not in validation_context: - validation_context["jexl_cache"] = defaultdict(dict) - self._cache = validation_context["jexl_cache"] - else: - self._cache = defaultdict(dict) + def __init__(self, field, **kwargs): + """Initialize QuestionJexl. + Note: The field *may* be set to `None` if you're intending + to use the object for expression analysis only (exctract_* methods + for example) + """ super().__init__(**kwargs) - - self._structure = None - self._form = None - - context_data = None - - if validation_context: - # cleaned up variant - self._form = validation_context.get("form") - self._structure = validation_context.get("structure") - context_data = { - "form": self._form.slug if self._form else None, - "info": self._structure, - } - - self.context = Context(context_data) + self.field = weakref.proxy(field) if field else None self.add_transform("answer", self.answer_transform) def answer_transform(self, question_slug, *args): - field = self._structure.get_field(question_slug) + field = self.field.get_field(question_slug) - # The first and only argument is the default value. If passed the field - # is not required and we return that argument. - if not field and len(args): - return args[0] - - if self.is_hidden(field): + def _default_or_empty(): + if len(args): + return args[0] return field.question.empty_value() - # This overrides the logic in field.value() to consider visibility for - # table cells - elif field.question.type == Question.TYPE_TABLE and field.answer is not None: - return [ - { - cell.question.slug: cell.value() - for cell in row.children() - if not self.is_hidden(cell) - } - for row in field.children() - ] + if not field: + if args: + return args[0] + else: + # No default arg, so we must raise an exception + raise QuestionMissing( + f"Question `{question_slug}` could not be found in form {self.field.get_form()}" + ) + + if field.is_hidden(): + # Hidden fields *always* return the empty value, even if we have + # a default + return field.question.empty_value() + elif field.is_empty(): + # not hidden, but empty + return _default_or_empty() - return field.value() + return field.get_value() def validate(self, expression, **kwargs): return super().validate(expression, QuestionValidatingAnalyzer) @@ -90,118 +84,21 @@ def extract_referenced_questions(self, expr): expr, partial(ExtractTransformSubjectAnalyzer, transforms=transforms) ) - def extract_referenced_questions_with_arguments(self, expr): - transforms = ["answer"] - yield from self.analyze( - expr, - partial(ExtractTransformSubjectAndArgumentsAnalyzer, transforms=transforms), - ) - def extract_referenced_mapby_questions(self, expr): transforms = ["mapby"] yield from self.analyze( expr, partial(ExtractTransformArgumentAnalyzer, transforms=transforms) ) - @contextmanager - 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 - of the corresponding question, not from where the question was - referenced. - This is relevant in table questions and form questions, so we always - lookup the correct answer value (no "crosstalk" between rows, for example) - """ - - # field's parent is the fieldset - which is a valid structure object - old_structure = self._structure - self._structure = field.parent() or self._structure - yield - self._structure = old_structure - - def _get_referenced_fields(self, field: Field, expr: str): - deps = list(self.extract_referenced_questions_with_arguments(expr)) - referenced_fields = [self._structure.get_field(slug) for slug, _ in deps] - - referenced_slugs = [ref.question.slug for ref in referenced_fields if ref] - - for slug, args in deps: - required = len(args) == 0 - if slug not in referenced_slugs and required: - raise QuestionMissing( - f"Question `{slug}` could not be found in form {field.form}" - ) - - return [field for field in referenced_fields if field] - - 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 field's is_hidden expression itself. - """ - 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. - 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(referenced_fields) and all( - 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. - 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) - - return self._cache["hidden"][cache_key] - - 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] - - referenced_fields = self._get_referenced_fields(field, question.is_required) - - # all() returns True for the empty set, thus we need to - # check that we have some deps at all first - all_deps_hidden = bool(referenced_fields) and all( - self.is_hidden(ref_field) for ref_field in referenced_fields - ) - if all_deps_hidden: - ret = False - else: - with self.use_field_context(field): - ret = self.evaluate(question.is_required) - self._cache["required"][cache_key] = ret - return ret - def evaluate(self, expr, raise_on_error=True): try: - return super().evaluate(expr) - except (TypeError, ValueError, ZeroDivisionError): + return super().evaluate(expr, ChainMap(self.context)) + except ( + TypeError, + ValueError, + ZeroDivisionError, + AttributeError, + ): if raise_on_error: raise return None diff --git a/caluma/caluma_form/serializers.py b/caluma/caluma_form/serializers.py index cd5861472..3427e5798 100644 --- a/caluma/caluma_form/serializers.py +++ b/caluma/caluma_form/serializers.py @@ -18,7 +18,7 @@ class QuestionJexlField(serializers.JexlField): def __init__(self, **kwargs): - super().__init__(QuestionJexl(), **kwargs) + super().__init__(QuestionJexl(field=None), **kwargs) class ButtonActionField(serializers.CalumaChoiceField): diff --git a/caluma/caluma_form/structure.py b/caluma/caluma_form/structure.py index 503af7216..7e586276d 100644 --- a/caluma/caluma_form/structure.py +++ b/caluma/caluma_form/structure.py @@ -1,275 +1,666 @@ -"""Hierarchical representation of a document / form.""" +""" +Structure - Fast and correct form structure representation. +Design requirements: + +* Fast initialisation from a document. Use preload if needed to reduce + number of queries +* Eager loading of a full document structure. No lazy-loading with questionable + performance expectations +* Correct navigation via question slugs (JEXL references) from any context +* Fast lookups once initialized +* Extensible for caching JEXL expression results + +* No properties. Code is always in methods +""" + +from __future__ import annotations + +import collections +import copy +import typing import weakref -from functools import singledispatch -from typing import List, Optional +from abc import ABC +from collections.abc import Iterable +from dataclasses import dataclass, field +from functools import singledispatch, wraps +from logging import getLogger +from typing import Optional + +from django.db.models import QuerySet -from .models import FormQuestion, Question +from caluma.caluma_core import exceptions + +if typing.TYPE_CHECKING: # pragma: no cover + from caluma.caluma_form.jexl import QuestionJexl + +from caluma.caluma_form.models import ( + Answer, + AnswerDocument, + FormQuestion, + Question, +) + +log = getLogger(__name__) def object_local_memoise(method): + """Decorate a method to become object-local memoised. + + In other words - The method will cache it's results. If the method is called + twice with the same arguments, it will return the cached result instead. + + For debugging purposes, you can also set `object_local_memoise.enabled` + to `False`, which will then behave just as if the memoising didn't happen. + """ + + @wraps(method) def new_method(self, *args, **kwargs): + if not object_local_memoise.enabled: # pragma: no cover + # for debugging purposes + return method(self, *args, **kwargs) if not hasattr(self, "_memoise"): self._memoise = {} + self._memoise_hit_count = 0 + self._memoise_miss_count = 0 key = str([args, kwargs, method]) if key in self._memoise: + object_local_memoise.hit_count += 1 + self._memoise_hit_count += 1 return self._memoise[key] ret = method(self, *args, **kwargs) + self._memoise_miss_count += 1 + object_local_memoise.miss_count += 1 self._memoise[key] = ret return ret return new_method -class Element: - aliases = {} +# This should only be set to `False` for debugging +setattr(object_local_memoise, "enabled", True) - def __init__(self, parent=None): - self._parent = weakref.ref(parent) if parent else None +# Statistics - for analysis / debugging +setattr(object_local_memoise, "hit_count", 0) +setattr(object_local_memoise, "miss_count", 0) - def parent(self): - return self._parent() if self._parent else None - def children(self): # pragma: no cover - return [] +def clear_memoise(obj): + """Clear memoise cache for given object. - def root(self): - parent = self.parent() - if parent: - return parent.root() - return self + If an object uses the `@object_local_memoise` decorator, you can then + call `clear_memoise()` on that object to clear all it's cached data. + """ + obj._memoise = {} - def get(self, name, default=None): - name = self.aliases.get(name, name) - out = getattr(self, name) +@dataclass +class BaseField(ABC): + """Base class for the field types. This is the interface we aim to provide.""" - # if a method is requested, execute it before continuing - if callable(out): - out = out() - if isinstance(out, Element) or isinstance(out, dict): - return out - if out is None: - return None - return str(out) + parent: Optional["FieldSet"] = field(default=None) + question: Optional[Question] = field(default=None) + answer: Optional[Answer] = field(default=None) -class Field(Element): - def __init__(self, document, form, question, answer=None, parent=None): - super().__init__(parent) - self.document = document - self.form = form - self.question = question - self.answer = answer + @object_local_memoise + def get_evaluator(self) -> QuestionJexl: + """Return the JEXL evaluator for this field.""" + + # JEXL is implemented such that it has one context in the engine, and + # trying to swap it out to switch context is just problematic. So we use + # one JEXL instance per field. + + # deferred import to avoid circular dependency + from caluma.caluma_form.jexl import QuestionJexl + + # The "info" block is provided by the global context, but we need + # to patch it to represent some local information: The "form" and + # "document" bits should point to the current context, not to the global + # structure. This is a bit unfortunate, but we *have* to fill this in + # two separate places to avoid breaking compatibility + context = collections.ChainMap( + # We need a deep copy of the global context, so we can + # extend the info block without leaking + copy.deepcopy(self.get_global_context()), + self.get_context(), + ) - @classmethod - def factory(cls, document, form, question, answer=None, parent=None): - if question.type == Question.TYPE_FORM: - return FieldSet( - document, form=question.sub_form, question=question, parent=parent - ) - elif question.type == Question.TYPE_TABLE: - return RowField( - document, - form=question.row_form, - question=question, - answer=answer, - parent=parent, - ) + context["info"].update(self.get_local_info_context()) - return Field(document, form, question, answer, parent=parent) + # Legacy form ref - pointing to the root form + context["form"] = self._get_root().get_form().slug - def value(self): - if self.answer is None: - # no answer object at all - return empty in every case - return self.question.empty_value() + return QuestionJexl(field=self, context=context) - elif self.answer.value is not None: - return self.answer.value + def _get_root(self): + return self.parent._get_root() if self.parent else self - elif self.question.type == Question.TYPE_TABLE: # pragma: no cover - return [ - {cell.question.slug: cell.value() for cell in row.children()} - for row in self.children() - ] + @object_local_memoise + def get_local_info_context(self): + """Return the dictionary to be used in the local `info` context block. + + Properties (See Ember-Caluma's field source for reference): + - `form`: Legacy property pointing to the root form. + -> defined in get_evaluator() as we're only returning `info` here + * Form information + - `info.form`: The form this question is attached to. + - `info.formMeta`: The meta of the form this question is attached to. + - `info.parent.form`: The parent form if applicable. + - `info.parent.formMeta`: The parent form meta if applicable. + - `info.root.form`: The new property for the root form. + - `info.root.formMeta`: The new property for the root form meta. + * Case information is taken from the global context + - `info.case.form`: The cases' form (works for task forms and case forms). + - `info.case.workflow`: The cases' workflow (works for task forms and case forms). + - `info.case.root.form`: The _root_ cases' form (works for task forms and case forms). + - `info.case.root.workflow`: The _root_ cases' workflow (works for task forms and case forms). - elif self.question.type == Question.TYPE_FILES: - return [f.name for f in self.answer.files.all()] + """ + form = self.get_form() - elif self.question.type == Question.TYPE_DATE: - return self.answer.date + if parent_info := self.get_parent_fieldset(): + parent_data = { + "form": parent_info.get_form().slug, + "formMeta": parent_info.get_form().meta, + } + else: + parent_data = None + return { + # "_type": type(self).__qualname__, + "question": self.question.slug if self.question else None, + "form": form and form.slug or None, + "formMeta": form and form.meta or None, + "parent": parent_data, + # TODO how is "root" expected to behave if we're *already* on root? + "root": self._get_root().get_local_info_context() if self.parent else None, + } - elif self.question.type in ( - Question.TYPE_MULTIPLE_CHOICE, - Question.TYPE_DYNAMIC_MULTIPLE_CHOICE, - ): - return [] + def get_parent_fieldset(self): + """Return the parent fieldset, according to JEXL semantics. - # no value, no special handling - return None + In JEXL, the parent refers to th next field up that represents another + form. In Rows for example, this is two levels up, but in regular nested + forms, it's only one level up. + """ + my_form = self.get_form() + p = self.parent + while p and p.get_form() == my_form: + p = p.parent + return p - def __repr__(self): - return f"