Fix error 500 on service page for Discord and add feature "group_to_role"

allianceauth/services/modules/discord/discord_client/app_settings.py

@@ -27,14 +27,14 @@ DISCORD_OAUTH_TOKEN_URL = clean_setting(
 )
 
 # How long the Discord guild names retrieved from the server are 
-# caches locally in milliseconds.
+# caches locally in seconds.
 DISCORD_GUILD_NAME_CACHE_MAX_AGE = clean_setting(
-    'DISCORD_GUILD_NAME_CACHE_MAX_AGE', 3600 * 1 * 1000
+    'DISCORD_GUILD_NAME_CACHE_MAX_AGE', 3600 * 24
 )
 
-# How long Discord roles retrieved from the server are caches locally in milliseconds.
+# How long Discord roles retrieved from the server are caches locally in seconds.
 DISCORD_ROLES_CACHE_MAX_AGE = clean_setting(
-    'DISCORD_ROLES_CACHE_MAX_AGE', 3600 * 1 * 1000
+    'DISCORD_ROLES_CACHE_MAX_AGE', 3600 * 1
 )
 
 # Turns off creation of new roles. In case the rate limit for creating roles is

allianceauth/services/modules/discord/discord_client/client.py

@@ -180,12 +180,19 @@ class DiscordClient:
         r = self._api_request(method='get', route=route)
         return r.json()
 
-    def guild_name(self, guild_id: int) -> str:
+    def guild_name(self, guild_id: int, use_cache: bool = True) -> str:
         """returns the name of this guild (cached) 
         or an empty string if something went wrong
+
+        Params:
+        - guild_id: ID of current guild
+        - use_cache: When set to False will force an API call to get the server name
         """
         key_name = self._guild_name_cache_key(guild_id)
-        guild_name = self._redis_decode(self._redis.get(key_name))
+        if use_cache:
+            guild_name = self._redis_decode(self._redis.get(key_name))
+        else:
+            guild_name = None
         if not guild_name:
             guild_infos = self.guild_infos(guild_id)
             if 'name' in guild_infos:
@@ -193,7 +200,7 @@ class DiscordClient:
                 self._redis.set(
                     name=key_name, 
                     value=guild_name, 
-                    px=DISCORD_GUILD_NAME_CACHE_MAX_AGE
+                    ex=DISCORD_GUILD_NAME_CACHE_MAX_AGE
                 )
             else:
                 guild_name = ''
@@ -230,7 +237,7 @@ class DiscordClient:
             self._redis.set(
                 name=cache_key, 
                 value=json.dumps(roles), 
-                px=DISCORD_ROLES_CACHE_MAX_AGE
+                ex=DISCORD_ROLES_CACHE_MAX_AGE
             )
         return roles
 
@@ -274,6 +281,11 @@ class DiscordClient:
         gen_key = cls._generate_hash(f'{guild_id}')
         return f'{cls._KEYPREFIX_GUILD_ROLES}__{gen_key}'
     
+    def match_role_from_name(self, guild_id: int, role_name: str) -> dict:
+        """returns Discord role matching the given name or an empty dict"""
+        guild_roles = DiscordRoles(self.guild_roles(guild_id))
+        return guild_roles.role_by_name(role_name)
+
     def match_or_create_roles_from_names(self, guild_id: int, role_names: list) -> list:
         """returns Discord roles matching the given names
         
@@ -281,6 +293,7 @@ class DiscordClient:
 
         Will try to match with existing roles names
         Non-existing roles will be created, then created flag will be True
+        
         Params:
         - guild_id: ID of guild
         - role_names: list of name strings each defining a role
@@ -311,6 +324,7 @@ class DiscordClient:
         
         Will try to match with existing roles names
         Non-existing roles will be created, then created flag will be True
+        
         Params:
         - guild_id: ID of guild
         - role_name: strings defining name of a role

allianceauth/services/modules/discord/discord_client/tests/test_client.py

@@ -280,6 +280,8 @@ class TestGuildGetName(TestCase):
         client = DiscordClient2(TEST_BOT_TOKEN, my_mock_redis)
         result = client.guild_name(TEST_GUILD_ID)
         self.assertEqual(result, guild_name)
+        self.assertTrue(my_mock_redis.get.called)
+        self.assertFalse(my_mock_redis.set.called)
 
     @patch(MODULE_PATH + '.DiscordClient.guild_infos')    
     def test_fetches_from_server_if_not_found_in_cache_and_stores_in_cache(
@@ -291,6 +293,20 @@ class TestGuildGetName(TestCase):
         client = DiscordClient2(TEST_BOT_TOKEN, my_mock_redis)
         result = client.guild_name(TEST_GUILD_ID)
         self.assertEqual(result, guild_name)
+        self.assertTrue(my_mock_redis.get.called)
+        self.assertTrue(my_mock_redis.set.called)
+
+    @patch(MODULE_PATH + '.DiscordClient.guild_infos')    
+    def test_fetches_from_server_if_asked_to_ignore_cache_and_stores_in_cache(
+        self, mock_guild_get_infos
+    ):
+        guild_name = 'Omega'
+        my_mock_redis = MagicMock(**{'get.return_value': False})
+        mock_guild_get_infos.return_value = {'id': TEST_GUILD_ID, 'name': guild_name}
+        client = DiscordClient2(TEST_BOT_TOKEN, my_mock_redis)
+        result = client.guild_name(TEST_GUILD_ID, use_cache=False)
+        self.assertFalse(my_mock_redis.get.called)
+        self.assertEqual(result, guild_name)
         self.assertTrue(my_mock_redis.set.called)
 
     @patch(MODULE_PATH + '.DiscordClient.guild_infos')    
@@ -302,6 +318,7 @@ class TestGuildGetName(TestCase):
         client = DiscordClient2(TEST_BOT_TOKEN, my_mock_redis)
         result = client.guild_name(TEST_GUILD_ID)
         self.assertEqual(result, '')
+        self.assertTrue(my_mock_redis.get.called)
         self.assertFalse(my_mock_redis.set.called)
 
 
@@ -844,9 +861,45 @@ class TestGuildMemberRemoveRole(TestCase):
         self.assertFalse(result)
 
 
+class TestMatchGuildRolesToName(TestCase):
+
+    def setUp(self):
+        self.url = f'{API_BASE_URL}guilds/{TEST_GUILD_ID}/roles'
+        
+    @requests_mock.Mocker()
+    def test_return_role_if_known(self, requests_mocker):
+        my_mock_redis = MagicMock(**{
+            'get.return_value': None,
+            'pttl.return_value': -1,
+        })
+        requests_mocker.get(     
+            url=self.url,
+            request_headers=DEFAULT_REQUEST_HEADERS,
+            json=ALL_ROLES
+        )
+        client = DiscordClient2(TEST_BOT_TOKEN, my_mock_redis)
+        result = client.match_role_from_name(TEST_GUILD_ID, "alpha")
+        self.assertDictEqual(result, ROLE_ALPHA)
+
+    @requests_mock.Mocker()
+    def test_return_empty_dict_if_not_known(self, requests_mocker):
+        my_mock_redis = MagicMock(**{
+            'get.return_value': None,
+            'pttl.return_value': -1,
+        })
+        requests_mocker.get(     
+            url=self.url,
+            request_headers=DEFAULT_REQUEST_HEADERS,
+            json=ALL_ROLES
+        )
+        client = DiscordClient2(TEST_BOT_TOKEN, my_mock_redis)
+        result = client.match_role_from_name(TEST_GUILD_ID, "unknown")
+        self.assertDictEqual(result, dict())
+
+
 @patch(MODULE_PATH + '.DiscordClient.create_guild_role')
 @patch(MODULE_PATH + '.DiscordClient.guild_roles')
-class TestMatchGuildRolesToName(TestCase):
+class TestMatchOrCreateGuildRolesToName(TestCase):
             
     def test_return_role_if_known(
         self, mock_guild_get_roles, mock_guild_create_role,
@@ -896,7 +949,7 @@ class TestMatchGuildRolesToName(TestCase):
 
 @patch(MODULE_PATH + '.DiscordClient.create_guild_role')
 @patch(MODULE_PATH + '.DiscordClient.guild_roles')
-class TestMatchGuildRolesToNames(TestCase):
+class TestMatchOrCreateGuildRolesToNames(TestCase):
 
     def test_return_roles_if_known(
         self, mock_guild_get_roles, mock_guild_create_role,

allianceauth/services/modules/discord/managers.py

@@ -4,7 +4,7 @@ from urllib.parse import urlencode
 from requests_oauthlib import OAuth2Session
 from requests.exceptions import HTTPError
 
-from django.contrib.auth.models import User
+from django.contrib.auth.models import User, Group
 from django.db import models
 from django.utils.timezone import now
 
@@ -19,7 +19,8 @@ from .app_settings import (
     DISCORD_GUILD_ID,    
     DISCORD_SYNC_NAMES
 )
-from .discord_client import DiscordClient, DiscordApiBackoff
+from .discord_client import DiscordClient
+from .discord_client.exceptions import DiscordClientException, DiscordApiBackoff
 from .discord_client.helpers import match_or_create_roles_from_names
 from .utils import LoggerAddTag
 
@@ -149,7 +150,7 @@ class DiscordUserManager(models.Manager):
         return self.filter(user=user).select_related('user').exists()
 
     @classmethod
-    def generate_bot_add_url(cls):
+    def generate_bot_add_url(cls) -> str:
         params = urlencode({
             'client_id': DISCORD_APP_ID,
             'scope': 'bot',
@@ -159,7 +160,7 @@ class DiscordUserManager(models.Manager):
         return f'{DiscordClient.OAUTH_BASE_URL}?{params}'        
 
     @classmethod
-    def generate_oauth_redirect_url(cls):
+    def generate_oauth_redirect_url(cls) -> str:
         oauth = OAuth2Session(
             DISCORD_APP_ID, redirect_uri=DISCORD_CALLBACK_URL, scope=cls.SCOPES
         )
@@ -178,18 +179,38 @@ class DiscordUserManager(models.Manager):
         return token['access_token']
     
     @classmethod
-    def server_name(cls):
+    def server_name(cls, use_cache: bool = True) -> str:
         """returns the name of the current Discord server 
         or an empty string if the name could not be retrieved
+
+        Params:
+        - use_cache: When set False will force an API call to get the server name
         """
         try:
-            server_name = cls._bot_client().guild_name(DISCORD_GUILD_ID)
+            server_name = cls._bot_client().guild_name(
-        except HTTPError:
+                guild_id=DISCORD_GUILD_ID, use_cache=use_cache
+            )
+        except (HTTPError, DiscordClientException):
+            server_name = ""
+        except Exception:
+            logger.warning(
+                "Unexpected error when trying to retrieve the server name from Discord", 
+                exc_info=True
+            )
             server_name = ""
 
         return server_name
 
+    @classmethod
+    def group_to_role(cls, group: Group) -> dict:
+        """returns the Discord role matching the given Django group by name
+        or an empty dict() if no matching role exist
+        """
+        return cls._bot_client().match_role_from_name(
+            guild_id=DISCORD_GUILD_ID, role_name=group.name
+        )
+
     @staticmethod
-    def _bot_client(is_rate_limited: bool = True):
+    def _bot_client(is_rate_limited: bool = True) -> DiscordClient:
         """returns a bot client for access to the Discord API"""
         return DiscordClient(DISCORD_BOT_TOKEN, is_rate_limited=is_rate_limited)

allianceauth/services/modules/discord/tasks.py

@@ -1,4 +1,5 @@
 import logging
+from typing import Any
 
 from celery import shared_task, chain
 from requests.exceptions import HTTPError
@@ -94,7 +95,7 @@ def _task_perform_user_action(self, user_pk: int, method: str, **kwargs) -> None
             raise self.retry(countdown=bo.retry_after_seconds)     
         
         except AttributeError:
-            raise ValueError(f'{method} not a valid method for DiscordUser: %r')
+            raise ValueError(f'{method} not a valid method for DiscordUser')
 
         except (HTTPError, ConnectionError):           
             logger.warning(
@@ -115,7 +116,7 @@ def _task_perform_user_action(self, user_pk: int, method: str, **kwargs) -> None
                 )
         except Exception:
             logger.error(
-                '%s for %s failed due to unexpected exception',
+                '%s for user %s failed due to unexpected exception',
                 method,
                 user,
                 exc_info=True
@@ -186,9 +187,58 @@ def _bulk_update_nicknames_for_users(discord_users_qs: QuerySet) -> None:
     chain(update_nicknames_chain).apply_async(priority=BULK_TASK_PRIORITY)
 
 
+def _task_perform_users_action(self, method: str, **kwargs) -> Any:   
+    """Perform an action that concerns a group of users or the whole server 
+    and that hits the API
+    """     
+    result = None
+    try:
+        result = getattr(DiscordUser.objects, method)(**kwargs)
+    
+    except AttributeError:
+        raise ValueError(f'{method} not a valid method for DiscordUser.objects')
+    
+    except DiscordApiBackoff as bo:
+        logger.info(
+            "API back off for %s due to %r, retrying in %s seconds",
+            method,
+            bo,
+            bo.retry_after_seconds
+        )
+        raise self.retry(countdown=bo.retry_after_seconds)     
+        
+    except (HTTPError, ConnectionError):           
+        logger.warning(
+            '%s failed, retrying in %d secs',             
+            method,
+            DISCORD_TASKS_RETRY_PAUSE,
+            exc_info=True
+        )
+        if self.request.retries < DISCORD_TASKS_MAX_RETRIES:
+            raise self.retry(countdown=DISCORD_TASKS_RETRY_PAUSE)
+        else:                                            
+            logger.error('%s failed after max retries', method, exc_info=True)
+    
+    except Exception:
+        logger.error('%s failed due to unexpected exception', method, exc_info=True)
+    
+    return result
+
+
+@shared_task(
+    bind=True, name='discord.update_servername', base=QueueOnce, max_retries=None
+)
+def update_servername(self) -> None:
+    """Updates the Discord server name"""
+    _task_perform_users_action(self, method="server_name", use_cache=False)
+
+
 @shared_task(name='discord.update_all_usernames')
 def update_all_usernames() -> None:
-    """Update all usernames for all known users with a Discord account."""
+    """Update all usernames for all known users with a Discord account. 
+    Also updates the server name
+    """
+    update_servername.delay()
     discord_users_qs = DiscordUser.objects.all()
     _bulk_update_usernames_for_users(discord_users_qs)
     

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

@@ -10,6 +10,7 @@ from ..discord_client.tests import (    # noqa
     ROLE_BRAVO, 
     ROLE_CHARLIE, 
     ROLE_MIKE, 
+    ALL_ROLES,
     create_user_info
 )   
 

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

@@ -41,7 +41,9 @@ from . import (
     create_user_info
 )
 from ..discord_client.app_settings import DISCORD_API_BASE_URL
+from ..discord_client.exceptions import DiscordApiBackoff
 from ..models import DiscordUser
+from .. import tasks
 
 logger = logging.getLogger('allianceauth')
 
@@ -444,25 +446,26 @@ class TestUserFeatures(WebTest):
             disconnect_signals=True
         )
         add_permissions_to_members()
-        
+
     @patch(MODULE_PATH + '.views.messages')    
     @patch(MODULE_PATH + '.managers.OAuth2Session')
     def test_user_activation_normal(
         self, requests_mocker, mock_OAuth2Session, mock_messages
     ): 
-        # user_get_current()
+        # setup
+        requests_mocker.get(
+            guild_infos_request.url, json={'id': TEST_GUILD_ID, 'name': 'Test Guild'}
+        )                
         requests_mocker.get(
             user_get_current_request.url, 
             json=create_user_info(
                 TEST_USER_ID, TEST_USER_NAME, TEST_USER_DISCRIMINATOR
             )
-        )        
+        )                
-        # guild_roles()        
         requests_mocker.get(
             guild_roles_request.url, 
             json=[ROLE_ALPHA, ROLE_BRAVO, ROLE_MIKE, ROLE_MEMBER]
-        )        
+        )                
-        # add_guild_member()
         requests_mocker.put(add_guild_member_request.url, status_code=201)
         
         authentication_code = 'auth_code'        
@@ -474,8 +477,12 @@ class TestUserFeatures(WebTest):
         # login
         self.app.set_user(self.member)
         
-        # click activate on the service page
+        # user opens services page
-        response = self.app.get(reverse('discord:activate'))
+        services_page = self.app.get(reverse('services:services'))
+        self.assertEqual(services_page.status_code, 200)
+
+        # user clicks Discord service activation link on page        
+        response = services_page.click(href=reverse('discord:activate'))
         
         # check we got a redirect to Discord OAuth        
         self.assertRedirects(
@@ -497,7 +504,10 @@ class TestUserFeatures(WebTest):
             requests_made.append(obj)
                 
         expected = [
-            user_get_current_request, guild_roles_request, add_guild_member_request
+            guild_infos_request, 
+            user_get_current_request, 
+            guild_roles_request, 
+            add_guild_member_request
         ]
         self.assertListEqual(requests_made, expected)
 
@@ -506,19 +516,21 @@ class TestUserFeatures(WebTest):
     def test_user_activation_failed(
         self, requests_mocker, mock_OAuth2Session, mock_messages
     ): 
-        # user_get_current()
+        # setup
+        requests_mocker.get(
+            guild_infos_request.url, json={'id': TEST_GUILD_ID, 'name': 'Test Guild'}
+        )
         requests_mocker.get(
             user_get_current_request.url, 
             json=create_user_info(
                 TEST_USER_ID, TEST_USER_NAME, TEST_USER_DISCRIMINATOR
             )
-        )        
+        )                
-        # guild_roles()        
         requests_mocker.get(
             guild_roles_request.url, 
             json=[ROLE_ALPHA, ROLE_BRAVO, ROLE_MIKE, ROLE_MEMBER]
         )        
-        # add_guild_member()
+        
         mock_exception = HTTPError('error')
         mock_exception.response = Mock()
         mock_exception.response.status_code = 503
@@ -532,9 +544,13 @@ class TestUserFeatures(WebTest):
         
         # login
         self.app.set_user(self.member)
+
+        # user opens services page
+        services_page = self.app.get(reverse('services:services'))
+        self.assertEqual(services_page.status_code, 200)
         
         # click activate on the service page
-        response = self.app.get(reverse('discord:activate'))
+        response = services_page.click(href=reverse('discord:activate'))
         
         # check we got a redirect to Discord OAuth        
         self.assertRedirects(
@@ -556,27 +572,31 @@ class TestUserFeatures(WebTest):
             requests_made.append(obj)
                 
         expected = [
-            user_get_current_request, guild_roles_request, add_guild_member_request
+            guild_infos_request,
+            user_get_current_request, 
+            guild_roles_request, 
+            add_guild_member_request
         ]
         self.assertListEqual(requests_made, expected)
 
     @patch(MODULE_PATH + '.views.messages')
     def test_user_deactivation_normal(self, requests_mocker, mock_messages): 
-        # guild_infos()
+        # setup
         requests_mocker.get(
-            guild_infos_request.url, json={'id': TEST_GUILD_ID, 'name': 'Test Guild'})
+            guild_infos_request.url, json={'id': TEST_GUILD_ID, 'name': 'Test Guild'}
-
+        )        
-        # remove_guild_member()
         requests_mocker.delete(remove_guild_member_request.url, status_code=204)
-        
-        # user needs have an account
         DiscordUser.objects.create(user=self.member, uid=TEST_USER_ID)
         
         # login
         self.app.set_user(self.member)
         
-        # click deactivate on the service page
+        # user opens services page
-        response = self.app.get(reverse('discord:deactivate'))
+        services_page = self.app.get(reverse('services:services'))
+        self.assertEqual(services_page.status_code, 200)
+
+        # click deactivate on the service page        
+        response = services_page.click(href=reverse('discord:deactivate'))
         
         # check we got a redirect to service page
         self.assertRedirects(response, expected_url=reverse('services:services'))
@@ -590,29 +610,31 @@ class TestUserFeatures(WebTest):
             obj = DiscordRequest(r.method, r.url)            
             requests_made.append(obj)
                 
-        expected = [remove_guild_member_request, guild_infos_request]
+        expected = [guild_infos_request, remove_guild_member_request]
         self.assertListEqual(requests_made, expected)
 
     @patch(MODULE_PATH + '.views.messages')
     def test_user_deactivation_fails(self, requests_mocker, mock_messages): 
-        # guild_infos()
+        # setup
         requests_mocker.get(
-            guild_infos_request.url, json={'id': TEST_GUILD_ID, 'name': 'Test Guild'})
+            guild_infos_request.url, json={'id': TEST_GUILD_ID, 'name': 'Test Guild'}
-
+        )        
-        # remove_guild_member()        
         mock_exception = HTTPError('error')
         mock_exception.response = Mock()
         mock_exception.response.status_code = 503
         requests_mocker.delete(remove_guild_member_request.url, exc=mock_exception)
-        
+                
-        # user needs have an account
         DiscordUser.objects.create(user=self.member, uid=TEST_USER_ID)
         
         # login
         self.app.set_user(self.member)
         
-        # click deactivate on the service page
+        # user opens services page
-        response = self.app.get(reverse('discord:deactivate'))
+        services_page = self.app.get(reverse('services:services'))
+        self.assertEqual(services_page.status_code, 200)
+
+        # click deactivate on the service page        
+        response = services_page.click(href=reverse('discord:deactivate'))
         
         # check we got a redirect to service page
         self.assertRedirects(response, expected_url=reverse('services:services'))
@@ -626,15 +648,13 @@ class TestUserFeatures(WebTest):
             obj = DiscordRequest(r.method, r.url)            
             requests_made.append(obj)
                 
-        expected = [remove_guild_member_request, guild_infos_request]
+        expected = [guild_infos_request, remove_guild_member_request]
         self.assertListEqual(requests_made, expected)
 
     @patch(MODULE_PATH + '.views.messages')
     def test_user_add_new_server(self, requests_mocker, mock_messages): 
-        # guild_infos()
+        # setup                
-        mock_exception = HTTPError('can not get guild info from Discord API')
+        mock_exception = HTTPError(Mock(**{"response.status_code": 400}))
-        mock_exception.response = Mock()
-        mock_exception.response.status_code = 440
         requests_mocker.get(guild_infos_request.url, exc=mock_exception)
         
         # login        
@@ -649,3 +669,39 @@ class TestUserFeatures(WebTest):
         # check we got can see the page and the "link server" button
         self.assertEqual(response.status_int, 200)
         self.assertIsNotNone(response.html.find(id='btnLinkDiscordServer'))
+    
+    def test_when_server_name_fails_user_can_still_see_service_page(
+        self, requests_mocker
+    ): 
+        # setup
+        requests_mocker.get(guild_infos_request.url, exc=DiscordApiBackoff(1000))
+                
+        # login
+        self.app.set_user(self.member)
+        
+        # user opens services page
+        services_page = self.app.get(reverse('services:services'))
+        self.assertEqual(services_page.status_code, 200)
+
+    @override_settings(CELERY_ALWAYS_EAGER=True)
+    def test_server_name_is_updated_by_task(
+        self, requests_mocker
+    ): 
+        # setup
+        requests_mocker.get(
+            guild_infos_request.url, json={'id': TEST_GUILD_ID, 'name': 'Test Guild'}
+        )
+        # run task to update usernames
+        tasks.update_all_usernames()
+
+        # login
+        self.app.set_user(self.member)
+        
+        # disable API call to make sure server name is not retrieved from API
+        mock_exception = HTTPError(Mock(**{"response.status_code": 400}))
+        requests_mocker.get(guild_infos_request.url, exc=mock_exception)
+        
+        # user opens services page
+        services_page = self.app.get(reverse('services:services'))
+        self.assertEqual(services_page.status_code, 200)
+        self.assertIn("Test Guild", services_page.text)

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

@@ -17,7 +17,7 @@ from . import (
     MODULE_PATH,
     ROLE_ALPHA,
     ROLE_BRAVO,
-    ROLE_CHARLIE
+    ROLE_CHARLIE, 
 )
 from ..discord_client.tests import create_matched_role
 from ..app_settings import (
@@ -364,6 +364,7 @@ class TestUserHasAccount(TestCase):
 
 
 @patch(MODULE_PATH + '.managers.DiscordClient', spec=DiscordClient)
+@patch(MODULE_PATH + '.managers.logger')
 class TestServerName(TestCase):
 
     @classmethod
@@ -371,16 +372,50 @@ class TestServerName(TestCase):
         super().setUpClass()
         cls.user = AuthUtils.create_user(TEST_USER_NAME)
     
-    def test_returns_name_when_api_returns_it(self, mock_DiscordClient):
+    def test_returns_name_when_api_returns_it(self, mock_logger, mock_DiscordClient):
         server_name = "El Dorado"
         mock_DiscordClient.return_value.guild_name.return_value = server_name
 
         self.assertEqual(DiscordUser.objects.server_name(), server_name)
+        self.assertFalse(mock_logger.warning.called)
 
-    def test_returns_empty_string_when_api_throws_http_error(self, mock_DiscordClient):
+    def test_returns_empty_string_when_api_throws_http_error(
+        self, mock_logger, mock_DiscordClient
+    ):
         mock_exception = HTTPError('Test exception')
         mock_exception.response = Mock(**{"status_code": 440})        
         mock_DiscordClient.return_value.guild_name.side_effect = mock_exception
 
         self.assertEqual(DiscordUser.objects.server_name(), "")
+        self.assertFalse(mock_logger.warning.called)
 
+    def test_returns_empty_string_when_api_throws_service_error(
+        self, mock_logger, mock_DiscordClient
+    ):
+        mock_DiscordClient.return_value.guild_name.side_effect = DiscordApiBackoff(1000)
+
+        self.assertEqual(DiscordUser.objects.server_name(), "")
+        self.assertFalse(mock_logger.warning.called)
+
+    def test_returns_empty_string_when_api_throws_unexpected_error(
+        self, mock_logger, mock_DiscordClient
+    ):
+        mock_DiscordClient.return_value.guild_name.side_effect = RuntimeError
+
+        self.assertEqual(DiscordUser.objects.server_name(), "")
+        self.assertTrue(mock_logger.warning.called)
+
+
+@patch(MODULE_PATH + '.managers.DiscordClient', spec=DiscordClient)
+class TestRoleForGroup(TestCase):    
+    def test_return_role_if_found(self, mock_DiscordClient):
+        mock_DiscordClient.return_value.match_role_from_name.return_value = ROLE_ALPHA
+
+        group = Group.objects.create(name='alpha')
+        self.assertEqual(DiscordUser.objects.group_to_role(group), ROLE_ALPHA)
+
+    def test_return_empty_dict_if_not_found(self, mock_DiscordClient):
+        mock_DiscordClient.return_value.match_role_from_name.return_value = dict()
+
+        group = Group.objects.create(name='unknown')
+        self.assertEqual(DiscordUser.objects.group_to_role(group), dict())

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

@@ -21,6 +21,7 @@ logger = set_logger_to_file(MODULE_PATH, __file__)
 
 
 @patch(MODULE_PATH + '.DiscordUser.update_groups')
+@patch(MODULE_PATH + ".logger")
 class TestUpdateGroups(TestCase):
     
     @classmethod
@@ -32,16 +33,18 @@ class TestUpdateGroups(TestCase):
         cls.group_1.user_set.add(cls.user)
         cls.group_2.user_set.add(cls.user)
         
-    def test_can_update_groups(self, mock_update_groups):
+    def test_can_update_groups(self, mock_logger, mock_update_groups):
         DiscordUser.objects.create(user=self.user, uid=TEST_USER_ID)
         tasks.update_groups(self.user.pk)
         self.assertTrue(mock_update_groups.called)
 
-    def test_no_action_if_user_has_no_discord_account(self, mock_update_groups):
+    def test_no_action_if_user_has_no_discord_account(
+        self, mock_logger, mock_update_groups
+    ):
         tasks.update_groups(self.user.pk)
         self.assertFalse(mock_update_groups.called)
     
-    def test_retries_on_api_backoff(self, mock_update_groups):
+    def test_retries_on_api_backoff(self, mock_logger, mock_update_groups):
         DiscordUser.objects.create(user=self.user, uid=TEST_USER_ID)
         mock_exception = DiscordApiBackoff(999)
         mock_update_groups.side_effect = mock_exception
@@ -49,7 +52,7 @@ class TestUpdateGroups(TestCase):
         with self.assertRaises(Retry):
             tasks.update_groups(self.user.pk)
 
-    def test_retry_on_http_error_except_404(self, mock_update_groups):
+    def test_retry_on_http_error_except_404(self, mock_logger, mock_update_groups):
         DiscordUser.objects.create(user=self.user, uid=TEST_USER_ID)
         mock_exception = HTTPError('error')
         mock_exception.response = MagicMock()
@@ -58,8 +61,12 @@ class TestUpdateGroups(TestCase):
 
         with self.assertRaises(Retry):
             tasks.update_groups(self.user.pk)
+
+        self.assertTrue(mock_logger.warning.called)
     
-    def test_retry_on_http_error_404_when_user_not_deleted(self, mock_update_groups):
+    def test_retry_on_http_error_404_when_user_not_deleted(
+        self, mock_logger, mock_update_groups
+    ):
         DiscordUser.objects.create(user=self.user, uid=TEST_USER_ID)
         mock_exception = HTTPError('error')
         mock_exception.response = MagicMock()
@@ -68,26 +75,31 @@ class TestUpdateGroups(TestCase):
 
         with self.assertRaises(Retry):
             tasks.update_groups(self.user.pk)
+
+        self.assertTrue(mock_logger.warning.called)
         
-    def test_retry_on_non_http_error(self, mock_update_groups):        
+    def test_retry_on_non_http_error(self, mock_logger, mock_update_groups):        
         DiscordUser.objects.create(user=self.user, uid=TEST_USER_ID)
         mock_update_groups.side_effect = ConnectionError
 
         with self.assertRaises(Retry):
             tasks.update_groups(self.user.pk)
+
+        self.assertTrue(mock_logger.warning.called)
         
     @patch(MODULE_PATH + '.DISCORD_TASKS_MAX_RETRIES', 3)
-    def test_log_error_if_retries_exhausted(self, mock_update_groups):
+    def test_log_error_if_retries_exhausted(self, mock_logger, mock_update_groups):
         DiscordUser.objects.create(user=self.user, uid=TEST_USER_ID)        
         mock_task = MagicMock(**{'request.retries': 3})
         mock_update_groups.side_effect = ConnectionError
         update_groups_inner = tasks.update_groups.__wrapped__.__func__
                 
         update_groups_inner(mock_task, self.user.pk)
+        self.assertTrue(mock_logger.error.called)
                 
     @patch(MODULE_PATH + '.delete_user.delay')
     def test_delete_user_if_user_is_no_longer_member_of_discord_server(
-        self, mock_delete_user, mock_update_groups
+        self, mock_delete_user, mock_logger, mock_update_groups
     ):
         mock_update_groups.return_value = None
         
@@ -222,6 +234,72 @@ class TestTaskPerformUserAction(TestCase):
         tasks._task_perform_user_action(mock_task, self.user.pk, 'update_groups')
 
 
+@patch(MODULE_PATH + '.DiscordUser.objects.server_name')
+@patch(MODULE_PATH + ".logger")
+class TestTaskUpdateServername(TestCase):
+
+    def test_normal(self, mock_logger, mock_server_name):
+        tasks.update_servername()
+        self.assertTrue(mock_server_name.called)
+        self.assertFalse(mock_logger.error.called)
+        _, kwargs = mock_server_name.call_args
+        self.assertFalse(kwargs["use_cache"])
+
+    def test_retries_on_api_backoff(self, mock_logger, mock_server_name):
+        mock_server_name.side_effect = DiscordApiBackoff(999)
+
+        with self.assertRaises(Retry):
+            tasks.update_servername()
+
+        self.assertFalse(mock_logger.error.called)
+
+    def test_retry_on_http_error(self, mock_logger, mock_server_name):        
+        mock_exception = HTTPError(MagicMock(**{"response.status_code": 500}))        
+        mock_server_name.side_effect = mock_exception
+
+        with self.assertRaises(Retry):
+            tasks.update_servername()
+
+        self.assertTrue(mock_logger.warning.called)
+
+    def test_retry_on_connection_error(self, mock_logger, mock_server_name):        
+        mock_server_name.side_effect = ConnectionError
+
+        with self.assertRaises(Retry):
+            tasks.update_servername()
+
+        self.assertTrue(mock_logger.warning.called)
+    
+    @patch(MODULE_PATH + '.DISCORD_TASKS_MAX_RETRIES', 3)
+    def test_log_error_if_retries_exhausted(self, mock_logger, mock_server_name):
+        mock_task = MagicMock(**{'request.retries': 3})
+        mock_server_name.side_effect = ConnectionError
+        update_groups_inner = tasks.update_servername.__wrapped__.__func__
+                
+        update_groups_inner(mock_task)
+        self.assertTrue(mock_logger.error.called)
+
+
+@patch(MODULE_PATH + '.DiscordUser.objects.server_name')
+class TestTaskPerformUsersAction(TestCase):
+
+    @classmethod
+    def setUpClass(cls):
+        super().setUpClass()        
+
+    def test_raise_value_error_on_unknown_method(self, mock_server_name):
+        mock_task = MagicMock(**{'request.retries': 0})
+        
+        with self.assertRaises(ValueError):
+            tasks._task_perform_users_action(mock_task, 'invalid_method')
+    
+    def test_catch_and_log_unexpected_exceptions(self, mock_server_name):
+        mock_server_name.side_effect = RuntimeError
+        mock_task = MagicMock(**{'request.retries': 0})
+        
+        tasks._task_perform_users_action(mock_task, 'server_name')
+
+
 @override_settings(CELERY_ALWAYS_EAGER=True)
 class TestBulkTasks(TestCase):
     
@@ -299,15 +377,19 @@ class TestBulkTasks(TestCase):
         
         self.assertSetEqual(set(current_pks), set(expected_pks))
 
-    @patch(MODULE_PATH + '.update_username.si')
+    @patch(MODULE_PATH + '.update_username')
-    def test_can_update_all_usernames(self, mock_update_username):
+    @patch(MODULE_PATH + '.update_servername')    
+    def test_can_update_all_usernames(
+        self, mock_update_servername, mock_update_username
+    ):
         du_1 = DiscordUser.objects.create(user=self.user_1, uid=123)
         du_2 = DiscordUser.objects.create(user=self.user_2, uid=456)
         du_3 = DiscordUser.objects.create(user=self.user_3, uid=789)
 
         tasks.update_all_usernames()
-        self.assertEqual(mock_update_username.call_count, 3)
+        self.assertTrue(mock_update_servername.delay.called)
-        current_pks = [args[0][0] for args in mock_update_username.call_args_list]
+        self.assertEqual(mock_update_username.si.call_count, 3)
+        current_pks = [args[0][0] for args in mock_update_username.si.call_args_list]
         expected_pks = [du_1.pk, du_2.pk, du_3.pk]
         self.assertSetEqual(set(current_pks), set(expected_pks))
 

docs/features/services/discord.md

@@ -131,8 +131,8 @@ Name                                Description
 `DISCORD_BOT_TOKEN`                 Generated bot token for the Discord Auth app                                                  `''`
 `DISCORD_CALLBACK_URL`              Oauth callback URL                                                                            `''`
 `DISCORD_GUILD_ID`                  Discord ID of your Discord server                                                             `''`
-`DISCORD_GUILD_NAME_CACHE_MAX_AGE`  How long the Discord server name is cached locally in milliseconds                            `3600000`
+`DISCORD_GUILD_NAME_CACHE_MAX_AGE`  How long the Discord server name is cached locally in seconds                                 `86400`
-`DISCORD_ROLES_CACHE_MAX_AGE`       How long roles retrieved from the Discord server are cached locally in milliseconds           `3600000`
+`DISCORD_ROLES_CACHE_MAX_AGE`       How long roles retrieved from the Discord server are cached locally in seconds                `3600`
 `DISCORD_SYNC_NAMES`                When set to True the nicknames of Discord users will be set to the user's main character name `False`
 `DISCORD_TASKS_RETRY_PAUSE`         Pause in seconds until next retry for tasks after an error occurred                           `60`
 `DISCORD_TASKS_MAX_RETRIES`         max retries of tasks after an error occurred                                                  `3`