diff --git a/openedx/core/djangoapps/discussions/admin.py b/openedx/core/djangoapps/discussions/admin.py index 39a59fc421..02414b2e36 100644 --- a/openedx/core/djangoapps/discussions/admin.py +++ b/openedx/core/djangoapps/discussions/admin.py @@ -2,9 +2,13 @@ Customize the django admin experience """ from django.contrib import admin +from django.contrib.admin import SimpleListFilter from simple_history.admin import SimpleHistoryAdmin +from openedx.core.djangoapps.config_model_utils.admin import StackedConfigModelAdmin + from .models import DiscussionsConfiguration +from .models import ProviderFilter class DiscussionsConfigurationAdmin(SimpleHistoryAdmin): @@ -19,4 +23,65 @@ class DiscussionsConfigurationAdmin(SimpleHistoryAdmin): ) +class AllowListFilter(SimpleListFilter): + """ + Customize the admin interface for the AllowList + """ + + title = 'Allow List' + parameter_name = 'allow' + + def lookups(self, request, model_admin): + queryset = model_admin.get_queryset(request) + values = tuple( + ( + ','.join(filters[self.parameter_name] or ['None']), + ', '.join(filters[self.parameter_name] or ['None']), + ) + for filters in queryset.values(self.parameter_name).distinct() + ) + return values + + def queryset(self, request, queryset): + value = self.value() + if value: + filter_kwargs = {} + if ',' in value: + for v in value.split(','): + filter_kwargs[self.parameter_name + '__contains'] = v + queryset = queryset.filter(**filter_kwargs) + else: + if value == 'None': + filter_kwargs[self.parameter_name + '__exact'] = '' + else: + filter_kwargs[self.parameter_name + '__contains'] = value + queryset = queryset.filter(**filter_kwargs) + return queryset + + +class DenyListFilter(AllowListFilter): + """ + Customize the admin interface for the DenyList + """ + title = 'Deny List' + parameter_name = 'deny' + + +class ProviderFilterAdmin(StackedConfigModelAdmin): + """ + Customize the admin interface for the ProviderFilter + """ + + search_fields = ( + 'allow', + 'deny', + ) + list_filter = ( + 'enabled', + AllowListFilter, + DenyListFilter, + ) + + admin.site.register(DiscussionsConfiguration, DiscussionsConfigurationAdmin) +admin.site.register(ProviderFilter, ProviderFilterAdmin) diff --git a/openedx/core/djangoapps/discussions/docs/decisions/0002-organization-wide-provider-filtering.rst b/openedx/core/djangoapps/discussions/docs/decisions/0002-organization-wide-provider-filtering.rst new file mode 100644 index 0000000000..ff1a283874 --- /dev/null +++ b/openedx/core/djangoapps/discussions/docs/decisions/0002-organization-wide-provider-filtering.rst @@ -0,0 +1,140 @@ +Allow organization-wide filtering of discussion providers +========================================================= + + +Status +------ + +Proposal + + +Context +------- + +As part of the BD-03 initiative (Blended Development, Project 3), +we want to allow Organizations to filter the list of available +third-party discussion providers. + +This could be due to a number of motivations, eg: +- desired experience +- support levels +- data sharing + +We would, however, want to allow for manual exceptions. + +This document proposes a new configuration system that allows +admins to maintain a deny-allow-list to allow filtering of discussions +providers on a per-organization basis. + + +Requirements +------------ + +Given a particular organization, we must be able to remove options +from the list of available discussions providers during configuration. + + +Consideration +------------- + +- Django settings based (remote config, etc.) + - pros: + - ease of implmentation + - cons: + - overhead in maintenance (even with remote config, it would require + an engineer to make a code/config change and pull request +- Database-backed + - pros: + - ease of use + - by (django) admins + - cons: + - possible schema lock-in + - eg: do we scope it to `organizations` only? + or do we generalize it enough to use elsewhere? + + +Decision +-------- + +We propose to implement this as a Django database-backed model, +`ProviderFilter`. + +The underlying model can be represented as: + +.. code-block:: python + class ProviderFilter(StackedConfigurationModel): + allow = models.CharField(blank=True, ...) + deny = models.CharField(blank=True, ...) + +- `allow` is a comma-delimited string of providers to allow + +- `deny` is a comma-delimited string of providers to deny + +By extending the `StackedConfigurationModel`, we gain the flexibility of +stacking global/site/org/course/run values to not only provide +organization-wide filtering today, but to arbitrary levels of +granularity in the future. As this base model already exists in +`edx-platform`, integration is low-effort. + + +Examples +-------- + +With this system, we can configure global/site-wide defaults, + +.. code-block:: python + ProviderFilter.objects.create() + +And then override to deny an external provider for an organization: + +.. code-block:: python + ProviderFilter.objects.create( + org='Piazza-Less', + deny='lti-providerx', + ) + +Or override to deny _all_ external providers for an organization: + +.. code-block:: python + ProviderFilter.objects.create( + org='InternalOrganization', + allow='cs_comments_service', + ) + +And grant an exemption to a specific course: + +.. code-block:: python + ProviderFilter.objects.create( + # a course in 'InternalOrganization' + course=course, + allow='cs_comments_service lti-providerx', + ) + + +Logic +----- + +.. code-block:: python + _filter = get_filter(course_key) + if _filter is None: + allow = set() + deny = set() + available = defaults + if len(allow) > 0: + available = available.union(allow) + if len(deny) > 0: + available = available.difference(deny) + +By default, all installed providers are available. + +If no database record exists for the organization, the allow and deny +lists are considered empty, ie, there are no restrictions. + +If there are any items in the allow list, all _other_ providers will be +removed from the available list. + +If there are any items in the deny list, _these_ providers will be +removed from the available list. + +The order of operations between the deny and allow should be +interchangeable. diff --git a/openedx/core/djangoapps/discussions/migrations/0002_add_provider_filter.py b/openedx/core/djangoapps/discussions/migrations/0002_add_provider_filter.py new file mode 100644 index 0000000000..de8d1479eb --- /dev/null +++ b/openedx/core/djangoapps/discussions/migrations/0002_add_provider_filter.py @@ -0,0 +1,46 @@ +# Generated by Django 2.2.17 on 2021-02-02 06:35 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion +import django_mysql.models +import openedx.core.djangoapps.config_model_utils.models + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('course_overviews', '0023_courseoverview_banner_image_url'), + ('sites', '0002_alter_domain_unique'), + ('discussions', '0001_initial'), + ] + + operations = [ + migrations.CreateModel( + name='ProviderFilter', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('change_date', models.DateTimeField(auto_now_add=True, verbose_name='Change date')), + ('enabled', models.NullBooleanField(default=None, verbose_name='Enabled')), + ('org', 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)), + ('org_course', 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, validators=[openedx.core.djangoapps.config_model_utils.models.validate_course_in_org], verbose_name='Course in Org')), + ('allow', django_mysql.models.ListCharField(models.CharField(choices=[('cs_comments_service', 'cs_comments_service'), ('lti', 'lti'), ('test', 'test')], max_length=20), blank=True, help_text='Comma-separated list of providers to allow, eg: cs_comments_service,lti,test', max_length=63, size=3, verbose_name='Allow List')), + ('deny', django_mysql.models.ListCharField(models.CharField(choices=[('cs_comments_service', 'cs_comments_service'), ('lti', 'lti'), ('test', 'test')], max_length=20), blank=True, help_text='Comma-separated list of providers to deny, eg: cs_comments_service,lti,test', max_length=63, size=3, verbose_name='Deny List')), + ('changed_by', models.ForeignKey(editable=False, null=True, on_delete=django.db.models.deletion.PROTECT, to=settings.AUTH_USER_MODEL, verbose_name='Changed by')), + ('course', 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')), + ('site', 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')), + ], + options={ + 'abstract': False, + }, + ), + migrations.AddIndex( + model_name='providerfilter', + index=models.Index(fields=['site', 'org', 'course'], name='discussions_site_id_48e4b2_idx'), + ), + migrations.AddIndex( + model_name='providerfilter', + index=models.Index(fields=['site', 'org', 'org_course', 'course'], name='discussions_site_id_0f23d5_idx'), + ), + ] diff --git a/openedx/core/djangoapps/discussions/models.py b/openedx/core/djangoapps/discussions/models.py index 1dbb438a90..ed1c6b7466 100644 --- a/openedx/core/djangoapps/discussions/models.py +++ b/openedx/core/djangoapps/discussions/models.py @@ -2,10 +2,13 @@ Provide django models to back the discussions app """ from __future__ import annotations +import logging +from typing import List from django.core.exceptions import ValidationError from django.db import models from django.utils.translation import ugettext_lazy as _ +from django_mysql.models import ListCharField from jsonfield import JSONField from model_utils.models import TimeStampedModel from opaque_keys.edx.django.models import LearningContextKeyField @@ -13,8 +16,109 @@ from simple_history.models import HistoricalRecords from lti_consumer.models import LtiConfiguration +from openedx.core.djangoapps.config_model_utils.models import StackedConfigurationModel from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +log = logging.getLogger(__name__) + + +def get_supported_providers(): + """ + Return the list of supported discussion providers + + TODO: Load this from entry points? + """ + providers = [ + 'cs_comments_service', + 'lti', + 'test', + ] + return providers + + +class ProviderFilter(StackedConfigurationModel): + """ + Associate allow/deny-lists of discussions providers with courses/orgs + """ + + allow = ListCharField( + base_field=models.CharField( + choices=[ + (provider, provider) + for provider in get_supported_providers() + ], + max_length=20, + ), + blank=True, + help_text=_("Comma-separated list of providers to allow, eg: {choices}").format( + choices=','.join(get_supported_providers()), + ), + # max_length = (size * (max_length + len(',')) + # max_length = ( 3 * ( 20 + 1)) + max_length=63, + # size = len(get_supported_providers()) + size=3, + verbose_name=_('Allow List'), + ) + deny = ListCharField( + base_field=models.CharField( + choices=[ + (provider, provider) + for provider in get_supported_providers() + ], + max_length=20, + ), + blank=True, + help_text=_("Comma-separated list of providers to deny, eg: {choices}").format( + choices=','.join(get_supported_providers()), + ), + # max_length = (size * (max_length + len(',')) + # max_length = ( 3 * ( 20 + 1)) + max_length=63, + # size = len(get_supported_providers()) + size=3, + verbose_name=_('Deny List'), + ) + + STACKABLE_FIELDS = ( + 'allow', + 'deny', + ) + + def __str__(self): + return 'ProviderFilter(org="{org}", course="{course}", allow={allow}, deny={deny})'.format( + allow=self.allow, + course=self.course or '', + deny=self.deny, + org=self.org or '', + ) + + @property + def available_providers(self) -> List[str]: + """ + Return a filtered list of available providers + """ + _providers = get_supported_providers() + if self.allow: + _providers = [ + provider + for provider in _providers + if provider in self.allow + ] + if self.deny: + _providers = [ + provider + for provider in _providers + if provider not in self.deny + ] + return _providers + + @classmethod + def get_available_providers(cls, course_key) -> List[str]: + _filter = cls.current(course_key=course_key) + providers = _filter.available_providers + return providers + class DiscussionsConfiguration(TimeStampedModel): """ @@ -90,3 +194,11 @@ class DiscussionsConfiguration(TimeStampedModel): except cls.DoesNotExist: configuration = cls(context_key=context_key, enabled=False) return configuration + + @property + def available_providers(self) -> List[str]: + return ProviderFilter.current(course_key=self.context_key).available_providers + + @classmethod + def get_available_providers(cls, context_key) -> List[str]: + return ProviderFilter.current(course_key=context_key).available_providers diff --git a/openedx/core/djangoapps/discussions/tests/test_models.py b/openedx/core/djangoapps/discussions/tests/test_models.py index f3aaefbfb1..b3f5b479fa 100644 --- a/openedx/core/djangoapps/discussions/tests/test_models.py +++ b/openedx/core/djangoapps/discussions/tests/test_models.py @@ -1,10 +1,118 @@ """ Perform basic validation of the models """ +from unittest.mock import patch + from django.test import TestCase from opaque_keys.edx.keys import CourseKey +from organizations.models import Organization from ..models import DiscussionsConfiguration +from ..models import ProviderFilter + +SUPPORTED_PROVIDERS = [ + 'cs_comments_service', + 'lti', + 'test', +] + + +class OrganizationFilterTest(TestCase): + """ + Perform basic validation on the filter model + """ + + def setUp(self): + """ + Configure shared test data + """ + super().setUp() + self.course_key = CourseKey.from_string("course-v1:Test+Course+Configured") + self.course_key_with_defaults = CourseKey.from_string("course-v1:TestX+Course+Configured") + self.organization = Organization(short_name=self.course_key.org) + self.organization.save() + self.provider_allowed = SUPPORTED_PROVIDERS[0] + self.provider_denied = SUPPORTED_PROVIDERS[1] + + @patch('openedx.core.djangoapps.discussions.models.get_supported_providers', return_value=SUPPORTED_PROVIDERS) + def test_get_nonexistent(self, _default_providers): + """ + Assert we retrieve defaults when no configuration set + """ + providers = ProviderFilter.get_available_providers(self.course_key_with_defaults) + assert len(providers) == len(SUPPORTED_PROVIDERS) + + @patch('openedx.core.djangoapps.discussions.models.get_supported_providers', return_value=SUPPORTED_PROVIDERS) + def test_get_allow(self, _default_providers): + """ + Assert we can set the allow list + """ + ProviderFilter.objects.create( + org=self.course_key.org, + allow=[self.provider_allowed], + ) + providers = ProviderFilter.get_available_providers(self.course_key) + assert self.provider_allowed in providers + assert len(providers) == 1 + + @patch('openedx.core.djangoapps.discussions.models.get_supported_providers', return_value=SUPPORTED_PROVIDERS) + def test_get_deny(self, _default_providers): + """ + Assert we can set the deny list + """ + ProviderFilter.objects.create( + org=self.course_key.org, + deny=[self.provider_denied], + ) + providers = ProviderFilter.get_available_providers(self.course_key) + assert self.provider_denied not in providers + + @patch('openedx.core.djangoapps.discussions.models.get_supported_providers', return_value=SUPPORTED_PROVIDERS) + def test_get_allow_and_deny(self, _default_providers): + """ + Assert we can add an item to both allow and deny lists + """ + ProviderFilter.objects.create( + org=self.course_key.org, + allow=[self.provider_allowed, self.provider_denied], + deny=[self.provider_denied], + ) + providers = ProviderFilter.get_available_providers(self.course_key) + assert len(providers) == 1 + assert self.provider_denied not in providers + assert self.provider_allowed in providers + + @patch('openedx.core.djangoapps.discussions.models.get_supported_providers', return_value=SUPPORTED_PROVIDERS) + def test_get_allow_or_deny(self, _default_providers): + """ + Assert we can exclusively add an items to both allow and deny lists + """ + ProviderFilter.objects.create( + org=self.course_key.org, + allow=[self.provider_allowed], + deny=[self.provider_denied], + ) + providers = ProviderFilter.get_available_providers(self.course_key) + assert len(providers) == 1 + assert self.provider_denied not in providers + assert self.provider_allowed in providers + + @patch('openedx.core.djangoapps.discussions.models.get_supported_providers', return_value=SUPPORTED_PROVIDERS) + def test_override(self, _default_providers): + """ + Assert we can override a configuration and get the latest data + """ + ProviderFilter.objects.create( + org=self.course_key.org, + allow=[self.provider_allowed, self.provider_denied], + ) + ProviderFilter.objects.create( + org=self.course_key.org, + allow=[self.provider_allowed], + ) + providers = ProviderFilter.get_available_providers(self.course_key) + assert self.provider_allowed in providers + assert len(providers) == 1 class DiscussionsConfigurationModelTest(TestCase):