fix: remove unnecessary signals, don't recompute on copy, tests

caluma/caluma_form/api.py

@@ -23,7 +23,10 @@ def save_answer(
     data.update(kwargs)
 
     answer = models.Answer.objects.filter(question=question, document=document).first()
-    answer = domain_logic.SaveAnswerLogic.get_new_answer(data, user, answer)
+    if answer:
+        answer = domain_logic.SaveAnswerLogic.update(answer, data, user)
+    else:
+        answer = domain_logic.SaveAnswerLogic.create(data, user)
 
     return answer
 
@@ -45,6 +48,7 @@ def save_default_answer(
     data.update(kwargs)
 
     answer = question.default_answer
+    # TODO refactor to use create / udpate as well?
     answer = domain_logic.SaveDefaultAnswerLogic.get_new_answer(data, user, answer)
 
     return answer

caluma/caluma_form/domain_logic.py

@@ -173,13 +173,7 @@ def create(
         if answer.question.type == models.Question.TYPE_TABLE:
             answer.create_answer_documents(documents)
 
-        print("creating answer", flush=True)
         if answer.question.calc_dependents:
-            print(
-                "creating answer for question",
-                answer.question,
-                answer.question.calc_dependents,
-            )
             root_doc = answer.document.family
             root_doc = (
                 models.Document.objects.filter(pk=answer.document.family_id)
@@ -188,13 +182,11 @@ def create(
                 )
                 .first()
             )
-            print("init structure top level")
             struc = structure.FieldSet(root_doc, root_doc.form)
 
             for question in models.Question.objects.filter(
                 pk__in=answer.question.calc_dependents
             ):
-                print(f"recalculating {question} from domain logic _create_")
                 update_or_create_calc_answer(question, root_doc, struc)
 
         return answer

caluma/caluma_form/models.py

@@ -396,9 +396,9 @@ def set_family(self, root_doc):
 
     def copy(self, family=None, user=None):
         """Create a copy including all its answers."""
-        from caluma.caluma_form.utils import recalculate_answers_from_document
 
-        # defer calculated questions, as many unecessary recomputations will happen otherwise
+        # no need to update calculated questions, since calculated answers
+        # are copied as well
         meta = dict(self.meta)
         meta["_defer_calculation"] = True
 
@@ -423,7 +423,6 @@ def copy(self, family=None, user=None):
 
         new_document.meta.pop("_defer_calculation", None)
         new_document.save()
-        recalculate_answers_from_document(new_document)
 
         return new_document
 

caluma/caluma_form/signals.py

@@ -1,11 +1,6 @@
-import itertools
-from django.db.models import Prefetch
-
 from django.db.models.signals import (
-    m2m_changed,
     post_delete,
     post_init,
-    post_save,
     pre_delete,
     pre_save,
 )
@@ -16,11 +11,7 @@
 from caluma.utils import disable_raw
 
 from . import models
-from .utils import (
-    recalculate_answers_from_document,
-    update_calc_dependents,
-    update_or_create_calc_answer,
-)
+from .utils import update_calc_dependents
 
 
 @receiver(pre_create_historical_record, sender=models.HistoricalAnswer)
@@ -94,83 +85,3 @@ def remove_calc_dependents(sender, instance, **kwargs):
     update_calc_dependents(
         instance.slug, old_expr=instance.calc_expression, new_expr="false"
     )
-
-
-# Update calculated answer on post_save
-#
-# Try to update the calculated answer value whenever a mutation on a possibly
-# related model is performed.
-
-
-@receiver(post_save, sender=models.Question)
-@disable_raw
-@filter_events(lambda instance: instance.type == models.Question.TYPE_CALCULATED_FLOAT)
-def update_calc_from_question(sender, instance, created, update_fields, **kwargs):
-    # TODO optimize: only update answers if calc_expression is updated
-    # needs to happen during save() or __init__()
-
-    for document in models.Document.objects.filter(form__questions=instance):
-        update_or_create_calc_answer(instance, document)
-
-
-@receiver(post_save, sender=models.FormQuestion)
-@disable_raw
-@filter_events(
-    lambda instance: instance.question.type == models.Question.TYPE_CALCULATED_FLOAT
-)
-def update_calc_from_form_question(sender, instance, created, **kwargs):
-    for document in instance.form.documents.all():
-        update_or_create_calc_answer(instance.question, document)
-
-
-# @receiver(post_save, sender=models.Answer)
-# @disable_raw
-# @filter_events(lambda instance: instance.document and instance.question.calc_dependents)
-# def update_calc_from_answer(sender, instance, **kwargs):
-#     # If there is no document on the answer it means that it's a default
-#     # answer. They shouldn't trigger a recalculation of a calculated field
-#     # even when they are technically listed as a dependency.
-#     # Also skip non-referenced answers.
-#     if instance.document.family.meta.get("_defer_calculation"):
-#         return
-#
-#     if instance.question.type == models.Question.TYPE_TABLE:
-#         print("skipping update calc of table questions in event layer, because we don't have access to the question slug here")
-#         return
-#
-#     print(f"saved answer to {instance.question.pk}, recalculate dependents:")
-#     document = models.Document.objects.filter(pk=instance.document_id).prefetch_related(
-#         *build_document_prefetch_statements(
-#             "family", prefetch_options=True
-#         ),
-#     ).first()
-#
-#     for question in models.Question.objects.filter(
-#         pk__in=instance.question.calc_dependents
-#     ):
-#         print(f"- {question.pk}")
-#         update_or_create_calc_answer(question, document)
-
-
-@receiver(post_save, sender=models.Document)
-@disable_raw
-# We're only interested in table row forms
-@filter_events(lambda instance, created: instance.pk != instance.family_id or created)
-def update_calc_from_document(sender, instance, created, **kwargs):
-    # we do this in a more focused way (only updating the calc dependents in the domain logic
-    # recalculate_answers_from_document(instance)
-    pass
-
-
-@receiver(m2m_changed, sender=models.AnswerDocument)
-def update_calc_from_answerdocument(sender, instance, **kwargs):
-    dependents = instance.document.form.questions.exclude(
-        calc_dependents=[]
-    ).values_list("calc_dependents", flat=True)
-
-    dependent_questions = list(itertools.chain(*dependents))
-    # TODO: when is this even called?
-    print(f"answerdocument {instance.pk} changed, update {dependent_questions}")
-
-    for question in models.Question.objects.filter(pk__in=dependent_questions):
-        update_or_create_calc_answer(question, instance.document)

caluma/caluma_form/structure.py

@@ -254,6 +254,11 @@ def children(self):
             for question in self.form.questions.all()
         ]
 
+    def set_answer(self, question_slug, answer):
+        field = self.get_field(question_slug)
+        if field:
+            field.answer = answer
+
     def __repr__(self):
         q_slug = self.question.slug if self.question else None
         if q_slug:

caluma/caluma_form/tests/test_document.py

@@ -492,7 +492,7 @@ def test_save_document(
         )
         assert (doc.pk == document.pk) == update
 
-        assert doc.answers.count() == 1 if update else 3
+        assert doc.answers.count() == (0 if update else 3)
         if not update:
             assert sorted([str(a.value) for a in doc.answers.iterator()]) == [
                 "23",

caluma/caluma_form/tests/test_question.py

@@ -1057,9 +1057,6 @@ def test_nested_calculated_question(
         question__calc_expression=expr,
     )
 
-    calc_ans = document.answers.get(question_id="calc")
-    assert calc_ans.value == expected
-
     for dep in calc_deps:
         questions[dep].refresh_from_db()
         assert questions[dep].calc_dependents == ["calc"]
@@ -1109,34 +1106,6 @@ def test_recursive_calculated_question(
     assert calc_ans.value == 8
 
 
-def test_calculated_question_update_calc_expr(
-    db, schema_executor, form_and_document, form_question_factory
-):
-    form, document, questions_dict, answers_dict = form_and_document(True, True)
-
-    sub_question = questions_dict["sub_question"]
-    sub_question.type = models.Question.TYPE_INTEGER
-    sub_question.save()
-    sub_question_a = answers_dict["sub_question"]
-    sub_question_a.value = 100
-    sub_question_a.save()
-
-    calc_question = form_question_factory(
-        form=form,
-        question__slug="calc_question",
-        question__type=models.Question.TYPE_CALCULATED_FLOAT,
-        question__calc_expression="'sub_question'|answer + 1",
-    ).question
-
-    calc_ans = document.answers.get(question_id="calc_question")
-    assert calc_ans.value == 101
-
-    calc_question.calc_expression = "'sub_question'|answer -1"
-    calc_question.save()
-    calc_ans.refresh_from_db()
-    assert calc_ans.value == 99
-
-
 def test_calculated_question_answer_document(
     db,
     schema_executor,
@@ -1165,27 +1134,36 @@ def test_calculated_question_answer_document(
         question__calc_expression="'table'|answer|mapby('column')[0] + 'table'|answer|mapby('column')[1]",
     ).question
 
-    calc_ans = document.answers.get(question_id="calc_question")
-    assert calc_ans.value is None
-
     # adding another row will make make the expression valid
     row_doc = document_factory(form=row_form, family=document)
     column_a2 = answer_factory(document=row_doc, question_id=column.slug, value=200)
-    table_a.documents.add(row_doc)
 
-    calc_ans.refresh_from_db()
+    api.save_answer(
+        question=table,
+        document=document,
+        documents=list(table_a.documents.all()) + [row_doc],
+    )
+
+    calc_ans = document.answers.get(question_id="calc_question")
     assert calc_ans.value == 300
 
-    column_a2.value = 100
-    column_a2.save()
+    api.save_answer(
+        question=column_a2.question,
+        document=row_doc,
+        value=100,
+    )
     calc_ans.refresh_from_db()
     column.refresh_from_db()
     assert column.calc_dependents == ["calc_question"]
     assert calc_ans.value == 200
 
     # removing the row will make it invalid again
     table_a.documents.remove(row_doc)
-    row_doc.delete()
+    api.save_answer(
+        question=table,
+        document=document,
+        documents=[table_a.documents.first()],
+    )
     calc_ans.refresh_from_db()
     assert calc_ans.value is None
 

caluma/caluma_form/utils.py

@@ -101,27 +101,20 @@ def update_calc_dependents(slug, old_expr, new_expr):
         question.save()
 
 
-def update_or_create_calc_answer(question, document, struc, update_dependents=True):
-    # print("callsite", inspect.stack()[1][3], flush=True)
-
+def update_or_create_calc_answer(
+    question, document, struc=None, update_dependents=True
+):
     root_doc = document.family
 
     if not struc:
-        print("init structure")
         struc = structure.FieldSet(root_doc, root_doc.form)
-    else:
-        # print("reusing struc")
-        pass
-    start = time()
+
     field = struc.get_field(question.slug)
-    # print(f"get_field: ", time() - start)
 
     # skip if question doesn't exist in this document structure
     if field is None:
-        print("-- didn't find question, stopping", question)
         return
 
-    print("-- found field", question)
     jexl = QuestionJexl(
         {"form": field.form, "document": field.document, "structure": field.parent()}
     )
@@ -131,26 +124,23 @@ def update_or_create_calc_answer(question, document, struc, update_dependents=Tr
     # be invalid, in which case we return None
     value = jexl.evaluate(field.question.calc_expression, raise_on_error=False)
 
-    models.Answer.objects.update_or_create(
+    answer, _ = models.Answer.objects.update_or_create(
         question=question, document=field.document, defaults={"value": value}
     )
+    # also save new answer to structure for reuse
+    struc.set_answer(question.slug, answer)
 
     if update_dependents:
-        print(
-            f"{question.pk}: updating {len(field.question.calc_dependents)} calc dependents)"
-        )
         for _question in models.Question.objects.filter(
             pk__in=field.question.calc_dependents
         ):
-            # print(f"{question.pk} -> {_question.pk}")
             update_or_create_calc_answer(_question, document, struc)
 
 
 def recalculate_answers_from_document(instance):
-    """When a table row is added, update dependent questions"""
     if (instance.family or instance).meta.get("_defer_calculation"):
-        print("- defered")
         return
+
     print(f"saved document {instance.pk}, recalculate answers")
     for question in models.Form.get_all_questions(
         [(instance.family or instance).form_id]