Merge pull request #16526 from edx/pwnage101/teams_mgmt_commands

teams and student management commands cleanup for django 1.11

common/djangoapps/student/management/commands/bulk_change_enrollment.py

@@ -5,7 +5,6 @@ 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 optparse import make_option
 from six import text_type
 from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
 

common/djangoapps/student/management/commands/transfer_students.py

 """
 Transfer Student Management Command
 """
+from __future__ import print_function, unicode_literals
+
+from textwrap import dedent
+
+from six import text_type
+
+from django.contrib.auth.models import User
 from django.db import transaction
 from opaque_keys.edx.keys import CourseKey
-from optparse import make_option
-from django.contrib.auth.models import User
-from student.models import CourseEnrollment
 from shoppingcart.models import CertificateItem
+from student.models import CourseEnrollment
 from track.management.tracked_command import TrackedCommand
 
 
 class TransferStudentError(Exception):
-    """Generic Error when handling student transfers."""
+    """
+    Generic Error when handling student transfers.
+    """
     pass
 
 
 class Command(TrackedCommand):
-    """Management Command for transferring students from one course to new courses."""
-    help = """
-    This command takes two course ids as input and transfers
-    all students enrolled in one course into the other.  This will
-    remove them from the first class and enroll them in the specified
-    class(es) in the same mode as the first one. eg. honor, verified,
-    audit.
-
-    example:
-        # Transfer students from the old demoX class to a new one.
-        manage.py ... transfer_students -f edX/Open_DemoX/edx_demo_course -t edX/Open_DemoX/new_demoX
-
-        # Transfer students from old course to new, with original certificate items.
-        manage.py ... transfer_students -f edX/Open_DemoX/edx_demo_course -t edX/Open_DemoX/new_demoX -c true
-
-        # Transfer students from the old demoX class into two new classes.
-        manage.py ... transfer_students -f edX/Open_DemoX/edx_demo_course
-            -t edX/Open_DemoX/new_demoX,edX/Open_DemoX/edX_Insider
-
     """
+    Transfer students enrolled in one course into one or more other courses.
 
-    option_list = TrackedCommand.option_list + (
-        make_option('-f', '--from',
-                    metavar='SOURCE_COURSE',
-                    dest='source_course',
-                    help='The course to transfer students from.'),
-        make_option('-t', '--to',
-                    metavar='DEST_COURSE_LIST',
-                    dest='dest_course_list',
-                    help='The new course(es) to enroll the student into.'),
-        make_option('-c', '--transfer-certificates',
-                    metavar='TRANSFER_CERTIFICATES',
-                    dest='transfer_certificates',
-                    help="If True, try to transfer certificate items to the new course.")
-    )
+    This will remove them from the first course.  Their enrollment mode (i.e.
+    honor, verified, audit, etc.) will persist into the other course(s).
+    """
+    help = dedent(__doc__)
+
+    def add_arguments(self, parser):
+        parser.add_argument('-f', '--from',
+                            metavar='SOURCE_COURSE',
+                            dest='source_course',
+                            required=True,
+                            help='the course to transfer students from')
+        parser.add_argument('-t', '--to',
+                            nargs='+',
+                            metavar='DEST_COURSE',
+                            dest='dest_course_list',
+                            required=True,
+                            help='the new course(s) to enroll the student into')
+        parser.add_argument('-c', '--transfer-certificates',
+                            action='store_true',
+                            help='try to transfer certificate items to the new course')
 
     @transaction.atomic
     def handle(self, *args, **options):
-        source_key = CourseKey.from_string(options.get('source_course', ''))
+        source_key = CourseKey.from_string(options['source_course'])
         dest_keys = []
-        for course_key in options.get('dest_course_list', '').split(','):
+        for course_key in options['dest_course_list']:
             dest_keys.append(CourseKey.from_string(course_key))
 
-        if not source_key or not dest_keys:
-            raise TransferStudentError(u"Must have a source course and destination course specified.")
-
-        tc_option = options.get('transfer_certificates', '')
-        transfer_certificates = ('true' == tc_option.lower()) if tc_option else False
-        if transfer_certificates and len(dest_keys) != 1:
-            raise TransferStudentError(u"Cannot transfer certificate items from one course to many.")
+        if options['transfer_certificates'] and len(dest_keys) > 1:
+            raise TransferStudentError('Cannot transfer certificate items from one course to many.')
 
         source_students = User.objects.filter(
             courseenrollment__course_id=source_key
@@ -73,7 +63,7 @@ class Command(TrackedCommand):
 
         for user in source_students:
             with transaction.atomic():
-                print "Moving {}.".format(user.username)
+                print('Moving {}.'.format(user.username))
                 # Find the old enrollment.
                 enrollment = CourseEnrollment.objects.get(
                     user=user,
@@ -84,14 +74,14 @@ class Command(TrackedCommand):
                 mode = enrollment.mode
                 old_is_active = enrollment.is_active
                 CourseEnrollment.unenroll(user, source_key, skip_refund=True)
-                print u"Unenrolled {} from {}".format(user.username, unicode(source_key))
+                print('Unenrolled {} from {}'.format(user.username, text_type(source_key)))
 
                 for dest_key in dest_keys:
                     if CourseEnrollment.is_enrolled(user, dest_key):
                         # Un Enroll from source course but don't mess
                         # with the enrollment in the destination course.
-                        msg = u"Skipping {}, already enrolled in destination course {}"
-                        print msg.format(user.username, unicode(dest_key))
+                        msg = 'Skipping {}, already enrolled in destination course {}'
+                        print(msg.format(user.username, text_type(dest_key)))
                     else:
                         new_enrollment = CourseEnrollment.enroll(user, dest_key, mode=mode)
 
@@ -100,12 +90,13 @@ class Command(TrackedCommand):
                         if not old_is_active:
                             new_enrollment.update_enrollment(is_active=False, skip_refund=True)
 
-                        if transfer_certificates:
+                        if options['transfer_certificates']:
                             self._transfer_certificate_item(source_key, enrollment, user, dest_keys, new_enrollment)
 
     @staticmethod
     def _transfer_certificate_item(source_key, enrollment, user, dest_keys, new_enrollment):
-        """ Transfer the certificate item from one course to another.
+        """
+        Transfer the certificate item from one course to another.
 
         Do not use this generally, since certificate items are directly associated with a particular purchase.
         This should only be used when a single course to a new location. This cannot be used when transferring
@@ -128,7 +119,7 @@ class Command(TrackedCommand):
                 course_enrollment=enrollment
             )
         except CertificateItem.DoesNotExist:
-            print u"No certificate for {}".format(user)
+            print('No certificate for {}'.format(user))
             return
 
         certificate_item.course_id = dest_keys[0]

common/djangoapps/student/management/tests/test_transfer_students.py

 """
 Tests the transfer student management command
 """
-from django.conf import settings
-from mock import patch, call
-from opaque_keys.edx import locator
 import unittest
-import ddt
 
-from shoppingcart.models import Order, CertificateItem  # pylint: disable=import-error
+from mock import call, patch
+from six import text_type
+
+import ddt
 from course_modes.models import CourseMode
-from student.management.commands import transfer_students
-from student.models import CourseEnrollment, EVENT_NAME_ENROLLMENT_DEACTIVATED, \
-    EVENT_NAME_ENROLLMENT_ACTIVATED, EVENT_NAME_ENROLLMENT_MODE_CHANGED
+from django.conf import settings
+from django.core.management import call_command
+from opaque_keys.edx import locator
+from shoppingcart.models import CertificateItem, Order  # pylint: disable=import-error
+from student.models import (
+    EVENT_NAME_ENROLLMENT_ACTIVATED,
+    EVENT_NAME_ENROLLMENT_DEACTIVATED,
+    EVENT_NAME_ENROLLMENT_MODE_CHANGED,
+    CourseEnrollment
+)
 from student.signals import UNENROLL_DONE
 from student.tests.factories import UserFactory
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
@@ -21,13 +27,17 @@ from xmodule.modulestore.tests.factories import CourseFactory
 @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
 @ddt.ddt
 class TestTransferStudents(ModuleStoreTestCase):
-    """Tests for transferring students between courses."""
+    """
+    Tests for transferring students between courses.
+    """
 
     PASSWORD = 'test'
     signal_fired = False
 
     def setUp(self, **kwargs):
-        """Connect a stub receiver, and analytics event tracking."""
+        """
+        Connect a stub receiver, and analytics event tracking.
+        """
         super(TestTransferStudents, self).setUp()
 
         UNENROLL_DONE.connect(self.assert_unenroll_signal)
@@ -37,13 +47,17 @@ class TestTransferStudents(ModuleStoreTestCase):
         self.addCleanup(UNENROLL_DONE.disconnect, self.assert_unenroll_signal)
 
     def assert_unenroll_signal(self, skip_refund=False, **kwargs):   # pylint: disable=unused-argument
-        """ Signal Receiver stub for testing that the unenroll signal was fired. """
+        """
+        Signal Receiver stub for testing that the unenroll signal was fired.
+        """
         self.assertFalse(self.signal_fired)
         self.assertTrue(skip_refund)
         self.signal_fired = True
 
     def test_transfer_students(self):
-        """ Verify the transfer student command works as intended. """
+        """
+        Verify the transfer student command works as intended.
+        """
         student = UserFactory.create()
         student.set_password(self.PASSWORD)
         student.save()
@@ -52,7 +66,7 @@ class TestTransferStudents(ModuleStoreTestCase):
         original_course_location = locator.CourseLocator('Org0', 'Course0', 'Run0')
         course = self._create_course(original_course_location)
         # Enroll the student in 'verified'
-        CourseEnrollment.enroll(student, course.id, mode="verified")
+        CourseEnrollment.enroll(student, course.id, mode='verified')
 
         # Create and purchase a verified cert for the original course.
         self._create_and_purchase_verified(student, course.id)
@@ -64,13 +78,15 @@ class TestTransferStudents(ModuleStoreTestCase):
         # New Course 2
         course_location_two = locator.CourseLocator('Org2', 'Course2', 'Run2')
         new_course_two = self._create_course(course_location_two)
-        original_key = unicode(course.id)
-        new_key_one = unicode(new_course_one.id)
-        new_key_two = unicode(new_course_two.id)
+        original_key = text_type(course.id)
+        new_key_one = text_type(new_course_one.id)
+        new_key_two = text_type(new_course_two.id)
 
         # Run the actual management command
-        transfer_students.Command().handle(
-            source_course=original_key, dest_course_list=new_key_one + "," + new_key_two
+        call_command(
+            'transfer_students',
+            '--from', original_key,
+            '--to', new_key_one, new_key_two,
         )
         self.assertTrue(self.signal_fired)
 
@@ -123,7 +139,9 @@ class TestTransferStudents(ModuleStoreTestCase):
         self.assertEquals(target_certs[0].order.status, 'purchased')
 
     def _create_course(self, course_location):
-        """ Creates a course """
+        """
+        Creates a course
+        """
         return CourseFactory.create(
             org=course_location.org,
             number=course_location.course,
@@ -131,10 +149,12 @@ class TestTransferStudents(ModuleStoreTestCase):
         )
 
     def _create_and_purchase_verified(self, student, course_id):
-        """ Creates a verified mode for the course and purchases it for the student. """
+        """
+        Creates a verified mode for the course and purchases it for the student.
+        """
         course_mode = CourseMode(course_id=course_id,
-                                 mode_slug="verified",
-                                 mode_display_name="verified cert",
+                                 mode_slug='verified',
+                                 mode_display_name='verified cert',
                                  min_price=50)
         course_mode.save()
         # When there is no expiration date on a verified mode, the user can always get a refund

lms/djangoapps/courseware/management/commands/dump_course_ids.py

@@ -5,7 +5,6 @@ Output is UTF-8 encoded by default.
 """
 from __future__ import unicode_literals
 
-from optparse import make_option
 from textwrap import dedent
 
 from six import text_type

lms/djangoapps/courseware/management/commands/dump_course_structure.py

@@ -17,7 +17,6 @@ The resulting JSON object has one entry for each module in the course:
 """
 
 import json
-from optparse import make_option
 from textwrap import dedent
 
 from django.core.management.base import BaseCommand, CommandError

lms/djangoapps/teams/management/commands/reindex_course_team.py

-""" Management command to update course_teams' search index. """
-from optparse import make_option
+"""
+Management command to update course_teams' search index.
+"""
+from __future__ import print_function, unicode_literals
+
 from textwrap import dedent
 
 from django.conf import settings
 from django.core.exceptions import ObjectDoesNotExist
 from django.core.management import BaseCommand, CommandError
-
 from lms.djangoapps.teams.models import CourseTeam
 
 
 class Command(BaseCommand):
     """
-    Command to reindex course_teams (single, multiple or all available).
-
-    Examples:
-
-        ./manage.py reindex_course_team team1 team2 - reindexes course teams with team_ids team1 and team2
-        ./manage.py reindex_course_team --all - reindexes all available course teams
+    Reindex course_teams (single, multiple or all available).
     """
     help = dedent(__doc__)
 
-    can_import_settings = True
-
-    args = "<course_team_id course_team_id ...>"
-
-    option_list = BaseCommand.option_list + (
-        make_option(
-            '--all',
-            action='store_true',
-            dest='all',
-            default=False,
-            help='Reindex all course teams'
-        ),
-    )
+    def add_arguments(self, parser):
+        # Mutually exclusive groups do not work here because nargs=* arguments
+        # are "required", but required args are not allowed to be part of a
+        # mutually exclusive group.
+        parser.add_argument('--all',
+                            action='store_true',
+                            help='reindex all course teams (do not specify any course teams)')
+        parser.add_argument('course_team_ids',
+                            nargs='*',
+                            metavar='course_team_id',
+                            help='a specific course team to reindex')
 
     def _get_course_team(self, team_id):
-        """ Returns course_team object from team_id. """
+        """
+        Returns course_team object from team_id.
+        """
         try:
             result = CourseTeam.objects.get(team_id=team_id)
         except ObjectDoesNotExist:
-            raise CommandError(u"Argument {0} is not a course_team team_id".format(team_id))
+            raise CommandError('Argument {} is not a course_team team_id'.format(team_id))
 
         return result
 
@@ -52,16 +49,21 @@ class Command(BaseCommand):
         # happen anywhere else that I can't figure out how to avoid it :(
         from ...search_indexes import CourseTeamIndexer
 
-        if len(args) == 0 and not options.get('all', False):
-            raise CommandError(u"reindex_course_team requires one or more arguments: <course_team_id>")
-        elif not settings.FEATURES.get('ENABLE_TEAMS', False):
-            raise CommandError(u"ENABLE_TEAMS must be enabled to use course team indexing")
+        if options['all']:
+            if len(options['course_team_ids']) > 0:
+                raise CommandError('Course teams cannot be specified when --all is also specified')
+        else:
+            if len(options['course_team_ids']) == 0:
+                raise CommandError('At least one course_team_id or --all needs to be specified')
+
+        if not settings.FEATURES.get('ENABLE_TEAMS', False):
+            raise CommandError('ENABLE_TEAMS must be enabled to use course team indexing')
 
-        if options.get('all', False):
+        if options['all']:
             course_teams = CourseTeam.objects.all()
         else:
-            course_teams = map(self._get_course_team, args)
+            course_teams = map(self._get_course_team, options['course_team_ids'])
 
         for course_team in course_teams:
-            print "Indexing {id}".format(id=course_team.team_id)
+            print('Indexing {}'.format(course_team.team_id))
             CourseTeamIndexer.index(course_team)

lms/djangoapps/teams/management/commands/tests/test_reindex_course_team.py

-""" Tests for course_team reindex command """
+"""
+Tests for course_team reindex command
+"""
 
 import ddt
 from django.core.management import CommandError, call_command
@@ -16,7 +18,9 @@ COURSE_KEY1 = CourseKey.from_string('edx/history/1')
 
 @ddt.ddt
 class ReindexCourseTeamTest(SharedModuleStoreTestCase):
-    """Tests for the ReindexCourseTeam command"""
+    """
+    Tests for the ReindexCourseTeam command
+    """
 
     def setUp(self):
         """
@@ -31,33 +35,50 @@ class ReindexCourseTeamTest(SharedModuleStoreTestCase):
         self.search_engine = SearchEngine.get_search_engine(index='index_course_team')
 
     def test_given_no_arguments_raises_command_error(self):
-        """ Test that raises CommandError for incorrect arguments. """
-        with self.assertRaisesRegexp(CommandError, ".* requires one or more arguments.*"):
+        """
+        Test that raises CommandError for incorrect arguments.
+        """
+        with self.assertRaisesRegexp(CommandError, '.*At least one course_team_id or --all needs to be specified.*'):
             call_command('reindex_course_team')
 
+    def test_given_conflicting_arguments_raises_command_error(self):
+        """
+        Test that raises CommandError for incorrect arguments.
+        """
+        with self.assertRaisesRegexp(CommandError, '.*Course teams cannot be specified when --all is also specified.*'):
+            call_command('reindex_course_team', self.team1.team_id, all=True)
+
     @patch.dict('django.conf.settings.FEATURES', {'ENABLE_TEAMS': False})
     def test_teams_search_flag_disabled_raises_command_error(self):
-        """ Test that raises CommandError for disabled feature flag. """
-        with self.assertRaisesRegexp(CommandError, ".*ENABLE_TEAMS must be enabled.*"):
+        """
+        Test that raises CommandError for disabled feature flag.
+        """
+        with self.assertRaisesRegexp(CommandError, '.*ENABLE_TEAMS must be enabled.*'):
             call_command('reindex_course_team', self.team1.team_id)
 
     def test_given_invalid_team_id_raises_command_error(self):
-        """ Test that raises CommandError for invalid team id. """
+        """
+        Test that raises CommandError for invalid team id.
+        """
         team_id = u'team4'
-        error_str = 'Argument {0} is not a course_team team_id'.format(team_id)
+        error_str = 'Argument {} is not a course_team team_id'.format(team_id)
         with self.assertRaisesRegexp(CommandError, error_str):
             call_command('reindex_course_team', team_id)
 
     @patch.object(CourseTeamIndexer, 'index')
     def test_single_team_id(self, mock_index):
-        """ Test that command indexes a single passed team. """
+        """
+        Test that command indexes a single passed team.
+        """
         call_command('reindex_course_team', self.team1.team_id)
         mock_index.assert_called_once_with(self.team1)
         mock_index.reset_mock()
 
     @patch.object(CourseTeamIndexer, 'index')
     def test_multiple_team_id(self, mock_index):
-        """ Test that command indexes multiple passed teams. """
+        """
+        Test that command indexes multiple passed teams.
+        """
         call_command('reindex_course_team', self.team1.team_id, self.team2.team_id)
         mock_index.assert_any_call(self.team1)
         mock_index.assert_any_call(self.team2)
@@ -65,7 +86,9 @@ class ReindexCourseTeamTest(SharedModuleStoreTestCase):
 
     @patch.object(CourseTeamIndexer, 'index')
     def test_all_teams(self, mock_index):
-        """ Test that command indexes all teams. """
+        """
+        Test that command indexes all teams.
+        """
         call_command('reindex_course_team', all=True)
         mock_index.assert_any_call(self.team1)
         mock_index.assert_any_call(self.team2)