fixup: Error instead of correcting on invalid inputs.

lms/djangoapps/grades/models.py

@@ -15,6 +15,7 @@ import json
 from lazy import lazy
 import logging
 
+from django.core.exceptions import ValidationError
 from django.db import models
 from django.utils.timezone import now
 from model_utils.models import TimeStampedModel
@@ -251,27 +252,13 @@ class PersistentSubsectionGrade(TimeStampedModel):
         """
         return self.first_attempted is None and any(field != 0.0 for field in (self.earned_all, self.earned_graded))
 
-    def enforce_unattempted(self, save=True):
+    def clean(self):
         """
         If an grade has not been attempted, but was given a non-zero score,
-        reset the score to 0.0.
-
-        Params:
-
-        save (bool, default: True):
-            By default, this method saves the model if and only if there was an
-            inconsistency.  If the caller needs to save the model regardless of
-            the result, or will be saving the model later after making other
-            changes, this may be an unwanted database request.  It can be
-            disabled by passing ``save=False``.
-
-        Return value: None
+        raise a ValidationError.
         """
         if self._is_unattempted_with_score():
-            self.earned_all = 0.0
-            self.earned_graded = 0.0
-            if save:
-                self.save()
+            raise ValidationError("Unattempted problems cannot have a non-zero score.")
 
     @property
     def full_usage_key(self):
@@ -343,7 +330,6 @@ class PersistentSubsectionGrade(TimeStampedModel):
         user_id = kwargs.pop('user_id')
         usage_key = kwargs.pop('usage_key')
         attempted = kwargs.pop('attempted')
-
         grade, _ = cls.objects.update_or_create(
             user_id=user_id,
             course_id=usage_key.course_key,
@@ -352,9 +338,8 @@ class PersistentSubsectionGrade(TimeStampedModel):
         )
         if attempted and not grade.first_attempted:
             grade.first_attempted = now()
-            grade.save()
-        else:
-            grade.enforce_unattempted()
+        grade.full_clean()
+        grade.save()
         return grade
 
     @classmethod
@@ -369,7 +354,7 @@ class PersistentSubsectionGrade(TimeStampedModel):
         grade = cls(**kwargs)
         if attempted:
             grade.first_attempted = now()
-        grade.enforce_unattempted(save=False)
+        grade.full_clean()
         grade.save()
         return grade
 
@@ -389,6 +374,8 @@ class PersistentSubsectionGrade(TimeStampedModel):
         for params in grade_params_iter:
             if params.pop('attempted'):
                 params['first_attempted'] = first_attempt_timestamp
+            elif params['earned_all'] != 0.0 or params['earned_graded'] != 0.0:
+                raise ValidationError("Unattempted problems cannot have a non-zero score.")
         return cls.objects.bulk_create([PersistentSubsectionGrade(**params) for params in grade_params_iter])
 
     @classmethod
@@ -423,14 +410,6 @@ class PersistentSubsectionGrade(TimeStampedModel):
         params['visible_blocks_id'] = params['visible_blocks'].hash_value
         del params['visible_blocks']
 
-    def remove_attempts(self):
-        """
-        Explicitly mark a subsection as unattempted
-        """
-        self.first_attempted = None
-        self.enforce_unattempted(save=False)
-        self.save()
-
 
 class PersistentCourseGrade(TimeStampedModel):
     """

lms/djangoapps/grades/tests/test_models.py

@@ -8,6 +8,7 @@ import ddt
 from hashlib import sha1
 import json
 
+from django.core.exceptions import ValidationError
 from django.db.utils import IntegrityError
 from django.test import TestCase
 from django.utils.timezone import now
@@ -225,7 +226,7 @@ class PersistentSubsectionGradeTest(GradesModelTestCase):
             )
             self.assertEqual(created_grade, read_grade)
             self.assertEqual(read_grade.visible_blocks.blocks, self.block_records)
-        with self.assertRaises(IntegrityError):
+        with self.assertRaises(ValidationError):
             PersistentSubsectionGrade.create_grade(**self.params)
 
     def test_optional_fields(self):
@@ -233,13 +234,13 @@ class PersistentSubsectionGradeTest(GradesModelTestCase):
         PersistentSubsectionGrade.create_grade(**self.params)
 
     @ddt.data(
-        ("user_id", IntegrityError),
+        ("user_id", ValidationError),
         ("usage_key", KeyError),
-        ("subtree_edited_timestamp", IntegrityError),
-        ("earned_all", IntegrityError),
-        ("possible_all", IntegrityError),
-        ("earned_graded", IntegrityError),
-        ("possible_graded", IntegrityError),
+        ("subtree_edited_timestamp", ValidationError),
+        ("earned_all", ValidationError),
+        ("possible_all", ValidationError),
+        ("earned_graded", ValidationError),
+        ("possible_graded", ValidationError),
         ("visible_blocks", KeyError),
         ("attempted", KeyError),
     )
@@ -260,20 +261,30 @@ class PersistentSubsectionGradeTest(GradesModelTestCase):
             self.assertEqual(created_grade.id, updated_grade.id)
             self.assertEqual(created_grade.earned_all, 6)
 
-    def test_update_or_create_with_implicit_attempted(self):
+    def test_update_or_create_attempted(self):
         grade = PersistentSubsectionGrade.update_or_create_grade(**self.params)
         self.assertIsInstance(grade.first_attempted, datetime)
 
-    def test_create_inconsistent_unattempted(self):
+    def test_unattempted(self):
         self.params['attempted'] = False
+        self.params['earned_all'] = 0.0
+        self.params['earned_graded'] = 0.0
         grade = PersistentSubsectionGrade.create_grade(**self.params)
+        self.assertIsNone(grade.first_attempted)
         self.assertEqual(grade.earned_all, 0.0)
+        self.assertEqual(grade.earned_graded, 0.0)
 
-    def test_update_inconsistent_unattempted(self):
+    def test_create_inconsistent_unattempted(self):
         self.params['attempted'] = False
-        PersistentSubsectionGrade.create_grade(**self.params)
-        grade = PersistentSubsectionGrade.update_or_create_grade(**self.params)
-        self.assertEqual(grade.earned_all, 0.0)
+        with self.assertRaises(ValidationError):
+            PersistentSubsectionGrade.create_grade(**self.params)
+
+    def test_update_or_create_inconsistent_unattempted(self):
+        self.params['attempted'] = False
+        self.params['earned_all'] = 1.0
+        self.params['earned_graded'] = 1.0
+        with self.assertRaises(ValidationError):
+            PersistentSubsectionGrade.update_or_create_grade(**self.params)
 
     def test_first_attempted_not_changed_on_update(self):
         PersistentSubsectionGrade.create_grade(**self.params)
@@ -283,19 +294,11 @@ class PersistentSubsectionGradeTest(GradesModelTestCase):
 
     def test_unattempted_save_does_not_remove_attempt(self):
         PersistentSubsectionGrade.create_grade(**self.params)
-        self.params['unattempted'] = False
+        self.params['attempted'] = False
         grade = PersistentSubsectionGrade.update_or_create_grade(**self.params)
         self.assertIsInstance(grade.first_attempted, datetime)
         self.assertEqual(grade.earned_all, 6.0)
 
-    def test_explicitly_remove_attempts(self):
-        grade = PersistentSubsectionGrade.create_grade(**self.params)
-        self.assertIsInstance(grade.first_attempted, datetime)
-        self.assertEqual(grade.earned_all, 6.0)
-        grade.remove_attempts()
-        self.assertIsNone(grade.first_attempted)
-        self.assertEqual(grade.earned_all, 0.0)
-
 
 @ddt.ddt
 class PersistentCourseGradesTest(GradesModelTestCase):

lms/djangoapps/grades/tests/test_new.py

@@ -214,7 +214,7 @@ class TestSubsectionGradeFactory(ProblemSubmissionTestMixin, GradeTestBase):
                 'lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory._get_bulk_cached_grade',
                 wraps=self.subsection_grade_factory._get_bulk_cached_grade
             ) as mock_get_bulk_cached_grade:
-                with self.assertNumQueries(12):
+                with self.assertNumQueries(14):
                     grade_a = self.subsection_grade_factory.create(self.sequence)
                 self.assertTrue(mock_get_bulk_cached_grade.called)
                 self.assertTrue(mock_create_grade.called)

lms/djangoapps/grades/tests/test_tasks.py

@@ -113,7 +113,7 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
         with self.store.default_store(default_store):
             self.set_up_course()
             self.assertTrue(PersistentGradesEnabledFlag.feature_enabled(self.course.id))
-            with check_mongo_calls(2) and self.assertNumQueries(20 + added_queries):
+            with check_mongo_calls(2) and self.assertNumQueries(23 + added_queries):
                 self._apply_recalculate_subsection_grade()
 
     @patch('lms.djangoapps.grades.signals.signals.SUBSECTION_SCORE_CHANGED.send')
@@ -161,7 +161,7 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
             self.assertTrue(PersistentGradesEnabledFlag.feature_enabled(self.course.id))
             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(20 + added_queries):
+            with check_mongo_calls(2) and self.assertNumQueries(23 + added_queries):
                 self._apply_recalculate_subsection_grade()
 
     @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
@@ -172,7 +172,7 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
             with check_mongo_calls(2) and self.assertNumQueries(0):
                 self._apply_recalculate_subsection_grade()
 
-    @skip("Pending completion of TNL-5089")
+    #@skip("Pending completion of TNL-5089")
     @ddt.data(
         (ModuleStoreEnum.Type.mongo, True),
         (ModuleStoreEnum.Type.split, True),