Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge PR #25920 bd03/model/filter

Browse Source 
* Commits:
  feat: Allow filtering of discussion providers
This commit is contained in:
stvn
2021-02-02 07:29:46 -08:00
parent b626a43fa1 cff2cb8941
commit 12ba5d2b87
5 changed files with 471 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

65
openedx/core/djangoapps/discussions/admin.py
View File

@@ -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)

140
openedx/core/djangoapps/discussions/docs/decisions/0002-organization-wide-provider-filtering.rst Normal file
View File

@@ -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.

46
openedx/core/djangoapps/discussions/migrations/0002_add_provider_filter.py Normal file
View File

@@ -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'),
),
]

112
openedx/core/djangoapps/discussions/models.py
View File

@@ -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

108
openedx/core/djangoapps/discussions/tests/test_models.py
View File

@@ -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):