Use database constraints instead of python asserts (#1957)

* Implemented DB constraints

* Fixed check_vote_date for single day votes

* Refactored Question to QuestionTypes

* Fixed check_count_null

* Fixed check_additional_textanswers in tests

* Fixed rebase issues

* Fixed code style

* Simplified constraint-migrations

* Fixed migration history

* Renamed QuestionTypes to QuestionType

* Renamed constraints

* Fixed check_evaluation_text_answer_is_modified

* Reformat code

* Implemented PR suggestions

* rename migration after rebase

---------

Co-authored-by: Niklas Mohrin <[email protected]>

evap/evaluation/migrations/0137_use_more_database_constraints.py

@@ -0,0 +1,55 @@
+# Generated by Django 4.2.2 on 2023-07-03 22:18
+
+from django.db import migrations, models
+import django.db.models.functions.datetime
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("evaluation", "0136_alter_userprofile_first_name_chosen_and_more"),
+    ]
+
+    operations = [
+        migrations.AddConstraint(
+            model_name="evaluation",
+            constraint=models.CheckConstraint(
+                check=models.Q(
+                    (
+                        "vote_end_date__gte",
+                        django.db.models.functions.datetime.TruncDate(models.F("vote_start_datetime")),
+                    )
+                ),
+                name="check_evaluation_start_before_end",
+            ),
+        ),
+        migrations.AddConstraint(
+            model_name="evaluation",
+            constraint=models.CheckConstraint(
+                check=models.Q(
+                    ("_participant_count__isnull", True),
+                    ("_voter_count__isnull", True),
+                    _connector="XOR",
+                    _negated=True,
+                ),
+                name="check_evaluation_participant_count_and_voter_count_both_set_or_not_set",
+            ),
+        ),
+        migrations.AddConstraint(
+            model_name="question",
+            constraint=models.CheckConstraint(
+                check=models.Q(
+                    models.Q(("type", 0), ("type", 5), _connector="OR", _negated=True),
+                    models.Q(("allows_additional_textanswers", True), _negated=True),
+                    _connector="OR",
+                ),
+                name="check_evaluation_textanswer_or_heading_question_has_no_additional_textanswers",
+            ),
+        ),
+        migrations.AddConstraint(
+            model_name="textanswer",
+            constraint=models.CheckConstraint(
+                check=models.Q(("answer", models.F("original_answer")), _negated=True),
+                name="check_evaluation_text_answer_is_modified",
+            ),
+        ),
+    ]

evap/evaluation/models.py

@@ -16,8 +16,8 @@
 from django.core.exceptions import ValidationError
 from django.core.mail import EmailMultiAlternatives
 from django.db import IntegrityError, models, transaction
-from django.db.models import CheckConstraint, Count, Manager, OuterRef, Q, Subquery, Value
-from django.db.models.functions import Coalesce, Lower, NullIf
+from django.db.models import CheckConstraint, Count, F, Manager, OuterRef, Q, Subquery, Value
+from django.db.models.functions import Coalesce, Lower, NullIf, TruncDate
 from django.dispatch import Signal, receiver
 from django.template import Context, Template
 from django.template.defaultfilters import linebreaksbr
@@ -457,6 +457,16 @@ class Meta:
         ]
         verbose_name = _("evaluation")
         verbose_name_plural = _("evaluations")
+        constraints = [
+            CheckConstraint(
+                check=Q(vote_end_date__gte=TruncDate(F("vote_start_datetime"))),
+                name="check_evaluation_start_before_end",
+            ),
+            CheckConstraint(
+                check=~(Q(_participant_count__isnull=True) ^ Q(_voter_count__isnull=True)),
+                name="check_evaluation_participant_count_and_voter_count_both_set_or_not_set",
+            ),
+        ]
 
     def __str__(self):
         return self.full_name
@@ -469,8 +479,6 @@ def save(self, *args, **kw):
             self.contributions.create(contributor=None)
             del self.general_contribution  # invalidate cached property
 
-        assert self.vote_end_date >= self.vote_start_datetime.date()
-
         if hasattr(self, "state_change_source"):
 
             def state_changed_to(self, state_set):
@@ -1077,9 +1085,7 @@ def remove_answers_to_questionnaires(self, questionnaires):
         RatingAnswerCounter.objects.filter(contribution=self, question__questionnaire__in=questionnaires).delete()
 
 
-class Question(models.Model):
-    """A question including a type."""
-
+class QuestionType:
     TEXT = 0
     LIKERT = 1
     GRADE = 2
@@ -1092,29 +1098,34 @@ class Question(models.Model):
     POSITIVE_YES_NO = 3
     NEGATIVE_YES_NO = 4
     HEADING = 5
+
+
+class Question(models.Model):
+    """A question including a type."""
+
     QUESTION_TYPES = (
-        (_("Text"), ((TEXT, _("Text question")),)),
-        (_("Unipolar Likert"), ((LIKERT, _("Agreement question")),)),
-        (_("Grade"), ((GRADE, _("Grade question")),)),
+        (_("Text"), ((QuestionType.TEXT, _("Text question")),)),
+        (_("Unipolar Likert"), ((QuestionType.LIKERT, _("Agreement question")),)),
+        (_("Grade"), ((QuestionType.GRADE, _("Grade question")),)),
         (
             _("Bipolar Likert"),
             (
-                (EASY_DIFFICULT, _("Easy-difficult question")),
-                (FEW_MANY, _("Few-many question")),
-                (LITTLE_MUCH, _("Little-much question")),
-                (SMALL_LARGE, _("Small-large question")),
-                (SLOW_FAST, _("Slow-fast question")),
-                (SHORT_LONG, _("Short-long question")),
+                (QuestionType.EASY_DIFFICULT, _("Easy-difficult question")),
+                (QuestionType.FEW_MANY, _("Few-many question")),
+                (QuestionType.LITTLE_MUCH, _("Little-much question")),
+                (QuestionType.SMALL_LARGE, _("Small-large question")),
+                (QuestionType.SLOW_FAST, _("Slow-fast question")),
+                (QuestionType.SHORT_LONG, _("Short-long question")),
             ),
         ),
         (
             _("Yes-no"),
             (
-                (POSITIVE_YES_NO, _("Positive yes-no question")),
-                (NEGATIVE_YES_NO, _("Negative yes-no question")),
+                (QuestionType.POSITIVE_YES_NO, _("Positive yes-no question")),
+                (QuestionType.NEGATIVE_YES_NO, _("Negative yes-no question")),
             ),
         ),
-        (_("Layout"), ((HEADING, _("Heading")),)),
+        (_("Layout"), ((QuestionType.HEADING, _("Heading")),)),
     )
 
     order = models.IntegerField(verbose_name=_("question order"), default=-1)
@@ -1130,9 +1141,16 @@ class Meta:
         ordering = ["order"]
         verbose_name = _("question")
         verbose_name_plural = _("questions")
+        constraints = [
+            CheckConstraint(
+                check=~(Q(type=QuestionType.TEXT) | Q(type=QuestionType.HEADING))
+                | ~Q(allows_additional_textanswers=True),
+                name="check_evaluation_textanswer_or_heading_question_has_no_additional_textanswers",
+            )
+        ]
 
     def save(self, *args, **kwargs):
-        if self.type in [Question.TEXT, Question.HEADING]:
+        if self.type in [QuestionType.TEXT, QuestionType.HEADING]:
             self.allows_additional_textanswers = False
             if "update_fields" in kwargs:
                 kwargs["update_fields"] = {"allows_additional_textanswers"}.union(kwargs["update_fields"])
@@ -1150,34 +1168,34 @@ def answer_class(self):
 
     @property
     def is_likert_question(self):
-        return self.type == self.LIKERT
+        return self.type == QuestionType.LIKERT
 
     @property
     def is_bipolar_likert_question(self):
         return self.type in (
-            self.EASY_DIFFICULT,
-            self.FEW_MANY,
-            self.LITTLE_MUCH,
-            self.SLOW_FAST,
-            self.SMALL_LARGE,
-            self.SHORT_LONG,
+            QuestionType.EASY_DIFFICULT,
+            QuestionType.FEW_MANY,
+            QuestionType.LITTLE_MUCH,
+            QuestionType.SLOW_FAST,
+            QuestionType.SMALL_LARGE,
+            QuestionType.SHORT_LONG,
         )
 
     @property
     def is_text_question(self):
-        return self.type == self.TEXT
+        return self.type == QuestionType.TEXT
 
     @property
     def is_grade_question(self):
-        return self.type == self.GRADE
+        return self.type == QuestionType.GRADE
 
     @property
     def is_positive_yes_no_question(self):
-        return self.type == self.POSITIVE_YES_NO
+        return self.type == QuestionType.POSITIVE_YES_NO
 
     @property
     def is_negative_yes_no_question(self):
-        return self.type == self.NEGATIVE_YES_NO
+        return self.type == QuestionType.NEGATIVE_YES_NO
 
     @property
     def is_yes_no_question(self):
@@ -1198,7 +1216,7 @@ def is_non_grade_rating_question(self):
 
     @property
     def is_heading_question(self):
-        return self.type == self.HEADING
+        return self.type == QuestionType.HEADING
 
     @property
     def can_have_textanswers(self):
@@ -1252,7 +1270,7 @@ def can_have_textanswers(self):
 }
 
 CHOICES: Dict[int, Union[Choices, BipolarChoices]] = {
-    Question.LIKERT: Choices(
+    QuestionType.LIKERT: Choices(
         names=[
             _("Strongly\nagree"),
             _("Agree"),
@@ -1263,7 +1281,7 @@ def can_have_textanswers(self):
         ],
         **BASE_UNIPOLAR_CHOICES,  # type: ignore
     ),
-    Question.GRADE: Choices(
+    QuestionType.GRADE: Choices(
         names=[
             "1",
             "2",
@@ -1274,7 +1292,7 @@ def can_have_textanswers(self):
         ],
         **BASE_UNIPOLAR_CHOICES,  # type: ignore
     ),
-    Question.EASY_DIFFICULT: BipolarChoices(
+    QuestionType.EASY_DIFFICULT: BipolarChoices(
         minus_name=_("Easy"),
         plus_name=_("Difficult"),
         names=[
@@ -1289,7 +1307,7 @@ def can_have_textanswers(self):
         ],
         **BASE_BIPOLAR_CHOICES,  # type: ignore
     ),
-    Question.FEW_MANY: BipolarChoices(
+    QuestionType.FEW_MANY: BipolarChoices(
         minus_name=_("Few"),
         plus_name=_("Many"),
         names=[
@@ -1304,7 +1322,7 @@ def can_have_textanswers(self):
         ],
         **BASE_BIPOLAR_CHOICES,  # type: ignore
     ),
-    Question.LITTLE_MUCH: BipolarChoices(
+    QuestionType.LITTLE_MUCH: BipolarChoices(
         minus_name=_("Little"),
         plus_name=_("Much"),
         names=[
@@ -1319,7 +1337,7 @@ def can_have_textanswers(self):
         ],
         **BASE_BIPOLAR_CHOICES,  # type: ignore
     ),
-    Question.SMALL_LARGE: BipolarChoices(
+    QuestionType.SMALL_LARGE: BipolarChoices(
         minus_name=_("Small"),
         plus_name=_("Large"),
         names=[
@@ -1334,7 +1352,7 @@ def can_have_textanswers(self):
         ],
         **BASE_BIPOLAR_CHOICES,  # type: ignore
     ),
-    Question.SLOW_FAST: BipolarChoices(
+    QuestionType.SLOW_FAST: BipolarChoices(
         minus_name=_("Slow"),
         plus_name=_("Fast"),
         names=[
@@ -1349,7 +1367,7 @@ def can_have_textanswers(self):
         ],
         **BASE_BIPOLAR_CHOICES,  # type: ignore
     ),
-    Question.SHORT_LONG: BipolarChoices(
+    QuestionType.SHORT_LONG: BipolarChoices(
         minus_name=_("Short"),
         plus_name=_("Long"),
         names=[
@@ -1364,15 +1382,15 @@ def can_have_textanswers(self):
         ],
         **BASE_BIPOLAR_CHOICES,  # type: ignore
     ),
-    Question.POSITIVE_YES_NO: Choices(
+    QuestionType.POSITIVE_YES_NO: Choices(
         names=[
             _("Yes"),
             _("No"),
             _("No answer"),
         ],
         **BASE_YES_NO_CHOICES,  # type: ignore
     ),
-    Question.NEGATIVE_YES_NO: Choices(
+    QuestionType.NEGATIVE_YES_NO: Choices(
         names=[
             _("No"),
             _("Yes"),
@@ -1454,6 +1472,9 @@ class Meta:
         ordering = ["id"]
         verbose_name = _("text answer")
         verbose_name_plural = _("text answers")
+        constraints = [
+            CheckConstraint(check=~Q(answer=F("original_answer")), name="check_evaluation_text_answer_is_modified")
+        ]
 
     @property
     def will_be_deleted(self):
@@ -1483,7 +1504,6 @@ def is_reviewed(self):
 
     def save(self, *args, **kwargs):
         super().save(*args, **kwargs)
-        assert self.answer != self.original_answer
 
 
 class FaqSection(models.Model):

evap/evaluation/tests/test_models.py

@@ -18,6 +18,7 @@
     NotArchiveable,
     Question,
     Questionnaire,
+    QuestionType,
     RatingAnswerCounter,
     Semester,
     TextAnswer,
@@ -296,7 +297,7 @@ def test_second_vote_sets_can_publish_text_results_to_true(self):
         )
         evaluation.save()
         top_general_questionnaire = baker.make(Questionnaire, type=Questionnaire.Type.TOP)
-        baker.make(Question, questionnaire=top_general_questionnaire, type=Question.LIKERT)
+        baker.make(Question, questionnaire=top_general_questionnaire, type=QuestionType.LIKERT)
         evaluation.general_contribution.questionnaires.set([top_general_questionnaire])
 
         self.assertFalse(evaluation.can_publish_text_results)
@@ -316,7 +317,7 @@ def test_textanswers_get_deleted_if_they_cannot_be_published(self):
             can_publish_text_results=False,
         )
         questionnaire = baker.make(Questionnaire, type=Questionnaire.Type.TOP)
-        question = baker.make(Question, type=Question.TEXT, questionnaire=questionnaire)
+        question = baker.make(Question, type=QuestionType.TEXT, questionnaire=questionnaire)
         evaluation.general_contribution.questionnaires.set([questionnaire])
         baker.make(TextAnswer, question=question, contribution=evaluation.general_contribution)
 
@@ -335,7 +336,7 @@ def test_textanswers_do_not_get_deleted_if_they_can_be_published(self):
             can_publish_text_results=True,
         )
         questionnaire = baker.make(Questionnaire, type=Questionnaire.Type.TOP)
-        question = baker.make(Question, type=Question.TEXT, questionnaire=questionnaire)
+        question = baker.make(Question, type=QuestionType.TEXT, questionnaire=questionnaire)
         evaluation.general_contribution.questionnaires.set([questionnaire])
         baker.make(TextAnswer, question=question, contribution=evaluation.general_contribution)
 
@@ -354,7 +355,7 @@ def test_textanswers_to_delete_get_deleted_on_publish(self):
             can_publish_text_results=True,
         )
         questionnaire = baker.make(Questionnaire, type=Questionnaire.Type.TOP)
-        question = baker.make(Question, type=Question.TEXT, questionnaire=questionnaire)
+        question = baker.make(Question, type=QuestionType.TEXT, questionnaire=questionnaire)
         evaluation.general_contribution.questionnaires.set([questionnaire])
         baker.make(
             TextAnswer,
@@ -388,7 +389,7 @@ def test_original_textanswers_get_deleted_on_publish(self):
             can_publish_text_results=True,
         )
         questionnaire = baker.make(Questionnaire, type=Questionnaire.Type.TOP)
-        question = baker.make(Question, type=Question.TEXT, questionnaire=questionnaire)
+        question = baker.make(Question, type=QuestionType.TEXT, questionnaire=questionnaire)
         evaluation.general_contribution.questionnaires.set([questionnaire])
         baker.make(
             TextAnswer,