Merge pull request #13867 from edx/efischer/grades_bug_bash

Update course grade query parameters and check scores when updating subsection grade

lms/djangoapps/grades/models.py

@@ -423,18 +423,17 @@ class PersistentCourseGrade(TimeStampedModel):
         return cls.objects.get(user_id=user_id, course_id=course_id)
 
     @classmethod
-    def update_or_create_course_grade(cls, user_id, course_id, course_version=None, **kwargs):
+    def update_or_create_course_grade(cls, user_id, course_id, **kwargs):
         """
         Creates a course grade in the database.
         Returns a PersistedCourseGrade object.
         """
-        if course_version is None:
-            course_version = ""
+        if kwargs.get('course_version', None) is None:
+            kwargs['course_version'] = ""
 
         grade, _ = cls.objects.update_or_create(
             user_id=user_id,
             course_id=course_id,
-            course_version=course_version,
             defaults=kwargs
         )
         return grade

lms/djangoapps/grades/signals/handlers.py

@@ -106,7 +106,6 @@ def score_published_handler(sender, block, user, raw_earned, raw_possible, only_
 
     if update_score:
         set_score(user.id, block.location, raw_earned, raw_possible)
-
         PROBLEM_SCORE_CHANGED.send(
             sender=None,
             points_earned=raw_earned,
@@ -130,6 +129,8 @@ def enqueue_subsection_update(sender, **kwargs):  # pylint: disable=unused-argum
             kwargs['course_id'],
             kwargs['usage_id'],
             kwargs.get('only_if_higher'),
+            kwargs.get('points_earned'),
+            kwargs.get('points_possible'),
         )
     )
 

lms/djangoapps/grades/tasks.py

@@ -6,7 +6,9 @@ from celery import task
 from django.conf import settings
 from django.contrib.auth.models import User
 from django.db.utils import IntegrityError
+from logging import getLogger
 
+from courseware.model_data import get_score
 from lms.djangoapps.course_blocks.api import get_course_blocks
 from opaque_keys.edx.keys import UsageKey
 from opaque_keys.edx.locator import CourseLocator
@@ -19,28 +21,120 @@ from .new.subsection_grade import SubsectionGradeFactory
 from .signals.signals import SUBSECTION_SCORE_CHANGED
 from .transformer import GradesTransformer
 
+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):
+def recalculate_subsection_grade(user_id, course_id, usage_id, only_if_higher, raw_earned, raw_possible):
     """
     Updates a saved subsection grade.
     This method expects the following parameters:
-       - user_id: serialized id of applicable User object
-       - course_id: Unicode string representing the course
-       - usage_id: Unicode string indicating the courseware instance
-       - only_if_higher: boolean indicating whether grades should
-        be updated only if the new grade is higher than the previous
+        - 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
     """
     if not PersistentGradesEnabledFlag.feature_enabled(course_id):
         return
 
     course_key = CourseLocator.from_string(course_id)
-    student = User.objects.get(id=user_id)
     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:
+        _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):
+        _retry_recalculate_subsection_grade(user_id, course_id, usage_id, only_if_higher, raw_earned, raw_possible)
+
+    _update_subsection_grades(
+        course_key,
+        scored_block_usage_key,
+        only_if_higher,
+        course_id,
+        user_id,
+        usage_id,
+        raw_earned,
+        raw_possible,
+    )
+
+
+@task(default_retry_delay=30, routing_key=settings.RECALCULATE_GRADES_ROUTING_KEY)
+def recalculate_course_grade(user_id, course_id):
+    """
+    Updates a saved course grade.
+    This method expects the following parameters:
+       - user_id: serialized id of applicable User object
+       - course_id: Unicode string representing the course
+    """
+    if not PersistentGradesEnabledFlag.feature_enabled(course_id):
+        return
+    student = User.objects.get(id=user_id)
+    course_key = CourseLocator.from_string(course_id)
+    course = modulestore().get_course(course_key, depth=0)
+
+    try:
+        CourseGradeFactory(student).update(course)
+    except IntegrityError as exc:
+        raise recalculate_course_grade.retry(args=[user_id, course_id], exc=exc)
 
+
+def _retry_recalculate_subsection_grade(user_id, course_id, usage_id, only_if_higher, grade, max_grade, exc=None):
+    """
+    Calls retry for the recalculate_subsection_grade task with the
+    given inputs.
+    """
+    raise recalculate_subsection_grade.retry(
+        args=[
+            user_id,
+            course_id,
+            usage_id,
+            only_if_higher,
+            grade,
+            max_grade,
+        ],
+        exc=exc
+    )
+
+
+def _update_subsection_grades(
+        course_key,
+        scored_block_usage_key,
+        only_if_higher,
+        course_id,
+        user_id,
+        usage_id,
+        raw_earned,
+        raw_possible,
+):
+    """
+    A helper function to update subsection grades in the database
+    for each subsection containing the given block, and to signal
+    that those subsection grades were updated.
+    """
     collected_block_structure = get_course_in_cache(course_key)
     course = modulestore().get_course(course_key, depth=0)
+    student = User.objects.get(id=user_id)
     subsection_grade_factory = SubsectionGradeFactory(student, course, collected_block_structure)
     subsections_to_update = collected_block_structure.get_transformer_block_field(
         scored_block_usage_key,
@@ -69,24 +163,4 @@ def recalculate_subsection_grade(user_id, course_id, usage_id, only_if_higher):
             )
 
     except IntegrityError as exc:
-        raise recalculate_subsection_grade.retry(args=[user_id, course_id, usage_id], exc=exc)
-
-
-@task(default_retry_delay=30, routing_key=settings.RECALCULATE_GRADES_ROUTING_KEY)
-def recalculate_course_grade(user_id, course_id):
-    """
-    Updates a saved course grade.
-    This method expects the following parameters:
-       - user_id: serialized id of applicable User object
-       - course_id: Unicode string representing the course
-    """
-    if not PersistentGradesEnabledFlag.feature_enabled(course_id):
-        return
-    student = User.objects.get(id=user_id)
-    course_key = CourseLocator.from_string(course_id)
-    course = modulestore().get_course(course_key, depth=0)
-
-    try:
-        CourseGradeFactory(student).update(course)
-    except IntegrityError as exc:
-        raise recalculate_course_grade.retry(args=[user_id, course_id], exc=exc)
+        _retry_recalculate_subsection_grade(user_id, course_id, usage_id, only_if_higher, raw_earned, raw_possible, exc)

lms/djangoapps/grades/tests/test_tasks.py

@@ -3,6 +3,7 @@ Tests for the functionality and infrastructure of grades tasks.
 """
 
 from collections import OrderedDict
+from contextlib import contextmanager
 import ddt
 from django.conf import settings
 from django.db.utils import IntegrityError
@@ -51,17 +52,37 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
         self.sequential = ItemFactory.create(parent=self.chapter, category='sequential', display_name="Open Sequential")
         self.problem = ItemFactory.create(parent=self.sequential, category='problem', display_name='problem')
 
-        self.score_changed_kwargs = OrderedDict([
+        self.problem_score_changed_kwargs = OrderedDict([
+            ('points_earned', 1.0),
+            ('points_possible', 2.0),
             ('user_id', self.user.id),
             ('course_id', unicode(self.course.id)),
             ('usage_id', unicode(self.problem.location)),
             ('only_if_higher', None),
         ])
 
+        self.recalculate_subsection_grade_kwargs = OrderedDict([
+            ('user_id', self.user.id),
+            ('course_id', unicode(self.course.id)),
+            ('usage_id', unicode(self.problem.location)),
+            ('only_if_higher', None),
+            ('grade', 1.0),
+            ('max_grade', 2.0),
+        ])
+
         # this call caches the anonymous id on the user object, saving 4 queries in all happy path tests
         _ = anonymous_id_for_user(self.user, self.course.id)
         # pylint: enable=attribute-defined-outside-init,no-member
 
+    @contextmanager
+    def mock_get_score(self, score=(1.0, 2.0)):
+        """
+        Mocks the scores needed by the SCORE_PUBLISHED signal
+        handler. By default, sets the returned score to 1/2.
+        """
+        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),
         ('lms.djangoapps.grades.tasks.recalculate_course_grade.apply_async', SUBSECTION_SCORE_CHANGED)
@@ -73,12 +94,15 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
         """
         self.set_up_course()
         if test_signal == PROBLEM_SCORE_CHANGED:
-            send_args = self.score_changed_kwargs
-            expected_args = tuple(self.score_changed_kwargs.values())
+            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.score_changed_kwargs['user_id'], self.score_changed_kwargs['course_id'])
-        with patch(
+            expected_args = (
+                self.problem_score_changed_kwargs['user_id'],
+                self.problem_score_changed_kwargs['course_id']
+            )
+        with self.mock_get_score() and patch(
             enqueue_op,
             return_value=None
         ) as mock_task_apply:
@@ -91,19 +115,20 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
         Ensures that the subsection update operation also updates the course grade.
         """
         self.set_up_course()
-        mock_return = uuid4()
+        mock_update_return = uuid4()
+
         course_key = CourseLocator.from_string(unicode(self.course.id))
         course = modulestore().get_course(course_key, depth=0)
         with patch(
             'lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory.update',
-            return_value=mock_return
+            return_value=mock_update_return
         ):
-            recalculate_subsection_grade.apply(args=tuple(self.score_changed_kwargs.values()))
+            self._apply_recalculate_subsection_grade()
         mock_course_signal.assert_called_once_with(
             sender=recalculate_subsection_grade,
             course=course,
             user=self.user,
-            subsection_grade=mock_return,
+            subsection_grade=mock_update_return,
         )
 
     @ddt.data(True, False)
@@ -132,8 +157,8 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
                 other_task.assert_not_called()
                 executed_task.assert_called_once_with(
                     args=(
-                        self.score_changed_kwargs['user_id'],
-                        self.score_changed_kwargs['course_id'],
+                        self.problem_score_changed_kwargs['user_id'],
+                        self.problem_score_changed_kwargs['course_id'],
                     )
                 )
 
@@ -147,7 +172,7 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
             self.set_up_course()
             self.assertTrue(PersistentGradesEnabledFlag.feature_enabled(self.course.id))
             with check_mongo_calls(2) and self.assertNumQueries(25 + added_queries):
-                recalculate_subsection_grade.apply(args=tuple(self.score_changed_kwargs.values()))
+                self._apply_recalculate_subsection_grade()
 
     def test_single_call_to_create_block_structure(self):
         self.set_up_course()
@@ -156,7 +181,7 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
             'openedx.core.lib.block_structure.factory.BlockStructureFactory.create_from_cache',
             return_value=None,
         ) as mock_block_structure_create:
-            recalculate_subsection_grade.apply(args=tuple(self.score_changed_kwargs.values()))
+            self._apply_recalculate_subsection_grade()
             self.assertEquals(mock_block_structure_create.call_count, 2)
 
     @ddt.data(
@@ -171,7 +196,7 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
             ItemFactory.create(parent=self.sequential, category='problem', display_name='problem2')
             ItemFactory.create(parent=self.sequential, category='problem', display_name='problem3')
             with check_mongo_calls(2) and self.assertNumQueries(25 + added_queries):
-                recalculate_subsection_grade.apply(args=tuple(self.score_changed_kwargs.values()))
+                self._apply_recalculate_subsection_grade()
 
     @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
     def test_subsection_grades_not_enabled_on_course(self, default_store):
@@ -179,7 +204,7 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
             self.set_up_course(enable_subsection_grades=False)
             self.assertFalse(PersistentGradesEnabledFlag.feature_enabled(self.course.id))
             with check_mongo_calls(2) and self.assertNumQueries(0):
-                recalculate_subsection_grade.apply(args=tuple(self.score_changed_kwargs.values()))
+                self._apply_recalculate_subsection_grade()
 
     @skip("Pending completion of TNL-5089")
     @ddt.data(
@@ -194,7 +219,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.score_changed_kwargs.values()))
+                recalculate_subsection_grade.apply(args=tuple(self.recalculate_subsection_grade_kwargs.values()))
 
     @patch('lms.djangoapps.grades.tasks.recalculate_subsection_grade.retry')
     @patch('lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory.update')
@@ -204,7 +229,7 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
         """
         self.set_up_course()
         mock_update.side_effect = IntegrityError("WHAMMY")
-        recalculate_subsection_grade.apply(args=tuple(self.score_changed_kwargs.values()))
+        self._apply_recalculate_subsection_grade()
         self.assertTrue(mock_retry.called)
 
     @patch('lms.djangoapps.grades.tasks.recalculate_course_grade.retry')
@@ -217,8 +242,30 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
         mock_update.side_effect = IntegrityError("WHAMMY")
         recalculate_course_grade.apply(
             args=(
-                self.score_changed_kwargs['user_id'],
-                self.score_changed_kwargs['course_id'],
+                self.problem_score_changed_kwargs['user_id'],
+                self.problem_score_changed_kwargs['course_id'],
             )
         )
         self.assertTrue(mock_retry.called)
+
+    @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)
+
+    @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)
+
+    def _apply_recalculate_subsection_grade(self):
+        """
+        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()))