diff --git a/lms/djangoapps/grades/signals/handlers.py b/lms/djangoapps/grades/signals/handlers.py index fada098f77..b84651c71a 100644 --- a/lms/djangoapps/grades/signals/handlers.py +++ b/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), ) ) diff --git a/lms/djangoapps/grades/tasks.py b/lms/djangoapps/grades/tasks.py index ef5be52971..870069096d 100644 --- a/lms/djangoapps/grades/tasks.py +++ b/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, ) diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py index 45dca070d1..4767acefdd 100644 --- a/lms/djangoapps/grades/tests/test_tasks.py +++ b/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)) diff --git a/lms/djangoapps/instructor/enrollment.py b/lms/djangoapps/instructor/enrollment.py index 6e7a62f6e9..fc0428d729 100644 --- a/lms/djangoapps/instructor/enrollment.py +++ b/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, )