Merge pull request #13433 from edx/tnl/enable-persistent-grades-in-tests

Enable Persistent Grades in unit tests

lms/djangoapps/ccx/tests/test_field_override_performance.py

@@ -36,6 +36,7 @@ from openedx.core.djangoapps.content.block_structure.api import get_course_in_ca
     'django.conf.settings.FEATURES',
     {
         'ENABLE_XBLOCK_VIEW_ENDPOINT': True,
+        'PERSISTENT_GRADES_ENABLED_FOR_ALL_TESTS': False  # disable persistent grades until TNL-5458 (reduces queries)
     }
 )
 @ddt.ddt

lms/djangoapps/courseware/tests/test_submitting_problems.py

@@ -25,6 +25,7 @@ from courseware.models import StudentModule, BaseStudentModuleHistory
 from courseware.tests.helpers import LoginEnrollmentTestCase
 from lms.djangoapps.lms_xblock.runtime import quote_slashes
 from student.models import anonymous_id_for_user, CourseEnrollment
+from submissions import api as submissions_api
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
 from xmodule.partitions.partitions import Group, UserPartition
@@ -143,9 +144,22 @@ class TestSubmittingProblems(ModuleStoreTestCase, LoginEnrollmentTestCase, Probl
         self.student_user = User.objects.get(email=self.student)
         self.factory = RequestFactory()
         # Disable the score change signal to prevent other components from being pulled into tests.
-        signal_patch = patch('courseware.module_render.SCORE_CHANGED.send')
-        signal_patch.start()
-        self.addCleanup(signal_patch.stop)
+        self.score_changed_signal_patch = patch('courseware.module_render.SCORE_CHANGED.send')
+        self.score_changed_signal_patch.start()
+
+    def tearDown(self):
+        super(TestSubmittingProblems, self).tearDown()
+        self._stop_signal_patch()
+
+    def _stop_signal_patch(self):
+        """
+        Stops the signal patch for the SCORE_CHANGED event.
+        In case a test wants to test with the event actually
+        firing.
+        """
+        if self.score_changed_signal_patch:
+            self.score_changed_signal_patch.stop()
+            self.score_changed_signal_patch = None
 
     def add_dropdown_to_section(self, section_location, name, num_inputs=2):
         """
@@ -540,12 +554,18 @@ class TestCourseGrader(TestSubmittingProblems):
         self.check_grade_percent(0.67)
         self.assertEqual(self.get_grade_summary()['grade'], 'B')
 
-        # But now we mock out a get_scores call, and watch as it overrides the
-        # score read from StudentModule and our student gets an A instead.
-        with patch('submissions.api.get_scores') as mock_get_scores:
-            mock_get_scores.return_value = {
-                self.problem_location('p3').to_deprecated_string(): (1, 1)
+        # But now, set the score with the submissions API and watch
+        # as it overrides the score read from StudentModule and our
+        # student gets an A instead.
+        self._stop_signal_patch()
+        student_item = {
+            'student_id': anonymous_id_for_user(self.student_user, self.course.id),
+            'course_id': unicode(self.course.id),
+            'item_id': unicode(self.problem_location('p3')),
+            'item_type': 'problem'
         }
+        submission = submissions_api.create_submission(student_item, 'any answer')
+        submissions_api.set_score(submission['uuid'], 1, 1)
         self.check_grade_percent(1.0)
         self.assertEqual(self.get_grade_summary()['grade'], 'A')
 

lms/djangoapps/courseware/tests/test_views.py

@@ -1345,6 +1345,8 @@ class ProgressPageTests(ModuleStoreTestCase):
         )
         self.assertContains(resp, u"Download Your Certificate")
 
+    # disable persistent grades until TNL-5458 (reduces query counts)
+    @patch.dict(settings.FEATURES, {'PERSISTENT_GRADES_ENABLED_FOR_ALL_TESTS': False})
     @ddt.data(
         *itertools.product(((39, 4, True), (39, 4, False)), (True, False))
     )

lms/djangoapps/grades/config/models.py

@@ -3,6 +3,7 @@ Models for configuration of the feature flags
 controlling persistent grades.
 """
 from config_models.models import ConfigurationModel
+from django.conf import settings
 from django.db.models import BooleanField
 from xmodule_django.models import CourseKeyField
 
@@ -29,6 +30,8 @@ class PersistentGradesEnabledFlag(ConfigurationModel):
         If the flag is enabled and no course ID is given,
             we return True since the global setting is enabled.
         """
+        if settings.FEATURES.get('PERSISTENT_GRADES_ENABLED_FOR_ALL_TESTS'):
+            return True
         if not PersistentGradesEnabledFlag.is_enabled():
             return False
         elif not PersistentGradesEnabledFlag.current().enabled_for_all_courses and course_id:

lms/djangoapps/grades/config/tests/test_models.py

@@ -3,6 +3,9 @@ Tests for the models that control the
 persistent grading feature.
 """
 import ddt
+from django.conf import settings
+import itertools
+from mock import patch
 
 from django.test import TestCase
 from opaque_keys.edx.locator import CourseLocator
@@ -10,6 +13,7 @@ from lms.djangoapps.grades.config.models import PersistentGradesEnabledFlag
 from lms.djangoapps.grades.config.tests.utils import persistent_grades_feature_flags
 
 
+@patch.dict(settings.FEATURES, {'PERSISTENT_GRADES_ENABLED_FOR_ALL_TESTS': False})
 @ddt.ddt
 class PersistentGradesFeatureFlagTests(TestCase):
     """
@@ -21,16 +25,11 @@ class PersistentGradesFeatureFlagTests(TestCase):
         self.course_id_1 = CourseLocator(org="edx", course="course", run="run")
         self.course_id_2 = CourseLocator(org="edx", course="course2", run="run")
 
-    @ddt.data(
-        (True, True, True),
-        (True, True, False),
-        (True, False, True),
-        (True, False, False),
-        (False, True, True),
-        (False, False, True),
-        (False, True, False),
-        (False, False, False),
-    )
+    @ddt.data(*itertools.product(
+        (True, False),
+        (True, False),
+        (True, False),
+    ))
     @ddt.unpack
     def test_persistent_grades_feature_flags(self, global_flag, enabled_for_all_courses, enabled_for_course_1):
         with persistent_grades_feature_flags(

lms/djangoapps/grades/models.py

@@ -29,13 +29,16 @@ log = logging.getLogger(__name__)
 BlockRecord = namedtuple('BlockRecord', ['locator', 'weight', 'max_score'])
 
 
-class BlockRecordSet(frozenset):
+class BlockRecordList(tuple):
     """
-    An immutable ordered collection of BlockRecord objects.
+    An immutable ordered list of BlockRecord objects.
     """
 
-    def __init__(self, *args, **kwargs):
-        super(BlockRecordSet, self).__init__(*args, **kwargs)
+    def __new__(cls, blocks):
+        return super(BlockRecordList, cls).__new__(cls, tuple(blocks))
+
+    def __init__(self, blocks):
+        super(BlockRecordList, self).__init__(blocks)
         self._json = None
         self._hash = None
 
@@ -56,8 +59,7 @@ class BlockRecordSet(frozenset):
         stable ordering.
         """
         if self._json is None:
-            sorted_blocks = sorted(self, key=attrgetter('locator'))
-            list_of_block_dicts = [block._asdict() for block in sorted_blocks]
+            list_of_block_dicts = [block._asdict() for block in self]
             course_key_string = self._get_course_key_string()  # all blocks are from the same course
 
             for block_dict in list_of_block_dicts:
@@ -77,7 +79,7 @@ class BlockRecordSet(frozenset):
     @classmethod
     def from_json(cls, blockrecord_json):
         """
-        Return a BlockRecordSet from a json list.
+        Return a BlockRecordList from a previously serialized json.
         """
         data = json.loads(blockrecord_json)
         course_key = data['course_key']
@@ -97,6 +99,13 @@ class BlockRecordSet(frozenset):
         )
         return cls(record_generator)
 
+    @classmethod
+    def from_list(cls, blocks):
+        """
+        Return a BlockRecordList from a list.
+        """
+        return cls(tuple(blocks))
+
     def to_hash(self):
         """
         Return a hashed version of the list of block records.
@@ -120,24 +129,18 @@ class VisibleBlocksQuerySet(models.QuerySet):
         """
         Creates a new VisibleBlocks model object.
 
-        Argument 'blocks' should be a BlockRecordSet.
+        Argument 'blocks' should be a BlockRecordList.
         """
-
-        if not isinstance(blocks, BlockRecordSet):
-            blocks = BlockRecordSet(blocks)
-
         model, _ = self.get_or_create(hashed=blocks.to_hash(), defaults={'blocks_json': blocks.to_json()})
         return model
 
     def hash_from_blockrecords(self, blocks):
         """
-        Return the hash for a given BlockRecordSet, serializing the records if
+        Return the hash for a given list of blocks, saving the records if
         possible, but returning the hash even if an IntegrityError occurs.
-        """
-
-        if not isinstance(blocks, BlockRecordSet):
-            blocks = BlockRecordSet(blocks)
 
+        Argument 'blocks' should be a BlockRecordList.
+        """
         try:
             with transaction.atomic():
                 model = self.create_from_blockrecords(blocks)
@@ -176,7 +179,7 @@ class VisibleBlocks(models.Model):
         Returns the blocks_json data stored on this model as a list of
         BlockRecords in the order they were provided.
         """
-        return BlockRecordSet.from_json(self.blocks_json)
+        return BlockRecordList.from_json(self.blocks_json)
 
 
 class PersistentSubsectionGradeQuerySet(models.QuerySet):
@@ -204,7 +207,7 @@ class PersistentSubsectionGradeQuerySet(models.QuerySet):
         if not kwargs.get('course_id', None):
             kwargs['course_id'] = kwargs['usage_key'].course_key
 
-        visible_blocks_hash = VisibleBlocks.objects.hash_from_blockrecords(blocks=visible_blocks)
+        visible_blocks_hash = VisibleBlocks.objects.hash_from_blockrecords(BlockRecordList.from_list(visible_blocks))
         return super(PersistentSubsectionGradeQuerySet, self).create(
             visible_blocks_id=visible_blocks_hash,
             **kwargs
@@ -358,7 +361,7 @@ class PersistentSubsectionGrade(TimeStampedModel):
         Modify an existing PersistentSubsectionGrade object, saving the new
         version.
         """
-        visible_blocks_hash = VisibleBlocks.objects.hash_from_blockrecords(blocks=visible_blocks)
+        visible_blocks_hash = VisibleBlocks.objects.hash_from_blockrecords(BlockRecordList.from_list(visible_blocks))
 
         self.course_version = course_version or ""
         self.subtree_edited_timestamp = subtree_edited_timestamp

lms/djangoapps/grades/signals/handlers.py

@@ -43,8 +43,6 @@ def submissions_score_set_handler(sender, **kwargs):  # pylint: disable=unused-a
     usage_id = kwargs['item_id']
     user = user_by_anonymous_id(kwargs['anonymous_user_id'])
 
-    # If any of the kwargs were missing, at least one of the following values
-    # will be None.
     SCORE_CHANGED.send(
         sender=None,
         points_possible=points_possible,
@@ -73,8 +71,6 @@ def submissions_score_reset_handler(sender, **kwargs):  # pylint: disable=unused
     usage_id = kwargs['item_id']
     user = user_by_anonymous_id(kwargs['anonymous_user_id'])
 
-    # If any of the kwargs were missing, at least one of the following values
-    # will be None.
     SCORE_CHANGED.send(
         sender=None,
         points_possible=0,

lms/djangoapps/grades/tests/test_models.py

@@ -14,27 +14,27 @@ from opaque_keys.edx.locator import CourseLocator, BlockUsageLocator
 
 from lms.djangoapps.grades.models import (
     BlockRecord,
-    BlockRecordSet,
+    BlockRecordList,
     PersistentSubsectionGrade,
     VisibleBlocks
 )
 
 
-class BlockRecordSetTestCase(TestCase):
+class BlockRecordListTestCase(TestCase):
     """
-    Verify the behavior of BlockRecordSets, particularly around edge cases
+    Verify the behavior of BlockRecordList, particularly around edge cases
     """
     empty_json = '{"blocks":[],"course_key":null}'
 
     def test_empty_block_record_set(self):
-        brs = BlockRecordSet()
+        brs = BlockRecordList(())
         self.assertFalse(brs)
         self.assertEqual(
             brs.to_json(),
             self.empty_json
         )
         self.assertEqual(
-            BlockRecordSet.from_json(self.empty_json),
+            BlockRecordList.from_json(self.empty_json),
             brs
         )
 
@@ -108,11 +108,17 @@ class VisibleBlocksTest(GradesModelTestCase):
     """
     Test the VisibleBlocks model.
     """
+    def _create_block_record_list(self, blocks):
+        """
+        Creates and returns a BlockRecordList for the given blocks.
+        """
+        return VisibleBlocks.objects.create_from_blockrecords(BlockRecordList.from_list(blocks))
+
     def test_creation(self):
         """
         Happy path test to ensure basic create functionality works as expected.
         """
-        vblocks = VisibleBlocks.objects.create_from_blockrecords([self.record_a])
+        vblocks = self._create_block_record_list([self.record_a])
         list_of_block_dicts = [self.record_a._asdict()]
         for block_dict in list_of_block_dicts:
             block_dict['locator'] = unicode(block_dict['locator'])  # BlockUsageLocator is not json-serializable
@@ -128,30 +134,34 @@ class VisibleBlocksTest(GradesModelTestCase):
         self.assertEqual(expected_json, vblocks.blocks_json)
         self.assertEqual(expected_hash, vblocks.hashed)
 
-    def test_ordering_does_not_matter(self):
+    def test_ordering_matters(self):
         """
-        When creating new vblocks, a different ordering of blocks produces the
-        same record in the database.
+        When creating new vblocks, different ordering of blocks produces
+        different records in the database.
         """
-        stored_vblocks = VisibleBlocks.objects.create_from_blockrecords([self.record_a, self.record_b])
-        repeat_vblocks = VisibleBlocks.objects.create_from_blockrecords([self.record_b, self.record_a])
-        new_vblocks = VisibleBlocks.objects.create_from_blockrecords([self.record_b])
+        stored_vblocks = self._create_block_record_list([self.record_a, self.record_b])
+        repeat_vblocks = self._create_block_record_list([self.record_b, self.record_a])
+        same_order_vblocks = self._create_block_record_list([self.record_a, self.record_b])
+        new_vblocks = self._create_block_record_list([self.record_b])
+
+        self.assertNotEqual(stored_vblocks.pk, repeat_vblocks.pk)
+        self.assertNotEqual(stored_vblocks.hashed, repeat_vblocks.hashed)
 
-        self.assertEqual(stored_vblocks.pk, repeat_vblocks.pk)
-        self.assertEqual(stored_vblocks.hashed, repeat_vblocks.hashed)
+        self.assertEquals(stored_vblocks.pk, same_order_vblocks.pk)
+        self.assertEquals(stored_vblocks.hashed, same_order_vblocks.hashed)
 
         self.assertNotEqual(stored_vblocks.pk, new_vblocks.pk)
         self.assertNotEqual(stored_vblocks.hashed, new_vblocks.hashed)
 
     def test_blocks_property(self):
         """
-        Ensures that, given an array of BlockRecord, creating visible_blocks and accessing
-        visible_blocks.blocks yields a copy of the initial array. Also, trying to set the blocks property should raise
-        an exception.
+        Ensures that, given an array of BlockRecord, creating visible_blocks
+        and accessing visible_blocks.blocks yields a copy of the initial array.
+        Also, trying to set the blocks property should raise an exception.
         """
-        expected_blocks = [self.record_a, self.record_b]
-        visible_blocks = VisibleBlocks.objects.create_from_blockrecords(expected_blocks)
-        self.assertEqual(BlockRecordSet(expected_blocks), visible_blocks.blocks)
+        expected_blocks = (self.record_a, self.record_b)
+        visible_blocks = self._create_block_record_list(expected_blocks)
+        self.assertEqual(expected_blocks, visible_blocks.blocks)
         with self.assertRaises(AttributeError):
             visible_blocks.blocks = expected_blocks
 

lms/djangoapps/grades/tests/test_new.py

@@ -3,6 +3,7 @@ Test saved subsection grade functionality.
 """
 
 import ddt
+from django.conf import settings
 from mock import patch
 
 from capa.tests.response_xml_factory import MultipleChoiceResponseXMLFactory
@@ -71,6 +72,7 @@ class TestCourseGradeFactory(GradeTestBase):
     Test that CourseGrades are calculated properly
     """
 
+    @patch.dict(settings.FEATURES, {'PERSISTENT_GRADES_ENABLED_FOR_ALL_TESTS': False})
     @ddt.data(
         (True, True),
         (True, False),
@@ -107,12 +109,6 @@ class SubsectionGradeFactoryTest(GradeTestBase):
         """
         Tests to ensure that a persistent subsection grade is created, saved, then fetched on re-request.
         """
-        with persistent_grades_feature_flags(
-            global_flag=True,
-            enabled_for_all_courses=False,
-            course_id=self.course.id,
-            enabled_for_course=True
-        ):
         with patch(
             'lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory._save_grade',
             wraps=self.subsection_grade_factory._save_grade  # pylint: disable=protected-access
@@ -121,7 +117,7 @@ class SubsectionGradeFactoryTest(GradeTestBase):
                 'lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory._get_saved_grade',
                 wraps=self.subsection_grade_factory._get_saved_grade  # pylint: disable=protected-access
             ) as mock_get_saved_grade:
-                    with self.assertNumQueries(22):
+                with self.assertNumQueries(19):
                     grade_a = self.subsection_grade_factory.create(
                         self.sequence,
                         self.course_structure,
@@ -133,7 +129,7 @@ class SubsectionGradeFactoryTest(GradeTestBase):
                 mock_get_saved_grade.reset_mock()
                 mock_save_grades.reset_mock()
 
-                    with self.assertNumQueries(4):
+                with self.assertNumQueries(3):
                     grade_b = self.subsection_grade_factory.create(
                         self.sequence,
                         self.course_structure,
@@ -145,6 +141,7 @@ class SubsectionGradeFactoryTest(GradeTestBase):
         self.assertEqual(grade_a.url_name, grade_b.url_name)
         self.assertEqual(grade_a.all_total, grade_b.all_total)
 
+    @patch.dict(settings.FEATURES, {'PERSISTENT_GRADES_ENABLED_FOR_ALL_TESTS': False})
     @ddt.data(
         (True, True),
         (True, False),

lms/djangoapps/grades/tests/test_signals.py

@@ -3,6 +3,7 @@ Tests for the score change signals defined in the courseware models module.
 """
 
 import ddt
+from django.conf import settings
 from django.test import TestCase
 from mock import patch, MagicMock
 from unittest import skip
@@ -169,6 +170,7 @@ class SubmissionSignalRelayTest(TestCase):
         self.signal_mock.assert_not_called()
 
 
+@patch.dict(settings.FEATURES, {'PERSISTENT_GRADES_ENABLED_FOR_ALL_TESTS': False})
 @ddt.ddt
 class ScoreChangedUpdatesSubsectionGradeTest(ModuleStoreTestCase):
     """

lms/djangoapps/instructor_task/tests/test_tasks_helper.py

@@ -1641,6 +1641,8 @@ class TestCertificateGeneration(InstructorTaskModuleTestCase):
         super(TestCertificateGeneration, self).setUp()
         self.initialize_course()
 
+    # disable persistent grades until TNL-5458 (reduces query counts)
+    @patch.dict(settings.FEATURES, {'PERSISTENT_GRADES_ENABLED_FOR_ALL_TESTS': False})
     def test_certificate_generation_for_students(self):
         """
         Verify that certificates generated for all eligible students enrolled in a course.

lms/envs/test.py

@@ -298,6 +298,9 @@ OIDC_COURSE_HANDLER_CACHE_TIMEOUT = 0
 FEATURES['ENABLE_MOBILE_REST_API'] = True
 FEATURES['ENABLE_VIDEO_ABSTRACTION_LAYER_API'] = True
 
+########################### Grades #################################
+FEATURES['PERSISTENT_GRADES_ENABLED_FOR_ALL_TESTS'] = True
+
 ###################### Payment ##############################3
 # Enable fake payment processing page
 FEATURES['ENABLE_PAYMENT_FAKE'] = True