Merge pull request #14027 from edx/cdyer/mark-attempted-problems

Use first_attempted value from database and attempted values from individual problems.

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
@@ -244,6 +245,21 @@ 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 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):
         """
@@ -305,30 +321,39 @@ class PersistentSubsectionGrade(TimeStampedModel):
         )
 
     @classmethod
-    def update_or_create_grade(cls, **kwargs):
+    def update_or_create_grade(cls, **params):
         """
         Wrapper for objects.update_or_create.
         """
-        cls._prepare_params_and_visible_blocks(kwargs)
+        cls._prepare_params_and_visible_blocks(params)
 
-        user_id = kwargs.pop('user_id')
-        usage_key = kwargs.pop('usage_key')
+        user_id = params.pop('user_id')
+        usage_key = params.pop('usage_key')
+        attempted = params.pop('attempted')
 
         grade, _ = cls.objects.update_or_create(
             user_id=user_id,
             course_id=usage_key.course_key,
             usage_key=usage_key,
-            defaults=kwargs,
+            defaults=params,
         )
+        if attempted and not grade.first_attempted:
+            grade.first_attempted = now()
+            grade.save()
+        grade.full_clean()
         return grade
 
     @classmethod
-    def create_grade(cls, **kwargs):
+    def create_grade(cls, **params):
         """
         Wrapper for objects.create.
         """
-        cls._prepare_params_and_visible_blocks(kwargs)
-        return cls.objects.create(**kwargs)
+        cls._prepare_params_and_visible_blocks(params)
+        cls._prepare_attempted_for_create(params, now())
+        grade = cls(**params)
+        grade.full_clean()
+        grade.save()
+        return grade
 
     @classmethod
     def bulk_create_grades(cls, grade_params_iter, course_key):
@@ -341,8 +366,13 @@ class PersistentSubsectionGrade(TimeStampedModel):
         map(cls._prepare_params, grade_params_iter)
         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)
-
-        return cls.objects.bulk_create([PersistentSubsectionGrade(**params) for params in grade_params_iter])
+        first_attempt_timestamp = now()
+        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()
+        return cls.objects.bulk_create(grades)
 
     @classmethod
     def _prepare_params_and_visible_blocks(cls, params):
@@ -364,6 +394,15 @@ class PersistentSubsectionGrade(TimeStampedModel):
         params['visible_blocks'] = BlockRecordList.from_list(params['visible_blocks'], params['course_id'])
 
     @classmethod
+    def _prepare_attempted_for_create(cls, params, timestamp):
+        """
+        When creating objects, an attempted subsection gets its timestamp set
+        unconditionally.
+        """
+        if params.pop('attempted'):
+            params['first_attempted'] = timestamp
+
+    @classmethod
     def _prepare_params_visible_blocks_id(cls, params):
         """
         Prepares the visible_blocks_id field for the grade record,
@@ -422,7 +461,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

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

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
@@ -210,7 +211,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):
@@ -225,31 +226,23 @@ 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_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(
-        ("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),
     )
     @ddt.unpack
     def test_non_optional_fields(self, field, error):
@@ -268,6 +261,44 @@ class PersistentSubsectionGradeTest(GradesModelTestCase):
             self.assertEqual(created_grade.id, updated_grade.id)
             self.assertEqual(created_grade.earned_all, 6)
 
+    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
+        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()
+        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['attempted'] = False
+        grade = PersistentSubsectionGrade.update_or_create_grade(**self.params)
+        self.assertIsInstance(grade.first_attempted, datetime)
+        self.assertEqual(grade.earned_all, 6.0)
+
 
 @ddt.ddt
 class PersistentCourseGradesTest(GradesModelTestCase):

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)

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(22 + 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(22 + added_queries):
                 self._apply_recalculate_subsection_grade()
 
     @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)