diff --git a/lms/djangoapps/instructor_task/api_helper.py b/lms/djangoapps/instructor_task/api_helper.py index 3decd36e1f..290166e347 100644 --- a/lms/djangoapps/instructor_task/api_helper.py +++ b/lms/djangoapps/instructor_task/api_helper.py @@ -277,7 +277,7 @@ def submit_task(request, task_type, task_class, course_id, task_input, task_key) # submit task: task_id = instructor_task.task_id - task_args = [instructor_task.id, course_id, task_input, _get_xmodule_instance_args(request)] + task_args = [instructor_task.id, _get_xmodule_instance_args(request)] task_class.apply_async(task_args, task_id=task_id) return instructor_task diff --git a/lms/djangoapps/instructor_task/tasks.py b/lms/djangoapps/instructor_task/tasks.py index ba5acc6f43..b1b2751195 100644 --- a/lms/djangoapps/instructor_task/tasks.py +++ b/lms/djangoapps/instructor_task/tasks.py @@ -1,19 +1,19 @@ """ -This file contains tasks that are designed to perform background operations on the +This file contains tasks that are designed to perform background operations on the running state of a course. """ from celery import task -from instructor_task.tasks_helper import (_update_problem_module_state, - _rescore_problem_module_state, - _reset_problem_attempts_module_state, - _delete_problem_module_state) +from instructor_task.tasks_helper import (update_problem_module_state, + rescore_problem_module_state, + reset_attempts_module_state, + delete_problem_module_state) @task -def rescore_problem(entry_id, course_id, task_input, xmodule_instance_args): +def rescore_problem(entry_id, xmodule_instance_args): """Rescores problem in `course_id`. `entry_id` is the id value of the InstructorTask entry that corresponds to this task. @@ -29,19 +29,15 @@ def rescore_problem(entry_id, course_id, task_input, xmodule_instance_args): to instantiate an xmodule instance. """ action_name = 'rescored' - update_fcn = _rescore_problem_module_state + update_fcn = rescore_problem_module_state filter_fcn = lambda(modules_to_update): modules_to_update.filter(state__contains='"done": true') - problem_url = task_input.get('problem_url') - student_ident = None - if 'student' in task_input: - student_ident = task_input['student'] - return _update_problem_module_state(entry_id, course_id, problem_url, student_ident, - update_fcn, action_name, filter_fcn=filter_fcn, - xmodule_instance_args=xmodule_instance_args) + return update_problem_module_state(entry_id, + update_fcn, action_name, filter_fcn=filter_fcn, + xmodule_instance_args=xmodule_instance_args) @task -def reset_problem_attempts(entry_id, course_id, task_input, xmodule_instance_args): +def reset_problem_attempts(entry_id, xmodule_instance_args): """Resets problem attempts to zero for `problem_url` in `course_id` for all students. `entry_id` is the id value of the InstructorTask entry that corresponds to this task. @@ -54,15 +50,14 @@ def reset_problem_attempts(entry_id, course_id, task_input, xmodule_instance_arg to instantiate an xmodule instance. """ action_name = 'reset' - update_fcn = _reset_problem_attempts_module_state - problem_url = task_input.get('problem_url') - return _update_problem_module_state(entry_id, course_id, problem_url, None, - update_fcn, action_name, filter_fcn=None, - xmodule_instance_args=xmodule_instance_args) + update_fcn = reset_attempts_module_state + return update_problem_module_state(entry_id, + update_fcn, action_name, filter_fcn=None, + xmodule_instance_args=xmodule_instance_args) @task -def delete_problem_state(entry_id, course_id, task_input, xmodule_instance_args): +def delete_problem_state(entry_id, xmodule_instance_args): """Deletes problem state entirely for `problem_url` in `course_id` for all students. `entry_id` is the id value of the InstructorTask entry that corresponds to this task. @@ -75,8 +70,7 @@ def delete_problem_state(entry_id, course_id, task_input, xmodule_instance_args) to instantiate an xmodule instance. """ action_name = 'deleted' - update_fcn = _delete_problem_module_state - problem_url = task_input.get('problem_url') - return _update_problem_module_state(entry_id, course_id, problem_url, None, - update_fcn, action_name, filter_fcn=None, - xmodule_instance_args=xmodule_instance_args) + update_fcn = delete_problem_module_state + return update_problem_module_state(entry_id, + update_fcn, action_name, filter_fcn=None, + xmodule_instance_args=xmodule_instance_args) diff --git a/lms/djangoapps/instructor_task/tasks_helper.py b/lms/djangoapps/instructor_task/tasks_helper.py index a5a2d758ac..faea903022 100644 --- a/lms/djangoapps/instructor_task/tasks_helper.py +++ b/lms/djangoapps/instructor_task/tasks_helper.py @@ -1,5 +1,5 @@ """ -This file contains tasks that are designed to perform background operations on the +This file contains tasks that are designed to perform background operations on the running state of a course. @@ -50,7 +50,7 @@ class UpdateProblemModuleStateError(Exception): def _perform_module_state_update(course_id, module_state_key, student_identifier, update_fcn, action_name, filter_fcn, - xmodule_instance_args): + xmodule_instance_args): """ Performs generic update by visiting StudentModule instances with the update_fcn provided. @@ -161,7 +161,7 @@ def _save_course_task(course_task): course_task.save() -def _update_problem_module_state(entry_id, course_id, module_state_key, student_ident, update_fcn, action_name, filter_fcn, +def update_problem_module_state(entry_id, update_fcn, action_name, filter_fcn, xmodule_instance_args): """ Performs generic update by visiting StudentModule instances with the update_fcn provided. @@ -195,15 +195,20 @@ def _update_problem_module_state(entry_id, course_id, module_state_key, student_ result object that Celery creates. """ - task_id = current_task.request.id - fmt = 'Starting to update problem modules as task "{task_id}": course "{course_id}" problem "{state_key}": nothing {action} yet' - TASK_LOG.info(fmt.format(task_id=task_id, course_id=course_id, state_key=module_state_key, action=action_name)) # get the InstructorTask to be updated. If this fails, then let the exception return to Celery. # There's no point in catching it here. entry = InstructorTask.objects.get(pk=entry_id) - entry.task_id = task_id - _save_course_task(entry) + + # get inputs to use in this task from the entry: + task_id = entry.task_id + course_id = entry.course_id + task_input = json.loads(entry.task_input) + module_state_key = task_input.get('problem_url') + student_ident = task_input['student'] if 'student' in task_input else None + + fmt = 'Starting to update problem modules as task "{task_id}": course "{course_id}" problem "{state_key}": nothing {action} yet' + TASK_LOG.info(fmt.format(task_id=task_id, course_id=course_id, state_key=module_state_key, action=action_name)) # add task_id to xmodule_instance_args, so that it can be output with tracking info: if xmodule_instance_args is not None: @@ -212,6 +217,16 @@ def _update_problem_module_state(entry_id, course_id, module_state_key, student_ # now that we have an entry we can try to catch failures: task_progress = None try: + # check that the task_id submitted in the InstructorTask matches the current task + # that is running. + request_task_id = current_task.request.id + if task_id != request_task_id: + fmt = 'Requested task "{task_id}" did not match actual task "{actual_id}"' + message = fmt.format(task_id=task_id, course_id=course_id, state_key=module_state_key, actual_id=request_task_id) + TASK_LOG.error(message) + raise UpdateProblemModuleStateError(message) + + # now do the work: with dog_stats_api.timer('courseware.tasks.module.{0}.overall_time'.format(action_name)): task_progress = _perform_module_state_update(course_id, module_state_key, student_ident, update_fcn, action_name, filter_fcn, xmodule_instance_args) @@ -280,7 +295,7 @@ def _get_module_instance_for_task(course_id, student, module_descriptor, xmodule @transaction.autocommit -def _rescore_problem_module_state(module_descriptor, student_module, xmodule_instance_args=None): +def rescore_problem_module_state(module_descriptor, student_module, xmodule_instance_args=None): ''' Takes an XModule descriptor and a corresponding StudentModule object, and performs rescoring on the student's problem submission. @@ -330,7 +345,7 @@ def _rescore_problem_module_state(module_descriptor, student_module, xmodule_ins @transaction.autocommit -def _reset_problem_attempts_module_state(_module_descriptor, student_module, xmodule_instance_args=None): +def reset_attempts_module_state(_module_descriptor, student_module, xmodule_instance_args=None): """ Resets problem attempts to zero for specified `student_module`. @@ -356,7 +371,7 @@ def _reset_problem_attempts_module_state(_module_descriptor, student_module, xmo @transaction.autocommit -def _delete_problem_module_state(_module_descriptor, student_module, xmodule_instance_args=None): +def delete_problem_module_state(_module_descriptor, student_module, xmodule_instance_args=None): """ Delete the StudentModule entry. diff --git a/lms/djangoapps/instructor_task/tests/test_api.py b/lms/djangoapps/instructor_task/tests/test_api.py index 14af159cd3..666d69dde0 100644 --- a/lms/djangoapps/instructor_task/tests/test_api.py +++ b/lms/djangoapps/instructor_task/tests/test_api.py @@ -24,7 +24,7 @@ from instructor_task.api import (get_running_instructor_tasks, submit_delete_problem_state_for_all_students) from instructor_task.api_helper import (QUEUING, - AlreadyRunningError, +# AlreadyRunningError, encode_problem_and_student_input, ) @@ -61,12 +61,12 @@ class TaskSubmitTestCase(TestCase): task_input, task_key = encode_problem_and_student_input(self.problem_url, student) instructor_task = InstructorTaskFactory.create(course_id=TEST_COURSE_ID, - requester=self.instructor, - task_input=json.dumps(task_input), - task_key=task_key, - task_id=task_id, - task_state=task_state, - task_output=progress_json) + requester=self.instructor, + task_input=json.dumps(task_input), + task_key=task_key, + task_id=task_id, + task_state=task_state, + task_output=progress_json) return instructor_task def _create_failure_entry(self): @@ -101,6 +101,7 @@ class TaskSubmitTestCase(TestCase): self.assertEquals(set(task_ids), set(progress_task_ids)) def _get_instructor_task_status(self, task_id): + """Returns status corresponding to task_id via api method.""" request = Mock() request.REQUEST = {'task_id': task_id} return instructor_task_status(request) diff --git a/lms/djangoapps/instructor_task/tests/test_integration.py b/lms/djangoapps/instructor_task/tests/test_integration.py index e875f5d8e7..7980715bfc 100644 --- a/lms/djangoapps/instructor_task/tests/test_integration.py +++ b/lms/djangoapps/instructor_task/tests/test_integration.py @@ -2,7 +2,7 @@ Integration Test for LMS instructor-initiated background tasks Runs tasks on answers to course problems to validate that code -paths actually work. +paths actually work. """ import logging @@ -32,7 +32,6 @@ from instructor_task.api import (submit_rescore_problem_for_all_students, submit_reset_problem_attempts_for_all_students, submit_delete_problem_state_for_all_students) from instructor_task.models import InstructorTask -from instructor_task.tests.factories import InstructorTaskFactory from instructor_task.views import instructor_task_status @@ -235,6 +234,7 @@ class TestRescoringBase(LoginEnrollmentTestCase, ModuleStoreTestCase): self.assertGreater(len(state['student_answers']), 0) def get_task_status(self, task_id): + """Use api method to fetch task status, using mock request.""" mock_request = Mock() mock_request.REQUEST = {'task_id': task_id} response = instructor_task_status(mock_request)