From 9ed14432f8c3d8e0fc4520fcb063363b37287c65 Mon Sep 17 00:00:00 2001 From: Ivar Nakken Date: Thu, 7 Mar 2024 00:22:38 +0100 Subject: [PATCH 1/3] Introduce "too late" as an event presence status Similar to the "not present" status, the user will automatically recieve a penalty. I figured all previous penalties can be removed since only the newest one should matter. I would like some input as to how this will affect other penalties, e.g. given due to late payments (or other things?), as they will also be removed if the presence is changed to something worthy of a penalty. Should we consider to introduce "penalty types" stored on the record, e.g. "presence", "payment", "other"? Also, this resolves a bug, as it is currently not possible to auto delete previous penalties related to the event due to the `.delete()` not being at the end the atomic block. Not sure why it had to be atomic in the first place. --- lego/apps/events/constants.py | 1 + .../0040_alter_registration_presence.py | 26 +++++++++++++ lego/apps/events/models.py | 38 ++++++++++++++----- lego/apps/events/serializers/registrations.py | 12 +++--- 4 files changed, 61 insertions(+), 16 deletions(-) create mode 100644 lego/apps/events/migrations/0040_alter_registration_presence.py diff --git a/lego/apps/events/constants.py b/lego/apps/events/constants.py index 2753b7b83..1f5281116 100644 --- a/lego/apps/events/constants.py +++ b/lego/apps/events/constants.py @@ -70,6 +70,7 @@ class PRESENCE_CHOICES(models.TextChoices): UNKNOWN = "UNKNOWN" PRESENT = "PRESENT" + LATE = "LATE" NOT_PRESENT = "NOT_PRESENT" diff --git a/lego/apps/events/migrations/0040_alter_registration_presence.py b/lego/apps/events/migrations/0040_alter_registration_presence.py new file mode 100644 index 000000000..5c960e230 --- /dev/null +++ b/lego/apps/events/migrations/0040_alter_registration_presence.py @@ -0,0 +1,26 @@ +# Generated by Django 4.0.10 on 2024-03-06 22:04 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("events", "0039_remove_event_use_contact_tracing"), + ] + + operations = [ + migrations.AlterField( + model_name="registration", + name="presence", + field=models.CharField( + choices=[ + ("UNKNOWN", "Unknown"), + ("PRESENT", "Present"), + ("LATE", "Late"), + ("NOT_PRESENT", "Not Present"), + ], + default="UNKNOWN", + max_length=20, + ), + ), + ] diff --git a/lego/apps/events/models.py b/lego/apps/events/models.py index efb369e70..d2ecac4cd 100644 --- a/lego/apps/events/models.py +++ b/lego/apps/events/models.py @@ -930,26 +930,46 @@ def set_presence(self, presence: constants.PRESENCE_CHOICES) -> None: """Wrap this method in a transaction""" if presence not in constants.PRESENCE_CHOICES: raise ValueError("Illegal presence choice") + self.presence = presence self.handle_user_penalty(presence) self.save() + def delete_penalties_for_event(self) -> None: + for penalty in self.user.penalties.filter(source_event=self.event): + penalty.delete() + def handle_user_penalty(self, presence: constants.PRESENCE_CHOICES) -> None: + """ + Previous penalties related to the event are deleted since the + newest presence is the only one that matters + """ + if ( self.event.heed_penalties and presence == constants.PRESENCE_CHOICES.NOT_PRESENT and self.event.penalty_weight_on_not_present ): - if not self.user.penalties.filter(source_event=self.event).exists(): - Penalty.objects.create( - user=self.user, - reason=f"Møtte ikke opp på {self.event.title}.", - weight=self.event.penalty_weight_on_not_present, - source_event=self.event, - ) + self.delete_penalties_for_event() + Penalty.objects.create( + user=self.user, + reason=f"Møtte ikke opp på {self.event.title}.", + weight=self.event.penalty_weight_on_not_present, + source_event=self.event, + ) + elif ( + self.event.heed_penalties + and presence == constants.PRESENCE_CHOICES.TOO_LATE + ): + self.delete_penalties_for_event() + Penalty.objects.create( + user=self.user, + reason=f"Møtte for sent opp på {self.event.title}.", + weight=1, + source_event=self.event, + ) else: - for penalty in self.user.penalties.filter(source_event=self.event): - penalty.delete() + self.delete_penalties_for_event() def add_to_pool(self, pool: Pool) -> Registration: allowed: bool = False diff --git a/lego/apps/events/serializers/registrations.py b/lego/apps/events/serializers/registrations.py index a3ed6dde4..3ce2220c3 100644 --- a/lego/apps/events/serializers/registrations.py +++ b/lego/apps/events/serializers/registrations.py @@ -1,4 +1,3 @@ -from django.db import transaction from rest_framework import serializers from rest_framework_jwt.serializers import ImpersonateAuthTokenSerializer @@ -65,13 +64,12 @@ class Meta: ) def update(self, instance, validated_data): - with transaction.atomic(): - presence = validated_data.pop("presence", None) - super().update(instance, validated_data) - if presence: - instance.set_presence(presence) + presence = validated_data.pop("presence", None) + super().update(instance, validated_data) + if presence: + instance.set_presence(presence) - return instance + return instance class RegistrationAnonymizedReadSerializer(BasisModelSerializer): From 3a209a246f9b49d293de3d38b50d5d03774f5c80 Mon Sep 17 00:00:00 2001 From: Ivar Nakken Date: Thu, 14 Mar 2024 11:42:55 +0100 Subject: [PATCH 2/3] Include penalty types to the model Used to distinguish and delete the correct penalties, as penalties from the same type (and event) should in theory never stack. --- lego/apps/events/models.py | 23 +++++++++-------- lego/apps/users/constants.py | 17 +++++++++++-- .../users/migrations/0042_penalty_type.py | 25 +++++++++++++++++++ lego/apps/users/models.py | 5 ++++ 4 files changed, 57 insertions(+), 13 deletions(-) create mode 100644 lego/apps/users/migrations/0042_penalty_type.py diff --git a/lego/apps/events/models.py b/lego/apps/events/models.py index d2ecac4cd..300ede1af 100644 --- a/lego/apps/events/models.py +++ b/lego/apps/events/models.py @@ -30,7 +30,7 @@ from lego.apps.files.models import FileField from lego.apps.followers.models import FollowEvent from lego.apps.permissions.models import ObjectPermissionsModel -from lego.apps.users.constants import AUTUMN, SPRING +from lego.apps.users.constants import AUTUMN, PENALTY_TYPES, PENALTY_WEIGHTS, SPRING from lego.apps.users.models import AbakusGroup, Membership, Penalty, User from lego.utils.models import BasisModel from lego.utils.youtube_validator import youtube_validator @@ -935,8 +935,10 @@ def set_presence(self, presence: constants.PRESENCE_CHOICES) -> None: self.handle_user_penalty(presence) self.save() - def delete_penalties_for_event(self) -> None: - for penalty in self.user.penalties.filter(source_event=self.event): + def delete_presence_penalties_for_event(self) -> None: + for penalty in self.user.penalties.filter( + source_event=self.event, type=PENALTY_TYPES.PRESENCE + ): penalty.delete() def handle_user_penalty(self, presence: constants.PRESENCE_CHOICES) -> None: @@ -950,26 +952,25 @@ def handle_user_penalty(self, presence: constants.PRESENCE_CHOICES) -> None: and presence == constants.PRESENCE_CHOICES.NOT_PRESENT and self.event.penalty_weight_on_not_present ): - self.delete_penalties_for_event() + self.delete_presence_penalties_for_event() Penalty.objects.create( user=self.user, reason=f"Møtte ikke opp på {self.event.title}.", weight=self.event.penalty_weight_on_not_present, source_event=self.event, + type=PENALTY_TYPES.PRESENCE, ) - elif ( - self.event.heed_penalties - and presence == constants.PRESENCE_CHOICES.TOO_LATE - ): - self.delete_penalties_for_event() + elif self.event.heed_penalties and presence == constants.PRESENCE_CHOICES.LATE: + self.delete_presence_penalties_for_event() Penalty.objects.create( user=self.user, reason=f"Møtte for sent opp på {self.event.title}.", - weight=1, + weight=PENALTY_WEIGHTS.LATE_PRESENCE, source_event=self.event, + type=PENALTY_TYPES.PRESENCE, ) else: - self.delete_penalties_for_event() + self.delete_presence_penalties_for_event() def add_to_pool(self, pool: Pool) -> Registration: allowed: bool = False diff --git a/lego/apps/users/constants.py b/lego/apps/users/constants.py index 7f10738de..c32db7448 100644 --- a/lego/apps/users/constants.py +++ b/lego/apps/users/constants.py @@ -1,5 +1,7 @@ from enum import Enum +from django.db import models + MALE = "male" FEMALE = "female" OTHER = "other" @@ -95,8 +97,6 @@ def values(cls) -> list[str]: FSGroup.MSSECCLO: FOURTH_GRADE_KOMTEK, } -STUDENT_EMAIL_DOMAIN = "stud.ntnu.no" - GROUP_COMMITTEE = "komite" GROUP_INTEREST = "interesse" GROUP_BOARD = "styre" @@ -142,3 +142,16 @@ def values(cls) -> list[str]: (LIGHT_THEME, LIGHT_THEME), (DARK_THEME, DARK_THEME), ) + + +LATE_PRESENCE_PENALTY_WEIGHT = 1 + + +class PENALTY_WEIGHTS(models.TextChoices): + LATE_PRESENCE = 1 + + +class PENALTY_TYPES(models.TextChoices): + PRESENCE = "presence" + PAYMENT = "payment" + OTHER = "other" diff --git a/lego/apps/users/migrations/0042_penalty_type.py b/lego/apps/users/migrations/0042_penalty_type.py new file mode 100644 index 000000000..7b4f6af4f --- /dev/null +++ b/lego/apps/users/migrations/0042_penalty_type.py @@ -0,0 +1,25 @@ +# Generated by Django 4.0.10 on 2024-03-14 10:27 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("users", "0041_user_linkedin_id_alter_user_github_username"), + ] + + operations = [ + migrations.AddField( + model_name="penalty", + name="type", + field=models.CharField( + choices=[ + ("presence", "Presence"), + ("payment", "Payment"), + ("other", "Other"), + ], + default="other", + max_length=50, + ), + ), + ] diff --git a/lego/apps/users/models.py b/lego/apps/users/models.py index d5797c3f9..7752a8b8a 100644 --- a/lego/apps/users/models.py +++ b/lego/apps/users/models.py @@ -537,6 +537,11 @@ class Penalty(BasisModel): source_event = models.ForeignKey( "events.Event", related_name="penalties", on_delete=models.CASCADE ) + type = models.CharField( + max_length=50, + choices=constants.PENALTY_TYPES.choices, + default=constants.PENALTY_TYPES.OTHER, + ) objects = UserPenaltyManager() # type: ignore From c9acb5f8fe4a36eff7e848d78b83fd70c5bb4588 Mon Sep 17 00:00:00 2001 From: Ivar Nakken Date: Thu, 14 Mar 2024 12:03:08 +0100 Subject: [PATCH 3/3] Test presence and penalties update --- lego/apps/events/tests/test_penalties.py | 66 +++++++++++++++++++++++- 1 file changed, 65 insertions(+), 1 deletion(-) diff --git a/lego/apps/events/tests/test_penalties.py b/lego/apps/events/tests/test_penalties.py index f518ea092..dc7d998a3 100644 --- a/lego/apps/events/tests/test_penalties.py +++ b/lego/apps/events/tests/test_penalties.py @@ -4,6 +4,7 @@ from lego.apps.events import constants from lego.apps.events.models import Event, Registration +from lego.apps.users.constants import LATE_PRESENCE_PENALTY_WEIGHT from lego.apps.users.models import AbakusGroup, Penalty from lego.utils.test_utils import BaseTestCase @@ -398,6 +399,19 @@ def test_penalties_created_when_not_present(self): self.assertEqual(penalties_before, 0) self.assertEqual(penalties_after, event.penalty_weight_on_not_present) + def test_penalties_created_when_late_present(self): + """Test that user gets penalties when late present""" + event = Event.objects.get(title="POOLS_WITH_REGISTRATIONS") + + registration = event.registrations.first() + penalties_before = registration.user.number_of_penalties() + + registration.set_presence(constants.PRESENCE_CHOICES.LATE) + + penalties_after = registration.user.number_of_penalties() + self.assertEqual(penalties_before, 0) + self.assertEqual(penalties_after, LATE_PRESENCE_PENALTY_WEIGHT) + def test_penalties_removed_when_not_present_changes(self): """Test that penalties for not_present gets removed when resetting presence""" event = Event.objects.get(title="POOLS_WITH_REGISTRATIONS") @@ -411,8 +425,23 @@ def test_penalties_removed_when_not_present_changes(self): self.assertEqual(penalties_before, event.penalty_weight_on_not_present) self.assertEqual(penalties_after, 0) + def test_penalties_removed_when_late_present_changes(self): + """Test that penalties for late presence gets removed when changing to present""" + event = Event.objects.get(title="POOLS_WITH_REGISTRATIONS") + registration = event.registrations.first() + registration.set_presence(constants.PRESENCE_CHOICES.LATE) + + penalties_before = registration.user.number_of_penalties() + registration.set_presence(constants.PRESENCE_CHOICES.PRESENT) + + penalties_after = registration.user.number_of_penalties() + self.assertEqual(penalties_before, LATE_PRESENCE_PENALTY_WEIGHT) + self.assertEqual(penalties_after, 0) + def test_only_correct_penalties_are_removed_on_presence_change(self): - """Test that only penalties for given event are removed when changing presence""" + """ + Test that only penalties of type presence for given event are removed when changing presence + """ event = Event.objects.get(title="POOLS_WITH_REGISTRATIONS") other_event = Event.objects.get(title="POOLS_NO_REGISTRATIONS") registration = event.registrations.first() @@ -447,6 +476,41 @@ def test_only_correct_penalties_are_removed_on_presence_change(self): ) self.assertEqual(penalties_after, other_event.penalty_weight_on_not_present) + def test_only_correct_penalties_are_removed_on_presence_change_on_same_event(self): + """Test that only penalties of type presence are removed when changing presence""" + event = Event.objects.get(title="POOLS_WITH_REGISTRATIONS") + registration = event.registrations.first() + + registration.set_presence(constants.PRESENCE_CHOICES.NOT_PRESENT) + penalties_before = registration.user.number_of_penalties() + penalties_object_before = list(registration.user.penalties.all()) + + # Default penalty type is other + Penalty.objects.create( + user=registration.user, + reason="SAME EVENT", + weight=2, + source_event=event, + ) + penalties_during = registration.user.number_of_penalties() + penalties_objects_during = list(registration.user.penalties.all()) + + registration.set_presence(constants.PRESENCE_CHOICES.UNKNOWN) + penalties_after = registration.user.number_of_penalties() + penalties_object_after = list(registration.user.penalties.all()) + + self.assertEqual(penalties_object_before[0].source_event, event) + self.assertEqual(penalties_object_after[0].source_event, event) + self.assertEqual(len(penalties_object_before), 1) + self.assertEqual(len(penalties_objects_during), 2) + self.assertEqual(len(penalties_object_after), 1) + self.assertEqual(penalties_before, event.penalty_weight_on_not_present) + self.assertEqual( + penalties_during, + event.penalty_weight_on_not_present + event.penalty_weight_on_not_present, + ) + self.assertEqual(penalties_after, event.penalty_weight_on_not_present) + def test_able_to_register_when_not_heed_penalties_with_penalties(self): """Test that user is able to register when heed_penalties is false and user has penalties""" event = Event.objects.get(title="POOLS_WITH_REGISTRATIONS")