From d657e3e258af9e7793a4e51826fa8b8fe14637ee Mon Sep 17 00:00:00 2001 From: imperosol Date: Sat, 30 Nov 2024 20:30:17 +0100 Subject: [PATCH] replace MetaGroups by proper group management --- ...012_club_board_group_club_members_group.py | 79 ++++++++ club/models.py | 183 ++++++++++-------- club/tests.py | 51 ++--- club/views.py | 5 +- core/admin.py | 4 +- core/baker_recipes.py | 3 +- core/management/commands/populate.py | 107 +++++----- core/migrations/0001_initial.py | 11 +- ..._metagroup_alter_group_options_and_more.py | 51 +++++ core/models.py | 112 ++--------- core/tests/test_core.py | 92 ++++----- core/views/forms.py | 11 +- core/views/group.py | 17 +- counter/models.py | 9 +- election/tests.py | 2 - .../commands/generate_galaxy_test_data.py | 18 +- 16 files changed, 400 insertions(+), 355 deletions(-) create mode 100644 club/migrations/0012_club_board_group_club_members_group.py create mode 100644 core/migrations/0041_delete_metagroup_alter_group_options_and_more.py diff --git a/club/migrations/0012_club_board_group_club_members_group.py b/club/migrations/0012_club_board_group_club_members_group.py new file mode 100644 index 00000000..1aef164f --- /dev/null +++ b/club/migrations/0012_club_board_group_club_members_group.py @@ -0,0 +1,79 @@ +# Generated by Django 4.2.16 on 2024-11-20 17:08 + +import django.db.models.deletion +from django.conf import settings +from django.db import migrations, models +from django.db.migrations.state import StateApps + + +def migrate_meta_groups(apps: StateApps, schema_editor): + Group = apps.get_model("core", "Group") + Club = apps.get_model("club", "Club") + + meta_groups = Group.objects.filter(is_meta=True) + clubs = list(Club.objects.all()) + for club in clubs: + club.board_group = meta_groups.get_or_create( + name=club.unix_name + settings.SITH_BOARD_SUFFIX + )[0] + club.members_group = meta_groups.get_or_create( + name=club.unix_name + settings.SITH_MEMBER_SUFFIX + )[0] + Club.objects.bulk_update(clubs, fields=["board_group", "members_group"]) + + +# steps of the migration : +# - Create a nullable field for the board group and the member group +# - Edit those new fields to make them point to currently existing meta groups +# - When this data migration is done, make the fields non-nullable +class Migration(migrations.Migration): + dependencies = [ + ("core", "0040_alter_user_options_user_user_permissions_and_more"), + ("club", "0011_auto_20180426_2013"), + ] + + operations = [ + migrations.AddField( + model_name="club", + name="board_group", + field=models.OneToOneField( + blank=True, + null=True, + on_delete=django.db.models.deletion.CASCADE, + related_name="club_board", + to="core.group", + ), + ), + migrations.AddField( + model_name="club", + name="members_group", + field=models.OneToOneField( + blank=True, + null=True, + on_delete=django.db.models.deletion.CASCADE, + related_name="club", + to="core.group", + ), + ), + migrations.RunPython( + migrate_meta_groups, reverse_code=migrations.RunPython.noop, elidable=True + ), + migrations.AlterField( + model_name="club", + name="board_group", + field=models.OneToOneField( + on_delete=django.db.models.deletion.CASCADE, + related_name="club_board", + to="core.group", + ), + ), + migrations.AlterField( + model_name="club", + name="members_group", + field=models.OneToOneField( + on_delete=django.db.models.deletion.CASCADE, + related_name="club", + to="core.group", + ), + ), + ] diff --git a/club/models.py b/club/models.py index 5300057d..49a711e0 100644 --- a/club/models.py +++ b/club/models.py @@ -38,7 +38,7 @@ from django.utils.functional import cached_property from django.utils.timezone import localdate from django.utils.translation import gettext_lazy as _ -from core.models import Group, MetaGroup, Notification, Page, SithFile, User +from core.models import Group, Notification, Page, SithFile, User # Create your models here. @@ -103,6 +103,12 @@ class Club(models.Model): page = models.OneToOneField( Page, related_name="club", blank=True, null=True, on_delete=models.CASCADE ) + members_group = models.OneToOneField( + Group, related_name="club", on_delete=models.CASCADE + ) + board_group = models.OneToOneField( + Group, related_name="club_board", on_delete=models.CASCADE + ) class Meta: ordering = ["name", "unix_name"] @@ -112,23 +118,25 @@ class Club(models.Model): @transaction.atomic() def save(self, *args, **kwargs): - old = Club.objects.filter(id=self.id).first() - creation = old is None - if not creation and old.unix_name != self.unix_name: - self._change_unixname(self.unix_name) + creation = self._state.adding + if not creation and Club.objects.get(id=self.id).unix_name != self.unix_name: + self.home.name = self.unix_name + self.home.save() + if creation: + self.board_group = Group.objects.create( + name=f"{self.name} - Bureau", is_manually_manageable=False + ) + self.members_group = Group.objects.create( + name=f"{self.name} - Membres", is_manually_manageable=False + ) super().save(*args, **kwargs) if creation: - board = MetaGroup(name=self.unix_name + settings.SITH_BOARD_SUFFIX) - board.save() - member = MetaGroup(name=self.unix_name + settings.SITH_MEMBER_SUFFIX) - member.save() subscribers = Group.objects.filter( name=settings.SITH_MAIN_MEMBERS_GROUP ).first() self.make_home() - self.home.edit_groups.set([board]) - self.home.view_groups.set([member, subscribers]) - self.home.save() + self.home.edit_groups.add(self.board_group) + self.home.view_groups.add(self.members_group, subscribers) self.make_page() cache.set(f"sith_club_{self.unix_name}", self) @@ -136,7 +144,8 @@ class Club(models.Model): return reverse("club:club_view", kwargs={"club_id": self.id}) @cached_property - def president(self): + def president(self) -> Membership | None: + """Fetch the membership of the current president of this club.""" return self.members.filter( role=settings.SITH_CLUB_ROLES_ID["President"], end_date=None ).first() @@ -154,36 +163,18 @@ class Club(models.Model): def clean(self): self.check_loop() - def _change_unixname(self, old_name, new_name): - c = Club.objects.filter(unix_name=new_name).first() - if c is None: - # Update all the groups names - Group.objects.filter(name=old_name).update(name=new_name) - Group.objects.filter(name=old_name + settings.SITH_BOARD_SUFFIX).update( - name=new_name + settings.SITH_BOARD_SUFFIX - ) - Group.objects.filter(name=old_name + settings.SITH_MEMBER_SUFFIX).update( - name=new_name + settings.SITH_MEMBER_SUFFIX - ) + def make_home(self) -> None: + if self.home: + return + home_root = SithFile.objects.filter(parent=None, name="clubs").first() + root = User.objects.filter(username="root").first() + if home_root and root: + home = SithFile(parent=home_root, name=self.unix_name, owner=root) + home.save() + self.home = home + self.save() - if self.home: - self.home.name = new_name - self.home.save() - - else: - raise ValidationError(_("A club with that unix_name already exists")) - - def make_home(self): - if not self.home: - home_root = SithFile.objects.filter(parent=None, name="clubs").first() - root = User.objects.filter(username="root").first() - if home_root and root: - home = SithFile(parent=home_root, name=self.unix_name, owner=root) - home.save() - self.home = home - self.save() - - def make_page(self): + def make_page(self) -> None: root = User.objects.filter(username="root").first() if not self.page: club_root = Page.objects.filter(name=settings.SITH_CLUB_ROOT_PAGE).first() @@ -213,35 +204,32 @@ class Club(models.Model): self.page.parent = self.parent.page self.page.save(force_lock=True) - def delete(self, *args, **kwargs): + def delete(self, *args, **kwargs) -> tuple[int, dict[str, int]]: # Invalidate the cache of this club and of its memberships for membership in self.members.ongoing().select_related("user"): cache.delete(f"membership_{self.id}_{membership.user.id}") cache.delete(f"sith_club_{self.unix_name}") - super().delete(*args, **kwargs) + return super().delete(*args, **kwargs) - def get_display_name(self): + def get_display_name(self) -> str: return self.name - def is_owned_by(self, user): + def is_owned_by(self, user: User) -> bool: """Method to see if that object can be super edited by the given user.""" if user.is_anonymous: return False return user.is_board_member - def get_full_logo_url(self): - return "https://%s%s" % (settings.SITH_URL, self.logo.url) + def get_full_logo_url(self) -> str: + return f"https://{settings.SITH_URL}{self.logo.url}" - def can_be_edited_by(self, user): + def can_be_edited_by(self, user: User) -> bool: """Method to see if that object can be edited by the given user.""" return self.has_rights_in_club(user) - def can_be_viewed_by(self, user): + def can_be_viewed_by(self, user: User) -> bool: """Method to see if that object can be seen by the given user.""" - sub = User.objects.filter(pk=user.pk).first() - if sub is None: - return False - return sub.was_subscribed + return user.was_subscribed def get_membership_for(self, user: User) -> Membership | None: """Return the current membership the given user. @@ -262,12 +250,19 @@ class Club(models.Model): cache.set(f"membership_{self.id}_{user.id}", membership) return membership - def has_rights_in_club(self, user): + def has_rights_in_club(self, user: User) -> bool: m = self.get_membership_for(user) return m is not None and m.role > settings.SITH_MAXIMUM_FREE_ROLE class MembershipQuerySet(models.QuerySet): + @staticmethod + def _remove_from_club_groups(users: list[int], clubs: list[int]): + groups = Group.objects.filter(Q(club__in=clubs) | Q(club_board__in=clubs)) + return User.groups.through.objects.filter( + Q(group__in=groups) & Q(user__in=users) + ).delete() + def ongoing(self) -> Self: """Filter all memberships which are not finished yet.""" return self.filter(Q(end_date=None) | Q(end_date__gt=localdate())) @@ -283,7 +278,7 @@ class MembershipQuerySet(models.QuerySet): """ return self.filter(role__gt=settings.SITH_MAXIMUM_FREE_ROLE) - def update(self, **kwargs): + def update(self, **kwargs) -> int: """Refresh the cache for the elements of the queryset. Besides that, does the same job as a regular update method. @@ -291,34 +286,48 @@ class MembershipQuerySet(models.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, - ) + if nb_rows == 0: + # if no row was affected, no need to refresh the cache + return 0 - def delete(self): + cache_memberships = {} + to_remove = {"users": [], "clubs": []} + for membership in self.all(): + cache_key = f"membership_{membership.club_id}_{membership.user_id}" + if membership.end_date is not None and membership.end_date <= localdate(): + cache_memberships[cache_key] = "not_member" + to_remove["users"].append(membership.user_id) + to_remove["clubs"].append(membership.club_id) + else: + cache_memberships[cache_key] = membership + cache.set_many(cache_memberships) + self._remove_from_club_groups(to_remove["users"], to_remove["clubs"]) + return nb_rows + + def delete(self) -> tuple[int, dict[str, int]]: """Work just like the default Django's delete() method, but add a cache invalidation for the elements of the queryset - before the deletion. + before the deletion, + and a removal of the user from the club groups. - Be aware that this adds a db query to retrieve the deleted element. - As this first query take place before the deletion operation, - it will be performed even if the deletion fails. + Be aware that this adds a db query to retrieve the deleted element + and another to remove users from the groups. + As queries take place before the deletion operation, + they will be performed even if the deletion fails. """ ids = list(self.values_list("club_id", "user_id")) - nb_rows, _ = super().delete() + nb_rows, rows_counts = super().delete() if nb_rows > 0: - for club_id, user_id in ids: - cache.set(f"membership_{club_id}_{user_id}", "not_member") + user_ids = [i[0] for i in ids] + club_ids = [i[1] for i in ids] + self._remove_from_club_groups(user_ids, club_ids) + cache.set_many( + { + f"membership_{club_id}_{user_id}": "not_member" + for club_id, user_id in ids + } + ) + return nb_rows, rows_counts class Membership(models.Model): @@ -369,7 +378,23 @@ class Membership(models.Model): ) def save(self, *args, **kwargs): + # adding must be set before calling super().save() + # because the call will switch _state.adding from True to False + adding = self._state.adding super().save(*args, **kwargs) + if adding: + groups = [ + User.groups.through( + user_id=self.user_id, group_id=self.club.members_group_id + ) + ] + if self.role > settings.SITH_MAXIMUM_FREE_ROLE: + groups.append( + User.groups.through( + user_id=self.user_id, group_id=self.club.board_group_id + ) + ) + User.groups.through.objects.bulk_create(groups) if self.end_date is None: cache.set(f"membership_{self.club_id}_{self.user_id}", self) else: @@ -378,6 +403,10 @@ class Membership(models.Model): def get_absolute_url(self): return reverse("club:club_members", kwargs={"club_id": self.club_id}) + @property + def is_ongoing(self): + return self.end_date is None or self.end_date + def is_owned_by(self, user): """Method to see if that object can be super edited by the given user.""" if user.is_anonymous: diff --git a/club/tests.py b/club/tests.py index ae055bd0..54ea1c67 100644 --- a/club/tests.py +++ b/club/tests.py @@ -21,6 +21,7 @@ from django.urls import reverse from django.utils import timezone from django.utils.timezone import localdate, localtime, now from django.utils.translation import gettext as _ +from model_bakery import baker from club.forms import MailingForm from club.models import Club, Mailing, Membership @@ -192,10 +193,8 @@ class TestClubModel(TestClub): assert membership.end_date is None assert membership.role == role assert membership.club.get_membership_for(user) == membership - member_group = self.club.unix_name + settings.SITH_MEMBER_SUFFIX - board_group = self.club.unix_name + settings.SITH_BOARD_SUFFIX - assert user.is_in_group(name=member_group) - assert user.is_in_group(name=board_group) + assert user.is_in_group(pk=self.club.members_group_id) + assert user.is_in_group(pk=self.club.board_group_id) def assert_membership_ended_today(self, user: User): """Assert that the given user have a membership which ended today.""" @@ -474,37 +473,23 @@ class TestClubModel(TestClub): assert self.club.members.count() == nb_memberships assert membership == new_mem - def test_delete_remove_from_meta_group(self): - """Test that when a club is deleted, all its members are removed from the - associated metagroup. - """ - memberships = self.club.members.select_related("user") - users = [membership.user for membership in memberships] - meta_group = self.club.unix_name + settings.SITH_MEMBER_SUFFIX + def test_remove_from_club_group(self): + """Test that when a membership ends, the user is removed from club groups.""" + user = baker.make(User) + baker.make(Membership, user=user, club=self.club, end_date=None, role=3) + assert user.groups.contains(self.club.members_group) + assert user.groups.contains(self.club.board_group) + user.memberships.update(end_date=localdate()) + assert not user.groups.contains(self.club.members_group) + assert not user.groups.contains(self.club.board_group) - self.club.delete() - for user in users: - assert not user.is_in_group(name=meta_group) - - def test_add_to_meta_group(self): - """Test that when a membership begins, the user is added to the meta group.""" - group_members = self.club.unix_name + settings.SITH_MEMBER_SUFFIX - board_members = self.club.unix_name + settings.SITH_BOARD_SUFFIX - assert not self.subscriber.is_in_group(name=group_members) - assert not self.subscriber.is_in_group(name=board_members) + def test_add_to_club_group(self): + """Test that when a membership begins, the user is added to the club group.""" + assert not self.subscriber.groups.contains(self.club.members_group) + assert not self.subscriber.groups.contains(self.club.board_group) Membership.objects.create(club=self.club, user=self.subscriber, role=3) - assert self.subscriber.is_in_group(name=group_members) - assert self.subscriber.is_in_group(name=board_members) - - def test_remove_from_meta_group(self): - """Test that when a membership ends, the user is removed from meta group.""" - group_members = self.club.unix_name + settings.SITH_MEMBER_SUFFIX - board_members = self.club.unix_name + settings.SITH_BOARD_SUFFIX - assert self.comptable.is_in_group(name=group_members) - assert self.comptable.is_in_group(name=board_members) - self.comptable.memberships.update(end_date=localtime(now())) - assert not self.comptable.is_in_group(name=group_members) - assert not self.comptable.is_in_group(name=board_members) + assert self.subscriber.groups.contains(self.club.members_group) + assert self.subscriber.groups.contains(self.club.board_group) def test_club_owner(self): """Test that a club is owned only by board members of the main club.""" diff --git a/club/views.py b/club/views.py index e1f3367e..d713a1cc 100644 --- a/club/views.py +++ b/club/views.py @@ -71,14 +71,13 @@ class ClubTabsMixin(TabedViewMixin): return self.object.get_display_name() def get_list_of_tabs(self): - tab_list = [] - tab_list.append( + tab_list = [ { "url": reverse("club:club_view", kwargs={"club_id": self.object.id}), "slug": "infos", "name": _("Infos"), } - ) + ] if self.request.user.can_view(self.object): tab_list.append( { diff --git a/core/admin.py b/core/admin.py index 367c056a..30da472a 100644 --- a/core/admin.py +++ b/core/admin.py @@ -23,8 +23,8 @@ admin.site.unregister(AuthGroup) @admin.register(Group) class GroupAdmin(admin.ModelAdmin): - list_display = ("name", "description", "is_meta") - list_filter = ("is_meta",) + list_display = ("name", "description", "is_manually_manageable") + list_filter = ("is_manually_manageable",) search_fields = ("name",) diff --git a/core/baker_recipes.py b/core/baker_recipes.py index 0abd83e0..4b873b0f 100644 --- a/core/baker_recipes.py +++ b/core/baker_recipes.py @@ -7,7 +7,7 @@ from model_bakery import seq from model_bakery.recipe import Recipe, related from club.models import Membership -from core.models import User +from core.models import Group, User from subscription.models import Subscription active_subscription = Recipe( @@ -60,5 +60,6 @@ board_user = Recipe( first_name="AE", last_name=seq("member "), memberships=related(ae_board_membership), + groups=lambda: [Group.objects.get(club_board=settings.SITH_MAIN_CLUB_ID)], ) """A user which is in the board of the AE.""" diff --git a/core/management/commands/populate.py b/core/management/commands/populate.py index 5770c715..23e370f3 100644 --- a/core/management/commands/populate.py +++ b/core/management/commands/populate.py @@ -46,7 +46,7 @@ from accounting.models import ( ) from club.models import Club, Membership from com.models import News, NewsDate, Sith, Weekmail -from core.models import Group, Page, PageRev, RealGroup, SithFile, User +from core.models import Group, Page, PageRev, SithFile, User from core.utils import resize_image from counter.models import Counter, Product, ProductType, StudentCard from election.models import Candidature, Election, ElectionList, Role @@ -80,19 +80,21 @@ class Command(BaseCommand): Sith.objects.create(weekmail_destinations="etudiants@git.an personnel@git.an") Site.objects.create(domain=settings.SITH_URL, name=settings.SITH_NAME) - root_group = Group.objects.create(name="Root") + root_group = Group.objects.create(name="Root", is_manually_manageable=True) public_group = Group.objects.create(name="Public") subscribers = Group.objects.create(name="Subscribers") old_subscribers = Group.objects.create(name="Old subscribers") - Group.objects.create(name="Accounting admin") - Group.objects.create(name="Communication admin") - Group.objects.create(name="Counter admin") - Group.objects.create(name="Banned from buying alcohol") - Group.objects.create(name="Banned from counters") - Group.objects.create(name="Banned to subscribe") - Group.objects.create(name="SAS admin") - Group.objects.create(name="Forum admin") - Group.objects.create(name="Pedagogy admin") + Group.objects.create(name="Accounting admin", is_manually_manageable=True) + Group.objects.create(name="Communication admin", is_manually_manageable=True) + Group.objects.create(name="Counter admin", is_manually_manageable=True) + Group.objects.create( + name="Banned from buying alcohol", is_manually_manageable=True + ) + Group.objects.create(name="Banned from counters", is_manually_manageable=True) + Group.objects.create(name="Banned to subscribe", is_manually_manageable=True) + Group.objects.create(name="SAS admin", is_manually_manageable=True) + Group.objects.create(name="Forum admin", is_manually_manageable=True) + Group.objects.create(name="Pedagogy admin", is_manually_manageable=True) self.reset_index("core", "auth") change_billing = Permission.objects.get(codename="change_billinginfo") @@ -148,7 +150,9 @@ class Command(BaseCommand): Counter.objects.bulk_create(counters) bar_groups = [] for bar_id, bar_name in settings.SITH_COUNTER_BARS: - group = RealGroup.objects.create(name=f"{bar_name} admin") + group = Group.objects.create( + name=f"{bar_name} admin", is_manually_manageable=True + ) bar_groups.append( Counter.edit_groups.through(counter_id=bar_id, group=group) ) @@ -381,46 +385,42 @@ Welcome to the wiki page! parent=main_club, ) - Membership.objects.bulk_create( - [ - Membership(user=skia, club=main_club, role=3), - Membership( - user=comunity, - club=bar_club, - start_date=localdate(), - role=settings.SITH_CLUB_ROLES_ID["Board member"], - ), - Membership( - user=sli, - club=troll, - role=9, - description="Padawan Troll", - start_date=localdate() - timedelta(days=17), - ), - Membership( - user=krophil, - club=troll, - role=10, - description="Maitre Troll", - start_date=localdate() - timedelta(days=200), - ), - Membership( - user=skia, - club=troll, - role=2, - description="Grand Ancien Troll", - start_date=localdate() - timedelta(days=400), - end_date=localdate() - timedelta(days=86), - ), - Membership( - user=richard, - club=troll, - role=2, - description="", - start_date=localdate() - timedelta(days=200), - end_date=localdate() - timedelta(days=100), - ), - ] + Membership.objects.create(user=skia, club=main_club, role=3) + Membership.objects.create( + user=comunity, + club=bar_club, + start_date=localdate(), + role=settings.SITH_CLUB_ROLES_ID["Board member"], + ) + Membership.objects.create( + user=sli, + club=troll, + role=9, + description="Padawan Troll", + start_date=localdate() - timedelta(days=17), + ) + Membership.objects.create( + user=krophil, + club=troll, + role=10, + description="Maitre Troll", + start_date=localdate() - timedelta(days=200), + ) + Membership.objects.create( + user=skia, + club=troll, + role=2, + description="Grand Ancien Troll", + start_date=localdate() - timedelta(days=400), + end_date=localdate() - timedelta(days=86), + ) + Membership.objects.create( + user=richard, + club=troll, + role=2, + description="", + start_date=localdate() - timedelta(days=200), + end_date=localdate() - timedelta(days=100), ) p = ProductType.objects.create(name="Bières bouteilles") @@ -607,7 +607,6 @@ Welcome to the wiki page! ) # Create an election - ae_board_group = Group.objects.get(name=settings.SITH_MAIN_BOARD_GROUP) el = Election.objects.create( title="Élection 2017", description="La roue tourne", @@ -617,7 +616,7 @@ Welcome to the wiki page! end_date="7942-06-12 10:28:45+01", ) el.view_groups.add(public_group) - el.edit_groups.add(ae_board_group) + el.edit_groups.add(main_club.board_group) el.candidature_groups.add(subscribers) el.vote_groups.add(subscribers) liste = ElectionList.objects.create(title="Candidature Libre", election=el) diff --git a/core/migrations/0001_initial.py b/core/migrations/0001_initial.py index 47cad8ad..23af8d06 100644 --- a/core/migrations/0001_initial.py +++ b/core/migrations/0001_initial.py @@ -563,14 +563,21 @@ class Migration(migrations.Migration): fields=[], options={"proxy": True}, bases=("core.group",), - managers=[("objects", core.models.MetaGroupManager())], + managers=[("objects", django.contrib.auth.models.GroupManager())], ), + # at first, there existed a RealGroupManager and a RealGroupManager, + # which have been since been removed. + # However, this removal broke the migrations because it caused an ImportError. + # Thus, the managers MetaGroupManager (above) and RealGroupManager (below) + # have been replaced by the base django GroupManager to fix the import. + # As those managers aren't actually used in migrations, + # this replacement doesn't break anything. migrations.CreateModel( name="RealGroup", fields=[], options={"proxy": True}, bases=("core.group",), - managers=[("objects", core.models.RealGroupManager())], + managers=[("objects", django.contrib.auth.models.GroupManager())], ), migrations.AlterUniqueTogether( name="page", unique_together={("name", "parent")} diff --git a/core/migrations/0041_delete_metagroup_alter_group_options_and_more.py b/core/migrations/0041_delete_metagroup_alter_group_options_and_more.py new file mode 100644 index 00000000..2a88b8c1 --- /dev/null +++ b/core/migrations/0041_delete_metagroup_alter_group_options_and_more.py @@ -0,0 +1,51 @@ +# Generated by Django 4.2.16 on 2024-11-30 13:16 + +from django.db import migrations, models +from django.db.migrations.state import StateApps +from django.db.models import F + + +def invert_is_manually_manageable(apps: StateApps, schema_editor): + """Invert `is_manually_manageable`. + + This field is a renaming of `is_meta`. + However, the meaning has been inverted : the groups + which were meta are not manually manageable and vice versa. + Thus, the value must be inverted. + """ + Group = apps.get_model("core", "Group") + Group.objects.all().update(is_manually_manageable=~F("is_manually_manageable")) + + +class Migration(migrations.Migration): + dependencies = [("core", "0040_alter_user_options_user_user_permissions_and_more")] + + operations = [ + migrations.DeleteModel( + name="MetaGroup", + ), + migrations.DeleteModel( + name="RealGroup", + ), + migrations.AlterModelOptions( + name="group", + options={}, + ), + migrations.RenameField( + model_name="group", + old_name="is_meta", + new_name="is_manually_manageable", + ), + migrations.AlterField( + model_name="group", + name="is_manually_manageable", + field=models.BooleanField( + default=False, + help_text="If False, this shouldn't be shown on group management pages", + verbose_name="Is manually manageable", + ), + ), + migrations.RunPython( + invert_is_manually_manageable, reverse_code=invert_is_manually_manageable + ), + ] diff --git a/core/models.py b/core/models.py index 347e9bd4..4e93b73c 100644 --- a/core/models.py +++ b/core/models.py @@ -36,7 +36,6 @@ from django.conf import settings from django.contrib.auth.models import AbstractUser, UserManager from django.contrib.auth.models import AnonymousUser as AuthAnonymousUser from django.contrib.auth.models import Group as AuthGroup -from django.contrib.auth.models import GroupManager as AuthGroupManager from django.contrib.staticfiles.storage import staticfiles_storage from django.core import validators from django.core.cache import cache @@ -58,34 +57,17 @@ if TYPE_CHECKING: from club.models import Club -class RealGroupManager(AuthGroupManager): - def get_queryset(self): - return super().get_queryset().filter(is_meta=False) - - -class MetaGroupManager(AuthGroupManager): - def get_queryset(self): - return super().get_queryset().filter(is_meta=True) - - class Group(AuthGroup): - """Implement both RealGroups and Meta groups. + """Wrapper around django.auth.Group""" - Groups are sorted by their is_meta property - """ - - #: If False, this is a RealGroup - is_meta = models.BooleanField( - _("meta group status"), + is_manually_manageable = models.BooleanField( + _("Is manually manageable"), default=False, - help_text=_("Whether a group is a meta group or not"), + help_text=_("If False, this shouldn't be shown on group management pages"), ) #: Description of the group description = models.CharField(_("description"), max_length=60) - class Meta: - ordering = ["name"] - def get_absolute_url(self) -> str: return reverse("core:group_list") @@ -100,65 +82,6 @@ class Group(AuthGroup): cache.delete(f"sith_group_{self.name.replace(' ', '_')}") -class MetaGroup(Group): - """MetaGroups are dynamically created groups. - - Generally used with clubs where creating a club creates two groups: - - * club-SITH_BOARD_SUFFIX - * club-SITH_MEMBER_SUFFIX - """ - - #: Assign a manager in a way that MetaGroup.objects only return groups with is_meta=False - objects = MetaGroupManager() - - class Meta: - proxy = True - - def __init__(self, *args, **kwargs): - super().__init__(*args, **kwargs) - self.is_meta = True - - @cached_property - def associated_club(self) -> Club | None: - """Return the group associated with this meta group. - - The result of this function is cached - - - Returns: - The associated club if it exists, else None - """ - from club.models import Club - - if self.name.endswith(settings.SITH_BOARD_SUFFIX): - # replace this with str.removesuffix as soon as Python - # is upgraded to 3.10 - club_name = self.name[: -len(settings.SITH_BOARD_SUFFIX)] - elif self.name.endswith(settings.SITH_MEMBER_SUFFIX): - club_name = self.name[: -len(settings.SITH_MEMBER_SUFFIX)] - else: - return None - club = cache.get(f"sith_club_{club_name}") - if club is None: - club = Club.objects.filter(unix_name=club_name).first() - cache.set(f"sith_club_{club_name}", club) - return club - - -class RealGroup(Group): - """RealGroups are created by the developer. - - Most of the time they match a number in settings to be easily used for permissions. - """ - - #: Assign a manager in a way that MetaGroup.objects only return groups with is_meta=True - objects = RealGroupManager() - - class Meta: - proxy = True - - def validate_promo(value: int) -> None: start_year = settings.SITH_SCHOOL_START_YEAR delta = (localdate() + timedelta(days=180)).year - start_year @@ -204,8 +127,8 @@ def get_group(*, pk: int | None = None, name: str | None = None) -> Group | None else: group = Group.objects.filter(name=name).first() if group is not None: - cache.set(f"sith_group_{group.id}", group) - cache.set(f"sith_group_{group.name.replace(' ', '_')}", group) + name = group.name.replace(" ", "_") + cache.set_many({f"sith_group_{group.id}": group, f"sith_group_{name}": group}) else: cache.set(f"sith_group_{pk_or_name}", "not_found") return group @@ -438,18 +361,6 @@ class User(AbstractUser): return self.was_subscribed if group.id == settings.SITH_GROUP_ROOT_ID: return self.is_root - if group.is_meta: - # check if this group is associated with a club - group.__class__ = MetaGroup - club = group.associated_club - if club is None: - return False - membership = club.get_membership_for(self) - if membership is None: - return False - if group.name.endswith(settings.SITH_MEMBER_SUFFIX): - return True - return membership.role > settings.SITH_MAXIMUM_FREE_ROLE return group in self.cached_groups @property @@ -474,12 +385,11 @@ class User(AbstractUser): return any(g.id == root_id for g in self.cached_groups) @cached_property - def is_board_member(self): - main_club = settings.SITH_MAIN_CLUB["unix_name"] - return self.is_in_group(name=main_club + settings.SITH_BOARD_SUFFIX) + def is_board_member(self) -> bool: + return self.groups.filter(club_board=settings.SITH_MAIN_CLUB_ID).exists() @cached_property - def can_read_subscription_history(self): + def can_read_subscription_history(self) -> bool: if self.is_root or self.is_board_member: return True @@ -951,7 +861,7 @@ class SithFile(models.Model): param="1", ).save() - def is_owned_by(self, user): + def is_owned_by(self, user: User) -> bool: if user.is_anonymous: return False if user.is_root: @@ -966,7 +876,7 @@ class SithFile(models.Model): return True return user.id == self.owner_id - def can_be_viewed_by(self, user): + def can_be_viewed_by(self, user: User) -> bool: if hasattr(self, "profile_of"): return user.can_view(self.profile_of) if hasattr(self, "avatar_of"): diff --git a/core/tests/test_core.py b/core/tests/test_core.py index a33a8705..a152b579 100644 --- a/core/tests/test_core.py +++ b/core/tests/test_core.py @@ -18,6 +18,7 @@ from smtplib import SMTPException import freezegun import pytest +from django.contrib.auth.hashers import make_password from django.core import mail from django.core.cache import cache from django.core.mail import EmailMessage @@ -30,7 +31,7 @@ from model_bakery import baker from pytest_django.asserts import assertInHTML, assertRedirects from antispam.models import ToxicDomain -from club.models import Membership +from club.models import Club, Membership from core.markdown import markdown from core.models import AnonymousUser, Group, Page, User from core.utils import get_semester_code, get_start_of_semester @@ -145,7 +146,7 @@ class TestUserRegistration: class TestUserLogin: @pytest.fixture() def user(self) -> User: - return User.objects.first() + return baker.make(User, password=make_password("plop")) def test_login_fail(self, client, user): """Should not login a user correctly.""" @@ -349,14 +350,9 @@ class TestUserIsInGroup(TestCase): @classmethod def setUpTestData(cls): - from club.models import Club - cls.root_group = Group.objects.get(name="Root") - cls.public = Group.objects.get(name="Public") - cls.skia = User.objects.get(username="skia") - cls.toto = User.objects.create( - username="toto", first_name="a", last_name="b", email="a.b@toto.fr" - ) + cls.public_group = Group.objects.get(name="Public") + cls.public_user = baker.make(User) cls.subscribers = Group.objects.get(name="Subscribers") cls.old_subscribers = Group.objects.get(name="Old subscribers") cls.accounting_admin = Group.objects.get(name="Accounting admin") @@ -366,22 +362,12 @@ class TestUserIsInGroup(TestCase): cls.banned_counters = Group.objects.get(name="Banned from counters") cls.banned_subscription = Group.objects.get(name="Banned to subscribe") cls.sas_admin = Group.objects.get(name="SAS admin") - cls.club = Club.objects.create( - name="Fake Club", - unix_name="fake-club", - address="Fake address", - ) + cls.club = baker.make(Club) cls.main_club = Club.objects.get(id=1) def assert_in_public_group(self, user): - assert user.is_in_group(pk=self.public.id) - assert user.is_in_group(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 - assert user.is_in_group(name=meta_groups_board) is False - assert user.is_in_group(name=meta_groups_members) is False + assert user.is_in_group(pk=self.public_group.id) + assert user.is_in_group(name=self.public_group.name) def assert_only_in_public_group(self, user): self.assert_in_public_group(user) @@ -392,13 +378,11 @@ class TestUserIsInGroup(TestCase): self.sas_admin, self.subscribers, self.old_subscribers, + self.club.members_group, + self.club.board_group, ): assert not user.is_in_group(pk=group.pk) assert not user.is_in_group(name=group.name) - meta_groups_board = self.club.unix_name + settings.SITH_BOARD_SUFFIX - meta_groups_members = self.club.unix_name + settings.SITH_MEMBER_SUFFIX - assert user.is_in_group(name=meta_groups_board) is False - assert user.is_in_group(name=meta_groups_members) is False def test_anonymous_user(self): """Test that anonymous users are only in the public group.""" @@ -407,80 +391,80 @@ class TestUserIsInGroup(TestCase): def test_not_subscribed_user(self): """Test that users who never subscribed are only in the public group.""" - self.assert_only_in_public_group(self.toto) + self.assert_only_in_public_group(self.public_user) def test_wrong_parameter_fail(self): """Test that when neither the pk nor the name argument is given, the function raises a ValueError. """ with self.assertRaises(ValueError): - self.toto.is_in_group() + self.public_user.is_in_group() def test_number_queries(self): """Test that the number of db queries is stable and that less queries are made when making a new call. """ # make sure Skia is in at least one group - self.skia.groups.add(Group.objects.first().pk) - skia_groups = self.skia.groups.all() + group_in = baker.make(Group) + self.public_user.groups.add(group_in) - group_in = skia_groups.first() cache.clear() # Test when the user is in the group with self.assertNumQueries(2): - self.skia.is_in_group(pk=group_in.id) + self.public_user.is_in_group(pk=group_in.id) with self.assertNumQueries(0): - self.skia.is_in_group(pk=group_in.id) + self.public_user.is_in_group(pk=group_in.id) - ids = skia_groups.values_list("pk", flat=True) - group_not_in = Group.objects.exclude(pk__in=ids).first() + group_not_in = baker.make(Group) cache.clear() # Test when the user is not in the group with self.assertNumQueries(2): - self.skia.is_in_group(pk=group_not_in.id) + self.public_user.is_in_group(pk=group_not_in.id) with self.assertNumQueries(0): - self.skia.is_in_group(pk=group_not_in.id) + self.public_user.is_in_group(pk=group_not_in.id) def test_cache_properly_cleared_membership(self): """Test that when the membership of a user end, the cache is properly invalidated. """ - membership = Membership.objects.create( - club=self.club, user=self.toto, end_date=None - ) - meta_groups_members = self.club.unix_name + settings.SITH_MEMBER_SUFFIX + membership = baker.make(Membership, club=self.club, user=self.public_user) cache.clear() - assert self.toto.is_in_group(name=meta_groups_members) is True - assert membership == cache.get(f"membership_{self.club.id}_{self.toto.id}") + self.club.get_membership_for(self.public_user) # this should populate the cache + assert membership == cache.get( + f"membership_{self.club.id}_{self.public_user.id}" + ) membership.end_date = now() - timedelta(minutes=5) membership.save() - cached_membership = cache.get(f"membership_{self.club.id}_{self.toto.id}") + cached_membership = cache.get( + f"membership_{self.club.id}_{self.public_user.id}" + ) assert cached_membership == "not_member" - assert self.toto.is_in_group(name=meta_groups_members) is False def test_cache_properly_cleared_group(self): """Test that when a user is removed from a group, the is_in_group_method return False when calling it again. """ # testing with pk - self.toto.groups.add(self.com_admin.pk) - assert self.toto.is_in_group(pk=self.com_admin.pk) is True + self.public_user.groups.add(self.com_admin.pk) + assert self.public_user.is_in_group(pk=self.com_admin.pk) is True - self.toto.groups.remove(self.com_admin.pk) - assert self.toto.is_in_group(pk=self.com_admin.pk) is False + self.public_user.groups.remove(self.com_admin.pk) + assert self.public_user.is_in_group(pk=self.com_admin.pk) is False # testing with name - self.toto.groups.add(self.sas_admin.pk) - assert self.toto.is_in_group(name="SAS admin") is True + self.public_user.groups.add(self.sas_admin.pk) + assert self.public_user.is_in_group(name="SAS admin") is True - self.toto.groups.remove(self.sas_admin.pk) - assert self.toto.is_in_group(name="SAS admin") is False + self.public_user.groups.remove(self.sas_admin.pk) + assert self.public_user.is_in_group(name="SAS admin") is False def test_not_existing_group(self): """Test that searching for a not existing group returns False. """ - assert self.skia.is_in_group(name="This doesn't exist") is False + user = baker.make(User) + user.groups.set(list(Group.objects.all())) + assert not user.is_in_group(name="This doesn't exist") class TestDateUtils(TestCase): diff --git a/core/views/forms.py b/core/views/forms.py index 8a998ab0..a3f38b83 100644 --- a/core/views/forms.py +++ b/core/views/forms.py @@ -32,19 +32,14 @@ from django.contrib.staticfiles.management.commands.collectstatic import ( ) from django.core.exceptions import ValidationError from django.db import transaction -from django.forms import ( - CheckboxSelectMultiple, - DateInput, - DateTimeInput, - TextInput, -) +from django.forms import CheckboxSelectMultiple, DateInput, DateTimeInput, TextInput from django.utils.translation import gettext from django.utils.translation import gettext_lazy as _ from phonenumber_field.widgets import RegionalPhoneNumberWidget from PIL import Image from antispam.forms import AntiSpamEmailField -from core.models import Gift, Page, RealGroup, SithFile, User +from core.models import Gift, Group, Page, SithFile, User from core.utils import resize_image from core.views.widgets.select import ( AutoCompleteSelect, @@ -290,7 +285,7 @@ class UserGroupsForm(forms.ModelForm): required_css_class = "required" groups = forms.ModelMultipleChoiceField( - queryset=RealGroup.objects.all(), + queryset=Group.objects.filter(is_manually_manageable=True), widget=CheckboxSelectMultiple, label=_("Groups"), ) diff --git a/core/views/group.py b/core/views/group.py index b6e77b54..978fe686 100644 --- a/core/views/group.py +++ b/core/views/group.py @@ -21,7 +21,7 @@ from django.utils.translation import gettext_lazy as _ from django.views.generic import ListView from django.views.generic.edit import CreateView, DeleteView, UpdateView -from core.models import RealGroup, User +from core.models import Group, User from core.views import CanCreateMixin, CanEditMixin, DetailFormView from core.views.widgets.select import AutoCompleteSelectMultipleUser @@ -57,7 +57,8 @@ class EditMembersForm(forms.Form): class GroupListView(CanEditMixin, ListView): """Displays the Group list.""" - model = RealGroup + model = Group + queryset = Group.objects.filter(is_manually_manageable=True) ordering = ["name"] template_name = "core/group_list.jinja" @@ -65,7 +66,8 @@ class GroupListView(CanEditMixin, ListView): class GroupEditView(CanEditMixin, UpdateView): """Edit infos of a Group.""" - model = RealGroup + model = Group + queryset = Group.objects.filter(is_manually_manageable=True) pk_url_kwarg = "group_id" template_name = "core/group_edit.jinja" fields = ["name", "description"] @@ -74,7 +76,8 @@ class GroupEditView(CanEditMixin, UpdateView): class GroupCreateView(CanCreateMixin, CreateView): """Add a new Group.""" - model = RealGroup + model = Group + queryset = Group.objects.filter(is_manually_manageable=True) template_name = "core/create.jinja" fields = ["name", "description"] @@ -84,7 +87,8 @@ class GroupTemplateView(CanEditMixin, DetailFormView): Allow adding and removing users from it. """ - model = RealGroup + model = Group + queryset = Group.objects.filter(is_manually_manageable=True) form_class = EditMembersForm pk_url_kwarg = "group_id" template_name = "core/group_detail.jinja" @@ -118,7 +122,8 @@ class GroupTemplateView(CanEditMixin, DetailFormView): class GroupDeleteView(CanEditMixin, DeleteView): """Delete a Group.""" - model = RealGroup + model = Group + queryset = Group.objects.filter(is_manually_manageable=True) pk_url_kwarg = "group_id" template_name = "core/delete_confirm.jinja" success_url = reverse_lazy("core:group_list") diff --git a/counter/models.py b/counter/models.py index 48bb841f..326ea66e 100644 --- a/counter/models.py +++ b/counter/models.py @@ -530,9 +530,12 @@ class Counter(models.Model): return user.is_in_group(pk=settings.SITH_GROUP_COUNTER_ADMIN_ID) def can_be_viewed_by(self, user: User) -> bool: - if self.type == "BAR": - return True - return user.is_board_member or user in self.sellers.all() + return ( + self.type == "BAR" + or user.is_root + or user.is_in_group(pk=self.club.board_group_id) + or user in self.sellers.all() + ) def gen_token(self) -> None: """Generate a new token for this counter.""" diff --git a/election/tests.py b/election/tests.py index 5152cc3d..f4cc380e 100644 --- a/election/tests.py +++ b/election/tests.py @@ -11,8 +11,6 @@ class TestElection(TestCase): def setUpTestData(cls): cls.election = Election.objects.first() cls.public_group = Group.objects.get(id=settings.SITH_GROUP_PUBLIC_ID) - cls.subscriber_group = Group.objects.get(name=settings.SITH_MAIN_MEMBERS_GROUP) - cls.ae_board_group = Group.objects.get(name=settings.SITH_MAIN_BOARD_GROUP) cls.sli = User.objects.get(username="sli") cls.subscriber = User.objects.get(username="subscriber") cls.public = User.objects.get(username="public") diff --git a/galaxy/management/commands/generate_galaxy_test_data.py b/galaxy/management/commands/generate_galaxy_test_data.py index 39334ddd..2c3ab7c8 100644 --- a/galaxy/management/commands/generate_galaxy_test_data.py +++ b/galaxy/management/commands/generate_galaxy_test_data.py @@ -118,18 +118,18 @@ class Command(BaseCommand): self.make_important_citizen(u) def make_clubs(self): - """Create all the clubs (:class:`club.models.Club`). + """Create all the clubs [club.models.Club][]. After creation, the clubs are stored in `self.clubs` for fast access later. - Don't create the meta groups (:class:`core.models.MetaGroup`) - nor the pages of the clubs (:class:`core.models.Page`). + Don't create the groups associated to the clubs + nor the pages of the clubs ([core.models.Page][]). """ - self.clubs = [] - for i in range(self.NB_CLUBS): - self.clubs.append(Club(unix_name=f"galaxy-club-{i}", name=f"club-{i}")) - # We don't need to create corresponding groups here, as the Galaxy doesn't care about them - Club.objects.bulk_create(self.clubs) - self.clubs = list(Club.objects.filter(unix_name__startswith="galaxy-").all()) + self.clubs = Club.objects.bulk_create( + [ + Club(unix_name=f"galaxy-club-{i}", name=f"club-{i}") + for i in range(self.NB_CLUBS) + ] + ) def make_users(self): """Create all the users and store them in `self.users` for fast access later.