Merge pull request #14639 from edx/neem/scorable-xblock

Make instructor_tasks aware of ScorableXBlockMixin

lms/djangoapps/grades/signals/handlers.py

@@ -6,6 +6,7 @@ from logging import getLogger
 
 from django.dispatch import receiver
 from submissions.models import score_set, score_reset
+from xblock.scorable import ScorableXBlockMixin, Score
 
 from courseware.model_data import get_score, set_score
 from eventtracking import tracker
@@ -131,7 +132,14 @@ def score_published_handler(sender, block, user, raw_earned, raw_possible, only_
                 )
 
     if update_score:
+        # Set the problem score in CSM.
         score_modified_time = set_score(user.id, block.location, raw_earned, raw_possible)
+
+        # Set the problem score on the xblock.
+        if isinstance(block, ScorableXBlockMixin):
+            block.set_score(Score(raw_earned=raw_earned, raw_possible=raw_possible))
+
+        # Fire a signal (consumed by enqueue_subsection_update, below)
         PROBLEM_RAW_SCORE_CHANGED.send(
             sender=None,
             raw_earned=raw_earned,

lms/djangoapps/instructor_task/tasks_helper.py

@@ -310,7 +310,8 @@ def perform_module_state_update(update_fcn, filter_fcn, _entry_id, course_id, ta
 
     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.
+    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.
@@ -405,12 +406,18 @@ def perform_module_state_update(update_fcn, filter_fcn, _entry_id, course_id, ta
 
 def _get_task_id_from_xmodule_args(xmodule_instance_args):
     """Gets task_id from `xmodule_instance_args` dict, or returns default value if missing."""
-    return xmodule_instance_args.get('task_id', UNKNOWN_TASK_ID) if xmodule_instance_args is not None else UNKNOWN_TASK_ID
+    if xmodule_instance_args is None:
+        return UNKNOWN_TASK_ID
+    else:
+        return xmodule_instance_args.get('task_id', UNKNOWN_TASK_ID)
 
 
 def _get_xqueue_callback_url_prefix(xmodule_instance_args):
     """Gets prefix to use when constructing xqueue_callback_url."""
-    return xmodule_instance_args.get('xqueue_callback_url_prefix', '') if xmodule_instance_args is not None else ''
+    if xmodule_instance_args is None:
+        return ''
+    else:
+        return xmodule_instance_args.get('xqueue_callback_url_prefix', '')
 
 
 def _get_track_function_for_task(student, xmodule_instance_args=None, source_page='x_module_task'):
@@ -517,61 +524,42 @@ def rescore_problem_module_state(xmodule_instance_args, module_descriptor, stude
             TASK_LOG.warning(msg)
             return UPDATE_STATUS_FAILED
 
-        if not hasattr(instance, 'rescore_problem'):
-            # This should also not happen, since it should be already checked in the caller,
-            # but check here to be sure.
+        # TODO: (TNL-6594)  Remove this switch once rescore_problem support
+        # once CAPA uses ScorableXBlockMixin.
+        for method in ['rescore', 'rescore_problem']:
+            rescore_method = getattr(instance, method, None)
+            if rescore_method is not None:
+                break
+        else:  # for-else: Neither method exists on the block.
+            # This should not happen, since it should be already checked in the
+            # caller, but check here to be sure.
             msg = "Specified problem does not support rescoring."
             raise UpdateProblemModuleStateError(msg)
 
-        # Set the tracking info before this call, because
-        # it makes downstream calls that create events.
-        # We retrieve and store the id here because
+        # Set the tracking info before this call, because it makes downstream
+        # calls that create events.  We retrieve and store the id here because
         # the request cache will be erased during downstream calls.
         event_transaction_id = create_new_event_transaction_id()
         set_event_transaction_type(GRADES_RESCORE_EVENT_TYPE)
 
-        result = instance.rescore_problem(only_if_higher=task_input['only_if_higher'])
+        result = rescore_method(only_if_higher=task_input['only_if_higher'])
         instance.save()
 
-        if 'success' not in result:
-            # don't consider these fatal, but false means that the individual call didn't complete:
-            TASK_LOG.warning(
-                u"error processing rescore call for course %(course)s, problem %(loc)s "
-                u"and student %(student)s: unexpected response %(msg)s",
-                dict(
-                    msg=result,
-                    course=course_id,
-                    loc=usage_key,
-                    student=student
-                )
-            )
-            return UPDATE_STATUS_FAILED
-        elif result['success'] not in ['correct', 'incorrect']:
-            TASK_LOG.warning(
-                u"error processing rescore call for course %(course)s, problem %(loc)s "
-                u"and student %(student)s: %(msg)s",
-                dict(
-                    msg=result['success'],
-                    course=course_id,
-                    loc=usage_key,
-                    student=student
-                )
-            )
-            return UPDATE_STATUS_FAILED
-        else:
+        if result is None or result.get(u'success') in {u'correct', u'incorrect'}:
             TASK_LOG.debug(
                 u"successfully processed rescore call for course %(course)s, problem %(loc)s "
-                u"and student %(student)s: %(msg)s",
+                u"and student %(student)s",
                 dict(
-                    msg=result['success'],
                     course=course_id,
                     loc=usage_key,
                     student=student
                 )
             )
+
+            if result is not None:  # Only for CAPA. This will get moved to the grade handler.
                 new_weighted_earned, new_weighted_possible = weighted_score(
-                result['new_raw_earned'],
-                result['new_raw_possible'],
+                    result['new_raw_earned'] if result else None,
+                    result['new_raw_possible'] if result else None,
                     module_descriptor.weight,
                 )
 
@@ -592,8 +580,19 @@ def rescore_problem_module_state(xmodule_instance_args, module_descriptor, stude
                             'event_transaction_type': unicode(GRADES_RESCORE_EVENT_TYPE),
                         }
                     )
-
             return UPDATE_STATUS_SUCCEEDED
+        else:
+            TASK_LOG.warning(
+                u"error processing rescore call for course %(course)s, problem %(loc)s "
+                u"and student %(student)s: %(msg)s",
+                dict(
+                    msg=result.get('success', result),
+                    course=course_id,
+                    loc=usage_key,
+                    student=student
+                )
+            )
+            return UPDATE_STATUS_FAILED
 
 
 @outer_atomic

lms/djangoapps/instructor_task/tests/test_tasks.py

@@ -3,24 +3,24 @@ Unit tests for LMS instructor-initiated background tasks.
 
 Runs tasks on answers to course problems to validate that code
 paths actually work.
-
 """
+
+from functools import partial
 import json
 from uuid import uuid4
 
-from mock import Mock, MagicMock, patch
-from nose.plugins.attrib import attr
-
 from celery.states import SUCCESS, FAILURE
+import ddt
 from django.utils.translation import ugettext_noop
-from functools import partial
+from mock import Mock, MagicMock, patch
+from nose.plugins.attrib import attr
 
-from xmodule.modulestore.exceptions import ItemNotFoundError
 from opaque_keys.edx.locations import i4xEncoder
 
 from courseware.models import StudentModule
 from courseware.tests.factories import StudentModuleFactory
 from student.tests.factories import UserFactory, CourseEnrollmentFactory
+from xmodule.modulestore.exceptions import ItemNotFoundError
 
 from lms.djangoapps.instructor_task.models import InstructorTask
 from lms.djangoapps.instructor_task.tests.test_base import InstructorTaskModuleTestCase
@@ -41,10 +41,16 @@ PROBLEM_URL_NAME = "test_urlname"
 
 
 class TestTaskFailure(Exception):
+    """
+    An example exception to indicate failure of a mocked task.
+    """
     pass
 
 
 class TestInstructorTasks(InstructorTaskModuleTestCase):
+    """
+    Ensure tasks behave as expected.
+    """
 
     def setUp(self):
         super(TestInstructorTasks, self).setUp()
@@ -219,6 +225,7 @@ class TestInstructorTasks(InstructorTaskModuleTestCase):
 
 
 @attr(shard=3)
+@ddt.ddt
 class TestRescoreInstructorTask(TestInstructorTasks):
     """Tests problem-rescoring instructor task."""
 
@@ -267,6 +274,7 @@ class TestRescoreInstructorTask(TestInstructorTasks):
         task_entry = self._create_input_entry()
         mock_instance = MagicMock()
         del mock_instance.rescore_problem
+        del mock_instance.rescore
         with patch('lms.djangoapps.instructor_task.tasks_helper.get_module_for_descriptor_internal') as mock_get_module:
             mock_get_module.return_value = mock_instance
             with self.assertRaises(UpdateProblemModuleStateError):
@@ -300,22 +308,24 @@ class TestRescoreInstructorTask(TestInstructorTasks):
             action_name='rescored'
         )
 
-    def test_rescoring_success(self):
+    @ddt.data(
+        ('rescore', None),
+        ('rescore_problem', {'success': 'correct', 'new_raw_earned': 1, 'new_raw_possible': 1})
+    )
+    @ddt.unpack
+    def test_rescoring_success(self, rescore_method, rescore_result):
         """
         Tests rescores a problem in a course, for all students succeeds.
         """
+        mock_instance = MagicMock()
+        other_method = ({'rescore', 'rescore_problem'} - {rescore_method}).pop()
+        getattr(mock_instance, rescore_method).return_value = rescore_result
+        delattr(mock_instance, other_method)
+
         input_state = json.dumps({'done': True})
         num_students = 10
         self._create_students_with_state(num_students, input_state)
         task_entry = self._create_input_entry()
-        mock_instance = Mock()
-        mock_instance.rescore_problem = Mock(
-            return_value={
-                'success': 'correct',
-                'new_raw_earned': 1,
-                'new_raw_possible': 1,
-            }
-        )
         with patch('lms.djangoapps.instructor_task.tasks_helper.get_module_for_descriptor_internal') as mock_get_module:
             mock_get_module.return_value = mock_instance
             self._run_task_with_mock_celery(rescore_problem, task_entry.id, task_entry.task_id)
@@ -340,6 +350,7 @@ class TestRescoreInstructorTask(TestInstructorTasks):
         task_entry = self._create_input_entry()
         mock_instance = Mock()
         mock_instance.rescore_problem = Mock(return_value={'success': 'bogus'})
+        del mock_instance.rescore
         with patch('lms.djangoapps.instructor_task.tasks_helper.get_module_for_descriptor_internal') as mock_get_module:
             mock_get_module.return_value = mock_instance
             self._run_task_with_mock_celery(rescore_problem, task_entry.id, task_entry.task_id)
@@ -364,6 +375,7 @@ class TestRescoreInstructorTask(TestInstructorTasks):
         task_entry = self._create_input_entry()
         mock_instance = Mock()
         mock_instance.rescore_problem = Mock(return_value={'bogus': 'value'})
+        del mock_instance.rescore
         with patch('lms.djangoapps.instructor_task.tasks_helper.get_module_for_descriptor_internal') as mock_get_module:
             mock_get_module.return_value = mock_instance
             self._run_task_with_mock_celery(rescore_problem, task_entry.id, task_entry.task_id)

lms/djangoapps/instructor_task/tests/test_tasks_helper.py

@@ -13,44 +13,43 @@ import shutil
 from datetime import datetime
 import urllib
 
+from django.conf import settings
+from django.core.urlresolvers import reverse
+from django.test.utils import override_settings
 import ddt
 from freezegun import freeze_time
 from mock import Mock, patch, MagicMock
 from nose.plugins.attrib import attr
+from pytz import UTC
 import tempfile
 import unicodecsv
-from django.core.urlresolvers import reverse
-from django.test.utils import override_settings
 
 from capa.tests.response_xml_factory import MultipleChoiceResponseXMLFactory
 from certificates.models import CertificateStatuses, GeneratedCertificate
 from certificates.tests.factories import GeneratedCertificateFactory, CertificateWhitelistFactory
 from course_modes.models import CourseMode
 from courseware.tests.factories import InstructorFactory
-from lms.djangoapps.instructor_task.tests.test_base import (
-    InstructorTaskCourseTestCase,
-    TestReportMixin,
-    InstructorTaskModuleTestCase
-)
+from instructor_analytics.basic import UNAVAILABLE
+from lms.djangoapps.teams.tests.factories import CourseTeamFactory, CourseTeamMembershipFactory
+from lms.djangoapps.verify_student.tests.factories import SoftwareSecurePhotoVerificationFactory
 from openedx.core.djangoapps.course_groups.models import CourseUserGroupPartitionGroup, CohortMembership
-from django.conf import settings
-from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase
-from pytz import UTC
-
-from student.tests.factories import CourseEnrollmentFactory, UserFactory
 from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory
 import openedx.core.djangoapps.user_api.course_tag.api as course_tag_api
 from openedx.core.djangoapps.user_api.partition_schemes import RandomUserPartitionScheme
-from shoppingcart.models import Order, PaidCourseRegistration, CourseRegistrationCode, Invoice, \
+from openedx.core.djangoapps.util.testing import ContentGroupTestCase, TestConditionalContent
+from shoppingcart.models import (
+    Order, PaidCourseRegistration, CourseRegistrationCode, Invoice,
     CourseRegistrationCodeInvoiceItem, InvoiceTransaction, Coupon
-from student.tests.factories import UserFactory, CourseModeFactory
+)
 from student.models import CourseEnrollment, CourseEnrollmentAllowed, ManualEnrollmentAudit, ALLOWEDTOENROLL_TO_ENROLLED
-from lms.djangoapps.verify_student.tests.factories import SoftwareSecurePhotoVerificationFactory
+from student.tests.factories import CourseEnrollmentFactory, CourseModeFactory, UserFactory
+from survey.models import SurveyForm, SurveyAnswer
+from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
 from xmodule.partitions.partitions import Group, UserPartition
-from lms.djangoapps.instructor_task.models import ReportStore
-from survey.models import SurveyForm, SurveyAnswer
-from lms.djangoapps.instructor_task.tasks_helper import (
+
+from ..models import ReportStore
+from ..tasks_helper import (
     cohort_students_and_upload,
     upload_problem_responses_csv,
     upload_grades_csv,
@@ -65,9 +64,12 @@ from lms.djangoapps.instructor_task.tasks_helper import (
     UPDATE_STATUS_FAILED,
     UPDATE_STATUS_SUCCEEDED,
 )
-from instructor_analytics.basic import UNAVAILABLE
-from openedx.core.djangoapps.util.testing import ContentGroupTestCase, TestConditionalContent
-from teams.tests.factories import CourseTeamFactory, CourseTeamMembershipFactory
+
+from lms.djangoapps.instructor_task.tests.test_base import (
+    InstructorTaskCourseTestCase,
+    TestReportMixin,
+    InstructorTaskModuleTestCase
+)
 
 
 class InstructorGradeReportTestCase(TestReportMixin, InstructorTaskCourseTestCase):

requirements/edx/base.txt

@@ -209,7 +209,7 @@ py2neo==3.1.2
 
 # Support for plugins
 web-fragments==0.2.1
-xblock==0.4.14
+xblock==0.5.0
 
 # Third Party XBlocks
 edx-sga==0.6.2