Grant reward points on evaluation deletion

evap/rewards/models.py

@@ -40,7 +40,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

@@ -1,3 +1,5 @@
+import logging
+
 from django.conf import settings
 from django.contrib import messages
 from django.db import models
@@ -7,8 +9,11 @@
 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
 
+logger = logging.getLogger(__name__)
+
 
 def can_reward_points_be_used_by(user):
     return not user.is_external and user.is_participant
@@ -100,21 +105,21 @@ def grant_reward_points_after_evaluate(request, semester, **_kwargs):
 
 
 @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 +128,27 @@ def grant_reward_points_after_delete(instance, action, reverse, pk_set, **_kwarg
                     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.error("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

@@ -302,7 +302,7 @@ def test_inactive_edit(self, mock_remove):
         mock_remove.assert_called_with(self.testuser)
         self.assertIn(mock_remove.return_value[0], response)
 
-    def test_reward_points_granting_message(self):
+    def test_reward_point_granting(self):
         evaluation = baker.make(Evaluation, course__semester__is_active=True)
         already_evaluated = baker.make(Evaluation, course=baker.make(Course, semester=evaluation.course.semester))
         SemesterActivation.objects.create(semester=evaluation.course.semester, is_active=True)
@@ -317,6 +317,8 @@ def test_reward_points_granting_message(self):
         form = page.forms["user-form"]
         form["evaluations_participating_in"] = [already_evaluated.pk]
 
+        self.assertEqual(reward_points_of_user(student), 0)
+
         page = form.submit().follow()
         # fetch the user name, which became lowercased
         student.refresh_from_db()
@@ -328,6 +330,8 @@ def test_reward_points_granting_message(self):
             page,
         )
 
+        self.assertEqual(reward_points_of_user(student), 3)
+
 
 class TestUserDeleteView(DeleteViewTestMixin, WebTestStaffMode):
     url = reverse("staff:user_delete")
@@ -2370,6 +2374,36 @@ 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(self):
+        already_evaluated = baker.make(Evaluation, course__semester=self.evaluation.course.semester)
+        SemesterActivation.objects.create(semester=self.evaluation.course.semester, is_active=True)
+
+        # does not get reward points as it was the only evaluation in this semester
+        student = baker.make(
+            UserProfile, email="student@institution.example.com", evaluations_participating_in=[self.evaluation]
+        )
+
+        # get reward points
+        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,
+        )
+
+        self.assertEqual(reward_points_of_user(student), 0)
+
+        with patch("evap.staff.views.logger") as mock:
+            self.app.post(self.url, user=self.manager, params=self.post_params, status=200)
+
+        self.assertEqual(mock.info.call_args_list[0][1]["extra"]["num_grantings"], 5)
+        self.assertEqual(reward_points_of_user(student), 0)
+
+        user_a = UserProfile.objects.get(email="a@institution.example.com")
+        self.assertEqual(reward_points_of_user(user_a), 3)
+
 
 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):
@@ -1313,7 +1316,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 +1421,18 @@ 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(
+            "Deletion of evaluation has created reward point grantings",
+            extra={"evaluation": evaluation, "num_grantings": len(grantings)},
+        )
+
     if not evaluation.can_be_deleted_by_manager:
         raise SuspiciousOperation("Deleting evaluation not allowed")
     if evaluation.is_single_result:
@@ -2296,7 +2308,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