Grant reward points on evaluation deletion (#2358)

Author: richardebeling

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.assertFalse(inside_transaction())
+
+        with transaction.atomic():
+            self.assertTrue(inside_transaction())

evap/evaluation/tests/tools.py

@@ -55,6 +55,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

@@ -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,20 +1,26 @@
+import logging
+
 from django.conf import settings
 from django.contrib import messages
 from django.db import models
 from django.db.models import Sum
 from django.dispatch import receiver
+from django.http import HttpRequest
 from django.utils.translation import gettext as _
 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):
+def can_reward_points_be_used_by(user: UserProfile) -> bool:
     return not user.is_external and user.is_participant
 
 
-def reward_points_of_user(user):
+def reward_points_of_user(user: UserProfile) -> int:
     count = 0
     for granting in user.reward_point_grantings.all():
         count += granting.value
@@ -24,15 +30,19 @@ def reward_points_of_user(user):
     return count
 
 
-def redeemed_points_of_user(user):
+def redeemed_points_of_user(user: UserProfile) -> int:
     return RewardPointRedemption.objects.filter(user_profile=user).aggregate(Sum("value"))["value__sum"] or 0
 
 
-def is_semester_activated(semester):
+def is_semester_activated(semester: Semester) -> bool:
     return SemesterActivation.objects.filter(semester=semester, is_active=True).exists()
 
 
-def grant_reward_points_if_eligible(user, semester):
+def deactivate_semester(semester: Semester) -> None:
+    SemesterActivation.objects.filter(semester=semester).update(is_active=False)
+
+
+def grant_reward_points_if_eligible(user: UserProfile, semester: Semester) -> tuple[RewardPointGranting | None, bool]:
     if not can_reward_points_be_used_by(user):
         return None, False
     if not is_semester_activated(semester):
@@ -57,7 +67,7 @@ def grant_reward_points_if_eligible(user, semester):
     return None, False
 
 
-def grant_eligible_reward_points_for_semester(request, semester):
+def grant_eligible_reward_points_for_semester(request: HttpRequest, semester: Semester) -> None:
     users = UserProfile.objects.filter(evaluations_voted_for__course__semester=semester)
     reward_point_sum = 0
     for user in users:
@@ -77,7 +87,9 @@ def grant_eligible_reward_points_for_semester(request, semester):
 
 
 @receiver(Evaluation.evaluation_evaluated)
-def grant_reward_points_after_evaluate(request, semester, **_kwargs):
+def grant_reward_points_after_evaluate(request: HttpRequest, semester: Semester, **_kwargs) -> None:
+    assert isinstance(request.user, UserProfile)
+
     granting, completed_evaluation = grant_reward_points_if_eligible(request.user, semester)
     if granting:
         message = ngettext(
@@ -100,21 +112,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: str, reverse: bool, pk_set, **_kwargs) -> None:
     # 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 +135,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: Evaluation, **_kwargs) -> None:
+    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

@@ -10,6 +10,7 @@
 from django.conf import settings
 from django.contrib.auth.models import Group
 from django.core import mail
+from django.db import transaction
 from django.db.models import Model
 from django.http import HttpResponse
 from django.test import override_settings
@@ -50,6 +51,7 @@
 from evap.grades.models import GradeDocument
 from evap.results.tools import TextResult, cache_results, get_results
 from evap.rewards.models import RewardPointGranting, SemesterActivation
+from evap.rewards.tools import reward_points_of_user
 from evap.staff.forms import ContributionCopyForm, ContributionCopyFormset, CourseCopyForm, EvaluationCopyForm
 from evap.staff.tests.utils import (
     WebTestStaffMode,
@@ -302,7 +304,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 +319,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 +332,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")
@@ -895,6 +901,36 @@ def test_success_with_data(self):
 
         self.assertFalse(Semester.objects.filter(pk=self.instance.pk).exists())
 
+    @override_settings(REWARD_POINTS=[(1, 3)])
+    def test_does_not_grant_redemption_points(self):
+        student = baker.make(UserProfile, email="[email protected]")
+        evaluation = baker.make(
+            Evaluation, participants=[student], state=Evaluation.State.PUBLISHED, course__semester=self.instance
+        )
+        SemesterActivation.objects.update_or_create(semester=self.instance, defaults={"is_active": True})
+
+        # student must be participant in at least one other evaluation
+        baker.make(
+            Evaluation,
+            participants=[student],
+            voters=[student],
+            state=Evaluation.State.PUBLISHED,
+            course__semester=self.instance,
+        )
+
+        self.instance.archive()
+        self.instance.delete_grade_documents()
+        self.instance.archive_results()
+
+        # just deleting the evaluation would grant redemption point -- ensures setup above is sufficient
+        with transaction.atomic():
+            Evaluation.objects.get(pk=evaluation.pk).delete()
+            assert reward_points_of_user(student) == 3
+            transaction.set_rollback(True)
+
+        self.app.post(self.url, params=self.post_params, user=self.user)
+        self.assertEqual(reward_points_of_user(student), 0)
+
 
 class TestSemesterAssignView(WebTestStaffMode):
     @classmethod
@@ -2370,6 +2406,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="[email protected]", 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="[email protected]")
+        self.assertEqual(reward_points_of_user(user_a), 3)
+
 
 class TestSingleResultEditView(WebTestStaffModeWith200Check):
     @classmethod