diff --git a/club/forms.py b/club/forms.py index 0c0f6294..b9b4baaa 100644 --- a/club/forms.py +++ b/club/forms.py @@ -267,8 +267,6 @@ class ClubMemberForm(forms.ModelForm): def clean(self): """Check user rights for adding a user.""" cleaned_data = super().clean() - if "role" not in cleaned_data: - return cleaned_data if ( self.request_user_membership is None or self.request_user_membership.role <= settings.SITH_MAXIMUM_FREE_ROLE @@ -280,6 +278,4 @@ class ClubMemberForm(forms.ModelForm): ), code="invalid", ) - if cleaned_data["role"] > self.max_available_role: - raise forms.ValidationError(_("You do not have the permission to do that")) return cleaned_data diff --git a/club/models.py b/club/models.py index 4746bb6a..3c0f720f 100644 --- a/club/models.py +++ b/club/models.py @@ -253,35 +253,38 @@ class MembershipQuerySet(models.QuerySet): """Filter Memberships that this user can edit. Users with the `club.change_membership` permission can edit all Membership. - The other users can end : + The other users can edit : - their own membership - - if they are board members, memberships with a role lower than their own + - if they are board members, ongoing memberships with a role lower than their own For example, let's suppose the following users : - A : board member - B : board member - C : simple member - D : curious + - E : old member - A will be able to end the memberships of A, C and D ; - C and D will be able to end only their own membership. + A will be able to edit the memberships of A, C and D ; + C and D will be able to edit only their own membership ; + nobody will be able to edit E's membership. """ if user.has_perm("club.change_membership"): return self.all() return self.filter( - Exists( + Q(user=user) + | Exists( Membership.objects.filter( Q( role__gt=Greatest( OuterRef("role"), Value(settings.SITH_MAXIMUM_FREE_ROLE) ) - ) - | Q(pk=OuterRef("pk")), + ), user=user, end_date=None, club=OuterRef("club"), ) - ) + ), + end_date=None, ) def update(self, **kwargs) -> int: diff --git a/club/tests/test_membership.py b/club/tests/test_membership.py index 1e8b7dfd..f8ef2b6d 100644 --- a/club/tests/test_membership.py +++ b/club/tests/test_membership.py @@ -1,3 +1,5 @@ +from datetime import timedelta + from bs4 import BeautifulSoup from django.conf import settings from django.contrib.auth.models import Permission @@ -368,6 +370,44 @@ class TestMembership(TestClub): assert not form.is_valid() assert form.errors == {"role": ["Ce champ est obligatoire."]} + def test_add_member_already_there(self): + form = ClubMemberForm( + data={"user": self.simple_board_member, "role": 3}, + request_user=self.root, + club=self.club, + ) + assert not form.is_valid() + assert form.errors == { + "user": ["Vous ne pouvez pas ajouter deux fois le même utilisateur"] + } + + def test_add_other_member_forbidden(self): + non_member = subscriber_user.make() + simple_member = baker.make(Membership, club=self.club, role=1).user + for user in non_member, simple_member: + form = ClubMemberForm( + data={"user": subscriber_user.make(), "role": 1}, + request_user=user, + club=self.club, + ) + assert not form.is_valid() + assert form.errors == { + "__all__": [ + "Vous ne pouvez pas ajouter d'autres utilisateurs " + "dans un club si vous ne faites pas partie de son bureau." + ] + } + + def test_simple_members_dont_see_form_anymore(self): + """Test that simple club members don't see the form to add members""" + user = subscriber_user.make() + baker.make(Membership, club=self.club, user=user, role=1) + self.client.force_login(user) + res = self.client.get(self.members_url) + assert res.status_code == 200 + soup = BeautifulSoup(res.text, "lxml") + assert not soup.find(id="add_club_members_form") + def test_end_membership_self(self): """Test that a member can end its own membership.""" self.client.force_login(self.simple_board_member) @@ -491,3 +531,34 @@ class TestMembership(TestClub): new_board = set(self.club.board_group.users.values_list("id", flat=True)) assert new_members == initial_members assert new_board == initial_board + + +class TestOldMembersView(TestCase): + @classmethod + def setUpTestData(cls): + club = baker.make(Club) + roles = [1, 1, 1, 2, 2, 4, 4, 5, 7, 9, 10] + cls.memberships = baker.make( + Membership, + role=iter(roles), + club=club, + start_date=now() - timedelta(days=14), + end_date=now() - timedelta(days=7), + _quantity=len(roles), + _bulk_create=True, + ) + cls.url = reverse("club:club_old_members", kwargs={"club_id": club.id}) + + def test_ok(self): + user = subscriber_user.make() + self.client.force_login(user) + res = self.client.get(self.url) + assert res.status_code == 200 + + def test_access_forbidden(self): + res = self.client.get(self.url) + assertRedirects(res, reverse("core:login", query={"next": self.url})) + + self.client.force_login(baker.make(User)) + res = self.client.get(self.url) + assert res.status_code == 403 diff --git a/club/views.py b/club/views.py index c32014eb..c0e496d4 100644 --- a/club/views.py +++ b/club/views.py @@ -274,9 +274,6 @@ class ClubAddMembersFragment( success_message = _("%(user)s has been added to club.") def dispatch(self, *args, **kwargs): - club_id = self.kwargs.get("club_id") - if not club_id: - raise Http404 self.club = get_object_or_404(Club, pk=kwargs.get("club_id")) return super().dispatch(*args, **kwargs) @@ -316,6 +313,8 @@ class ClubMembersView( and membership.role <= settings.SITH_MAXIMUM_FREE_ROLE and not self.request.user.has_perm("club.add_membership") ): + # Simple club members won't see the form anymore. + # Even if they saw it, they couldn't add anyone to the club anyway return {} return {"add_member_fragment": ClubAddMembersFragment}