diff --git a/evap/evaluation/tests/test_tools.py b/evap/evaluation/tests/test_tools.py index 84efbdda4..73bc17e0c 100644 --- a/evap/evaluation/tests/test_tools.py +++ b/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) diff --git a/evap/evaluation/tests/tools.py b/evap/evaluation/tests/tools.py index 64c116bf7..af0427879 100644 --- a/evap/evaluation/tests/tools.py +++ b/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 diff --git a/evap/evaluation/tools.py b/evap/evaluation/tools.py index 825e7078c..0514a652d 100644 --- a/evap/evaluation/tools.py +++ b/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) diff --git a/evap/rewards/models.py b/evap/rewards/models.py index 5259d41b6..bbda9df44 100644 --- a/evap/rewards/models.py +++ b/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): diff --git a/evap/rewards/tests/test_tools.py b/evap/rewards/tests/test_tools.py index d3769de8c..182341069 100644 --- a/evap/rewards/tests/test_tools.py +++ b/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) diff --git a/evap/rewards/tools.py b/evap/rewards/tools.py index 67a3bef9d..b48716685 100644 --- a/evap/rewards/tools.py +++ b/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,13 +101,13 @@ 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): @@ -114,7 +115,7 @@ def grant_reward_points_after_delete(instance, action, reverse, pk_set, **_kwarg 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 @@ 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.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) diff --git a/evap/staff/tests/test_views.py b/evap/staff/tests/test_views.py index cd7132bc5..ba7a2e842 100644 --- a/evap/staff/tests/test_views.py +++ b/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 diff --git a/evap/staff/views.py b/evap/staff/views.py index 16e58022c..6aba3c3d3 100644 --- a/evap/staff/views.py +++ b/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