Implement a new configuration system for discussions plugins

Additional details available in the attached decisions document.

Co-authored-by: Kshitij Sobti <kshitij@sobti.in>
Co-authored-by: stvn <stvn@mit.edu>

cms/envs/common.py

@@ -1376,6 +1376,7 @@ INSTALLED_APPS = [
 
     # Discussion
     'openedx.core.djangoapps.django_comment_common',
+    'openedx.core.djangoapps.discussions',
 
     # for course creator table
     'django.contrib.admin',

lms/envs/common.py

@@ -2558,6 +2558,7 @@ INSTALLED_APPS = [
 
     # Discussion forums
     'openedx.core.djangoapps.django_comment_common',
+    'openedx.core.djangoapps.discussions',
 
     # Notes
     'lms.djangoapps.edxnotes',

openedx/core/djangoapps/discussions/__init__.py

@@ -0,0 +1,4 @@
+"""
+Handle discussions integrations
+"""
+default_app_config = 'openedx.core.djangoapps.discussions.apps.DiscussionsConfig'

openedx/core/djangoapps/discussions/admin.py

@@ -0,0 +1,22 @@
+"""
+Customize the django admin experience
+"""
+from django.contrib import admin
+from simple_history.admin import SimpleHistoryAdmin
+
+from .models import DiscussionsConfiguration
+
+
+class DiscussionsConfigurationAdmin(SimpleHistoryAdmin):
+    search_fields = (
+        'context_key',
+        'enabled',
+        'provider_type',
+    )
+    list_filter = (
+        'enabled',
+        'provider_type',
+    )
+
+
+admin.site.register(DiscussionsConfiguration, DiscussionsConfigurationAdmin)

openedx/core/djangoapps/discussions/apps.py

@@ -0,0 +1,11 @@
+"""
+Configure the django app
+"""
+from django.apps import AppConfig
+
+
+class DiscussionsConfig(AppConfig):
+    """
+    Configure the discussions django app
+    """
+    name = 'openedx.core.djangoapps.discussions'

openedx/core/djangoapps/discussions/docs/decisions/0001-discussion-configuration-api.rst

@@ -0,0 +1,73 @@
+Discussion provider configuration
+=================================
+
+
+Status
+------
+
+Proposal
+
+
+Context
+-------
+
+As part of the BD-03 initiative (Blended Development, Project 3),
+we want to enable third-party discussion providers to replace the
+default forums experience in edx-platform.
+
+To accomplish this, we should establish a configuration system to
+enable/disable/configure these new discussion-provider plugins;
+the existing forums experience will be included as one of these plugins.
+
+This ADR proposes a new configuration system that allows operators to create
+pre-populated configurations for specific discussions providers,
+which can then be used and customized by course authors/admins.
+
+
+Decision
+--------
+
+We propose to implement this as a Django database-backed model with
+historical records, `DiscussionsConfiguration`.
+
+The underlying model can be represented as:
+
+.. code-block:: python
+    class DiscussionsConfiguration:
+        context_key = LearningContextKeyField(primary_key=True, ...)
+        enabled = models.BooleanField(default=True, ...)
+        lti_configuration = models.ForeignKey(LtiConfiguration, blank=True, ...)
+        plugin_configuration = JSONField(default={}, ...)
+        provider_type = models.CharField(blank=False, ...)
+        history = HistoricalRecords()
+
+- Discussions can be disabled on a course by setting `enabled=False`; if
+  no record exists, the plugin is considered to be disabled by default.
+
+- `lti_configuration` is a collection of LTI launch configuration data
+  (URL, key, secret, etc.), but this data structure should be extracted
+  into its own model; see notes below (`Dependent Work`).
+
+- `plugin_configuration` is a free-form dictionary of configuration
+  values, to be passed to plugin's renderer
+
+- `provider_type` is the (arbitrary, but stable) id for the discussion provider.
+
+- `history` is tracked using Django Simple History.
+
+
+Future Work
+-----------
+
+- `LtiConfiguration` should be extended for feature-parity with_out_
+  XBlock dependencies.
+
+- Discussion apps/plugins/providers, implemented as Python models, will
+  be available via some discovery mechanism (Python entry points?,
+  hard-coded list?)
+  They will be responsible for:
+  - the provider_type, referenced by `DiscussionsConfiguration`
+  - any relevant renderers, accessors, etc.
+
+- No site/organization-based provider restrictions will be implemented, at this time.
+  This could be added later via an allow/deny-list.

openedx/core/djangoapps/discussions/migrations/0001_initial.py

@@ -0,0 +1,60 @@
+from django.conf import settings
+from django.db import migrations, models
+import django.db.models.deletion
+import django.utils.timezone
+import jsonfield.encoder
+import jsonfield.fields
+import model_utils.fields
+import opaque_keys.edx.django.models
+import simple_history.models
+
+
+class Migration(migrations.Migration):
+
+    initial = True
+
+    dependencies = [
+        migrations.swappable_dependency(settings.AUTH_USER_MODEL),
+        ('lti_consumer', '0001_initial'),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name='HistoricalDiscussionsConfiguration',
+            fields=[
+                ('created', model_utils.fields.AutoCreatedField(default=django.utils.timezone.now, editable=False, verbose_name='created')),
+                ('modified', model_utils.fields.AutoLastModifiedField(default=django.utils.timezone.now, editable=False, verbose_name='modified')),
+                ('context_key', opaque_keys.edx.django.models.LearningContextKeyField(db_index=True, max_length=255, verbose_name='Learning Context Key')),
+                ('enabled', models.BooleanField(default=True, help_text='If disabled, the discussions in the associated learning context/course will be disabled.')),
+                ('plugin_configuration', jsonfield.fields.JSONField(blank=True, default={}, dump_kwargs={'cls': jsonfield.encoder.JSONEncoder, 'separators': (',', ':')}, help_text='The plugin configuration data for this context/provider.', load_kwargs={})),
+                ('provider_type', models.CharField(help_text="The discussion tool/provider's id", max_length=100, verbose_name='Discussion provider')),
+                ('history_id', models.AutoField(primary_key=True, serialize=False)),
+                ('history_date', models.DateTimeField()),
+                ('history_change_reason', models.CharField(max_length=100, null=True)),
+                ('history_type', models.CharField(choices=[('+', 'Created'), ('~', 'Changed'), ('-', 'Deleted')], max_length=1)),
+                ('history_user', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='+', to=settings.AUTH_USER_MODEL)),
+                ('lti_configuration', models.ForeignKey(blank=True, db_constraint=False, help_text='The LTI configuration data for this context/provider.', null=True, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to='lti_consumer.LtiConfiguration')),
+            ],
+            options={
+                'verbose_name': 'historical discussions configuration',
+                'ordering': ('-history_date', '-history_id'),
+                'get_latest_by': 'history_date',
+            },
+            bases=(simple_history.models.HistoricalChanges, models.Model),
+        ),
+        migrations.CreateModel(
+            name='DiscussionsConfiguration',
+            fields=[
+                ('created', model_utils.fields.AutoCreatedField(default=django.utils.timezone.now, editable=False, verbose_name='created')),
+                ('modified', model_utils.fields.AutoLastModifiedField(default=django.utils.timezone.now, editable=False, verbose_name='modified')),
+                ('context_key', opaque_keys.edx.django.models.LearningContextKeyField(db_index=True, max_length=255, primary_key=True, serialize=False, unique=True, verbose_name='Learning Context Key')),
+                ('enabled', models.BooleanField(default=True, help_text='If disabled, the discussions in the associated learning context/course will be disabled.')),
+                ('plugin_configuration', jsonfield.fields.JSONField(blank=True, default={}, dump_kwargs={'cls': jsonfield.encoder.JSONEncoder, 'separators': (',', ':')}, help_text='The plugin configuration data for this context/provider.', load_kwargs={})),
+                ('provider_type', models.CharField(help_text="The discussion tool/provider's id", max_length=100, verbose_name='Discussion provider')),
+                ('lti_configuration', models.ForeignKey(blank=True, help_text='The LTI configuration data for this context/provider.', null=True, on_delete=django.db.models.deletion.SET_NULL, to='lti_consumer.LtiConfiguration')),
+            ],
+            options={
+                'abstract': False,
+            },
+        ),
+    ]

openedx/core/djangoapps/discussions/migrations/__init__.py

@@ -0,0 +1,3 @@
+"""
+Provide django migrations
+"""

openedx/core/djangoapps/discussions/models.py

@@ -0,0 +1,69 @@
+"""
+Provide django models to back the discussions app
+"""
+from django.core.exceptions import ValidationError
+from django.db import models
+from django.utils.translation import ugettext_lazy as _
+from jsonfield import JSONField
+from model_utils.models import TimeStampedModel
+from opaque_keys.edx.django.models import LearningContextKeyField
+from simple_history.models import HistoricalRecords
+
+from lti_consumer.models import LtiConfiguration
+
+from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
+
+
+class DiscussionsConfiguration(TimeStampedModel):
+    """
+    Associates a learning context with discussion provider and configuration
+    """
+
+    context_key = LearningContextKeyField(
+        primary_key=True,
+        db_index=True,
+        unique=True,
+        max_length=255,
+        # Translators: A key specifying a course, library, program, website, or some other collection of content where learning happens.
+        verbose_name=_("Learning Context Key"),
+    )
+    enabled = models.BooleanField(
+        default=True,
+        help_text=_("If disabled, the discussions in the associated learning context/course will be disabled.")
+    )
+    lti_configuration = models.ForeignKey(
+        LtiConfiguration,
+        on_delete=models.SET_NULL,
+        blank=True,
+        null=True,
+        help_text=_("The LTI configuration data for this context/provider."),
+    )
+    plugin_configuration = JSONField(
+        blank=True,
+        default={},
+        help_text=_("The plugin configuration data for this context/provider."),
+    )
+    provider_type = models.CharField(
+        blank=False,
+        max_length=100,
+        verbose_name=_("Discussion provider"),
+        help_text=_("The discussion tool/provider's id"),
+    )
+    history = HistoricalRecords()
+
+    def clean(self):
+        """
+        Validate the model
+
+        Currently, this only support courses, this can be extended
+        whenever discussions are available in other contexts
+        """
+        if not CourseOverview.course_exists(self.context_key):
+            raise ValidationError('Context Key should be an existing learning context.')
+
+    def __str__(self):
+        return "{context_key}: provider={provider} enabled={enabled}".format(
+            context_key=self.context_key,
+            provider=self.provider_type,
+            enabled=self.enabled,
+        )

openedx/core/djangoapps/discussions/tests/__init__.py

@@ -0,0 +1,3 @@
+"""
+Perform basic validation on the app
+"""

openedx/core/djangoapps/discussions/tests/test_models.py

@@ -0,0 +1,84 @@
+"""
+Perform basic validation of the models
+"""
+from django.test import TestCase
+from opaque_keys.edx.keys import CourseKey
+
+from ..models import DiscussionsConfiguration
+
+
+class DiscussionsConfigurationModelTest(TestCase):
+    """
+    Perform basic validation on the configuration model
+    """
+
+    def setUp(self):
+        """
+        Configure shared test data (configuration, course_key, etc.)
+        """
+        self.course_key_with_defaults = CourseKey.from_string("course-v1:TestX+Course+Configured")
+        self.course_key_without_config = CourseKey.from_string("course-v1:TestX+Course+NoConfig")
+        self.course_key_with_values = CourseKey.from_string("course-v1:TestX+Course+Values")
+        self.configuration_with_defaults = DiscussionsConfiguration(
+            context_key=self.course_key_with_defaults,
+        )
+        self.configuration_with_defaults.save()
+        self.configuration_with_values = DiscussionsConfiguration(
+            context_key=self.course_key_with_values,
+            enabled=False,
+            provider_type='cs_comments_service',
+            plugin_configuration={
+                'url': 'http://localhost',
+            },
+        )
+        self.configuration_with_values.save()
+        pass
+
+    def test_get_nonexistent(self):
+        """
+        Assert we can not fetch a non-existent record
+        """
+        with self.assertRaises(DiscussionsConfiguration.DoesNotExist):
+            configuration = DiscussionsConfiguration.objects.get(
+                context_key=self.course_key_without_config,
+            )
+
+    def test_get_with_defaults(self):
+        """
+        Assert we can lookup a record with default values
+        """
+        configuration = DiscussionsConfiguration.objects.get(context_key=self.course_key_with_defaults)
+        assert configuration is not None
+        assert configuration.enabled  # by default
+        assert configuration.lti_configuration is None
+        assert len(configuration.plugin_configuration.keys()) == 0
+        assert not configuration.provider_type
+
+    def test_get_with_values(self):
+        """
+        Assert we can lookup a record with custom values
+        """
+        configuration = DiscussionsConfiguration.objects.get(context_key=self.course_key_with_values)
+        assert configuration is not None
+        assert not configuration.enabled
+        assert configuration.lti_configuration is None
+        assert configuration.plugin_configuration['url'] == self.configuration_with_values.plugin_configuration['url']
+        assert configuration.provider_type == self.configuration_with_values.provider_type
+
+    def test_update_defaults(self):
+        """
+        Assert we can update an existing record
+        """
+        configuration = DiscussionsConfiguration.objects.get(context_key=self.course_key_with_defaults)
+        configuration.enabled = False
+        configuration.plugin_configuration = {
+            'url': 'http://localhost',
+        }
+        configuration.provider_type = 'cs_comments_service'
+        configuration.save()
+        configuration = DiscussionsConfiguration.objects.get(context_key=self.course_key_with_defaults)
+        assert configuration is not None
+        assert not configuration.enabled
+        assert configuration.lti_configuration is None
+        assert configuration.plugin_configuration['url'] == 'http://localhost'
+        assert configuration.provider_type == 'cs_comments_service'

openedx/tests/settings.py

@@ -60,7 +60,9 @@ INSTALLED_APPS = (
     'django.contrib.sessions',
     'django.contrib.sites',
     'django_sites_extensions',
+    'lti_consumer',
     'openedx.core.djangoapps.django_comment_common',
+    'openedx.core.djangoapps.discussions',
     'openedx.core.djangoapps.video_config',
     'openedx.core.djangoapps.video_pipeline',
     'openedx.core.djangoapps.bookmarks.apps.BookmarksConfig',