Merge pull request #18813 from edx/bmedx/update_bulk_username_rehash

Update bulk_rehash_retired_usernames

openedx/core/djangoapps/user_api/management/commands/bulk_rehash_retired_usernames.py

@@ -50,20 +50,10 @@ class Command(BaseCommand):
                 settings.RETIRED_USER_SALTS,
                 settings.RETIRED_USERNAME_FMT
             )
-            # Sanity check:
-            if retirement.user.username != old_retired_username:
-                print(
-                    'WARNING: Skipping UserRetirementStatus ID {} / User ID {} because the user does not appear to '
-                    'have a retired username: {} != {}.'.format(
-                        retirement.id,
-                        retirement.user.id,
-                        retirement.user.username,
-                        old_retired_username
-                    )
-                )
+
             # If the original username was already normalized (or all lowercase), the old and new hashes would
             # match:
-            elif old_retired_username == new_retired_username:
+            if old_retired_username == new_retired_username:
                 print(
                     'Skipping UserRetirementStatus ID {} / User ID {} because the hash would not change.'.format(
                         retirement.id,
@@ -88,12 +78,26 @@ class Command(BaseCommand):
                         # as the local db updates and can be slow, so keeping it
                         # outside to cut down on potential deadlocks.
                         cc_user = comment_client.User.from_django_user(retirement.user)
-                        cc_user.retire(new_retired_username)
+
+                        # The user may not exist in forums, if it doesn't that's not
+                        # an error.
+                        try:
+                            cc_user.retire(new_retired_username)
+                        except comment_client.utils.CommentClientRequestError as e:
+                            if e.status_code != 404:
+                                print(
+                                    'UserRetirementStatus ID {} User ID {} failed to rename in forums: {}'.format(
+                                        retirement.id, retirement.user.id, text_type(e)
+                                    )
+                                )
+                                raise
 
                         # Update and save both the user table and retirement queue table:
                         with transaction.atomic():
-                            retirement.user.username = new_retired_username
-                            retirement.user.save()
+                            # Only rename them in auth_user if they've already been retired
+                            if retirement.user.username == old_retired_username:
+                                retirement.user.username = new_retired_username
+                                retirement.user.save()
                             retirement.retired_username = new_retired_username
                             retirement.save()
                     except Exception as exc:  # pylint: disable=broad-except

openedx/core/djangoapps/user_api/management/tests/test_bulk_rehash_retired_usernames.py

@@ -8,6 +8,7 @@ from django.conf import settings
 from django.core.management import call_command
 from user_util.user_util import get_retired_username
 
+from lms.lib import comment_client
 from openedx.core.djangoapps.user_api.accounts.tests.retirement_helpers import (
     setup_retirement_states, fake_completed_retirement
 )
@@ -21,16 +22,14 @@ pytestmark = pytest.mark.django_db
 def _setup_users():
     """
     Creates and returns test users in the different states of needing rehash:
-    - Skipped: has not yet been retired
-    - Faked: has been fake-retired, but the retired username does not require updating
-    - Needing rehash: has been fake-retired and name changed so it triggers a hash update
+    - Skipped: The retired username does not require updating, some of these are fake retired
+    - Needing rehash: Has been fake-retired and name changed so it triggers a hash update
     """
     # When we loop through creating users, take additional action on these
     user_indexes_to_be_fake_retired = (2, 4, 6, 8, 10)
     user_indexes_to_be_rehashed = (4, 6)
 
     users_skipped = []
-    users_faked = []
     users_needing_rehash = []
     retirements = {}
 
@@ -44,19 +43,18 @@ def _setup_users():
             fake_completed_retirement(user)
 
             if i in user_indexes_to_be_rehashed:
-                # In order to need a rehash user.username must be the same as
-                # retirement.retired_username and NOT the same as the hash
-                # generated when the script is run. So we force that here.
+                # In order to need a rehash user.username the new hash must be
+                # different, we force that here.
                 retirement.retired_username = retirement.retired_username.upper()
                 user.username = retirement.retired_username
                 retirement.save()
                 user.save()
                 users_needing_rehash.append(user)
             else:
-                users_faked.append(user)
+                users_skipped.append(user)
         else:
             users_skipped.append(user)
-    return users_skipped, users_faked, users_needing_rehash, retirements
+    return users_skipped, users_needing_rehash, retirements
 
 
 @skip_unless_lms
@@ -66,7 +64,7 @@ def test_successful_rehash(retire_user_forums, capsys):
     """
     Run the command with users of all different hash statuses, expect success
     """
-    users_skipped, users_faked, users_needing_rehash, retirements = _setup_users()
+    users_skipped, users_needing_rehash, retirements = _setup_users()
 
     call_command('bulk_rehash_retired_usernames')
     output = capsys.readouterr().out
@@ -75,9 +73,6 @@ def test_successful_rehash(retire_user_forums, capsys):
     assert retire_user_forums.call_count == 2
 
     for user in users_skipped:
-        assert "User ID {} because the user does not appear to have a retired username:".format(user.id) in output
-
-    for user in users_faked:
         assert "User ID {} because the hash would not change.".format(user.id) in output
 
     expected_username_calls = []
@@ -106,7 +101,7 @@ def test_forums_failed(retire_user_forums, capsys):
     """
     Run the command with users of all different hash statuses, expect success
     """
-    users_skipped, users_faked, users_needing_rehash, retirements = _setup_users()
+    users_skipped, users_needing_rehash, retirements = _setup_users()
     retire_user_forums.side_effect = Exception('something bad happened with forums')
 
     call_command('bulk_rehash_retired_usernames')
@@ -116,9 +111,6 @@ def test_forums_failed(retire_user_forums, capsys):
     assert retire_user_forums.call_count == 2
 
     for user in users_skipped:
-        assert "User ID {} because the user does not appear to have a retired username:".format(user.id) in output
-
-    for user in users_faked:
         assert "User ID {} because the hash would not change.".format(user.id) in output
 
     expected_username_calls = []
@@ -140,3 +132,43 @@ def test_forums_failed(retire_user_forums, capsys):
 
     assert "FAILED! 2 retirements failed to rehash. Retirement IDs:" in output
     retire_user_forums.assert_has_calls(expected_username_calls)
+
+
+@skip_unless_lms
+@pytest.mark.usefixtures("setup_retirement_states")
+@patch('lms.lib.comment_client.User.retire')
+def test_forums_404(retire_user_forums, capsys):
+    """
+    Run the command with users of all different hash statuses, expect success
+    """
+    users_skipped, users_needing_rehash, retirements = _setup_users()
+    retire_user_forums.side_effect = comment_client.utils.CommentClientRequestError('not found', status_codes=404)
+
+    call_command('bulk_rehash_retired_usernames')
+    output = capsys.readouterr().out
+
+    # Make sure forums was called the correct number of times
+    assert retire_user_forums.call_count == 2
+
+    for user in users_skipped:
+        assert "User ID {} because the hash would not change.".format(user.id) in output
+
+    expected_username_calls = []
+    for user in users_needing_rehash:
+        retirement = retirements[user.id]
+        user.refresh_from_db()
+        retirement.refresh_from_db()
+        new_retired_username = get_retired_username(
+            retirement.original_username,
+            settings.RETIRED_USER_SALTS,
+            settings.RETIRED_USERNAME_FMT
+        )
+        expected_username_calls.append(call(new_retired_username))
+
+        assert "User ID {} to rehash their retired username".format(user.id) in output
+        # Confirm that the usernames *are* updated, since this is a non-blocking forums error
+        assert new_retired_username == user.username
+        assert new_retired_username == retirement.retired_username
+
+    assert "Success!" in output
+    retire_user_forums.assert_has_calls(expected_username_calls)