From 13687e475321b546a4a9e25acfd737d1607e6408 Mon Sep 17 00:00:00 2001 From: Eric Fischer Date: Tue, 23 Aug 2016 15:49:03 -0400 Subject: [PATCH] Grades cleanup Small changes to the grades djangoapp -sets up mock_get_score context handler -fixes an issue with not invalidating lazy scores property -code quality fixes -query count optimization in PersistentSubscetionGrade.create -adds atomic blocks to avoid IntegrityErrors corrupting an entire request --- common/lib/xmodule/xmodule/graders.py | 19 ++++----------- lms/djangoapps/grades/models.py | 23 +++++++++---------- lms/djangoapps/grades/new/subsection_grade.py | 14 +++++++---- lms/djangoapps/grades/tests/test_models.py | 6 ++--- lms/djangoapps/grades/tests/test_new.py | 9 ++++---- lms/djangoapps/grades/tests/test_signals.py | 8 +++---- lms/djangoapps/grades/tests/utils.py | 10 ++++++++ 7 files changed, 46 insertions(+), 43 deletions(-) diff --git a/common/lib/xmodule/xmodule/graders.py b/common/lib/xmodule/xmodule/graders.py index be6aefb34d..c84699df5d 100644 --- a/common/lib/xmodule/xmodule/graders.py +++ b/common/lib/xmodule/xmodule/graders.py @@ -40,22 +40,11 @@ def aggregate_scores(scores, section_name="summary", location=None): total_correct = float_sum(score.earned for score in scores) total_possible = float_sum(score.possible for score in scores) - #regardless of whether or not it is graded - all_total = Score( - total_correct, - total_possible, - False, - section_name, - location, - ) + #regardless of whether it is graded + all_total = Score(total_correct, total_possible, False, section_name, location) + #selecting only graded things - graded_total = Score( - total_correct_graded, - total_possible_graded, - True, - section_name, - location, - ) + graded_total = Score(total_correct_graded, total_possible_graded, True, section_name, location) return all_total, graded_total diff --git a/lms/djangoapps/grades/models.py b/lms/djangoapps/grades/models.py index 6ac8468145..39f6bff548 100644 --- a/lms/djangoapps/grades/models.py +++ b/lms/djangoapps/grades/models.py @@ -12,7 +12,7 @@ import json import logging from operator import attrgetter -from django.db import models +from django.db import models, transaction from django.db.utils import IntegrityError from model_utils.models import TimeStampedModel @@ -107,7 +107,8 @@ class VisibleBlocksQuerySet(models.QuerySet): blocks = BlockRecordSet(blocks) try: - model = self.create_from_blockrecords(blocks) + with transaction.atomic(): + model = self.create_from_blockrecords(blocks) except IntegrityError: # If an integrity error occurs, the VisibleBlocks model we want to # create already exists. The hash is still the correct value. @@ -172,13 +173,10 @@ class PersistentSubsectionGradeQuerySet(models.QuerySet): kwargs['course_id'] = kwargs['usage_key'].course_key visible_blocks_hash = VisibleBlocks.objects.hash_from_blockrecords(blocks=visible_blocks) - grade = self.model( + return super(PersistentSubsectionGradeQuerySet, self).create( visible_blocks_id=visible_blocks_hash, **kwargs ) - grade.full_clean() - grade.save() - return grade class PersistentSubsectionGrade(TimeStampedModel): @@ -244,12 +242,13 @@ class PersistentSubsectionGrade(TimeStampedModel): user_id = kwargs.pop('user_id') usage_key = kwargs.pop('usage_key') try: - grade, is_created = cls.objects.get_or_create( - user_id=user_id, - course_id=usage_key.course_key, - usage_key=usage_key, - defaults=kwargs, - ) + with transaction.atomic(): + grade, is_created = cls.objects.get_or_create( + user_id=user_id, + course_id=usage_key.course_key, + usage_key=usage_key, + defaults=kwargs, + ) except IntegrityError: cls.update_grade(user_id=user_id, usage_key=usage_key, **kwargs) else: diff --git a/lms/djangoapps/grades/new/subsection_grade.py b/lms/djangoapps/grades/new/subsection_grade.py index 781567de23..8d56fcac1b 100644 --- a/lms/djangoapps/grades/new/subsection_grade.py +++ b/lms/djangoapps/grades/new/subsection_grade.py @@ -44,11 +44,15 @@ class SubsectionGrade(object): """ Compute the grade of this subsection for the given student and course. """ - for descendant_key in course_structure.post_order_traversal( - filter_func=possibly_scored, - start_node=self.location, - ): - self._compute_block_score(student, descendant_key, course_structure, scores_client, submissions_scores) + try: + for descendant_key in course_structure.post_order_traversal( + filter_func=possibly_scored, + start_node=self.location, + ): + self._compute_block_score(student, descendant_key, course_structure, scores_client, submissions_scores) + finally: + # self.scores may hold outdated data, force it to refresh on next access + lazy.invalidate(self, 'scores') self.all_total, self.graded_total = graders.aggregate_scores(self.scores, self.display_name, self.location) diff --git a/lms/djangoapps/grades/tests/test_models.py b/lms/djangoapps/grades/tests/test_models.py index f731d3a713..94bcd270f1 100644 --- a/lms/djangoapps/grades/tests/test_models.py +++ b/lms/djangoapps/grades/tests/test_models.py @@ -8,7 +8,7 @@ from hashlib import sha1 import json from mock import patch -from django.core.exceptions import ValidationError +from django.db.utils import IntegrityError from django.test import TestCase from opaque_keys.edx.locator import CourseLocator, BlockUsageLocator @@ -156,7 +156,7 @@ class PersistentSubsectionGradeTest(GradesModelTestCase): usage_key=self.params["usage_key"], ) self.assertEqual(created_grade, read_grade) - with self.assertRaises(ValidationError): + with self.assertRaises(IntegrityError): created_grade = PersistentSubsectionGrade.objects.create(**self.params) def test_create_bad_params(self): @@ -164,7 +164,7 @@ class PersistentSubsectionGradeTest(GradesModelTestCase): Confirms create will fail if params are missing. """ del self.params["earned_graded"] - with self.assertRaises(ValidationError): + with self.assertRaises(IntegrityError): PersistentSubsectionGrade.objects.create(**self.params) def test_course_version_is_optional(self): diff --git a/lms/djangoapps/grades/tests/test_new.py b/lms/djangoapps/grades/tests/test_new.py index 181330401c..e043865045 100644 --- a/lms/djangoapps/grades/tests/test_new.py +++ b/lms/djangoapps/grades/tests/test_new.py @@ -17,6 +17,7 @@ from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from ..models import PersistentSubsectionGrade from ..new.course_grade import CourseGradeFactory from ..new.subsection_grade import SubsectionGrade, SubsectionGradeFactory +from lms.djangoapps.grades.tests.utils import mock_get_score class GradeTestBase(SharedModuleStoreTestCase): @@ -128,7 +129,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(17): + with self.assertNumQueries(19): grade_a = self.subsection_grade_factory.create(self.sequence, self.course_structure, self.course) self.assertTrue(mock_get_saved_grade.called) self.assertTrue(mock_save_grades.called) @@ -180,11 +181,11 @@ class SubsectionGradeTest(GradeTestBase): Assuming the underlying score reporting methods work, test that the score is calculated properly. """ grade = self.subsection_grade_factory.create(self.sequence, self.course_structure, self.course) - with patch('lms.djangoapps.grades.new.subsection_grade.get_score', return_value=(0, 1)): + with mock_get_score(1, 2): # The final 2 parameters are only passed through to our mocked-out get_score method grade.compute(self.request.user, self.course_structure, None, None) - self.assertEqual(grade.all_total.earned, 0) - self.assertEqual(grade.all_total.possible, 1) + self.assertEqual(grade.all_total.earned, 1) + self.assertEqual(grade.all_total.possible, 2) def test_save_and_load(self): """ diff --git a/lms/djangoapps/grades/tests/test_signals.py b/lms/djangoapps/grades/tests/test_signals.py index 517fbec914..5a62cb4fe8 100644 --- a/lms/djangoapps/grades/tests/test_signals.py +++ b/lms/djangoapps/grades/tests/test_signals.py @@ -203,7 +203,7 @@ class ScoreChangedUpdatesSubsectionGradeTest(ModuleStoreTestCase): def test_subsection_grade_updated_on_signal(self, default_store): with self.store.default_store(default_store): self.set_up_course() - with check_mongo_calls(2) and self.assertNumQueries(13): + with check_mongo_calls(2) and self.assertNumQueries(15): recalculate_subsection_grade_handler(None, **self.score_changed_kwargs) @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) @@ -212,7 +212,7 @@ class ScoreChangedUpdatesSubsectionGradeTest(ModuleStoreTestCase): self.set_up_course() 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(13): + with check_mongo_calls(2) and self.assertNumQueries(15): recalculate_subsection_grade_handler(None, **self.score_changed_kwargs) @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) @@ -238,8 +238,8 @@ class ScoreChangedUpdatesSubsectionGradeTest(ModuleStoreTestCase): SCORE_CHANGED.send(sender=None, **self.score_changed_kwargs) @ddt.data( - ('points_possible', 2, 13), - ('points_earned', 2, 13), + ('points_possible', 2, 15), + ('points_earned', 2, 15), ('user', 0, 0), ('course_id', 0, 0), ('usage_id', 0, 0), diff --git a/lms/djangoapps/grades/tests/utils.py b/lms/djangoapps/grades/tests/utils.py index e646d875bb..0397fe005f 100644 --- a/lms/djangoapps/grades/tests/utils.py +++ b/lms/djangoapps/grades/tests/utils.py @@ -13,3 +13,13 @@ def mock_passing_grade(grade_pass='Pass', percent=0.75): with patch('lms.djangoapps.grades.course_grades.summary') as mock_grade: mock_grade.return_value = {'grade': grade_pass, 'percent': percent} yield + + +@contextmanager +def mock_get_score(earned=0, possible=1): + """ + Mocks the get_score function to return a valid grade. + """ + with patch('lms.djangoapps.grades.new.subsection_grade.get_score') as mock_score: + mock_score.return_value = (earned, possible) + yield mock_score