Merge branch 'fix-discord-update-username' into 'master'

Fix discord update username

See merge request allianceauth/allianceauth!1442

allianceauth/services/modules/discord/models.py

@@ -141,28 +141,18 @@ class DiscordUser(models.Model):
         Returns:
         - True on success
         - None if user is no longer a member of the Discord server
-        - False on error or raises exception
         """
-        user_info = default_bot_client.guild_member(
+        member_info = default_bot_client.guild_member(
             guild_id=DISCORD_GUILD_ID, user_id=self.uid
         )
-        if user_info is None:
+        if not member_info:
-            success = None
+            logger.warning('%s: User not a guild member', self.user)
-        elif (
+            return None
-            user_info
+        self.username = member_info.user.username
-            and 'user' in user_info
+        self.discriminator = member_info.user.discriminator
-            and 'username' in user_info['user']
+        self.save()
-            and 'discriminator' in user_info['user']
+        logger.info('%s: Username has been updated', self.user)
-        ):
+        return True
-            self.username = user_info['user']['username']
-            self.discriminator = user_info['user']['discriminator']
-            self.save()
-            logger.info('Username for %s has been updated', self.user)
-            success = True
-        else:
-            logger.warning('Failed to update username for %s', self.user)
-            success = False
-        return success
 
     def delete_user(
         self,

allianceauth/services/modules/discord/tests/factories.py

@@ -0,0 +1,31 @@
+from django.utils.timezone import now
+
+from allianceauth.authentication.backends import StateBackend
+from allianceauth.tests.auth_utils import AuthUtils
+
+from ..discord_client.tests.factories import (
+    TEST_USER_DISCRIMINATOR,
+    TEST_USER_ID,
+    TEST_USER_NAME,
+)
+from ..models import DiscordUser
+
+
+def create_user(**kwargs):
+    params = {"username": TEST_USER_NAME}
+    params.update(kwargs)
+    username = StateBackend.iterate_username(params["username"])
+    user = AuthUtils.create_user(username)
+    return AuthUtils.add_permission_to_user_by_name("discord.access_discord", user)
+
+
+def create_discord_user(user=None, **kwargs):
+    params = {
+        "user": user or create_user(),
+        "uid": TEST_USER_ID,
+        "username": TEST_USER_NAME,
+        "discriminator": TEST_USER_DISCRIMINATOR,
+        "activated": now(),
+    }
+    params.update(kwargs)
+    return DiscordUser.objects.create(**params)

allianceauth/services/modules/discord/tests/test_integration.py

@@ -14,8 +14,8 @@ import requests_mock
 from requests.exceptions import HTTPError
 
 from django.contrib.auth.models import Group, User
-from django.shortcuts import reverse
 from django.test import TransactionTestCase, override_settings
+from django.urls import reverse
 from django_webtest import WebTest
 
 from allianceauth.authentication.models import State
@@ -42,6 +42,7 @@ from ..discord_client.tests.factories import (
 )
 from ..models import DiscordUser
 from . import MODULE_PATH, TEST_MAIN_ID, TEST_MAIN_NAME, add_permissions_to_members
+from .factories import create_discord_user, create_user
 
 logger = logging.getLogger('allianceauth')
 
@@ -349,6 +350,28 @@ class TestServiceFeatures(TransactionTestCase):
         self.assertTrue(DiscordUser.objects.user_has_account(self.user))
 
 
+@override_settings(CELERY_ALWAYS_EAGER=True, CELERY_EAGER_PROPAGATES_EXCEPTIONS=True)
+@patch(MODULE_PATH + '.managers.DISCORD_GUILD_ID', TEST_GUILD_ID)
+@patch(MODULE_PATH + '.models.DISCORD_GUILD_ID', TEST_GUILD_ID)
+@requests_mock.Mocker()
+class TestTasks(NoSocketsTestCase):
+    def test_should_update_username(self, requests_mocker):
+        # given
+        user = create_user()
+        discord_user = create_discord_user(user)
+        discord_user_obj = create_discord_user_object()
+        data = create_discord_guild_member_object(user=discord_user_obj)
+        requests_mocker.get(guild_member_request.url, json=data)
+        # when
+        tasks.update_username.delay(user.pk)
+        # then
+        discord_user.refresh_from_db()
+        self.assertEqual(discord_user.username, discord_user_obj["username"])
+        self.assertEqual(
+            discord_user.discriminator, discord_user_obj["discriminator"]
+        )
+
+
 @override_settings(CELERY_ALWAYS_EAGER=True, CELERY_EAGER_PROPAGATES_EXCEPTIONS=True)
 @patch(MODULE_PATH + '.managers.DISCORD_GUILD_ID', TEST_GUILD_ID)
 @patch(MODULE_PATH + '.models.DISCORD_GUILD_ID', TEST_GUILD_ID)

allianceauth/services/modules/discord/tests/test_models.py

@@ -6,10 +6,17 @@ from allianceauth.tests.auth_utils import AuthUtils
 from allianceauth.utils.testing import NoSocketsTestCase
 
 from ..discord_client import DiscordApiBackoff, RolesSet
-from ..discord_client.tests.factories import TEST_USER_ID, TEST_USER_NAME, create_role
+from ..discord_client.tests.factories import (
+    TEST_USER_ID,
+    TEST_USER_NAME,
+    create_guild_member,
+    create_role,
+)
+from ..discord_client.tests.factories import create_user as create_guild_user
 from ..models import DiscordUser
 from ..utils import set_logger_to_file
 from . import MODULE_PATH, TEST_MAIN_ID, TEST_MAIN_NAME
+from .factories import create_discord_user, create_user
 
 logger = set_logger_to_file(MODULE_PATH + '.models', __file__)
 
@@ -84,99 +91,41 @@ class TestUpdateNick(NoSocketsTestCase):
         self.assertTrue(mock_default_bot_client.modify_guild_member.called)
 
 
-@patch(MODULE_PATH + '.models.default_bot_client', spec=True)
+@patch(MODULE_PATH + '.models.default_bot_client.guild_member', spec=True)
 class TestUpdateUsername(NoSocketsTestCase):
 
     @classmethod
     def setUpClass(cls):
         super().setUpClass()
-        cls.user = AuthUtils.create_user(TEST_USER_NAME)
+        cls.user = create_user()
 
-    def setUp(self):
+    def test_can_update(self, mock_guild_member):
-        self.discord_user = DiscordUser.objects.create(
-            user=self.user,
-            uid=TEST_USER_ID,
-            username=TEST_MAIN_NAME,
-            discriminator='1234'
-        )
-
-    def test_can_update(self, mock_default_bot_client):
         # given
+        discord_user = create_discord_user(user=self.user)
         new_username = 'New name'
         new_discriminator = '9876'
-        user_info = {
+        guild_user = create_guild_user(
-            'user': {
+            username='New name', discriminator=new_discriminator
-                'id': str(TEST_USER_ID),
+        )
-                'username': new_username,
+        mock_guild_member.return_value = create_guild_member(user=guild_user)
-                'discriminator': new_discriminator,
-            }
-        }
-        mock_default_bot_client.guild_member.return_value = user_info
         # when
-        result = self.discord_user.update_username()
+        result = discord_user.update_username()
         # then
         self.assertTrue(result)
-        self.assertTrue(mock_default_bot_client.guild_member.called)
+        self.assertTrue(mock_guild_member.called)
-        self.discord_user.refresh_from_db()
+        discord_user.refresh_from_db()
-        self.assertEqual(self.discord_user.username, new_username)
+        self.assertEqual(discord_user.username, new_username)
-        self.assertEqual(self.discord_user.discriminator, new_discriminator)
+        self.assertEqual(discord_user.discriminator, new_discriminator)
 
-    def test_return_none_if_user_no_longer_a_member(self, mock_default_bot_client):
+    def test_return_none_if_user_no_longer_a_member(self, mock_guild_member):
         # given
-        mock_default_bot_client.guild_member.return_value = None
+        discord_user = create_discord_user(user=self.user)
+        mock_guild_member.return_value = None
         # when
-        result = self.discord_user.update_username()
+        result = discord_user.update_username()
         # then
         self.assertIsNone(result)
-        self.assertTrue(mock_default_bot_client.guild_member.called)
+        self.assertTrue(mock_guild_member.called)
-
-    def test_return_false_if_api_returns_false(self, mock_default_bot_client):
-        # given
-        mock_default_bot_client.guild_member.return_value = False
-        # when
-        result = self.discord_user.update_username()
-        # then
-        self.assertFalse(result)
-        self.assertTrue(mock_default_bot_client.guild_member.called)
-
-    def test_return_false_if_api_returns_corrput_data_1(self, mock_default_bot_client):
-        # given
-        mock_default_bot_client.guild_member.return_value = {'invalid': True}
-        # when
-        result = self.discord_user.update_username()
-        # then
-        self.assertFalse(result)
-        self.assertTrue(mock_default_bot_client.guild_member.called)
-
-    def test_return_false_if_api_returns_corrput_data_2(self, mock_default_bot_client):
-        # given
-        user_info = {
-            'user': {
-                'id': str(TEST_USER_ID),
-                'discriminator': '1234',
-            }
-        }
-        mock_default_bot_client.guild_member.return_value = user_info
-        # when
-        result = self.discord_user.update_username()
-        # then
-        self.assertFalse(result)
-        self.assertTrue(mock_default_bot_client.guild_member.called)
-
-    def test_return_false_if_api_returns_corrput_data_3(self, mock_default_bot_client):
-        # given
-        user_info = {
-            'user': {
-                'id': str(TEST_USER_ID),
-                'username': TEST_USER_NAME,
-            }
-        }
-        mock_default_bot_client.guild_member.return_value = user_info
-        # when
-        result = self.discord_user.update_username()
-        # then
-        self.assertFalse(result)
-        self.assertTrue(mock_default_bot_client.guild_member.called)
 
 
 @patch(MODULE_PATH + '.models.notify', spec=True)