Signals: Handle (and test) mapping updates/deletions

This also enforces unique values for both attributes on the RoleMapping model. Supporting configurations where this isn't the case would introduce quite a lot of added complexity.

3 files changed, 125 insertions, 14 deletions

diff --git a/pydis_site/apps/home/signals.py b/pydis_site/apps/home/signals.py
index c8bf5376..65b24ddf 100644
--- a/pydis_site/apps/home/signals.py
+++ b/pydis_site/apps/home/signals.py
@@ -1,4 +1,4 @@
-from typing import List, Type
+from typing import List, Optional, Type
 
 from allauth.account.signals import user_logged_in
 from allauth.socialaccount.models import SocialAccount, SocialLogin
@@ -8,7 +8,7 @@ from allauth.socialaccount.signals import (
     pre_social_login, social_account_added, social_account_removed,
     social_account_updated)
 from django.contrib.auth.models import Group, User as DjangoUser
-from django.db.models.signals import post_save
+from django.db.models.signals import post_save, pre_delete, pre_save
 
 from pydis_site.apps.api.models import User as DiscordUser
 from pydis_site.apps.staff.models import RoleMapping
@@ -35,7 +35,10 @@ class SignalListener:
     """
 
     def __init__(self):
-        post_save.connect(self.model_updated, sender=DiscordUser)
+        post_save.connect(self.user_model_updated, sender=DiscordUser)
+
+        pre_delete.connect(self.mapping_model_deleted, sender=RoleMapping)
+        pre_save.connect(self.mapping_model_updated, sender=RoleMapping)
 
         pre_social_login.connect(self.social_account_updated)
         social_account_added.connect(self.social_account_updated)
@@ -94,7 +97,41 @@ class SignalListener:
 
         self._apply_groups(user, account, deletion=True)
 
-    def model_updated(self, sender: Type[DiscordUser], **kwargs) -> None:
+    def mapping_model_deleted(self, sender: Type[RoleMapping], **kwargs) -> None:
+        """Handle signals related to the deletion of Role Mapping model entries."""
+        instance: RoleMapping = kwargs["instance"]
+
+        for user in instance.group.user_set.all():
+            user.groups.remove(instance.group)
+
+    def mapping_model_updated(self, sender: Type[RoleMapping], **kwargs) -> None:
+        """Handle signals related to the updating of Role Mapping model entries."""
+        instance: RoleMapping = kwargs["instance"]
+        raw: bool = kwargs["raw"]
+
+        if raw:
+            # Fixtures are being loaded, so don't touch anything
+            return
+
+        old_instance: Optional[RoleMapping] = None
+
+        if instance.id is not None:
+            # We don't try to catch DoesNotExist here because we can't test for it,
+            # it should never happen (unless we have a bad DB failure)
+
+            old_instance = RoleMapping.objects.get(id=instance.id)
+
+        if old_instance:
+            self.mapping_model_deleted(RoleMapping, instance=old_instance)
+
+        accounts = SocialAccount.objects.filter(
+            uid__in=(u.id for u in instance.role.user_set.all())
+        )
+
+        for account in accounts:
+            account.user.groups.add(instance.group)
+
+    def user_model_updated(self, sender: Type[DiscordUser], **kwargs) -> None:
         """Handle signals related to the updating of Discord User model entries."""
         instance: DiscordUser = kwargs["instance"]
         raw: bool = kwargs["raw"]
diff --git a/pydis_site/apps/home/tests/test_signal_listener.py b/pydis_site/apps/home/tests/test_signal_listener.py
index 2eaaa945..ee78999f 100644
--- a/pydis_site/apps/home/tests/test_signal_listener.py
+++ b/pydis_site/apps/home/tests/test_signal_listener.py
@@ -9,7 +9,7 @@ from allauth.socialaccount.signals import (
     pre_social_login, social_account_added, social_account_removed,
     social_account_updated)
 from django.contrib.auth.models import Group, User as DjangoUser
-from django.db.models.signals import post_save
+from django.db.models.signals import post_save, pre_save
 from django.test import TestCase
 
 from pydis_site.apps.api.models import Role, User as DiscordUser
@@ -37,21 +37,35 @@ class SignalListenerTests(TestCase):
             position=0
         )
 
-        cls.admin_group = Group.objects.create(name="admin")
-
-        cls.role_mapping = RoleMapping.objects.create(
-            role=cls.admin_role,
-            group=cls.admin_group
+        cls.moderator_role = Role.objects.create(
+            id=1,
+            name="moderator",
+            colour=0,
+            permissions=0,
+            position=1
         )
 
         cls.unmapped_role = Role.objects.create(
-            id=1,
+            id=2,
             name="unmapped",
             colour=0,
             permissions=0,
             position=1
         )
 
+        cls.admin_group = Group.objects.create(name="admin")
+        cls.moderator_group = Group.objects.create(name="moderator")
+
+        cls.admin_mapping = RoleMapping.objects.create(
+            role=cls.admin_role,
+            group=cls.admin_group
+        )
+
+        cls.moderator_mapping = RoleMapping.objects.create(
+            role=cls.moderator_role,
+            group=cls.moderator_group
+        )
+
         cls.discord_user = DiscordUser.objects.create(
             id=0,
             name="user",
@@ -87,6 +101,16 @@ class SignalListenerTests(TestCase):
         cls.discord_admin.roles.set([cls.admin_role])
         cls.discord_admin.save()
 
+        cls.discord_moderator = DiscordUser.objects.create(
+            id=4,
+            name="admin",
+            discriminator=0,
+            avatar_hash=None
+        )
+
+        cls.discord_moderator.roles.set([cls.moderator_role])
+        cls.discord_moderator.save()
+
         cls.django_user_discordless = DjangoUser.objects.create(username="no-discord")
         cls.django_user_never_joined = DjangoUser.objects.create(username="never-joined")
 
@@ -131,6 +155,18 @@ class SignalListenerTests(TestCase):
             uid=cls.discord_admin.id
         )
 
+        cls.django_moderator = DjangoUser.objects.create(
+            username="moderator",
+            is_staff=True,
+            is_superuser=False
+        )
+
+        cls.social_moderator = SocialAccount.objects.create(
+            user=cls.django_moderator,
+            provider=DiscordProvider.id,
+            uid=cls.discord_moderator.id
+        )
+
     def test_model_save(self):
         """Test signal handling for when Discord user model objects are saved to DB."""
         mock_obj = mock.Mock()
@@ -315,6 +351,42 @@ class SignalListenerTests(TestCase):
     def test_role_mapping_str(self):
         """Test that role mappings stringify correctly."""
         self.assertEqual(
-            str(self.role_mapping),
+            str(self.admin_mapping),
             f"@{self.admin_role.name} -> {self.admin_group.name}"
         )
+
+    def test_role_mapping_changes(self):
+        """Test that role mapping listeners work when changes are made."""
+        # Set up (just for this test)
+        self.django_moderator.groups.add(self.moderator_group)
+        self.django_admin.groups.add(self.admin_group)
+
+        self.assertEqual(self.django_moderator.groups.all().count(), 1)
+        self.assertEqual(self.django_admin.groups.all().count(), 1)
+
+        # Test mapping deletion
+        self.admin_mapping.delete()
+
+        self.assertEqual(self.django_admin.groups.all().count(), 0)
+
+        # Test mapping update
+        self.moderator_mapping.group = self.admin_group
+        self.moderator_mapping.save()
+
+        self.assertEqual(self.django_moderator.groups.all().count(), 1)
+        self.assertTrue(self.admin_group in self.django_moderator.groups.all())
+
+        # Test mapping creation
+        new_mapping = RoleMapping.objects.create(
+            role=self.admin_role,
+            group=self.moderator_group
+        )
+
+        self.assertEqual(self.django_admin.groups.all().count(), 1)
+        self.assertTrue(self.moderator_group in self.django_admin.groups.all())
+
+        # Test that nothing happens when fixtures are loaded
+        pre_save.send(RoleMapping, instance=new_mapping, raw=True)
+
+        self.assertEqual(self.django_admin.groups.all().count(), 1)
+        self.assertTrue(self.moderator_group in self.django_admin.groups.all())
diff --git a/pydis_site/apps/staff/models/role_mapping.py b/pydis_site/apps/staff/models/role_mapping.py
index 5c728283..10c09cf1 100644
--- a/pydis_site/apps/staff/models/role_mapping.py
+++ b/pydis_site/apps/staff/models/role_mapping.py
@@ -10,13 +10,15 @@ class RoleMapping(models.Model):
     role = models.OneToOneField(
         Role,
         on_delete=models.CASCADE,
-        help_text="The Discord role to use for this mapping."
+        help_text="The Discord role to use for this mapping.",
+        unique=True,  # Unique in order to simplify group assignment logic
     )
 
     group = models.OneToOneField(
         Group,
         on_delete=models.CASCADE,
-        help_text="The Django permissions group to use for this mapping."
+        help_text="The Django permissions group to use for this mapping.",
+        unique=True,  # Unique in order to simplify group assignment logic
     )
 
     def __str__(self):