From fd1ed2ce6d0f122650eec6528681e231adb9e568 Mon Sep 17 00:00:00 2001 From: Eric Fischer Date: Fri, 17 Feb 2017 09:46:00 -0500 Subject: [PATCH 1/2] 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 --- lms/djangoapps/grades/models.py | 23 +------------------- lms/djangoapps/grades/tests/test_models.py | 25 ++++++---------------- lms/djangoapps/grades/tests/test_new.py | 2 +- lms/djangoapps/grades/tests/test_tasks.py | 24 ++++++++++++--------- 4 files changed, 22 insertions(+), 52 deletions(-) diff --git a/lms/djangoapps/grades/models.py b/lms/djangoapps/grades/models.py index ed6c66426b..0532a89521 100644 --- a/lms/djangoapps/grades/models.py +++ b/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) diff --git a/lms/djangoapps/grades/tests/test_models.py b/lms/djangoapps/grades/tests/test_models.py index d8805a06f2..f7b495e125 100644 --- a/lms/djangoapps/grades/tests/test_models.py +++ b/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() diff --git a/lms/djangoapps/grades/tests/test_new.py b/lms/djangoapps/grades/tests/test_new.py index fcdfaccaa5..ce5c0c5a81 100644 --- a/lms/djangoapps/grades/tests/test_new.py +++ b/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) diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py index a2330ff2e7..6a3d6a69b0 100644 --- a/lms/djangoapps/grades/tests/test_tasks.py +++ b/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 From 7d82f32f5bdb8e6862e144b49b2dc97585cb89a1 Mon Sep 17 00:00:00 2001 From: Eric Fischer Date: Fri, 17 Feb 2017 09:47:17 -0500 Subject: [PATCH 2/2] Fix issue with test ordering The second test here is not idempotent, so we must needs reset the course before each test. --- lms/djangoapps/grades/tests/integration/test_events.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/grades/tests/integration/test_events.py b/lms/djangoapps/grades/tests/integration/test_events.py index c177e9a124..b6ce701130 100644 --- a/lms/djangoapps/grades/tests/integration/test_events.py +++ b/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