diff --git a/lms/djangoapps/grades/new/subsection_grade.py b/lms/djangoapps/grades/new/subsection_grade.py index 1ab85d3ccf..a2adf11e8f 100644 --- a/lms/djangoapps/grades/new/subsection_grade.py +++ b/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, ) diff --git a/lms/djangoapps/grades/tasks.py b/lms/djangoapps/grades/tasks.py index ef5be52971..4e05677a3d 100644 --- a/lms/djangoapps/grades/tasks.py +++ b/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, ) diff --git a/lms/djangoapps/grades/tests/test_new.py b/lms/djangoapps/grades/tests/test_new.py index 2804dc132a..d0e306ac3e 100644 --- a/lms/djangoapps/grades/tests/test_new.py +++ b/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, diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py index 45dca070d1..943d241f44 100644 --- a/lms/djangoapps/grades/tests/test_tasks.py +++ b/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)