Make Robust Grades Robust Again

As part of the Robust Grades rollout, we expect to see some DatabaseErrors in
various methods that write to the database. Since the success of this write
operation is not needed for the end-user, we just log and swallow the error.

In the future, we'll also enqueue an async task to finish the write operation
that failed.

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 = {