Merge pull request #14529 from edx/efischer/unclean_grades

Unclean Grades

lms/djangoapps/grades/models.py

@@ -15,7 +15,6 @@ 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 eventtracking import tracker
@@ -295,21 +294,6 @@ class PersistentSubsectionGrade(DeleteGradesMixin, TimeStampedModel):
     # track which blocks were visible at the time of grade calculation
     visible_blocks = models.ForeignKey(VisibleBlocks, db_column='visible_blocks_hash', to_field='hashed')
 
-    def _is_unattempted_with_score(self):
-        """
-        Return True if the object has a non-zero score, but has not been
-        attempted.  This is an inconsistent state, and needs to be cleaned up.
-        """
-        return self.first_attempted is None and any(field != 0.0 for field in (self.earned_all, self.earned_graded))
-
-    def clean(self):
-        """
-        If an grade has not been attempted, but was given a non-zero score,
-        raise a ValidationError.
-        """
-        if self._is_unattempted_with_score():
-            raise ValidationError("Unattempted problems cannot have a non-zero score.")
-
     @property
     def full_usage_key(self):
         """
@@ -391,7 +375,6 @@ class PersistentSubsectionGrade(DeleteGradesMixin, TimeStampedModel):
         if attempted and not grade.first_attempted:
             grade.first_attempted = now()
             grade.save()
-        grade.full_clean()
         cls._emit_grade_calculated_event(grade)
         return grade
 
@@ -402,9 +385,7 @@ class PersistentSubsectionGrade(DeleteGradesMixin, TimeStampedModel):
         """
         cls._prepare_params_and_visible_blocks(params)
         cls._prepare_attempted_for_create(params, now())
-        grade = cls(**params)
-        grade.full_clean()
-        grade.save()
+        grade = cls.objects.create(**params)
         cls._emit_grade_calculated_event(grade)
         return grade
 
@@ -423,8 +404,6 @@ class PersistentSubsectionGrade(DeleteGradesMixin, TimeStampedModel):
         for params in grade_params_iter:
             cls._prepare_attempted_for_create(params, first_attempt_timestamp)
         grades = [PersistentSubsectionGrade(**params) for params in grade_params_iter]
-        for grade in grades:
-            grade.full_clean()
         grades = cls.objects.bulk_create(grades)
         for grade in grades:
             cls._emit_grade_calculated_event(grade)

lms/djangoapps/grades/tests/integration/test_events.py

@@ -8,7 +8,6 @@ from lms.djangoapps.instructor.enrollment import reset_student_attempts
 from lms.djangoapps.instructor_task.api import submit_rescore_problem_for_student
 from mock import patch
 from openedx.core.djangolib.testing.utils import get_mock_request
-from xmodule.modulestore.django import modulestore
 from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
 
@@ -28,8 +27,10 @@ class GradesEventIntegrationTest(ProblemSubmissionTestMixin, SharedModuleStoreTe
     of the grading infrastructure.
     """
     @classmethod
-    def setUpClass(cls):
-        super(GradesEventIntegrationTest, cls).setUpClass()
+    def reset_course(cls):
+        """
+        Sets up the course anew.
+        """
         with cls.store.default_store(ModuleStoreEnum.Type.split):
             cls.course = CourseFactory.create()
             cls.chapter = ItemFactory.create(
@@ -63,6 +64,7 @@ class GradesEventIntegrationTest(ProblemSubmissionTestMixin, SharedModuleStoreTe
             )
 
     def setUp(self):
+        self.reset_course()
         super(GradesEventIntegrationTest, self).setUp()
         self.request = get_mock_request(UserFactory())
         self.student = self.request.user

lms/djangoapps/grades/tests/test_models.py

@@ -9,7 +9,6 @@ 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 django.utils.timezone import now
@@ -228,7 +227,7 @@ class PersistentSubsectionGradeTest(GradesModelTestCase):
             )
             self.assertEqual(created_grade, read_grade)
             self.assertEqual(read_grade.visible_blocks.blocks, self.block_records)
-        with self.assertRaises(ValidationError):
+        with self.assertRaises(IntegrityError):
             PersistentSubsectionGrade.create_grade(**self.params)
 
     @ddt.data('course_version', 'subtree_edited_timestamp')
@@ -237,12 +236,12 @@ class PersistentSubsectionGradeTest(GradesModelTestCase):
         PersistentSubsectionGrade.create_grade(**self.params)
 
     @ddt.data(
-        ("user_id", ValidationError),
+        ("user_id", IntegrityError),
         ("usage_key", KeyError),
-        ("earned_all", ValidationError),
-        ("possible_all", ValidationError),
-        ("earned_graded", ValidationError),
-        ("possible_graded", ValidationError),
+        ("earned_all", IntegrityError),
+        ("possible_all", IntegrityError),
+        ("earned_graded", IntegrityError),
+        ("possible_graded", IntegrityError),
         ("visible_blocks", KeyError),
         ("attempted", KeyError),
     )
@@ -276,18 +275,6 @@ class PersistentSubsectionGradeTest(GradesModelTestCase):
         self.assertEqual(grade.earned_all, 0.0)
         self.assertEqual(grade.earned_graded, 0.0)
 
-    def test_create_inconsistent_unattempted(self):
-        self.params['attempted'] = False
-        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)
         moment = now()

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(14):
+                with self.assertNumQueries(12):
                     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

@@ -42,7 +42,7 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
         self.user = UserFactory()
         PersistentGradesEnabledFlag.objects.create(enabled_for_all_courses=True, enabled=True)
 
-    def set_up_course(self, enable_persistent_grades=True):
+    def set_up_course(self, enable_persistent_grades=True, create_multiple_subsections=False):
         """
         Configures the course for this test.
         """
@@ -59,6 +59,10 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
         self.sequential = ItemFactory.create(parent=self.chapter, category='sequential', display_name="Sequential1")
         self.problem = ItemFactory.create(parent=self.sequential, category='problem', display_name='Problem')
 
+        if create_multiple_subsections:
+            seq2 = ItemFactory.create(parent=self.chapter, category='sequential')
+            ItemFactory.create(parent=seq2, category='problem')
+
         self.frozen_now_datetime = datetime.now().replace(tzinfo=pytz.UTC)
         self.frozen_now_timestamp = to_timestamp(self.frozen_now_datetime)
 
@@ -137,28 +141,28 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
             self.assertEquals(mock_block_structure_create.call_count, 1)
 
     @ddt.data(
-        (ModuleStoreEnum.Type.mongo, 1, 23),
-        (ModuleStoreEnum.Type.split, 3, 22),
+        (ModuleStoreEnum.Type.mongo, 1, 24, True),
+        (ModuleStoreEnum.Type.mongo, 1, 21, False),
+        (ModuleStoreEnum.Type.split, 3, 23, True),
+        (ModuleStoreEnum.Type.split, 3, 20, False),
     )
     @ddt.unpack
-    def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls):
+    def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls, create_multiple_subsections):
         with self.store.default_store(default_store):
-            self.set_up_course()
+            self.set_up_course(create_multiple_subsections=create_multiple_subsections)
             self.assertTrue(PersistentGradesEnabledFlag.feature_enabled(self.course.id))
             with check_mongo_calls(num_mongo_calls):
                 with self.assertNumQueries(num_sql_calls):
                     self._apply_recalculate_subsection_grade()
 
-    # TODO (TNL-6225) Fix the number of SQL queries so they
-    # don't grow linearly with the number of sequentials.
     @ddt.data(
-        (ModuleStoreEnum.Type.mongo, 1, 46),
-        (ModuleStoreEnum.Type.split, 3, 45),
+        (ModuleStoreEnum.Type.mongo, 1, 24),
+        (ModuleStoreEnum.Type.split, 3, 23),
     )
     @ddt.unpack
     def test_query_counts_dont_change_with_more_content(self, default_store, num_mongo_calls, num_sql_calls):
         with self.store.default_store(default_store):
-            self.set_up_course()
+            self.set_up_course(create_multiple_subsections=True)
             self.assertTrue(PersistentGradesEnabledFlag.feature_enabled(self.course.id))
 
             num_problems = 10