From 80f1f9699cc2b9e0f0cea898867104cb4bc67e79 Mon Sep 17 00:00:00 2001
From: Bartuccio Antoine <klmp200@klmp200.net>
Date: Wed, 24 Apr 2019 13:48:32 +0200
Subject: [PATCH] clubs: move Membership form validation outside of model and
 fix and add tests

---
 club/models.py | 12 ----------
 club/tests.py  | 65 +++++++++++++++++++++++++++++++++++++++++++-------
 club/views.py  | 13 ++++++----
 3 files changed, 65 insertions(+), 25 deletions(-)

diff --git a/club/models.py b/club/models.py
index 00c9239d..250d584f 100644
--- a/club/models.py
+++ b/club/models.py
@@ -276,18 +276,6 @@ class Membership(models.Model):
         _("description"), max_length=128, null=False, blank=True
     )
 
-    def clean(self):
-        sub = User.objects.filter(pk=self.user.pk).first()
-        if sub is None or not sub.is_subscribed:
-            raise ValidationError(_("User must be subscriber to take part to a club"))
-        if (
-            Membership.objects.filter(user=self.user)
-            .filter(club=self.club)
-            .filter(end_date=None)
-            .exists()
-        ):
-            raise ValidationError(_("User is already member of that club"))
-
     def __str__(self):
         return (
             self.club.name
diff --git a/club/tests.py b/club/tests.py
index c0409a9f..b9130bb4 100644
--- a/club/tests.py
+++ b/club/tests.py
@@ -44,7 +44,7 @@ class ClubTest(TestCase):
         self.client.login(username="root", password="plop")
         self.client.post(
             reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
-            {"user": self.skia.id, "start_date": "12/06/2016", "role": 3},
+            {"users": self.skia.id, "start_date": "12/06/2016", "role": 3},
         )
         response = self.client.get(
             reverse("club:club_members", kwargs={"club_id": self.bdf.id})
@@ -55,14 +55,37 @@ class ClubTest(TestCase):
             in str(response.content)
         )
 
+    def test_create_add_multiple_user_to_club_from_root_ok(self):
+        self.client.login(username="root", password="plop")
+        self.client.post(
+            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
+            {
+                "users": "|%d|%d|" % (self.skia.id, self.rbatsbak.id),
+                "start_date": "12/06/2016",
+                "role": 3,
+            },
+        )
+        response = self.client.get(
+            reverse("club:club_members", kwargs={"club_id": self.bdf.id})
+        )
+        self.assertTrue(response.status_code == 200)
+        content = str(response.content)
+        self.assertTrue(
+            "S&#39; Kia</a></td>\\n                <td>Responsable info</td>" in content
+        )
+        self.assertTrue(
+            "Richard Batsbak</a></td>\\n                <td>Responsable info</td>"
+            in content
+        )
+
     def test_create_add_user_to_club_from_root_fail_not_subscriber(self):
         self.client.login(username="root", password="plop")
         response = self.client.post(
             reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
-            {"user": self.guy.id, "start_date": "12/06/2016", "role": 3},
+            {"users": self.guy.id, "start_date": "12/06/2016", "role": 3},
         )
         self.assertTrue(response.status_code == 200)
-        self.assertTrue('<ul class="errorlist nonfield"><li>' in str(response.content))
+        self.assertTrue('<ul class="errorlist"><li>' in str(response.content))
         response = self.client.get(
             reverse("club:club_members", kwargs={"club_id": self.bdf.id})
         )
@@ -75,7 +98,7 @@ class ClubTest(TestCase):
         self.client.login(username="root", password="plop")
         self.client.post(
             reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
-            {"user": self.skia.id, "start_date": "12/06/2016", "role": 3},
+            {"users": self.skia.id, "start_date": "12/06/2016", "role": 3},
         )
         response = self.client.get(
             reverse("club:club_members", kwargs={"club_id": self.bdf.id})
@@ -86,7 +109,7 @@ class ClubTest(TestCase):
         )
         response = self.client.post(
             reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
-            {"user": self.skia.id, "start_date": "12/06/2016", "role": 4},
+            {"users": self.skia.id, "start_date": "12/06/2016", "role": 4},
         )
         self.assertTrue(response.status_code == 200)
         self.assertFalse(
@@ -94,16 +117,40 @@ class ClubTest(TestCase):
             in str(response.content)
         )
 
+    def test_create_add_user_non_existent_to_club_from_root_fail(self):
+        self.client.login(username="root", password="plop")
+        response = self.client.post(
+            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
+            {"users": [9999], "start_date": "12/06/2016", "role": 3},
+        )
+        self.assertTrue(response.status_code == 200)
+        content = str(response.content)
+        self.assertTrue('<ul class="errorlist"><li>' in content)
+        self.assertFalse("<td>Responsable info</td>" in content)
+        self.client.login(username="root", password="plop")
+        response = self.client.post(
+            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
+            {
+                "users": "|%d|%d|" % (self.skia.id, 9999),
+                "start_date": "12/06/2016",
+                "role": 3,
+            },
+        )
+        self.assertTrue(response.status_code == 200)
+        content = str(response.content)
+        self.assertTrue('<ul class="errorlist"><li>' in content)
+        self.assertFalse("<td>Responsable info</td>" in content)
+
     def test_create_add_user_to_club_from_skia_ok(self):
         self.client.login(username="root", password="plop")
         self.client.post(
             reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
-            {"user": self.skia.id, "start_date": "12/06/2016", "role": 10},
+            {"users": self.skia.id, "start_date": "12/06/2016", "role": 10},
         )
         self.client.login(username="skia", password="plop")
         self.client.post(
             reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
-            {"user": self.rbatsbak.id, "start_date": "12/06/2016", "role": 9},
+            {"users": self.rbatsbak.id, "start_date": "12/06/2016", "role": 9},
         )
         response = self.client.get(
             reverse("club:club_members", kwargs={"club_id": self.bdf.id})
@@ -118,12 +165,12 @@ class ClubTest(TestCase):
         self.client.login(username="root", password="plop")
         self.client.post(
             reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
-            {"user": self.rbatsbak.id, "start_date": "12/06/2016", "role": 3},
+            {"users": self.rbatsbak.id, "start_date": "12/06/2016", "role": 3},
         )
         self.client.login(username="rbatsbak", password="plop")
         response = self.client.post(
             reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
-            {"user": self.skia.id, "start_date": "12/06/2016", "role": 10},
+            {"users": self.skia.id, "start_date": "12/06/2016", "role": 10},
         )
         self.assertTrue(response.status_code == 200)
         self.assertTrue(
diff --git a/club/views.py b/club/views.py
index 5b7e1349..64b5e301 100644
--- a/club/views.py
+++ b/club/views.py
@@ -326,8 +326,9 @@ class ClubMemberForm(forms.Form):
         self.request_user = kwargs.pop("request_user")
         super(ClubMemberForm, self).__init__(*args, **kwargs)
 
-        # Using a ModelForm forces a save and we don't want that
+        # Using a ModelForm binds too much the form with the model and we don't want that
         # We want the view to process the model creation since they are multiple users
+        # We also want the form to handle bulk deletion
         self.fields.update(
             forms.fields_for_model(
                 Membership,
@@ -341,6 +342,7 @@ class ClubMemberForm(forms.Form):
     def clean_users(self):
         """
             Check that the user is not trying to add an user already in the club
+            Also check that the user is valid and has a valid subscription
         """
         cleaned_data = super(ClubMemberForm, self).clean()
         users = []
@@ -348,13 +350,17 @@ class ClubMemberForm(forms.Form):
             user = User.objects.filter(id=user_id).first()
             if not user:
                 raise forms.ValidationError(
-                    _("One of the selected users doesn't exist", code="invalid")
+                    _("One of the selected users doesn't exist"), code="invalid"
+                )
+            if not user.is_subscribed:
+                raise forms.ValidationError(
+                    _("User must be subscriber to take part to a club"), code="invalid"
                 )
-            users.append(user)
             if self.club.get_membership_for(user):
                 raise forms.ValidationError(
                     _("You can not add the same user twice"), code="invalid"
                 )
+            users.append(user)
         return users
 
     def clean(self):
@@ -364,7 +370,6 @@ class ClubMemberForm(forms.Form):
         cleaned_data = super(ClubMemberForm, self).clean()
         request_user = self.request_user
         membership = self.club.get_membership_for(request_user)
-        print(request_user.is_root)
         if not (
             cleaned_data["role"] <= SITH_MAXIMUM_FREE_ROLE
             or (membership is not None and membership.role >= cleaned_data["role"])