From 86c12e67cc1d067de645a5a6d1707432e18744bf Mon Sep 17 00:00:00 2001 From: "J. Cliff Dyer" Date: Tue, 22 Nov 2016 11:48:33 -0500 Subject: [PATCH] fixup: Error instead of correcting on invalid inputs. --- lms/djangoapps/grades/models.py | 39 ++++------------- lms/djangoapps/grades/tests/test_models.py | 49 ++++++++++++---------- lms/djangoapps/grades/tests/test_new.py | 2 +- lms/djangoapps/grades/tests/test_tasks.py | 6 +-- 4 files changed, 39 insertions(+), 57 deletions(-) diff --git a/lms/djangoapps/grades/models.py b/lms/djangoapps/grades/models.py index f1b9ec4e93..eabebf861f 100644 --- a/lms/djangoapps/grades/models.py +++ b/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): """ diff --git a/lms/djangoapps/grades/tests/test_models.py b/lms/djangoapps/grades/tests/test_models.py index aa1a4cc9a5..eede32568f 100644 --- a/lms/djangoapps/grades/tests/test_models.py +++ b/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_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_create_inconsistent_unattempted(self): self.params['attempted'] = False - grade = PersistentSubsectionGrade.create_grade(**self.params) - self.assertEqual(grade.earned_all, 0.0) + with self.assertRaises(ValidationError): + PersistentSubsectionGrade.create_grade(**self.params) - def test_update_inconsistent_unattempted(self): + def test_update_or_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) + 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): diff --git a/lms/djangoapps/grades/tests/test_new.py b/lms/djangoapps/grades/tests/test_new.py index 056dcdea4d..e5584059ab 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(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) diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py index 37743257b9..153ec574dc 100644 --- a/lms/djangoapps/grades/tests/test_tasks.py +++ b/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),