diff --git a/evap/rewards/forms.py b/evap/rewards/forms.py index 0e68699681..9bb1a65a1b 100644 --- a/evap/rewards/forms.py +++ b/evap/rewards/forms.py @@ -1,14 +1,109 @@ +from contextlib import contextmanager +from datetime import date + from django import forms +from django.core.exceptions import ValidationError +from django.db import transaction +from django.utils.translation import gettext as _ -from evap.rewards.models import RewardPointRedemptionEvent +from evap.evaluation.models import UserProfile +from evap.rewards.models import RewardPointRedemption, RewardPointRedemptionEvent +from evap.rewards.tools import reward_points_of_user class RewardPointRedemptionEventForm(forms.ModelForm): class Meta: model = RewardPointRedemptionEvent - fields = ("name", "date", "redeem_end_date") + fields = ("name", "date", "redeem_end_date", "step") def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) self.fields["date"].localize = True self.fields["redeem_end_date"].localize = True + + +class RewardPointRedemptionForm(forms.Form): + event = forms.ModelChoiceField(queryset=RewardPointRedemptionEvent.objects.all(), widget=forms.HiddenInput()) + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + if not self.initial: + return + + help_text = _("multiples of {}").format(self.initial["event"].step) if self.initial["event"].step > 1 else "" + self.fields["points"] = forms.IntegerField( + min_value=0, + max_value=self.initial["total_points_available"], + step_size=self.initial["event"].step, + label="", + help_text=help_text, + ) + + def clean_event(self): + event = self.cleaned_data["event"] + if event.redeem_end_date < date.today(): + raise ValidationError(_("Sorry, the deadline for this event expired already.")) + return event + + +class BaseRewardPointRedemptionFormSet(forms.BaseFormSet): + def __init__(self, *args, user: UserProfile, **kwargs) -> None: + super().__init__(*args, **kwargs) + self.user = user + self.locked = False + + def get_form_kwargs(self, index): + kwargs = super().get_form_kwargs(index) + if not self.initial: + return kwargs + kwargs["initial"] = self.initial[index] + kwargs["initial"]["total_points_available"] = reward_points_of_user(self.user) + return kwargs + + @contextmanager + def lock(self): + with transaction.atomic(): + # lock these rows to prevent race conditions + list(self.user.reward_point_grantings.select_for_update()) + list(self.user.reward_point_redemptions.select_for_update()) + + self.locked = True + try: + yield + finally: + self.locked = False + + def clean(self): + assert self.locked + + if any(self.errors): + return + + total_points_available = reward_points_of_user(self.user) + total_points_redeemed = sum(form.cleaned_data["points"] for form in self.forms) + assert all(form.cleaned_data["points"] >= 0 for form in self.forms) + + if total_points_redeemed <= 0: + raise ValidationError(_("You cannot redeem 0 points.")) + + if total_points_redeemed > total_points_available: + raise ValidationError(_("You don't have enough reward points.")) + + def save(self) -> list[RewardPointRedemption]: + assert self.locked + + created = [] + for form in self.forms: + points = form.cleaned_data["points"] + if not points: + continue + redemption = RewardPointRedemption.objects.create( + user_profile=self.user, value=points, event=form.cleaned_data["event"] + ) + created.append(redemption) + return created + + +RewardPointRedemptionFormSet = forms.formset_factory( + RewardPointRedemptionForm, formset=BaseRewardPointRedemptionFormSet, extra=0 +) diff --git a/evap/rewards/migrations/0006_rewardpointredemptionevent_step.py b/evap/rewards/migrations/0006_rewardpointredemptionevent_step.py new file mode 100644 index 0000000000..8453cb9da8 --- /dev/null +++ b/evap/rewards/migrations/0006_rewardpointredemptionevent_step.py @@ -0,0 +1,20 @@ +# Generated by Django 5.0.4 on 2024-08-05 19:49 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("rewards", "0005_alter_rewardpoint_minvalue"), + ] + + operations = [ + migrations.AddField( + model_name="rewardpointredemptionevent", + name="step", + field=models.PositiveSmallIntegerField( + default=1, help_text="Only multiples of this step can be redeemed.", verbose_name="redemption step" + ), + ), + ] diff --git a/evap/rewards/models.py b/evap/rewards/models.py index da092d394a..5259d41b63 100644 --- a/evap/rewards/models.py +++ b/evap/rewards/models.py @@ -7,26 +7,13 @@ from evap.evaluation.models import Semester, UserProfile -class NoPointsSelectedError(Exception): - """An attempt has been made to redeem <= 0 points.""" - - -class NotEnoughPointsError(Exception): - """An attempt has been made to redeem more points than available.""" - - -class OutdatedRedemptionDataError(Exception): - """A redemption request has been sent with outdated data, e.g. when a request has been sent twice.""" - - -class RedemptionEventExpiredError(Exception): - """An attempt has been made to redeem more points for an event whose redeem_end_date lies in the past.""" - - class RewardPointRedemptionEvent(models.Model): name = models.CharField(max_length=1024, verbose_name=_("event name")) date = models.DateField(verbose_name=_("event date")) redeem_end_date = models.DateField(verbose_name=_("redemption end date")) + step = models.PositiveSmallIntegerField( + verbose_name=_("redemption step"), help_text=_("Only multiples of this step can be redeemed."), default=1 + ) @property def can_delete(self): diff --git a/evap/rewards/templates/rewards_index.html b/evap/rewards/templates/rewards_index.html index d6ea795b03..5987f60e68 100644 --- a/evap/rewards/templates/rewards_index.html +++ b/evap/rewards/templates/rewards_index.html @@ -22,6 +22,9 @@
{% csrf_token %} + {% include 'bootstrap_form_errors.html' with errors=formset.non_form_errors %} + {{ formset.management_form }} + @@ -33,13 +36,14 @@ - {% for event in events %} + {% for form, event in forms %} {% endfor %} diff --git a/evap/rewards/tests/test_views.py b/evap/rewards/tests/test_views.py index 933af5d9fc..5efe0fff7f 100644 --- a/evap/rewards/tests/test_views.py +++ b/evap/rewards/tests/test_views.py @@ -12,7 +12,7 @@ RewardPointRedemptionEvent, SemesterActivation, ) -from evap.rewards.tools import is_semester_activated, redeemed_points_of_user, reward_points_of_user +from evap.rewards.tools import is_semester_activated, reward_points_of_user from evap.staff.tests.utils import WebTestStaffMode, WebTestStaffModeWith200Check @@ -49,79 +49,67 @@ def setUpTestData(cls): baker.make(RewardPointGranting, user_profile=cls.student, value=5) cls.event1 = baker.make(RewardPointRedemptionEvent, redeem_end_date=date.today() + timedelta(days=1)) cls.event2 = baker.make(RewardPointRedemptionEvent, redeem_end_date=date.today() + timedelta(days=1)) + cls.event_with_step = baker.make( + RewardPointRedemptionEvent, redeem_end_date=date.today() + timedelta(days=1), step=5 + ) def test_redeem_all_points(self): response = self.app.get(self.url, user=self.student) form = response.forms["reward-redemption-form"] - form.set(f"points-{self.event1.pk}", 2) - form.set(f"points-{self.event2.pk}", 3) - response = form.submit() + form.set("form-0-points", 2) + form.set("form-1-points", 3) + response = form.submit().follow() self.assertContains(response, "You successfully redeemed your points.") self.assertEqual(0, reward_points_of_user(self.student)) def test_redeem_too_many_points(self): response = self.app.get(self.url, user=self.student) form = response.forms["reward-redemption-form"] - form.set(f"points-{self.event1.pk}", 3) - form.set(f"points-{self.event2.pk}", 3) - response = form.submit(status=400) - self.assertContains(response, "have enough reward points.", status_code=400) + form.set("form-0-points", 3) + form.set("form-1-points", 3) + response = form.submit() + self.assertContains(response, "have enough reward points.") self.assertEqual(5, reward_points_of_user(self.student)) def test_redeem_zero_points(self): response = self.app.get(self.url, user=self.student) form = response.forms["reward-redemption-form"] - form.set(f"points-{self.event1.pk}", 0) - response = form.submit(status=400) - self.assertContains(response, "cannot redeem 0 points.", status_code=400) + response = form.submit() + self.assertContains(response, "cannot redeem 0 points.") + self.assertEqual(5, reward_points_of_user(self.student)) + + def test_redeem_step(self): + response = self.app.get(self.url, user=self.student) + form = response.forms["reward-redemption-form"] + form.set("form-2-points", 5) + response = form.submit().follow() + self.assertContains(response, "You successfully redeemed your points.") + self.assertEqual(0, reward_points_of_user(self.student)) + + def test_redeem_wrong_step(self): + response = self.app.get(self.url, user=self.student) + form = response.forms["reward-redemption-form"] + form.set("form-2-points", 3) + response = form.submit() + self.assertContains(response, "this value is a multiple of step size 5") self.assertEqual(5, reward_points_of_user(self.student)) def test_redeem_points_for_expired_event(self): """Regression test for #846""" response = self.app.get(self.url, user=self.student) form = response.forms["reward-redemption-form"] - form.set(f"points-{self.event2.pk}", 1) + form.set("form-1-points", 1) RewardPointRedemptionEvent.objects.update(redeem_end_date=date.today() - timedelta(days=1)) - response = form.submit(status=400) - self.assertContains(response, "event expired already.", status_code=400) + response = form.submit() self.assertEqual(5, reward_points_of_user(self.student)) - def post_redemption_request(self, redemption_params, additional_params=None, status=200): - if additional_params is None: - additional_params = { - "previous_redeemed_points": redeemed_points_of_user(self.student), - } - return self.app.post( - self.url, params={**redemption_params, **additional_params}, user=self.student, status=status - ) - - def test_invalid_post_parameters(self): - self.post_redemption_request({"points-asd": 2}, status=400) - self.post_redemption_request({"points-": 2}, status=400) - self.post_redemption_request({f"points-{self.event1.pk}": ""}, status=400) - self.post_redemption_request({f"points-{self.event1.pk}": "asd"}, status=400) - - # redemption without or with invalid point parameters - self.post_redemption_request( - redemption_params={f"points-{self.event1.pk}": 1}, additional_params={}, status=400 - ) - self.post_redemption_request( - redemption_params={f"points-{self.event1.pk}": 1}, - additional_params={"previous_redeemed_points": "asd"}, - status=400, - ) - self.assertFalse(RewardPointRedemption.objects.filter(user_profile=self.student).exists()) - - # now, a correct request succeeds - self.post_redemption_request({f"points-{self.event1.pk}": 2}) - def test_inconsistent_previous_redemption_counts(self): response1 = self.app.get(self.url, user=self.student) form1 = response1.forms["reward-redemption-form"] - form1.set(f"points-{self.event1.pk}", 2) + form1.set("form-1-points", 2) response2 = self.app.get(self.url, user=self.student) form2 = response2.forms["reward-redemption-form"] - form2.set(f"points-{self.event1.pk}", 2) + form2.set("form-1-points", 2) form1.submit() form2.submit(status=409) self.assertEqual(1, RewardPointRedemption.objects.filter(user_profile=self.student).count()) diff --git a/evap/rewards/tools.py b/evap/rewards/tools.py index 7e5bae5fba..67a3bef9d6 100644 --- a/evap/rewards/tools.py +++ b/evap/rewards/tools.py @@ -1,54 +1,13 @@ -from datetime import date - from django.conf import settings from django.contrib import messages -from django.db import models, transaction +from django.db import models from django.db.models import Sum from django.dispatch import receiver -from django.shortcuts import get_object_or_404 from django.utils.translation import gettext as _ from django.utils.translation import ngettext from evap.evaluation.models import Evaluation, Semester, UserProfile -from evap.rewards.models import ( - NoPointsSelectedError, - NotEnoughPointsError, - OutdatedRedemptionDataError, - RedemptionEventExpiredError, - RewardPointGranting, - RewardPointRedemption, - RewardPointRedemptionEvent, - SemesterActivation, -) - - -@transaction.atomic -def save_redemptions(request, redemptions: dict[int, int], previous_redeemed_points: int): - # lock these rows to prevent race conditions - list(request.user.reward_point_grantings.select_for_update()) - list(request.user.reward_point_redemptions.select_for_update()) - - # check consistent previous redeemed points - # do not validate reward points, to allow receiving points after page load - if previous_redeemed_points != redeemed_points_of_user(request.user): - raise OutdatedRedemptionDataError - - total_points_available = reward_points_of_user(request.user) - total_points_redeemed = sum(redemptions.values()) - - if total_points_redeemed <= 0: - raise NoPointsSelectedError - - if total_points_redeemed > total_points_available: - raise NotEnoughPointsError - - for event_id in redemptions: - if redemptions[event_id] > 0: - event = get_object_or_404(RewardPointRedemptionEvent, pk=event_id) - if event.redeem_end_date < date.today(): - raise RedemptionEventExpiredError - - RewardPointRedemption.objects.create(user_profile=request.user, value=redemptions[event_id], event=event) +from evap.rewards.models import RewardPointGranting, RewardPointRedemption, SemesterActivation def can_reward_points_be_used_by(user): diff --git a/evap/rewards/views.py b/evap/rewards/views.py index 475f1bf357..169dcf689a 100644 --- a/evap/rewards/views.py +++ b/evap/rewards/views.py @@ -1,5 +1,5 @@ import csv -from datetime import datetime +from datetime import date, datetime from django.contrib import messages from django.contrib.messages.views import SuccessMessageMixin @@ -13,76 +13,58 @@ from django.utils.translation import gettext_lazy from django.views.decorators.http import require_POST from django.views.generic import CreateView, UpdateView -from typing_extensions import assert_never from evap.evaluation.auth import manager_required, reward_user_required from evap.evaluation.models import Semester, UserProfile from evap.evaluation.tools import AttachmentResponse, get_object_from_dict_pk_entry_or_logged_40x from evap.rewards.exporters import RewardsExporter -from evap.rewards.forms import RewardPointRedemptionEventForm +from evap.rewards.forms import RewardPointRedemptionEventForm, RewardPointRedemptionFormSet from evap.rewards.models import ( - NoPointsSelectedError, - NotEnoughPointsError, - OutdatedRedemptionDataError, - RedemptionEventExpiredError, RewardPointGranting, RewardPointRedemption, RewardPointRedemptionEvent, SemesterActivation, ) -from evap.rewards.tools import grant_eligible_reward_points_for_semester, reward_points_of_user, save_redemptions - - -def redeem_reward_points(request): - redemptions = {} - try: - for key, value in request.POST.items(): - if key.startswith("points-"): - event_id = int(key.rpartition("-")[2]) - redemptions[event_id] = int(value) - previous_redeemed_points = int(request.POST["previous_redeemed_points"]) - except (ValueError, KeyError, TypeError) as e: - raise BadRequest from e - - try: - save_redemptions(request, redemptions, previous_redeemed_points) - messages.success(request, _("You successfully redeemed your points.")) - except ( - NoPointsSelectedError, - NotEnoughPointsError, - RedemptionEventExpiredError, - OutdatedRedemptionDataError, - ) as error: - status_code = 400 - match error: - case NoPointsSelectedError(): - error_string = _("You cannot redeem 0 points.") - case NotEnoughPointsError(): - error_string = _("You don't have enough reward points.") - case RedemptionEventExpiredError(): - error_string = _("Sorry, the deadline for this event expired already.") - case OutdatedRedemptionDataError(): - status_code = 409 - error_string = _( - "It appears that your browser sent multiple redemption requests. You can see all successful redemptions below." - ) - case _: - assert_never(type(error)) - - messages.error(request, error_string) - return status_code - return 200 +from evap.rewards.tools import grant_eligible_reward_points_for_semester, redeemed_points_of_user, reward_points_of_user @reward_user_required def index(request): status = 200 + + events = RewardPointRedemptionEvent.objects.filter(redeem_end_date__gte=date.today()).order_by("date") + + # pylint: disable=unexpected-keyword-arg + formset = RewardPointRedemptionFormSet( + request.POST or None, + initial=[{"event": e, "points": 0} for e in events], + user=request.user, + ) if request.method == "POST": - status = redeem_reward_points(request) + with formset.lock(): + try: + previous_redeemed_points = int(request.POST["previous_redeemed_points"]) + except ValueError as e: + raise BadRequest from e + + if previous_redeemed_points != redeemed_points_of_user(request.user): + # Do formset validation here in order to do within the lock + formset.is_valid() + status = 409 + messages.error( + request, + _( + "It appears that your browser sent multiple redemption requests. You can see all successful redemptions below." + ), + ) + elif formset.is_valid(): + formset.save() + messages.success(request, _("You successfully redeemed your points.")) + return redirect("rewards:index") + total_points_available = reward_points_of_user(request.user) reward_point_grantings = RewardPointGranting.objects.filter(user_profile=request.user) reward_point_redemptions = RewardPointRedemption.objects.filter(user_profile=request.user) - events = RewardPointRedemptionEvent.objects.filter(redeem_end_date__gte=datetime.now()).order_by("date") granted_point_actions = [ (granting.granting_time, _("Reward for") + " " + granting.semester.name, granting.value, "") @@ -102,6 +84,8 @@ def index(request): "total_points_available": total_points_available, "total_points_spent": sum(redemption.value for redemption in reward_point_redemptions), "events": events, + "formset": formset, + "forms": zip(formset, events, strict=True), } return render(request, "rewards_index.html", template_data, status=status)
{{ event.date }} {{ event.name }} {{ event.redeem_end_date }} - + {{ form.event }} + {% include 'bootstrap_form_field_widget.html' with field=form.points %}