Separate questions and questionnaires

[Kakadus]

close #457

featuring...
☑️ ~11 year old issue
☑️ >24k changed lines (nearly all in test data / tests though)
☑️ touch every file in the git tree
☑️ some non-trivial migrations

I await your comments or questions of questions with questions :)

[niklasmohrin]

Oh wow, this is quite the undertaking. I did not look too close at stuff yet, but here are some initial thoughts that I want to clear out first.

There are a number of changes in this PR that we could make independently (typing, refactors, added assertions), can you PR them in separately so that we clear some of the diff here? :D

[niklasmohrin]

didn't get far again, but here are some more thoughts :D

[richardebeling]

surprisingly little changes to production logic. Nice! I will have to look at migrations, tests, and test_data separately, but let's first agree on the actual implementation

[niklasmohrin]

crossed off some more files :) I agree with Richard, maybe we can actually get this PR through quicker than I thought and before #2450. Let's see how we progress on the frontend, I think the backend is pretty solid 👍

[richardebeling]

Almost time to break production! 😅

[richardebeling]

Thank you so much :)

[richardebeling]

Oh, by the way, since this is large scale with high probability of fatigue leading to us missing subtle errors, I asked Copilot for a review on richardebeling#1 (review). I closed a few bogus review points, I'll have to go through the remaining ones, I'll try to get that done tomorrow

[Kakadus]

from ai review:

The migration has a potential issue with question deduplication. It uses a tuple of (text_de, text_en, allows_additional_textanswers, type) as a key to deduplicate questions. However, this approach may incorrectly merge questions that happen to have the same text but are actually different questions in different contexts. For instance, two different questionnaires might have a question "How satisfied are you?" that should remain separate. This could lead to unintended data merging during migration. Consider whether this deduplication is intended or if each question should be preserved separately.

Question deduplication is not bad per se, and the "copy on write" logic mimics current behavior.
What I haven't thought of specifically is questionnaires that contain the same question multiple times. We prohibited this specifically with unique_together = [("question", "questionnaire")], so the check fails then. This may happen when migrating production.
@janno42, can you check this by running

from django.db.models import CharField, Count, F, Value
from django.db.models.functions import Concat

questionnaires_with_duplicates = Questionnaire.objects.annotate(
    total_questions=Count('questions'),
    unique_combinations=Count(
        Concat(
            'questions__text_de',
            Value('|==||==|'),
            'questions__text_en',
            Value('|==||==|'),
            'questions__allows_additional_textanswers',
            Value('|==||==|'),
            'questions__type',
            output_field=CharField()
        ),
        distinct=True
    )
).filter(total_questions__gt=F('unique_combinations'))
print(len(questionnaires_with_duplicates), "questionnaires ask the same question multiple times.")

in production?

[richardebeling]

I cannot imagine a situation where we intentionally included the same question twice in a questionnaire, but it probably doesn't hurt to double check

[richardebeling]

These were the things Copilot review found

[niklasmohrin]

@Kakadus The current state is compatible with main and there are no user facing changes right? We are considering to use this chance right now and merge, is there any reason against that in your view?

[niklasmohrin]

Thanks, great changes ahead!

[richardebeling]

Thanks again @Kakadus!

[niklasmohrin]

first flaky test? https://github.com/e-valuation/EvaP/actions/runs/22596857685/job/65468976117?pr=2587

[richardebeling]

We have seen one flaky failure if a question was of type HeadingQuestion, this was fixed in #2665.

There was another flaky failure in https://github.com/e-valuation/EvaP/actions/runs/22596857685/job/65468976117?pr=2587, the test failed at

self.assertQuerySetEqual(self.question.questionnaires.all(), [])

with

======================================================================
FAIL: test_delete_question (evap.staff.tests.test_views.TestQuestionnaireEditView.test_delete_question)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/EvaP/EvaP/evap/staff/tests/test_views.py", line 3405, in test_delete_question
    self.assertQuerySetEqual(self.question.questionnaires.all(), [])
    ~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: Lists differ: [<Questionnaire: nspLpceuXDUWCqsJcEsx>] != []

I cannot reproduce this at all, and I don't have a convincing explanation on what happened. Based on the error message, I'd say this must be a different problem. A heading question should have ran into the assertion consistently. I could see database transaction ordering or django related-model caching to be a problem here, but:

• If django related-model caching was a problem, we should be able to reproduce this somewhat consistently
• Database transaction ordering should be nice enough to us. Iirc we have a guarantee that effects of previously completed transactions will be visible when we look at the database again. I could see that QuestionAssignment::delete not being wrapped in a transaction could cause trouble, but only if question deletion fails. If both queries succeed, I would expect that our code here is sequenced strictly after both delete queries (with automatic single-query transactions) have succeeded, so we should never observe the assignment existing here.

We have changed some stuff around this test and the delete logic now. We may want to put delete in @transaction.atomic, but I don't see how it would have fixed this test failure.

[niklasmohrin]

Maybe it is related to this?

Overridden model methods are not called on bulk operations

Note that the delete() method for an object is not necessarily called when deleting objects in bulk using a QuerySet or as a result of a cascading delete. To ensure customized delete logic gets executed, you can use pre_delete and/or post_delete signals.

From https://docs.djangoproject.com/en/6.0/topics/db/models/#overriding-model-methods

Could it be that we somehow run into this being a cascading delete in some case? I don't see Django using a queryset delete in one case and a normal one in the other, but calling .delete on models in a non-deterministic order I could see

[richardebeling]

@karyon has pointed out that our QuestionAssignment::delete method will first delete related objects via self.question.delete, which will in turn trigger a cascaded delete on the instance that delete was just called on -- but I would still (naively) expect that this behaves somewhat consistently if this was an issue