Merge pull request #13295 from edx/efischer/opt_grade

Grades cleanup

common/lib/xmodule/xmodule/graders.py

@@ -40,22 +40,11 @@ def aggregate_scores(scores, section_name="summary", location=None):
     total_correct = float_sum(score.earned for score in scores)
     total_possible = float_sum(score.possible for score in scores)
 
-    #regardless of whether or not it is graded
-    all_total = Score(
-        total_correct,
-        total_possible,
-        False,
-        section_name,
-        location,
-    )
+    #regardless of whether it is graded
+    all_total = Score(total_correct, total_possible, False, section_name, location)
+
     #selecting only graded things
-    graded_total = Score(
-        total_correct_graded,
-        total_possible_graded,
-        True,
-        section_name,
-        location,
-    )
+    graded_total = Score(total_correct_graded, total_possible_graded, True, section_name, location)
 
     return all_total, graded_total
 

lms/djangoapps/grades/models.py

@@ -12,7 +12,7 @@ import json
 import logging
 from operator import attrgetter
 
-from django.db import models
+from django.db import models, transaction
 from django.db.utils import IntegrityError
 from model_utils.models import TimeStampedModel
 
@@ -107,6 +107,7 @@ class VisibleBlocksQuerySet(models.QuerySet):
             blocks = BlockRecordSet(blocks)
 
         try:
+            with transaction.atomic():
                 model = self.create_from_blockrecords(blocks)
         except IntegrityError:
             # If an integrity error occurs, the VisibleBlocks model we want to
@@ -172,13 +173,10 @@ class PersistentSubsectionGradeQuerySet(models.QuerySet):
             kwargs['course_id'] = kwargs['usage_key'].course_key
 
         visible_blocks_hash = VisibleBlocks.objects.hash_from_blockrecords(blocks=visible_blocks)
-        grade = self.model(
+        return super(PersistentSubsectionGradeQuerySet, self).create(
             visible_blocks_id=visible_blocks_hash,
             **kwargs
         )
-        grade.full_clean()
-        grade.save()
-        return grade
 
 
 class PersistentSubsectionGrade(TimeStampedModel):
@@ -244,6 +242,7 @@ class PersistentSubsectionGrade(TimeStampedModel):
         user_id = kwargs.pop('user_id')
         usage_key = kwargs.pop('usage_key')
         try:
+            with transaction.atomic():
                 grade, is_created = cls.objects.get_or_create(
                     user_id=user_id,
                     course_id=usage_key.course_key,

lms/djangoapps/grades/new/subsection_grade.py

@@ -44,11 +44,15 @@ class SubsectionGrade(object):
         """
         Compute the grade of this subsection for the given student and course.
         """
+        try:
             for descendant_key in course_structure.post_order_traversal(
                     filter_func=possibly_scored,
                     start_node=self.location,
             ):
                 self._compute_block_score(student, descendant_key, course_structure, scores_client, submissions_scores)
+        finally:
+            # self.scores may hold outdated data, force it to refresh on next access
+            lazy.invalidate(self, 'scores')
 
         self.all_total, self.graded_total = graders.aggregate_scores(self.scores, self.display_name, self.location)
 

lms/djangoapps/grades/tests/test_models.py

@@ -8,7 +8,7 @@ from hashlib import sha1
 import json
 from mock import patch
 
-from django.core.exceptions import ValidationError
+from django.db.utils import IntegrityError
 from django.test import TestCase
 from opaque_keys.edx.locator import CourseLocator, BlockUsageLocator
 
@@ -156,7 +156,7 @@ class PersistentSubsectionGradeTest(GradesModelTestCase):
             usage_key=self.params["usage_key"],
         )
         self.assertEqual(created_grade, read_grade)
-        with self.assertRaises(ValidationError):
+        with self.assertRaises(IntegrityError):
             created_grade = PersistentSubsectionGrade.objects.create(**self.params)
 
     def test_create_bad_params(self):
@@ -164,7 +164,7 @@ class PersistentSubsectionGradeTest(GradesModelTestCase):
         Confirms create will fail if params are missing.
         """
         del self.params["earned_graded"]
-        with self.assertRaises(ValidationError):
+        with self.assertRaises(IntegrityError):
             PersistentSubsectionGrade.objects.create(**self.params)
 
     def test_course_version_is_optional(self):

lms/djangoapps/grades/tests/test_new.py

@@ -17,6 +17,7 @@ from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
 from ..models import PersistentSubsectionGrade
 from ..new.course_grade import CourseGradeFactory
 from ..new.subsection_grade import SubsectionGrade, SubsectionGradeFactory
+from lms.djangoapps.grades.tests.utils import mock_get_score
 
 
 class GradeTestBase(SharedModuleStoreTestCase):
@@ -128,7 +129,7 @@ class SubsectionGradeFactoryTest(GradeTestBase):
                 'lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory._get_saved_grade',
                 wraps=self.subsection_grade_factory._get_saved_grade  # pylint: disable=protected-access
             ) as mock_get_saved_grade:
-                with self.assertNumQueries(17):
+                with self.assertNumQueries(19):
                     grade_a = self.subsection_grade_factory.create(self.sequence, self.course_structure, self.course)
                 self.assertTrue(mock_get_saved_grade.called)
                 self.assertTrue(mock_save_grades.called)
@@ -180,11 +181,11 @@ class SubsectionGradeTest(GradeTestBase):
         Assuming the underlying score reporting methods work, test that the score is calculated properly.
         """
         grade = self.subsection_grade_factory.create(self.sequence, self.course_structure, self.course)
-        with patch('lms.djangoapps.grades.new.subsection_grade.get_score', return_value=(0, 1)):
+        with mock_get_score(1, 2):
             # The final 2 parameters are only passed through to our mocked-out get_score method
             grade.compute(self.request.user, self.course_structure, None, None)
-        self.assertEqual(grade.all_total.earned, 0)
-        self.assertEqual(grade.all_total.possible, 1)
+        self.assertEqual(grade.all_total.earned, 1)
+        self.assertEqual(grade.all_total.possible, 2)
 
     def test_save_and_load(self):
         """

lms/djangoapps/grades/tests/test_signals.py

@@ -203,7 +203,7 @@ class ScoreChangedUpdatesSubsectionGradeTest(ModuleStoreTestCase):
     def test_subsection_grade_updated_on_signal(self, default_store):
         with self.store.default_store(default_store):
             self.set_up_course()
-            with check_mongo_calls(2) and self.assertNumQueries(13):
+            with check_mongo_calls(2) and self.assertNumQueries(15):
                 recalculate_subsection_grade_handler(None, **self.score_changed_kwargs)
 
     @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
@@ -212,7 +212,7 @@ class ScoreChangedUpdatesSubsectionGradeTest(ModuleStoreTestCase):
             self.set_up_course()
             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(13):
+            with check_mongo_calls(2) and self.assertNumQueries(15):
                 recalculate_subsection_grade_handler(None, **self.score_changed_kwargs)
 
     @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
@@ -238,8 +238,8 @@ class ScoreChangedUpdatesSubsectionGradeTest(ModuleStoreTestCase):
                     SCORE_CHANGED.send(sender=None, **self.score_changed_kwargs)
 
     @ddt.data(
-        ('points_possible', 2, 13),
-        ('points_earned', 2, 13),
+        ('points_possible', 2, 15),
+        ('points_earned', 2, 15),
         ('user', 0, 0),
         ('course_id', 0, 0),
         ('usage_id', 0, 0),

lms/djangoapps/grades/tests/utils.py

@@ -13,3 +13,13 @@ def mock_passing_grade(grade_pass='Pass', percent=0.75):
     with patch('lms.djangoapps.grades.course_grades.summary') as mock_grade:
         mock_grade.return_value = {'grade': grade_pass, 'percent': percent}
         yield
+
+
+@contextmanager
+def mock_get_score(earned=0, possible=1):
+    """
+    Mocks the get_score function to return a valid grade.
+    """
+    with patch('lms.djangoapps.grades.new.subsection_grade.get_score') as mock_score:
+        mock_score.return_value = (earned, possible)
+        yield mock_score