Allow setting step size for reward point redemption and use formset (#…

…2264)

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
+)

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"
+            ),
+        ),
+    ]

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):

evap/rewards/templates/rewards_index.html

@@ -22,6 +22,9 @@
                     <form id="reward-redemption-form" action="#" method="POST" class="form-horizontal multiselect-form">
                         {% csrf_token %}
 
+                        {% include 'bootstrap_form_errors.html' with errors=formset.non_form_errors %}
+                        {{ formset.management_form }}
+
                         <input type="hidden" name="previous_redeemed_points" value="{{ total_points_spent }}">
                         <table class="table table-striped table-vertically-aligned mb-3">
                             <thead>
@@ -33,13 +36,14 @@
                                 </tr>
                             </thead>
                             <tbody>
-                                {% for event in events %}
+                                {% for form, event in forms %}
                                     <tr>
                                         <td>{{ event.date }}</td>
                                         <td>{{ event.name }}</td>
                                         <td>{{ event.redeem_end_date }}</td>
                                         <td>
-                                            <input class="form-control" id="id_points-{{ event.id }}" name="points-{{ event.id }}" type="number" value="0" min="0" max="{{ total_points_available }}">
+                                            {{ form.event }}
+                                            {% include 'bootstrap_form_field_widget.html' with field=form.points %}
                                         </td>
                                     </tr>
                                 {% endfor %}

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())

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):