Reduce sql queries with persisted grades

TNL-5458
TNL-5493

lms/djangoapps/courseware/views/views.py

@@ -719,7 +719,7 @@ def _progress(request, course_key, student_id):
     # additional DB lookup (this kills the Progress page in particular).
     student = User.objects.prefetch_related("groups").get(id=student.id)
 
-    course_grade = CourseGradeFactory(student).create(course)
+    course_grade = CourseGradeFactory(student).create(course, read_only=False)
     if not course_grade.has_access_to_course:
         # This means the student didn't have access to the course (which the instructor requested)
         raise Http404

lms/djangoapps/grades/new/course_grade.py

@@ -40,10 +40,7 @@ class CourseGrade(object):
                     graded_total = subsection_grade.graded_total
                     if graded_total.possible > 0:
                         subsections_by_format[subsection_grade.format].append(graded_total)
-        log.info(u"Persistent Grades: Calculated subsections_by_format. course id: {0}, user: {1}".format(
-            self.course.location,
-            self.student.id
-        ))
+        self._log_event(log.info, u"subsections_by_format")
         return subsections_by_format
 
     @lazy
@@ -55,10 +52,7 @@ class CourseGrade(object):
         for chapter in self.chapter_grades:
             for subsection_grade in chapter['sections']:
                 locations_to_weighted_scores.update(subsection_grade.locations_to_weighted_scores)
-        log.info(u"Persistent Grades: Calculated locations_to_weighted_scores. course id: {0}, user: {1}".format(
-            self.course.id,
-            self.student.id
-        ))
+        self._log_event(log.info, u"locations_to_weighted_scores")
         return locations_to_weighted_scores
 
     @lazy
@@ -72,10 +66,7 @@ class CourseGrade(object):
             self.subsection_grade_totals_by_format,
             generate_random_scores=settings.GENERATE_PROFILE_SCORES
         )
-        log.info(u"Persistent Grades: Calculated grade_value. course id: {0}, user: {1}".format(
-            self.course.location,
-            self.student.id
-        ))
+        self._log_event(log.info, u"grade_value")
         return grade_value
 
     @property
@@ -123,31 +114,34 @@ class CourseGrade(object):
         grade_summary['totaled_scores'] = self.subsection_grade_totals_by_format
         grade_summary['raw_scores'] = list(self.locations_to_weighted_scores.itervalues())
 
+        self._log_event(log.warning, u"grade_summary, percent: {0}, grade: {1}".format(self.percent, self.letter_grade))
         return grade_summary
 
-    def compute(self):
+    def compute_and_update(self, read_only=False):
         """
         Computes the grade for the given student and course.
+
+        If read_only is True, doesn't save any updates to the grades.
         """
-        subsection_grade_factory = SubsectionGradeFactory(self.student)
+        self._log_event(log.warning, u"compute_and_update, read_only: {}".format(read_only))
+        subsection_grade_factory = SubsectionGradeFactory(self.student, self.course, self.course_structure)
         for chapter_key in self.course_structure.get_children(self.course.location):
             chapter = self.course_structure[chapter_key]
-            subsection_grades = []
+            chapter_subsection_grades = []
             for subsection_key in self.course_structure.get_children(chapter_key):
-                subsection_grades.append(
-                    subsection_grade_factory.create(
-                        self.course_structure[subsection_key],
-                        self.course_structure,
-                        self.course,
-                    )
+                chapter_subsection_grades.append(
+                    subsection_grade_factory.create(self.course_structure[subsection_key], read_only=True)
                 )
 
             self.chapter_grades.append({
                 'display_name': block_metadata_utils.display_name_with_default_escaped(chapter),
                 'url_name': block_metadata_utils.url_name_for_block(chapter),
-                'sections': subsection_grades
+                'sections': chapter_subsection_grades
             })
 
+        if not read_only:
+            subsection_grade_factory.bulk_create_unsaved()
+
         self._signal_listeners_when_grade_computed()
 
     def score_for_module(self, location):
@@ -212,6 +206,16 @@ class CourseGrade(object):
                 receiver, response
             )
 
+    def _log_event(self, log_func, log_statement):
+        """
+        Logs the given statement, for this instance.
+        """
+        log_func(u"Persistent Grades: CourseGrade.{0}, course: {1}, user: {2}".format(
+            log_statement,
+            self.course.id,
+            self.student.id
+        ))
+
 
 class CourseGradeFactory(object):
     """
@@ -220,22 +224,26 @@ class CourseGradeFactory(object):
     def __init__(self, student):
         self.student = student
 
-    def create(self, course):
+    def create(self, course, read_only=False):
         """
         Returns the CourseGrade object for the given student and course.
+
+        If read_only is True, doesn't save any updates to the grades.
         """
         course_structure = get_course_blocks(self.student, course.location)
         return (
             self._get_saved_grade(course, course_structure) or
-            self._compute_and_update_grade(course, course_structure)
+            self._compute_and_update_grade(course, course_structure, read_only)
         )
 
-    def _compute_and_update_grade(self, course, course_structure):
+    def _compute_and_update_grade(self, course, course_structure, read_only):
         """
         Freshly computes and updates the grade for the student and course.
+
+        If read_only is True, doesn't save any updates to the grades.
         """
         course_grade = CourseGrade(self.student, course, course_structure)
-        course_grade.compute()
+        course_grade.compute_and_update(read_only)
         return course_grade
 
     def _get_saved_grade(self, course, course_structure):  # pylint: disable=unused-argument

lms/djangoapps/grades/scores.py

 """
 Functionality for problem scores.
 """
+from logging import getLogger
+
 from openedx.core.lib.cache_utils import memoized
 from xblock.core import XBlock
 from .transformer import GradesTransformer
 
 
+log = getLogger(__name__)
+
+
 @memoized
 def block_types_possibly_scored():
     """
@@ -30,15 +35,17 @@ def possibly_scored(usage_key):
     return usage_key.block_type in block_types_possibly_scored()
 
 
-def weighted_score(raw_earned, raw_possible, weight):
-    """Return a tuple that represents the weighted (correct, total) score."""
-    # If there is no weighting, or weighting can't be applied, return input.
+def weighted_score(raw_earned, raw_possible, weight=None):
+    """
+    Return a tuple that represents the weighted (earned, possible) score.
+    If weight is None or raw_possible is 0, returns the original values.
+    """
     if weight is None or raw_possible == 0:
         return (raw_earned, raw_possible)
     return float(raw_earned) * weight / raw_possible, float(weight)
 
 
-def get_score(user, block, scores_client, submissions_scores_cache):
+def get_score(user, block, scores_client, submissions_scores_cache, weight, possible=None):
     """
     Return the score for a user on a problem, as a tuple (earned, possible).
     e.g. (5,7) if you got 5 out of 7 points.
@@ -49,8 +56,13 @@ def get_score(user, block, scores_client, submissions_scores_cache):
     user: a Student object
     block: a BlockStructure's BlockData object
     scores_client: an initialized ScoresClient
-    submissions_scores_cache: A dict of location names to (earned, possible) point tuples.
-           If an entry is found in this cache, it takes precedence.
+    submissions_scores_cache: A dict of location names to (earned, possible)
+        point tuples.  If an entry is found in this cache, it takes precedence.
+    weight: The weight of the problem to use in the calculation.  A value of
+        None signifies that the weight should not be applied.
+    possible (optional): The possible maximum score of the problem to use in the
+        calculation.  If None, uses the value found either in scores_client or
+        from the block.
     """
     submissions_scores_cache = submissions_scores_cache or {}
 
@@ -74,16 +86,20 @@ def get_score(user, block, scores_client, submissions_scores_cache):
     if score and score.total is not None:
         # We have a valid score, just use it.
         earned = score.correct if score.correct is not None else 0.0
-        possible = score.total
+        if possible is None:
+            possible = score.total
+        elif possible != score.total:
+            log.error(u"Persisted Grades: possible value {} != score.total value {}".format(possible, score.total))
     else:
         # This means we don't have a valid score entry and we don't have a
         # cached_max_score on hand. We know they've earned 0.0 points on this.
         earned = 0.0
-        possible = block.transformer_data[GradesTransformer].max_score
+        if possible is None:
+            possible = block.transformer_data[GradesTransformer].max_score
 
         # Problem may be an error module (if something in the problem builder failed)
         # In which case possible might be None
         if possible is None:
             return (None, None)
 
-    return weighted_score(earned, possible, block.weight)
+    return weighted_score(earned, possible, weight)

lms/djangoapps/grades/signals/handlers.py

@@ -108,10 +108,13 @@ def recalculate_subsection_grade_handler(sender, **kwargs):  # pylint: disable=u
         'subsections',
         set()
     )
+    subsection_grade_factory = SubsectionGradeFactory(student, course, collected_block_structure)
     for subsection_usage_key in subsections_to_update:
         transformed_subsection_structure = get_course_blocks(
             student,
             subsection_usage_key,
             collected_block_structure=collected_block_structure,
         )
-        SubsectionGradeFactory(student).update(subsection_usage_key, transformed_subsection_structure, course)
+        subsection_grade_factory.update(
+            transformed_subsection_structure[subsection_usage_key], transformed_subsection_structure
+        )

lms/djangoapps/grades/tests/test_grades.py

@@ -395,7 +395,7 @@ class TestGetModuleScore(LoginEnrollmentTestCase, SharedModuleStoreTestCase):
         # then stored in the request).
         with self.assertNumQueries(1):
             score = get_module_score(self.request.user, self.course, self.seq1)
-        new_score = SubsectionGradeFactory(self.request.user).create(self.seq1, self.course_structure, self.course)
+        new_score = SubsectionGradeFactory(self.request.user, self.course, self.course_structure).create(self.seq1)
         self.assertEqual(score, 0)
         self.assertEqual(new_score.all_total.earned, 0)
 
@@ -404,7 +404,7 @@ class TestGetModuleScore(LoginEnrollmentTestCase, SharedModuleStoreTestCase):
 
         with self.assertNumQueries(1):
             score = get_module_score(self.request.user, self.course, self.seq1)
-        new_score = SubsectionGradeFactory(self.request.user).create(self.seq1, self.course_structure, self.course)
+        new_score = SubsectionGradeFactory(self.request.user, self.course, self.course_structure).create(self.seq1)
         self.assertEqual(score, 1.0)
         self.assertEqual(new_score.all_total.earned, 2.0)
         # These differ because get_module_score normalizes the subsection score
@@ -416,7 +416,7 @@ class TestGetModuleScore(LoginEnrollmentTestCase, SharedModuleStoreTestCase):
 
         with self.assertNumQueries(1):
             score = get_module_score(self.request.user, self.course, self.seq1)
-        new_score = SubsectionGradeFactory(self.request.user).create(self.seq1, self.course_structure, self.course)
+        new_score = SubsectionGradeFactory(self.request.user, self.course, self.course_structure).create(self.seq1)
         self.assertEqual(score, .5)
         self.assertEqual(new_score.all_total.earned, 1.0)
 

lms/djangoapps/grades/tests/test_models.py

@@ -6,7 +6,6 @@ from collections import OrderedDict
 import ddt
 from hashlib import sha1
 import json
-from mock import patch
 
 from django.db.utils import IntegrityError
 from django.test import TestCase
@@ -24,17 +23,25 @@ class BlockRecordListTestCase(TestCase):
     """
     Verify the behavior of BlockRecordList, particularly around edge cases
     """
-    empty_json = '{"blocks":[],"course_key":null}'
+    def setUp(self):
+        super(BlockRecordListTestCase, self).setUp()
+        self.course_key = CourseLocator(
+            org='some_org',
+            course='some_course',
+            run='some_run'
+        )
 
     def test_empty_block_record_set(self):
-        brs = BlockRecordList(())
+        empty_json = '{0}"blocks":[],"course_key":"{1}"{2}'.format('{', unicode(self.course_key), '}')
+
+        brs = BlockRecordList((), self.course_key)
         self.assertFalse(brs)
         self.assertEqual(
-            brs.to_json(),
-            self.empty_json
+            brs.json_value,
+            empty_json
         )
         self.assertEqual(
-            BlockRecordList.from_json(self.empty_json),
+            BlockRecordList.from_json(empty_json),
             brs
         )
 
@@ -112,7 +119,7 @@ class VisibleBlocksTest(GradesModelTestCase):
         """
         Creates and returns a BlockRecordList for the given blocks.
         """
-        return VisibleBlocks.objects.create_from_blockrecords(BlockRecordList.from_list(blocks))
+        return VisibleBlocks.objects.create_from_blockrecords(BlockRecordList.from_list(blocks, self.course_key))
 
     def test_creation(self):
         """
@@ -159,7 +166,7 @@ class VisibleBlocksTest(GradesModelTestCase):
         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)
+        expected_blocks = BlockRecordList.from_list([self.record_a, self.record_b], self.course_key)
         visible_blocks = self._create_block_record_list(expected_blocks)
         self.assertEqual(expected_blocks, visible_blocks.blocks)
         with self.assertRaises(AttributeError):
@@ -178,6 +185,7 @@ class PersistentSubsectionGradeTest(GradesModelTestCase):
             block_type='subsection',
             block_id='subsection_12345',
         )
+        self.block_records = BlockRecordList([self.record_a, self.record_b], self.course_key)
         self.params = {
             "user_id": 12345,
             "usage_key": self.usage_key,
@@ -187,21 +195,23 @@ class PersistentSubsectionGradeTest(GradesModelTestCase):
             "possible_all": 12.0,
             "earned_graded": 6.0,
             "possible_graded": 8.0,
-            "visible_blocks": [self.record_a, self.record_b],
+            "visible_blocks": self.block_records,
         }
 
     def test_create(self):
         """
         Tests model creation, and confirms error when trying to recreate model.
         """
-        created_grade = PersistentSubsectionGrade.objects.create(**self.params)
-        read_grade = PersistentSubsectionGrade.read_grade(
-            user_id=self.params["user_id"],
-            usage_key=self.params["usage_key"],
-        )
-        self.assertEqual(created_grade, read_grade)
+        created_grade = PersistentSubsectionGrade.create_grade(**self.params)
+        with self.assertNumQueries(1):
+            read_grade = PersistentSubsectionGrade.read_grade(
+                user_id=self.params["user_id"],
+                usage_key=self.params["usage_key"],
+            )
+            self.assertEqual(created_grade, read_grade)
+            self.assertEquals(read_grade.visible_blocks.blocks, self.block_records)
         with self.assertRaises(IntegrityError):
-            created_grade = PersistentSubsectionGrade.objects.create(**self.params)
+            PersistentSubsectionGrade.create_grade(**self.params)
 
     def test_create_bad_params(self):
         """
@@ -209,56 +219,19 @@ class PersistentSubsectionGradeTest(GradesModelTestCase):
         """
         del self.params["earned_graded"]
         with self.assertRaises(IntegrityError):
-            PersistentSubsectionGrade.objects.create(**self.params)
+            PersistentSubsectionGrade.create_grade(**self.params)
 
     def test_course_version_is_optional(self):
         del self.params["course_version"]
-        PersistentSubsectionGrade.objects.create(**self.params)
-
-    def test_update_grade(self):
-        """
-        Tests model update, and confirms error when updating a nonexistent model.
-        """
-        with self.assertRaises(PersistentSubsectionGrade.DoesNotExist):
-            PersistentSubsectionGrade.update_grade(**self.params)
-        PersistentSubsectionGrade.objects.create(**self.params)
-        self.params['earned_all'] = 12.0
-        self.params['earned_graded'] = 8.0
-
-        with patch('lms.djangoapps.grades.models.log') as log_mock:
-            PersistentSubsectionGrade.update_grade(**self.params)
-            read_grade = PersistentSubsectionGrade.read_grade(
-                user_id=self.params["user_id"],
-                usage_key=self.params["usage_key"],
-            )
-            log_mock.info.assert_called_with(
-                u"Persistent Grades: Grade model updated: {0}".format(read_grade)
-            )
-
-        self.assertEqual(read_grade.earned_all, 12.0)
-        self.assertEqual(read_grade.earned_graded, 8.0)
+        PersistentSubsectionGrade.create_grade(**self.params)
 
     @ddt.data(True, False)
-    def test_save(self, already_created):
-        if already_created:
-            PersistentSubsectionGrade.objects.create(**self.params)
-        module_prefix = "lms.djangoapps.grades.models.PersistentSubsectionGrade."
-        with patch(
-            module_prefix + "objects.get_or_create",
-            wraps=PersistentSubsectionGrade.objects.get_or_create
-        ) as mock_get_or_create:
-            with patch(module_prefix + "update") as mock_update:
-                PersistentSubsectionGrade.save_grade(**self.params)
-                self.assertTrue(mock_get_or_create.called)
-                self.assertEqual(mock_update.called, already_created)
+    def test_update_or_create_grade(self, already_created):
+        created_grade = PersistentSubsectionGrade.create_grade(**self.params) if already_created else None
 
-    def test_logging_for_save(self):
-        with patch('lms.djangoapps.grades.models.log') as log_mock:
-            PersistentSubsectionGrade.save_grade(**self.params)
-            read_grade = PersistentSubsectionGrade.read_grade(
-                user_id=self.params["user_id"],
-                usage_key=self.params["usage_key"],
-            )
-            log_mock.info.assert_called_with(
-                u"Persistent Grades: Grade model saved: {0}".format(read_grade)
-            )
+        self.params["earned_all"] = 7
+        updated_grade = PersistentSubsectionGrade.update_or_create_grade(**self.params)
+        self.assertEquals(updated_grade.earned_all, 7)
+        if already_created:
+            self.assertEquals(created_grade.id, updated_grade.id)
+            self.assertEquals(created_grade.earned_all, 6)

lms/djangoapps/grades/tests/test_new.py

@@ -61,8 +61,8 @@ class GradeTestBase(SharedModuleStoreTestCase):
         super(GradeTestBase, self).setUp()
         self.request = get_request_for_user(UserFactory())
         self.client.login(username=self.request.user.username, password="test")
-        self.subsection_grade_factory = SubsectionGradeFactory(self.request.user)
         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)
 
 
@@ -110,33 +110,25 @@ class SubsectionGradeFactoryTest(GradeTestBase):
         Tests to ensure that a persistent subsection grade is created, saved, then fetched on re-request.
         """
         with patch(
-            'lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory._save_grade',
-            wraps=self.subsection_grade_factory._save_grade  # pylint: disable=protected-access
-        ) as mock_save_grades:
+            'lms.djangoapps.grades.new.subsection_grade.PersistentSubsectionGrade.create_grade',
+            wraps=PersistentSubsectionGrade.create_grade
+        ) as mock_create_grade:
             with patch(
                 '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(19):
-                    grade_a = self.subsection_grade_factory.create(
-                        self.sequence,
-                        self.course_structure,
-                        self.course
-                    )
+                with self.assertNumQueries(12):
+                    grade_a = self.subsection_grade_factory.create(self.sequence)
                 self.assertTrue(mock_get_saved_grade.called)
-                self.assertTrue(mock_save_grades.called)
+                self.assertTrue(mock_create_grade.called)
 
                 mock_get_saved_grade.reset_mock()
-                mock_save_grades.reset_mock()
-
-                with self.assertNumQueries(3):
-                    grade_b = self.subsection_grade_factory.create(
-                        self.sequence,
-                        self.course_structure,
-                        self.course
-                    )
+                mock_create_grade.reset_mock()
+
+                with self.assertNumQueries(0):
+                    grade_b = self.subsection_grade_factory.create(self.sequence)
                 self.assertTrue(mock_get_saved_grade.called)
-                self.assertFalse(mock_save_grades.called)
+                self.assertFalse(mock_create_grade.called)
 
         self.assertEqual(grade_a.url_name, grade_b.url_name)
         self.assertEqual(grade_a.all_total, grade_b.all_total)
@@ -153,7 +145,7 @@ class SubsectionGradeFactoryTest(GradeTestBase):
         # Grades are only saved if the feature flag and the advanced setting are
         # both set to True.
         with patch(
-            'lms.djangoapps.grades.models.PersistentSubsectionGrade.read_grade'
+            'lms.djangoapps.grades.models.PersistentSubsectionGrade.bulk_read_grades'
         ) as mock_read_saved_grade:
             with persistent_grades_feature_flags(
                 global_flag=feature_flag,
@@ -161,7 +153,7 @@ class SubsectionGradeFactoryTest(GradeTestBase):
                 course_id=self.course.id,
                 enabled_for_course=course_setting
             ):
-                self.subsection_grade_factory.create(self.sequence, self.course_structure, self.course)
+                self.subsection_grade_factory.create(self.sequence)
         self.assertEqual(mock_read_saved_grade.called, feature_flag and course_setting)
 
 
@@ -174,10 +166,8 @@ class SubsectionGradeTest(GradeTestBase):
         """
         Assuming the underlying score reporting methods work, test that the score is calculated properly.
         """
-        grade = self.subsection_grade_factory.create(self.sequence, self.course_structure, self.course)
         with mock_get_score(1, 2):
-            # The final 2 parameters are only passed through to our mocked-out get_score method
-            grade.compute(self.request.user, self.course_structure, None, None)
+            grade = self.subsection_grade_factory.create(self.sequence)
         self.assertEqual(grade.all_total.earned, 1)
         self.assertEqual(grade.all_total.possible, 2)
 
@@ -186,9 +176,8 @@ class SubsectionGradeTest(GradeTestBase):
         Test that grades are persisted to the database properly, and that loading saved grades returns the same data.
         """
         # Create a grade that *isn't* saved to the database
-        self.subsection_grade_factory._prefetch_scores(self.course_structure, self.course)  # pylint: disable=protected-access
-        input_grade = SubsectionGrade(self.sequence)
-        input_grade.compute(
+        input_grade = SubsectionGrade(self.sequence, self.course)
+        input_grade.init_from_structure(
             self.request.user,
             self.course_structure,
             self.subsection_grade_factory._scores_client,  # pylint: disable=protected-access
@@ -197,16 +186,17 @@ class SubsectionGradeTest(GradeTestBase):
         self.assertEqual(PersistentSubsectionGrade.objects.count(), 0)
 
         # save to db, and verify object is in database
-        input_grade.save(self.request.user, self.sequence, self.course)
+        input_grade.create_model(self.request.user)
         self.assertEqual(PersistentSubsectionGrade.objects.count(), 1)
 
         # load from db, and ensure output matches input
-        loaded_grade = SubsectionGrade(self.sequence)
+        loaded_grade = SubsectionGrade(self.sequence, self.course)
         saved_model = PersistentSubsectionGrade.read_grade(
             user_id=self.request.user.id,
             usage_key=self.sequence.location,
         )
-        loaded_grade.load_from_data(
+        loaded_grade.init_from_model(
+            self.request.user,
             saved_model,
             self.course_structure,
             self.subsection_grade_factory._scores_client,  # pylint: disable=protected-access