From 4c353d93182f72b1274bdd764b6960576bbc0759 Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Tue, 25 Jul 2017 11:58:08 -0400 Subject: [PATCH 01/29] Create GradesService to override persistent grades --- lms/djangoapps/grades/models.py | 35 ++++++++ lms/djangoapps/grades/services.py | 51 +++++++++++ lms/djangoapps/grades/tests/test_models.py | 9 ++ lms/djangoapps/grades/tests/test_services.py | 91 ++++++++++++++++++++ lms/startup.py | 6 +- 5 files changed, 191 insertions(+), 1 deletion(-) create mode 100644 lms/djangoapps/grades/services.py create mode 100644 lms/djangoapps/grades/tests/test_services.py diff --git a/lms/djangoapps/grades/models.py b/lms/djangoapps/grades/models.py index ea9a7d27f7..0845591103 100644 --- a/lms/djangoapps/grades/models.py +++ b/lms/djangoapps/grades/models.py @@ -411,6 +411,24 @@ class PersistentSubsectionGrade(DeleteGradesMixin, TimeStampedModel): user_id = params.pop('user_id') usage_key = params.pop('usage_key') + # apply grade override if one exists before saving model + try: + override = PersistentSubsectionGradeOverride.objects.get( + grade__user_id=user_id, + grade__course_id=usage_key.course_key, + grade__usage_key=usage_key, + ) + if override.earned_all_override is not None: + params['earned_all'] = override.earned_all_override + if override.possible_all_override is not None: + params['possible_all'] = override.possible_all_override + if override.earned_graded_override is not None: + params['earned_graded'] = override.earned_graded_override + if override.possible_graded_override is not None: + params['possible_graded'] = override.possible_graded_override + except PersistentSubsectionGradeOverride.DoesNotExist: + pass + grade, _ = cls.objects.update_or_create( user_id=user_id, course_id=usage_key.course_key, @@ -666,3 +684,20 @@ class PersistentCourseGrade(DeleteGradesMixin, TimeStampedModel): 'grading_policy_hash': unicode(grade.grading_policy_hash), } ) + + +class PersistentSubsectionGradeOverride(models.Model): + """ + A django model tracking persistent grades overrides at the subsection level. + """ + class Meta(object): + app_label = "grades" + + grade = models.OneToOneField(PersistentSubsectionGrade, related_name='override') + + # 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) + possible_all_override = models.FloatField(null=True, blank=True) + earned_graded_override = models.FloatField(null=True, blank=True) + possible_graded_override = models.FloatField(null=True, blank=True) diff --git a/lms/djangoapps/grades/services.py b/lms/djangoapps/grades/services.py new file mode 100644 index 0000000000..7fec14824d --- /dev/null +++ b/lms/djangoapps/grades/services.py @@ -0,0 +1,51 @@ +from lms.djangoapps.grades.models import PersistentSubsectionGrade, PersistentSubsectionGradeOverride + + +class GradesService(object): + """ + Course grade service + + Provides various functions related to getting, setting, and overriding user grades. + """ + + def get_subsection_grade(self, user_id, course_key_or_id, subsection): + """ + 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. + """ + grade = PersistentSubsectionGrade.objects.get( + user_id=user_id, + course_id=course_key_or_id, + usage_key=subsection + ) + return { + 'earned_all': grade.earned_all, + 'earned_graded': grade.earned_graded + } + + def override_subsection_grade(self, user_id, course_key_or_id, subsection, 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. + """ + grade = PersistentSubsectionGrade.objects.get( + user_id=user_id, + course_id=course_key_or_id, + usage_key=subsection + ) + + # Create override that will prevent any future updates to grade + PersistentSubsectionGradeOverride.objects.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() diff --git a/lms/djangoapps/grades/tests/test_models.py b/lms/djangoapps/grades/tests/test_models.py index 5ea10805db..39e31e273b 100644 --- a/lms/djangoapps/grades/tests/test_models.py +++ b/lms/djangoapps/grades/tests/test_models.py @@ -23,6 +23,7 @@ from lms.djangoapps.grades.models import ( BlockRecordList, PersistentCourseGrade, PersistentSubsectionGrade, + PersistentSubsectionGradeOverride, VisibleBlocks ) from track.event_transaction_utils import get_event_transaction_id, get_event_transaction_type @@ -306,6 +307,14 @@ class PersistentSubsectionGradeTest(GradesModelTestCase): grade = PersistentSubsectionGrade.create_grade(**self.params) self._assert_tracker_emitted_event(tracker_mock, grade) + def test_grade_override(self): + grade = PersistentSubsectionGrade.create_grade(**self.params) + override = PersistentSubsectionGradeOverride(grade=grade, earned_all_override=0.0, earned_graded_override=0.0) + override.save() + grade = PersistentSubsectionGrade.update_or_create_grade(**self.params) + self.assertEqual(grade.earned_all, 0.0) + self.assertEqual(grade.earned_graded, 0.0) + def _assert_tracker_emitted_event(self, tracker_mock, grade): """ Helper function to ensure that the mocked event tracker diff --git a/lms/djangoapps/grades/tests/test_services.py b/lms/djangoapps/grades/tests/test_services.py new file mode 100644 index 0000000000..223440cf77 --- /dev/null +++ b/lms/djangoapps/grades/tests/test_services.py @@ -0,0 +1,91 @@ +import ddt +from lms.djangoapps.grades.models import PersistentSubsectionGrade, PersistentSubsectionGradeOverride +from lms.djangoapps.grades.services import GradesService +from student.tests.factories import UserFactory +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory + + +@ddt.ddt +class GradesServiceTests(ModuleStoreTestCase): + """ + Tests for the Grades service + """ + def setUp(self, **kwargs): + super(GradesServiceTests, self).setUp() + self.service = GradesService() + self.course = CourseFactory.create(org='edX', number='DemoX', display_name='Demo_Course') + self.subsection = ItemFactory.create(parent=self.course, category="subsection", display_name="Subsection") + self.user = UserFactory() + self.grade = PersistentSubsectionGrade.update_or_create_grade( + user_id=self.user.id, + course_id=self.course.id, + usage_key=self.subsection.location, + first_attempted=None, + visible_blocks=[], + earned_all=6.0, + possible_all=6.0, + earned_graded=5.0, + possible_graded=5.0 + ) + + def test_get_subsection_grade(self): + self.assertDictEqual(self.service.get_subsection_grade( + user_id=self.user.id, + course_key_or_id=self.course.id, + subsection=self.subsection.location + ), { + 'earned_all': 6.0, + 'earned_graded': 5.0 + }) + + @ddt.data( + [{ + 'earned_all': 0.0, + 'earned_graded': 0.0 + }, { + 'earned_all': 0.0, + 'earned_graded': 0.0 + }], + [{ + 'earned_all': 0.0, + 'earned_graded': None + }, { + 'earned_all': 0.0, + 'earned_graded': 5.0 + }], + [{ + 'earned_all': None, + 'earned_graded': None + }, { + 'earned_all': 6.0, + 'earned_graded': 5.0 + }], + [{ + 'earned_all': 3.0, + 'earned_graded': 2.0 + }, { + 'earned_all': 3.0, + 'earned_graded': 2.0 + }], + ) + @ddt.unpack + def test_override_subsection_grade(self, override, expected): + PersistentSubsectionGradeOverride.objects.all().delete() # clear out all previous overrides + + self.service.override_subsection_grade( + user_id=self.user.id, + course_key_or_id=self.course.id, + subsection=self.subsection.location, + earned_all=override['earned_all'], + earned_graded=override['earned_graded'] + ) + + grade = PersistentSubsectionGrade.objects.get( + user_id=self.user.id, + course_id=self.course.id, + usage_key=self.subsection.location + ) + + self.assertEqual(grade.earned_all, expected['earned_all']) + self.assertEqual(grade.earned_graded, expected['earned_graded']) diff --git a/lms/startup.py b/lms/startup.py index 5aae2949c6..4a928edb02 100644 --- a/lms/startup.py +++ b/lms/startup.py @@ -63,18 +63,22 @@ def run(): analytics.write_key = settings.LMS_SEGMENT_KEY # register any dependency injections that we need to support in edx_proctoring - # right now edx_proctoring is dependent on the openedx.core.djangoapps.credit + # right now edx_proctoring is dependent on the openedx.core.djangoapps.credit and + # lms.djangoapps.grades if settings.FEATURES.get('ENABLE_SPECIAL_EXAMS'): # Import these here to avoid circular dependencies of the form: # edx-platform app --> DRF --> django translation --> edx-platform app from edx_proctoring.runtime import set_runtime_service from lms.djangoapps.instructor.services import InstructorService from openedx.core.djangoapps.credit.services import CreditService + from lms.djangoapps.grades.services import GradesService set_runtime_service('credit', CreditService()) # register InstructorService (for deleting student attempts and user staff access roles) set_runtime_service('instructor', InstructorService()) + set_runtime_service('grades', GradesService()) + # In order to allow modules to use a handler url, we need to # monkey-patch the x_module library. # TODO: Remove this code when Runtimes are no longer created by modulestores From e61940652649475e2351e22ba1a5f8fddd538eb9 Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Tue, 25 Jul 2017 13:18:34 -0400 Subject: [PATCH 02/29] Temporarily install edx-proctoring@EDUCATOR-927 --- requirements/edx/github.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index f446535686..5a5f09d934 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -86,7 +86,7 @@ git+https://github.com/edx/xblock-utils.git@v1.0.5#egg=xblock-utils==1.0.5 -e git+https://github.com/edx-solutions/xblock-google-drive.git@138e6fa0bf3a2013e904a085b9fed77dab7f3f21#egg=xblock-google-drive git+https://github.com/edx/edx-user-state-client.git@1.0.1#egg=edx-user-state-client==1.0.1 git+https://github.com/edx/xblock-lti-consumer.git@v1.1.5#egg=lti_consumer-xblock==1.1.5 -git+https://github.com/edx/edx-proctoring.git@0.18.2#egg=edx-proctoring==0.18.2 +git+https://github.com/edx/edx-proctoring.git@EDUCATOR-927 # Third Party XBlocks git+https://github.com/open-craft/xblock-poll@v1.2.7#egg=xblock-poll==1.2.7 From cf39bef74fa28cc60deb76088e165e25e00c0de4 Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Wed, 26 Jul 2017 15:49:26 -0400 Subject: [PATCH 03/29] GradesService: accept both string id or opaque key --- lms/djangoapps/grades/services.py | 32 ++++++++++++++++---- lms/djangoapps/grades/tests/test_services.py | 31 +++++++++++++++++-- 2 files changed, 54 insertions(+), 9 deletions(-) diff --git a/lms/djangoapps/grades/services.py b/lms/djangoapps/grades/services.py index 7fec14824d..d6f76444d2 100644 --- a/lms/djangoapps/grades/services.py +++ b/lms/djangoapps/grades/services.py @@ -1,6 +1,19 @@ +from opaque_keys.edx.keys import CourseKey, UsageKey from lms.djangoapps.grades.models import PersistentSubsectionGrade, PersistentSubsectionGradeOverride +def _get_key(key_or_id, key_cls): + """ + Helper method to get a course/usage key either from a string or a key_cls, + where the key_cls (CourseKey or UsageKey) will simply be returned. + """ + return ( + key_cls.from_string(key_or_id) + if isinstance(key_or_id, basestring) + else key_or_id + ) + + class GradesService(object): """ Course grade service @@ -8,32 +21,39 @@ class GradesService(object): Provides various functions related to getting, setting, and overriding user grades. """ - def get_subsection_grade(self, user_id, course_key_or_id, subsection): + 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) + grade = PersistentSubsectionGrade.objects.get( user_id=user_id, - course_id=course_key_or_id, - usage_key=subsection + 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, subsection, earned_all=None, earned_graded=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) 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_or_id, - usage_key=subsection + course_id=course_key, + usage_key=subsection_key ) # Create override that will prevent any future updates to grade diff --git a/lms/djangoapps/grades/tests/test_services.py b/lms/djangoapps/grades/tests/test_services.py index 223440cf77..7b09eb2918 100644 --- a/lms/djangoapps/grades/tests/test_services.py +++ b/lms/djangoapps/grades/tests/test_services.py @@ -1,6 +1,7 @@ import ddt from lms.djangoapps.grades.models import PersistentSubsectionGrade, PersistentSubsectionGradeOverride -from lms.djangoapps.grades.services import GradesService +from lms.djangoapps.grades.services import GradesService, _get_key +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 @@ -33,7 +34,17 @@ class GradesServiceTests(ModuleStoreTestCase): self.assertDictEqual(self.service.get_subsection_grade( user_id=self.user.id, course_key_or_id=self.course.id, - subsection=self.subsection.location + 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( + user_id=self.user.id, + course_key_or_id=str(self.course.id), + usage_key_or_id=str(self.subsection.location) ), { 'earned_all': 6.0, 'earned_graded': 5.0 @@ -76,7 +87,7 @@ class GradesServiceTests(ModuleStoreTestCase): self.service.override_subsection_grade( user_id=self.user.id, course_key_or_id=self.course.id, - subsection=self.subsection.location, + usage_key_or_id=self.subsection.location, earned_all=override['earned_all'], earned_graded=override['earned_graded'] ) @@ -89,3 +100,17 @@ class GradesServiceTests(ModuleStoreTestCase): self.assertEqual(grade.earned_all, expected['earned_all']) self.assertEqual(grade.earned_graded, expected['earned_graded']) + + @ddt.data( + ['edX/DemoX/Demo_Course', CourseKey.from_string('edX/DemoX/Demo_Course'), CourseKey], + ['course-v1:edX+DemoX+Demo_Course', CourseKey.from_string('course-v1:edX+DemoX+Demo_Course'), CourseKey], + [CourseKey.from_string('course-v1:edX+DemoX+Demo_Course'), + CourseKey.from_string('course-v1:edX+DemoX+Demo_Course'), CourseKey], + ['block-v1:edX+DemoX+Demo_Course+type@sequential+block@workflow', + UsageKey.from_string('block-v1:edX+DemoX+Demo_Course+type@sequential+block@workflow'), UsageKey], + [UsageKey.from_string('block-v1:edX+DemoX+Demo_Course+type@sequential+block@workflow'), + UsageKey.from_string('block-v1:edX+DemoX+Demo_Course+type@sequential+block@workflow'), UsageKey], + ) + @ddt.unpack + def test_get_key(self, input_key, output_key, key_cls): + self.assertEqual(_get_key(input_key, key_cls), output_key) From e4a9bef8d5f77d7c60603edeb6da2f6cd3cffff3 Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Thu, 27 Jul 2017 18:27:56 -0400 Subject: [PATCH 04/29] Trigger recalculate subsection, undo override --- lms/djangoapps/grades/constants.py | 1 + lms/djangoapps/grades/models.py | 4 + lms/djangoapps/grades/services.py | 111 ++++++++++++++----- lms/djangoapps/grades/tasks.py | 12 +- lms/djangoapps/grades/tests/test_services.py | 108 ++++++++++++++++-- lms/djangoapps/grades/tests/test_tasks.py | 45 ++++++-- 6 files changed, 227 insertions(+), 54 deletions(-) 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) From f41cf12134ed85dfdd7d522bbb8119988c4fd50a Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Fri, 28 Jul 2017 15:51:49 -0400 Subject: [PATCH 05/29] Only attempt to delete grade override if exists --- lms/djangoapps/grades/services.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/grades/services.py b/lms/djangoapps/grades/services.py index a06b4abf6d..055312aee9 100644 --- a/lms/djangoapps/grades/services.py +++ b/lms/djangoapps/grades/services.py @@ -100,7 +100,8 @@ class GradesService(object): """ 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. + Fires off a recalculate_subsection_grade async task to update the PersistentSubsectionGrade table. If the + override does not exist, no error is raised, it just triggers the recalculation. """ from .tasks import recalculate_subsection_grade_v3 # prevent circular import @@ -108,7 +109,9 @@ class GradesService(object): usage_key = _get_key(usage_key_or_id, UsageKey) override = self.get_subsection_grade_override(user_id, course_key, usage_key) - override.delete() + # Older rejected exam attempts that transition to verified might not have an override created + if override is not None: + override.delete() recalculate_subsection_grade_v3.apply_async( sender=None, From 26fcbefee171f8d56504a7eba121027f0c5be8b5 Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Fri, 28 Jul 2017 16:57:25 -0400 Subject: [PATCH 06/29] Add migration for new overrides table --- .../0013_persistentsubsectiongradeoverride.py | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 lms/djangoapps/grades/migrations/0013_persistentsubsectiongradeoverride.py diff --git a/lms/djangoapps/grades/migrations/0013_persistentsubsectiongradeoverride.py b/lms/djangoapps/grades/migrations/0013_persistentsubsectiongradeoverride.py new file mode 100644 index 0000000000..b61db4c3e8 --- /dev/null +++ b/lms/djangoapps/grades/migrations/0013_persistentsubsectiongradeoverride.py @@ -0,0 +1,27 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('grades', '0012_computegradessetting'), + ] + + operations = [ + migrations.CreateModel( + name='PersistentSubsectionGradeOverride', + fields=[ + ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)), + ('created', models.DateTimeField(auto_now_add=True, db_index=True)), + ('modified', models.DateTimeField(auto_now=True, db_index=True)), + ('earned_all_override', models.FloatField(null=True, blank=True)), + ('possible_all_override', models.FloatField(null=True, blank=True)), + ('earned_graded_override', models.FloatField(null=True, blank=True)), + ('possible_graded_override', models.FloatField(null=True, blank=True)), + ('grade', models.OneToOneField(related_name='override', to='grades.PersistentSubsectionGrade')), + ], + ), + ] From 6b97c1bdcb7d7152c7c0a14833caadbf3aa5ad04 Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Mon, 31 Jul 2017 11:22:56 -0400 Subject: [PATCH 07/29] Add a rel_db_type to UnsignedBigIntAutoField --- lms/djangoapps/coursewarehistoryextended/fields.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/lms/djangoapps/coursewarehistoryextended/fields.py b/lms/djangoapps/coursewarehistoryextended/fields.py index a001543d31..795449535a 100644 --- a/lms/djangoapps/coursewarehistoryextended/fields.py +++ b/lms/djangoapps/coursewarehistoryextended/fields.py @@ -23,3 +23,13 @@ class UnsignedBigIntAutoField(AutoField): return "BIGSERIAL" else: return None + + def rel_db_type(self, connection): + if connection.settings_dict['ENGINE'] == 'django.db.backends.mysql': + return "bigint UNSIGNED" + elif connection.settings_dict['ENGINE'] == 'django.db.backends.sqlite3': + return "integer" + elif connection.settings_dict['ENGINE'] == 'django.db.backends.postgresql_psycopg2': + return "BIGSERIAL" + else: + return None From e8cb7b77387e7546c7f08c7b7ed2a0daf7704311 Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Mon, 31 Jul 2017 11:57:20 -0400 Subject: [PATCH 08/29] Custom one-to-one field for ref to custom id field --- .../coursewarehistoryextended/fields.py | 19 +++++++++++++++++++ .../0013_persistentsubsectiongradeoverride.py | 4 +++- lms/djangoapps/grades/models.py | 4 ++-- 3 files changed, 24 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/coursewarehistoryextended/fields.py b/lms/djangoapps/coursewarehistoryextended/fields.py index 795449535a..ee5d71a1e5 100644 --- a/lms/djangoapps/coursewarehistoryextended/fields.py +++ b/lms/djangoapps/coursewarehistoryextended/fields.py @@ -3,6 +3,7 @@ Custom fields for use in the coursewarehistoryextended django app. """ from django.db.models.fields import AutoField +from django.db.models.fields.related import OneToOneField class UnsignedBigIntAutoField(AutoField): @@ -24,6 +25,7 @@ class UnsignedBigIntAutoField(AutoField): else: return None + # rel_db_type was added in Django 1.10. For versions before, use UnsignedBigIntOneToOneField. def rel_db_type(self, connection): if connection.settings_dict['ENGINE'] == 'django.db.backends.mysql': return "bigint UNSIGNED" @@ -33,3 +35,20 @@ class UnsignedBigIntAutoField(AutoField): return "BIGSERIAL" else: return None + + +class UnsignedBigIntOneToOneField(OneToOneField): + """ + An unsigned 8-byte integer one-to-one foreign key to a unsigned 8-byte integer id field. + + Should only be necessary for versions of Django < 1.10. + """ + def db_type(self, connection): + if connection.settings_dict['ENGINE'] == 'django.db.backends.mysql': + return "bigint UNSIGNED" + elif connection.settings_dict['ENGINE'] == 'django.db.backends.sqlite3': + return "integer" + elif connection.settings_dict['ENGINE'] == 'django.db.backends.postgresql_psycopg2': + return "BIGSERIAL" + else: + return None diff --git a/lms/djangoapps/grades/migrations/0013_persistentsubsectiongradeoverride.py b/lms/djangoapps/grades/migrations/0013_persistentsubsectiongradeoverride.py index b61db4c3e8..ec060e366e 100644 --- a/lms/djangoapps/grades/migrations/0013_persistentsubsectiongradeoverride.py +++ b/lms/djangoapps/grades/migrations/0013_persistentsubsectiongradeoverride.py @@ -3,6 +3,8 @@ from __future__ import unicode_literals from django.db import migrations, models +from coursewarehistoryextended.fields import UnsignedBigIntOneToOneField + class Migration(migrations.Migration): @@ -21,7 +23,7 @@ class Migration(migrations.Migration): ('possible_all_override', models.FloatField(null=True, blank=True)), ('earned_graded_override', models.FloatField(null=True, blank=True)), ('possible_graded_override', models.FloatField(null=True, blank=True)), - ('grade', models.OneToOneField(related_name='override', to='grades.PersistentSubsectionGrade')), + ('grade', UnsignedBigIntOneToOneField(related_name='override', to='grades.PersistentSubsectionGrade')), ], ), ] diff --git a/lms/djangoapps/grades/models.py b/lms/djangoapps/grades/models.py index 3f4985e8e0..d11a6bdb35 100644 --- a/lms/djangoapps/grades/models.py +++ b/lms/djangoapps/grades/models.py @@ -20,7 +20,7 @@ from lazy import lazy from model_utils.models import TimeStampedModel from opaque_keys.edx.keys import CourseKey, UsageKey -from coursewarehistoryextended.fields import UnsignedBigIntAutoField +from coursewarehistoryextended.fields import UnsignedBigIntAutoField, UnsignedBigIntOneToOneField from eventtracking import tracker from openedx.core.djangoapps.xmodule_django.models import CourseKeyField, UsageKeyField from request_cache import get_cache @@ -693,7 +693,7 @@ class PersistentSubsectionGradeOverride(models.Model): class Meta(object): app_label = "grades" - grade = models.OneToOneField(PersistentSubsectionGrade, related_name='override') + grade = UnsignedBigIntOneToOneField(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) From dc484b8099d53acd0e8d93b681a5d7e225a9c048 Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Mon, 31 Jul 2017 12:27:26 -0400 Subject: [PATCH 09/29] Fix the expected_timestamp recalculate arg --- lms/djangoapps/grades/services.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/grades/services.py b/lms/djangoapps/grades/services.py index 055312aee9..ac5a516d5d 100644 --- a/lms/djangoapps/grades/services.py +++ b/lms/djangoapps/grades/services.py @@ -3,6 +3,7 @@ from datetime import datetime import pytz from opaque_keys.edx.keys import CourseKey, UsageKey +from util.date_utils import to_timestamp from .constants import ScoreDatabaseTableEnum from .models import PersistentSubsectionGrade, PersistentSubsectionGradeOverride @@ -92,7 +93,7 @@ class GradesService(object): course_id=unicode(course_key), usage_id=unicode(usage_key), only_if_higher=False, - expeected_modified=override.modified, + expected_modified=to_timestamp(override.modified), score_db_table=ScoreDatabaseTableEnum.overrides ) From 735824e2cde419bbc22655d41bed3964309ceca5 Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Mon, 31 Jul 2017 16:21:13 -0400 Subject: [PATCH 10/29] Fix recalculate async kwargs --- lms/djangoapps/grades/services.py | 33 +++++++++++++++++-------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/lms/djangoapps/grades/services.py b/lms/djangoapps/grades/services.py index ac5a516d5d..58117e0496 100644 --- a/lms/djangoapps/grades/services.py +++ b/lms/djangoapps/grades/services.py @@ -88,13 +88,14 @@ class GradesService(object): # 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, - expected_modified=to_timestamp(override.modified), - score_db_table=ScoreDatabaseTableEnum.overrides + kwargs=dict( + user_id=user_id, + course_id=unicode(course_key), + usage_id=unicode(usage_key), + only_if_higher=False, + expected_modified=to_timestamp(override.modified), + score_db_table=ScoreDatabaseTableEnum.overrides + ) ) def undo_override_subsection_grade(self, user_id, course_key_or_id, usage_key_or_id): @@ -115,12 +116,14 @@ class GradesService(object): 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 + kwargs=dict( + user_id=user_id, + course_id=unicode(course_key), + usage_id=unicode(usage_key), + only_if_higher=False, + # Not used when score_deleted=True: + expected_modified=to_timestamp(datetime.now().replace(tzinfo=pytz.UTC)), + score_deleted=True, + score_db_table=ScoreDatabaseTableEnum.overrides + ) ) From 750f30b273b4639d9adf0f391b4e5c91c1e5b0ea Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Mon, 31 Jul 2017 16:52:42 -0400 Subject: [PATCH 11/29] Pass event transaction id & type to recalculate --- lms/djangoapps/grades/services.py | 10 ++++++++++ lms/djangoapps/grades/signals/handlers.py | 14 ++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/lms/djangoapps/grades/services.py b/lms/djangoapps/grades/services.py index 58117e0496..25bf65e29b 100644 --- a/lms/djangoapps/grades/services.py +++ b/lms/djangoapps/grades/services.py @@ -3,10 +3,12 @@ from datetime import datetime import pytz from opaque_keys.edx.keys import CourseKey, UsageKey +from track.event_transaction_utils import create_new_event_transaction_id, set_event_transaction_type from util.date_utils import to_timestamp from .constants import ScoreDatabaseTableEnum from .models import PersistentSubsectionGrade, PersistentSubsectionGradeOverride +from .signals.handlers import SUBSECTION_RESCORE_EVENT_TYPE def _get_key(key_or_id, key_cls): @@ -87,6 +89,8 @@ class GradesService(object): # Recalculation will call PersistentSubsectionGrade.update_or_create_grade which will use the above override # to update the grade before writing to the table. + event_transaction_id = create_new_event_transaction_id() + set_event_transaction_type(SUBSECTION_RESCORE_EVENT_TYPE) recalculate_subsection_grade_v3.apply_async( kwargs=dict( user_id=user_id, @@ -94,6 +98,8 @@ class GradesService(object): usage_id=unicode(usage_key), only_if_higher=False, expected_modified=to_timestamp(override.modified), + event_transaction_id=unicode(event_transaction_id), + event_transaction_type=SUBSECTION_RESCORE_EVENT_TYPE, score_db_table=ScoreDatabaseTableEnum.overrides ) ) @@ -115,6 +121,8 @@ class GradesService(object): if override is not None: override.delete() + event_transaction_id = create_new_event_transaction_id() + set_event_transaction_type(SUBSECTION_RESCORE_EVENT_TYPE) recalculate_subsection_grade_v3.apply_async( kwargs=dict( user_id=user_id, @@ -124,6 +132,8 @@ class GradesService(object): # Not used when score_deleted=True: expected_modified=to_timestamp(datetime.now().replace(tzinfo=pytz.UTC)), score_deleted=True, + event_transaction_id=unicode(event_transaction_id), + event_transaction_type=SUBSECTION_RESCORE_EVENT_TYPE, score_db_table=ScoreDatabaseTableEnum.overrides ) ) diff --git a/lms/djangoapps/grades/signals/handlers.py b/lms/djangoapps/grades/signals/handlers.py index f29fd88135..539b20e225 100644 --- a/lms/djangoapps/grades/signals/handlers.py +++ b/lms/djangoapps/grades/signals/handlers.py @@ -38,6 +38,7 @@ log = getLogger(__name__) # define values to be used in grading events GRADES_RESCORE_EVENT_TYPE = 'edx.grades.problem.rescored' PROBLEM_SUBMITTED_EVENT_TYPE = 'edx.grades.problem.submitted' +SUBSECTION_RESCORE_EVENT_TYPE = 'edx.grades.subsection.rescored' @receiver(score_set) @@ -292,3 +293,16 @@ def _emit_event(kwargs): 'event_transaction_type': unicode(root_type), } ) + + if root_type in [SUBSECTION_RESCORE_EVENT_TYPE]: + tracker.emit( + unicode(SUBSECTION_RESCORE_EVENT_TYPE), + { + 'course_id': unicode(kwargs['course_id']), + 'user_id': unicode(kwargs['user_id']), + 'problem_id': unicode(kwargs['usage_id']), + 'only_if_higher': kwargs.get('only_if_higher'), + 'event_transaction_id': unicode(get_event_transaction_id()), + 'event_transaction_type': unicode(root_type), + } + ) From 08e04dbd633486be0e4e86f19149bb5b0063fe69 Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Mon, 31 Jul 2017 17:25:41 -0400 Subject: [PATCH 12/29] Fix circular import of signals from services --- lms/djangoapps/grades/services.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/grades/services.py b/lms/djangoapps/grades/services.py index 25bf65e29b..90f79ba8dd 100644 --- a/lms/djangoapps/grades/services.py +++ b/lms/djangoapps/grades/services.py @@ -8,7 +8,6 @@ from util.date_utils import to_timestamp from .constants import ScoreDatabaseTableEnum from .models import PersistentSubsectionGrade, PersistentSubsectionGradeOverride -from .signals.handlers import SUBSECTION_RESCORE_EVENT_TYPE def _get_key(key_or_id, key_cls): @@ -69,7 +68,9 @@ class GradesService(object): 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 + # prevent circular imports: + from .signals.handlers import SUBSECTION_RESCORE_EVENT_TYPE + from .tasks import recalculate_subsection_grade_v3 course_key = _get_key(course_key_or_id, CourseKey) usage_key = _get_key(usage_key_or_id, UsageKey) @@ -111,7 +112,9 @@ class GradesService(object): Fires off a recalculate_subsection_grade async task to update the PersistentSubsectionGrade table. If the override does not exist, no error is raised, it just triggers the recalculation. """ - from .tasks import recalculate_subsection_grade_v3 # prevent circular import + # prevent circular imports: + from .signals.handlers import SUBSECTION_RESCORE_EVENT_TYPE + from .tasks import recalculate_subsection_grade_v3 course_key = _get_key(course_key_or_id, CourseKey) usage_key = _get_key(usage_key_or_id, UsageKey) From b63fc6251acfa769397b4d2973fd04cb88298f6c Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Mon, 31 Jul 2017 17:48:46 -0400 Subject: [PATCH 13/29] The kwargs are still wrong... --- lms/djangoapps/grades/services.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/grades/services.py b/lms/djangoapps/grades/services.py index 90f79ba8dd..574d3738ed 100644 --- a/lms/djangoapps/grades/services.py +++ b/lms/djangoapps/grades/services.py @@ -98,7 +98,8 @@ class GradesService(object): course_id=unicode(course_key), usage_id=unicode(usage_key), only_if_higher=False, - expected_modified=to_timestamp(override.modified), + expected_modified_time=to_timestamp(override.modified), + score_deleted=True, event_transaction_id=unicode(event_transaction_id), event_transaction_type=SUBSECTION_RESCORE_EVENT_TYPE, score_db_table=ScoreDatabaseTableEnum.overrides @@ -133,7 +134,7 @@ class GradesService(object): usage_id=unicode(usage_key), only_if_higher=False, # Not used when score_deleted=True: - expected_modified=to_timestamp(datetime.now().replace(tzinfo=pytz.UTC)), + expected_modified_time=to_timestamp(datetime.now().replace(tzinfo=pytz.UTC)), score_deleted=True, event_transaction_id=unicode(event_transaction_id), event_transaction_type=SUBSECTION_RESCORE_EVENT_TYPE, From 5276070e0826c3ab1f584f54d4cb81e4df70bbc2 Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Mon, 31 Jul 2017 17:51:26 -0400 Subject: [PATCH 14/29] Bad copy and paste --- lms/djangoapps/grades/services.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/grades/services.py b/lms/djangoapps/grades/services.py index 574d3738ed..387d86521d 100644 --- a/lms/djangoapps/grades/services.py +++ b/lms/djangoapps/grades/services.py @@ -99,7 +99,7 @@ class GradesService(object): usage_id=unicode(usage_key), only_if_higher=False, expected_modified_time=to_timestamp(override.modified), - score_deleted=True, + score_deleted=False, event_transaction_id=unicode(event_transaction_id), event_transaction_type=SUBSECTION_RESCORE_EVENT_TYPE, score_db_table=ScoreDatabaseTableEnum.overrides From c51c47f52421a4d2d5228c204ba1425223b58b4b Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Mon, 31 Jul 2017 18:41:32 -0400 Subject: [PATCH 15/29] Fix tests --- lms/djangoapps/grades/tests/test_services.py | 58 +++++++++++++------- 1 file changed, 38 insertions(+), 20 deletions(-) diff --git a/lms/djangoapps/grades/tests/test_services.py b/lms/djangoapps/grades/tests/test_services.py index 2ca2ad45bc..f6e8a05c2c 100644 --- a/lms/djangoapps/grades/tests/test_services.py +++ b/lms/djangoapps/grades/tests/test_services.py @@ -4,9 +4,11 @@ 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 lms.djangoapps.grades.signals.handlers import SUBSECTION_RESCORE_EVENT_TYPE from mock import patch from opaque_keys.edx.keys import CourseKey, UsageKey from student.tests.factories import UserFactory +from util.date_utils import to_timestamp from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory @@ -35,11 +37,18 @@ 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() + self.recalc_patcher = patch('lms.djangoapps.grades.tasks.recalculate_subsection_grade_v3.apply_async') + self.mock_recalculate = self.recalc_patcher.start() + self.id_patcher = patch('lms.djangoapps.grades.services.create_new_event_transaction_id') + self.mock_create_id = self.id_patcher.start() + self.mock_create_id.return_value = 1 + self.type_patcher = patch('lms.djangoapps.grades.services.set_event_transaction_type') + self.mock_set_type = self.type_patcher.start() def tearDown(self): - self.patcher.stop() + self.recalc_patcher.stop() + self.id_patcher.stop() + self.type_patcher.stop() def subsection_grade_to_dict(self, grade): return { @@ -153,14 +162,19 @@ class GradesServiceTests(ModuleStoreTestCase): 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=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.assertDictEqual( + self.mock_recalculate.call_args[1]['kwargs'], + dict( + user_id=self.user.id, + course_id=unicode(self.course.id), + usage_id=unicode(self.subsection.location), + only_if_higher=False, + expected_modified_time=to_timestamp(override_obj.modified), + score_deleted=False, + event_transaction_id=unicode(self.mock_create_id.return_value), + event_transaction_type=SUBSECTION_RESCORE_EVENT_TYPE, + score_db_table=ScoreDatabaseTableEnum.overrides + ) ) @freeze_time('2017-01-01') @@ -176,15 +190,19 @@ class GradesServiceTests(ModuleStoreTestCase): 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 + self.assertDictEqual( + self.mock_recalculate.call_args[1]['kwargs'], + dict( + user_id=self.user.id, + course_id=unicode(self.course.id), + usage_id=unicode(self.subsection.location), + only_if_higher=False, + expected_modified_time=to_timestamp(datetime.now().replace(tzinfo=pytz.UTC)), + score_deleted=True, + event_transaction_id=unicode(self.mock_create_id.return_value), + event_transaction_type=SUBSECTION_RESCORE_EVENT_TYPE, + score_db_table=ScoreDatabaseTableEnum.overrides + ) ) @ddt.data( From f92c96ef05acad66940e693255d39e5c807d4543 Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Wed, 2 Aug 2017 10:22:19 -0400 Subject: [PATCH 16/29] Add course flag and service for grade override --- lms/djangoapps/grades/config/waffle.py | 20 +++++++++++++++++- lms/djangoapps/grades/services.py | 5 +++++ lms/djangoapps/grades/tests/test_services.py | 22 ++++++++++++++++++++ 3 files changed, 46 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/grades/config/waffle.py b/lms/djangoapps/grades/config/waffle.py index c5705ac491..6453a75e97 100644 --- a/lms/djangoapps/grades/config/waffle.py +++ b/lms/djangoapps/grades/config/waffle.py @@ -2,7 +2,7 @@ This module contains various configuration settings via waffle switches for the Grades app. """ -from openedx.core.djangoapps.waffle_utils import WaffleSwitchNamespace +from openedx.core.djangoapps.waffle_utils import WaffleSwitchNamespace, WaffleFlagNamespace, CourseWaffleFlag # Namespace WAFFLE_NAMESPACE = u'grades' @@ -12,9 +12,27 @@ WRITE_ONLY_IF_ENGAGED = u'write_only_if_engaged' ASSUME_ZERO_GRADE_IF_ABSENT = u'assume_zero_grade_if_absent' ESTIMATE_FIRST_ATTEMPTED = u'estimate_first_attempted' +# Course Flags +REJECTED_EXAM_OVERRIDES_GRADE = u'rejected_exam_overrides_grade' + def waffle(): """ Returns the namespaced, cached, audited Waffle class for Grades. """ return WaffleSwitchNamespace(name=WAFFLE_NAMESPACE, log_prefix=u'Grades: ') + + +def waffle_flags(): + """ + Returns the namespaced, cached, audited Waffle flags dictionary for Grades. + """ + namespace = WaffleFlagNamespace(name=WAFFLE_NAMESPACE, log_prefix=u'Grades: ') + return { + # By default, enable rejected exam grade overrides. Can be disabled on a course-by-course basis. + REJECTED_EXAM_OVERRIDES_GRADE: CourseWaffleFlag( + namespace, + REJECTED_EXAM_OVERRIDES_GRADE, + flag_undefined_default=True + ) + } diff --git a/lms/djangoapps/grades/services.py b/lms/djangoapps/grades/services.py index 387d86521d..cbc5521ac4 100644 --- a/lms/djangoapps/grades/services.py +++ b/lms/djangoapps/grades/services.py @@ -6,6 +6,7 @@ from opaque_keys.edx.keys import CourseKey, UsageKey from track.event_transaction_utils import create_new_event_transaction_id, set_event_transaction_type from util.date_utils import to_timestamp +from .config.waffle import waffle_flags, REJECTED_EXAM_OVERRIDES_GRADE from .constants import ScoreDatabaseTableEnum from .models import PersistentSubsectionGrade, PersistentSubsectionGradeOverride @@ -141,3 +142,7 @@ class GradesService(object): score_db_table=ScoreDatabaseTableEnum.overrides ) ) + + def should_override_grade_on_rejected_exam(self, course_key): + """Convienence function to return the state of the CourseWaffleFlag REJECTED_EXAM_OVERRIDES_GRADE""" + return waffle_flags()[REJECTED_EXAM_OVERRIDES_GRADE].is_enabled(course_key) diff --git a/lms/djangoapps/grades/tests/test_services.py b/lms/djangoapps/grades/tests/test_services.py index f6e8a05c2c..e41f1f72ce 100644 --- a/lms/djangoapps/grades/tests/test_services.py +++ b/lms/djangoapps/grades/tests/test_services.py @@ -12,9 +12,18 @@ from util.date_utils import to_timestamp from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory +from ..config.waffle import REJECTED_EXAM_OVERRIDES_GRADE from ..constants import ScoreDatabaseTableEnum +class MockWaffleFlag(): + def __init__(self, state): + self.state = state + + def is_enabled(self, course_key): + return self.state + + @ddt.ddt class GradesServiceTests(ModuleStoreTestCase): """ @@ -44,11 +53,17 @@ class GradesServiceTests(ModuleStoreTestCase): self.mock_create_id.return_value = 1 self.type_patcher = patch('lms.djangoapps.grades.services.set_event_transaction_type') self.mock_set_type = self.type_patcher.start() + self.flag_patcher = patch('lms.djangoapps.grades.services.waffle_flags') + self.mock_waffle_flags = self.flag_patcher.start() + self.mock_waffle_flags.return_value = { + REJECTED_EXAM_OVERRIDES_GRADE: MockWaffleFlag(True) + } def tearDown(self): self.recalc_patcher.stop() self.id_patcher.stop() self.type_patcher.stop() + self.flag_patcher.stop() def subsection_grade_to_dict(self, grade): return { @@ -218,3 +233,10 @@ class GradesServiceTests(ModuleStoreTestCase): @ddt.unpack def test_get_key(self, input_key, output_key, key_cls): self.assertEqual(_get_key(input_key, key_cls), output_key) + + def test_should_override_grade_on_rejected_exam(self): + self.assertTrue(self.service.should_override_grade_on_rejected_exam('course-v1:edX+DemoX+Demo_Course')) + self.mock_waffle_flags.return_value = { + REJECTED_EXAM_OVERRIDES_GRADE: MockWaffleFlag(False) + } + self.assertFalse(self.service.should_override_grade_on_rejected_exam('course-v1:edX+DemoX+Demo_Course')) From a6b94a240075da508b7b696f4c391257221f60de Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Wed, 2 Aug 2017 13:26:35 -0400 Subject: [PATCH 17/29] Use signal instead of calling recalculate directly --- lms/djangoapps/grades/services.py | 70 ++++++++++---------- lms/djangoapps/grades/signals/handlers.py | 12 ++-- lms/djangoapps/grades/signals/signals.py | 20 ++++++ lms/djangoapps/grades/tests/test_services.py | 31 ++++----- 4 files changed, 74 insertions(+), 59 deletions(-) diff --git a/lms/djangoapps/grades/services.py b/lms/djangoapps/grades/services.py index cbc5521ac4..533776ba41 100644 --- a/lms/djangoapps/grades/services.py +++ b/lms/djangoapps/grades/services.py @@ -9,6 +9,7 @@ from util.date_utils import to_timestamp from .config.waffle import waffle_flags, REJECTED_EXAM_OVERRIDES_GRADE from .constants import ScoreDatabaseTableEnum from .models import PersistentSubsectionGrade, PersistentSubsectionGradeOverride +from .signals.signals import SUBSECTION_OVERRIDE_CHANGED def _get_key(key_or_id, key_cls): @@ -70,8 +71,7 @@ class GradesService(object): override earned_all or earned_graded value if they are None. Both default to None. """ # prevent circular imports: - from .signals.handlers import SUBSECTION_RESCORE_EVENT_TYPE - from .tasks import recalculate_subsection_grade_v3 + from .signals.handlers import SUBSECTION_OVERRIDE_EVENT_TYPE course_key = _get_key(course_key_or_id, CourseKey) usage_key = _get_key(usage_key_or_id, UsageKey) @@ -89,22 +89,20 @@ class GradesService(object): earned_graded_override=earned_graded ) - # Recalculation will call PersistentSubsectionGrade.update_or_create_grade which will use the above override - # to update the grade before writing to the table. - event_transaction_id = create_new_event_transaction_id() - set_event_transaction_type(SUBSECTION_RESCORE_EVENT_TYPE) - recalculate_subsection_grade_v3.apply_async( - kwargs=dict( - user_id=user_id, - course_id=unicode(course_key), - usage_id=unicode(usage_key), - only_if_higher=False, - expected_modified_time=to_timestamp(override.modified), - score_deleted=False, - event_transaction_id=unicode(event_transaction_id), - event_transaction_type=SUBSECTION_RESCORE_EVENT_TYPE, - score_db_table=ScoreDatabaseTableEnum.overrides - ) + # Cache a new event id and event type which the signal handler will use to emit a tracking log event. + create_new_event_transaction_id() + set_event_transaction_type(SUBSECTION_OVERRIDE_EVENT_TYPE) + + # Signal will trigger subsection recalculation which will call PersistentSubsectionGrade.update_or_create_grade + # which will use the above override to update the grade before writing to the table. + SUBSECTION_OVERRIDE_CHANGED.send( + user_id=user_id, + course_id=unicode(course_key), + usage_id=unicode(usage_key), + only_if_higher=False, + modified=override.modified, + score_deleted=False, + score_db_table=ScoreDatabaseTableEnum.overrides ) def undo_override_subsection_grade(self, user_id, course_key_or_id, usage_key_or_id): @@ -115,8 +113,7 @@ class GradesService(object): override does not exist, no error is raised, it just triggers the recalculation. """ # prevent circular imports: - from .signals.handlers import SUBSECTION_RESCORE_EVENT_TYPE - from .tasks import recalculate_subsection_grade_v3 + from .signals.handlers import SUBSECTION_OVERRIDE_EVENT_TYPE course_key = _get_key(course_key_or_id, CourseKey) usage_key = _get_key(usage_key_or_id, UsageKey) @@ -126,23 +123,24 @@ class GradesService(object): if override is not None: override.delete() - event_transaction_id = create_new_event_transaction_id() - set_event_transaction_type(SUBSECTION_RESCORE_EVENT_TYPE) - recalculate_subsection_grade_v3.apply_async( - kwargs=dict( - user_id=user_id, - course_id=unicode(course_key), - usage_id=unicode(usage_key), - only_if_higher=False, - # Not used when score_deleted=True: - expected_modified_time=to_timestamp(datetime.now().replace(tzinfo=pytz.UTC)), - score_deleted=True, - event_transaction_id=unicode(event_transaction_id), - event_transaction_type=SUBSECTION_RESCORE_EVENT_TYPE, - score_db_table=ScoreDatabaseTableEnum.overrides - ) + # Cache a new event id and event type which the signal handler will use to emit a tracking log event. + create_new_event_transaction_id() + set_event_transaction_type(SUBSECTION_OVERRIDE_EVENT_TYPE) + + # Signal will trigger subsection recalculation which will call PersistentSubsectionGrade.update_or_create_grade + # which will no longer use the above deleted override, and instead return the grade to the original score from + # the actual problem responses before writing to the table. + SUBSECTION_OVERRIDE_CHANGED.send( + user_id=user_id, + course_id=unicode(course_key), + usage_id=unicode(usage_key), + only_if_higher=False, + modified=datetime.now().replace(tzinfo=pytz.UTC), # Not used when score_deleted=True + score_deleted=True, + score_db_table=ScoreDatabaseTableEnum.overrides ) - def should_override_grade_on_rejected_exam(self, course_key): + def should_override_grade_on_rejected_exam(self, course_key_or_id): """Convienence function to return the state of the CourseWaffleFlag REJECTED_EXAM_OVERRIDES_GRADE""" + course_key = _get_key(course_key_or_id, CourseKey) return waffle_flags()[REJECTED_EXAM_OVERRIDES_GRADE].is_enabled(course_key) diff --git a/lms/djangoapps/grades/signals/handlers.py b/lms/djangoapps/grades/signals/handlers.py index 539b20e225..d8a05c8932 100644 --- a/lms/djangoapps/grades/signals/handlers.py +++ b/lms/djangoapps/grades/signals/handlers.py @@ -30,7 +30,8 @@ from .signals import ( PROBLEM_RAW_SCORE_CHANGED, PROBLEM_WEIGHTED_SCORE_CHANGED, SCORE_PUBLISHED, - SUBSECTION_SCORE_CHANGED + SUBSECTION_SCORE_CHANGED, + SUBSECTION_OVERRIDE_CHANGED ) log = getLogger(__name__) @@ -38,7 +39,7 @@ log = getLogger(__name__) # define values to be used in grading events GRADES_RESCORE_EVENT_TYPE = 'edx.grades.problem.rescored' PROBLEM_SUBMITTED_EVENT_TYPE = 'edx.grades.problem.submitted' -SUBSECTION_RESCORE_EVENT_TYPE = 'edx.grades.subsection.rescored' +SUBSECTION_OVERRIDE_EVENT_TYPE = 'edx.grades.subsection.score_overridden' @receiver(score_set) @@ -210,9 +211,10 @@ def problem_raw_score_changed_handler(sender, **kwargs): # pylint: disable=unus @receiver(PROBLEM_WEIGHTED_SCORE_CHANGED) +@receiver(SUBSECTION_OVERRIDE_CHANGED) def enqueue_subsection_update(sender, **kwargs): # pylint: disable=unused-argument """ - Handles the PROBLEM_WEIGHTED_SCORE_CHANGED signal by + Handles the PROBLEM_WEIGHTED_SCORE_CHANGED or SUBSECTION_OVERRIDE_CHANGED signals by enqueueing a subsection update operation to occur asynchronously. """ _emit_event(kwargs) @@ -294,9 +296,9 @@ def _emit_event(kwargs): } ) - if root_type in [SUBSECTION_RESCORE_EVENT_TYPE]: + if root_type in [SUBSECTION_OVERRIDE_EVENT_TYPE]: tracker.emit( - unicode(SUBSECTION_RESCORE_EVENT_TYPE), + unicode(SUBSECTION_OVERRIDE_EVENT_TYPE), { 'course_id': unicode(kwargs['course_id']), 'user_id': unicode(kwargs['user_id']), diff --git a/lms/djangoapps/grades/signals/signals.py b/lms/djangoapps/grades/signals/signals.py index b1c4c4f32f..c97cc55b89 100644 --- a/lms/djangoapps/grades/signals/signals.py +++ b/lms/djangoapps/grades/signals/signals.py @@ -81,3 +81,23 @@ SUBSECTION_SCORE_CHANGED = Signal( 'subsection_grade', # SubsectionGrade object ] ) + +# Signal that indicates that a user's score for a subsection has been overridden. +# This signal is generated when an admin changes a user's exam attempt state to +# rejected or to verified from rejected. This signal may also be generated by any +# other client using the GradesService to override subsections in the future. +SUBSECTION_OVERRIDE_CHANGED = Signal( + providing_args=[ + 'user_id', # Integer User ID + 'course_id', # Unicode string representing the course + 'usage_id', # Unicode string indicating the courseware instance + 'only_if_higher', # Boolean indicating whether updates should be + # made only if the new score is higher than previous. + 'modified', # A datetime indicating when the database representation of + # this subsection override score was saved. + 'score_deleted', # Boolean indicating whether the override score was + # deleted in this event. + 'score_db_table', # The database table that houses the subsection override + # score that was created. + ] +) diff --git a/lms/djangoapps/grades/tests/test_services.py b/lms/djangoapps/grades/tests/test_services.py index e41f1f72ce..ec6f4d61d2 100644 --- a/lms/djangoapps/grades/tests/test_services.py +++ b/lms/djangoapps/grades/tests/test_services.py @@ -4,11 +4,10 @@ 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 lms.djangoapps.grades.signals.handlers import SUBSECTION_RESCORE_EVENT_TYPE -from mock import patch +from lms.djangoapps.grades.signals.handlers import SUBSECTION_OVERRIDE_EVENT_TYPE +from mock import patch, call from opaque_keys.edx.keys import CourseKey, UsageKey from student.tests.factories import UserFactory -from util.date_utils import to_timestamp from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory @@ -46,8 +45,8 @@ class GradesServiceTests(ModuleStoreTestCase): earned_graded=5.0, possible_graded=5.0 ) - self.recalc_patcher = patch('lms.djangoapps.grades.tasks.recalculate_subsection_grade_v3.apply_async') - self.mock_recalculate = self.recalc_patcher.start() + self.signal_patcher = patch('lms.djangoapps.grades.signals.signals.SUBSECTION_OVERRIDE_CHANGED.send') + self.mock_signal = self.signal_patcher.start() self.id_patcher = patch('lms.djangoapps.grades.services.create_new_event_transaction_id') self.mock_create_id = self.id_patcher.start() self.mock_create_id.return_value = 1 @@ -60,7 +59,7 @@ class GradesServiceTests(ModuleStoreTestCase): } def tearDown(self): - self.recalc_patcher.stop() + self.signal_patcher.stop() self.id_patcher.stop() self.type_patcher.stop() self.flag_patcher.stop() @@ -177,17 +176,15 @@ class GradesServiceTests(ModuleStoreTestCase): self.assertEqual(override_obj.earned_all_override, override['earned_all']) self.assertEqual(override_obj.earned_graded_override, override['earned_graded']) - self.assertDictEqual( - self.mock_recalculate.call_args[1]['kwargs'], - dict( + self.assertEqual( + self.mock_signal.call_args, + call( user_id=self.user.id, course_id=unicode(self.course.id), usage_id=unicode(self.subsection.location), only_if_higher=False, - expected_modified_time=to_timestamp(override_obj.modified), + modified=override_obj.modified, score_deleted=False, - event_transaction_id=unicode(self.mock_create_id.return_value), - event_transaction_type=SUBSECTION_RESCORE_EVENT_TYPE, score_db_table=ScoreDatabaseTableEnum.overrides ) ) @@ -205,17 +202,15 @@ class GradesServiceTests(ModuleStoreTestCase): override = self.service.get_subsection_grade_override(self.user.id, self.course.id, self.subsection.location) self.assertIsNone(override) - self.assertDictEqual( - self.mock_recalculate.call_args[1]['kwargs'], - dict( + self.assertEqual( + self.mock_signal.call_args, + call( user_id=self.user.id, course_id=unicode(self.course.id), usage_id=unicode(self.subsection.location), only_if_higher=False, - expected_modified_time=to_timestamp(datetime.now().replace(tzinfo=pytz.UTC)), + modified=datetime.now().replace(tzinfo=pytz.UTC), score_deleted=True, - event_transaction_id=unicode(self.mock_create_id.return_value), - event_transaction_type=SUBSECTION_RESCORE_EVENT_TYPE, score_db_table=ScoreDatabaseTableEnum.overrides ) ) From 147ca0bcf19a9cf6664192c317a8a1d4fc75d10c Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Wed, 2 Aug 2017 14:07:26 -0400 Subject: [PATCH 18/29] Send signal requires sender argument --- lms/djangoapps/grades/services.py | 2 ++ lms/djangoapps/grades/tests/test_services.py | 2 ++ 2 files changed, 4 insertions(+) diff --git a/lms/djangoapps/grades/services.py b/lms/djangoapps/grades/services.py index 533776ba41..475e29c860 100644 --- a/lms/djangoapps/grades/services.py +++ b/lms/djangoapps/grades/services.py @@ -96,6 +96,7 @@ class GradesService(object): # Signal will trigger subsection recalculation which will call PersistentSubsectionGrade.update_or_create_grade # which will use the above override to update the grade before writing to the table. SUBSECTION_OVERRIDE_CHANGED.send( + sender=None, user_id=user_id, course_id=unicode(course_key), usage_id=unicode(usage_key), @@ -131,6 +132,7 @@ class GradesService(object): # which will no longer use the above deleted override, and instead return the grade to the original score from # the actual problem responses before writing to the table. SUBSECTION_OVERRIDE_CHANGED.send( + sender=None, user_id=user_id, course_id=unicode(course_key), usage_id=unicode(usage_key), diff --git a/lms/djangoapps/grades/tests/test_services.py b/lms/djangoapps/grades/tests/test_services.py index ec6f4d61d2..254f688b08 100644 --- a/lms/djangoapps/grades/tests/test_services.py +++ b/lms/djangoapps/grades/tests/test_services.py @@ -179,6 +179,7 @@ class GradesServiceTests(ModuleStoreTestCase): self.assertEqual( self.mock_signal.call_args, call( + sender=None, user_id=self.user.id, course_id=unicode(self.course.id), usage_id=unicode(self.subsection.location), @@ -205,6 +206,7 @@ class GradesServiceTests(ModuleStoreTestCase): self.assertEqual( self.mock_signal.call_args, call( + sender=None, user_id=self.user.id, course_id=unicode(self.course.id), usage_id=unicode(self.subsection.location), From 81e8f906a5d7137c4c52d1156aaefcd22d00b200 Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Wed, 2 Aug 2017 14:31:30 -0400 Subject: [PATCH 19/29] Add override_deleted to subsection override event --- lms/djangoapps/grades/signals/handlers.py | 1 + 1 file changed, 1 insertion(+) diff --git a/lms/djangoapps/grades/signals/handlers.py b/lms/djangoapps/grades/signals/handlers.py index d8a05c8932..59874b6b00 100644 --- a/lms/djangoapps/grades/signals/handlers.py +++ b/lms/djangoapps/grades/signals/handlers.py @@ -304,6 +304,7 @@ def _emit_event(kwargs): 'user_id': unicode(kwargs['user_id']), 'problem_id': unicode(kwargs['usage_id']), 'only_if_higher': kwargs.get('only_if_higher'), + 'override_deleted': kwargs.get('score_deleted', False), 'event_transaction_id': unicode(get_event_transaction_id()), 'event_transaction_type': unicode(root_type), } From 503aff2518bd6cb2cc5c58cc69162c45fc24460e Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Thu, 3 Aug 2017 13:30:53 -0400 Subject: [PATCH 20/29] Address PR comments --- lms/djangoapps/grades/signals/signals.py | 6 +++--- lms/djangoapps/grades/tests/test_services.py | 3 +-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/lms/djangoapps/grades/signals/signals.py b/lms/djangoapps/grades/signals/signals.py index c97cc55b89..a0c34b8756 100644 --- a/lms/djangoapps/grades/signals/signals.py +++ b/lms/djangoapps/grades/signals/signals.py @@ -83,9 +83,9 @@ SUBSECTION_SCORE_CHANGED = Signal( ) # Signal that indicates that a user's score for a subsection has been overridden. -# This signal is generated when an admin changes a user's exam attempt state to -# rejected or to verified from rejected. This signal may also be generated by any -# other client using the GradesService to override subsections in the future. +# This signal is generated when a user's exam attempt state is set to rejected or +# to verified from rejected. This signal may also be sent by any other client +# using the GradesService to override subsections in the future. SUBSECTION_OVERRIDE_CHANGED = Signal( providing_args=[ 'user_id', # Integer User ID diff --git a/lms/djangoapps/grades/tests/test_services.py b/lms/djangoapps/grades/tests/test_services.py index 254f688b08..98262a88df 100644 --- a/lms/djangoapps/grades/tests/test_services.py +++ b/lms/djangoapps/grades/tests/test_services.py @@ -59,6 +59,7 @@ class GradesServiceTests(ModuleStoreTestCase): } def tearDown(self): + PersistentSubsectionGradeOverride.objects.all().delete() # clear out all previous overrides self.signal_patcher.stop() self.id_patcher.stop() self.type_patcher.stop() @@ -157,8 +158,6 @@ class GradesServiceTests(ModuleStoreTestCase): ) @ddt.unpack def test_override_subsection_grade(self, override, expected): - PersistentSubsectionGradeOverride.objects.all().delete() # clear out all previous overrides - self.service.override_subsection_grade( user_id=self.user.id, course_key_or_id=self.course.id, From 05de4a485bd7fe33cbfe737708ead53cca20e707 Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Thu, 3 Aug 2017 16:17:37 -0400 Subject: [PATCH 21/29] Add override notice to progress page --- lms/djangoapps/grades/new/subsection_grade.py | 1 + lms/templates/courseware/progress.html | 9 +++++++++ 2 files changed, 10 insertions(+) diff --git a/lms/djangoapps/grades/new/subsection_grade.py b/lms/djangoapps/grades/new/subsection_grade.py index fcb40f731a..4408b9ce3a 100644 --- a/lms/djangoapps/grades/new/subsection_grade.py +++ b/lms/djangoapps/grades/new/subsection_grade.py @@ -130,6 +130,7 @@ class SubsectionGrade(SubsectionGradeBase): graded=False, first_attempted=model.first_attempted, ) + self.override = model.override if hasattr(model, 'override') else None self._log_event(log.debug, u"init_from_model", student) return self diff --git a/lms/templates/courseware/progress.html b/lms/templates/courseware/progress.html index 34596885f4..d3414fe90d 100644 --- a/lms/templates/courseware/progress.html +++ b/lms/templates/courseware/progress.html @@ -183,6 +183,15 @@ from django.utils.http import urlquote_plus %endif

+

+ %if section.override is not None: + %if section.format is not None and section.format == "Exam": + ${_("Exam grade has been overridden due to a failed proctoring review.")} + %else + ${_("Section grade has been overridden.")} + %endif + %endif +

%if len(section.problem_scores.values()) > 0: %if section.show_grades(staff_access):
From 8a851b3ec175f6b98e3146211979913897452e27 Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Fri, 4 Aug 2017 11:28:09 -0400 Subject: [PATCH 22/29] Color the override notice differently --- lms/static/sass/course/_profile.scss | 6 +++++- lms/templates/courseware/progress.html | 4 ++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/lms/static/sass/course/_profile.scss b/lms/static/sass/course/_profile.scss index f0ec9eda03..e9200e47de 100644 --- a/lms/static/sass/course/_profile.scss +++ b/lms/static/sass/course/_profile.scss @@ -294,9 +294,13 @@ p { margin: lh(0.5) 0; - color: $gray-d1;; + color: $gray-d1; font-size: em(14); font-weight: 600; + + &.override-notice { + color: $red-d1; + } } .scores { diff --git a/lms/templates/courseware/progress.html b/lms/templates/courseware/progress.html index d3414fe90d..aebeb6c63b 100644 --- a/lms/templates/courseware/progress.html +++ b/lms/templates/courseware/progress.html @@ -183,11 +183,11 @@ from django.utils.http import urlquote_plus %endif

-

+

%if section.override is not None: %if section.format is not None and section.format == "Exam": ${_("Exam grade has been overridden due to a failed proctoring review.")} - %else + %else: ${_("Section grade has been overridden.")} %endif %endif From 3cefec63f68dc64a12065e7bcb9d4876e99b1873 Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Fri, 4 Aug 2017 17:54:53 -0400 Subject: [PATCH 23/29] Fix python unit tests --- .../commands/tests/test_reset_grades.py | 8 ++++---- lms/djangoapps/grades/tests/test_new.py | 4 ++-- lms/djangoapps/grades/tests/test_tasks.py | 16 ++++++++-------- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/lms/djangoapps/grades/management/commands/tests/test_reset_grades.py b/lms/djangoapps/grades/management/commands/tests/test_reset_grades.py index be57882af6..3917944b63 100644 --- a/lms/djangoapps/grades/management/commands/tests/test_reset_grades.py +++ b/lms/djangoapps/grades/management/commands/tests/test_reset_grades.py @@ -155,7 +155,7 @@ class TestResetGrades(TestCase): self._update_or_create_grades() self._assert_grades_exist_for_courses(self.course_keys) - with self.assertNumQueries(4): + with self.assertNumQueries(7): self.command.handle(delete=True, all_courses=True) self._assert_grades_absent_for_courses(self.course_keys) @@ -174,7 +174,7 @@ class TestResetGrades(TestCase): self._update_or_create_grades() self._assert_grades_exist_for_courses(self.course_keys) - with self.assertNumQueries(4): + with self.assertNumQueries(6): self.command.handle( delete=True, courses=[unicode(course_key) for course_key in self.course_keys[:num_courses_to_reset]] @@ -199,7 +199,7 @@ class TestResetGrades(TestCase): with freeze_time(self._date_from_now(days=4)): self._update_or_create_grades(self.course_keys[:num_courses_with_updated_grades]) - with self.assertNumQueries(4): + with self.assertNumQueries(6): self.command.handle(delete=True, modified_start=self._date_str_from_now(days=2), all_courses=True) self._assert_grades_absent_for_courses(self.course_keys[:num_courses_with_updated_grades]) @@ -214,7 +214,7 @@ class TestResetGrades(TestCase): with freeze_time(self._date_from_now(days=5)): self._update_or_create_grades(self.course_keys[2:4]) - with self.assertNumQueries(4): + with self.assertNumQueries(6): self.command.handle( delete=True, modified_start=self._date_str_from_now(days=2), diff --git a/lms/djangoapps/grades/tests/test_new.py b/lms/djangoapps/grades/tests/test_new.py index ccf589f726..5c1d07ce01 100644 --- a/lms/djangoapps/grades/tests/test_new.py +++ b/lms/djangoapps/grades/tests/test_new.py @@ -197,7 +197,7 @@ class TestCourseGradeFactory(GradeTestBase): self._update_grading_policy(passing=0.9) - with self.assertNumQueries(6): + with self.assertNumQueries(8): _assert_create(expected_pass=False) @ddt.data(True, False) @@ -310,7 +310,7 @@ class TestSubsectionGradeFactory(ProblemSubmissionTestMixin, GradeTestBase): mock_get_bulk_cached_grade.reset_mock() mock_create_grade.reset_mock() - with self.assertNumQueries(0): + with self.assertNumQueries(1): grade_b = self.subsection_grade_factory.create(self.sequence) self.assertTrue(mock_get_bulk_cached_grade.called) self.assertFalse(mock_create_grade.called) diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py index d27bc8206d..005fba01cc 100644 --- a/lms/djangoapps/grades/tests/test_tasks.py +++ b/lms/djangoapps/grades/tests/test_tasks.py @@ -163,10 +163,10 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest self.assertEquals(mock_block_structure_create.call_count, 1) @ddt.data( - (ModuleStoreEnum.Type.mongo, 1, 29, True), - (ModuleStoreEnum.Type.mongo, 1, 25, False), - (ModuleStoreEnum.Type.split, 3, 29, True), - (ModuleStoreEnum.Type.split, 3, 25, False), + (ModuleStoreEnum.Type.mongo, 1, 30, True), + (ModuleStoreEnum.Type.mongo, 1, 26, False), + (ModuleStoreEnum.Type.split, 3, 30, True), + (ModuleStoreEnum.Type.split, 3, 26, False), ) @ddt.unpack def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls, create_multiple_subsections): @@ -178,8 +178,8 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest self._apply_recalculate_subsection_grade() @ddt.data( - (ModuleStoreEnum.Type.mongo, 1, 29), - (ModuleStoreEnum.Type.split, 3, 29), + (ModuleStoreEnum.Type.mongo, 1, 30), + (ModuleStoreEnum.Type.split, 3, 30), ) @ddt.unpack def test_query_counts_dont_change_with_more_content(self, default_store, num_mongo_calls, num_sql_calls): @@ -239,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, 26), - (ModuleStoreEnum.Type.split, 3, 26), + (ModuleStoreEnum.Type.mongo, 1, 27), + (ModuleStoreEnum.Type.split, 3, 27), ) @ddt.unpack def test_persistent_grades_enabled_on_course(self, default_store, num_mongo_queries, num_sql_queries): From 01a4f9af25d6d90533a8d6038f607e1562ac5950 Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Mon, 7 Aug 2017 15:37:10 -0400 Subject: [PATCH 24/29] Set override to None on init_from_structure --- lms/djangoapps/grades/new/subsection_grade.py | 1 + 1 file changed, 1 insertion(+) diff --git a/lms/djangoapps/grades/new/subsection_grade.py b/lms/djangoapps/grades/new/subsection_grade.py index 4408b9ce3a..f0d30d5858 100644 --- a/lms/djangoapps/grades/new/subsection_grade.py +++ b/lms/djangoapps/grades/new/subsection_grade.py @@ -108,6 +108,7 @@ class SubsectionGrade(SubsectionGradeBase): self._compute_block_score(descendant_key, course_structure, submissions_scores, csm_scores) self.all_total, self.graded_total = graders.aggregate_scores(self.problem_scores.values()) + self.override = None self._log_event(log.debug, u"init_from_structure", student) return self From 410cc166e7f5aa0c4b091d6b7239ffe591db2975 Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Mon, 7 Aug 2017 16:43:27 -0400 Subject: [PATCH 25/29] Fix python unit tests --- lms/djangoapps/courseware/tests/test_module_render.py | 7 ++++++- lms/djangoapps/grades/new/subsection_grade.py | 3 ++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index e37ef2a3ac..24f39fd900 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -18,7 +18,7 @@ from django.test.client import RequestFactory from django.test.utils import override_settings from edx_proctoring.api import create_exam, create_exam_attempt, update_attempt_status from edx_proctoring.runtime import set_runtime_service -from edx_proctoring.tests.test_services import MockCreditService +from edx_proctoring.tests.test_services import MockCreditService, MockGradesService from freezegun import freeze_time from milestones.tests.utils import MilestonesTestCaseMixin from mock import MagicMock, Mock, patch @@ -995,6 +995,11 @@ class TestProctoringRendering(SharedModuleStoreTestCase): MockCreditService(enrollment_mode=enrollment_mode) ) + set_runtime_service( + 'grades', + MockGradesService() + ) + exam_id = create_exam( course_id=unicode(self.course_key), content_id=unicode(sequence.location), diff --git a/lms/djangoapps/grades/new/subsection_grade.py b/lms/djangoapps/grades/new/subsection_grade.py index f0d30d5858..ff767bf5f7 100644 --- a/lms/djangoapps/grades/new/subsection_grade.py +++ b/lms/djangoapps/grades/new/subsection_grade.py @@ -36,6 +36,8 @@ class SubsectionGradeBase(object): self.graded_total = None # aggregated grade for all graded problems self.all_total = None # aggregated grade for all problems, regardless of whether they are graded + self.override = None + @property def attempted(self): """ @@ -108,7 +110,6 @@ class SubsectionGrade(SubsectionGradeBase): self._compute_block_score(descendant_key, course_structure, submissions_scores, csm_scores) self.all_total, self.graded_total = graders.aggregate_scores(self.problem_scores.values()) - self.override = None self._log_event(log.debug, u"init_from_structure", student) return self From 4953cf30d9161ac0e9b9eab3f73b7bcf5960926e Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Wed, 9 Aug 2017 11:19:31 -0400 Subject: [PATCH 26/29] Comment out actual override and log instead --- lms/djangoapps/grades/models.py | 29 +++++++++++++++++++------- lms/templates/courseware/progress.html | 17 ++++++++------- 2 files changed, 31 insertions(+), 15 deletions(-) diff --git a/lms/djangoapps/grades/models.py b/lms/djangoapps/grades/models.py index d11a6bdb35..f322cef38d 100644 --- a/lms/djangoapps/grades/models.py +++ b/lms/djangoapps/grades/models.py @@ -418,14 +418,27 @@ class PersistentSubsectionGrade(DeleteGradesMixin, TimeStampedModel): grade__course_id=usage_key.course_key, grade__usage_key=usage_key, ) - if override.earned_all_override is not None: - params['earned_all'] = override.earned_all_override - if override.possible_all_override is not None: - params['possible_all'] = override.possible_all_override - if override.earned_graded_override is not None: - params['earned_graded'] = override.earned_graded_override - if override.possible_graded_override is not None: - params['possible_graded'] = override.possible_graded_override + # EDUCTATOR-1127: no-op and log until this behavior is verified in production + # if override.earned_all_override is not None: + # params['earned_all'] = override.earned_all_override + # if override.possible_all_override is not None: + # params['possible_all'] = override.possible_all_override + # if override.earned_graded_override is not None: + # params['earned_graded'] = override.earned_graded_override + # if override.possible_graded_override is not None: + # params['possible_graded'] = override.possible_graded_override + log.info(u"EDUCATOR-1127: Subsection grade for user {user_id} on subsection {usage_key} in course " + u"{course_key} would be overridden with params: {params}".format( + user_id=unicode(user_id), + usage_key=unicode(usage_key), + course_key=unicode(usage_key.course_key), + params=unicode({ + 'earned_all': override.earned_all_override, + 'possible_all': override.possible_all_override, + 'earned_graded': override.earned_graded_override, + 'possible_graded': override.possible_graded_override + })) + ) except PersistentSubsectionGradeOverride.DoesNotExist: pass diff --git a/lms/templates/courseware/progress.html b/lms/templates/courseware/progress.html index aebeb6c63b..ce71201f62 100644 --- a/lms/templates/courseware/progress.html +++ b/lms/templates/courseware/progress.html @@ -183,15 +183,18 @@ from django.utils.http import urlquote_plus %endif

-

+ <%doc> + EDUCATOR-1127: Do not display override notice until override is enabled %if section.override is not None: - %if section.format is not None and section.format == "Exam": - ${_("Exam grade has been overridden due to a failed proctoring review.")} - %else: - ${_("Section grade has been overridden.")} - %endif +

+ %if section.format is not None and section.format == "Exam": + ${_("Exam grade has been overridden due to a failed proctoring review.")} + %else: + ${_("Section grade has been overridden.")} + %endif +

%endif -

+ %if len(section.problem_scores.values()) > 0: %if section.show_grades(staff_access):
From 1b2fec21296f9ae022a8fa6a4c196a55a05a5098 Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Wed, 9 Aug 2017 13:51:07 -0400 Subject: [PATCH 27/29] Fix tests --- lms/djangoapps/grades/models.py | 25 ++++++++++++---------- lms/djangoapps/grades/tests/test_models.py | 5 +++-- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/lms/djangoapps/grades/models.py b/lms/djangoapps/grades/models.py index f322cef38d..e4aef248d3 100644 --- a/lms/djangoapps/grades/models.py +++ b/lms/djangoapps/grades/models.py @@ -427,17 +427,20 @@ class PersistentSubsectionGrade(DeleteGradesMixin, TimeStampedModel): # params['earned_graded'] = override.earned_graded_override # if override.possible_graded_override is not None: # params['possible_graded'] = override.possible_graded_override - log.info(u"EDUCATOR-1127: Subsection grade for user {user_id} on subsection {usage_key} in course " - u"{course_key} would be overridden with params: {params}".format( - user_id=unicode(user_id), - usage_key=unicode(usage_key), - course_key=unicode(usage_key.course_key), - params=unicode({ - 'earned_all': override.earned_all_override, - 'possible_all': override.possible_all_override, - 'earned_graded': override.earned_graded_override, - 'possible_graded': override.possible_graded_override - })) + log.info( + u"EDUCATOR-1127: Subsection grade for user {user_id} on subsection {usage_key} in course " + u"{course_key} would be overridden with params: {params}" + .format( + user_id=unicode(user_id), + usage_key=unicode(usage_key), + course_key=unicode(usage_key.course_key), + params=unicode({ + 'earned_all': override.earned_all_override, + 'possible_all': override.possible_all_override, + 'earned_graded': override.earned_graded_override, + 'possible_graded': override.possible_graded_override + }) + ) ) except PersistentSubsectionGradeOverride.DoesNotExist: pass diff --git a/lms/djangoapps/grades/tests/test_models.py b/lms/djangoapps/grades/tests/test_models.py index 39e31e273b..9fabce19d0 100644 --- a/lms/djangoapps/grades/tests/test_models.py +++ b/lms/djangoapps/grades/tests/test_models.py @@ -312,8 +312,9 @@ class PersistentSubsectionGradeTest(GradesModelTestCase): override = PersistentSubsectionGradeOverride(grade=grade, earned_all_override=0.0, earned_graded_override=0.0) override.save() grade = PersistentSubsectionGrade.update_or_create_grade(**self.params) - self.assertEqual(grade.earned_all, 0.0) - self.assertEqual(grade.earned_graded, 0.0) + # EDUCATOR-1127 Override is not enabled yet, change to 0.0 when enabled + self.assertEqual(grade.earned_all, 6.0) + self.assertEqual(grade.earned_graded, 6.0) def _assert_tracker_emitted_event(self, tracker_mock, grade): """ From 1ab188033741e5a147746fa15ab965570439abb7 Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Wed, 9 Aug 2017 17:27:47 -0400 Subject: [PATCH 28/29] Remove override behavior and log instead This will break tests. --- lms/djangoapps/grades/models.py | 33 +-------- lms/djangoapps/grades/services.py | 78 ++++++-------------- lms/djangoapps/grades/tests/test_services.py | 34 +-------- 3 files changed, 25 insertions(+), 120 deletions(-) diff --git a/lms/djangoapps/grades/models.py b/lms/djangoapps/grades/models.py index e4aef248d3..006880c580 100644 --- a/lms/djangoapps/grades/models.py +++ b/lms/djangoapps/grades/models.py @@ -412,38 +412,7 @@ class PersistentSubsectionGrade(DeleteGradesMixin, TimeStampedModel): usage_key = params.pop('usage_key') # apply grade override if one exists before saving model - try: - override = PersistentSubsectionGradeOverride.objects.get( - grade__user_id=user_id, - grade__course_id=usage_key.course_key, - grade__usage_key=usage_key, - ) - # EDUCTATOR-1127: no-op and log until this behavior is verified in production - # if override.earned_all_override is not None: - # params['earned_all'] = override.earned_all_override - # if override.possible_all_override is not None: - # params['possible_all'] = override.possible_all_override - # if override.earned_graded_override is not None: - # params['earned_graded'] = override.earned_graded_override - # if override.possible_graded_override is not None: - # params['possible_graded'] = override.possible_graded_override - log.info( - u"EDUCATOR-1127: Subsection grade for user {user_id} on subsection {usage_key} in course " - u"{course_key} would be overridden with params: {params}" - .format( - user_id=unicode(user_id), - usage_key=unicode(usage_key), - course_key=unicode(usage_key.course_key), - params=unicode({ - 'earned_all': override.earned_all_override, - 'possible_all': override.possible_all_override, - 'earned_graded': override.earned_graded_override, - 'possible_graded': override.possible_graded_override - }) - ) - ) - except PersistentSubsectionGradeOverride.DoesNotExist: - pass + # EDUCTATOR-1127: remove override until this behavior is verified in production grade, _ = cls.objects.update_or_create( user_id=user_id, diff --git a/lms/djangoapps/grades/services.py b/lms/djangoapps/grades/services.py index 475e29c860..53ca93fab3 100644 --- a/lms/djangoapps/grades/services.py +++ b/lms/djangoapps/grades/services.py @@ -1,5 +1,6 @@ from datetime import datetime +import logging import pytz from opaque_keys.edx.keys import CourseKey, UsageKey @@ -11,6 +12,8 @@ from .constants import ScoreDatabaseTableEnum from .models import PersistentSubsectionGrade, PersistentSubsectionGradeOverride from .signals.signals import SUBSECTION_OVERRIDE_CHANGED +log = logging.getLogger(__name__) + def _get_key(key_or_id, key_cls): """ @@ -70,40 +73,21 @@ class GradesService(object): 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. """ - # prevent circular imports: - from .signals.handlers import SUBSECTION_OVERRIDE_EVENT_TYPE - 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 - ) - - # Create override that will prevent any future updates to grade - override, _ = PersistentSubsectionGradeOverride.objects.update_or_create( - grade=grade, - earned_all_override=earned_all, - earned_graded_override=earned_graded - ) - - # Cache a new event id and event type which the signal handler will use to emit a tracking log event. - create_new_event_transaction_id() - set_event_transaction_type(SUBSECTION_OVERRIDE_EVENT_TYPE) - - # Signal will trigger subsection recalculation which will call PersistentSubsectionGrade.update_or_create_grade - # which will use the above override to update the grade before writing to the table. - SUBSECTION_OVERRIDE_CHANGED.send( - sender=None, - user_id=user_id, - course_id=unicode(course_key), - usage_id=unicode(usage_key), - only_if_higher=False, - modified=override.modified, - score_deleted=False, - score_db_table=ScoreDatabaseTableEnum.overrides + log.info( + u"EDUCATOR-1127: Subsection grade override for user {user_id} on subsection {usage_key} in course " + u"{course_key} would be created with params: {params}" + .format( + user_id=unicode(user_id), + usage_key=unicode(usage_key), + course_key=unicode(course_key), + params=unicode({ + 'earned_all': earned_all, + 'earned_graded': earned_graded, + }) + ) ) def undo_override_subsection_grade(self, user_id, course_key_or_id, usage_key_or_id): @@ -113,33 +97,17 @@ class GradesService(object): Fires off a recalculate_subsection_grade async task to update the PersistentSubsectionGrade table. If the override does not exist, no error is raised, it just triggers the recalculation. """ - # prevent circular imports: - from .signals.handlers import SUBSECTION_OVERRIDE_EVENT_TYPE - 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) - # Older rejected exam attempts that transition to verified might not have an override created - if override is not None: - override.delete() - - # Cache a new event id and event type which the signal handler will use to emit a tracking log event. - create_new_event_transaction_id() - set_event_transaction_type(SUBSECTION_OVERRIDE_EVENT_TYPE) - - # Signal will trigger subsection recalculation which will call PersistentSubsectionGrade.update_or_create_grade - # which will no longer use the above deleted override, and instead return the grade to the original score from - # the actual problem responses before writing to the table. - SUBSECTION_OVERRIDE_CHANGED.send( - sender=None, - user_id=user_id, - course_id=unicode(course_key), - usage_id=unicode(usage_key), - only_if_higher=False, - modified=datetime.now().replace(tzinfo=pytz.UTC), # Not used when score_deleted=True - score_deleted=True, - score_db_table=ScoreDatabaseTableEnum.overrides + log.info( + u"EDUCATOR-1127: Subsection grade override for user {user_id} on subsection {usage_key} in course " + u"{course_key} would be deleted" + .format( + user_id=unicode(user_id), + usage_key=unicode(usage_key), + course_key=unicode(course_key) + ) ) def should_override_grade_on_rejected_exam(self, course_key_or_id): diff --git a/lms/djangoapps/grades/tests/test_services.py b/lms/djangoapps/grades/tests/test_services.py index 98262a88df..8a172d0bef 100644 --- a/lms/djangoapps/grades/tests/test_services.py +++ b/lms/djangoapps/grades/tests/test_services.py @@ -171,28 +171,10 @@ class GradesServiceTests(ModuleStoreTestCase): 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.assertEqual( - self.mock_signal.call_args, - call( - sender=None, - user_id=self.user.id, - course_id=unicode(self.course.id), - usage_id=unicode(self.subsection.location), - only_if_higher=False, - modified=override_obj.modified, - score_deleted=False, - score_db_table=ScoreDatabaseTableEnum.overrides - ) - ) + self.assertIsNone(override_obj) @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, @@ -202,20 +184,6 @@ class GradesServiceTests(ModuleStoreTestCase): override = self.service.get_subsection_grade_override(self.user.id, self.course.id, self.subsection.location) self.assertIsNone(override) - self.assertEqual( - self.mock_signal.call_args, - call( - sender=None, - user_id=self.user.id, - course_id=unicode(self.course.id), - usage_id=unicode(self.subsection.location), - only_if_higher=False, - 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], ['course-v1:edX+DemoX+Demo_Course', CourseKey.from_string('course-v1:edX+DemoX+Demo_Course'), CourseKey], From 842ce8365f142ce6bb69d3a9848e638ee3221c88 Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Wed, 9 Aug 2017 18:08:37 -0400 Subject: [PATCH 29/29] Fix python tests and use proctoring 1.0.0 --- lms/djangoapps/grades/tests/test_tasks.py | 16 ++++++++-------- requirements/edx/github.txt | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py index 005fba01cc..d27bc8206d 100644 --- a/lms/djangoapps/grades/tests/test_tasks.py +++ b/lms/djangoapps/grades/tests/test_tasks.py @@ -163,10 +163,10 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest self.assertEquals(mock_block_structure_create.call_count, 1) @ddt.data( - (ModuleStoreEnum.Type.mongo, 1, 30, True), - (ModuleStoreEnum.Type.mongo, 1, 26, False), - (ModuleStoreEnum.Type.split, 3, 30, True), - (ModuleStoreEnum.Type.split, 3, 26, 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): @@ -178,8 +178,8 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest self._apply_recalculate_subsection_grade() @ddt.data( - (ModuleStoreEnum.Type.mongo, 1, 30), - (ModuleStoreEnum.Type.split, 3, 30), + (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): @@ -239,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, 27), - (ModuleStoreEnum.Type.split, 3, 27), + (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): diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index 422eb521d0..1c9552b41e 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -95,7 +95,7 @@ git+https://github.com/edx/xblock-utils.git@v1.0.5#egg=xblock-utils==1.0.5 -e git+https://github.com/edx-solutions/xblock-google-drive.git@138e6fa0bf3a2013e904a085b9fed77dab7f3f21#egg=xblock-google-drive git+https://github.com/edx/edx-user-state-client.git@1.0.1#egg=edx-user-state-client==1.0.1 git+https://github.com/edx/xblock-lti-consumer.git@v1.1.5#egg=lti_consumer-xblock==1.1.5 -git+https://github.com/edx/edx-proctoring.git@EDUCATOR-927 +git+https://github.com/edx/edx-proctoring.git@1.0.0#egg=edx-proctoring==1.0.0 # Third Party XBlocks git+https://github.com/open-craft/xblock-poll@v1.2.7#egg=xblock-poll==1.2.7