Merge pull request #935 from ae-utbm/groups

Remove `RealGroup` and `MetaGroup`

club/admin.py

@@ -20,6 +20,14 @@ from club.models import Club, Membership
 @admin.register(Club)
 class ClubAdmin(admin.ModelAdmin):
     list_display = ("name", "unix_name", "parent", "is_active")
+    search_fields = ("name", "unix_name")
+    autocomplete_fields = (
+        "parent",
+        "board_group",
+        "members_group",
+        "home",
+        "page",
+    )
 
 
 @admin.register(Membership)

club/migrations/0012_club_board_group_club_members_group.py

@@ -0,0 +1,129 @@
+# Generated by Django 4.2.16 on 2024-11-20 17:08
+
+import django.db.models.deletion
+import django.db.models.functions.datetime
+from django.conf import settings
+from django.db import migrations, models
+from django.db.migrations.state import StateApps
+from django.db.models import Q
+from django.utils.timezone import localdate
+
+
+def migrate_meta_groups(apps: StateApps, schema_editor):
+    """Attach the existing meta groups to the clubs.
+
+    Until now, the meta groups were not attached to the clubs,
+    nor to the users.
+    This creates actual foreign relationships between the clubs
+    and theirs groups and the users and theirs groups.
+
+    Warnings:
+        When the meta groups associated with the clubs aren't found,
+        they are created.
+        Thus the migration shouldn't fail, and all the clubs will
+        have their groups.
+        However, there will probably be some groups that have
+        not been found but exist nonetheless,
+        so there will be duplicates and dangling groups.
+        There must be a manual cleanup after this migration.
+    """
+    Group = apps.get_model("core", "Group")
+    Club = apps.get_model("club", "Club")
+
+    meta_groups = Group.objects.filter(is_meta=True)
+    clubs = list(Club.objects.all())
+    for club in clubs:
+        club.board_group = meta_groups.get_or_create(
+            name=club.unix_name + settings.SITH_BOARD_SUFFIX
+        )[0]
+        club.members_group = meta_groups.get_or_create(
+            name=club.unix_name + settings.SITH_MEMBER_SUFFIX
+        )[0]
+        club.save()
+        club.refresh_from_db()
+        memberships = club.members.filter(
+            Q(end_date=None) | Q(end_date__gt=localdate())
+        ).select_related("user")
+        club.members_group.users.set([m.user for m in memberships])
+        club.board_group.users.set(
+            [
+                m.user
+                for m in memberships.filter(role__gt=settings.SITH_MAXIMUM_FREE_ROLE)
+            ]
+        )
+
+
+# steps of the migration :
+# - Create a nullable field for the board group and the member group
+# - Edit those new fields to make them point to currently existing meta groups
+# - When this data migration is done, make the fields non-nullable
+class Migration(migrations.Migration):
+    dependencies = [
+        ("core", "0040_alter_user_options_user_user_permissions_and_more"),
+        ("club", "0011_auto_20180426_2013"),
+    ]
+
+    operations = [
+        migrations.RemoveField(
+            model_name="club",
+            name="edit_groups",
+        ),
+        migrations.RemoveField(
+            model_name="club",
+            name="owner_group",
+        ),
+        migrations.RemoveField(
+            model_name="club",
+            name="view_groups",
+        ),
+        migrations.AddField(
+            model_name="club",
+            name="board_group",
+            field=models.OneToOneField(
+                blank=True,
+                null=True,
+                on_delete=django.db.models.deletion.PROTECT,
+                related_name="club_board",
+                to="core.group",
+            ),
+        ),
+        migrations.AddField(
+            model_name="club",
+            name="members_group",
+            field=models.OneToOneField(
+                blank=True,
+                null=True,
+                on_delete=django.db.models.deletion.PROTECT,
+                related_name="club",
+                to="core.group",
+            ),
+        ),
+        migrations.RunPython(
+            migrate_meta_groups, reverse_code=migrations.RunPython.noop, elidable=True
+        ),
+        migrations.AlterField(
+            model_name="club",
+            name="board_group",
+            field=models.OneToOneField(
+                on_delete=django.db.models.deletion.CASCADE,
+                related_name="club_board",
+                to="core.group",
+            ),
+        ),
+        migrations.AlterField(
+            model_name="club",
+            name="members_group",
+            field=models.OneToOneField(
+                on_delete=django.db.models.deletion.CASCADE,
+                related_name="club",
+                to="core.group",
+            ),
+        ),
+        migrations.AddConstraint(
+            model_name="membership",
+            constraint=models.CheckConstraint(
+                check=models.Q(("end_date__gte", models.F("start_date"))),
+                name="end_after_start",
+            ),
+        ),
+    ]

club/models.py

@@ -23,7 +23,7 @@
 #
 from __future__ import annotations
 
-from typing import Self
+from typing import Iterable, Self
 
 from django.conf import settings
 from django.core import validators
@@ -31,14 +31,14 @@ from django.core.cache import cache
 from django.core.exceptions import ObjectDoesNotExist, ValidationError
 from django.core.validators import RegexValidator, validate_email
 from django.db import models, transaction
-from django.db.models import Exists, OuterRef, Q
+from django.db.models import Exists, F, OuterRef, Q
 from django.urls import reverse
 from django.utils import timezone
 from django.utils.functional import cached_property
 from django.utils.timezone import localdate
 from django.utils.translation import gettext_lazy as _
 
-from core.models import Group, MetaGroup, Notification, Page, SithFile, User
+from core.models import Group, Notification, Page, SithFile, User
 
 # Create your models here.
 
@@ -79,19 +79,6 @@ class Club(models.Model):
         _("short description"), max_length=1000, default="", blank=True, null=True
     )
     address = models.CharField(_("address"), max_length=254)
-
-    owner_group = models.ForeignKey(
-        Group,
-        related_name="owned_club",
-        default=get_default_owner_group,
-        on_delete=models.CASCADE,
-    )
-    edit_groups = models.ManyToManyField(
-        Group, related_name="editable_club", blank=True
-    )
-    view_groups = models.ManyToManyField(
-        Group, related_name="viewable_club", blank=True
-    )
     home = models.OneToOneField(
         SithFile,
         related_name="home_of_club",
@@ -103,6 +90,12 @@ class Club(models.Model):
     page = models.OneToOneField(
         Page, related_name="club", blank=True, null=True, on_delete=models.CASCADE
     )
+    members_group = models.OneToOneField(
+        Group, related_name="club", on_delete=models.PROTECT
+    )
+    board_group = models.OneToOneField(
+        Group, related_name="club_board", on_delete=models.PROTECT
+    )
 
     class Meta:
         ordering = ["name", "unix_name"]
@@ -112,23 +105,27 @@ class Club(models.Model):
 
     @transaction.atomic()
     def save(self, *args, **kwargs):
-        old = Club.objects.filter(id=self.id).first()
+        creation = self._state.adding
-        creation = old is None
+        if not creation:
-        if not creation and old.unix_name != self.unix_name:
+            db_club = Club.objects.get(id=self.id)
-            self._change_unixname(self.unix_name)
+            if self.unix_name != db_club.unix_name:
+                self.home.name = self.unix_name
+                self.home.save()
+            if self.name != db_club.name:
+                self.board_group.name = f"{self.name} - Bureau"
+                self.board_group.save()
+                self.members_group.name = f"{self.name} - Membres"
+                self.members_group.save()
+        if creation:
+            self.board_group = Group.objects.create(
+                name=f"{self.name} - Bureau", is_manually_manageable=False
+            )
+            self.members_group = Group.objects.create(
+                name=f"{self.name} - Membres", is_manually_manageable=False
+            )
         super().save(*args, **kwargs)
         if creation:
-            board = MetaGroup(name=self.unix_name + settings.SITH_BOARD_SUFFIX)
-            board.save()
-            member = MetaGroup(name=self.unix_name + settings.SITH_MEMBER_SUFFIX)
-            member.save()
-            subscribers = Group.objects.filter(
-                name=settings.SITH_MAIN_MEMBERS_GROUP
-            ).first()
             self.make_home()
-            self.home.edit_groups.set([board])
-            self.home.view_groups.set([member, subscribers])
-            self.home.save()
         self.make_page()
         cache.set(f"sith_club_{self.unix_name}", self)
 
@@ -136,7 +133,8 @@ class Club(models.Model):
         return reverse("club:club_view", kwargs={"club_id": self.id})
 
     @cached_property
-    def president(self):
+    def president(self) -> Membership | None:
+        """Fetch the membership of the current president of this club."""
         return self.members.filter(
             role=settings.SITH_CLUB_ROLES_ID["President"], end_date=None
         ).first()
@@ -154,27 +152,9 @@ class Club(models.Model):
     def clean(self):
         self.check_loop()
 
-    def _change_unixname(self, old_name, new_name):
+    def make_home(self) -> None:
-        c = Club.objects.filter(unix_name=new_name).first()
-        if c is None:
-            # Update all the groups names
-            Group.objects.filter(name=old_name).update(name=new_name)
-            Group.objects.filter(name=old_name + settings.SITH_BOARD_SUFFIX).update(
-                name=new_name + settings.SITH_BOARD_SUFFIX
-            )
-            Group.objects.filter(name=old_name + settings.SITH_MEMBER_SUFFIX).update(
-                name=new_name + settings.SITH_MEMBER_SUFFIX
-            )
-
         if self.home:
-                self.home.name = new_name
+            return
-                self.home.save()
-
-        else:
-            raise ValidationError(_("A club with that unix_name already exists"))
-
-    def make_home(self):
-        if not self.home:
         home_root = SithFile.objects.filter(parent=None, name="clubs").first()
         root = User.objects.filter(username="root").first()
         if home_root and root:
@@ -183,7 +163,7 @@ class Club(models.Model):
             self.home = home
             self.save()
 
-    def make_page(self):
+    def make_page(self) -> None:
         root = User.objects.filter(username="root").first()
         if not self.page:
             club_root = Page.objects.filter(name=settings.SITH_CLUB_ROOT_PAGE).first()
@@ -213,35 +193,34 @@ class Club(models.Model):
             self.page.parent = self.parent.page
             self.page.save(force_lock=True)
 
-    def delete(self, *args, **kwargs):
+    def delete(self, *args, **kwargs) -> tuple[int, dict[str, int]]:
         # Invalidate the cache of this club and of its memberships
         for membership in self.members.ongoing().select_related("user"):
             cache.delete(f"membership_{self.id}_{membership.user.id}")
         cache.delete(f"sith_club_{self.unix_name}")
-        super().delete(*args, **kwargs)
+        self.board_group.delete()
+        self.members_group.delete()
+        return super().delete(*args, **kwargs)
 
-    def get_display_name(self):
+    def get_display_name(self) -> str:
         return self.name
 
-    def is_owned_by(self, user):
+    def is_owned_by(self, user: User) -> bool:
         """Method to see if that object can be super edited by the given user."""
         if user.is_anonymous:
             return False
-        return user.is_board_member
+        return user.is_root or user.is_board_member
 
-    def get_full_logo_url(self):
+    def get_full_logo_url(self) -> str:
-        return "https://%s%s" % (settings.SITH_URL, self.logo.url)
+        return f"https://{settings.SITH_URL}{self.logo.url}"
 
-    def can_be_edited_by(self, user):
+    def can_be_edited_by(self, user: User) -> bool:
         """Method to see if that object can be edited by the given user."""
         return self.has_rights_in_club(user)
 
-    def can_be_viewed_by(self, user):
+    def can_be_viewed_by(self, user: User) -> bool:
         """Method to see if that object can be seen by the given user."""
-        sub = User.objects.filter(pk=user.pk).first()
+        return user.was_subscribed
-        if sub is None:
-            return False
-        return sub.was_subscribed
 
     def get_membership_for(self, user: User) -> Membership | None:
         """Return the current membership the given user.
@@ -262,9 +241,8 @@ class Club(models.Model):
                 cache.set(f"membership_{self.id}_{user.id}", membership)
         return membership
 
-    def has_rights_in_club(self, user):
+    def has_rights_in_club(self, user: User) -> bool:
-        m = self.get_membership_for(user)
+        return user.is_in_group(pk=self.board_group_id)
-        return m is not None and m.role > settings.SITH_MAXIMUM_FREE_ROLE
 
 
 class MembershipQuerySet(models.QuerySet):
@@ -283,42 +261,65 @@ class MembershipQuerySet(models.QuerySet):
         """
         return self.filter(role__gt=settings.SITH_MAXIMUM_FREE_ROLE)
 
-    def update(self, **kwargs):
+    def update(self, **kwargs) -> int:
-        """Refresh the cache for the elements of the queryset.
+        """Refresh the cache and edit group ownership.
 
-        Besides that, does the same job as a regular update method.
+        Update the cache, when necessary, remove
+        users from club groups they are no more in
+        and add them in the club groups they should be in.
 
-        Be aware that this adds a db query to retrieve the updated objects
+        Be aware that this adds three db queries :
+        one to retrieve the updated memberships,
+        one to perform group removal and one to perform
+        group attribution.
         """
         nb_rows = super().update(**kwargs)
-        if nb_rows > 0:
+        if nb_rows == 0:
-            # if at least a row was affected, refresh the cache
+            # if no row was affected, no need to refresh the cache
-            for membership in self.all():
+            return 0
-                if membership.end_date is not None:
-                    cache.set(
-                        f"membership_{membership.club_id}_{membership.user_id}",
-                        "not_member",
-                    )
-                else:
-                    cache.set(
-                        f"membership_{membership.club_id}_{membership.user_id}",
-                        membership,
-                    )
 
-    def delete(self):
+        cache_memberships = {}
+        memberships = set(self.select_related("club"))
+        # delete all User-Group relations and recreate the necessary ones
+        # It's more concise to write and more reliable
+        Membership._remove_club_groups(memberships)
+        Membership._add_club_groups(memberships)
+        for member in memberships:
+            cache_key = f"membership_{member.club_id}_{member.user_id}"
+            if member.end_date is None:
+                cache_memberships[cache_key] = member
+            else:
+                cache_memberships[cache_key] = "not_member"
+        cache.set_many(cache_memberships)
+        return nb_rows
+
+    def delete(self) -> tuple[int, dict[str, int]]:
         """Work just like the default Django's delete() method,
         but add a cache invalidation for the elements of the queryset
-        before the deletion.
+        before the deletion,
+        and a removal of the user from the club groups.
 
-        Be aware that this adds a db query to retrieve the deleted element.
+        Be aware that this adds some db queries :
-        As this first query take place before the deletion operation,
+
-        it will be performed even if the deletion fails.
+        - 1 to retrieve the deleted elements in order to perform
+          post-delete operations.
+          As we can't know if a delete will affect rows or not,
+          this query will always happen
+        - 1 query to remove the users from the club groups.
+          If the delete operation affected no row,
+          this query won't happen.
         """
-        ids = list(self.values_list("club_id", "user_id"))
+        memberships = set(self.all())
-        nb_rows, _ = super().delete()
+        nb_rows, rows_counts = super().delete()
         if nb_rows > 0:
-            for club_id, user_id in ids:
+            Membership._remove_club_groups(memberships)
-                cache.set(f"membership_{club_id}_{user_id}", "not_member")
+            cache.set_many(
+                {
+                    f"membership_{m.club_id}_{m.user_id}": "not_member"
+                    for m in memberships
+                }
+            )
+        return nb_rows, rows_counts
 
 
 class Membership(models.Model):
@@ -361,6 +362,13 @@ class Membership(models.Model):
 
     objects = MembershipQuerySet.as_manager()
 
+    class Meta:
+        constraints = [
+            models.CheckConstraint(
+                check=Q(end_date__gte=F("start_date")), name="end_after_start"
+            ),
+        ]
+
     def __str__(self):
         return (
             f"{self.club.name} - {self.user.username} "
@@ -370,7 +378,14 @@ class Membership(models.Model):
 
     def save(self, *args, **kwargs):
         super().save(*args, **kwargs)
+        # a save may either be an update or a creation
+        # and may result in either an ongoing or an ended membership.
+        # It could also be a retrogradation from the board to being a simple member.
+        # To avoid problems, the user is removed from the club groups beforehand ;
+        # he will be added back if necessary
+        self._remove_club_groups([self])
         if self.end_date is None:
+            self._add_club_groups([self])
             cache.set(f"membership_{self.club_id}_{self.user_id}", self)
         else:
             cache.set(f"membership_{self.club_id}_{self.user_id}", "not_member")
@@ -378,11 +393,11 @@ class Membership(models.Model):
     def get_absolute_url(self):
         return reverse("club:club_members", kwargs={"club_id": self.club_id})
 
-    def is_owned_by(self, user):
+    def is_owned_by(self, user: User) -> bool:
         """Method to see if that object can be super edited by the given user."""
         if user.is_anonymous:
             return False
-        return user.is_board_member
+        return user.is_root or user.is_board_member
 
     def can_be_edited_by(self, user: User) -> bool:
         """Check if that object can be edited by the given user."""
@@ -392,9 +407,91 @@ class Membership(models.Model):
         return membership is not None and membership.role >= self.role
 
     def delete(self, *args, **kwargs):
+        self._remove_club_groups([self])
         super().delete(*args, **kwargs)
         cache.delete(f"membership_{self.club_id}_{self.user_id}")
 
+    @staticmethod
+    def _remove_club_groups(
+        memberships: Iterable[Membership],
+    ) -> tuple[int, dict[str, int]]:
+        """Remove users of those memberships from the club groups.
+
+        For example, if a user is in the Troll club board,
+        he is in the board group and the members group of the Troll.
+        After calling this function, he will be in neither.
+
+        Returns:
+            The result of the deletion queryset.
+
+        Warnings:
+            If this function isn't used in combination
+            with an actual deletion of the memberships,
+            it will result in an inconsistent state,
+            where users will be in the clubs, without
+            having the associated rights.
+        """
+        clubs = {m.club_id for m in memberships}
+        users = {m.user_id for m in memberships}
+        groups = Group.objects.filter(Q(club__in=clubs) | Q(club_board__in=clubs))
+        return User.groups.through.objects.filter(
+            Q(group__in=groups) & Q(user__in=users)
+        ).delete()
+
+    @staticmethod
+    def _add_club_groups(
+        memberships: Iterable[Membership],
+    ) -> list[User.groups.through]:
+        """Add users of those memberships to the club groups.
+
+        For example, if a user just joined the Troll club board,
+        he will be added in both the members group and the board group
+        of the club.
+
+        Returns:
+            The created User-Group relations.
+
+        Warnings:
+            If this function isn't used in combination
+            with an actual update/creation of the memberships,
+            it will result in an inconsistent state,
+            where users will have the rights associated to the
+            club, without actually being part of it.
+        """
+        # only active membership (i.e. `end_date=None`)
+        # grant the attribution of club groups.
+        memberships = [m for m in memberships if m.end_date is None]
+        if not memberships:
+            return []
+
+        if sum(1 for m in memberships if not hasattr(m, "club")) > 1:
+            # if more than one membership hasn't its `club` attribute set
+            # it's less expensive to reload the whole query with
+            # a select_related than perform a distinct query
+            # to fetch each club.
+            ids = {m.id for m in memberships}
+            memberships = list(
+                Membership.objects.filter(id__in=ids).select_related("club")
+            )
+        club_groups = []
+        for membership in memberships:
+            club_groups.append(
+                User.groups.through(
+                    user_id=membership.user_id,
+                    group_id=membership.club.members_group_id,
+                )
+            )
+            if membership.role > settings.SITH_MAXIMUM_FREE_ROLE:
+                club_groups.append(
+                    User.groups.through(
+                        user_id=membership.user_id,
+                        group_id=membership.club.board_group_id,
+                    )
+                )
+        return User.groups.through.objects.bulk_create(
+            club_groups, ignore_conflicts=True
+        )
+
 
 class Mailing(models.Model):
     """A Mailing list for a club.

club/tests.py

@@ -21,6 +21,7 @@ from django.urls import reverse
 from django.utils import timezone
 from django.utils.timezone import localdate, localtime, now
 from django.utils.translation import gettext as _
+from model_bakery import baker
 
 from club.forms import MailingForm
 from club.models import Club, Mailing, Membership
@@ -164,6 +165,27 @@ class TestMembershipQuerySet(TestClub):
         assert new_mem != "not_member"
         assert new_mem.role == 5
 
+    def test_update_change_club_groups(self):
+        """Test that `update` set the user groups accordingly."""
+        user = baker.make(User)
+        membership = baker.make(Membership, end_date=None, user=user, role=5)
+        members_group = membership.club.members_group
+        board_group = membership.club.board_group
+        assert user.groups.contains(members_group)
+        assert user.groups.contains(board_group)
+
+        user.memberships.update(role=1)  # from board to simple member
+        assert user.groups.contains(members_group)
+        assert not user.groups.contains(board_group)
+
+        user.memberships.update(role=5)  # from member to board
+        assert user.groups.contains(members_group)
+        assert user.groups.contains(board_group)
+
+        user.memberships.update(end_date=localdate())  # end the membership
+        assert not user.groups.contains(members_group)
+        assert not user.groups.contains(board_group)
+
     def test_delete_invalidate_cache(self):
         """Test that the `delete` queryset properly invalidate cache."""
         mem_skia = self.skia.memberships.get(club=self.club)
@@ -182,6 +204,19 @@ class TestMembershipQuerySet(TestClub):
             )
             assert cached_mem == "not_member"
 
+    def test_delete_remove_from_groups(self):
+        """Test that `delete` removes from club groups"""
+        user = baker.make(User)
+        memberships = baker.make(Membership, role=iter([1, 5]), user=user, _quantity=2)
+        club_groups = {
+            memberships[0].club.members_group,
+            memberships[1].club.members_group,
+            memberships[1].club.board_group,
+        }
+        assert set(user.groups.all()) == club_groups
+        user.memberships.all().delete()
+        assert user.groups.all().count() == 0
+
 
 class TestClubModel(TestClub):
     def assert_membership_started_today(self, user: User, role: int):
@@ -192,10 +227,8 @@ class TestClubModel(TestClub):
         assert membership.end_date is None
         assert membership.role == role
         assert membership.club.get_membership_for(user) == membership
-        member_group = self.club.unix_name + settings.SITH_MEMBER_SUFFIX
+        assert user.is_in_group(pk=self.club.members_group_id)
-        board_group = self.club.unix_name + settings.SITH_BOARD_SUFFIX
+        assert user.is_in_group(pk=self.club.board_group_id)
-        assert user.is_in_group(name=member_group)
-        assert user.is_in_group(name=board_group)
 
     def assert_membership_ended_today(self, user: User):
         """Assert that the given user have a membership which ended today."""
@@ -474,37 +507,35 @@ class TestClubModel(TestClub):
         assert self.club.members.count() == nb_memberships
         assert membership == new_mem
 
-    def test_delete_remove_from_meta_group(self):
+    def test_remove_from_club_group(self):
-        """Test that when a club is deleted, all its members are removed from the
+        """Test that when a membership ends, the user is removed from club groups."""
-        associated metagroup.
+        user = baker.make(User)
-        """
+        baker.make(Membership, user=user, club=self.club, end_date=None, role=3)
-        memberships = self.club.members.select_related("user")
+        assert user.groups.contains(self.club.members_group)
-        users = [membership.user for membership in memberships]
+        assert user.groups.contains(self.club.board_group)
-        meta_group = self.club.unix_name + settings.SITH_MEMBER_SUFFIX
+        user.memberships.update(end_date=localdate())
+        assert not user.groups.contains(self.club.members_group)
+        assert not user.groups.contains(self.club.board_group)
 
-        self.club.delete()
+    def test_add_to_club_group(self):
-        for user in users:
+        """Test that when a membership begins, the user is added to the club group."""
-            assert not user.is_in_group(name=meta_group)
+        assert not self.subscriber.groups.contains(self.club.members_group)
+        assert not self.subscriber.groups.contains(self.club.board_group)
+        baker.make(Membership, club=self.club, user=self.subscriber, role=3)
+        assert self.subscriber.groups.contains(self.club.members_group)
+        assert self.subscriber.groups.contains(self.club.board_group)
 
-    def test_add_to_meta_group(self):
+    def test_change_position_in_club(self):
-        """Test that when a membership begins, the user is added to the meta group."""
+        """Test that when moving from board to members, club group change"""
-        group_members = self.club.unix_name + settings.SITH_MEMBER_SUFFIX
+        membership = baker.make(
-        board_members = self.club.unix_name + settings.SITH_BOARD_SUFFIX
+            Membership, club=self.club, user=self.subscriber, role=3
-        assert not self.subscriber.is_in_group(name=group_members)
+        )
-        assert not self.subscriber.is_in_group(name=board_members)
+        assert self.subscriber.groups.contains(self.club.members_group)
-        Membership.objects.create(club=self.club, user=self.subscriber, role=3)
+        assert self.subscriber.groups.contains(self.club.board_group)
-        assert self.subscriber.is_in_group(name=group_members)
+        membership.role = 1
-        assert self.subscriber.is_in_group(name=board_members)
+        membership.save()
-
+        assert self.subscriber.groups.contains(self.club.members_group)
-    def test_remove_from_meta_group(self):
+        assert not self.subscriber.groups.contains(self.club.board_group)
-        """Test that when a membership ends, the user is removed from meta group."""
-        group_members = self.club.unix_name + settings.SITH_MEMBER_SUFFIX
-        board_members = self.club.unix_name + settings.SITH_BOARD_SUFFIX
-        assert self.comptable.is_in_group(name=group_members)
-        assert self.comptable.is_in_group(name=board_members)
-        self.comptable.memberships.update(end_date=localtime(now()))
-        assert not self.comptable.is_in_group(name=group_members)
-        assert not self.comptable.is_in_group(name=board_members)
 
     def test_club_owner(self):
         """Test that a club is owned only by board members of the main club."""
@@ -517,6 +548,26 @@ class TestClubModel(TestClub):
         Membership(club=self.ae, user=self.sli, role=3).save()
         assert self.club.is_owned_by(self.sli)
 
+    def test_change_club_name(self):
+        """Test that changing the club name doesn't break things."""
+        members_group = self.club.members_group
+        board_group = self.club.board_group
+        initial_members = set(members_group.users.values_list("id", flat=True))
+        initial_board = set(board_group.users.values_list("id", flat=True))
+        self.club.name = "something else"
+        self.club.save()
+        self.club.refresh_from_db()
+
+        # The names should have changed, but not the ids nor the group members
+        assert self.club.members_group.name == "something else - Membres"
+        assert self.club.board_group.name == "something else - Bureau"
+        assert self.club.members_group.id == members_group.id
+        assert self.club.board_group.id == board_group.id
+        new_members = set(self.club.members_group.users.values_list("id", flat=True))
+        new_board = set(self.club.board_group.users.values_list("id", flat=True))
+        assert new_members == initial_members
+        assert new_board == initial_board
+
 
 class TestMailingForm(TestCase):
     """Perform validation tests for MailingForm."""

club/views.py

@@ -71,14 +71,13 @@ class ClubTabsMixin(TabedViewMixin):
         return self.object.get_display_name()
 
     def get_list_of_tabs(self):
-        tab_list = []
+        tab_list = [
-        tab_list.append(
             {
                 "url": reverse("club:club_view", kwargs={"club_id": self.object.id}),
                 "slug": "infos",
                 "name": _("Infos"),
             }
-        )
+        ]
         if self.request.user.can_view(self.object):
             tab_list.append(
                 {

core/admin.py

@@ -15,6 +15,7 @@
 
 from django.contrib import admin
 from django.contrib.auth.models import Group as AuthGroup
+from django.contrib.auth.models import Permission
 
 from core.models import Group, OperationLog, Page, SithFile, User
 
@@ -23,9 +24,10 @@ admin.site.unregister(AuthGroup)
 
 @admin.register(Group)
 class GroupAdmin(admin.ModelAdmin):
-    list_display = ("name", "description", "is_meta")
+    list_display = ("name", "description", "is_manually_manageable")
-    list_filter = ("is_meta",)
+    list_filter = ("is_manually_manageable",)
     search_fields = ("name",)
+    autocomplete_fields = ("permissions",)
 
 
 @admin.register(User)
@@ -37,10 +39,17 @@ class UserAdmin(admin.ModelAdmin):
         "profile_pict",
         "avatar_pict",
         "scrub_pict",
+        "user_permissions",
+        "groups",
     )
     search_fields = ["first_name", "last_name", "username"]
 
 
+@admin.register(Permission)
+class PermissionAdmin(admin.ModelAdmin):
+    search_fields = ("codename",)
+
+
 @admin.register(Page)
 class PageAdmin(admin.ModelAdmin):
     list_display = ("name", "_full_name", "owner_group")

core/baker_recipes.py

@@ -7,7 +7,7 @@ from model_bakery import seq
 from model_bakery.recipe import Recipe, related
 
 from club.models import Membership
-from core.models import User
+from core.models import Group, User
 from subscription.models import Subscription
 
 active_subscription = Recipe(
@@ -60,5 +60,6 @@ board_user = Recipe(
     first_name="AE",
     last_name=seq("member "),
     memberships=related(ae_board_membership),
+    groups=lambda: [Group.objects.get(club_board=settings.SITH_MAIN_CLUB_ID)],
 )
 """A user which is in the board of the AE."""

core/management/commands/populate.py

@@ -47,7 +47,7 @@ from accounting.models import (
 )
 from club.models import Club, Membership
 from com.models import News, NewsDate, Sith, Weekmail
-from core.models import Group, Page, PageRev, RealGroup, SithFile, User
+from core.models import Group, Page, PageRev, SithFile, User
 from core.utils import resize_image
 from counter.models import Counter, Product, ProductType, StudentCard
 from election.models import Candidature, Election, ElectionList, Role
@@ -143,7 +143,9 @@ class Command(BaseCommand):
         Counter.objects.bulk_create(counters)
         bar_groups = []
         for bar_id, bar_name in settings.SITH_COUNTER_BARS:
-            group = RealGroup.objects.create(name=f"{bar_name} admin")
+            group = Group.objects.create(
+                name=f"{bar_name} admin", is_manually_manageable=True
+            )
             bar_groups.append(
                 Counter.edit_groups.through(counter_id=bar_id, group=group)
             )
@@ -366,46 +368,42 @@ Welcome to the wiki page!
             parent=main_club,
         )
 
-        Membership.objects.bulk_create(
+        Membership.objects.create(user=skia, club=main_club, role=3)
-            [
+        Membership.objects.create(
-                Membership(user=skia, club=main_club, role=3),
-                Membership(
             user=comunity,
             club=bar_club,
             start_date=localdate(),
             role=settings.SITH_CLUB_ROLES_ID["Board member"],
-                ),
+        )
-                Membership(
+        Membership.objects.create(
             user=sli,
             club=troll,
             role=9,
             description="Padawan Troll",
             start_date=localdate() - timedelta(days=17),
-                ),
+        )
-                Membership(
+        Membership.objects.create(
             user=krophil,
             club=troll,
             role=10,
             description="Maitre Troll",
             start_date=localdate() - timedelta(days=200),
-                ),
+        )
-                Membership(
+        Membership.objects.create(
             user=skia,
             club=troll,
             role=2,
             description="Grand Ancien Troll",
             start_date=localdate() - timedelta(days=400),
             end_date=localdate() - timedelta(days=86),
-                ),
+        )
-                Membership(
+        Membership.objects.create(
             user=richard,
             club=troll,
             role=2,
             description="",
             start_date=localdate() - timedelta(days=200),
             end_date=localdate() - timedelta(days=100),
-                ),
-            ]
         )
 
         p = ProductType.objects.create(name="Bières bouteilles")
@@ -594,7 +592,6 @@ Welcome to the wiki page!
         )
 
         # Create an election
-        ae_board_group = Group.objects.get(name=settings.SITH_MAIN_BOARD_GROUP)
         el = Election.objects.create(
             title="Élection 2017",
             description="La roue tourne",
@@ -604,7 +601,7 @@ Welcome to the wiki page!
             end_date="7942-06-12 10:28:45+01",
         )
         el.view_groups.add(groups.public)
-        el.edit_groups.add(ae_board_group)
+        el.edit_groups.add(main_club.board_group)
         el.candidature_groups.add(groups.subscribers)
         el.vote_groups.add(groups.subscribers)
         liste = ElectionList.objects.create(title="Candidature Libre", election=el)
@@ -889,7 +886,7 @@ Welcome to the wiki page!
     def _create_groups(self) -> PopulatedGroups:
         perms = Permission.objects.all()
 
-        root_group = Group.objects.create(name="Root")
+        root_group = Group.objects.create(name="Root", is_manually_manageable=True)
         root_group.permissions.add(*list(perms.values_list("pk", flat=True)))
         # public has no permission.
         # Its purpose is not to link users to permissions,
@@ -911,7 +908,9 @@ Welcome to the wiki page!
                 )
             )
         )
-        accounting_admin = Group.objects.create(name="Accounting admin")
+        accounting_admin = Group.objects.create(
+            name="Accounting admin", is_manually_manageable=True
+        )
         accounting_admin.permissions.add(
             *list(
                 perms.filter(
@@ -931,13 +930,17 @@ Welcome to the wiki page!
                 ).values_list("pk", flat=True)
             )
         )
-        com_admin = Group.objects.create(name="Communication admin")
+        com_admin = Group.objects.create(
+            name="Communication admin", is_manually_manageable=True
+        )
         com_admin.permissions.add(
             *list(
                 perms.filter(content_type__app_label="com").values_list("pk", flat=True)
             )
         )
-        counter_admin = Group.objects.create(name="Counter admin")
+        counter_admin = Group.objects.create(
+            name="Counter admin", is_manually_manageable=True
+        )
         counter_admin.permissions.add(
             *list(
                 perms.filter(
@@ -946,16 +949,20 @@ Welcome to the wiki page!
                 )
             )
         )
-        Group.objects.create(name="Banned from buying alcohol")
+        Group.objects.create(
-        Group.objects.create(name="Banned from counters")
+            name="Banned from buying alcohol", is_manually_manageable=True
-        Group.objects.create(name="Banned to subscribe")
+        )
-        sas_admin = Group.objects.create(name="SAS admin")
+        Group.objects.create(name="Banned from counters", is_manually_manageable=True)
+        Group.objects.create(name="Banned to subscribe", is_manually_manageable=True)
+        sas_admin = Group.objects.create(name="SAS admin", is_manually_manageable=True)
         sas_admin.permissions.add(
             *list(
                 perms.filter(content_type__app_label="sas").values_list("pk", flat=True)
             )
         )
-        forum_admin = Group.objects.create(name="Forum admin")
+        forum_admin = Group.objects.create(
+            name="Forum admin", is_manually_manageable=True
+        )
         forum_admin.permissions.add(
             *list(
                 perms.filter(content_type__app_label="forum").values_list(
@@ -963,7 +970,9 @@ Welcome to the wiki page!
                 )
             )
         )
-        pedagogy_admin = Group.objects.create(name="Pedagogy admin")
+        pedagogy_admin = Group.objects.create(
+            name="Pedagogy admin", is_manually_manageable=True
+        )
         pedagogy_admin.permissions.add(
             *list(
                 perms.filter(content_type__app_label="pedagogy").values_list(

core/management/commands/populate_more.py

@@ -173,7 +173,8 @@ class Command(BaseCommand):
                     club=club,
                 )
             )
-        Membership.objects.bulk_create(memberships)
+        memberships = Membership.objects.bulk_create(memberships)
+        Membership._add_club_groups(memberships)
 
     def create_uvs(self):
         root = User.objects.get(username="root")

core/migrations/0001_initial.py

@@ -563,14 +563,21 @@ class Migration(migrations.Migration):
             fields=[],
             options={"proxy": True},
             bases=("core.group",),
-            managers=[("objects", core.models.MetaGroupManager())],
+            managers=[("objects", django.contrib.auth.models.GroupManager())],
         ),
+        # at first, there existed a RealGroupManager and a RealGroupManager,
+        # which have been since been removed.
+        # However, this removal broke the migrations because it caused an ImportError.
+        # Thus, the managers MetaGroupManager (above) and RealGroupManager (below)
+        # have been replaced by the base django GroupManager to fix the import.
+        # As those managers aren't actually used in migrations,
+        # this replacement doesn't break anything.
         migrations.CreateModel(
             name="RealGroup",
             fields=[],
             options={"proxy": True},
             bases=("core.group",),
-            managers=[("objects", core.models.RealGroupManager())],
+            managers=[("objects", django.contrib.auth.models.GroupManager())],
         ),
         migrations.AlterUniqueTogether(
             name="page", unique_together={("name", "parent")}

core/migrations/0041_delete_metagroup_alter_group_options_and_more.py

@@ -0,0 +1,51 @@
+# Generated by Django 4.2.16 on 2024-11-30 13:16
+
+from django.db import migrations, models
+from django.db.migrations.state import StateApps
+from django.db.models import F
+
+
+def invert_is_manually_manageable(apps: StateApps, schema_editor):
+    """Invert `is_manually_manageable`.
+
+    This field is a renaming of `is_meta`.
+    However, the meaning has been inverted : the groups
+    which were meta are not manually manageable and vice versa.
+    Thus, the value must be inverted.
+    """
+    Group = apps.get_model("core", "Group")
+    Group.objects.all().update(is_manually_manageable=~F("is_manually_manageable"))
+
+
+class Migration(migrations.Migration):
+    dependencies = [("core", "0040_alter_user_options_user_user_permissions_and_more")]
+
+    operations = [
+        migrations.DeleteModel(
+            name="MetaGroup",
+        ),
+        migrations.DeleteModel(
+            name="RealGroup",
+        ),
+        migrations.AlterModelOptions(
+            name="group",
+            options={},
+        ),
+        migrations.RenameField(
+            model_name="group",
+            old_name="is_meta",
+            new_name="is_manually_manageable",
+        ),
+        migrations.AlterField(
+            model_name="group",
+            name="is_manually_manageable",
+            field=models.BooleanField(
+                default=False,
+                help_text="If False, this shouldn't be shown on group management pages",
+                verbose_name="Is manually manageable",
+            ),
+        ),
+        migrations.RunPython(
+            invert_is_manually_manageable, reverse_code=invert_is_manually_manageable
+        ),
+    ]

core/models.py

@@ -36,7 +36,6 @@ from django.conf import settings
 from django.contrib.auth.models import AbstractUser, UserManager
 from django.contrib.auth.models import AnonymousUser as AuthAnonymousUser
 from django.contrib.auth.models import Group as AuthGroup
-from django.contrib.auth.models import GroupManager as AuthGroupManager
 from django.contrib.staticfiles.storage import staticfiles_storage
 from django.core import validators
 from django.core.cache import cache
@@ -58,34 +57,17 @@ if TYPE_CHECKING:
     from club.models import Club
 
 
-class RealGroupManager(AuthGroupManager):
-    def get_queryset(self):
-        return super().get_queryset().filter(is_meta=False)
-
-
-class MetaGroupManager(AuthGroupManager):
-    def get_queryset(self):
-        return super().get_queryset().filter(is_meta=True)
-
-
 class Group(AuthGroup):
-    """Implement both RealGroups and Meta groups.
+    """Wrapper around django.auth.Group"""
 
-    Groups are sorted by their is_meta property
+    is_manually_manageable = models.BooleanField(
-    """
+        _("Is manually manageable"),
-
-    #: If False, this is a RealGroup
-    is_meta = models.BooleanField(
-        _("meta group status"),
         default=False,
-        help_text=_("Whether a group is a meta group or not"),
+        help_text=_("If False, this shouldn't be shown on group management pages"),
     )
     #: Description of the group
     description = models.CharField(_("description"), max_length=60)
 
-    class Meta:
-        ordering = ["name"]
-
     def get_absolute_url(self) -> str:
         return reverse("core:group_list")
 
@@ -100,65 +82,6 @@ class Group(AuthGroup):
         cache.delete(f"sith_group_{self.name.replace(' ', '_')}")
 
 
-class MetaGroup(Group):
-    """MetaGroups are dynamically created groups.
-
-    Generally used with clubs where creating a club creates two groups:
-
-    * club-SITH_BOARD_SUFFIX
-    * club-SITH_MEMBER_SUFFIX
-    """
-
-    #: Assign a manager in a way that MetaGroup.objects only return groups with is_meta=False
-    objects = MetaGroupManager()
-
-    class Meta:
-        proxy = True
-
-    def __init__(self, *args, **kwargs):
-        super().__init__(*args, **kwargs)
-        self.is_meta = True
-
-    @cached_property
-    def associated_club(self) -> Club | None:
-        """Return the group associated with this meta group.
-
-        The result of this function is cached
-
-
-        Returns:
-            The associated club if it exists, else None
-        """
-        from club.models import Club
-
-        if self.name.endswith(settings.SITH_BOARD_SUFFIX):
-            # replace this with str.removesuffix as soon as Python
-            # is upgraded to 3.10
-            club_name = self.name[: -len(settings.SITH_BOARD_SUFFIX)]
-        elif self.name.endswith(settings.SITH_MEMBER_SUFFIX):
-            club_name = self.name[: -len(settings.SITH_MEMBER_SUFFIX)]
-        else:
-            return None
-        club = cache.get(f"sith_club_{club_name}")
-        if club is None:
-            club = Club.objects.filter(unix_name=club_name).first()
-            cache.set(f"sith_club_{club_name}", club)
-        return club
-
-
-class RealGroup(Group):
-    """RealGroups are created by the developer.
-
-    Most of the time they match a number in settings to be easily used for permissions.
-    """
-
-    #: Assign a manager in a way that MetaGroup.objects only return groups with is_meta=True
-    objects = RealGroupManager()
-
-    class Meta:
-        proxy = True
-
-
 def validate_promo(value: int) -> None:
     start_year = settings.SITH_SCHOOL_START_YEAR
     delta = (localdate() + timedelta(days=180)).year - start_year
@@ -204,8 +127,8 @@ def get_group(*, pk: int | None = None, name: str | None = None) -> Group | None
     else:
         group = Group.objects.filter(name=name).first()
     if group is not None:
-        cache.set(f"sith_group_{group.id}", group)
+        name = group.name.replace(" ", "_")
-        cache.set(f"sith_group_{group.name.replace(' ', '_')}", group)
+        cache.set_many({f"sith_group_{group.id}": group, f"sith_group_{name}": group})
     else:
         cache.set(f"sith_group_{pk_or_name}", "not_found")
     return group
@@ -438,18 +361,6 @@ class User(AbstractUser):
             return self.was_subscribed
         if group.id == settings.SITH_GROUP_ROOT_ID:
             return self.is_root
-        if group.is_meta:
-            # check if this group is associated with a club
-            group.__class__ = MetaGroup
-            club = group.associated_club
-            if club is None:
-                return False
-            membership = club.get_membership_for(self)
-            if membership is None:
-                return False
-            if group.name.endswith(settings.SITH_MEMBER_SUFFIX):
-                return True
-            return membership.role > settings.SITH_MAXIMUM_FREE_ROLE
         return group in self.cached_groups
 
     @property
@@ -474,12 +385,11 @@ class User(AbstractUser):
         return any(g.id == root_id for g in self.cached_groups)
 
     @cached_property
-    def is_board_member(self):
+    def is_board_member(self) -> bool:
-        main_club = settings.SITH_MAIN_CLUB["unix_name"]
+        return self.groups.filter(club_board=settings.SITH_MAIN_CLUB_ID).exists()
-        return self.is_in_group(name=main_club + settings.SITH_BOARD_SUFFIX)
 
     @cached_property
-    def can_read_subscription_history(self):
+    def can_read_subscription_history(self) -> bool:
         if self.is_root or self.is_board_member:
             return True
 
@@ -943,7 +853,7 @@ class SithFile(models.Model):
                     param="1",
                 ).save()
 
-    def is_owned_by(self, user):
+    def is_owned_by(self, user: User) -> bool:
         if user.is_anonymous:
             return False
         if user.is_root:
@@ -958,7 +868,7 @@ class SithFile(models.Model):
             return True
         return user.id == self.owner_id
 
-    def can_be_viewed_by(self, user):
+    def can_be_viewed_by(self, user: User) -> bool:
         if hasattr(self, "profile_of"):
             return user.can_view(self.profile_of)
         if hasattr(self, "avatar_of"):

core/tests/test_core.py

@@ -18,6 +18,7 @@ from smtplib import SMTPException
 
 import freezegun
 import pytest
+from django.contrib.auth.hashers import make_password
 from django.core import mail
 from django.core.cache import cache
 from django.core.mail import EmailMessage
@@ -30,7 +31,7 @@ from model_bakery import baker
 from pytest_django.asserts import assertInHTML, assertRedirects
 
 from antispam.models import ToxicDomain
-from club.models import Membership
+from club.models import Club, Membership
 from core.markdown import markdown
 from core.models import AnonymousUser, Group, Page, User
 from core.utils import get_semester_code, get_start_of_semester
@@ -145,7 +146,7 @@ class TestUserRegistration:
 class TestUserLogin:
     @pytest.fixture()
     def user(self) -> User:
-        return User.objects.first()
+        return baker.make(User, password=make_password("plop"))
 
     def test_login_fail(self, client, user):
         """Should not login a user correctly."""
@@ -349,14 +350,9 @@ class TestUserIsInGroup(TestCase):
 
     @classmethod
     def setUpTestData(cls):
-        from club.models import Club
-
         cls.root_group = Group.objects.get(name="Root")
-        cls.public = Group.objects.get(name="Public")
+        cls.public_group = Group.objects.get(name="Public")
-        cls.skia = User.objects.get(username="skia")
+        cls.public_user = baker.make(User)
-        cls.toto = User.objects.create(
-            username="toto", first_name="a", last_name="b", email="a.b@toto.fr"
-        )
         cls.subscribers = Group.objects.get(name="Subscribers")
         cls.old_subscribers = Group.objects.get(name="Old subscribers")
         cls.accounting_admin = Group.objects.get(name="Accounting admin")
@@ -366,22 +362,12 @@ class TestUserIsInGroup(TestCase):
         cls.banned_counters = Group.objects.get(name="Banned from counters")
         cls.banned_subscription = Group.objects.get(name="Banned to subscribe")
         cls.sas_admin = Group.objects.get(name="SAS admin")
-        cls.club = Club.objects.create(
+        cls.club = baker.make(Club)
-            name="Fake Club",
-            unix_name="fake-club",
-            address="Fake address",
-        )
         cls.main_club = Club.objects.get(id=1)
 
     def assert_in_public_group(self, user):
-        assert user.is_in_group(pk=self.public.id)
+        assert user.is_in_group(pk=self.public_group.id)
-        assert user.is_in_group(name=self.public.name)
+        assert user.is_in_group(name=self.public_group.name)
-
-    def assert_in_club_metagroups(self, user, club):
-        meta_groups_board = club.unix_name + settings.SITH_BOARD_SUFFIX
-        meta_groups_members = club.unix_name + settings.SITH_MEMBER_SUFFIX
-        assert user.is_in_group(name=meta_groups_board) is False
-        assert user.is_in_group(name=meta_groups_members) is False
 
     def assert_only_in_public_group(self, user):
         self.assert_in_public_group(user)
@@ -392,13 +378,11 @@ class TestUserIsInGroup(TestCase):
             self.sas_admin,
             self.subscribers,
             self.old_subscribers,
+            self.club.members_group,
+            self.club.board_group,
         ):
             assert not user.is_in_group(pk=group.pk)
             assert not user.is_in_group(name=group.name)
-        meta_groups_board = self.club.unix_name + settings.SITH_BOARD_SUFFIX
-        meta_groups_members = self.club.unix_name + settings.SITH_MEMBER_SUFFIX
-        assert user.is_in_group(name=meta_groups_board) is False
-        assert user.is_in_group(name=meta_groups_members) is False
 
     def test_anonymous_user(self):
         """Test that anonymous users are only in the public group."""
@@ -407,80 +391,80 @@ class TestUserIsInGroup(TestCase):
 
     def test_not_subscribed_user(self):
         """Test that users who never subscribed are only in the public group."""
-        self.assert_only_in_public_group(self.toto)
+        self.assert_only_in_public_group(self.public_user)
 
     def test_wrong_parameter_fail(self):
         """Test that when neither the pk nor the name argument is given,
         the function raises a ValueError.
         """
         with self.assertRaises(ValueError):
-            self.toto.is_in_group()
+            self.public_user.is_in_group()
 
     def test_number_queries(self):
         """Test that the number of db queries is stable
         and that less queries are made when making a new call.
         """
         # make sure Skia is in at least one group
-        self.skia.groups.add(Group.objects.first().pk)
+        group_in = baker.make(Group)
-        skia_groups = self.skia.groups.all()
+        self.public_user.groups.add(group_in)
 
-        group_in = skia_groups.first()
         cache.clear()
         # Test when the user is in the group
         with self.assertNumQueries(2):
-            self.skia.is_in_group(pk=group_in.id)
+            self.public_user.is_in_group(pk=group_in.id)
         with self.assertNumQueries(0):
-            self.skia.is_in_group(pk=group_in.id)
+            self.public_user.is_in_group(pk=group_in.id)
 
-        ids = skia_groups.values_list("pk", flat=True)
+        group_not_in = baker.make(Group)
-        group_not_in = Group.objects.exclude(pk__in=ids).first()
         cache.clear()
         # Test when the user is not in the group
         with self.assertNumQueries(2):
-            self.skia.is_in_group(pk=group_not_in.id)
+            self.public_user.is_in_group(pk=group_not_in.id)
         with self.assertNumQueries(0):
-            self.skia.is_in_group(pk=group_not_in.id)
+            self.public_user.is_in_group(pk=group_not_in.id)
 
     def test_cache_properly_cleared_membership(self):
         """Test that when the membership of a user end,
         the cache is properly invalidated.
         """
-        membership = Membership.objects.create(
+        membership = baker.make(Membership, club=self.club, user=self.public_user)
-            club=self.club, user=self.toto, end_date=None
-        )
-        meta_groups_members = self.club.unix_name + settings.SITH_MEMBER_SUFFIX
         cache.clear()
-        assert self.toto.is_in_group(name=meta_groups_members) is True
+        self.club.get_membership_for(self.public_user)  # this should populate the cache
-        assert membership == cache.get(f"membership_{self.club.id}_{self.toto.id}")
+        assert membership == cache.get(
+            f"membership_{self.club.id}_{self.public_user.id}"
+        )
         membership.end_date = now() - timedelta(minutes=5)
         membership.save()
-        cached_membership = cache.get(f"membership_{self.club.id}_{self.toto.id}")
+        cached_membership = cache.get(
+            f"membership_{self.club.id}_{self.public_user.id}"
+        )
         assert cached_membership == "not_member"
-        assert self.toto.is_in_group(name=meta_groups_members) is False
 
     def test_cache_properly_cleared_group(self):
         """Test that when a user is removed from a group,
         the is_in_group_method return False when calling it again.
         """
         # testing with pk
-        self.toto.groups.add(self.com_admin.pk)
+        self.public_user.groups.add(self.com_admin.pk)
-        assert self.toto.is_in_group(pk=self.com_admin.pk) is True
+        assert self.public_user.is_in_group(pk=self.com_admin.pk) is True
 
-        self.toto.groups.remove(self.com_admin.pk)
+        self.public_user.groups.remove(self.com_admin.pk)
-        assert self.toto.is_in_group(pk=self.com_admin.pk) is False
+        assert self.public_user.is_in_group(pk=self.com_admin.pk) is False
 
         # testing with name
-        self.toto.groups.add(self.sas_admin.pk)
+        self.public_user.groups.add(self.sas_admin.pk)
-        assert self.toto.is_in_group(name="SAS admin") is True
+        assert self.public_user.is_in_group(name="SAS admin") is True
 
-        self.toto.groups.remove(self.sas_admin.pk)
+        self.public_user.groups.remove(self.sas_admin.pk)
-        assert self.toto.is_in_group(name="SAS admin") is False
+        assert self.public_user.is_in_group(name="SAS admin") is False
 
     def test_not_existing_group(self):
         """Test that searching for a not existing group
         returns False.
         """
-        assert self.skia.is_in_group(name="This doesn't exist") is False
+        user = baker.make(User)
+        user.groups.set(list(Group.objects.all()))
+        assert not user.is_in_group(name="This doesn't exist")
 
 
 class TestDateUtils(TestCase):

core/views/forms.py

@@ -32,19 +32,14 @@ from django.contrib.staticfiles.management.commands.collectstatic import (
 )
 from django.core.exceptions import ValidationError
 from django.db import transaction
-from django.forms import (
+from django.forms import CheckboxSelectMultiple, DateInput, DateTimeInput, TextInput
-    CheckboxSelectMultiple,
-    DateInput,
-    DateTimeInput,
-    TextInput,
-)
 from django.utils.translation import gettext
 from django.utils.translation import gettext_lazy as _
 from phonenumber_field.widgets import RegionalPhoneNumberWidget
 from PIL import Image
 
 from antispam.forms import AntiSpamEmailField
-from core.models import Gift, Page, RealGroup, SithFile, User
+from core.models import Gift, Group, Page, SithFile, User
 from core.utils import resize_image
 from core.views.widgets.select import (
     AutoCompleteSelect,
@@ -293,7 +288,7 @@ class UserGroupsForm(forms.ModelForm):
     required_css_class = "required"
 
     groups = forms.ModelMultipleChoiceField(
-        queryset=RealGroup.objects.all(),
+        queryset=Group.objects.filter(is_manually_manageable=True),
         widget=CheckboxSelectMultiple,
         label=_("Groups"),
         required=False,

core/views/group.py

@@ -21,7 +21,7 @@ from django.utils.translation import gettext_lazy as _
 from django.views.generic import ListView
 from django.views.generic.edit import CreateView, DeleteView, UpdateView
 
-from core.models import RealGroup, User
+from core.models import Group, User
 from core.views import CanCreateMixin, CanEditMixin, DetailFormView
 from core.views.widgets.select import AutoCompleteSelectMultipleUser
 
@@ -57,7 +57,8 @@ class EditMembersForm(forms.Form):
 class GroupListView(CanEditMixin, ListView):
     """Displays the Group list."""
 
-    model = RealGroup
+    model = Group
+    queryset = Group.objects.filter(is_manually_manageable=True)
     ordering = ["name"]
     template_name = "core/group_list.jinja"
 
@@ -65,7 +66,8 @@ class GroupListView(CanEditMixin, ListView):
 class GroupEditView(CanEditMixin, UpdateView):
     """Edit infos of a Group."""
 
-    model = RealGroup
+    model = Group
+    queryset = Group.objects.filter(is_manually_manageable=True)
     pk_url_kwarg = "group_id"
     template_name = "core/group_edit.jinja"
     fields = ["name", "description"]
@@ -74,7 +76,8 @@ class GroupEditView(CanEditMixin, UpdateView):
 class GroupCreateView(CanCreateMixin, CreateView):
     """Add a new Group."""
 
-    model = RealGroup
+    model = Group
+    queryset = Group.objects.filter(is_manually_manageable=True)
     template_name = "core/create.jinja"
     fields = ["name", "description"]
 
@@ -84,7 +87,8 @@ class GroupTemplateView(CanEditMixin, DetailFormView):
     Allow adding and removing users from it.
     """
 
-    model = RealGroup
+    model = Group
+    queryset = Group.objects.filter(is_manually_manageable=True)
     form_class = EditMembersForm
     pk_url_kwarg = "group_id"
     template_name = "core/group_detail.jinja"
@@ -118,7 +122,8 @@ class GroupTemplateView(CanEditMixin, DetailFormView):
 class GroupDeleteView(CanEditMixin, DeleteView):
     """Delete a Group."""
 
-    model = RealGroup
+    model = Group
+    queryset = Group.objects.filter(is_manually_manageable=True)
     pk_url_kwarg = "group_id"
     template_name = "core/delete_confirm.jinja"
     success_url = reverse_lazy("core:group_list")

counter/models.py

@@ -532,9 +532,12 @@ class Counter(models.Model):
         return user.is_in_group(pk=settings.SITH_GROUP_COUNTER_ADMIN_ID)
 
     def can_be_viewed_by(self, user: User) -> bool:
-        if self.type == "BAR":
+        return (
-            return True
+            self.type == "BAR"
-        return user.is_board_member or user in self.sellers.all()
+            or user.is_root
+            or user.is_in_group(pk=self.club.board_group_id)
+            or user in self.sellers.all()
+        )
 
     def gen_token(self) -> None:
         """Generate a new token for this counter."""

docs/tutorial/groups.md

@@ -1,54 +1,165 @@
-Il existe deux types de groupes sur le site AE :
+## Un peu d'histoire
+
+Par défaut, Django met à disposition un modèle `Group`,
+lié par clef étrangère au modèle `User`.
+Pour créer un système de gestion des groupes qui semblait plus
+approprié aux développeurs initiaux, un nouveau
+modèle [core.models.Group][]
+a été crée, et la relation de clef étrangère a été modifiée
+pour lier [core.models.User][] à ce dernier.
+
+L'ancien modèle `Group` était implicitement
+divisé en deux catégories :
+
+- les *méta-groupes* : groupes liés aux clubs et créés à la volée.
+  Ces groupes n'étaient liés par clef étrangère à aucun utilisateur.
+  Ils étaient récupérés à partir de leur nom uniquement
+  et étaient plus une indirection pour désigner l'appartenance à un club
+  que des groupes à proprement parler.
+- les *groupes réels* : groupes créés à la main 
+  et souvent hardcodés dans la configuration.
+
+Cependant, ce nouveau système s'éloignait trop du cadre de Django
+et a fini par devenir une gêne.
+La vérification des droits lors des opérations est devenue
+une opération complexe et coûteuse en temps.
+
+La gestion des groupes a donc été modifiée pour recoller un
+peu plus au cadre de Django.
+Toutefois, il n'a pas été tenté de revenir à 100%
+sur l'architecture prônée par Django.
+
+D'une part, cela représentait un risque pour le succès de l'application 
+de la migration sur la base de données de production.
+
+D'autre part, si une autre architecture a été tentée au début, 
+ce n'était pas sans raison :
+ce que nous voulons modéliser sur le site AE n'est pas
+complètement modélisable avec ce qu'offre Django.
+Il faut donc bien garder une surcouche au-dessus de l'authentification
+de Django.
+Tout le défi est de réussir à maintenir cette surcouche aussi fine
+que possible sans limiter ce que nous voulons faire.
+
+## Représentation en base de données
+
+Le modèle [core.models.Group][] a donc été légèrement remanié
+et la distinction entre groupes méta et groupes réels a été plus ou moins
+supprimée.
+La liaison de clef étrangère se fait toujours entre [core.models.User][]
+et [core.models.Group][].
+
+Cependant, il y a une subtilité.
+Depuis le début, le modèle `Group` de django n'a jamais disparu.
+En effet, lorsqu'un modèle hérite d'un modèle qui n'est pas
+abstrait, Django garde les deux tables et les lie
+par une clef étrangère unique de clef primaire à clef primaire
+(pour plus de détail, lire 
+[la doc de django sur l'héritage de modèle](https://docs.djangoproject.com/fr/stable/topics/db/models/#model-inheritance))
+
+L'organisation réelle de notre système de groupes
+est donc la suivante :
+<!-- J'ai utilisé un diagramme entité-relation
+ au lieu d'un diagramme de db, parce que Mermaid n'a que 
+ le diagramme entité-relation. -->
+
+```mermaid
+---
+title: Représentation des groupes
+---
+erDiagram
+    core_user }o..o{ core_group: core_user_groups
+    auth_group }o..o{ auth_permission: auth_group_permissions
+    core_group ||--|| auth_group: ""
+    core_user }o..o{ auth_permission :"core_user_user_permissions"
+    
+    core_user {
+        int id PK
+        string username
+        string email
+        string first_name
+        etc etc
+    }
+    core_group {
+        int group_ptr_id PK,FK
+        string description
+        bool is_manually_manageable
+    }
+    auth_group {
+        int id PK
+        name string
+    }
+    auth_permission {
+        int id PK
+        string name
+    }
+```
+
+Cette organisation, rajoute une certaine complexité,
+mais celle-ci est presque entièrement gérée par django,
+ce qui fait que la gestion n'est pas tellement plus compliquée
+du point de vue du développeur.
+
+Chaque fois qu'un queryset implique notre `Group`
+ou le `Group` de django, l'autre modèle est automatiquement
+ajouté à la requête par jointure.
+De cette façon, on peut manipuler l'un ou l'autre,
+sans même se rendre que les tables sont dans des tables séparées.
+
+Par exemple :
+
+=== "python"
+
+    ```python
+    from core.models import Group
+
+    Group.objects.all()
+    ```
+
+=== "SQL généré"
+
+    ```sql
+    SELECT "auth_group"."id",
+           "auth_group"."name",
+           "core_group"."group_ptr_id",
+           "core_group"."is_manually_manageable",
+           "core_group"."description"
+    FROM "core_group"
+             INNER JOIN "auth_group" ON ("core_group"."group_ptr_id" = "auth_group"."id")
+    ```
+
+!!!warning
+
+    Django réussit à abstraire assez bien la logique relationnelle.
+    Cependant, gardez bien en mémoire que ce n'est pas quelque chose
+    de magique et que cette manière de faire a des limitations.
+    Par exemple, il devient impossible de `bulk_create`
+    des groupes.
 
-- l'un se base sur des groupes enregistrés en base de données pendant le développement,
-  c'est le système de groupes réels.
-- L'autre est plus dynamique et comprend tous les groupes générés
-  pendant l'exécution et l'utilisation du programme.
-  Cela correspond généralement aux groupes liés aux clubs. 
-  Ce sont les méta-groupes.
 
 ## La définition d'un groupe
 
-Les deux types de groupes sont stockés dans la même table
+Un groupe est constitué des informations suivantes :
-en base de données, et ne sont différenciés que par un attribut `is_meta`.
 
-### Les groupes réels
+- son nom : `name`
+- sa description : `description` (optionnelle)
+- si on autorise sa gestion par les utilisateurs du site : `is_manually_manageable`
 
-Pour plus différencier l'utilisation de ces deux types de groupe,
+Si un groupe est gérable manuellement, alors les administrateurs du site
-il a été créé une classe proxy 
+auront le droit d'assigner des utilisateurs à ce groupe depuis l'interface dédiée.
-(c'est-à-dire qu'elle ne correspond pas à une vraie table en base de donnée)
-qui encapsule leur utilisation. 
-`RealGroup` peut être utilisé pour créer des groupes réels dans le code
-et pour faire une recherche sur ceux-ci
-(dans le cadre d'une vérification de permissions par exemple).
 
-### Les méta-groupes
+S'il n'est pas gérable manuellement, on cache aux utilisateurs du site
+la gestion des membres de ce groupe.
+La gestion se fait alors uniquement "sous le capot",
+de manière automatique lors de certains évènements.
+Par exemple, lorsqu'un utilisateur rejoint un club,
+il est automatiquement ajouté au groupe des membres
+du club.
+Lorsqu'il quitte le club, il est retiré du groupe.
 
-Les méta-groupes, comme expliqué précédemment,
+## Les principaux groupes utilisés
-sont utilisés dans les contextes où il est nécessaire de créer un groupe dynamiquement.
-Les objets `MetaGroup`, bien que dynamiques, doivent tout de même s'enregistrer
-en base de données comme des vrais groupes afin de pouvoir être affectés 
-dans les permissions d'autres objets, comme un forum ou une page de wiki par exemple.
-C'est principalement utilisé au travers des clubs,
-qui génèrent automatiquement deux groupes à leur création :
 
-- club-bureau : contient tous les membres d'un club **au dessus**
+Les groupes les plus notables gérables par les administrateurs du site sont :
-  du grade défini dans `settings.SITH_MAXIMUM_FREE_ROLE`.
-- club-membres : contient tous les membres d'un club 
-  **en dessous** du grade défini dans `settings.SITH_MAXIMUM_FREE_ROLE`.
-
-
-## Les groupes réels utilisés
-
-Les groupes réels que l'on utilise dans le site sont les suivants :
-
-Groupes gérés automatiquement par le site :
-
-- `Public` : tous les utilisateurs du site
-- `Subscribers` : tous les cotisants du site
-- `Old subscribers` : tous les anciens cotisants
-
-Groupes gérés par les administrateurs (à appliquer à la main sur un utilisateur) :
 
 - `Root` : administrateur global du site
 - `Accounting admin` : les administrateurs de la comptabilité
@@ -62,3 +173,10 @@ Groupes gérés par les administrateurs (à appliquer à la main sur un utilisat
 - `Banned to subscribe` : les utilisateurs interdits de cotisation
 
 
+En plus de ces groupes, on peut noter :
+
+- `Public` : tous les utilisateurs du site
+- `Subscribers` : tous les cotisants du site
+- `Old subscribers` : tous les anciens cotisants
+
+

docs/tutorial/perms.md

@@ -4,7 +4,7 @@
 Le fonctionnement de l'AE ne permet pas d'utiliser le système de permissions
 intégré à Django tel quel. Lors de la conception du Sith, ce qui paraissait le
 plus simple à l'époque était de concevoir un système maison afin de se calquer
-sur ce que faisais l'ancien site.
+sur ce que faisait l'ancien site.
 
 ### Protéger un modèle
 

election/tests.py

@@ -11,8 +11,6 @@ class TestElection(TestCase):
     def setUpTestData(cls):
         cls.election = Election.objects.first()
         cls.public_group = Group.objects.get(id=settings.SITH_GROUP_PUBLIC_ID)
-        cls.subscriber_group = Group.objects.get(name=settings.SITH_MAIN_MEMBERS_GROUP)
-        cls.ae_board_group = Group.objects.get(name=settings.SITH_MAIN_BOARD_GROUP)
         cls.sli = User.objects.get(username="sli")
         cls.subscriber = User.objects.get(username="subscriber")
         cls.public = User.objects.get(username="public")

galaxy/management/commands/generate_galaxy_test_data.py

@@ -118,18 +118,26 @@ class Command(BaseCommand):
             self.make_important_citizen(u)
 
     def make_clubs(self):
-        """Create all the clubs (:class:`club.models.Club`).
+        """Create all the clubs [club.models.Club][].
 
         After creation, the clubs are stored in `self.clubs` for fast access later.
-        Don't create the meta groups (:class:`core.models.MetaGroup`)
+        Don't create the pages of the clubs ([core.models.Page][]).
-        nor the pages of the clubs (:class:`core.models.Page`).
         """
-        self.clubs = []
+        # dummy groups.
-        for i in range(self.NB_CLUBS):
+        # the galaxy doesn't care about the club groups,
-            self.clubs.append(Club(unix_name=f"galaxy-club-{i}", name=f"club-{i}"))
+        # but it's necessary to add them nonetheless in order
-        # We don't need to create corresponding groups here, as the Galaxy doesn't care about them
+        # not to break the integrity constraints
-        Club.objects.bulk_create(self.clubs)
+        self.clubs = Club.objects.bulk_create(
-        self.clubs = list(Club.objects.filter(unix_name__startswith="galaxy-").all())
+            [
+                Club(
+                    unix_name=f"galaxy-club-{i}",
+                    name=f"club-{i}",
+                    board_group=Group.objects.create(name=f"board {i}"),
+                    members_group=Group.objects.create(name=f"members {i}"),
+                )
+                for i in range(self.NB_CLUBS)
+            ]
+        )
 
     def make_users(self):
         """Create all the users and store them in `self.users` for fast access later.