Fix update_nickname runs on ever save of userprofile, fix nickname not updated after main's name changes

allianceauth/services/modules/discord/auth_hooks.py

@@ -68,7 +68,11 @@ class DiscordService(ServicesHook):
         logger.debug('Syncing %s nickname for user %s', self.name, user)
         if self.user_has_account(user):
             tasks.update_nickname.apply_async(
-                kwargs={'user_pk': user.pk}, priority=SINGLE_TASK_PRIORITY
+                kwargs={
+                    'user_pk': user.pk,
+                    'nickname': DiscordUser.objects.user_formatted_nick(user)
+                }, 
+                priority=SINGLE_TASK_PRIORITY
             )
 
     def sync_nicknames_bulk(self, users: list):

allianceauth/services/modules/discord/models.py

@@ -67,21 +67,25 @@ class DiscordUser(models.Model):
     def __repr__(self):
         return f'{type(self).__name__}(user=\'{self.user}\', uid={self.uid})'
 
-    def update_nickname(self) -> bool:
+    def update_nickname(self, nickname: str = None) -> bool:
         """Update nickname with formatted name of main character
 
+        Params:
+        - nickname: optional nickname to be used instead of user's main
+
         Returns:
         - True on success
         - None if user is no longer a member of the Discord server
         - False on error or raises exception
         """        
-        requested_nick = DiscordUser.objects.user_formatted_nick(self.user)
-        if requested_nick:            
+        if not nickname:
+            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=requested_nick
+                nick=nickname
             )
             if success:
                 logger.info('Nickname for %s has been updated', self.user)

allianceauth/services/modules/discord/tasks.py

@@ -38,13 +38,14 @@ def update_groups(self, user_pk: int) -> None:
 @shared_task(
     bind=True, name='discord.update_nickname', base=QueueOnce, max_retries=None
 )
-def update_nickname(self, user_pk: int) -> None:
+def update_nickname(self, user_pk: int, nickname: str = None) -> None:
     """Set nickname on Discord for given user to his main character name
     
     Params:
     - user_pk: PK of given user
+    - nickname: optional nickname to be used instead of user's main
     """
-    _task_perform_user_action(self, user_pk, 'update_nickname')
+    _task_perform_user_action(self, user_pk, 'update_nickname', nickname=nickname)
 
 
 @shared_task(

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

@@ -114,7 +114,7 @@ class TestServiceFeatures(TransactionTestCase):
         """All tests: Given a user with member state, service permission and active Discord account"""
         clear_cache()
         reset_testdata()
-        self.group_3 = Group.objects.create(name='charlie')        
+        self.group_charlie = Group.objects.create(name='charlie')        
         
         # States
         self.member_state = AuthUtils.get_member_state()
@@ -144,8 +144,9 @@ class TestServiceFeatures(TransactionTestCase):
         self.discord_user = DiscordUser.objects.create(user=self.user, uid=TEST_USER_ID)
         self.assertTrue(DiscordUser.objects.user_has_account(self.user))
         
-    def test_name_of_main_changes(self, requests_mocker):      
-        # modify_guild_member()        
+    def test_when_name_of_main_changes_then_discord_nick_is_updated(
+        self, requests_mocker
+    ):        
         requests_mocker.patch(modify_guild_member_request.url, status_code=204)
         
         # changing nick to trigger signals
@@ -153,22 +154,23 @@ class TestServiceFeatures(TransactionTestCase):
         self.user.profile.main_character.character_name = new_nick
         self.user.profile.main_character.save()
         
-        # Need to have called modify_guild_member two times only
-        # Once for sync nickname
-        # Once for change of main character        
-        requests_made = list()
+        # verify Discord nick was updates
+        nick_updated = False
         for r in requests_mocker.request_history:            
-            requests_made.append(DiscordRequest(r.method, r.url))
+            my_request = DiscordRequest(r.method, r.url)                        
+            if my_request == modify_guild_member_request and "nick" in r.json():
+                nick_updated = True
+                self.assertEqual(r.json()["nick"], new_nick)
         
-        expected = [modify_guild_member_request]        
-        self.assertListEqual(requests_made, expected)
+        self.assertTrue(nick_updated)
+        self.assertTrue(DiscordUser.objects.user_has_account(self.user))
 
-    def test_name_of_main_changes_but_user_deleted(self, requests_mocker):      
-        # modify_guild_member()        
+    def test_when_name_of_main_changes_and_user_deleted_then_account_is_deleted(
+        self, requests_mocker
+    ):        
         requests_mocker.patch(
             modify_guild_member_request.url, status_code=404, json={'code': 10007}
         )        
-        # remove_guild_member()
         requests_mocker.delete(remove_guild_member_request.url, status_code=204)
         
         # changing nick to trigger signals
@@ -176,24 +178,11 @@ class TestServiceFeatures(TransactionTestCase):
         self.user.profile.main_character.character_name = new_nick
         self.user.profile.main_character.save()
         
-        # Need to have called modify_guild_member two times only
-        # Once for sync nickname
-        # Once for change of main character        
-        requests_made = list()
-        for r in requests_mocker.request_history:            
-            requests_made.append(DiscordRequest(r.method, r.url))
+        self.assertFalse(DiscordUser.objects.user_has_account(self.user))
 
-        expected = [
-            modify_guild_member_request, 
-            remove_guild_member_request,            
-        ]            
-        self.assertListEqual(requests_made, expected)
-        # self.assertFalse(DiscordUser.objects.user_has_account(self.user))
-
-    def test_name_of_main_changes_but_user_rate_limited(
+    def test_when_name_of_main_changes_and_and_rate_limited_then_dont_call_api(
         self, requests_mocker
     ):        
-        # modify_guild_member()        
         requests_mocker.patch(modify_guild_member_request.url, status_code=204)
         
         # exhausting rate limit
@@ -210,12 +199,12 @@ class TestServiceFeatures(TransactionTestCase):
         self.user.profile.main_character.save()
 
         # should not have called the API
-        requests_made = list()
-        for r in requests_mocker.request_history:            
+        requests_made = [
             requests_made.append(DiscordRequest(r.method, r.url)) 
+            for r in requests_mocker.request_history
+        ]
                 
-        expected = list()  
-        self.assertListEqual(requests_made, expected)
+        self.assertListEqual(requests_made, list())
         
     def test_when_member_is_demoted_to_guest_then_his_account_is_deleted(
         self, requests_mocker
@@ -269,79 +258,75 @@ class TestServiceFeatures(TransactionTestCase):
             if my_request == modify_guild_member_request and "roles" in r.json():
                 roles_updated = True
                 self.assertSetEqual(set(r.json()["roles"]), {13, 98})
+                break
         
         self.assertTrue(roles_updated)
+        self.assertTrue(DiscordUser.objects.user_has_account(self.user))
 
-    def test_adding_group_to_user_role_exists(self, requests_mocker):
-        # guild_member()                
+    def test_when_group_added_to_member_and_role_known_then_his_roles_are_updated(
+        self, requests_mocker
+    ):        
         requests_mocker.get(
             guild_member_request.url,
             json={
                 'user': create_user_info(),
-                'roles': ['1', '13', '99']
+                'roles': ['13', '99']
             }
         )              
-        # guild_roles()        
         requests_mocker.get(
             guild_roles_request.url, 
             json=[ROLE_ALPHA, ROLE_BRAVO, ROLE_CHARLIE, ROLE_MIKE, ROLE_MEMBER]
         )                
-        # create_guild_role()        
         requests_mocker.post(create_guild_role_request.url, json=ROLE_CHARLIE)        
-        # modify_guild_member()                
         requests_mocker.patch(modify_guild_member_request.url, status_code=204)
                 
         # adding new group to trigger signals
-        self.user.groups.add(self.group_3)        
-        self.user.refresh_from_db()
+        self.user.groups.add(self.group_charlie)
                 
-        # compare the list of made requests with expected
-        requests_made = list()
+        # verify roles for user where updated
+        roles_updated = False
         for r in requests_mocker.request_history:            
-            requests_made.append(DiscordRequest(r.method, r.url))
+            my_request = DiscordRequest(r.method, r.url)                        
+            if my_request == modify_guild_member_request and "roles" in r.json():
+                roles_updated = True
+                self.assertSetEqual(set(r.json()["roles"]), {3, 13, 99})
+                break
         
-        expected = [
-            guild_member_request,
-            guild_roles_request,            
-            modify_guild_member_request
-        ]        
-        self.assertListEqual(requests_made, expected)
+        self.assertTrue(roles_updated)
+        self.assertTrue(DiscordUser.objects.user_has_account(self.user))
 
-    def test_adding_group_to_user_role_does_not_exist(self, requests_mocker):    
-        # guild_member()                
+    def test_when_group_added_to_member_and_role_unknown_then_his_roles_are_updated(
+        self, requests_mocker
+    ):        
         requests_mocker.get(
             guild_member_request.url,
             json={
                 'user': {'id': str(TEST_USER_ID), 'username': TEST_MAIN_NAME},
-                'roles': ['1', '13', '99']
+                'roles': ['13', '99']
             }
         )              
-        # guild_roles()        
         requests_mocker.get(
             guild_roles_request.url, 
             json=[ROLE_ALPHA, ROLE_BRAVO, ROLE_MIKE, ROLE_MEMBER]
         )                
-        # create_guild_role()        
         requests_mocker.post(create_guild_role_request.url, json=ROLE_CHARLIE)        
-        # modify_guild_member()                
         requests_mocker.patch(modify_guild_member_request.url, status_code=204)
                 
         # adding new group to trigger signals
-        self.user.groups.add(self.group_3)        
+        self.user.groups.add(self.group_charlie)
         self.user.refresh_from_db()
         
-        # compare the list of made requests with expected
-        requests_made = list()
+       # verify roles for user where updated
+        roles_updated = False
         for r in requests_mocker.request_history:            
-            requests_made.append(DiscordRequest(r.method, r.url))
+            my_request = DiscordRequest(r.method, r.url)                        
+            if my_request == modify_guild_member_request and "roles" in r.json():
+                roles_updated = True
+                self.assertSetEqual(set(r.json()["roles"]), {3, 13, 99})
+                break
         
-        expected = [
-            guild_member_request,
-            guild_roles_request,
-            create_guild_role_request,
-            modify_guild_member_request
-        ]        
-        self.assertListEqual(requests_made, expected)
+        self.assertTrue(roles_updated)
+        self.assertTrue(DiscordUser.objects.user_has_account(self.user))
     
 
 @override_settings(CELERY_ALWAYS_EAGER=True)
@@ -373,7 +358,10 @@ class StateTestCase(TestCase):
         self.user = User.objects.get(pk=self.user.pk)
 
     def test_perm_changes_to_higher_priority_state_creation(self, requests_mocker):
-        mock_url = DiscordRequest(method='DELETE',url=f'{DISCORD_API_BASE_URL}guilds/{TEST_GUILD_ID}/members/12345678910')
+        mock_url = DiscordRequest(
+            method='DELETE',
+            url=f'{DISCORD_API_BASE_URL}guilds/{TEST_GUILD_ID}/members/12345678910'
+        )
         requests_mocker.delete(mock_url.url, status_code=204)
         self._add_discord_user()
         self._refresh_user()
@@ -394,7 +382,10 @@ class StateTestCase(TestCase):
             self.user.discord
 
     def test_perm_changes_to_lower_priority_state_creation(self, requests_mocker):
-        mock_url = DiscordRequest(method='DELETE',url=f'{DISCORD_API_BASE_URL}guilds/{TEST_GUILD_ID}/members/12345678910')
+        mock_url = DiscordRequest(
+            method='DELETE',
+            url=f'{DISCORD_API_BASE_URL}guilds/{TEST_GUILD_ID}/members/12345678910'
+        )
         requests_mocker.delete(mock_url.url, status_code=204)
         self._add_discord_user()
         self._refresh_user()

allianceauth/services/signals.py

@@ -167,7 +167,7 @@ def process_main_character_change(sender, instance, *args, **kwargs):
         if old_instance.main_character and not instance.main_character:  # lost main char disable services
             logger.info("Disabling services due to loss of main character for user {0}".format(instance.user))
             disable_user(instance.user)
-        elif old_instance.main_character is not instance.main_character:  # swapping/changing main character
+        elif old_instance.main_character != instance.main_character:  # swapping/changing main character
             logger.info("Updating Names due to change of main character for user {0}".format(instance.user))
             for svc in ServicesHook.get_services():
                 try: