From 0016d6284d744226a7dc0b2423408a24f5bf556a Mon Sep 17 00:00:00 2001 From: Awais Jibran Date: Thu, 18 Jan 2018 00:31:49 +0500 Subject: [PATCH] Fix staff override score If a learner has not accessed/attempted the score override functionality didn't work. This PR is intended to fix this behaviour and the override should work regardless of learner has accessed or attempted a problem or not. --- lms/djangoapps/courseware/models.py | 12 +++ .../tasks_helper/module_state.py | 95 ++++++++++++------- .../instructor_task/tests/test_tasks.py | 77 +++++++++++---- 3 files changed, 132 insertions(+), 52 deletions(-) diff --git a/lms/djangoapps/courseware/models.py b/lms/djangoapps/courseware/models.py index 9ebc903a0f..90ebbfe5ee 100644 --- a/lms/djangoapps/courseware/models.py +++ b/lms/djangoapps/courseware/models.py @@ -149,6 +149,18 @@ class StudentModule(models.Model): def __unicode__(self): return unicode(repr(self)) + @classmethod + def get_state_by_params(cls, course_id, module_state_keys, student_id=None): + """ + Return all model instances that correspond to a course and module keys. + + Student ID is optional keyword argument, if provided it narrows down the instances. + """ + module_states = cls.objects.filter(course_id=course_id, module_state_key__in=module_state_keys) + if student_id: + module_states = module_states.filter(student_id=student_id) + return module_states + class BaseStudentModuleHistory(models.Model): """Abstract class containing most fields used by any class diff --git a/lms/djangoapps/instructor_task/tasks_helper/module_state.py b/lms/djangoapps/instructor_task/tasks_helper/module_state.py index e4bb470246..c9ad9f19cc 100644 --- a/lms/djangoapps/instructor_task/tasks_helper/module_state.py +++ b/lms/djangoapps/instructor_task/tasks_helper/module_state.py @@ -6,6 +6,7 @@ import logging from time import time from django.contrib.auth.models import User +from django.utils.translation import ugettext_noop from opaque_keys.edx.keys import UsageKey import dogstats_wrapper as dog_stats_api @@ -33,19 +34,12 @@ def perform_module_state_update(update_fcn, filter_fcn, _entry_id, course_id, ta """ Performs generic update by visiting StudentModule instances with the update_fcn provided. - StudentModule instances are those that match the specified `course_id` and `module_state_key`. - If `student_identifier` is not None, it is used as an additional filter to limit the modules to those belonging - to that student. If `student_identifier` is None, performs update on modules for all students on the specified - problem. - - If a `filter_fcn` is not None, it is applied to the query that has been constructed. It takes one - argument, which is the query being filtered, and returns the filtered version of the query. - - The `update_fcn` is called on each StudentModule that passes the resulting filtering. - It is passed four arguments: the module_descriptor for the module pointed to by the - module_state_key, the particular StudentModule to update, the xmodule_instance_args, and the task_input - being passed through. If the value returned by the update function evaluates to a boolean True, - the update is successful; False indicates the update on the particular student module failed. + The student modules are fetched for update the `update_fcn` is called on each StudentModule + that passes the resulting filtering. It is passed four arguments: the module_descriptor for + the module pointed to by the module_state_key, the particular StudentModule to update, the + xmodule_instance_args, and the task_input being passed through. If the value returned by the + update function evaluates to a boolean True, the update is successful; False indicates the update + on the particular student module failed. A raised exception indicates a fatal condition -- that no other student modules should be considered. The return value is a dict containing the task's results, with the following keys: @@ -69,6 +63,7 @@ def perform_module_state_update(update_fcn, filter_fcn, _entry_id, course_id, ta problem_url = task_input.get('problem_url') entrance_exam_url = task_input.get('entrance_exam_url') student_identifier = task_input.get('student') + override_score_task = action_name == ugettext_noop('overridden') problems = {} # if problem_url is present make a usage key from it @@ -85,27 +80,11 @@ def perform_module_state_update(update_fcn, filter_fcn, _entry_id, course_id, ta problems = get_problems_in_section(entrance_exam_url) usage_keys = [UsageKey.from_string(location) for location in problems.keys()] - # find the modules in question - modules_to_update = StudentModule.objects.filter(course_id=course_id, module_state_key__in=usage_keys) + modules_to_update = _get_modules_to_update( + course_id, usage_keys, student_identifier, filter_fcn, override_score_task + ) - # give the option of updating an individual student. If not specified, - # then updates all students who have responded to a problem so far - student = None - if student_identifier is not None: - # if an identifier is supplied, then look for the student, - # and let it throw an exception if none is found. - if "@" in student_identifier: - student = User.objects.get(email=student_identifier) - elif student_identifier is not None: - student = User.objects.get(username=student_identifier) - - if student is not None: - modules_to_update = modules_to_update.filter(student_id=student.id) - - if filter_fcn is not None: - modules_to_update = filter_fcn(modules_to_update) - - task_progress = TaskProgress(action_name, modules_to_update.count(), start_time) + task_progress = TaskProgress(action_name, len(modules_to_update), start_time) task_progress.update_task_state() for module_to_update in modules_to_update: @@ -406,3 +385,53 @@ def _get_task_id_from_xmodule_args(xmodule_instance_args): return UNKNOWN_TASK_ID else: return xmodule_instance_args.get('task_id', UNKNOWN_TASK_ID) + + +def _get_modules_to_update(course_id, usage_keys, student_identifier, filter_fcn, override_score_task=False): + """ + Fetches a StudentModule instances for a given `course_id`, `student` object, and `usage_keys`. + + StudentModule instances are those that match the specified `course_id` and `module_state_key`. + If `student_identifier` is not None, it is used as an additional filter to limit the modules to those belonging + to that student. If `student_identifier` is None, performs update on modules for all students on the specified + problem. + The matched instances are then applied `filter_fcn` if not None. It filters out the matched instances. + It takes one argument, which is the query being filtered, and returns the filtered version of the query. + If `override_score_task` is True and we there were not matching instances of StudentModule, try to create + those instances. This is only for override scores and the use case is for learners that have missed the deadline. + + Arguments: + course_id(str): The unique identifier for the course. + usage_keys(list): List of UsageKey objects + student_identifier(str): Identifier for a student or None. The identifier can be either username or email + filter_fcn: If it is not None, it is applied to the query that has been constructed. + override_score_task (bool): Optional argument which indicates if it is an override score or not. + """ + def get_student(): + """ Fetches student instance if an identifier is provided, else return None """ + if student_identifier is None: + return None + + student_identifier_type = 'email' if '@' in student_identifier else 'username' + student_query_params = {student_identifier_type: student_identifier} + return User.objects.get(**student_query_params) + + module_query_params = {'course_id': course_id, 'module_state_keys': usage_keys} + + # give the option of updating an individual student. If not specified, + # then updates all students who have responded to a problem so far + student = get_student() + if student: + module_query_params['student_id'] = student.id + + student_modules = StudentModule.get_state_by_params(**module_query_params) + if filter_fcn is not None: + student_modules = filter_fcn(student_modules) + + can_create_student_modules = (override_score_task and (student_modules.count() == 0) and student is not None) + if can_create_student_modules: + student_modules = [ + StudentModule.objects.get_or_create(course_id=course_id, student=student, module_state_key=key)[0] + for key in usage_keys + ] + return student_modules diff --git a/lms/djangoapps/instructor_task/tests/test_tasks.py b/lms/djangoapps/instructor_task/tests/test_tasks.py index 5bfd43a09a..704b227958 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks.py @@ -16,6 +16,7 @@ from mock import MagicMock, Mock, patch from nose.plugins.attrib import attr from opaque_keys.edx.locations import i4xEncoder +from course_modes.models import CourseMode from courseware.models import StudentModule from courseware.tests.factories import StudentModuleFactory from lms.djangoapps.instructor_task.exceptions import UpdateProblemModuleStateError @@ -31,7 +32,6 @@ from lms.djangoapps.instructor_task.tasks import ( from lms.djangoapps.instructor_task.tasks_helper.misc import upload_ora2_data from lms.djangoapps.instructor_task.tests.factories import InstructorTaskFactory from lms.djangoapps.instructor_task.tests.test_base import InstructorTaskModuleTestCase -from student.tests.factories import CourseEnrollmentFactory, UserFactory from xmodule.modulestore.exceptions import ItemNotFoundError PROBLEM_URL_NAME = "test_urlname" @@ -69,11 +69,13 @@ class TestInstructorTasks(InstructorTaskModuleTestCase): task_input['score'] = score course_id = course_id or self.course.id - instructor_task = InstructorTaskFactory.create(course_id=course_id, - requester=self.instructor, - task_input=json.dumps(task_input, cls=i4xEncoder), - task_key='dummy value', - task_id=task_id) + instructor_task = InstructorTaskFactory.create( + course_id=course_id, + requester=self.instructor, + task_input=json.dumps(task_input, cls=i4xEncoder), + task_key='dummy value', + task_id=task_id + ) return instructor_task def _get_xmodule_instance_args(self): @@ -149,19 +151,31 @@ class TestInstructorTasks(InstructorTaskModuleTestCase): def _create_students_with_state(self, num_students, state=None, grade=0, max_grade=1): """Create students, a problem, and StudentModule objects for testing""" self.define_option_problem(PROBLEM_URL_NAME) - students = [ - UserFactory.create(username='robot%d' % i, email='robot+test+%d@edx.org' % i) + enrolled_students = self._create_and_enroll_students(num_students) + + for student in enrolled_students: + StudentModuleFactory.create( + course_id=self.course.id, + module_state_key=self.location, + student=student, + grade=grade, + max_grade=max_grade, + state=state + ) + return enrolled_students + + def _create_and_enroll_students(self, num_students, mode=CourseMode.DEFAULT_MODE_SLUG): + """Create & enroll students for testing""" + return [ + self.create_student(username='robot%d' % i, email='robot+test+%d@edx.org' % i, mode=mode) for i in xrange(num_students) ] - for student in students: - CourseEnrollmentFactory.create(course_id=self.course.id, user=student) - StudentModuleFactory.create(course_id=self.course.id, - module_state_key=self.location, - student=student, - grade=grade, - max_grade=max_grade, - state=state) - return students + + def _create_students_with_no_state(self, num_students): + """Create students and a problem for testing""" + self.define_option_problem(PROBLEM_URL_NAME) + enrolled_students = self._create_and_enroll_students(num_students) + return enrolled_students def _assert_num_attempts(self, students, num_attempts): """Check the number attempts for all students is the same""" @@ -248,12 +262,15 @@ class TestOverrideScoreInstructorTask(TestInstructorTasks): self._test_missing_current_task(override_problem_score) def test_override_undefined_course(self): + """Tests that override problem score raises exception with undefined course""" self._test_undefined_course(override_problem_score) def test_override_undefined_problem(self): + """Tests that override problem score raises exception with undefined problem""" self._test_undefined_problem(override_problem_score) def test_override_with_no_state(self): + """Tests override score with no problem state in StudentModule""" self._test_run_with_no_state(override_problem_score, 'overridden') def test_override_with_failure(self): @@ -266,6 +283,9 @@ class TestOverrideScoreInstructorTask(TestInstructorTasks): self._test_run_with_short_error_msg(override_problem_score) def test_overriding_non_scorable(self): + """ + Tests that override problem score raises an error if module descriptor has not `set_score` method. + """ input_state = json.dumps({'done': True}) num_students = 1 self._create_students_with_state(num_students, input_state) @@ -287,7 +307,7 @@ class TestOverrideScoreInstructorTask(TestInstructorTasks): def test_overriding_unaccessable(self): """ - Tests rescores a problem in a course, for all students fails if user has answered a + Tests score override for a problem in a course, for all students fails if user has answered a problem to which user does not have access to. """ input_state = json.dumps({'done': True}) @@ -310,7 +330,7 @@ class TestOverrideScoreInstructorTask(TestInstructorTasks): def test_overriding_success(self): """ - Tests rescores a problem in a course, for all students succeeds. + Tests score override for a problem in a course, for all students succeeds. """ mock_instance = MagicMock() getattr(mock_instance, 'override_problem_score').return_value = None @@ -334,6 +354,25 @@ class TestOverrideScoreInstructorTask(TestInstructorTasks): action_name='overridden' ) + def test_overriding_success_with_no_state(self): + """ + Tests that score override is successful for a learner when they have no state. + """ + num_students = 1 + enrolled_students = self._create_students_with_no_state(num_students=num_students) + task_entry = self._create_input_entry(score=1, student_ident=enrolled_students[0].username) + + self._run_task_with_mock_celery(override_problem_score, task_entry.id, task_entry.task_id) + self.assert_task_output( + output=self.get_task_output(task_entry.id), + total=num_students, + attempted=num_students, + succeeded=num_students, + skipped=0, + failed=0, + action_name='overridden' + ) + @attr(shard=3) @ddt.ddt