Better usage of cache for groups and clubs related operations (#634)

* Better usage of cache for group retrieval

* Cache clearing on object deletion or update

* replace signals by save and delete override

* add is_anonymous check in is_owned_by

Add in many is_owned_by(self, user) methods that user is not anonymous. Since many of those functions do db queries, this should reduce a little bit the load of the db.

* Stricter usage of User.is_in_group

Constrain the parameters that can be passed to the function to make sure only a str or an int can be used. Also force to explicitly specify if the group id or the group name is used.

* write test and correct bugs

* remove forgotten populate commands

* Correct test

accounting/models.py

@@ -66,7 +66,7 @@ class Company(models.Model):
         """
         Method to see if that object can be edited by the given user
         """
-        if user.is_in_group(settings.SITH_GROUP_ACCOUNTING_ADMIN_ID):
+        if user.is_in_group(pk=settings.SITH_GROUP_ACCOUNTING_ADMIN_ID):
             return True
         return False
 
@@ -117,7 +117,9 @@ class BankAccount(models.Model):
         """
         Method to see if that object can be edited by the given user
         """
-        if user.is_in_group(settings.SITH_GROUP_ACCOUNTING_ADMIN_ID):
+        if user.is_anonymous:
+            return False
+        if user.is_in_group(pk=settings.SITH_GROUP_ACCOUNTING_ADMIN_ID):
             return True
         m = self.club.get_membership_for(user)
         if m is not None and m.role >= settings.SITH_CLUB_ROLES_ID["Treasurer"]:
@@ -154,7 +156,9 @@ class ClubAccount(models.Model):
         """
         Method to see if that object can be edited by the given user
         """
-        if user.is_in_group(settings.SITH_GROUP_ACCOUNTING_ADMIN_ID):
+        if user.is_anonymous:
+            return False
+        if user.is_in_group(pk=settings.SITH_GROUP_ACCOUNTING_ADMIN_ID):
             return True
         return False
 
@@ -225,7 +229,9 @@ class GeneralJournal(models.Model):
         """
         Method to see if that object can be edited by the given user
         """
-        if user.is_in_group(settings.SITH_GROUP_ACCOUNTING_ADMIN_ID):
+        if user.is_anonymous:
+            return False
+        if user.is_in_group(pk=settings.SITH_GROUP_ACCOUNTING_ADMIN_ID):
             return True
         if self.club_account.can_be_edited_by(user):
             return True
@@ -235,7 +241,7 @@ class GeneralJournal(models.Model):
         """
         Method to see if that object can be edited by the given user
         """
-        if user.is_in_group(settings.SITH_GROUP_ACCOUNTING_ADMIN_ID):
+        if user.is_in_group(pk=settings.SITH_GROUP_ACCOUNTING_ADMIN_ID):
             return True
         if self.club_account.can_be_edited_by(user):
             return True
@@ -414,7 +420,9 @@ class Operation(models.Model):
         """
         Method to see if that object can be edited by the given user
         """
-        if user.is_in_group(settings.SITH_GROUP_ACCOUNTING_ADMIN_ID):
+        if user.is_anonymous:
+            return False
+        if user.is_in_group(pk=settings.SITH_GROUP_ACCOUNTING_ADMIN_ID):
             return True
         if self.journal.closed:
             return False
@@ -427,7 +435,7 @@ class Operation(models.Model):
         """
         Method to see if that object can be edited by the given user
         """
-        if user.is_in_group(settings.SITH_GROUP_ACCOUNTING_ADMIN_ID):
+        if user.is_in_group(pk=settings.SITH_GROUP_ACCOUNTING_ADMIN_ID):
             return True
         if self.journal.closed:
             return False
@@ -483,7 +491,9 @@ class AccountingType(models.Model):
         """
         Method to see if that object can be edited by the given user
         """
-        if user.is_in_group(settings.SITH_GROUP_ACCOUNTING_ADMIN_ID):
+        if user.is_anonymous:
+            return False
+        if user.is_in_group(pk=settings.SITH_GROUP_ACCOUNTING_ADMIN_ID):
             return True
         return False
 
@@ -554,6 +564,8 @@ class Label(models.Model):
         )
 
     def is_owned_by(self, user):
+        if user.is_anonymous:
+            return False
         return self.club_account.is_owned_by(user)
 
     def can_be_edited_by(self, user):

accounting/templates/accounting/bank_account_details.jinja

@@ -12,7 +12,7 @@
         </p>
         <hr>
         <h2>{% trans %}Bank account: {% endtrans %}{{ object.name }}</h2>
-        {% if user.is_in_group(settings.SITH_GROUP_ACCOUNTING_ADMIN_ID) and not object.club_accounts.exists() %}
+        {% if user.is_in_group(pk=settings.SITH_GROUP_ACCOUNTING_ADMIN_ID) and not object.club_accounts.exists() %}
         <a href="{{ url('accounting:bank_delete', b_account_id=object.id) }}">{% trans %}Delete{% endtrans %}</a>
         {% endif %}
         <h4>{% trans %}Infos{% endtrans %}</h4>

accounting/templates/accounting/bank_account_list.jinja

@@ -9,7 +9,7 @@
         <h4>
         {% trans %}Accounting{% endtrans %}
         </h4>
-        {% if user.is_in_group(settings.SITH_GROUP_ACCOUNTING_ADMIN_ID) %}
+        {% if user.is_in_group(pk=settings.SITH_GROUP_ACCOUNTING_ADMIN_ID) %}
         <p><a href="{{ url('accounting:simple_type_list') }}">{% trans %}Manage simplified types{% endtrans %}</a></p>
         <p><a href="{{ url('accounting:type_list') }}">{% trans %}Manage accounting types{% endtrans %}</a></p>
         <p><a href="{{ url('accounting:bank_new') }}">{% trans %}New bank account{% endtrans %}</a></p>

accounting/templates/accounting/club_account_details.jinja

@@ -16,7 +16,7 @@
         {% if user.is_root and not object.journals.exists() %}
         <a href="{{ url('accounting:club_delete', c_account_id=object.id) }}">{% trans %}Delete{% endtrans %}</a>
         {% endif %}
-        {% if user.is_in_group(settings.SITH_GROUP_ACCOUNTING_ADMIN_ID) %}
+        {% if user.is_in_group(pk=settings.SITH_GROUP_ACCOUNTING_ADMIN_ID) %}
         <p><a href="{{ url('accounting:label_new') }}?parent={{ object.id }}">{% trans %}New label{% endtrans %}</a></p>
         {% endif %}
         <p><a href="{{ url('accounting:label_list', clubaccount_id=object.id) }}">{% trans %}Label list{% endtrans %}</a></p>
@@ -56,7 +56,7 @@
                 {% endif %}
                 <td> <a href="{{ url('accounting:journal_details', j_id=j.id) }}">{% trans %}View{% endtrans %}</a>
                     <a href="{{ url('accounting:journal_edit', j_id=j.id) }}">{% trans %}Edit{% endtrans %}</a>
-                    {% if user.is_in_group(settings.SITH_GROUP_ACCOUNTING_ADMIN_ID) and j.operations.count() == 0 %}
+                    {% if user.is_in_group(pk=settings.SITH_GROUP_ACCOUNTING_ADMIN_ID) and j.operations.count() == 0 %}
                         <a href="{{ url('accounting:journal_delete', j_id=j.id) }}">{% trans %}Delete{% endtrans %}</a>
                     {% endif %}
                     </td>

accounting/templates/accounting/co_list.jinja

@@ -6,11 +6,12 @@
 
 {% block content %}
     <div id="accounting">
-        {% if user.is_in_group(settings.SITH_GROUP_ACCOUNTING_ADMIN_ID) or user.is_root %}
+        {% if user.is_root
+           or user.is_in_group(pk=settings.SITH_GROUP_ACCOUNTING_ADMIN_ID)
+        %}
         <p><a href="{{ url('accounting:co_new') }}">{% trans %}Create new company{% endtrans %}</a></p>
         {% endif %}
-
+        <br/>
-        </br>
         <table>
             <thead>
             <tr>

accounting/templates/accounting/journal_details.jinja

@@ -84,10 +84,13 @@
                 <td>-</td>
                 {% endif %}
                 <td>
-                    {% if o.journal.club_account.bank_account.name != "AE TI" and o.journal.club_account.bank_account.name != "TI" or user.is_in_group(settings.SITH_GROUP_ACCOUNTING_ADMIN_ID) %}
+                    {%
-                    {% if not o.journal.closed %}
+                        if o.journal.club_account.bank_account.name not in ["AE TI", "TI"]
-                    <a href="{{ url('accounting:op_edit', op_id=o.id) }}">{% trans %}Edit{% endtrans %}</a>
+                        or user.is_in_group(pk=settings.SITH_GROUP_ACCOUNTING_ADMIN_ID)
-                    {% endif %}
+                    %}
+                        {% if not o.journal.closed %}
+                            <a href="{{ url('accounting:op_edit', op_id=o.id) }}">{% trans %}Edit{% endtrans %}</a>
+                        {% endif %}
                     {% endif %}
                 </td>
                 <td><a href="{{ url('accounting:op_pdf', op_id=o.id) }}">{% trans %}Generate{% endtrans %}</a></td>

accounting/templates/accounting/label_list.jinja

@@ -13,7 +13,7 @@
         </p>
         <hr>
         <p><a href="{{ url('accounting:club_details', c_account_id=object.id) }}">{% trans %}Back to club account{% endtrans %}</a></p>
-        {% if user.is_in_group(settings.SITH_GROUP_ACCOUNTING_ADMIN_ID) %}
+        {% if user.is_in_group(pk=settings.SITH_GROUP_ACCOUNTING_ADMIN_ID) %}
         <p><a href="{{ url('accounting:label_new') }}?parent={{ object.id }}">{% trans %}New label{% endtrans %}</a></p>
         {% endif %}
         {% if object.labels.all() %}
@@ -21,7 +21,7 @@
         <ul>
             {% for l in object.labels.all()  %}
             <li><a href="{{ url('accounting:label_edit', label_id=l.id) }}">{{ l }}</a>
-            {% if user.is_in_group(settings.SITH_GROUP_ACCOUNTING_ADMIN_ID) %}
+            {% if user.is_in_group(pk=settings.SITH_GROUP_ACCOUNTING_ADMIN_ID) %}
              -
                 <a href="{{ url('accounting:label_delete', label_id=l.id) }}">{% trans %}Delete{% endtrans %}</a>
             {% endif %}

accounting/views.py

@@ -891,7 +891,7 @@ class RefoundAccountView(FormView):
     form_class = CloseCustomerAccountForm
 
     def permission(self, user):
-        if user.is_root or user.is_in_group(settings.SITH_GROUP_ACCOUNTING_ADMIN_ID):
+        if user.is_root or user.is_in_group(pk=settings.SITH_GROUP_ACCOUNTING_ADMIN_ID):
             return True
         else:
             raise PermissionDenied

club/forms.py

@@ -229,9 +229,7 @@ class ClubMemberForm(forms.Form):
                 id__in=[
                     ms.user.id
                     for ms in self.club_members
-                    if ms.can_be_edited_by(
+                    if ms.can_be_edited_by(self.request_user)
-                        self.request_user, self.request_user_membership
-                    )
                 ]
             ).all(),
             label=_("Mark as old"),

club/models.py

@@ -22,10 +22,14 @@
 # Place - Suite 330, Boston, MA 02111-1307, USA.
 #
 #
+from typing import Optional
 
+from django.core.cache import cache
 from django.db import models
 from django.core import validators
 from django.conf import settings
+from django.db.models import Q
+from django.utils.timezone import now
 from django.utils.translation import gettext_lazy as _
 from django.core.exceptions import ValidationError, ObjectDoesNotExist
 from django.db import transaction
@@ -172,29 +176,34 @@ class Club(models.Model):
             self.page.parent = self.parent.page
             self.page.save(force_lock=True)
 
+    @transaction.atomic()
     def save(self, *args, **kwargs):
-        with transaction.atomic():
+        old = Club.objects.filter(id=self.id).first()
-            creation = False
+        creation = old is None
-            old = Club.objects.filter(id=self.id).first()
+        if not creation and old.unix_name != self.unix_name:
-            if not old:
+            self._change_unixname(self.unix_name)
-                creation = True
+        super(Club, self).save(*args, **kwargs)
-            else:
+        if creation:
-                if old.unix_name != self.unix_name:
+            board = MetaGroup(name=self.unix_name + settings.SITH_BOARD_SUFFIX)
-                    self._change_unixname(self.unix_name)
+            board.save()
-            super(Club, self).save(*args, **kwargs)
+            member = MetaGroup(name=self.unix_name + settings.SITH_MEMBER_SUFFIX)
-            if creation:
+            member.save()
-                board = MetaGroup(name=self.unix_name + settings.SITH_BOARD_SUFFIX)
+            subscribers = Group.objects.filter(
-                board.save()
+                name=settings.SITH_MAIN_MEMBERS_GROUP
-                member = MetaGroup(name=self.unix_name + settings.SITH_MEMBER_SUFFIX)
+            ).first()
-                member.save()
+            self.make_home()
-                subscribers = Group.objects.filter(
+            self.home.edit_groups.set([board])
-                    name=settings.SITH_MAIN_MEMBERS_GROUP
+            self.home.view_groups.set([member, subscribers])
-                ).first()
+            self.home.save()
-                self.make_home()
+        self.make_page()
-                self.home.edit_groups.set([board])
+        cache.set(f"sith_club_{self.unix_name}", self)
-                self.home.view_groups.set([member, subscribers])
+
-                self.home.save()
+    def delete(self, *args, **kwargs):
-            self.make_page()
+        super().delete(*args, **kwargs)
+        # 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}")
 
     def __str__(self):
         return self.name
@@ -209,7 +218,9 @@ class Club(models.Model):
         """
         Method to see if that object can be super edited by the given user
         """
-        return user.is_in_group(settings.SITH_MAIN_BOARD_GROUP)
+        if user.is_anonymous:
+            return False
+        return user.is_board_member
 
     def get_full_logo_url(self):
         return "https://%s%s" % (settings.SITH_URL, self.logo.url)
@@ -229,28 +240,89 @@ class Club(models.Model):
             return False
         return sub.was_subscribed
 
-    _memberships = {}
+    def get_membership_for(self, user: User) -> Optional["Membership"]:
-
-    def get_membership_for(self, user):
         """
-        Returns the current membership the given user
+        Return the current membership the given user.
+        The result is cached.
         """
-        try:
+        if user.is_anonymous:
-            return Club._memberships[self.id][user.id]
+            return None
-        except:
+        membership = cache.get(f"membership_{self.id}_{user.id}")
-            m = self.members.filter(user=user.id).filter(end_date=None).first()
+        if membership == "not_member":
-            try:
+            return None
-                Club._memberships[self.id][user.id] = m
+        if membership is None:
-            except:
+            membership = self.members.filter(user=user, end_date=None).first()
-                Club._memberships[self.id] = {}
+            if membership is None:
-                Club._memberships[self.id][user.id] = m
+                cache.set(f"membership_{self.id}_{user.id}", "not_member")
-            return m
+            else:
+                cache.set(f"membership_{self.id}_{user.id}", membership)
+        return membership
 
     def has_rights_in_club(self, user):
         m = self.get_membership_for(user)
         return m is not None and m.role > settings.SITH_MAXIMUM_FREE_ROLE
 
 
+class MembershipQuerySet(models.QuerySet):
+    def ongoing(self) -> "MembershipQuerySet":
+        """
+        Filter all memberships which are not finished yet
+        """
+        # noinspection PyTypeChecker
+        return self.filter(Q(end_date=None) | Q(end_date__gte=timezone.now()))
+
+    def board(self) -> "MembershipQuerySet":
+        """
+        Filter all memberships where the user is/was in the board.
+
+        Be aware that users who were in the board in the past
+        are included, even if there are no more members.
+
+        If you want to get the users who are currently in the board,
+        mind combining this with the :meth:`ongoing` queryset method
+        """
+        # noinspection PyTypeChecker
+        return self.filter(role__gt=settings.SITH_MAXIMUM_FREE_ROLE)
+
+    def update(self, **kwargs):
+        """
+        Work just like the default Django's update() method,
+        but add a cache refresh for the elements of the queryset.
+
+        Be aware that this adds a db query to retrieve the updated objects
+        """
+        nb_rows = super().update(**kwargs)
+        if nb_rows > 0:
+            # if at least a row was affected, refresh the cache
+            for membership in self.all():
+                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):
+        """
+        Work just like the default Django's delete() method,
+        but add a cache invalidation for the elements of the queryset
+        before the deletion.
+
+        Be aware that this adds a db query to retrieve the deleted element.
+        As this first query take place before the deletion operation,
+        it will be performed even if the deletion fails.
+        """
+        ids = list(self.values_list("club_id", "user_id"))
+        nb_rows, _ = super().delete()
+        if nb_rows > 0:
+            for club_id, user_id in ids:
+                cache.set(f"membership_{club_id}_{user_id}", "not_member")
+
+
 class Membership(models.Model):
     """
     The Membership class makes the connection between User and Clubs
@@ -290,6 +362,8 @@ class Membership(models.Model):
         _("description"), max_length=128, null=False, blank=True
     )
 
+    objects = MembershipQuerySet.as_manager()
+
     def __str__(self):
         return (
             self.club.name
@@ -304,24 +378,34 @@ class Membership(models.Model):
         """
         Method to see if that object can be super edited by the given user
         """
-        return user.is_in_group(settings.SITH_MAIN_BOARD_GROUP)
+        if user.is_anonymous:
+            return False
+        return user.is_board_member
 
-    def can_be_edited_by(self, user, membership=None):
+    def can_be_edited_by(self, user: User) -> bool:
         """
-        Method to see if that object can be edited by the given user
+        Check if that object can be edited by the given user
         """
-        if user.memberships:
+        if user.is_root or user.is_board_member:
-            if membership:  # This is for optimisation purpose
+            return True
-                ms = membership
+        membership = self.club.get_membership_for(user)
-            else:
+        if membership is not None and membership.role >= self.role:
-                ms = user.memberships.filter(club=self.club, end_date=None).first()
+            return True
-            return (ms and ms.role >= self.role) or user.is_in_group(
+        return False
-                settings.SITH_MAIN_BOARD_GROUP
-            )
-        return user.is_in_group(settings.SITH_MAIN_BOARD_GROUP)
 
     def get_absolute_url(self):
-        return reverse("club:club_members", kwargs={"club_id": self.club.id})
+        return reverse("club:club_members", kwargs={"club_id": self.club_id})
+
+    def save(self, *args, **kwargs):
+        super().save(*args, **kwargs)
+        if self.end_date is None:
+            cache.set(f"membership_{self.club_id}_{self.user_id}", self)
+        else:
+            cache.set(f"membership_{self.club_id}_{self.user_id}", "not_member")
+
+    def delete(self, *args, **kwargs):
+        super().delete(*args, **kwargs)
+        cache.delete(f"membership_{self.club_id}_{self.user_id}")
 
 
 class Mailing(models.Model):
@@ -374,14 +458,12 @@ class Mailing(models.Model):
         return self.email + "@" + settings.SITH_MAILING_DOMAIN
 
     def can_moderate(self, user):
-        return user.is_root or user.is_in_group(settings.SITH_GROUP_COM_ADMIN_ID)
+        return user.is_root or user.is_com_admin
 
     def is_owned_by(self, user):
-        return (
+        if user.is_anonymous:
-            user.is_in_group(self)
+            return False
-            or user.is_root
+        return user.is_root or user.is_com_admin
-            or user.is_in_group(settings.SITH_GROUP_COM_ADMIN_ID)
-        )
 
     def can_view(self, user):
         return self.club.has_rights_in_club(user)
@@ -389,9 +471,8 @@ class Mailing(models.Model):
     def can_be_edited_by(self, user):
         return self.club.has_rights_in_club(user)
 
-    def delete(self):
+    def delete(self, *args, **kwargs):
-        for sub in self.subscriptions.all():
+        self.subscriptions.all().delete()
-            sub.delete()
         super(Mailing, self).delete()
 
     def fetch_format(self):
@@ -464,10 +545,12 @@ class MailingSubscription(models.Model):
         super(MailingSubscription, self).clean()
 
     def is_owned_by(self, user):
+        if user.is_anonymous:
+            return False
         return (
             self.mailing.club.has_rights_in_club(user)
             or user.is_root
-            or self.user.is_in_group(settings.SITH_GROUP_COM_ADMIN_ID)
+            or self.user.is_com_admin
         )
 
     def can_be_edited_by(self, user):

club/templates/club/club_members.jinja

@@ -13,13 +13,15 @@
         {% endif %}
         <table>
             <thead>
-                <td>{% trans %}User{% endtrans %}</td>
+                <tr>
-                <td>{% trans %}Role{% endtrans %}</td>
+                    <td>{% trans %}User{% endtrans %}</td>
-                <td>{% trans %}Description{% endtrans %}</td>
+                    <td>{% trans %}Role{% endtrans %}</td>
-                <td>{% trans %}Since{% endtrans %}</td>
+                    <td>{% trans %}Description{% endtrans %}</td>
-                {% if users_old %}
+                    <td>{% trans %}Since{% endtrans %}</td>
-                    <td>{% trans %}Mark as old{% endtrans %}</td>
+                    {% if users_old %}
-                {% endif %}
+                        <td>{% trans %}Mark as old{% endtrans %}</td>
+                    {% endif %}
+                </tr>
             </thead>
             <tbody>
             {% for m in members %}

club/tests.py

@@ -13,376 +13,564 @@
 # OR WITHIN THE LOCAL FILE "LICENSE"
 #
 #
+from datetime import timedelta
 
 from django.conf import settings
+from django.core.cache import cache
 from django.test import TestCase
 from django.utils import timezone, html
+from django.utils.timezone import now, localtime
 from django.utils.translation import gettext as _
 from django.urls import reverse
 from django.core.management import call_command
-from django.core.exceptions import ValidationError, NON_FIELD_ERRORS
 
-from core.models import User
+from core.models import User, AnonymousUser
 from club.models import Club, Membership, Mailing
 from club.forms import MailingForm
-from sith.settings import SITH_BAR_MANAGER
+from sith.settings import SITH_BAR_MANAGER, SITH_MAIN_CLUB_ID
-
-
-# Create your tests here.
 
 
 class ClubTest(TestCase):
+    """
+    Set up data for test cases related to clubs and membership
+    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` has two board members : skia (role 3) and comptable (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.
+    """
+
+    @classmethod
+    def setUpTestData(cls):
+        # subscribed users - initial members
+        cls.skia = User.objects.get(username="skia")
+        cls.richard = User.objects.get(username="rbatsbak")
+        cls.comptable = User.objects.get(username="comptable")
+        cls.sli = User.objects.get(username="sli")
+
+        # subscribed users - not initial members
+        cls.krophil = User.objects.get(username="krophil")
+        cls.subscriber = User.objects.get(username="subscriber")
+
+        # old subscriber
+        cls.old_subscriber = User.objects.get(username="old_subscriber")
+
+        # not subscribed
+        cls.public = User.objects.get(username="public")
+
+        cls.ae = Club.objects.filter(pk=SITH_MAIN_CLUB_ID)[0]
+
     def setUp(self):
-        self.skia = User.objects.filter(username="skia").first()
+        # by default, Skia is in the AE, which creates side effect
-        self.rbatsbak = User.objects.filter(username="rbatsbak").first()
+        self.skia.memberships.all().delete()
-        self.guy = User.objects.filter(username="guy").first()
-        self.bdf = Club.objects.filter(unix_name=SITH_BAR_MANAGER["unix_name"]).first()
 
-    def test_create_add_user_to_club_from_root_ok(self):
+        # create a fake club
-        self.client.login(username="root", password="plop")
+        self.club = Club.objects.create(
-        self.client.post(
+            name="Fake Club",
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
+            unix_name="fake-club",
-            {"users": self.skia.id, "start_date": "12/06/2016", "role": 3},
+            address="5 rue de la République, 90000 Belfort",
         )
-        response = self.client.get(
+        self.members_url = reverse(
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id})
+            "club:club_members", kwargs={"club_id": self.club.id}
         )
-        self.assertTrue(response.status_code == 200)
+        a_month_ago = now() - timedelta(days=30)
-        self.assertTrue(
+        yesterday = now() - timedelta(days=1)
-            "S&#39; Kia</a></td>\\n                    <td>Responsable info</td>"
+        Membership.objects.create(
-            in str(response.content)
+            club=self.club, user=self.skia, start_date=a_month_ago, role=3
+        )
+        Membership.objects.create(club=self.club, user=self.richard, role=1)
+        Membership.objects.create(
+            club=self.club, user=self.comptable, start_date=a_month_ago, role=10
         )
 
-    def test_create_add_multiple_user_to_club_from_root_ok(self):
+        # sli was a member but isn't anymore
+        Membership.objects.create(
+            club=self.club,
+            user=self.sli,
+            start_date=a_month_ago,
+            end_date=yesterday,
+            role=2,
+        )
+        cache.clear()
+
+
+class MembershipQuerySetTest(ClubTest):
+    def test_ongoing(self):
+        """
+        Test that the ongoing queryset method returns the memberships that
+        are not ended.
+        """
+        current_members = self.club.members.ongoing()
+        expected = [
+            self.skia.memberships.get(club=self.club),
+            self.comptable.memberships.get(club=self.club),
+            self.richard.memberships.get(club=self.club),
+        ]
+        self.assertEqual(len(current_members), len(expected))
+        for member in current_members:
+            self.assertIn(member, expected)
+
+    def test_board(self):
+        """
+        Test that the board queryset method returns the memberships
+        of user in the club board
+        """
+        board_members = list(self.club.members.board())
+        expected = [
+            self.skia.memberships.get(club=self.club),
+            self.comptable.memberships.get(club=self.club),
+            # sli is no more member, but he was in the board
+            self.sli.memberships.get(club=self.club),
+        ]
+        self.assertEqual(len(board_members), len(expected))
+        for member in board_members:
+            self.assertIn(member, expected)
+
+    def test_ongoing_board(self):
+        """
+        Test that combining ongoing and board returns users
+        who are currently board members of the club
+        """
+        members = list(self.club.members.ongoing().board())
+        expected = [
+            self.skia.memberships.get(club=self.club),
+            self.comptable.memberships.get(club=self.club),
+        ]
+        self.assertEqual(len(members), len(expected))
+        for member in members:
+            self.assertIn(member, 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)
+        cache.set(f"membership_{mem_skia.club_id}_{mem_skia.user_id}", mem_skia)
+        self.skia.memberships.update(end_date=localtime(now()).date())
+        self.assertEqual(
+            cache.get(f"membership_{mem_skia.club_id}_{mem_skia.user_id}"), "not_member"
+        )
+
+        mem_richard = self.richard.memberships.get(club=self.club)
+        cache.set(
+            f"membership_{mem_richard.club_id}_{mem_richard.user_id}", mem_richard
+        )
+        self.richard.memberships.update(role=5)
+        new_mem = self.richard.memberships.get(club=self.club)
+        self.assertNotEqual(new_mem, "not_member")
+        self.assertEqual(new_mem.role, 5)
+
+    def test_delete_invalidate_cache(self):
+        """
+        Test that the `delete` queryset properly invalidate cache
+        """
+
+        mem_skia = self.skia.memberships.get(club=self.club)
+        mem_comptable = self.comptable.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
+        self.club.members.ongoing().board().delete()
+
+        self.assertEqual(
+            cache.get(f"membership_{mem_skia.club_id}_{mem_skia.user_id}"), "not_member"
+        )
+        self.assertEqual(
+            cache.get(f"membership_{mem_comptable.club_id}_{mem_comptable.user_id}"),
+            "not_member",
+        )
+
+
+class ClubModelTest(ClubTest):
+    def assert_membership_just_started(self, user: User, role: int):
+        """
+        Assert that the given membership is active and started today
+        """
+        membership = user.memberships.ongoing().filter(club=self.club).first()
+        self.assertIsNotNone(membership)
+        self.assertEqual(localtime(now()).date(), membership.start_date)
+        self.assertIsNone(membership.end_date)
+        self.assertEqual(membership.role, role)
+        self.assertEqual(membership.club.get_membership_for(user), membership)
+        member_group = self.club.unix_name + settings.SITH_MEMBER_SUFFIX
+        board_group = self.club.unix_name + settings.SITH_BOARD_SUFFIX
+        self.assertTrue(user.is_in_group(name=member_group))
+        self.assertTrue(user.is_in_group(name=board_group))
+
+    def assert_membership_just_ended(self, user: User):
+        """
+        Assert that the given user have a membership which ended today
+        """
+        today = localtime(now()).date()
+        self.assertIsNotNone(
+            user.memberships.filter(club=self.club, end_date=today).first()
+        )
+        self.assertIsNone(self.club.get_membership_for(user))
+
+    def test_access_unauthorized(self):
+        """
+        Test that users who never subscribed and anonymous users
+        cannot see the page
+        """
+        response = self.client.post(self.members_url)
+        self.assertEqual(response.status_code, 403)
+
+        self.client.login(username="public", password="plop")
+        response = self.client.post(self.members_url)
+        self.assertEqual(response.status_code, 403)
+
+    def test_display(self):
+        """
+        Test that a GET request return a page where the requested
+        information are displayed.
+        """
+        self.client.login(username=self.skia.username, password="plop")
+        response = self.client.get(self.members_url)
+        self.assertEqual(response.status_code, 200)
+        expected_html = (
+            "<table><thead><tr>"
+            "<td>Utilisateur</td><td>Rôle</td><td>Description</td>"
+            "<td>Depuis</td><td>Marquer comme ancien</td>"
+            "</tr></thead><tbody>"
+        )
+        memberships = self.club.members.ongoing().order_by("-role")
+        input_id = 0
+        for membership in memberships.select_related("user"):
+            user = membership.user
+            expected_html += (
+                f"<tr><td><a href=\"{reverse('core:user_profile', args=[user.id])}\">"
+                f"{user.get_display_name()}</a></td>"
+                f"<td>{settings.SITH_CLUB_ROLES[membership.role]}</td>"
+                f"<td>{membership.description}</td>"
+                f"<td>{membership.start_date}</td><td>"
+            )
+            if membership.role <= 3:  # 3 is the role of skia
+                expected_html += (
+                    '<input type="checkbox" name="users_old" '
+                    f'value="{user.id}" '
+                    f'id="id_users_old_{input_id}">'
+                )
+                input_id += 1
+            expected_html += "</td></tr>"
+        expected_html += "</tbody></table>"
+        self.assertInHTML(expected_html, response.content.decode())
+
+    def test_root_add_one_club_member(self):
+        """
+        Test that root users can add members to clubs, one at a time
+        """
         self.client.login(username="root", password="plop")
-        self.client.post(
+        response = self.client.post(
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
+            self.members_url,
+            {"users": self.subscriber.id, "role": 3},
+        )
+        self.assertRedirects(response, self.members_url)
+        self.subscriber.refresh_from_db()
+        self.assert_membership_just_started(self.subscriber, role=3)
+
+    def test_root_add_multiple_club_member(self):
+        """
+        Test that root users can add multiple members at once to clubs
+        """
+        self.client.login(username="root", password="plop")
+        response = self.client.post(
+            self.members_url,
             {
-                "users": "|%d|%d|" % (self.skia.id, self.rbatsbak.id),
+                "users": f"|{self.subscriber.id}|{self.krophil.id}|",
-                "start_date": "12/06/2016",
                 "role": 3,
             },
         )
-        response = self.client.get(
+        self.assertRedirects(response, self.members_url)
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id})
+        self.subscriber.refresh_from_db()
-        )
+        self.assert_membership_just_started(self.subscriber, role=3)
-        self.assertTrue(response.status_code == 200)
+        self.assert_membership_just_started(self.krophil, role=3)
-        content = str(response.content)
-        self.assertTrue(
-            "S&#39; Kia</a></td>\\n                    <td>Responsable info</td>"
-            in content
-        )
-        self.assertTrue(
-            "Richard Batsbak</a></td>\\n                    <td>Responsable info</td>"
-            in content
-        )
 
-    def test_create_add_user_to_club_from_root_fail_not_subscriber(self):
+    def test_add_unauthorized_members(self):
+        """
+        Test that users who are not currently subscribed
+        cannot be members of clubs.
+        """
         self.client.login(username="root", password="plop")
         response = self.client.post(
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
+            self.members_url,
-            {"users": self.guy.id, "start_date": "12/06/2016", "role": 3},
+            {"users": self.public.id, "role": 1},
         )
-        self.assertTrue(response.status_code == 200)
+        self.assertIsNone(self.public.memberships.filter(club=self.club).first())
         self.assertTrue('<ul class="errorlist"><li>' in str(response.content))
-        response = self.client.get(
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id})
-        )
-        self.assertFalse(
-            "Guy Carlier</a></td>\\n                    <td>Responsable info</td>"
-            in str(response.content)
-        )
 
-    def test_create_add_user_to_club_from_root_fail_already_in_club(self):
+        response = self.client.post(
+            self.members_url,
+            {"users": self.old_subscriber.id, "role": 1},
+        )
+        self.assertIsNone(self.public.memberships.filter(club=self.club).first())
+        self.assertIsNone(self.club.get_membership_for(self.public))
+        self.assertTrue('<ul class="errorlist"><li>' in str(response.content))
+
+    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.login(username="root", password="plop")
+        current_membership = self.skia.memberships.ongoing().get(club=self.club)
+        nb_memberships = self.skia.memberships.count()
         self.client.post(
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
+            self.members_url,
-            {"users": self.skia.id, "start_date": "12/06/2016", "role": 3},
+            {"users": self.skia.id, "role": current_membership.role + 1},
-        )
-        response = self.client.get(
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id})
-        )
-        self.assertTrue(
-            "S&#39; Kia</a></td>\\n                    <td>Responsable info</td>"
-            in str(response.content)
-        )
-        response = self.client.post(
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
-            {"users": self.skia.id, "start_date": "12/06/2016", "role": 4},
-        )
-        self.assertTrue(response.status_code == 200)
-        self.assertFalse(
-            "S&#39; Kia</a></td>\\n                <td>Secrétaire</td>"
-            in str(response.content)
         )
+        self.skia.refresh_from_db()
+        self.assertEqual(nb_memberships, self.skia.memberships.count())
+        new_membership = self.skia.memberships.ongoing().get(club=self.club)
+        self.assertEqual(current_membership, new_membership)
+        self.assertEqual(self.club.get_membership_for(self.skia), new_membership)
 
-    def test_create_add_user_non_existent_to_club_from_root_fail(self):
+    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.login(username="root", password="plop")
+        nb_memberships = self.club.members.count()
         response = self.client.post(
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
+            self.members_url,
-            {"users": [9999], "start_date": "12/06/2016", "role": 3},
+            {"users": [9999], "role": 1},
         )
-        self.assertTrue(response.status_code == 200)
+        self.assertContains(response, '<ul class="errorlist"><li>')
-        content = str(response.content)
+        self.club.refresh_from_db()
-        self.assertTrue('<ul class="errorlist"><li>' in content)
+        self.assertEqual(self.club.members.count(), nb_memberships)
-        self.assertFalse("<td>Responsable info</td>" in content)
-        self.client.login(username="root", password="plop")
         response = self.client.post(
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
+            self.members_url,
             {
-                "users": "|%d|%d|" % (self.skia.id, 9999),
+                "users": f"|{self.subscriber.id}|{9999}|",
                 "start_date": "12/06/2016",
                 "role": 3,
             },
         )
-        self.assertTrue(response.status_code == 200)
+        self.assertContains(response, '<ul class="errorlist"><li>')
-        content = str(response.content)
+        self.club.refresh_from_db()
-        self.assertTrue('<ul class="errorlist"><li>' in content)
+        self.assertEqual(self.club.members.count(), nb_memberships)
-        self.assertFalse("<td>Responsable info</td>" in content)
 
-    def test_create_add_user_to_club_from_skia_ok(self):
+    def test_president_add_members(self):
-        self.client.login(username="root", password="plop")
+        """
-        self.client.post(
+        Test that the president of the club can add members
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
+        """
-            {"users": self.skia.id, "start_date": "12/06/2016", "role": 10},
+        president = self.club.members.get(role=10).user
+        nb_club_membership = self.club.members.count()
+        nb_subscriber_memberships = self.subscriber.memberships.count()
+        self.client.login(username=president.username, password="plop")
+        response = self.client.post(
+            self.members_url,
+            {"users": self.subscriber.id, "role": 9},
         )
-        self.client.login(username="skia", password="plop")
+        self.assertRedirects(response, self.members_url)
-        self.client.post(
+        self.club.refresh_from_db()
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
+        self.subscriber.refresh_from_db()
-            {"users": self.rbatsbak.id, "start_date": "12/06/2016", "role": 9},
+        self.assertEqual(self.club.members.count(), nb_club_membership + 1)
+        self.assertEqual(
+            self.subscriber.memberships.count(), nb_subscriber_memberships + 1
         )
-        response = self.client.get(
+        self.assert_membership_just_started(self.subscriber, role=9)
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id})
+
+    def test_add_member_greater_role(self):
+        """
+        Test that a member of the club member cannot create
+        a membership with a greater role than its own.
+        """
+        self.client.login(username=self.skia.username, password="plop")
+        nb_memberships = self.club.members.count()
+        response = self.client.post(
+            self.members_url,
+            {"users": self.subscriber.id, "role": 10},
         )
         self.assertEqual(response.status_code, 200)
-        self.assertIn(
+        self.assertInHTML(
-            """Richard Batsbak</a></td>\n                    <td>Vice-Président⸱e</td>""",
+            "<li>Vous n'avez pas la permission de faire cela</li>",
             response.content.decode(),
         )
+        self.club.refresh_from_db()
+        self.assertEqual(nb_memberships, self.club.members.count())
+        self.assertIsNone(self.subscriber.memberships.filter(club=self.club).first())
 
-    def test_create_add_user_to_club_from_richard_fail(self):
+    def test_add_member_without_role(self):
-        self.client.login(username="root", password="plop")
+        """
-        self.client.post(
+        Test that trying to add members without specifying their role fails
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
+        """
-            {"users": self.rbatsbak.id, "start_date": "12/06/2016", "role": 3},
-        )
-        self.client.login(username="rbatsbak", password="plop")
-        response = self.client.post(
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
-            {"users": self.skia.id, "start_date": "12/06/2016", "role": 10},
-        )
-        self.assertTrue(response.status_code == 200)
-        self.assertTrue(
-            "<li>Vous n&#x27;avez pas la permission de faire cela</li>"
-            in str(response.content)
-        )
-
-    def test_role_required_if_users_specified(self):
         self.client.login(username="root", password="plop")
         response = self.client.post(
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
+            self.members_url,
-            {"users": self.rbatsbak.id, "start_date": "12/06/2016"},
+            {"users": self.subscriber.id, "start_date": "12/06/2016"},
         )
         self.assertTrue(
             '<ul class="errorlist"><li>Vous devez choisir un r' in str(response.content)
         )
 
-    def test_mark_old_user_to_club_from_skia_ok(self):
+    def test_end_membership_self(self):
-        self.client.login(username="root", password="plop")
+        """
-        self.client.post(
+        Test that a member can end its own membership
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
+        """
-            {
-                "users": "|%d|%d|" % (self.skia.id, self.rbatsbak.id),
-                "start_date": "12/06/2016",
-                "role": 3,
-            },
-        )
         self.client.login(username="skia", password="plop")
-        response = self.client.post(
+        self.client.post(
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
+            self.members_url,
-            {"users_old": self.rbatsbak.id},
-        )
-        self.assertTrue(response.status_code == 302)
-
-        response = self.client.get(
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id})
-        )
-        self.assertTrue(response.status_code == 200)
-        content = str(response.content)
-        self.assertFalse(
-            "Richard Batsbak</a></td>\\n                    <td>Responsable info</td>"
-            in content
-        )
-        self.assertTrue(
-            "S&#39; Kia</a></td>\\n                    <td>Responsable info</td>"
-            in content
-        )
-
-        # Skia is board member so he should be able to mark as old even without being in the club
-        response = self.client.post(
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
             {"users_old": self.skia.id},
         )
+        self.skia.refresh_from_db()
+        self.assert_membership_just_ended(self.skia)
+
+    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
+        self.client.login(username=self.skia.username, password="plop")
+        response = self.client.post(
+            self.members_url,
+            {"users_old": self.richard.id},
+        )
+        self.assertRedirects(response, self.members_url)
+        self.club.refresh_from_db()
+        self.assert_membership_just_ended(self.richard)
+
+    def test_end_membership_higher_role(self):
+        """
+        Test that board members of the club cannot end memberships
+        of users with higher roles
+        """
+        membership = self.comptable.memberships.filter(club=self.club).first()
+        self.client.login(username=self.skia.username, password="plop")
+        self.client.post(
+            self.members_url,
+            {"users_old": self.comptable.id},
+        )
+        self.club.refresh_from_db()
+        new_membership = self.club.get_membership_for(self.comptable)
+        self.assertIsNotNone(new_membership)
+        self.assertEqual(new_membership, membership)
+
+        membership = self.comptable.memberships.filter(club=self.club).first()
+        self.assertIsNone(membership.end_date)
+
+    def test_end_membership_as_main_club_board(self):
+        """
+        Test that board members of the main club can end the membership
+        of anyone
+        """
+        # make subscriber a board member
+        self.subscriber.memberships.all().delete()
+        Membership.objects.create(club=self.ae, user=self.subscriber, role=3)
+
+        nb_memberships = self.club.members.count()
+        self.client.login(username=self.subscriber.username, password="plop")
+        response = self.client.post(
+            self.members_url,
+            {"users_old": self.comptable.id},
+        )
+        self.assertRedirects(response, self.members_url)
+        self.assert_membership_just_ended(self.comptable)
+        self.assertEqual(self.club.members.ongoing().count(), nb_memberships - 1)
+
+    def test_end_membership_as_root(self):
+        """
+        Test that root users can end the membership of anyone
+        """
+        nb_memberships = self.club.members.count()
         self.client.login(username="root", password="plop")
-        self.client.post(
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
-            {"users": self.rbatsbak.id, "start_date": "12/06/2016", "role": 3},
-        )
-        self.client.login(username="skia", password="plop")
         response = self.client.post(
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
+            self.members_url,
-            {"users_old": self.rbatsbak.id},
+            {"users_old": [self.comptable.id]},
-        )
-        self.assertFalse(
-            "Richard Batsbak</a></td>\\n                    <td>Responsable info</td>"
-            in str(response.content)
         )
+        self.assertRedirects(response, self.members_url)
+        self.assert_membership_just_ended(self.comptable)
+        self.assertEqual(self.club.members.ongoing().count(), nb_memberships - 1)
+        self.assertEqual(self.club.members.count(), nb_memberships)
 
-    def test_mark_old_multiple_users_from_skia_ok(self):
+    def test_end_membership_as_foreigner(self):
-        self.client.login(username="root", password="plop")
+        """
+        Test that users who are not in this club cannot end its memberships
+        """
+        nb_memberships = self.club.members.count()
+        membership = self.richard.memberships.filter(club=self.club).first()
+        self.client.login(username="subscriber", password="root")
         self.client.post(
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
+            self.members_url,
-            {
+            {"users_old": [self.richard.id]},
-                "users": "|%d|%d|" % (self.skia.id, self.rbatsbak.id),
-                "start_date": "12/06/2016",
-                "role": 3,
-            },
         )
-        self.client.login(username="skia", password="plop")
+        # nothing should have changed
-        response = self.client.post(
+        new_mem = self.club.get_membership_for(self.richard)
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
+        self.assertIsNotNone(new_mem)
-            {"users_old": [self.rbatsbak.id, self.skia.id]},
+        self.assertEqual(self.club.members.count(), nb_memberships)
-        )
+        self.assertEqual(membership, new_mem)
-        self.assertTrue(response.status_code == 302)
 
-        response = self.client.get(
+    def test_delete_remove_from_meta_group(self):
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id})
+        """
-        )
+        Test that when a club is deleted, all its members are removed from the
-        self.assertTrue(response.status_code == 200)
+        associated metagroup
-        content = str(response.content)
+        """
-        self.assertFalse(
+        memberships = self.club.members.select_related("user")
-            "Richard Batsbak</a></td>\\n                    <td>Responsable info</td>"
+        users = [membership.user for membership in memberships]
-            in content
+        meta_group = self.club.unix_name + settings.SITH_MEMBER_SUFFIX
-        )
-        self.assertFalse(
-            "S&#39; Kia</a></td>\\n                    <td>Responsable info</td>"
-            in content
-        )
 
-    def test_mark_old_user_to_club_from_richard_ok(self):
+        self.club.delete()
-        self.client.login(username="root", password="plop")
+        for user in users:
-        self.client.post(
+            self.assertFalse(user.is_in_group(name=meta_group))
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
-            {
-                "users": "|%d|%d|" % (self.skia.id, self.rbatsbak.id),
-                "start_date": "12/06/2016",
-                "role": 3,
-            },
-        )
 
-        # Test with equal rights
+    def test_add_to_meta_group(self):
-        self.client.login(username="rbatsbak", password="plop")
+        """
-        response = self.client.post(
+        Test that when a membership begins, the user is added to the meta group
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
+        """
-            {"users_old": self.skia.id},
+        group_members = self.club.unix_name + settings.SITH_MEMBER_SUFFIX
-        )
+        board_members = self.club.unix_name + settings.SITH_BOARD_SUFFIX
-        self.assertTrue(response.status_code == 302)
+        self.assertFalse(self.subscriber.is_in_group(name=group_members))
+        self.assertFalse(self.subscriber.is_in_group(name=board_members))
+        Membership.objects.create(club=self.club, user=self.subscriber, role=3)
+        self.assertTrue(self.subscriber.is_in_group(name=group_members))
+        self.assertTrue(self.subscriber.is_in_group(name=board_members))
 
-        response = self.client.get(
+    def test_remove_from_meta_group(self):
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id})
+        """
-        )
+        Test that when a membership ends, the user is removed from meta group
-        self.assertTrue(response.status_code == 200)
+        """
-        content = str(response.content)
+        group_members = self.club.unix_name + settings.SITH_MEMBER_SUFFIX
-        self.assertTrue(
+        board_members = self.club.unix_name + settings.SITH_BOARD_SUFFIX
-            "Richard Batsbak</a></td>\\n                    <td>Responsable info</td>"
+        self.assertTrue(self.comptable.is_in_group(name=group_members))
-            in content
+        self.assertTrue(self.comptable.is_in_group(name=board_members))
-        )
+        self.comptable.memberships.update(end_date=localtime(now()))
-        self.assertFalse(
+        self.assertFalse(self.comptable.is_in_group(name=group_members))
-            "S&#39; Kia</a></td>\\n                    <td>Responsable info</td>"
+        self.assertFalse(self.comptable.is_in_group(name=board_members))
-            in content
-        )
 
-        # Test with lower rights
+    def test_club_owner(self):
-        self.client.post(
+        """
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
+        Test that a club is owned only by board members of the main club
-            {"users": self.skia.id, "start_date": "12/06/2016", "role": 0},
+        """
-        )
+        anonymous = AnonymousUser()
+        self.assertFalse(self.club.is_owned_by(anonymous))
+        self.assertFalse(self.club.is_owned_by(self.subscriber))
 
-        self.client.post(
+        # make sli a board member
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
+        self.sli.memberships.all().delete()
-            {"users_old": self.skia.id},
+        Membership(club=self.ae, user=self.sli, role=3).save()
-        )
+        self.assertTrue(self.club.is_owned_by(self.sli))
-        response = self.client.get(
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id})
-        )
-        self.assertTrue(response.status_code == 200)
-        content = str(response.content)
-        self.assertTrue(
-            "Richard Batsbak</a></td>\\n                    <td>Responsable info</td>"
-            in content
-        )
-        self.assertFalse(
-            "S&#39; Kia</a></td>\\n                    <td>Curieux</td>" in content
-        )
-
-    def test_mark_old_user_to_club_from_richard_fail(self):
-        self.client.login(username="root", password="plop")
-        self.client.post(
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
-            {"users": self.skia.id, "start_date": "12/06/2016", "role": 3},
-        )
-
-        # Test with richard outside of the club
-        self.client.login(username="rbatsbak", password="plop")
-        response = self.client.post(
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
-            {"users_old": self.skia.id},
-        )
-        self.assertTrue(response.status_code == 200)
-
-        response = self.client.get(
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id})
-        )
-        self.assertTrue(response.status_code == 200)
-        self.assertTrue(
-            "S&#39; Kia</a></td>\\n                    <td>Responsable info</td>"
-            in str(response.content)
-        )
-
-        # Test with lower rights
-        self.client.post(
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
-            {"users": self.rbatsbak.id, "start_date": "12/06/2016", "role": 0},
-        )
-
-        self.client.post(
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id}),
-            {"users_old": self.skia.id},
-        )
-        response = self.client.get(
-            reverse("club:club_members", kwargs={"club_id": self.bdf.id})
-        )
-        self.assertEqual(response.status_code, 200)
-        content = response.content.decode()
-        self.assertIn(
-            "Richard Batsbak</a></td>\n                    <td>Curieux⸱euse</td>",
-            content,
-        )
-        self.assertIn(
-            "S&#39; Kia</a></td>\n                    <td>Responsable info</td>",
-            content,
-        )
 
 
 class MailingFormTest(TestCase):
     """Perform validation tests for MailingForm"""
 
+    @classmethod
+    def setUpTestData(cls):
+        cls.skia = User.objects.filter(username="skia").first()
+        cls.rbatsbak = User.objects.filter(username="rbatsbak").first()
+        cls.krophil = User.objects.filter(username="krophil").first()
+        cls.comunity = User.objects.filter(username="comunity").first()
+        cls.bdf = Club.objects.filter(unix_name=SITH_BAR_MANAGER["unix_name"]).first()
+
     def setUp(self):
-        self.skia = User.objects.filter(username="skia").first()
-        self.rbatsbak = User.objects.filter(username="rbatsbak").first()
-        self.krophil = User.objects.filter(username="krophil").first()
-        self.comunity = User.objects.filter(username="comunity").first()
-        self.bdf = Club.objects.filter(unix_name=SITH_BAR_MANAGER["unix_name"]).first()
         Membership(
             user=self.rbatsbak,
             club=self.bdf,

club/views.py

@@ -306,9 +306,7 @@ class ClubMembersView(ClubTabsMixin, CanViewMixin, DetailFormView):
         return resp
 
     def dispatch(self, request, *args, **kwargs):
-        self.members = (
+        self.members = self.get_object().members.ongoing().order_by("-role")
-            self.get_object().members.filter(end_date=None).order_by("-role").all()
-        )
         return super(ClubMembersView, self).dispatch(request, *args, **kwargs)
 
     def get_success_url(self, **kwargs):

com/models.py

@@ -50,7 +50,9 @@ class Sith(models.Model):
     version = utils.get_git_revision_short_hash()
 
     def is_owned_by(self, user):
-        return user.is_in_group(settings.SITH_GROUP_COM_ADMIN_ID)
+        if user.is_anonymous:
+            return False
+        return user.is_com_admin
 
     def __str__(self):
         return "⛩ Sith ⛩"
@@ -92,13 +94,15 @@ class News(models.Model):
     )
 
     def is_owned_by(self, user):
-        return user.is_in_group(settings.SITH_GROUP_COM_ADMIN_ID) or user == self.author
+        if user.is_anonymous:
+            return False
+        return user.is_com_admin or user == self.author
 
     def can_be_edited_by(self, user):
-        return user.is_in_group(settings.SITH_GROUP_COM_ADMIN_ID)
+        return user.is_com_admin
 
     def can_be_viewed_by(self, user):
-        return self.is_moderated or user.is_in_group(settings.SITH_GROUP_COM_ADMIN_ID)
+        return self.is_moderated or user.is_com_admin
 
     def get_absolute_url(self):
         return reverse("com:news_detail", kwargs={"news_id": self.id})
@@ -243,7 +247,9 @@ class Weekmail(models.Model):
         return "Weekmail %s (sent: %s) - %s" % (self.id, self.sent, self.title)
 
     def is_owned_by(self, user):
-        return user.is_in_group(settings.SITH_GROUP_COM_ADMIN_ID)
+        if user.is_anonymous:
+            return False
+        return user.is_com_admin
 
 
 class WeekmailArticle(models.Model):
@@ -271,7 +277,9 @@ class WeekmailArticle(models.Model):
     rank = models.IntegerField(_("rank"), default=-1)
 
     def is_owned_by(self, user):
-        return user.is_in_group(settings.SITH_GROUP_COM_ADMIN_ID)
+        if user.is_anonymous:
+            return False
+        return user.is_com_admin
 
     def __str__(self):
         return "%s - %s (%s)" % (self.title, self.author, self.club)
@@ -287,7 +295,9 @@ class Screen(models.Model):
         )
 
     def is_owned_by(self, user):
-        return user.is_in_group(settings.SITH_GROUP_COM_ADMIN_ID)
+        if user.is_anonymous:
+            return False
+        return user.is_com_admin
 
     def __str__(self):
         return "%s" % (self.name)
@@ -340,12 +350,12 @@ class Poster(models.Model):
             raise ValidationError(_("Begin date should be before end date"))
 
     def is_owned_by(self, user):
-        return user.is_in_group(
+        if user.is_anonymous:
-            settings.SITH_GROUP_COM_ADMIN_ID
+            return False
-        ) or Club.objects.filter(id__in=user.clubs_with_rights)
+        return user.is_com_admin or len(user.clubs_with_rights) > 0
 
     def can_be_moderated_by(self, user):
-        return user.is_in_group(settings.SITH_GROUP_COM_ADMIN_ID)
+        return user.is_com_admin
 
     def get_display_name(self):
         return self.club.get_display_name()

com/templates/com/news_detail.jinja

@@ -35,7 +35,7 @@
         <p>{% trans %}Author: {% endtrans %}{{ user_profile_link(news.author) }}</p>
         {% if news.moderator %}
         <p>{% trans %}Moderator: {% endtrans %}{{ user_profile_link(news.moderator) }}</p>
-        {% elif user.is_in_group(settings.SITH_GROUP_COM_ADMIN_ID) %}
+        {% elif user.is_com_admin %}
         <p> <a href="{{ url('com:news_moderate', news_id=news.id) }}">{% trans %}Moderate{% endtrans %}</a></p>
         {% endif %}
         {% if user.can_edit(news) %}

com/templates/com/news_edit.jinja

@@ -49,7 +49,7 @@
     <p>{{ form.club.errors }}<label for="{{ form.club.name }}">{{ form.club.label }}</label> {{ form.club }}</p>
     <p>{{ form.summary.errors }}<label for="{{ form.summary.name }}">{{ form.summary.label }}</label> {{ form.summary }}</p>
     <p>{{ form.content.errors }}<label for="{{ form.content.name }}">{{ form.content.label }}</label> {{ form.content }}</p>
-    {% if user.is_in_group(settings.SITH_GROUP_COM_ADMIN_ID) %}
+    {% if user.is_com_admin %}
     <p>{{ form.automoderation.errors }}<label for="{{ form.automoderation.name }}">{{ form.automoderation.label }}</label>
         {{ form.automoderation }}</p>
     {% endif %}

com/templates/com/news_list.jinja

@@ -6,7 +6,7 @@
 {% endblock %}
 
 {% block content %}
-{% if user.is_in_group(settings.SITH_GROUP_COM_ADMIN_ID) %}
+{% if user.is_com_admin %}
 <div id="news_admin">
   <a class="button" href="{{ url('com:news_admin_list') }}">{% trans %}Administrate news{% endtrans %}</a>
 </div>

com/tests.py

@@ -13,16 +13,18 @@
 # OR WITHIN THE LOCAL FILE "LICENSE"
 #
 #
-
+from django.core.files.uploadedfile import SimpleUploadedFile
 from django.test import TestCase
 from django.conf import settings
 from django.urls import reverse
 from django.core.management import call_command
 from django.utils import html
+from django.utils.timezone import localtime, now
 from django.utils.translation import gettext as _
 
-
+from club.models import Club, Membership
-from core.models import User, RealGroup
+from com.models import Sith, News, Weekmail, WeekmailArticle, Poster
+from core.models import User, RealGroup, AnonymousUser
 
 
 class ComAlertTest(TestCase):
@@ -110,3 +112,102 @@ class ComTest(TestCase):
                 _("You need an up to date subscription to access this content")
             ),
         )
+
+
+class SithTest(TestCase):
+    def test_sith_owner(self):
+        """
+        Test that the sith instance is owned by com admins
+        and nobody else
+        """
+        sith: Sith = Sith.objects.first()
+
+        com_admin = User.objects.get(username="comunity")
+        self.assertTrue(sith.is_owned_by(com_admin))
+
+        anonymous = AnonymousUser()
+        self.assertFalse(sith.is_owned_by(anonymous))
+
+        sli = User.objects.get(username="sli")
+        self.assertFalse(sith.is_owned_by(sli))
+
+
+class NewsTest(TestCase):
+    @classmethod
+    def setUpTestData(cls):
+        cls.com_admin = User.objects.get(username="comunity")
+        new = News.objects.create(
+            title="dummy new",
+            summary="This is a dummy new",
+            content="Look at that beautiful dummy new",
+            author=User.objects.get(username="subscriber"),
+            club=Club.objects.first(),
+        )
+        cls.new = new
+        cls.author = new.author
+        cls.sli = User.objects.get(username="sli")
+        cls.anonymous = AnonymousUser()
+
+    def test_news_owner(self):
+        """
+        Test that news are owned by com admins
+        or by their author but nobody else
+        """
+
+        self.assertTrue(self.new.is_owned_by(self.com_admin))
+        self.assertTrue(self.new.is_owned_by(self.author))
+        self.assertFalse(self.new.is_owned_by(self.anonymous))
+        self.assertFalse(self.new.is_owned_by(self.sli))
+
+
+class WeekmailArticleTest(TestCase):
+    @classmethod
+    def setUpTestData(cls):
+        cls.com_admin = User.objects.get(username="comunity")
+        author = User.objects.get(username="subscriber")
+        cls.article = WeekmailArticle.objects.create(
+            weekmail=Weekmail.objects.create(),
+            author=author,
+            title="title",
+            content="Some content",
+            club=Club.objects.first(),
+        )
+        cls.author = author
+        cls.sli = User.objects.get(username="sli")
+        cls.anonymous = AnonymousUser()
+
+    def test_weekmail_owner(self):
+        """
+        Test that weekmails are owned only by com admins
+        """
+        self.assertTrue(self.article.is_owned_by(self.com_admin))
+        self.assertFalse(self.article.is_owned_by(self.author))
+        self.assertFalse(self.article.is_owned_by(self.anonymous))
+        self.assertFalse(self.article.is_owned_by(self.sli))
+
+
+class PosterTest(TestCase):
+    @classmethod
+    def setUpTestData(cls):
+        cls.com_admin = User.objects.get(username="comunity")
+        cls.poster = Poster.objects.create(
+            name="dummy",
+            file=SimpleUploadedFile("dummy.jpg", b"azertyuiop"),
+            club=Club.objects.first(),
+            date_begin=localtime(now()),
+        )
+        cls.sli = User.objects.get(username="sli")
+        cls.sli.memberships.all().delete()
+        Membership(user=cls.sli, club=Club.objects.first(), role=5).save()
+        cls.susbcriber = User.objects.get(username="subscriber")
+        cls.anonymous = AnonymousUser()
+
+    def test_poster_owner(self):
+        """
+        Test that poster are owned by com admins and board members in clubs
+        """
+        self.assertTrue(self.poster.is_owned_by(self.com_admin))
+        self.assertFalse(self.poster.is_owned_by(self.anonymous))
+
+        self.assertFalse(self.poster.is_owned_by(self.susbcriber))
+        self.assertTrue(self.poster.is_owned_by(self.sli))

com/views.py

@@ -146,7 +146,7 @@ class ComTabsMixin(TabedViewMixin):
 
 class IsComAdminMixin(View):
     def dispatch(self, request, *args, **kwargs):
-        if not (request.user.is_in_group(settings.SITH_GROUP_COM_ADMIN_ID)):
+        if not request.user.is_com_admin:
             raise PermissionDenied
         return super(IsComAdminMixin, self).dispatch(request, *args, **kwargs)
 
@@ -283,9 +283,7 @@ class NewsEditView(CanEditMixin, UpdateView):
 
     def form_valid(self, form):
         self.object = form.save()
-        if form.cleaned_data["automoderation"] and self.request.user.is_in_group(
+        if form.cleaned_data["automoderation"] and self.request.user.is_com_admin:
-            settings.SITH_GROUP_COM_ADMIN_ID
-        ):
             self.object.moderator = self.request.user
             self.object.is_moderated = True
             self.object.save()
@@ -333,9 +331,7 @@ class NewsCreateView(CanCreateMixin, CreateView):
 
     def form_valid(self, form):
         self.object = form.save()
-        if form.cleaned_data["automoderation"] and self.request.user.is_in_group(
+        if form.cleaned_data["automoderation"] and self.request.user.is_com_admin:
-            settings.SITH_GROUP_COM_ADMIN_ID
-        ):
             self.object.moderator = self.request.user
             self.object.is_moderated = True
             self.object.save()
@@ -617,10 +613,7 @@ class MailingListAdminView(ComTabsMixin, ListView):
     current_tab = "mailings"
 
     def dispatch(self, request, *args, **kwargs):
-        if not (
+        if not (request.user.is_com_admin or request.user.is_root):
-            request.user.is_in_group(settings.SITH_GROUP_COM_ADMIN_ID)
-            or request.user.is_root
-        ):
             raise PermissionDenied
         return super(MailingListAdminView, self).dispatch(request, *args, **kwargs)
 

core/apps.py

@@ -25,6 +25,7 @@
 import sys
 
 from django.apps import AppConfig
+from django.core.cache import cache
 from django.core.signals import request_started
 
 
@@ -33,26 +34,17 @@ class SithConfig(AppConfig):
     verbose_name = "Core app of the Sith"
 
     def ready(self):
-        from core.models import User
-        from club.models import Club
         from forum.models import Forum
+        import core.signals
 
-        def clear_cached_groups(**kwargs):
+        cache.clear()
-            User._group_ids = {}
-            User._group_name = {}
 
         def clear_cached_memberships(**kwargs):
-            User._club_memberships = {}
-            Club._memberships = {}
             Forum._club_memberships = {}
 
         print("Connecting signals!", file=sys.stderr)
-        request_started.connect(
-            clear_cached_groups, weak=False, dispatch_uid="clear_cached_groups"
-        )
         request_started.connect(
             clear_cached_memberships,
             weak=False,
             dispatch_uid="clear_cached_memberships",
         )
-        # TODO: there may be a need to add more cache clearing

core/management/commands/populate.py

@@ -155,12 +155,10 @@ class Command(BaseCommand):
         Counter(name="Eboutic", club=main_club, type="EBOUTIC").save()
         Counter(name="AE", club=main_club, type="OFFICE").save()
 
-        home_root.view_groups.set(
+        ae_members = Group.objects.get(name=settings.SITH_MAIN_MEMBERS_GROUP)
-            [Group.objects.filter(name=settings.SITH_MAIN_MEMBERS_GROUP).first()]
+
-        )
+        home_root.view_groups.set([ae_members])
-        club_root.view_groups.set(
+        club_root.view_groups.set([ae_members])
-            [Group.objects.filter(name=settings.SITH_MAIN_MEMBERS_GROUP).first()]
-        )
         home_root.save()
         club_root.save()
 
@@ -220,9 +218,7 @@ Welcome to the wiki page!
             )
             skia.set_password("plop")
             skia.save()
-            skia.view_groups = [
+            skia.view_groups = [ae_members.id]
-                Group.objects.filter(name=settings.SITH_MAIN_MEMBERS_GROUP).first().id
-            ]
             skia.save()
             skia_profile_path = (
                 root_path
@@ -261,9 +257,7 @@ Welcome to the wiki page!
             )
             public.set_password("plop")
             public.save()
-            public.view_groups = [
+            public.view_groups = [ae_members.id]
-                Group.objects.filter(name=settings.SITH_MAIN_MEMBERS_GROUP).first().id
-            ]
             public.save()
             # Adding user Subscriber
             subscriber = User(
@@ -277,9 +271,7 @@ Welcome to the wiki page!
             )
             subscriber.set_password("plop")
             subscriber.save()
-            subscriber.view_groups = [
+            subscriber.view_groups = [ae_members.id]
-                Group.objects.filter(name=settings.SITH_MAIN_MEMBERS_GROUP).first().id
-            ]
             subscriber.save()
             # Adding user old Subscriber
             old_subscriber = User(
@@ -293,9 +285,7 @@ Welcome to the wiki page!
             )
             old_subscriber.set_password("plop")
             old_subscriber.save()
-            old_subscriber.view_groups = [
+            old_subscriber.view_groups = [ae_members.id]
-                Group.objects.filter(name=settings.SITH_MAIN_MEMBERS_GROUP).first().id
-            ]
             old_subscriber.save()
             # Adding user Counter admin
             counter = User(
@@ -309,9 +299,7 @@ Welcome to the wiki page!
             )
             counter.set_password("plop")
             counter.save()
-            counter.view_groups = [
+            counter.view_groups = [ae_members.id]
-                Group.objects.filter(name=settings.SITH_MAIN_MEMBERS_GROUP).first().id
-            ]
             counter.groups.set(
                 [
                     Group.objects.filter(id=settings.SITH_GROUP_COUNTER_ADMIN_ID)
@@ -332,9 +320,7 @@ Welcome to the wiki page!
             )
             comptable.set_password("plop")
             comptable.save()
-            comptable.view_groups = [
+            comptable.view_groups = [ae_members.id]
-                Group.objects.filter(name=settings.SITH_MAIN_MEMBERS_GROUP).first().id
-            ]
             comptable.groups.set(
                 [
                     Group.objects.filter(id=settings.SITH_GROUP_ACCOUNTING_ADMIN_ID)
@@ -355,9 +341,7 @@ Welcome to the wiki page!
             )
             u.set_password("plop")
             u.save()
-            u.view_groups = [
+            u.view_groups = [ae_members.id]
-                Group.objects.filter(name=settings.SITH_MAIN_MEMBERS_GROUP).first().id
-            ]
             u.save()
             # Adding user Richard Batsbak
             richard = User(
@@ -394,9 +378,7 @@ Welcome to the wiki page!
                 richard_profile.save()
                 richard.profile_pict = richard_profile
                 richard.save()
-            richard.view_groups = [
+            richard.view_groups = [ae_members.id]
-                Group.objects.filter(name=settings.SITH_MAIN_MEMBERS_GROUP).first().id
-            ]
             richard.save()
             # Adding syntax help page
             p = Page(name="Aide_sur_la_syntaxe")
@@ -428,7 +410,7 @@ Welcome to the wiki page!
             default_subscription = "un-semestre"
             # Root
             s = Subscription(
-                member=User.objects.filter(pk=root.pk).first(),
+                member=root,
                 subscription_type=default_subscription,
                 payment_method=settings.SITH_SUBSCRIPTION_PAYMENT_METHOD[0][0],
             )
@@ -528,7 +510,7 @@ Welcome to the wiki page!
             Club(
                 name="Woenzel'UT", unix_name="woenzel", address="Woenzel", parent=guyut
             ).save()
-            Membership(user=skia, club=main_club, role=3, description="").save()
+            Membership(user=skia, club=main_club, role=3).save()
             troll = Club(
                 name="Troll Penché",
                 unix_name="troll",
@@ -855,9 +837,7 @@ Welcome to the wiki page!
             )
             sli.set_password("plop")
             sli.save()
-            sli.view_groups = [
+            sli.view_groups = [ae_members.id]
-                Group.objects.filter(name=settings.SITH_MAIN_MEMBERS_GROUP).first().id
-            ]
             sli.save()
             sli_profile_path = (
                 root_path
@@ -934,7 +914,6 @@ Welcome to the wiki page!
             Membership(
                 user=comunity,
                 club=bar_club,
-                start_date=timezone.now(),
                 role=settings.SITH_CLUB_ROLES_ID["Board member"],
             ).save()
             # Adding user tutu

core/models.py

@@ -23,12 +23,12 @@
 #
 #
 import importlib
+from typing import Union, Optional, List
 
-from django.db import models
+from django.core.cache import cache
 from django.core.mail import send_mail
 from django.contrib.auth.models import (
     AbstractBaseUser,
-    PermissionsMixin,
     UserManager,
     Group as AuthGroup,
     GroupManager as AuthGroupManager,
@@ -40,7 +40,7 @@ from django.core import validators
 from django.core.exceptions import ValidationError, PermissionDenied
 from django.urls import reverse
 from django.conf import settings
-from django.db import transaction
+from django.db import models, transaction
 from django.contrib.staticfiles.storage import staticfiles_storage
 from django.utils.html import escape
 from django.utils.functional import cached_property
@@ -50,7 +50,7 @@ from core import utils
 
 from phonenumber_field.modelfields import PhoneNumberField
 
-from datetime import datetime, timedelta, date
+from datetime import timedelta, date
 
 import unicodedata
 
@@ -90,14 +90,24 @@ class Group(AuthGroup):
         """
         return reverse("core:group_list")
 
+    def save(self, *args, **kwargs):
+        super().save(*args, **kwargs)
+        cache.set(f"sith_group_{self.id}", self)
+        cache.set(f"sith_group_{self.name.replace(' ', '_')}", self)
+
+    def delete(self, *args, **kwargs):
+        super().delete(*args, **kwargs)
+        cache.delete(f"sith_group_{self.id}")
+        cache.delete(f"sith_group_{self.name.replace(' ', '_')}")
+
 
 class MetaGroup(Group):
     """
     MetaGroups are dynamically created groups.
-    Generaly used with clubs where creating a club creates two groups:
+    Generally used with clubs where creating a club creates two groups:
 
-        * club-SITH_BOARD_SUFFIX
+    * club-SITH_BOARD_SUFFIX
-        * club-SITH_MEMBER_SUFFIX
+    * club-SITH_MEMBER_SUFFIX
     """
 
     #: Assign a manager in a way that MetaGroup.objects only return groups with is_meta=False
@@ -110,6 +120,32 @@ class MetaGroup(Group):
         super(MetaGroup, self).__init__(*args, **kwargs)
         self.is_meta = True
 
+    @cached_property
+    def associated_club(self):
+        """
+        Return the group associated with this meta group
+
+        The result of this function is cached
+
+        :return: The associated club if it exists, else None
+        :rtype: club.models.Club | 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):
     """
@@ -134,6 +170,43 @@ def validate_promo(value):
         )
 
 
+def get_group(*, pk: int = None, name: str = None) -> Optional[Group]:
+    """
+    Search for a group by its primary key or its name.
+    Either one of the two must be set.
+
+    The result is cached for the default duration (should be 5 minutes).
+
+    :param pk: The primary key of the group
+    :param name: The name of the group
+    :return: The group if it exists, else None
+    :raises ValueError: If no group matches the criteria
+    """
+    if pk is None and name is None:
+        raise ValueError("Either pk or name must be set")
+    if name is not None:
+        name = name.replace(" ", "_")  # avoid errors with memcached backend
+    pk_or_name: Union[str, int] = pk if pk is not None else name
+    group = cache.get(f"sith_group_{pk_or_name}")
+    if group == "not_found":
+        # Using None as a cache value is a little bit tricky,
+        # so we use a special string to represent None
+        return None
+    elif group is not None:
+        return group
+    # if this point is reached, the group is not in cache
+    if pk is not None:
+        group = Group.objects.filter(pk=pk).first()
+    else:
+        group = Group.objects.filter(name=name).first()
+    if group is not None:
+        cache.set(f"sith_group_{group.id}", group)
+        cache.set(f"sith_group_{group.name.replace(' ', '_')}", group)
+    else:
+        cache.set(f"sith_group_{pk_or_name}", "not_found")
+    return group
+
+
 class User(AbstractBaseUser):
     """
     Defines the base user class, useable in every app
@@ -295,7 +368,6 @@ class User(AbstractBaseUser):
     objects = UserManager()
 
     USERNAME_FIELD = "username"
-    # REQUIRED_FIELDS = ['email']
 
     def promo_has_logo(self):
         return utils.file_exist("./core/static/core/img/promo_%02d.png" % self.promo)
@@ -336,94 +408,72 @@ class User(AbstractBaseUser):
         else:
             return 0
 
-    _club_memberships = {}
+    def is_in_group(self, *, pk: int = None, name: str = None) -> bool:
-    _group_names = {}
+        """
-    _group_ids = {}
+        Check if this user is in the given group.
+        Either a group id or a group name must be provided.
+        If both are passed, only the id will be considered.
 
-    def is_in_group(self, group_name):
+        The group will be fetched using the given parameter.
-        """If the user is in the group passed in argument (as string or by id)"""
+        If no group is found, return False.
-        group_id = 0
+        If a group is found, check if this user is in the latter.
-        g = None
+
-        if isinstance(group_name, int):  # Handle the case where group_name is an ID
+        :return: True if the user is the group, else False
-            if group_name in User._group_ids.keys():
+        """
-                g = User._group_ids[group_name]
+        if pk is not None:
-            else:
+            group: Optional[Group] = get_group(pk=pk)
-                g = Group.objects.filter(id=group_name).first()
+        elif name is not None:
-                User._group_ids[group_name] = g
+            group: Optional[Group] = get_group(name=name)
-        else:
-            if group_name in User._group_names.keys():
-                g = User._group_names[group_name]
-            else:
-                g = Group.objects.filter(name=group_name).first()
-                User._group_names[group_name] = g
-        if g:
-            group_name = g.name
-            group_id = g.id
         else:
+            raise ValueError("You must either provide the id or the name of the group")
+        if group is None:
             return False
-        if group_id == settings.SITH_GROUP_PUBLIC_ID:
+        if group.id == settings.SITH_GROUP_PUBLIC_ID:
             return True
-        if group_id == settings.SITH_GROUP_SUBSCRIBERS_ID:
+        if group.id == settings.SITH_GROUP_SUBSCRIBERS_ID:
             return self.is_subscribed
-        if group_id == settings.SITH_GROUP_OLD_SUBSCRIBERS_ID:
+        if group.id == settings.SITH_GROUP_OLD_SUBSCRIBERS_ID:
             return self.was_subscribed
-        if (
+        if group.id == settings.SITH_GROUP_ROOT_ID:
-            group_name == settings.SITH_MAIN_MEMBERS_GROUP
+            return self.is_root
-        ):  # We check the subscription if asked
+        if group.is_meta:
-            return self.is_subscribed
+            # check if this group is associated with a club
-        if group_name[-len(settings.SITH_BOARD_SUFFIX) :] == settings.SITH_BOARD_SUFFIX:
+            group.__class__ = MetaGroup
-            name = group_name[: -len(settings.SITH_BOARD_SUFFIX)]
+            club = group.associated_club
-            if name in User._club_memberships.keys():
+            if club is None:
-                mem = User._club_memberships[name]
+                return False
-            else:
+            membership = club.get_membership_for(self)
-                from club.models import Club
+            if membership is None:
-
+                return False
-                c = Club.objects.filter(unix_name=name).first()
+            if group.name.endswith(settings.SITH_MEMBER_SUFFIX):
-                mem = c.get_membership_for(self)
-                User._club_memberships[name] = mem
-            if mem:
-                return mem.role > settings.SITH_MAXIMUM_FREE_ROLE
-            return False
-        if (
-            group_name[-len(settings.SITH_MEMBER_SUFFIX) :]
-            == settings.SITH_MEMBER_SUFFIX
-        ):
-            name = group_name[: -len(settings.SITH_MEMBER_SUFFIX)]
-            if name in User._club_memberships.keys():
-                mem = User._club_memberships[name]
-            else:
-                from club.models import Club
-
-                c = Club.objects.filter(unix_name=name).first()
-                mem = c.get_membership_for(self)
-                User._club_memberships[name] = mem
-            if mem:
                 return True
-            return False
+            return membership.role > settings.SITH_MAXIMUM_FREE_ROLE
-        if group_id == settings.SITH_GROUP_ROOT_ID and self.is_superuser:
+        return group in self.cached_groups
+
+    @property
+    def cached_groups(self) -> List[Group]:
+        """
+        Get the list of groups this user is in.
+        The result is cached for the default duration (should be 5 minutes)
+        :return: A list of all the groups this user is in
+        """
+        groups = cache.get(f"user_{self.id}_groups")
+        if groups is None:
+            groups = list(self.groups.all())
+            cache.set(f"user_{self.id}_groups", groups)
+        return groups
+
+    @cached_property
+    def is_root(self) -> bool:
+        if self.is_superuser:
             return True
-        return group_name in self.cached_groups_names
+        root_id = settings.SITH_GROUP_ROOT_ID
-
+        return any(g.id == root_id for g in self.cached_groups)
-    @cached_property
-    def cached_groups_names(self):
-        return [g.name for g in self.groups.all()]
-
-    @cached_property
-    def is_root(self):
-        return (
-            self.is_superuser
-            or self.groups.filter(id=settings.SITH_GROUP_ROOT_ID).exists()
-        )
 
     @cached_property
     def is_board_member(self):
-        from club.models import Club
+        main_club = settings.SITH_MAIN_CLUB["unix_name"]
-
+        return self.is_in_group(name=main_club + settings.SITH_BOARD_SUFFIX)
-        return (
-            Club.objects.filter(unix_name=settings.SITH_MAIN_CLUB["unix_name"])
-            .first()
-            .has_rights_in_club(self)
-        )
 
     @cached_property
     def can_read_subscription_history(self):
@@ -434,8 +484,8 @@ class User(AbstractBaseUser):
 
         for club in Club.objects.filter(
             id__in=settings.SITH_CAN_READ_SUBSCRIPTION_HISTORY
-        ).all():
+        ):
-            if club.has_rights_in_club(self):
+            if club in self.clubs_with_rights:
                 return True
         return False
 
@@ -443,10 +493,8 @@ class User(AbstractBaseUser):
     def can_create_subscription(self):
         from club.models import Club
 
-        for club in Club.objects.filter(
+        for club in Club.objects.filter(id__in=settings.SITH_CAN_CREATE_SUBSCRIPTIONS):
-            id__in=settings.SITH_CAN_CREATE_SUBSCRIPTIONS
+            if club in self.clubs_with_rights:
-        ).all():
-            if club.has_rights_in_club(self):
                 return True
         return False
 
@@ -464,11 +512,11 @@ class User(AbstractBaseUser):
 
     @cached_property
     def is_banned_alcohol(self):
-        return self.is_in_group(settings.SITH_GROUP_BANNED_ALCOHOL_ID)
+        return self.is_in_group(pk=settings.SITH_GROUP_BANNED_ALCOHOL_ID)
 
     @cached_property
     def is_banned_counter(self):
-        return self.is_in_group(settings.SITH_GROUP_BANNED_COUNTER_ID)
+        return self.is_in_group(pk=settings.SITH_GROUP_BANNED_COUNTER_ID)
 
     @cached_property
     def age(self) -> int:
@@ -598,9 +646,9 @@ class User(AbstractBaseUser):
         """
         if hasattr(obj, "is_owned_by") and obj.is_owned_by(self):
             return True
-        if hasattr(obj, "owner_group") and self.is_in_group(obj.owner_group.name):
+        if hasattr(obj, "owner_group") and self.is_in_group(pk=obj.owner_group.id):
             return True
-        if self.is_superuser or self.is_in_group(settings.SITH_GROUP_ROOT_ID):
+        if self.is_root:
             return True
         return False
 
@@ -611,8 +659,8 @@ class User(AbstractBaseUser):
         if hasattr(obj, "can_be_edited_by") and obj.can_be_edited_by(self):
             return True
         if hasattr(obj, "edit_groups"):
-            for g in obj.edit_groups.all():
+            for pk in obj.edit_groups.values_list("pk", flat=True):
-                if self.is_in_group(g.name):
+                if self.is_in_group(pk=pk):
                     return True
         if isinstance(obj, User) and obj == self:
             return True
@@ -627,15 +675,15 @@ class User(AbstractBaseUser):
         if hasattr(obj, "can_be_viewed_by") and obj.can_be_viewed_by(self):
             return True
         if hasattr(obj, "view_groups"):
-            for g in obj.view_groups.all():
+            for pk in obj.view_groups.values_list("pk", flat=True):
-                if self.is_in_group(g.name):
+                if self.is_in_group(pk=pk):
                     return True
         if self.can_edit(obj):
             return True
         return False
 
     def can_be_edited_by(self, user):
-        return user.is_in_group(settings.SITH_MAIN_BOARD_GROUP) or user.is_root
+        return user.is_root or user.is_board_member
 
     def can_be_viewed_by(self, user):
         return (user.was_subscribed and self.is_subscriber_viewable) or user.is_root
@@ -656,10 +704,6 @@ class User(AbstractBaseUser):
             escape(self.get_display_name()),
         )
 
-    @cached_property
-    def subscribed(self):
-        return self.is_in_group(settings.SITH_MAIN_MEMBERS_GROUP)
-
     @cached_property
     def preferences(self):
         try:
@@ -682,17 +726,16 @@ class User(AbstractBaseUser):
 
     @cached_property
     def clubs_with_rights(self):
-        return [
+        """
-            m.club.id
+        :return: the list of clubs where the user has rights
-            for m in self.memberships.filter(
+        :rtype: list[club.models.Club]
-                models.Q(end_date__isnull=True) | models.Q(end_date__gte=timezone.now())
+        """
-            ).all()
+        memberships = self.memberships.ongoing().board().select_related("club")
-            if m.club.has_rights_in_club(self)
+        return [m.club for m in memberships]
-        ]
 
     @cached_property
     def is_com_admin(self):
-        return self.is_in_group(settings.SITH_GROUP_COM_ADMIN_ID)
+        return self.is_in_group(pk=settings.SITH_GROUP_COM_ADMIN_ID)
 
 
 class AnonymousUser(AuthAnonymousUser):
@@ -747,21 +790,18 @@ class AnonymousUser(AuthAnonymousUser):
     def favorite_topics(self):
         raise PermissionDenied
 
-    def is_in_group(self, group_name):
+    def is_in_group(self, *, pk: int = None, name: str = None) -> bool:
         """
-        The anonymous user is only the public group
+        The anonymous user is only in the public group
         """
-        group_id = 0
+        allowed_id = settings.SITH_GROUP_PUBLIC_ID
-        if isinstance(group_name, int):  # Handle the case where group_name is an ID
+        if pk is not None:
-            g = Group.objects.filter(id=group_name).first()
+            return pk == allowed_id
-            if g:
+        elif name is not None:
-                group_name = g.name
+            group = get_group(name=name)
-                group_id = g.id
+            return group is not None and group.id == allowed_id
-            else:
+        else:
-                return False
+            raise ValueError("You must either provide the id or the name of the group")
-        if group_id == settings.SITH_GROUP_PUBLIC_ID:
-            return True
-        return False
 
     def is_owner(self, obj):
         return False
@@ -880,13 +920,13 @@ class SithFile(models.Model):
         verbose_name = _("file")
 
     def is_owned_by(self, user):
-        if hasattr(self, "profile_of") and user.is_in_group(
+        if user.is_anonymous:
-            settings.SITH_MAIN_BOARD_GROUP
+            return False
-        ):
+        if hasattr(self, "profile_of") and user.is_board_member:
             return True
-        if user.is_in_group(settings.SITH_GROUP_COM_ADMIN_ID):
+        if user.is_com_admin:
             return True
-        if self.is_in_sas and user.is_in_group(settings.SITH_GROUP_SAS_ADMIN_ID):
+        if self.is_in_sas and user.is_in_group(pk=settings.SITH_GROUP_SAS_ADMIN_ID):
             return True
         return user.id == self.owner.id
 
@@ -1493,6 +1533,8 @@ class Gift(models.Model):
         return self.label
 
     def is_owned_by(self, user):
+        if user.is_anonymous:
+            return False
         return user.is_board_member or user.is_root
 
 

core/signals.py

@@ -0,0 +1,17 @@
+from django.core.cache import cache
+from django.db.models.signals import m2m_changed
+from django.dispatch import receiver
+
+from core.models import User
+
+
+@receiver(m2m_changed, sender=User.groups.through, dispatch_uid="user_groups_changed")
+def user_groups_changed(sender, instance: User, **kwargs):
+    """
+    Clear the cached clubs of the user
+    """
+    # As a m2m relationship doesn't live within the model
+    # but rather on an intermediary table, there is no
+    # model method to override, meaning we must use
+    # a signal to invalidate the cache when a user is removed from a club
+    cache.delete(f"user_{instance.id}_groups")

core/templates/core/file_detail.jinja

@@ -61,7 +61,7 @@
 {% if not file.home_of and not file.home_of_club and file.parent %}
 <p><a href="{{ url('core:file_delete', file_id=file.id, popup=popup) }}">{% trans %}Delete{% endtrans %}</a></p>
 {% endif %}
-{% if user.is_in_group(settings.SITH_GROUP_COM_ADMIN_ID) %}
+{% if user.is_com_admin %}
 <p><a href="{{ url('core:file_moderate', file_id=file.id) }}">{% trans %}Moderate{% endtrans %}</a></p>
 {% endif %}
 {% endblock %}

core/templates/core/user_clubs.jinja

@@ -67,7 +67,10 @@
     </tbody>
 </table>
 
-{% if profile.mailing_subscriptions.exists() and (profile.id == user.id or user.is_root or user.is_in_group(settings.SITH_GROUP_COM_ADMIN_ID)) %}
+{% if
+    profile.mailing_subscriptions.exists()
+    and (profile.id == user.id or user.is_root or user.is_com_admin)
+%}
     <h4>{% trans %}Subscribed mailing lists{% endtrans %}</h4>
     {% for sub in profile.mailing_subscriptions.all() %}
         <p>{{ sub.mailing.email }} <a href="{{ url('club:mailing_subscription_delete', mailing_subscription_id=sub.id) }}">{% trans %}Unsubscribe{% endtrans %}</a></p>

core/templates/core/user_detail.jinja

@@ -136,7 +136,12 @@
             </div>
         </div>
     </main>
-    {% if user.memberships.filter(end_date=None).exists() or user.is_in_group(settings.SITH_MAIN_BOARD_GROUP) or user == profile or user.is_in_group(settings.SITH_BAR_MANAGER_BOARD_GROUP) %}
+    {% if
+        user == profile
+        or user.memberships.ongoing().exists()
+        or user.is_board_member
+        or user.is_in_group(name=settings.SITH_BAR_MANAGER_BOARD_GROUP)
+    %}
         {# if the user is member of a club, he can view the subscription state #}
         <hr>
         {% if profile.is_subscribed %}

core/templates/core/user_edit.jinja

@@ -35,7 +35,7 @@
                 {%- else -%}
                     <em>{% trans %}To edit your profile picture, ask a member of the AE{% endtrans %}</em>
                 {%- endif -%}
-                {%- if user.is_in_group(settings.SITH_MAIN_BOARD_GROUP) and form.instance.profile_pict.id -%}
+                {%- if user.is_board_member and form.instance.profile_pict.id -%}
                     <a href="{{ url('core:file_delete', file_id=form.instance.profile_pict.id, popup='') }}">
                         {%- trans -%}Delete{%- endtrans -%}
                     </a>
@@ -55,7 +55,7 @@
             <div class="profile-picture-edit">
                 <p>{{ form["avatar_pict"].label }}</p>
                 {{ form["avatar_pict"] }}
-                {%- if user.is_in_group(settings.SITH_MAIN_BOARD_GROUP) and form.instance.avatar_pict.id -%}
+                {%- if user.is_board_member and form.instance.avatar_pict.id -%}
                     <a href="{{ url('core:file_delete', file_id=form.instance.avatar_pict.id, popup='') }}">
                         {%- trans -%}Delete{%- endtrans -%}
                     </a>
@@ -75,7 +75,7 @@
             <div class="profile-picture-edit">
                 <p>{{ form["scrub_pict"].label }}</p>
                 {{ form["scrub_pict"] }}
-                {%- if user.is_in_group(settings.SITH_MAIN_BOARD_GROUP) and form.instance.scrub_pict.id -%}
+                {%- if user.is_board_member and form.instance.scrub_pict.id -%}
                     <a href="{{ url('core:file_delete', file_id=form.instance.scrub_pict.id, popup='') }}">
                         {%- trans -%}Delete{%-endtrans -%}
                     </a>

core/templates/core/user_tools.jinja

@@ -35,18 +35,21 @@
         {% endif %}
 
         {% set is_admin_on_a_counter = false %}
-        {% for b in settings.SITH_COUNTER_BARS if user.is_in_group(b[1] + " admin") %}
+        {% for b in settings.SITH_COUNTER_BARS if user.is_in_group(name=b[1] + " admin") %}
             {% set is_admin_on_a_counter = true %}
         {% endfor %}
 
         {% if
-            user.is_in_group(settings.SITH_GROUP_COUNTER_ADMIN_ID) or user.is_root
+            is_admin_on_a_counter
-            or is_admin_on_a_counter
+            or user.is_root
+            or user.is_in_group(pk=settings.SITH_GROUP_COUNTER_ADMIN_ID)
         %}
             <div>
                 <h4>{% trans %}Counters{% endtrans %}</h4>
                 <ul>
-                    {% if user.is_in_group(settings.SITH_GROUP_COUNTER_ADMIN_ID) or user.is_root %}
+                    {% if user.is_root
+                       or user.is_in_group(pk=settings.SITH_GROUP_COUNTER_ADMIN_ID)
+                    %}
                         <li><a href="{{ url('counter:admin_list') }}">{% trans %}General counters management{% endtrans %}</a></li>
                         <li><a href="{{ url('counter:product_list') }}">{% trans %}Products management{% endtrans %}</a></li>
                         <li><a href="{{ url('counter:producttype_list') }}">{% trans %}Product types management{% endtrans %}</a></li>
@@ -57,7 +60,7 @@
                 </ul>
                 <ul>
                     {% for b in settings.SITH_COUNTER_BARS %}
-                        {% if user.is_in_group(b[1]+" admin") %}
+                        {% if user.is_in_group(name=b[1]+" admin") %}
                             {% set c = Counter.objects.filter(id=b[0]).first() %}
 
                             <li class="rows counter">
@@ -85,13 +88,16 @@
         {% endif %}
 
         {% if
-            user.is_in_group(settings.SITH_GROUP_ACCOUNTING_ADMIN_ID) or user.is_root
+            user.is_root
-            or user.memberships.filter(end_date=None).filter(role__gte=7).all() | length > 10
+            or user.is_in_group(pk=settings.SITH_GROUP_ACCOUNTING_ADMIN_ID)
+            or user.memberships.ongoing().filter(role__gte=7).count() > 10
         %}
             <div>
                 <h4>{% trans %}Accounting{% endtrans %}</h4>
                 <ul>
-                    {% if user.is_in_group(settings.SITH_GROUP_ACCOUNTING_ADMIN_ID) or user.is_root %}
+                    {% if user.is_root
+                       or user.is_in_group(pk=settings.SITH_GROUP_ACCOUNTING_ADMIN_ID)
+                    %}
                         <li><a href="{{ url('accounting:refound_account') }}">{% trans %}Refound Account{% endtrans %}</a></li>
                         <li><a href="{{ url('accounting:bank_list') }}">{% trans %}General accounting{% endtrans %}</a></li>
                         <li><a href="{{ url('accounting:co_list') }}">{% trans %}Company list{% endtrans %}</a></li>
@@ -118,11 +124,15 @@
             </div>
         {% endif %}
 
-        {% if user.is_in_group(settings.SITH_GROUP_SAS_ADMIN_ID) or user.is_in_group(settings.SITH_GROUP_COM_ADMIN_ID) or user.is_root %}
+        {% if
+            user.is_root
+            or user.is_com_admin
+            or user.is_in_group(pk=settings.SITH_GROUP_SAS_ADMIN_ID)
+        %}
             <div>
                 <h4>{% trans %}Communication{% endtrans %}</h4>
                 <ul>
-                    {% if user.is_in_group(settings.SITH_GROUP_COM_ADMIN_ID) or user.is_root %}
+                    {% if user.is_com_admin or user.is_root %}
                         <li><a href="{{ url('com:weekmail_article') }}">{% trans %}Create weekmail article{% endtrans %}</a></li>
                         <li><a href="{{ url('com:weekmail') }}">{% trans %}Weekmail{% endtrans %}</a></li>
                         <li><a href="{{ url('com:weekmail_destinations') }}">{% trans %}Weekmail destinations{% endtrans %}</a></li>
@@ -135,7 +145,7 @@
                         <li><a href="{{ url('com:poster_list') }}">{% trans %}Posters{% endtrans %}</a></li>
                         <li><a href="{{ url('com:screen_list') }}">{% trans %}Screens{% endtrans %}</a></li>
                     {% endif %}
-                    {% if user.is_in_group(settings.SITH_GROUP_SAS_ADMIN_ID) %}
+                    {% if user.is_in_group(pk=settings.SITH_GROUP_SAS_ADMIN_ID) %}
                         <li><a href="{{ url('sas:moderation') }}">{% trans %}Moderate pictures{% endtrans %}</a></li>
                     {% endif %}
                 </ul>
@@ -153,7 +163,10 @@
             </div>
         {% endif %}
 
-        {% if user.is_in_group(settings.SITH_GROUP_PEDAGOGY_ADMIN_ID) or user.is_root %}
+        {% if
+            user.is_root
+            or user.is_in_group(pk=settings.SITH_GROUP_PEDAGOGY_ADMIN_ID)
+        %}
             <div>
                 <h4>{% trans %}Pedagogy{% endtrans %}</h4>
                 <ul>

core/tests.py

@@ -15,13 +15,18 @@
 #
 
 import os
+from datetime import timedelta
 
+from django.core.cache import cache
 from django.test import Client, TestCase
 from django.urls import reverse
 from django.core.management import call_command
+from django.utils.timezone import now
 
-from core.models import User, Group, Page
+from club.models import Membership
+from core.models import User, Group, Page, AnonymousUser
 from core.markdown import markdown
+from sith import settings
 
 """
 to run these tests :
@@ -457,3 +462,150 @@ class FileHandlingTest(TestCase):
         )
         self.assertTrue(response.status_code == 200)
         self.assertTrue("ls</a>" in str(response.content))
+
+
+class UserIsInGroupTest(TestCase):
+    """
+    Test that the User.is_in_group() and AnonymousUser.is_in_group()
+    work as intended
+    """
+
+    @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.subscribers = Group.objects.get(name="Subscribers")
+        cls.old_subscribers = Group.objects.get(name="Old subscribers")
+        cls.accounting_admin = Group.objects.get(name="Accounting admin")
+        cls.com_admin = Group.objects.get(name="Communication admin")
+        cls.counter_admin = Group.objects.get(name="Counter admin")
+        cls.banned_alcohol = Group.objects.get(name="Banned from buying alcohol")
+        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(
+            name="Fake Club",
+            unix_name="fake-club",
+            address="Fake address",
+        )
+        cls.main_club = Club.objects.get(id=1)
+
+    def setUp(self) -> None:
+        self.toto = User.objects.create(
+            username="toto", first_name="a", last_name="b", email="a.b@toto.fr"
+        )
+        self.skia = User.objects.get(username="skia")
+
+    def assert_in_public_group(self, user):
+        self.assertTrue(user.is_in_group(pk=self.public.id))
+        self.assertTrue(user.is_in_group(name=self.public.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
+        self.assertFalse(user.is_in_group(name=meta_groups_board))
+        self.assertFalse(user.is_in_group(name=meta_groups_members))
+
+    def assert_only_in_public_group(self, user):
+        self.assert_in_public_group(user)
+        for group in (
+            self.root_group,
+            self.banned_counters,
+            self.accounting_admin,
+            self.sas_admin,
+            self.subscribers,
+            self.old_subscribers,
+        ):
+            self.assertFalse(user.is_in_group(pk=group.pk))
+            self.assertFalse(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
+        self.assertFalse(user.is_in_group(name=meta_groups_board))
+        self.assertFalse(user.is_in_group(name=meta_groups_members))
+
+    def test_anonymous_user(self):
+        """
+        Test that anonymous users are only in the public group
+        """
+        user = AnonymousUser()
+        self.assert_only_in_public_group(user)
+
+    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)
+
+    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()
+
+    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)
+        skia_groups = self.skia.groups.all()
+
+        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)
+        with self.assertNumQueries(0):
+            self.skia.is_in_group(pk=group_in.id)
+
+        ids = skia_groups.values_list("pk", flat=True)
+        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)
+        with self.assertNumQueries(0):
+            self.skia.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(
+            club=self.club, user=self.toto, end_date=None
+        )
+        meta_groups_members = self.club.unix_name + settings.SITH_MEMBER_SUFFIX
+        cache.clear()
+        self.assertTrue(self.toto.is_in_group(name=meta_groups_members))
+        self.assertEqual(
+            membership, cache.get(f"membership_{self.club.id}_{self.toto.id}")
+        )
+        membership.end_date = now() - timedelta(minutes=5)
+        membership.save()
+        cached_membership = cache.get(f"membership_{self.club.id}_{self.toto.id}")
+        self.assertEqual(cached_membership, "not_member")
+        self.assertFalse(self.toto.is_in_group(name=meta_groups_members))
+
+    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
+        """
+        self.toto.groups.add(self.com_admin.pk)
+        self.assertTrue(self.toto.is_in_group(pk=self.com_admin.pk))
+
+        self.toto.groups.remove(self.com_admin.pk)
+        self.assertFalse(self.toto.is_in_group(pk=self.com_admin.pk))
+
+    def test_not_existing_group(self):
+        """
+        Test that searching for a not existing group
+        returns False
+        """
+        self.assertFalse(self.skia.is_in_group(name="This doesn't exist"))

core/views/user.py

@@ -254,10 +254,11 @@ class UserTabsMixin(TabedViewMixin):
             if user.customer and (
                 user == self.request.user
                 or self.request.user.is_in_group(
-                    settings.SITH_GROUP_ACCOUNTING_ADMIN_ID
+                    pk=settings.SITH_GROUP_ACCOUNTING_ADMIN_ID
                 )
                 or self.request.user.is_in_group(
-                    settings.SITH_BAR_MANAGER["unix_name"] + settings.SITH_BOARD_SUFFIX
+                    name=settings.SITH_BAR_MANAGER["unix_name"]
+                    + settings.SITH_BOARD_SUFFIX
                 )
                 or self.request.user.is_root
             ):
@@ -488,9 +489,9 @@ class UserStatsView(UserTabsMixin, CanViewMixin, DetailView):
 
         if not (
             profile == request.user
-            or request.user.is_in_group(settings.SITH_GROUP_ACCOUNTING_ADMIN_ID)
+            or request.user.is_in_group(pk=settings.SITH_GROUP_ACCOUNTING_ADMIN_ID)
             or request.user.is_in_group(
-                settings.SITH_BAR_MANAGER["unix_name"] + settings.SITH_BOARD_SUFFIX
+                name=settings.SITH_BAR_MANAGER["unix_name"] + settings.SITH_BOARD_SUFFIX
             )
             or request.user.is_root
         ):
@@ -772,9 +773,9 @@ class UserAccountBase(UserTabsMixin, DetailView):
         res = super(UserAccountBase, self).dispatch(request, *arg, **kwargs)
         if (
             self.object == request.user
-            or request.user.is_in_group(settings.SITH_GROUP_ACCOUNTING_ADMIN_ID)
+            or request.user.is_in_group(pk=settings.SITH_GROUP_ACCOUNTING_ADMIN_ID)
             or request.user.is_in_group(
-                settings.SITH_BAR_MANAGER["unix_name"] + settings.SITH_BOARD_SUFFIX
+                name=settings.SITH_BAR_MANAGER["unix_name"] + settings.SITH_BOARD_SUFFIX
             )
             or request.user.is_root
         ):

counter/models.py

@@ -228,7 +228,9 @@ class ProductType(models.Model):
         """
         Method to see if that object can be edited by the given user
         """
-        if user.is_in_group(settings.SITH_GROUP_ACCOUNTING_ADMIN_ID):
+        if user.is_anonymous:
+            return False
+        if user.is_in_group(pk=settings.SITH_GROUP_ACCOUNTING_ADMIN_ID):
             return True
         return False
 
@@ -294,9 +296,11 @@ class Product(models.Model):
         """
         Method to see if that object can be edited by the given user
         """
+        if user.is_anonymous:
+            return False
         if user.is_in_group(
-            settings.SITH_GROUP_ACCOUNTING_ADMIN_ID
+            pk=settings.SITH_GROUP_ACCOUNTING_ADMIN_ID
-        ) or user.is_in_group(settings.SITH_GROUP_COUNTER_ADMIN_ID):
+        ) or user.is_in_group(pk=settings.SITH_GROUP_COUNTER_ADMIN_ID):
             return True
         return False
 
@@ -318,8 +322,8 @@ class Product(models.Model):
         """
         if not self.buying_groups.exists():
             return True
-        for group in self.buying_groups.all():
+        for group_id in self.buying_groups.values_list("pk", flat=True):
-            if user.is_in_group(group.name):
+            if user.is_in_group(pk=group_id):
                 return True
         return False
 
@@ -402,18 +406,17 @@ class Counter(models.Model):
         return reverse("counter:details", kwargs={"counter_id": self.id})
 
     def is_owned_by(self, user):
+        if user.is_anonymous:
+            return False
         mem = self.club.get_membership_for(user)
         if mem and mem.role >= 7:
             return True
-        return user.is_in_group(settings.SITH_GROUP_COUNTER_ADMIN_ID)
+        return user.is_in_group(pk=settings.SITH_GROUP_COUNTER_ADMIN_ID)
 
     def can_be_viewed_by(self, user):
         if self.type == "BAR":
             return True
-        return (
+        return user.is_board_member or user in self.sellers.all()
-            user.is_in_group(settings.SITH_MAIN_BOARD_GROUP)
-            or user in self.sellers.all()
-        )
 
     def gen_token(self):
         """Generate a new token for this counter"""
@@ -621,6 +624,8 @@ class Refilling(models.Model):
         )
 
     def is_owned_by(self, user):
+        if user.is_anonymous:
+            return False
         return user.is_owner(self.counter) and self.payment_method != "CARD"
 
     def delete(self, *args, **kwargs):
@@ -713,6 +718,8 @@ class Selling(models.Model):
         )
 
     def is_owned_by(self, user):
+        if user.is_anonymous:
+            return False
         return user.is_owner(self.counter) and self.payment_method != "CARD"
 
     def can_be_viewed_by(self, user):
@@ -953,7 +960,9 @@ class CashRegisterSummary(models.Model):
         """
         Method to see if that object can be edited by the given user
         """
-        if user.is_in_group(settings.SITH_GROUP_COUNTER_ADMIN_ID):
+        if user.is_anonymous:
+            return False
+        if user.is_in_group(pk=settings.SITH_GROUP_COUNTER_ADMIN_ID):
             return True
         return False
 
@@ -1022,7 +1031,9 @@ class Eticket(models.Model):
         """
         Method to see if that object can be edited by the given user
         """
-        return user.is_in_group(settings.SITH_GROUP_COUNTER_ADMIN_ID)
+        if user.is_anonymous:
+            return False
+        return user.is_in_group(pk=settings.SITH_GROUP_COUNTER_ADMIN_ID)
 
     def get_hash(self, string):
         import hashlib

counter/views.py

@@ -87,8 +87,8 @@ class CounterAdminMixin(View):
     edit_club = []
 
     def _test_group(self, user):
-        for g in self.edit_group:
+        for grp_id in self.edit_group:
-            if user.is_in_group(g):
+            if user.is_in_group(pk=grp_id):
                 return True
         return False
 
@@ -486,14 +486,12 @@ class CounterClick(CounterTabsMixin, CanViewMixin, DetailView):
         pid = str(pid)
         price = self.get_price(pid)
         total = self.sum_basket(request)
-        product = self.get_product(pid)
+        product: Product = self.get_product(pid)
-        can_buy = False
+        user: User = self.customer.user
-        if not product.buying_groups.exists():
+        buying_groups = list(product.buying_groups.values_list("pk", flat=True))
-            can_buy = True
+        can_buy = len(buying_groups) == 0 or any(
-        else:
+            user.is_in_group(pk=group_id) for group_id in buying_groups
-            for g in product.buying_groups.all():
+        )
-                if self.customer.user.is_in_group(g.name):
-                    can_buy = True
         if not can_buy:
             request.session["not_allowed"] = True
             return False
@@ -514,18 +512,17 @@ class CounterClick(CounterTabsMixin, CanViewMixin, DetailView):
         ):
             request.session["not_allowed"] = True
             return False
-        if product.limit_age >= 18 and not self.customer.user.date_of_birth:
+        if product.limit_age >= 18 and not user.date_of_birth:
             request.session["no_age"] = True
             return False
-        if product.limit_age >= 18 and self.customer.user.is_banned_alcohol:
+        if product.limit_age >= 18 and user.is_banned_alcohol:
             request.session["not_allowed"] = True
             return False
-        if self.customer.user.is_banned_counter:
+        if user.is_banned_counter:
             request.session["not_allowed"] = True
             return False
         if (
-            self.customer.user.date_of_birth
+            user.date_of_birth and self.customer.user.get_age() < product.limit_age
-            and self.customer.user.get_age() < product.limit_age
         ):  # Check if affordable
             request.session["too_young"] = True
             return False

doc/TW_Skia/Rapport.tex

@@ -342,7 +342,7 @@ tous les autres.
 {% if user.is_in_group(settings.SITH_GROUP_ACCOUNTING_ADMIN_ID) %}
     <li><a href="{{ url('accounting:bank_list') }}">Accounting</a></li>
 {% endif %}
-{% if user.is_in_group(settings.SITH_MAIN_BOARD_GROUP) or user.is_root %}
+{% if user.is_board_member or user.is_root %}
     <li><a href="{{ url('subscription:subscription') }}">Subscriptions</a></li>
     <li><a href="{{ url('counter:admin_list') }}">Counters management</a></li>
 {% endif %}

election/models.py

@@ -73,16 +73,16 @@ class Election(models.Model):
         return bool(timezone.now() <= self.end_candidature)
 
     def can_candidate(self, user):
-        for group in self.candidature_groups.all():
+        for group_id in self.candidature_groups.values_list("pk", flat=True):
-            if user.is_in_group(group):
+            if user.is_in_group(pk=group_id):
                 return True
         return False
 
     def can_vote(self, user):
         if not self.is_vote_active or self.has_voted(user):
             return False
-        for group in self.vote_groups.all():
+        for group_id in self.vote_groups.values_list("pk", flat=True):
-            if user.is_in_group(group):
+            if user.is_in_group(pk=group_id):
                 return True
         return False
 
@@ -97,10 +97,9 @@ class Election(models.Model):
             results[role.title] = role.results(total_vote)
         return results
 
-    def delete(self):
+    def delete(self, *args, **kwargs):
-        for election_list in self.election_lists.all():
+        self.election_lists.all().delete()
-            election_list.delete()
+        super(Election, self).delete(*args, **kwargs)
-        super(Election, self).delete()
 
     # Permissions
 

election/views.py

@@ -2,14 +2,14 @@ from django.shortcuts import get_object_or_404
 from django.views.generic import ListView, DetailView
 from django.views.generic.edit import UpdateView, CreateView
 from django.views.generic.edit import DeleteView, FormView
-from django.urls import reverse_lazy
+from django.urls import reverse_lazy, reverse
 from django.utils.translation import gettext_lazy as _
 from django.core.exceptions import PermissionDenied
 from django.db import transaction
-from django.forms import CheckboxSelectMultiple
 from django.shortcuts import redirect
 from django import forms
 
+from core.models import User
 from core.views import CanViewMixin, CanEditMixin, CanCreateMixin
 from django.db.models.query import QuerySet
 from core.views.forms import SelectDateTime, MarkdownInput
@@ -224,8 +224,8 @@ class ElectionDetailView(CanViewMixin, DetailView):
     pk_url_kwarg = "election_id"
 
     def get(self, request, *arg, **kwargs):
-        r = super(ElectionDetailView, self).get(request, *arg, **kwargs)
+        response = super(ElectionDetailView, self).get(request, *arg, **kwargs)
-        election = self.get_object()
+        election: Election = self.get_object()
         if request.user.can_edit(election) and election.is_vote_editable:
             action = request.GET.get("action", None)
             role = request.GET.get("role", None)
@@ -239,9 +239,9 @@ class ElectionDetailView(CanViewMixin, DetailView):
                 elif action == "top":
                     Role.objects.get(id=role).top()
                 return redirect(
-                    reverse_lazy("election:detail", kwargs={"election_id": election.id})
+                    reverse("election:detail", kwargs={"election_id": election.id})
                 )
-        return r
+        return response
 
     def get_context_data(self, **kwargs):
         """Add additionnal data to the template"""
@@ -295,8 +295,8 @@ class VoteFormView(CanCreateMixin, FormView):
         """
         data = form.clean()
         res = super(FormView, self).form_valid(form)
-        for grp in self.election.vote_groups.all():
+        for grp_id in self.election.vote_groups.values_list("pk", flat=True):
-            if self.request.user.is_in_group(grp):
+            if self.request.user.is_in_group(pk=grp_id):
                 self.vote(data)
                 return res
         return res
@@ -401,12 +401,13 @@ class RoleCreateView(CanCreateMixin, CreateView):
 
     def form_valid(self, form):
         """
-        Verify that the user can edit proprely
+        Verify that the user can edit properly
         """
-        obj = form.instance
+        obj: Role = form.instance
+        user: User = self.request.user
         if obj.election:
-            for grp in obj.election.edit_groups.all():
+            for grp_id in obj.election.edit_groups.values_list("pk", flat=True):
-                if self.request.user.is_in_group(grp):
+                if user.is_in_group(pk=grp_id):
                     return super(CreateView, self).form_valid(form)
         raise PermissionDenied
 
@@ -446,13 +447,14 @@ class ElectionListCreateView(CanCreateMixin, CreateView):
         """
         Verify that the user can vote on this election
         """
-        obj = form.instance
+        obj: ElectionList = form.instance
+        user: User = self.request.user
         if obj.election:
-            for grp in obj.election.candidature_groups.all():
+            for grp_id in obj.election.candidature_groups.values_list("pk", flat=True):
-                if self.request.user.is_in_group(grp):
+                if user.is_in_group(pk=grp_id):
                     return super(CreateView, self).form_valid(form)
-            for grp in obj.election.edit_groups.all():
+            for grp_id in obj.election.edit_groups.values_list("pk", flat=True):
-                if self.request.user.is_in_group(grp):
+                if user.is_in_group(pk=grp_id):
                     return super(CreateView, self).form_valid(form)
         raise PermissionDenied
 

forum/models.py

@@ -161,7 +161,9 @@ class Forum(models.Model):
     # divided by 3 the number of requests on the main forum page
     # after the first load
     def is_owned_by(self, user):
-        if user.is_in_group(settings.SITH_GROUP_FORUM_ADMIN_ID):
+        if user.is_anonymous:
+            return False
+        if user.is_in_group(pk=settings.SITH_GROUP_FORUM_ADMIN_ID):
             return True
         try:
             m = Forum._club_memberships[self.id][user.id]
@@ -337,7 +339,10 @@ class ForumMessage(models.Model):
     def is_last_in_topic(self):
         return bool(self.id == self.topic.messages.order_by("date").last().id)
 
-    def is_owned_by(self, user):  # Anyone can create a topic: it's better to
+    def is_owned_by(self, user):
+        if user.is_anonymous:
+            return False
+        # Anyone can create a topic: it's better to
         # check the rights at the forum level, since it's more controlled
         return self.topic.forum.is_owned_by(user) or user.id == self.author.id
 

forum/templates/forum/forum.jinja

@@ -10,7 +10,11 @@
 <div id="forum">
     <h3>{{ forum.name }}</h3>
     <p>
-    {% if user.is_in_group(settings.SITH_GROUP_FORUM_ADMIN_ID) or user.is_in_group(settings.SITH_GROUP_COM_ADMIN_ID) or user.can_edit(forum) %}
+    {%
+        if user.is_com_admin
+        or user.is_in_group(pk=settings.SITH_GROUP_FORUM_ADMIN_ID)
+        or user.can_edit(forum)
+    %}
         <a class="ib button" href="{{ url('forum:new_forum') }}?parent={{ forum.id }}">{% trans %}New forum{% endtrans %}</a> <br/>
     {% endif %}
     {% if not forum.is_category %}

forum/templates/forum/main.jinja

@@ -17,7 +17,10 @@
         <a class="ib button" href="{{ url('forum:favorite_topics') }}">{% trans %}Favorite topics{% endtrans %}</a>
         {{ display_search_bar(request) }}
     </p>
-    {% if user.is_in_group(settings.SITH_GROUP_FORUM_ADMIN_ID) or user.is_in_group(settings.SITH_GROUP_COM_ADMIN_ID) %}
+    {% if
+        user.is_com_admin
+        or user.is_in_group(pk=settings.SITH_GROUP_FORUM_ADMIN_ID)
+    %}
     <p>
         <a href="{{ url('forum:new_forum') }}">{% trans %}New forum{% endtrans %}</a>
     </p>

launderette/models.py

@@ -42,6 +42,8 @@ class Launderette(models.Model):
         """
         Method to see if that object can be edited by the given user
         """
+        if user.is_anonymous:
+            return False
         launderette_club = Club.objects.filter(
             unix_name=settings.SITH_LAUNDERETTE_MANAGER["unix_name"]
         ).first()
@@ -60,7 +62,7 @@ class Launderette(models.Model):
         return False
 
     def can_be_viewed_by(self, user):
-        return user.is_in_group(settings.SITH_MAIN_MEMBERS_GROUP)
+        return user.is_subscribed
 
     def __str__(self):
         return self.name
@@ -101,6 +103,8 @@ class Machine(models.Model):
         """
         Method to see if that object can be edited by the given user
         """
+        if user.is_anonymous:
+            return False
         launderette_club = Club.objects.filter(
             unix_name=settings.SITH_LAUNDERETTE_MANAGER["unix_name"]
         ).first()
@@ -155,6 +159,8 @@ class Token(models.Model):
         """
         Method to see if that object can be edited by the given user
         """
+        if user.is_anonymous:
+            return False
         launderette_club = Club.objects.filter(
             unix_name=settings.SITH_LAUNDERETTE_MANAGER["unix_name"]
         ).first()

launderette/templates/launderette/launderette_book_choose.jinja

@@ -5,7 +5,7 @@
 {% endblock %}
 
 {% block content %}
-{% if request.user.is_in_group(settings.SITH_MAIN_MEMBERS_GROUP) %}
+{% if request.user.is_subscribed %}
 <ul>
     {% for l in launderette_list  %}
     <li><a href="{{ url('launderette:book_slot', launderette_id=l.id) }}">{{ l }}</a></li>

launderette/templates/launderette/launderette_main.jinja

@@ -8,7 +8,7 @@
 {% if request.user.can_edit(page) %}
 <p><a href="{{ url('core:page_edit', page_name=page.get_full_name()) }}">{% trans %}Edit presentation page{% endtrans %}</a></p>
 {% endif %}
-{% if request.user.is_in_group(settings.SITH_MAIN_MEMBERS_GROUP) %}
+{% if request.user.is_subscribed %}
 <p><a href="{{ url('launderette:book_main') }}">{% trans %}Book launderette slot{% endtrans %}</a></p>
 {% endif %}
 

pedagogy/models.py

@@ -147,7 +147,7 @@ class UV(models.Model):
         """
         Can be created by superuser, root or pedagogy admin user
         """
-        return user.is_in_group(settings.SITH_GROUP_PEDAGOGY_ADMIN_ID)
+        return user.is_in_group(pk=settings.SITH_GROUP_PEDAGOGY_ADMIN_ID)
 
     def can_be_viewed_by(self, user):
         """

pedagogy/tests.py

@@ -1073,7 +1073,7 @@ class UVCommentReportCreateTest(TestCase):
         # Check that only pedagogy admins recieves this notification
         for notif in Notification.objects.filter(type="PEDAGOGY_MODERATION").all():
             self.assertTrue(
-                notif.user.is_in_group(settings.SITH_GROUP_PEDAGOGY_ADMIN_ID)
+                notif.user.is_in_group(pk=settings.SITH_GROUP_PEDAGOGY_ADMIN_ID)
             )
 
         # Check that notifications are not duplicated if not viewed

sas/models.py

@@ -65,7 +65,7 @@ class Picture(SithFile):
     def can_be_edited_by(self, user):
         perm = cache.get("%d_can_edit_pictures" % (user.id), None)
         if perm is None:
-            perm = user.is_root or user.is_in_group(settings.SITH_GROUP_SAS_ADMIN_ID)
+            perm = user.is_root or user.is_in_group(pk=settings.SITH_GROUP_SAS_ADMIN_ID)
 
         cache.set("%d_can_edit_pictures" % (user.id), perm, timeout=4)
         return perm
@@ -190,10 +190,7 @@ class Album(SithFile):
         return Album.objects.filter(parent=self)
 
     def can_be_edited_by(self, user):
-        # file = SithFile.objects.filter(id=self.id).first()
+        return user.is_in_group(pk=settings.SITH_GROUP_SAS_ADMIN_ID)
-        return user.is_in_group(
-            settings.SITH_GROUP_SAS_ADMIN_ID
-        )  # or user.can_edit(file)
 
     def can_be_viewed_by(self, user):
         # file = SithFile.objects.filter(id=self.id).first()

sas/templates/sas/album.jinja

@@ -119,7 +119,7 @@
         </form>
     {% endif %}
 
-    {% if user.is_in_group(settings.SITH_GROUP_SAS_ADMIN_ID) %}
+    {% if user.is_in_group(pk=settings.SITH_GROUP_SAS_ADMIN_ID) %}
         <form class="add-files" id="upload_form" action="" method="post" enctype="multipart/form-data">
             {% csrf_token %}
             <div class="inputs">

sas/templates/sas/main.jinja

@@ -8,7 +8,7 @@
     {% trans %}SAS{% endtrans %}
 {% endblock %}
 
-{% set edit_mode = user.is_in_group(settings.SITH_GROUP_SAS_ADMIN_ID) %}
+{% set edit_mode = user.is_in_group(pk=settings.SITH_GROUP_SAS_ADMIN_ID) %}
 
 {% macro display_album(a, checkbox) %}
     <a href="{{ url('sas:album', album_id=a.id) }}">

sas/views.py

@@ -110,7 +110,7 @@ class SASMainView(FormView):
         files = request.FILES.getlist("images")
         root = User.objects.filter(username="root").first()
         if request.user.is_authenticated and request.user.is_in_group(
-            settings.SITH_GROUP_SAS_ADMIN_ID
+            pk=settings.SITH_GROUP_SAS_ADMIN_ID
         ):
             if self.form.is_valid():
                 self.form.process(
@@ -151,7 +151,7 @@ class PictureView(CanViewMixin, DetailView, FormMixin):
             try:
                 user = User.objects.filter(id=int(request.GET["remove_user"])).first()
                 if user.id == request.user.id or request.user.is_in_group(
-                    settings.SITH_GROUP_SAS_ADMIN_ID
+                    pk=settings.SITH_GROUP_SAS_ADMIN_ID
                 ):
                     PeoplePictureRelation.objects.filter(
                         user=user, picture=self.object
@@ -238,7 +238,7 @@ class AlbumUploadView(CanViewMixin, DetailView, FormMixin):
                     owner=request.user,
                     files=files,
                     automodere=request.user.is_in_group(
-                        settings.SITH_GROUP_SAS_ADMIN_ID
+                        pk=settings.SITH_GROUP_SAS_ADMIN_ID
                     ),
                 )
                 if self.form.is_valid():
@@ -284,7 +284,7 @@ class AlbumView(CanViewMixin, DetailView, FormMixin):
                     owner=request.user,
                     files=files,
                     automodere=request.user.is_in_group(
-                        settings.SITH_GROUP_SAS_ADMIN_ID
+                        pk=settings.SITH_GROUP_SAS_ADMIN_ID
                     ),
                 )
                 if self.form.is_valid():
@@ -319,12 +319,12 @@ class ModerationView(TemplateView):
     template_name = "sas/moderation.jinja"
 
     def get(self, request, *args, **kwargs):
-        if request.user.is_in_group(settings.SITH_GROUP_SAS_ADMIN_ID):
+        if request.user.is_in_group(pk=settings.SITH_GROUP_SAS_ADMIN_ID):
             return super(ModerationView, self).get(request, *args, **kwargs)
         raise PermissionDenied
 
     def post(self, request, *args, **kwargs):
-        if request.user.is_in_group(settings.SITH_GROUP_SAS_ADMIN_ID):
+        if request.user.is_in_group(pk=settings.SITH_GROUP_SAS_ADMIN_ID):
             try:
                 a = Album.objects.filter(id=request.POST["album_id"]).first()
                 if "moderate" in request.POST.keys():

sith/settings.py

@@ -570,8 +570,6 @@ SITH_SUBSCRIPTIONS = {
     # To be completed....
 }
 
-SITH_CLUB_ROLES = {}
-
 SITH_CLUB_ROLES_ID = {
     "President": 10,
     "Vice-President": 9,
@@ -597,7 +595,8 @@ SITH_CLUB_ROLES = {
 }
 
 # This corresponds to the maximum role a user can freely subscribe to
-# In this case, SITH_MAXIMUM_FREE_ROLE=1 means that a user can set himself as "Membre actif" or "Curieux", but not higher
+# In this case, SITH_MAXIMUM_FREE_ROLE=1 means that a user can
+# set himself as "Membre actif" or "Curieux", but not higher
 SITH_MAXIMUM_FREE_ROLE = 1
 
 # Minutes to timeout the logged barmen

stock/models.py

@@ -52,7 +52,7 @@ class Stock(models.Model):
         return reverse("stock:list")
 
     def can_be_viewed_by(self, user):
-        return user.is_in_group(settings.SITH_GROUP_COUNTER_ADMIN_ID)
+        return user.is_in_group(pk=settings.SITH_GROUP_COUNTER_ADMIN_ID)
 
 
 class StockItem(models.Model):
@@ -93,7 +93,7 @@ class StockItem(models.Model):
         return reverse("stock:items_list", kwargs={"stock_id": self.stock_owner.id})
 
     def can_be_viewed_by(self, user):
-        return user.is_in_group(settings.SITH_GROUP_COUNTER_ADMIN_ID)
+        return user.is_in_group(pk=settings.SITH_GROUP_COUNTER_ADMIN_ID)
 
 
 class ShoppingList(models.Model):
@@ -116,7 +116,7 @@ class ShoppingList(models.Model):
         return reverse("stock:shoppinglist_list")
 
     def can_be_viewed_by(self, user):
-        return user.is_in_group(settings.SITH_GROUP_COUNTER_ADMIN_ID)
+        return user.is_in_group(pk=settings.SITH_GROUP_COUNTER_ADMIN_ID)
 
 
 class ShoppingListItem(models.Model):
@@ -154,7 +154,7 @@ class ShoppingListItem(models.Model):
         return "%s - %s" % (self.name, self.shopping_lists.first())
 
     def can_be_viewed_by(self, user):
-        return user.is_in_group(settings.SITH_GROUP_COUNTER_ADMIN_ID)
+        return user.is_in_group(pk=settings.SITH_GROUP_COUNTER_ADMIN_ID)
 
     def get_absolute_url(self):
         return reverse("stock:shoppinglist_list")

subscription/models.py

@@ -154,7 +154,7 @@ class Subscription(models.Model):
         )
 
     def can_be_edited_by(self, user):
-        return user.is_in_group(settings.SITH_MAIN_BOARD_GROUP) or user.is_root
+        return user.is_board_member or user.is_root
 
     def is_valid_now(self):
         return self.subscription_start <= date.today() <= self.subscription_end