Version Bump v3.0.0a5

This reverts commit 13e2f4e27d.

.gitlab-ci.yml

@@ -14,6 +14,7 @@ stages:
 include:
 - template: Dependency-Scanning.gitlab-ci.yml
 - template: Security/SAST.gitlab-ci.yml
+- template: Security/Secret-Detection.gitlab-ci.yml
 
 before_script:
   - apt-get update && apt-get install redis-server -y
@@ -24,7 +25,7 @@ before_script:
 pre-commit-check:
   <<: *only-default
   stage: pre-commit
-  image: python:3.6-buster
+  image: python:3.8-bullseye
   variables:
     PRE_COMMIT_HOME: ${CI_PROJECT_DIR}/.cache/pre-commit
   cache:
@@ -46,18 +47,6 @@ dependency_scanning:
   - python -V
   - pip install wheel tox
 
-test-3.7-core:
-  <<: *only-default
-  image: python:3.7-bullseye
-  script:
-  - tox -e py37-core
-  artifacts:
-    when: always
-    reports:
-      coverage_report:
-        coverage_format: cobertura
-        path: coverage.xml
-
 test-3.8-core:
   <<: *only-default
   image: python:3.8-bullseye
@@ -107,18 +96,6 @@ test-3.11-core:
         path: coverage.xml
   allow_failure: true
 
-test-3.7-all:
-  <<: *only-default
-  image: python:3.7-bullseye
-  script:
-  - tox -e py37-all
-  artifacts:
-    when: always
-    reports:
-      coverage_report:
-        coverage_format: cobertura
-        path: coverage.xml
-
 test-3.8-all:
   <<: *only-default
   image: python:3.8-bullseye
@@ -168,22 +145,18 @@ test-3.11-all:
         path: coverage.xml
   allow_failure: true
 
-test-docs:
-  <<: *only-default
-  image: python:3.9-bullseye
-  script:
-  - tox -e docs
-
 deploy_production:
   stage: deploy
   image: python:3.10-bullseye
 
   before_script:
-    - pip install twine wheel
+    - python -m pip install --upgrade pip
+    - python -m pip install --upgrade build
+    - python -m pip install --upgrade setuptools wheel twine
 
   script:
-    - python setup.py sdist bdist_wheel
-    - twine upload dist/*
+    - python -m build
+    - python -m twine upload dist/*
 
   rules:
     - if: $CI_COMMIT_TAG

.pre-commit-config.yaml

@@ -5,7 +5,7 @@
 
 repos:
   - repo: https://github.com/pre-commit/pre-commit-hooks
-    rev: v4.0.1
+    rev: v4.1.0
     hooks:
       - id: check-case-conflict
       - id: check-json
@@ -22,13 +22,18 @@ repos:
         args: [ '--remove' ]
 
   - repo: https://github.com/editorconfig-checker/editorconfig-checker.python
-    rev: 2.3.54
+    rev: 2.4.0
     hooks:
       - id: editorconfig-checker
         exclude: ^(LICENSE|allianceauth\/static\/css\/themes\/bootstrap-locals.less|allianceauth\/eveonline\/swagger.json|(.*.po)|(.*.mo))
 
   - repo: https://github.com/asottile/pyupgrade
-    rev: v2.29.0
+    rev: v2.31.0
     hooks:
       - id: pyupgrade
-        args: [ --py37-plus ]
+        args: [ --py38-plus ]
+
+  - repo: https://github.com/asottile/setup-cfg-fmt
+    rev: v1.20.0
+    hooks:
+      - id: setup-cfg-fmt

.readthedocs.yml

@@ -5,19 +5,22 @@
 # Required
 version: 2
 
+# Set the version of Python and other tools you might need
+build:
+  os: ubuntu-20.04
+  apt_packages:
+    - redis
+  tools:
+    python: "3.10"
+
 # Build documentation in the docs/ directory with Sphinx
 sphinx:
   configuration: docs/conf.py
 
-# Build documentation with MkDocs
-#mkdocs:
-#  configuration: mkdocs.yml
-
 # Optionally build your docs in additional formats such as PDF and ePub
 formats: all
 
 # Optionally set the version of Python and requirements required to build your docs
 python:
-  version: 3.7
   install:
     - requirements: docs/requirements.txt

allianceauth/__init__.py

@@ -1,8 +1,7 @@
 # This will make sure the app is always imported when
 # Django starts so that shared_task will use this app.
 
-__version__ = '2.15.0'
+__version__ = '3.0.0a5'
 __title__ = 'Alliance Auth'
 __url__ = 'https://gitlab.com/allianceauth/allianceauth'
 NAME = f'{__title__} v{__version__}'
-default_app_config = 'allianceauth.apps.AllianceAuthConfig'

allianceauth/analytics/__init__.py

@@ -1 +0,0 @@
-default_app_config = 'allianceauth.analytics.apps.AnalyticsConfig'

allianceauth/analytics/tests/test_integration.py

@@ -3,17 +3,16 @@ from urllib.parse import parse_qs
 
 import requests_mock
 
-from django.test import override_settings
+from django.test import TestCase, override_settings
 
 from allianceauth.analytics.tasks import ANALYTICS_URL
 from allianceauth.eveonline.tasks import update_character
 from allianceauth.tests.auth_utils import AuthUtils
-from allianceauth.utils.testing import NoSocketsTestCase
 
 
 @override_settings(CELERY_ALWAYS_EAGER=True)
 @requests_mock.mock()
-class TestAnalyticsForViews(NoSocketsTestCase):
+class TestAnalyticsForViews(TestCase):
     @override_settings(ANALYTICS_DISABLED=False)
     def test_should_run_analytics(self, requests_mocker):
         # given
@@ -41,7 +40,7 @@ class TestAnalyticsForViews(NoSocketsTestCase):
 
 @override_settings(CELERY_ALWAYS_EAGER=True)
 @requests_mock.mock()
-class TestAnalyticsForTasks(NoSocketsTestCase):
+class TestAnalyticsForTasks(TestCase):
     @override_settings(ANALYTICS_DISABLED=False)
     @patch("allianceauth.eveonline.models.EveCharacter.objects.update_character")
     def test_should_run_analytics_for_successful_task(

allianceauth/analytics/tests/test_middleware.py

@@ -1,5 +1,6 @@
 from allianceauth.analytics.middleware import AnalyticsMiddleware
 from unittest.mock import Mock
+from django.http import HttpResponse
 
 from django.test.testcases import TestCase
 
@@ -7,7 +8,7 @@ from django.test.testcases import TestCase
 class TestAnalyticsMiddleware(TestCase):
 
     def setUp(self):
-        self.middleware = AnalyticsMiddleware()
+        self.middleware = AnalyticsMiddleware(HttpResponse)
         self.request = Mock()
         self.request.headers = {
                 "User-Agent": "AUTOMATED TEST"

allianceauth/analytics/tests/test_tasks.py

@@ -1,22 +1,12 @@
-import requests_mock
-
-from django.test.utils import override_settings
-
 from allianceauth.analytics.tasks import (
     analytics_event,
     send_ga_tracking_celery_event,
     send_ga_tracking_web_view)
-from allianceauth.utils.testing import NoSocketsTestCase
+from django.test.testcases import TestCase
 
 
-GOOGLE_ANALYTICS_DEBUG_URL = 'https://www.google-analytics.com/debug/collect'
-
-
-@override_settings(CELERY_ALWAYS_EAGER=True, CELERY_EAGER_PROPAGATES_EXCEPTIONS=True)
-@requests_mock.Mocker()
-class TestAnalyticsTasks(NoSocketsTestCase):
-    def test_analytics_event(self, requests_mocker):
-        requests_mocker.register_uri('POST', GOOGLE_ANALYTICS_DEBUG_URL)
+class TestAnalyticsTasks(TestCase):
+    def test_analytics_event(self):
         analytics_event(
                         category='allianceauth.analytics',
                         action='send_tests',
@@ -24,19 +14,15 @@ class TestAnalyticsTasks(NoSocketsTestCase):
                         value=1,
                         event_type='Stats')
 
-    def test_send_ga_tracking_web_view_sent(self, requests_mocker):
-        """This test sends if the event SENDS to google.
-        Not if it was successful.
-        """
-        # given
-        requests_mocker.register_uri('POST', GOOGLE_ANALYTICS_DEBUG_URL)
+    def test_send_ga_tracking_web_view_sent(self):
+        # This test sends if the event SENDS to google
+        # Not if it was successful
         tracking_id = 'UA-186249766-2'
         client_id = 'ab33e241fbf042b6aa77c7655a768af7'
         page = '/index/'
         title = 'Hello World'
         locale = 'en'
         useragent = "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/87.0.4280.88 Safari/537.36"
-        # when
         response = send_ga_tracking_web_view(
                                             tracking_id,
                                             client_id,
@@ -44,23 +30,15 @@ class TestAnalyticsTasks(NoSocketsTestCase):
                                             title,
                                             locale,
                                             useragent)
-        # then
         self.assertEqual(response.status_code, 200)
 
-    def test_send_ga_tracking_web_view_success(self, requests_mocker):
-        # given
-        requests_mocker.register_uri(
-            'POST',
-            GOOGLE_ANALYTICS_DEBUG_URL,
-            json={"hitParsingResult":[{'valid': True}]}
-        )
+    def test_send_ga_tracking_web_view_success(self):
         tracking_id = 'UA-186249766-2'
         client_id = 'ab33e241fbf042b6aa77c7655a768af7'
         page = '/index/'
         title = 'Hello World'
         locale = 'en'
         useragent = "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/87.0.4280.88 Safari/537.36"
-        # when
         json_response = send_ga_tracking_web_view(
                                                 tracking_id,
                                                 client_id,
@@ -68,42 +46,15 @@ class TestAnalyticsTasks(NoSocketsTestCase):
                                                 title,
                                                 locale,
                                                 useragent).json()
-        # then
         self.assertTrue(json_response["hitParsingResult"][0]["valid"])
 
-    def test_send_ga_tracking_web_view_invalid_token(self, requests_mocker):
-        # given
-        requests_mocker.register_uri(
-            'POST',
-            GOOGLE_ANALYTICS_DEBUG_URL,
-            json={
-                "hitParsingResult":[
-                    {
-                        'valid': False,
-                        'parserMessage': [
-                            {
-                                'messageType': 'INFO',
-                                'description': 'IP Address from this hit was anonymized to 1.132.110.0.',
-                                'messageCode': 'VALUE_MODIFIED'
-                            },
-                            {
-                                'messageType': 'ERROR',
-                                'description': "The value provided for parameter 'tid' is invalid. Please see http://goo.gl/a8d4RP#tid for details.",
-                                'messageCode': 'VALUE_INVALID', 'parameter': 'tid'
-                            }
-                        ],
-                        'hit': '/debug/collect?v=1&tid=UA-IntentionallyBadTrackingID-2&cid=ab33e241fbf042b6aa77c7655a768af7&t=pageview&dp=/index/&dt=Hello World&ul=en&ua=Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/87.0.4280.88 Safari/537.36&aip=1&an=allianceauth&av=2.9.0a2'
-                    }
-                ]
-            }
-        )
+    def test_send_ga_tracking_web_view_invalid_token(self):
         tracking_id = 'UA-IntentionallyBadTrackingID-2'
         client_id = 'ab33e241fbf042b6aa77c7655a768af7'
         page = '/index/'
         title = 'Hello World'
         locale = 'en'
         useragent = "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/87.0.4280.88 Safari/537.36"
-        # when
         json_response = send_ga_tracking_web_view(
                                             tracking_id,
                                             client_id,
@@ -111,25 +62,18 @@ class TestAnalyticsTasks(NoSocketsTestCase):
                                             title,
                                             locale,
                                             useragent).json()
-        # then
         self.assertFalse(json_response["hitParsingResult"][0]["valid"])
-        self.assertEqual(
-            json_response["hitParsingResult"][0]["parserMessage"][1]["description"],
-            "The value provided for parameter 'tid' is invalid. Please see http://goo.gl/a8d4RP#tid for details."
-        )
+        self.assertEqual(json_response["hitParsingResult"][0]["parserMessage"][1]["description"], "The value provided for parameter 'tid' is invalid. Please see http://goo.gl/a8d4RP#tid for details.")
 
         # [{'valid': False, 'parserMessage': [{'messageType': 'INFO', 'description': 'IP Address from this hit was anonymized to 1.132.110.0.', 'messageCode': 'VALUE_MODIFIED'}, {'messageType': 'ERROR', 'description': "The value provided for parameter 'tid' is invalid. Please see http://goo.gl/a8d4RP#tid for details.", 'messageCode': 'VALUE_INVALID', 'parameter': 'tid'}], 'hit': '/debug/collect?v=1&tid=UA-IntentionallyBadTrackingID-2&cid=ab33e241fbf042b6aa77c7655a768af7&t=pageview&dp=/index/&dt=Hello World&ul=en&ua=Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/87.0.4280.88 Safari/537.36&aip=1&an=allianceauth&av=2.9.0a2'}]
 
-    def test_send_ga_tracking_celery_event_sent(self, requests_mocker):
-        # given
-        requests_mocker.register_uri('POST', GOOGLE_ANALYTICS_DEBUG_URL)
+    def test_send_ga_tracking_celery_event_sent(self):
         tracking_id = 'UA-186249766-2'
         client_id = 'ab33e241fbf042b6aa77c7655a768af7'
         category = 'test'
         action = 'test'
         label = 'test'
         value = '1'
-        # when
         response = send_ga_tracking_celery_event(
                                                 tracking_id,
                                                 client_id,
@@ -137,23 +81,15 @@ class TestAnalyticsTasks(NoSocketsTestCase):
                                                 action,
                                                 label,
                                                 value)
-        # then
         self.assertEqual(response.status_code, 200)
 
-    def test_send_ga_tracking_celery_event_success(self, requests_mocker):
-        # given
-        requests_mocker.register_uri(
-            'POST',
-            GOOGLE_ANALYTICS_DEBUG_URL,
-            json={"hitParsingResult":[{'valid': True}]}
-        )
+    def test_send_ga_tracking_celery_event_success(self):
         tracking_id = 'UA-186249766-2'
         client_id = 'ab33e241fbf042b6aa77c7655a768af7'
         category = 'test'
         action = 'test'
         label = 'test'
         value = '1'
-        # when
         json_response = send_ga_tracking_celery_event(
                                                     tracking_id,
                                                     client_id,
@@ -161,42 +97,15 @@ class TestAnalyticsTasks(NoSocketsTestCase):
                                                     action,
                                                     label,
                                                     value).json()
-        # then
         self.assertTrue(json_response["hitParsingResult"][0]["valid"])
 
-    def test_send_ga_tracking_celery_event_invalid_token(self, requests_mocker):
-        # given
-        requests_mocker.register_uri(
-            'POST',
-            GOOGLE_ANALYTICS_DEBUG_URL,
-            json={
-                "hitParsingResult":[
-                    {
-                        'valid': False,
-                        'parserMessage': [
-                            {
-                                'messageType': 'INFO',
-                                'description': 'IP Address from this hit was anonymized to 1.132.110.0.',
-                                'messageCode': 'VALUE_MODIFIED'
-                            },
-                            {
-                                'messageType': 'ERROR',
-                                'description': "The value provided for parameter 'tid' is invalid. Please see http://goo.gl/a8d4RP#tid for details.",
-                                'messageCode': 'VALUE_INVALID', 'parameter': 'tid'
-                            }
-                        ],
-                        'hit': '/debug/collect?v=1&tid=UA-IntentionallyBadTrackingID-2&cid=ab33e241fbf042b6aa77c7655a768af7&t=pageview&dp=/index/&dt=Hello World&ul=en&ua=Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/87.0.4280.88 Safari/537.36&aip=1&an=allianceauth&av=2.9.0a2'
-                    }
-                ]
-            }
-        )
+    def test_send_ga_tracking_celery_event_invalid_token(self):
         tracking_id = 'UA-IntentionallyBadTrackingID-2'
         client_id = 'ab33e241fbf042b6aa77c7655a768af7'
         category = 'test'
         action = 'test'
         label = 'test'
         value = '1'
-        # when
         json_response = send_ga_tracking_celery_event(
                                                     tracking_id,
                                                     client_id,
@@ -204,9 +113,7 @@ class TestAnalyticsTasks(NoSocketsTestCase):
                                                     action,
                                                     label,
                                                     value).json()
-        # then
         self.assertFalse(json_response["hitParsingResult"][0]["valid"])
-        self.assertEqual(
-            json_response["hitParsingResult"][0]["parserMessage"][1]["description"],
-            "The value provided for parameter 'tid' is invalid. Please see http://goo.gl/a8d4RP#tid for details."
-        )
+        self.assertEqual(json_response["hitParsingResult"][0]["parserMessage"][1]["description"], "The value provided for parameter 'tid' is invalid. Please see http://goo.gl/a8d4RP#tid for details.")
+
+        # [{'valid': False, 'parserMessage': [{'messageType': 'INFO', 'description': 'IP Address from this hit was anonymized to 1.132.110.0.', 'messageCode': 'VALUE_MODIFIED'}, {'messageType': 'ERROR', 'description': "The value provided for parameter 'tid' is invalid. Please see http://goo.gl/a8d4RP#tid for details.", 'messageCode': 'VALUE_INVALID', 'parameter': 'tid'}], 'hit': '/debug/collect?v=1&tid=UA-IntentionallyBadTrackingID-2&cid=ab33e241fbf042b6aa77c7655a768af7&t=pageview&dp=/index/&dt=Hello World&ul=en&ua=Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/87.0.4280.88 Safari/537.36&aip=1&an=allianceauth&av=2.9.0a2'}]

allianceauth/authentication/__init__.py

@@ -1 +0,0 @@
-default_app_config = 'allianceauth.authentication.apps.AuthenticationConfig'

allianceauth/authentication/hmac_urls.py

@@ -1,14 +1,16 @@
-from django.conf.urls import url, include
+from django.conf.urls import include
 
 from allianceauth.authentication import views
+from django.urls import re_path
+from django.urls import path
 
 urlpatterns = [
-    url(r'^activate/complete/$', views.activation_complete, name='registration_activation_complete'),
+    path('activate/complete/', views.activation_complete, name='registration_activation_complete'),
     # The activation key can make use of any character from the
     # URL-safe base64 alphabet, plus the colon as a separator.
-    url(r'^activate/(?P<activation_key>[-:\w]+)/$', views.ActivationView.as_view(), name='registration_activate'),
-    url(r'^register/$', views.RegistrationView.as_view(), name='registration_register'),
-    url(r'^register/complete/$', views.registration_complete, name='registration_complete'),
-    url(r'^register/closed/$', views.registration_closed, name='registration_disallowed'),
-    url(r'', include('django.contrib.auth.urls')),
+    re_path(r'^activate/(?P<activation_key>[-:\w]+)/$', views.ActivationView.as_view(), name='registration_activate'),
+    path('register/', views.RegistrationView.as_view(), name='registration_register'),
+    path('register/complete/', views.registration_complete, name='registration_complete'),
+    path('register/closed/', views.registration_closed, name='registration_disallowed'),
+    path('', include('django.contrib.auth.urls')),
 ]

allianceauth/authentication/middleware.py

@@ -0,0 +1,45 @@
+from django.conf import settings
+from django.utils.deprecation import MiddlewareMixin
+
+import logging
+
+logger = logging.getLogger(__name__)
+
+
+class UserSettingsMiddleware(MiddlewareMixin):
+    def process_response(self, request, response):
+        """Django Middleware: User Settings."""
+
+        # Intercept the built in django /setlang/ view and also save it to Database.
+        # Note the annoymous user check, only logged in users will ever hit the DB here
+        if request.path == '/i18n/setlang/' and not request.user.is_anonymous:
+            try:
+                request.user.profile.language = request.POST['language']
+                request.user.profile.save()
+            except Exception as e:
+                logger.exception(e)
+
+        # Only act during the login flow, _after_ user is activated (step 2: post-sso)
+        elif request.path == '/sso/login' and not request.user.is_anonymous:
+            # Set the Language Cookie, if it doesnt match the DB
+            # Null = hasnt been set by the user ever, dont act.
+            try:
+                if request.user.profile.language != request.LANGUAGE_CODE and request.user.profile.language is not None:
+                    response.set_cookie(key=settings.LANGUAGE_COOKIE_NAME,
+                                        value=request.user.profile.language,
+                                        max_age=settings.LANGUAGE_COOKIE_AGE)
+            except Exception as e:
+                logger.exception(e)
+
+            # Set our Night mode flag from the DB
+            # Null = hasnt been set by the user ever, dont act.
+            #
+            # Night mode intercept is not needed in this middleware.
+            # is saved direct to DB in NightModeRedirectView
+            try:
+                if request.user.profile.night_mode is not None:
+                    request.session["NIGHT_MODE"] = request.user.profile.night_mode
+            except Exception as e:
+                logger.exception(e)
+
+        return response

allianceauth/authentication/migrations/0020_userprofile_language_userprofile_night_mode.py

@@ -0,0 +1,23 @@
+# Generated by Django 4.0.2 on 2022-02-26 03:45
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('authentication', '0019_merge_20211026_0919'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='userprofile',
+            name='language',
+            field=models.CharField(blank=True, choices=[('en', 'English'), ('de', 'German'), ('es', 'Spanish'), ('zh-hans', 'Chinese Simplified'), ('ru', 'Russian'), ('ko', 'Korean'), ('fr', 'French'), ('ja', 'Japanese'), ('it', 'Italian')], default='', max_length=10, verbose_name='Language'),
+        ),
+        migrations.AddField(
+            model_name='userprofile',
+            name='night_mode',
+            field=models.BooleanField(blank=True, null=True, verbose_name='Night Mode'),
+        ),
+    ]

allianceauth/authentication/models.py

@@ -2,9 +2,10 @@ import logging
 
 from django.contrib.auth.models import User, Permission
 from django.db import models, transaction
-from django.utils.translation import ugettext_lazy as _
+from django.utils.translation import gettext_lazy as _
 from allianceauth.eveonline.models import EveCharacter, EveCorporationInfo, EveAllianceInfo, EveFactionInfo
 from allianceauth.notifications import notify
+from django.conf import settings
 
 from .managers import CharacterOwnershipManager, StateManager
 
@@ -62,9 +63,39 @@ class UserProfile(models.Model):
     class Meta:
         default_permissions = ('change',)
 
-    user = models.OneToOneField(User, related_name='profile', on_delete=models.CASCADE)
-    main_character = models.OneToOneField(EveCharacter, blank=True, null=True, on_delete=models.SET_NULL)
-    state = models.ForeignKey(State, on_delete=models.SET_DEFAULT, default=get_guest_state_pk)
+    user = models.OneToOneField(
+        User,
+        related_name='profile',
+        on_delete=models.CASCADE)
+    main_character = models.OneToOneField(
+        EveCharacter,
+        blank=True,
+        null=True,
+        on_delete=models.SET_NULL)
+    state = models.ForeignKey(
+        State,
+        on_delete=models.SET_DEFAULT,
+        default=get_guest_state_pk)
+    LANGUAGE_CHOICES = [
+        ('en', _('English')),
+        ('de', _('German')),
+        ('es', _('Spanish')),
+        ('zh-hans', _('Chinese Simplified')),
+        ('ru', _('Russian')),
+        ('ko', _('Korean')),
+        ('fr', _('French')),
+        ('ja', _('Japanese')),
+        ('it', _('Italian')),
+    ]
+    language = models.CharField(
+        _("Language"), max_length=10,
+        choices=LANGUAGE_CHOICES,
+        blank=True,
+        default='')
+    night_mode = models.BooleanField(
+        _("Night Mode"),
+        blank=True,
+        null=True)
 
     def assign_state(self, state=None, commit=True):
         if not state:
@@ -93,8 +124,6 @@ class UserProfile(models.Model):
 
     def __str__(self):
         return str(self.user)
-
-
 class CharacterOwnership(models.Model):
     class Meta:
         default_permissions = ('change', 'delete')

allianceauth/authentication/signals.py

@@ -1,6 +1,11 @@
 import logging
 
-from .models import CharacterOwnership, UserProfile, get_guest_state, State, OwnershipRecord
+from .models import (
+    CharacterOwnership,
+    UserProfile,
+    get_guest_state,
+    State,
+    OwnershipRecord)
 from django.contrib.auth.models import User
 from django.db.models import Q
 from django.db.models.signals import pre_save, post_save, pre_delete, post_delete, m2m_changed
@@ -11,7 +16,7 @@ from allianceauth.eveonline.models import EveCharacter
 
 logger = logging.getLogger(__name__)
 
-state_changed = Signal(providing_args=['user', 'state'])
+state_changed = Signal()
 
 
 def trigger_state_check(state):
@@ -71,7 +76,7 @@ def reassess_on_profile_save(sender, instance, created, *args, **kwargs):
 
 @receiver(post_save, sender=User)
 def create_required_models(sender, instance, created, *args, **kwargs):
-    # ensure all users have a model
+    # ensure all users have our Sub-Models
     if created:
         logger.debug(f'User {instance} created. Creating default UserProfile.')
         UserProfile.objects.get_or_create(user=instance)

allianceauth/authentication/task_statistics/event_series.py

@@ -1,62 +1,27 @@
 import datetime as dt
-import logging
-from typing import List, Optional
+from typing import Optional, List
 
+from redis import Redis
 from pytz import utc
-from redis import Redis, RedisError
 
-from allianceauth.utils.cache import get_redis_client
-
-logger = logging.getLogger(__name__)
-
-
-class _RedisStub:
-    """Stub of a Redis client.
-
-    It's purpose is to prevent EventSeries objects from trying to access Redis
-    when it is not available. e.g. when the Sphinx docs are rendered by readthedocs.org.
-    """
-
-    def delete(self, *args, **kwargs):
-        pass
-
-    def incr(self, *args, **kwargs):
-        return 0
-
-    def zadd(self, *args, **kwargs):
-        pass
-
-    def zcount(self, *args, **kwargs):
-        pass
-
-    def zrangebyscore(self, *args, **kwargs):
-        pass
+from django_redis import get_redis_connection
 
 
 class EventSeries:
-    """API for recording and analyzing a series of events."""
+    """API for recording and analysing a series of events."""
 
     _ROOT_KEY = "ALLIANCEAUTH_EVENT_SERIES"
 
     def __init__(self, key_id: str, redis: Redis = None) -> None:
-        self._redis = get_redis_client() if not redis else redis
-        try:
-            if not self._redis.ping():
-                raise RuntimeError()
-        except (AttributeError, RedisError, RuntimeError):
-            logger.exception(
-                "Failed to establish a connection with Redis. "
-                "This EventSeries object is disabled.",
+        self._redis = get_redis_connection("default") if not redis else redis
+        if not isinstance(self._redis, Redis):
+            raise TypeError(
+                "This class requires a Redis client, but none was provided "
+                "and the default Django cache backend is not Redis either."
             )
-            self._redis = _RedisStub()
         self._key_id = str(key_id)
         self.clear()
 
-    @property
-    def is_disabled(self):
-        """True when this object is disabled, e.g. Redis was not available at startup."""
-        return isinstance(self._redis, _RedisStub)
-
     @property
     def _key_counter(self):
         return f"{self._ROOT_KEY}_{self._key_id}_COUNTER"

allianceauth/authentication/task_statistics/tests/test_event_series.py

@@ -1,48 +1,13 @@
 import datetime as dt
-from unittest.mock import patch
 
 from pytz import utc
-from redis import RedisError
-
 from django.test import TestCase
 from django.utils.timezone import now
 
-from allianceauth.authentication.task_statistics.event_series import (
-    EventSeries,
-    _RedisStub,
-)
-
-MODULE_PATH = "allianceauth.authentication.task_statistics.event_series"
+from allianceauth.authentication.task_statistics.event_series import EventSeries
 
 
 class TestEventSeries(TestCase):
-    def test_should_abort_without_redis_client(self):
-        # when
-        with patch(MODULE_PATH + ".get_redis_client") as mock:
-            mock.return_value = None
-            events = EventSeries("dummy")
-        # then
-        self.assertTrue(events._redis, _RedisStub)
-        self.assertTrue(events.is_disabled)
-
-    def test_should_disable_itself_if_redis_not_available_1(self):
-        # when
-        with patch(MODULE_PATH + ".get_redis_client") as mock_get_master_client:
-            mock_get_master_client.return_value.ping.side_effect = RedisError
-            events = EventSeries("dummy")
-        # then
-        self.assertIsInstance(events._redis, _RedisStub)
-        self.assertTrue(events.is_disabled)
-
-    def test_should_disable_itself_if_redis_not_available_2(self):
-        # when
-        with patch(MODULE_PATH + ".get_redis_client") as mock_get_master_client:
-            mock_get_master_client.return_value.ping.return_value = False
-            events = EventSeries("dummy")
-        # then
-        self.assertIsInstance(events._redis, _RedisStub)
-        self.assertTrue(events.is_disabled)
-
     def test_should_add_event(self):
         # given
         events = EventSeries("dummy")

allianceauth/authentication/templates/registration/registration_form.html

@@ -7,8 +7,8 @@
     <form class="form-signin" role="form" action="" method="POST">
         {% csrf_token %}
         {{ form|bootstrap }}
-        <br/>
+        <br>
         <button class="btn btn-lg btn-primary btn-block" type="submit">{% translate "Submit" %}</button>
-        <br/>
+        <br>
     </form>
 {% endblock %}

allianceauth/authentication/tests/__init__.py

@@ -1,4 +1,17 @@
+from django.db.models.signals import (
+    m2m_changed,
+    post_save,
+    pre_delete,
+    pre_save
+)
 from django.urls import reverse
+from unittest import mock
+
+MODULE_PATH = 'allianceauth.authentication'
+
+
+def patch(target, *args, **kwargs):
+    return mock.patch(f'{MODULE_PATH}{target}', *args, **kwargs)
 
 
 def get_admin_change_view_url(obj: object) -> str:

allianceauth/authentication/tests/test_middleware.py

@@ -0,0 +1,175 @@
+from unittest import mock
+from allianceauth.authentication.middleware import UserSettingsMiddleware
+from unittest.mock import Mock
+from django.http import HttpResponse
+
+from django.test.testcases import TestCase
+
+
+class TestUserSettingsMiddlewareSaveLang(TestCase):
+
+    def setUp(self):
+        self.middleware = UserSettingsMiddleware(HttpResponse)
+        self.request = Mock()
+        self.request.headers = {
+                "User-Agent": "AUTOMATED TEST"
+            }
+        self.request.path = '/i18n/setlang/'
+        self.request.POST = {
+            'language': 'fr'
+        }
+        self.request.user.profile.language = 'de'
+        self.request.user.is_anonymous = False
+        self.response = Mock()
+        self.response.content = 'hello world'
+
+    def test_middleware_passthrough(self):
+        """
+        Simply tests the middleware runs cleanly
+        """
+        response = self.middleware.process_response(
+            self.request,
+            self.response
+            )
+        self.assertEqual(self.response, response)
+
+    def test_middleware_save_language_false_anonymous(self):
+        """
+        Ensures the middleware wont change the usersettings
+        of a non-existent (anonymous) user
+        """
+        self.request.user.is_anonymous = True
+        response = self.middleware.process_response(
+            self.request,
+            self.response
+            )
+        self.assertEqual(self.request.user.profile.language, 'de')
+        self.assertFalse(self.request.user.profile.save.called)
+        self.assertEqual(self.request.user.profile.save.call_count, 0)
+
+    def test_middleware_save_language_new(self):
+        """
+        does the middleware change a language not set in the DB
+        """
+        self.request.user.profile.language = None
+        response = self.middleware.process_response(
+            self.request,
+            self.response
+            )
+        self.assertEqual(self.request.user.profile.language, 'fr')
+        self.assertTrue(self.request.user.profile.save.called)
+        self.assertEqual(self.request.user.profile.save.call_count, 1)
+
+    def test_middleware_save_language_changed(self):
+        """
+        Tests the middleware will change a language setting
+        """
+        response = self.middleware.process_response(
+            self.request,
+            self.response
+            )
+        self.assertEqual(self.request.user.profile.language, 'fr')
+        self.assertTrue(self.request.user.profile.save.called)
+        self.assertEqual(self.request.user.profile.save.call_count, 1)
+
+
+class TestUserSettingsMiddlewareLoginFlow(TestCase):
+
+    def setUp(self):
+        self.middleware = UserSettingsMiddleware(HttpResponse)
+        self.request = Mock()
+        self.request.headers = {
+                "User-Agent": "AUTOMATED TEST"
+            }
+        self.request.path = '/sso/login'
+        self.request.session = {
+            'NIGHT_MODE': False
+        }
+        self.request.LANGUAGE_CODE = 'en'
+        self.request.user.profile.language = 'de'
+        self.request.user.profile.night_mode = True
+        self.request.user.is_anonymous = False
+        self.response = Mock()
+        self.response.content = 'hello world'
+
+    def test_middleware_passthrough(self):
+        """
+        Simply tests the middleware runs cleanly
+        """
+        middleware_response = self.middleware.process_response(
+            self.request,
+            self.response
+            )
+        self.assertEqual(self.response, middleware_response)
+
+    def test_middleware_sets_language_cookie_true_no_cookie(self):
+        """
+        tests the middleware will set a cookie, while none is set
+        """
+        self.request.LANGUAGE_CODE = None
+        middleware_response = self.middleware.process_response(
+            self.request,
+            self.response
+            )
+        self.assertTrue(middleware_response.set_cookie.called)
+        self.assertEqual(middleware_response.set_cookie.call_count, 1)
+        args, kwargs = middleware_response.set_cookie.call_args
+        self.assertEqual(kwargs['value'], 'de')
+
+    def test_middleware_sets_language_cookie_true_wrong_cookie(self):
+        """
+        tests the middleware will set a cookie, while a different value is set
+        """
+        middleware_response = self.middleware.process_response(
+            self.request,
+            self.response
+            )
+        self.assertTrue(middleware_response.set_cookie.called)
+        self.assertEqual(middleware_response.set_cookie.call_count, 1)
+        args, kwargs = middleware_response.set_cookie.call_args
+        self.assertEqual(kwargs['value'], 'de')
+
+    def test_middleware_sets_language_cookie_false_anonymous(self):
+        """
+        ensures the middleware wont set a value for a non existent user (anonymous)
+        """
+        self.request.user.is_anonymous = True
+        middleware_response = self.middleware.process_response(
+            self.request,
+            self.response
+            )
+        self.assertFalse = middleware_response.set_cookie.called
+        self.assertEqual(middleware_response.set_cookie.call_count, 0)
+
+    def test_middleware_sets_language_cookie_false_already_set(self):
+        """
+        tests the middleware skips setting the cookie, if its already set correctly
+        """
+        self.request.user.profile.language = 'en'
+        middleware_response = self.middleware.process_response(
+            self.request,
+            self.response
+            )
+        self.assertFalse = middleware_response.set_cookie.called
+        self.assertEqual(middleware_response.set_cookie.call_count, 0)
+
+    def test_middleware_sets_night_mode_not_set(self):
+        """
+        tests the middleware will set night_mode if not set
+        """
+        self.request.session = {}
+        response = self.middleware.process_response(
+            self.request,
+            self.response
+            )
+        self.assertEqual(self.request.session["NIGHT_MODE"], True)
+
+    def test_middleware_sets_night_mode_set(self):
+        """
+        tests the middleware will set night_mode if set.
+        """
+        response = self.middleware.process_response(
+            self.request,
+            self.response
+            )
+        self.assertEqual(self.request.session["NIGHT_MODE"], True)

allianceauth/authentication/tests/test_signals.py

@@ -0,0 +1,94 @@
+from allianceauth.authentication.models import User, UserProfile
+from allianceauth.eveonline.models import (
+    EveCharacter,
+    EveCorporationInfo,
+    EveAllianceInfo
+)
+from django.db.models.signals import (
+    pre_save,
+    post_save,
+    pre_delete,
+    m2m_changed
+)
+from allianceauth.tests.auth_utils import AuthUtils
+
+from django.test.testcases import TestCase
+from unittest.mock import Mock
+from . import patch
+
+
+class TestUserProfileSignals(TestCase):
+
+    def setUp(self):
+        state = AuthUtils.get_member_state()
+
+        self.char = EveCharacter.objects.create(
+            character_id='1234',
+            character_name='test character',
+            corporation_id='2345',
+            corporation_name='test corp',
+            corporation_ticker='tickr',
+            alliance_id='3456',
+            alliance_name='alliance name',
+        )
+
+        self.alliance = EveAllianceInfo.objects.create(
+            alliance_id='3456',
+            alliance_name='alliance name',
+            alliance_ticker='TIKR',
+            executor_corp_id='2345',
+        )
+
+        self.corp = EveCorporationInfo.objects.create(
+            corporation_id='2345',
+            corporation_name='corp name',
+            corporation_ticker='TIKK',
+            member_count=10,
+            alliance=self.alliance,
+        )
+
+        state.member_alliances.add(self.alliance)
+        state.member_corporations.add(self.corp)
+
+        self.member = AuthUtils.create_user('test user')
+        self.member.profile.main_character = self.char
+        self.member.profile.save()
+
+    @patch('.signals.create_required_models')
+    def test_create_required_models_triggered_true(
+            self, create_required_models):
+        """
+        Create a User object here,
+        to generate UserProfile models
+        """
+        post_save.connect(create_required_models, sender=User)
+        AuthUtils.create_user('test_create_required_models_triggered')
+        self.assertTrue = create_required_models.called
+        self.assertEqual(create_required_models.call_count, 1)
+
+        user = User.objects.get(username='test_create_required_models_triggered')
+        self.assertIsNot(UserProfile.objects.get(user=user), False)
+
+    @patch('.signals.create_required_models')
+    def test_create_required_models_triggered_false(
+            self, create_required_models):
+        """
+        Only call a User object Update here,
+        which does not need to generate UserProfile models
+        """
+        post_save.connect(create_required_models, sender=User)
+        char = EveCharacter.objects.create(
+            character_id='1266',
+            character_name='test character2',
+            corporation_id='2345',
+            corporation_name='test corp',
+            corporation_ticker='tickr',
+            alliance_id='3456',
+            alliance_name='alliance name',
+        )
+        self.member.profile.main_character = char
+        self.member.profile.save()
+
+        self.assertTrue = create_required_models.called
+        self.assertEqual(create_required_models.call_count, 0)
+        self.assertIsNot(UserProfile.objects.get(user=self.member), False)

allianceauth/authentication/urls.py

@@ -1,5 +1,4 @@
-from django.conf.urls import url
-from django.contrib.auth.decorators import login_required
+from django.urls import path
 from django.views.generic.base import TemplateView
 
 from . import views
@@ -7,21 +6,21 @@ from . import views
 app_name = 'authentication'
 
 urlpatterns = [
-    url(r'^$', views.index, name='index'),
-    url(
-        r'^account/login/$',
+    path('', views.index, name='index'),
+    path(
+        'account/login/',
         TemplateView.as_view(template_name='public/login.html'),
         name='login'
     ),
-    url(
-        r'^account/characters/main/$',
+    path(
+        'account/characters/main/',
         views.main_character_change,
         name='change_main_character'
     ),
-    url(
-        r'^account/characters/add/$',
+    path(
+        'account/characters/add/',
         views.add_character,
         name='add_character'
     ),
-    url(r'^dashboard/$', views.dashboard, name='dashboard'),
+    path('dashboard/', views.dashboard, name='dashboard'),
 ]

allianceauth/corputils/__init__.py

@@ -1 +0,0 @@
-default_app_config = 'allianceauth.corputils.apps.CorpUtilsConfig'

allianceauth/corputils/auth_hooks.py

@@ -1,5 +1,5 @@
 from allianceauth.services.hooks import MenuItemHook, UrlHook
-from django.utils.translation import ugettext_lazy as _
+from django.utils.translation import gettext_lazy as _
 from allianceauth import hooks
 from allianceauth.corputils import urls
 

allianceauth/corputils/urls.py

@@ -1,12 +1,11 @@
-from django.conf.urls import url
-
+from django.urls import path
 from . import views
 
 app_name = 'corputils'
 urlpatterns = [
-    url(r'^$', views.corpstats_view, name='view'),
-    url(r'^add/$', views.corpstats_add, name='add'),
-    url(r'^(?P<corp_id>(\d)*)/$', views.corpstats_view, name='view_corp'),
-    url(r'^(?P<corp_id>(\d)+)/update/$', views.corpstats_update, name='update'),
-    url(r'^search/$', views.corpstats_search, name='search'),
-    ]
+    path('', views.corpstats_view, name='view'),
+    path('add/', views.corpstats_add, name='add'),
+    path('<int:corp_id>/', views.corpstats_view, name='view_corp'),
+    path('<int:corp_id>/update/', views.corpstats_update, name='update'),
+    path('search/', views.corpstats_search, name='search'),
+]

allianceauth/corputils/views.py

@@ -6,7 +6,7 @@ from django.contrib.auth.decorators import login_required, permission_required,
 from django.core.exceptions import PermissionDenied, ObjectDoesNotExist
 from django.db import IntegrityError
 from django.shortcuts import render, redirect, get_object_or_404
-from django.utils.translation import ugettext_lazy as _
+from django.utils.translation import gettext_lazy as _
 from esi.decorators import token_required
 from allianceauth.eveonline.models import EveCharacter, EveCorporationInfo
 

allianceauth/eveonline/__init__.py

@@ -1 +0,0 @@
-default_app_config = 'allianceauth.eveonline.apps.EveonlineConfig'

allianceauth/eveonline/autogroups/__init__.py

@@ -1 +0,0 @@
-default_app_config = 'allianceauth.eveonline.autogroups.apps.EveAutogroupsConfig'

allianceauth/eveonline/migrations/0016_character_names_are_not_unique.py

@@ -0,0 +1,18 @@
+# Generated by Django 3.2.10 on 2022-01-05 18:44
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('eveonline', '0015_factions'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='evecharacter',
+            name='character_name',
+            field=models.CharField(db_index=True, max_length=254),
+        ),
+    ]

allianceauth/eveonline/models.py

@@ -25,6 +25,8 @@ DOOMHEIM_CORPORATION_ID = 1000001
 
 
 class EveFactionInfo(models.Model):
+    """A faction in Eve Online."""
+
     faction_id = models.PositiveIntegerField(unique=True, db_index=True)
     faction_name = models.CharField(max_length=254, unique=True)
 
@@ -66,6 +68,8 @@ class EveFactionInfo(models.Model):
 
 
 class EveAllianceInfo(models.Model):
+    """An alliance in Eve Online."""
+
     alliance_id = models.PositiveIntegerField(unique=True)
     alliance_name = models.CharField(max_length=254, unique=True)
     alliance_ticker = models.CharField(max_length=254)
@@ -132,6 +136,8 @@ class EveAllianceInfo(models.Model):
 
 
 class EveCorporationInfo(models.Model):
+    """A corporation in Eve Online."""
+
     corporation_id = models.PositiveIntegerField(unique=True)
     corporation_name = models.CharField(max_length=254, unique=True)
     corporation_ticker = models.CharField(max_length=254)
@@ -195,9 +201,10 @@ class EveCorporationInfo(models.Model):
 
 
 class EveCharacter(models.Model):
-    """Character in Eve Online"""
+    """A character in Eve Online."""
+
     character_id = models.PositiveIntegerField(unique=True)
-    character_name = models.CharField(max_length=254, unique=True)
+    character_name = models.CharField(max_length=254, db_index=True)
     corporation_id = models.PositiveIntegerField()
     corporation_name = models.CharField(max_length=254)
     corporation_ticker = models.CharField(max_length=5)

allianceauth/eveonline/tasks.py

@@ -40,7 +40,7 @@ def update_character(character_id: int) -> None:
 def run_model_update():
     """Update all alliances, corporations and characters from ESI"""
 
-    # update existing corp models
+    #update existing corp models
     for corp in EveCorporationInfo.objects.all().values('corporation_id'):
         update_corp.apply_async(args=[corp['corporation_id']], priority=TASK_PRIORITY)
 

allianceauth/fleetactivitytracking/__init__.py

@@ -1 +0,0 @@
-default_app_config = 'allianceauth.fleetactivitytracking.apps.FatConfig'

allianceauth/fleetactivitytracking/auth_hooks.py

@@ -1,5 +1,5 @@
 from . import urls
-from django.utils.translation import ugettext_lazy as _
+from django.utils.translation import gettext_lazy as _
 from allianceauth import hooks
 from allianceauth.services.hooks import MenuItemHook, UrlHook
 

allianceauth/fleetactivitytracking/forms.py

@@ -1,5 +1,5 @@
 from django import forms
-from django.utils.translation import ugettext_lazy as _
+from django.utils.translation import gettext_lazy as _
 
 
 class FatlinkForm(forms.Form):

allianceauth/fleetactivitytracking/templates/fleetactivitytracking/fatlinkformatter.html

@@ -21,7 +21,7 @@
                         <form class="form-signin" role="form" action="" method="POST">
                             {% csrf_token %}
                             {{ form|bootstrap }}
-                            <br/>
+                            <br>
                             <button class="btn btn-lg btn-primary btn-block" type="submit" name="submit_fat">{% translate "Create fatlink" %}</button>
                         </form>
                 </div>

allianceauth/fleetactivitytracking/urls.py

@@ -1,30 +1,30 @@
-from django.conf.urls import url
+from django.urls import path
 
 from . import views
 app_name = 'fleetactivitytracking'
 
 urlpatterns = [
     # FleetActivityTracking (FAT)
-    url(r'^$', views.fatlink_view, name='view'),
-    url(r'^statistics/$', views.fatlink_statistics_view, name='statistics'),
-    url(r'^statistics/corp/(\w+)$', views.fatlink_statistics_corp_view,
+    path('', views.fatlink_view, name='view'),
+    path('statistics/', views.fatlink_statistics_view, name='statistics'),
+    path('statistics/corp/<int:corpid>/', views.fatlink_statistics_corp_view,
         name='statistics_corp'),
-    url(r'^statistics/corp/(?P<corpid>\w+)/(?P<year>[0-9]+)/(?P<month>[0-9]+)/',
+    path('statistics/corp/<int:corpid>/<int:year>/<int:month>/',
         views.fatlink_statistics_corp_view,
         name='statistics_corp_month'),
-    url(r'^statistics/(?P<year>[0-9]+)/(?P<month>[0-9]+)/$', views.fatlink_statistics_view,
+    path('statistics/<int:year>/<int:month>/', views.fatlink_statistics_view,
         name='statistics_month'),
-    url(r'^user/statistics/$', views.fatlink_personal_statistics_view,
+    path('user/statistics/', views.fatlink_personal_statistics_view,
         name='personal_statistics'),
-    url(r'^user/statistics/(?P<year>[0-9]+)/$', views.fatlink_personal_statistics_view,
+    path('user/statistics/<int:year>/', views.fatlink_personal_statistics_view,
         name='personal_statistics_year'),
-    url(r'^user/statistics/(?P<year>[0-9]+)/(?P<month>[0-9]+)/$',
+    path('user/statistics/<int:year>/<int:month>/',
         views.fatlink_monthly_personal_statistics_view,
         name='personal_statistics_month'),
-    url(r'^user/(?P<char_id>[0-9]+)/statistics/(?P<year>[0-9]+)/(?P<month>[0-9]+)/$',
+    path('user/<int:char_id>/statistics/<int:year>/<int:month>/',
         views.fatlink_monthly_personal_statistics_view,
         name='user_statistics_month'),
-    url(r'^create/$', views.create_fatlink_view, name='create'),
-    url(r'^modify/(?P<fat_hash>[a-zA-Z0-9_-]+)/$', views.modify_fatlink_view, name='modify'),
-    url(r'^link/(?P<fat_hash>[a-zA-Z0-9]+)/$', views.click_fatlink_view, name='click'),
+    path('create/', views.create_fatlink_view, name='create'),
+    path('modify/<str:fat_hash>/', views.modify_fatlink_view, name='modify'),
+    path('link/<str:fat_hash>/', views.click_fatlink_view, name='click'),
 ]

allianceauth/fleetactivitytracking/views.py

@@ -10,7 +10,7 @@ from django.contrib.auth.models import User
 from django.core.exceptions import ValidationError, ObjectDoesNotExist
 from django.shortcuts import render, redirect, get_object_or_404, Http404
 from django.utils import timezone
-from django.utils.translation import ugettext_lazy as _
+from django.utils.translation import gettext_lazy as _
 from esi.decorators import token_required
 from allianceauth.eveonline.providers import provider
 from .forms import FatlinkForm
@@ -212,14 +212,7 @@ def fatlink_monthly_personal_statistics_view(request, year, month, char_id=None)
     start_of_previous_month = first_day_of_previous_month(year, month)
 
     if request.user.has_perm('auth.fleetactivitytracking_statistics') and char_id:
-        try:
-            user = EveCharacter.objects.get(character_id=char_id).character_ownership.user
-        except EveCharacter.DoesNotExist:
-            messages.error(request, _('Character does not exist'))
-            return redirect('fatlink:view')
-        except AttributeError:
-            messages.error(request, _('User does not exist'))
-            return redirect('fatlink:view')
+        user = EveCharacter.objects.get(character_id=char_id).user
     else:
         user = request.user
     logger.debug(f"Personal monthly statistics view for user {user} called by {request.user}")

allianceauth/groupmanagement/__init__.py

@@ -1 +0,0 @@
-default_app_config = 'allianceauth.groupmanagement.apps.GroupManagementConfig'

allianceauth/groupmanagement/admin.py

@@ -1,8 +1,8 @@
 from django.apps import apps
 from django.contrib import admin
-
-from django.contrib.auth.models import Group as BaseGroup, Permission, User
-from django.db.models import Count, Exists, OuterRef
+from django.contrib.auth.models import Group as BaseGroup
+from django.contrib.auth.models import Permission, User
+from django.db.models import Count
 from django.db.models.functions import Lower
 from django.db.models.signals import (
     m2m_changed,
@@ -15,7 +15,6 @@ from django.dispatch import receiver
 
 from .forms import GroupAdminForm, ReservedGroupNameAdminForm
 from .models import AuthGroup, GroupRequest, ReservedGroupName
-from .tasks import remove_users_not_matching_states_from_group
 
 if 'eve_autogroups' in apps.app_configs:
     _has_auto_groups = True
@@ -107,13 +106,14 @@ class HasLeaderFilter(admin.SimpleListFilter):
 
 class GroupAdmin(admin.ModelAdmin):
     form = GroupAdminForm
+    list_select_related = ('authgroup',)
     ordering = ('name',)
     list_display = (
         'name',
         '_description',
         '_properties',
         '_member_count',
-        'has_leader',
+        'has_leader'
     )
     list_filter = [
         'authgroup__internal',
@@ -129,51 +129,31 @@ class GroupAdmin(admin.ModelAdmin):
 
     def get_queryset(self, request):
         qs = super().get_queryset(request)
-        has_leader_qs = (
-            AuthGroup.objects.filter(group=OuterRef('pk'), group_leaders__isnull=False)
-        )
-        has_leader_groups_qs = (
-            AuthGroup.objects.filter(
-                group=OuterRef('pk'), group_leader_groups__isnull=False
-            )
-        )
-        qs = (
-            qs.select_related('authgroup')
-            .annotate(member_count=Count('user', distinct=True))
-            .annotate(has_leader=Exists(has_leader_qs))
-            .annotate(has_leader_groups=Exists(has_leader_groups_qs))
-        )
         if _has_auto_groups:
-            is_autogroup_corp = (
-                Group.objects.filter(
-                    pk=OuterRef('pk'), managedcorpgroup__isnull=False
-                )
-            )
-            is_autogroup_alliance = (
-                Group.objects.filter(
-                    pk=OuterRef('pk'), managedalliancegroup__isnull=False
-                )
-            )
-            qs = (
-                qs.annotate(is_autogroup_corp=Exists(is_autogroup_corp))
-                .annotate(is_autogroup_alliance=Exists(is_autogroup_alliance))
-            )
+            qs = qs.prefetch_related('managedalliancegroup_set', 'managedcorpgroup_set')
+        qs = qs.prefetch_related('authgroup__group_leaders').select_related('authgroup')
+        qs = qs.annotate(
+            member_count=Count('user', distinct=True),
+        )
         return qs
 
     def _description(self, obj):
         return obj.authgroup.description
 
-    @admin.display(description='Members', ordering='member_count')
+    @admin.display(description="Members", ordering="member_count")
     def _member_count(self, obj):
         return obj.member_count
 
     @admin.display(boolean=True)
     def has_leader(self, obj):
-        return obj.has_leader or obj.has_leader_groups
+        return obj.authgroup.group_leaders.exists() or obj.authgroup.group_leader_groups.exists()
 
     def _properties(self, obj):
         properties = list()
-        if _has_auto_groups and (obj.is_autogroup_corp or obj.is_autogroup_alliance):
+        if _has_auto_groups and (
+            obj.managedalliancegroup_set.exists()
+            or obj.managedcorpgroup_set.exists()
+        ):
             properties.append('Auto Group')
         elif obj.authgroup.internal:
             properties.append('Internal')
@@ -203,8 +183,6 @@ class GroupAdmin(admin.ModelAdmin):
             ag_instance = inline_form.save(commit=False)
             ag_instance.group = form.instance
             ag_instance.save()
-            if ag_instance.states.exists():
-                remove_users_not_matching_states_from_group.delay(ag_instance.group.pk)
         formset.save()
 
     def get_readonly_fields(self, request, obj=None):

allianceauth/groupmanagement/auth_hooks.py

@@ -1,4 +1,4 @@
-from django.utils.translation import ugettext_lazy as _
+from django.utils.translation import gettext_lazy as _
 
 from allianceauth.services.hooks import MenuItemHook, UrlHook
 from allianceauth import hooks

allianceauth/groupmanagement/models.py

@@ -189,15 +189,6 @@ class AuthGroup(models.Model):
             | User.objects.filter(groups__in=list(self.group_leader_groups.all()))
         )
 
-    def remove_users_not_matching_states(self):
-        """Remove users not matching defined states from related group."""
-        states_qs = self.states.all()
-        if states_qs.exists():
-            states = list(states_qs)
-            non_compliant_users = self.group.user_set.exclude(profile__state__in=states)
-            for user in non_compliant_users:
-                self.group.user_set.remove(user)
-
 
 class ReservedGroupName(models.Model):
     """Name that can not be used for groups.

allianceauth/groupmanagement/tasks.py

@@ -1,10 +0,0 @@
-from celery import shared_task
-
-from django.contrib.auth.models import Group
-
-
-@shared_task
-def remove_users_not_matching_states_from_group(group_pk: int) -> None:
-    """Remove users not matching defined states from related group."""
-    group = Group.objects.get(pk=group_pk)
-    group.authgroup.remove_users_not_matching_states()

allianceauth/groupmanagement/tests/test_admin.py

@@ -6,7 +6,7 @@ from django.conf import settings
 from django.contrib import admin
 from django.contrib.admin.sites import AdminSite
 from django.contrib.auth.models import User
-from django.test import TestCase, RequestFactory, Client, override_settings
+from django.test import TestCase, RequestFactory, Client
 
 from allianceauth.authentication.models import CharacterOwnership, State
 from allianceauth.eveonline.models import (
@@ -236,104 +236,60 @@ class TestGroupAdmin(TestCase):
         self.assertEqual(result, expected)
 
     def test_member_count(self):
-        # given
-        request = MockRequest(user=self.user_1)
-        obj = self.modeladmin.get_queryset(request).get(pk=self.group_1.pk)
-        # when
+        expected = 1
+        obj = self.modeladmin.get_queryset(MockRequest(user=self.user_1))\
+            .get(pk=self.group_1.pk)
         result = self.modeladmin._member_count(obj)
-        # then
-        self.assertEqual(result, 1)
+        self.assertEqual(result, expected)
 
     def test_has_leader_user(self):
-        # given
-        request = MockRequest(user=self.user_1)
-        obj = self.modeladmin.get_queryset(request).get(pk=self.group_1.pk)
-        # when
-        result = self.modeladmin.has_leader(obj)
-        # then
+        result = self.modeladmin.has_leader(self.group_1)
         self.assertTrue(result)
 
     def test_has_leader_group(self):
-        # given
-        request = MockRequest(user=self.user_1)
-        obj = self.modeladmin.get_queryset(request).get(pk=self.group_2.pk)
-        # when
-        result = self.modeladmin.has_leader(obj)
-        # then
+        result = self.modeladmin.has_leader(self.group_2)
         self.assertTrue(result)
 
     def test_properties_1(self):
-        # given
-        request = MockRequest(user=self.user_1)
-        obj = self.modeladmin.get_queryset(request).get(pk=self.group_1.pk)
-        # when
-        result = self.modeladmin._properties(obj)
-        self.assertListEqual(result, ['Default'])
+        expected = ['Default']
+        result = self.modeladmin._properties(self.group_1)
+        self.assertListEqual(result, expected)
 
     def test_properties_2(self):
-        # given
-        request = MockRequest(user=self.user_1)
-        obj = self.modeladmin.get_queryset(request).get(pk=self.group_2.pk)
-        # when
-        result = self.modeladmin._properties(obj)
-        self.assertListEqual(result, ['Internal'])
+        expected = ['Internal']
+        result = self.modeladmin._properties(self.group_2)
+        self.assertListEqual(result, expected)
 
     def test_properties_3(self):
-        # given
-        request = MockRequest(user=self.user_1)
-        obj = self.modeladmin.get_queryset(request).get(pk=self.group_3.pk)
-        # when
-        result = self.modeladmin._properties(obj)
-        self.assertListEqual(result, ['Hidden'])
+        expected = ['Hidden']
+        result = self.modeladmin._properties(self.group_3)
+        self.assertListEqual(result, expected)
 
     def test_properties_4(self):
-        # given
-        request = MockRequest(user=self.user_1)
-        obj = self.modeladmin.get_queryset(request).get(pk=self.group_4.pk)
-        # when
-        result = self.modeladmin._properties(obj)
-        self.assertListEqual(result, ['Open'])
+        expected = ['Open']
+        result = self.modeladmin._properties(self.group_4)
+        self.assertListEqual(result, expected)
 
     def test_properties_5(self):
-        # given
-        request = MockRequest(user=self.user_1)
-        obj = self.modeladmin.get_queryset(request).get(pk=self.group_5.pk)
-        # when
-        result = self.modeladmin._properties(obj)
-        self.assertListEqual(result, ['Public'])
+        expected = ['Public']
+        result = self.modeladmin._properties(self.group_5)
+        self.assertListEqual(result, expected)
 
     def test_properties_6(self):
-        # given
-        request = MockRequest(user=self.user_1)
-        obj = self.modeladmin.get_queryset(request).get(pk=self.group_6.pk)
-        # when
-        result = self.modeladmin._properties(obj)
-        self.assertListEqual(result, ['Hidden', 'Open', 'Public'])
+        expected = ['Hidden', 'Open', 'Public']
+        result = self.modeladmin._properties(self.group_6)
+        self.assertListEqual(result, expected)
 
     if _has_auto_groups:
         @patch(MODULE_PATH + '._has_auto_groups', True)
-        def test_should_show_autogroup_for_corporation(self):
-            # given
+        def test_properties_7(self):
             self._create_autogroups()
-            request = MockRequest(user=self.user_1)
-            queryset = self.modeladmin.get_queryset(request)
-            obj = queryset.filter(managedcorpgroup__isnull=False).first()
-            # when
-            result = self.modeladmin._properties(obj)
-            # then
-            self.assertListEqual(result, ['Auto Group'])
-
-        @patch(MODULE_PATH + '._has_auto_groups', True)
-        def test_should_show_autogroup_for_alliance(self):
-            # given
-            self._create_autogroups()
-            request = MockRequest(user=self.user_1)
-            queryset = self.modeladmin.get_queryset(request)
-            obj = queryset.filter(managedalliancegroup__isnull=False).first()
-            # when
-            result = self.modeladmin._properties(obj)
-            # then
-            self.assertListEqual(result, ['Auto Group'])
+            expected = ['Auto Group']
+            my_group = Group.objects\
+                .filter(managedcorpgroup__isnull=False)\
+                .first()
+            result = self.modeladmin._properties(my_group)
+            self.assertListEqual(result, expected)
 
     # actions
 
@@ -583,68 +539,6 @@ class TestGroupAdminChangeFormSuperuserExclusiveEdits(WebTest):
                 self.assertNotIn(field, form.fields)
 
 
-@override_settings(CELERY_ALWAYS_EAGER=True, CELERY_EAGER_PROPAGATES_EXCEPTIONS=True)
-class TestGroupAdmin2(TestCase):
-    @classmethod
-    def setUpClass(cls):
-        super().setUpClass()
-        cls.superuser = User.objects.create_superuser("super")
-
-    def test_should_remove_users_from_state_groups(self):
-        # given
-        user_member = AuthUtils.create_user("Bruce Wayne")
-        character_member = AuthUtils.add_main_character_2(
-            user_member,
-            name="Bruce Wayne",
-            character_id=1001,
-            corp_id=2001,
-            corp_name="Wayne Technologies",
-        )
-        user_guest = AuthUtils.create_user("Lex Luthor")
-        AuthUtils.add_main_character_2(
-            user_guest,
-            name="Lex Luthor",
-            character_id=1011,
-            corp_id=2011,
-            corp_name="Luthor Corp",
-        )
-        member_state = AuthUtils.get_member_state()
-        member_state.member_characters.add(character_member)
-        user_member.refresh_from_db()
-        user_guest.refresh_from_db()
-        group = Group.objects.create(name="dummy")
-        user_member.groups.add(group)
-        user_guest.groups.add(group)
-        group.authgroup.states.add(member_state)
-        self.client.force_login(self.superuser)
-        # when
-        response = self.client.post(
-            f"/admin/groupmanagement/group/{group.pk}/change/",
-            data={
-                "name": f"{group.name}",
-                "authgroup-TOTAL_FORMS": "1",
-                "authgroup-INITIAL_FORMS": "1",
-                "authgroup-MIN_NUM_FORMS": "0",
-                "authgroup-MAX_NUM_FORMS": "1",
-                "authgroup-0-description": "",
-                "authgroup-0-states": f"{member_state.pk}",
-                "authgroup-0-internal": "on",
-                "authgroup-0-hidden": "on",
-                "authgroup-0-group": f"{group.pk}",
-                "authgroup-__prefix__-description": "",
-                "authgroup-__prefix__-internal": "on",
-                "authgroup-__prefix__-hidden": "on",
-                "authgroup-__prefix__-group": f"{group.pk}",
-                "_save": "Save"
-            }
-        )
-        # then
-        self.assertEqual(response.status_code, 302)
-        self.assertEqual(response.url, "/admin/groupmanagement/group/")
-        self.assertIn(group, user_member.groups.all())
-        self.assertNotIn(group, user_guest.groups.all())
-
-
 class TestReservedGroupNameAdmin(TestCase):
     @classmethod
     def setUpClass(cls):

allianceauth/groupmanagement/tests/test_models.py

@@ -232,38 +232,6 @@ class TestAuthGroup(TestCase):
         expected = 'Superheros'
         self.assertEqual(str(group.authgroup), expected)
 
-    def test_should_remove_guests_from_group_when_restricted_to_members_only(self):
-        # given
-        user_member = AuthUtils.create_user("Bruce Wayne")
-        character_member = AuthUtils.add_main_character_2(
-            user_member,
-            name="Bruce Wayne",
-            character_id=1001,
-            corp_id=2001,
-            corp_name="Wayne Technologies",
-        )
-        user_guest = AuthUtils.create_user("Lex Luthor")
-        AuthUtils.add_main_character_2(
-            user_guest,
-            name="Lex Luthor",
-            character_id=1011,
-            corp_id=2011,
-            corp_name="Luthor Corp",
-        )
-        member_state = AuthUtils.get_member_state()
-        member_state.member_characters.add(character_member)
-        user_member.refresh_from_db()
-        user_guest.refresh_from_db()
-        group = Group.objects.create(name="dummy")
-        user_member.groups.add(group)
-        user_guest.groups.add(group)
-        group.authgroup.states.add(member_state)
-        # when
-        group.authgroup.remove_users_not_matching_states()
-        # then
-        self.assertIn(group, user_member.groups.all())
-        self.assertNotIn(group, user_guest.groups.all())
-
 
 class TestAuthGroupRequestApprovers(TestCase):
     def setUp(self) -> None:

allianceauth/groupmanagement/urls.py

@@ -1,51 +1,50 @@
+from django.urls import path
 from . import views
 
-from django.conf.urls import url
-
 app_name = "groupmanagement"
 
 urlpatterns = [
     # groups
-    url(r"^groups/$", views.groups_view, name="groups"),
-    url(r"^group/request/join/(\w+)/$", views.group_request_add, name="request_add"),
-    url(
-        r"^group/request/leave/(\w+)/$", views.group_request_leave, name="request_leave"
+    path("groups", views.groups_view, name="groups"),
+    path("group/request/join/<int:group_id>/", views.group_request_add, name="request_add"),
+    path(
+        "group/request/leave/<int:group_id>/", views.group_request_leave, name="request_leave"
     ),
     # group management
-    url(r"^groupmanagement/requests/$", views.group_management, name="management"),
-    url(r"^groupmanagement/membership/$", views.group_membership, name="membership"),
-    url(
-        r"^groupmanagement/membership/(\w+)/$",
+    path("groupmanagement/requests/", views.group_management, name="management"),
+    path("groupmanagement/membership/", views.group_membership, name="membership"),
+    path(
+        "groupmanagement/membership/<int:group_id>/",
         views.group_membership_list,
         name="membership",
     ),
-    url(
-        r"^groupmanagement/membership/(\w+)/audit-log/$",
+    path(
+        "groupmanagement/membership/<int:group_id>/audit-log/",
         views.group_membership_audit,
         name="audit_log",
     ),
-    url(
-        r"^groupmanagement/membership/(\w+)/remove/(\w+)/$",
+    path(
+        "groupmanagement/membership/<int:group_id>/remove/<int:user_id>/",
         views.group_membership_remove,
         name="membership_remove",
     ),
-    url(
-        r"^groupmanagement/request/join/accept/(\w+)/$",
+    path(
+        "groupmanagement/request/join/accept/<int:group_request_id>/",
         views.group_accept_request,
         name="accept_request",
     ),
-    url(
-        r"^groupmanagement/request/join/reject/(\w+)/$",
+    path(
+        "groupmanagement/request/join/reject/<int:group_request_id>/",
         views.group_reject_request,
         name="reject_request",
     ),
-    url(
-        r"^groupmanagement/request/leave/accept/(\w+)/$",
+    path(
+        "groupmanagement/request/leave/accept/<int:group_request_id>/",
         views.group_leave_accept_request,
         name="leave_accept_request",
     ),
-    url(
-        r"^groupmanagement/request/leave/reject/(\w+)/$",
+    path(
+        "groupmanagement/request/leave/reject/<int:group_request_id>/",
         views.group_leave_reject_request,
         name="leave_reject_request",
     ),

allianceauth/groupmanagement/views.py

@@ -9,7 +9,7 @@ from django.core.exceptions import ObjectDoesNotExist, PermissionDenied
 from django.db.models import Count
 from django.http import Http404
 from django.shortcuts import render, redirect, get_object_or_404
-from django.utils.translation import ugettext_lazy as _
+from django.utils.translation import gettext_lazy as _
 
 from allianceauth.notifications import notify
 

allianceauth/hrapplications/__init__.py

@@ -1 +0,0 @@
-default_app_config = 'allianceauth.hrapplications.apps.HRApplicationsConfig'

allianceauth/hrapplications/auth_hooks.py

@@ -1,4 +1,4 @@
-from django.utils.translation import ugettext_lazy as _
+from django.utils.translation import gettext_lazy as _
 
 from allianceauth import hooks
 from allianceauth.services.hooks import MenuItemHook, UrlHook

allianceauth/hrapplications/forms.py

@@ -1,5 +1,5 @@
 from django import forms
-from django.utils.translation import ugettext_lazy as _
+from django.utils.translation import gettext_lazy as _
 
 
 class HRApplicationCommentForm(forms.Form):

allianceauth/hrapplications/templates/hrapplications/create.html

@@ -19,8 +19,8 @@
                                 <div cass="text-center">{{ question.help_text }}</div>
                             {% endif %}
                             {% for choice in question.choices.all %}
-                                <input type={% if question.multi_select == False %}"radio"{% else %}"checkbox"{% endif %} name="{{ question.pk }}" id="id_{{ question.pk }}" value="{{ choice.choice_text }}" />
-                                <label for="choice{{ forloop.counter }}">{{ choice.choice_text }}</label><br />
+                                <input type={% if question.multi_select == False %}"radio"{% else %}"checkbox"{% endif %} name="{{ question.pk }}" id="id_{{ question.pk }}" value="{{ choice.choice_text }}">
+                                <label for="choice{{ forloop.counter }}">{{ choice.choice_text }}</label><br>
                             {% empty %}
                                 <textarea class="form-control" cols="30" id="id_{{ question.pk }}" name="{{ question.pk }}" rows="4"></textarea>
                             {% endfor %}

allianceauth/hrapplications/templates/hrapplications/management.html

@@ -181,7 +181,7 @@
                         <form class="form-signin" role="form" action={% url 'hrapplications:search' %} method="POST">
                             {% csrf_token %}
                             {{ search_form|bootstrap }}
-                            <br/>
+                            <br>
                             <button class="btn btn-lg btn-primary btn-block" type="submit">{% translate "Search" %}</button>
                         </form>
                     </div>

allianceauth/hrapplications/templates/hrapplications/searchview.html

@@ -67,7 +67,7 @@
                         <form class="form-signin" role="form" action={% url 'hrapplications:search' %} method="POST">
                             {% csrf_token %}
                             {{ search_form|bootstrap }}
-                            <br/>
+                            <br>
                             <button class="btn btn-lg btn-primary btn-block" type="submit">{% translate "Search" %}</button>
                         </form>
                     </div>

allianceauth/hrapplications/templates/hrapplications/view.html

@@ -140,7 +140,7 @@
                         <form class="form-signin" role="form" action="" method="POST">
                             {% csrf_token %}
                             {{ comment_form|bootstrap }}
-                            <br/>
+                            <br>
                             <button class="btn btn-lg btn-primary btn-block" type="submit">{% translate "Add Comment" %}</button>
                         </form>
                     </div>

allianceauth/hrapplications/urls.py

@@ -1,31 +1,31 @@
-from django.conf.urls import url
+from django.urls import path
 
 from . import views
 
 app_name = 'hrapplications'
 
 urlpatterns = [
-    url(r'^$', views.hr_application_management_view,
+    path('', views.hr_application_management_view,
         name="index"),
-    url(r'^create/$', views.hr_application_create_view,
+    path('create/', views.hr_application_create_view,
         name="create_view"),
-    url(r'^create/(\d+)', views.hr_application_create_view,
+    path('create/<int:form_id>/', views.hr_application_create_view,
         name="create_view"),
-    url(r'^remove/(\w+)', views.hr_application_remove,
+    path('remove/<int:app_id>/', views.hr_application_remove,
         name="remove"),
-    url(r'^view/(\w+)', views.hr_application_view,
+    path('view/<int:app_id>/', views.hr_application_view,
         name="view"),
-    url(r'^personal/view/(\w+)', views.hr_application_personal_view,
+    path('personal/view/<int:app_id>/', views.hr_application_personal_view,
         name="personal_view"),
-    url(r'^personal/removal/(\w+)',
+    path('personal/removal/<int:app_id>/',
         views.hr_application_personal_removal,
         name="personal_removal"),
-    url(r'^approve/(\w+)', views.hr_application_approve,
+    path('approve/<int:app_id>/', views.hr_application_approve,
         name="approve"),
-    url(r'^reject/(\w+)', views.hr_application_reject,
+    path('reject/<int:app_id>/', views.hr_application_reject,
         name="reject"),
-    url(r'^search/', views.hr_application_search,
+    path('search/', views.hr_application_search,
         name="search"),
-    url(r'^mark_in_progress/(\w+)', views.hr_application_mark_in_progress,
+    path('mark_in_progress/<int:app_id>/', views.hr_application_mark_in_progress,
         name="mark_in_progress"),
-    ]
+]

allianceauth/notifications/urls.py

@@ -1,16 +1,16 @@
-from django.conf.urls import url
+from django.urls import path
 from . import views
 
 app_name = 'notifications'
 # Notifications
 urlpatterns = [
-    url(r'^remove_notifications/(\w+)/$', views.remove_notification, name='remove'),
-    url(r'^notifications/mark_all_read/$', views.mark_all_read, name='mark_all_read'),
-    url(r'^notifications/delete_all_read/$', views.delete_all_read, name='delete_all_read'),
-    url(r'^notifications/$', views.notification_list, name='list'),
-    url(r'^notifications/(\w+)/$', views.notification_view, name='view'),
-    url(
-        r'^user_notifications_count/(?P<user_pk>\d+)/$',
+    path('remove_notifications/<int:notif_id>/', views.remove_notification, name='remove'),
+    path('notifications/mark_all_read/', views.mark_all_read, name='mark_all_read'),
+    path('notifications/delete_all_read/', views.delete_all_read, name='delete_all_read'),
+    path('notifications/', views.notification_list, name='list'),
+    path('notifications/<int:notif_id>/', views.notification_view, name='view'),
+    path(
+        'user_notifications_count/<int:user_pk>/',
         views.user_notifications_count,
         name='user_notifications_count'
     ),

allianceauth/optimer/__init__.py

@@ -1 +0,0 @@
-default_app_config = 'allianceauth.optimer.apps.OptimerConfig'

allianceauth/optimer/auth_hooks.py

@@ -1,5 +1,5 @@
 from allianceauth.services.hooks import MenuItemHook, UrlHook
-from django.utils.translation import ugettext_lazy as _
+from django.utils.translation import gettext_lazy as _
 from allianceauth import hooks
 from . import urls
 

allianceauth/optimer/form.py

@@ -1,5 +1,5 @@
 from django import forms
-from django.utils.translation import ugettext_lazy as _
+from django.utils.translation import gettext_lazy as _
 
 from allianceauth.optimer.form_widgets import DataListWidget
 

allianceauth/optimer/templates/optimer/add.html

@@ -19,7 +19,7 @@
                         <form class="form-signin" role="form" action="" method="POST">
                             {% csrf_token %}
                             {{ form|bootstrap }}
-                            <br/>
+                            <br>
                             <button class="btn btn-lg btn-primary btn-block" type="submit">{% translate "Create Fleet Operation" %}</button>
                         </form>
                 </div>

allianceauth/optimer/templates/optimer/management.html

@@ -21,7 +21,7 @@
                 <b>{% translate "Current Eve Time:" %} </b>
             </div>
             <strong class="label label-info text-left" id="current-time"></strong>
-            <br  />
+            <br>
         </div>
 
         <h4><b>{% translate "Next Fleet Operations" %}</b></h4>

allianceauth/optimer/templates/optimer/update.html

@@ -24,7 +24,7 @@
                         <form class="form-signin" role="form" action="" method="POST">
                             {% csrf_token %}
                             {{ form|bootstrap }}
-                            <br/>
+                            <br>
                             <button class="btn btn-lg btn-primary btn-block" type="submit">{% translate "Update Fleet Operation" %}
                             </button>
                         </form>

allianceauth/optimer/urls.py

@@ -1,12 +1,12 @@
-from django.conf.urls import url
+from django.urls import path
 
 from . import views
 
 app_name = 'optimer'
 
 urlpatterns = [
-    url(r'^$', views.optimer_view, name='view'),
-    url(r'^add$', views.add_optimer_view, name='add'),
-    url(r'^(\w+)/remove$', views.remove_optimer, name='remove'),
-    url(r'^(\w+)/edit$', views.edit_optimer, name='edit'),
-    ]
+    path('', views.optimer_view, name='view'),
+    path('add/', views.add_optimer_view, name='add'),
+    path('<int:optimer_id>/remove/', views.remove_optimer, name='remove'),
+    path('<int:optimer_id>/edit/', views.edit_optimer, name='edit'),
+]

allianceauth/optimer/views.py

@@ -6,7 +6,7 @@ from django.contrib.auth.decorators import permission_required
 from django.shortcuts import get_object_or_404
 from django.shortcuts import render, redirect
 from django.utils import timezone
-from django.utils.translation import ugettext_lazy as _
+from django.utils.translation import gettext_lazy as _
 from .form import OpForm
 
 from .models import OpTimer, OpTimerType

allianceauth/permissions_tool/__init__.py

@@ -1 +0,0 @@
-default_app_config = 'allianceauth.permissions_tool.apps.PermissionsToolConfig'

allianceauth/permissions_tool/urls.py

@@ -1,11 +1,12 @@
-from django.conf.urls import url
+from django.urls import re_path
+from django.urls import path
 
 from . import views
 
 app_name = 'permissions_tool'
 
 urlpatterns = [
-    url(r'^overview/$', views.permissions_overview, name='overview'),
-    url(r'^audit/(?P<app_label>[\w\-_]+)/(?P<model>[\w\-_]+)/(?P<codename>[\w\-_]+)/$', views.permissions_audit,
+    path('overview/', views.permissions_overview, name='overview'),
+    re_path(r'^audit/(?P<app_label>[\w\-_]+)/(?P<model>[\w\-_]+)/(?P<codename>[\w\-_]+)/$', views.permissions_audit,
         name='audit'),
 ]

allianceauth/project_template/project_name/settings/base.py

@@ -68,6 +68,7 @@ BASE_DIR = os.path.dirname(PROJECT_DIR)
 MIDDLEWARE = [
     'django.middleware.security.SecurityMiddleware',
     'django.contrib.sessions.middleware.SessionMiddleware',
+    'allianceauth.authentication.middleware.UserSettingsMiddleware',
     'django.middleware.locale.LocaleMiddleware',
     'django.middleware.common.CommonMiddleware',
     'django.middleware.csrf.CsrfViewMiddleware',
@@ -153,8 +154,6 @@ TIME_ZONE = 'UTC'
 
 USE_I18N = True
 
-USE_L10N = True
-
 USE_TZ = True
 
 # Static files (CSS, JavaScript, Images)
@@ -172,11 +171,8 @@ MESSAGE_TAGS = {
 
 CACHES = {
     "default": {
-        "BACKEND": "redis_cache.RedisCache",
-        "LOCATION": "localhost:6379",
-        "OPTIONS": {
-            "DB": 1,
-        }
+        "BACKEND": "django_redis.cache.RedisCache",
+        "LOCATION": "redis://127.0.0.1:6379/1" # change the 1 here to change the database used
     }
 }
 

allianceauth/project_template/project_name/settings/local.py

@@ -61,6 +61,13 @@ EMAIL_HOST_PASSWORD = ''
 EMAIL_USE_TLS = True
 DEFAULT_FROM_EMAIL = ''
 
+# Cache compression can help on bigger auths where ram starts to become an issue.
+# Uncomment the following 3 lines to enable.
+
+#CACHES["default"]["OPTIONS"] = {
+#    "COMPRESSOR": "django_redis.compressors.lzma.LzmaCompressor",
+#}
+
 #######################################
 # Add any custom settings below here. #
 #######################################

allianceauth/project_template/project_name/urls.py

@@ -1,8 +1,9 @@
-from django.conf.urls import include, url
+from django.conf.urls import include
 from allianceauth import urls
+from django.urls import re_path
 
 urlpatterns = [
-    url(r'', include(urls)),
+    re_path(r'', include(urls)),
 ]
 
 handler500 = 'allianceauth.views.Generic500Redirect'

allianceauth/services/__init__.py

@@ -1 +0,0 @@
-default_app_config = 'allianceauth.services.apps.ServicesConfig'

allianceauth/services/admin.py

@@ -3,11 +3,11 @@ from django.contrib import admin
 
 from allianceauth import hooks
 from allianceauth.authentication.admin import (
-    MainAllianceFilter,
-    MainCorporationsFilter,
-    user_main_organization,
     user_profile_pic,
     user_username,
+    user_main_organization,
+    MainCorporationsFilter,
+    MainAllianceFilter
 )
 
 from .models import NameFormatConfig
@@ -36,18 +36,19 @@ class ServicesUserAdmin(admin.ModelAdmin):
         MainAllianceFilter,
         'user__date_joined',
     )
-    list_select_related = (
-        'user', 'user__profile__main_character', 'user__profile__state'
-    )
 
-    @admin.display(ordering='user__profile__state__name')
     def _state(self, obj):
         return obj.user.profile.state.name
 
-    @admin.display(ordering='user__date_joined')
+    _state.short_description = 'state'
+    _state.admin_order_field = 'user__profile__state__name'
+
     def _date_joined(self, obj):
         return obj.user.date_joined
 
+    _date_joined.short_description = 'date joined'
+    _date_joined.admin_order_field = 'user__date_joined'
+
 
 class NameFormatConfigForm(forms.ModelForm):
     def __init__(self, *args, **kwargs):
@@ -61,7 +62,6 @@ class NameFormatConfigForm(forms.ModelForm):
         self.fields['service_name'] = forms.ChoiceField(choices=SERVICE_CHOICES)
 
 
-@admin.register(NameFormatConfig)
 class NameFormatConfigAdmin(admin.ModelAdmin):
     form = NameFormatConfigForm
     list_display = ('service_name', 'get_state_display_string')
@@ -69,3 +69,6 @@ class NameFormatConfigAdmin(admin.ModelAdmin):
     def get_state_display_string(self, obj):
         return ', '.join([state.name for state in obj.states.all()])
     get_state_display_string.short_description = 'States'
+
+
+admin.site.register(NameFormatConfig, NameFormatConfigAdmin)

allianceauth/services/forms.py

@@ -1,5 +1,5 @@
 from django import forms
-from django.utils.translation import ugettext_lazy as _
+from django.utils.translation import gettext_lazy as _
 
 
 class FleetFormatterForm(forms.Form):

allianceauth/services/hooks.py

@@ -1,4 +1,5 @@
-from django.conf.urls import include, url
+from django.conf.urls import include
+from django.urls import re_path
 from django.core.exceptions import ObjectDoesNotExist
 from django.template.loader import render_to_string
 from django.utils.functional import cached_property
@@ -9,7 +10,6 @@ from allianceauth.hooks import get_hooks
 
 from .models import NameFormatConfig
 
-
 def get_extension_logger(name):
     """
     Takes the name of a plugin/extension and generates a child logger of the extensions logger
@@ -157,7 +157,7 @@ class MenuItemHook:
 
 class UrlHook:
     def __init__(self, urls, namespace, base_url):
-        self.include_pattern = url(base_url, include(urls, namespace=namespace))
+        self.include_pattern = re_path(base_url, include(urls, namespace=namespace))
 
 
 class NameFormatter:

allianceauth/services/modules/discord/__init__.py

@@ -1,3 +1 @@
-default_app_config = 'allianceauth.services.modules.discord.apps.DiscordServiceConfig'  # noqa
-
 __title__ = 'Discord Service'

allianceauth/services/modules/discord/admin.py

@@ -2,11 +2,12 @@ import logging
 
 from django.contrib import admin
 
-from ...admin import ServicesUserAdmin
 from . import __title__
+from ...admin import ServicesUserAdmin
 from .models import DiscordUser
 from .utils import LoggerAddTag
 
+
 logger = LoggerAddTag(logging.getLogger(__name__), __title__)
 
 
@@ -17,16 +18,21 @@ class DiscordUserAdmin(ServicesUserAdmin):
     list_filter = ServicesUserAdmin.list_filter + ('activated',)
     ordering = ('-activated',)
 
+    def _uid(self, obj):
+        return obj.uid
+
+    _uid.short_description = 'Discord ID (UID)'
+    _uid.admin_order_field = 'uid'
+
+    def _username(self, obj):
+        if obj.username and obj.discriminator:
+            return f'{obj.username}#{obj.discriminator}'
+        else:
+            return ''
+
     def delete_queryset(self, request, queryset):
         for user in queryset:
             user.delete_user()
 
-    @admin.display(description='Discord ID (UID)', ordering='uid')
-    def _uid(self, obj):
-        return obj.uid
-
-    @admin.display(description='Discord Username', ordering='username')
-    def _username(self, obj):
-        if obj.username and obj.discriminator:
-            return f'{obj.username}#{obj.discriminator}'
-        return ''
+    _username.short_description = 'Discord Username'
+    _username.admin_order_field = 'username'

allianceauth/services/modules/discord/api.py

@@ -1,37 +0,0 @@
-"""Public interface for community apps who want to interact with the Discord server
-of the current Alliance Auth instance.
-
-Example
-=======
-
-Here is an example for using the api to fetch the current roles from the configured Discord server.
-
-.. code-block:: python
-
-    from allianceauth.services.modules.discord.api import create_bot_client, discord_guild_id
-
-    client = create_bot_client()  # create a new Discord client
-    guild_id = discord_guild_id()  # get the ID of the configured Discord server
-    roles = client.guild_roles(guild_id)  # fetch the roles from our Discord server
-
-.. seealso::
-    The docs for the client class can be found here: :py:class:`~allianceauth.services.modules.discord.discord_client.client.DiscordClient`
-"""
-
-from typing import Optional
-
-from .app_settings import DISCORD_GUILD_ID
-from .core import create_bot_client, group_to_role, server_name  # noqa
-from .discord_client.models import Role  # noqa
-from .models import DiscordUser  # noqa
-
-__all__ = ["create_bot_client", "group_to_role", "server_name", "DiscordUser", "Role"]
-
-
-def discord_guild_id() -> Optional[int]:
-    """Guild ID of configured Discord server.
-
-    Returns:
-        Guild ID or ``None`` if not configured
-    """
-    return int(DISCORD_GUILD_ID) if DISCORD_GUILD_ID else None

allianceauth/services/modules/discord/app_settings.py

@@ -2,25 +2,16 @@ from .utils import clean_setting
 
 
 DISCORD_APP_ID = clean_setting('DISCORD_APP_ID', '')
-"""App ID for the AA bot on Discord. Needs to be set."""
-
 DISCORD_APP_SECRET = clean_setting('DISCORD_APP_SECRET', '')
-"""App secret for the AA bot on Discord. Needs to be set."""
-
 DISCORD_BOT_TOKEN = clean_setting('DISCORD_BOT_TOKEN', '')
-"""Token used by the AA bot on Discord.  Needs to be set."""
-
 DISCORD_CALLBACK_URL = clean_setting('DISCORD_CALLBACK_URL', '')
-"""Callback URL for OAuth with Discord. Needs to be set."""
-
 DISCORD_GUILD_ID = clean_setting('DISCORD_GUILD_ID', '')
-"""ID of the Discord Server. Needs to be set."""
 
+# max retries of tasks after an error occurred
 DISCORD_TASKS_MAX_RETRIES = clean_setting('DISCORD_TASKS_MAX_RETRIES', 3)
-"""Max retries of tasks after an error occurred."""
 
+# Pause in seconds until next retry for tasks after the API returned an error
 DISCORD_TASKS_RETRY_PAUSE = clean_setting('DISCORD_TASKS_RETRY_PAUSE', 60)
-"""Pause in seconds until next retry for tasks after the API returned an error."""
 
+# automatically sync Discord users names to user's main character name when created
 DISCORD_SYNC_NAMES = clean_setting('DISCORD_SYNC_NAMES', False)
-"""Automatically sync Discord users names to user's main character name when created."""

allianceauth/services/modules/discord/auth_hooks.py

@@ -6,7 +6,6 @@ from django.template.loader import render_to_string
 from allianceauth import hooks
 from allianceauth.services.hooks import ServicesHook
 
-from .core import server_name, user_formatted_nick
 from .models import DiscordUser
 from .urls import urlpatterns
 from .utils import LoggerAddTag
@@ -54,7 +53,7 @@ class DiscordService(ServicesHook):
         return render_to_string(
             self.service_ctrl_template,
             {
-                'server_name': server_name(),
+                'server_name': DiscordUser.objects.server_name(),
                 'user_has_account': user_has_account,
                 'discord_username': discord_username
             },
@@ -74,7 +73,7 @@ class DiscordService(ServicesHook):
                     'user_pk': user.pk,
                     # since the new nickname is not yet in the DB we need to
                     # provide it manually to the task
-                    'nickname': user_formatted_nick(user)
+                    'nickname': DiscordUser.objects.user_formatted_nick(user)
                 },
                 priority=SINGLE_TASK_PRIORITY
             )

allianceauth/services/modules/discord/core.py

@@ -1,129 +0,0 @@
-"""Core functionality of the Discord service not directly related to models."""
-
-import logging
-from typing import List, Optional, Tuple
-
-from requests.exceptions import HTTPError
-
-from django.contrib.auth.models import Group, User
-
-from allianceauth.groupmanagement.models import ReservedGroupName
-from allianceauth.services.hooks import NameFormatter
-
-from . import __title__
-from .app_settings import DISCORD_BOT_TOKEN, DISCORD_GUILD_ID
-from .discord_client import DiscordClient, RolesSet, Role
-from .discord_client.exceptions import DiscordClientException
-from .utils import LoggerAddTag
-
-logger = LoggerAddTag(logging.getLogger(__name__), __title__)
-
-
-def create_bot_client(is_rate_limited: bool = True) -> DiscordClient:
-    """Create new bot client for accessing the configured Discord server.
-
-    Args:
-        is_rate_limited: Set to False to turn off rate limiting (use with care).
-
-    Return:
-        Discord client instance
-    """
-    return DiscordClient(DISCORD_BOT_TOKEN, is_rate_limited=is_rate_limited)
-
-
-def calculate_roles_for_user(
-    user: User,
-    client: DiscordClient,
-    discord_uid: int,
-    state_name: str = None,
-) -> Tuple[RolesSet, Optional[bool]]:
-    """Calculate current Discord roles for an Auth user.
-
-    Takes into account reserved groups and existing managed roles (e.g. nitro).
-
-    Returns:
-        - Discord roles, changed flag:
-            - True when roles have changed,
-            - False when they have not changed,
-            - None if user is not a member of the guild
-    """
-    roles_calculated = client.match_or_create_roles_from_names_2(
-        guild_id=DISCORD_GUILD_ID,
-        role_names=_user_group_names(user=user, state_name=state_name),
-    )
-    logger.debug("Calculated roles for user %s: %s", user, roles_calculated.ids())
-    roles_current = client.guild_member_roles(
-        guild_id=DISCORD_GUILD_ID, user_id=discord_uid
-    )
-    if roles_current is None:
-        logger.debug("User %s is not a member of the guild.", user)
-        return roles_calculated, None
-    logger.debug("Current roles user %s: %s", user, roles_current.ids())
-    reserved_role_names = ReservedGroupName.objects.values_list("name", flat=True)
-    roles_reserved = roles_current.subset(role_names=reserved_role_names)
-    roles_managed = roles_current.subset(managed_only=True)
-    roles_persistent = roles_managed.union(roles_reserved)
-    if roles_calculated == roles_current.difference(roles_persistent):
-        return roles_calculated, False
-    return roles_calculated.union(roles_persistent), True
-
-
-def _user_group_names(user: User, state_name: str = None) -> List[str]:
-    """Names of groups and state the given user is a member of."""
-    if not state_name:
-        state_name = user.profile.state.name
-    group_names = [group.name for group in user.groups.all()] + [state_name]
-    logger.debug("Group names for roles updates of user %s are: %s", user, group_names)
-    return group_names
-
-
-def user_formatted_nick(user: User) -> Optional[str]:
-    """Name of the given user's main character with name formatting applied.
-
-    Returns:
-        Name or ``None`` if user has no main.
-    """
-    from .auth_hooks import DiscordService
-
-    if user.profile.main_character:
-        return NameFormatter(DiscordService(), user).format_name()
-    return None
-
-
-def group_to_role(group: Group) -> Optional[Role]:
-    """Fetch the Discord role matching the given Django group by name.
-
-    Returns:
-        Discord role or None if no matching role exist
-    """
-    return default_bot_client.match_role_from_name(
-        guild_id=DISCORD_GUILD_ID, role_name=group.name
-    )
-
-
-def server_name(use_cache: bool = True) -> str:
-    """Fetches the name of the current Discord server.
-
-    Args:
-        use_cache: When set False will force an API call to get the server name
-
-    Returns:
-        Server name or an empty string if the name could not be retrieved
-    """
-    try:
-        server_name = default_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
-
-
-# Default bot client to be used by modules of this package
-default_bot_client = create_bot_client()

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

@@ -1,10 +1,3 @@
-from .app_settings import DISCORD_OAUTH_BASE_URL, DISCORD_OAUTH_TOKEN_URL  # noqa
-from .client import DiscordClient  # noqa
-from .exceptions import (  # noqa
-    DiscordApiBackoff,
-    DiscordClientException,
-    DiscordRateLimitExhausted,
-    DiscordTooManyRequestsError,
-)
-from .helpers import RolesSet  # noqa
-from .models import Guild, GuildMember, Role, User  # noqa
+from .client import DiscordClient   # noqa
+from .exceptions import DiscordApiBackoff  # noqa
+from .helpers import DiscordRoles  # noqa

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

@@ -1,56 +1,45 @@
-"""Settings for the Discord client.
-
-To overwrite a default set the variable in your local Django settings, e.g:
-
-.. code:: python
-
-    DISCORD_GUILD_NAME_CACHE_MAX_AGE = 7200
-"""
-
 from ..utils import clean_setting
 
 
+# Base URL for all API calls. Must end with /.
 DISCORD_API_BASE_URL = clean_setting(
     'DISCORD_API_BASE_URL', 'https://discord.com/api/'
 )
-"""Base URL for all API calls. Must end with /."""
 
+# Low level connecttimeout for requests to the Discord API in seconds
 DISCORD_API_TIMEOUT_CONNECT = clean_setting(
     'DISCORD_API_TIMEOUT', 5
 )
-"""Low level connect timeout for requests to the Discord API in seconds."""
 
+# Low level read timeout for requests to the Discord API in seconds
 DISCORD_API_TIMEOUT_READ = clean_setting(
     'DISCORD_API_TIMEOUT', 30
 )
-"""Low level read timeout for requests to the Discord API in seconds."""
 
+# Base authorization URL for Discord Oauth
 DISCORD_OAUTH_BASE_URL = clean_setting(
     'DISCORD_OAUTH_BASE_URL', 'https://discord.com/api/oauth2/authorize'
 )
-"""Base authorization URL for Discord Oauth."""
 
+# Base authorization URL for Discord Oauth
 DISCORD_OAUTH_TOKEN_URL = clean_setting(
     'DISCORD_OAUTH_TOKEN_URL', 'https://discord.com/api/oauth2/token'
 )
-"""Base authorization URL for Discord Oauth."""
 
+# How long the Discord guild names retrieved from the server are
+# caches locally in seconds.
 DISCORD_GUILD_NAME_CACHE_MAX_AGE = clean_setting(
     'DISCORD_GUILD_NAME_CACHE_MAX_AGE', 3600 * 24
 )
-"""How long the Discord guild names retrieved from the server
-are caches locally in seconds.
-"""
 
+# 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
 )
-"""How long Discord roles retrieved from the server are caches locally in seconds."""
 
+# Turns off creation of new roles. In case the rate limit for creating roles is
+# exhausted, this setting allows the Discord service to continue to function
+# and wait out the reset. Rate limit is about 250 per 48 hrs.
 DISCORD_DISABLE_ROLE_CREATION = clean_setting(
     'DISCORD_DISABLE_ROLE_CREATION', False
 )
-"""Turns off creation of new roles. In case the rate limit for creating roles is
-exhausted, this setting allows the Discord service to continue to function
-and wait out the reset. Rate limit is about 250 per 48 hrs.
-"""

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

@@ -1,37 +1,32 @@
-"""Client for interacting with the Discord API."""
-
+from hashlib import md5
 import json
 import logging
-from enum import IntEnum
-from hashlib import md5
-from http import HTTPStatus
 from time import sleep
-from typing import Iterable, List, Optional, Set, Tuple
 from urllib.parse import urljoin
 from uuid import uuid1
 
-import requests
-from requests.exceptions import HTTPError
 from redis import Redis
+import requests
 
-from allianceauth.utils.cache import get_redis_client
+from django_redis import get_redis_connection
 
-from allianceauth import __title__ as AUTH_TITLE
-from allianceauth import __url__, __version__
+from allianceauth import __title__ as AUTH_TITLE, __url__, __version__
 
 from .. import __title__
-from ..utils import LoggerAddTag
 from .app_settings import (
     DISCORD_API_BASE_URL,
     DISCORD_API_TIMEOUT_CONNECT,
     DISCORD_API_TIMEOUT_READ,
     DISCORD_DISABLE_ROLE_CREATION,
     DISCORD_GUILD_NAME_CACHE_MAX_AGE,
+    DISCORD_OAUTH_BASE_URL,
+    DISCORD_OAUTH_TOKEN_URL,
     DISCORD_ROLES_CACHE_MAX_AGE,
 )
 from .exceptions import DiscordRateLimitExhausted, DiscordTooManyRequestsError
-from .helpers import RolesSet
-from .models import Guild, GuildMember, Role, User
+from .helpers import DiscordRoles
+from ..utils import LoggerAddTag
+
 
 logger = LoggerAddTag(logging.getLogger(__name__), __title__)
 
@@ -63,13 +58,8 @@ MINIMUM_BLOCKING_WAIT = 50
 RATE_LIMIT_RETRIES = 1000
 
 
-class DiscordApiStatusCode(IntEnum):
-    """Status code returned from the Discord API."""
-    UNKNOWN_MEMBER = 10007  #:
-
-
 class DiscordClient:
-    """This class provides a web client for interacting with the Discord API.
+    """This class provides a web client for interacting with the Discord API
 
     The client has rate limiting that supports concurrency.
     This means it is able to ensure the API rate limit is not violated,
@@ -77,30 +67,24 @@ class DiscordClient:
 
     In addition the client support proper API backoff.
 
-    Synchronization of rate limit infos across multiple processes
+    Synchronization of rate limit infos accross multiple processes
     is implemented with Redis and thus requires Redis as Django cache backend.
 
-    The cache is shared across all clients and processes (also using Redis).
-
     All durations are in milliseconds.
-
-    Most errors from the API will raise a requests.HTTPError.
-
-    Args:
-        access_token: Discord access token used to authenticate all calls to the API
-        redis: Redis instance to be used.
-        is_rate_limited: Set to False to turn off rate limiting (use with care).
-            If not specified will try to use the Redis instance
-            from the default Django cache backend.
-
-    Raises:
-        ValueError: No access token provided
     """
+    OAUTH_BASE_URL = DISCORD_OAUTH_BASE_URL
+    OAUTH_TOKEN_URL = DISCORD_OAUTH_TOKEN_URL
+
     _KEY_GLOBAL_BACKOFF_UNTIL = 'DISCORD_GLOBAL_BACKOFF_UNTIL'
     _KEY_GLOBAL_RATE_LIMIT_REMAINING = 'DISCORD_GLOBAL_RATE_LIMIT_REMAINING'
     _KEYPREFIX_GUILD_NAME = 'DISCORD_GUILD_NAME'
     _KEYPREFIX_GUILD_ROLES = 'DISCORD_GUILD_ROLES'
     _KEYPREFIX_ROLE_NAME = 'DISCORD_ROLE_NAME'
+    _NICK_MAX_CHARS = 32
+
+    _HTTP_STATUS_CODE_NOT_FOUND = 404
+    _HTTP_STATUS_CODE_RATE_LIMITED = 429
+    _DISCORD_STATUS_CODE_UNKNOWN_MEMBER = 10007
 
     def __init__(
         self,
@@ -108,12 +92,18 @@ class DiscordClient:
         redis: Redis = None,
         is_rate_limited: bool = True
     ) -> None:
-        if not access_token:
-            raise ValueError('You must provide an access token.')
+        """
+        Params:
+        - access_token: Discord access token used to authenticate all calls to the API
+        - redis: Redis instance to be used.
+        - is_rate_limited: Set to False to run of rate limiting (use with care)
+        If not specified will try to use the Redis instance
+        from the default Django cache backend.
+        """
         self._access_token = str(access_token)
         self._is_rate_limited = bool(is_rate_limited)
         if not redis:
-            self._redis = get_redis_client()
+            self._redis = get_redis_connection("default")
             if not isinstance(self._redis, Redis):
                 raise RuntimeError(
                     'This class requires a Redis client, but none was provided '
@@ -141,20 +131,19 @@ class DiscordClient:
         self.__redis_script_set_longer = self._redis.register_script(lua_2)
 
     @property
-    def access_token(self) -> str:
-        """Discord access token."""
+    def access_token(self):
         return self._access_token
 
     @property
-    def is_rate_limited(self) -> bool:
-        """Wether this instance is rate limited."""
+    def is_rate_limited(self):
         return self._is_rate_limited
 
     def __repr__(self):
         return f'{type(self).__name__}(access_token=...{self.access_token[-5:]})'
 
     def _redis_decr_or_set(self, name: str, value: str, px: int) -> bool:
-        """Decrease the key value if it exists and returns the result else set the key.
+        """decreases the key value if it exists and returns the result
+        else sets the key
 
         Implemented as Lua script to ensure atomicity.
         """
@@ -163,7 +152,7 @@ class DiscordClient:
         )
 
     def _redis_set_if_longer(self, name: str, value: str, px: int) -> bool:
-        """Like set, but only goes through if either key doesn't exist
+        """like set, but only goes through if either key doesn't exist
         or px would be extended.
 
         Implemented as Lua script to ensure atomicity.
@@ -174,134 +163,111 @@ class DiscordClient:
 
     # users
 
-    def current_user(self) -> User:
-        """Fetch user belonging to the current access_token."""
+    def current_user(self) -> dict:
+        """returns the user belonging to the current access_token"""
         authorization = f'Bearer {self.access_token}'
         r = self._api_request(
             method='get', route='users/@me', authorization=authorization
         )
-        return User.from_dict(r.json())
+        return r.json()
 
     # guild
 
-    def guild_infos(self, guild_id: int) -> Guild:
-        """Fetch all basic infos about this guild.
-
-        Args:
-            guild_id: Discord ID of the guild
-        """
+    def guild_infos(self, guild_id: int) -> dict:
+        """Returns all basic infos about this guild"""
         route = f"guilds/{guild_id}"
         r = self._api_request(method='get', route=route)
-        return Guild.from_dict(r.json())
+        return r.json()
 
     def guild_name(self, guild_id: int, use_cache: bool = True) -> str:
-        """Fetch the name of this guild (cached).
+        """returns the name of this guild (cached)
+        or an empty string if something went wrong
 
-        Args:
-            guild_id: Discord ID of the guild
-            use_cache: When set to False will force an API call to get the server name
-
-        Returns:
-            Name of the server 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)
         if use_cache:
             guild_name = self._redis_decode(self._redis.get(key_name))
         else:
-            guild_name = ""
+            guild_name = None
         if not guild_name:
-            try:
-                guild = self.guild_infos(guild_id)
-            except HTTPError:
-                guild_name = ""
-            else:
-                guild_name = guild.name
+            guild_infos = self.guild_infos(guild_id)
+            if 'name' in guild_infos:
+                guild_name = guild_infos['name']
                 self._redis.set(
-                    name=key_name, value=guild_name, ex=DISCORD_GUILD_NAME_CACHE_MAX_AGE
+                    name=key_name,
+                    value=guild_name,
+                    ex=DISCORD_GUILD_NAME_CACHE_MAX_AGE
                 )
+            else:
+                guild_name = ''
+
         return guild_name
 
     @classmethod
     def _guild_name_cache_key(cls, guild_id: int) -> str:
-        """Construct key for accessing role given by name in the role cache.
-
-        Args:
-            guild_id: Discord ID of the guild
-        """
+        """Returns key for accessing role given by name in the role cache"""
         gen_key = DiscordClient._generate_hash(f'{guild_id}')
         return f'{cls._KEYPREFIX_GUILD_NAME}__{gen_key}'
 
     # guild roles
 
-    def guild_roles(self, guild_id: int, use_cache: bool = True) -> Set[Role]:
-        """Fetch all roles for this guild.
+    def guild_roles(self, guild_id: int, use_cache: bool = True) -> list:
+        """Returns the list of all roles for this guild
 
-        Args:
-            guild_id: Discord ID of the guild
-            use_cache: If is set to False it will always hit the API to retrieve
-                fresh data and update the cache.
-
-        Returns:
+        If use_cache is set to False it will always hit the API to retrieve
+        fresh data and update the cache
         """
         cache_key = self._guild_roles_cache_key(guild_id)
-        roles = None
         if use_cache:
             roles_raw = self._redis.get(name=cache_key)
             if roles_raw:
                 logger.debug('Returning roles for guild %s from cache', guild_id)
-                roles = json.loads(self._redis_decode(roles_raw))
-            logger.debug('No roles for guild %s in cache', guild_id)
-        if roles is None:
-            route = f"guilds/{guild_id}/roles"
-            r = self._api_request(method='get', route=route)
-            roles = r.json()
-            if not roles or not isinstance(roles, list):
-                raise RuntimeError(
-                    f"Unexpected response when fetching roles from API: {roles}"
-                )
+                return json.loads(self._redis_decode(roles_raw))
+            else:
+                logger.debug('No roles for guild %s in cache', guild_id)
+
+        route = f"guilds/{guild_id}/roles"
+        r = self._api_request(method='get', route=route)
+        roles = r.json()
+        if roles and isinstance(roles, list):
             self._redis.set(
                 name=cache_key,
                 value=json.dumps(roles),
                 ex=DISCORD_ROLES_CACHE_MAX_AGE
             )
-        return {Role.from_dict(role) for role in roles}
+        return roles
 
-    def create_guild_role(
-        self, guild_id: int, role_name: str, **kwargs
-    ) -> Optional[Role]:
+    def create_guild_role(self, guild_id: int, role_name: str, **kwargs) -> dict:
         """Create a new guild role with the given name.
-
         See official documentation for additional optional parameters.
 
         Note that Discord allows the creation of multiple roles with the same name,
         so to avoid duplicates it's important to check existing roles
         before creating new one
 
-        Args:
-            guild_id: Discord ID of the guild
-            role_name: Name of new role to create
-
-        Returns:
-            new role on success
+        returns a new role dict on success
         """
         route = f"guilds/{guild_id}/roles"
-        data = {'name': Role.sanitize_name(role_name)}
+        data = {'name': DiscordRoles.sanitize_role_name(role_name)}
         data.update(kwargs)
         r = self._api_request(method='post', route=route, data=data)
         role = r.json()
         if role:
             self._invalidate_guild_roles_cache(guild_id)
-            return Role.from_dict(role)
-        return None
+        return role
 
     def delete_guild_role(self, guild_id: int, role_id: int) -> bool:
-        """Delete a guild role."""
+        """Deletes a guild role"""
         route = f"guilds/{guild_id}/roles/{role_id}"
         r = self._api_request(method='delete', route=route)
         if r.status_code == 204:
             self._invalidate_guild_roles_cache(guild_id)
             return True
-        return False
+        else:
+            return False
 
     def _invalidate_guild_roles_cache(self, guild_id: int) -> None:
         cache_key = self._guild_roles_cache_key(guild_id)
@@ -310,79 +276,67 @@ class DiscordClient:
 
     @classmethod
     def _guild_roles_cache_key(cls, guild_id: int) -> str:
-        """Construct key for accessing cached roles for a guild.
-
-        Args:
-            guild_id: Discord ID of the guild
-        """
+        """Returns key for accessing cached roles for a guild"""
         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) -> Optional[Role]:
-        """Fetch Discord role matching the given name (cached).
-
-        Args:
-            guild_id: Discord ID of the guild
-            role_name: Name of role
-
-        Returns:
-            Matching role or None if no match is found
-        """
-        guild_roles = RolesSet(self.guild_roles(guild_id))
+    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: Iterable[str]
-    ) -> List[Tuple[Role, bool]]:
-        """Fetch or create Discord roles matching the given names (cached).
+    def match_or_create_roles_from_names(self, guild_id: int, role_names: list) -> list:
+        """returns Discord roles matching the given names
+
+        Returns as list of tuple of role and created flag
 
         Will try to match with existing roles names
         Non-existing roles will be created, then created flag will be True
 
-        Args:
-            guild_id: ID of guild
-            role_names: list of name strings each defining a role
-
-        Returns:
-            List of tuple of Role and created flag
+        Params:
+        - guild_id: ID of guild
+        - role_names: list of name strings each defining a role
         """
         roles = list()
-        guild_roles = RolesSet(self.guild_roles(guild_id))
-        role_names_cleaned = {Role.sanitize_name(name) for name in role_names}
+        guild_roles = DiscordRoles(self.guild_roles(guild_id))
+        role_names_cleaned = {
+            DiscordRoles.sanitize_role_name(name) for name in role_names
+        }
         for role_name in role_names_cleaned:
             role, created = self.match_or_create_role_from_name(
-                guild_id=guild_id, role_name=role_name, guild_roles=guild_roles
+                guild_id=guild_id,
+                role_name=DiscordRoles.sanitize_role_name(role_name),
+                guild_roles=guild_roles
             )
             if role:
                 roles.append((role, created))
             if created:
-                guild_roles = guild_roles.union(RolesSet([role]))
+                guild_roles = guild_roles.union(DiscordRoles([role]))
         return roles
 
     def match_or_create_role_from_name(
-        self, guild_id: int, role_name: str, guild_roles: RolesSet = None
-    ) -> Tuple[Role, bool]:
-        """Fetch or create Discord role matching the given name.
+        self, guild_id: int, role_name: str, guild_roles: DiscordRoles = None
+    ) -> tuple:
+        """returns Discord role matching the given name
+
+        Returns as tuple of role and created flag
 
         Will try to match with existing roles names
         Non-existing roles will be created, then created flag will be True
 
-        Args:
-            guild_id: ID of guild
-            role_name: strings defining name of a role
-            guild_roles: All known guild roles as RolesSet object.
-                Helps to void redundant lookups of guild roles
-                when this method is used multiple times.
-
-        Returns:
-            Tuple of Role and created flag
+        Params:
+        - guild_id: ID of guild
+        - role_name: strings defining name of a role
+        - guild_roles: All known guild roles as DiscordRoles object.
+        Helps to void redundant lookups of guild roles
+        when this method is used multiple times.
         """
         if not isinstance(role_name, str):
             raise TypeError('role_name must be of type string')
 
         created = False
         if guild_roles is None:
-            guild_roles = RolesSet(self.guild_roles(guild_id))
+            guild_roles = DiscordRoles(self.guild_roles(guild_id))
         role = guild_roles.role_by_name(role_name)
         if not role:
             if not DISCORD_DISABLE_ROLE_CREATION:
@@ -391,24 +345,9 @@ class DiscordClient:
                 created = True
             else:
                 role = None
+
         return role, created
 
-    def match_or_create_roles_from_names_2(
-        self, guild_id: int, role_names: Iterable[str]
-    ) -> RolesSet:
-        """Fetch or create Discord role matching the given name.
-
-        Wrapper for ``match_or_create_role_from_name()``
-
-        Returns:
-            Roles as RolesSet object.
-        """
-        return RolesSet.create_from_matched_roles(
-            self.match_or_create_roles_from_names(
-                guild_id=guild_id, role_names=role_names
-            )
-        )
-
     # guild members
 
     def add_guild_member(
@@ -418,13 +357,13 @@ class DiscordClient:
         access_token: str,
         role_ids: list = None,
         nick: str = None
-    ) -> Optional[bool]:
-        """Adds a user to the guild.
+    ) -> bool:
+        """Adds a user to the guilds.
 
         Returns:
-            - True when a new user was added
-            - None if the user already existed
-            - False when something went wrong or raises exception
+        - True when a new user was added
+        - None if the user already existed
+        - False when something went wrong or raises exception
         """
         route = f"guilds/{guild_id}/members/{user_id}"
         data = {
@@ -432,49 +371,42 @@ class DiscordClient:
         }
         if role_ids:
             data['roles'] = self._sanitize_role_ids(role_ids)
+
         if nick:
-            data['nick'] = GuildMember.sanitize_nick(nick)
+            data['nick'] = str(nick)[:self._NICK_MAX_CHARS]
+
         r = self._api_request(method='put', route=route, data=data)
         r.raise_for_status()
         if r.status_code == 201:
             return True
         elif r.status_code == 204:
             return None
-        return False
+        else:
+            return False
 
-    def guild_member(self, guild_id: int, user_id: int) -> Optional[GuildMember]:
-        """Fetch info for a guild member.
+    def guild_member(self, guild_id: int, user_id: int) -> dict:
+        """returns the user info for a guild member
 
-        Args:
-            guild_id: Discord ID of the guild
-            user_id: Discord ID of the user
-
-        Returns:
-            guild member or ``None`` if the user is not a member of the guild
+        or None if the user is not a member of the guild
         """
         route = f'guilds/{guild_id}/members/{user_id}'
         r = self._api_request(method='get', route=route, raise_for_status=False)
         if self._is_member_unknown_error(r):
             logger.warning("Discord user ID %s could not be found on server.", user_id)
             return None
-        r.raise_for_status()
-        return GuildMember.from_dict(r.json())
+        else:
+            r.raise_for_status()
+            return r.json()
 
     def modify_guild_member(
-        self, guild_id: int, user_id: int, role_ids: List[int] = None, nick: str = None
-    ) -> Optional[bool]:
-        """Set properties of a guild member.
-
-        Args:
-            guild_id: Discord ID of the guild
-            user_id: Discord ID of the user
-            roles_id: New list of role IDs (if provided)
-            nick: New nickname (if provided)
+        self, guild_id: int, user_id: int, role_ids: list = None, nick: str = None
+    ) -> bool:
+        """Modify attributes of a guild member.
 
         Returns
-            - True when successful
-            - None if user is not a member of this guild
-            - False otherwise
+        - True when successful
+        - None if user is not a member of this guild
+        - False otherwise
         """
         if not role_ids and not nick:
             raise ValueError('Must specify role_ids or nick')
@@ -487,7 +419,7 @@ class DiscordClient:
             data['roles'] = self._sanitize_role_ids(role_ids)
 
         if nick:
-            data['nick'] = GuildMember.sanitize_nick(nick)
+            data['nick'] = self._sanitize_nick(nick)
 
         route = f"guilds/{guild_id}/members/{user_id}"
         r = self._api_request(
@@ -496,22 +428,21 @@ class DiscordClient:
         if self._is_member_unknown_error(r):
             logger.warning('User ID %s is not a member of this guild', user_id)
             return None
-        r.raise_for_status()
+        else:
+            r.raise_for_status()
+
         if r.status_code == 204:
             return True
-        return False
+        else:
+            return False
 
-    def remove_guild_member(self, guild_id: int, user_id: int) -> Optional[bool]:
-        """Remove a member from a guild.
-
-        Args:
-            guild_id: Discord ID of the guild
-            user_id: Discord ID of the user
+    def remove_guild_member(self, guild_id: int, user_id: int) -> bool:
+        """Remove a member from a guild
 
         Returns:
-            - True when successful
-            - None if member does not exist
-            - False otherwise
+        - True when successful
+        - None if member does not exist
+        - False otherwise
         """
         route = f"guilds/{guild_id}/members/{user_id}"
         r = self._api_request(
@@ -520,16 +451,19 @@ class DiscordClient:
         if self._is_member_unknown_error(r):
             logger.warning('User ID %s is not a member of this guild', user_id)
             return None
-        r.raise_for_status()
+        else:
+            r.raise_for_status()
+
         if r.status_code == 204:
             return True
-        return False
+        else:
+            return False
 
     # Guild member roles
 
     def add_guild_member_role(
         self, guild_id: int, user_id: int, role_id: int
-    ) -> Optional[bool]:
+    ) -> bool:
         """Adds a role to a guild member
 
         Returns:
@@ -542,69 +476,43 @@ class DiscordClient:
         if self._is_member_unknown_error(r):
             logger.warning('User ID %s is not a member of this guild', user_id)
             return None
-        r.raise_for_status()
+        else:
+            r.raise_for_status()
+
         if r.status_code == 204:
             return True
-        return False
+        else:
+            return False
 
     def remove_guild_member_role(
         self, guild_id: int, user_id: int, role_id: int
-    ) -> Optional[bool]:
-        """Remove a role to a guild member
-
-        Args:
-            guild_id: Discord ID of the guild
-            user_id: Discord ID of the user
-            role_id: Discord ID of role to be removed
+    ) -> bool:
+        """Removes a role to a guild member
 
         Returns:
-            - True when successful
-            - None if member does not exist
-            - False otherwise
+        - True when successful
+        - None if member does not exist
+        - False otherwise
         """
         route = f"guilds/{guild_id}/members/{user_id}/roles/{role_id}"
         r = self._api_request(method='delete', route=route, raise_for_status=False)
         if self._is_member_unknown_error(r):
             logger.warning('User ID %s is not a member of this guild', user_id)
             return None
-        r.raise_for_status()
+        else:
+            r.raise_for_status()
+
         if r.status_code == 204:
             return True
-        return False
-
-    def guild_member_roles(self, guild_id: int, user_id: int) -> Optional[RolesSet]:
-        """Fetch the current guild roles of a guild member.
-
-        Args:
-        - guild_id: Discord guild ID
-        - user_id: Discord user ID
-
-        Returns:
-        - Member roles
-        - None if user is not a member of the guild
-        """
-        member_info = self.guild_member(guild_id=guild_id, user_id=user_id)
-        if member_info is None:
-            return None  # User is no longer a member
-        guild_roles = RolesSet(self.guild_roles(guild_id=guild_id))
-        logger.debug('Current guild roles: %s', guild_roles.ids())
-        if not guild_roles.has_roles(member_info.roles):
-            guild_roles = RolesSet(
-                self.guild_roles(guild_id=guild_id, use_cache=False)
-            )
-            if not guild_roles.has_roles(member_info.roles):
-                role_ids = set(member_info.roles).difference(guild_roles.ids())
-                raise RuntimeError(
-                    f'Discord user {user_id} has unknown roles: {role_ids}'
-                )
-        return guild_roles.subset(member_info.roles)
+        else:
+            return False
 
     @classmethod
     def _is_member_unknown_error(cls, r: requests.Response) -> bool:
         try:
             result = (
-                r.status_code == HTTPStatus.NOT_FOUND
-                and r.json()['code'] == DiscordApiStatusCode.UNKNOWN_MEMBER
+                r.status_code == cls._HTTP_STATUS_CODE_NOT_FOUND
+                and r.json()['code'] == cls._DISCORD_STATUS_CODE_UNKNOWN_MEMBER
             )
         except (ValueError, KeyError):
             result = False
@@ -621,19 +529,7 @@ class DiscordClient:
         authorization: str = None,
         raise_for_status: bool = True
     ) -> requests.Response:
-        """Core method for performing all API calls.
-
-        Args:
-            method: HTTP method of the request, e.g. "get"
-            route: Route in the Discord API, e.g. "users/@me"
-            data: Data to be send with the request
-            authorization: The authorization string to be used.
-                Will use the default bot token if not set.
-            raise_for_status: Whether a requests exception is to be raised when not ok
-
-        Returns:
-            The raw response from the API
-        """
+        """Core method for performing all API calls"""
         uid = uuid1().hex
 
         if not hasattr(requests, method):
@@ -681,7 +577,7 @@ class DiscordClient:
                 r.text
             )
 
-        if r.status_code == HTTPStatus.TOO_MANY_REQUESTS:
+        if r.status_code == self._HTTP_STATUS_CODE_RATE_LIMITED:
             self._handle_new_api_backoff(r, uid)
 
         self._report_rate_limit_from_api(r, uid)
@@ -692,10 +588,9 @@ class DiscordClient:
         return r
 
     def _handle_ongoing_api_backoff(self, uid: str) -> None:
-        """Check if api is currently on backoff.
-
-        If on backoff: will do a blocking wait if it expires soon,
-        else raises exception.
+        """checks if api is currently on backoff
+        if on backoff: will do a blocking wait if it expires soon,
+        else raises exception
         """
         global_backoff_duration = self._redis.pttl(self._KEY_GLOBAL_BACKOFF_UNTIL)
         if global_backoff_duration > 0:
@@ -715,9 +610,8 @@ class DiscordClient:
                 raise DiscordTooManyRequestsError(retry_after=global_backoff_duration)
 
     def _ensure_rate_limed_not_exhausted(self, uid: str) -> int:
-        """Ensures that the rate limit is not exhausted.
-
-        If exhausted: will do a blocking wait if rate limit resets soon,
+        """ensures that the rate limit is not exhausted
+        if exhausted: will do a blocking wait if rate limit resets soon,
         else raises exception
 
         returns requests remaining on success
@@ -760,10 +654,10 @@ class DiscordClient:
                 )
                 raise DiscordRateLimitExhausted(resets_in)
 
-        raise RuntimeError('Failed to handle rate limit after after too many tries.')
+        raise RuntimeError('Failed to handle rate limit after after too tries.')
 
     def _handle_new_api_backoff(self, r: requests.Response, uid: str) -> None:
-        """Raise exception for new API backoff error."""
+        """raises exception for new API backoff error"""
         response = r.json()
         if 'retry_after' in response:
             try:
@@ -785,8 +679,8 @@ class DiscordClient:
         )
         raise DiscordTooManyRequestsError(retry_after=retry_after)
 
-    def _report_rate_limit_from_api(self, r, uid) -> None:
-        """Try to log the current rate limit reported from API."""
+    def _report_rate_limit_from_api(self, r, uid):
+        """Tries to log the current rate limit reported from API"""
         if (
             logger.getEffectiveLevel() <= logging.DEBUG
             and 'x-ratelimit-limit' in r.headers
@@ -809,17 +703,22 @@ class DiscordClient:
 
     @staticmethod
     def _redis_decode(value: str) -> str:
-        """Decode a string from Redis and passes through None and Booleans."""
+        """Decodes a string from Redis and passes through None and Booleans"""
         if value is not None and not isinstance(value, bool):
             return value.decode('utf-8')
-        return value
+        else:
+            return value
 
     @staticmethod
     def _generate_hash(key: str) -> str:
-        """Generate hash key for given string."""
         return md5(key.encode('utf-8')).hexdigest()
 
     @staticmethod
-    def _sanitize_role_ids(role_ids: Iterable[int]) -> List[int]:
-        """Sanitize a list of role IDs, i.e. make sure its a list of unique integers."""
-        return [int(role_id) for role_id in set(role_ids)]
+    def _sanitize_role_ids(role_ids: list) -> list:
+        """make sure its a list of integers"""
+        return [int(role_id) for role_id in list(role_ids)]
+
+    @classmethod
+    def _sanitize_nick(cls, nick: str) -> str:
+        """shortens too long strings if necessary"""
+        return str(nick)[:cls._NICK_MAX_CHARS]

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

@@ -1,26 +1,23 @@
-"""Custom exceptions for the Discord Client package."""
-
 import math
 
 
 class DiscordClientException(Exception):
-    """Base Exception for the Discord client."""
+    """Base Exception for the Discord client"""
 
 
 class DiscordApiBackoff(DiscordClientException):
-    """Exception signaling we need to backoff from sending requests to the API for now.
-
-    Args:
-        retry_after: time to retry after in milliseconds
+    """Exception signaling we need to backoff from sending requests to the API for now
     """
 
     def __init__(self, retry_after: int):
+        """
+        :param retry_after: int time to retry after in milliseconds
+        """
         super().__init__()
         self.retry_after = int(retry_after)
 
     @property
     def retry_after_seconds(self):
-        """Time to retry after in seconds."""
         return math.ceil(self.retry_after / 1000)
 
 

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

@@ -1,37 +1,27 @@
 from copy import copy
-from typing import Iterable, List, Optional, Set, Tuple
-
-from .models import Role
+from typing import Set, Iterable
 
 
-class RolesSet:
-    """Container of Discord roles with added functionality.
+class DiscordRoles:
+    """Container class that helps dealing with Discord roles.
 
     Objects of this class are immutable and work in many ways like sets.
 
     Ideally objects are initialized from raw API responses,
-    e.g. from DiscordClient.guild.roles().
-
-    Args:
-        roles_lst: List of dicts, each defining a role
+    e.g. from DiscordClient.guild.roles()
     """
-    def __init__(self, roles_lst: Iterable[Role]) -> None:
+    _ROLE_NAME_MAX_CHARS = 100
+
+    def __init__(self, roles_lst: list) -> None:
+        """roles_lst must be a list of dict, each defining a role"""
         if not isinstance(roles_lst, (list, set, tuple)):
             raise TypeError('roles_lst must be of type list, set or tuple')
         self._roles = dict()
         self._roles_by_name = dict()
         for role in list(roles_lst):
-            if not isinstance(role, Role):
-                raise TypeError('Roles must be of type Role: %s' % role)
-            self._roles[role.id] = role
-            self._roles_by_name[role.name] = role
-
-    def __repr__(self) -> str:
-        if self._roles_by_name:
-            roles = '"' + '", "'.join(sorted(list(self._roles_by_name.keys()))) + '"'
-        else:
-            roles = ""
-        return f'{self.__class__.__name__}([{roles}])'
+            self._assert_valid_role(role)
+            self._roles[int(role['id'])] = role
+            self._roles_by_name[self.sanitize_role_name(role['name'])] = role
 
     def __eq__(self, other):
         if isinstance(other, type(self)):
@@ -51,15 +41,15 @@ class RolesSet:
         return len(self._roles.keys())
 
     def has_roles(self, role_ids: Set[int]) -> bool:
-        """True if this objects contains all roles defined by given role_ids
-        incl. managed roles.
+        """returns true if this objects contains all roles defined by given role_ids
+        incl. managed roles
         """
         role_ids = {int(id) for id in role_ids}
         all_role_ids = self._roles.keys()
         return role_ids.issubset(all_role_ids)
 
     def ids(self) -> Set[int]:
-        """Set of all role IDs."""
+        """return a set of all role IDs"""
         return set(self._roles.keys())
 
     def subset(
@@ -67,13 +57,13 @@ class RolesSet:
         role_ids: Iterable[int] = None,
         managed_only: bool = False,
         role_names: Iterable[str] = None
-    ) -> "RolesSet":
-        """Create instance containing the subset of roles
+    ) -> "DiscordRoles":
+        """returns a new object containing the subset of roles
 
         Args:
-            role_ids: role ids must be in the provided list
-            managed_only: roles must be managed
-            role_names: role names must match provided list (not case sensitive)
+        - role_ids: role ids must be in the provided list
+        - managed_only: roles must be managed
+        - role_names: role names must match provided list (not case sensitive)
         """
         if role_ids is not None:
             role_ids = {int(id) for id in role_ids}
@@ -85,50 +75,72 @@ class RolesSet:
 
         elif role_ids is None and managed_only:
             return type(self)([
-                role for _, role in self._roles.items() if role.managed
+                role for _, role in self._roles.items() if role['managed']
             ])
 
         elif role_ids is not None and managed_only:
             return type(self)([
                 role for role_id, role in self._roles.items()
-                if role_id in role_ids and role.managed
+                if role_id in role_ids and role['managed']
             ])
 
         elif role_ids is None and managed_only is False and role_names is not None:
-            role_names = {Role.sanitize_name(name).lower() for name in role_names}
+            role_names = {self.sanitize_role_name(name).lower() for name in role_names}
             return type(self)([
                 role for role in self._roles.values()
-                if role.name.lower() in role_names
+                if role["name"].lower() in role_names
             ])
 
         return copy(self)
 
-    def union(self, other: object) -> "RolesSet":
-        """Create instance that is the union of this roles object with other."""
+    def union(self, other: object) -> "DiscordRoles":
+        """returns a new roles object that is the union of this roles object
+        with other"""
         return type(self)(list(self) + list(other))
 
-    def difference(self, other: object) -> "RolesSet":
-        """Create instance that only contains the roles
-        that exist in the current objects, but not in other.
+    def difference(self, other: object) -> "DiscordRoles":
+        """returns a new roles object that only contains the roles
+        that exist in the current objects, but not in other
         """
         new_ids = self.ids().difference(other.ids())
         return self.subset(role_ids=new_ids)
 
-    def role_by_name(self, role_name: str) -> Optional[Role]:
-        """Role if one with matching name is found else None."""
-        role_name = Role.sanitize_name(role_name)
+    def role_by_name(self, role_name: str) -> dict:
+        """returns role if one with matching name is found else an empty dict"""
+        role_name = self.sanitize_role_name(role_name)
         if role_name in self._roles_by_name:
             return self._roles_by_name[role_name]
-        return None
+        return dict()
 
     @classmethod
-    def create_from_matched_roles(
-        cls, matched_roles: List[Tuple[Role, bool]]
-    ) -> "RolesSet":
-        """Create new instance from the given list of matches roles.
+    def create_from_matched_roles(cls, matched_roles: list) -> "DiscordRoles":
+        """returns a new object created from the given list of matches roles
 
-        Args:
-            matches_roles: list of matches roles
+        matches_roles must be a list of tuples in the form: (role, created)
         """
         raw_roles = [x[0] for x in matched_roles]
         return cls(raw_roles)
+
+    @staticmethod
+    def _assert_valid_role(role: dict) -> None:
+        if not isinstance(role, dict):
+            raise TypeError('Roles must be of type dict: %s' % role)
+
+        if 'id' not in role or 'name' not in role or 'managed' not in role:
+            raise ValueError('This role is not valid: %s' % role)
+
+    @classmethod
+    def sanitize_role_name(cls, role_name: str) -> str:
+        """shortens too long strings if necessary"""
+        return str(role_name)[:cls._ROLE_NAME_MAX_CHARS]
+
+
+def match_or_create_roles_from_names(
+    client: object, guild_id: int, role_names: list
+) -> DiscordRoles:
+    """Shortcut for getting the result of matching role names as DiscordRoles object"""
+    return DiscordRoles.create_from_matched_roles(
+        client.match_or_create_roles_from_names(
+            guild_id=guild_id, role_names=role_names
+        )
+    )

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

@@ -1,125 +0,0 @@
-"""Implementation of Discord objects used by this client.
-
-Note that only those objects and properties are implemented, which are needed by AA.
-
-Names and types are mirrored from the API whenever possible.
-Discord's snowflake type (used by Discord IDs) is implemented as int.
-"""
-
-from dataclasses import asdict, dataclass
-from typing import FrozenSet
-
-
-@dataclass(frozen=True)
-class User:
-    """A user on Discord."""
-
-    id: int
-    username: str
-    discriminator: str
-
-    def __post_init__(self):
-        object.__setattr__(self, "id", int(self.id))
-        object.__setattr__(self, "username", str(self.username))
-        object.__setattr__(self, "discriminator", str(self.discriminator))
-
-    @classmethod
-    def from_dict(cls, data: dict) -> "User":
-        """Create object from dictionary as received from the API."""
-        return cls(
-            id=int(data["id"]),
-            username=data["username"],
-            discriminator=data["discriminator"],
-        )
-
-
-@dataclass(frozen=True)
-class Role:
-    """A role on Discord."""
-
-    _ROLE_NAME_MAX_CHARS = 100
-
-    id: int
-    name: str
-    managed: bool = False
-
-    def __post_init__(self):
-        object.__setattr__(self, "id", int(self.id))
-        object.__setattr__(self, "name", self.sanitize_name(self.name))
-        object.__setattr__(self, "managed", bool(self.managed))
-
-    def asdict(self) -> dict:
-        """Convert object into a dictionary representation."""
-        return asdict(self)
-
-    @classmethod
-    def from_dict(cls, data: dict) -> "Role":
-        """Create object from dictionary as received from the API."""
-        return cls(id=int(data["id"]), name=data["name"], managed=data["managed"])
-
-    @classmethod
-    def sanitize_name(cls, role_name: str) -> str:
-        """Shorten too long names if necessary."""
-        return str(role_name)[: cls._ROLE_NAME_MAX_CHARS]
-
-
-@dataclass(frozen=True)
-class Guild:
-    """A guild on Discord."""
-
-    id: int
-    name: str
-    roles: FrozenSet[Role]
-
-    def __post_init__(self):
-        object.__setattr__(self, "id", int(self.id))
-        object.__setattr__(self, "name", str(self.name))
-        for role in self.roles:
-            if not isinstance(role, Role):
-                raise TypeError("roles can only contain Role objects.")
-        object.__setattr__(self, "roles", frozenset(self.roles))
-
-    @classmethod
-    def from_dict(cls, data: dict) -> "Guild":
-        """Create object from dictionary as received from the API."""
-        return cls(
-            id=int(data["id"]),
-            name=data["name"],
-            roles=frozenset(Role.from_dict(obj) for obj in data["roles"]),
-        )
-
-
-@dataclass(frozen=True)
-class GuildMember:
-    """A member of a guild on Discord."""
-
-    _NICK_MAX_CHARS = 32
-
-    roles: FrozenSet[int]
-    nick: str = None
-    user: User = None
-
-    def __post_init__(self):
-        if self.nick:
-            object.__setattr__(self, "nick", self.sanitize_nick(self.nick))
-        if self.user and not isinstance(self.user, User):
-            raise TypeError("user must be of type User")
-        for role in self.roles:
-            if not isinstance(role, int):
-                raise TypeError("roles can only contain ints")
-        object.__setattr__(self, "roles", frozenset(self.roles))
-
-    @classmethod
-    def from_dict(cls, data: dict) -> "GuildMember":
-        """Create object from dictionary as received from the API."""
-        params = {"roles": {int(obj) for obj in data["roles"]}}
-        if data.get("user"):
-            params["user"] = User.from_dict(data["user"])
-        if data.get("nick"):
-            params["nick"] = data["nick"]
-        return cls(**params)
-
-    @classmethod
-    def sanitize_nick(cls, nick: str) -> str:
-        """Sanitize a nick, i.e. shorten too long strings if necessary."""
-        return str(nick)[: cls._NICK_MAX_CHARS]

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

@@ -0,0 +1,40 @@
+TEST_GUILD_ID = 123456789012345678
+TEST_USER_ID = 198765432012345678
+TEST_USER_NAME = 'Peter Parker'
+TEST_USER_DISCRIMINATOR = '1234'
+TEST_BOT_TOKEN = 'abcdefhijlkmnopqastzvwxyz1234567890ABCDEFGHOJKLMNOPQRSTUVWXY'
+TEST_ROLE_ID = 654321012345678912
+
+
+def create_role(id: int, name: str, managed=False) -> dict:
+    return {
+        'id': int(id),
+        'name': str(name),
+        'managed': bool(managed)
+    }
+
+
+def create_matched_role(role, created=False) -> tuple:
+    return role, created
+
+
+ROLE_ALPHA = create_role(1, 'alpha')
+ROLE_BRAVO = create_role(2, 'bravo')
+ROLE_CHARLIE = create_role(3, 'charlie')
+ROLE_CHARLIE_2 = create_role(4, 'Charlie')  # Discord roles are case sensitive
+ROLE_MIKE = create_role(13, 'mike', True)
+
+
+ALL_ROLES = [ROLE_ALPHA, ROLE_BRAVO, ROLE_CHARLIE, ROLE_MIKE]
+
+
+def create_user_info(
+    id: int = TEST_USER_ID,
+    username: str = TEST_USER_NAME,
+    discriminator: str = TEST_USER_DISCRIMINATOR
+):
+    return {
+        'id': str(id),
+        'username': str(username[:32]),
+        'discriminator': str(discriminator[:4])
+    }

allianceauth/services/modules/discord/discord_client/tests/example_objects.json

@@ -1,155 +0,0 @@
-{
-    "guilds": {
-        "2909267986263572999": {
-            "id": "2909267986263572999",
-            "name": "Mason's Test Server",
-            "icon": "389030ec9db118cb5b85a732333b7c98",
-            "description": null,
-            "splash": "75610b05a0dd09ec2c3c7df9f6975ea0",
-            "discovery_splash": null,
-            "approximate_member_count": 2,
-            "approximate_presence_count": 2,
-            "features": [
-                "INVITE_SPLASH",
-                "VANITY_URL",
-                "COMMERCE",
-                "BANNER",
-                "NEWS",
-                "VERIFIED",
-                "VIP_REGIONS"
-            ],
-            "emojis": [
-                {
-                    "name": "ultrafastparrot",
-                    "roles": [],
-                    "id": "393564762228785161",
-                    "require_colons": true,
-                    "managed": false,
-                    "animated": true,
-                    "available": true
-                }
-            ],
-            "banner": "5c3cb8d1bc159937fffe7e641ec96ca7",
-            "owner_id": "53908232506183680",
-            "application_id": null,
-            "region": null,
-            "afk_channel_id": null,
-            "afk_timeout": 300,
-            "system_channel_id": null,
-            "widget_enabled": true,
-            "widget_channel_id": "639513352485470208",
-            "verification_level": 0,
-            "roles": [
-                {
-                    "id": "2909267986263572999",
-                    "name": "@everyone",
-                    "permissions": "49794752",
-                    "position": 0,
-                    "color": 0,
-                    "hoist": false,
-                    "managed": false,
-                    "mentionable": false
-                }
-            ],
-            "default_message_notifications": 1,
-            "mfa_level": 0,
-            "explicit_content_filter": 0,
-            "max_presences": null,
-            "max_members": 250000,
-            "max_video_channel_users": 25,
-            "vanity_url_code": "no",
-            "premium_tier": 0,
-            "premium_subscription_count": 0,
-            "system_channel_flags": 0,
-            "preferred_locale": "en-US",
-            "rules_channel_id": null,
-            "public_updates_channel_id": null
-        }
-    },
-    "guildMembers": {
-        "1": {
-            "user": {},
-            "nick": null,
-            "avatar": null,
-            "roles": [],
-            "joined_at": "2015-04-26T06:26:56.936000+00:00",
-            "deaf": false,
-            "mute": false
-        },
-        "2": {
-            "user": {
-                "id": "80351110224678912",
-                "username": "Nelly",
-                "discriminator": "1337",
-                "avatar": "8342729096ea3675442027381ff50dfe",
-                "verified": true,
-                "email": "nelly@discord.com",
-                "flags": 64,
-                "banner": "06c16474723fe537c283b8efa61a30c8",
-                "accent_color": 16711680,
-                "premium_type": 1,
-                "public_flags": 64
-            },
-            "nick": "Nelly the great",
-            "avatar": null,
-            "roles": [
-                "197150972374548480",
-                "41771983423143936"
-            ],
-            "joined_at": "2015-04-26T06:26:56.936000+00:00",
-            "deaf": false,
-            "mute": false
-        }
-    },
-    "roles": {
-        "197150972374548480": {
-            "id": "197150972374548480",
-            "name": "My Managed Role",
-            "color": 3447003,
-            "hoist": false,
-            "icon": "cf3ced8600b777c9486c6d8d84fb4327",
-            "unicode_emoji": null,
-            "position": 2,
-            "permissions": "66321471",
-            "managed": true,
-            "mentionable": false
-        },
-        "2909267986263572999": {
-            "id": "2909267986263572999",
-            "name": "@everyone",
-            "permissions": "49794752",
-            "position": 0,
-            "color": 0,
-            "hoist": false,
-            "managed": false,
-            "mentionable": false
-        },
-        "41771983423143936": {
-            "id": "41771983423143936",
-            "name": "WE DEM BOYZZ!!!!!!",
-            "color": 3447003,
-            "hoist": true,
-            "icon": "cf3ced8600b777c9486c6d8d84fb4327",
-            "unicode_emoji": null,
-            "position": 1,
-            "permissions": "66321471",
-            "managed": false,
-            "mentionable": false
-        }
-    },
-    "users": {
-        "80351110224678912": {
-            "id": "80351110224678912",
-            "username": "Nelly",
-            "discriminator": "1337",
-            "avatar": "8342729096ea3675442027381ff50dfe",
-            "verified": true,
-            "email": "nelly@discord.com",
-            "flags": 64,
-            "banner": "06c16474723fe537c283b8efa61a30c8",
-            "accent_color": 16711680,
-            "premium_type": 1,
-            "public_flags": 64
-        }
-    }
-}

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

@@ -1,110 +0,0 @@
-from itertools import count
-
-from django.utils.timezone import now
-
-from ..client import DiscordApiStatusCode
-from ..models import Guild, GuildMember, Role, User
-
-TEST_GUILD_ID = 123456789012345678
-TEST_GUILD_NAME = "Test Guild"
-TEST_USER_ID = 198765432012345678
-TEST_USER_NAME = "Peter Parker"
-TEST_USER_DISCRIMINATOR = "1234"
-TEST_BOT_TOKEN = "abcdefhijlkmnopqastzvwxyz1234567890ABCDEFGHOJKLMNOPQRSTUVWXY"
-TEST_ROLE_ID = 654321012345678912
-
-
-def create_discord_role_object(id: int, name: str, managed: bool = False) -> dict:
-    return {"id": str(int(id)), "name": str(name), "managed": bool(managed)}
-
-
-def create_matched_role(role, created=False) -> tuple:
-    return role, created
-
-
-def create_discord_user_object(**kwargs):
-    params = {
-        "id": TEST_USER_ID,
-        "username": TEST_USER_NAME,
-        "discriminator": TEST_USER_DISCRIMINATOR,
-    }
-    params.update(kwargs)
-    params["id"] = str(int(params["id"]))
-    return params
-
-
-def create_discord_guild_member_object(user=None, **kwargs):
-    user_params = {}
-    if user:
-        user_params["user"] = user
-    params = {
-        "user": create_discord_user_object(**user_params),
-        "roles": [],
-        "joined_at": now().isoformat(),
-        "deaf": False,
-        "mute": False,
-    }
-    params.update(kwargs)
-    params["roles"] = [str(int(obj)) for obj in params["roles"]]
-    return params
-
-
-def create_discord_error_response(code: int) -> dict:
-    return {"code": int(code)}
-
-
-def create_discord_error_response_unknown_member() -> dict:
-    return create_discord_error_response(DiscordApiStatusCode.UNKNOWN_MEMBER.value)
-
-
-def create_discord_guild_object(**kwargs):
-    params = {"id": TEST_GUILD_ID, "name": TEST_GUILD_NAME, "roles": []}
-    params.update(kwargs)
-    params["id"] = str(int(params["id"]))
-    return params
-
-
-def create_user(**kwargs):
-    params = {
-        "id": TEST_USER_ID,
-        "username": TEST_USER_NAME,
-        "discriminator": TEST_USER_DISCRIMINATOR,
-    }
-    params.update(kwargs)
-    return User(**params)
-
-
-def create_guild(**kwargs):
-    params = {"id": TEST_GUILD_ID, "name": TEST_GUILD_NAME, "roles": []}
-    params.update(kwargs)
-    return Guild(**params)
-
-
-def create_guild_member(**kwargs):
-    params = {"user": create_user(), "roles": []}
-    params.update(kwargs)
-    return GuildMember(**params)
-
-
-def create_role(**kwargs) -> dict:
-    params = {"managed": False}
-    params.update(kwargs)
-    if "id" not in params:
-        params["id"] = next_number("role")
-    if "name" not in params:
-        params["name"] = f"Test Role #{params['id']}"
-    return Role(**params)
-
-
-def next_number(key: str = None) -> int:
-    """Calculate the next number in a persistent sequence."""
-    if key is None:
-        key = "_general"
-    try:
-        return next_number._counter[key].__next__()
-    except AttributeError:
-        next_number._counter = dict()
-    except KeyError:
-        pass
-    next_number._counter[key] = count(start=1)
-    return next_number._counter[key].__next__()

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

@@ -1,201 +1,177 @@
-from allianceauth.utils.testing import NoSocketsTestCase
+from unittest import TestCase
 
-from ..helpers import RolesSet
-from .factories import create_matched_role, create_role
+from . import (
+    ROLE_ALPHA,
+    ROLE_BRAVO,
+    ROLE_CHARLIE,
+    ROLE_CHARLIE_2,
+    ROLE_MIKE,
+    ALL_ROLES,
+    create_role
+)
+from .. import DiscordRoles
 
 
-class TestRolesSet(NoSocketsTestCase):
+MODULE_PATH = 'allianceauth.services.modules.discord.discord_client.client'
+
+
+class TestDiscordRoles(TestCase):
+
+    def setUp(self):
+        self.all_roles = DiscordRoles(ALL_ROLES)
+
     def test_can_create_simple(self):
-        # given
-        roles_raw = [create_role()]
-        # when
-        roles = RolesSet(roles_raw)
-        # then
+        roles_raw = [ROLE_ALPHA]
+        roles = DiscordRoles(roles_raw)
         self.assertListEqual(list(roles), roles_raw)
 
     def test_can_create_empty(self):
-        # when
-        roles = RolesSet([])
-        # then
+        roles_raw = []
+        roles = DiscordRoles(roles_raw)
         self.assertListEqual(list(roles), [])
 
     def test_raises_exception_if_roles_raw_of_wrong_type(self):
         with self.assertRaises(TypeError):
-            RolesSet({"id": 1})
+            DiscordRoles({'id': 1})
 
     def test_raises_exception_if_list_contains_non_dict(self):
-        # given
-        roles_raw = [create_role(), "not_valid"]
-        # when/then
+        roles_raw = [ROLE_ALPHA, 'not_valid']
         with self.assertRaises(TypeError):
-            RolesSet(roles_raw)
+            DiscordRoles(roles_raw)
+
+    def test_raises_exception_if_invalid_role_1(self):
+        roles_raw = [{'name': 'alpha', 'managed': False}]
+        with self.assertRaises(ValueError):
+            DiscordRoles(roles_raw)
+
+    def test_raises_exception_if_invalid_role_2(self):
+        roles_raw = [{'id': 1, 'managed': False}]
+        with self.assertRaises(ValueError):
+            DiscordRoles(roles_raw)
+
+    def test_raises_exception_if_invalid_role_3(self):
+        roles_raw = [{'id': 1, 'name': 'alpha'}]
+        with self.assertRaises(ValueError):
+            DiscordRoles(roles_raw)
 
     def test_roles_are_equal(self):
-        # given
-        role_a = create_role()
-        role_b = create_role()
-        roles_a = RolesSet([role_a, role_b])
-        roles_b = RolesSet([role_a, role_b])
-        # when/then
+        roles_a = DiscordRoles([ROLE_ALPHA, ROLE_BRAVO])
+        roles_b = DiscordRoles([ROLE_ALPHA, ROLE_BRAVO])
         self.assertEqual(roles_a, roles_b)
 
     def test_roles_are_not_equal(self):
-        # given
-        role_a = create_role()
-        role_b = create_role()
-        roles_a = RolesSet([role_a, role_b])
-        roles_b = RolesSet([role_a])
-        # when/then
+        roles_a = DiscordRoles([ROLE_ALPHA, ROLE_BRAVO])
+        roles_b = DiscordRoles([ROLE_ALPHA])
         self.assertNotEqual(roles_a, roles_b)
 
     def test_different_objects_are_not_equal(self):
-        roles_a = RolesSet([])
+        roles_a = DiscordRoles([ROLE_ALPHA, ROLE_BRAVO])
         self.assertFalse(roles_a == "invalid")
 
     def test_len(self):
-        # given
-        role_a = create_role()
-        role_b = create_role()
-        roles = RolesSet([role_a, role_b])
-        # when/then
-        self.assertEqual(len(roles), 2)
+        self.assertEqual(len(self.all_roles), 4)
 
     def test_contains(self):
-        # given
-        role_a = create_role(id=1)
-        roles = RolesSet([role_a])
-        # when/then
-        self.assertTrue(1 in roles)
-        self.assertFalse(99 in roles)
+        self.assertTrue(1 in self.all_roles)
+        self.assertFalse(99 in self.all_roles)
+
+    def test_sanitize_role_name(self):
+        role_name_input = 'x' * 110
+        role_name_expected = 'x' * 100
+        result = DiscordRoles.sanitize_role_name(role_name_input)
+        self.assertEqual(result, role_name_expected)
 
     def test_objects_are_hashable(self):
-        # given
-        role_a = create_role()
-        role_b = create_role()
-        role_c = create_role()
-        roles_a = RolesSet([role_a, role_b])
-        roles_b = RolesSet([role_b, role_a])
-        roles_c = RolesSet([role_a, role_b, role_c])
-        # when/then
+        roles_a = DiscordRoles([ROLE_ALPHA, ROLE_BRAVO])
+        roles_b = DiscordRoles([ROLE_BRAVO, ROLE_ALPHA])
+        roles_c = DiscordRoles([ROLE_ALPHA, ROLE_BRAVO, ROLE_MIKE])
         self.assertIsNotNone(hash(roles_a))
         self.assertEqual(hash(roles_a), hash(roles_b))
         self.assertNotEqual(hash(roles_a), hash(roles_c))
 
     def test_create_from_matched_roles(self):
-        role_a = create_role()
-        role_b = create_role()
         matched_roles = [
-            create_matched_role(role_a, True),
-            create_matched_role(role_b, False),
+            (ROLE_ALPHA, True),
+            (ROLE_BRAVO, False)
         ]
-        # when
-        roles = RolesSet.create_from_matched_roles(matched_roles)
-        # then
-        self.assertEqual(roles, RolesSet([role_a, role_b]))
-
-    def test_return_role_ids_default(self):
-        role_a = create_role(id=1)
-        role_b = create_role(id=2)
-        roles = RolesSet([role_a, role_b])
-        # when/then
+        roles = DiscordRoles.create_from_matched_roles(matched_roles)
         self.assertSetEqual(roles.ids(), {1, 2})
 
+
+class TestIds(TestCase):
+
+    def setUp(self):
+        self.all_roles = DiscordRoles(ALL_ROLES)
+
+    def test_return_role_ids_default(self):
+        result = self.all_roles.ids()
+        expected = {1, 2, 3, 13}
+        self.assertSetEqual(result, expected)
+
     def test_return_role_ids_empty(self):
-        # given
-        roles = RolesSet([])
-        # when/then
+        roles = DiscordRoles([])
         self.assertSetEqual(roles.ids(), set())
 
 
-class TestRolesSetSubset(NoSocketsTestCase):
+class TestSubset(TestCase):
+
+    def setUp(self):
+        self.all_roles = DiscordRoles(ALL_ROLES)
+
     def test_ids_only(self):
-        # given
-        role_a = create_role(id=1)
-        role_b = create_role(id=2)
-        role_c = create_role(id=3)
-        roles_all = RolesSet([role_a, role_b, role_c])
-        # when
-        roles_subset = roles_all.subset({1, 3})
-        # then
-        self.assertEqual(roles_subset, RolesSet([role_a, role_c]))
+        role_ids = {1, 3}
+        roles_subset = self.all_roles.subset(role_ids)
+        expected = {1, 3}
+        self.assertSetEqual(roles_subset.ids(), expected)
 
     def test_ids_as_string_work_too(self):
-        # given
-        role_a = create_role(id=1)
-        role_b = create_role(id=2)
-        role_c = create_role(id=3)
-        roles_all = RolesSet([role_a, role_b, role_c])
-        # when
-        roles_subset = roles_all.subset({"1", "3"})
-        # then
-        self.assertEqual(roles_subset, RolesSet([role_a, role_c]))
+        role_ids = {'1', '3'}
+        roles_subset = self.all_roles.subset(role_ids)
+        expected = {1, 3}
+        self.assertSetEqual(roles_subset.ids(), expected)
 
     def test_managed_only(self):
-        # given
-        role_a = create_role(id=1)
-        role_m = create_role(id=13, managed=True)
-        roles_all = RolesSet([role_a, role_m])
-        # when
-        roles_subset = roles_all.subset(managed_only=True)
-        # then
-        self.assertEqual(roles_subset, RolesSet([role_m]))
+        roles = self.all_roles.subset(managed_only=True)
+        expected = {13}
+        self.assertSetEqual(roles.ids(), expected)
 
     def test_ids_and_managed_only(self):
-        # given
-        role_a = create_role(id=1)
-        role_b = create_role(id=2)
-        role_m = create_role(id=13, managed=True)
-        roles_all = RolesSet([role_a, role_b, role_m])
-        # when
-        roles_subset = roles_all.subset({1, 13}, managed_only=True)
-        # then
-        self.assertEqual(roles_subset, RolesSet([role_m]))
+        role_ids = {1, 3, 13}
+        roles_subset = self.all_roles.subset(role_ids, managed_only=True)
+        expected = {13}
+        self.assertSetEqual(roles_subset.ids(), expected)
 
     def test_ids_are_empty(self):
-        # given
-        role_a = create_role(id=1)
-        role_b = create_role(id=2)
-        roles_all = RolesSet([role_a, role_b])
-        roles_subset = roles_all.subset([])
-        # then
-        self.assertEqual(roles_subset, RolesSet([]))
+        roles = self.all_roles.subset([])
+        expected = set()
+        self.assertSetEqual(roles.ids(), expected)
 
     def test_no_parameters(self):
-        # given
-        role_a = create_role(id=1)
-        role_b = create_role(id=2)
-        roles_all = RolesSet([role_a, role_b])
-        roles_subset = roles_all.subset()
-        # then
-        self.assertEqual(roles_subset, roles_all)
+        roles = self.all_roles.subset()
+        expected = {1, 2, 3, 13}
+        self.assertSetEqual(roles.ids(), expected)
 
     def test_should_return_role_names_only(self):
         # given
-        role_a = create_role(name="alpha")
-        role_b = create_role(name="bravo")
-        role_c1 = create_role(name="charlie")
-        role_c2 = create_role(name="Charlie")
-        roles_all = RolesSet([role_a, role_b, role_c1, role_c2])
+        all_roles = DiscordRoles([
+            ROLE_ALPHA, ROLE_BRAVO, ROLE_CHARLIE, ROLE_MIKE, ROLE_CHARLIE_2
+        ])
         # when
-        roles_subset = roles_all.subset(role_names={"bravo", "charlie"})
+        roles = all_roles.subset(role_names={"bravo", "charlie"})
         # then
-        self.assertSetEqual(roles_subset, RolesSet([role_b, role_c1, role_c2]))
+        self.assertSetEqual(roles.ids(), {2, 3, 4})
 
 
-class TestRolesSetHasRoles(NoSocketsTestCase):
-    @classmethod
-    def setUpClass(cls):
-        super().setUpClass()
-        role_a = create_role(id=1)
-        role_b = create_role(id=2)
-        role_c = create_role(id=3)
-        cls.all_roles = RolesSet([role_a, role_b, role_c])
+class TestHasRoles(TestCase):
+
+    def setUp(self):
+        self.all_roles = DiscordRoles(ALL_ROLES)
 
     def test_true_if_all_roles_exit(self):
         self.assertTrue(self.all_roles.has_roles([1, 2]))
 
     def test_true_if_all_roles_exit_str(self):
-        self.assertTrue(self.all_roles.has_roles(["1", "2"]))
+        self.assertTrue(self.all_roles.has_roles(['1', '2']))
 
     def test_false_if_role_does_not_exit(self):
         self.assertFalse(self.all_roles.has_roles([99]))
@@ -207,104 +183,74 @@ class TestRolesSetHasRoles(NoSocketsTestCase):
         self.assertTrue(self.all_roles.has_roles([]))
 
 
-class TestRolesSetGetMatchingRolesByName(NoSocketsTestCase):
+class TestGetMatchingRolesByName(TestCase):
+
+    def setUp(self):
+        self.all_roles = DiscordRoles(ALL_ROLES)
+
     def test_return_role_if_matches(self):
-        # given
-        role_a = create_role(name="alpha")
-        role_b = create_role(name="bravo")
-        roles = RolesSet([role_a, role_b])
-        # when
-        result = roles.role_by_name("alpha")
-        # then
-        self.assertEqual(result, role_a)
+        role_name = 'alpha'
+        expected = ROLE_ALPHA
+        result = self.all_roles.role_by_name(role_name)
+        self.assertEqual(result, expected)
 
     def test_return_role_if_matches_and_limit_max_length(self):
-        # given
-        role_name = "x" * 120
-        role = create_role(name="x" * 100)
-        roles = RolesSet([role])
-        # when
+        role_name = 'x' * 120
+        expected = create_role(77, 'x' * 100)
+        roles = DiscordRoles([expected])
         result = roles.role_by_name(role_name)
-        # then
-        self.assertEqual(result, role)
+        self.assertEqual(result, expected)
 
     def test_return_empty_if_not_matches(self):
-        # given
-        role_a = create_role(name="alpha")
-        role_b = create_role(name="bravo")
-        roles = RolesSet([role_a, role_b])
-        # when
-        result = roles.role_by_name("unknown")
-        # then
-        self.assertIsNone(result)
+        role_name = 'lima'
+        expected = {}
+        result = self.all_roles.role_by_name(role_name)
+        self.assertEqual(result, expected)
 
 
-class TestRolesSetUnion(NoSocketsTestCase):
+class TestUnion(TestCase):
+
     def test_distinct_sets(self):
-        # given
-        role_a = create_role()
-        role_b = create_role()
-        roles_1 = RolesSet([role_a])
-        roles_2 = RolesSet([role_b])
-        # when
-        result = roles_1.union(roles_2)
-        # then
-        self.assertEqual(result, RolesSet([role_a, role_b]))
+        roles_1 = DiscordRoles([ROLE_ALPHA, ROLE_BRAVO])
+        roles_2 = DiscordRoles([ROLE_CHARLIE, ROLE_MIKE])
+        roles_3 = roles_1.union(roles_2)
+        expected = DiscordRoles([ROLE_ALPHA, ROLE_BRAVO, ROLE_CHARLIE, ROLE_MIKE])
+        self.assertEqual(roles_3, expected)
 
     def test_overlapping_sets(self):
-        # given
-        role_a = create_role()
-        role_b = create_role()
-        role_c = create_role()
-        roles_1 = RolesSet([role_a, role_b])
-        roles_2 = RolesSet([role_b, role_c])
-        # when
-        result = roles_1.union(roles_2)
-        self.assertEqual(result, RolesSet([role_a, role_b, role_c]))
+        roles_1 = DiscordRoles([ROLE_ALPHA, ROLE_BRAVO])
+        roles_2 = DiscordRoles([ROLE_BRAVO, ROLE_MIKE])
+        roles_3 = roles_1.union(roles_2)
+        expected = DiscordRoles([ROLE_ALPHA, ROLE_BRAVO, ROLE_MIKE])
+        self.assertEqual(roles_3, expected)
 
     def test_identical_sets(self):
-        role_a = create_role()
-        role_b = create_role()
-        roles_1 = RolesSet([role_a, role_b])
-        roles_2 = RolesSet([role_a, role_b])
-        # when
-        result = roles_1.union(roles_2)
-        self.assertEqual(result, RolesSet([role_a, role_b]))
+        roles_1 = DiscordRoles([ROLE_ALPHA, ROLE_BRAVO])
+        roles_2 = DiscordRoles([ROLE_ALPHA, ROLE_BRAVO])
+        roles_3 = roles_1.union(roles_2)
+        expected = DiscordRoles([ROLE_ALPHA, ROLE_BRAVO])
+        self.assertEqual(roles_3, expected)
 
 
-class TestRolesSetDifference(NoSocketsTestCase):
+class TestDifference(TestCase):
+
     def test_distinct_sets(self):
-        # given
-        role_a = create_role()
-        role_b = create_role()
-        role_c = create_role()
-        role_d = create_role()
-        roles_1 = RolesSet([role_a, role_b])
-        roles_2 = RolesSet([role_c, role_d])
-        # when
-        result = roles_1.difference(roles_2)
-        # then
-        self.assertEqual(result, RolesSet([role_a, role_b]))
+        roles_1 = DiscordRoles([ROLE_ALPHA, ROLE_BRAVO])
+        roles_2 = DiscordRoles([ROLE_CHARLIE, ROLE_MIKE])
+        roles_3 = roles_1.difference(roles_2)
+        expected = DiscordRoles([ROLE_ALPHA, ROLE_BRAVO])
+        self.assertEqual(roles_3, expected)
 
     def test_overlapping_sets(self):
-        # given
-        role_a = create_role()
-        role_b = create_role()
-        role_c = create_role()
-        roles_1 = RolesSet([role_a, role_b])
-        roles_2 = RolesSet([role_b, role_c])
-        # when
-        result = roles_1.difference(roles_2)
-        # then
-        self.assertEqual(result, RolesSet([role_a]))
+        roles_1 = DiscordRoles([ROLE_ALPHA, ROLE_BRAVO])
+        roles_2 = DiscordRoles([ROLE_BRAVO, ROLE_MIKE])
+        roles_3 = roles_1.difference(roles_2)
+        expected = DiscordRoles([ROLE_ALPHA])
+        self.assertEqual(roles_3, expected)
 
     def test_identical_sets(self):
-        # given
-        role_a = create_role()
-        role_b = create_role()
-        roles_1 = RolesSet([role_a, role_b])
-        roles_2 = RolesSet([role_a, role_b])
-        # when
-        result = roles_1.difference(roles_2)
-        # then
-        self.assertEqual(result, RolesSet([]))
+        roles_1 = DiscordRoles([ROLE_ALPHA, ROLE_BRAVO])
+        roles_2 = DiscordRoles([ROLE_ALPHA, ROLE_BRAVO])
+        roles_3 = roles_1.difference(roles_2)
+        expected = DiscordRoles([])
+        self.assertEqual(roles_3, expected)

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

@@ -1,156 +0,0 @@
-import json
-from pathlib import Path
-from unittest import TestCase
-
-from ..models import Guild, GuildMember, Role, User
-from .factories import create_guild, create_guild_member, create_role, create_user
-
-
-def _fetch_example_objects() -> dict:
-    path = Path(__file__).parent / "example_objects.json"
-    with path.open("r", encoding="utf-8") as fp:
-        return json.load(fp)
-
-
-class TestUser(TestCase):
-    def test_should_create_new_object(self):
-        # when
-        obj = User(id="42", username=123, discriminator=456)
-        # then
-        self.assertEqual(obj.id, 42)
-        self.assertEqual(obj.username, "123")
-        self.assertTrue(obj.discriminator, "456")
-
-    def test_should_create_from_dict(self):
-        # given
-        data = example_objects["users"]["80351110224678912"]
-        # when
-        obj = User.from_dict(data)
-        # then
-        self.assertEqual(obj.id, 80351110224678912)
-        self.assertEqual(obj.username, "Nelly")
-        self.assertEqual(obj.discriminator, "1337")
-
-
-class TestRole(TestCase):
-    def test_should_create_new_object_with_defaults(self):
-        # when
-        obj = Role(id="42", name="x" * 110)
-        # then
-        self.assertEqual(obj.id, 42)
-        self.assertEqual(obj.name, "x" * 100)
-        self.assertFalse(obj.managed)
-
-    def test_should_create_new_object(self):
-        # when
-        obj = Role(id=42, name="name", managed=1)
-        # then
-        self.assertEqual(obj.id, 42)
-        self.assertEqual(obj.name, "name")
-        self.assertTrue(obj.managed)
-
-    def test_should_create_from_dict(self):
-        # given
-        data = example_objects["roles"]["41771983423143936"]
-        # when
-        obj = Role.from_dict(data)
-        # then
-        self.assertEqual(obj.id, 41771983423143936)
-        self.assertEqual(obj.name, "WE DEM BOYZZ!!!!!!")
-        self.assertFalse(obj.managed)
-
-    def test_should_convert_to_dict(self):
-        # given
-        role = create_role(id=42, name="Special Name", managed=True)
-        # when/then
-        self.assertDictEqual(
-            role.asdict(), {"id": 42, "name": "Special Name", "managed": True}
-        )
-
-    def test_sanitize_role_name(self):
-        # given
-        role_name_input = "x" * 110
-        role_name_expected = "x" * 100
-        # when
-        result = Role.sanitize_name(role_name_input)
-        # then
-        self.assertEqual(result, role_name_expected)
-
-
-class TestGuild(TestCase):
-    def test_should_create_new_object(self):
-        # given
-        role_a = create_role()
-        # when
-        obj = Guild(id="42", name=123, roles=[role_a])
-        # then
-        self.assertEqual(obj.id, 42)
-        self.assertEqual(obj.name, "123")
-        self.assertEqual(obj.roles, frozenset([role_a]))
-
-    def test_should_create_from_dict(self):
-        # given
-        data = example_objects["guilds"]["2909267986263572999"]
-        # when
-        obj = Guild.from_dict(data)
-        # then
-        self.assertEqual(obj.id, 2909267986263572999)
-        self.assertEqual(obj.name, "Mason's Test Server")
-        (first_role,) = obj.roles
-        self.assertEqual(first_role.id, 2909267986263572999)
-
-    def test_should_raise_error_when_role_type_is_wrong(self):
-        with self.assertRaises(TypeError):
-            create_guild(roles=[create_role(), "invalid"])
-
-
-class TestGuildMember(TestCase):
-    def test_should_create_new_object(self):
-        # given
-        user = create_user()
-        # when
-        obj = GuildMember(user=user, nick="x" * 40, roles=[1, 2])
-        # then
-        self.assertEqual(obj.user, user)
-        self.assertEqual(obj.nick, "x" * 32)
-        self.assertEqual(obj.roles, frozenset([1, 2]))
-
-    def test_should_create_from_dict_empty(self):
-        # given
-        data = example_objects["guildMembers"]["1"]
-        # when
-        obj = GuildMember.from_dict(data)
-        # then
-        self.assertIsNone(obj.user)
-        self.assertSetEqual(obj.roles, set())
-        self.assertIsNone(obj.nick)
-
-    def test_should_create_from_dict_full(self):
-        # given
-        data = example_objects["guildMembers"]["2"]
-        # when
-        obj = GuildMember.from_dict(data)
-        # then
-        self.assertEqual(obj.user.username, "Nelly")
-        self.assertSetEqual(obj.roles, {197150972374548480, 41771983423143936})
-        self.assertEqual(obj.nick, "Nelly the great")
-
-    def test_should_raise_error_when_user_type_is_wrong(self):
-        with self.assertRaises(TypeError):
-            create_guild_member(user="invalid")
-
-    def test_should_raise_error_when_role_type_is_wrong(self):
-        with self.assertRaises(TypeError):
-            GuildMember(roles=[1, 2, "invalid"])
-
-    def test_sanitize_nick(self):
-        # given
-        nick_input = "x" * 40
-        nick_expected = "x" * 32
-        # when
-        result = GuildMember.sanitize_nick(nick_input)
-        # then
-        self.assertEqual(result, nick_expected)
-
-
-example_objects = _fetch_example_objects()

allianceauth/services/modules/discord/managers.py

@@ -1,33 +1,30 @@
 import logging
 from urllib.parse import urlencode
 
-from requests.exceptions import HTTPError
 from requests_oauthlib import OAuth2Session
+from requests.exceptions import HTTPError
 
-from django.contrib.auth.models import Group, User
+from django.contrib.auth.models import User, Group
 from django.db import models
 from django.utils.timezone import now
 
+from allianceauth.services.hooks import NameFormatter
+
 from . import __title__
 from .app_settings import (
     DISCORD_APP_ID,
     DISCORD_APP_SECRET,
+    DISCORD_BOT_TOKEN,
     DISCORD_CALLBACK_URL,
     DISCORD_GUILD_ID,
-    DISCORD_SYNC_NAMES,
-)
-from .core import calculate_roles_for_user, create_bot_client
-from .core import group_to_role as core_group_to_role
-from .core import server_name as core_server_name
-from .core import user_formatted_nick
-from .discord_client import (
-    DISCORD_OAUTH_BASE_URL,
-    DISCORD_OAUTH_TOKEN_URL,
-    DiscordApiBackoff,
-    DiscordClient,
+    DISCORD_SYNC_NAMES
 )
+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
 
+
 logger = LoggerAddTag(logging.getLogger(__name__), __title__)
 
 
@@ -59,68 +56,79 @@ class DiscordUserManager(models.Manager):
         Returns: True on success, else False or raises exception
         """
         try:
-            nickname = user_formatted_nick(user) if DISCORD_SYNC_NAMES else None
+            nickname = self.user_formatted_nick(user) if DISCORD_SYNC_NAMES else None
+            group_names = self.user_group_names(user)
             access_token = self._exchange_auth_code_for_token(authorization_code)
             user_client = DiscordClient(access_token, is_rate_limited=is_rate_limited)
             discord_user = user_client.current_user()
-            bot_client = create_bot_client(is_rate_limited=is_rate_limited)
-            roles, changed = calculate_roles_for_user(
-                user=user, client=bot_client, discord_uid=discord_user.id
-            )
-            if changed is None:
-                # Handle new member
-                created = bot_client.add_guild_member(
-                    guild_id=DISCORD_GUILD_ID,
-                    user_id=discord_user.id,
-                    access_token=access_token,
-                    role_ids=list(roles.ids()),
-                    nick=nickname
-                )
-                if not created:
-                    logger.warning(
-                        "Failed to add user %s with Discord ID %s to Discord server",
-                        user,
-                        discord_user.id,
-                    )
-                    return False
-            else:
-                # Handle existing member
-                logger.debug(
-                    "User %s with Discord ID %s is already a member. Forcing a Refresh",
-                    user,
-                    discord_user.id,
-                )
-                # Force an update cause the discord API won't do it for us.
-                updated = bot_client.modify_guild_member(
-                    guild_id=DISCORD_GUILD_ID,
-                    user_id=discord_user.id,
-                    role_ids=list(roles.ids()),
-                    nick=nickname
-                )
-                if not updated:
-                    # Could not update the new user so fail.
-                    logger.warning(
-                        "Failed to add user %s with Discord ID %s to Discord server",
-                        user,
-                        discord_user.id,
-                    )
-                    return False
+            user_id = discord_user['id']
+            bot_client = self._bot_client(is_rate_limited=is_rate_limited)
 
-            self.update_or_create(
-                user=user,
-                defaults={
-                    'uid': discord_user.id,
-                    'username': discord_user.username[:32],
-                    'discriminator': discord_user.discriminator[:4],
-                    'activated': now()
-                }
+            if group_names:
+                role_ids = match_or_create_roles_from_names(
+                    client=bot_client,
+                    guild_id=DISCORD_GUILD_ID,
+                    role_names=group_names
+                ).ids()
+            else:
+                role_ids = None
+
+            created = bot_client.add_guild_member(
+                guild_id=DISCORD_GUILD_ID,
+                user_id=user_id,
+                access_token=access_token,
+                role_ids=role_ids,
+                nick=nickname
             )
-            logger.info(
-                "Added user %s with Discord ID %s to Discord server",
-                user,
-                discord_user.id
-            )
-            return True
+            if created is not False:
+                if created is None:
+                    logger.debug(
+                        "User %s with Discord ID %s is already a member. Forcing a Refresh",
+                        user,
+                        user_id,
+                    )
+
+                    # Force an update cause the discord API won't do it for us.
+                    if role_ids:
+                        role_ids = list(role_ids)
+
+                    updated = bot_client.modify_guild_member(
+                        guild_id=DISCORD_GUILD_ID,
+                        user_id=user_id,
+                        role_ids=role_ids,
+                        nick=nickname
+                    )
+
+                    if not updated:
+                        # Could not update the new user so fail.
+                        logger.warning(
+                            "Failed to add user %s with Discord ID %s to Discord server",
+                            user,
+                            user_id,
+                        )
+                        return False
+
+                self.update_or_create(
+                    user=user,
+                    defaults={
+                        'uid': user_id,
+                        'username': discord_user['username'][:32],
+                        'discriminator': discord_user['discriminator'][:4],
+                        'activated': now()
+                    }
+                )
+                logger.info(
+                    "Added user %s with Discord ID %s to Discord server", user, user_id
+                )
+                return True
+
+            else:
+                logger.warning(
+                    "Failed to add user %s with Discord ID %s to Discord server",
+                    user,
+                    user_id,
+                )
+                return False
 
         except (HTTPError, ConnectionError, DiscordApiBackoff) as ex:
             logger.exception(
@@ -128,6 +136,31 @@ class DiscordUserManager(models.Manager):
             )
             return False
 
+    @staticmethod
+    def user_formatted_nick(user: User) -> str:
+        """returns the name of the given users main character with name formatting
+        or None if user has no main
+        """
+        from .auth_hooks import DiscordService
+
+        if user.profile.main_character:
+            return NameFormatter(DiscordService(), user).format_name()
+        else:
+            return None
+
+    @staticmethod
+    def user_group_names(user: User, state_name: str = None) -> list:
+        """returns list of group names plus state the given user is a member of"""
+        if not state_name:
+            state_name = user.profile.state.name
+        group_names = (
+            [group.name for group in user.groups.all()] + [state_name]
+        )
+        logger.debug(
+            "Group names for roles updates of user %s are: %s", user, group_names
+        )
+        return group_names
+
     def user_has_account(self, user: User) -> bool:
         """Returns True if the user has an Discord account, else False
 
@@ -145,41 +178,60 @@ class DiscordUserManager(models.Manager):
             'permissions': str(cls.BOT_PERMISSIONS)
 
         })
-        return f'{DISCORD_OAUTH_BASE_URL}?{params}'
+        return f'{DiscordClient.OAUTH_BASE_URL}?{params}'
 
     @classmethod
     def generate_oauth_redirect_url(cls) -> str:
         oauth = OAuth2Session(
             DISCORD_APP_ID, redirect_uri=DISCORD_CALLBACK_URL, scope=cls.SCOPES
         )
-        url, _ = oauth.authorization_url(DISCORD_OAUTH_BASE_URL)
+        url, state = oauth.authorization_url(DiscordClient.OAUTH_BASE_URL)
         return url
 
     @staticmethod
     def _exchange_auth_code_for_token(authorization_code: str) -> str:
         oauth = OAuth2Session(DISCORD_APP_ID, redirect_uri=DISCORD_CALLBACK_URL)
         token = oauth.fetch_token(
-            DISCORD_OAUTH_TOKEN_URL,
+            DiscordClient.OAUTH_TOKEN_URL,
             client_secret=DISCORD_APP_SECRET,
             code=authorization_code
         )
         logger.debug("Received token from OAuth")
         return token['access_token']
 
-    @staticmethod
-    def group_to_role(group: Group) -> dict:
-        """Fetch the Discord role matching the given Django group by name.
+    @classmethod
+    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
 
-        Returns:
-            - Discord role as dict
-            - empty dict if no matching role found
+        Params:
+        - use_cache: When set False will force an API call to get the server name
         """
-        role = core_group_to_role(group)
-        return role.asdict() if role else dict()
+        try:
+            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 server_name(use_cache: bool = True) -> str:
-        """Fetches the name of the current Discord server.
-        This method is kept to ensure backwards compatibility of this API.
-        """
-        return core_server_name(use_cache)
+    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/models.py

@@ -1,5 +1,4 @@
 import logging
-from typing import Optional
 
 from requests.exceptions import HTTPError
 
@@ -7,17 +6,13 @@ from django.contrib.auth.models import User
 from django.db import models
 from django.utils.translation import gettext_lazy
 
+from allianceauth.groupmanagement.models import ReservedGroupName
 from allianceauth.notifications import notify
 
 from . import __title__
 from .app_settings import DISCORD_GUILD_ID
-from .core import (
-    create_bot_client,
-    default_bot_client,
-    calculate_roles_for_user,
-    user_formatted_nick
-)
-from .discord_client import DiscordApiBackoff
+from .discord_client import DiscordApiBackoff, DiscordClient, DiscordRoles
+from .discord_client.helpers import match_or_create_roles_from_names
 from .managers import DiscordUserManager
 from .utils import LoggerAddTag
 
@@ -26,13 +21,14 @@ logger = LoggerAddTag(logging.getLogger(__name__), __title__)
 
 
 class DiscordUser(models.Model):
-    """The Discord user account of an Auth user."""
+
+    USER_RELATED_NAME = 'discord'
 
     user = models.OneToOneField(
         User,
         primary_key=True,
         on_delete=models.CASCADE,
-        related_name='discord',
+        related_name=USER_RELATED_NAME,
         help_text='Auth user owning this Discord account'
     )
     uid = models.BigIntegerField(
@@ -84,21 +80,24 @@ class DiscordUser(models.Model):
         - False on error or raises exception
         """
         if not nickname:
-            nickname = user_formatted_nick(self.user)
-        if not nickname:
-            return False
-        success = default_bot_client.modify_guild_member(
-            guild_id=DISCORD_GUILD_ID,
-            user_id=self.uid,
-            nick=nickname
-        )
-        if success:
-            logger.info('Nickname for %s has been updated', self.user)
-        else:
-            logger.warning('Failed to update nickname for %s', self.user)
-        return success
+            nickname = DiscordUser.objects.user_formatted_nick(self.user)
+        if nickname:
+            client = DiscordUser.objects._bot_client()
+            success = client.modify_guild_member(
+                guild_id=DISCORD_GUILD_ID,
+                user_id=self.uid,
+                nick=nickname
+            )
+            if success:
+                logger.info('Nickname for %s has been updated', self.user)
+            else:
+                logger.warning('Failed to update nickname for %s', self.user)
+            return success
 
-    def update_groups(self, state_name: str = None) -> Optional[bool]:
+        else:
+            return False
+
+    def update_groups(self, state_name: str = None) -> bool:
         """update groups for a user based on his current group memberships.
         Will add or remove roles of a user as needed.
 
@@ -110,18 +109,57 @@ class DiscordUser(models.Model):
         - None if user is no longer a member of the Discord server
         - False on error or raises exception
         """
-        new_roles, is_changed = calculate_roles_for_user(
-            user=self.user,
-            client=default_bot_client,
-            discord_uid=self.uid,
-            state_name=state_name
-        )
-        if is_changed is None:
-            logger.debug('User is not a member of this guild %s', self.user)
+        client = DiscordUser.objects._bot_client()
+        member_roles = self._determine_member_roles(client)
+        if member_roles is None:
             return None
-        if is_changed:
+        return self._update_roles_if_needed(client, state_name, member_roles)
+
+    def _determine_member_roles(self, client: DiscordClient) -> DiscordRoles:
+        """Determine the roles of the current member / user."""
+        member_info = client.guild_member(guild_id=DISCORD_GUILD_ID, user_id=self.uid)
+        if member_info is None:
+            return None  # User is no longer a member
+        guild_roles = DiscordRoles(client.guild_roles(guild_id=DISCORD_GUILD_ID))
+        logger.debug('Current guild roles: %s', guild_roles.ids())
+        if 'roles' in member_info:
+            if not guild_roles.has_roles(member_info['roles']):
+                guild_roles = DiscordRoles(
+                    client.guild_roles(guild_id=DISCORD_GUILD_ID, use_cache=False)
+                )
+                if not guild_roles.has_roles(member_info['roles']):
+                    raise RuntimeError(
+                        'Member {} has unknown roles: {}'.format(
+                            self.user,
+                            set(member_info['roles']).difference(guild_roles.ids())
+                        )
+                    )
+            return guild_roles.subset(member_info['roles'])
+        raise RuntimeError('member_info from %s is not valid' % self.user)
+
+    def _update_roles_if_needed(
+        self, client: DiscordClient, state_name: str, member_roles: DiscordRoles
+    ) -> bool:
+        """Update the roles of this member/user if needed."""
+        requested_roles = match_or_create_roles_from_names(
+            client=client,
+            guild_id=DISCORD_GUILD_ID,
+            role_names=DiscordUser.objects.user_group_names(
+                user=self.user, state_name=state_name
+            )
+        )
+        logger.debug(
+            'Requested roles for user %s: %s', self.user, requested_roles.ids()
+        )
+        logger.debug('Current roles user %s: %s', self.user, member_roles.ids())
+        reserved_role_names = ReservedGroupName.objects.values_list("name", flat=True)
+        member_roles_reserved = member_roles.subset(role_names=reserved_role_names)
+        member_roles_managed = member_roles.subset(managed_only=True)
+        member_roles_persistent = member_roles_managed.union(member_roles_reserved)
+        if requested_roles != member_roles.difference(member_roles_persistent):
             logger.debug('Need to update roles for user %s', self.user)
-            success = default_bot_client.modify_guild_member(
+            new_roles = requested_roles.union(member_roles_persistent)
+            success = client.modify_guild_member(
                 guild_id=DISCORD_GUILD_ID,
                 user_id=self.uid,
                 role_ids=list(new_roles.ids())
@@ -134,7 +172,7 @@ class DiscordUser(models.Model):
         logger.info('No need to update roles for user %s', self.user)
         return True
 
-    def update_username(self) -> Optional[bool]:
+    def update_username(self) -> bool:
         """Updates the username incl. the discriminator
         from the Discord server and saves it
 
@@ -143,9 +181,8 @@ class DiscordUser(models.Model):
         - 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(
-            guild_id=DISCORD_GUILD_ID, user_id=self.uid
-        )
+        client = DiscordUser.objects._bot_client()
+        user_info = client.guild_member(guild_id=DISCORD_GUILD_ID, user_id=self.uid)
         if user_info is None:
             success = None
         elif (
@@ -169,7 +206,7 @@ class DiscordUser(models.Model):
         notify_user: bool = False,
         is_rate_limited: bool = True,
         handle_api_exceptions: bool = False
-    ) -> Optional[bool]:
+    ) -> bool:
         """Deletes the Discount user both on the server and locally
 
         Params:
@@ -184,7 +221,7 @@ class DiscordUser(models.Model):
         """
         try:
             _user = self.user
-            client = create_bot_client(is_rate_limited=is_rate_limited)
+            client = DiscordUser.objects._bot_client(is_rate_limited=is_rate_limited)
             success = client.remove_guild_member(
                 guild_id=DISCORD_GUILD_ID, user_id=self.uid
             )
@@ -204,13 +241,15 @@ class DiscordUser(models.Model):
                         )
                     logger.info('Account for user %s was deleted.', _user)
                     return True
-                logger.debug('Account for user %s was already deleted.', _user)
-                return None
+                else:
+                    logger.debug('Account for user %s was already deleted.', _user)
+                    return None
 
-            logger.warning(
-                'Failed to remove user %s from the Discord server', _user
-            )
-            return False
+            else:
+                logger.warning(
+                    'Failed to remove user %s from the Discord server', _user
+                )
+                return False
 
         except (HTTPError, ConnectionError, DiscordApiBackoff) as ex:
             if handle_api_exceptions:
@@ -218,4 +257,5 @@ class DiscordUser(models.Model):
                     'Failed to remove user %s from Discord server: %s',self.user, ex
                 )
                 return False
-            raise ex
+            else:
+                raise ex

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

@@ -1,6 +1,19 @@
 from django.contrib.auth.models import Group
-
 from allianceauth.tests.auth_utils import AuthUtils
+from ..discord_client.tests import (    # noqa
+    TEST_GUILD_ID,
+    TEST_USER_ID,
+    TEST_USER_NAME,
+    TEST_USER_DISCRIMINATOR,
+    create_role,
+    ROLE_ALPHA,
+    ROLE_BRAVO,
+    ROLE_CHARLIE,
+    ROLE_CHARLIE_2,
+    ROLE_MIKE,
+    ALL_ROLES,
+    create_user_info
+)
 
 DEFAULT_AUTH_GROUP = 'Member'
 MODULE_PATH = 'allianceauth.services.modules.discord'

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

@@ -35,17 +35,17 @@ import logging
 from uuid import uuid1
 import random
 
+from django.core.cache import caches
 from django.contrib.auth.models import User, Group
 
 from allianceauth.services.modules.discord.models import DiscordUser
-from allianceauth.utils.cache import get_redis_client
 
 logger = logging.getLogger('allianceauth')
 MAX_RUNS = 3
 
-
 def clear_cache():
-    redis = get_redis_client()
+    default_cache = caches['default']
+    redis = default_cache.get_master_client()
     redis.flushall()
     logger.info('Cache flushed')
 

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

@@ -1,32 +1,26 @@
-from unittest.mock import patch
-
+from django.test import TestCase, RequestFactory
 from django.contrib.admin.sites import AdminSite
 from django.contrib.auth.models import User
-from django.test import RequestFactory
 from django.utils.timezone import now
 
 from allianceauth.authentication.models import CharacterOwnership
 from allianceauth.eveonline.models import (
-    EveAllianceInfo,
-    EveCharacter,
-    EveCorporationInfo,
+    EveCharacter, EveCorporationInfo, EveAllianceInfo
 )
-from allianceauth.utils.testing import NoSocketsTestCase
 
 from ....admin import (
-    MainAllianceFilter,
-    MainCorporationsFilter,
-    ServicesUserAdmin,
-    user_main_organization,
     user_profile_pic,
     user_username,
+    user_main_organization,
+    ServicesUserAdmin,
+    MainCorporationsFilter,
+    MainAllianceFilter
 )
 from ..admin import DiscordUserAdmin
 from ..models import DiscordUser
-from . import MODULE_PATH
 
 
-class TestDataMixin(NoSocketsTestCase):
+class TestDataMixin(TestCase):
 
     @classmethod
     def setUpClass(cls):
@@ -174,7 +168,7 @@ class TestDataMixin(NoSocketsTestCase):
         )
 
 
-class TestColumnRendering(TestDataMixin, NoSocketsTestCase):
+class TestColumnRendering(TestDataMixin, TestCase):
 
     def test_user_profile_pic_u1(self):
         expected = (
@@ -235,7 +229,7 @@ class TestColumnRendering(TestDataMixin, NoSocketsTestCase):
     # actions
 
 
-class TestFilters(TestDataMixin, NoSocketsTestCase):
+class TestFilters(TestDataMixin, TestCase):
 
     def test_filter_main_corporations(self):
 
@@ -293,16 +287,3 @@ class TestFilters(TestDataMixin, NoSocketsTestCase):
         queryset = changelist.get_queryset(request)
         expected = [self.user_1.discord]
         self.assertSetEqual(set(queryset), set(expected))
-
-
-@patch(MODULE_PATH + ".admin.DiscordUser.delete_user")
-class TestDeleteQueryset(TestDataMixin, NoSocketsTestCase):
-    def test_should_delete_all_objects(self, mock_delete_user):
-        # given
-        request = self.factory.get('/')
-        request.user = self.user_1
-        queryset = DiscordUser.objects.filter(user__in=[self.user_2, self.user_3])
-        # when
-        self.modeladmin.delete_queryset(request, queryset)
-        # then
-        self.assertEqual(mock_delete_user.call_count, 2)

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

@@ -1,16 +0,0 @@
-from unittest.mock import patch
-
-from allianceauth.utils.testing import NoSocketsTestCase
-
-from ..api import discord_guild_id
-from . import MODULE_PATH
-
-
-class TestDiscordGuildId(NoSocketsTestCase):
-    @patch(MODULE_PATH + ".api.DISCORD_GUILD_ID", "123")
-    def test_should_return_guild_id_when_configured(self):
-        self.assertEqual(discord_guild_id(), 123)
-
-    @patch(MODULE_PATH + ".api.DISCORD_GUILD_ID", "")
-    def test_should_return_none_when_not_configured(self):
-        self.assertIsNone(discord_guild_id())

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

@@ -1,23 +1,23 @@
 from unittest.mock import patch
 
-from django.test import RequestFactory
+from django.test import TestCase, RequestFactory
 from django.test.utils import override_settings
 
 from allianceauth.notifications.models import Notification
 from allianceauth.tests.auth_utils import AuthUtils
-from allianceauth.utils.testing import NoSocketsTestCase
 
+from . import TEST_USER_NAME, TEST_USER_ID, add_permissions_to_members, MODULE_PATH
 from ..auth_hooks import DiscordService
-from ..discord_client.tests.factories import TEST_USER_ID, TEST_USER_NAME
+from ..discord_client import DiscordClient
 from ..models import DiscordUser
 from ..utils import set_logger_to_file
-from . import MODULE_PATH, add_permissions_to_members
+
 
 logger = set_logger_to_file(MODULE_PATH + '.auth_hooks', __file__)
 
 
-@override_settings(CELERY_ALWAYS_EAGER=True, CELERY_EAGER_PROPAGATES_EXCEPTIONS=True)
-class TestDiscordService(NoSocketsTestCase):
+@override_settings(CELERY_ALWAYS_EAGER=True)
+class TestDiscordService(TestCase):
 
     def setUp(self):
         self.member = AuthUtils.create_member(TEST_USER_NAME)
@@ -64,11 +64,11 @@ class TestDiscordService(NoSocketsTestCase):
 
     @patch(MODULE_PATH + '.models.notify')
     @patch(MODULE_PATH + '.tasks.DiscordUser')
-    @patch(MODULE_PATH + '.models.create_bot_client')
+    @patch(MODULE_PATH + '.managers.DiscordClient', spec=DiscordClient)
     def test_validate_user(
-        self, mock_create_bot_client, mock_DiscordUser, mock_notify
+        self, mock_DiscordClient, mock_DiscordUser, mock_notify
     ):
-        mock_create_bot_client.return_value.remove_guild_member.return_value = True
+        mock_DiscordClient.return_value.remove_guild_member.return_value = True
 
         # Test member is not deleted
         service = self.service()
@@ -92,38 +92,33 @@ class TestDiscordService(NoSocketsTestCase):
         service.sync_nicknames_bulk([self.member])
         self.assertTrue(mock_update_nicknames_bulk.delay.called)
 
-    @patch(MODULE_PATH + '.models.create_bot_client')
-    def test_delete_user_is_member(self, mock_create_bot_client):
-        # given
-        mock_create_bot_client.return_value.remove_guild_member.return_value = True
+    @patch(MODULE_PATH + '.managers.DiscordClient', spec=DiscordClient)
+    def test_delete_user_is_member(self, mock_DiscordClient):
+        mock_DiscordClient.return_value.remove_guild_member.return_value = True
+
         service = self.service()
-        # when
         service.delete_user(self.member, notify_user=True)
-        # then
-        self.assertTrue(mock_create_bot_client.return_value.remove_guild_member.called)
+
+        self.assertTrue(mock_DiscordClient.return_value.remove_guild_member.called)
         self.assertFalse(DiscordUser.objects.filter(user=self.member).exists())
         self.assertTrue(Notification.objects.filter(user=self.member).exists())
 
-    @patch(MODULE_PATH + '.models.create_bot_client')
-    def test_delete_user_is_not_member(self, mock_create_bot_client):
-        # given
-        mock_create_bot_client.return_value.remove_guild_member.return_value = True
-        service = self.service()
-        # when
-        service.delete_user(self.none_member)
-        # then
-        self.assertFalse(mock_create_bot_client.return_value.remove_guild_member.called)
+    @patch(MODULE_PATH + '.managers.DiscordClient', spec=DiscordClient)
+    def test_delete_user_is_not_member(self, mock_DiscordClient):
+        mock_DiscordClient.return_value.remove_guild_member.return_value = True
 
-    @patch(MODULE_PATH + '.auth_hooks.server_name')
-    def test_render_services_ctrl_with_username(self, mock_server_name):
-        # given
-        mock_server_name.return_value = "My server"
+        service = self.service()
+        service.delete_user(self.none_member)
+
+        self.assertFalse(mock_DiscordClient.return_value.remove_guild_member.called)
+
+    @patch(MODULE_PATH + '.managers.DiscordClient', spec=DiscordClient)
+    def test_render_services_ctrl_with_username(self, mock_DiscordClient):
         service = self.service()
         request = self.factory.get('/services/')
         request.user = self.member
-        # when
+
         response = service.render_services_ctrl(request)
-        # then
         self.assertTemplateUsed(service.service_ctrl_template)
         self.assertIn('/discord/reset/', response)
         self.assertIn('/discord/deactivate/', response)
@@ -135,18 +130,15 @@ class TestDiscordService(NoSocketsTestCase):
         response = service.render_services_ctrl(request)
         self.assertIn('/discord/activate/', response)
 
-    @patch(MODULE_PATH + '.auth_hooks.server_name')
-    def test_render_services_ctrl_wo_username(self, mock_server_name):
-        # given
-        mock_server_name.return_value = "My server"
+    @patch(MODULE_PATH + '.managers.DiscordClient', spec=DiscordClient)
+    def test_render_services_ctrl_wo_username(self, mock_DiscordClient):
         my_member = AuthUtils.create_member('John Doe')
         DiscordUser.objects.create(user=my_member, uid=111222333)
         service = self.service()
         request = self.factory.get('/services/')
         request.user = my_member
-        # when
+
         response = service.render_services_ctrl(request)
-        # then
         self.assertTemplateUsed(service.service_ctrl_template)
         self.assertIn('/discord/reset/', response)
         self.assertIn('/discord/deactivate/', response)

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

@@ -1,221 +0,0 @@
-from unittest.mock import Mock, patch
-
-from requests.exceptions import HTTPError
-
-from django.contrib.auth.models import Group
-
-from allianceauth.groupmanagement.models import ReservedGroupName
-from allianceauth.tests.auth_utils import AuthUtils
-from allianceauth.utils.testing import NoSocketsTestCase
-
-from ..core import (
-    _user_group_names,
-    calculate_roles_for_user,
-    group_to_role,
-    server_name,
-    user_formatted_nick,
-)
-from ..discord_client import DiscordApiBackoff, DiscordClient, RolesSet
-from ..discord_client.tests.factories import TEST_USER_NAME, create_role
-from . import MODULE_PATH, TEST_MAIN_ID, TEST_MAIN_NAME
-
-
-class TestUserGroupNames(NoSocketsTestCase):
-    @classmethod
-    def setUpClass(cls):
-        super().setUpClass()
-        cls.group_1 = Group.objects.create(name="Group 1")
-        cls.group_2 = Group.objects.create(name="Group 2")
-
-    def setUp(self):
-        self.user = AuthUtils.create_member(TEST_USER_NAME)
-
-    def test_return_groups_and_state_names_for_user(self):
-        self.user.groups.add(self.group_1)
-        result = _user_group_names(self.user)
-        expected = ["Group 1", "Member"]
-        self.assertSetEqual(set(result), set(expected))
-
-    def test_return_state_only_if_user_has_no_groups(self):
-        result = _user_group_names(self.user)
-        expected = ["Member"]
-        self.assertSetEqual(set(result), set(expected))
-
-
-class TestUserFormattedNick(NoSocketsTestCase):
-    def setUp(self):
-        self.user = AuthUtils.create_user(TEST_USER_NAME)
-
-    def test_return_nick_when_user_has_main(self):
-        AuthUtils.add_main_character_2(self.user, TEST_MAIN_NAME, TEST_MAIN_ID)
-        result = user_formatted_nick(self.user)
-        expected = TEST_MAIN_NAME
-        self.assertEqual(result, expected)
-
-    def test_return_none_if_user_has_no_main(self):
-        result = user_formatted_nick(self.user)
-        self.assertIsNone(result)
-
-
-@patch(MODULE_PATH + ".core.default_bot_client", spec=True)
-class TestRoleForGroup(NoSocketsTestCase):
-    def test_return_role_if_found(self, mock_bot_client):
-        # given
-        role = create_role(name="alpha")
-        mock_bot_client.match_role_from_name.side_effect = (
-            lambda guild_id, role_name: role if role.name == role_name else None
-        )
-        group = Group.objects.create(name="alpha")
-        # when/then
-        self.assertEqual(group_to_role(group), role)
-
-    def test_return_empty_dict_if_not_found(self, mock_bot_client):
-        # given
-        role = create_role(name="alpha")
-        mock_bot_client.match_role_from_name.side_effect = (
-            lambda guild_id, role_name: role if role.name == role_name else None
-        )
-        group = Group.objects.create(name="unknown")
-        # when/then
-        self.assertIsNone(group_to_role(group))
-
-
-@patch(MODULE_PATH + ".core.default_bot_client", spec=True)
-@patch(MODULE_PATH + ".core.logger", spec=True)
-class TestServerName(NoSocketsTestCase):
-    @classmethod
-    def setUpClass(cls):
-        super().setUpClass()
-        cls.user = AuthUtils.create_user(TEST_USER_NAME)
-
-    def test_returns_name_when_api_returns_it(self, mock_logger, mock_bot_client):
-        # given
-        my_server_name = "El Dorado"
-        mock_bot_client.guild_name.return_value = my_server_name
-        # when
-        self.assertEqual(server_name(), my_server_name)
-        # then
-        self.assertFalse(mock_logger.warning.called)
-
-    def test_returns_empty_string_when_api_throws_http_error(
-        self, mock_logger, mock_bot_client
-    ):
-        mock_exception = HTTPError("Test exception")
-        mock_exception.response = Mock(**{"status_code": 440})
-        mock_bot_client.guild_name.side_effect = mock_exception
-
-        self.assertEqual(server_name(), "")
-        self.assertFalse(mock_logger.warning.called)
-
-    def test_returns_empty_string_when_api_throws_service_error(
-        self, mock_logger, mock_bot_client
-    ):
-        mock_bot_client.guild_name.side_effect = DiscordApiBackoff(1000)
-
-        self.assertEqual(server_name(), "")
-        self.assertFalse(mock_logger.warning.called)
-
-    def test_returns_empty_string_when_api_throws_unexpected_error(
-        self, mock_logger, mock_bot_client
-    ):
-        mock_bot_client.guild_name.side_effect = RuntimeError
-
-        self.assertEqual(server_name(), "")
-        self.assertTrue(mock_logger.warning.called)
-
-
-class TestCalculateRolesForUser(NoSocketsTestCase):
-    @classmethod
-    def setUpClass(cls):
-        super().setUpClass()
-        cls.user = AuthUtils.create_user(TEST_USER_NAME)
-
-    def test_should_return_roles_for_new_member(self):
-        # given
-        roles = RolesSet([create_role()])
-        my_client = Mock(spec=DiscordClient)
-        my_client.guild_member_roles.return_value = RolesSet([])
-        my_client.match_or_create_roles_from_names_2.return_value = roles
-        # when
-        roles_calculated, changed = calculate_roles_for_user(self.user, my_client, 42)
-        # then
-        self.assertTrue(changed)
-        self.assertEqual(roles_calculated, roles)
-
-    def test_should_return_changed_roles_for_existing_member(self):
-        # given
-        role_a = create_role()
-        role_b = create_role()
-        roles_current = RolesSet([role_a])
-        roles_matching = RolesSet([role_a, role_b])
-        my_client = Mock(spec=DiscordClient)
-        my_client.guild_member_roles.return_value = roles_current
-        my_client.match_or_create_roles_from_names_2.return_value = roles_matching
-        # when
-        roles_calculated, changed = calculate_roles_for_user(self.user, my_client, 42)
-        # then
-        self.assertTrue(changed)
-        self.assertEqual(roles_calculated, roles_matching)
-
-    def test_should_indicate_when_roles_are_unchanged(self):
-        # given
-        role_a = create_role()
-        roles_current = RolesSet([role_a])
-        roles_matching = RolesSet([role_a])
-        my_client = Mock(spec=DiscordClient)
-        my_client.guild_member_roles.return_value = roles_current
-        my_client.match_or_create_roles_from_names_2.return_value = roles_matching
-        # when
-        roles_calculated, changed = calculate_roles_for_user(self.user, my_client, 42)
-        # then
-        self.assertFalse(changed)
-        self.assertEqual(roles_calculated, roles_matching)
-
-    def test_should_indicate_when_user_is_no_guild_member(self):
-        # given
-        role_a = create_role()
-        roles_matching = RolesSet([role_a])
-        my_client = Mock(spec=DiscordClient)
-        my_client.guild_member_roles.return_value = None
-        my_client.match_or_create_roles_from_names_2.return_value = roles_matching
-        # when
-        roles_calculated, changed = calculate_roles_for_user(self.user, my_client, 42)
-        # then
-        self.assertIsNone(changed)
-        self.assertEqual(roles_calculated, roles_matching)
-
-    def test_should_preserve_managed_roles_for_existing_member(self):
-        # given
-        role_a = create_role()
-        role_b = create_role()
-        role_m = create_role(managed=True)
-        roles_current = RolesSet([role_a, role_m])
-        roles_matching = RolesSet([role_b])
-        my_client = Mock(spec=DiscordClient)
-        my_client.guild_member_roles.return_value = roles_current
-        my_client.match_or_create_roles_from_names_2.return_value = roles_matching
-        # when
-        roles_calculated, changed = calculate_roles_for_user(self.user, my_client, 42)
-        # then
-        self.assertTrue(changed)
-        self.assertEqual(roles_calculated, RolesSet([role_b, role_m]))
-
-    def test_should_preserve_reserved_roles_for_existing_member(self):
-        # given
-        role_a = create_role()
-        role_b = create_role()
-        role_c1 = create_role(name="charlie")
-        role_c2 = create_role(name="Charlie")
-        roles_current = RolesSet([role_a, role_c1, role_c2])
-        roles_matching = RolesSet([role_b])
-        my_client = Mock(spec=DiscordClient)
-        my_client.guild_member_roles.return_value = roles_current
-        my_client.match_or_create_roles_from_names_2.return_value = roles_matching
-        ReservedGroupName.objects.create(
-            name="charlie", reason="dummy", created_by="xyz"
-        )
-        # when
-        roles_calculated, changed = calculate_roles_for_user(self.user, my_client, 42)
-        # then
-        self.assertTrue(changed)
-        self.assertEqual(roles_calculated, RolesSet([role_b, role_c1, role_c2]))

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

@@ -4,67 +4,54 @@ Testing all components of the service, with the exception of the Discord API.
 
 Please note that these tests require Redis and will flush it
 """
-import dataclasses
-import json
+from collections import namedtuple
 import logging
-from unittest.mock import Mock, patch
+from unittest.mock import patch, Mock
 from uuid import uuid1
 
-import requests_mock
+from django_webtest import WebTest
 from requests.exceptions import HTTPError
+import requests_mock
 
 from django.contrib.auth.models import Group, User
+from django_redis import get_redis_connection
 from django.shortcuts import reverse
-from django.test import TransactionTestCase, override_settings
-from django_webtest import WebTest
+from django.test import TransactionTestCase, TestCase
+from django.test.utils import override_settings
 
 from allianceauth.authentication.models import State
 from allianceauth.eveonline.models import EveCharacter
-from allianceauth.groupmanagement.models import ReservedGroupName
 from allianceauth.notifications.models import Notification
 from allianceauth.tests.auth_utils import AuthUtils
-from allianceauth.utils.cache import get_redis_client
-from allianceauth.utils.testing import NoSocketsTestCase
 
-from .. import tasks
-from ..core import create_bot_client
-from ..discord_client import DiscordApiBackoff
-from ..discord_client.app_settings import DISCORD_API_BASE_URL
-from ..discord_client.tests.factories import (
+from . import (
     TEST_GUILD_ID,
-    TEST_USER_ID,
     TEST_USER_NAME,
-    create_discord_error_response_unknown_member,
-    create_discord_guild_member_object,
-    create_discord_guild_object,
-    create_discord_role_object,
-    create_discord_user_object,
+    TEST_USER_ID,
+    TEST_USER_DISCRIMINATOR,
+    TEST_MAIN_NAME,
+    TEST_MAIN_ID,
+    MODULE_PATH,
+    add_permissions_to_members,
+    ROLE_ALPHA,
+    ROLE_BRAVO,
+    ROLE_CHARLIE,
+    ROLE_MIKE,
+    create_role,
+    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 MODULE_PATH, TEST_MAIN_ID, TEST_MAIN_NAME, add_permissions_to_members
+from .. import tasks
 
 logger = logging.getLogger('allianceauth')
 
-ROLE_ALPHA = create_discord_role_object(id=1, name="alpha")
-ROLE_BRAVO = create_discord_role_object(id=2, name="bravo")
-ROLE_CHARLIE = create_discord_role_object(id=3, name="charlie")
-ROLE_CHARLIE_2 = create_discord_role_object(id=4, name="Charlie")  # Discord roles are case sensitive
-ROLE_MIKE = create_discord_role_object(id=13, name="mike", managed=True)
-ROLE_MEMBER = create_discord_role_object(99, 'Member')
-ROLE_BLUE = create_discord_role_object(98, 'Blue')
-
-
-@dataclasses.dataclass(frozen=True)
-class DiscordRequest:
-    """Helper for comparing requests made to the Discord API."""
-    method: str
-    url: str
-    text: str = dataclasses.field(compare=False, default=None)
-
-    def json(self):
-        return json.loads(self.text)
-
+ROLE_MEMBER = create_role(99, 'Member')
+ROLE_BLUE = create_role(98, 'Blue')
 
+# Putting all requests to Discord into objects so we can compare them better
+DiscordRequest = namedtuple('DiscordRequest', ['method', 'url'])
 user_get_current_request = DiscordRequest(
     method='GET',
     url=f'{DISCORD_API_BASE_URL}users/@me'
@@ -100,7 +87,7 @@ remove_guild_member_request = DiscordRequest(
 
 
 def clear_cache():
-    redis = get_redis_client()
+    redis = get_redis_connection('default')
     redis.flushall()
     logger.info('Cache flushed')
 
@@ -115,13 +102,13 @@ def reset_testdata():
     Notification.objects.all().delete()
 
 
-@patch(MODULE_PATH + '.core.DISCORD_GUILD_ID', TEST_GUILD_ID)
 @patch(MODULE_PATH + '.models.DISCORD_GUILD_ID', TEST_GUILD_ID)
-@override_settings(CELERY_ALWAYS_EAGER=True, CELERY_EAGER_PROPAGATES_EXCEPTIONS=False)
+@override_settings(CELERY_ALWAYS_EAGER=True)
 @requests_mock.Mocker()
 class TestServiceFeatures(TransactionTestCase):
     fixtures = ['disable_analytics.json']
 
+
     @classmethod
     def setUpClass(cls):
         super().setUpClass()
@@ -205,7 +192,7 @@ class TestServiceFeatures(TransactionTestCase):
         requests_mocker.patch(modify_guild_member_request.url, status_code=204)
 
         # exhausting rate limit
-        client = create_bot_client()
+        client = DiscordUser.objects._bot_client()
         client._redis.set(
             name=client._KEY_GLOBAL_RATE_LIMIT_REMAINING,
             value=0,
@@ -221,6 +208,7 @@ class TestServiceFeatures(TransactionTestCase):
         requests_made = [
             DiscordRequest(r.method, r.url) for r in requests_mocker.request_history
         ]
+
         self.assertListEqual(requests_made, list())
 
     def test_when_member_is_demoted_to_guest_then_his_account_is_deleted(
@@ -258,7 +246,7 @@ class TestServiceFeatures(TransactionTestCase):
         # request mocks
         requests_mocker.get(
             guild_member_request.url,
-            json=create_discord_guild_member_object(roles=[3, 13, 99])
+            json={'user': create_user_info(), 'roles': ['3', '13', '99']}
         )
         requests_mocker.get(
             guild_roles_request.url,
@@ -294,7 +282,10 @@ class TestServiceFeatures(TransactionTestCase):
     ):
         requests_mocker.get(
             guild_member_request.url,
-            json=create_discord_guild_member_object(roles=[13, 99])
+            json={
+                'user': create_user_info(),
+                'roles': ['13', '99']
+            }
         )
         requests_mocker.get(
             guild_roles_request.url,
@@ -323,7 +314,10 @@ class TestServiceFeatures(TransactionTestCase):
     ):
         requests_mocker.get(
             guild_member_request.url,
-            json=create_discord_guild_member_object(roles=['13', '99'])
+            json={
+                'user': {'id': str(TEST_USER_ID), 'username': TEST_MAIN_NAME},
+                'roles': ['13', '99']
+            }
         )
         requests_mocker.get(
             guild_roles_request.url,
@@ -349,11 +343,11 @@ class TestServiceFeatures(TransactionTestCase):
         self.assertTrue(DiscordUser.objects.user_has_account(self.user))
 
 
-@override_settings(CELERY_ALWAYS_EAGER=True, CELERY_EAGER_PROPAGATES_EXCEPTIONS=True)
+@override_settings(CELERY_ALWAYS_EAGER=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 StateTestCase(NoSocketsTestCase):
+class StateTestCase(TestCase):
 
     def setUp(self):
         clear_cache()
@@ -437,7 +431,6 @@ class StateTestCase(NoSocketsTestCase):
             self.user.discord
 
 
-@patch(MODULE_PATH + '.core.DISCORD_GUILD_ID', TEST_GUILD_ID)
 @patch(MODULE_PATH + '.managers.DISCORD_GUILD_ID', TEST_GUILD_ID)
 @patch(MODULE_PATH + '.models.DISCORD_GUILD_ID', TEST_GUILD_ID)
 @requests_mock.Mocker()
@@ -456,25 +449,24 @@ class TestUserFeatures(WebTest):
         )
         add_permissions_to_members()
 
-    @patch(MODULE_PATH + '.views.messages', spec=True)
-    @patch(MODULE_PATH + '.managers.OAuth2Session', spec=True)
+    @patch(MODULE_PATH + '.views.messages')
+    @patch(MODULE_PATH + '.managers.OAuth2Session')
     def test_user_activation_normal(
         self, requests_mocker, mock_OAuth2Session, mock_messages
     ):
         # setup
         requests_mocker.get(
-            guild_infos_request.url, json=create_discord_guild_object()
+            guild_infos_request.url, json={'id': TEST_GUILD_ID, 'name': 'Test Guild'}
         )
         requests_mocker.get(
-            user_get_current_request.url, json=create_discord_user_object()
+            user_get_current_request.url,
+            json=create_user_info(
+                TEST_USER_ID, TEST_USER_NAME, TEST_USER_DISCRIMINATOR
+            )
         )
         requests_mocker.get(
-            guild_roles_request.url, json=[ROLE_ALPHA, ROLE_BRAVO, ROLE_MEMBER]
-        )
-        requests_mocker.get(
-            guild_member_request.url,
-            status_code=404,
-            json=create_discord_error_response_unknown_member()
+            guild_roles_request.url,
+            json=[ROLE_ALPHA, ROLE_BRAVO, ROLE_MIKE, ROLE_MEMBER]
         )
         requests_mocker.put(add_guild_member_request.url, status_code=201)
 
@@ -512,93 +504,33 @@ class TestUserFeatures(WebTest):
         for r in requests_mocker.request_history:
             obj = DiscordRequest(r.method, r.url)
             requests_made.append(obj)
-        self.assertIn(add_guild_member_request, requests_made)
 
-    @patch(MODULE_PATH + '.views.messages', spec=True)
-    @patch(MODULE_PATH + '.managers.OAuth2Session', spec=True)
-    def test_should_activate_existing_user_and_keep_managed_and_reserved_roles(
-        self, requests_mocker, mock_OAuth2Session, mock_messages
-    ):
-        # setup
-        requests_mocker.get(
-            guild_infos_request.url, json=create_discord_guild_object()
-        )
-        requests_mocker.get(
-            user_get_current_request.url, json=create_discord_user_object()
-        )
-        requests_mocker.get(
-            guild_roles_request.url, json=[
-                ROLE_ALPHA, ROLE_CHARLIE, ROLE_MEMBER, ROLE_MIKE
-            ]
-        )
-        requests_mocker.get(
-            guild_member_request.url,
-            json=create_discord_guild_member_object(roles=[1, 3, 13])
-        )
-        requests_mocker.patch(modify_guild_member_request.url, status_code=204)
-        ReservedGroupName.objects.create(
-            name="charlie", reason="dummy", created_by="xyz"
-        )
+        expected = [
+            guild_infos_request,
+            user_get_current_request,
+            guild_roles_request,
+            add_guild_member_request
+        ]
+        self.assertListEqual(requests_made, expected)
 
-        authentication_code = 'auth_code'
-        oauth_url = 'https://www.example.com/oauth'
-        state = ''
-        mock_OAuth2Session.return_value.authorization_url.return_value = \
-            oauth_url, state
-
-        # 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)
-
-        # 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(
-            response, expected_url=oauth_url, fetch_redirect_response=False
-        )
-
-        # simulate Discord callback
-        response = self.app.get(
-            reverse('discord:callback'), params={'code': authentication_code}
-        )
-
-        # user got a success message
-        self.assertTrue(mock_messages.success.called)
-        self.assertFalse(mock_messages.error.called)
-
-        my_request = None
-        for r in requests_mocker.request_history:
-            obj = DiscordRequest(r.method, r.url, r.text)
-            if obj == modify_guild_member_request:
-                my_request = obj
-                break
-        else:
-            self.fail("Request not found")
-        self.assertSetEqual(set(my_request.json()["roles"]), {3, 13, 99})
-
-    @patch(MODULE_PATH + '.views.messages', spec=True)
-    @patch(MODULE_PATH + '.managers.OAuth2Session', spec=True)
+    @patch(MODULE_PATH + '.views.messages')
+    @patch(MODULE_PATH + '.managers.OAuth2Session')
     def test_user_activation_failed(
         self, requests_mocker, mock_OAuth2Session, mock_messages
     ):
         # setup
         requests_mocker.get(
-            guild_infos_request.url, json=create_discord_guild_object()
+            guild_infos_request.url, json={'id': TEST_GUILD_ID, 'name': 'Test Guild'}
         )
         requests_mocker.get(
-            user_get_current_request.url, json=create_discord_user_object()
+            user_get_current_request.url,
+            json=create_user_info(
+                TEST_USER_ID, TEST_USER_NAME, TEST_USER_DISCRIMINATOR
+            )
         )
         requests_mocker.get(
-            guild_roles_request.url, json=[ROLE_ALPHA, ROLE_BRAVO, ROLE_MEMBER]
-        )
-        requests_mocker.get(
-            guild_member_request.url,
-            status_code=404,
-            json=create_discord_error_response_unknown_member()
+            guild_roles_request.url,
+            json=[ROLE_ALPHA, ROLE_BRAVO, ROLE_MIKE, ROLE_MEMBER]
         )
 
         mock_exception = HTTPError('error')
@@ -640,13 +572,20 @@ class TestUserFeatures(WebTest):
         for r in requests_mocker.request_history:
             obj = DiscordRequest(r.method, r.url)
             requests_made.append(obj)
-        self.assertIn(add_guild_member_request, requests_made)
 
-    @patch(MODULE_PATH + '.views.messages', spec=True)
+        expected = [
+            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):
         # setup
         requests_mocker.get(
-            guild_infos_request.url, json=create_discord_guild_object()
+            guild_infos_request.url, json={'id': TEST_GUILD_ID, 'name': 'Test Guild'}
         )
         requests_mocker.delete(remove_guild_member_request.url, status_code=204)
         DiscordUser.objects.create(user=self.member, uid=TEST_USER_ID)
@@ -672,13 +611,15 @@ class TestUserFeatures(WebTest):
         for r in requests_mocker.request_history:
             obj = DiscordRequest(r.method, r.url)
             requests_made.append(obj)
-        self.assertIn(remove_guild_member_request, requests_made)
 
-    @patch(MODULE_PATH + '.views.messages', spec=True)
+        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):
         # setup
         requests_mocker.get(
-            guild_infos_request.url, json=create_discord_guild_object()
+            guild_infos_request.url, json={'id': TEST_GUILD_ID, 'name': 'Test Guild'}
         )
         mock_exception = HTTPError('error')
         mock_exception.response = Mock()
@@ -708,9 +649,11 @@ class TestUserFeatures(WebTest):
         for r in requests_mocker.request_history:
             obj = DiscordRequest(r.method, r.url)
             requests_made.append(obj)
-        self.assertIn(remove_guild_member_request, requests_made)
 
-    @patch(MODULE_PATH + '.views.messages', spec=True)
+        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):
         # setup
         mock_exception = HTTPError(Mock(**{"response.status_code": 400}))
@@ -742,13 +685,14 @@ class TestUserFeatures(WebTest):
         services_page = self.app.get(reverse('services:services'))
         self.assertEqual(services_page.status_code, 200)
 
-    @override_settings(CELERY_ALWAYS_EAGER=True, CELERY_EAGER_PROPAGATES_EXCEPTIONS=True)
-    @patch(MODULE_PATH + ".core.default_bot_client", spec=True)
+    @override_settings(CELERY_ALWAYS_EAGER=True)
     def test_server_name_is_updated_by_task(
-        self, requests_mocker, mock_bot_client
+        self, requests_mocker
     ):
         # setup
-        mock_bot_client.guild_name.return_value = "Test Guild"
+        requests_mocker.get(
+            guild_infos_request.url, json={'id': TEST_GUILD_ID, 'name': 'Test Guild'}
+        )
         # run task to update usernames
         tasks.update_all_usernames()
 

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

@@ -1,395 +1,364 @@
+from unittest.mock import patch, Mock
 import urllib
-from unittest.mock import Mock, patch
 
 from requests.exceptions import HTTPError
 
-from django.contrib.auth.models import User
+from django.contrib.auth.models import Group, User
+from django.test import TestCase
 
 from allianceauth.tests.auth_utils import AuthUtils
-from allianceauth.utils.testing import NoSocketsTestCase
 
-from ..app_settings import DISCORD_APP_ID, DISCORD_APP_SECRET, DISCORD_CALLBACK_URL
-from ..discord_client import (
-    DISCORD_OAUTH_BASE_URL,
-    DISCORD_OAUTH_TOKEN_URL,
-    DiscordApiBackoff,
-    DiscordClient,
-    RolesSet,
-)
-from ..discord_client.tests.factories import (
+from . import (
     TEST_GUILD_ID,
-    TEST_USER_ID,
     TEST_USER_NAME,
-    create_role,
-    create_user,
+    TEST_USER_ID,
+    TEST_MAIN_NAME,
+    TEST_MAIN_ID,
+    MODULE_PATH,
+    ROLE_ALPHA,
+    ROLE_BRAVO,
+    ROLE_CHARLIE,
 )
+from ..discord_client.tests import create_matched_role
+from ..app_settings import (
+    DISCORD_APP_ID,
+    DISCORD_APP_SECRET,
+    DISCORD_CALLBACK_URL,
+)
+from ..discord_client import DiscordClient, DiscordApiBackoff
 from ..models import DiscordUser
 from ..utils import set_logger_to_file
-from . import MODULE_PATH, TEST_MAIN_NAME
+
 
 logger = set_logger_to_file(MODULE_PATH + '.managers', __file__)
 
 
 @patch(MODULE_PATH + '.managers.DISCORD_GUILD_ID', TEST_GUILD_ID)
 @patch(MODULE_PATH + '.managers.DiscordClient', spec=DiscordClient)
-@patch(MODULE_PATH + '.managers.create_bot_client', spec=True)
-@patch(
-    MODULE_PATH + '.models.DiscordUser.objects._exchange_auth_code_for_token', spec=True
-)
-@patch(MODULE_PATH + '.managers.calculate_roles_for_user', spec=True)
-@patch(MODULE_PATH + '.managers.user_formatted_nick', spec=True)
-class TestAddUser(NoSocketsTestCase):
+@patch(MODULE_PATH + '.models.DiscordUser.objects._exchange_auth_code_for_token')
+@patch(MODULE_PATH + '.models.DiscordUser.objects.user_group_names')
+@patch(MODULE_PATH + '.models.DiscordUser.objects.user_formatted_nick')
+class TestAddUser(TestCase):
 
     def setUp(self):
         self.user = AuthUtils.create_user(TEST_USER_NAME)
+        self.user_info = {
+            'id': TEST_USER_ID,
+            'name': TEST_USER_NAME,
+            'username': TEST_USER_NAME,
+            'discriminator': '1234',
+        }
         self.access_token = 'accesstoken'
 
     def test_can_create_user_no_roles_no_nick(
         self,
         mock_user_formatted_nick,
-        mock_calculate_roles_for_user,
+        mock_user_group_names,
         mock_exchange_auth_code_for_token,
-        mock_create_bot_client,
-        mock_DiscordClient,
+        mock_DiscordClient
     ):
-        # given
-        discord_user = create_user(id=TEST_USER_ID)
         mock_user_formatted_nick.return_value = None
+        mock_user_group_names.return_value = []
         mock_exchange_auth_code_for_token.return_value = self.access_token
-        mock_DiscordClient.return_value.current_user.return_value = discord_user
-        mock_calculate_roles_for_user.return_value = RolesSet([]), None
-        mock_create_bot_client.return_value.add_guild_member.return_value = True
-        # when
+        mock_DiscordClient.return_value.current_user.return_value = self.user_info
+        mock_DiscordClient.return_value.match_or_create_roles_from_names\
+            .return_value = []
+        mock_DiscordClient.return_value.add_guild_member.return_value = True
+
         result = DiscordUser.objects.add_user(self.user, authorization_code='abcdef')
-        # then
         self.assertTrue(result)
         self.assertTrue(
             DiscordUser.objects.filter(user=self.user, uid=TEST_USER_ID).exists()
         )
-        _, kwargs = mock_create_bot_client.return_value.add_guild_member.call_args
+        self.assertTrue(mock_DiscordClient.return_value.add_guild_member.called)
+        args, kwargs = mock_DiscordClient.return_value.add_guild_member.call_args
         self.assertEqual(kwargs['guild_id'], TEST_GUILD_ID)
         self.assertEqual(kwargs['user_id'], TEST_USER_ID)
         self.assertEqual(kwargs['access_token'], self.access_token)
-        self.assertFalse(kwargs['role_ids'])
+        self.assertIsNone(kwargs['role_ids'])
         self.assertIsNone(kwargs['nick'])
 
     def test_can_create_user_with_roles_no_nick(
         self,
         mock_user_formatted_nick,
-        mock_calculate_roles_for_user,
+        mock_user_group_names,
         mock_exchange_auth_code_for_token,
-        mock_create_bot_client,
-        mock_DiscordClient,
+        mock_DiscordClient
     ):
-        # given
-        role_a = create_role(id=1)
-        role_b = create_role(id=2)
-        roles_calculated = RolesSet([role_a, role_b])
-        discord_user = create_user(id=TEST_USER_ID)
+        roles = [
+            create_matched_role(ROLE_ALPHA),
+            create_matched_role(ROLE_BRAVO),
+            create_matched_role(ROLE_CHARLIE)
+        ]
         mock_user_formatted_nick.return_value = None
+        mock_user_group_names.return_value = ['a', 'b', 'c']
         mock_exchange_auth_code_for_token.return_value = self.access_token
-        mock_DiscordClient.return_value.current_user.return_value = discord_user
-        mock_calculate_roles_for_user.return_value = roles_calculated, None
-        mock_create_bot_client.return_value.add_guild_member.return_value = True
-        # when
+        mock_DiscordClient.return_value.current_user.return_value = self.user_info
+        mock_DiscordClient.return_value.match_or_create_roles_from_names\
+            .return_value = roles
+        mock_DiscordClient.return_value.add_guild_member.return_value = True
+
         result = DiscordUser.objects.add_user(self.user, authorization_code='abcdef')
-        # then
         self.assertTrue(result)
         self.assertTrue(
             DiscordUser.objects.filter(user=self.user, uid=TEST_USER_ID).exists()
         )
-        _, kwargs = mock_create_bot_client.return_value.add_guild_member.call_args
+        self.assertTrue(mock_DiscordClient.return_value.add_guild_member.called)
+        args, kwargs = mock_DiscordClient.return_value.add_guild_member.call_args
         self.assertEqual(kwargs['guild_id'], TEST_GUILD_ID)
         self.assertEqual(kwargs['user_id'], TEST_USER_ID)
         self.assertEqual(kwargs['access_token'], self.access_token)
-        self.assertSetEqual(set(kwargs['role_ids']), {1, 2})
+        self.assertSetEqual(set(kwargs['role_ids']), {1, 2, 3})
         self.assertIsNone(kwargs['nick'])
 
     def test_can_activate_existing_user_with_roles_no_nick(
         self,
         mock_user_formatted_nick,
-        mock_calculate_roles_for_user,
+        mock_user_group_names,
         mock_exchange_auth_code_for_token,
-        mock_create_bot_client,
-        mock_DiscordClient,
+        mock_DiscordClient
     ):
-        # given
-        role_a = create_role(id=1)
-        role_b = create_role(id=2)
-        roles_calculated = RolesSet([role_a, role_b])
-        discord_user = create_user(id=TEST_USER_ID)
+        roles = [
+            create_matched_role(ROLE_ALPHA),
+            create_matched_role(ROLE_BRAVO),
+            create_matched_role(ROLE_CHARLIE)
+        ]
         mock_user_formatted_nick.return_value = None
+        mock_user_group_names.return_value = ['a', 'b', 'c']
         mock_exchange_auth_code_for_token.return_value = self.access_token
-        mock_DiscordClient.return_value.current_user.return_value = discord_user
-        mock_calculate_roles_for_user.return_value = roles_calculated, False
-        mock_create_bot_client.return_value.add_guild_member.return_value = None
-        mock_create_bot_client.return_value.modify_guild_member.return_value = True
-        # when
+        mock_DiscordClient.return_value.current_user.return_value = self.user_info
+        mock_DiscordClient.return_value.match_or_create_roles_from_names\
+            .return_value = roles
+        mock_DiscordClient.return_value.add_guild_member.return_value = None
+        mock_DiscordClient.return_value.modify_guild_member.return_value = True
+
         result = DiscordUser.objects.add_user(self.user, authorization_code='abcdef')
-        # then
         self.assertTrue(result)
         self.assertTrue(
             DiscordUser.objects.filter(user=self.user, uid=TEST_USER_ID).exists()
         )
-        _, kwargs = mock_create_bot_client.return_value.modify_guild_member.call_args
+        self.assertTrue(mock_DiscordClient.return_value.add_guild_member.called)
+        self.assertTrue(mock_DiscordClient.return_value.modify_guild_member.called)
+        args, kwargs = mock_DiscordClient.return_value.modify_guild_member.call_args
         self.assertEqual(kwargs['guild_id'], TEST_GUILD_ID)
         self.assertEqual(kwargs['user_id'], TEST_USER_ID)
-        self.assertSetEqual(set(kwargs['role_ids']), {1, 2})
+        self.assertSetEqual(set(kwargs['role_ids']), {1, 2, 3})
         self.assertIsNone(kwargs['nick'])
 
     @patch(MODULE_PATH + '.managers.DISCORD_SYNC_NAMES', True)
     def test_can_create_user_no_roles_with_nick(
         self,
         mock_user_formatted_nick,
-        mock_calculate_roles_for_user,
+        mock_user_group_names,
         mock_exchange_auth_code_for_token,
-        mock_create_bot_client,
-        mock_DiscordClient,
+        mock_DiscordClient
     ):
-        # given
-        discord_user = create_user(id=TEST_USER_ID)
         mock_user_formatted_nick.return_value = TEST_MAIN_NAME
+        mock_user_group_names.return_value = []
         mock_exchange_auth_code_for_token.return_value = self.access_token
-        mock_DiscordClient.return_value.current_user.return_value = discord_user
-        mock_calculate_roles_for_user.return_value = RolesSet([]), None
-        mock_create_bot_client.return_value.add_guild_member.return_value = True
-        # when
+        mock_DiscordClient.return_value.current_user.return_value = self.user_info
+        mock_DiscordClient.return_value.match_or_create_roles_from_names\
+            .return_value = []
+        mock_DiscordClient.return_value.add_guild_member.return_value = True
+
         result = DiscordUser.objects.add_user(self.user, authorization_code='abcdef')
-        # then
         self.assertTrue(result)
         self.assertTrue(
             DiscordUser.objects.filter(user=self.user, uid=TEST_USER_ID).exists()
         )
-        _, kwargs = mock_create_bot_client.return_value.add_guild_member.call_args
+        self.assertTrue(mock_DiscordClient.return_value.add_guild_member.called)
+        args, kwargs = mock_DiscordClient.return_value.add_guild_member.call_args
         self.assertEqual(kwargs['guild_id'], TEST_GUILD_ID)
         self.assertEqual(kwargs['user_id'], TEST_USER_ID)
         self.assertEqual(kwargs['access_token'], self.access_token)
-        self.assertFalse(kwargs['role_ids'])
+        self.assertIsNone(kwargs['role_ids'])
         self.assertEqual(kwargs['nick'], TEST_MAIN_NAME)
 
     @patch(MODULE_PATH + '.managers.DISCORD_SYNC_NAMES', True)
     def test_can_activate_existing_user_no_roles_with_nick(
         self,
         mock_user_formatted_nick,
-        mock_calculate_roles_for_user,
+        mock_user_group_names,
         mock_exchange_auth_code_for_token,
-        mock_create_bot_client,
-        mock_DiscordClient,
+        mock_DiscordClient
     ):
-        # given
-        discord_user = create_user(id=TEST_USER_ID)
         mock_user_formatted_nick.return_value = TEST_MAIN_NAME
+        mock_user_group_names.return_value = []
         mock_exchange_auth_code_for_token.return_value = self.access_token
-        mock_DiscordClient.return_value.current_user.return_value = discord_user
-        mock_calculate_roles_for_user.return_value = RolesSet([]), False
-        mock_create_bot_client.return_value.add_guild_member.return_value = None
-        mock_create_bot_client.return_value.modify_guild_member.return_value = True
-        # when
+        mock_DiscordClient.return_value.current_user.return_value = self.user_info
+        mock_DiscordClient.return_value.match_or_create_roles_from_names\
+            .return_value = []
+        mock_DiscordClient.return_value.add_guild_member.return_value = None
+        mock_DiscordClient.return_value.modify_guild_member.return_value = True
+
         result = DiscordUser.objects.add_user(self.user, authorization_code='abcdef')
-        # then
         self.assertTrue(result)
         self.assertTrue(
             DiscordUser.objects.filter(user=self.user, uid=TEST_USER_ID).exists()
         )
-        _, kwargs = mock_create_bot_client.return_value.modify_guild_member.call_args
+        self.assertTrue(mock_DiscordClient.return_value.add_guild_member.called)
+        self.assertTrue(mock_DiscordClient.return_value.modify_guild_member.called)
+        args, kwargs = mock_DiscordClient.return_value.modify_guild_member.call_args
         self.assertEqual(kwargs['guild_id'], TEST_GUILD_ID)
         self.assertEqual(kwargs['user_id'], TEST_USER_ID)
-        self.assertFalse(kwargs['role_ids'])
+        self.assertIsNone(kwargs['role_ids'])
         self.assertEqual(kwargs['nick'], TEST_MAIN_NAME)
 
     @patch(MODULE_PATH + '.managers.DISCORD_SYNC_NAMES', False)
     def test_can_create_user_no_roles_and_without_nick_if_turned_off(
         self,
         mock_user_formatted_nick,
-        mock_calculate_roles_for_user,
+        mock_user_group_names,
         mock_exchange_auth_code_for_token,
-        mock_create_bot_client,
-        mock_DiscordClient,
+        mock_DiscordClient
     ):
-        # given
-        discord_user = create_user(id=TEST_USER_ID)
         mock_user_formatted_nick.return_value = TEST_MAIN_NAME
+        mock_user_group_names.return_value = []
         mock_exchange_auth_code_for_token.return_value = self.access_token
-        mock_DiscordClient.return_value.current_user.return_value = discord_user
-        mock_calculate_roles_for_user.return_value = RolesSet([]), None
-        mock_create_bot_client.return_value.add_guild_member.return_value = True
-        # when
+        mock_DiscordClient.return_value.current_user.return_value = self.user_info
+        mock_DiscordClient.return_value.match_or_create_roles_from_names\
+            .return_value = []
+        mock_DiscordClient.return_value.add_guild_member.return_value = True
+
         result = DiscordUser.objects.add_user(self.user, authorization_code='abcdef')
-        # then
         self.assertTrue(result)
         self.assertTrue(
             DiscordUser.objects.filter(user=self.user, uid=TEST_USER_ID).exists()
         )
-        _, kwargs = mock_create_bot_client.return_value.add_guild_member.call_args
+        self.assertTrue(mock_DiscordClient.return_value.add_guild_member.called)
+        args, kwargs = mock_DiscordClient.return_value.add_guild_member.call_args
         self.assertEqual(kwargs['guild_id'], TEST_GUILD_ID)
         self.assertEqual(kwargs['user_id'], TEST_USER_ID)
         self.assertEqual(kwargs['access_token'], self.access_token)
-        self.assertFalse(kwargs['role_ids'])
+        self.assertIsNone(kwargs['role_ids'])
         self.assertIsNone(kwargs['nick'])
 
     def test_can_activate_existing_guild_member(
         self,
         mock_user_formatted_nick,
-        mock_calculate_roles_for_user,
+        mock_user_group_names,
         mock_exchange_auth_code_for_token,
-        mock_create_bot_client,
-        mock_DiscordClient,
+        mock_DiscordClient
     ):
-        # given
-        discord_user = create_user(id=TEST_USER_ID)
-        roles_calculated = RolesSet([create_role()])
         mock_user_formatted_nick.return_value = None
+        mock_user_group_names.return_value = []
         mock_exchange_auth_code_for_token.return_value = self.access_token
-        mock_DiscordClient.return_value.current_user.return_value = discord_user
-        mock_calculate_roles_for_user.return_value = roles_calculated, False
-        mock_create_bot_client.return_value.add_guild_member.return_value = None
-        mock_create_bot_client.return_value.modify_guild_member.return_value = True
-        # when
-        result = DiscordUser.objects.add_user(self.user, authorization_code='abcdef')
-        # then
-        self.assertTrue(result)
-        self.assertTrue(
-            DiscordUser.objects.filter(user=self.user, uid=TEST_USER_ID).exists()
-        )
-        self.assertTrue(mock_create_bot_client.return_value.modify_guild_member.called)
+        mock_DiscordClient.return_value.current_user.return_value = self.user_info
+        mock_DiscordClient.return_value.match_or_create_roles_from_names\
+            .return_value = []
+        mock_DiscordClient.return_value.add_guild_member.return_value = None
+        mock_DiscordClient.return_value.modify_guild_member.return_value = True
 
-    def test_can_activate_existing_member_with_roles(
-        self,
-        mock_user_formatted_nick,
-        mock_calculate_roles_for_user,
-        mock_exchange_auth_code_for_token,
-        mock_create_bot_client,
-        mock_DiscordClient,
-    ):
-        # given
-        discord_user = create_user(id=TEST_USER_ID)
-        roles_calculated = RolesSet([create_role(id=1)])
-        mock_user_formatted_nick.return_value = None
-        mock_exchange_auth_code_for_token.return_value = self.access_token
-        mock_DiscordClient.return_value.current_user.return_value = discord_user
-        mock_calculate_roles_for_user.return_value = roles_calculated, False
-        mock_create_bot_client.return_value.add_guild_member.return_value = None
-        mock_create_bot_client.return_value.modify_guild_member.return_value = True
-        # when
         result = DiscordUser.objects.add_user(self.user, authorization_code='abcdef')
-        # then
         self.assertTrue(result)
         self.assertTrue(
             DiscordUser.objects.filter(user=self.user, uid=TEST_USER_ID).exists()
         )
-        _, kwargs = mock_create_bot_client.return_value.modify_guild_member.call_args
-        self.assertSetEqual(set(kwargs['role_ids']), {1})
+        self.assertTrue(mock_DiscordClient.return_value.add_guild_member.called)
+        self.assertTrue(mock_DiscordClient.return_value.modify_guild_member.called)
 
     def test_can_activate_existing_guild_member_failure(
         self,
         mock_user_formatted_nick,
-        mock_calculate_roles_for_user,
+        mock_user_group_names,
         mock_exchange_auth_code_for_token,
-        mock_create_bot_client,
-        mock_DiscordClient,
+        mock_DiscordClient
     ):
-        # given
-        discord_user = create_user(id=TEST_USER_ID)
-        roles_calculated = RolesSet([create_role()])
         mock_user_formatted_nick.return_value = None
+        mock_user_group_names.return_value = []
         mock_exchange_auth_code_for_token.return_value = self.access_token
-        mock_DiscordClient.return_value.current_user.return_value = discord_user
-        mock_calculate_roles_for_user.return_value = roles_calculated, False
-        mock_create_bot_client.return_value.add_guild_member.return_value = None
-        mock_create_bot_client.return_value.modify_guild_member.return_value = False
-        # when
+        mock_DiscordClient.return_value.current_user.return_value = self.user_info
+        mock_DiscordClient.return_value.match_or_create_roles_from_names\
+            .return_value = []
+        mock_DiscordClient.return_value.add_guild_member.return_value = None
+        mock_DiscordClient.return_value.modify_guild_member.return_value = False
+
         result = DiscordUser.objects.add_user(self.user, authorization_code='abcdef')
-        # then
         self.assertFalse(result)
         self.assertFalse(
             DiscordUser.objects.filter(user=self.user, uid=TEST_USER_ID).exists()
         )
-        self.assertTrue(mock_create_bot_client.return_value.modify_guild_member.called)
+        self.assertTrue(mock_DiscordClient.return_value.add_guild_member.called)
+        self.assertTrue(mock_DiscordClient.return_value.modify_guild_member.called)
 
     def test_return_false_when_user_creation_fails(
         self,
         mock_user_formatted_nick,
-        mock_calculate_roles_for_user,
+        mock_user_group_names,
         mock_exchange_auth_code_for_token,
-        mock_create_bot_client,
-        mock_DiscordClient,
+        mock_DiscordClient
     ):
-        # given
-        discord_user = create_user(id=TEST_USER_ID)
         mock_user_formatted_nick.return_value = None
+        mock_user_group_names.return_value = []
         mock_exchange_auth_code_for_token.return_value = self.access_token
-        mock_DiscordClient.return_value.current_user.return_value = discord_user
-        mock_calculate_roles_for_user.return_value = RolesSet([]), None
-        mock_create_bot_client.return_value.add_guild_member.return_value = False
-        # when
+        mock_DiscordClient.return_value.current_user.return_value = self.user_info
+        mock_DiscordClient.return_value.match_or_create_roles_from_names\
+            .return_value = []
+        mock_DiscordClient.return_value.add_guild_member.return_value = False
+
         result = DiscordUser.objects.add_user(self.user, authorization_code='abcdef')
-        # then
         self.assertFalse(result)
         self.assertFalse(
             DiscordUser.objects.filter(user=self.user, uid=TEST_USER_ID).exists()
         )
-        self.assertTrue(mock_create_bot_client.return_value.add_guild_member.called)
+        self.assertTrue(mock_DiscordClient.return_value.add_guild_member.called)
 
     def test_return_false_when_on_api_backoff(
         self,
         mock_user_formatted_nick,
-        mock_calculate_roles_for_user,
+        mock_user_group_names,
         mock_exchange_auth_code_for_token,
-        mock_create_bot_client,
-        mock_DiscordClient,
+        mock_DiscordClient
     ):
-        # given
-        discord_user = create_user(id=TEST_USER_ID)
         mock_user_formatted_nick.return_value = None
+        mock_user_group_names.return_value = []
         mock_exchange_auth_code_for_token.return_value = self.access_token
-        mock_DiscordClient.return_value.current_user.return_value = discord_user
-        mock_calculate_roles_for_user.return_value = RolesSet([]), None
-        mock_create_bot_client.return_value.add_guild_member.side_effect = \
+        mock_DiscordClient.return_value.current_user.return_value = self.user_info
+        mock_DiscordClient.return_value.match_or_create_roles_from_names\
+            .return_value = []
+        mock_DiscordClient.return_value.add_guild_member.side_effect = \
             DiscordApiBackoff(999)
-        # when
+
         result = DiscordUser.objects.add_user(self.user, authorization_code='abcdef')
-        # then
         self.assertFalse(result)
         self.assertFalse(
             DiscordUser.objects.filter(user=self.user, uid=TEST_USER_ID).exists()
         )
-        self.assertTrue(mock_create_bot_client.return_value.add_guild_member.called)
+        self.assertTrue(mock_DiscordClient.return_value.add_guild_member.called)
 
     def test_return_false_on_http_error(
         self,
         mock_user_formatted_nick,
-        mock_calculate_roles_for_user,
+        mock_user_group_names,
         mock_exchange_auth_code_for_token,
-        mock_create_bot_client,
-        mock_DiscordClient,
+        mock_DiscordClient
     ):
-        # given
-        discord_user = create_user(id=TEST_USER_ID)
         mock_user_formatted_nick.return_value = None
+        mock_user_group_names.return_value = []
         mock_exchange_auth_code_for_token.return_value = self.access_token
-        mock_DiscordClient.return_value.current_user.return_value = discord_user
-        mock_calculate_roles_for_user.return_value = RolesSet([]), None
+        mock_DiscordClient.return_value.current_user.return_value = self.user_info
+        mock_DiscordClient.return_value.match_or_create_roles_from_names\
+            .return_value = []
         mock_exception = HTTPError('error')
         mock_exception.response = Mock()
         mock_exception.response.status_code = 500
-        mock_create_bot_client.return_value.add_guild_member.side_effect = mock_exception
-        # when
+        mock_DiscordClient.return_value.add_guild_member.side_effect = mock_exception
+
         result = DiscordUser.objects.add_user(self.user, authorization_code='abcdef')
-        # then
         self.assertFalse(result)
         self.assertFalse(
             DiscordUser.objects.filter(user=self.user, uid=TEST_USER_ID).exists()
         )
-        self.assertTrue(mock_create_bot_client.return_value.add_guild_member.called)
+        self.assertTrue(mock_DiscordClient.return_value.add_guild_member.called)
 
 
-class TestOauthHelpers(NoSocketsTestCase):
+class TestOauthHelpers(TestCase):
 
     @patch(MODULE_PATH + '.managers.DISCORD_APP_ID', '123456')
     def test_generate_bot_add_url(self):
         bot_add_url = DiscordUser.objects.generate_bot_add_url()
 
-        auth_url = DISCORD_OAUTH_BASE_URL
+        auth_url = DiscordClient.OAUTH_BASE_URL
         real_bot_add_url = (
             f'{auth_url}?client_id=123456&scope=bot'
             f'&permissions={DiscordUser.objects.BOT_PERMISSIONS}'
@@ -399,12 +368,12 @@ class TestOauthHelpers(NoSocketsTestCase):
     def test_generate_oauth_redirect_url(self):
         oauth_url = DiscordUser.objects.generate_oauth_redirect_url()
 
-        self.assertIn(DISCORD_OAUTH_BASE_URL, oauth_url)
+        self.assertIn(DiscordClient.OAUTH_BASE_URL, oauth_url)
         self.assertIn('+'.join(DiscordUser.objects.SCOPES), oauth_url)
         self.assertIn(DISCORD_APP_ID, oauth_url)
         self.assertIn(urllib.parse.quote_plus(DISCORD_CALLBACK_URL), oauth_url)
 
-    @patch(MODULE_PATH + '.managers.OAuth2Session', spec=True)
+    @patch(MODULE_PATH + '.managers.OAuth2Session')
     def test_process_callback_code(self, oauth):
         instance = oauth.return_value
         instance.fetch_token.return_value = {'access_token': 'mywonderfultoken'}
@@ -417,13 +386,52 @@ class TestOauthHelpers(NoSocketsTestCase):
         self.assertEqual(kwargs['redirect_uri'], DISCORD_CALLBACK_URL)
         self.assertTrue(instance.fetch_token.called)
         args, kwargs = instance.fetch_token.call_args
-        self.assertEqual(args[0], DISCORD_OAUTH_TOKEN_URL)
+        self.assertEqual(args[0], DiscordClient.OAUTH_TOKEN_URL)
         self.assertEqual(kwargs['client_secret'], DISCORD_APP_SECRET)
         self.assertEqual(kwargs['code'], '12345')
         self.assertEqual(token, 'mywonderfultoken')
 
 
-class TestUserHasAccount(NoSocketsTestCase):
+class TestUserFormattedNick(TestCase):
+
+    def setUp(self):
+        self.user = AuthUtils.create_user(TEST_USER_NAME)
+
+    def test_return_nick_when_user_has_main(self):
+        AuthUtils.add_main_character_2(self.user, TEST_MAIN_NAME, TEST_MAIN_ID)
+        result = DiscordUser.objects.user_formatted_nick(self.user)
+        expected = TEST_MAIN_NAME
+        self.assertEqual(result, expected)
+
+    def test_return_none_if_user_has_no_main(self):
+        result = DiscordUser.objects.user_formatted_nick(self.user)
+        self.assertIsNone(result)
+
+
+class TestUserGroupNames(TestCase):
+
+    @classmethod
+    def setUpClass(cls):
+        super().setUpClass()
+        cls.group_1 = Group.objects.create(name='Group 1')
+        cls.group_2 = Group.objects.create(name='Group 2')
+
+    def setUp(self):
+        self.user = AuthUtils.create_member(TEST_USER_NAME)
+
+    def test_return_groups_and_state_names_for_user(self):
+        self.user.groups.add(self.group_1)
+        result = DiscordUser.objects.user_group_names(self.user)
+        expected = ['Group 1', 'Member']
+        self.assertSetEqual(set(result), set(expected))
+
+    def test_return_state_only_if_user_has_no_groups(self):
+        result = DiscordUser.objects.user_group_names(self.user)
+        expected = ['Member']
+        self.assertSetEqual(set(result), set(expected))
+
+
+class TestUserHasAccount(TestCase):
 
     @classmethod
     def setUpClass(cls):
@@ -445,22 +453,59 @@ class TestUserHasAccount(NoSocketsTestCase):
         self.assertFalse(DiscordUser.objects.user_has_account('abc'))
 
 
-class TestOtherMethods(NoSocketsTestCase):
-    @patch(MODULE_PATH + '.managers.core_group_to_role', spec=True)
-    def test_should_call_group_to_role(self, mock_core_group_to_role):
-        # given
-        role = create_role(id=1, name="alpha", managed=False)
-        mock_core_group_to_role.return_value = role
-        # when
-        result = DiscordUser.objects.group_to_role(Mock())
-        # then
-        self.assertEqual(result["id"], 1)
-        self.assertEqual(result["name"], "alpha")
-        self.assertEqual(result["managed"], False)
+@patch(MODULE_PATH + '.managers.DiscordClient', spec=DiscordClient)
+@patch(MODULE_PATH + '.managers.logger')
+class TestServerName(TestCase):
 
-    @patch(MODULE_PATH + '.managers.core_server_name', spec=True)
-    def test_should_call_server_name(self, mock_core_server_name):
-        # when
-        DiscordUser.objects.server_name()
-        # then
-        self.assertTrue(mock_core_server_name.called)
+    @classmethod
+    def setUpClass(cls):
+        super().setUpClass()
+        cls.user = AuthUtils.create_user(TEST_USER_NAME)
+
+    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_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_models.py

@@ -1,20 +1,34 @@
-from unittest.mock import Mock, patch
+from unittest.mock import patch, Mock
 
 from requests.exceptions import HTTPError
 
-from allianceauth.tests.auth_utils import AuthUtils
-from allianceauth.utils.testing import NoSocketsTestCase
+from django.test import TestCase
 
-from ..discord_client import DiscordApiBackoff, RolesSet
-from ..discord_client.tests.factories import TEST_USER_ID, TEST_USER_NAME, create_role
+from allianceauth.tests.auth_utils import AuthUtils
+from allianceauth.groupmanagement.models import ReservedGroupName
+
+from . import (
+    TEST_USER_NAME,
+    TEST_USER_ID,
+    TEST_MAIN_NAME,
+    TEST_MAIN_ID,
+    MODULE_PATH,
+    ROLE_ALPHA,
+    ROLE_BRAVO,
+    ROLE_CHARLIE,
+    ROLE_CHARLIE_2,
+    ROLE_MIKE,
+)
+from ..discord_client import DiscordClient, DiscordApiBackoff
+from ..discord_client.tests import create_matched_role
 from ..models import DiscordUser
 from ..utils import set_logger_to_file
-from . import MODULE_PATH, TEST_MAIN_ID, TEST_MAIN_NAME
+
 
 logger = set_logger_to_file(MODULE_PATH + '.models', __file__)
 
 
-class TestBasicsAndHelpers(NoSocketsTestCase):
+class TestBasicsAndHelpers(TestCase):
 
     def test_str(self):
         user = AuthUtils.create_user(TEST_USER_NAME)
@@ -29,8 +43,8 @@ class TestBasicsAndHelpers(NoSocketsTestCase):
         self.assertEqual(repr(discord_user), expected)
 
 
-@patch(MODULE_PATH + '.models.default_bot_client', spec=True)
-class TestUpdateNick(NoSocketsTestCase):
+@patch(MODULE_PATH + '.managers.DiscordClient', spec=DiscordClient)
+class TestUpdateNick(TestCase):
 
     def setUp(self):
         self.user = AuthUtils.create_user(TEST_USER_NAME)
@@ -38,54 +52,40 @@ class TestUpdateNick(NoSocketsTestCase):
             user=self.user, uid=TEST_USER_ID
         )
 
-    def test_can_update(self, mock_default_bot_client):
-        # given
-        AuthUtils.add_main_character_2(
-            self.user, TEST_MAIN_NAME, TEST_MAIN_ID, disconnect_signals=True
-        )
-        mock_default_bot_client.modify_guild_member.return_value = True
-        # when
+    def test_can_update(self, mock_DiscordClient):
+        AuthUtils.add_main_character_2(self.user, TEST_MAIN_NAME, TEST_MAIN_ID)
+        mock_DiscordClient.return_value.modify_guild_member.return_value = True
+
         result = self.discord_user.update_nickname()
-        # then
         self.assertTrue(result)
-        self.assertTrue(mock_default_bot_client.modify_guild_member.called)
+        self.assertTrue(mock_DiscordClient.return_value.modify_guild_member.called)
+
+    def test_dont_update_if_user_has_no_main(self, mock_DiscordClient):
+        mock_DiscordClient.return_value.modify_guild_member.return_value = False
 
-    def test_dont_update_if_user_has_no_main(self, mock_default_bot_client):
-        # given
-        mock_default_bot_client.modify_guild_member.return_value = False
-        # when
         result = self.discord_user.update_nickname()
-        # then
         self.assertFalse(result)
-        self.assertFalse(mock_default_bot_client.modify_guild_member.called)
+        self.assertFalse(mock_DiscordClient.return_value.modify_guild_member.called)
+
+    def test_return_none_if_user_no_longer_a_member(self, mock_DiscordClient):
+        AuthUtils.add_main_character_2(self.user, TEST_MAIN_NAME, TEST_MAIN_ID)
+        mock_DiscordClient.return_value.modify_guild_member.return_value = None
 
-    def test_return_none_if_user_no_longer_a_member(self, mock_default_bot_client):
-        # given
-        AuthUtils.add_main_character_2(
-            self.user, TEST_MAIN_NAME, TEST_MAIN_ID, disconnect_signals=True
-        )
-        mock_default_bot_client.modify_guild_member.return_value = None
-        # when
         result = self.discord_user.update_nickname()
-        # then
         self.assertIsNone(result)
-        self.assertTrue(mock_default_bot_client.modify_guild_member.called)
+        self.assertTrue(mock_DiscordClient.return_value.modify_guild_member.called)
+
+    def test_return_false_if_api_returns_false(self, mock_DiscordClient):
+        AuthUtils.add_main_character_2(self.user, TEST_MAIN_NAME, TEST_MAIN_ID)
+        mock_DiscordClient.return_value.modify_guild_member.return_value = False
 
-    def test_return_false_if_api_returns_false(self, mock_default_bot_client):
-        # given
-        AuthUtils.add_main_character_2(
-            self.user, TEST_MAIN_NAME, TEST_MAIN_ID, disconnect_signals=True
-        )
-        mock_default_bot_client.modify_guild_member.return_value = False
-        # when
         result = self.discord_user.update_nickname()
-        # then
         self.assertFalse(result)
-        self.assertTrue(mock_default_bot_client.modify_guild_member.called)
+        self.assertTrue(mock_DiscordClient.return_value.modify_guild_member.called)
 
 
-@patch(MODULE_PATH + '.models.default_bot_client', spec=True)
-class TestUpdateUsername(NoSocketsTestCase):
+@patch(MODULE_PATH + '.managers.DiscordClient', spec=DiscordClient)
+class TestUpdateUsername(TestCase):
 
     @classmethod
     def setUpClass(cls):
@@ -100,8 +100,7 @@ class TestUpdateUsername(NoSocketsTestCase):
             discriminator='1234'
         )
 
-    def test_can_update(self, mock_default_bot_client):
-        # given
+    def test_can_update(self, mock_DiscordClient):
         new_username = 'New name'
         new_discriminator = '9876'
         user_info = {
@@ -111,77 +110,61 @@ class TestUpdateUsername(NoSocketsTestCase):
                 'discriminator': new_discriminator,
             }
         }
-        mock_default_bot_client.guild_member.return_value = user_info
-        # when
+        mock_DiscordClient.return_value.guild_member.return_value = user_info
+
         result = self.discord_user.update_username()
-        # then
         self.assertTrue(result)
-        self.assertTrue(mock_default_bot_client.guild_member.called)
+        self.assertTrue(mock_DiscordClient.return_value.guild_member.called)
         self.discord_user.refresh_from_db()
         self.assertEqual(self.discord_user.username, new_username)
         self.assertEqual(self.discord_user.discriminator, new_discriminator)
 
-    def test_return_none_if_user_no_longer_a_member(self, mock_default_bot_client):
-        # given
-        mock_default_bot_client.guild_member.return_value = None
-        # when
+    def test_return_none_if_user_no_longer_a_member(self, mock_DiscordClient):
+        mock_DiscordClient.return_value.guild_member.return_value = None
         result = self.discord_user.update_username()
-        # then
         self.assertIsNone(result)
-        self.assertTrue(mock_default_bot_client.guild_member.called)
+        self.assertTrue(mock_DiscordClient.return_value.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
+    def test_return_false_if_api_returns_false(self, mock_DiscordClient):
+        mock_DiscordClient.return_value.guild_member.return_value = False
         result = self.discord_user.update_username()
-        # then
         self.assertFalse(result)
-        self.assertTrue(mock_default_bot_client.guild_member.called)
+        self.assertTrue(mock_DiscordClient.return_value.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
+    def test_return_false_if_api_returns_corrput_data_1(self, mock_DiscordClient):
+        mock_DiscordClient.return_value.guild_member.return_value = {'invalid': True}
         result = self.discord_user.update_username()
-        # then
         self.assertFalse(result)
-        self.assertTrue(mock_default_bot_client.guild_member.called)
+        self.assertTrue(mock_DiscordClient.return_value.guild_member.called)
 
-    def test_return_false_if_api_returns_corrput_data_2(self, mock_default_bot_client):
-        # given
+    def test_return_false_if_api_returns_corrput_data_2(self, mock_DiscordClient):
         user_info = {
             'user': {
                 'id': str(TEST_USER_ID),
                 'discriminator': '1234',
             }
         }
-        mock_default_bot_client.guild_member.return_value = user_info
-        # when
+        mock_DiscordClient.return_value.guild_member.return_value = user_info
         result = self.discord_user.update_username()
-        # then
         self.assertFalse(result)
-        self.assertTrue(mock_default_bot_client.guild_member.called)
+        self.assertTrue(mock_DiscordClient.return_value.guild_member.called)
 
-    def test_return_false_if_api_returns_corrput_data_3(self, mock_default_bot_client):
-        # given
+    def test_return_false_if_api_returns_corrput_data_3(self, mock_DiscordClient):
         user_info = {
             'user': {
                 'id': str(TEST_USER_ID),
                 'username': TEST_USER_NAME,
             }
         }
-        mock_default_bot_client.guild_member.return_value = user_info
-        # when
+        mock_DiscordClient.return_value.guild_member.return_value = user_info
         result = self.discord_user.update_username()
-        # then
         self.assertFalse(result)
-        self.assertTrue(mock_default_bot_client.guild_member.called)
+        self.assertTrue(mock_DiscordClient.return_value.guild_member.called)
 
 
-@patch(MODULE_PATH + '.models.notify', spec=True)
-@patch(MODULE_PATH + '.models.create_bot_client', spec=True)
-class TestDeleteUser(NoSocketsTestCase):
+@patch(MODULE_PATH + '.models.notify')
+@patch(MODULE_PATH + '.managers.DiscordClient', spec=DiscordClient)
+class TestDeleteUser(TestCase):
 
     @classmethod
     def setUpClass(cls):
@@ -193,168 +176,272 @@ class TestDeleteUser(NoSocketsTestCase):
             user=self.user, uid=TEST_USER_ID
         )
 
-    def test_can_delete_user(self, mock_create_bot_client, mock_notify):
-        # given
-        mock_create_bot_client.return_value.remove_guild_member.return_value = True
-        # when
+    def test_can_delete_user(self, mock_DiscordClient, mock_notify):
+        mock_DiscordClient.return_value.remove_guild_member.return_value = True
         result = self.discord_user.delete_user()
-        # then
         self.assertTrue(result)
         self.assertFalse(
             DiscordUser.objects.filter(user=self.user, uid=TEST_USER_ID).exists()
         )
-        self.assertTrue(mock_create_bot_client.return_value.remove_guild_member.called)
+        self.assertTrue(mock_DiscordClient.return_value.remove_guild_member.called)
         self.assertFalse(mock_notify.called)
 
-    def test_can_delete_user_and_notify_user(self, mock_create_bot_client, mock_notify):
-        # given
-        mock_create_bot_client.return_value.remove_guild_member.return_value = True
-        # when
+    def test_can_delete_user_and_notify_user(self, mock_DiscordClient, mock_notify):
+        mock_DiscordClient.return_value.remove_guild_member.return_value = True
         result = self.discord_user.delete_user(notify_user=True)
-        # then
         self.assertTrue(result)
         self.assertTrue(mock_notify.called)
 
     def test_can_delete_user_when_member_is_unknown(
-        self, mock_create_bot_client, mock_notify
+        self, mock_DiscordClient, mock_notify
     ):
-        # given
-        mock_create_bot_client.return_value.remove_guild_member.return_value = None
-        # when
+        mock_DiscordClient.return_value.remove_guild_member.return_value = None
         result = self.discord_user.delete_user()
-        # then
         self.assertTrue(result)
         self.assertFalse(
             DiscordUser.objects.filter(user=self.user, uid=TEST_USER_ID).exists()
         )
-        self.assertTrue(mock_create_bot_client.return_value.remove_guild_member.called)
+        self.assertTrue(mock_DiscordClient.return_value.remove_guild_member.called)
         self.assertFalse(mock_notify.called)
 
-    def test_return_false_when_api_fails(self, mock_create_bot_client, mock_notify):
-        # given
-        mock_create_bot_client.return_value.remove_guild_member.return_value = False
-        # when
+    def test_return_false_when_api_fails(self, mock_DiscordClient, mock_notify):
+        mock_DiscordClient.return_value.remove_guild_member.return_value = False
         result = self.discord_user.delete_user()
-        # then
         self.assertFalse(result)
 
     def test_dont_notify_if_user_was_already_deleted_and_return_none(
-        self, mock_create_bot_client, mock_notify
+        self, mock_DiscordClient, mock_notify
     ):
-        # given
-        mock_create_bot_client.return_value.remove_guild_member.return_value = None
+        mock_DiscordClient.return_value.remove_guild_member.return_value = None
         DiscordUser.objects.get(pk=self.discord_user.pk).delete()
-        # when
         result = self.discord_user.delete_user()
-        # then
         self.assertIsNone(result)
         self.assertFalse(
             DiscordUser.objects.filter(user=self.user, uid=TEST_USER_ID).exists()
         )
-        self.assertTrue(mock_create_bot_client.return_value.remove_guild_member.called)
+        self.assertTrue(mock_DiscordClient.return_value.remove_guild_member.called)
         self.assertFalse(mock_notify.called)
 
     def test_raise_exception_on_api_backoff(
-        self, mock_create_bot_client, mock_notify
+        self, mock_DiscordClient, mock_notify
     ):
-        # given
-        mock_create_bot_client.return_value.remove_guild_member.side_effect = \
+        mock_DiscordClient.return_value.remove_guild_member.side_effect = \
             DiscordApiBackoff(999)
-        # when/then
         with self.assertRaises(DiscordApiBackoff):
             self.discord_user.delete_user()
 
     def test_return_false_on_api_backoff_and_exception_handling_on(
-        self, mock_create_bot_client, mock_notify
+        self, mock_DiscordClient, mock_notify
     ):
-        # given
-        mock_create_bot_client.return_value.remove_guild_member.side_effect = \
+        mock_DiscordClient.return_value.remove_guild_member.side_effect = \
             DiscordApiBackoff(999)
-        # when
         result = self.discord_user.delete_user(handle_api_exceptions=True)
-        # then
         self.assertFalse(result)
 
     def test_raise_exception_on_http_error(
-        self, mock_create_bot_client, mock_notify
+        self, mock_DiscordClient, mock_notify
     ):
-        # given
         mock_exception = HTTPError('error')
         mock_exception.response = Mock()
         mock_exception.response.status_code = 500
-        mock_create_bot_client.return_value.remove_guild_member.side_effect = \
+        mock_DiscordClient.return_value.remove_guild_member.side_effect = \
             mock_exception
-        # when/then
+
         with self.assertRaises(HTTPError):
             self.discord_user.delete_user()
 
     def test_return_false_on_http_error_and_exception_handling_on(
-        self, mock_create_bot_client, mock_notify
+        self, mock_DiscordClient, mock_notify
     ):
-        # given
         mock_exception = HTTPError('error')
         mock_exception.response = Mock()
         mock_exception.response.status_code = 500
-        mock_create_bot_client.return_value.remove_guild_member.side_effect = \
+        mock_DiscordClient.return_value.remove_guild_member.side_effect = \
             mock_exception
-        # when
         result = self.discord_user.delete_user(handle_api_exceptions=True)
-        # then
         self.assertFalse(result)
 
 
-@patch(MODULE_PATH + '.models.default_bot_client', spec=True)
-@patch(MODULE_PATH + '.models.calculate_roles_for_user', spec=True)
-class TestUpdateGroups(NoSocketsTestCase):
+@patch(MODULE_PATH + '.managers.DiscordClient', spec=DiscordClient)
+@patch(MODULE_PATH + '.models.DiscordUser.objects.user_group_names')
+class TestUpdateGroups(TestCase):
 
     def setUp(self):
-        user = AuthUtils.create_user(TEST_USER_NAME)
-        self.discord_user = DiscordUser.objects.create(user=user, uid=TEST_USER_ID)
+        self.user = AuthUtils.create_user(TEST_USER_NAME)
+        self.discord_user = DiscordUser.objects.create(
+            user=self.user, uid=TEST_USER_ID
+        )
+        self.guild_roles = [ROLE_ALPHA, ROLE_BRAVO, ROLE_CHARLIE, ROLE_MIKE]
+        self.roles_requested = [
+            create_matched_role(ROLE_ALPHA), create_matched_role(ROLE_BRAVO)
+        ]
 
-    def test_should_update_when_roles_have_changed(
-        self, mock_calculate_roles_for_user, mock_client
+    def test_update_if_needed(
+        self,
+        mock_user_group_names,
+        mock_DiscordClient
+    ):
+        roles_current = [1]
+        mock_user_group_names.return_value = []
+        mock_DiscordClient.return_value.match_or_create_roles_from_names\
+            .return_value = self.roles_requested
+        mock_DiscordClient.return_value.guild_roles.return_value = self.guild_roles
+        mock_DiscordClient.return_value.guild_member.return_value = \
+            {'roles': roles_current}
+        mock_DiscordClient.return_value.modify_guild_member.return_value = True
+
+        result = self.discord_user.update_groups()
+        self.assertTrue(result)
+        self.assertTrue(mock_DiscordClient.return_value.modify_guild_member.called)
+        args, kwargs = mock_DiscordClient.return_value.modify_guild_member.call_args
+        self.assertEqual(set(kwargs['role_ids']), {1, 2})
+
+    def test_should_update_and_preserve_managed_and_reserved_roles(
+        self,
+        mock_user_group_names,
+        mock_DiscordClient
     ):
         # given
-        mock_calculate_roles_for_user.return_value = RolesSet([create_role()]), True
-        mock_client.modify_guild_member.return_value = True
+        roles_current = [1, 3, 4, 13]
+        mock_user_group_names.return_value = []
+        mock_DiscordClient.return_value.match_or_create_roles_from_names\
+            .return_value = self.roles_requested
+        mock_DiscordClient.return_value.guild_roles.return_value = [
+            ROLE_ALPHA, ROLE_BRAVO, ROLE_CHARLIE, ROLE_MIKE, ROLE_CHARLIE_2
+        ]
+        mock_DiscordClient.return_value.guild_member.return_value = {
+            'roles': roles_current
+        }
+        mock_DiscordClient.return_value.modify_guild_member.return_value = True
+        ReservedGroupName.objects.create(
+            name="charlie", reason="dummy", created_by="xyz"
+        )
         # when
         result = self.discord_user.update_groups()
         # then
         self.assertTrue(result)
-        self.assertTrue(mock_client.modify_guild_member.called)
+        self.assertTrue(mock_DiscordClient.return_value.modify_guild_member.called)
+        args, kwargs = mock_DiscordClient.return_value.modify_guild_member.call_args
+        self.assertEqual(set(kwargs['role_ids']), {1, 2, 3, 4, 13})
 
-    def test_should_not_update_when_roles_have_not_changed(
-        self, mock_calculate_roles_for_user, mock_client
+    def test_dont_update_if_not_needed(
+        self,
+        mock_user_group_names,
+        mock_DiscordClient
     ):
-        # given
-        mock_calculate_roles_for_user.return_value = RolesSet([create_role()]), False
-        mock_client.modify_guild_member.return_value = True
-        # when
+        roles_current = [1, 2, 13]
+        mock_user_group_names.return_value = []
+        mock_DiscordClient.return_value.match_or_create_roles_from_names\
+            .return_value = self.roles_requested
+        mock_DiscordClient.return_value.guild_roles.return_value = self.guild_roles
+        mock_DiscordClient.return_value.guild_member.return_value = \
+            {'roles': roles_current}
+
         result = self.discord_user.update_groups()
-        # then
         self.assertTrue(result)
-        self.assertFalse(mock_client.modify_guild_member.called)
+        self.assertFalse(mock_DiscordClient.return_value.modify_guild_member.called)
 
-    def test_should_not_update_when_user_not_guild_member(
-        self, mock_calculate_roles_for_user, mock_client
+    def test_update_if_user_has_no_roles_on_discord(
+        self,
+        mock_user_group_names,
+        mock_DiscordClient
     ):
-        # given
-        mock_calculate_roles_for_user.return_value = RolesSet([create_role()]), None
-        mock_client.modify_guild_member.return_value = True
-        # when
+        roles_current = []
+        mock_user_group_names.return_value = []
+        mock_DiscordClient.return_value.match_or_create_roles_from_names\
+            .return_value = self.roles_requested
+        mock_DiscordClient.return_value.guild_roles.return_value = self.guild_roles
+        mock_DiscordClient.return_value.guild_member.return_value = \
+            {'roles': roles_current}
+        mock_DiscordClient.return_value.modify_guild_member.return_value = True
+
+        result = self.discord_user.update_groups()
+        self.assertTrue(result)
+        self.assertTrue(mock_DiscordClient.return_value.modify_guild_member.called)
+        args, kwargs = mock_DiscordClient.return_value.modify_guild_member.call_args
+        self.assertEqual(set(kwargs['role_ids']), {1, 2})
+
+    def test_return_none_if_user_no_longer_a_member(
+        self,
+        mock_user_group_names,
+        mock_DiscordClient
+    ):
+        mock_DiscordClient.return_value.guild_member.return_value = None
+
         result = self.discord_user.update_groups()
-        # then
         self.assertIsNone(result)
-        self.assertFalse(mock_client.modify_guild_member.called)
+        self.assertFalse(mock_DiscordClient.return_value.modify_guild_member.called)
 
-    def test_should_return_false_when_update_failed(
-        self, mock_calculate_roles_for_user, mock_client
+    def test_return_false_if_api_returns_false(
+        self,
+        mock_user_group_names,
+        mock_DiscordClient
     ):
-        # given
-        mock_calculate_roles_for_user.return_value = RolesSet([create_role()]), True
-        mock_client.modify_guild_member.return_value = False
-        # when
+        roles_current = [1]
+        mock_user_group_names.return_value = []
+        mock_DiscordClient.return_value.match_or_create_roles_from_names\
+            .return_value = self.roles_requested
+        mock_DiscordClient.return_value.guild_roles.return_value = self.guild_roles
+        mock_DiscordClient.return_value.guild_member.return_value = \
+            {'roles': roles_current}
+        mock_DiscordClient.return_value.modify_guild_member.return_value = False
+
         result = self.discord_user.update_groups()
-        # then
         self.assertFalse(result)
-        self.assertTrue(mock_client.modify_guild_member.called)
+        self.assertTrue(mock_DiscordClient.return_value.modify_guild_member.called)
+
+    def test_raise_exception_if_member_has_unknown_roles(
+        self,
+        mock_user_group_names,
+        mock_DiscordClient
+    ):
+        roles_current = [99]
+        mock_user_group_names.return_value = []
+        mock_DiscordClient.return_value.match_or_create_roles_from_names\
+            .return_value = self.roles_requested
+        mock_DiscordClient.return_value.guild_roles.return_value = self.guild_roles
+        mock_DiscordClient.return_value.guild_member.return_value = \
+            {'roles': roles_current}
+        mock_DiscordClient.return_value.modify_guild_member.return_value = True
+
+        with self.assertRaises(RuntimeError):
+            self.discord_user.update_groups()
+
+    def test_refresh_guild_roles_user_roles_dont_not_match(
+        self,
+        mock_user_group_names,
+        mock_DiscordClient
+    ):
+        def my_guild_roles(guild_id, use_cache=True):
+            if use_cache:
+                return [ROLE_ALPHA, ROLE_BRAVO, ROLE_MIKE]
+            else:
+                return [ROLE_ALPHA, ROLE_BRAVO, ROLE_CHARLIE, ROLE_MIKE]
+
+        roles_current = [3]
+        mock_user_group_names.return_value = []
+        mock_DiscordClient.return_value.match_or_create_roles_from_names\
+            .return_value = self.roles_requested
+        mock_DiscordClient.return_value.guild_roles.side_effect = my_guild_roles
+        mock_DiscordClient.return_value.guild_member.return_value = \
+            {'roles': roles_current}
+        mock_DiscordClient.return_value.modify_guild_member.return_value = True
+        result = self.discord_user.update_groups()
+        self.assertTrue(result)
+        self.assertEqual(mock_DiscordClient.return_value.guild_roles.call_count, 2)
+
+    def test_raise_exception_if_member_info_is_invalid(
+        self,
+        mock_user_group_names,
+        mock_DiscordClient
+    ):
+        mock_user_group_names.return_value = []
+        mock_DiscordClient.return_value.match_or_create_roles_from_names\
+            .return_value = self.roles_requested
+        mock_DiscordClient.return_value.guild_roles.return_value = self.guild_roles
+        mock_DiscordClient.return_value.guild_member.return_value = \
+            {'user': 'dummy'}
+        mock_DiscordClient.return_value.modify_guild_member.return_value = True
+
+        with self.assertRaises(RuntimeError):
+            self.discord_user.update_groups()