From b6305a1553a958265f21cfb34abfc3bf43ae7be0 Mon Sep 17 00:00:00 2001 From: "J. Cliff Dyer" Date: Fri, 18 Nov 2016 12:23:02 -0500 Subject: [PATCH] Use first_attempted value from database TNL-5930 --- lms/djangoapps/grades/models.py | 59 ++++++++++++++++++- lms/djangoapps/grades/new/subsection_grade.py | 10 +++- lms/djangoapps/grades/tests/test_models.py | 52 ++++++++++++---- 3 files changed, 105 insertions(+), 16 deletions(-) diff --git a/lms/djangoapps/grades/models.py b/lms/djangoapps/grades/models.py index 186ad3930d..f1b9ec4e93 100644 --- a/lms/djangoapps/grades/models.py +++ b/lms/djangoapps/grades/models.py @@ -244,6 +244,35 @@ class PersistentSubsectionGrade(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 enforce_unattempted(self, save=True): + """ + 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 + """ + if self._is_unattempted_with_score(): + self.earned_all = 0.0 + self.earned_graded = 0.0 + if save: + self.save() + @property def full_usage_key(self): """ @@ -313,6 +342,7 @@ 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, @@ -320,6 +350,11 @@ class PersistentSubsectionGrade(TimeStampedModel): usage_key=usage_key, defaults=kwargs, ) + if attempted and not grade.first_attempted: + grade.first_attempted = now() + grade.save() + else: + grade.enforce_unattempted() return grade @classmethod @@ -328,7 +363,15 @@ class PersistentSubsectionGrade(TimeStampedModel): Wrapper for objects.create. """ cls._prepare_params_and_visible_blocks(kwargs) - return cls.objects.create(**kwargs) + + attempted = kwargs.pop('attempted') + + grade = cls(**kwargs) + if attempted: + grade.first_attempted = now() + grade.enforce_unattempted(save=False) + grade.save() + return grade @classmethod def bulk_create_grades(cls, grade_params_iter, course_key): @@ -342,6 +385,10 @@ class PersistentSubsectionGrade(TimeStampedModel): VisibleBlocks.bulk_get_or_create([params['visible_blocks'] for params in grade_params_iter], course_key) map(cls._prepare_params_visible_blocks_id, grade_params_iter) + first_attempt_timestamp = now() + for params in grade_params_iter: + if params.pop('attempted'): + params['first_attempted'] = first_attempt_timestamp return cls.objects.bulk_create([PersistentSubsectionGrade(**params) for params in grade_params_iter]) @classmethod @@ -376,6 +423,14 @@ 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): """ @@ -422,7 +477,7 @@ class PersistentCourseGrade(TimeStampedModel): u"grading policy: {}".format(self.grading_policy_hash), u"percent grade: {}%".format(self.percent_grade), u"letter grade: {}".format(self.letter_grade), - u"passed_timestamp: {}".format(self.passed_timestamp), + u"passed timestamp: {}".format(self.passed_timestamp), ]) @classmethod diff --git a/lms/djangoapps/grades/new/subsection_grade.py b/lms/djangoapps/grades/new/subsection_grade.py index fda6113819..9e4b601aa1 100644 --- a/lms/djangoapps/grades/new/subsection_grade.py +++ b/lms/djangoapps/grades/new/subsection_grade.py @@ -51,6 +51,11 @@ class SubsectionGrade(object): Returns whether any problem in this subsection was attempted by the student. """ + + assert self.all_total is not None, ( + "SubsectionGrade not fully populated yet. Call init_from_structure or init_from_model " + "before use." + ) return self.all_total.attempted def init_from_structure(self, student, course_structure, submissions_scores, csm_scores): @@ -80,7 +85,7 @@ class SubsectionGrade(object): graded=True, display_name=self.display_name, module_id=self.location, - attempted=True, # TODO TNL-5930 + attempted=model.first_attempted is not None, ) self.all_total = AggregatedScore( tw_earned=model.earned_all, @@ -88,7 +93,7 @@ class SubsectionGrade(object): graded=False, display_name=self.display_name, module_id=self.location, - attempted=True, # TODO TNL-5930 + attempted=model.first_attempted is not None, ) self._log_event(log.debug, u"init_from_model", student) return self @@ -156,6 +161,7 @@ class SubsectionGrade(object): earned_graded=self.graded_total.earned, possible_graded=self.graded_total.possible, visible_blocks=self._get_visible_blocks, + attempted=self.attempted ) @property diff --git a/lms/djangoapps/grades/tests/test_models.py b/lms/djangoapps/grades/tests/test_models.py index c31f54a4fe..aa1a4cc9a5 100644 --- a/lms/djangoapps/grades/tests/test_models.py +++ b/lms/djangoapps/grades/tests/test_models.py @@ -210,7 +210,7 @@ class PersistentSubsectionGradeTest(GradesModelTestCase): "earned_graded": 6.0, "possible_graded": 8.0, "visible_blocks": self.block_records, - "first_attempted": "2016-08-01 18:53:24.354741", + "attempted": True, } def test_create(self): @@ -228,17 +228,8 @@ class PersistentSubsectionGradeTest(GradesModelTestCase): with self.assertRaises(IntegrityError): PersistentSubsectionGrade.create_grade(**self.params) - def test_create_bad_params(self): - """ - Confirms create will fail if params are missing. - """ - del self.params["earned_graded"] - with self.assertRaises(IntegrityError): - PersistentSubsectionGrade.create_grade(**self.params) - - @ddt.data("course_version", "first_attempted") - def test_optional_fields(self, field): - del self.params[field] + def test_optional_fields(self): + del self.params["course_version"] PersistentSubsectionGrade.create_grade(**self.params) @ddt.data( @@ -250,6 +241,7 @@ class PersistentSubsectionGradeTest(GradesModelTestCase): ("earned_graded", IntegrityError), ("possible_graded", IntegrityError), ("visible_blocks", KeyError), + ("attempted", KeyError), ) @ddt.unpack def test_non_optional_fields(self, field, error): @@ -268,6 +260,42 @@ 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): + grade = PersistentSubsectionGrade.update_or_create_grade(**self.params) + self.assertIsInstance(grade.first_attempted, datetime) + + def test_create_inconsistent_unattempted(self): + self.params['attempted'] = False + grade = PersistentSubsectionGrade.create_grade(**self.params) + self.assertEqual(grade.earned_all, 0.0) + + def test_update_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) + + def test_first_attempted_not_changed_on_update(self): + PersistentSubsectionGrade.create_grade(**self.params) + moment = now() + grade = PersistentSubsectionGrade.update_or_create_grade(**self.params) + self.assertLess(grade.first_attempted, moment) + + def test_unattempted_save_does_not_remove_attempt(self): + PersistentSubsectionGrade.create_grade(**self.params) + self.params['unattempted'] = 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):