Merge pull request #13459 from edx/efischer/catchall

Make Robust Grades Robust Again

lms/djangoapps/ccx/tests/test_field_override_performance.py

@@ -229,18 +229,18 @@ class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase):
         #     # of sql queries to default,
         #     # of mongo queries,
         # )
-        ('no_overrides', 1, True, False): (20, 6),
-        ('no_overrides', 2, True, False): (20, 6),
-        ('no_overrides', 3, True, False): (20, 6),
-        ('ccx', 1, True, False): (20, 6),
-        ('ccx', 2, True, False): (20, 6),
-        ('ccx', 3, True, False): (20, 6),
-        ('no_overrides', 1, False, False): (20, 6),
-        ('no_overrides', 2, False, False): (20, 6),
-        ('no_overrides', 3, False, False): (20, 6),
-        ('ccx', 1, False, False): (20, 6),
-        ('ccx', 2, False, False): (20, 6),
-        ('ccx', 3, False, False): (20, 6),
+        ('no_overrides', 1, True, False): (22, 6),
+        ('no_overrides', 2, True, False): (22, 6),
+        ('no_overrides', 3, True, False): (22, 6),
+        ('ccx', 1, True, False): (22, 6),
+        ('ccx', 2, True, False): (22, 6),
+        ('ccx', 3, True, False): (22, 6),
+        ('no_overrides', 1, False, False): (22, 6),
+        ('no_overrides', 2, False, False): (22, 6),
+        ('no_overrides', 3, False, False): (22, 6),
+        ('ccx', 1, False, False): (22, 6),
+        ('ccx', 2, False, False): (22, 6),
+        ('ccx', 3, False, False): (22, 6),
     }
 
 
@@ -252,19 +252,19 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase):
     __test__ = True
 
     TEST_DATA = {
-        ('no_overrides', 1, True, False): (20, 3),
-        ('no_overrides', 2, True, False): (20, 3),
-        ('no_overrides', 3, True, False): (20, 3),
-        ('ccx', 1, True, False): (20, 3),
-        ('ccx', 2, True, False): (20, 3),
-        ('ccx', 3, True, False): (20, 3),
-        ('ccx', 1, True, True): (21, 3),
-        ('ccx', 2, True, True): (21, 3),
-        ('ccx', 3, True, True): (21, 3),
-        ('no_overrides', 1, False, False): (20, 3),
-        ('no_overrides', 2, False, False): (20, 3),
-        ('no_overrides', 3, False, False): (20, 3),
-        ('ccx', 1, False, False): (20, 3),
-        ('ccx', 2, False, False): (20, 3),
-        ('ccx', 3, False, False): (20, 3),
+        ('no_overrides', 1, True, False): (22, 3),
+        ('no_overrides', 2, True, False): (22, 3),
+        ('no_overrides', 3, True, False): (22, 3),
+        ('ccx', 1, True, False): (22, 3),
+        ('ccx', 2, True, False): (22, 3),
+        ('ccx', 3, True, False): (22, 3),
+        ('ccx', 1, True, True): (23, 3),
+        ('ccx', 2, True, True): (23, 3),
+        ('ccx', 3, True, True): (23, 3),
+        ('no_overrides', 1, False, False): (22, 3),
+        ('no_overrides', 2, False, False): (22, 3),
+        ('no_overrides', 3, False, False): (22, 3),
+        ('ccx', 1, False, False): (22, 3),
+        ('ccx', 2, False, False): (22, 3),
+        ('ccx', 3, False, False): (22, 3),
     }

lms/djangoapps/courseware/tests/test_views.py

@@ -1345,17 +1345,17 @@ class ProgressPageTests(ModuleStoreTestCase):
         """Test that query counts remain the same for self-paced and instructor-paced courses."""
         SelfPacedConfiguration(enabled=self_paced_enabled).save()
         self.setup_course(self_paced=self_paced)
-        with self.assertNumQueries(37), check_mongo_calls(4):
+        with self.assertNumQueries(39), check_mongo_calls(4):
             self._get_progress_page()
 
     def test_progress_queries(self):
         self.setup_course()
-        with self.assertNumQueries(37), check_mongo_calls(4):
+        with self.assertNumQueries(39), check_mongo_calls(4):
             self._get_progress_page()
 
         # subsequent accesses to the progress page require fewer queries.
         for _ in range(2):
-            with self.assertNumQueries(20), check_mongo_calls(4):
+            with self.assertNumQueries(22), check_mongo_calls(4):
                 self._get_progress_page()
 
     @patch(

lms/djangoapps/grades/new/subsection_grade.py

@@ -2,6 +2,9 @@
 SubsectionGrade Class
 """
 from collections import OrderedDict
+from contextlib import contextmanager
+from django.db import transaction
+from django.db.utils import DatabaseError
 from lazy import lazy
 from logging import getLogger
 from courseware.model_data import ScoresClient
@@ -10,6 +13,7 @@ from lms.djangoapps.grades.models import BlockRecord, PersistentSubsectionGrade
 from lms.djangoapps.grades.config.models import PersistentGradesEnabledFlag
 from student.models import anonymous_id_for_user, User
 from submissions import api as submissions_api
+from traceback import format_exc
 from xmodule import block_metadata_utils, graders
 from xmodule.graders import Score
 
@@ -17,6 +21,20 @@ from xmodule.graders import Score
 log = getLogger(__name__)
 
 
+@contextmanager
+def persistence_safe_fallback():
+    """
+    A context manager intended to be used to wrap robust grades database-specific code that can be ignored on failure.
+    """
+    try:
+        with transaction.atomic():
+            yield
+    except DatabaseError:
+        # Error deliberately logged, then swallowed. It's assumed that the wrapped code is not guaranteed to succeed.
+        # TODO: enqueue a celery task to finish the task asynchronously, see TNL-5471
+        log.warning("Persistent Grades: Persistence Error, falling back.\n{}".format(format_exc()))
+
+
 class SubsectionGrade(object):
     """
     Class for Subsection Grades.
@@ -254,8 +272,9 @@ class SubsectionGradeFactory(object):
                 if read_only:
                     self._unsaved_subsection_grades.append(subsection_grade)
                 else:
-                    grade_model = subsection_grade.create_model(self.student)
-                    self._update_saved_subsection_grade(subsection.location, grade_model)
+                    with persistence_safe_fallback():
+                        grade_model = subsection_grade.create_model(self.student)
+                        self._update_saved_subsection_grade(subsection.location, grade_model)
         return subsection_grade
 
     def bulk_create_unsaved(self):
@@ -264,8 +283,9 @@ class SubsectionGradeFactory(object):
         """
         self._log_event(log.warning, u"bulk_create_unsaved")
 
-        SubsectionGrade.bulk_create_models(self.student, self._unsaved_subsection_grades, self.course.id)
-        self._unsaved_subsection_grades = []
+        with persistence_safe_fallback():
+            SubsectionGrade.bulk_create_models(self.student, self._unsaved_subsection_grades, self.course.id)
+            self._unsaved_subsection_grades = []
 
     def update(self, subsection, block_structure=None):
         """

lms/djangoapps/grades/tests/test_new.py

@@ -4,6 +4,7 @@ Test saved subsection grade functionality.
 
 import ddt
 from django.conf import settings
+from django.db.utils import DatabaseError
 from mock import patch
 
 from capa.tests.response_xml_factory import MultipleChoiceResponseXMLFactory
@@ -117,7 +118,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(12):
+                with self.assertNumQueries(14):
                     grade_a = self.subsection_grade_factory.create(self.sequence)
                 self.assertTrue(mock_get_saved_grade.called)
                 self.assertTrue(mock_create_grade.called)
@@ -133,6 +134,30 @@ class SubsectionGradeFactoryTest(GradeTestBase):
         self.assertEqual(grade_a.url_name, grade_b.url_name)
         self.assertEqual(grade_a.all_total, grade_b.all_total)
 
+    @ddt.data(
+        (
+            'lms.djangoapps.grades.new.subsection_grade.SubsectionGrade.create_model',
+            lambda self: self.subsection_grade_factory.create(self.sequence)
+        ),
+        (
+            'lms.djangoapps.grades.new.subsection_grade.SubsectionGrade.bulk_create_models',
+            lambda self: self.subsection_grade_factory.bulk_create_unsaved()
+        ),
+    )
+    @ddt.unpack
+    def test_fallback_handling(self, underlying_method, method_to_test):
+        """
+        Tests that the persistent grades fallback handler functions as expected.
+        """
+        with patch('lms.djangoapps.grades.new.subsection_grade.log') as log_mock:
+            with patch(underlying_method) as underlying:
+                underlying.side_effect = DatabaseError("I'm afraid I can't do that")
+                method_to_test(self)
+                # By making it this far, we implicitly assert "the factory method swallowed the exception correctly"
+                self.assertTrue(
+                    log_mock.warning.call_args_list[0].startswith("Persistent Grades: Persistence Error, falling back.")
+                )
+
     @patch.dict(settings.FEATURES, {'PERSISTENT_GRADES_ENABLED_FOR_ALL_TESTS': False})
     @ddt.data(
         (True, True),

lms/djangoapps/instructor_task/tests/test_tasks_helper.py

@@ -1670,7 +1670,7 @@ class TestCertificateGeneration(InstructorTaskModuleTestCase):
             'failed': 3,
             'skipped': 2
         }
-        with self.assertNumQueries(175):
+        with self.assertNumQueries(191):
             self.assertCertificatesGenerated(task_input, expected_results)
 
         expected_results = {