From 5913fe5e461e616ce882737671683df4c0b02631 Mon Sep 17 00:00:00 2001 From: Fabio Ambauen Date: Fri, 17 Feb 2023 15:20:22 +0100 Subject: [PATCH] fix: replace faulty migration for calc answers with a command The migration `0047_recalculate_calc_answers` was error prone and took a long time to execute. This commit replaces it with a management command that is more optimized and stable. Still this operation will take some time to finish. --- .../commands/recalculate_calc_answers.py | 35 ++++++++ .../0047_recalculate_calc_answers.py | 54 ------------- caluma/caluma_form/tests/test_migration.py | 81 ------------------- .../test_recalculate_calc_answers_command.py | 74 +++++++++++++++++ 4 files changed, 109 insertions(+), 135 deletions(-) create mode 100644 caluma/caluma_form/management/commands/recalculate_calc_answers.py delete mode 100644 caluma/caluma_form/migrations/0047_recalculate_calc_answers.py create mode 100644 caluma/caluma_form/tests/test_recalculate_calc_answers_command.py diff --git a/caluma/caluma_form/management/commands/recalculate_calc_answers.py b/caluma/caluma_form/management/commands/recalculate_calc_answers.py new file mode 100644 index 000000000..a967d83f5 --- /dev/null +++ b/caluma/caluma_form/management/commands/recalculate_calc_answers.py @@ -0,0 +1,35 @@ +from django.core.management.base import BaseCommand + +from caluma.caluma_form.models import Answer, Question +from caluma.caluma_form.signals import _update_or_create_calc_answer + + +class Command(BaseCommand): + """ + Recalculate calculated answers containing values from TableAnswers. + + Due to a bug, answers to calculated questions were wrong, if they contained values + from table rows. This command recalculates all of them. + """ + + help = "Recalculate calculated answers containing values from TableAnswers." + + def handle(self, *args, **options): + affected_questions = ( + Question.objects.filter(type="table") + .exclude(calc_dependents=[]) + .values_list("calc_dependents", flat=True) + ) + + affected_questions_clean = set( + [slug for entry in affected_questions for slug in entry] + ) + + calc_answers = Answer.objects.filter( + question__type="calculated_float", + document__isnull=False, + question__slug__in=affected_questions_clean, + ) + + for answer in calc_answers.iterator(): + _update_or_create_calc_answer(answer.question, answer.document) diff --git a/caluma/caluma_form/migrations/0047_recalculate_calc_answers.py b/caluma/caluma_form/migrations/0047_recalculate_calc_answers.py deleted file mode 100644 index bdd24c9e9..000000000 --- a/caluma/caluma_form/migrations/0047_recalculate_calc_answers.py +++ /dev/null @@ -1,54 +0,0 @@ -from django.db import migrations - -from caluma.caluma_form.jexl import QuestionJexl -from caluma.caluma_form.structure import FieldSet - - -def recalculate_answer(answer): - """ - Recalculate a single calc-answer. - - This is more or less a copy/paste from `_update_or_create_calc_answer` in order to - use the old models. - """ - root_doc = answer.document.family - - struc = FieldSet(root_doc, root_doc.form) - field = struc.get_field(answer.question.slug) - - # skip if question doesn't exist in this document structure - if field is None: # pragma: no cover - return - - jexl = QuestionJexl( - {"form": field.form, "document": field.document, "structure": field.parent()} - ) - - # Ignore errors because we might evaluate partially built forms. So we might be - # missing some answers or the expression might be invalid, in which case we return - # None - value = jexl.evaluate(field.question.calc_expression, raise_on_error=False) - - answer.value = value - answer.save() - - -def recalculate_all_answers(apps, _): - AnswerModel = apps.get_model("caluma_form.Answer") - - calc_answers = AnswerModel.objects.filter( - question__type="calculated_float", document__isnull=False - ) - for answer in calc_answers.iterator(): - recalculate_answer(answer) - - -class Migration(migrations.Migration): - - dependencies = [ - ("caluma_form", "0046_file_answer_reverse_keys"), - ] - - operations = [ - migrations.RunPython(recalculate_all_answers, migrations.RunPython.noop), - ] diff --git a/caluma/caluma_form/tests/test_migration.py b/caluma/caluma_form/tests/test_migration.py index b48eff5d9..1fdbc6670 100644 --- a/caluma/caluma_form/tests/test_migration.py +++ b/caluma/caluma_form/tests/test_migration.py @@ -8,8 +8,6 @@ from django.db.utils import DataError from django.utils import timezone -from caluma.caluma_form.api import save_answer, save_document -from caluma.caluma_form.models import Question from caluma.utils import col_type_from_db @@ -431,82 +429,3 @@ def test_migrate_answer_history_question_type(post_migrate_to_current_state): new_hist_ans = MigratedHistAns.objects.get(history_id=new_hist_ans.history_id) assert new_hist_ans.history_question_type == new_hist_quest.type - - -def test_migrate_recalculate_calc_answers( - post_migrate_to_current_state, - form_factory, - question_factory, - form_question_factory, -): - # set up form structure - main_form_question = form_question_factory( - question__type=Question.TYPE_INTEGER, question__slug="dep1_main" - ) - main_form = main_form_question.form - dep1_main = main_form_question.question - - row_form = form_factory(slug="row_form") - table_question = question_factory( - type="table", - slug="table_question", - row_form=row_form, - is_required="true", - is_hidden="false", - ) - form_question_factory(form=main_form, question=table_question) - - row_form_question = form_question_factory( - form=row_form, - question__type=Question.TYPE_INTEGER, - question__slug="dep2_row", - question__is_required=True, - ) - dep2_row = row_form_question.question - - form_question_factory( - form=main_form, - question__slug="calc_question", - question__type=Question.TYPE_CALCULATED_FLOAT, - question__calc_expression=( - f'"dep1_main"|answer(0) + "{table_question.slug}"|answer([])|mapby("dep2_row")|sum' - ), - ) - - # assert calc_dependents - dep1_main.refresh_from_db() - dep2_row.refresh_from_db() - assert dep1_main.calc_dependents == ["calc_question"] - assert dep2_row.calc_dependents == ["calc_question"] - - # set up document and answers - main_doc = save_document(form=main_form) - save_answer(question=dep1_main, value=10, document=main_doc) - row_doc = save_document(form=row_form) - save_answer(question=dep2_row, value=13, document=row_doc) - save_answer(table_question, document=main_doc, value=[str(row_doc.pk)]) - calc_answer = main_doc.answers.get(question_id="calc_question") - - # assert calc bug is fixed - assert calc_answer.value == 23 - - # set calc_answer.value to 10, like it would have been without the fix - calc_answer.value = 10 - calc_answer.save() - - # migrate back and forth in order to run the data-migration - executor = MigrationExecutor(connection) - app = "caluma_form" - migrate_from = [(app, "0046_file_answer_reverse_keys")] - migrate_to = [(app, "0047_recalculate_calc_answers")] - executor.migrate(migrate_from) - executor.loader.build_graph() - executor.migrate(migrate_to) - - new_apps = executor.loader.project_state(migrate_to).apps - - MigratedAnswer = new_apps.get_model(app, "Answer") - - # assert correct calc_value after migration - calc_answer = MigratedAnswer.objects.get(question_id="calc_question") - assert calc_answer.value == 23 diff --git a/caluma/caluma_form/tests/test_recalculate_calc_answers_command.py b/caluma/caluma_form/tests/test_recalculate_calc_answers_command.py new file mode 100644 index 000000000..6b0a3e1ae --- /dev/null +++ b/caluma/caluma_form/tests/test_recalculate_calc_answers_command.py @@ -0,0 +1,74 @@ +import os + +from django.core.management import call_command + +from caluma.caluma_form.api import save_answer, save_document +from caluma.caluma_form.models import Question + + +def test_recalculate_calc_answers( + db, + form_factory, + question_factory, + form_question_factory, +): + # set up form structure + main_form_question = form_question_factory( + question__type=Question.TYPE_INTEGER, question__slug="dep1_main" + ) + main_form = main_form_question.form + dep1_main = main_form_question.question + + row_form = form_factory(slug="row_form") + table_question = question_factory( + type="table", + slug="table_question", + row_form=row_form, + is_required="true", + is_hidden="false", + ) + form_question_factory(form=main_form, question=table_question) + + row_form_question = form_question_factory( + form=row_form, + question__type=Question.TYPE_INTEGER, + question__slug="dep2_row", + question__is_required=True, + ) + dep2_row = row_form_question.question + + form_question_factory( + form=main_form, + question__slug="calc_question", + question__type=Question.TYPE_CALCULATED_FLOAT, + question__calc_expression=( + f'"dep1_main"|answer(0) + "{table_question.slug}"|answer([])|mapby("dep2_row")|sum' + ), + ) + + # assert calc_dependents + dep1_main.refresh_from_db() + dep2_row.refresh_from_db() + assert dep1_main.calc_dependents == ["calc_question"] + assert dep2_row.calc_dependents == ["calc_question"] + + # set up document and answers + main_doc = save_document(form=main_form) + save_answer(question=dep1_main, value=10, document=main_doc) + row_doc = save_document(form=row_form) + save_answer(question=dep2_row, value=13, document=row_doc) + save_answer(table_question, document=main_doc, value=[str(row_doc.pk)]) + calc_answer = main_doc.answers.get(question_id="calc_question") + + # assert calc bug is fixed + assert calc_answer.value == 23 + + # set calc_answer.value to 10, like it would have been without the fix + calc_answer.value = 10 + calc_answer.save() + + call_command("recalculate_calc_answers", stderr=open(os.devnull, "w")) + + # assert correct calc_value after migration + calc_answer.refresh_from_db() + assert calc_answer.value == 23