From 27315f442a3ee475df75eb13dc0363788fa0015c Mon Sep 17 00:00:00 2001 From: John Eskew Date: Mon, 13 Nov 2017 16:23:06 -0500 Subject: [PATCH] Move more mgmt commands from optparse to argparse. --- .../commands/bulk_change_enrollment.py | 7 +- .../commands/generate_course_structure.py | 24 ++-- .../management/commands/email_opt_in_list.py | 134 ++++++++---------- .../tests/test_email_opt_in_list.py | 18 +-- 4 files changed, 87 insertions(+), 96 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..e1f65034ba 100644 --- a/common/djangoapps/student/management/commands/bulk_change_enrollment.py +++ b/common/djangoapps/student/management/commands/bulk_change_enrollment.py @@ -5,11 +5,10 @@ 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 from course_modes.models import CourseMode +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from student.models import CourseEnrollment from xmodule.modulestore.django import modulestore @@ -17,7 +16,9 @@ logger = logging.getLogger(__name__) # pylint: disable=invalid-name class Command(BaseCommand): - """Management command to change many user enrollments at once.""" + """ + Management command to change many user enrollments at once. + """ help = """ Change the enrollment status for all users enrolled in a diff --git a/openedx/core/djangoapps/content/course_structures/management/commands/generate_course_structure.py b/openedx/core/djangoapps/content/course_structures/management/commands/generate_course_structure.py index da347a3e61..5d2e164a28 100644 --- a/openedx/core/djangoapps/content/course_structures/management/commands/generate_course_structure.py +++ b/openedx/core/djangoapps/content/course_structures/management/commands/generate_course_structure.py @@ -2,16 +2,14 @@ Django Management Command: Generate Course Structure Generates and stores course structure information for one or more courses. """ - import logging -from optparse import make_option from django.core.management.base import BaseCommand from opaque_keys.edx.keys import CourseKey -from xmodule.modulestore.django import modulestore +from six import text_type from openedx.core.djangoapps.content.course_structures.tasks import update_course_structure - +from xmodule.modulestore.django import modulestore log = logging.getLogger(__name__) @@ -20,15 +18,13 @@ class Command(BaseCommand): """ Generates and stores course structure information for one or more courses. """ - args = '' help = 'Generates and stores course structure for one or more courses.' - option_list = BaseCommand.option_list + ( - make_option('--all', - action='store_true', - default=False, - help='Generate structures for all courses.'), - ) + def add_arguments(self, parser): + parser.add_argument('course_id', nargs='*') + parser.add_argument('--all', + action='store_true', + help='Generate structures for all courses.') def handle(self, *args, **options): """ @@ -37,7 +33,7 @@ class Command(BaseCommand): if options['all']: course_keys = [course.id for course in modulestore().get_courses()] else: - course_keys = [CourseKey.from_string(arg) for arg in args] + course_keys = [CourseKey.from_string(arg) for arg in options['course_id']] if not course_keys: log.fatal('No courses specified.') @@ -52,9 +48,9 @@ class Command(BaseCommand): # TODO Future improvement: Use .delay(), add return value to ResultSet, and wait for execution of # all tasks using ResultSet.join(). I (clintonb) am opting not to make this improvement right now # as I do not have time to test it fully. - update_course_structure.apply(args=[unicode(course_key)]) + update_course_structure.apply(args=[text_type(course_key)]) except Exception as ex: log.exception('An error occurred while generating course structure for %s: %s', - unicode(course_key), ex.message) + text_type(course_key), ex.message) log.info('Finished generating course structures.') diff --git a/openedx/core/djangoapps/user_api/management/commands/email_opt_in_list.py b/openedx/core/djangoapps/user_api/management/commands/email_opt_in_list.py index dc80990998..0a2bc14f8f 100644 --- a/openedx/core/djangoapps/user_api/management/commands/email_opt_in_list.py +++ b/openedx/core/djangoapps/user_api/management/commands/email_opt_in_list.py @@ -19,22 +19,23 @@ When reports are generated, we need to handle: The command will always use the read replica database if one is configured. """ -import os.path -import csv -import time -import datetime -import contextlib -import logging -import optparse +from __future__ import unicode_literals + +import contextlib +import csv +import datetime +import logging +import os.path +import time -from django.core.management.base import BaseCommand, CommandError from django.conf import settings +from django.core.management.base import BaseCommand, CommandError from django.db import connections from django.utils import timezone - from opaque_keys.edx.keys import CourseKey -from xmodule.modulestore.django import modulestore +from six import text_type +from xmodule.modulestore.django import modulestore DEFAULT_CHUNK_SIZE = 10 @@ -46,20 +47,28 @@ def chunks(sequence, chunk_size): class Command(BaseCommand): - """Generate a list of email opt-in values for user enrollments. """ - - args = " --courses=COURSE_ID_LIST --email-optin-chunk-size=CHUNK_SIZE" + """ + Generate a list of email opt-in values for user enrollments. + """ help = "Generate a list of email opt-in values for user enrollments." - option_list = BaseCommand.option_list + ( - optparse.make_option('--courses ', action='store'), - optparse.make_option( - '--email-optin-chunk-size', - action='store', - type='int', - default=DEFAULT_CHUNK_SIZE, - dest='email_optin_chunk_size', - help='The number of courses to get opt-in information for in a single query.') - ) + + def add_arguments(self, parser): + parser.add_argument('file_path', + metavar='OUTPUT_FILENAME', + help='Path where to output the email opt-in list.') + parser.add_argument('org_list', + nargs='+', + metavar='ORG_ALIASES', + help='List of orgs for which to retrieve email opt-in info.') + parser.add_argument('--courses', + metavar='COURSE_ID_LIST', + help='List of course IDs for which to retrieve email opt-in info.') + parser.add_argument('--email-optin-chunk-size', + type=int, + default=DEFAULT_CHUNK_SIZE, + dest='email_optin_chunk_size', + metavar='CHUNK_SIZE', + help='Number of courses for which to get opt-in information in a single query.') # Fields output in the CSV OUTPUT_FIELD_NAMES = [ @@ -77,10 +86,11 @@ class Command(BaseCommand): QUERY_INTERVAL = 1000 # Default datetime if the user has not set a preference - DEFAULT_DATETIME_STR = datetime.datetime(year=2014, month=12, day=1).isoformat(' ') + DEFAULT_DATETIME_STR = datetime.datetime(year=2014, month=12, day=1).isoformat(str(' ')) def handle(self, *args, **options): - """Execute the command. + """ + Execute the command. Arguments: file_path (str): Path to the output file. @@ -92,9 +102,12 @@ class Command(BaseCommand): Raises: CommandError - """ - file_path, org_list = self._parse_args(args) + file_path = options['file_path'] + org_list = options['org_list'] + + if os.path.exists(file_path): + raise CommandError("File already exists at '{path}'".format(path=file_path)) # Retrieve all the courses for the org. # If we were given a specific list of courses to include, @@ -116,15 +129,15 @@ class Command(BaseCommand): # If no courses are found, abort if not courses: raise CommandError( - u"No courses found for orgs: {orgs}".format( + "No courses found for orgs: {orgs}".format( orgs=", ".join(org_list) ) ) # Let the user know what's about to happen LOGGER.info( - u"Retrieving data for courses: {courses}".format( - courses=", ".join([unicode(course) for course in courses]) + "Retrieving data for courses: {courses}".format( + courses=", ".join([text_type(course) for course in courses]) ) ) @@ -137,44 +150,17 @@ class Command(BaseCommand): self._write_email_opt_in_prefs(file_handle, org_list, course_group) # Remind the user where the output file is - LOGGER.info(u"Output file: {file_path}".format(file_path=file_path)) - - def _parse_args(self, args): - """Check and parse arguments. - - Validates that the right number of args were provided - and that the output file doesn't already exist. - - Arguments: - args (list): List of arguments given at the command line. - - Returns: - Tuple of (file_path, org_list) - - Raises: - CommandError - - """ - if len(args) < 2: - raise CommandError(u"Usage: {args}".format(args=self.args)) - - file_path = args[0] - org_list = args[1:] - - if os.path.exists(file_path): - raise CommandError("File already exists at '{path}'".format(path=file_path)) - - return file_path, org_list + LOGGER.info("Output file: {file_path}".format(file_path=file_path)) def _get_courses_for_org(self, org_aliases): - """Retrieve all course keys for a particular org. + """ + Retrieve all course keys for a particular org. Arguments: org_aliases (list): List of aliases for the org. Returns: List of `CourseKey`s - """ all_courses = modulestore().get_courses() orgs_lowercase = [org.lower() for org in org_aliases] @@ -186,14 +172,17 @@ class Command(BaseCommand): @contextlib.contextmanager def _log_execution_time(self): - """Context manager for measuring execution time. """ + """ + Context manager for measuring execution time. + """ start_time = time.time() yield execution_time = time.time() - start_time - LOGGER.info(u"Execution time: {time} seconds".format(time=execution_time)) + LOGGER.info("Execution time: {time} seconds".format(time=execution_time)) def _write_email_opt_in_prefs(self, file_handle, org_aliases, courses): - """Write email opt-in preferences to the output file. + """ + Write email opt-in preferences to the output file. This will generate a CSV with one row for each enrollment. This means that the user's "opt in" preference will be specified @@ -209,14 +198,13 @@ class Command(BaseCommand): Returns: None - """ writer = csv.DictWriter(file_handle, fieldnames=self.OUTPUT_FIELD_NAMES) writer.writeheader() cursor = self._db_cursor() query = ( - u""" + """ SELECT user.`id` AS `user_id`, user.`username` AS username, @@ -281,17 +269,17 @@ class Command(BaseCommand): row_count += 1 # Log the number of rows we processed - LOGGER.info(u"Retrieved {num_rows} records.".format(num_rows=row_count)) + LOGGER.info("Retrieved {num_rows} records.".format(num_rows=row_count)) def _iterate_results(self, cursor): - """Iterate through the results of a database query, fetching in chunks. + """ + Iterate through the results of a database query, fetching in chunks. Arguments: cursor: The database cursor Yields: tuple of row values from the query - """ while True: rows = cursor.fetchmany(self.QUERY_INTERVAL) @@ -301,11 +289,15 @@ class Command(BaseCommand): yield row def _sql_list(self, values): - """Serialize a list of values for including in a SQL "IN" statement. """ - return u",".join([u'"{}"'.format(val) for val in values]) + """ + Serialize a list of values for including in a SQL "IN" statement. + """ + return ",".join(['"{}"'.format(val) for val in values]) def _db_cursor(self): - """Return a database cursor to the read replica if one is available. """ + """ + Return a database cursor to the read replica if one is available. + """ # Use the read replica if one has been configured db_alias = ( 'read_replica' diff --git a/openedx/core/djangoapps/user_api/management/tests/test_email_opt_in_list.py b/openedx/core/djangoapps/user_api/management/tests/test_email_opt_in_list.py index b42102decf..332f7fbe95 100644 --- a/openedx/core/djangoapps/user_api/management/tests/test_email_opt_in_list.py +++ b/openedx/core/djangoapps/user_api/management/tests/test_email_opt_in_list.py @@ -9,7 +9,9 @@ from nose.plugins.attrib import attr import ddt from django.contrib.auth.models import User +from django.core.management import call_command from django.core.management.base import CommandError +from six import text_type from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory @@ -214,7 +216,7 @@ class EmailOptInListTest(ModuleStoreTestCase): output = self._run_command(self.TEST_ORG, chunk_size=2) course_ids = [row['course_id'].strip().decode('utf-8') for row in output] for course in self.courses: - assert unicode(course.id) in course_ids + assert text_type(course.id) in course_ids # Choose numbers before and after the query interval boundary @ddt.data(2, 3, 4, 5, 6, 7, 8, 9) @@ -261,10 +263,10 @@ class EmailOptInListTest(ModuleStoreTestCase): def test_not_enough_args(self, num_args): args = ["dummy"] * num_args expected_msg_regex = ( - "^Usage: --courses=COURSE_ID_LIST --email-optin-chunk-size=CHUNK_SIZE$" + "^Error: too few arguments$" ) with self.assertRaisesRegexp(CommandError, expected_msg_regex): - email_opt_in_list.Command().handle(*args) + call_command('email_opt_in_list', *args) def test_file_already_exists(self): temp_file = tempfile.NamedTemporaryFile(delete=True) @@ -273,7 +275,7 @@ class EmailOptInListTest(ModuleStoreTestCase): temp_file.close() with self.assertRaisesRegexp(CommandError, "^File already exists"): - email_opt_in_list.Command().handle(temp_file.name, self.TEST_ORG) + call_command('email_opt_in_list', temp_file.name, self.TEST_ORG) def test_no_user_profile(self): """ @@ -376,7 +378,7 @@ class EmailOptInListTest(ModuleStoreTestCase): output_path = os.path.join(temp_dir_path, self.OUTPUT_FILE_NAME) org_list = [org] + other_names if only_courses is not None: - only_courses = ",".join(unicode(course_id) for course_id in only_courses) + only_courses = ",".join(text_type(course_id) for course_id in only_courses) command = email_opt_in_list.Command() @@ -388,7 +390,7 @@ class EmailOptInListTest(ModuleStoreTestCase): kwargs = {'courses': only_courses} if chunk_size: kwargs['email_optin_chunk_size'] = chunk_size - command.handle(output_path, *org_list, **kwargs) + call_command('email_opt_in_list', output_path, *org_list, **kwargs) # Retrieve the output from the file try: @@ -442,8 +444,8 @@ class EmailOptInListTest(ModuleStoreTestCase): if hasattr(user, 'profile') else '' ), - "course_id": unicode(course_id).encode('utf-8'), - "is_opted_in_for_email": unicode(opt_in_pref), + "course_id": text_type(course_id).encode('utf-8'), + "is_opted_in_for_email": text_type(opt_in_pref), "preference_set_datetime": ( self._latest_pref_set_datetime(self.user) if kwargs.get("expect_pref_datetime", True)