diff --git a/evap/evaluation/tests/test_tools.py b/evap/evaluation/tests/test_tools.py index 84efbdda4d..dff26cf3af 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.assertFalse(inside_transaction()) + + with transaction.atomic(): + self.assertTrue(inside_transaction()) diff --git a/evap/evaluation/tests/tools.py b/evap/evaluation/tests/tools.py index b77a84dea2..af37bf55f6 100644 --- a/evap/evaluation/tests/tools.py +++ b/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 diff --git a/evap/evaluation/tools.py b/evap/evaluation/tools.py index 825e7078cd..0514a652d4 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 e8abdf2d76..937358ee11 100644 --- a/evap/rewards/models.py +++ b/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): diff --git a/evap/rewards/tests/test_tools.py b/evap/rewards/tests/test_tools.py index d3769de8c0..1823410695 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 67a3bef9d6..336ed55f5c 100644 --- a/evap/rewards/tools.py +++ b/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,13 +112,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: 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): @@ -114,7 +126,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 +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) diff --git a/evap/staff/tests/test_views.py b/evap/staff/tests/test_views.py index 2247a65b7c..441576f018 100644 --- a/evap/staff/tests/test_views.py +++ b/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="student@institution.example.com") + 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="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 diff --git a/evap/staff/views.py b/evap/staff/views.py index c584406dd4..998ad670ae 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 @@ -76,7 +77,7 @@ from evap.results.tools import TextResult, calculate_average_distribution, distribution_to_grade from evap.results.views import update_template_cache_of_published_evaluations_in_course from evap.rewards.models import RewardPointGranting -from evap.rewards.tools import can_reward_points_be_used_by, is_semester_activated +from evap.rewards.tools import can_reward_points_be_used_by, deactivate_semester, is_semester_activated from evap.staff import staff_mode from evap.staff.forms import ( AtLeastOneFormset, @@ -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): @@ -676,6 +679,9 @@ def semester_delete(request): if not semester.can_be_deleted_by_manager: raise SuspiciousOperation("Deleting semester not allowed") with transaction.atomic(): + # ensure deleting evaluations can not grant redemption points + deactivate_semester(semester) + RatingAnswerCounter.objects.filter(contribution__evaluation__course__semester=semester).delete() TextAnswer.objects.filter(contribution__evaluation__course__semester=semester).delete() Contribution.objects.filter(evaluation__course__semester=semester).delete() @@ -1313,7 +1319,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 +1424,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 +2311,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