Merge pull request #14866 from edx/neem/tests-for-grade-only-for-engaged

Missing grades tests and don't persist when creating grades unless policy changed

lms/djangoapps/grades/new/course_data.py

@@ -30,14 +30,14 @@ class CourseData(object):
             if self._course:
                 self._course_key = self._course.id
             else:
-                structure = self.effective_structure
+                structure = self._effective_structure
                 self._course_key = structure.root_block_usage_key.course_key
         return self._course_key
 
     @property
     def location(self):
         if not self._location:
-            structure = self.effective_structure
+            structure = self._effective_structure
             if structure:
                 self._location = structure.root_block_usage_key
             elif self._course:
@@ -64,7 +64,7 @@ class CourseData(object):
 
     @property
     def grading_policy_hash(self):
-        structure = self.effective_structure
+        structure = self._effective_structure
         if structure:
             return structure.get_transformer_block_field(
                 structure.root_block_usage_key,
@@ -76,7 +76,7 @@ class CourseData(object):
 
     @property
     def version(self):
-        structure = self.effective_structure
+        structure = self._effective_structure
         course_block = structure[self.location] if structure else self.course
         return getattr(course_block, 'course_version', None)
 
@@ -86,17 +86,17 @@ class CourseData(object):
         course_block = self.structure[self.location]
         return getattr(course_block, 'subtree_edited_on', None)
 
-    @property
-    def effective_structure(self):
-        return self._structure or self._collected_block_structure
-
     def __unicode__(self):
         return u'Course: course_key: {}'.format(self.course_key)
 
     def full_string(self):
-        if self.effective_structure:
+        if self._effective_structure:
             return u'Course: course_key: {}, version: {}, edited_on: {}, grading_policy: {}'.format(
                 self.course_key, self.version, self.edited_on, self.grading_policy_hash,
             )
         else:
             return u'Course: course_key: {}, empty course structure'.format(self.course_key)
+
+    @property
+    def _effective_structure(self):
+        return self._structure or self._collected_block_structure

lms/djangoapps/grades/new/course_grade.py

@@ -65,6 +65,18 @@ class CourseGradeBase(object):
         }
 
     @lazy
+    def subsection_grades(self):
+        """
+        Returns an ordered dictionary of subsection grades,
+        keyed by subsection location.
+        """
+        subsection_grades = defaultdict(OrderedDict)
+        for chapter in self.chapter_grades.itervalues():
+            for subsection_grade in chapter['sections']:
+                subsection_grades[subsection_grade.location] = subsection_grade
+        return subsection_grades
+
+    @lazy
     def locations_to_scores(self):
         """
         Returns a dict of problem scores keyed by their locations.

lms/djangoapps/grades/new/course_grade_factory.py

@@ -153,9 +153,9 @@ class CourseGradeFactory(object):
         course_grade.update()
 
         should_persist = (
-            not read_only and  # TODO(TNL-6786) Remove the read_only boolean once all grades are back-filled.
+            (not read_only) and  # TODO(TNL-6786) Remove the read_only boolean once all grades are back-filled.
             should_persist_grades(course_data.course_key) and
-            not waffle().is_enabled(WRITE_ONLY_IF_ENGAGED) or course_grade.attempted
+            (not waffle().is_enabled(WRITE_ONLY_IF_ENGAGED) or course_grade.attempted)
         )
         if should_persist:
             course_grade._subsection_grade_factory.bulk_create_unsaved()

lms/djangoapps/grades/tests/test_course_data.py

 """
 Tests for CourseData utility class.
 """
-from lms.djangoapps.course_blocks.api import get_course_blocks
 from mock import patch
+
+from lms.djangoapps.course_blocks.api import get_course_blocks
 from student.tests.factories import UserFactory
+from xmodule.modulestore import ModuleStoreEnum
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory
 from ..new.course_data import CourseData
@@ -16,7 +18,10 @@ class CourseDataTest(ModuleStoreTestCase):
 
     def setUp(self):
         super(CourseDataTest, self).setUp()
+        with self.store.default_store(ModuleStoreEnum.Type.split):
             self.course = CourseFactory.create()
+            # need to re-retrieve the course since the version on the original course isn't accurate.
+            self.course = self.store.get_course(self.course.id)
         self.user = UserFactory.create()
         self.one_true_structure = get_course_blocks(self.user, self.course.location)
         self.expected_results = {
@@ -45,11 +50,28 @@ class CourseDataTest(ModuleStoreTestCase):
                     actual = getattr(course_data, arg)
                     self.assertEqual(expected, actual)
 
-    def test_no_data(self):
-        """
-        Tests to ensure ??? happens when none of the data are provided.
+    def test_properties(self):
+        expected_edited_on = getattr(
+            self.one_true_structure[self.one_true_structure.root_block_usage_key],
+            'subtree_edited_on',
+        )
 
-        Maybe a dict pairing asked-for properties to resulting exceptions? Or an exception on init?
-        """
+        for kwargs in [
+            dict(course=self.course),
+            dict(collected_block_structure=self.one_true_structure),
+            dict(structure=self.one_true_structure),
+            dict(course_key=self.course.id),
+        ]:
+            course_data = CourseData(self.user, **kwargs)
+            self.assertEquals(course_data.course_key, self.course.id)
+            self.assertEquals(course_data.location, self.course.location)
+            self.assertEquals(course_data.structure.root_block_usage_key, self.one_true_structure.root_block_usage_key)
+            self.assertEquals(course_data.course.id, self.course.id)
+            self.assertEquals(course_data.version, self.course.course_version)
+            self.assertEquals(course_data.edited_on, expected_edited_on)
+            self.assertIn(u'Course: course_key', unicode(course_data))
+            self.assertIn(u'Course: course_key', course_data.full_string())
+
+    def test_no_data(self):
         with self.assertRaises(ValueError):
             _ = CourseData(self.user)

lms/djangoapps/grades/tests/test_new.py

@@ -25,12 +25,13 @@ from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
 from xmodule.modulestore.tests.utils import TEST_DATA_DIR
 from xmodule.modulestore.xml_importer import import_course_from_xml
 
-from ..config.waffle import waffle, ASSUME_ZERO_GRADE_IF_ABSENT
+from ..config.waffle import waffle, ASSUME_ZERO_GRADE_IF_ABSENT, WRITE_ONLY_IF_ENGAGED
 from ..models import PersistentSubsectionGrade
 from ..new.course_data import CourseData
 from ..new.course_grade_factory import CourseGradeFactory
 from ..new.course_grade import ZeroCourseGrade, CourseGrade
-from ..new.subsection_grade_factory import SubsectionGrade, SubsectionGradeFactory
+from ..new.subsection_grade_factory import SubsectionGradeFactory
+from ..new.subsection_grade import ZeroSubsectionGrade, SubsectionGrade
 from .utils import mock_get_score, mock_get_submissions_score
 
 
@@ -42,6 +43,7 @@ class GradeTestBase(SharedModuleStoreTestCase):
     def setUpClass(cls):
         super(GradeTestBase, cls).setUpClass()
         cls.course = CourseFactory.create()
+        with cls.store.bulk_operations(cls.course.id):
             cls.chapter = ItemFactory.create(
                 parent=cls.course,
                 category="chapter",
@@ -70,24 +72,34 @@ class GradeTestBase(SharedModuleStoreTestCase):
                 display_name="Test Problem",
                 data=problem_xml
             )
+            cls.sequence2 = ItemFactory.create(
+                parent=cls.chapter,
+                category='sequential',
+                display_name="Test Sequential 2",
+                graded=True,
+                format="Homework"
+            )
+            cls.problem2 = ItemFactory.create(
+                parent=cls.sequence2,
+                category="problem",
+                display_name="Test Problem",
+                data=problem_xml
+            )
 
     def setUp(self):
         super(GradeTestBase, self).setUp()
         self.request = get_mock_request(UserFactory())
         self.client.login(username=self.request.user.username, password="test")
+        self._update_grading_policy()
         self.course_structure = get_course_blocks(self.request.user, self.course.location)
         self.subsection_grade_factory = SubsectionGradeFactory(self.request.user, self.course, self.course_structure)
         CourseEnrollment.enroll(self.request.user, self.course.id)
 
-
-@ddt.ddt
-class TestCourseGradeFactory(GradeTestBase):
+    def _update_grading_policy(self, passing=0.5):
         """
-    Test that CourseGrades are calculated properly
+        Updates the course's grading policy.
         """
-    def setUp(self):
-        super(TestCourseGradeFactory, self).setUp()
-        grading_policy = {
+        self.grading_policy = {
             "GRADER": [
                 {
                     "type": "Homework",
@@ -98,10 +110,27 @@ class TestCourseGradeFactory(GradeTestBase):
                 },
             ],
             "GRADE_CUTOFFS": {
-                "Pass": 0.5,
+                "Pass": passing,
             },
         }
-        self.course.set_grading_policy(grading_policy)
+        self.course.set_grading_policy(self.grading_policy)
+        self.store.update_item(self.course, 0)
+
+
+@ddt.ddt
+class TestCourseGradeFactory(GradeTestBase):
+    """
+    Test that CourseGrades are calculated properly
+    """
+    def _assert_zero_grade(self, course_grade, expected_grade_class):
+        """
+        Asserts whether the given course_grade is as expected with
+        zero values.
+        """
+        self.assertIsInstance(course_grade, expected_grade_class)
+        self.assertIsNone(course_grade.letter_grade)
+        self.assertEqual(course_grade.percent, 0.0)
+        self.assertIsNotNone(course_grade.chapter_grades)
 
     def test_course_grade_no_access(self):
         """
@@ -138,60 +167,76 @@ class TestCourseGradeFactory(GradeTestBase):
                 grade_factory.create(self.request.user, self.course)
         self.assertEqual(mock_read_grade.called, feature_flag and course_setting)
 
-    def test_course_grade_creation(self):
+    def test_create(self):
         grade_factory = CourseGradeFactory()
-        with mock_get_score(1, 2):
+
+        def _assert_create(expected_pass):
+            """
+            Creates the grade, ensuring it is as expected.
+            """
             course_grade = grade_factory.create(self.request.user, self.course)
-        self.assertEqual(course_grade.letter_grade, u'Pass')
+            self.assertEqual(course_grade.letter_grade, u'Pass' if expected_pass else None)
             self.assertEqual(course_grade.percent, 0.5)
 
+        with self.assertNumQueries(12), mock_get_score(1, 2):
+            _assert_create(expected_pass=True)
+
+        with self.assertNumQueries(14), mock_get_score(1, 2):
+            grade_factory.update(self.request.user, self.course)
+
+        with self.assertNumQueries(1):
+            _assert_create(expected_pass=True)
+
+        self._update_grading_policy(passing=0.9)
+
+        with self.assertNumQueries(8):
+            _assert_create(expected_pass=False)
+
     @ddt.data(True, False)
-    def test_zero_course_grade(self, assume_zero_enabled):
+    def test_create_zero(self, assume_zero_enabled):
         with waffle().override(ASSUME_ZERO_GRADE_IF_ABSENT, active=assume_zero_enabled):
             grade_factory = CourseGradeFactory()
-            with mock_get_score(0, 2):
             course_grade = grade_factory.create(self.request.user, self.course)
+            self._assert_zero_grade(course_grade, ZeroCourseGrade if assume_zero_enabled else CourseGrade)
 
-            self.assertIsInstance(course_grade, ZeroCourseGrade if assume_zero_enabled else CourseGrade)
-            self.assertIsNone(course_grade.letter_grade)
-            self.assertEqual(course_grade.percent, 0.0)
-            self.assertIsNotNone(course_grade.chapter_grades)
-
-    def test_get_persisted(self):
+    def test_create_zero_subs_grade_for_nonzero_course_grade(self):
+        with waffle().override(ASSUME_ZERO_GRADE_IF_ABSENT), waffle().override(WRITE_ONLY_IF_ENGAGED):
+            subsection = self.course_structure[self.sequence.location]
+            with mock_get_score(1, 2):
+                self.subsection_grade_factory.update(subsection)
+            course_grade = CourseGradeFactory().update(self.request.user, self.course)
+            subsection1_grade = course_grade.subsection_grades[self.sequence.location]
+            subsection2_grade = course_grade.subsection_grades[self.sequence2.location]
+            self.assertIsInstance(subsection1_grade, SubsectionGrade)
+            self.assertIsInstance(subsection2_grade, ZeroSubsectionGrade)
+
+    def test_read(self):
         grade_factory = CourseGradeFactory()
-        # first, create a grade in the database
         with mock_get_score(1, 2):
             grade_factory.update(self.request.user, self.course)
 
-        # retrieve the grade, ensuring it is as expected and take just one query
+        def _assert_read():
+            """
+            Reads the grade, ensuring it is as expected and requires just one query
+            """
             with self.assertNumQueries(1):
                 course_grade = grade_factory.read(self.request.user, self.course)
             self.assertEqual(course_grade.letter_grade, u'Pass')
             self.assertEqual(course_grade.percent, 0.5)
 
-        # update the grading policy
-        new_grading_policy = {
-            "GRADER": [
-                {
-                    "type": "Homework",
-                    "min_count": 1,
-                    "drop_count": 0,
-                    "short_label": "HW",
-                    "weight": 1.0,
-                },
-            ],
-            "GRADE_CUTOFFS": {
-                "Pass": 0.9,
-            },
-        }
-        self.course.set_grading_policy(new_grading_policy)
+        _assert_read()
+        self._update_grading_policy(passing=0.9)
+        _assert_read()
 
-        # ensure the grade can still be retrieved via read
-        # despite its outdated grading policy
-        with self.assertNumQueries(1):
-            course_grade = grade_factory.read(self.request.user, self.course)
-        self.assertEqual(course_grade.letter_grade, u'Pass')
-        self.assertEqual(course_grade.percent, 0.5)
+    @ddt.data(True, False)
+    def test_read_zero(self, assume_zero_enabled):
+        with waffle().override(ASSUME_ZERO_GRADE_IF_ABSENT, active=assume_zero_enabled):
+            grade_factory = CourseGradeFactory()
+            course_grade = grade_factory.read(self.request.user, course_key=self.course.id)
+            if assume_zero_enabled:
+                self._assert_zero_grade(course_grade, ZeroCourseGrade)
+            else:
+                self.assertIsNone(course_grade)
 
 
 @ddt.ddt
@@ -305,7 +350,6 @@ class ZeroGradeTest(GradeTestBase):
     Tests ZeroCourseGrade (and, implicitly, ZeroSubsectionGrade)
     functionality.
     """
-
     def test_zero(self):
         """
         Creates a ZeroCourseGrade and ensures it's empty.
@@ -450,6 +494,7 @@ class TestVariedMetadata(ProblemSubmissionTestMixin, ModuleStoreTestCase):
     def setUp(self):
         super(TestVariedMetadata, self).setUp()
         self.course = CourseFactory.create()
+        with self.store.bulk_operations(self.course.id):
             self.chapter = ItemFactory.create(
                 parent=self.course,
                 category="chapter",
@@ -551,6 +596,7 @@ class TestCourseGradeLogging(ProblemSubmissionTestMixin, SharedModuleStoreTestCa
     def setUp(self):
         super(TestCourseGradeLogging, self).setUp()
         self.course = CourseFactory.create()
+        with self.store.bulk_operations(self.course.id):
             self.chapter = ItemFactory.create(
                 parent=self.course,
                 category="chapter",