From fe36e57d72de7abcbb038e3e1f7f72876b9c80f5 Mon Sep 17 00:00:00 2001 From: Erik Kalkoken Date: Thu, 23 Jul 2020 20:58:26 +0000 Subject: [PATCH] Fix error 500 on service page for Discord and add feature "group_to_role" --- .../discord/discord_client/app_settings.py | 8 +- .../modules/discord/discord_client/client.py | 22 ++- .../discord_client/tests/test_client.py | 57 +++++++- .../services/modules/discord/managers.py | 37 +++-- .../services/modules/discord/tasks.py | 56 +++++++- .../modules/discord/tests/__init__.py | 1 + .../modules/discord/tests/test_integration.py | 130 +++++++++++++----- .../modules/discord/tests/test_managers.py | 41 +++++- .../modules/discord/tests/test_tasks.py | 106 ++++++++++++-- docs/features/services/discord.md | 4 +- 10 files changed, 387 insertions(+), 75 deletions(-) diff --git a/allianceauth/services/modules/discord/discord_client/app_settings.py b/allianceauth/services/modules/discord/discord_client/app_settings.py index 5f8b21ce..8c09475a 100644 --- a/allianceauth/services/modules/discord/discord_client/app_settings.py +++ b/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 diff --git a/allianceauth/services/modules/discord/discord_client/client.py b/allianceauth/services/modules/discord/discord_client/client.py index aff50c16..afd739e7 100644 --- a/allianceauth/services/modules/discord/discord_client/client.py +++ b/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 diff --git a/allianceauth/services/modules/discord/discord_client/tests/test_client.py b/allianceauth/services/modules/discord/discord_client/tests/test_client.py index 0d9437fc..25b3655b 100644 --- a/allianceauth/services/modules/discord/discord_client/tests/test_client.py +++ b/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, diff --git a/allianceauth/services/modules/discord/managers.py b/allianceauth/services/modules/discord/managers.py index 60b26de8..d54599de 100644 --- a/allianceauth/services/modules/discord/managers.py +++ b/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) - except HTTPError: + server_name = cls._bot_client().guild_name( + 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) diff --git a/allianceauth/services/modules/discord/tasks.py b/allianceauth/services/modules/discord/tasks.py index 41de134b..373abbe4 100644 --- a/allianceauth/services/modules/discord/tasks.py +++ b/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) diff --git a/allianceauth/services/modules/discord/tests/__init__.py b/allianceauth/services/modules/discord/tests/__init__.py index 6958ea48..c323d4f4 100644 --- a/allianceauth/services/modules/discord/tests/__init__.py +++ b/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 ) diff --git a/allianceauth/services/modules/discord/tests/test_integration.py b/allianceauth/services/modules/discord/tests/test_integration.py index 908cf198..2a6c65c2 100644 --- a/allianceauth/services/modules/discord/tests/test_integration.py +++ b/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 - response = self.app.get(reverse('discord:activate')) + # user opens services page + 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'}) - - # remove_guild_member() + guild_infos_request.url, json={'id': TEST_GUILD_ID, 'name': 'Test Guild'} + ) 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 - response = self.app.get(reverse('discord:deactivate')) + # user opens services page + 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'}) - - # remove_guild_member() + guild_infos_request.url, json={'id': TEST_GUILD_ID, 'name': 'Test Guild'} + ) 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 - response = self.app.get(reverse('discord:deactivate')) + # user opens services page + 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() - mock_exception = HTTPError('can not get guild info from Discord API') - mock_exception.response = Mock() - mock_exception.response.status_code = 440 + # setup + mock_exception = HTTPError(Mock(**{"response.status_code": 400})) 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) diff --git a/allianceauth/services/modules/discord/tests/test_managers.py b/allianceauth/services/modules/discord/tests/test_managers.py index bffa172d..79eb2249 100644 --- a/allianceauth/services/modules/discord/tests/test_managers.py +++ b/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()) diff --git a/allianceauth/services/modules/discord/tests/test_tasks.py b/allianceauth/services/modules/discord/tests/test_tasks.py index dde446a2..4c33b012 100644 --- a/allianceauth/services/modules/discord/tests/test_tasks.py +++ b/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') - def test_can_update_all_usernames(self, mock_update_username): + @patch(MODULE_PATH + '.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) - current_pks = [args[0][0] for args in mock_update_username.call_args_list] + self.assertTrue(mock_update_servername.delay.called) + 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)) diff --git a/docs/features/services/discord.md b/docs/features/services/discord.md index eaef7ce6..59571d86 100644 --- a/docs/features/services/discord.md +++ b/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_ROLES_CACHE_MAX_AGE` How long roles retrieved from the Discord server are 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 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`