Merge pull request #397 from python-discord/django-role-api-changes

Django role api changes

3 files changed, 143 insertions, 77 deletions

diff --git a/bot/cogs/sync/cog.py b/bot/cogs/sync/cog.py
index 222c1668b..79177b69e 100644
--- a/bot/cogs/sync/cog.py
+++ b/bot/cogs/sync/cog.py
@@ -3,7 +3,7 @@ from typing import Callable, Iterable
 
 from discord import Guild, Member, Role
 from discord.ext import commands
-from discord.ext.commands import Bot
+from discord.ext.commands import Bot, Context
 
 from bot import constants
 from bot.api import ResponseCodeError
@@ -26,58 +26,71 @@ class Sync:
         syncers.sync_users
     )
 
-    def __init__(self, bot):
+    def __init__(self, bot: Bot) -> None:
         self.bot = bot
 
-    async def on_ready(self):
+    async def on_ready(self) -> None:
+        """Syncs the roles/users of the guild with the database."""
         guild = self.bot.get_guild(self.SYNC_SERVER_ID)
         if guild is not None:
             for syncer in self.ON_READY_SYNCERS:
                 syncer_name = syncer.__name__[5:]  # drop off `sync_`
                 log.info("Starting `%s` syncer.", syncer_name)
-                total_created, total_updated = await syncer(self.bot, guild)
-                log.info(
-                    "`%s` syncer finished, created `%d`, updated `%d`.",
-                    syncer_name, total_created, total_updated
-                )
-
-    async def on_guild_role_create(self, role: Role):
+                total_created, total_updated, total_deleted = await syncer(self.bot, guild)
+                if total_deleted is None:
+                    log.info(
+                        f"`{syncer_name}` syncer finished, created `{total_created}`, updated `{total_updated}`."
+                    )
+                else:
+                    log.info(
+                        f"`{syncer_name}` syncer finished, created `{total_created}`, updated `{total_updated}`, "
+                        f"deleted `{total_deleted}`."
+                    )
+
+    async def on_guild_role_create(self, role: Role) -> None:
+        """Adds newly create role to the database table over the API."""
         await self.bot.api_client.post(
             'bot/roles',
             json={
                 'colour': role.colour.value,
                 'id': role.id,
                 'name': role.name,
-                'permissions': role.permissions.value
+                'permissions': role.permissions.value,
+                'position': role.position,
             }
         )
 
-    async def on_guild_role_delete(self, role: Role):
-        log.warning(
-            (
-                "Attempted to delete role `%s` (`%d`), but role deletion "
-                "is currently not implementeed."
-            ),
-            role.name, role.id
-        )
+    async def on_guild_role_delete(self, role: Role) -> None:
+        """Deletes role from the database when it's deleted from the guild."""
+        await self.bot.api_client.delete(f'bot/roles/{role.id}')
 
-    async def on_guild_role_update(self, before: Role, after: Role):
+    async def on_guild_role_update(self, before: Role, after: Role) -> None:
+        """Syncs role with the database if any of the stored attributes were updated."""
         if (
                 before.name != after.name
                 or before.colour != after.colour
                 or before.permissions != after.permissions
+                or before.position != after.position
         ):
             await self.bot.api_client.put(
-                'bot/roles/' + str(after.id),
+                f'bot/roles/{after.id}',
                 json={
                     'colour': after.colour.value,
                     'id': after.id,
                     'name': after.name,
-                    'permissions': after.permissions.value
+                    'permissions': after.permissions.value,
+                    'position': after.position,
                 }
             )
 
-    async def on_member_join(self, member: Member):
+    async def on_member_join(self, member: Member) -> None:
+        """
+        Adds a new user or updates existing user to the database when a member joins the guild.
+
+        If the joining member is a user that is already known to the database (i.e., a user that
+        previously left), it will update the user's information. If the user is not yet known by
+        the database, the user is added.
+        """
         packed = {
             'avatar_hash': member.avatar,
             'discriminator': int(member.discriminator),
@@ -92,7 +105,7 @@ class Sync:
         try:
             # First try an update of the user to set the `in_guild` field and other
             # fields that may have changed since the last time we've seen them.
-            await self.bot.api_client.put('bot/users/' + str(member.id), json=packed)
+            await self.bot.api_client.put(f'bot/users/{member.id}', json=packed)
 
         except ResponseCodeError as e:
             # If we didn't get 404, something else broke - propagate it up.
@@ -105,20 +118,22 @@ class Sync:
             # If we got `404`, the user is new. Create them.
             await self.bot.api_client.post('bot/users', json=packed)
 
-    async def on_member_leave(self, member: Member):
+    async def on_member_leave(self, member: Member) -> None:
+        """Updates the user information when a member leaves the guild."""
         await self.bot.api_client.put(
-            'bot/users/' + str(member.id),
+            f'bot/users/{member.id}',
             json={
                 'avatar_hash': member.avatar,
                 'discriminator': int(member.discriminator),
                 'id': member.id,
-                'in_guild': True,
+                'in_guild': False,
                 'name': member.name,
                 'roles': sorted(role.id for role in member.roles)
             }
         )
 
-    async def on_member_update(self, before: Member, after: Member):
+    async def on_member_update(self, before: Member, after: Member) -> None:
+        """Updates the user information if any of relevant attributes have changed."""
         if (
                 before.name != after.name
                 or before.avatar != after.avatar
@@ -148,30 +163,30 @@ class Sync:
 
     @commands.group(name='sync')
     @commands.has_permissions(administrator=True)
-    async def sync_group(self, ctx):
+    async def sync_group(self, ctx: Context) -> None:
         """Run synchronizations between the bot and site manually."""
 
     @sync_group.command(name='roles')
     @commands.has_permissions(administrator=True)
-    async def sync_roles_command(self, ctx):
+    async def sync_roles_command(self, ctx: Context) -> None:
         """Manually synchronize the guild's roles with the roles on the site."""
 
         initial_response = await ctx.send("📊 Synchronizing roles.")
-        total_created, total_updated = await syncers.sync_roles(self.bot, ctx.guild)
+        total_created, total_updated, total_deleted = await syncers.sync_roles(self.bot, ctx.guild)
         await initial_response.edit(
             content=(
                 f"👌 Role synchronization complete, created **{total_created}** "
-                f"and updated **{total_created}** roles."
+                f", updated **{total_created}** roles, and deleted **{total_deleted}** roles."
             )
         )
 
     @sync_group.command(name='users')
     @commands.has_permissions(administrator=True)
-    async def sync_users_command(self, ctx):
+    async def sync_users_command(self, ctx: Context) -> None:
         """Manually synchronize the guild's users with the users on the site."""
 
         initial_response = await ctx.send("📊 Synchronizing users.")
-        total_created, total_updated = await syncers.sync_users(self.bot, ctx.guild)
+        total_created, total_updated, total_deleted = await syncers.sync_users(self.bot, ctx.guild)
         await initial_response.edit(
             content=(
                 f"👌 User synchronization complete, created **{total_created}** "
diff --git a/bot/cogs/sync/syncers.py b/bot/cogs/sync/syncers.py
index 3037d2e31..414c24adb 100644
--- a/bot/cogs/sync/syncers.py
+++ b/bot/cogs/sync/syncers.py
@@ -6,13 +6,13 @@ from discord.ext.commands import Bot
 
 # These objects are declared as namedtuples because tuples are hashable,
 # something that we make use of when diffing site roles against guild roles.
-Role = namedtuple('Role', ('id', 'name', 'colour', 'permissions'))
+Role = namedtuple('Role', ('id', 'name', 'colour', 'permissions', 'position'))
 User = namedtuple('User', ('id', 'name', 'discriminator', 'avatar_hash', 'roles', 'in_guild'))
 
 
 def get_roles_for_sync(
         guild_roles: Set[Role], api_roles: Set[Role]
-) -> Tuple[Set[Role], Set[Role]]:
+) -> Tuple[Set[Role], Set[Role], Set[Role]]:
     """
     Determine which roles should be created or updated on the site.
 
@@ -24,27 +24,31 @@ def get_roles_for_sync(
             Roles that were retrieved from the API at startup.
 
     Returns:
-        Tuple[Set[Role], Set[Role]]:
-            A tuple with two elements. The first element represents
+        Tuple[Set[Role], Set[Role]. Set[Role]]:
+            A tuple with three elements. The first element represents
             roles to be created on the site, meaning that they were
             present on the cached guild but not on the API. The second
             element represents roles to be updated, meaning they were
             present on both the cached guild and the API but non-ID
-            fields have changed inbetween.
+            fields have changed inbetween. The third represents roles
+            to be deleted on the site, meaning the roles are present on
+            the API but not in the cached guild.
     """
 
     guild_role_ids = {role.id for role in guild_roles}
     api_role_ids = {role.id for role in api_roles}
     new_role_ids = guild_role_ids - api_role_ids
+    deleted_role_ids = api_role_ids - guild_role_ids
 
     # New roles are those which are on the cached guild but not on the
     # API guild, going by the role ID. We need to send them in for creation.
     roles_to_create = {role for role in guild_roles if role.id in new_role_ids}
     roles_to_update = guild_roles - api_roles - roles_to_create
-    return roles_to_create, roles_to_update
+    roles_to_delete = {role for role in api_roles if role.id in deleted_role_ids}
+    return roles_to_create, roles_to_update, roles_to_delete
 
 
-async def sync_roles(bot: Bot, guild: Guild):
+async def sync_roles(bot: Bot, guild: Guild) -> Tuple[int, int, int]:
     """
     Synchronize roles found on the given `guild` with the ones on the API.
 
@@ -57,9 +61,10 @@ async def sync_roles(bot: Bot, guild: Guild):
             to synchronize roles with.
 
     Returns:
-        Tuple[int, int]:
-            A tuple with two integers representing how many roles were created
-            (element `0`) and how many roles were updated (element `1`).
+        Tuple[int, int, int]:
+            A tuple with three integers representing how many roles were created
+            (element `0`) , how many roles were updated (element `1`), and how many
+            roles were deleted (element `2`) on the API.
     """
 
     roles = await bot.api_client.get('bot/roles')
@@ -71,11 +76,12 @@ async def sync_roles(bot: Bot, guild: Guild):
     guild_roles = {
         Role(
             id=role.id, name=role.name,
-            colour=role.colour.value, permissions=role.permissions.value
+            colour=role.colour.value, permissions=role.permissions.value,
+            position=role.position,
         )
         for role in guild.roles
     }
-    roles_to_create, roles_to_update = get_roles_for_sync(guild_roles, api_roles)
+    roles_to_create, roles_to_update, roles_to_delete = get_roles_for_sync(guild_roles, api_roles)
 
     for role in roles_to_create:
         await bot.api_client.post(
@@ -84,22 +90,27 @@ async def sync_roles(bot: Bot, guild: Guild):
                 'id': role.id,
                 'name': role.name,
                 'colour': role.colour,
-                'permissions': role.permissions
+                'permissions': role.permissions,
+                'position': role.position,
             }
         )
 
     for role in roles_to_update:
         await bot.api_client.put(
-            'bot/roles/' + str(role.id),
+            f'bot/roles/{role.id}',
             json={
                 'id': role.id,
                 'name': role.name,
                 'colour': role.colour,
-                'permissions': role.permissions
+                'permissions': role.permissions,
+                'position': role.position,
             }
         )
 
-    return (len(roles_to_create), len(roles_to_update))
+    for role in roles_to_delete:
+        await bot.api_client.delete(f'bot/roles/{role.id}')
+
+    return len(roles_to_create), len(roles_to_update), len(roles_to_delete)
 
 
 def get_users_for_sync(
@@ -156,7 +167,7 @@ def get_users_for_sync(
     return users_to_create, users_to_update
 
 
-async def sync_users(bot: Bot, guild: Guild):
+async def sync_users(bot: Bot, guild: Guild) -> Tuple[int, int, None]:
     """
     Synchronize users found on the given
     `guild` with the ones on the API.
@@ -170,9 +181,10 @@ async def sync_users(bot: Bot, guild: Guild):
             to synchronize roles with.
 
     Returns:
-        Tuple[int, int]:
-            A tuple with two integers representing how many users were created
-            (element `0`) and how many users were updated (element `1`).
+        Tuple[int, int, None]:
+            A tuple with two integers, representing how many users were created
+            (element `0`) and how many users were updated (element `1`), and `None`
+            to indicate that a user sync never deletes entries from the API.
     """
 
     current_users = await bot.api_client.get('bot/users')
@@ -213,7 +225,7 @@ async def sync_users(bot: Bot, guild: Guild):
 
     for user in users_to_update:
         await bot.api_client.put(
-            'bot/users/' + str(user.id),
+            f'bot/users/{user.id}',
             json={
                 'avatar_hash': user.avatar_hash,
                 'discriminator': user.discriminator,
@@ -224,4 +236,4 @@ async def sync_users(bot: Bot, guild: Guild):
             }
         )
 
-    return (len(users_to_create), len(users_to_update))
+    return len(users_to_create), len(users_to_update), None
diff --git a/tests/cogs/sync/test_roles.py b/tests/cogs/sync/test_roles.py
index 18682f39f..c561ba447 100644
--- a/tests/cogs/sync/test_roles.py
+++ b/tests/cogs/sync/test_roles.py
@@ -2,63 +2,102 @@ from bot.cogs.sync.syncers import Role, get_roles_for_sync
 
 
 def test_get_roles_for_sync_empty_return_for_equal_roles():
-    api_roles = {Role(id=41, name='name', colour=33, permissions=0x8)}
-    guild_roles = {Role(id=41, name='name', colour=33, permissions=0x8)}
+    api_roles = {Role(id=41, name='name', colour=33, permissions=0x8, position=1)}
+    guild_roles = {Role(id=41, name='name', colour=33, permissions=0x8, position=1)}
 
-    assert get_roles_for_sync(guild_roles, api_roles) == (set(), set())
+    assert get_roles_for_sync(guild_roles, api_roles) == (set(), set(), set())
 
 
 def test_get_roles_for_sync_returns_roles_to_update_with_non_id_diff():
-    api_roles = {Role(id=41, name='old name', colour=35, permissions=0x8)}
-    guild_roles = {Role(id=41, name='new name', colour=33, permissions=0x8)}
+    api_roles = {Role(id=41, name='old name', colour=35, permissions=0x8, position=1)}
+    guild_roles = {Role(id=41, name='new name', colour=33, permissions=0x8, position=2)}
 
     assert get_roles_for_sync(guild_roles, api_roles) == (
         set(),
-        guild_roles
+        guild_roles,
+        set(),
     )
 
 
 def test_get_roles_only_returns_roles_that_require_update():
     api_roles = {
-        Role(id=41, name='old name', colour=33, permissions=0x8),
-        Role(id=53, name='other role', colour=55, permissions=0)
+        Role(id=41, name='old name', colour=33, permissions=0x8, position=1),
+        Role(id=53, name='other role', colour=55, permissions=0, position=3)
     }
     guild_roles = {
-        Role(id=41, name='new name', colour=35, permissions=0x8),
-        Role(id=53, name='other role', colour=55, permissions=0)
+        Role(id=41, name='new name', colour=35, permissions=0x8, position=2),
+        Role(id=53, name='other role', colour=55, permissions=0, position=3)
     }
 
     assert get_roles_for_sync(guild_roles, api_roles) == (
         set(),
-        {Role(id=41, name='new name', colour=35, permissions=0x8)},
+        {Role(id=41, name='new name', colour=35, permissions=0x8, position=2)},
+        set(),
     )
 
 
 def test_get_roles_returns_new_roles_in_first_tuple_element():
     api_roles = {
-        Role(id=41, name='name', colour=35, permissions=0x8),
+        Role(id=41, name='name', colour=35, permissions=0x8, position=1),
     }
     guild_roles = {
-        Role(id=41, name='name', colour=35, permissions=0x8),
-        Role(id=53, name='other role', colour=55, permissions=0)
+        Role(id=41, name='name', colour=35, permissions=0x8, position=1),
+        Role(id=53, name='other role', colour=55, permissions=0, position=2)
     }
 
     assert get_roles_for_sync(guild_roles, api_roles) == (
-        {Role(id=53, name='other role', colour=55, permissions=0)},
-        set()
+        {Role(id=53, name='other role', colour=55, permissions=0, position=2)},
+        set(),
+        set(),
     )
 
 
 def test_get_roles_returns_roles_to_update_and_new_roles():
     api_roles = {
-        Role(id=41, name='old name', colour=35, permissions=0x8),
+        Role(id=41, name='old name', colour=35, permissions=0x8, position=1),
+    }
+    guild_roles = {
+        Role(id=41, name='new name', colour=40, permissions=0x16, position=2),
+        Role(id=53, name='other role', colour=55, permissions=0, position=3)
+    }
+
+    assert get_roles_for_sync(guild_roles, api_roles) == (
+        {Role(id=53, name='other role', colour=55, permissions=0, position=3)},
+        {Role(id=41, name='new name', colour=40, permissions=0x16, position=2)},
+        set(),
+    )
+
+
+def test_get_roles_returns_roles_to_delete():
+    api_roles = {
+        Role(id=41, name='name', colour=35, permissions=0x8, position=1),
+        Role(id=61, name='to delete', colour=99, permissions=0x9, position=2),
+    }
+    guild_roles = {
+        Role(id=41, name='name', colour=35, permissions=0x8, position=1),
+    }
+
+    assert get_roles_for_sync(guild_roles, api_roles) == (
+        set(),
+        set(),
+        {Role(id=61, name='to delete', colour=99, permissions=0x9, position=2)},
+    )
+
+
+def test_get_roles_returns_roles_to_delete_update_and_new_roles():
+    api_roles = {
+        Role(id=41, name='not changed', colour=35, permissions=0x8, position=1),
+        Role(id=61, name='to delete', colour=99, permissions=0x9, position=2),
+        Role(id=71, name='to update', colour=99, permissions=0x9, position=3),
     }
     guild_roles = {
-        Role(id=41, name='new name', colour=40, permissions=0x16),
-        Role(id=53, name='other role', colour=55, permissions=0)
+        Role(id=41, name='not changed', colour=35, permissions=0x8, position=1),
+        Role(id=81, name='to create', colour=99, permissions=0x9, position=4),
+        Role(id=71, name='updated', colour=101, permissions=0x5, position=3),
     }
 
     assert get_roles_for_sync(guild_roles, api_roles) == (
-        {Role(id=53, name='other role', colour=55, permissions=0)},
-        {Role(id=41, name='new name', colour=40, permissions=0x16)}
+        {Role(id=81, name='to create', colour=99, permissions=0x9, position=4)},
+        {Role(id=71, name='updated', colour=101, permissions=0x5, position=3)},
+        {Role(id=61, name='to delete', colour=99, permissions=0x9, position=2)},
     )