Fix Delete-Student-State CSM entry deletion

lms/djangoapps/grades/signals/handlers.py

@@ -124,13 +124,14 @@ def enqueue_subsection_update(sender, **kwargs):  # pylint: disable=unused-argum
     Handles the PROBLEM_SCORE_CHANGED signal by enqueueing a subsection update operation to occur asynchronously.
     """
     recalculate_subsection_grade.apply_async(
-        args=(
-            kwargs['user_id'],
-            kwargs['course_id'],
-            kwargs['usage_id'],
-            kwargs.get('only_if_higher'),
-            kwargs.get('points_earned'),
-            kwargs.get('points_possible'),
+        kwargs=dict(
+            user_id=kwargs['user_id'],
+            course_id=kwargs['course_id'],
+            usage_id=kwargs['usage_id'],
+            only_if_higher=kwargs.get('only_if_higher'),
+            raw_earned=kwargs.get('points_earned'),
+            raw_possible=kwargs.get('points_possible'),
+            score_deleted=kwargs.get('score_deleted', False),
         )
     )
 

lms/djangoapps/grades/tasks.py

@@ -23,50 +23,40 @@ log = getLogger(__name__)
 
 
 @task(default_retry_delay=30, routing_key=settings.RECALCULATE_GRADES_ROUTING_KEY)
-def recalculate_subsection_grade(user_id, course_id, usage_id, only_if_higher, raw_earned, raw_possible):
+def recalculate_subsection_grade(user_id, course_id, usage_id, only_if_higher, raw_earned, raw_possible, **kwargs):
     """
     Updates a saved subsection grade.
-    This method expects the following parameters:
-        - user_id: serialized id of applicable User object
-        - course_id: Unicode string identifying the course
-        - usage_id: Unicode string identifying the course block
-        - only_if_higher: boolean indicating whether grades should
-        be updated only if the new raw_earned is higher than the previous
-        value.
-        - raw_earned: the raw points the learner earned on the problem that
-        triggered the update
-        - raw_possible: the max raw points the leaner could have earned
-        on the problem
+
+    Arguments:
+        user_id (int): id of applicable User object
+        course_id (string): identifying the course
+        usage_id (string): identifying the course block
+        only_if_higher (boolean): indicating whether grades should
+            be updated only if the new raw_earned is higher than the
+            previous value.
+        raw_earned (float): the raw points the learner earned on the
+            problem that triggered the update.
+        raw_possible (float): the max raw points the leaner could have
+            earned on the problem.
+        score_deleted (boolean): indicating whether the grade change is
+            a result of the problem's score being deleted.
     """
     course_key = CourseLocator.from_string(course_id)
     if not PersistentGradesEnabledFlag.feature_enabled(course_key):
         return
 
+    score_deleted = kwargs['score_deleted']
     scored_block_usage_key = UsageKey.from_string(usage_id).replace(course_key=course_key)
-    score = get_score(user_id, scored_block_usage_key)
 
-    # If the score is None, it has not been saved at all yet
-    # and we need to retry until it has been saved.
-    if score is None:
+    # Verify the database has been updated with the scores when the task was
+    # created. This race condition occurs if the transaction in the task
+    # creator's process hasn't committed before the task initiates in the worker
+    # process.
+    if not _has_database_updated_with_new_score(
+            user_id, scored_block_usage_key, raw_earned, raw_possible, score_deleted,
+    ):
         raise _retry_recalculate_subsection_grade(
-            user_id, course_id, usage_id, only_if_higher, raw_earned, raw_possible,
-        )
-    else:
-        module_raw_earned, module_raw_possible = score  # pylint: disable=unpacking-non-sequence
-
-    # Validate that the retrieved scores match the scores when the task was created.
-    # This race condition occurs if the transaction in the task creator's process hasn't
-    # committed before the task initiates in the worker process.
-    grades_match = module_raw_earned == raw_earned and module_raw_possible == raw_possible
-
-    # We have to account for the situation where a student's state
-    # has been deleted- in this case, get_score returns (None, None), but
-    # the score change signal will contain 0 for raw_earned.
-    state_deleted = module_raw_earned is None and module_raw_possible is None and raw_earned == 0
-
-    if not (state_deleted or grades_match):
-        raise _retry_recalculate_subsection_grade(
-            user_id, course_id, usage_id, only_if_higher, raw_earned, raw_possible,
+            user_id, course_id, usage_id, only_if_higher, raw_earned, raw_possible, score_deleted,
         )
 
     _update_subsection_grades(
@@ -78,6 +68,28 @@ def recalculate_subsection_grade(user_id, course_id, usage_id, only_if_higher, r
         usage_id,
         raw_earned,
         raw_possible,
+        score_deleted,
+    )
+
+
+def _has_database_updated_with_new_score(
+        user_id, scored_block_usage_key, expected_raw_earned, expected_raw_possible, score_deleted,
+):
+    """
+    Returns whether the database has been updated with the
+    expected new score values for the given problem and user.
+    """
+    score = get_score(user_id, scored_block_usage_key)
+
+    if score is None:
+        # score should be None only if it was deleted.
+        # Otherwise, it hasn't yet been saved.
+        return score_deleted
+
+    found_raw_earned, found_raw_possible = score  # pylint: disable=unpacking-non-sequence
+    return (
+        found_raw_earned == expected_raw_earned and
+        found_raw_possible == expected_raw_possible
     )
 
 
@@ -90,6 +102,7 @@ def _update_subsection_grades(
         usage_id,
         raw_earned,
         raw_possible,
+        score_deleted,
 ):
     """
     A helper function to update subsection grades in the database
@@ -124,23 +137,26 @@ def _update_subsection_grades(
 
     except IntegrityError as exc:
         raise _retry_recalculate_subsection_grade(
-            user_id, course_id, usage_id, only_if_higher, raw_earned, raw_possible, exc,
+            user_id, course_id, usage_id, only_if_higher, raw_earned, raw_possible, score_deleted, exc,
         )
 
 
-def _retry_recalculate_subsection_grade(user_id, course_id, usage_id, only_if_higher, grade, max_grade, exc=None):
+def _retry_recalculate_subsection_grade(
+        user_id, course_id, usage_id, only_if_higher, raw_earned, raw_possible, score_deleted, exc=None,
+):
     """
     Calls retry for the recalculate_subsection_grade task with the
     given inputs.
     """
     recalculate_subsection_grade.retry(
-        args=[
-            user_id,
-            course_id,
-            usage_id,
-            only_if_higher,
-            grade,
-            max_grade,
-        ],
-        exc=exc
+        kwargs=dict(
+            user_id=user_id,
+            course_id=course_id,
+            usage_id=usage_id,
+            only_if_higher=only_if_higher,
+            raw_earned=raw_earned,
+            raw_possible=raw_possible,
+            score_deleted=score_deleted,
+        ),
+        exc=exc,
     )

lms/djangoapps/grades/tests/test_tasks.py

@@ -8,19 +8,16 @@ import ddt
 from django.conf import settings
 from django.db.utils import IntegrityError
 from mock import patch
-from uuid import uuid4
 from unittest import skip
 
-from opaque_keys.edx.locator import CourseLocator
 from student.models import anonymous_id_for_user
 from student.tests.factories import UserFactory
-from xmodule.modulestore.django import modulestore
 from xmodule.modulestore import ModuleStoreEnum
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, check_mongo_calls
 
 from lms.djangoapps.grades.config.models import PersistentGradesEnabledFlag
-from lms.djangoapps.grades.signals.signals import PROBLEM_SCORE_CHANGED, SUBSECTION_SCORE_CHANGED
+from lms.djangoapps.grades.signals.signals import PROBLEM_SCORE_CHANGED
 from lms.djangoapps.grades.tasks import recalculate_subsection_grade
 
 
@@ -66,8 +63,9 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
             ('course_id', unicode(self.course.id)),
             ('usage_id', unicode(self.problem.location)),
             ('only_if_higher', None),
-            ('grade', 1.0),
-            ('max_grade', 2.0),
+            ('raw_earned', 1.0),
+            ('raw_possible', 2.0),
+            ('score_deleted', False),
         ])
 
         # this call caches the anonymous id on the user object, saving 4 queries in all happy path tests
@@ -83,30 +81,18 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
         with patch("lms.djangoapps.grades.tasks.get_score", return_value=score):
             yield
 
-    @ddt.data(
-        ('lms.djangoapps.grades.tasks.recalculate_subsection_grade.apply_async', PROBLEM_SCORE_CHANGED),
-    )
-    @ddt.unpack
-    def test_signal_queues_task(self, enqueue_op, test_signal):
+    def test_problem_score_changed_queues_task(self):
         """
-        Ensures that the PROBLEM_SCORE_CHANGED and SUBSECTION_SCORE_CHANGED signals enqueue the correct tasks.
+        Ensures that the PROBLEM_SCORE_CHANGED signal enqueues the correct task.
         """
         self.set_up_course()
-        if test_signal == PROBLEM_SCORE_CHANGED:
-            send_args = self.problem_score_changed_kwargs
-            expected_args = tuple(self.recalculate_subsection_grade_kwargs.values())
-        else:
-            send_args = {'user': self.user, 'course': self.course}
-            expected_args = (
-                self.problem_score_changed_kwargs['user_id'],
-                self.problem_score_changed_kwargs['course_id']
-            )
+        send_args = self.problem_score_changed_kwargs
         with self.mock_get_score() and patch(
-            enqueue_op,
+            'lms.djangoapps.grades.tasks.recalculate_subsection_grade.apply_async',
             return_value=None
         ) as mock_task_apply:
-            test_signal.send(sender=None, **send_args)
-            mock_task_apply.assert_called_once_with(args=expected_args)
+            PROBLEM_SCORE_CHANGED.send(sender=None, **send_args)
+            mock_task_apply.assert_called_once_with(kwargs=self.recalculate_subsection_grade_kwargs)
 
     @patch('lms.djangoapps.grades.signals.signals.SUBSECTION_SCORE_CHANGED.send')
     def test_subsection_update_triggers_course_update(self, mock_course_signal):
@@ -174,7 +160,7 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
         with self.store.default_store(default_store):
             self.set_up_course()
             with check_mongo_calls(0) and self.assertNumQueries(3 if feature_flag else 2):
-                recalculate_subsection_grade.apply(args=tuple(self.recalculate_subsection_grade_kwargs.values()))
+                recalculate_subsection_grade.apply(kwargs=self.recalculate_subsection_grade_kwargs)
 
     @patch('lms.djangoapps.grades.tasks.recalculate_subsection_grade.retry')
     @patch('lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory.update')
@@ -185,26 +171,43 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
         self.set_up_course()
         mock_update.side_effect = IntegrityError("WHAMMY")
         self._apply_recalculate_subsection_grade()
-        self.assertTrue(mock_retry.called)
+        self._assert_retry_called(mock_retry)
 
     @patch('lms.djangoapps.grades.tasks.recalculate_subsection_grade.retry')
     def test_retry_subsection_grade_on_update_not_complete(self, mock_retry):
         self.set_up_course()
-        with self.mock_get_score(score=(0.5, 3.0)):
-            recalculate_subsection_grade.apply(args=tuple(self.recalculate_subsection_grade_kwargs.values()))
-        self.assertTrue(mock_retry.called)
+        self._apply_recalculate_subsection_grade(mock_score=(0.5, 3.0))
+        self._assert_retry_called(mock_retry)
 
     @patch('lms.djangoapps.grades.tasks.recalculate_subsection_grade.retry')
     def test_retry_subsection_grade_on_no_score(self, mock_retry):
         self.set_up_course()
-        with self.mock_get_score(score=None):
-            recalculate_subsection_grade.apply(args=tuple(self.recalculate_subsection_grade_kwargs.values()))
-        self.assertTrue(mock_retry.called)
+        self._apply_recalculate_subsection_grade(mock_score=None)
+        self._assert_retry_called(mock_retry)
 
-    def _apply_recalculate_subsection_grade(self):
+    @patch('lms.djangoapps.grades.signals.signals.SUBSECTION_SCORE_CHANGED.send')
+    @patch('lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory.update')
+    def test_retry_first_time_only(self, mock_update, mock_course_signal):
+        """
+        Ensures that a task retry completes after a one-time failure.
+        """
+        self.set_up_course()
+        mock_update.side_effect = [IntegrityError("WHAMMY"), None]
+        self._apply_recalculate_subsection_grade()
+        self.assertEquals(mock_course_signal.call_count, 1)
+
+    def _apply_recalculate_subsection_grade(self, mock_score=(1.0, 2.0)):
         """
         Calls the recalculate_subsection_grade task with necessary
         mocking in place.
         """
-        with self.mock_get_score():
-            recalculate_subsection_grade.apply(args=tuple(self.recalculate_subsection_grade_kwargs.values()))
+        with self.mock_get_score(mock_score):
+            recalculate_subsection_grade.apply(kwargs=self.recalculate_subsection_grade_kwargs)
+
+    def _assert_retry_called(self, mock_retry):
+        """
+        Verifies the task was retried and with the correct
+        number of arguments.
+        """
+        self.assertTrue(mock_retry.called)
+        self.assertEquals(len(mock_retry.call_args[1]['kwargs']), len(self.recalculate_subsection_grade_kwargs))

lms/djangoapps/instructor/enrollment.py

@@ -4,7 +4,6 @@ Enrollment operations for use by instructor APIs.
 Does not include any access control, be sure to check access before calling.
 """
 
-import crum
 import json
 import logging
 from django.contrib.auth.models import User
@@ -15,8 +14,6 @@ from django.utils.translation import override as override_language
 
 from course_modes.models import CourseMode
 from courseware.models import StudentModule
-from courseware.model_data import FieldDataCache
-from courseware.module_render import get_module_for_descriptor
 from edxmako.shortcuts import render_to_string
 from lms.djangoapps.grades.scores import weighted_score
 from lms.djangoapps.grades.signals.signals import PROBLEM_SCORE_CHANGED
@@ -298,37 +295,22 @@ def _fire_score_changed_for_block(course_id, student, block, module_state_key):
     noted below.
     """
     if block and block.has_score:
-        cache = FieldDataCache.cache_for_descriptor_descendents(
-            course_id=course_id,
-            user=student,
-            descriptor=block,
-            depth=0
-        )
-        # For implementation reasons, we need to pull the max_score from the XModule,
-        # even though the data is not user-specific.  Here we bind the data to the
-        # current user.
-        request = crum.get_current_request()
-        module = get_module_for_descriptor(
-            user=student,
-            request=request,
-            descriptor=block,
-            field_data_cache=cache,
-            course_key=course_id
-        )
-        max_score = module.max_score()
+        max_score = block.max_score()
         if max_score is None:
             return
         else:
-            points_earned, points_possible = weighted_score(0, max_score, getattr(module, 'weight', None))
+            points_earned, points_possible = weighted_score(0, max_score, getattr(block, 'weight', None))
     else:
         points_earned, points_possible = 0, 0
+
     PROBLEM_SCORE_CHANGED.send(
         sender=None,
         points_possible=points_possible,
         points_earned=points_earned,
         user_id=student.id,
         course_id=unicode(course_id),
-        usage_id=unicode(module_state_key)
+        usage_id=unicode(module_state_key),
+        score_deleted=True,
     )