Reward point redemption events

[hansegucker]

Close #2258
Close #1825

[janno42]

✔️ Meets requirements
✔️ UI functionality checked

[Kakadus]

some minor comments besides the transaction discussion

[richardebeling]

thanks!

[niklasmohrin]

Very nice! I have some final thoughts that I want to clear up before I approve :)

[niklasmohrin]

I got stuck for a second here on why we have this and the "You don't have enough reward points" check later, but I now got that the later one is for the sum of the redemptions.

Should we add a test that ensures these validations here are also run? Thinking cyber, I would want to check that we reject a request where we redeem 5 and -3 points when 4 are available (or similar) (and maybe also just 5 and just -3).

We can also do it in a follow up though, if you want

[richardebeling]

Also bothered me. iirc, we need the validator to get the correct drop-down choices in the frontend.

I wouldn't want to extensively test django validation. We can check that it is configured correctly (max value via the validator, min value via the form vield), but two simple assertions should be enough for that, I wouldn't start checking cross product stuff.

We can add an assertion to "our" validation logic that none of the values are negative.

[niklasmohrin]

I agree, but I mean it's just three cases, I think that it is fair to do (I think that all three are interesting enough). In particular the case that the sum is bound, but the individual values are not, seems important to me.

[niklasmohrin]

Suggested change

	events = RewardPointRedemptionEvent.objects.filter(redeem_end_date__gte=datetime.now().date()).order_by("date")
	events = RewardPointRedemptionEvent.objects.filter(redeem_end_date__gte=date.today()).order_by("date")

[niklasmohrin]

Suggested change

	with formset.lock():
	if request.method == "POST":
	if request.method == "POST":
	with formset.lock():

[niklasmohrin]

This seems a bit fragile, I would much rather have something where formset is created inside a with lock_for_reward_changes(request.user) and never escapes it, but I see that this conflicts with the Django form workflow. Thoughts?

I now wonder whether .lock() is the best name here, because the formset is not being locked, but rather we touch some rows in the database to include them in the transaction that we have made atomic. From my current understanding, formset.atomic() would better fit this. What do you think?

[richardebeling]

where formset is created inside a with lock_for_reward_changes(request.user) and never escapes it, but I see that this conflicts with the Django form workflow.

We can have the context manager be useable as a class thing instead of a instance thing, have it notify its locking globally. I don't see why this wouldn't work. Don't know if its more elegant, though.

No opinion on the name. lock, assert, critical_section, whatever, all fine with me. I think "context within which we validate correctness of requested redemptions and apply them" is too long, so I don't see a name that wouldn't require people to look up what exactly it does when modifying the code.

[niklasmohrin]

We can have the context manager be useable as a class thing instead of a instance thing, have it notify its locking globally.

I think I would prefer that, it would probably look okay with the form escaping the with-block:

with lock_for_reward_points_upgrade(request.user):
    formset = ...
    if ...:
        formset.save()
        return ...

return render(..., formset)

[niklasmohrin]

@hansegucker pointed out that this would make the assertions in clean and save more awkward - I didn't know that these were a requirement, it's fair though.

I now thought that maybe a good way to do it would be something like

class TheFormset:
    @classmethod
    @contextmanager
    @transaction.atomic
    def make_in_transaction(cls, *args, **kwargs):
        list(...) # lock the stuff
        instance = cls(*args, **kwargs)
        instance.locked = True
        try:
            yield instance
        finally:
            instance.locked = False

[richardebeling]

@hansegucker pointed out that this would make the assertions in clean and save more awkward - I didn't know that these were a requirement, it's fair though.

Why? I'd have thought we just set and unset a global/class variable, instead of an instance variable.

I now thought that maybe a good way to do it would be something like

Also fine with your approach

[niklasmohrin]

Does this need to be inside of formset.lock()? From my current understanding, the answer is yes, but it doesn't matter because normal usage doesn't trigger the locking edge cases and exploiting the race here will be caught by our final checks before creating the redemption objects. Is that correct?

[richardebeling]

Doesn't need to be inside the critical section. This only ends up in the MaxValueValidator in RewardPointRedemptionForm, which is only there for frontend/usability. For (security) correctness, only the total_points_available = reward_points_of_user(self.user) inside BaseRewardPointRedemptionFormSet.clean is relevant.

[niklasmohrin]

But just to be sure, these validators also run later when handling the POST data right? Just with maybe outdated bounds

[richardebeling]

It's just a django form field validator -- yes, it better be validated when calling is_valid.

The reward points being outdated could theoretically be an issue if a user was awarded points in the meantime. The MaxValueValidator allows providing a callable instead of a value. Maybe that allows shifting the computation to a time inside the locked phase -- but for just rendering the page, we never lock any rows anyway...

[richardebeling]

@hansegucker where does "max" come from, and why do we need a separate MaxValueValidator? In the django docs on IntegerField, it sounds to me as if the field automatically does min- and max value validation if the values are set up, but it says that this should be done as min_value and max_value. We use min_value above, why not just max_value here?

Same for the StepValidator below, the IntegerField is supposed to have support for step_size.

[hansegucker]

We have to differentiate on form fields and form widgets. max is a form widget attribute, max_value is a form field attribute. At this time, the form is already initialized and changing max_value doesn't have any effect anymore.

[richardebeling]

To make sure the comment from the other thread isn't lost: I propose to add

Suggested change

	total_points_redeemed = sum(form.cleaned_data["points"] for form in self.forms)
	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)

here

[hansegucker]

We allow points to be zero if the user doesn't want to redeem points for an event. For values below 0, we validate the IntegerField with min_value=0. So, this additional check seems to be superfluous in my opinion.