From fa2ee761d923b1074a8c3c2164ee5bdaaa2e7b84 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 28 Jan 2019 15:24:47 -0500 Subject: [PATCH] Allow overriding Content Type Gating and Course Duration Limits on a per-course (rather than per-course-run) basis REVMI-94 --- .../djangoapps/config_model_utils/models.py | 152 ++++++++++++------ .../migrations/0007_auto_20190311_1919.py | 40 +++++ .../content_type_gating/tests/test_models.py | 8 +- .../migrations/0007_auto_20190311_1919.py | 40 +++++ .../tests/test_models.py | 8 +- 5 files changed, 195 insertions(+), 53 deletions(-) create mode 100644 openedx/features/content_type_gating/migrations/0007_auto_20190311_1919.py create mode 100644 openedx/features/course_duration_limits/migrations/0007_auto_20190311_1919.py diff --git a/openedx/core/djangoapps/config_model_utils/models.py b/openedx/core/djangoapps/config_model_utils/models.py index 909099c328..d532c5184f 100644 --- a/openedx/core/djangoapps/config_model_utils/models.py +++ b/openedx/core/djangoapps/config_model_utils/models.py @@ -29,7 +29,8 @@ class Provenance(Enum): """ Provenance enum """ - course = 'Course' + run = 'Course Run' + org_course = 'Org/Course' org = 'Org' site = 'Site' global_ = 'Global' @@ -38,7 +39,7 @@ class Provenance(Enum): class StackedConfigurationModel(ConfigurationModel): """ - A ConfigurationModel that stacks Global, Site, Org, and Course level + A ConfigurationModel that stacks Global, Site, Org, Course, and Course Run level configuration values. """ class Meta(object): @@ -46,24 +47,58 @@ class StackedConfigurationModel(ConfigurationModel): indexes = [ # This index optimizes the .object.current_set() query # by preventing a filesort - models.Index(fields=['site', 'org', 'course']) + models.Index(fields=['site', 'org', 'course']), + models.Index(fields=['site', 'org', 'org_course', 'course']) ] - KEY_FIELDS = ('site', 'org', 'course') + KEY_FIELDS = ('site', 'org', 'org_course', 'course') STACKABLE_FIELDS = ('enabled',) enabled = models.NullBooleanField(default=None, verbose_name=_("Enabled")) - site = models.ForeignKey(Site, on_delete=models.CASCADE, null=True, blank=True) - org = models.CharField(max_length=255, db_index=True, null=True, blank=True) + site = models.ForeignKey( + Site, + on_delete=models.CASCADE, + null=True, + blank=True, + help_text=_( + "Configure values for all course runs associated with this site." + ), + ) + org = models.CharField( + max_length=255, + db_index=True, + null=True, + blank=True, + help_text=_( + "Configure values for all course runs associated with this " + "Organization. This is the organization string (i.e. edX, MITx)." + ) + ) + org_course = models.CharField( + max_length=255, + db_index=True, + null=True, + blank=True, + verbose_name=_("Course in Org"), + help_text=_( + "Configure values for all course runs associated with this course. " + "This is should be formatted as 'org/course' (i.e. MITx/6.002x, HarvardX/CS50)." + ), + ) course = models.ForeignKey( CourseOverview, on_delete=models.DO_NOTHING, null=True, blank=True, + verbose_name=_("Course Run"), + help_text=_( + "Configure values for this course run. This should be " + "formatted as the CourseKey (i.e. course-v1://MITx+6.002x+2019_Q1)" + ), ) @classmethod - def current(cls, site=None, org=None, course_key=None): # pylint: disable=arguments-differ + def current(cls, site=None, org=None, org_course=None, course_key=None): # pylint: disable=arguments-differ """ Return the current overridden configuration at the specified level. @@ -74,8 +109,9 @@ class StackedConfigurationModel(ConfigurationModel): Global: Feature Disabled edx.org (Site): Feature Enabled - Harvard (org): Feature Disabled - CS50 (course): Feature Enabled + HarvardX (org): Feature Disabled + HarvardX/CS50 (org_course): Feature Enabled + CS50 in 2018_Q1 (course run): Feature Disabled Assuming these values had been properly configured, these calls would result @@ -84,10 +120,15 @@ class StackedConfigurationModel(ConfigurationModel): MyStackedConfig.current(site=Site(domain='whitelabel.edx.org')) # False -- value derived from global setting MyStackedConfig.current(org='HarvardX') # False MyStackedConfig.current(org='MITx') # True -- value derived from edx.org site - MyStackedConfig.current(course=CourseKey(org='HarvardX', course='CS50', run='2018_Q1')) # True + MyStackedConfig.current(org_course='HarvardX/CS50') # True + MyStackedConfig.current(org_course='HarvardX/Bio101') # False -- value derived from HarvardX setting + MyStackedConfig.current(course_key=CourseKey(org='HarvardX', course='CS50', run='2018_Q1')) # False + MyStackedConfig.current( + course_key=CourseKey(org='HarvardX', course='CS50', run='2019_Q1') + ) # True -- value derived from HarvardX/CS50 setting - cs50 = CourseKey(org='HarvardX', course='Bio101', run='2018_Q1') - MyStackedConfig.current(course=cs50) # False -- value derived from HarvardX org + bio101 = CourseKey(org='HarvardX', course='Bio101', run='2018_Q1') + MyStackedConfig.current(course_key=cs50) # False -- value derived from HarvardX org The following calls would result in errors due to overspecification: @@ -98,25 +139,29 @@ class StackedConfigurationModel(ConfigurationModel): Arguments: site: The Site to check current values for org: The org to check current values for - course: The course to check current values for + org_course: The course in a specific org to check current values for + course_key: The course to check current values for Returns: An instance of :class:`cls.attribute_tuple()` with the overridden values specified down to the level of the supplied argument (or global values if no arguments are supplied). """ - cache_key_name = cls.cache_key_name(site, org, course_key=course_key) + cache_key_name = cls.cache_key_name(site, org, org_course, course_key) cached = cache.get(cache_key_name) if cached is not None: return cached # Raise an error if more than one of site/org/course are specified simultaneously. - if len([arg for arg in [site, org, course_key] if arg is not None]) > 1: - raise ValueError("Only one of site, org, and course can be specified") + if len([arg for arg in [site, org, org_course, course_key] if arg is not None]) > 1: + raise ValueError("Only one of site, org, org_course, and course can be specified") - if org is None and course_key is not None: - org = cls._org_from_course_key(course_key) + if org_course is None and course_key is not None: + org_course = cls._org_course_from_course_key(course_key) + + if org is None and org_course is not None: + org = cls._org_from_org_course(org_course) if site is None and org is not None: site = cls._site_from_org(org) @@ -129,34 +174,48 @@ class StackedConfigurationModel(ConfigurationModel): values = field_defaults.copy() - global_override_q = Q(site=None, org=None, course_id=None) - site_override_q = Q(site=site, org=None, course_id=None) - org_override_q = Q(site=None, org=org, course_id=None) - course_override_q = Q(site=None, org=None, course_id=course_key) + global_override_q = Q(site=None, org=None, org_course=None, course_id=None) + site_override_q = Q(site=site, org=None, org_course=None, course_id=None) + org_override_q = Q(site=None, org=org, org_course=None, course_id=None) + org_course_override_q = Q(site=None, org=None, org_course=org_course, course_id=None) + course_override_q = Q(site=None, org=None, org_course=None, course_id=course_key) overrides = cls.objects.current_set().filter( global_override_q | site_override_q | org_override_q | + org_course_override_q | course_override_q ) provenances = defaultdict(lambda: Provenance.default) # We are sorting in python to avoid doing a filesort in the database for # what will only be 4 rows at maximum - for override in sorted( - overrides, - # This particular sort order sorts None before not-None (because False < True) - # It sorts global first (because all entries are None), then site entries - # (because course_id and org are None), then org and course (by the same logic) - key=lambda o: (o.course_id is not None, o.org is not None, o.site_id is not None) - ): + + def sort_key(override): + """ + Sort overrides in increasing specificity. + + This particular sort order sorts None before not-None (because False < True) + It sorts global first (because all entries are None), then site entries + (because course_id and org are None), then org, org_course and course (by the same logic) + """ + return ( + override.course_id is not None, + override.org_course is not None, + override.org is not None, + override.site_id is not None + ) + + for override in sorted(overrides, key=sort_key): for field in stackable_fields: value = field.value_from_object(override) if value != field_defaults[field.name]: values[field.name] = value if override.course_id is not None: - provenances[field.name] = Provenance.course + provenances[field.name] = Provenance.run + elif override.org_course is not None: + provenances[field.name] = Provenance.org_course elif override.org is not None: provenances[field.name] = Provenance.org elif override.site_id is not None: @@ -197,7 +256,7 @@ class StackedConfigurationModel(ConfigurationModel): all_overrides = cls.objects.current_set() overrides = { - (override.site_id, override.org, override.course_id): override + (override.site_id, override.org, override.org_course, override.course_id): override for override in all_overrides } @@ -211,11 +270,15 @@ class StackedConfigurationModel(ConfigurationModel): """ Return provenance for given field """ + org_course = cls._org_course_from_course_key(course.id) + org = cls._org_from_org_course(org_course) + for (config_key, provenance) in [ - ((None, None, course.id), Provenance.course), - ((None, course.id.org, None), Provenance.org), - ((sites_by_org[course.id.org].id, None, None), Provenance.site), - ((None, None, None), Provenance.global_), + ((None, None, None, course.id), Provenance.run), + ((None, None, org_course, None), Provenance.org_course), + ((None, org, None, None), Provenance.org), + ((sites_by_org[course.id.org].id, None, None, None), Provenance.site), + ((None, None, None, None), Provenance.global_), ]: config = overrides.get(config_key) if config is None: @@ -235,22 +298,21 @@ class StackedConfigurationModel(ConfigurationModel): } @classmethod - def cache_key_name(cls, site, org, course=None, course_key=None): # pylint: disable=arguments-differ - if course is not None and course_key is not None: - raise ValueError("Only one of course and course_key can be specified at a time") - if course is not None: - course_key = course - + def cache_key_name(cls, site, org, org_course, course_key): # pylint: disable=arguments-differ if site is None: site_id = None else: site_id = site.id - return super(StackedConfigurationModel, cls).cache_key_name(site_id, org, course_key) + return super(StackedConfigurationModel, cls).cache_key_name(site_id, org, org_course, course_key) @classmethod - def _org_from_course_key(cls, course_key): - return course_key.org + def _org_from_org_course(cls, org_course): + return org_course.partition('+')[0] + + @classmethod + def _org_course_from_course_key(cls, course_key): + return u"{}+{}".format(course_key.org, course_key.course) @classmethod @request_cached() @@ -267,7 +329,7 @@ class StackedConfigurationModel(ConfigurationModel): def clean(self): # fail validation if more than one of site/org/course are specified simultaneously - if len([arg for arg in [self.site, self.org, self.course] if arg is not None]) > 1: + if len([arg for arg in [self.site, self.org, self.org_course, self.course] if arg is not None]) > 1: raise ValidationError( _('Configuration may not be specified at more than one level at once.') ) diff --git a/openedx/features/content_type_gating/migrations/0007_auto_20190311_1919.py b/openedx/features/content_type_gating/migrations/0007_auto_20190311_1919.py new file mode 100644 index 0000000000..1c2d2e18d9 --- /dev/null +++ b/openedx/features/content_type_gating/migrations/0007_auto_20190311_1919.py @@ -0,0 +1,40 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.20 on 2019-03-11 19:19 +from __future__ import unicode_literals + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('content_type_gating', '0006_auto_20190308_1447'), + ] + + operations = [ + migrations.AddField( + model_name='contenttypegatingconfig', + name='org_course', + field=models.CharField(blank=True, db_index=True, help_text="Configure values for all course runs associated with this course. This is should be formatted as 'org/course' (i.e. MITx/6.002x, HarvardX/CS50).", max_length=255, null=True, verbose_name='Course in Org'), + ), + migrations.AlterField( + model_name='contenttypegatingconfig', + name='course', + field=models.ForeignKey(blank=True, help_text='Configure values for this course run. This should be formatted as the CourseKey (i.e. course-v1://MITx+6.002x+2019_Q1)', null=True, on_delete=django.db.models.deletion.DO_NOTHING, to='course_overviews.CourseOverview', verbose_name='Course Run'), + ), + migrations.AlterField( + model_name='contenttypegatingconfig', + name='org', + field=models.CharField(blank=True, db_index=True, help_text='Configure values for all course runs associated with this Organization. This is the organization string (i.e. edX, MITx).', max_length=255, null=True), + ), + migrations.AlterField( + model_name='contenttypegatingconfig', + name='site', + field=models.ForeignKey(blank=True, help_text='Configure values for all course runs associated with this site.', null=True, on_delete=django.db.models.deletion.CASCADE, to='sites.Site'), + ), + migrations.AddIndex( + model_name='contenttypegatingconfig', + index=models.Index(fields=['site', 'org', 'org_course', 'course'], name='content_typ_site_id_650310_idx'), + ), + ] diff --git a/openedx/features/content_type_gating/tests/test_models.py b/openedx/features/content_type_gating/tests/test_models.py index ef3421b925..16c013a812 100644 --- a/openedx/features/content_type_gating/tests/test_models.py +++ b/openedx/features/content_type_gating/tests/test_models.py @@ -202,15 +202,15 @@ class TestContentTypeGatingConfig(CacheIsolationTestCase): all_configs[CourseLocator('7-True', 'test_course', 'run-None')], { 'enabled': (True, Provenance.org), - 'enabled_as_of': (datetime(2018, 1, 1, 5, tzinfo=pytz.UTC), Provenance.course), + 'enabled_as_of': (datetime(2018, 1, 1, 5, tzinfo=pytz.UTC), Provenance.run), 'studio_override_enabled': (None, Provenance.default), } ) self.assertEqual( all_configs[CourseLocator('7-True', 'test_course', 'run-False')], { - 'enabled': (False, Provenance.course), - 'enabled_as_of': (datetime(2018, 1, 1, 5, tzinfo=pytz.UTC), Provenance.course), + 'enabled': (False, Provenance.run), + 'enabled_as_of': (datetime(2018, 1, 1, 5, tzinfo=pytz.UTC), Provenance.run), 'studio_override_enabled': (None, Provenance.default), } ) @@ -218,7 +218,7 @@ class TestContentTypeGatingConfig(CacheIsolationTestCase): all_configs[CourseLocator('7-None', 'test_course', 'run-None')], { 'enabled': (True, Provenance.site), - 'enabled_as_of': (datetime(2018, 1, 1, 5, tzinfo=pytz.UTC), Provenance.course), + 'enabled_as_of': (datetime(2018, 1, 1, 5, tzinfo=pytz.UTC), Provenance.run), 'studio_override_enabled': (None, Provenance.default), } ) diff --git a/openedx/features/course_duration_limits/migrations/0007_auto_20190311_1919.py b/openedx/features/course_duration_limits/migrations/0007_auto_20190311_1919.py new file mode 100644 index 0000000000..22b045d18c --- /dev/null +++ b/openedx/features/course_duration_limits/migrations/0007_auto_20190311_1919.py @@ -0,0 +1,40 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.20 on 2019-03-11 19:19 +from __future__ import unicode_literals + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('course_duration_limits', '0006_auto_20190308_1447'), + ] + + operations = [ + migrations.AddField( + model_name='coursedurationlimitconfig', + name='org_course', + field=models.CharField(blank=True, db_index=True, help_text="Configure values for all course runs associated with this course. This is should be formatted as 'org/course' (i.e. MITx/6.002x, HarvardX/CS50).", max_length=255, null=True, verbose_name='Course in Org'), + ), + migrations.AlterField( + model_name='coursedurationlimitconfig', + name='course', + field=models.ForeignKey(blank=True, help_text='Configure values for this course run. This should be formatted as the CourseKey (i.e. course-v1://MITx+6.002x+2019_Q1)', null=True, on_delete=django.db.models.deletion.DO_NOTHING, to='course_overviews.CourseOverview', verbose_name='Course Run'), + ), + migrations.AlterField( + model_name='coursedurationlimitconfig', + name='org', + field=models.CharField(blank=True, db_index=True, help_text='Configure values for all course runs associated with this Organization. This is the organization string (i.e. edX, MITx).', max_length=255, null=True), + ), + migrations.AlterField( + model_name='coursedurationlimitconfig', + name='site', + field=models.ForeignKey(blank=True, help_text='Configure values for all course runs associated with this site.', null=True, on_delete=django.db.models.deletion.CASCADE, to='sites.Site'), + ), + migrations.AddIndex( + model_name='coursedurationlimitconfig', + index=models.Index(fields=['site', 'org', 'org_course', 'course'], name='course_dura_site_id_b5bbcd_idx'), + ), + ] diff --git a/openedx/features/course_duration_limits/tests/test_models.py b/openedx/features/course_duration_limits/tests/test_models.py index 90e8d30be0..0f7f1ef6fe 100644 --- a/openedx/features/course_duration_limits/tests/test_models.py +++ b/openedx/features/course_duration_limits/tests/test_models.py @@ -232,21 +232,21 @@ class TestCourseDurationLimitConfig(CacheIsolationTestCase): all_configs[CourseLocator('7-True', 'test_course', 'run-None')], { 'enabled': (True, Provenance.org), - 'enabled_as_of': (datetime(2018, 1, 1, 5, tzinfo=pytz.UTC), Provenance.course), + 'enabled_as_of': (datetime(2018, 1, 1, 5, tzinfo=pytz.UTC), Provenance.run), } ) self.assertEqual( all_configs[CourseLocator('7-True', 'test_course', 'run-False')], { - 'enabled': (False, Provenance.course), - 'enabled_as_of': (datetime(2018, 1, 1, 5, tzinfo=pytz.UTC), Provenance.course), + 'enabled': (False, Provenance.run), + 'enabled_as_of': (datetime(2018, 1, 1, 5, tzinfo=pytz.UTC), Provenance.run), } ) self.assertEqual( all_configs[CourseLocator('7-None', 'test_course', 'run-None')], { 'enabled': (True, Provenance.site), - 'enabled_as_of': (datetime(2018, 1, 1, 5, tzinfo=pytz.UTC), Provenance.course), + 'enabled_as_of': (datetime(2018, 1, 1, 5, tzinfo=pytz.UTC), Provenance.run), } )