Merge branch 'beta' into 'master'

Ensure the integrity of all note balances in multiple transactions

See merge request bde/nk20!131

apps/member/tests/test_memberships.py

@@ -205,7 +205,7 @@ class TestMemberships(TestCase):
                 first_name="Toto",
                 bank="Le matelas",
             ))
-            self.assertRedirects(response, club.get_absolute_url(), 302, 200)
+            self.assertRedirects(response, user.profile.get_absolute_url(), 302, 200)
 
             self.assertTrue(Membership.objects.filter(user=user, club=club).exists())
 
@@ -244,9 +244,9 @@ class TestMemberships(TestCase):
                 first_name="Toto",
                 bank="Bank",
             ))
-            self.assertRedirects(response, club.get_absolute_url(), 302, 200)
+            self.assertRedirects(response, user.profile.get_absolute_url(), 302, 200)
 
-            response = self.client.get(user.profile.get_absolute_url())
+            response = self.client.get(club.get_absolute_url())
             self.assertEqual(response.status_code, 200)
 
     def test_auto_join_kfet_when_join_bde_with_soge(self):
@@ -273,7 +273,7 @@ class TestMemberships(TestCase):
             first_name="Toto",
             bank="Société générale",
         ))
-        self.assertRedirects(response, bde.get_absolute_url(), 302, 200)
+        self.assertRedirects(response, user.profile.get_absolute_url(), 302, 200)
 
         self.assertTrue(Membership.objects.filter(user=user, club=bde).exists())
         self.assertTrue(Membership.objects.filter(user=user, club=kfet).exists())

apps/member/views.py

@@ -157,7 +157,7 @@ class UserDetailView(ProtectQuerysetMixin, LoginRequiredMixin, DetailView):
         history_table.paginate(per_page=20, page=self.request.GET.get("transaction-page", 1))
         context['history_list'] = history_table
 
-        club_list = Membership.objects.filter(user=user, date_end__gte=date.today())\
+        club_list = Membership.objects.filter(user=user, date_end__gte=date.today() - timedelta(days=15))\
             .filter(PermissionBackend.filter_queryset(self.request.user, Membership, "view"))
         membership_table = MembershipTable(data=club_list, prefix='membership-')
         membership_table.paginate(per_page=10, page=self.request.GET.get("membership-page", 1))
@@ -409,7 +409,7 @@ class ClubDetailView(ProtectQuerysetMixin, LoginRequiredMixin, DetailView):
         # member list
         club_member = Membership.objects.filter(
             club=club,
-            date_end__gte=date.today(),
+            date_end__gte=date.today() - timedelta(days=15),
         ).filter(PermissionBackend.filter_queryset(self.request.user, Membership, "view"))
 
         membership_table = MembershipTable(data=club_member, prefix="membership-")
@@ -680,6 +680,7 @@ class ClubAddMemberView(ProtectQuerysetMixin, ProtectedCreateView):
             club = Club.objects.filter(PermissionBackend.filter_queryset(self.request.user, Club, "view")) \
                 .get(pk=self.kwargs["club_pk"])
             user = form.instance.user
+            old_membership = None
         else:  # get from url for renewal
             old_membership = self.get_queryset().get(pk=self.kwargs["pk"])
             club = old_membership.club
@@ -754,6 +755,9 @@ class ClubAddMemberView(ProtectQuerysetMixin, ProtectedCreateView):
         member_role = Role.objects.filter(Q(name="Adhérent BDE") | Q(name="Membre de club")).all() \
             if club.name == "BDE" else Role.objects.filter(Q(name="Adhérent Kfet") | Q(name="Membre de club")).all() \
             if club.name == "Kfet"else Role.objects.filter(name="Membre de club").all()
+        # Set the same roles as before
+        if old_membership:
+            member_role = member_role.union(old_membership.roles.all())
         form.instance.roles.set(member_role)
         form.instance._force_save = True
         form.instance.save()
@@ -791,7 +795,7 @@ class ClubAddMemberView(ProtectQuerysetMixin, ProtectedCreateView):
         return ret
 
     def get_success_url(self):
-        return reverse_lazy('member:club_detail', kwargs={'pk': self.object.club.id})
+        return reverse_lazy('member:user_detail', kwargs={'pk': self.object.user.id})
 
 
 class ClubManageRolesView(ProtectQuerysetMixin, LoginRequiredMixin, UpdateView):

apps/note/apps.py

@@ -3,7 +3,7 @@
 
 from django.apps import AppConfig
 from django.conf import settings
-from django.db.models.signals import post_save, pre_delete
+from django.db.models.signals import pre_delete, pre_save, post_save
 from django.utils.translation import gettext_lazy as _
 
 from . import signals
@@ -17,6 +17,15 @@ class NoteConfig(AppConfig):
         """
         Define app internal signals to interact with other apps
         """
+        pre_save.connect(
+            signals.pre_save_note,
+            sender="note.noteuser",
+        )
+        pre_save.connect(
+            signals.pre_save_note,
+            sender="note.noteclub",
+        )
+
         post_save.connect(
             signals.save_user_note,
             sender=settings.AUTH_USER_MODEL,

apps/note/models/notes.py

@@ -159,20 +159,6 @@ class NoteUser(Note):
     def pretty(self):
         return _("%(user)s's note") % {'user': str(self.user)}
 
-    @transaction.atomic
-    def save(self, *args, **kwargs):
-        if self.pk and self.balance < 0:
-            old_note = NoteUser.objects.get(pk=self.pk)
-            super().save(*args, **kwargs)
-            if old_note.balance >= 0:
-                # Passage en négatif
-                self.last_negative = timezone.now()
-                self._force_save = True
-                self.save(*args, **kwargs)
-                self.send_mail_negative_balance()
-        else:
-            super().save(*args, **kwargs)
-
     def send_mail_negative_balance(self):
         plain_text = render_to_string("note/mails/negative_balance.txt", dict(note=self))
         html = render_to_string("note/mails/negative_balance.html", dict(note=self))
@@ -201,20 +187,6 @@ class NoteClub(Note):
     def pretty(self):
         return _("Note of %(club)s club") % {'club': str(self.club)}
 
-    @transaction.atomic
-    def save(self, *args, **kwargs):
-        if self.pk and self.balance < 0:
-            old_note = NoteClub.objects.get(pk=self.pk)
-            super().save(*args, **kwargs)
-            if old_note.balance >= 0:
-                # Passage en négatif
-                self.last_negative = timezone.now()
-                self._force_save = True
-                self.save(*args, **kwargs)
-                self.send_mail_negative_balance()
-        else:
-            super().save(*args, **kwargs)
-
     def send_mail_negative_balance(self):
         plain_text = render_to_string("note/mails/negative_balance.txt", dict(note=self))
         html = render_to_string("note/mails/negative_balance.html", dict(note=self))

apps/note/models/transactions.py

@@ -217,6 +217,9 @@ class Transaction(PolymorphicModel):
             # When source == destination, no money is transferred and no transaction is created
             return
 
+        self.source = Note.objects.select_for_update().get(pk=self.source_id)
+        self.destination = Note.objects.select_for_update().get(pk=self.destination_id)
+
         # Check that the amounts stay between big integer bounds
         diff_source, diff_dest = self.validate()
 

apps/note/signals.py

@@ -1,6 +1,8 @@
 # Copyright (C) 2018-2020 by BDE ENS Paris-Saclay
 # SPDX-License-Identifier: GPL-3.0-or-later
 
+from django.utils import timezone
+
 
 def save_user_note(instance, raw, **_kwargs):
     """
@@ -25,6 +27,16 @@ def save_club_note(instance, raw, **_kwargs):
         instance.note.save()
 
 
+def pre_save_note(instance, raw, **_kwargs):
+    if not raw and instance.pk and not hasattr(instance, "_no_signal") and instance.balance < 0:
+        from note.models import Note
+        old_note = Note.objects.get(pk=instance.pk)
+        if old_note.balance >= 0:
+            # Passage en négatif
+            instance.last_negative = timezone.now()
+            instance.send_mail_negative_balance()
+
+
 def delete_transaction(instance, **_kwargs):
     """
     Whenever we want to delete a transaction (caution with this), we ensure the transaction is invalid first.