From d815028c4dccb232a593475945fc0bc41a633b00 Mon Sep 17 00:00:00 2001 From: Erik Kalkoken Date: Sat, 18 Jun 2022 02:47:14 +0000 Subject: [PATCH] Fix: Changing group's state setting does not kick existing non-conforming group members --- allianceauth/groupmanagement/admin.py | 54 ++++-- allianceauth/groupmanagement/models.py | 9 + allianceauth/groupmanagement/tasks.py | 10 ++ .../groupmanagement/tests/test_admin.py | 170 ++++++++++++++---- .../groupmanagement/tests/test_models.py | 32 ++++ 5 files changed, 227 insertions(+), 48 deletions(-) create mode 100644 allianceauth/groupmanagement/tasks.py diff --git a/allianceauth/groupmanagement/admin.py b/allianceauth/groupmanagement/admin.py index 3657fc73..d51cf7d2 100644 --- a/allianceauth/groupmanagement/admin.py +++ b/allianceauth/groupmanagement/admin.py @@ -1,8 +1,8 @@ from django.apps import apps from django.contrib import admin -from django.contrib.auth.models import Group as BaseGroup -from django.contrib.auth.models import Permission, User -from django.db.models import Count + +from django.contrib.auth.models import Group as BaseGroup, Permission, User +from django.db.models import Count, Exists, OuterRef from django.db.models.functions import Lower from django.db.models.signals import ( m2m_changed, @@ -15,6 +15,7 @@ from django.dispatch import receiver from .forms import GroupAdminForm, ReservedGroupNameAdminForm from .models import AuthGroup, GroupRequest, ReservedGroupName +from .tasks import remove_users_not_matching_states_from_group if 'eve_autogroups' in apps.app_configs: _has_auto_groups = True @@ -106,14 +107,13 @@ class HasLeaderFilter(admin.SimpleListFilter): class GroupAdmin(admin.ModelAdmin): form = GroupAdminForm - list_select_related = ('authgroup',) ordering = ('name',) list_display = ( 'name', '_description', '_properties', '_member_count', - 'has_leader' + 'has_leader', ) list_filter = [ 'authgroup__internal', @@ -129,31 +129,51 @@ class GroupAdmin(admin.ModelAdmin): def get_queryset(self, request): qs = super().get_queryset(request) - if _has_auto_groups: - qs = qs.prefetch_related('managedalliancegroup_set', 'managedcorpgroup_set') - qs = qs.prefetch_related('authgroup__group_leaders').select_related('authgroup') - qs = qs.annotate( - member_count=Count('user', distinct=True), + has_leader_qs = ( + AuthGroup.objects.filter(group=OuterRef('pk'), group_leaders__isnull=False) ) + has_leader_groups_qs = ( + AuthGroup.objects.filter( + group=OuterRef('pk'), group_leader_groups__isnull=False + ) + ) + qs = ( + qs.select_related('authgroup') + .annotate(member_count=Count('user', distinct=True)) + .annotate(has_leader=Exists(has_leader_qs)) + .annotate(has_leader_groups=Exists(has_leader_groups_qs)) + ) + if _has_auto_groups: + is_autogroup_corp = ( + Group.objects.filter( + pk=OuterRef('pk'), managedcorpgroup__isnull=False + ) + ) + is_autogroup_alliance = ( + Group.objects.filter( + pk=OuterRef('pk'), managedalliancegroup__isnull=False + ) + ) + qs = ( + qs.annotate(is_autogroup_corp=Exists(is_autogroup_corp)) + .annotate(is_autogroup_alliance=Exists(is_autogroup_alliance)) + ) return qs def _description(self, obj): return obj.authgroup.description - @admin.display(description="Members", ordering="member_count") + @admin.display(description='Members', ordering='member_count') def _member_count(self, obj): return obj.member_count @admin.display(boolean=True) def has_leader(self, obj): - return obj.authgroup.group_leaders.exists() or obj.authgroup.group_leader_groups.exists() + return obj.has_leader or obj.has_leader_groups def _properties(self, obj): properties = list() - if _has_auto_groups and ( - obj.managedalliancegroup_set.exists() - or obj.managedcorpgroup_set.exists() - ): + if _has_auto_groups and (obj.is_autogroup_corp or obj.is_autogroup_alliance): properties.append('Auto Group') elif obj.authgroup.internal: properties.append('Internal') @@ -183,6 +203,8 @@ class GroupAdmin(admin.ModelAdmin): ag_instance = inline_form.save(commit=False) ag_instance.group = form.instance ag_instance.save() + if ag_instance.states.exists(): + remove_users_not_matching_states_from_group.delay(ag_instance.group.pk) formset.save() def get_readonly_fields(self, request, obj=None): diff --git a/allianceauth/groupmanagement/models.py b/allianceauth/groupmanagement/models.py index 102f70a7..3ca6b8c6 100644 --- a/allianceauth/groupmanagement/models.py +++ b/allianceauth/groupmanagement/models.py @@ -189,6 +189,15 @@ class AuthGroup(models.Model): | User.objects.filter(groups__in=list(self.group_leader_groups.all())) ) + def remove_users_not_matching_states(self): + """Remove users not matching defined states from related group.""" + states_qs = self.states.all() + if states_qs.exists(): + states = list(states_qs) + non_compliant_users = self.group.user_set.exclude(profile__state__in=states) + for user in non_compliant_users: + self.group.user_set.remove(user) + class ReservedGroupName(models.Model): """Name that can not be used for groups. diff --git a/allianceauth/groupmanagement/tasks.py b/allianceauth/groupmanagement/tasks.py new file mode 100644 index 00000000..ae80f25f --- /dev/null +++ b/allianceauth/groupmanagement/tasks.py @@ -0,0 +1,10 @@ +from celery import shared_task + +from django.contrib.auth.models import Group + + +@shared_task +def remove_users_not_matching_states_from_group(group_pk: int) -> None: + """Remove users not matching defined states from related group.""" + group = Group.objects.get(pk=group_pk) + group.authgroup.remove_users_not_matching_states() diff --git a/allianceauth/groupmanagement/tests/test_admin.py b/allianceauth/groupmanagement/tests/test_admin.py index 3b4a50c1..3873fe4c 100644 --- a/allianceauth/groupmanagement/tests/test_admin.py +++ b/allianceauth/groupmanagement/tests/test_admin.py @@ -6,7 +6,7 @@ from django.conf import settings from django.contrib import admin from django.contrib.admin.sites import AdminSite from django.contrib.auth.models import User -from django.test import TestCase, RequestFactory, Client +from django.test import TestCase, RequestFactory, Client, override_settings from allianceauth.authentication.models import CharacterOwnership, State from allianceauth.eveonline.models import ( @@ -236,60 +236,104 @@ class TestGroupAdmin(TestCase): self.assertEqual(result, expected) def test_member_count(self): - expected = 1 - obj = self.modeladmin.get_queryset(MockRequest(user=self.user_1))\ - .get(pk=self.group_1.pk) + # given + request = MockRequest(user=self.user_1) + obj = self.modeladmin.get_queryset(request).get(pk=self.group_1.pk) + # when result = self.modeladmin._member_count(obj) - self.assertEqual(result, expected) + # then + self.assertEqual(result, 1) def test_has_leader_user(self): - result = self.modeladmin.has_leader(self.group_1) + # given + request = MockRequest(user=self.user_1) + obj = self.modeladmin.get_queryset(request).get(pk=self.group_1.pk) + # when + result = self.modeladmin.has_leader(obj) + # then self.assertTrue(result) def test_has_leader_group(self): - result = self.modeladmin.has_leader(self.group_2) + # given + request = MockRequest(user=self.user_1) + obj = self.modeladmin.get_queryset(request).get(pk=self.group_2.pk) + # when + result = self.modeladmin.has_leader(obj) + # then self.assertTrue(result) def test_properties_1(self): - expected = ['Default'] - result = self.modeladmin._properties(self.group_1) - self.assertListEqual(result, expected) + # given + request = MockRequest(user=self.user_1) + obj = self.modeladmin.get_queryset(request).get(pk=self.group_1.pk) + # when + result = self.modeladmin._properties(obj) + self.assertListEqual(result, ['Default']) def test_properties_2(self): - expected = ['Internal'] - result = self.modeladmin._properties(self.group_2) - self.assertListEqual(result, expected) + # given + request = MockRequest(user=self.user_1) + obj = self.modeladmin.get_queryset(request).get(pk=self.group_2.pk) + # when + result = self.modeladmin._properties(obj) + self.assertListEqual(result, ['Internal']) def test_properties_3(self): - expected = ['Hidden'] - result = self.modeladmin._properties(self.group_3) - self.assertListEqual(result, expected) + # given + request = MockRequest(user=self.user_1) + obj = self.modeladmin.get_queryset(request).get(pk=self.group_3.pk) + # when + result = self.modeladmin._properties(obj) + self.assertListEqual(result, ['Hidden']) def test_properties_4(self): - expected = ['Open'] - result = self.modeladmin._properties(self.group_4) - self.assertListEqual(result, expected) + # given + request = MockRequest(user=self.user_1) + obj = self.modeladmin.get_queryset(request).get(pk=self.group_4.pk) + # when + result = self.modeladmin._properties(obj) + self.assertListEqual(result, ['Open']) def test_properties_5(self): - expected = ['Public'] - result = self.modeladmin._properties(self.group_5) - self.assertListEqual(result, expected) + # given + request = MockRequest(user=self.user_1) + obj = self.modeladmin.get_queryset(request).get(pk=self.group_5.pk) + # when + result = self.modeladmin._properties(obj) + self.assertListEqual(result, ['Public']) def test_properties_6(self): - expected = ['Hidden', 'Open', 'Public'] - result = self.modeladmin._properties(self.group_6) - self.assertListEqual(result, expected) + # given + request = MockRequest(user=self.user_1) + obj = self.modeladmin.get_queryset(request).get(pk=self.group_6.pk) + # when + result = self.modeladmin._properties(obj) + self.assertListEqual(result, ['Hidden', 'Open', 'Public']) if _has_auto_groups: @patch(MODULE_PATH + '._has_auto_groups', True) - def test_properties_7(self): + def test_should_show_autogroup_for_corporation(self): + # given self._create_autogroups() - expected = ['Auto Group'] - my_group = Group.objects\ - .filter(managedcorpgroup__isnull=False)\ - .first() - result = self.modeladmin._properties(my_group) - self.assertListEqual(result, expected) + request = MockRequest(user=self.user_1) + queryset = self.modeladmin.get_queryset(request) + obj = queryset.filter(managedcorpgroup__isnull=False).first() + # when + result = self.modeladmin._properties(obj) + # then + self.assertListEqual(result, ['Auto Group']) + + @patch(MODULE_PATH + '._has_auto_groups', True) + def test_should_show_autogroup_for_alliance(self): + # given + self._create_autogroups() + request = MockRequest(user=self.user_1) + queryset = self.modeladmin.get_queryset(request) + obj = queryset.filter(managedalliancegroup__isnull=False).first() + # when + result = self.modeladmin._properties(obj) + # then + self.assertListEqual(result, ['Auto Group']) # actions @@ -539,6 +583,68 @@ class TestGroupAdminChangeFormSuperuserExclusiveEdits(WebTest): self.assertNotIn(field, form.fields) +@override_settings(CELERY_ALWAYS_EAGER=True, CELERY_EAGER_PROPAGATES_EXCEPTIONS=True) +class TestGroupAdmin2(TestCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.superuser = User.objects.create_superuser("super") + + def test_should_remove_users_from_state_groups(self): + # given + user_member = AuthUtils.create_user("Bruce Wayne") + character_member = AuthUtils.add_main_character_2( + user_member, + name="Bruce Wayne", + character_id=1001, + corp_id=2001, + corp_name="Wayne Technologies", + ) + user_guest = AuthUtils.create_user("Lex Luthor") + AuthUtils.add_main_character_2( + user_guest, + name="Lex Luthor", + character_id=1011, + corp_id=2011, + corp_name="Luthor Corp", + ) + member_state = AuthUtils.get_member_state() + member_state.member_characters.add(character_member) + user_member.refresh_from_db() + user_guest.refresh_from_db() + group = Group.objects.create(name="dummy") + user_member.groups.add(group) + user_guest.groups.add(group) + group.authgroup.states.add(member_state) + self.client.force_login(self.superuser) + # when + response = self.client.post( + f"/admin/groupmanagement/group/{group.pk}/change/", + data={ + "name": f"{group.name}", + "authgroup-TOTAL_FORMS": "1", + "authgroup-INITIAL_FORMS": "1", + "authgroup-MIN_NUM_FORMS": "0", + "authgroup-MAX_NUM_FORMS": "1", + "authgroup-0-description": "", + "authgroup-0-states": f"{member_state.pk}", + "authgroup-0-internal": "on", + "authgroup-0-hidden": "on", + "authgroup-0-group": f"{group.pk}", + "authgroup-__prefix__-description": "", + "authgroup-__prefix__-internal": "on", + "authgroup-__prefix__-hidden": "on", + "authgroup-__prefix__-group": f"{group.pk}", + "_save": "Save" + } + ) + # then + self.assertEqual(response.status_code, 302) + self.assertEqual(response.url, "/admin/groupmanagement/group/") + self.assertIn(group, user_member.groups.all()) + self.assertNotIn(group, user_guest.groups.all()) + + class TestReservedGroupNameAdmin(TestCase): @classmethod def setUpClass(cls): diff --git a/allianceauth/groupmanagement/tests/test_models.py b/allianceauth/groupmanagement/tests/test_models.py index dcf9a1e2..b9f60bba 100644 --- a/allianceauth/groupmanagement/tests/test_models.py +++ b/allianceauth/groupmanagement/tests/test_models.py @@ -232,6 +232,38 @@ class TestAuthGroup(TestCase): expected = 'Superheros' self.assertEqual(str(group.authgroup), expected) + def test_should_remove_guests_from_group_when_restricted_to_members_only(self): + # given + user_member = AuthUtils.create_user("Bruce Wayne") + character_member = AuthUtils.add_main_character_2( + user_member, + name="Bruce Wayne", + character_id=1001, + corp_id=2001, + corp_name="Wayne Technologies", + ) + user_guest = AuthUtils.create_user("Lex Luthor") + AuthUtils.add_main_character_2( + user_guest, + name="Lex Luthor", + character_id=1011, + corp_id=2011, + corp_name="Luthor Corp", + ) + member_state = AuthUtils.get_member_state() + member_state.member_characters.add(character_member) + user_member.refresh_from_db() + user_guest.refresh_from_db() + group = Group.objects.create(name="dummy") + user_member.groups.add(group) + user_guest.groups.add(group) + group.authgroup.states.add(member_state) + # when + group.authgroup.remove_users_not_matching_states() + # then + self.assertIn(group, user_member.groups.all()) + self.assertNotIn(group, user_guest.groups.all()) + class TestAuthGroupRequestApprovers(TestCase): def setUp(self) -> None: