Skip to content

E
edx-platform
Commit b9adb42f by Eric Fischer
Options

Remove Extraneous Subsection Grade DatabaseError protection 

These lines are now being run in async tasks anyways, so we can remove this
decorator and rely on the async task to retry itself. Also cleans up a few
pylint violoations noticed in subsection_grade.py.

Tests have been updated to match new behavior.
parent 48507af2
Showing with 16 additions and 61 deletions
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)
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment