From e8a9fd0ff42f817252521a0442f1885e31ee4a1e Mon Sep 17 00:00:00 2001 From: "J. Cliff Dyer" Date: Tue, 21 Mar 2017 10:36:30 -0400 Subject: [PATCH 1/2] Remove broken management command Also remove tests that weren't getting run. --- .../management/commands/compute_grades.py | 55 ------------------- .../management/tests/test_compute_grades.py | 34 ------------ 2 files changed, 89 deletions(-) delete mode 100644 lms/djangoapps/instructor/management/commands/compute_grades.py delete mode 100644 lms/djangoapps/instructor/management/tests/test_compute_grades.py diff --git a/lms/djangoapps/instructor/management/commands/compute_grades.py b/lms/djangoapps/instructor/management/commands/compute_grades.py deleted file mode 100644 index 3cdc1852ec..0000000000 --- a/lms/djangoapps/instructor/management/commands/compute_grades.py +++ /dev/null @@ -1,55 +0,0 @@ -#!/usr/bin/python -""" -django management command: dump grades to csv files -for use by batch processes -""" -from django.http import Http404 -from django.core.management.base import BaseCommand - -from courseware.courses import get_course_by_id -from lms.djangoapps.instructor.offline_gradecalc import offline_grade_calculation -from opaque_keys import InvalidKeyError -from opaque_keys.edx.keys import CourseKey -from opaque_keys.edx.locations import SlashSeparatedCourseKey - - -class Command(BaseCommand): - help = "Compute grades for all students in a course, and store result in DB.\n" - help += "Usage: compute_grades course_id_or_dir \n" - help += " course_id_or_dir: space separated list of either course_ids or course_dirs\n" - help += 'Example course_id: MITx/8.01rq_MW/Classical_Mechanics_Reading_Questions_Fall_2012_MW_Section' - - def add_arguments(self, parser): - parser.add_argument('course_id_or_dir', nargs='+') - - def handle(self, *args, **options): - - print "options = ", options - - try: - course_ids = options['course_id_or_dir'] - except KeyError: - print self.help - return - course_key = None - # parse out the course id into a coursekey - for course_id in course_ids: - try: - course_key = CourseKey.from_string(course_id) - # if it's not a new-style course key, parse it from an old-style - # course key - except InvalidKeyError: - course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) - try: - get_course_by_id(course_key) - except Http404 as err: - print "-----------------------------------------------------------------------------" - print "Sorry, cannot find course with id {}".format(course_id) - print "Got exception {}".format(err) - print "Please provide a course ID or course data directory name, eg content-mit-801rq" - return - - print "-----------------------------------------------------------------------------" - print "Computing grades for {}".format(course_id) - - offline_grade_calculation(course_key) diff --git a/lms/djangoapps/instructor/management/tests/test_compute_grades.py b/lms/djangoapps/instructor/management/tests/test_compute_grades.py deleted file mode 100644 index 78a7feb87d..0000000000 --- a/lms/djangoapps/instructor/management/tests/test_compute_grades.py +++ /dev/null @@ -1,34 +0,0 @@ -# coding=utf-8 - -"""Tests for Django instructor management commands""" - -from unittest import TestCase - -from django.core.management import call_command -from mock import Mock - -from lms.djangoapps.instructor.offline_gradecalc import offline_grade_calculation # pylint: disable=unused-import -from opaque_keys.edx.keys import CourseKey -from opaque_keys.edx.locator import CourseLocator - - -class InstructorCommandsTest(TestCase): - """Unittest subclass for instructor module management commands.""" - - def test_compute_grades_command(self): - course_id = 'MITx/0.0001/2016_Fall' - offline_grade_calculation = Mock() # pylint: disable=redefined-outer-name - CourseKey.from_string = Mock(return_value=CourseLocator(*course_id.split('/'))) - call_command('compute_grades', ) - self.asertEqual(offline_grade_calculation.call_count, 1) # pylint: disable=no-member - offline_grade_calculation.assert_called_with(CourseKey.from_string('MITx/0.0001/2016_Fall')) - - def test_compute_grades_command_multiple_courses(self): - course_id1 = 'MITx/0.0001/2016_Fall' - course_id2 = 'MITx/0.0002/2016_Fall' - CourseKey.from_string = Mock() - offline_grade_calculation = Mock() # pylint: disable=redefined-outer-name - call_command('compute_grades', '{0} {1}'.format(course_id1, course_id1)) - self.asertEqual(offline_grade_calculation.call_count, 2) # pylint: disable=no-member - CourseKey.from_string.assert_called_with(course_id1) - CourseKey.from_string.assert_called_with(course_id2) From b89736c75ca42b15b9054570b66f3b5101a6ab34 Mon Sep 17 00:00:00 2001 From: "J. Cliff Dyer" Date: Wed, 15 Mar 2017 16:43:19 -0400 Subject: [PATCH 2/2] Add compute_grades management command. TNL-6689 --- .../management/commands/compute_grades.py | 122 ++++++++++++++++++ .../management/commands/tests/__init__.py | 0 .../commands/tests/test_compute_grades.py | 93 +++++++++++++ .../commands/tests/test_reset_grades.py | 8 +- lms/djangoapps/grades/tasks.py | 9 +- .../tests/test_generate_course_blocks.py | 4 +- openedx/core/lib/command_utils.py | 11 +- openedx/core/lib/tests/test_command_utils.py | 48 +++++++ 8 files changed, 283 insertions(+), 12 deletions(-) create mode 100644 lms/djangoapps/grades/management/commands/compute_grades.py create mode 100644 lms/djangoapps/grades/management/commands/tests/__init__.py create mode 100644 lms/djangoapps/grades/management/commands/tests/test_compute_grades.py create mode 100644 openedx/core/lib/tests/test_command_utils.py diff --git a/lms/djangoapps/grades/management/commands/compute_grades.py b/lms/djangoapps/grades/management/commands/compute_grades.py new file mode 100644 index 0000000000..4d19c7483f --- /dev/null +++ b/lms/djangoapps/grades/management/commands/compute_grades.py @@ -0,0 +1,122 @@ +""" +Command to compute all grades for specified courses. +""" + +from __future__ import absolute_import, division, print_function, unicode_literals + +import logging + +from django.core.management.base import BaseCommand +import six + +from openedx.core.lib.command_utils import ( + get_mutually_exclusive_required_option, + parse_course_keys, +) +from student.models import CourseEnrollment +from xmodule.modulestore.django import modulestore + +from ... import tasks + + +log = logging.getLogger(__name__) + + +class Command(BaseCommand): + """ + Example usage: + $ ./manage.py lms compute_grades --all_courses --settings=devstack + $ ./manage.py lms compute_grades 'edX/DemoX/Demo_Course' --settings=devstack + """ + args = '' + help = 'Computes grade values for all learners in specified courses.' + + def add_arguments(self, parser): + """ + Entry point for subclassed commands to add custom arguments. + """ + parser.add_argument( + '--courses', + dest='courses', + nargs='+', + help='List of (space separated) courses that need grades computed.', + ) + parser.add_argument( + '--all_courses', + help='Compute grades for all courses.', + action='store_true', + default=False, + ) + parser.add_argument( + '--routing_key', + dest='routing_key', + help='Celery routing key to use.', + ) + parser.add_argument( + '--batch_size', + help='Maximum number of students to calculate grades for, per celery task.', + default=100, + type=int, + ) + parser.add_argument( + '--start_index', + help='Offset from which to start processing enrollments.', + default=0, + type=int, + ) + + def handle(self, *args, **options): + self._set_log_level(options) + + for course_key in self._get_course_keys(options): + self.enqueue_compute_grades_for_course_tasks(course_key, options) + + def enqueue_compute_grades_for_course_tasks(self, course_key, options): + """ + Enqueue celery tasks to compute and persist all grades for the + specified course, in batches. + """ + enrollment_count = CourseEnrollment.objects.filter(course_id=course_key).count() + if enrollment_count == 0: + log.warning("No enrollments found for {}".format(course_key)) + for offset in six.moves.range(options['start_index'], enrollment_count, options['batch_size']): + # If the number of enrollments increases after the tasks are + # created, the most recent enrollments may not get processed. + # This is an acceptable limitation for our known use cases. + task_options = {'routing_key': options['routing_key']} if options.get('routing_key') else {} + kwargs = { + 'course_key': six.text_type(course_key), + 'offset': offset, + 'batch_size': options['batch_size'], + } + result = tasks.compute_grades_for_course.apply_async( + kwargs=kwargs, + options=task_options, + ) + log.info("Persistent grades: Created {task_name}[{task_id}] with arguments {kwargs}".format( + task_name=tasks.compute_grades_for_course.name, + task_id=result.task_id, + kwargs=kwargs, + )) + + def _get_course_keys(self, options): + """ + Return a list of courses that need scores computed. + """ + courses_mode = get_mutually_exclusive_required_option(options, 'courses', 'all_courses') + if courses_mode == 'all_courses': + course_keys = [course.id for course in modulestore().get_course_summaries()] + else: + course_keys = parse_course_keys(options['courses']) + return course_keys + + def _set_log_level(self, options): + """ + Sets logging levels for this module and the block structure + cache module, based on the given the options. + """ + if options.get('verbosity') == 0: + log_level = logging.WARNING + elif options.get('verbosity') >= 1: + log_level = logging.INFO + log.setLevel(log_level) diff --git a/lms/djangoapps/grades/management/commands/tests/__init__.py b/lms/djangoapps/grades/management/commands/tests/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/lms/djangoapps/grades/management/commands/tests/test_compute_grades.py b/lms/djangoapps/grades/management/commands/tests/test_compute_grades.py new file mode 100644 index 0000000000..319030f466 --- /dev/null +++ b/lms/djangoapps/grades/management/commands/tests/test_compute_grades.py @@ -0,0 +1,93 @@ +""" +Tests for compute_grades management command. +""" + +# pylint: disable=protected-access + +from __future__ import absolute_import, division, print_function, unicode_literals + +import ddt +from django.contrib.auth import get_user_model +from django.core.management import CommandError, call_command +from mock import patch +import six + +from student.models import CourseEnrollment +from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory + +from lms.djangoapps.grades.management.commands import compute_grades + + +@ddt.ddt +class TestComputeGrades(SharedModuleStoreTestCase): + """ + Tests compute_grades management command. + """ + num_users = 3 + num_courses = 5 + + @classmethod + def setUpClass(cls): + super(TestComputeGrades, cls).setUpClass() + User = get_user_model() # pylint: disable=invalid-name + cls.command = compute_grades.Command() + + cls.courses = [CourseFactory.create() for _ in range(cls.num_courses)] + cls.course_keys = [six.text_type(course.id) for course in cls.courses] + cls.users = [User.objects.create(username='user{}'.format(idx)) for idx in range(cls.num_users)] + + for user in cls.users: + for course in cls.courses: + CourseEnrollment.enroll(user, course.id) + + def test_select_all_courses(self): + courses = self.command._get_course_keys({'all_courses': True}) + self.assertEqual( + sorted(six.text_type(course) for course in courses), + self.course_keys, + ) + + def test_specify_courses(self): + courses = self.command._get_course_keys({'courses': [self.course_keys[0], self.course_keys[1], 'd/n/e']}) + self.assertEqual( + [six.text_type(course) for course in courses], + [self.course_keys[0], self.course_keys[1], 'd/n/e'], + ) + + def test_selecting_invalid_course(self): + with self.assertRaises(CommandError): + self.command._get_course_keys({'courses': [self.course_keys[0], self.course_keys[1], 'badcoursekey']}) + + @patch('lms.djangoapps.grades.tasks.compute_grades_for_course') + def test_tasks_fired(self, mock_task): + call_command( + 'compute_grades', + '--routing_key=key', + '--batch_size=2', + '--courses', + self.course_keys[0], + self.course_keys[3], + 'd/n/e' # No tasks created for nonexistent course, because it has no enrollments + ) + self.assertEqual( + mock_task.apply_async.call_args_list, + [ + ({ + 'options': {'routing_key': 'key'}, + 'kwargs': {'course_key': self.course_keys[0], 'batch_size': 2, 'offset': 0} + },), + ({ + 'options': {'routing_key': 'key'}, + 'kwargs': {'course_key': self.course_keys[0], 'batch_size': 2, 'offset': 2} + },), + ({ + 'options': {'routing_key': 'key'}, + 'kwargs': {'course_key': self.course_keys[3], 'batch_size': 2, 'offset': 0} + },), + ({ + 'options': {'routing_key': 'key'}, + 'kwargs': {'course_key': self.course_keys[3], 'batch_size': 2, 'offset': 2} + },), + ], + ) diff --git a/lms/djangoapps/grades/management/commands/tests/test_reset_grades.py b/lms/djangoapps/grades/management/commands/tests/test_reset_grades.py index 94e2298521..9977aec467 100644 --- a/lms/djangoapps/grades/management/commands/tests/test_reset_grades.py +++ b/lms/djangoapps/grades/management/commands/tests/test_reset_grades.py @@ -302,17 +302,17 @@ class TestResetGrades(TestCase): self.command.handle(delete=True, all_courses=True, db_table="not course or subsection") def test_no_run_mode(self): - with self.assertRaisesMessage(CommandError, 'Either --delete or --dry_run must be specified.'): + with self.assertRaisesMessage(CommandError, 'Must specify exactly one of --delete, --dry_run'): self.command.handle(all_courses=True) def test_both_run_modes(self): - with self.assertRaisesMessage(CommandError, 'Both --delete and --dry_run cannot be specified.'): + with self.assertRaisesMessage(CommandError, 'Must specify exactly one of --delete, --dry_run'): self.command.handle(all_courses=True, dry_run=True, delete=True) def test_no_course_mode(self): - with self.assertRaisesMessage(CommandError, 'Either --courses or --all_courses must be specified.'): + with self.assertRaisesMessage(CommandError, 'Must specify exactly one of --courses, --all_courses'): self.command.handle(dry_run=True) def test_both_course_modes(self): - with self.assertRaisesMessage(CommandError, 'Both --courses and --all_courses cannot be specified.'): + with self.assertRaisesMessage(CommandError, 'Must specify exactly one of --courses, --all_courses'): self.command.handle(dry_run=True, all_courses=True, courses=['some/course/key']) diff --git a/lms/djangoapps/grades/tasks.py b/lms/djangoapps/grades/tasks.py index 263be346cf..15cffcadc7 100644 --- a/lms/djangoapps/grades/tasks.py +++ b/lms/djangoapps/grades/tasks.py @@ -3,7 +3,6 @@ This module contains tasks for asynchronous execution of grade updates. """ from celery import task -from celery.exceptions import Retry from django.conf import settings from django.contrib.auth.models import User from django.core.exceptions import ValidationError @@ -56,6 +55,14 @@ class _BaseTask(PersistOnFailureTask, LoggedTask): # pylint: disable=abstract-m abstract = True +@task +def compute_grades_for_course(course_key, offset, batch_size): # pylint: disable=unused-argument + """ + TODO: TNL-6690: Fill this task in and remove pylint disables + """ + pass + + @task(bind=True, base=_BaseTask, default_retry_delay=30, routing_key=settings.RECALCULATE_GRADES_ROUTING_KEY) def recalculate_subsection_grade_v3(self, **kwargs): """ diff --git a/openedx/core/djangoapps/content/block_structure/management/commands/tests/test_generate_course_blocks.py b/openedx/core/djangoapps/content/block_structure/management/commands/tests/test_generate_course_blocks.py index 14bddeac7b..3de601b239 100644 --- a/openedx/core/djangoapps/content/block_structure/management/commands/tests/test_generate_course_blocks.py +++ b/openedx/core/djangoapps/content/block_structure/management/commands/tests/test_generate_course_blocks.py @@ -158,11 +158,11 @@ class TestGenerateCourseBlocks(ModuleStoreTestCase): self.command.handle(all_courses=False) def test_no_course_mode(self): - with self.assertRaisesMessage(CommandError, 'Either --courses or --all_courses must be specified.'): + with self.assertRaisesMessage(CommandError, 'Must specify exactly one of --courses, --all_courses'): self.command.handle() def test_both_course_modes(self): - with self.assertRaisesMessage(CommandError, 'Both --courses and --all_courses cannot be specified.'): + with self.assertRaisesMessage(CommandError, 'Must specify exactly one of --courses, --all_courses'): self.command.handle(all_courses=True, courses=['some/course/key']) @ddt.data( diff --git a/openedx/core/lib/command_utils.py b/openedx/core/lib/command_utils.py index 08c17207a8..66bec9dabb 100644 --- a/openedx/core/lib/command_utils.py +++ b/openedx/core/lib/command_utils.py @@ -8,17 +8,18 @@ from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey -def get_mutually_exclusive_required_option(options, option_1, option_2): +def get_mutually_exclusive_required_option(options, *selections): """ Validates that exactly one of the 2 given options is specified. Returns the name of the found option. """ - validate_mutually_exclusive_option(options, option_1, option_2) - if not options.get(option_1) and not options.get(option_2): - raise CommandError('Either --{} or --{} must be specified.'.format(option_1, option_2)) + selected = [sel for sel in selections if options.get(sel)] + if len(selected) != 1: + selection_string = u', '.join('--{}'.format(selection) for selection in selections) - return option_1 if options.get(option_1) else option_2 + raise CommandError(u'Must specify exactly one of {}'.format(selection_string)) + return selected[0] def validate_mutually_exclusive_option(options, option_1, option_2): diff --git a/openedx/core/lib/tests/test_command_utils.py b/openedx/core/lib/tests/test_command_utils.py new file mode 100644 index 0000000000..b9ef428fd0 --- /dev/null +++ b/openedx/core/lib/tests/test_command_utils.py @@ -0,0 +1,48 @@ +""" +Tests of management command utility code +""" + +from unittest import TestCase + +import ddt +from django.core.management import CommandError + +from .. import command_utils + + +@ddt.ddt +class MutuallyExclusiveRequiredOptionsTestCase(TestCase): + """ + Test that mutually exclusive required options allow one and only one option + to be specified with a true value. + """ + @ddt.data( + (['opta'], {'opta': 1}, 'opta'), + (['opta', 'optb'], {'opta': 1}, 'opta'), + (['opta', 'optb'], {'optb': 1}, 'optb'), + (['opta', 'optb'], {'opta': 1, 'optc': 1}, 'opta'), + (['opta', 'optb'], {'opta': 1, 'optb': 0}, 'opta'), + (['opta', 'optb', 'optc'], {'optc': 1, 'optd': 1}, 'optc'), + (['opta', 'optb', 'optc'], {'optc': 1}, 'optc'), + (['opta', 'optb', 'optc'], {'optd': 0, 'optc': 1}, 'optc'), + ) + @ddt.unpack + def test_successful_exclusive_options(self, exclusions, opts, expected): + result = command_utils.get_mutually_exclusive_required_option(opts, *exclusions) + self.assertEqual(result, expected) + + @ddt.data( + (['opta'], {'opta': 0}), + (['opta', 'optb'], {'opta': 1, 'optb': 1}), + (['opta', 'optb'], {'optc': 1, 'optd': 1}), + (['opta', 'optb'], {}), + (['opta', 'optb', 'optc'], {'opta': 1, 'optc': 1}), + (['opta', 'optb', 'optc'], {'opta': 1, 'optb': 1}), + (['opta', 'optb', 'optc'], {'optb': 1, 'optc': 1}), + (['opta', 'optb', 'optc'], {'opta': 1, 'optb': 1, 'optc': 1}), + (['opta', 'optb', 'optc'], {}), + ) + @ddt.unpack + def test_invalid_exclusive_options(self, exclusions, opts): + with self.assertRaises(CommandError): + command_utils.get_mutually_exclusive_required_option(opts, *exclusions)