From ecae21e66cd94e8b872831bc1480f27d88a2c5ad Mon Sep 17 00:00:00 2001 From: Bill DeRusha Date: Fri, 15 Jan 2016 14:24:19 -0500 Subject: [PATCH] Update change_enrollment management command to be more robust and tested --- .../management/commands/change_enrollment.py | 111 +++++++++++----- .../tests/test_change_enrollment.py | 123 ++++++++++++++++++ 2 files changed, 204 insertions(+), 30 deletions(-) create mode 100644 common/djangoapps/student/management/tests/test_change_enrollment.py diff --git a/common/djangoapps/student/management/commands/change_enrollment.py b/common/djangoapps/student/management/commands/change_enrollment.py index d6199a468c..8b5c1346f1 100644 --- a/common/djangoapps/student/management/commands/change_enrollment.py +++ b/common/djangoapps/student/management/commands/change_enrollment.py @@ -1,10 +1,24 @@ -from django.core.management.base import BaseCommand, CommandError -from opaque_keys import InvalidKeyError -from optparse import make_option -from student.models import CourseEnrollment, User +""" Command line script to change user enrollments. """ +import logging + +from django.core.management.base import BaseCommand, CommandError +from django.db import transaction +from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locations import SlashSeparatedCourseKey +from optparse import make_option + +from student.models import CourseEnrollment, User + +logger = logging.getLogger(__name__) # pylint: disable=invalid-name + + +class RollbackException(Exception): + """ + Exception raised explicitly to cause a database transaction rollback. + """ + pass class Command(BaseCommand): @@ -15,17 +29,17 @@ class Command(BaseCommand): Example: - Change enrollment for user joe from audit to honor: + Change enrollment for users joe, frank, and bill from audit to honor: $ ... change_enrollment -u joe,frank,bill -c some/course/id --from audit --to honor Or - $ ... change_enrollment -u "joe@example.com,frank@example.com,bill@example.com" -c some/course/id --from audit --to honor + $ ... change_enrollment -e "joe@example.com,frank@example.com,bill@example.com" -c some/course/id --from audit --to honor - Change enrollment for all users in some/course/id from audit to honor + See what would have been changed from audit to honor without making that change - $ ... change_enrollment -c some/course/id --from audit --to honor + $ ... change_enrollment -u joe,frank,bill -c some/course/id --from audit --to honor -n """ @@ -40,11 +54,16 @@ class Command(BaseCommand): dest='to_mode', default=False, help='move to this enrollment mode'), - make_option('-u', '--user', - metavar='USER', - dest='user', + make_option('-u', '--usernames', + metavar='USERNAME', + dest='username', default=False, - help="Comma-separated list of users to move in the course"), + help="Comma-separated list of usernames to move in the course"), + make_option('-e', '--emails', + metavar='EMAIL', + dest='email', + default=False, + help="Comma-separated list of email addresses to move in the course"), make_option('-c', '--course', metavar='COURSE_ID', dest='course_id', @@ -59,8 +78,11 @@ class Command(BaseCommand): ) def handle(self, *args, **options): + error_users = [] + success_users = [] + if not options['course_id']: - raise CommandError("You must specify a course id for this command") + raise CommandError('You must specify a course id for this command') if not options['from_mode'] or not options['to_mode']: raise CommandError('You must specify a "to" and "from" mode as parameters') @@ -69,26 +91,55 @@ class Command(BaseCommand): except InvalidKeyError: course_key = SlashSeparatedCourseKey.from_deprecated_string(options['course_id']) - filter_args = dict( + enrollment_args = dict( course_id=course_key, mode=options['from_mode'] ) - if options['user']: - user_str = options['user'] - for username in user_str.split(","): - if '@' in username: - user = User.objects.get(email=username) - else: - user = User.objects.get(username=username) - filter_args['user'] = user - enrollments = CourseEnrollment.objects.filter(**filter_args) - if options['noop']: - print "Would have changed {num_enrollments} students from {from_mode} to {to_mode}".format( - num_enrollments=enrollments.count(), - from_mode=options['from_mode'], - to_mode=options['to_mode'] - ) - else: + + if options['username']: + self.update_enrollments('username', enrollment_args, options, error_users, success_users) + + if options['email']: + self.update_enrollments('email', enrollment_args, options, error_users, success_users) + + self.report(error_users, success_users) + + def update_enrollments(self, identifier, enrollment_args, options, error_users, success_users): + """ Update enrollments for a specific user identifier (email or username). """ + users = options[identifier].split(",") + for identified_user in users: + logger.info(identified_user) + try: + user_args = { + identifier: identified_user + } + + enrollment_args['user'] = User.objects.get(**user_args) + enrollments = CourseEnrollment.objects.filter(**enrollment_args) + + with transaction.atomic(): for enrollment in enrollments: enrollment.update_enrollment(mode=options['to_mode']) enrollment.save() + + if options['noop']: + raise RollbackException('Forced rollback.') + + except RollbackException: + success_users.append(identified_user) + continue + except Exception as exception: # pylint: disable=broad-except + error_users.append((identified_user, exception)) + continue + + success_users.append(identified_user) + logger.info('Updated user [%s] to mode [%s]', identified_user, options['to_mode']) + + def report(self, error_users, success_users): + """ Log and overview of the results of the command. """ + total_users = len(success_users) + len(error_users) + logger.info('Successfully updated %i out of %i users', len(success_users), total_users) + if len(error_users) > 0: + logger.info('The following %i user(s) not saved:', len(error_users)) + for user, error in error_users: + logger.info('user: [%s] reason: [%s] %s', user, type(error).__name__, error.message) diff --git a/common/djangoapps/student/management/tests/test_change_enrollment.py b/common/djangoapps/student/management/tests/test_change_enrollment.py new file mode 100644 index 0000000000..1d554ba47b --- /dev/null +++ b/common/djangoapps/student/management/tests/test_change_enrollment.py @@ -0,0 +1,123 @@ +""" Test the change_enrollment command line script.""" + +import ddt +from mock import patch + +from django.core.management import call_command +from xmodule.modulestore.tests.factories import CourseFactory +from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase + +from student.tests.factories import UserFactory, CourseModeFactory +from student.models import CourseEnrollment + + +@ddt.ddt +class ChangeEnrollmentTests(SharedModuleStoreTestCase): + """ Test the enrollment change functionality of the change_enrollment script.""" + def setUp(self): + super(ChangeEnrollmentTests, self).setUp() + self.course = CourseFactory.create() + self.audit_mode = CourseModeFactory.create( + course_id=self.course.id, + mode_slug='audit', + mode_display_name='Audit', + ) + self.honor_mode = CourseModeFactory.create( + course_id=self.course.id, + mode_slug='honor', + mode_display_name='Honor', + ) + + self.user_info = [ + ('amy', 'amy@pond.com', 'password'), + ('rory', 'rory@theroman.com', 'password'), + ('river', 'river@song.com', 'password') + ] + self.enrollments = [] + self.users = [] + for username, email, password in self.user_info: + user = UserFactory.create(username=username, email=email, password=password) + self.users.append(user) + self.enrollments.append(CourseEnrollment.enroll(user, self.course.id, mode='audit')) + + @patch('student.management.commands.change_enrollment.logger') + @ddt.data( + ('email', False, 3), + ('username', False, 3), + ('email', True, 0), + ('username', True, 0), + ) + @ddt.unpack + def test_convert_users(self, method, noop, expected_conversions, mock_logger): + """ The command should update the user's enrollment. """ + user_str = ','.join([getattr(user, method) for user in self.users]) + user_ids = [u.id for u in self.users] + command_args = { + 'course_id': unicode(self.course.id), + 'to_mode': 'honor', + 'from_mode': 'audit', + 'noop': noop, + method: user_str, + } + + # Verify users are not in honor mode yet + self.assertEqual( + len(CourseEnrollment.objects.filter(mode='honor', user_id__in=user_ids)), + 0 + ) + + call_command( + 'change_enrollment', + **command_args + ) + + # Verify correct number of users are now in honor mode + self.assertEqual( + len(CourseEnrollment.objects.filter(mode='honor', user_id__in=user_ids)), + expected_conversions + ) + + mock_logger.info.assert_called_with( + 'Successfully updated %i out of %i users', + len(self.users), + len(self.users) + ) + + @patch('student.management.commands.change_enrollment.logger') + @ddt.data( + ('email', 'dtennant@thedoctor.com', 3), + ('username', 'dtennant', 3), + ) + @ddt.unpack + def test_user_not_found(self, method, fake_user, expected_success, mock_logger): + all_users = [getattr(user, method) for user in self.users] + all_users.append(fake_user) + user_str = ','.join(all_users) + real_user_ids = [u.id for u in self.users] + command_args = { + 'course_id': unicode(self.course.id), + 'to_mode': 'honor', + 'from_mode': 'audit', + method: user_str, + } + + # Verify users are not in honor mode yet + self.assertEqual( + len(CourseEnrollment.objects.filter(mode='honor', user_id__in=real_user_ids)), + 0 + ) + + call_command( + 'change_enrollment', + **command_args + ) + + # Verify correct number of users are now in honor mode + self.assertEqual( + len(CourseEnrollment.objects.filter(mode='honor', user_id__in=real_user_ids)), + expected_success + ) + + mock_logger.info.assert_called_with( + 'user: [%s] reason: [%s] %s', fake_user, 'DoesNotExist', 'User matching query does not exist.' + )