diff --git a/lms/djangoapps/grades/models.py b/lms/djangoapps/grades/models.py index fcb5a4a26f..0cb1baf5f9 100644 --- a/lms/djangoapps/grades/models.py +++ b/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 diff --git a/lms/djangoapps/grades/signals/handlers.py b/lms/djangoapps/grades/signals/handlers.py index bebb14456b..e78ee4a0d0 100644 --- a/lms/djangoapps/grades/signals/handlers.py +++ b/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'), ) ) diff --git a/lms/djangoapps/grades/tasks.py b/lms/djangoapps/grades/tasks.py index 4802ee9da2..ba02ac2d47 100644 --- a/lms/djangoapps/grades/tasks.py +++ b/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) diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py index 5e7877611d..60689b47a5 100644 --- a/lms/djangoapps/grades/tests/test_tasks.py +++ b/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()))