add tests

club/migrations/0015_clubrole_alter_membership_role.py

@@ -128,6 +128,9 @@ class Migration(migrations.Migration):
                     ("is_presidency", False), ("is_board", True), _connector="OR"
                 ),
                 name="clubrole_presidency_implies_board",
+                violation_error_message=(
+                    "A role cannot be in the presidency while not being in the board"
+                ),
             ),
         ),
         migrations.RunPython(migrate_roles, migrations.RunPython.noop),

club/models.py

@@ -208,7 +208,9 @@ class Club(models.Model):
         """Return True if the given user can edit the roles of this club"""
         return (
             user.has_perm("club.change_clubrole")
-            or self.members.ongoing().filter(user=user, role__is_presidency=True).exists()
+            or self.members.ongoing()
+            .filter(user=user, role__is_presidency=True)
+            .exists()
         )
 
     @cached_property
@@ -258,6 +260,9 @@ class ClubRole(OrderedModel):
             models.CheckConstraint(
                 condition=Q(is_presidency=False) | Q(is_board=True),
                 name="clubrole_presidency_implies_board",
+                violation_error_message=_(
+                    "A role cannot be in the presidency while not being in the board"
+                ),
             )
         ]
 
@@ -267,26 +272,13 @@ class ClubRole(OrderedModel):
     def get_display_name(self):
         return f"{self.name} - {self.club.name}"
 
-    def get_absolute_url(self):
-        return reverse("club:club_roles", kwargs={"club_id": self.club_id})
-
     def clean(self):
         errors = []
-        if self.is_presidency and not self.is_board:
-            errors.append(
-                ValidationError(
-                    _(
-                        "Role %(name)s was declared as a presidency role "
-                        "without being a board role"
-                    )
-                    % {"name": self.name}
-                )
-            )
         roles = list(self.club.roles.all())
         if (
             self.is_board
             and self.order
-            and any(r.order < self.order and not r.is_board for r in roles)
+            and any(r.order <= self.order and not r.is_board for r in roles)
         ):
             errors.append(
                 ValidationError(
@@ -297,7 +289,7 @@ class ClubRole(OrderedModel):
         if (
             self.is_presidency
             and self.order
-            and any(r.order < self.order and not r.is_presidency for r in roles)
+            and any(r.order <= self.order and not r.is_presidency for r in roles)
         ):
             errors.append(
                 ValidationError(

club/tests/test_club_controller.py

@@ -9,7 +9,7 @@ from model_bakery import baker
 from model_bakery.recipe import Recipe
 from pytest_django.asserts import assertNumQueries
 
-from club.models import Club, ClubRole, Membership, ClubRole
+from club.models import Club, ClubRole, Membership
 from core.baker_recipes import subscriber_user
 from core.models import Group, Page, User
 

club/tests/test_clubrole.py

@@ -1,25 +1,48 @@
+from collections.abc import Callable
+
 import pytest
+from django.contrib.auth.models import Permission
+from django.test import Client, TestCase
+from django.urls import reverse
 from model_bakery import baker, seq
 from model_bakery.recipe import Recipe
+from pytest_django.asserts import assertRedirects
 
-from club.models import Club, ClubRole
+from club.forms import ClubRoleFormSet
+from club.models import Club, ClubRole, Membership
+from core.baker_recipes import subscriber_user
+from core.models import User
 
 
-@pytest.mark.django_db
+def make_club():
-def test_order_auto():
+    # unittest-style tests cannot use fixture, so we create a function
-    """Test that newly created roles are put in the right place."""
+    # that will be callable either by a pytest fixture or inside
+    # a TestCase.setUpTestData method.
     club = baker.make(Club)
     recipe = Recipe(ClubRole, club=club, name=seq("role "))
-    # bulk create initial roles (1 presidency, 1 board, 1 member)
+    recipe.make(
-    roles = recipe.make(
         is_board=iter([True, True, False]),
         is_presidency=iter([True, False, False]),
         order=iter([1, 2, 3]),
         _quantity=3,
         _bulk_create=True,
     )
-    # then create the remaining roles one by one (like they will be in prod)
+    return club
+
+
+@pytest.fixture
+def club(db):
+    """A club with a presidency role, a board role and a member role"""
+    return make_club()
+
+
+@pytest.mark.django_db
+def test_order_auto(club):
+    """Test that newly created roles are put in the right place."""
+    roles = list(club.roles.all())
+    # create new roles one by one (like they will be in prod)
     # each new role should be placed at the end of its category
+    recipe = Recipe(ClubRole, club=club, name=seq("new role "))
     role_a = recipe.make(is_board=True, is_presidency=True, order=None)
     role_b = recipe.make(is_board=True, is_presidency=False, order=None)
     role_c = recipe.make(is_board=False, is_presidency=False, order=None)
@@ -31,3 +54,183 @@ def test_order_auto():
         roles[2],
         role_c,
     ]
+
+
+@pytest.mark.django_db
+@pytest.mark.parametrize(
+    ("user_factory", "is_allowed"),
+    [
+        (
+            lambda club: baker.make(
+                User,
+                user_permissions=[Permission.objects.get(codename="change_clubrole")],
+            ),
+            True,
+        ),
+        (  # user with presidency roles can edit the club roles
+            lambda club: subscriber_user.make(
+                memberships=[
+                    baker.make(
+                        Membership,
+                        club=club,
+                        role=club.roles.filter(is_presidency=True).first(),
+                    )
+                ]
+            ),
+            True,
+        ),
+        (  # user in the board but not in the presidency cannot edit roles
+            lambda club: subscriber_user.make(
+                memberships=[
+                    baker.make(
+                        Membership,
+                        club=club,
+                        role=club.roles.filter(
+                            is_presidency=False, is_board=True
+                        ).first(),
+                    )
+                ]
+            ),
+            False,
+        ),
+    ],
+)
+def test_can_roles_be_edited_by(
+    club: Club, user_factory: Callable[[Club], User], is_allowed
+):
+    """Test that `Club.can_roles_be_edited_by` return the right value"""
+    user = user_factory(club)
+    assert club.can_roles_be_edited_by(user) == is_allowed
+
+
+@pytest.mark.django_db
+@pytest.mark.parametrize(
+    ["route", "is_presidency", "is_board"],
+    [
+        ("club:new_role_president", True, True),
+        ("club:new_role_board", False, True),
+        ("club:new_role_member", False, False),
+    ],
+)
+def test_create_role_view(client: Client, route: str, is_presidency, is_board):
+    """Test that the role creation views work."""
+    club = baker.make(Club)
+    role = baker.make(ClubRole, club=club, is_presidency=True, is_board=True)
+    user = subscriber_user.make()
+    baker.make(Membership, club=club, role=role, user=user, end_date=None)
+    url = reverse(route, kwargs={"club_id": club.id})
+    client.force_login(user)
+
+    res = client.get(url)
+    assert res.status_code == 200
+
+    res = client.post(url, data={"name": "foo"})
+    assertRedirects(res, reverse("club:club_roles", kwargs={"club_id": club.id}))
+    new_role = club.roles.last()
+    assert new_role.name == "foo"
+    assert new_role.is_presidency == is_presidency
+    assert new_role.is_board == is_board
+
+
+class TestClubRoleUpdate(TestCase):
+    @classmethod
+    def setUpTestData(cls):
+        cls.club = make_club()
+        cls.roles = list(cls.club.roles.all())
+        cls.user = subscriber_user.make()
+        baker.make(
+            Membership, club=cls.club, role=cls.roles[0], user=cls.user, end_date=None
+        )
+        cls.url = reverse("club:club_roles", kwargs={"club_id": cls.club.id})
+
+    def setUp(self):
+        self.payload = {
+            "roles-TOTAL_FORMS": 3,
+            "roles-INITIAL_FORMS": 3,
+            "roles-MIN_NUM_FORMS": 0,
+            "roles-MAX_NUM_FORMS": 1000,
+            "roles-0-ORDER": self.roles[0].order,
+            "roles-0-id": self.roles[0].id,
+            "roles-0-club": self.club.id,
+            "roles-0-is_presidency": True,
+            "roles-0-is_board": True,
+            "roles-0-name": self.roles[0].name,
+            "roles-0-description": self.roles[0].description,
+            "roles-0-is_active": True,
+            "roles-1-ORDER": self.roles[1].order,
+            "roles-1-id": self.roles[1].id,
+            "roles-1-club": self.club.id,
+            "roles-1-is_presidency": False,
+            "roles-1-is_board": True,
+            "roles-1-name": self.roles[1].name,
+            "roles-1-description": self.roles[1].description,
+            "roles-1-is_active": True,
+            "roles-2-ORDER": self.roles[2].order,
+            "roles-2-id": self.roles[2].id,
+            "roles-2-club": self.club.id,
+            "roles-2-is_presidency": False,
+            "roles-2-is_board": False,
+            "roles-2-name": self.roles[2].name,
+            "roles-2-description": self.roles[2].description,
+            "roles-2-is_active": True,
+        }
+
+    def test_view_ok(self):
+        """Basic test to check that the view works."""
+        self.client.force_login(self.user)
+        res = self.client.get(self.url)
+        assert res.status_code == 200
+        self.payload["roles-2-name"] = "foo"
+        res = self.client.post(self.url, data=self.payload)
+        assertRedirects(res, self.url)
+        self.roles[2].refresh_from_db()
+        assert self.roles[2].name == "foo"
+
+    def test_incoherent_order(self):
+        """Test that placing a member role over a board role fails."""
+        self.payload["roles-0-ORDER"] = 4
+        formset = ClubRoleFormSet(data=self.payload, instance=self.club)
+        assert not formset.is_valid()
+        assert formset.errors == [
+            {
+                "__all__": [
+                    f"Le rôle {self.roles[0].name} ne peut pas "
+                    "être placé en-dessous d'un rôle de membre.",
+                    f"Le rôle {self.roles[0].name} ne peut pas être placé "
+                    "en-dessous d'un rôle qui n'est pas de la présidence.",
+                ]
+            },
+            {},
+            {},
+        ]
+
+    def test_change_order_ok(self):
+        """Test that changing order the intended way works"""
+        self.payload["roles-1-ORDER"] = 3
+        self.payload["roles-1-is_board"] = False
+        self.payload["roles-2-ORDER"] = 2
+        formset = ClubRoleFormSet(data=self.payload, instance=self.club)
+        assert formset.is_valid()
+        formset.save()
+        assert list(self.club.roles.order_by("order")) == [
+            self.roles[0],
+            self.roles[2],
+            self.roles[1],
+        ]
+        self.roles[1].refresh_from_db()
+        assert not self.roles[1].is_board
+
+    def test_non_board_presidency_is_forbidden(self):
+        """Test that a role cannot be in the presidency without being in the board."""
+        self.payload["roles-0-is_board"] = False
+        formset = ClubRoleFormSet(data=self.payload, instance=self.club)
+        assert not formset.is_valid()
+        assert formset.errors == [
+            {
+                "__all__": [
+                    "Un rôle ne peut pas appartenir à la présidence sans être dans le bureau",
+                ]
+            },
+            {},
+            {},
+        ]

club/views.py

@@ -431,12 +431,8 @@ class ClubRoleUpdateView(
     success_message = _("Club roles updated")
 
     def test_func(self):
-        if self.request.user.has_perm("club.change_clubrole"):
-            return True
         club: Club = self.get_object()
-        return club.members.filter(
+        return club.can_roles_be_edited_by(self.request.user)
-            user=self.request.user, role__is_presidency=True
-        ).exists()
 
     def get_form_kwargs(self):
         return super().get_form_kwargs() | {"form_kwargs": {"label_suffix": ""}}

locale/fr/LC_MESSAGES/django.po

@@ -256,10 +256,9 @@ msgstr "rôles de club"
 #: club/models.py
 #, python-format
 msgid ""
-"Role %(name)s was declared as a presidency role without being a board role"
+"A role cannot be in the presidency while not being in the board"
 msgstr ""
-"Le rôle %(name)s a été déclaré comme rôle de présidence sans être un rôle du "
+"Un rôle ne peut pas appartenir à la présidence sans être dans le bureau"
-"bureau."
 
 #: club/models.py
 #, python-format
@@ -271,7 +270,7 @@ msgstr ""
 #, python-format
 msgid "Role %(role)s cannot be placed below a non-presidency role"
 msgstr ""
-"Le rôle %(role)s ne peut pas être placé en-dessous d'un rôle de membre."
+"Le rôle %(role)s ne peut pas être placé en-dessous d'un rôle qui n'est pas de la présidence."
 
 #: club/models.py core/models.py counter/models.py eboutic/models.py
 #: election/models.py pedagogy/models.py sas/models.py trombi/models.py