From 745aadbcd52073256710e95aa9d65569334bde9f Mon Sep 17 00:00:00 2001 From: bmedx Date: Mon, 20 Aug 2018 13:25:57 -0400 Subject: [PATCH] Update bulk_rehash_retired_usernames - Handle forums 404s - Update UserRetirementStatus whether or not the user has completed retirement --- .../commands/bulk_rehash_retired_usernames.py | 34 +++++----- .../test_bulk_rehash_retired_usernames.py | 66 ++++++++++++++----- 2 files changed, 68 insertions(+), 32 deletions(-) diff --git a/openedx/core/djangoapps/user_api/management/commands/bulk_rehash_retired_usernames.py b/openedx/core/djangoapps/user_api/management/commands/bulk_rehash_retired_usernames.py index cc1ecf1653..7bfdc39432 100644 --- a/openedx/core/djangoapps/user_api/management/commands/bulk_rehash_retired_usernames.py +++ b/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 diff --git a/openedx/core/djangoapps/user_api/management/tests/test_bulk_rehash_retired_usernames.py b/openedx/core/djangoapps/user_api/management/tests/test_bulk_rehash_retired_usernames.py index e6096635b2..84e10f29f6 100644 --- a/openedx/core/djangoapps/user_api/management/tests/test_bulk_rehash_retired_usernames.py +++ b/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)