From b07402e75154a87fef7d12d049ace94b183579f3 Mon Sep 17 00:00:00 2001 From: Troy Sankey Date: Thu, 9 Nov 2017 11:55:41 -0500 Subject: [PATCH 1/3] remove unused optparse imports --- .../student/management/commands/bulk_change_enrollment.py | 1 - lms/djangoapps/courseware/management/commands/dump_course_ids.py | 1 - .../courseware/management/commands/dump_course_structure.py | 1 - 3 files changed, 3 deletions(-) diff --git a/common/djangoapps/student/management/commands/bulk_change_enrollment.py b/common/djangoapps/student/management/commands/bulk_change_enrollment.py index 9dbda939c0..3214d4d932 100644 --- a/common/djangoapps/student/management/commands/bulk_change_enrollment.py +++ b/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 diff --git a/lms/djangoapps/courseware/management/commands/dump_course_ids.py b/lms/djangoapps/courseware/management/commands/dump_course_ids.py index 25dee61594..1175d3447e 100644 --- a/lms/djangoapps/courseware/management/commands/dump_course_ids.py +++ b/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 diff --git a/lms/djangoapps/courseware/management/commands/dump_course_structure.py b/lms/djangoapps/courseware/management/commands/dump_course_structure.py index d272f8d6a2..120db44b2f 100644 --- a/lms/djangoapps/courseware/management/commands/dump_course_structure.py +++ b/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 From 877c9fafc36a537eaed8bc34873552d1effb6957 Mon Sep 17 00:00:00 2001 From: Troy Sankey Date: Thu, 9 Nov 2017 14:16:46 -0500 Subject: [PATCH 2/3] teams management commands cleanup for django 1.11 --- .../commands/reindex_course_team.py | 64 ++++++++++--------- .../tests/test_reindex_course_team.py | 45 +++++++++---- 2 files changed, 67 insertions(+), 42 deletions(-) diff --git a/lms/djangoapps/teams/management/commands/reindex_course_team.py b/lms/djangoapps/teams/management/commands/reindex_course_team.py index cb5647e7e2..76183b6346 100644 --- a/lms/djangoapps/teams/management/commands/reindex_course_team.py +++ b/lms/djangoapps/teams/management/commands/reindex_course_team.py @@ -1,45 +1,42 @@ -""" 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 = "" - - 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: ") - 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 options.get('all', False): + if not settings.FEATURES.get('ENABLE_TEAMS', False): + raise CommandError('ENABLE_TEAMS must be enabled to use course team indexing') + + 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) diff --git a/lms/djangoapps/teams/management/commands/tests/test_reindex_course_team.py b/lms/djangoapps/teams/management/commands/tests/test_reindex_course_team.py index f04a731c10..5bf099a24a 100644 --- a/lms/djangoapps/teams/management/commands/tests/test_reindex_course_team.py +++ b/lms/djangoapps/teams/management/commands/tests/test_reindex_course_team.py @@ -1,4 +1,6 @@ -""" 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) From 3685b1ada8a434245625f58b0721c6991cc68ab5 Mon Sep 17 00:00:00 2001 From: Troy Sankey Date: Thu, 9 Nov 2017 15:40:01 -0500 Subject: [PATCH 3/3] More student management commands cleanup for Django 1.11 Seems like transfer_students.py was missed in the last attempt to cleanup commands in this app. --- .../management/commands/transfer_students.py | 97 +++++++++---------- .../tests/test_transfer_students.py | 64 +++++++----- 2 files changed, 86 insertions(+), 75 deletions(-) diff --git a/common/djangoapps/student/management/commands/transfer_students.py b/common/djangoapps/student/management/commands/transfer_students.py index 555ef1ec3b..84306e732e 100644 --- a/common/djangoapps/student/management/commands/transfer_students.py +++ b/common/djangoapps/student/management/commands/transfer_students.py @@ -1,71 +1,61 @@ """ 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] diff --git a/common/djangoapps/student/management/tests/test_transfer_students.py b/common/djangoapps/student/management/tests/test_transfer_students.py index 6431f60095..1b8ab9f087 100644 --- a/common/djangoapps/student/management/tests/test_transfer_students.py +++ b/common/djangoapps/student/management/tests/test_transfer_students.py @@ -1,17 +1,23 @@ """ 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