fix club members tests

club/forms.py

@@ -196,9 +196,7 @@ class ClubMemberForm(forms.Form):
         self.request_user = kwargs.pop("request_user")
         self.club_members = kwargs.pop("club_members", None)
         if not self.club_members:
-            self.club_members = (
+            self.club_members = self.club.members.ongoing().order_by("-role").all()
-                self.club.members.filter(end_date=None).order_by("-role").all()
-            )
         self.request_user_membership = self.club.get_membership_for(self.request_user)
         super().__init__(*args, **kwargs)
 

club/templates/club/club_members.jinja

@@ -11,7 +11,7 @@
         {{ select_all_checkbox("users_old") }}
         <p></p>
       {% endif %}
-      <table>
+      <table id="club_members_table">
         <thead>
           <tr>
             <td>{% trans %}User{% endtrans %}</td>

club/tests/base.py

@@ -6,8 +6,10 @@ from django.test import TestCase
 from django.urls import reverse
 from django.utils.timezone import now
 from model_bakery import baker
+from model_bakery.recipe import Recipe
 
 from club.models import Club, Membership
+from core.baker_recipes import old_subscriber_user, subscriber_user
 from core.models import User
 
 
@@ -17,8 +19,9 @@ class TestClub(TestCase):
     The generated dataset is the one created by the populate command,
     plus the following modifications :
 
-    - `self.club` is a dummy club recreated for each test
+    - `self.club` is a dummy club
-    - `self.club` has two board members : skia (role 3) and comptable (role 10)
+    - `self.club` has two board members :
+       simple_board_member (role 3) and president (role 10)
     - `self.club` has one regular member : richard
     - `self.club` has one former member : sli (who had role 2)
     - None of the `self.club` members are in the AE club.
@@ -27,44 +30,30 @@ class TestClub(TestCase):
     @classmethod
     def setUpTestData(cls):
         # subscribed users - initial members
-        cls.skia = User.objects.get(username="skia")
+        cls.president, cls.simple_board_member = subscriber_user.make(_quantity=2)
-        # by default, Skia is in the AE, which creates side effect
-        cls.skia.memberships.all().delete()
         cls.richard = User.objects.get(username="rbatsbak")
-        cls.comptable = User.objects.get(username="comptable")
         cls.sli = User.objects.get(username="sli")
-        cls.root = User.objects.get(username="root")
+        cls.root = baker.make(User, is_superuser=True)
+        cls.old_subscriber = old_subscriber_user.make()
+        cls.public = baker.make(User)
 
-        # subscribed users - not initial members
+        # subscribed users - not initial member
         cls.krophil = User.objects.get(username="krophil")
-        cls.subscriber = User.objects.get(username="subscriber")
+        cls.subscriber = subscriber_user.make()
-
-        # old subscriber
-        cls.old_subscriber = User.objects.get(username="old_subscriber")
-
-        # not subscribed
-        cls.public = User.objects.get(username="public")
 
         cls.ae = Club.objects.get(pk=settings.SITH_MAIN_CLUB_ID)
         cls.club = baker.make(Club)
         cls.members_url = reverse("club:club_members", kwargs={"club_id": cls.club.id})
         a_month_ago = now() - timedelta(days=30)
         yesterday = now() - timedelta(days=1)
-        Membership.objects.create(
+        membership_recipe = Recipe(Membership, club=cls.club)
-            club=cls.club, user=cls.skia, start_date=a_month_ago, role=3
+        membership_recipe.make(
+            user=cls.simple_board_member, start_date=a_month_ago, role=3
         )
-        Membership.objects.create(club=cls.club, user=cls.richard, role=1)
+        membership_recipe.make(user=cls.richard, role=1)
-        Membership.objects.create(
+        membership_recipe.make(user=cls.president, start_date=a_month_ago, role=10)
-            club=cls.club, user=cls.comptable, start_date=a_month_ago, role=10
+        membership_recipe.make(  # sli was a member but isn't anymore
-        )
+            user=cls.sli, start_date=a_month_ago, end_date=yesterday, role=2
-
-        # sli was a member but isn't anymore
-        Membership.objects.create(
-            club=cls.club,
-            user=cls.sli,
-            start_date=a_month_ago,
-            end_date=yesterday,
-            role=2,
         )
 
     def setUp(self):

club/tests/test_membership.py

@@ -1,9 +1,12 @@
+from bs4 import BeautifulSoup
 from django.conf import settings
 from django.core.cache import cache
+from django.db.models import Max
 from django.urls import reverse
 from django.utils.timezone import localdate, localtime, now
 from model_bakery import baker
 
+from club.forms import ClubMemberForm
 from club.models import Membership
 from club.tests.base import TestClub
 from core.baker_recipes import subscriber_user
@@ -17,8 +20,8 @@ class TestMembershipQuerySet(TestClub):
         """
         current_members = list(self.club.members.ongoing().order_by("id"))
         expected = [
-            self.skia.memberships.get(club=self.club),
+            self.simple_board_member.memberships.get(club=self.club),
-            self.comptable.memberships.get(club=self.club),
+            self.president.memberships.get(club=self.club),
             self.richard.memberships.get(club=self.club),
         ]
         expected.sort(key=lambda i: i.id)
@@ -30,8 +33,8 @@ class TestMembershipQuerySet(TestClub):
         self.richard.memberships.filter(club=self.club).update(end_date=today)
         current_members = list(self.club.members.ongoing().order_by("id"))
         expected = [
-            self.skia.memberships.get(club=self.club),
+            self.simple_board_member.memberships.get(club=self.club),
-            self.comptable.memberships.get(club=self.club),
+            self.president.memberships.get(club=self.club),
         ]
         expected.sort(key=lambda i: i.id)
         assert current_members == expected
@@ -42,8 +45,8 @@ class TestMembershipQuerySet(TestClub):
         """
         board_members = list(self.club.members.board().order_by("id"))
         expected = [
-            self.skia.memberships.get(club=self.club),
+            self.simple_board_member.memberships.get(club=self.club),
-            self.comptable.memberships.get(club=self.club),
+            self.president.memberships.get(club=self.club),
             # sli is no more member, but he was in the board
             self.sli.memberships.get(club=self.club),
         ]
@@ -56,17 +59,17 @@ class TestMembershipQuerySet(TestClub):
         """
         members = list(self.club.members.ongoing().board().order_by("id"))
         expected = [
-            self.skia.memberships.get(club=self.club),
+            self.simple_board_member.memberships.get(club=self.club),
-            self.comptable.memberships.get(club=self.club),
+            self.president.memberships.get(club=self.club),
         ]
         expected.sort(key=lambda i: i.id)
         assert members == expected
 
     def test_update_invalidate_cache(self):
         """Test that the `update` queryset method properly invalidate cache."""
-        mem_skia = self.skia.memberships.get(club=self.club)
+        mem_skia = self.simple_board_member.memberships.get(club=self.club)
         cache.set(f"membership_{mem_skia.club_id}_{mem_skia.user_id}", mem_skia)
-        self.skia.memberships.update(end_date=localtime(now()).date())
+        self.simple_board_member.memberships.update(end_date=localtime(now()).date())
         assert (
             cache.get(f"membership_{mem_skia.club_id}_{mem_skia.user_id}")
             == "not_member"
@@ -104,14 +107,14 @@ class TestMembershipQuerySet(TestClub):
 
     def test_delete_invalidate_cache(self):
         """Test that the `delete` queryset properly invalidate cache."""
-        mem_skia = self.skia.memberships.get(club=self.club)
+        mem_skia = self.simple_board_member.memberships.get(club=self.club)
-        mem_comptable = self.comptable.memberships.get(club=self.club)
+        mem_comptable = self.president.memberships.get(club=self.club)
         cache.set(f"membership_{mem_skia.club_id}_{mem_skia.user_id}", mem_skia)
         cache.set(
             f"membership_{mem_comptable.club_id}_{mem_comptable.user_id}", mem_comptable
         )
 
-        # should delete the subscriptions of skia and comptable
+        # should delete the subscriptions of simple_board_member and president
         self.club.members.ongoing().board().delete()
 
         for membership in (mem_skia, mem_comptable):
@@ -167,36 +170,43 @@ class TestMembership(TestClub):
         """Test that a GET request return a page where the requested
         information are displayed.
         """
-        self.client.force_login(self.skia)
+        self.client.force_login(self.simple_board_member)
         response = self.client.get(self.members_url)
         assert response.status_code == 200
-        expected_html = (
+        soup = BeautifulSoup(response.text, "lxml")
-            "<table><thead><tr>"
+        table = soup.find("table", id="club_members_table")
-            "<td>Utilisateur</td><td>Rôle</td><td>Description</td>"
+        assert [r.text for r in table.find("thead").find_all("td")] == [
-            "<td>Depuis</td><td>Marquer comme ancien</td>"
+            "Utilisateur",
-            "</tr></thead><tbody>"
+            "Rôle",
-        )
+            "Description",
+            "Depuis",
+            "Marquer comme ancien",
+        ]
+        rows = table.find("tbody").find_all("tr")
         memberships = self.club.members.ongoing().order_by("-role")
-        input_id = 0
+        for row, membership in zip(
-        for membership in memberships.select_related("user"):
+            rows, memberships.select_related("user"), strict=False
+        ):
             user = membership.user
-            expected_html += (
+            user_url = reverse("core:user_profile", args=[user.id])
-                f'<tr><td><a href="{reverse("core:user_profile", args=[user.id])}">'
+            cols = row.find_all("td")
-                f"{user.get_display_name()}</a></td>"
+            user_link = cols[0].find("a")
-                f"<td>{settings.SITH_CLUB_ROLES[membership.role]}</td>"
+            assert user_link.attrs["href"] == user_url
-                f"<td>{membership.description}</td>"
+            assert user_link.text == user.get_display_name()
-                f"<td>{membership.start_date}</td><td>"
+            assert cols[1].text == settings.SITH_CLUB_ROLES[membership.role]
-            )
+            assert cols[2].text == membership.description
-            if membership.role <= 3:  # 3 is the role of skia
+            assert cols[3].text == str(membership.start_date)
-                expected_html += (
+
-                    '<input type="checkbox" name="users_old" '
+            if membership.role <= 3:  # 3 is the role of simple_board_member
-                    f'value="{user.id}" '
+                form_input = cols[4].find("input")
-                    f'id="id_users_old_{input_id}">'
+                expected_attrs = {
-                )
+                    "type": "checkbox",
-                input_id += 1
+                    "name": "users_old",
-            expected_html += "</td></tr>"
+                    "value": str(user.id),
-        expected_html += "</tbody></table>"
+                }
-        self.assertInHTML(expected_html, response.content.decode())
+                assert form_input.attrs.items() >= expected_attrs.items()
+            else:
+                assert cols[4].find_all() == []
 
     def test_root_add_one_club_member(self):
         """Test that root users can add members to clubs, one at a time."""
@@ -228,64 +238,61 @@ class TestMembership(TestClub):
         """Test that users who are not currently subscribed
         cannot be members of clubs.
         """
-        self.client.force_login(self.root)
+        for user in self.public, self.old_subscriber:
-        response = self.client.post(
+            form = ClubMemberForm(
-            self.members_url,
+                data={"users": [user.id], "role": 1},
-            {"users": self.public.id, "role": 1},
+                request_user=self.root,
-        )
+                club=self.club,
-        assert not self.public.memberships.filter(club=self.club).exists()
+            )
-        assert '<ul class="errorlist"><li>' in response.content.decode()
 
-        response = self.client.post(
+            assert not form.is_valid()
-            self.members_url,
+            assert form.errors == {
-            {"users": self.old_subscriber.id, "role": 1},
+                "users": [
-        )
+                    "L'utilisateur doit être cotisant pour faire partie d'un club"
-        assert not self.public.memberships.filter(club=self.club).exists()
+                ]
-        assert self.club.get_membership_for(self.public) is None
+            }
-        assert '<ul class="errorlist"><li>' in response.content.decode()
 
     def test_add_members_already_members(self):
         """Test that users who are already members of a club
         cannot be added again to this club.
         """
         self.client.force_login(self.root)
-        current_membership = self.skia.memberships.ongoing().get(club=self.club)
+        current_membership = self.simple_board_member.memberships.ongoing().get(
-        nb_memberships = self.skia.memberships.count()
+            club=self.club
+        )
+        nb_memberships = self.simple_board_member.memberships.count()
         self.client.post(
             self.members_url,
-            {"users": self.skia.id, "role": current_membership.role + 1},
+            {"users": self.simple_board_member.id, "role": current_membership.role + 1},
+        )
+        self.simple_board_member.refresh_from_db()
+        assert nb_memberships == self.simple_board_member.memberships.count()
+        new_membership = self.simple_board_member.memberships.ongoing().get(
+            club=self.club
         )
-        self.skia.refresh_from_db()
-        assert nb_memberships == self.skia.memberships.count()
-        new_membership = self.skia.memberships.ongoing().get(club=self.club)
         assert current_membership == new_membership
-        assert self.club.get_membership_for(self.skia) == new_membership
+        assert self.club.get_membership_for(self.simple_board_member) == new_membership
 
     def test_add_not_existing_users(self):
         """Test that not existing users cannot be added in clubs.
         If one user in the request is invalid, no membership creation at all
         can take place.
         """
-        self.client.force_login(self.root)
         nb_memberships = self.club.members.count()
-        response = self.client.post(
+        max_id = User.objects.aggregate(id=Max("id"))["id"]
-            self.members_url,
+        for members in [max_id + 1], [max_id + 1, self.subscriber.id]:
-            {"users": [9999], "role": 1},
+            form = ClubMemberForm(
-        )
+                data={"users": members, "role": 1},
-        assert response.status_code == 200
+                request_user=self.root,
-        assert '<ul class="errorlist"><li>' in response.content.decode()
+                club=self.club,
-        self.club.refresh_from_db()
+            )
-        assert self.club.members.count() == nb_memberships
+            assert not form.is_valid()
-        response = self.client.post(
+            assert form.errors == {
-            self.members_url,
+                "users": [
-            {
+                    "Sélectionnez un choix valide. "
-                "users": (self.subscriber.id, 9999),
+                    f"{max_id + 1} n\u2019en fait pas partie."
-                "start_date": "12/06/2016",
+                ]
-                "role": 3,
+            }
-            },
-        )
-        assert response.status_code == 200
-        assert '<ul class="errorlist"><li>' in response.content.decode()
         self.club.refresh_from_db()
         assert self.club.members.count() == nb_memberships
 
@@ -310,17 +317,17 @@ class TestMembership(TestClub):
         """Test that a member of the club member cannot create
         a membership with a greater role than its own.
         """
-        self.client.force_login(self.skia)
+        form = ClubMemberForm(
+            data={"users": [self.subscriber.id], "role": 10},
+            request_user=self.simple_board_member,
+            club=self.club,
+        )
         nb_memberships = self.club.members.count()
-        response = self.client.post(
+
-            self.members_url,
+        assert not form.is_valid()
-            {"users": self.subscriber.id, "role": 10},
+        assert form.errors == {
-        )
+            "__all__": ["Vous n'avez pas la permission de faire cela"]
-        assert response.status_code == 200
+        }
-        self.assertInHTML(
-            "<li>Vous n'avez pas la permission de faire cela</li>",
-            response.content.decode(),
-        )
         self.club.refresh_from_db()
         assert nb_memberships == self.club.members.count()
         assert not self.subscriber.memberships.filter(club=self.club).exists()
@@ -328,31 +335,31 @@ class TestMembership(TestClub):
     def test_add_member_without_role(self):
         """Test that trying to add members without specifying their role fails."""
         self.client.force_login(self.root)
-        response = self.client.post(
+        form = ClubMemberForm(
-            self.members_url,
+            data={"users": [self.subscriber.id]},
-            {"users": self.subscriber.id, "start_date": "12/06/2016"},
+            request_user=self.simple_board_member,
-        )
+            club=self.club,
-        assert (
-            '<ul class="errorlist"><li>Vous devez choisir un r'
-            in response.content.decode()
         )
 
+        assert not form.is_valid()
+        assert form.errors == {"role": ["Vous devez choisir un rôle"]}
+
     def test_end_membership_self(self):
         """Test that a member can end its own membership."""
-        self.client.force_login(self.skia)
+        self.client.force_login(self.simple_board_member)
         self.client.post(
             self.members_url,
-            {"users_old": self.skia.id},
+            {"users_old": self.simple_board_member.id},
         )
-        self.skia.refresh_from_db()
+        self.simple_board_member.refresh_from_db()
-        self.assert_membership_ended_today(self.skia)
+        self.assert_membership_ended_today(self.simple_board_member)
 
     def test_end_membership_lower_role(self):
         """Test that board members of the club can end memberships
         of users with lower roles.
         """
-        # remainder : skia has role 3, comptable has role 10, richard has role 1
+        # remainder : simple_board_member has role 3, president has role 10, richard has role 1
-        self.client.force_login(self.skia)
+        self.client.force_login(self.simple_board_member)
         response = self.client.post(
             self.members_url,
             {"users_old": self.richard.id},
@@ -365,18 +372,18 @@ class TestMembership(TestClub):
         """Test that board members of the club cannot end memberships
         of users with higher roles.
         """
-        membership = self.comptable.memberships.filter(club=self.club).first()
+        membership = self.president.memberships.filter(club=self.club).first()
-        self.client.force_login(self.skia)
+        self.client.force_login(self.simple_board_member)
         self.client.post(
             self.members_url,
-            {"users_old": self.comptable.id},
+            {"users_old": self.president.id},
         )
         self.club.refresh_from_db()
-        new_membership = self.club.get_membership_for(self.comptable)
+        new_membership = self.club.get_membership_for(self.president)
         assert new_membership is not None
         assert new_membership == membership
 
-        membership = self.comptable.memberships.filter(club=self.club).first()
+        membership = self.president.memberships.filter(club=self.club).first()
         assert membership.end_date is None
 
     def test_end_membership_as_main_club_board(self):
@@ -391,10 +398,10 @@ class TestMembership(TestClub):
         self.client.force_login(subscriber)
         response = self.client.post(
             self.members_url,
-            {"users_old": self.comptable.id},
+            {"users_old": self.president.id},
         )
         self.assertRedirects(response, self.members_url)
-        self.assert_membership_ended_today(self.comptable)
+        self.assert_membership_ended_today(self.president)
         assert self.club.members.ongoing().count() == nb_memberships - 1
 
     def test_end_membership_as_root(self):
@@ -403,10 +410,10 @@ class TestMembership(TestClub):
         self.client.force_login(self.root)
         response = self.client.post(
             self.members_url,
-            {"users_old": [self.comptable.id]},
+            {"users_old": [self.president.id]},
         )
         self.assertRedirects(response, self.members_url)
-        self.assert_membership_ended_today(self.comptable)
+        self.assert_membership_ended_today(self.president)
         assert self.club.members.ongoing().count() == nb_memberships - 1
 
     def test_end_membership_as_foreigner(self):

club/views.py

@@ -37,6 +37,7 @@ from django.http import (
 from django.shortcuts import get_object_or_404, redirect
 from django.urls import reverse, reverse_lazy
 from django.utils import timezone
+from django.utils.functional import cached_property
 from django.utils.translation import gettext as _t
 from django.utils.translation import gettext_lazy as _
 from django.views.generic import DetailView, ListView, View
@@ -250,6 +251,10 @@ class ClubMembersView(ClubTabsMixin, CanViewMixin, DetailFormView):
     template_name = "club/club_members.jinja"
     current_tab = "members"
 
+    @cached_property
+    def members(self) -> list[Membership]:
+        return list(self.object.members.ongoing().order_by("-role"))
+
     def get_form_kwargs(self):
         kwargs = super().get_form_kwargs()
         kwargs["request_user"] = self.request.user
@@ -257,8 +262,8 @@ class ClubMembersView(ClubTabsMixin, CanViewMixin, DetailFormView):
         kwargs["club_members"] = self.members
         return kwargs
 
-    def get_context_data(self, *args, **kwargs):
+    def get_context_data(self, **kwargs):
-        kwargs = super().get_context_data(*args, **kwargs)
+        kwargs = super().get_context_data(**kwargs)
         kwargs["members"] = self.members
         return kwargs
 
@@ -277,12 +282,8 @@ class ClubMembersView(ClubTabsMixin, CanViewMixin, DetailFormView):
             membership.save()
         return resp
 
-    def dispatch(self, request, *args, **kwargs):
-        self.members = self.get_object().members.ongoing().order_by("-role")
-        return super().dispatch(request, *args, **kwargs)
-
     def get_success_url(self, **kwargs):
-        return reverse_lazy("club:club_members", kwargs={"club_id": self.object.id})
+        return self.request.path
 
 
 class ClubOldMembersView(ClubTabsMixin, CanViewMixin, DetailView):