diff --git a/cms/djangoapps/models/settings/course_metadata.py b/cms/djangoapps/models/settings/course_metadata.py index 5d6f42c097..157bb3c7d1 100644 --- a/cms/djangoapps/models/settings/course_metadata.py +++ b/cms/djangoapps/models/settings/course_metadata.py @@ -100,9 +100,6 @@ class CourseMetadata(object): if not XBlockStudioConfigurationFlag.is_enabled(): filtered_list.append('allow_unsupported_xblocks') - if not settings.FEATURES.get('ENABLE_SUBSECTION_GRADES_SAVED'): - filtered_list.append('enable_subsection_grades_saved') - return filtered_list @classmethod diff --git a/cms/envs/bok_choy.py b/cms/envs/bok_choy.py index ad547db949..76bb50ca8d 100644 --- a/cms/envs/bok_choy.py +++ b/cms/envs/bok_choy.py @@ -93,9 +93,6 @@ FEATURES['LICENSING'] = True FEATURES['ENABLE_MOBILE_REST_API'] = True # Enable video bumper in Studio FEATURES['ENABLE_VIDEO_BUMPER'] = True # Enable video bumper in Studio settings -# Enable persistent subsection grades, so that feature can be tested. -FEATURES['ENABLE_SUBSECTION_GRADES_SAVED'] = True - # Enable partner support link in Studio footer PARTNER_SUPPORT_EMAIL = 'partner-support@example.com' diff --git a/cms/envs/common.py b/cms/envs/common.py index 373bf3104d..7be15a377d 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -212,12 +212,6 @@ FEATURES = { # Show Language selector 'SHOW_LANGUAGE_SELECTOR': False, - - # Temporary feature flag for disabling saving of subsection grades. - # There is also an advanced setting in the course module. The - # feature flag and the advanced setting must both be true for - # a course to use saved grades. - 'ENABLE_SUBSECTION_GRADES_SAVED': False, } ENABLE_JASMINE = False diff --git a/cms/envs/test.py b/cms/envs/test.py index e7d511ee0d..5074c48d98 100644 --- a/cms/envs/test.py +++ b/cms/envs/test.py @@ -322,9 +322,6 @@ SEARCH_ENGINE = "search.tests.mock_search_engine.MockSearchEngine" # teams feature FEATURES['ENABLE_TEAMS'] = True -# Enable persistent subsection grades, so that feature can be tested. -FEATURES['ENABLE_SUBSECTION_GRADES_SAVED'] = True - # Dummy secret key for dev/test SECRET_KEY = '85920908f28904ed733fe576320db18cabd7b6cd' diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index a6dcddb5cd..b1ef5a2f0a 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -801,17 +801,6 @@ class CourseFields(object): scope=Scope.settings ) - enable_subsection_grades_saved = Boolean( - display_name=_("Enable Subsection Grades Saved"), - help=_( - "Enter true or false. If this value is true, the robust " - "grades feature of saving subsection grades is enabled " - "for this course." - ), - default=False, - scope=Scope.settings - ) - learning_info = List( display_name=_("Course Learning Information"), help=_("Specify what student can learn from the course."), diff --git a/common/test/acceptance/pages/studio/settings_advanced.py b/common/test/acceptance/pages/studio/settings_advanced.py index 1fb20495e8..b70a25f78c 100644 --- a/common/test/acceptance/pages/studio/settings_advanced.py +++ b/common/test/acceptance/pages/studio/settings_advanced.py @@ -228,5 +228,4 @@ class AdvancedSettingsPage(CoursePage): 'create_zendesk_tickets', 'ccx_connector', 'enable_ccx', - 'enable_subsection_grades_saved', ] diff --git a/common/test/db_fixtures/grades.json b/common/test/db_fixtures/grades.json new file mode 100644 index 0000000000..353e087d2f --- /dev/null +++ b/common/test/db_fixtures/grades.json @@ -0,0 +1,11 @@ +[ + { + "pk": 1, + "model": "grades.persistentgradesenabledflag", + "fields": { + "enabled": true, + "enabled_for_all_courses": true, + "change_date": "2016-09-01" + } + } +] diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index 6d7e8ddb91..faf2bfa01d 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -229,18 +229,18 @@ class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase): # # of sql queries to default, # # of mongo queries, # ) - ('no_overrides', 1, True, False): (21, 6), - ('no_overrides', 2, True, False): (21, 6), - ('no_overrides', 3, True, False): (21, 6), - ('ccx', 1, True, False): (21, 6), - ('ccx', 2, True, False): (21, 6), - ('ccx', 3, True, False): (21, 6), - ('no_overrides', 1, False, False): (21, 6), - ('no_overrides', 2, False, False): (21, 6), - ('no_overrides', 3, False, False): (21, 6), - ('ccx', 1, False, False): (21, 6), - ('ccx', 2, False, False): (21, 6), - ('ccx', 3, False, False): (21, 6), + ('no_overrides', 1, True, False): (22, 6), + ('no_overrides', 2, True, False): (22, 6), + ('no_overrides', 3, True, False): (22, 6), + ('ccx', 1, True, False): (22, 6), + ('ccx', 2, True, False): (22, 6), + ('ccx', 3, True, False): (22, 6), + ('no_overrides', 1, False, False): (22, 6), + ('no_overrides', 2, False, False): (22, 6), + ('no_overrides', 3, False, False): (22, 6), + ('ccx', 1, False, False): (22, 6), + ('ccx', 2, False, False): (22, 6), + ('ccx', 3, False, False): (22, 6), } @@ -252,19 +252,19 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase): __test__ = True TEST_DATA = { - ('no_overrides', 1, True, False): (21, 3), - ('no_overrides', 2, True, False): (21, 3), - ('no_overrides', 3, True, False): (21, 3), - ('ccx', 1, True, False): (21, 3), - ('ccx', 2, True, False): (21, 3), - ('ccx', 3, True, False): (21, 3), - ('ccx', 1, True, True): (22, 3), - ('ccx', 2, True, True): (22, 3), - ('ccx', 3, True, True): (22, 3), - ('no_overrides', 1, False, False): (21, 3), - ('no_overrides', 2, False, False): (21, 3), - ('no_overrides', 3, False, False): (21, 3), - ('ccx', 1, False, False): (21, 3), - ('ccx', 2, False, False): (21, 3), - ('ccx', 3, False, False): (21, 3), + ('no_overrides', 1, True, False): (22, 3), + ('no_overrides', 2, True, False): (22, 3), + ('no_overrides', 3, True, False): (22, 3), + ('ccx', 1, True, False): (22, 3), + ('ccx', 2, True, False): (22, 3), + ('ccx', 3, True, False): (22, 3), + ('ccx', 1, True, True): (23, 3), + ('ccx', 2, True, True): (23, 3), + ('ccx', 3, True, True): (23, 3), + ('no_overrides', 1, False, False): (22, 3), + ('no_overrides', 2, False, False): (22, 3), + ('no_overrides', 3, False, False): (22, 3), + ('ccx', 1, False, False): (22, 3), + ('ccx', 2, False, False): (22, 3), + ('ccx', 3, False, False): (22, 3), } diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 51b2e009df..e5088f7441 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -1346,7 +1346,7 @@ class ProgressPageTests(ModuleStoreTestCase): self.assertContains(resp, u"Download Your Certificate") @ddt.data( - *itertools.product(((38, 4, True), (38, 4, False)), (True, False)) + *itertools.product(((39, 4, True), (39, 4, False)), (True, False)) ) @ddt.unpack def test_query_counts(self, (sql_calls, mongo_calls, self_paced), self_paced_enabled): diff --git a/lms/djangoapps/grades/admin.py b/lms/djangoapps/grades/admin.py new file mode 100644 index 0000000000..437d3ffe04 --- /dev/null +++ b/lms/djangoapps/grades/admin.py @@ -0,0 +1,27 @@ +""" +Django admin page for grades models +""" +from django.contrib import admin + +from config_models.admin import ConfigurationModelAdmin, KeyedConfigurationModelAdmin + +from lms.djangoapps.grades.config.models import CoursePersistentGradesFlag, PersistentGradesEnabledFlag +from lms.djangoapps.grades.config.forms import CoursePersistentGradesAdminForm + + +class CoursePersistentGradesAdmin(KeyedConfigurationModelAdmin): + """ + Admin for enabling subsection grades on a course-by-course basis. + Allows searching by course id. + """ + form = CoursePersistentGradesAdminForm + search_fields = ['course_id'] + fieldsets = ( + (None, { + 'fields': ('course_id', 'enabled'), + 'description': 'Enter a valid course id. If it is invalid, an error message will display.' + }), + ) + +admin.site.register(CoursePersistentGradesFlag, CoursePersistentGradesAdmin) +admin.site.register(PersistentGradesEnabledFlag, ConfigurationModelAdmin) diff --git a/lms/djangoapps/grades/config/__init__.py b/lms/djangoapps/grades/config/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/lms/djangoapps/grades/config/forms.py b/lms/djangoapps/grades/config/forms.py new file mode 100644 index 0000000000..5db1d6a29d --- /dev/null +++ b/lms/djangoapps/grades/config/forms.py @@ -0,0 +1,37 @@ +""" +Defines a form for providing validation of subsection grade templates. +""" +import logging + +from django import forms + +from lms.djangoapps.grades.config.models import CoursePersistentGradesFlag + +from opaque_keys import InvalidKeyError +from xmodule.modulestore.django import modulestore +from opaque_keys.edx.locator import CourseLocator + +log = logging.getLogger(__name__) + + +class CoursePersistentGradesAdminForm(forms.ModelForm): + """Input form for subsection grade enabling, allowing us to verify data.""" + + class Meta(object): + model = CoursePersistentGradesFlag + fields = '__all__' + + def clean_course_id(self): + """Validate the course id""" + cleaned_id = self.cleaned_data["course_id"] + try: + course_key = CourseLocator.from_string(cleaned_id) + except InvalidKeyError: + msg = u'Course id invalid. Entered course id was: "{0}."'.format(cleaned_id) + raise forms.ValidationError(msg) + + if not modulestore().has_course(course_key): + msg = u'Course not found. Entered course id was: "{0}". '.format(course_key.to_deprecated_string()) + raise forms.ValidationError(msg) + + return course_key diff --git a/lms/djangoapps/grades/config/models.py b/lms/djangoapps/grades/config/models.py new file mode 100644 index 0000000000..05f852d08a --- /dev/null +++ b/lms/djangoapps/grades/config/models.py @@ -0,0 +1,70 @@ +""" +Models for configuration of the feature flags +controlling persistent grades. +""" +from config_models.models import ConfigurationModel +from django.db.models import BooleanField +from xmodule_django.models import CourseKeyField + + +class PersistentGradesEnabledFlag(ConfigurationModel): + """ + Enables persistent grades across the platform. + When this feature flag is set to true, individual courses + must also have persistent grades enabled for the + feature to take effect. + """ + # this field overrides course-specific settings to enable the feature for all courses + enabled_for_all_courses = BooleanField(default=False) + + @classmethod + def feature_enabled(cls, course_id=None): + """ + Looks at the currently active configuration model to determine whether + the persistent grades feature is available. + + If the flag is not enabled, the feature is not available. + If the flag is enabled and the provided course_id is for an course + with persistent grades enabled, the feature is available. + If the flag is enabled and no course ID is given, + we return True since the global setting is enabled. + """ + if not PersistentGradesEnabledFlag.is_enabled(): + return False + elif not PersistentGradesEnabledFlag.current().enabled_for_all_courses and course_id: + try: + return CoursePersistentGradesFlag.objects.get(course_id=course_id).enabled + except CoursePersistentGradesFlag.DoesNotExist: + return False + return True + + class Meta(object): + app_label = "grades" + + def __unicode__(self): + current_model = PersistentGradesEnabledFlag.current() + return u"PersistentGradesEnabledFlag: enabled {}".format( + current_model.is_enabled() + ) + + +class CoursePersistentGradesFlag(ConfigurationModel): + """ + Enables persistent grades for a specific + course. Only has an effect if the general + flag above is set to True. + """ + KEY_FIELDS = ('course_id',) + + class Meta(object): + app_label = "grades" + + # The course that these features are attached to. + course_id = CourseKeyField(max_length=255, db_index=True, unique=True) + + def __unicode__(self): + not_en = "Not " + if self.enabled: + not_en = "" + # pylint: disable=no-member + return u"Course '{}': Persistent Grades {}Enabled".format(self.course_id.to_deprecated_string(), not_en) diff --git a/lms/djangoapps/grades/config/tests/__init__.py b/lms/djangoapps/grades/config/tests/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/lms/djangoapps/grades/config/tests/test_models.py b/lms/djangoapps/grades/config/tests/test_models.py new file mode 100644 index 0000000000..2e4d5614c6 --- /dev/null +++ b/lms/djangoapps/grades/config/tests/test_models.py @@ -0,0 +1,50 @@ +""" +Tests for the models that control the +persistent grading feature. +""" +import ddt + +from django.test import TestCase +from opaque_keys.edx.locator import CourseLocator +from lms.djangoapps.grades.config.models import PersistentGradesEnabledFlag +from lms.djangoapps.grades.config.tests.utils import persistent_grades_feature_flags + + +@ddt.ddt +class PersistentGradesFeatureFlagTests(TestCase): + """ + Tests the behavior of the feature flags for persistent grading. + These are set via Django admin settings. + """ + def setUp(self): + super(PersistentGradesFeatureFlagTests, self).setUp() + self.course_id_1 = CourseLocator(org="edx", course="course", run="run") + self.course_id_2 = CourseLocator(org="edx", course="course2", run="run") + + @ddt.data( + (True, True, True), + (True, True, False), + (True, False, True), + (True, False, False), + (False, True, True), + (False, False, True), + (False, True, False), + (False, False, False), + ) + @ddt.unpack + def test_persistent_grades_feature_flags(self, global_flag, enabled_for_all_courses, enabled_for_course_1): + with persistent_grades_feature_flags( + global_flag=global_flag, + enabled_for_all_courses=enabled_for_all_courses, + course_id=self.course_id_1, + enabled_for_course=enabled_for_course_1 + ): + self.assertEqual(PersistentGradesEnabledFlag.feature_enabled(), global_flag) + self.assertEqual( + PersistentGradesEnabledFlag.feature_enabled(self.course_id_1), + global_flag and (enabled_for_all_courses or enabled_for_course_1) + ) + self.assertEqual( + PersistentGradesEnabledFlag.feature_enabled(self.course_id_2), + global_flag and enabled_for_all_courses + ) diff --git a/lms/djangoapps/grades/config/tests/utils.py b/lms/djangoapps/grades/config/tests/utils.py new file mode 100644 index 0000000000..a4cccf6701 --- /dev/null +++ b/lms/djangoapps/grades/config/tests/utils.py @@ -0,0 +1,24 @@ +""" +Provides helper functions for tests that want +to configure flags related to persistent grading. +""" +from lms.djangoapps.grades.config.models import PersistentGradesEnabledFlag, CoursePersistentGradesFlag +from contextlib import contextmanager + + +@contextmanager +def persistent_grades_feature_flags( + global_flag, + enabled_for_all_courses=False, + course_id=None, + enabled_for_course=False +): + """ + Most test cases will use a single call to this manager, + as they need to set the global setting and the course-specific + setting for a single course. + """ + PersistentGradesEnabledFlag.objects.create(enabled=global_flag, enabled_for_all_courses=enabled_for_all_courses) + if course_id: + CoursePersistentGradesFlag.objects.create(course_id=course_id, enabled=enabled_for_course) + yield diff --git a/lms/djangoapps/grades/migrations/0003_coursepersistentgradesflag_persistentgradesenabledflag.py b/lms/djangoapps/grades/migrations/0003_coursepersistentgradesflag_persistentgradesenabledflag.py new file mode 100644 index 0000000000..161e28e121 --- /dev/null +++ b/lms/djangoapps/grades/migrations/0003_coursepersistentgradesflag_persistentgradesenabledflag.py @@ -0,0 +1,38 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import migrations, models +import django.db.models.deletion +from django.conf import settings +import xmodule_django.models + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('grades', '0002_rename_last_edited_field'), + ] + + operations = [ + migrations.CreateModel( + name='CoursePersistentGradesFlag', + fields=[ + ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)), + ('change_date', models.DateTimeField(auto_now_add=True, verbose_name='Change date')), + ('enabled', models.BooleanField(default=False, verbose_name='Enabled')), + ('course_id', xmodule_django.models.CourseKeyField(unique=True, max_length=255, db_index=True)), + ('changed_by', models.ForeignKey(on_delete=django.db.models.deletion.PROTECT, editable=False, to=settings.AUTH_USER_MODEL, null=True, verbose_name='Changed by')), + ], + ), + migrations.CreateModel( + name='PersistentGradesEnabledFlag', + fields=[ + ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)), + ('change_date', models.DateTimeField(auto_now_add=True, verbose_name='Change date')), + ('enabled', models.BooleanField(default=False, verbose_name='Enabled')), + ('enabled_for_all_courses', models.BooleanField(default=False)), + ('changed_by', models.ForeignKey(on_delete=django.db.models.deletion.PROTECT, editable=False, to=settings.AUTH_USER_MODEL, null=True, verbose_name='Changed by')), + ], + ), + ] diff --git a/lms/djangoapps/grades/new/course_grade.py b/lms/djangoapps/grades/new/course_grade.py index 38a398625f..c648007a3a 100644 --- a/lms/djangoapps/grades/new/course_grade.py +++ b/lms/djangoapps/grades/new/course_grade.py @@ -7,6 +7,7 @@ from django.conf import settings from lazy import lazy from logging import getLogger from lms.djangoapps.course_blocks.api import get_course_blocks +from lms.djangoapps.grades.config.models import PersistentGradesEnabledFlag from openedx.core.djangoapps.signals.signals import GRADES_UPDATED from xmodule import block_metadata_utils @@ -228,7 +229,7 @@ class CourseGradeFactory(object): """ Returns the saved grade for the given course and student. """ - if settings.FEATURES.get('ENABLE_SUBSECTION_GRADES_SAVED') and course.enable_subsection_grades_saved: + if PersistentGradesEnabledFlag.feature_enabled(course.id): # TODO LATER Retrieve the saved grade for the course, if it exists. _pretend_to_save_course_grades() diff --git a/lms/djangoapps/grades/new/subsection_grade.py b/lms/djangoapps/grades/new/subsection_grade.py index 1ff46d4b40..a8d1bef12f 100644 --- a/lms/djangoapps/grades/new/subsection_grade.py +++ b/lms/djangoapps/grades/new/subsection_grade.py @@ -10,6 +10,7 @@ from course_blocks.api import get_course_blocks from courseware.model_data import ScoresClient from lms.djangoapps.grades.scores import get_score, possibly_scored from lms.djangoapps.grades.models import BlockRecord, PersistentSubsectionGrade +from lms.djangoapps.grades.config.models import PersistentGradesEnabledFlag from student.models import anonymous_id_for_user, User from submissions import api as submissions_api from xmodule import block_metadata_utils, graders @@ -180,7 +181,7 @@ class SubsectionGradeFactory(object): from courseware.courses import get_course_by_id # avoids circular import with courseware.py course = get_course_by_id(course_key, depth=0) # save ourselves the extra queries if the course does not use subsection grades - if not course.enable_subsection_grades_saved: + if not PersistentGradesEnabledFlag.feature_enabled(course.id): return course_structure = get_course_blocks(self.student, usage_key) @@ -201,7 +202,7 @@ class SubsectionGradeFactory(object): """ Returns the saved grade for the student and subsection. """ - if settings.FEATURES.get('ENABLE_SUBSECTION_GRADES_SAVED') and course.enable_subsection_grades_saved: + if PersistentGradesEnabledFlag.feature_enabled(course.id): try: model = PersistentSubsectionGrade.read_grade( user_id=self.student.id, @@ -217,7 +218,7 @@ class SubsectionGradeFactory(object): """ Updates the saved grade for the student and subsection. """ - if settings.FEATURES.get('ENABLE_SUBSECTION_GRADES_SAVED') and course.enable_subsection_grades_saved: + if PersistentGradesEnabledFlag.feature_enabled(course.id): subsection_grade.save(self.student, subsection, course) def _prefetch_scores(self, course_structure, course): diff --git a/lms/djangoapps/grades/signals.py b/lms/djangoapps/grades/signals.py index ba034a8c52..dce9a7bdf5 100644 --- a/lms/djangoapps/grades/signals.py +++ b/lms/djangoapps/grades/signals.py @@ -3,6 +3,7 @@ Grades related signals. """ from django.conf import settings from django.dispatch import receiver, Signal +from lms.djangoapps.grades.config.models import PersistentGradesEnabledFlag from logging import getLogger from opaque_keys.edx.locator import CourseLocator from opaque_keys.edx.keys import UsageKey @@ -122,14 +123,15 @@ def recalculate_subsection_grade_handler(sender, **kwargs): # pylint: disable=u - course_id: Unicode string representing the course - usage_id: Unicode string indicating the courseware instance """ - if not settings.FEATURES.get('ENABLE_SUBSECTION_GRADES_SAVED', False): - return try: course_id = kwargs.get('course_id', None) usage_id = kwargs.get('usage_id', None) student = kwargs.get('user', None) course_key = CourseLocator.from_string(course_id) + if not PersistentGradesEnabledFlag.feature_enabled(course_key): + return + usage_key = UsageKey.from_string(usage_id).replace(course_key=course_key) from lms.djangoapps.grades.new.subsection_grade import SubsectionGradeFactory diff --git a/lms/djangoapps/grades/tests/test_models.py b/lms/djangoapps/grades/tests/test_models.py index b9889c7933..5779c2e86b 100644 --- a/lms/djangoapps/grades/tests/test_models.py +++ b/lms/djangoapps/grades/tests/test_models.py @@ -12,7 +12,12 @@ from django.db.utils import IntegrityError from django.test import TestCase from opaque_keys.edx.locator import CourseLocator, BlockUsageLocator -from lms.djangoapps.grades.models import BlockRecord, BlockRecordSet, PersistentSubsectionGrade, VisibleBlocks +from lms.djangoapps.grades.models import ( + BlockRecord, + BlockRecordSet, + PersistentSubsectionGrade, + VisibleBlocks +) class GradesModelTestCase(TestCase): diff --git a/lms/djangoapps/grades/tests/test_new.py b/lms/djangoapps/grades/tests/test_new.py index e043865045..e64ec033c8 100644 --- a/lms/djangoapps/grades/tests/test_new.py +++ b/lms/djangoapps/grades/tests/test_new.py @@ -3,12 +3,12 @@ Test saved subsection grade functionality. """ import ddt -from django.conf import settings from mock import patch from capa.tests.response_xml_factory import MultipleChoiceResponseXMLFactory from courseware.tests.helpers import get_request_for_user from lms.djangoapps.course_blocks.api import get_course_blocks +from lms.djangoapps.grades.config.tests.utils import persistent_grades_feature_flags from student.models import CourseEnrollment from student.tests.factories import UserFactory from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase @@ -62,7 +62,6 @@ class GradeTestBase(SharedModuleStoreTestCase): self.client.login(username=self.request.user.username, password="test") self.subsection_grade_factory = SubsectionGradeFactory(self.request.user) self.course_structure = get_course_blocks(self.request.user, self.course.location) - self.course.enable_subsection_grades_saved = True CourseEnrollment.enroll(self.request.user, self.course.id) @@ -90,10 +89,14 @@ class TestCourseGradeFactory(GradeTestBase): # Grades are only saved if the feature flag and the advanced setting are # both set to True. grade_factory = CourseGradeFactory(self.request.user) - with patch('lms.djangoapps.grades.new.course_grade._pretend_to_save_course_grades') as mock_save_grades: - with patch.dict(settings.FEATURES, {'ENABLE_SUBSECTION_GRADES_SAVED': feature_flag}): - with patch.object(self.course, 'enable_subsection_grades_saved', new=course_setting): - grade_factory.create(self.course) + with persistent_grades_feature_flags( + global_flag=feature_flag, + enabled_for_all_courses=False, + course_id=self.course.id, + enabled_for_course=course_setting + ): + with patch('lms.djangoapps.grades.new.course_grade._pretend_to_save_course_grades') as mock_save_grades: + grade_factory.create(self.course) self.assertEqual(mock_save_grades.called, feature_flag and course_setting) @@ -121,26 +124,40 @@ class SubsectionGradeFactoryTest(GradeTestBase): """ Tests to ensure that a persistent subsection grade is created, saved, then fetched on re-request. """ - with patch( - 'lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory._save_grade', - wraps=self.subsection_grade_factory._save_grade # pylint: disable=protected-access - ) as mock_save_grades: + with persistent_grades_feature_flags( + global_flag=True, + enabled_for_all_courses=False, + course_id=self.course.id, + enabled_for_course=True + ): with patch( - 'lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory._get_saved_grade', - wraps=self.subsection_grade_factory._get_saved_grade # pylint: disable=protected-access - ) as mock_get_saved_grade: - with self.assertNumQueries(19): - grade_a = self.subsection_grade_factory.create(self.sequence, self.course_structure, self.course) - self.assertTrue(mock_get_saved_grade.called) - self.assertTrue(mock_save_grades.called) + 'lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory._save_grade', + wraps=self.subsection_grade_factory._save_grade # pylint: disable=protected-access + ) as mock_save_grades: + with patch( + 'lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory._get_saved_grade', + wraps=self.subsection_grade_factory._get_saved_grade # pylint: disable=protected-access + ) as mock_get_saved_grade: + with self.assertNumQueries(22): + grade_a = self.subsection_grade_factory.create( + self.sequence, + self.course_structure, + self.course + ) + self.assertTrue(mock_get_saved_grade.called) + self.assertTrue(mock_save_grades.called) - mock_get_saved_grade.reset_mock() - mock_save_grades.reset_mock() + mock_get_saved_grade.reset_mock() + mock_save_grades.reset_mock() - with self.assertNumQueries(3): - grade_b = self.subsection_grade_factory.create(self.sequence, self.course_structure, self.course) - self.assertTrue(mock_get_saved_grade.called) - self.assertFalse(mock_save_grades.called) + with self.assertNumQueries(4): + grade_b = self.subsection_grade_factory.create( + self.sequence, + self.course_structure, + self.course + ) + self.assertTrue(mock_get_saved_grade.called) + self.assertFalse(mock_save_grades.called) self.assertEqual(grade_a.url_name, grade_b.url_name) self.assertEqual(grade_a.all_total, grade_b.all_total) @@ -158,9 +175,13 @@ class SubsectionGradeFactoryTest(GradeTestBase): with patch( 'lms.djangoapps.grades.models.PersistentSubsectionGrade.read_grade' ) as mock_read_saved_grade: - with patch.dict(settings.FEATURES, {'ENABLE_SUBSECTION_GRADES_SAVED': feature_flag}): - with patch.object(self.course, 'enable_subsection_grades_saved', new=course_setting): - self.subsection_grade_factory.create(self.sequence, self.course_structure, self.course) + with persistent_grades_feature_flags( + global_flag=feature_flag, + enabled_for_all_courses=False, + course_id=self.course.id, + enabled_for_course=course_setting + ): + self.subsection_grade_factory.create(self.sequence, self.course_structure, self.course) self.assertEqual(mock_read_saved_grade.called, feature_flag and course_setting) diff --git a/lms/djangoapps/grades/tests/test_signals.py b/lms/djangoapps/grades/tests/test_signals.py index 5a62cb4fe8..68c85dfecb 100644 --- a/lms/djangoapps/grades/tests/test_signals.py +++ b/lms/djangoapps/grades/tests/test_signals.py @@ -5,6 +5,7 @@ Tests for the score change signals defined in the courseware models module. import ddt from unittest import skip from django.test import TestCase +from lms.djangoapps.grades.config.models import PersistentGradesEnabledFlag, CoursePersistentGradesFlag from mock import patch, MagicMock from student.models import anonymous_id_for_user from student.tests.factories import UserFactory @@ -171,6 +172,7 @@ class ScoreChangedUpdatesSubsectionGradeTest(ModuleStoreTestCase): def setUp(self): super(ScoreChangedUpdatesSubsectionGradeTest, self).setUp() self.user = UserFactory() + PersistentGradesEnabledFlag.objects.create(enabled=True) def set_up_course(self, enable_subsection_grades=True): """ @@ -181,7 +183,8 @@ class ScoreChangedUpdatesSubsectionGradeTest(ModuleStoreTestCase): org='edx', name='course', run='run', - metadata={'enable_subsection_grades_saved': enable_subsection_grades}) + ) + CoursePersistentGradesFlag.objects.create(course_id=self.course.id, enabled=enable_subsection_grades) self.chapter = ItemFactory.create(parent=self.course, category="chapter", display_name="Chapter") self.sequential = ItemFactory.create(parent=self.chapter, category='sequential', display_name="Open Sequential") @@ -203,7 +206,7 @@ class ScoreChangedUpdatesSubsectionGradeTest(ModuleStoreTestCase): def test_subsection_grade_updated_on_signal(self, default_store): with self.store.default_store(default_store): self.set_up_course() - with check_mongo_calls(2) and self.assertNumQueries(15): + with check_mongo_calls(2) and self.assertNumQueries(19): recalculate_subsection_grade_handler(None, **self.score_changed_kwargs) @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) @@ -212,14 +215,14 @@ class ScoreChangedUpdatesSubsectionGradeTest(ModuleStoreTestCase): self.set_up_course() ItemFactory.create(parent=self.sequential, category='problem', display_name='problem2') ItemFactory.create(parent=self.sequential, category='problem', display_name='problem3') - with check_mongo_calls(2) and self.assertNumQueries(15): + with check_mongo_calls(2) and self.assertNumQueries(19): recalculate_subsection_grade_handler(None, **self.score_changed_kwargs) @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) def test_subsection_grades_not_enabled_on_course(self, default_store): with self.store.default_store(default_store): self.set_up_course(enable_subsection_grades=False) - with check_mongo_calls(2) and self.assertNumQueries(0): + with check_mongo_calls(2) and self.assertNumQueries(2): recalculate_subsection_grade_handler(None, **self.score_changed_kwargs) @skip("Pending completion of TNL-5089") @@ -231,18 +234,18 @@ class ScoreChangedUpdatesSubsectionGradeTest(ModuleStoreTestCase): ) @ddt.unpack def test_score_changed_sent_with_feature_flag(self, default_store, feature_flag): - with patch.dict('django.conf.settings.FEATURES', {'ENABLE_SUBSECTION_GRADES_SAVED': feature_flag}): - with self.store.default_store(default_store): - self.set_up_course() - with check_mongo_calls(0) and self.assertNumQueries(19 if feature_flag else 1): - SCORE_CHANGED.send(sender=None, **self.score_changed_kwargs) + PersistentGradesEnabledFlag.objects.create(enabled=feature_flag) + with self.store.default_store(default_store): + self.set_up_course() + with check_mongo_calls(0) and self.assertNumQueries(19 if feature_flag else 1): + SCORE_CHANGED.send(sender=None, **self.score_changed_kwargs) @ddt.data( - ('points_possible', 2, 15), - ('points_earned', 2, 15), - ('user', 0, 0), + ('points_possible', 2, 19), + ('points_earned', 2, 19), + ('user', 0, 3), ('course_id', 0, 0), - ('usage_id', 0, 0), + ('usage_id', 0, 2), ) @ddt.unpack def test_missing_kwargs(self, kwarg, expected_mongo_calls, expected_sql_calls): diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index f4caa11407..d9907ae012 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -1671,7 +1671,7 @@ class TestCertificateGeneration(InstructorTaskModuleTestCase): 'skipped': 2 } - with self.assertNumQueries(150): + with self.assertNumQueries(151): self.assertCertificatesGenerated(task_input, expected_results) @ddt.data( diff --git a/lms/envs/bok_choy.py b/lms/envs/bok_choy.py index 2758c77380..37c0fe861f 100644 --- a/lms/envs/bok_choy.py +++ b/lms/envs/bok_choy.py @@ -124,9 +124,6 @@ FEATURES['ENABLE_TEAMS'] = True # Enable custom content licensing FEATURES['LICENSING'] = True -# Enable persistent subsection grades, so that feature can be tested. -FEATURES['ENABLE_SUBSECTION_GRADES_SAVED'] = True - # Use the auto_auth workflow for creating users and logging them in FEATURES['AUTOMATIC_AUTH_FOR_TESTING'] = True diff --git a/lms/envs/common.py b/lms/envs/common.py index 94c01207ac..631a36b206 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -356,12 +356,6 @@ FEATURES = { # lives in the Extended table, saving the frontend from # making multiple queries. 'ENABLE_READING_FROM_MULTIPLE_HISTORY_TABLES': True, - - # Temporary feature flag for disabling saving of subsection grades. - # There is also an advanced setting in the course module. The - # feature flag and the advanced setting must both be true for - # a course to use saved grades. - 'ENABLE_SUBSECTION_GRADES_SAVED': False, } # Ignore static asset files on import which match this pattern diff --git a/lms/envs/test.py b/lms/envs/test.py index d6388c3302..4983e6787e 100644 --- a/lms/envs/test.py +++ b/lms/envs/test.py @@ -78,9 +78,6 @@ FEATURES['ENABLE_COMBINED_LOGIN_REGISTRATION'] = True # Enable the milestones app in tests to be consistent with it being enabled in production FEATURES['MILESTONES_APP'] = True -# Enable persistent subsection grades, so that feature can be tested. -FEATURES['ENABLE_SUBSECTION_GRADES_SAVED'] = True - # Need wiki for courseware views to work. TODO (vshnayder): shouldn't need it. WIKI_ENABLED = True