ask for user confirmation if its role was moved out of presidency

club/models.py

@@ -240,7 +240,7 @@ class Club(models.Model):
 
     def can_roles_be_edited_by(self, user: User) -> bool:
         """Return True if the given user can edit the roles of this club"""
-        return (
+        return user.is_authenticated and (
             user.has_perm("club.change_clubrole")
             or self.members.ongoing().filter(user=user, role__is_presidency=True).exists()
         )

club/static/bundled/club/role-list-index.ts

@@ -3,16 +3,22 @@ import type { AlpineComponent } from "alpinejs";
 interface RoleGroupData {
   isBoard: boolean;
   isPresidency: boolean;
+  roleId: number;
 }
 
 document.addEventListener("alpine:init", () => {
-  Alpine.data("clubRoleList", () => ({
+  Alpine.data("clubRoleList", (config: { userRoleId: number | null }) => ({
+    confirmOnSubmit: false,
+
     /**
      * Edit relevant item data after it has been moved by x-sort
      */
     reorder(item: AlpineComponent<RoleGroupData>, conf: RoleGroupData) {
       item.isBoard = conf.isBoard;
       item.isPresidency = conf.isPresidency;
+      // if the user has moved its own role outside the presidency,
+      // submitting the form will require a confirmation
+      this.confirmOnSubmit = config.userRoleId === item.roleId && !item.isPresidency;
       this.resetOrder();
     },
     /**
@@ -33,5 +39,23 @@ document.addEventListener("alpine:init", () => {
         elem.value = (i + 1).toString();
       }
     },
+
+    /**
+     * If the user moved its role out of the presidency, ask a confirmation
+     * before submitting the form
+     */
+    confirmSubmission(event: SubmitEvent) {
+      if (
+        this.confirmOnSubmit &&
+        !confirm(
+          gettext(
+            "You're going to remove your own role from the presidency. " +
+              "You may lock yourself out of this page. Do you want to continue ? ",
+          ),
+        )
+      ) {
+        event.preventDefault();
+      }
+    },
   }));
 });

club/templates/club/club_roles.jinja

@@ -14,6 +14,7 @@
     x-data="{
             isPresidency: {{ subform.is_presidency.value()|lower }},
             isBoard: {{ subform.is_board.value()|lower }},
+            roleId: {{ subform.id.value() }},
             }"
     x-sort:item="$data"
   >
@@ -68,7 +69,11 @@
       (e.g. a board role can be made into a presidency role).
     {% endtrans %}
   </p>
-  <form method="post" x-data="clubRoleList">
+  <form
+    method="post"
+    x-data="clubRoleList({ userRoleId: {{ user_role or "null" }} })"
+    @submit="confirmSubmission"
+  >
     {% csrf_token %}
     {{ form.management_form }}
     {{ form.non_form_errors() }}
@@ -93,7 +98,6 @@
     <div
       x-sort="reorder($item, { isBoard: true, isPresidency: true })"
       x-sort:group="roles"
-      x-ref="presidencyFormSet"
     >
       {% for subform in form %}
         {% if subform.is_presidency.value() %}
@@ -129,7 +133,6 @@
     <div
       x-sort="reorder($item, { isBoard: true, isPresidency: false })"
       x-sort:group="roles"
-      x-ref="boardFormSet"
     >
       {% for subform in form %}
         {% if subform.is_board.value() and not subform.is_presidency.value() %}
@@ -150,7 +153,6 @@
     <div
       x-sort="reorder($item, { isBoard: false, isPresidency: false })"
       x-sort:group="roles"
-      x-ref="memberFormSet"
     >
       {% for subform in form %}
         {% if not subform.is_board.value() %}

club/tests/test_clubrole.py

@@ -2,7 +2,10 @@ import pytest
 from model_bakery import baker, seq
 from model_bakery.recipe import Recipe
 
-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 AnonymousUser, User
 
 
 @pytest.mark.django_db
@@ -31,3 +34,201 @@ 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,
+        ),
+        (lambda _: AnonymousUser(), 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",
+                ]
+            },
+            {},
+            {},
+        ]
+
+    def test_president_moves_itself_out_of_the_presidency(self):
+        """Test that if the user moves its own role out of the presidency,
+        then it's redirected to another page and loses access to the update page."""
+        self.payload["roles-0-is_presidency"] = False
+        self.client.force_login(self.user)
+        res = self.client.post(self.url, data=self.payload)
+        assertRedirects(
+            res, reverse("club:club_members", kwargs={"club_id": self.club.id})
+        )
+        # When the user clicked that button, it still had the right to update roles,
+        # so the modification should be applied
+        self.roles[0].refresh_from_db()
+        assert self.roles[0].is_presidency is False
+
+        res = self.client.get(self.url)
+        assert res.status_code == 403

club/views.py

@@ -430,19 +430,34 @@ class ClubRoleUpdateView(
     current_tab = "members"
     success_message = _("Club roles updated")
 
+    @cached_property
+    def club(self) -> Club:
+        return self.get_object()
+
     def test_func(self):
-        if self.request.user.has_perm("club.change_clubrole"):
+        return self.club.can_roles_be_edited_by(self.request.user)
-            return True
-        club: Club = self.get_object()
-        return club.members.filter(
-            user=self.request.user, role__is_presidency=True
-        ).exists()
 
     def get_form_kwargs(self):
         return super().get_form_kwargs() | {"form_kwargs": {"label_suffix": ""}}
 
     def get_success_url(self):
-        return self.request.path
+        # if the user lost the right to view the role update page
+        # (because it moved its own role out of the presidency),
+        # redirect to the club member page, else stay on the same page.
+        if self.club.can_roles_be_edited_by(self.request.user):
+            return self.request.path
+        return reverse("club:club_members", kwargs={"club_id": self.club.id})
+
+    def get_context_data(self, **kwargs):
+        return super().get_context_data(**kwargs) | {
+            "user_role": ClubRole.objects.filter(
+                club=self.object,
+                members__user=self.request.user,
+                members__end_date=None,
+            )
+            .values_list("id", flat=True)
+            .first()
+        }
 
 
 class ClubRoleBaseCreateView(UserPassesTestMixin, SuccessMessageMixin, CreateView):