From a47c6e1b0349fbf9e184074c316384ff19d596f3 Mon Sep 17 00:00:00 2001 From: Christian Zosel Date: Mon, 30 Dec 2024 17:13:49 +0100 Subject: [PATCH] fix: remove unnecessary signals, don't recompute on copy, tests --- caluma/caluma_form/api.py | 6 +- caluma/caluma_form/domain_logic.py | 8 - caluma/caluma_form/models.py | 5 +- caluma/caluma_form/signals.py | 91 +------ caluma/caluma_form/structure.py | 5 + caluma/caluma_form/tests/test_document.py | 296 +++------------------- caluma/caluma_form/tests/test_question.py | 56 ++-- caluma/caluma_form/utils.py | 26 +- 8 files changed, 68 insertions(+), 425 deletions(-) diff --git a/caluma/caluma_form/api.py b/caluma/caluma_form/api.py index c0a8255af..a34a23538 100644 --- a/caluma/caluma_form/api.py +++ b/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 diff --git a/caluma/caluma_form/domain_logic.py b/caluma/caluma_form/domain_logic.py index 23e2289f2..8ee9cf630 100644 --- a/caluma/caluma_form/domain_logic.py +++ b/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 diff --git a/caluma/caluma_form/models.py b/caluma/caluma_form/models.py index fe695a924..f1bd59b8e 100644 --- a/caluma/caluma_form/models.py +++ b/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 diff --git a/caluma/caluma_form/signals.py b/caluma/caluma_form/signals.py index 7a2f338d1..7cb4fc3b0 100644 --- a/caluma/caluma_form/signals.py +++ b/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) diff --git a/caluma/caluma_form/structure.py b/caluma/caluma_form/structure.py index 4e1853474..687b1b090 100644 --- a/caluma/caluma_form/structure.py +++ b/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: diff --git a/caluma/caluma_form/tests/test_document.py b/caluma/caluma_form/tests/test_document.py index 4aed01b32..6d7088a71 100644 --- a/caluma/caluma_form/tests/test_document.py +++ b/caluma/caluma_form/tests/test_document.py @@ -466,10 +466,8 @@ def test_save_document( # if updating, the resulting document must be the same assert same_id == update - assert ( - len(result.data["saveDocument"]["document"]["answers"]["edges"]) == 1 - if update - else 3 + assert len(result.data["saveDocument"]["document"]["answers"]["edges"]) == ( + 0 if update else 3 ) if not update: assert sorted( @@ -492,7 +490,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", @@ -511,269 +509,35 @@ def test_save_document( @pytest.mark.parametrize( "question__type,question__configuration,question__data_source,question__format_validators,answer__value,answer__date,mutation,success", [ - ( - Question.TYPE_INTEGER, - {}, - None, - [], - 1, - None, - "SaveDocumentIntegerAnswer", - True, - ), - ( - Question.TYPE_INTEGER, - {"min_value": 100}, - None, - [], - 1, - None, - "SaveDocumentIntegerAnswer", - False, - ), + (Question.TYPE_INTEGER, {}, None, [], 1, None, "SaveDocumentIntegerAnswer", True), + (Question.TYPE_INTEGER, {"min_value": 100}, None, [], 1, None, "SaveDocumentIntegerAnswer", False), (Question.TYPE_FLOAT, {}, None, [], 2.1, None, "SaveDocumentFloatAnswer", True), - ( - Question.TYPE_FLOAT, - {"min_value": 100.0}, - None, - [], - 1, - None, - "SaveDocumentFloatAnswer", - False, - ), - ( - Question.TYPE_TEXT, - {}, - None, - [], - "Test", - None, - "SaveDocumentStringAnswer", - True, - ), - ( - Question.TYPE_TEXT, - {"max_length": 1}, - None, - [], - "toolong", - None, - "SaveDocumentStringAnswer", - False, - ), - ( - Question.TYPE_DATE, - {}, - None, - [], - None, - "1900-01-01", - "SaveDocumentDateAnswer", - False, - ), - ( - Question.TYPE_DATE, - {}, - None, - [], - None, - "2019-02-22", - "SaveDocumentDateAnswer", - True, - ), - ( - Question.TYPE_FILES, - {}, - None, - [], - None, - None, - "SaveDocumentFilesAnswer", - False, - ), - ( - Question.TYPE_FILES, - {}, - None, - [], - [{"name": "some-file.pdf"}], - None, - "SaveDocumentFilesAnswer", - True, - ), - ( - Question.TYPE_FILES, - {}, - None, - [], - [{"name": "not-exist.pdf"}], - None, - "SaveDocumentFilesAnswer", - True, - ), - ( - Question.TYPE_TEXT, - {"min_length": 10}, - None, - [], - "tooshort", - None, - "SaveDocumentStringAnswer", - False, - ), - ( - Question.TYPE_TABLE, - {}, - None, - [], - None, - None, - "SaveDocumentTableAnswer", - True, - ), - ( - Question.TYPE_TEXTAREA, - {}, - None, - [], - "Test", - None, - "SaveDocumentStringAnswer", - True, - ), - ( - Question.TYPE_TEXTAREA, - {"max_length": 1}, - None, - [], - "toolong", - None, - "SaveDocumentStringAnswer", - False, - ), - ( - Question.TYPE_MULTIPLE_CHOICE, - {}, - None, - [], - ["option-slug"], - None, - "SaveDocumentListAnswer", - True, - ), - ( - Question.TYPE_MULTIPLE_CHOICE, - {}, - None, - [], - ["option-slug", "option-invalid-slug"], - None, - "SaveDocumentStringAnswer", - False, - ), - ( - Question.TYPE_CHOICE, - {}, - None, - [], - "option-slug", - None, - "SaveDocumentStringAnswer", - True, - ), - ( - Question.TYPE_CHOICE, - {}, - None, - [], - "invalid-option-slug", - None, - "SaveDocumentStringAnswer", - False, - ), - ( - Question.TYPE_DYNAMIC_MULTIPLE_CHOICE, - {}, - "MyDataSource", - [], - ["5.5", "1"], - None, - "SaveDocumentListAnswer", - True, - ), - ( - Question.TYPE_DYNAMIC_MULTIPLE_CHOICE, - {}, - "MyDataSource", - [], - ["not in data"], - None, - "SaveDocumentStringAnswer", - False, - ), - ( - Question.TYPE_DYNAMIC_CHOICE, - {}, - "MyDataSource", - [], - "5.5", - None, - "SaveDocumentStringAnswer", - True, - ), - ( - Question.TYPE_DYNAMIC_CHOICE, - {}, - "MyDataSource", - [], - "not in data", - None, - "SaveDocumentStringAnswer", - False, - ), - ( - Question.TYPE_TEXT, - {}, - None, - ["email"], - "some text", - None, - "SaveDocumentStringAnswer", - False, - ), - ( - Question.TYPE_TEXT, - {}, - None, - ["email"], - "test@example.com", - None, - "SaveDocumentStringAnswer", - True, - ), - ( - Question.TYPE_TEXTAREA, - {}, - None, - ["email"], - "some text", - None, - "SaveDocumentStringAnswer", - False, - ), - ( - Question.TYPE_TEXTAREA, - {}, - None, - ["email"], - "test@example.com", - None, - "SaveDocumentStringAnswer", - True, - ), + (Question.TYPE_FLOAT, {"min_value": 100.0}, None, [], 1, None, "SaveDocumentFloatAnswer", False), + (Question.TYPE_TEXT, {}, None, [], "Test", None, "SaveDocumentStringAnswer", True), + (Question.TYPE_TEXT, {"max_length": 1}, None, [], "toolong", None, "SaveDocumentStringAnswer", False), + (Question.TYPE_DATE, {}, None, [], None, "1900-01-01", "SaveDocumentDateAnswer", False), + (Question.TYPE_DATE, {}, None, [], None, "2019-02-22", "SaveDocumentDateAnswer", True), + (Question.TYPE_FILES, {}, None, [], None, None, "SaveDocumentFilesAnswer", False), + (Question.TYPE_FILES, {}, None, [], [{"name": "some-file.pdf"}], None, "SaveDocumentFilesAnswer", True), + (Question.TYPE_FILES, {}, None, [], [{"name": "not-exist.pdf"}], None, "SaveDocumentFilesAnswer", True), + (Question.TYPE_TEXT, {"min_length": 10}, None, [], "tooshort", None, "SaveDocumentStringAnswer", False), + (Question.TYPE_TABLE, {}, None, [], None, None, "SaveDocumentTableAnswer", True), + (Question.TYPE_TEXTAREA, {}, None, [], "Test", None, "SaveDocumentStringAnswer", True), + (Question.TYPE_TEXTAREA, {"max_length": 1}, None, [], "toolong", None, "SaveDocumentStringAnswer", False), + (Question.TYPE_MULTIPLE_CHOICE, {}, None, [], ["option-slug"], None, "SaveDocumentListAnswer", True), + (Question.TYPE_MULTIPLE_CHOICE, {}, None, [], ["option-slug", "option-invalid-slug"], None, "SaveDocumentStringAnswer", False), + (Question.TYPE_CHOICE, {}, None, [], "option-slug", None, "SaveDocumentStringAnswer", True), + (Question.TYPE_CHOICE, {}, None, [], "invalid-option-slug", None, "SaveDocumentStringAnswer", False), + (Question.TYPE_DYNAMIC_MULTIPLE_CHOICE, {}, "MyDataSource", [], ["5.5", "1"], None, "SaveDocumentListAnswer", True), + (Question.TYPE_DYNAMIC_MULTIPLE_CHOICE, {}, "MyDataSource", [], ["not in data"], None, "SaveDocumentStringAnswer", False), + (Question.TYPE_DYNAMIC_CHOICE, {}, "MyDataSource", [], "5.5", None, "SaveDocumentStringAnswer", True), + (Question.TYPE_DYNAMIC_CHOICE, {}, "MyDataSource", [], "not in data", None, "SaveDocumentStringAnswer", False), + (Question.TYPE_TEXT, {}, None, ["email"], "some text", None, "SaveDocumentStringAnswer", False), + (Question.TYPE_TEXT, {}, None, ["email"], "test@example.com", None, "SaveDocumentStringAnswer", True), + (Question.TYPE_TEXTAREA, {}, None, ["email"], "some text", None, "SaveDocumentStringAnswer", False), + (Question.TYPE_TEXTAREA, {}, None, ["email"], "test@example.com", None, "SaveDocumentStringAnswer", True), ], -) +) # fmt:skip def test_save_document_answer( # noqa:C901 db, snapshot, diff --git a/caluma/caluma_form/tests/test_question.py b/caluma/caluma_form/tests/test_question.py index 7f8a1a974..7595688ad 100644 --- a/caluma/caluma_form/tests/test_question.py +++ b/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,19 +1134,24 @@ 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"] @@ -1185,7 +1159,11 @@ def test_calculated_question_answer_document( # 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 diff --git a/caluma/caluma_form/utils.py b/caluma/caluma_form/utils.py index 3a248c3bb..8204ccbe6 100644 --- a/caluma/caluma_form/utils.py +++ b/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]