Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fixes from robust grades bug bash

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

For TNL-5861
This commit is contained in:
Sanford Student
2016-10-28 13:56:55 -04:00
committed by Sanford Student
parent 08c5a35899
commit 6e52fd791f
4 changed files with 173 additions and 52 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
lms/djangoapps/grades/models.py
View File

@@ -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

3
lms/djangoapps/grades/signals/handlers.py
View File

@@ -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'),
)
)

130
lms/djangoapps/grades/tasks.py
View File

@@ -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)

85
lms/djangoapps/grades/tests/test_tasks.py
View File

@@ -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()))