diff --git a/club/models.py b/club/models.py index 408090b2..2c5b5ff0 100644 --- a/club/models.py +++ b/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() ) diff --git a/club/static/bundled/club/role-list-index.ts b/club/static/bundled/club/role-list-index.ts index a750501a..2020bba4 100644 --- a/club/static/bundled/club/role-list-index.ts +++ b/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, 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(); + } + }, })); }); diff --git a/club/templates/club/club_roles.jinja b/club/templates/club/club_roles.jinja index 4c1d2111..eaa693f0 100644 --- a/club/templates/club/club_roles.jinja +++ b/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 %}

-
+ {% csrf_token %} {{ form.management_form }} {{ form.non_form_errors() }} @@ -93,7 +98,6 @@
{% for subform in form %} {% if subform.is_presidency.value() %} @@ -129,7 +133,6 @@
{% for subform in form %} {% if subform.is_board.value() and not subform.is_presidency.value() %} @@ -150,7 +153,6 @@
{% for subform in form %} {% if not subform.is_board.value() %} diff --git a/club/tests/test_clubrole.py b/club/tests/test_clubrole.py index 6956ac8c..e090f7e6 100644 --- a/club/tests/test_clubrole.py +++ b/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 diff --git a/club/views.py b/club/views.py index 7c893c55..1bb51d6a 100644 --- a/club/views.py +++ b/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 True - club: Club = self.get_object() - return club.members.filter( - user=self.request.user, role__is_presidency=True - ).exists() + return self.club.can_roles_be_edited_by(self.request.user) 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):