diff --git a/lms/djangoapps/grades/constants.py b/lms/djangoapps/grades/constants.py index 2ac18f3ddb..d11f639797 100644 --- a/lms/djangoapps/grades/constants.py +++ b/lms/djangoapps/grades/constants.py @@ -9,3 +9,4 @@ class ScoreDatabaseTableEnum(object): """ courseware_student_module = 'csm' submissions = 'submissions' + overrides = 'overrides' diff --git a/lms/djangoapps/grades/models.py b/lms/djangoapps/grades/models.py index 0845591103..3f4985e8e0 100644 --- a/lms/djangoapps/grades/models.py +++ b/lms/djangoapps/grades/models.py @@ -695,6 +695,10 @@ class PersistentSubsectionGradeOverride(models.Model): grade = models.OneToOneField(PersistentSubsectionGrade, related_name='override') + # Created/modified timestamps prevent race-conditions when using with async rescoring tasks + created = models.DateTimeField(auto_now_add=True, db_index=True) + modified = models.DateTimeField(auto_now=True, db_index=True) + # earned/possible refers to the number of points achieved and available to achieve. # graded refers to the subset of all problems that are marked as being graded. earned_all_override = models.FloatField(null=True, blank=True) diff --git a/lms/djangoapps/grades/services.py b/lms/djangoapps/grades/services.py index d6f76444d2..a06b4abf6d 100644 --- a/lms/djangoapps/grades/services.py +++ b/lms/djangoapps/grades/services.py @@ -1,5 +1,11 @@ +from datetime import datetime + +import pytz + from opaque_keys.edx.keys import CourseKey, UsageKey -from lms.djangoapps.grades.models import PersistentSubsectionGrade, PersistentSubsectionGradeOverride + +from .constants import ScoreDatabaseTableEnum +from .models import PersistentSubsectionGrade, PersistentSubsectionGradeOverride def _get_key(key_or_id, key_cls): @@ -24,48 +30,93 @@ class GradesService(object): def get_subsection_grade(self, user_id, course_key_or_id, usage_key_or_id): """ Finds and returns the earned subsection grade for user - - Result is a dict of two key value pairs with keys: earned_all and earned_graded. """ course_key = _get_key(course_key_or_id, CourseKey) usage_key = _get_key(usage_key_or_id, UsageKey) + return PersistentSubsectionGrade.objects.get( + user_id=user_id, + course_id=course_key, + usage_key=usage_key + ) + + def get_subsection_grade_override(self, user_id, course_key_or_id, usage_key_or_id): + """ + Finds the subsection grade for user and returns the override for that grade if it exists + + If override does not exist, returns None. If subsection grade does not exist, will raise an exception. + """ + course_key = _get_key(course_key_or_id, CourseKey) + usage_key = _get_key(usage_key_or_id, UsageKey) + + grade = self.get_subsection_grade(user_id, course_key, usage_key) + + try: + return PersistentSubsectionGradeOverride.objects.get( + grade=grade + ) + except PersistentSubsectionGradeOverride.DoesNotExist: + return None + + def override_subsection_grade(self, user_id, course_key_or_id, usage_key_or_id, earned_all=None, + earned_graded=None): + """ + Override subsection grade (the PersistentSubsectionGrade model must already exist) + + Fires off a recalculate_subsection_grade async task to update the PersistentSubsectionGrade table. Will not + override earned_all or earned_graded value if they are None. Both default to None. + """ + from .tasks import recalculate_subsection_grade_v3 # prevent circular import + + course_key = _get_key(course_key_or_id, CourseKey) + usage_key = _get_key(usage_key_or_id, UsageKey) + grade = PersistentSubsectionGrade.objects.get( user_id=user_id, course_id=course_key, usage_key=usage_key ) - return { - 'earned_all': grade.earned_all, - 'earned_graded': grade.earned_graded - } - - def override_subsection_grade(self, user_id, course_key_or_id, usage_key_or_id, earned_all=None, - earned_graded=None): - """ - Override subsection grade (the PersistentSubsectionGrade model must already exist) - - Will not override earned_all or earned_graded value if they are None. Both default to None. - """ - course_key = _get_key(course_key_or_id, CourseKey) - subsection_key = _get_key(usage_key_or_id, UsageKey) - - grade = PersistentSubsectionGrade.objects.get( - user_id=user_id, - course_id=course_key, - usage_key=subsection_key - ) # Create override that will prevent any future updates to grade - PersistentSubsectionGradeOverride.objects.create( + override, _ = PersistentSubsectionGradeOverride.objects.update_or_create( grade=grade, earned_all_override=earned_all, earned_graded_override=earned_graded ) - # Change the grade as it is now - if earned_all is not None: - grade.earned_all = earned_all - if earned_graded is not None: - grade.earned_graded = earned_graded - grade.save() + # Recalculation will call PersistentSubsectionGrade.update_or_create_grade which will use the above override + # to update the grade before writing to the table. + recalculate_subsection_grade_v3.apply_async( + sender=None, + user_id=user_id, + course_id=unicode(course_key), + usage_id=unicode(usage_key), + only_if_higher=False, + expeected_modified=override.modified, + score_db_table=ScoreDatabaseTableEnum.overrides + ) + + def undo_override_subsection_grade(self, user_id, course_key_or_id, usage_key_or_id): + """ + Delete the override subsection grade row (the PersistentSubsectionGrade model must already exist) + + Fires off a recalculate_subsection_grade async task to update the PersistentSubsectionGrade table. + """ + from .tasks import recalculate_subsection_grade_v3 # prevent circular import + + course_key = _get_key(course_key_or_id, CourseKey) + usage_key = _get_key(usage_key_or_id, UsageKey) + + override = self.get_subsection_grade_override(user_id, course_key, usage_key) + override.delete() + + recalculate_subsection_grade_v3.apply_async( + sender=None, + user_id=user_id, + course_id=unicode(course_key), + usage_id=unicode(usage_key), + only_if_higher=False, + expected_modified=datetime.now().replace(tzinfo=pytz.UTC), # Not used when score_deleted=True + score_deleted=True, + score_db_table=ScoreDatabaseTableEnum.overrides + ) diff --git a/lms/djangoapps/grades/tasks.py b/lms/djangoapps/grades/tasks.py index 3e4df7605e..b107a847cb 100644 --- a/lms/djangoapps/grades/tasks.py +++ b/lms/djangoapps/grades/tasks.py @@ -31,6 +31,7 @@ from .constants import ScoreDatabaseTableEnum from .exceptions import DatabaseNotReadyError from .new.course_grade_factory import CourseGradeFactory from .new.subsection_grade_factory import SubsectionGradeFactory +from .services import GradesService from .signals.signals import SUBSECTION_SCORE_CHANGED from .transformer import GradesTransformer @@ -201,8 +202,7 @@ def _has_db_updated_with_new_score(self, scored_block_usage_key, **kwargs): score = get_score(kwargs['user_id'], scored_block_usage_key) found_modified_time = score.modified if score is not None else None - else: - assert kwargs['score_db_table'] == ScoreDatabaseTableEnum.submissions + elif kwargs['score_db_table'] == ScoreDatabaseTableEnum.submissions: score = sub_api.get_score( { "student_id": kwargs['anonymous_user_id'], @@ -212,6 +212,14 @@ def _has_db_updated_with_new_score(self, scored_block_usage_key, **kwargs): } ) found_modified_time = score['created_at'] if score is not None else None + else: + assert kwargs['score_db_table'] == ScoreDatabaseTableEnum.overrides + score = GradesService().get_subsection_grade_override( + user_id=kwargs['user_id'], + course_key_or_id=kwargs['course_id'], + usage_key_or_id=kwargs['usage_id'] + ) + found_modified_time = score.modified if score is not None else None if score is None: # score should be None only if it was deleted. diff --git a/lms/djangoapps/grades/tests/test_services.py b/lms/djangoapps/grades/tests/test_services.py index 7b09eb2918..2ca2ad45bc 100644 --- a/lms/djangoapps/grades/tests/test_services.py +++ b/lms/djangoapps/grades/tests/test_services.py @@ -1,11 +1,17 @@ import ddt +import pytz +from datetime import datetime +from freezegun import freeze_time from lms.djangoapps.grades.models import PersistentSubsectionGrade, PersistentSubsectionGradeOverride from lms.djangoapps.grades.services import GradesService, _get_key +from mock import patch from opaque_keys.edx.keys import CourseKey, UsageKey from student.tests.factories import UserFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory +from ..constants import ScoreDatabaseTableEnum + @ddt.ddt class GradesServiceTests(ModuleStoreTestCase): @@ -29,27 +35,73 @@ class GradesServiceTests(ModuleStoreTestCase): earned_graded=5.0, possible_graded=5.0 ) + self.patcher = patch('lms.djangoapps.grades.tasks.recalculate_subsection_grade_v3.apply_async') + self.mock_recalculate = self.patcher.start() + + def tearDown(self): + self.patcher.stop() + + def subsection_grade_to_dict(self, grade): + return { + 'earned_all': grade.earned_all, + 'earned_graded': grade.earned_graded + } + + def subsection_grade_override_to_dict(self, grade): + return { + 'earned_all_override': grade.earned_all_override, + 'earned_graded_override': grade.earned_graded_override + } def test_get_subsection_grade(self): - self.assertDictEqual(self.service.get_subsection_grade( + self.assertDictEqual(self.subsection_grade_to_dict(self.service.get_subsection_grade( user_id=self.user.id, course_key_or_id=self.course.id, usage_key_or_id=self.subsection.location - ), { + )), { 'earned_all': 6.0, 'earned_graded': 5.0 }) # test with id strings as parameters instead - self.assertDictEqual(self.service.get_subsection_grade( + self.assertDictEqual(self.subsection_grade_to_dict(self.service.get_subsection_grade( user_id=self.user.id, - course_key_or_id=str(self.course.id), - usage_key_or_id=str(self.subsection.location) - ), { + course_key_or_id=unicode(self.course.id), + usage_key_or_id=unicode(self.subsection.location) + )), { 'earned_all': 6.0, 'earned_graded': 5.0 }) + def test_get_subsection_grade_override(self): + override, _ = PersistentSubsectionGradeOverride.objects.update_or_create(grade=self.grade) + + self.assertDictEqual(self.subsection_grade_override_to_dict(self.service.get_subsection_grade_override( + user_id=self.user.id, + course_key_or_id=self.course.id, + usage_key_or_id=self.subsection.location + )), { + 'earned_all_override': override.earned_all_override, + 'earned_graded_override': override.earned_graded_override + }) + + override, _ = PersistentSubsectionGradeOverride.objects.update_or_create( + grade=self.grade, + defaults={ + 'earned_all_override': 9.0 + } + ) + + # test with id strings as parameters instead + self.assertDictEqual(self.subsection_grade_override_to_dict(self.service.get_subsection_grade_override( + user_id=self.user.id, + course_key_or_id=unicode(self.course.id), + usage_key_or_id=unicode(self.subsection.location) + )), { + 'earned_all_override': override.earned_all_override, + 'earned_graded_override': override.earned_graded_override + }) + @ddt.data( [{ 'earned_all': 0.0, @@ -92,14 +144,48 @@ class GradesServiceTests(ModuleStoreTestCase): earned_graded=override['earned_graded'] ) - grade = PersistentSubsectionGrade.objects.get( + override_obj = self.service.get_subsection_grade_override( + self.user.id, + self.course.id, + self.subsection.location + ) + self.assertIsNotNone(override_obj) + self.assertEqual(override_obj.earned_all_override, override['earned_all']) + self.assertEqual(override_obj.earned_graded_override, override['earned_graded']) + + self.mock_recalculate.called_with( + sender=None, user_id=self.user.id, - course_id=self.course.id, - usage_key=self.subsection.location + course_id=unicode(self.course.id), + usage_id=unicode(self.subsection.location), + only_if_higher=False, + expected_modified=override_obj.modified, + score_db_table=ScoreDatabaseTableEnum.overrides ) - self.assertEqual(grade.earned_all, expected['earned_all']) - self.assertEqual(grade.earned_graded, expected['earned_graded']) + @freeze_time('2017-01-01') + def test_undo_override_subsection_grade(self): + override, _ = PersistentSubsectionGradeOverride.objects.update_or_create(grade=self.grade) + + self.service.undo_override_subsection_grade( + user_id=self.user.id, + course_key_or_id=self.course.id, + usage_key_or_id=self.subsection.location, + ) + + override = self.service.get_subsection_grade_override(self.user.id, self.course.id, self.subsection.location) + self.assertIsNone(override) + + self.mock_recalculate.called_with( + sender=None, + user_id=self.user.id, + course_id=unicode(self.course.id), + usage_id=unicode(self.subsection.location), + only_if_higher=False, + expected_modified=datetime.now().replace(tzinfo=pytz.UTC), + score_deleted=True, + score_db_table=ScoreDatabaseTableEnum.overrides + ) @ddt.data( ['edX/DemoX/Demo_Course', CourseKey.from_string('edX/DemoX/Demo_Course'), CourseKey], diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py index 39556b77d2..d27bc8206d 100644 --- a/lms/djangoapps/grades/tests/test_tasks.py +++ b/lms/djangoapps/grades/tests/test_tasks.py @@ -17,6 +17,7 @@ from mock import MagicMock, patch from lms.djangoapps.grades.config.models import PersistentGradesEnabledFlag from lms.djangoapps.grades.constants import ScoreDatabaseTableEnum from lms.djangoapps.grades.models import PersistentCourseGrade, PersistentSubsectionGrade +from lms.djangoapps.grades.services import GradesService from lms.djangoapps.grades.signals.signals import PROBLEM_WEIGHTED_SCORE_CHANGED from lms.djangoapps.grades.tasks import ( RECALCULATE_GRADE_DELAY, @@ -36,6 +37,15 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, check_mongo_calls +class MockGradesService(GradesService): + def __init__(self, mocked_return_value=None): + super(MockGradesService, self).__init__() + self.mocked_return_value = mocked_return_value + + def get_subsection_grade_override(self, user_id, course_key_or_id, usage_key_or_id): + return self.mocked_return_value + + class HasCourseWithProblemsMixin(object): """ Mixin to provide tests with a sample course with graded subsections @@ -153,10 +163,10 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest self.assertEquals(mock_block_structure_create.call_count, 1) @ddt.data( - (ModuleStoreEnum.Type.mongo, 1, 28, True), - (ModuleStoreEnum.Type.mongo, 1, 24, False), - (ModuleStoreEnum.Type.split, 3, 28, True), - (ModuleStoreEnum.Type.split, 3, 24, False), + (ModuleStoreEnum.Type.mongo, 1, 29, True), + (ModuleStoreEnum.Type.mongo, 1, 25, False), + (ModuleStoreEnum.Type.split, 3, 29, True), + (ModuleStoreEnum.Type.split, 3, 25, False), ) @ddt.unpack def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls, create_multiple_subsections): @@ -168,8 +178,8 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest self._apply_recalculate_subsection_grade() @ddt.data( - (ModuleStoreEnum.Type.mongo, 1, 28), - (ModuleStoreEnum.Type.split, 3, 28), + (ModuleStoreEnum.Type.mongo, 1, 29), + (ModuleStoreEnum.Type.split, 3, 29), ) @ddt.unpack def test_query_counts_dont_change_with_more_content(self, default_store, num_mongo_calls, num_sql_calls): @@ -229,8 +239,8 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest self.assertEqual(len(PersistentSubsectionGrade.bulk_read_grades(self.user.id, self.course.id)), 0) @ddt.data( - (ModuleStoreEnum.Type.mongo, 1, 25), - (ModuleStoreEnum.Type.split, 3, 25), + (ModuleStoreEnum.Type.mongo, 1, 26), + (ModuleStoreEnum.Type.split, 3, 26), ) @ddt.unpack def test_persistent_grades_enabled_on_course(self, default_store, num_mongo_queries, num_sql_queries): @@ -264,7 +274,8 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest self._apply_recalculate_subsection_grade() self._assert_retry_called(mock_retry) - @ddt.data(ScoreDatabaseTableEnum.courseware_student_module, ScoreDatabaseTableEnum.submissions) + @ddt.data(ScoreDatabaseTableEnum.courseware_student_module, ScoreDatabaseTableEnum.submissions, + ScoreDatabaseTableEnum.overrides) @patch('lms.djangoapps.grades.tasks.recalculate_subsection_grade_v3.retry') @patch('lms.djangoapps.grades.tasks.log') def test_retry_when_db_not_updated(self, score_db_table, mock_log, mock_retry): @@ -279,10 +290,16 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest self._apply_recalculate_subsection_grade( mock_score=MagicMock(module_type='any_block_type') ) - else: + elif score_db_table == ScoreDatabaseTableEnum.courseware_student_module: self._apply_recalculate_subsection_grade( mock_score=MagicMock(modified=modified_datetime) ) + else: + with patch( + 'lms.djangoapps.grades.tasks.GradesService', + return_value=MockGradesService(mocked_return_value=MagicMock(modified=modified_datetime)) + ): + recalculate_subsection_grade_v3.apply(kwargs=self.recalculate_subsection_grade_kwargs) self._assert_retry_called(mock_retry) self.assertIn( @@ -293,7 +310,8 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest @ddt.data( *itertools.product( (True, False), - (ScoreDatabaseTableEnum.courseware_student_module, ScoreDatabaseTableEnum.submissions), + (ScoreDatabaseTableEnum.courseware_student_module, ScoreDatabaseTableEnum.submissions, + ScoreDatabaseTableEnum.overrides), ) ) @ddt.unpack @@ -310,6 +328,11 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest self._apply_recalculate_subsection_grade( mock_score=MagicMock(module_type='any_block_type') ) + elif score_db_table == ScoreDatabaseTableEnum.overrides: + with patch('lms.djangoapps.grades.tasks.GradesService', + return_value=MockGradesService(mocked_return_value=None)) as mock_service: + mock_service.get_subsection_grade_override.return_value = None + recalculate_subsection_grade_v3.apply(kwargs=self.recalculate_subsection_grade_kwargs) else: self._apply_recalculate_subsection_grade(mock_score=None)