Merge pull request #14360 from edx/beryl/fix_db_updated_check

Update has_database_updated_with_new_score to handle all block types

lms/djangoapps/courseware/tests/test_module_render.py

@@ -1848,7 +1848,8 @@ class TestXmoduleRuntimeEvent(TestSubmittingProblems):
             'course_id': unicode(self.course.id),
             'usage_id': unicode(self.problem.location),
             'only_if_higher': None,
-            'modified': datetime.now().replace(tzinfo=pytz.UTC)
+            'modified': datetime.now().replace(tzinfo=pytz.UTC),
+            'score_db_table': 'csm',
         }
         send_mock.assert_called_with(**expected_signal_kwargs)
 

lms/djangoapps/grades/constants.py

+"""
+Constants and Enums used by Grading.
+"""
+
+
+class ScoreDatabaseTableEnum(object):
+    """
+    The various database tables that store scores.
+    """
+    courseware_student_module = 'csm'
+    submissions = 'submissions'

lms/djangoapps/grades/signals/handlers.py

@@ -24,9 +24,10 @@ from .signals import (
     SUBSECTION_SCORE_CHANGED,
     SCORE_PUBLISHED,
 )
+from ..constants import ScoreDatabaseTableEnum
 from ..new.course_grade import CourseGradeFactory
 from ..scores import weighted_score
-from ..tasks import recalculate_subsection_grade_v2
+from ..tasks import recalculate_subsection_grade_v3
 
 log = getLogger(__name__)
 
@@ -62,9 +63,11 @@ def submissions_score_set_handler(sender, **kwargs):  # pylint: disable=unused-a
         weighted_earned=points_earned,
         weighted_possible=points_possible,
         user_id=user.id,
+        anonymous_user_id=kwargs['anonymous_user_id'],
         course_id=course_id,
         usage_id=usage_id,
         modified=kwargs['created_at'],
+        score_db_table=ScoreDatabaseTableEnum.submissions,
     )
 
 
@@ -93,9 +96,11 @@ def submissions_score_reset_handler(sender, **kwargs):  # pylint: disable=unused
         weighted_earned=0,
         weighted_possible=0,
         user_id=user.id,
+        anonymous_user_id=kwargs['anonymous_user_id'],
         course_id=course_id,
         usage_id=usage_id,
         modified=kwargs['created_at'],
+        score_db_table=ScoreDatabaseTableEnum.submissions,
     )
 
 
@@ -133,6 +138,7 @@ def score_published_handler(sender, block, user, raw_earned, raw_possible, only_
             usage_id=unicode(block.location),
             only_if_higher=only_if_higher,
             modified=score_modified_time,
+            score_db_table=ScoreDatabaseTableEnum.courseware_student_module,
         )
     return update_score
 
@@ -162,6 +168,7 @@ def problem_raw_score_changed_handler(sender, **kwargs):  # pylint: disable=unus
         only_if_higher=kwargs['only_if_higher'],
         score_deleted=kwargs.get('score_deleted', False),
         modified=kwargs['modified'],
+        score_db_table=kwargs['score_db_table'],
     )
 
 
@@ -172,9 +179,10 @@ def enqueue_subsection_update(sender, **kwargs):  # pylint: disable=unused-argum
     enqueueing a subsection update operation to occur asynchronously.
     """
     _emit_problem_submitted_event(kwargs)
-    result = recalculate_subsection_grade_v2.apply_async(
+    result = recalculate_subsection_grade_v3.apply_async(
         kwargs=dict(
             user_id=kwargs['user_id'],
+            anonymous_user_id=kwargs.get('anonymous_user_id'),
             course_id=kwargs['course_id'],
             usage_id=kwargs['usage_id'],
             only_if_higher=kwargs.get('only_if_higher'),
@@ -182,6 +190,7 @@ def enqueue_subsection_update(sender, **kwargs):  # pylint: disable=unused-argum
             score_deleted=kwargs.get('score_deleted', False),
             event_transaction_id=unicode(get_event_transaction_id()),
             event_transaction_type=unicode(get_event_transaction_type()),
+            score_db_table=kwargs['score_db_table'],
         )
     )
     log.info(

lms/djangoapps/grades/signals/signals.py

@@ -22,6 +22,7 @@ PROBLEM_RAW_SCORE_CHANGED = Signal(
                             # made only if the new score is higher than previous.
         'modified',  # A datetime indicating when the database representation of
                      # this the problem score was saved.
+        'score_db_table',  # The database table that houses the score that changed.
     ]
 )
 
@@ -35,6 +36,7 @@ PROBLEM_RAW_SCORE_CHANGED = Signal(
 PROBLEM_WEIGHTED_SCORE_CHANGED = Signal(
     providing_args=[
         'user_id',  # Integer User ID
+        'anonymous_user_id',  # Anonymous User ID
         'course_id',  # Unicode string representing the course
         'usage_id',  # Unicode string indicating the courseware instance
         'weighted_earned',   # Score obtained by the user
@@ -43,6 +45,7 @@ PROBLEM_WEIGHTED_SCORE_CHANGED = Signal(
                             # made only if the new score is higher than previous.
         'modified',  # A datetime indicating when the database representation of
                      # this the problem score was saved.
+        'score_db_table',  # The database table that houses the score that changed.
     ]
 )
 
@@ -59,6 +62,7 @@ SCORE_PUBLISHED = Signal(
         'raw_possible',  # Maximum score available for the exercise
         'only_if_higher',   # Boolean indicating whether updates should be
                             # made only if the new score is higher than previous.
+        'score_db_table',  # The database table that houses the score that changed.
     ]
 )
 

lms/djangoapps/grades/tasks.py

@@ -26,6 +26,7 @@ from util.date_utils import from_timestamp
 from xmodule.modulestore.django import modulestore
 
 from .config.models import PersistentGradesEnabledFlag
+from .constants import ScoreDatabaseTableEnum
 from .new.subsection_grade import SubsectionGradeFactory
 from .signals.signals import SUBSECTION_SCORE_CHANGED
 from .transformer import GradesTransformer
@@ -35,33 +36,31 @@ log = getLogger(__name__)
 KNOWN_RETRY_ERRORS = (DatabaseError, ValidationError)  # Errors we expect occasionally, should be resolved on retry
 
 
-@task(default_retry_delay=30, routing_key=settings.RECALCULATE_GRADES_ROUTING_KEY)
-def recalculate_subsection_grade(
-        # pylint: disable=unused-argument
-        user_id, course_id, usage_id, only_if_higher, weighted_earned, weighted_possible, **kwargs
-):
+# TODO (TNL-6373) DELETE ME once v3 is successfully deployed to Prod.
+@task(base=PersistOnFailureTask, default_retry_delay=30, routing_key=settings.RECALCULATE_GRADES_ROUTING_KEY)
+def recalculate_subsection_grade_v2(**kwargs):
     """
-    Shim to allow us to modify this task's signature without blowing up production on deployment.
+    Shim to support tasks enqueued by older workers during initial deployment.
     """
-    recalculate_subsection_grade_v2.apply(
-        kwargs=dict(
-            user_id=user_id,
-            course_id=course_id,
-            usage_id=usage_id,
-            only_if_higher=only_if_higher,
-            expected_modified_time=kwargs.get('expected_modified_time', 0),  # Use the unix epoch as a default
-            score_deleted=kwargs['score_deleted'],
-        )
-    )
+    _recalculate_subsection_grade(recalculate_subsection_grade_v2, **kwargs)
 
 
 @task(base=PersistOnFailureTask, default_retry_delay=30, routing_key=settings.RECALCULATE_GRADES_ROUTING_KEY)
-def recalculate_subsection_grade_v2(**kwargs):
+def recalculate_subsection_grade_v3(**kwargs):
+    """
+    Latest version of the recalculate_subsection_grade task.  See docstring
+    for _recalculate_subsection_grade for further description.
+    """
+    _recalculate_subsection_grade(recalculate_subsection_grade_v3, **kwargs)
+
+
+def _recalculate_subsection_grade(task_func, **kwargs):
     """
     Updates a saved subsection grade.
 
-    Arguments:
+    Keyword Arguments:
         user_id (int): id of applicable User object
+        anonymous_user_id (int, OPTIONAL): Anonymous ID of the User
         course_id (string): identifying the course
         usage_id (string): identifying the course block
         only_if_higher (boolean): indicating whether grades should
@@ -71,36 +70,44 @@ def recalculate_subsection_grade_v2(**kwargs):
             was queued so that we can verify the underlying data update.
         score_deleted (boolean): indicating whether the grade change is
             a result of the problem's score being deleted.
-        event_transaction_id(string): uuid identifying the current
+        event_transaction_id (string): uuid identifying the current
             event transaction.
-        event_transaction_type(string): human-readable type of the
+        event_transaction_type (string): human-readable type of the
             event at the root of the current event transaction.
+        score_db_table (ScoreDatabaseTableEnum): database table that houses
+            the changed score. Used in conjunction with expected_modified_time.
     """
     try:
         course_key = CourseLocator.from_string(kwargs['course_id'])
         if not PersistentGradesEnabledFlag.feature_enabled(course_key):
             return
 
-        score_deleted = kwargs['score_deleted']
         scored_block_usage_key = UsageKey.from_string(kwargs['usage_id']).replace(course_key=course_key)
-        expected_modified_time = from_timestamp(kwargs['expected_modified_time'])
 
         # The request cache is not maintained on celery workers,
         # where this code runs. So we take the values from the
         # main request cache and store them in the local request
         # cache. This correlates model-level grading events with
         # higher-level ones.
-        set_event_transaction_id(kwargs.pop('event_transaction_id', None))
-        set_event_transaction_type(kwargs.pop('event_transaction_type', None))
+        set_event_transaction_id(kwargs.get('event_transaction_id'))
+        set_event_transaction_type(kwargs.get('event_transaction_type'))
 
         # 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(
-                kwargs['user_id'], scored_block_usage_key, expected_modified_time, score_deleted,
-        ):
-            raise _retry_recalculate_subsection_grade(**kwargs)
+        if task_func == recalculate_subsection_grade_v2:
+            has_database_updated = _has_db_updated_with_new_score_bwc_v2(
+                kwargs['user_id'],
+                scored_block_usage_key,
+                from_timestamp(kwargs['expected_modified_time']),
+                kwargs['score_deleted'],
+            )
+        else:
+            has_database_updated = _has_db_updated_with_new_score(scored_block_usage_key, **kwargs)
+
+        if not has_database_updated:
+            raise _retry_recalculate_subsection_grade(task_func, **kwargs)
 
         _update_subsection_grades(
             course_key,
@@ -115,13 +122,45 @@ def recalculate_subsection_grade_v2(**kwargs):
                 repr(exc),
                 kwargs
             ))
-        raise _retry_recalculate_subsection_grade(exc=exc, **kwargs)
+        raise _retry_recalculate_subsection_grade(task_func, exc=exc, **kwargs)
 
 
-def _has_database_updated_with_new_score(
+def _has_db_updated_with_new_score(scored_block_usage_key, **kwargs):
+    """
+    Returns whether the database has been updated with the
+    expected new score values for the given problem and user.
+    """
+    if kwargs['score_db_table'] == ScoreDatabaseTableEnum.courseware_student_module:
+        score = get_score(kwargs['user_id'], scored_block_usage_key)
+        found_modified_time = score.modified if score is not None else None
+
+    else:
+        assert kwargs['score_db_table'] == ScoreDatabaseTableEnum.submissions
+        score = sub_api.get_score(
+            {
+                "student_id": kwargs['anonymous_user_id'],
+                "course_id": unicode(scored_block_usage_key.course_key),
+                "item_id": unicode(scored_block_usage_key),
+                "item_type": scored_block_usage_key.block_type,
+            }
+        )
+        found_modified_time = score['created_at'] if score is not None else None
+
+    if score is None:
+        # score should be None only if it was deleted.
+        # Otherwise, it hasn't yet been saved.
+        return kwargs['score_deleted']
+
+    return found_modified_time >= from_timestamp(kwargs['expected_modified_time'])
+
+
+# TODO (TNL-6373) DELETE ME once v3 is successfully deployed to Prod.
+def _has_db_updated_with_new_score_bwc_v2(
         user_id, scored_block_usage_key, expected_modified_time, score_deleted,
 ):
     """
+    DEPRECATED version for backward compatibility with v2 tasks.
+
     Returns whether the database has been updated with the
     expected new score values for the given problem and user.
     """
@@ -186,7 +225,7 @@ def _update_subsection_grades(
                     only_if_higher,
                 )
                 SUBSECTION_SCORE_CHANGED.send(
-                    sender=recalculate_subsection_grade,
+                    sender=None,
                     course=course,
                     course_structure=course_structure,
                     user=student,
@@ -194,29 +233,9 @@ def _update_subsection_grades(
                 )
 
 
-def _retry_recalculate_subsection_grade(
-        user_id,
-        course_id,
-        usage_id,
-        only_if_higher,
-        expected_modified_time,
-        score_deleted,
-        exc=None,
-):
+def _retry_recalculate_subsection_grade(task_func, exc=None, **kwargs):
     """
     Calls retry for the recalculate_subsection_grade task with the
     given inputs.
     """
-    recalculate_subsection_grade_v2.retry(
-        kwargs=dict(
-            user_id=user_id,
-            course_id=course_id,
-            usage_id=usage_id,
-            only_if_higher=only_if_higher,
-            expected_modified_time=expected_modified_time,
-            score_deleted=score_deleted,
-            event_transaction_id=unicode(get_event_transaction_id()),
-            event_transaction_type=unicode(get_event_transaction_type()),
-        ),
-        exc=exc,
-    )
+    task_func.retry(kwargs=kwargs, exc=exc)

lms/djangoapps/grades/tests/test_signals.py

@@ -11,6 +11,7 @@ from mock import patch, MagicMock
 import pytz
 from util.date_utils import to_timestamp
 
+from ..constants import ScoreDatabaseTableEnum
 from ..signals.handlers import (
     enqueue_subsection_update,
     submissions_score_set_handler,
@@ -32,7 +33,6 @@ SUBMISSION_SET_KWARGS = {
     'created_at': FROZEN_NOW_TIMESTAMP,
 }
 
-
 SUBMISSION_RESET_KWARGS = {
     'anonymous_user_id': 'anonymous_id',
     'course_id': 'CourseID',
@@ -50,6 +50,20 @@ PROBLEM_RAW_SCORE_CHANGED_KWARGS = {
     'only_if_higher': False,
     'score_deleted': True,
     'modified': FROZEN_NOW_TIMESTAMP,
+    'score_db_table': ScoreDatabaseTableEnum.courseware_student_module,
+}
+
+PROBLEM_WEIGHTED_SCORE_CHANGED_KWARGS = {
+    'sender': None,
+    'weighted_earned': 2.0,
+    'weighted_possible': 4.0,
+    'user_id': 'UserID',
+    'course_id': 'CourseID',
+    'usage_id': 'i4x://org/course/usage/123456',
+    'only_if_higher': False,
+    'score_deleted': True,
+    'modified': FROZEN_NOW_TIMESTAMP,
+    'score_db_table': ScoreDatabaseTableEnum.courseware_student_module,
 }
 
 
@@ -110,9 +124,11 @@ class ScoreChangedSignalRelayTest(TestCase):
             'weighted_possible': possible,
             'weighted_earned': earned,
             'user_id': self.user_mock.id,
+            'anonymous_user_id': 'anonymous_id',
             'course_id': 'CourseID',
             'usage_id': 'i4x://org/course/usage/123456',
             'modified': FROZEN_NOW_TIMESTAMP,
+            'score_db_table': 'submissions',
         }
         self.signal_mock.assert_called_once_with(**expected_set_kwargs)
         self.get_user_mock.assert_called_once_with(kwargs['anonymous_user_id'])
@@ -153,44 +169,26 @@ class ScoreChangedSignalRelayTest(TestCase):
 
     def test_raw_score_changed_signal_handler(self):
         problem_raw_score_changed_handler(None, **PROBLEM_RAW_SCORE_CHANGED_KWARGS)
-        expected_set_kwargs = {
-            'sender': None,
-            'weighted_earned': 2.0,
-            'weighted_possible': 4.0,
-            'user_id': 'UserID',
-            'course_id': 'CourseID',
-            'usage_id': 'i4x://org/course/usage/123456',
-            'only_if_higher': False,
-            'score_deleted': True,
-            'modified': FROZEN_NOW_TIMESTAMP
-        }
+        expected_set_kwargs = PROBLEM_WEIGHTED_SCORE_CHANGED_KWARGS.copy()
         self.signal_mock.assert_called_with(**expected_set_kwargs)
 
     def test_raw_score_changed_score_deleted_optional(self):
         local_kwargs = PROBLEM_RAW_SCORE_CHANGED_KWARGS.copy()
         del local_kwargs['score_deleted']
         problem_raw_score_changed_handler(None, **local_kwargs)
-        expected_set_kwargs = {
-            'sender': None,
-            'weighted_earned': 2.0,
-            'weighted_possible': 4.0,
-            'user_id': 'UserID',
-            'course_id': 'CourseID',
-            'usage_id': 'i4x://org/course/usage/123456',
-            'only_if_higher': False,
-            'score_deleted': False,
-            'modified': FROZEN_NOW_TIMESTAMP
-        }
+        expected_set_kwargs = PROBLEM_WEIGHTED_SCORE_CHANGED_KWARGS.copy()
+        expected_set_kwargs['score_deleted'] = False
         self.signal_mock.assert_called_with(**expected_set_kwargs)
 
     @patch('lms.djangoapps.grades.signals.handlers.log.info')
-    def test_problem_score_changed_logging(self, mocklog):
+    def test_subsection_update_logging(self, mocklog):
         enqueue_subsection_update(
             sender='test',
             user_id=1,
             course_id=u'course-v1:edX+Demo_Course+DemoX',
             usage_id=u'block-v1:block-key',
             modified=FROZEN_NOW_DATETIME,
+            score_db_table=ScoreDatabaseTableEnum.courseware_student_module,
         )
         log_statement = mocklog.call_args[0][0]
         log_statement = UUID_REGEX.sub(u'*UUID*', log_statement)
@@ -199,6 +197,7 @@ class ScoreChangedSignalRelayTest(TestCase):
             (
                 u'Grades: Request async calculation of subsection grades with args: '
                 u'course_id:course-v1:edX+Demo_Course+DemoX, modified:{time}, '
+                u'score_db_table:csm, '
                 u'usage_id:block-v1:block-key, user_id:1. Task [*UUID*]'
             ).format(time=FROZEN_NOW_DATETIME)
         )

lms/djangoapps/instructor/enrollment.py

@@ -16,10 +16,8 @@ from django.utils.translation import override as override_language
 from eventtracking import tracker
 import pytz
 
-from course_modes.models import CourseMode
-from courseware.models import StudentModule
-from edxmako.shortcuts import render_to_string
 from lms.djangoapps.grades.signals.signals import PROBLEM_RAW_SCORE_CHANGED
+from lms.djangoapps.grades.constants import ScoreDatabaseTableEnum
 from openedx.core.djangoapps.lang_pref import LANGUAGE_KEY
 from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers
 from openedx.core.djangoapps.user_api.models import UserPreference
@@ -329,19 +327,22 @@ def _fire_score_changed_for_block(
     The earned points are always zero. We must retrieve the possible points
     from the XModule, as noted below. The effective time is now().
     """
-    if block and block.has_score and block.max_score() is not None:
-        PROBLEM_RAW_SCORE_CHANGED.send(
-            sender=None,
-            raw_earned=0,
-            raw_possible=block.max_score(),
-            weight=getattr(block, 'weight', None),
-            user_id=student.id,
-            course_id=unicode(course_id),
-            usage_id=unicode(module_state_key),
-            score_deleted=True,
-            only_if_higher=False,
-            modified=datetime.now().replace(tzinfo=pytz.UTC),
-        )
+    if block and block.has_score:
+        max_score = block.max_score()
+        if max_score is not None:
+            PROBLEM_RAW_SCORE_CHANGED.send(
+                sender=None,
+                raw_earned=0,
+                raw_possible=max_score,
+                weight=getattr(block, 'weight', None),
+                user_id=student.id,
+                course_id=unicode(course_id),
+                usage_id=unicode(module_state_key),
+                score_deleted=True,
+                only_if_higher=False,
+                modified=datetime.now().replace(tzinfo=pytz.UTC),
+                score_db_table=ScoreDatabaseTableEnum.courseware_student_module,
+            )
 
 
 def get_email_params(course, auto_enroll, secure=True, course_key=None, display_name=None):