From 577315f2a7ff7f143787ba838c0002dd7ea97df3 Mon Sep 17 00:00:00 2001 From: Alex Dusenbery Date: Thu, 20 Dec 2018 16:45:05 -0500 Subject: [PATCH] Refactor how grade override values are persisted. --- lms/djangoapps/grades/api/urls.py | 1 - .../grades/api/v1/gradebook_views.py | 32 ++------- .../api/v1/tests/test_gradebook_views.py | 45 +++++++++---- .../docs/decisions/0001-gradebook-api.rst | 4 +- .../decisions/0002-gradebook-front-end.rst | 4 +- .../docs/decisions/0003-persistent-grades.rst | 22 ++++++ .../0004-persistent-grade-overrides.rst | 33 +++++++++ .../docs/decisions/0005-grade-freeze.rst | 37 ++++++++++ .../0006-overrides-applied-separately.rst | 39 +++++++++++ lms/djangoapps/grades/models.py | 65 +++++++++++++----- lms/djangoapps/grades/services.py | 39 +++++------ lms/djangoapps/grades/subsection_grade.py | 67 +++++++++---------- .../grades/tests/test_course_grade_factory.py | 9 ++- lms/djangoapps/grades/tests/test_models.py | 30 +++++++-- lms/djangoapps/grades/tests/test_services.py | 7 +- .../grades/tests/test_subsection_grade.py | 6 +- .../tests/test_subsection_grade_factory.py | 20 ++++-- lms/djangoapps/grades/tests/test_tasks.py | 16 ++--- 18 files changed, 333 insertions(+), 143 deletions(-) create mode 100644 lms/djangoapps/grades/docs/decisions/0003-persistent-grades.rst create mode 100644 lms/djangoapps/grades/docs/decisions/0004-persistent-grade-overrides.rst create mode 100644 lms/djangoapps/grades/docs/decisions/0005-grade-freeze.rst create mode 100644 lms/djangoapps/grades/docs/decisions/0006-overrides-applied-separately.rst diff --git a/lms/djangoapps/grades/api/urls.py b/lms/djangoapps/grades/api/urls.py index 933d9b8925..b29b3888da 100644 --- a/lms/djangoapps/grades/api/urls.py +++ b/lms/djangoapps/grades/api/urls.py @@ -2,7 +2,6 @@ Grades API URLs. """ -from django.conf import settings from django.conf.urls import include, url diff --git a/lms/djangoapps/grades/api/v1/gradebook_views.py b/lms/djangoapps/grades/api/v1/gradebook_views.py index cffa6d5cf5..5f883c162d 100644 --- a/lms/djangoapps/grades/api/v1/gradebook_views.py +++ b/lms/djangoapps/grades/api/v1/gradebook_views.py @@ -32,7 +32,7 @@ from lms.djangoapps.grades.models import ( PersistentCourseGrade, PersistentSubsectionGrade, PersistentSubsectionGradeOverride, - PersistentSubsectionGradeOverrideHistory + PersistentSubsectionGradeOverrideHistory, ) from lms.djangoapps.grades.subsection_grade import CreateSubsectionGrade from lms.djangoapps.grades.tasks import are_grades_frozen, recalculate_subsection_grade_v3 @@ -731,16 +731,11 @@ class GradebookBulkUpdateView(GradeViewMixin, PaginatedAPIView): Helper method to create a `PersistentSubsectionGradeOverride` object and send a `SUBSECTION_OVERRIDE_CHANGED` signal. """ - override, _ = PersistentSubsectionGradeOverride.objects.update_or_create( - grade=subsection_grade_model, - defaults=self._clean_override_data(override_data), - ) - - _ = PersistentSubsectionGradeOverrideHistory.objects.create( - override_id=override.id, - user=request_user, + override = PersistentSubsectionGradeOverride.update_or_create_override( + requesting_user=request_user, + subsection_grade_model=subsection_grade_model, feature=PersistentSubsectionGradeOverrideHistory.GRADEBOOK, - action=PersistentSubsectionGradeOverrideHistory.CREATE_OR_UPDATE, + **override_data ) set_event_transaction_type(SUBSECTION_GRADE_CALCULATED) @@ -765,23 +760,6 @@ class GradebookBulkUpdateView(GradeViewMixin, PaginatedAPIView): subsection_grade_calculated(subsection_grade_model) return override - def _clean_override_data(self, override_data): - """ - Helper method to strip any grade override field names that won't work - as defaults when calling PersistentSubsectionGradeOverride.update_or_create(). - """ - allowed_fields = { - 'earned_all_override', - 'possible_all_override', - 'earned_graded_override', - 'possible_graded_override', - } - stripped_data = {} - for field in override_data.keys(): - if field in allowed_fields: - stripped_data[field] = override_data[field] - return stripped_data - @staticmethod def _log_update_result( request_user, diff --git a/lms/djangoapps/grades/api/v1/tests/test_gradebook_views.py b/lms/djangoapps/grades/api/v1/tests/test_gradebook_views.py index 39088a1d75..dea04f01b5 100644 --- a/lms/djangoapps/grades/api/v1/tests/test_gradebook_views.py +++ b/lms/djangoapps/grades/api/v1/tests/test_gradebook_views.py @@ -385,6 +385,8 @@ class GradebookViewTest(GradebookViewTestBase): Helper function to mock a subsection grade. """ model = MagicMock(**kwargs) + if 'override' not in kwargs: + del model.override factory = MagicMock() return ReadSubsectionGrade(subsection, model, factory) @@ -623,42 +625,48 @@ class GradebookViewTest(GradebookViewTestBase): with patch('lms.djangoapps.grades.course_grade_factory.CourseGradeFactory.read') as mock_grade: course_grade = self.mock_course_grade(self.student, passed=True, letter_grade='A', percent=0.85) + mock_override = MagicMock( + earned_all_override=1.0, + possible_all_override=2.0, + earned_graded_override=1.0, + possible_graded_override=2.0, + ) mock_subsection_grades = { self.subsections[self.chapter_1.location][0].location: self.mock_subsection_grade( self.subsections[self.chapter_1.location][0], earned_all=1.0, possible_all=2.0, - earned_graded=1.0, + earned_graded=2.0, possible_graded=2.0, first_attempted=None, - override=MagicMock(), + override=mock_override, ), self.subsections[self.chapter_1.location][1].location: self.mock_subsection_grade( self.subsections[self.chapter_1.location][1], earned_all=1.0, possible_all=2.0, - earned_graded=1.0, + earned_graded=2.0, possible_graded=2.0, first_attempted=None, - override=MagicMock(), + override=mock_override, ), self.subsections[self.chapter_2.location][0].location: self.mock_subsection_grade( self.subsections[self.chapter_2.location][0], earned_all=1.0, possible_all=2.0, - earned_graded=1.0, + earned_graded=2.0, possible_graded=2.0, first_attempted=None, - override=MagicMock(), + override=mock_override, ), self.subsections[self.chapter_2.location][1].location: self.mock_subsection_grade( self.subsections[self.chapter_2.location][1], earned_all=1.0, possible_all=2.0, - earned_graded=1.0, + earned_graded=2.0, possible_graded=2.0, first_attempted=None, - override=MagicMock(), + override=mock_override, ), } course_grade.subsection_grade = lambda key: mock_subsection_grades[key] @@ -1163,20 +1171,31 @@ class GradebookBulkUpdateViewTest(GradebookViewTestBase): # We should now have PersistentSubsectionGradeOverride records corresponding to # our bulk-update request, and PersistentSubsectionGrade records with grade values - # equal to those of the override. - for usage_key, expected_grades in ( - (self.subsections[self.chapter_1.location][0].location, GradeFields(3, 3, 2, 2)), - (self.subsections[self.chapter_1.location][1].location, GradeFields(3, 4, 3, 4)), + # equal to the aggregate of their problem scores (in this case, zeros, since we + # didn't mock out CourseGradeFactory.read() to return a non-zero score for anything). + for usage_key, expected_grades, expected_grade_overrides in ( + ( + self.subsections[self.chapter_1.location][0].location, + GradeFields(0, 0, 0, 0), + GradeFields(3, 3, 2, 2) + ), + ( + self.subsections[self.chapter_1.location][1].location, + GradeFields(0, 0, 0, 0), + GradeFields(3, 4, 3, 4) + ), ): # this selects related PersistentSubsectionGradeOverride objects. grade = PersistentSubsectionGrade.read_grade( user_id=self.student.id, usage_key=usage_key, ) + for field_name in expected_grade_overrides._fields: + expected_value = getattr(expected_grade_overrides, field_name) + self.assertEqual(expected_value, getattr(grade.override, field_name + '_override')) for field_name in expected_grades._fields: expected_value = getattr(expected_grades, field_name) self.assertEqual(expected_value, getattr(grade, field_name)) - self.assertEqual(expected_value, getattr(grade.override, field_name + '_override')) update_records = PersistentSubsectionGradeOverrideHistory.objects.filter(user=request_user) self.assertEqual(update_records.count(), 3) diff --git a/lms/djangoapps/grades/docs/decisions/0001-gradebook-api.rst b/lms/djangoapps/grades/docs/decisions/0001-gradebook-api.rst index 197979a2e3..e4c50a4aed 100644 --- a/lms/djangoapps/grades/docs/decisions/0001-gradebook-api.rst +++ b/lms/djangoapps/grades/docs/decisions/0001-gradebook-api.rst @@ -1,5 +1,5 @@ -1. Gradebook read and write APIs --------------------------------- +Gradebook read and write APIs +----------------------------- Status ====== diff --git a/lms/djangoapps/grades/docs/decisions/0002-gradebook-front-end.rst b/lms/djangoapps/grades/docs/decisions/0002-gradebook-front-end.rst index f395c23097..04267337b6 100644 --- a/lms/djangoapps/grades/docs/decisions/0002-gradebook-front-end.rst +++ b/lms/djangoapps/grades/docs/decisions/0002-gradebook-front-end.rst @@ -1,5 +1,5 @@ -2. Gradebook User Interface and Experience ------------------------------------------- +Gradebook User Interface and Experience +--------------------------------------- Status ====== diff --git a/lms/djangoapps/grades/docs/decisions/0003-persistent-grades.rst b/lms/djangoapps/grades/docs/decisions/0003-persistent-grades.rst new file mode 100644 index 0000000000..e202a147e0 --- /dev/null +++ b/lms/djangoapps/grades/docs/decisions/0003-persistent-grades.rst @@ -0,0 +1,22 @@ +Persistent Subsection and Course Grades +--------------------------------------- + +Status +====== + +Accepted (circa June 2017) + +Context +======= + +There are several reasons that we need to persist a learner's computed grade in the database, +both at the subsection-level, course-level and problem-level. Prior to the introduction +of persistent subsection and course grades, only problem-level grades were persisted, with +higher-level grades computed dynamically from these problem grades. + +Decisions +========= + +More background and the decisions around persistent subsection and course grades +are captured in the `Robust Grades Document +`_. diff --git a/lms/djangoapps/grades/docs/decisions/0004-persistent-grade-overrides.rst b/lms/djangoapps/grades/docs/decisions/0004-persistent-grade-overrides.rst new file mode 100644 index 0000000000..6ab80c6425 --- /dev/null +++ b/lms/djangoapps/grades/docs/decisions/0004-persistent-grade-overrides.rst @@ -0,0 +1,33 @@ +Persistent Subsection Grade Overrides +------------------------------------- + +New decisions have been made about subsection grade overrides, see 0006-overrides-applied-separately_. + +.. _0006-overrides-applied-separately: 0006-overrides-applied-separately.rst + +Status +====== + +Accepted (circa Summer 2017) + +Superseded (January 2019) + +Context +======= + +It is sometimes necessary to override a learner's score for an entire subsection/assignment. +For example, for a graded and proctored exam, we may wish to automatically fail a learner's +attempt on the exam with a score of zero if the proctor indicates suspicious behavior +on the part of the learner. + +Decisions +========= + +* We want to allow for the automatic or manual creation of overrides at the subsection-level. +* We'll create a class called ``PersistentSubsectionGradeOverride`` to store the overridden scores, and + an instance of this class will always be associated with an instance of ``PersistentSubsectionGrade``. +* When updating a ``PersistentSubsectionGrade``, we'll persist the overridden scores from an associated + ``PersistentSubsectionGradeOverride`` (if one exists) instead of the computed value for the subsection grade. +* No more than one ``PersistentSubsectionGradeOverride`` will ever exist for a given ``PersistentSubsectionGrade``. + That is, write actions against ``PersistentSubsectionGradeOverride`` are always an update-or-create action, + using the primary key of the ``PersistentSubsectionGrade`` to perform the ``SELECT ... FOR UPDATE`` query. diff --git a/lms/djangoapps/grades/docs/decisions/0005-grade-freeze.rst b/lms/djangoapps/grades/docs/decisions/0005-grade-freeze.rst new file mode 100644 index 0000000000..b572f69ffb --- /dev/null +++ b/lms/djangoapps/grades/docs/decisions/0005-grade-freeze.rst @@ -0,0 +1,37 @@ +Freezing Changes to Subsection and Course Grades +------------------------------------------------ + +Status +====== + +Accepted (circa Summer 2018) + +Context +======= + +We would like to prevent subsection-level and course-level grades from being +updated in a course that has ended, after some fixed period of time. After that fixed +period of time, we consider grades for that course "frozen" - they can no longer be changed +by the system. This is important for maintaining the integrity of grades and a learner's completion of a course, +particularly in credit-bearing courses. + +Decisions +========= + +* In ``lms/djangoapps/grades/tasks.py``, we'll introduce an ``are_grades_frozen`` function + to determine, for a given course key, whether subsection and course grades should now be + frozen for that course. +* The fixed period of time after course end at which grades will be frozen is 30 days. +* We'll freeze grades after 30 days for all courses, unless course waffle flag override is + enabled. An enabled override causes grades to not be frozen (after any amount of time) + for that particular course. +* Any grades celery task that can update grades will now check if grades are frozen + before taking any action. If grades for the course are frozen, the task will simply + return without taking any further action. + +Consequences +============ + +As a consequence of this decision, persistent grades will now be inconsistent with the +values computed from input scores, course content, grading policy, etc. if changes +to any of these occur after the frozen date. diff --git a/lms/djangoapps/grades/docs/decisions/0006-overrides-applied-separately.rst b/lms/djangoapps/grades/docs/decisions/0006-overrides-applied-separately.rst new file mode 100644 index 0000000000..0fb52bee60 --- /dev/null +++ b/lms/djangoapps/grades/docs/decisions/0006-overrides-applied-separately.rst @@ -0,0 +1,39 @@ +Subsection Grade Override History and Refactoring +------------------------------------------------- + +For background on persistent subsection grade overrides, see 0004-persistent-grade-overrides_. + +.. _0004-persistent-grade-overrides: 0004-persistent-grade-overrides.rst + +Status +====== + +Accepted (January 2019) + +Context +======= + +We want to maintain the score values in the ``PersistentSubsectionGrade`` table as a +source of truth as it relates to the learner's attempt at an assignment. This is necessary +when, for instance, we want to see an "audit trail" of manually changed grades via the +Gradebook feature. We'd like an instructor to see both the original grade as obtained +by the student, as well as all changes made via ``PersistentSubsectionGradeOverrides``. +Therefore, we need to make two changes: + +* The existence of a ``PersistentSubsectionGradeOverride`` should no longer change the score values + of the associated ``PersistentSubsectionGrade``. +* We'll introduce a ``PersistentSubsectionGradeOverrideHistory`` table to track changes to grades + made via grade overrides. + +Decisions +========= + +* We'll introduce a ``PersistentSubsectionGradeOverrideHistory`` table. This table will track + who created the override, the reason for the change (i.e. due to proctoring failure or a change + via Gradebook), and any comments made by the overriding user. +* As detailed in this `Jira issue `_, we'll + refactor the grades override logic to no longer override the parameters of a ``PersistentSubsectionGrade`` + during updates where an associated ``PersistentSubsectionGradeOverride`` exists. +* The ``SubsectionGrade`` classes should now account for any associated overrides and factor those + into the subsection- and course-level grades when presenting grade data via a client + (e.g. Gradebook, instructor grade reports, the Progress page). diff --git a/lms/djangoapps/grades/models.py b/lms/djangoapps/grades/models.py index e150b93681..b803856bfb 100644 --- a/lms/djangoapps/grades/models.py +++ b/lms/djangoapps/grades/models.py @@ -415,7 +415,6 @@ class PersistentSubsectionGrade(TimeStampedModel): cls._prepare_params(params) VisibleBlocks.cached_get_or_create(params['user_id'], params['visible_blocks']) cls._prepare_params_visible_blocks_id(params) - cls._prepare_params_override(params) # TODO: do we NEED to pop these? first_attempted = params.pop('first_attempted') @@ -428,6 +427,7 @@ class PersistentSubsectionGrade(TimeStampedModel): usage_key=usage_key, defaults=params, ) + grade.override = PersistentSubsectionGradeOverride.get_override(user_id, usage_key) if first_attempted is not None and grade.first_attempted is None: grade.first_attempted = first_attempted grade.save() @@ -450,7 +450,6 @@ class PersistentSubsectionGrade(TimeStampedModel): user_id, course_key, [params['visible_blocks'] for params in grade_params_iter] ) map(cls._prepare_params_visible_blocks_id, grade_params_iter) - map(cls._prepare_params_override, grade_params_iter) grades = [PersistentSubsectionGrade(**params) for params in grade_params_iter] grades = cls.objects.bulk_create(grades) @@ -481,19 +480,6 @@ class PersistentSubsectionGrade(TimeStampedModel): params['visible_blocks_id'] = params['visible_blocks'].hash_value del params['visible_blocks'] - @classmethod - def _prepare_params_override(cls, params): - override = PersistentSubsectionGradeOverride.get_override(params['user_id'], params['usage_key']) - if override: - 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 - @staticmethod def _emit_grade_calculated_event(grade): events.subsection_grade_calculated(grade) @@ -690,6 +676,55 @@ class PersistentSubsectionGradeOverride(models.Model): except PersistentSubsectionGradeOverride.DoesNotExist: pass + @classmethod + def update_or_create_override( + cls, requesting_user, subsection_grade_model, feature=None, action=None, **override_data + ): + """ + Creates or updates an override object for the given PersistentSubsectionGrade. + Args: + requesting_user: The user that is creating the override (so we can record this action in + a PersistentSubsectionGradeOverrideHistory record). + subsection_grade_model: The PersistentSubsectionGrade object associated with this override. + override_data: The parameters of score values used to create the override record. + """ + override, _ = PersistentSubsectionGradeOverride.objects.update_or_create( + grade=subsection_grade_model, + defaults=cls._prepare_override_params(subsection_grade_model, override_data), + ) + + action = action or PersistentSubsectionGradeOverrideHistory.CREATE_OR_UPDATE + + PersistentSubsectionGradeOverrideHistory.objects.create( + override_id=override.id, + user=requesting_user, + feature=feature, + action=action, + ) + return override + + @staticmethod + def _prepare_override_params(subsection_grade_model, override_data): + """ + Helper method to strip any grade override field names that won't work + as defaults when calling PersistentSubsectionGradeOverride.update_or_create(), + and to use default values from the associated PersistentSubsectionGrade + for any override fields that are not specified. + """ + allowed_fields_and_defaults = { + 'earned_all_override': 'earned_all', + 'possible_all_override': 'possible_all', + 'earned_graded_override': 'earned_graded', + 'possible_graded_override': 'possible_graded', + } + cleaned_data = {} + for override_field_name, field_name in allowed_fields_and_defaults.items(): + cleaned_data[override_field_name] = override_data.get( + override_field_name, + getattr(subsection_grade_model, field_name) + ) + return cleaned_data + class PersistentSubsectionGradeOverrideHistory(models.Model): """ diff --git a/lms/djangoapps/grades/services.py b/lms/djangoapps/grades/services.py index a2d9f4f9a1..074926c9fc 100644 --- a/lms/djangoapps/grades/services.py +++ b/lms/djangoapps/grades/services.py @@ -4,6 +4,7 @@ Grade service from datetime import datetime import pytz +from six import text_type from lms.djangoapps.utils import _get_key from opaque_keys.edx.keys import CourseKey, UsageKey @@ -46,17 +47,13 @@ class GradesService(object): 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) + # Verify that a corresponding subsection grade exists for the given user and usage_key + # Raises PersistentSubsectionGrade.DoesNotExist if it does not exist. + _ = self.get_subsection_grade(user_id, course_key_or_id, usage_key_or_id) - try: - return PersistentSubsectionGradeOverride.objects.get( - grade=grade - ) - except PersistentSubsectionGradeOverride.DoesNotExist: - return None + return PersistentSubsectionGradeOverride.get_override(user_id, usage_key) def override_subsection_grade(self, user_id, course_key_or_id, usage_key_or_id, earned_all=None, earned_graded=None): @@ -69,23 +66,17 @@ class GradesService(object): course_key = _get_key(course_key_or_id, CourseKey) usage_key = _get_key(usage_key_or_id, UsageKey) - grade = PersistentSubsectionGrade.objects.get( + grade = PersistentSubsectionGrade.read_grade( 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 - ) - - _ = PersistentSubsectionGradeOverrideHistory.objects.create( - override_id=override.id, + override = PersistentSubsectionGradeOverride.update_or_create_override( + requesting_user=None, + subsection_grade_model=grade, feature=PersistentSubsectionGradeOverrideHistory.PROCTORING, - action=PersistentSubsectionGradeOverrideHistory.CREATE_OR_UPDATE + 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. @@ -97,8 +88,8 @@ class GradesService(object): SUBSECTION_OVERRIDE_CHANGED.send( sender=None, user_id=user_id, - course_id=unicode(course_key), - usage_id=unicode(usage_key), + course_id=text_type(course_key), + usage_id=text_type(usage_key), only_if_higher=False, modified=override.modified, score_deleted=False, @@ -139,8 +130,8 @@ class GradesService(object): SUBSECTION_OVERRIDE_CHANGED.send( sender=None, user_id=user_id, - course_id=unicode(course_key), - usage_id=unicode(usage_key), + course_id=text_type(course_key), + usage_id=text_type(usage_key), only_if_higher=False, modified=datetime.now().replace(tzinfo=pytz.UTC), # Not used when score_deleted=True score_deleted=True, diff --git a/lms/djangoapps/grades/subsection_grade.py b/lms/djangoapps/grades/subsection_grade.py index 9b3f013c0b..9d1fb462f0 100644 --- a/lms/djangoapps/grades/subsection_grade.py +++ b/lms/djangoapps/grades/subsection_grade.py @@ -43,7 +43,7 @@ class SubsectionGradeBase(object): Returns whether any problem in this subsection was attempted by the student. """ - + # pylint: disable=no-member assert self.all_total is not None, ( "SubsectionGrade not fully populated yet. Call init_from_structure or init_from_model " "before use." @@ -182,24 +182,34 @@ class NonZeroSubsectionGrade(SubsectionGradeBase): block, ) + @staticmethod + def _aggregated_score_from_model(grade_model, is_graded): + """ + Helper method that returns `AggregatedScore` objects based on + the values in the given `grade_model`. If the given model + has an associated override, the values from the override are + used instead. + """ + score_type = 'graded' if is_graded else 'all' + grade_object = grade_model + if hasattr(grade_model, 'override'): + score_type = 'graded_override' if is_graded else 'all_override' + grade_object = grade_model.override + return AggregatedScore( + tw_earned=getattr(grade_object, 'earned_{}'.format(score_type)), + tw_possible=getattr(grade_object, 'possible_{}'.format(score_type)), + graded=is_graded, + first_attempted=grade_model.first_attempted, + ) + class ReadSubsectionGrade(NonZeroSubsectionGrade): """ Class for Subsection grades that are read from the database. """ def __init__(self, subsection, model, factory): - all_total = AggregatedScore( - tw_earned=model.earned_all, - tw_possible=model.possible_all, - graded=False, - first_attempted=model.first_attempted, - ) - graded_total = AggregatedScore( - tw_earned=model.earned_graded, - tw_possible=model.possible_graded, - graded=True, - first_attempted=model.first_attempted, - ) + all_total = self._aggregated_score_from_model(model, is_graded=False) + graded_total = self._aggregated_score_from_model(model, is_graded=True) override = model.override if hasattr(model, 'override') else None # save these for later since we compute problem_scores lazily @@ -216,6 +226,7 @@ class ReadSubsectionGrade(NonZeroSubsectionGrade): It means we look through the user-specific subtree of this subsection, taking into account which problems are visible to the user. """ + # pylint: disable=protected-access problem_scores = OrderedDict() for block in self.model.visible_blocks.blocks: problem_score = self._compute_block_score( @@ -254,29 +265,17 @@ class CreateSubsectionGrade(NonZeroSubsectionGrade): """ if self._should_persist_per_attempted(score_deleted, force_update_subsections): model = PersistentSubsectionGrade.update_or_create_grade(**self._persisted_model_params(student)) - self._update_aggregated_scores_from_model(model) + + if hasattr(model, 'override'): + # When we're doing an update operation, the PersistentSubsectionGrade model + # will be updated based on the problem_scores, but if a grade override + # exists that's related to the updated persistent grade, we need to update + # the aggregated scores for this object to reflect the override. + self.all_total = self._aggregated_score_from_model(model, is_graded=False) + self.graded_total = self._aggregated_score_from_model(model, is_graded=True) + return model - def _update_aggregated_scores_from_model(self, model): - """ - Updates this grade's `all_total` and `graded_total` attributes - to reflect the values from the related persisted model. - This is important, because PersistentSubsectionGradeOverrides - are only taken into account when reading/writing at the persistence layer, - so after we update a PersistentSubsectionGrade model (which could involve - writing values that are overridden by the PersistentSubsectionGradeOverride model) - we also need to update this grade object's attributes to reflect the - now (possibly) overriden values. - TODO: https://openedx.atlassian.net/browse/EDUCATOR-3835 - """ - self.all_total.earned = model.earned_all - self.all_total.possible = model.possible_all - self.all_total.first_attempted = model.first_attempted - - self.graded_total.earned = model.earned_graded - self.graded_total.possible = model.possible_graded - self.graded_total.first_attempted = model.first_attempted - @classmethod def bulk_create_models(cls, student, subsection_grades, course_key): """ diff --git a/lms/djangoapps/grades/tests/test_course_grade_factory.py b/lms/djangoapps/grades/tests/test_course_grade_factory.py index 5ca188635c..4b66ba447b 100644 --- a/lms/djangoapps/grades/tests/test_course_grade_factory.py +++ b/lms/djangoapps/grades/tests/test_course_grade_factory.py @@ -1,3 +1,6 @@ +""" +Tests for the CourseGradeFactory class. +""" import itertools import ddt @@ -95,7 +98,7 @@ class TestCourseGradeFactory(GradeTestBase): with self.assertNumQueries(5), mock_get_score(1, 2): _assert_read(expected_pass=False, expected_percent=0) # start off with grade of 0 - num_queries = 44 + num_queries = 47 with self.assertNumQueries(num_queries), mock_get_score(1, 2): grade_factory.update(self.request.user, self.course, force_update_subsections=True) @@ -109,14 +112,14 @@ class TestCourseGradeFactory(GradeTestBase): with self.assertNumQueries(5): _assert_read(expected_pass=True, expected_percent=0.5) # NOT updated to grade of .25 - num_queries = 23 + num_queries = 26 with self.assertNumQueries(num_queries), mock_get_score(2, 2): grade_factory.update(self.request.user, self.course, force_update_subsections=True) with self.assertNumQueries(5): _assert_read(expected_pass=True, expected_percent=1.0) # updated to grade of 1.0 - num_queries = 26 + num_queries = 29 with self.assertNumQueries(num_queries), mock_get_score(0, 0): # the subsection now is worth zero grade_factory.update(self.request.user, self.course, force_update_subsections=True) diff --git a/lms/djangoapps/grades/tests/test_models.py b/lms/djangoapps/grades/tests/test_models.py index fe5ba95bdd..129a6221ee 100644 --- a/lms/djangoapps/grades/tests/test_models.py +++ b/lms/djangoapps/grades/tests/test_models.py @@ -23,8 +23,10 @@ from lms.djangoapps.grades.models import ( PersistentCourseGrade, PersistentSubsectionGrade, PersistentSubsectionGradeOverride, + PersistentSubsectionGradeOverrideHistory, VisibleBlocks ) +from student.tests.factories import UserFactory from track.event_transaction_utils import get_event_transaction_id, get_event_transaction_type @@ -226,6 +228,7 @@ class PersistentSubsectionGradeTest(GradesModelTestCase): "visible_blocks": self.block_records, "first_attempted": datetime(2000, 1, 1, 12, 30, 45, tzinfo=pytz.UTC), } + self.user = UserFactory(id=self.params['user_id']) @ddt.data('course_version', 'subtree_edited_timestamp') def test_optional_fields(self, field): @@ -300,12 +303,31 @@ class PersistentSubsectionGradeTest(GradesModelTestCase): self._assert_tracker_emitted_event(tracker_mock, grade) def test_grade_override(self): + """ + Creating a subsection grade override should NOT change the score values + of the related PersistentSubsectionGrade. + """ grade = PersistentSubsectionGrade.update_or_create_grade(**self.params) - override = PersistentSubsectionGradeOverride(grade=grade, earned_all_override=0.0, earned_graded_override=0.0) - override.save() + override = PersistentSubsectionGradeOverride.update_or_create_override( + requesting_user=self.user, + subsection_grade_model=grade, + earned_all_override=0.0, + earned_graded_override=0.0, + feature=PersistentSubsectionGradeOverrideHistory.GRADEBOOK, + ) + grade = PersistentSubsectionGrade.update_or_create_grade(**self.params) - self.assertEqual(grade.earned_all, 0.0) - self.assertEqual(grade.earned_graded, 0.0) + self.assertEqual(self.params['earned_all'], grade.earned_all) + self.assertEqual(self.params['earned_graded'], grade.earned_graded) + + # Any score values that aren't specified should use the values from grade as defaults + self.assertEqual(0, override.earned_all_override) + self.assertEqual(0, override.earned_graded_override) + self.assertEqual(grade.possible_all, override.possible_all_override) + self.assertEqual(grade.possible_graded, override.possible_graded_override) + + # An override history record should be created + self.assertEqual(1, PersistentSubsectionGradeOverrideHistory.objects.filter(override_id=override.id).count()) def _assert_tracker_emitted_event(self, tracker_mock, grade): """ diff --git a/lms/djangoapps/grades/tests/test_services.py b/lms/djangoapps/grades/tests/test_services.py index 4d25e2b7a9..c110f5d726 100644 --- a/lms/djangoapps/grades/tests/test_services.py +++ b/lms/djangoapps/grades/tests/test_services.py @@ -24,6 +24,7 @@ class MockWaffleFlag(object): def __init__(self, state): self.state = state + # pylint: disable=unused-argument def is_enabled(self, course_key): return self.state @@ -38,7 +39,7 @@ class GradesServiceTests(ModuleStoreTestCase): def setUp(self): super(GradesServiceTests, self).setUp() self.service = GradesService() - self.course = CourseFactory.create(org='edX', number='DemoX', display_name='Demo_Course') + self.course = CourseFactory.create(org='edX', number='DemoX', display_name='Demo_Course', run='Spring2019') self.subsection = ItemFactory.create(parent=self.course, category="subsection", display_name="Subsection") self.user = UserFactory() self.grade = PersistentSubsectionGrade.update_or_create_grade( @@ -123,11 +124,11 @@ class GradesServiceTests(ModuleStoreTestCase): } ) - # test with id strings as parameters instead + # test with course key parameter as string 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) + usage_key_or_id=self.subsection.location )), { 'earned_all_override': override.earned_all_override, 'earned_graded_override': override.earned_graded_override diff --git a/lms/djangoapps/grades/tests/test_subsection_grade.py b/lms/djangoapps/grades/tests/test_subsection_grade.py index b3beb45dd2..8915026c07 100644 --- a/lms/djangoapps/grades/tests/test_subsection_grade.py +++ b/lms/djangoapps/grades/tests/test_subsection_grade.py @@ -1,8 +1,12 @@ +""" +Tests of the SubsectionGrade classes. +""" +from ddt import data, ddt, unpack + from ..models import PersistentSubsectionGrade from ..subsection_grade import CreateSubsectionGrade, ReadSubsectionGrade from .utils import mock_get_score from .base import GradeTestBase -from ddt import data, ddt, unpack @ddt diff --git a/lms/djangoapps/grades/tests/test_subsection_grade_factory.py b/lms/djangoapps/grades/tests/test_subsection_grade_factory.py index a9e9a618f1..7d376326ff 100644 --- a/lms/djangoapps/grades/tests/test_subsection_grade_factory.py +++ b/lms/djangoapps/grades/tests/test_subsection_grade_factory.py @@ -2,12 +2,18 @@ Tests for the SubsectionGradeFactory class. """ import ddt -from courseware.tests.test_submitting_problems import ProblemSubmissionTestMixin -from django.conf import settings -from lms.djangoapps.grades.config.tests.utils import persistent_grades_feature_flags from mock import patch +from django.conf import settings -from ..models import PersistentSubsectionGrade, PersistentSubsectionGradeOverride +from courseware.tests.test_submitting_problems import ProblemSubmissionTestMixin +from lms.djangoapps.grades.config.tests.utils import persistent_grades_feature_flags +from student.tests.factories import UserFactory + +from ..models import ( + PersistentSubsectionGrade, + PersistentSubsectionGradeOverride, + PersistentSubsectionGradeOverrideHistory, +) from ..subsection_grade_factory import ZeroSubsectionGrade from .base import GradeTestBase from .utils import mock_get_score @@ -125,10 +131,12 @@ class TestSubsectionGradeFactory(ProblemSubmissionTestMixin, GradeTestBase): self.assertEqual(3, persistent_grade.possible_graded) # Now create the override - PersistentSubsectionGradeOverride.objects.create( - grade=persistent_grade, + PersistentSubsectionGradeOverride.update_or_create_override( + UserFactory(), + persistent_grade, earned_graded_override=0, earned_all_override=0, + feature=PersistentSubsectionGradeOverrideHistory.GRADEBOOK, ) # Now, even if the problem scores interface gives us a 2/3, diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py index 888857d8ed..1b3306e5db 100644 --- a/lms/djangoapps/grades/tests/test_tasks.py +++ b/lms/djangoapps/grades/tests/test_tasks.py @@ -176,10 +176,10 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest self.assertEquals(mock_block_structure_create.call_count, 1) @ddt.data( - (ModuleStoreEnum.Type.mongo, 1, 34, True), - (ModuleStoreEnum.Type.mongo, 1, 34, False), - (ModuleStoreEnum.Type.split, 3, 34, True), - (ModuleStoreEnum.Type.split, 3, 34, False), + (ModuleStoreEnum.Type.mongo, 1, 35, True), + (ModuleStoreEnum.Type.mongo, 1, 35, False), + (ModuleStoreEnum.Type.split, 3, 35, True), + (ModuleStoreEnum.Type.split, 3, 35, False), ) @ddt.unpack def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls, create_multiple_subsections): @@ -191,8 +191,8 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest self._apply_recalculate_subsection_grade() @ddt.data( - (ModuleStoreEnum.Type.mongo, 1, 34), - (ModuleStoreEnum.Type.split, 3, 34), + (ModuleStoreEnum.Type.mongo, 1, 35), + (ModuleStoreEnum.Type.split, 3, 35), ) @ddt.unpack def test_query_counts_dont_change_with_more_content(self, default_store, num_mongo_calls, num_sql_calls): @@ -252,8 +252,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, 35), - (ModuleStoreEnum.Type.split, 3, 35), + (ModuleStoreEnum.Type.mongo, 1, 36), + (ModuleStoreEnum.Type.split, 3, 36), ) @ddt.unpack def test_persistent_grades_enabled_on_course(self, default_store, num_mongo_queries, num_sql_queries):