Grant reward points on evaluation deletion

evap/evaluation/tests/test_tools.py

@@ -3,16 +3,18 @@
 
 from django.core import management
 from django.core.exceptions import SuspiciousOperation
+from django.db import transaction
 from django.db.models import Model, prefetch_related_objects
 from django.http import Http404
 from django.utils import translation
 from model_bakery import baker
 
 from evap.evaluation.models import Contribution, Course, Evaluation, TextAnswer, UserProfile
-from evap.evaluation.tests.tools import TestCase, WebTest
+from evap.evaluation.tests.tools import SimpleTestCase, TestCase, WebTest
 from evap.evaluation.tools import (
     discard_cached_related_objects,
     get_object_from_dict_pk_entry_or_logged_40x,
+    inside_transaction,
     is_prefetched,
 )
 
@@ -58,7 +60,7 @@ def test_log_exceptions_decorator(self, mock_logger, __):
         self.assertIn("failed. Traceback follows:", mock_logger.call_args[0][0])
 
 
-class TestHelperMethods(WebTest):
+class TestHelperMethods(TestCase):
     def test_is_prefetched(self):
         evaluation = baker.make(Evaluation, voters=[baker.make(UserProfile)])
         baker.make(Contribution, evaluation=evaluation)
@@ -196,3 +198,14 @@ def test_get_object_from_dict_pk_entry_or_logged_40x_for_uuids(self):
 
         answer = baker.make(TextAnswer)
         self.assertEqual(get_object_from_dict_pk_entry_or_logged_40x(TextAnswer, {"pk": str(answer.pk)}, "pk"), answer)
+
+
+class TestHelperMethodsWithoutTransaction(SimpleTestCase):
+    # WARNING: Do not execute any queries modifying the database. Changes will not be rolled back
+    databases = ["default"]
+
+    def test_inside_transaction(self):
+        self.assertEqual(inside_transaction(), False)
+
+        with transaction.atomic():
+            self.assertEqual(inside_transaction(), True)

evap/evaluation/tests/tools.py

@@ -39,6 +39,10 @@ class TestCase(ResetLanguageOnTearDownMixin, django.test.TestCase):
     pass
 
 
+class SimpleTestCase(ResetLanguageOnTearDownMixin, django.test.SimpleTestCase):
+    pass
+
+
 class WebTest(ResetLanguageOnTearDownMixin, django_webtest.WebTest):
     pass
 

evap/evaluation/tools.py

@@ -8,6 +8,7 @@
 import xlwt
 from django.conf import settings
 from django.core.exceptions import SuspiciousOperation, ValidationError
+from django.db import DEFAULT_DB_ALIAS, connections
 from django.db.models import Model
 from django.forms.formsets import BaseFormSet
 from django.http import HttpRequest, HttpResponse
@@ -87,6 +88,11 @@ def discard_cached_related_objects(instance: M) -> M:
     return instance
 
 
+def inside_transaction() -> bool:
+    # https://github.com/django/django/blob/402ae37873974afa5093e6d6149175a118979cd9/django/db/transaction.py#L208
+    return connections[DEFAULT_DB_ALIAS].in_atomic_block
+
+
 def is_external_email(email: str) -> bool:
     return not any(email.endswith("@" + domain) for domain in settings.INSTITUTION_EMAIL_DOMAINS)
 

evap/rewards/models.py

@@ -36,7 +36,8 @@ class RewardPointGranting(models.Model):
     granting_time = models.DateTimeField(verbose_name=_("granting time"), auto_now_add=True)
     value = models.IntegerField(verbose_name=_("value"), validators=[MinValueValidator(1)])
 
-    granted_by_removal = Signal()
+    granted_by_participation_removal = Signal()
+    granted_by_evaluation_deletion = Signal()
 
 
 class RewardPointRedemption(models.Model):

evap/rewards/tests/test_tools.py

@@ -97,10 +97,12 @@ def setUpTestData(cls):
 
     def test_participant_removed_from_evaluation(self):
         self.evaluation.participants.remove(self.student)
-
         self.assertEqual(reward_points_of_user(self.student), 3)
 
     def test_evaluation_removed_from_participant(self):
         self.student.evaluations_participating_in.remove(self.evaluation)
+        self.assertEqual(reward_points_of_user(self.student), 3)
 
+    def test_evaluation_deleted(self):
+        self.evaluation.delete()
         self.assertEqual(reward_points_of_user(self.student), 3)

evap/rewards/tools.py

@@ -7,6 +7,7 @@
 from django.utils.translation import ngettext
 
 from evap.evaluation.models import Evaluation, Semester, UserProfile
+from evap.evaluation.tools import inside_transaction
 from evap.rewards.models import RewardPointGranting, RewardPointRedemption, SemesterActivation
 
 
@@ -100,21 +101,21 @@
 
 
 @receiver(models.signals.m2m_changed, sender=Evaluation.participants.through)
-def grant_reward_points_after_delete(instance, action, reverse, pk_set, **_kwargs):
+def grant_reward_points_on_participation_change(instance, action, reverse, pk_set, **_kwargs):
     # if users do not need to evaluate anymore, they may have earned reward points
     if action == "post_remove":
         grantings = []
 
         if reverse:
-            # an evaluation got removed from a participant
+            # one or more evaluations got removed from a participant
             user = instance
 
             for semester in Semester.objects.filter(courses__evaluations__pk__in=pk_set):
                 granting, __ = grant_reward_points_if_eligible(user, semester)
                 if granting:
                     grantings = [granting]
         else:
-            # a participant got removed from an evaluation
+            # one or more participants got removed from an evaluation
             evaluation = instance
 
             for user in UserProfile.objects.filter(pk__in=pk_set):
@@ -123,4 +124,27 @@
                     grantings.append(granting)
 
         if grantings:
-            RewardPointGranting.granted_by_removal.send(sender=RewardPointGranting, grantings=grantings)
+            RewardPointGranting.granted_by_participation_removal.send(sender=RewardPointGranting, grantings=grantings)
+
+
+@receiver(models.signals.pre_delete, sender=Evaluation)
+def grant_reward_points_on_evaluation_delete(instance, **_kwargs):
+    if not inside_transaction():
+        # This will always be true in a django TestCase, so our tests can't meaningfully catch calls that are not
+        # wrapped in a transaction. Requiring a transaction is a precaution so that an (unlikely) failing .delete()
+        # execution afterwards doesn't leave us in half-deleted state. Chances are, if deletion fails, staff users
+        # will still want to delete the instance.
+        # Currently, only staff:evaluation_delete and staff:semester_delete call .delete()
+        logger.exception("Called while not inside transaction")
+
+    grantings = []
+
+    participants = list(instance.participants.all())
+    instance.participants.clear()
+    for user in participants:
+        granting, __ = grant_reward_points_if_eligible(user, instance.course.semester)
+        if granting:
+            grantings.append(granting)
+
+    if grantings:
+        RewardPointGranting.granted_by_evaluation_deletion.send(sender=RewardPointGranting, grantings=grantings)

evap/staff/tests/test_views.py

@@ -2386,6 +2386,25 @@ def test_invalid_deletion(self):
         self.app.post(self.url, user=self.manager, params=self.post_params, status=400)
         self.assertTrue(Evaluation.objects.filter(pk=self.evaluation.pk).exists())
 
+    @patch.object(Evaluation, "can_be_deleted_by_manager", True)
+    def test_reward_point_granting_message(self):
+        already_evaluated = baker.make(Evaluation, course__semester=self.evaluation.course.semester)
+        SemesterActivation.objects.create(semester=self.evaluation.course.semester, is_active=True)
+        student = baker.make(UserProfile, evaluations_participating_in=[self.evaluation])
+
+        baker.make(
+            UserProfile,
+            email=iter(f"{name}@institution.example.com" for name in ["a", "b", "c", "d", "e"]),
+            evaluations_participating_in=[self.evaluation, already_evaluated],
+            evaluations_voted_for=[already_evaluated],
+            _quantity=5,
+        )
+
+        with patch("evap.staff.views.logger") as mock:
+            page = self.app.post(self.url, user=self.manager, params=self.post_params, status=200)
+
+        mock.info.assert_called_with(f"Deletion of evaluation {self.evaluation} has created 5 reward point grantings")
+
 
 class TestSingleResultEditView(WebTestStaffModeWith200Check):
     @classmethod

evap/staff/views.py

@@ -1,5 +1,6 @@
 import csv
 import itertools
+import logging
 from collections import OrderedDict, defaultdict, namedtuple
 from collections.abc import Container
 from dataclasses import dataclass
@@ -135,6 +136,8 @@
 from evap.student.views import render_vote_page
 from evap.tools import unordered_groupby
 
+logger = logging.getLogger(__name__)
+
 
 @manager_required
 def index(request):
@@ -671,6 +674,8 @@ def semester_make_active(request):
 @require_POST
 @manager_required
 def semester_delete(request):
+    # TODO(rebeling): Do we expect reward point granting here? Do we want to notify? I don't see why it couldn't happen
+
     semester = get_object_from_dict_pk_entry_or_logged_40x(Semester, request.POST, "semester_id")
 
     if not semester.can_be_deleted_by_manager:
@@ -1313,7 +1318,7 @@ def helper_evaluation_edit(request, evaluation):
     # as the callback is captured by a weak reference in the Django Framework
     # and no other strong references are being kept.
     # See https://github.com/e-valuation/EvaP/issues/1361 for more information and discussion.
-    @receiver(RewardPointGranting.granted_by_removal, weak=True)
+    @receiver(RewardPointGranting.granted_by_participation_removal, weak=True)
     def notify_reward_points(grantings, **_kwargs):
         for granting in grantings:
             messages.info(
@@ -1418,9 +1423,15 @@ def helper_single_result_edit(request, evaluation):
 
 @require_POST
 @manager_required
+@transaction.atomic
 def evaluation_delete(request):
     evaluation = get_object_from_dict_pk_entry_or_logged_40x(Evaluation, request.POST, "evaluation_id")
 
+    # See comment in helper_evaluation_edit
+    @receiver(RewardPointGranting.granted_by_evaluation_deletion, weak=True)
+    def notify_reward_points(grantings, **_kwargs):
+        logger.info(f"Deletion of evaluation {evaluation} has created {len(grantings)} reward point grantings")
+
     if not evaluation.can_be_deleted_by_manager:
         raise SuspiciousOperation("Deleting evaluation not allowed")
     if evaluation.is_single_result:
@@ -2296,7 +2307,7 @@ def user_import(request):
 @manager_required
 def user_edit(request, user_id):
     # See comment in helper_evaluation_edit
-    @receiver(RewardPointGranting.granted_by_removal, weak=True)
+    @receiver(RewardPointGranting.granted_by_participation_removal, weak=True)
     def notify_reward_points(grantings, **_kwargs):
         assert len(grantings) == 1