Commit fd1ed2ce by Eric Fischer

Remove unneeded validation on grades model

These checks are causing SQL query numbers to scale linearly with the
number of subsections being created/updated, and the errors they
check for have not been seen in prod.

TNL-6225
parent be887833
...@@ -15,7 +15,6 @@ import json ...@@ -15,7 +15,6 @@ import json
from lazy import lazy from lazy import lazy
import logging import logging
from django.core.exceptions import ValidationError
from django.db import models from django.db import models
from django.utils.timezone import now from django.utils.timezone import now
from eventtracking import tracker from eventtracking import tracker
...@@ -295,21 +294,6 @@ class PersistentSubsectionGrade(DeleteGradesMixin, TimeStampedModel): ...@@ -295,21 +294,6 @@ class PersistentSubsectionGrade(DeleteGradesMixin, TimeStampedModel):
# track which blocks were visible at the time of grade calculation # track which blocks were visible at the time of grade calculation
visible_blocks = models.ForeignKey(VisibleBlocks, db_column='visible_blocks_hash', to_field='hashed') 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 @property
def full_usage_key(self): def full_usage_key(self):
""" """
...@@ -391,7 +375,6 @@ class PersistentSubsectionGrade(DeleteGradesMixin, TimeStampedModel): ...@@ -391,7 +375,6 @@ class PersistentSubsectionGrade(DeleteGradesMixin, TimeStampedModel):
if attempted and not grade.first_attempted: if attempted and not grade.first_attempted:
grade.first_attempted = now() grade.first_attempted = now()
grade.save() grade.save()
grade.full_clean()
cls._emit_grade_calculated_event(grade) cls._emit_grade_calculated_event(grade)
return grade return grade
...@@ -402,9 +385,7 @@ class PersistentSubsectionGrade(DeleteGradesMixin, TimeStampedModel): ...@@ -402,9 +385,7 @@ class PersistentSubsectionGrade(DeleteGradesMixin, TimeStampedModel):
""" """
cls._prepare_params_and_visible_blocks(params) cls._prepare_params_and_visible_blocks(params)
cls._prepare_attempted_for_create(params, now()) cls._prepare_attempted_for_create(params, now())
grade = cls(**params) grade = cls.objects.create(**params)
grade.full_clean()
grade.save()
cls._emit_grade_calculated_event(grade) cls._emit_grade_calculated_event(grade)
return grade return grade
...@@ -423,8 +404,6 @@ class PersistentSubsectionGrade(DeleteGradesMixin, TimeStampedModel): ...@@ -423,8 +404,6 @@ class PersistentSubsectionGrade(DeleteGradesMixin, TimeStampedModel):
for params in grade_params_iter: for params in grade_params_iter:
cls._prepare_attempted_for_create(params, first_attempt_timestamp) cls._prepare_attempted_for_create(params, first_attempt_timestamp)
grades = [PersistentSubsectionGrade(**params) for params in grade_params_iter] grades = [PersistentSubsectionGrade(**params) for params in grade_params_iter]
for grade in grades:
grade.full_clean()
grades = cls.objects.bulk_create(grades) grades = cls.objects.bulk_create(grades)
for grade in grades: for grade in grades:
cls._emit_grade_calculated_event(grade) cls._emit_grade_calculated_event(grade)
......
...@@ -9,7 +9,6 @@ from hashlib import sha1 ...@@ -9,7 +9,6 @@ from hashlib import sha1
import json import json
from mock import patch from mock import patch
from django.core.exceptions import ValidationError
from django.db.utils import IntegrityError from django.db.utils import IntegrityError
from django.test import TestCase from django.test import TestCase
from django.utils.timezone import now from django.utils.timezone import now
...@@ -228,7 +227,7 @@ class PersistentSubsectionGradeTest(GradesModelTestCase): ...@@ -228,7 +227,7 @@ class PersistentSubsectionGradeTest(GradesModelTestCase):
) )
self.assertEqual(created_grade, read_grade) self.assertEqual(created_grade, read_grade)
self.assertEqual(read_grade.visible_blocks.blocks, self.block_records) self.assertEqual(read_grade.visible_blocks.blocks, self.block_records)
with self.assertRaises(ValidationError): with self.assertRaises(IntegrityError):
PersistentSubsectionGrade.create_grade(**self.params) PersistentSubsectionGrade.create_grade(**self.params)
@ddt.data('course_version', 'subtree_edited_timestamp') @ddt.data('course_version', 'subtree_edited_timestamp')
...@@ -237,12 +236,12 @@ class PersistentSubsectionGradeTest(GradesModelTestCase): ...@@ -237,12 +236,12 @@ class PersistentSubsectionGradeTest(GradesModelTestCase):
PersistentSubsectionGrade.create_grade(**self.params) PersistentSubsectionGrade.create_grade(**self.params)
@ddt.data( @ddt.data(
("user_id", ValidationError), ("user_id", IntegrityError),
("usage_key", KeyError), ("usage_key", KeyError),
("earned_all", ValidationError), ("earned_all", IntegrityError),
("possible_all", ValidationError), ("possible_all", IntegrityError),
("earned_graded", ValidationError), ("earned_graded", IntegrityError),
("possible_graded", ValidationError), ("possible_graded", IntegrityError),
("visible_blocks", KeyError), ("visible_blocks", KeyError),
("attempted", KeyError), ("attempted", KeyError),
) )
...@@ -276,18 +275,6 @@ class PersistentSubsectionGradeTest(GradesModelTestCase): ...@@ -276,18 +275,6 @@ class PersistentSubsectionGradeTest(GradesModelTestCase):
self.assertEqual(grade.earned_all, 0.0) self.assertEqual(grade.earned_all, 0.0)
self.assertEqual(grade.earned_graded, 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): def test_first_attempted_not_changed_on_update(self):
PersistentSubsectionGrade.create_grade(**self.params) PersistentSubsectionGrade.create_grade(**self.params)
moment = now() moment = now()
......
...@@ -214,7 +214,7 @@ class TestSubsectionGradeFactory(ProblemSubmissionTestMixin, GradeTestBase): ...@@ -214,7 +214,7 @@ class TestSubsectionGradeFactory(ProblemSubmissionTestMixin, GradeTestBase):
'lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory._get_bulk_cached_grade', 'lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory._get_bulk_cached_grade',
wraps=self.subsection_grade_factory._get_bulk_cached_grade wraps=self.subsection_grade_factory._get_bulk_cached_grade
) as mock_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) grade_a = self.subsection_grade_factory.create(self.sequence)
self.assertTrue(mock_get_bulk_cached_grade.called) self.assertTrue(mock_get_bulk_cached_grade.called)
self.assertTrue(mock_create_grade.called) self.assertTrue(mock_create_grade.called)
......
...@@ -42,7 +42,7 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase): ...@@ -42,7 +42,7 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
self.user = UserFactory() self.user = UserFactory()
PersistentGradesEnabledFlag.objects.create(enabled_for_all_courses=True, enabled=True) 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. Configures the course for this test.
""" """
...@@ -59,6 +59,10 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase): ...@@ -59,6 +59,10 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
self.sequential = ItemFactory.create(parent=self.chapter, category='sequential', display_name="Sequential1") self.sequential = ItemFactory.create(parent=self.chapter, category='sequential', display_name="Sequential1")
self.problem = ItemFactory.create(parent=self.sequential, category='problem', display_name='Problem') 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_datetime = datetime.now().replace(tzinfo=pytz.UTC)
self.frozen_now_timestamp = to_timestamp(self.frozen_now_datetime) self.frozen_now_timestamp = to_timestamp(self.frozen_now_datetime)
...@@ -137,28 +141,28 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase): ...@@ -137,28 +141,28 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
self.assertEquals(mock_block_structure_create.call_count, 1) self.assertEquals(mock_block_structure_create.call_count, 1)
@ddt.data( @ddt.data(
(ModuleStoreEnum.Type.mongo, 1, 23), (ModuleStoreEnum.Type.mongo, 1, 24, True),
(ModuleStoreEnum.Type.split, 3, 22), (ModuleStoreEnum.Type.mongo, 1, 21, False),
(ModuleStoreEnum.Type.split, 3, 23, True),
(ModuleStoreEnum.Type.split, 3, 20, False),
) )
@ddt.unpack @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): 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)) self.assertTrue(PersistentGradesEnabledFlag.feature_enabled(self.course.id))
with check_mongo_calls(num_mongo_calls): with check_mongo_calls(num_mongo_calls):
with self.assertNumQueries(num_sql_calls): with self.assertNumQueries(num_sql_calls):
self._apply_recalculate_subsection_grade() 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( @ddt.data(
(ModuleStoreEnum.Type.mongo, 1, 46), (ModuleStoreEnum.Type.mongo, 1, 24),
(ModuleStoreEnum.Type.split, 3, 45), (ModuleStoreEnum.Type.split, 3, 23),
) )
@ddt.unpack @ddt.unpack
def test_query_counts_dont_change_with_more_content(self, default_store, num_mongo_calls, num_sql_calls): 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): 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)) self.assertTrue(PersistentGradesEnabledFlag.feature_enabled(self.course.id))
num_problems = 10 num_problems = 10
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment