Merge pull request #13993 from edx/efischer/tnl-5471

Remove Extraneous Subsection Grade DatabaseError protection

lms/djangoapps/grades/new/subsection_grade.py

@@ -2,9 +2,6 @@
 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
@@ -14,7 +11,6 @@ from lms.djangoapps.grades.config.models import PersistentGradesEnabledFlag
 from openedx.core.lib.grade_utils import is_score_higher
 from student.models import anonymous_id_for_user
 from submissions import api as submissions_api
-from traceback import format_exc
 from xmodule import block_metadata_utils, graders
 from xmodule.graders import AggregatedScore
 
@@ -22,25 +18,11 @@ from xmodule.graders import AggregatedScore
 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: database_error.safe_fallback.\n{}".format(format_exc()))
-
-
 class SubsectionGrade(object):
     """
     Class for Subsection Grades.
     """
-    def __init__(self, subsection, course):
+    def __init__(self, subsection):
         self.location = subsection.location
         self.display_name = block_metadata_utils.display_name_with_default_escaped(subsection)
         self.url_name = block_metadata_utils.url_name_for_block(subsection)
@@ -223,25 +205,23 @@ class SubsectionGradeFactory(object):
 
         subsection_grade = self._get_bulk_cached_grade(subsection)
         if not subsection_grade:
-            subsection_grade = SubsectionGrade(subsection, self.course).init_from_structure(
+            subsection_grade = SubsectionGrade(subsection).init_from_structure(
                 self.student, self.course_structure, self._submissions_scores, self._csm_scores,
             )
             if PersistentGradesEnabledFlag.feature_enabled(self.course.id):
                 if read_only:
                     self._unsaved_subsection_grades.append(subsection_grade)
                 else:
-                    with persistence_safe_fallback():
-                        grade_model = subsection_grade.create_model(self.student)
-                        self._update_saved_subsection_grade(subsection.location, grade_model)
+                    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):
         """
         Bulk creates all the unsaved subsection_grades to this point.
         """
-        with persistence_safe_fallback():
-            SubsectionGrade.bulk_create_models(self.student, self._unsaved_subsection_grades, self.course.id)
-            self._unsaved_subsection_grades = []
+        SubsectionGrade.bulk_create_models(self.student, self._unsaved_subsection_grades, self.course.id)
+        self._unsaved_subsection_grades = []
 
     def update(self, subsection, only_if_higher=None):
         """
@@ -254,7 +234,7 @@ class SubsectionGradeFactory(object):
 
         self._log_event(log.warning, u"update, subsection: {}".format(subsection.location), subsection)
 
-        calculated_grade = SubsectionGrade(subsection, self.course).init_from_structure(
+        calculated_grade = SubsectionGrade(subsection).init_from_structure(
             self.student, self.course_structure, self._submissions_scores, self._csm_scores,
         )
 
@@ -264,7 +244,7 @@ class SubsectionGradeFactory(object):
             except PersistentSubsectionGrade.DoesNotExist:
                 pass
             else:
-                orig_subsection_grade = SubsectionGrade(subsection, self.course).init_from_model(
+                orig_subsection_grade = SubsectionGrade(subsection).init_from_model(
                     self.student, grade_model, self.course_structure, self._submissions_scores, self._csm_scores,
                 )
                 if not is_score_higher(
@@ -310,7 +290,7 @@ class SubsectionGradeFactory(object):
         saved_subsection_grades = self._get_bulk_cached_subsection_grades()
         subsection_grade = saved_subsection_grades.get(subsection.location)
         if subsection_grade:
-            return SubsectionGrade(subsection, self.course).init_from_model(
+            return SubsectionGrade(subsection).init_from_model(
                 self.student, subsection_grade, self.course_structure, self._submissions_scores, self._csm_scores,
             )
 

lms/djangoapps/grades/tasks.py

@@ -5,7 +5,7 @@ This module contains tasks for asynchronous execution of grade updates.
 from celery import task
 from django.conf import settings
 from django.contrib.auth.models import User
-from django.db.utils import IntegrityError
+from django.db.utils import DatabaseError
 from logging import getLogger
 
 from courseware.model_data import get_score
@@ -122,7 +122,7 @@ def _update_subsection_grades(
                 subsection_grade=subsection_grade,
             )
 
-    except IntegrityError as exc:
+    except DatabaseError as exc:
         raise _retry_recalculate_subsection_grade(
             user_id, course_id, usage_id, only_if_higher, raw_earned, raw_possible, exc,
         )

lms/djangoapps/grades/tests/test_new.py

@@ -214,7 +214,7 @@ class TestSubsectionGradeFactory(ProblemSubmissionTestMixin, GradeTestBase):
                 'lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory._get_bulk_cached_grade',
                 wraps=self.subsection_grade_factory._get_bulk_cached_grade
             ) as mock_get_bulk_cached_grade:
-                with self.assertNumQueries(14):
+                with self.assertNumQueries(12):
                     grade_a = self.subsection_grade_factory.create(self.sequence)
                 self.assertTrue(mock_get_bulk_cached_grade.called)
                 self.assertTrue(mock_create_grade.called)
@@ -254,31 +254,6 @@ class TestSubsectionGradeFactory(ProblemSubmissionTestMixin, GradeTestBase):
         verify_update_if_higher((1, 4), (1, 2))  # previous value was greater
         verify_update_if_higher((3, 4), (3, 4))  # previous value was less
 
-    @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),
@@ -314,7 +289,7 @@ class SubsectionGradeTest(GradeTestBase):
         and that loading saved grades returns the same data.
         """
         # Create a grade that *isn't* saved to the database
-        input_grade = SubsectionGrade(self.sequence, self.course)
+        input_grade = SubsectionGrade(self.sequence)
         input_grade.init_from_structure(
             self.request.user,
             self.course_structure,
@@ -328,7 +303,7 @@ class SubsectionGradeTest(GradeTestBase):
         self.assertEqual(PersistentSubsectionGrade.objects.count(), 1)
 
         # load from db, and ensure output matches input
-        loaded_grade = SubsectionGrade(self.sequence, self.course)
+        loaded_grade = SubsectionGrade(self.sequence)
         saved_model = PersistentSubsectionGrade.read_grade(
             user_id=self.request.user.id,
             usage_key=self.sequence.location,

lms/djangoapps/grades/tests/test_tasks.py

@@ -126,7 +126,7 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
         with self.store.default_store(default_store):
             self.set_up_course()
             self.assertTrue(PersistentGradesEnabledFlag.feature_enabled(self.course.id))
-            with check_mongo_calls(2) and self.assertNumQueries(22 + added_queries):
+            with check_mongo_calls(2) and self.assertNumQueries(20 + added_queries):
                 self._apply_recalculate_subsection_grade()
 
     def test_single_call_to_create_block_structure(self):
@@ -150,7 +150,7 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
             self.assertTrue(PersistentGradesEnabledFlag.feature_enabled(self.course.id))
             ItemFactory.create(parent=self.sequential, category='problem', display_name='problem2')
             ItemFactory.create(parent=self.sequential, category='problem', display_name='problem3')
-            with check_mongo_calls(2) and self.assertNumQueries(22 + added_queries):
+            with check_mongo_calls(2) and self.assertNumQueries(20 + added_queries):
                 self._apply_recalculate_subsection_grade()
 
     @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)