diff --git a/cms/djangoapps/contentstore/tests/test_course_settings.py b/cms/djangoapps/contentstore/tests/test_course_settings.py index cd5035192d..56b73bfea3 100644 --- a/cms/djangoapps/contentstore/tests/test_course_settings.py +++ b/cms/djangoapps/contentstore/tests/test_course_settings.py @@ -26,6 +26,7 @@ from course_modes.models import CourseMode from models.settings.course_grading import GRADING_POLICY_CHANGED_EVENT_TYPE, CourseGradingModel, hash_grading_policy from models.settings.course_metadata import CourseMetadata from models.settings.encoder import CourseSettingsEncoder +from models.settings.waffle import MATERIAL_RECOMPUTE_ONLY_FLAG from openedx.core.djangoapps.models.course_details import CourseDetails from openedx.core.djangoapps.waffle_utils.testutils import override_waffle_flag from student.roles import CourseInstructorRole, CourseStaffRole @@ -518,9 +519,9 @@ class CourseGradingTest(CourseTestCase): uuid.return_value = "mockUUID" self.course = CourseFactory.create(default_store=store) test_grader = CourseGradingModel.fetch(self.course.id) + # there should be no event raised after this call, since nothing got modified altered_grader = CourseGradingModel.update_from_json(self.course.id, test_grader.__dict__, self.user) self.assertDictEqual(test_grader.__dict__, altered_grader.__dict__, "Noop update") - grading_policy_1 = self._grading_policy_hash_for_course() test_grader.graders[0]['weight'] = test_grader.graders[0].get('weight') * 2 altered_grader = CourseGradingModel.update_from_json(self.course.id, test_grader.__dict__, self.user) self.assertDictEqual(test_grader.__dict__, altered_grader.__dict__, "Weight[0] * 2") @@ -548,11 +549,9 @@ class CourseGradingTest(CourseTestCase): # one for each of the calls to update_from_json() send_signal.assert_has_calls([ # pylint: disable=line-too-long - mock.call(sender=CourseGradingModel, user_id=self.user.id, course_key=self.course.id, grading_policy_hash=grading_policy_1), mock.call(sender=CourseGradingModel, user_id=self.user.id, course_key=self.course.id, grading_policy_hash=grading_policy_2), mock.call(sender=CourseGradingModel, user_id=self.user.id, course_key=self.course.id, grading_policy_hash=grading_policy_3), mock.call(sender=CourseGradingModel, user_id=self.user.id, course_key=self.course.id, grading_policy_hash=grading_policy_4), - mock.call(sender=CourseGradingModel, user_id=self.user.id, course_key=self.course.id, grading_policy_hash=grading_policy_4), # pylint: enable=line-too-long ]) @@ -569,10 +568,86 @@ class CourseGradingTest(CourseTestCase): 'event_transaction_id': 'mockUUID', } ) for policy_hash in ( - grading_policy_1, grading_policy_2, grading_policy_3, grading_policy_4, grading_policy_4 + grading_policy_2, grading_policy_3, grading_policy_4 ) ]) + @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) + def test_must_fire_grading_event_and_signal_multiple_type(self, store): + """ + Verifies that 'must_fire_grading_event_and_signal' ignores (returns False) if we modify + short_label and or name + use test_must_fire_grading_event_and_signal_multiple_type_2_split to run this test only + """ + self.course = CourseFactory.create(default_store=store) + # .raw_grader approximates what our UI sends down. It uses decimal representation of percent + # without it, the weights would be percentages + raw_grader_list = modulestore().get_course(self.course.id).raw_grader + course_grading_model = CourseGradingModel.fetch(self.course.id) + raw_grader_list[0]['type'] += '_foo' + raw_grader_list[0]['short_label'] += '_foo' + raw_grader_list[2]['type'] += '_foo' + raw_grader_list[3]['type'] += '_foo' + + result = CourseGradingModel.must_fire_grading_event_and_signal( + self.course.id, + raw_grader_list, + modulestore().get_course(self.course.id), + course_grading_model.__dict__ + ) + self.assertTrue(result) + + @override_waffle_flag(MATERIAL_RECOMPUTE_ONLY_FLAG, True) + @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) + def test_must_fire_grading_event_and_signal_multiple_type_waffle_on(self, store): + """ + Verifies that 'must_fire_grading_event_and_signal' ignores (returns False) if we modify + short_label and or name + use test_must_fire_grading_event_and_signal_multiple_type_2_split to run this test only + """ + self.course = CourseFactory.create(default_store=store) + # .raw_grader approximates what our UI sends down. It uses decimal representation of percent + # without it, the weights would be percentages + raw_grader_list = modulestore().get_course(self.course.id).raw_grader + course_grading_model = CourseGradingModel.fetch(self.course.id) + raw_grader_list[0]['type'] += '_foo' + raw_grader_list[0]['short_label'] += '_foo' + raw_grader_list[2]['type'] += '_foo' + raw_grader_list[3]['type'] += '_foo' + + result = CourseGradingModel.must_fire_grading_event_and_signal( + self.course.id, + raw_grader_list, + modulestore().get_course(self.course.id), + course_grading_model.__dict__ + ) + self.assertFalse(result) + + @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) + def test_must_fire_grading_event_and_signal_return_true(self, store): + """ + Verifies that 'must_fire_grading_event_and_signal' ignores (returns False) if we modify + short_label and or name + use _2_split suffix to run this test only + """ + self.course = CourseFactory.create(default_store=store) + # .raw_grader approximates what our UI sends down. It uses decimal representation of percent + # without it, the weights would be percentages + raw_grader_list = modulestore().get_course(self.course.id).raw_grader + course_grading_model = CourseGradingModel.fetch(self.course.id) + raw_grader_list[0]['weight'] *= 2 + raw_grader_list[0]['short_label'] += '_foo' + raw_grader_list[2]['type'] += '_foo' + raw_grader_list[3]['type'] += '_foo' + + result = CourseGradingModel.must_fire_grading_event_and_signal( + self.course.id, + raw_grader_list, + modulestore().get_course(self.course.id), + course_grading_model.__dict__ + ) + self.assertTrue(result) + @mock.patch('track.event_transaction_utils.uuid4') @mock.patch('models.settings.course_grading.tracker') @mock.patch('contentstore.signals.signals.GRADING_POLICY_CHANGED.send') @@ -583,7 +658,6 @@ class CourseGradingTest(CourseTestCase): self.course.id, test_grader.graders[1], self.user ) self.assertDictEqual(test_grader.graders[1], altered_grader, "Noop update") - grading_policy_1 = self._grading_policy_hash_for_course() test_grader.graders[1]['min_count'] = test_grader.graders[1].get('min_count') + 2 altered_grader = CourseGradingModel.update_grader_from_json( @@ -600,7 +674,6 @@ class CourseGradingTest(CourseTestCase): # one for each of the calls to update_grader_from_json() send_signal.assert_has_calls([ # pylint: disable=line-too-long - mock.call(sender=CourseGradingModel, user_id=self.user.id, course_key=self.course.id, grading_policy_hash=grading_policy_1), mock.call(sender=CourseGradingModel, user_id=self.user.id, course_key=self.course.id, grading_policy_hash=grading_policy_2), mock.call(sender=CourseGradingModel, user_id=self.user.id, course_key=self.course.id, grading_policy_hash=grading_policy_3), # pylint: enable=line-too-long @@ -617,7 +690,7 @@ class CourseGradingTest(CourseTestCase): 'event_transaction_id': 'mockUUID', 'event_transaction_type': 'edx.grades.grading_policy_changed', } - ) for policy_hash in {grading_policy_1, grading_policy_2, grading_policy_3} + ) for policy_hash in {grading_policy_2, grading_policy_3} ], any_order=True) @mock.patch('track.event_transaction_utils.uuid4') diff --git a/cms/djangoapps/models/settings/course_grading.py b/cms/djangoapps/models/settings/course_grading.py index 9601936c9e..f937c139af 100644 --- a/cms/djangoapps/models/settings/course_grading.py +++ b/cms/djangoapps/models/settings/course_grading.py @@ -12,6 +12,7 @@ from eventtracking import tracker from contentstore.signals.signals import GRADING_POLICY_CHANGED from track.event_transaction_utils import create_new_event_transaction_id from xmodule.modulestore.django import modulestore +from models.settings.waffle import material_recompute_only GRADING_POLICY_CHANGED_EVENT_TYPE = 'edx.grades.grading_policy_changed' @@ -69,7 +70,12 @@ class CourseGradingModel(object): descriptor = modulestore().get_course(course_key) graders_parsed = [CourseGradingModel.parse_grader(jsonele) for jsonele in jsondict['graders']] - + fire_signal = CourseGradingModel.must_fire_grading_event_and_signal( + course_key, + graders_parsed, + descriptor, + jsondict + ) descriptor.raw_grader = graders_parsed descriptor.grade_cutoffs = jsondict['grade_cutoffs'] @@ -78,10 +84,84 @@ class CourseGradingModel(object): CourseGradingModel.update_grace_period_from_json(course_key, jsondict['grace_period'], user) CourseGradingModel.update_minimum_grade_credit_from_json(course_key, jsondict['minimum_grade_credit'], user) - _grading_event_and_signal(course_key, user.id) + if fire_signal: + _grading_event_and_signal(course_key, user.id) return CourseGradingModel.fetch(course_key) + @staticmethod + def update_from_json_selective(course_key, jsondict, user): + """ + New version that doesn't fire change events when only name or short name are changed. + Decode the json into CourseGradingModel and save any changes. Returns the modified model. + Probably not the usual path for updates as it's too coarse grained. + """ + descriptor = modulestore().get_course(course_key) + + graders_parsed = [CourseGradingModel.parse_grader(jsonele) for jsonele in jsondict['graders']] + fire_signal = CourseGradingModel.must_fire_grading_event_and_signal( + course_key, + graders_parsed, + descriptor, + jsondict + ) + descriptor.raw_grader = graders_parsed + descriptor.grade_cutoffs = jsondict['grade_cutoffs'] + + modulestore().update_item(descriptor, user.id) + + CourseGradingModel.update_grace_period_from_json(course_key, jsondict['grace_period'], user) + + CourseGradingModel.update_minimum_grade_credit_from_json(course_key, jsondict['minimum_grade_credit'], user) + if fire_signal: + _grading_event_and_signal(course_key, user.id) + + return CourseGradingModel.fetch(course_key) + + @staticmethod + def must_fire_grading_event_and_signal(course_key, proposed_grader_settings, course_from_modulestore, jsondict): + """ + Detects if substantive enough changes were made to the proposed grader settings to warrant the firing of + _grading_event_and_sngal + Substantive changes mean the following values were changed: + drop_count, weight, min_count + An assignment type was added or removed + """ + if course_from_modulestore.grade_cutoffs != jsondict['grade_cutoffs'] or \ + len(proposed_grader_settings) != len(course_from_modulestore.raw_grader): + return True + + # because grading policy lists remain in the same order, we can do a single loop + for i in range(len(course_from_modulestore.raw_grader)): + if CourseGradingModel.must_fire_grading_event_and_signal_single_grader( + course_key, + proposed_grader_settings[i], + course_from_modulestore.raw_grader[i] + ): + return True + return False + + @staticmethod + def must_fire_grading_event_and_signal_single_grader( + course_key, + proposed_grader_settings, + existing_grader_settings + ): + """ + Detects changes in an individual grader vs an entire grading policy + Detects if substantive enough changes were made to the proposed grader settings to warrant the firing of + _grading_event_and_sngal + Substantive changes mean the following values were changed: + drop_count, weight, min_count + """ + if not material_recompute_only(course_key): + return True + if existing_grader_settings['drop_count'] != proposed_grader_settings['drop_count'] or \ + existing_grader_settings['weight'] != proposed_grader_settings['weight'] or \ + existing_grader_settings['min_count'] != proposed_grader_settings['min_count']: + return True + return False + @staticmethod def update_grader_from_json(course_key, grader, user): """ @@ -94,13 +174,20 @@ class CourseGradingModel(object): index = int(grader.get('id', len(descriptor.raw_grader))) grader = CourseGradingModel.parse_grader(grader) + fire_signal = True if index < len(descriptor.raw_grader): + fire_signal = CourseGradingModel.must_fire_grading_event_and_signal_single_grader( + course_key, + grader, + descriptor.raw_grader[index] + ) descriptor.raw_grader[index] = grader else: descriptor.raw_grader.append(grader) modulestore().update_item(descriptor, user.id) - _grading_event_and_signal(course_key, user.id) + if fire_signal: + _grading_event_and_signal(course_key, user.id) return CourseGradingModel.jsonize_grader(index, descriptor.raw_grader[index]) diff --git a/cms/djangoapps/models/settings/waffle.py b/cms/djangoapps/models/settings/waffle.py new file mode 100644 index 0000000000..d5a018e436 --- /dev/null +++ b/cms/djangoapps/models/settings/waffle.py @@ -0,0 +1,24 @@ +""" +Togglable settings for Course Grading behavior +""" +from openedx.core.djangoapps.waffle_utils import CourseWaffleFlag + + +WAFFLE_NAMESPACE = 'grades' + +# edx/edx-platform feature +MATERIAL_RECOMPUTE_ONLY = 'MATERIAL_RECOMPUTE_ONLY' + +MATERIAL_RECOMPUTE_ONLY_FLAG = CourseWaffleFlag( + waffle_namespace=WAFFLE_NAMESPACE, + flag_name=MATERIAL_RECOMPUTE_ONLY, +) + + +def material_recompute_only(course_key): + """ + Checks to see if the CourseWaffleFlag or Django setting for material recomputer only is enabled + """ + if MATERIAL_RECOMPUTE_ONLY_FLAG.is_enabled(course_key): + return True + return False