From 72851f671d073488aa6af79df0b7a0e3cb794de9 Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Tue, 9 Jun 2020 20:56:40 +0530 Subject: [PATCH 01/11] Creates a new mechanism for configuring Discussion plugins This is the first of many steps required to enabled pluggable Discussion apps. This first step adds a configuration system and API that can be used by plugins. This will allow creating a link between a course and a discussion provider that will be utilised in future PRs to specify which plugin to use for a particular course's dicussions. --- cms/envs/common.py | 1 + lms/envs/common.py | 1 + .../djangoapps/config_model_utils/models.py | 45 +++-- .../core/djangoapps/discussions/__init__.py | 1 + openedx/core/djangoapps/discussions/admin.py | 12 ++ .../djangoapps/discussions/api/__init__.py | 0 .../core/djangoapps/discussions/api/config.py | 119 ++++++++++++ .../core/djangoapps/discussions/api/data.py | 36 ++++ openedx/core/djangoapps/discussions/apps.py | 11 ++ .../0001-discussion-configuration-api.rst | 65 +++++++ .../discussions/migrations/0001_initial.py | 152 +++++++++++++++ .../discussions/migrations/__init__.py | 0 openedx/core/djangoapps/discussions/models.py | 113 +++++++++++ .../djangoapps/discussions/tests/__init__.py | 0 .../djangoapps/discussions/tests/test_api.py | 179 ++++++++++++++++++ openedx/tests/settings.py | 1 + 16 files changed, 720 insertions(+), 16 deletions(-) create mode 100644 openedx/core/djangoapps/discussions/__init__.py create mode 100644 openedx/core/djangoapps/discussions/admin.py create mode 100644 openedx/core/djangoapps/discussions/api/__init__.py create mode 100644 openedx/core/djangoapps/discussions/api/config.py create mode 100644 openedx/core/djangoapps/discussions/api/data.py create mode 100644 openedx/core/djangoapps/discussions/apps.py create mode 100644 openedx/core/djangoapps/discussions/docs/decisions/0001-discussion-configuration-api.rst create mode 100644 openedx/core/djangoapps/discussions/migrations/0001_initial.py create mode 100644 openedx/core/djangoapps/discussions/migrations/__init__.py create mode 100644 openedx/core/djangoapps/discussions/models.py create mode 100644 openedx/core/djangoapps/discussions/tests/__init__.py create mode 100644 openedx/core/djangoapps/discussions/tests/test_api.py diff --git a/cms/envs/common.py b/cms/envs/common.py index 0539804ce1ca..7f210e307f7a 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -1377,6 +1377,7 @@ # Discussion 'openedx.core.djangoapps.django_comment_common', + 'openedx.core.djangoapps.discussions', # for course creator table 'django.contrib.admin', diff --git a/lms/envs/common.py b/lms/envs/common.py index 186a98d759d1..c70855dbdf76 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -2559,6 +2559,7 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring # Discussion forums 'openedx.core.djangoapps.django_comment_common', + 'openedx.core.djangoapps.discussions', # Notes 'lms.djangoapps.edxnotes', diff --git a/openedx/core/djangoapps/config_model_utils/models.py b/openedx/core/djangoapps/config_model_utils/models.py index 466baddf032d..8090625b8211 100644 --- a/openedx/core/djangoapps/config_model_utils/models.py +++ b/openedx/core/djangoapps/config_model_utils/models.py @@ -9,6 +9,7 @@ from collections import defaultdict from enum import Enum +from typing import Union import crum from config_models.models import ConfigurationModel, cache @@ -45,6 +46,27 @@ def validate_course_in_org(value): ) +@request_cached() +def site_from_org(org: str) -> Union[Site, RequestSite]: + """ + Return the site associated with an org. + + Args: + org (str): The org for which the associated site is to be fetched. + + Returns: + Site object + """ + configuration = SiteConfiguration.get_configuration_for_org(org, select_related=['site']) + if configuration is None: + try: + return Site.objects.get(id=settings.SITE_ID) + except Site.DoesNotExist: + return RequestSite(crum.get_current_request()) + else: + return configuration.site + + class StackedConfigurationModel(ConfigurationModel): """ A ConfigurationModel that stacks Global, Site, Org, Course, and Course Run level @@ -173,7 +195,7 @@ def current(cls, site=None, org=None, org_course=None, course_key=None): # pyli org = cls._org_from_org_course(org_course) if site is None and org is not None: - site = cls._site_from_org(org) + site = site_from_org(org) stackable_fields = [cls._meta.get_field(field_name) for field_name in cls.STACKABLE_FIELDS] field_defaults = { @@ -234,7 +256,11 @@ def sort_key(override): provenances[field.name] = Provenance.global_ current = cls(**values) - current.provenances = {field.name: provenances[field.name] for field in stackable_fields} # pylint: disable=attribute-defined-outside-init + # pylint: disable=attribute-defined-outside-init + current.provenances = { + field.name: provenances[field.name] + for field in stackable_fields + } cache.set(cache_key_name, current, cls.cache_timeout) return current @@ -324,21 +350,8 @@ def _org_from_org_course(cls, org_course): def _org_course_from_course_key(cls, course_key): return u"{}+{}".format(course_key.org, course_key.course) - @classmethod - @request_cached() - def _site_from_org(cls, org): - - configuration = SiteConfiguration.get_configuration_for_org(org, select_related=['site']) - if configuration is None: - try: - return Site.objects.get(id=settings.SITE_ID) - except Site.DoesNotExist: - return RequestSite(crum.get_current_request()) - else: - return configuration.site - def clean(self): - # fail validation if more than one of site/org/course are specified simultaneously + """ Ensure that only one of site, org, org_course, and course are specified simultaneously. """ 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/core/djangoapps/discussions/__init__.py b/openedx/core/djangoapps/discussions/__init__.py new file mode 100644 index 000000000000..99e6eebffe23 --- /dev/null +++ b/openedx/core/djangoapps/discussions/__init__.py @@ -0,0 +1 @@ +default_app_config = 'openedx.core.djangoapps.discussions.apps.DiscussionsConfig' diff --git a/openedx/core/djangoapps/discussions/admin.py b/openedx/core/djangoapps/discussions/admin.py new file mode 100644 index 000000000000..19fcd179bef3 --- /dev/null +++ b/openedx/core/djangoapps/discussions/admin.py @@ -0,0 +1,12 @@ +from django.contrib import admin + +from .models import DiscussionProviderConfig, LearningContextDiscussionConfig + + +class DiscussionProviderConfigAdminModel(admin.ModelAdmin): + search_fields = ("name", "provider", "config") + list_filter = ("restrict_to_site", "restrict_to_org", "provider") + + +admin.site.register(DiscussionProviderConfig, DiscussionProviderConfigAdminModel) +admin.site.register(LearningContextDiscussionConfig) diff --git a/openedx/core/djangoapps/discussions/api/__init__.py b/openedx/core/djangoapps/discussions/api/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/openedx/core/djangoapps/discussions/api/config.py b/openedx/core/djangoapps/discussions/api/config.py new file mode 100644 index 000000000000..583ec4f0ee70 --- /dev/null +++ b/openedx/core/djangoapps/discussions/api/config.py @@ -0,0 +1,119 @@ +from typing import Iterable, List, Optional + +from django.contrib.sites.models import Site +from django.db.models import Q +from opaque_keys.edx.keys import CourseKey + +from .data import CourseDiscussionConfigData, DiscussionPluginConfigData +from ..models import DiscussionProviderConfig, LearningContextDiscussionConfig +from ...config_model_utils.models import site_from_org + + +def _org_and_site_from_course_key(context_key: CourseKey): + org_key = getattr(context_key, 'org', None) + site = site_from_org(org_key) + return org_key, site + + +def _get_discussion_plugin_config_objects(org_key: str, site: Site) -> Iterable[DiscussionProviderConfig]: + return DiscussionProviderConfig.objects.filter( + Q(restrict_to_org__isnull=True, restrict_to_site__isnull=True) | + Q(restrict_to_org__short_name=org_key) | + Q(restrict_to_site=site) + ) + + +def get_course_discussion_config_options(course_key: CourseKey) -> List[DiscussionPluginConfigData]: + """ + Returns the available discussion configuration options for provided course. + + Args: + course_key (CourseKey): Learning context, currently only a course + + Returns: + A list of :class:`DiscussionConfigData` objects. + + """ + org_key, site = _org_and_site_from_course_key(course_key) + return [ + DiscussionPluginConfigData( + name=discussion_config.name, + provider=discussion_config.provider, + config=discussion_config.config, + ) + for discussion_config in _get_discussion_plugin_config_objects(org_key, site) + ] + + +def get_course_discussion_config(course_key: CourseKey) -> Optional[CourseDiscussionConfigData]: + """ + Returns the active discussion configuration for the course. + + Args: + course_key (CourseKey): Learning context, currently only a course + + Returns: + A :class:`CourseDiscussionConfigData` object with the active configuration for this course. + Returns `None` if a discussion tool isn't configured for the course yet. + + """ + try: + course_config = LearningContextDiscussionConfig.objects.get(pk=course_key) + except LearningContextDiscussionConfig.DoesNotExist: + return None + + provider_config = course_config.provider_config + if not provider_config: + return CourseDiscussionConfigData( + course_key=course_key, + config_name=None, + provider=None, + config=None, + enabled=False, + ) + merged_config = provider_config.config + merged_config.update(course_config.config_overrides) + return CourseDiscussionConfigData( + course_key=course_key, + config_name=provider_config.name, + provider=provider_config.provider, + config=merged_config, + enabled=course_config.enabled, + ) + + +def update_course_discussion_config( + course_key: CourseKey, + updated_config: dict +) -> Optional[CourseDiscussionConfigData]: + """ + Updates the configuration for the specified course. + + Args: + course_key (CourseKey): Learning context, currently only a course + updated_config (dict): Update configuration to save for specified course + + Returns: + A :class:`CourseDiscussionConfigData` object with the active configuration for this course. + Returns `None` if a discussion tool isn't configured for the course yet. + + """ + try: + course_config = LearningContextDiscussionConfig.objects.get(pk=course_key) + except LearningContextDiscussionConfig.DoesNotExist: + raise CourseDiscussionConfigData.DoesNotExist + + provider_config = course_config.provider_config + + if not provider_config: + raise CourseDiscussionConfigData.DoesNotExist + + course_config.config_overrides = updated_config + course_config.save() + return CourseDiscussionConfigData( + course_key=course_key, + config_name=provider_config.name, + provider=provider_config.provider, + config=updated_config, + enabled=course_config.enabled, + ) diff --git a/openedx/core/djangoapps/discussions/api/data.py b/openedx/core/djangoapps/discussions/api/data.py new file mode 100644 index 000000000000..61e61173d506 --- /dev/null +++ b/openedx/core/djangoapps/discussions/api/data.py @@ -0,0 +1,36 @@ +import attr +from opaque_keys.edx.keys import CourseKey + + +class ObjectDoesNotExist(Exception): + """ + Imitating Django model conventions, we put a subclass of this in some of our + data classes to indicate when something is not found. + """ + + +@attr.s(frozen=True) +class DiscussionPluginConfigData: + """ + Discussion Plugin Configuration Data Object + """ + + name = attr.ib(type=str) + provider = attr.ib(type=str) + config = attr.ib(type=dict) + + +@attr.s(frozen=True) +class CourseDiscussionConfigData: + """ + Course Discussion Configuration Data Object + """ + + course_key = attr.ib(type=CourseKey) + config_name = attr.ib(type=str) + provider = attr.ib(type=str) + config = attr.ib(type=dict) + enabled = attr.ib(type=bool) + + class DoesNotExist(ObjectDoesNotExist): + pass diff --git a/openedx/core/djangoapps/discussions/apps.py b/openedx/core/djangoapps/discussions/apps.py new file mode 100644 index 000000000000..18f2a0f46c8b --- /dev/null +++ b/openedx/core/djangoapps/discussions/apps.py @@ -0,0 +1,11 @@ +""" +Django AppConfig for Discussions +""" +# -*- coding: utf-8 -*- + + +from django.apps import AppConfig + + +class DiscussionsConfig(AppConfig): + name = "openedx.core.djangoapps.discussions" diff --git a/openedx/core/djangoapps/discussions/docs/decisions/0001-discussion-configuration-api.rst b/openedx/core/djangoapps/discussions/docs/decisions/0001-discussion-configuration-api.rst new file mode 100644 index 000000000000..ac5822a238f9 --- /dev/null +++ b/openedx/core/djangoapps/discussions/docs/decisions/0001-discussion-configuration-api.rst @@ -0,0 +1,65 @@ +Discussion provider configuration +================================= + +Status +------ + +Proposal + + +Context +------- + +During the development of discussions plugins, a need arose for a system of +configuration that would allow admins to configure connections to different +discussion tools, and have course authors select an established configuration +without requiring admins to share any secrets with course authors. + +For example, a discussion tool provider might require admins to specify OAuth2 +credentials that will be needed to integrate their tool with the platform. This +needs to be configured by an admin, and used by a course, but ideally course +authors should not have access to this information. + +It would also be useful to have multiple sets of configuration for different +discussion tools. A course creator can then select one of the available +configurations based on their requirements. + +For instance an organisation might be using a particular Discourse instance +across a number of their courses. So an admin will create a new configuration +object that specifies all the settings the Discourse plugin needs to operate, +such as the site location, the auth credentials etc. When setting up a new +course, a course author will then be able to see this as one of the possible +options when configuring Discourse as the discussion tool. + +Additionally, we might want to prevent certain configurations to be available +to all courses on a site. So it would be useful to limit configuration options +to a specific site, or Organization. + +This ADR proposes a new configuration system that allows admins to create +pre-populated configurations for specific discussions providers, which can then +be used and optionally overridden by course authors/admins. + +Decision +-------- + +We can create two models, `DiscussionProviderConfig` and +`LearningContextDiscussionConfig` that together provide the functionality +described above. + +The `DiscussionProviderConfig` model stores a configuration name (this will be +visible to course authors configuring a discussion tool), a provider (this will +be the id of the plugin for which the configuration is defined), and a config +JSON field (this will store the actual config). A configuration can be also be +restricted to only be visible/available for a specific site, or org by setting +`restrict_to_site` or `restrict_to_org`. Only one of these can be set at a time +and if set only courses on that site, or that org can see the configuration as +an option. Changes to this model are tracked using Django Simple History. + +To actually use a configuration for a particular course, we use the +`LearningContextDiscussionConfig` model. This will link a course to a particular +config. There is the ability to override the configuration for a particular +course if needed using the `config_overrides` JSON field. + +To disable discussions for a course, it is possible to set the `enabled` field +on this model to false. If such a object isn't linked to a provider config then +it is considered to be disabled. diff --git a/openedx/core/djangoapps/discussions/migrations/0001_initial.py b/openedx/core/djangoapps/discussions/migrations/0001_initial.py new file mode 100644 index 000000000000..e50a3e42fcdd --- /dev/null +++ b/openedx/core/djangoapps/discussions/migrations/0001_initial.py @@ -0,0 +1,152 @@ +# Generated by Django 2.2.16 on 2020-10-16 16:36 + +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 = [ + ('sites', '0002_alter_domain_unique'), + ('organizations', '0001_squashed_0007_historicalorganization'), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.CreateModel( + name='DiscussionProviderConfig', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('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')), + ('name', + models.CharField(help_text='A user-friendly name for this configuration. e.g. SomeOrg Discourse', + max_length=100)), + ('provider', models.CharField(db_index=True, help_text='The discussion tool/provider.', max_length=100, + verbose_name='Discussion provider')), + ('config', jsonfield.fields.JSONField(default={}, + blank=True, + dump_kwargs={'cls': jsonfield.encoder.JSONEncoder, 'separators': (',', ':')}, + help_text='The configuration data for this provider.', load_kwargs={})), + ('restrict_to_org', models.ForeignKey(blank=True, + help_text='Optionally restrict this config for use only on this Organization.', null=True, + on_delete=django.db.models.deletion.CASCADE, to='organizations.Organization')), + ('restrict_to_site', + models.ForeignKey(blank=True, help_text='Optionally restrict this config for use only on this site.', + null=True, on_delete=django.db.models.deletion.CASCADE, to='sites.Site')), + ], + options={ + 'abstract': False, + }, + ), + migrations.CreateModel( + name='LearningContextDiscussionConfig', + 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')), + ('enabled', models.BooleanField(default=True, + help_text='If disabled, the discussions in the associated learning context/course will be disabled.')), + ('config_overrides', jsonfield.fields.JSONField(default={}, + blank=True, + dump_kwargs={'cls': jsonfield.encoder.JSONEncoder, 'separators': (',', ':')}, + help_text='Overrides course-specific configuration.', load_kwargs={})), + ('provider_config', + models.ForeignKey(blank=True, help_text='The configuration to use for this learning context.', + null=True, on_delete=django.db.models.deletion.SET_NULL, + to='discussions.DiscussionProviderConfig')), + ], + options={ + 'abstract': False, + }, + ), + migrations.CreateModel( + name='HistoricalLearningContextDiscussionConfig', + 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')), + ('enabled', models.BooleanField(default=True, + help_text='If disabled, the discussions in the associated learning context/course will be disabled.')), + ('config_overrides', jsonfield.fields.JSONField(default={}, + blank=True, + dump_kwargs={'cls': jsonfield.encoder.JSONEncoder, 'separators': (',', ':')}, + help_text='Overrides course-specific configuration.', load_kwargs={})), + ('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)), + ('provider_config', models.ForeignKey(blank=True, db_constraint=False, + help_text='The configuration to use for this learning context.', null=True, + on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', + to='discussions.DiscussionProviderConfig')), + ], + options={ + 'verbose_name': 'historical learning context discussion config', + 'ordering': ('-history_date', '-history_id'), + 'get_latest_by': 'history_date', + }, + bases=(simple_history.models.HistoricalChanges, models.Model), + ), + migrations.CreateModel( + name='HistoricalDiscussionProviderConfig', + fields=[ + ('id', models.IntegerField(auto_created=True, blank=True, db_index=True, verbose_name='ID')), + ('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')), + ('name', + models.CharField(help_text='A user-friendly name for this configuration. e.g. SomeOrg Discourse', + max_length=100)), + ('provider', models.CharField(db_index=True, help_text='The discussion tool/provider.', max_length=100, + verbose_name='Discussion provider')), + ('config', jsonfield.fields.JSONField(default={}, + blank=True, + dump_kwargs={'cls': jsonfield.encoder.JSONEncoder, 'separators': (',', ':')}, + help_text='The configuration data for this provider.', load_kwargs={})), + ('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)), + ('restrict_to_org', models.ForeignKey(blank=True, db_constraint=False, + help_text='Optionally restrict this config for use only on this Organization.', null=True, + on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to='organizations.Organization')), + ('restrict_to_site', models.ForeignKey(blank=True, db_constraint=False, + help_text='Optionally restrict this config for use only on this site.', null=True, + on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to='sites.Site')), + ], + options={ + 'verbose_name': 'historical discussion provider config', + 'ordering': ('-history_date', '-history_id'), + 'get_latest_by': 'history_date', + }, + bases=(simple_history.models.HistoricalChanges, models.Model), + ), + ] diff --git a/openedx/core/djangoapps/discussions/migrations/__init__.py b/openedx/core/djangoapps/discussions/migrations/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/openedx/core/djangoapps/discussions/models.py b/openedx/core/djangoapps/discussions/models.py new file mode 100644 index 000000000000..1c05e8c886d2 --- /dev/null +++ b/openedx/core/djangoapps/discussions/models.py @@ -0,0 +1,113 @@ +from django.contrib.sites.models import Site +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 organizations.models import Organization +from simple_history.models import HistoricalRecords + +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview + + +class DiscussionProviderConfig(TimeStampedModel): + """ + Configuration model to store configuration for Discussions applications. + """ + + name = models.CharField( + blank=False, + max_length=100, + help_text=_("A user-friendly name for this configuration. e.g. SomeOrg Discourse") + ) + provider = models.CharField( + blank=False, + db_index=True, + max_length=100, + verbose_name=_("Discussion provider"), + help_text=_("The discussion tool/provider."), + ) + config = JSONField( + blank=True, + default={}, + help_text=_("The configuration data for this provider."), + ) + restrict_to_site = models.ForeignKey( + to=Site, + blank=True, + null=True, + on_delete=models.CASCADE, + help_text=_("Optionally restrict this config for use only on this site."), + db_index=True, + ) + restrict_to_org = models.ForeignKey( + to=Organization, + blank=True, + null=True, + on_delete=models.CASCADE, + help_text=_("Optionally restrict this config for use only on this Organization."), + db_index=True, + ) + + history = HistoricalRecords() + + def clean(self): + if self.restrict_to_org and self.restrict_to_site: + raise ValidationError("Can only set one form of restriction, site or org.") + + def __str__(self): + return "{name} provider={provider} restricted to [site={site} org={org}]".format( + provider=self.provider, + name=self.name, + site=self.restrict_to_site, + org=self.restrict_to_org + ) + + +class LearningContextDiscussionConfig(TimeStampedModel): + """ + Associates a learning context with a :class:`DiscussionProviderConfig`. + + Also allows overriding some configuration on a course-by-course basis. + """ + + context_key = LearningContextKeyField( + primary_key=True, + db_index=True, + unique=True, + max_length=255, + verbose_name=_("Learning Context"), + ) + enabled = models.BooleanField( + default=True, + help_text=_("If disabled, the discussions in the associated learning context/course will be disabled.") + ) + provider_config = models.ForeignKey( + to=DiscussionProviderConfig, + null=True, + blank=True, + help_text=_("The configuration to use for this learning context."), + on_delete=models.SET_NULL, + ) + config_overrides = JSONField( + blank=True, + default={}, + help_text=_("Overrides course-specific configuration."), + ) + + history = HistoricalRecords() + + def clean(self): + # 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}: enabled={enabled} config="{config}" has_overrides={has_overrides}'.format( + context_key=self.context_key, + enabled=self.enabled, + config=self.provider_config and self.provider_config.name, + has_overrides=bool(self.config_overrides), + ) diff --git a/openedx/core/djangoapps/discussions/tests/__init__.py b/openedx/core/djangoapps/discussions/tests/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/openedx/core/djangoapps/discussions/tests/test_api.py b/openedx/core/djangoapps/discussions/tests/test_api.py new file mode 100644 index 000000000000..fa12513403e7 --- /dev/null +++ b/openedx/core/djangoapps/discussions/tests/test_api.py @@ -0,0 +1,179 @@ +from unittest import mock +from uuid import uuid4 + +import ddt + +from django.test import TestCase +from opaque_keys.edx.keys import CourseKey +from organizations.models import Organization + +from ..api.config import ( + get_course_discussion_config, + get_course_discussion_config_options, update_course_discussion_config, +) +from ..api.data import CourseDiscussionConfigData, DiscussionPluginConfigData +from ..models import DiscussionProviderConfig, LearningContextDiscussionConfig +from ...config_model_utils.models import site_from_org +from ...site_configuration.tests.factories import SiteConfigurationFactory, SiteFactory + + +@ddt.ddt +class DiscussionAPITest(TestCase): + + def setUp(self): + site = SiteFactory() + SiteConfigurationFactory.create(site=site, site_values={"course_org_filter": ["TestX"]}) + self.course_key = CourseKey.from_string("course-v1:TestX+Course+Configured") + self.course_key_with_override = CourseKey.from_string("course-v1:TestX+Course+Override") + self.course_key_with_blank_config = CourseKey.from_string("course-v1:TestX+Course+BlankConfig") + self.course_key_without_config = CourseKey.from_string("course-v1:TestX+Course+NoConfig") + self.course_key_with_other_org = CourseKey.from_string("course-v1:TestX2+Course+OtherOrg") + self.provider = 'test-provider' + self.raw_config_data = { + "config": "base", + "key": "some-key", + "secret": "some-secret", + } + self.config_data_global = DiscussionPluginConfigData( + name="test-config-global", + provider=self.provider, + config=self.raw_config_data, + ) + self.config_data_site = DiscussionPluginConfigData( + name="test-config-site", + provider=self.provider, + config={}, + ) + self.config_data_org = DiscussionPluginConfigData( + name="test-config-org", + provider=self.provider, + config={}, + ) + self.course_config_data = CourseDiscussionConfigData( + course_key=self.course_key, + config_name="test-config-global", + provider=self.provider, + config=self.raw_config_data, + enabled=True, + ) + self.global_provider_config = DiscussionProviderConfig.objects.create( + name="test-config-global", + provider=self.provider, + config=self.raw_config_data, + ) + self.test_org, _ = Organization.objects.get_or_create(short_name="TestX") + self.test_org2, _ = Organization.objects.get_or_create(short_name="TestX2") + DiscussionProviderConfig.objects.create( + name="test-config-site", + provider=self.provider, + config={}, + restrict_to_site=site, + ) + DiscussionProviderConfig.objects.create( + name="test-config-org", + provider=self.provider, + config={}, + restrict_to_org=self.test_org, + ) + LearningContextDiscussionConfig.objects.create( + context_key=self.course_key, + enabled=True, + provider_config=self.global_provider_config, + ) + LearningContextDiscussionConfig.objects.create( + context_key=self.course_key_with_override, + enabled=True, + provider_config=self.global_provider_config, + config_overrides={ + "config": "overridden", + "more-config": True, + } + ) + LearningContextDiscussionConfig.objects.create( + context_key=self.course_key_with_blank_config, + enabled=True, + provider_config=None, + ) + super(DiscussionAPITest, self).setUp() + + def test_get_discussion_config_success(self): + config = get_course_discussion_config(self.course_key) + assert config == self.course_config_data + + def test_get_discussion_config_no_config(self): + config = get_course_discussion_config(self.course_key_without_config) + assert config is None + + def test_get_discussion_config_override_config(self): + config = get_course_discussion_config(self.course_key_with_override) + assert config.config == { + "config": "overridden", + "key": "some-key", + "secret": "some-secret", + "more-config": True, + } + + def test_get_discussion_config_blank_config(self): + config = get_course_discussion_config(self.course_key_with_blank_config) + assert config == CourseDiscussionConfigData( + course_key=self.course_key_with_blank_config, + config_name=None, + provider=None, + config=None, + enabled=False, + ) + + def test_get_discussion_config_options_all(self): + options = get_course_discussion_config_options(self.course_key) + assert len(options) == 3 + assert self.config_data_global in options + assert self.config_data_site in options + assert self.config_data_org in options + + def test_get_discussion_config_options_site(self): + site = SiteFactory.create() + options = get_course_discussion_config_options(self.course_key_with_other_org) + assert len(options) == 1 + assert self.config_data_global in options + with mock.patch("openedx.core.djangoapps.discussions.api.config.site_from_org", return_value=site): + DiscussionProviderConfig.objects.create( + name="test-config-site2", + provider=self.provider, + config={}, + restrict_to_site=site, + ) + options = get_course_discussion_config_options(self.course_key_with_other_org) + assert len(options) == 2 + + def test_get_discussion_config_options_org(self): + options = get_course_discussion_config_options(self.course_key_with_other_org) + assert len(options) == 1 + assert self.config_data_global in options + assert self.config_data_org not in options + DiscussionProviderConfig.objects.create( + name="test-config-org2", + provider=self.provider, + config={}, + restrict_to_org=self.test_org2, + ) + options = get_course_discussion_config_options(self.course_key_with_other_org) + assert len(options) == 2 + assert self.config_data_org not in options + + def test_update_config(self): + original_config = get_course_discussion_config(self.course_key) + assert 'new-key' not in original_config.config + assert original_config.config.get('secret') == "some-secret" + new_secret = str(uuid4()) + new_config = update_course_discussion_config(self.course_key, { + 'new-key': 'abc', + "secret": new_secret, + }) + assert 'new-key' in new_config.config + assert new_config.config.get('secret') == new_secret + new_config = get_course_discussion_config(self.course_key) + assert 'new-key' in new_config.config + assert new_config.config.get('secret') == new_secret + overrides = LearningContextDiscussionConfig.objects.get(context_key=self.course_key).config_overrides + assert 'new-key' in overrides + assert overrides.get('secret') == new_secret diff --git a/openedx/tests/settings.py b/openedx/tests/settings.py index f3531473fc25..1e1a4eefc0b7 100644 --- a/openedx/tests/settings.py +++ b/openedx/tests/settings.py @@ -66,6 +66,7 @@ 'django.contrib.sites', 'django_sites_extensions', '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', From ecdde068957c2b9c020745090dab5c85b59b7c64 Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Thu, 24 Sep 2020 02:58:15 +0530 Subject: [PATCH 02/11] Add ADR --- .../decisions/0001-discussion-plugin-api.rst | 71 +++++++++++++++++++ 1 file changed, 71 insertions(+) create mode 100644 openedx/core/djangoapps/discussions/docs/decisions/0001-discussion-plugin-api.rst diff --git a/openedx/core/djangoapps/discussions/docs/decisions/0001-discussion-plugin-api.rst b/openedx/core/djangoapps/discussions/docs/decisions/0001-discussion-plugin-api.rst new file mode 100644 index 000000000000..ac5849f1dd74 --- /dev/null +++ b/openedx/core/djangoapps/discussions/docs/decisions/0001-discussion-plugin-api.rst @@ -0,0 +1,71 @@ +Discussions Plugin API +====================== + +Status +------ + +Proposal + +Context +------- + +Currently the Open edX platform provides its own discussions experience, +using a form tool that ships with the platform and is well integrated +into it. There is a common need to support other, external discussion +tools into the platform. + +To make it possible for the platform to support integration with other +discussion tools, this ADR proposes a system for a new type of plugin +that allows extending the platform by adding other discussion tools +that can integrate more closely with the platform. + + +Decision +-------- + +We can enable a new type of plugin, a discussion app, that will provide +a standard interface that will hook into different parts of the platform +using standard APIs. New internal python APIs will also be created to +cater to the specific needs of discussion tools. + +The platform with load a new type of plugin exposed via the +`openedx.discussion_apps` entrypoint. These plugins can be configured +and linked to a learning context using the new discussion configuration +APIs added in https://github.com/edx/edx-platform/pull/24190. + +If a particular discussion plugin is linked to a course, then the +platform will load that plugin, and call the desired API in the plugin. +For instance, the plugin may provide a view to render for the course +tab for discussions, and another view to render for in-context +discussion instances. + +The plugin can also hook into platform events such as a new user +registering in a course, which the plugin can use to create a synced +user account for the discussion tool, and set up proper groups and +permissions for the user if needed. + +The plugin can expose signals for when new threads or comments are +posted, or content is followed or flagged. + +An optional base class is provided for such discussion apps. This class +can serve as a reference for plugin developers. A plugin class will need +to provide at least some bare minimum attributes and methods to work +as a discussion tool provider. This class will be interface through +which the platform will integrate and interact with the plugin. + +The initial implementation allows providing an internal name, and a +friendly name, a list of capabilities, a view to render in the course +tab, a view name for the tab content, and an `is_enabled` method that +allows enabling/disabling the plugin for a particular +request/context/user. + +Not all plugins can (or will) support all the features that the internal +forums do. A plugin can declare a set of capabilities, such as whether +it supports LTI, in-context discussions, is accessible, or +internationalised etc via an attribute called `capabilities`, this list +will be used to create a comparison view of different tools during +course setup. + +The intention is to support a way to override/extend this list so that +an Open edX instance can focus on the key attributes they find +interesting to expose. From fe03dac7e1e3d25496b419cafb429573d8fd366f Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Wed, 22 Jul 2020 17:58:31 +0530 Subject: [PATCH 03/11] Add support for loading discussion tabs from plugins --- common/lib/xmodule/xmodule/tabs.py | 4 +- lms/djangoapps/discussion/plugins.py | 36 ++++----- .../djangoapps/discussions/api/providers.py | 44 +++++++++++ .../discussions/discussions_apps.py | 68 +++++++++++++++++ ...api.rst => 0002-discussion-plugin-api.rst} | 4 +- .../migrations/0002_auto_20201029_2143.py | 23 ++++++ openedx/core/djangoapps/discussions/models.py | 10 +++ openedx/core/djangoapps/discussions/tabs.py | 76 +++++++++++++++++++ .../test_discussions_plugin/__init__.py | 41 ++++++++++ setup.py | 6 +- 10 files changed, 285 insertions(+), 27 deletions(-) create mode 100644 openedx/core/djangoapps/discussions/api/providers.py create mode 100644 openedx/core/djangoapps/discussions/discussions_apps.py rename openedx/core/djangoapps/discussions/docs/decisions/{0001-discussion-plugin-api.rst => 0002-discussion-plugin-api.rst} (96%) create mode 100644 openedx/core/djangoapps/discussions/migrations/0002_auto_20201029_2143.py create mode 100644 openedx/core/djangoapps/discussions/tabs.py create mode 100644 openedx/features/test_discussions_plugin/__init__.py diff --git a/common/lib/xmodule/xmodule/tabs.py b/common/lib/xmodule/xmodule/tabs.py index a1aa8065e4fb..afe64288f258 100644 --- a/common/lib/xmodule/xmodule/tabs.py +++ b/common/lib/xmodule/xmodule/tabs.py @@ -7,7 +7,7 @@ from abc import ABCMeta import six -from django.core.files.storage import get_storage_class +from django.utils.module_loading import import_string from six import text_type from xblock.fields import List @@ -283,7 +283,7 @@ def fragment_view(self): Returns the view that will be used to render the fragment. """ if not self._fragment_view: - self._fragment_view = get_storage_class(self.fragment_view_name)() + self._fragment_view = import_string(self.fragment_view_name)() return self._fragment_view def render_to_fragment(self, request, course, **kwargs): diff --git a/lms/djangoapps/discussion/plugins.py b/lms/djangoapps/discussion/plugins.py index dae5ade9d396..0de37f331aad 100644 --- a/lms/djangoapps/discussion/plugins.py +++ b/lms/djangoapps/discussion/plugins.py @@ -1,33 +1,25 @@ """ Views handling read (GET) requests for the Discussion tab and inline discussions. """ +from django.utils.translation import ugettext_lazy as _ +from openedx.core.djangoapps.discussions.discussions_apps import DiscussionApp +from .django_comment_client.utils import is_discussion_enabled -from django.conf import settings -from django.utils.translation import ugettext_noop -import lms.djangoapps.discussion.django_comment_client.utils as utils -from lms.djangoapps.courseware.tabs import EnrolledTab -from xmodule.tabs import TabFragmentViewMixin - - -class DiscussionTab(TabFragmentViewMixin, EnrolledTab): +class CommentServiceDiscussionApp(DiscussionApp): """ - A tab for the cs_comments_service forums. + Discussion Plugin app for cs_comments_service. """ + name = "cs_comments" + friendly_name = _("Inbuilt Discussion Forums") + + capabilities = [ - type = 'discussion' - title = ugettext_noop('Discussion') - priority = None - view_name = 'forum_form_discussion' - fragment_view_name = 'lms.djangoapps.discussion.views.DiscussionBoardFragmentView' - is_hideable = settings.FEATURES.get('ALLOW_HIDING_DISCUSSION_TAB', False) - is_default = False - body_class = 'discussion' - online_help_token = 'discussions' + ] + course_tab_view = "lms.djangoapps.discussion.views.DiscussionBoardFragmentView" + course_tab_view_name = "forum_form_discussion" @classmethod - def is_enabled(cls, course, user=None): - if not super(DiscussionTab, cls).is_enabled(course, user): - return False - return utils.is_discussion_enabled(course.id) + def is_enabled(cls, request=None, context_key=None, user=None): + return is_discussion_enabled(context_key) diff --git a/openedx/core/djangoapps/discussions/api/providers.py b/openedx/core/djangoapps/discussions/api/providers.py new file mode 100644 index 000000000000..855b2f6e026f --- /dev/null +++ b/openedx/core/djangoapps/discussions/api/providers.py @@ -0,0 +1,44 @@ +from typing import List, Optional, Type + +from opaque_keys.edx.keys import CourseKey, LearningContextKey + +from lms.djangoapps.discussion.plugins import CommentServiceDiscussionApp +from .config import get_course_discussion_config +from ..discussions_apps import DiscussionApp, DiscussionAppsPluginManager +from ..models import DiscussionProviderConfig + + +def get_discussion_provider(context_key: LearningContextKey) -> Optional[Type[DiscussionApp]]: + """ + Returns the discussion app provider associated with the provided context key. + + Args: + context_key (LearningContextKey): Learning context, currently only a course + + Returns: + A DiscussionApp instance for the discussion provider associated with this context. + """ + # Currently, only CourseKeys are supported. + assert isinstance(context_key, CourseKey) + config = get_course_discussion_config(context_key) + + # Fall back to the cs comments service Discussion provider + if config is None: + return CommentServiceDiscussionApp + + # If the configuration associated with this context is disabled, + # then we return nothing, and discussion integration will be disabled. + if config.enabled: + return DiscussionAppsPluginManager.get_plugin(config.provider) + + +def get_configured_discussion_providers() -> List[Type[DiscussionApp]]: + """ + Returns a list of discussion providers that have a usable configuration. + """ + configured_providers = DiscussionProviderConfig.objects.values_list('provider', flat=True).distinct() + return [ + tool + for tool in DiscussionAppsPluginManager.get_discussion_apps() + if tool.name in configured_providers + ] diff --git a/openedx/core/djangoapps/discussions/discussions_apps.py b/openedx/core/djangoapps/discussions/discussions_apps.py new file mode 100644 index 000000000000..ef24f9f7855d --- /dev/null +++ b/openedx/core/djangoapps/discussions/discussions_apps.py @@ -0,0 +1,68 @@ +from typing import List, Optional, Type + +from django.contrib.auth.models import User +from django.http import HttpRequest +from opaque_keys.edx.keys import LearningContextKey + +from edx_django_utils.plugins import PluginManager + +DISCUSSION_APPS_NAMESPACE = 'openedx.discussion_apps' + + +class DiscussionAppCapabilities: + """ Enum that lists capabilities supported by a discussion tool. """ + LTI1p1 = "lti1.1" + LTI1p3 = "lti1.3" + IN_CONTEXT_DISCUSSIONS = "in_context_discussions" + NOTIFICATIONS = "notifications" + COHORTS = "cohort_aware" + + +class DiscussionApp: + """ + Optional base class for discussion apps. + """ + + # Name of the discussion app for internal use + name = None + # Name of the discussion app that will show up in UI + friendly_name = None + # A list of capabilities of this discussion app that can't be + # automatically derived + capabilities = [] + # The view to render in the course tab page. + # If the discussion app would like to provide its own view to + # embed in the main Discussions tab, it can provide it here. + course_tab_view = None + # If the discussions apps has mounted its own urls for the tab + # this can provide the name of that view to reverse to. + course_tab_view_name = None + + @classmethod + def is_enabled( + cls, + request: Optional[HttpRequest] = None, + context_key: Optional[LearningContextKey] = None, + user: Optional[User] = None + ) -> bool: + """ + Given a context key, this returns if the plugin is enabled for the course. + """ + return True + + +class DiscussionAppsPluginManager(PluginManager): + """ + Manager for all of the discussion apps that have been made available. + + A discussion app implementation can subclass :class:`DiscussionApp` + or can implement the required class methods themselves. + """ + NAMESPACE = DISCUSSION_APPS_NAMESPACE + + @classmethod + def get_discussion_apps(cls) -> List[Type[DiscussionApp]]: + """ + Returns the list of available discussion apps. + """ + return list(cls.get_available_plugins(cls.NAMESPACE).values()) diff --git a/openedx/core/djangoapps/discussions/docs/decisions/0001-discussion-plugin-api.rst b/openedx/core/djangoapps/discussions/docs/decisions/0002-discussion-plugin-api.rst similarity index 96% rename from openedx/core/djangoapps/discussions/docs/decisions/0001-discussion-plugin-api.rst rename to openedx/core/djangoapps/discussions/docs/decisions/0002-discussion-plugin-api.rst index ac5849f1dd74..8a44624654fd 100644 --- a/openedx/core/djangoapps/discussions/docs/decisions/0001-discussion-plugin-api.rst +++ b/openedx/core/djangoapps/discussions/docs/decisions/0002-discussion-plugin-api.rst @@ -10,7 +10,7 @@ Context ------- Currently the Open edX platform provides its own discussions experience, -using a form tool that ships with the platform and is well integrated +using a forum tool that ships with the platform and is well integrated into it. There is a common need to support other, external discussion tools into the platform. @@ -68,4 +68,4 @@ course setup. The intention is to support a way to override/extend this list so that an Open edX instance can focus on the key attributes they find -interesting to expose. +interesting to expose. diff --git a/openedx/core/djangoapps/discussions/migrations/0002_auto_20201029_2143.py b/openedx/core/djangoapps/discussions/migrations/0002_auto_20201029_2143.py new file mode 100644 index 000000000000..c0b5b4f142e1 --- /dev/null +++ b/openedx/core/djangoapps/discussions/migrations/0002_auto_20201029_2143.py @@ -0,0 +1,23 @@ +# Generated by Django 2.2.16 on 2020-10-29 21:43 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('discussions', '0001_initial'), + ] + + operations = [ + migrations.AlterField( + model_name='discussionproviderconfig', + name='provider', + field=models.CharField(choices=[('cs_comments', 'Inbuilt Discussion Forums')], db_index=True, help_text='The discussion tool/provider.', max_length=100, verbose_name='Discussion provider'), + ), + migrations.AlterField( + model_name='historicaldiscussionproviderconfig', + name='provider', + field=models.CharField(choices=[('cs_comments', 'Inbuilt Discussion Forums')], db_index=True, help_text='The discussion tool/provider.', max_length=100, verbose_name='Discussion provider'), + ), + ] diff --git a/openedx/core/djangoapps/discussions/models.py b/openedx/core/djangoapps/discussions/models.py index 1c05e8c886d2..16dbe7641a6d 100644 --- a/openedx/core/djangoapps/discussions/models.py +++ b/openedx/core/djangoapps/discussions/models.py @@ -9,6 +9,14 @@ from simple_history.models import HistoricalRecords from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +from openedx.core.djangoapps.discussions.discussions_apps import DiscussionAppsPluginManager + + +def _get_provider_choices(): + return [ + (provider.name, provider.friendly_name) + for provider in DiscussionAppsPluginManager.get_discussion_apps() + ] class DiscussionProviderConfig(TimeStampedModel): @@ -23,10 +31,12 @@ class DiscussionProviderConfig(TimeStampedModel): ) provider = models.CharField( blank=False, + null=False, db_index=True, max_length=100, verbose_name=_("Discussion provider"), help_text=_("The discussion tool/provider."), + choices=_get_provider_choices(), ) config = JSONField( blank=True, diff --git a/openedx/core/djangoapps/discussions/tabs.py b/openedx/core/djangoapps/discussions/tabs.py new file mode 100644 index 000000000000..a43dfa6d8e3c --- /dev/null +++ b/openedx/core/djangoapps/discussions/tabs.py @@ -0,0 +1,76 @@ +from typing import Callable, Optional, Type + +from django.conf import settings +from django.contrib.auth.models import User +from django.http import HttpRequest +from django.utils.module_loading import import_string +from django.utils.translation import ugettext_noop +from opaque_keys.edx.keys import CourseKey +from web_fragments.fragment import Fragment + +from lms.djangoapps.courseware.tabs import EnrolledTab +from xmodule.course_module import CourseDescriptor +from xmodule.tabs import TabFragmentViewMixin +from .api.providers import get_discussion_provider +from .discussions_apps import DiscussionApp + + +class DiscussionTab(TabFragmentViewMixin, EnrolledTab): + """ + A tab for the discussion forums. + """ + + type = 'discussion' + title = ugettext_noop('Discussion') + priority = None + is_hideable = settings.FEATURES.get('ALLOW_HIDING_DISCUSSION_TAB', False) + is_default = False + body_class = 'discussion' + online_help_token = 'discussions' + + def __init__(self, tab_dict): + super().__init__(tab_dict) + self._discussion_provider = None + + def _get_discussion_provider(self, course_id: CourseKey) -> Type[DiscussionApp]: + if not self._discussion_provider: + self._discussion_provider = get_discussion_provider(course_id) + return self._discussion_provider + + def _get_tab_view(self, course_key): + tab_view = self._get_discussion_provider(course_key).course_tab_view + if isinstance(tab_view, str): + tab_view = import_string(tab_view) + return tab_view() + + @property + def link_func(self) -> Callable[[CourseDescriptor, Callable], str]: + def inner_link_func(course, reverse_func): + provider = self._get_discussion_provider(course.id) + if provider.course_tab_view_name: + return reverse_func(provider.course_tab_view_name, args=[str(course.id)]) + else: + return reverse_func("course_tab_view", args=[str(course.id), self.type]) + + return inner_link_func + + def render_to_fragment(self, request: HttpRequest, course: CourseDescriptor, **kwargs) -> Fragment: + """ + Renders this tab to a web fragment. + """ + tab_view = self._get_tab_view(course.id) + return tab_view.render_to_fragment(request, course_id=str(course.id), **kwargs) + + @classmethod + def is_enabled(cls, course: CourseDescriptor, user: Optional[User] = None) -> bool: + if not super(DiscussionTab, cls).is_enabled(course, user): + return False + provider = get_discussion_provider(course.id) + return provider and provider.course_tab_view and provider.is_enabled(context_key=course.id) + + @property + def uses_bootstrap(self) -> bool: + """ + Returns true if this tab is rendered with Bootstrap. + """ + return True diff --git a/openedx/features/test_discussions_plugin/__init__.py b/openedx/features/test_discussions_plugin/__init__.py new file mode 100644 index 000000000000..637b799a4ac0 --- /dev/null +++ b/openedx/features/test_discussions_plugin/__init__.py @@ -0,0 +1,41 @@ +""" +Test plugin for Discussions. +""" +from opaque_keys.edx.keys import CourseKey +from web_fragments.fragment import Fragment + +from openedx.core.djangoapps.discussions.discussions_apps import DiscussionApp +from openedx.core.djangoapps.plugin_api.views import EdxFragmentView +from openedx.core.djangolib.markup import HTML + + +class TestFragmentView(EdxFragmentView): + """ + View for test plugin for discussions. + """ + def render_to_fragment( + self, + request, + course_id=None, + ): # pylint: disable=arguments-differ + course_key = CourseKey.from_string(course_id) + fragment = Fragment( + HTML( + """ +
+ This is a test plugin for discussions. +

Course Key: {course_key}

+
+ """ + ).format(course_key=course_key) + ) + return fragment + + +class TestDiscussionsApp(DiscussionApp): + """ + Test plugin for discussions. + """ + name = "test_app" + friendly_name = "Test app" + course_tab_view = TestFragmentView() diff --git a/setup.py b/setup.py index f21177e1b04d..51a2bed86338 100644 --- a/setup.py +++ b/setup.py @@ -22,7 +22,7 @@ "courseware = lms.djangoapps.courseware.tabs:CoursewareTab", "course_info = lms.djangoapps.courseware.tabs:CourseInfoTab", "dates = lms.djangoapps.courseware.tabs:DatesTab", - "discussion = lms.djangoapps.discussion.plugins:DiscussionTab", + "discussion = openedx.core.djangoapps.discussions.tabs:DiscussionTab", "edxnotes = lms.djangoapps.edxnotes.plugins:EdxNotesTab", "external_discussion = lms.djangoapps.courseware.tabs:ExternalDiscussionCourseTab", "external_link = lms.djangoapps.courseware.tabs:ExternalLinkCourseTab", @@ -44,6 +44,10 @@ "verified_upgrade = lms.djangoapps.courseware.course_tools:VerifiedUpgradeTool", "financial_assistance = lms.djangoapps.courseware.course_tools:FinancialAssistanceTool", ], + "openedx.discussion_apps": [ + "cs_comments = lms.djangoapps.discussion.plugins:CommentServiceDiscussionApp", + "test_app = openedx.features.test_discussions_plugin:TestDiscussionsApp", + ], "openedx.user_partition_scheme": [ "random = openedx.core.djangoapps.user_api.partition_schemes:RandomUserPartitionScheme", "cohort = openedx.core.djangoapps.course_groups.partition_scheme:CohortPartitionScheme", From a1f47e8594a4daeeeb6bbc37c55582e336b96e59 Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Fri, 30 Oct 2020 16:06:16 +0530 Subject: [PATCH 04/11] Update query counts --- .../ccx/tests/test_field_override_performance.py | 4 ++-- lms/djangoapps/courseware/tests/test_views.py | 12 ++++++------ .../tests/views/test_course_home.py | 2 +- .../tests/views/test_course_updates.py | 2 +- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index 14c0ecda177f..cce18042f1eb 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -244,7 +244,7 @@ class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase): __test__ = True # TODO: decrease query count as part of REVO-28 - QUERY_COUNT = 31 + QUERY_COUNT = 32 TEST_DATA = { # (providers, course_width, enable_ccx, view_as_ccx): ( # # of sql queries to default, @@ -273,7 +273,7 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase): __test__ = True # TODO: decrease query count as part of REVO-28 - QUERY_COUNT = 31 + QUERY_COUNT = 32 TEST_DATA = { ('no_overrides', 1, True, False): (QUERY_COUNT, 3), diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 7efe81977d5d..00ed87f6f931 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -269,8 +269,8 @@ class IndexQueryTestCase(ModuleStoreTestCase): NUM_PROBLEMS = 20 @ddt.data( - (ModuleStoreEnum.Type.mongo, 10, 171), - (ModuleStoreEnum.Type.split, 4, 167), + (ModuleStoreEnum.Type.mongo, 10, 172), + (ModuleStoreEnum.Type.split, 4, 168), ) @ddt.unpack def test_index_query_counts(self, store_type, expected_mongo_query_count, expected_mysql_query_count): @@ -1425,8 +1425,8 @@ def test_view_certificate_link_hidden(self): self.assertContains(resp, u"Download Your Certificate") @ddt.data( - (True, 53), - (False, 52), + (True, 54), + (False, 53), ) @ddt.unpack def test_progress_queries_paced_courses(self, self_paced, query_count): @@ -1439,8 +1439,8 @@ def test_progress_queries_paced_courses(self, self_paced, query_count): @patch.dict(settings.FEATURES, {'ASSUME_ZERO_GRADE_IF_ABSENT_FOR_ALL_TESTS': False}) @ddt.data( - (False, 61, 42), - (True, 52, 37) + (False, 62, 43), + (True, 53, 38) ) @ddt.unpack def test_progress_queries(self, enable_waffle, initial, subsequent): diff --git a/openedx/features/course_experience/tests/views/test_course_home.py b/openedx/features/course_experience/tests/views/test_course_home.py index 1a71adcd66b9..72287f65a088 100644 --- a/openedx/features/course_experience/tests/views/test_course_home.py +++ b/openedx/features/course_experience/tests/views/test_course_home.py @@ -219,7 +219,7 @@ def test_queries(self): # Fetch the view and verify the query counts # TODO: decrease query count as part of REVO-28 - with self.assertNumQueries(73, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): + with self.assertNumQueries(74, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): with check_mongo_calls(4): url = course_home_url(self.course) self.client.get(url) diff --git a/openedx/features/course_experience/tests/views/test_course_updates.py b/openedx/features/course_experience/tests/views/test_course_updates.py index 0adf7a1e89f7..69ad5b78d115 100644 --- a/openedx/features/course_experience/tests/views/test_course_updates.py +++ b/openedx/features/course_experience/tests/views/test_course_updates.py @@ -134,7 +134,7 @@ def test_queries(self): # Fetch the view and verify that the query counts haven't changed # TODO: decrease query count as part of REVO-28 - with self.assertNumQueries(49, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): + with self.assertNumQueries(50, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): with check_mongo_calls(4): url = course_updates_url(self.course) self.client.get(url) From 85b80044c940cff703243a15cebe0bf5e25059ea Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Wed, 4 Nov 2020 19:06:04 +0530 Subject: [PATCH 05/11] Fix Python 3.5 issues --- openedx/core/djangoapps/discussions/api/providers.py | 4 ++-- openedx/core/djangoapps/discussions/tabs.py | 4 ++-- openedx/core/djangoapps/discussions/tests/test_api.py | 1 - 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/openedx/core/djangoapps/discussions/api/providers.py b/openedx/core/djangoapps/discussions/api/providers.py index 855b2f6e026f..64fd3a8f84c5 100644 --- a/openedx/core/djangoapps/discussions/api/providers.py +++ b/openedx/core/djangoapps/discussions/api/providers.py @@ -8,7 +8,7 @@ from ..models import DiscussionProviderConfig -def get_discussion_provider(context_key: LearningContextKey) -> Optional[Type[DiscussionApp]]: +def get_discussion_provider(context_key: LearningContextKey) -> 'Optional[Type[DiscussionApp]]': """ Returns the discussion app provider associated with the provided context key. @@ -32,7 +32,7 @@ def get_discussion_provider(context_key: LearningContextKey) -> Optional[Type[Di return DiscussionAppsPluginManager.get_plugin(config.provider) -def get_configured_discussion_providers() -> List[Type[DiscussionApp]]: +def get_configured_discussion_providers() -> 'List[Type[DiscussionApp]]': """ Returns a list of discussion providers that have a usable configuration. """ diff --git a/openedx/core/djangoapps/discussions/tabs.py b/openedx/core/djangoapps/discussions/tabs.py index a43dfa6d8e3c..00240ea4f00d 100644 --- a/openedx/core/djangoapps/discussions/tabs.py +++ b/openedx/core/djangoapps/discussions/tabs.py @@ -8,9 +8,9 @@ from opaque_keys.edx.keys import CourseKey from web_fragments.fragment import Fragment +from common.lib.xmodule.xmodule.course_module import CourseDescriptor +from common.lib.xmodule.xmodule.tabs import TabFragmentViewMixin from lms.djangoapps.courseware.tabs import EnrolledTab -from xmodule.course_module import CourseDescriptor -from xmodule.tabs import TabFragmentViewMixin from .api.providers import get_discussion_provider from .discussions_apps import DiscussionApp diff --git a/openedx/core/djangoapps/discussions/tests/test_api.py b/openedx/core/djangoapps/discussions/tests/test_api.py index fa12513403e7..1748d27850d7 100644 --- a/openedx/core/djangoapps/discussions/tests/test_api.py +++ b/openedx/core/djangoapps/discussions/tests/test_api.py @@ -13,7 +13,6 @@ ) from ..api.data import CourseDiscussionConfigData, DiscussionPluginConfigData from ..models import DiscussionProviderConfig, LearningContextDiscussionConfig -from ...config_model_utils.models import site_from_org from ...site_configuration.tests.factories import SiteConfigurationFactory, SiteFactory From e4f9fde061f2c8240804ee837748e0c4ee1f5778 Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Tue, 10 Nov 2020 01:27:45 +0530 Subject: [PATCH 06/11] Review Feedback #1 --- lms/djangoapps/discussion/plugins.py | 2 +- .../discussions/migrations/0002_auto_20201029_2143.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/discussion/plugins.py b/lms/djangoapps/discussion/plugins.py index 0de37f331aad..e63f59adcd4b 100644 --- a/lms/djangoapps/discussion/plugins.py +++ b/lms/djangoapps/discussion/plugins.py @@ -12,7 +12,7 @@ class CommentServiceDiscussionApp(DiscussionApp): Discussion Plugin app for cs_comments_service. """ name = "cs_comments" - friendly_name = _("Inbuilt Discussion Forums") + friendly_name = _("edX Discussions") capabilities = [ diff --git a/openedx/core/djangoapps/discussions/migrations/0002_auto_20201029_2143.py b/openedx/core/djangoapps/discussions/migrations/0002_auto_20201029_2143.py index c0b5b4f142e1..096c93581d64 100644 --- a/openedx/core/djangoapps/discussions/migrations/0002_auto_20201029_2143.py +++ b/openedx/core/djangoapps/discussions/migrations/0002_auto_20201029_2143.py @@ -13,11 +13,11 @@ class Migration(migrations.Migration): migrations.AlterField( model_name='discussionproviderconfig', name='provider', - field=models.CharField(choices=[('cs_comments', 'Inbuilt Discussion Forums')], db_index=True, help_text='The discussion tool/provider.', max_length=100, verbose_name='Discussion provider'), + field=models.CharField(choices=[('cs_comments', 'edX Discussions')], db_index=True, help_text='The discussion tool/provider.', max_length=100, verbose_name='Discussion provider'), ), migrations.AlterField( model_name='historicaldiscussionproviderconfig', name='provider', - field=models.CharField(choices=[('cs_comments', 'Inbuilt Discussion Forums')], db_index=True, help_text='The discussion tool/provider.', max_length=100, verbose_name='Discussion provider'), + field=models.CharField(choices=[('cs_comments', 'edX Discussions')], db_index=True, help_text='The discussion tool/provider.', max_length=100, verbose_name='Discussion provider'), ), ] From e2e70aaa02bc356666cd7feedda26d0c22541f50 Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Tue, 10 Nov 2020 01:30:43 +0530 Subject: [PATCH 07/11] Review Feedback #2 Fix for setting provider to none --- openedx/core/djangoapps/discussions/api/providers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/discussions/api/providers.py b/openedx/core/djangoapps/discussions/api/providers.py index 64fd3a8f84c5..32b768a4188f 100644 --- a/openedx/core/djangoapps/discussions/api/providers.py +++ b/openedx/core/djangoapps/discussions/api/providers.py @@ -28,7 +28,7 @@ def get_discussion_provider(context_key: LearningContextKey) -> 'Optional[Type[D # If the configuration associated with this context is disabled, # then we return nothing, and discussion integration will be disabled. - if config.enabled: + if config.enabled and config.provider: return DiscussionAppsPluginManager.get_plugin(config.provider) From c5e85c7a313ee313a9fd1aa2d373546af9122ce0 Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Tue, 10 Nov 2020 01:44:21 +0530 Subject: [PATCH 08/11] If a provider doesn't exist and a user is still on the discussion tab page, it will now raise 404. --- openedx/core/djangoapps/discussions/tabs.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/discussions/tabs.py b/openedx/core/djangoapps/discussions/tabs.py index 00240ea4f00d..daa31d2f1952 100644 --- a/openedx/core/djangoapps/discussions/tabs.py +++ b/openedx/core/djangoapps/discussions/tabs.py @@ -2,7 +2,7 @@ from django.conf import settings from django.contrib.auth.models import User -from django.http import HttpRequest +from django.http import Http404, HttpRequest from django.utils.module_loading import import_string from django.utils.translation import ugettext_noop from opaque_keys.edx.keys import CourseKey @@ -38,7 +38,10 @@ def _get_discussion_provider(self, course_id: CourseKey) -> Type[DiscussionApp]: return self._discussion_provider def _get_tab_view(self, course_key): - tab_view = self._get_discussion_provider(course_key).course_tab_view + provider = self._get_discussion_provider(course_key) + if not provider: + raise Http404 + tab_view = provider.course_tab_view if isinstance(tab_view, str): tab_view = import_string(tab_view) return tab_view() From 551c3f809bf72a9cdd62bc7e6b6f19acfb04561b Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Tue, 10 Nov 2020 01:24:32 +0530 Subject: [PATCH 09/11] Add a Piazza plugin for discussions --- .../discussion_provider_piazza/__init__.py | 0 .../discussion_provider_piazza/app.py | 18 ++++ .../discussion_provider_piazza/views.py | 87 +++++++++++++++++++ setup.py | 1 + 4 files changed, 106 insertions(+) create mode 100644 openedx/features/discussion_provider_piazza/__init__.py create mode 100644 openedx/features/discussion_provider_piazza/app.py create mode 100644 openedx/features/discussion_provider_piazza/views.py diff --git a/openedx/features/discussion_provider_piazza/__init__.py b/openedx/features/discussion_provider_piazza/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/openedx/features/discussion_provider_piazza/app.py b/openedx/features/discussion_provider_piazza/app.py new file mode 100644 index 000000000000..78bfdf1d03fb --- /dev/null +++ b/openedx/features/discussion_provider_piazza/app.py @@ -0,0 +1,18 @@ +from django.utils.translation import ugettext as _ +from openedx.core.djangoapps.discussions.discussions_apps import ( + DiscussionApp, + DiscussionAppCapabilities, +) + + +class PiazzaDiscussionApp(DiscussionApp): + """ + Discussion Plugin app for Piazza. + """ + name = "piazza" + friendly_name = _("Piazza") + + capabilities = [ + DiscussionAppCapabilities.LTI1p1, + ] + course_tab_view = "openedx.features.discussion_provider_piazza.views.PiazzaCourseTabView" diff --git a/openedx/features/discussion_provider_piazza/views.py b/openedx/features/discussion_provider_piazza/views.py new file mode 100644 index 000000000000..c81af648a380 --- /dev/null +++ b/openedx/features/discussion_provider_piazza/views.py @@ -0,0 +1,87 @@ +from urllib.parse import quote + +from django.contrib.sites.shortcuts import get_current_site +from lti_consumer.lti_1p1.contrib.django import lti_embed +from opaque_keys.edx.keys import CourseKey +from web_fragments.fragment import Fragment + +from lms.djangoapps.courseware.access import get_user_role +from openedx.core.djangoapps.discussions.api.config import get_course_discussion_config +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +from openedx.core.djangoapps.plugin_api.views import EdxFragmentView +from openedx.core.djangolib.markup import HTML +from student.models import anonymous_id_for_user + +PIAZZA_LTI_LAUNCH_URL = "https://piazza.com/connect" +ROLE_MAP = { + 'student': u'Student', + 'staff': u'Administrator', + 'instructor': u'Instructor', +} +DEFAULT_ROLE = u'Student' + + +class PiazzaCourseTabView(EdxFragmentView): + + def render_to_fragment(self, request, course_id=None): + course_key = CourseKey.from_string(course_id) + context_id = quote(course_id) + site = get_current_site(request) + user_id = quote(anonymous_id_for_user(request.user, course_id)) + resource_link_id = quote('{}-{}'.format( + site.domain, + str(course_key.make_usage_key('course', course_key.run)), + )) + result_sourcedid = "{context}:{resource_link}:{user_id}".format( + context=quote(context_id), + resource_link=resource_link_id, + user_id=user_id + ) + role = ROLE_MAP.get( + get_user_role(request.user, course_key), + DEFAULT_ROLE, + ) + + course = CourseOverview.get_from_id(course_key) + context_title = " - ".join([ + course.display_name_with_default, + course.display_org_with_default + ]) + + discussion_config = get_course_discussion_config(course_key) + + lti_embed_html = lti_embed( + html_element_id='piazza-discussion-provider-lti-launcher', + lti_launch_url=PIAZZA_LTI_LAUNCH_URL, + oauth_key=discussion_config.config["consumer_key"], + oauth_secret=discussion_config.config["consumer_secret"], + resource_link_id=resource_link_id, + user_id=user_id, + roles=role, + context_id=context_id, + context_title=context_title, + context_label=context_id, + result_sourcedid=result_sourcedid, + ) + + fragment = Fragment( + HTML( + """ + + """ + ).format(lti_embed_html) + ) + fragment.add_css( + """ + #piazza-discussion-provider-lti-embed { + width: 100%; + min-height: 400px; + border: none; + } + """ + ) + return fragment diff --git a/setup.py b/setup.py index 51a2bed86338..c6e719b51ed4 100644 --- a/setup.py +++ b/setup.py @@ -46,6 +46,7 @@ ], "openedx.discussion_apps": [ "cs_comments = lms.djangoapps.discussion.plugins:CommentServiceDiscussionApp", + "piazza = openedx.features.discussion_provider_piazza.app:PiazzaDiscussionApp", "test_app = openedx.features.test_discussions_plugin:TestDiscussionsApp", ], "openedx.user_partition_scheme": [ From 1dae717218567d736ef0750844bc0f47ea3245e5 Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Tue, 10 Nov 2020 21:22:38 +0530 Subject: [PATCH 10/11] Fix quality issues Move common code to mixin --- .../discussion_provider_piazza/app.py | 3 + .../discussion_provider_piazza/views.py | 120 +++++++++++++----- 2 files changed, 91 insertions(+), 32 deletions(-) diff --git a/openedx/features/discussion_provider_piazza/app.py b/openedx/features/discussion_provider_piazza/app.py index 78bfdf1d03fb..98f34a1b3005 100644 --- a/openedx/features/discussion_provider_piazza/app.py +++ b/openedx/features/discussion_provider_piazza/app.py @@ -1,3 +1,6 @@ +""" +Piazza Discussion provider plugin. +""" from django.utils.translation import ugettext as _ from openedx.core.djangoapps.discussions.discussions_apps import ( DiscussionApp, diff --git a/openedx/features/discussion_provider_piazza/views.py b/openedx/features/discussion_provider_piazza/views.py index c81af648a380..c5c9120e6c8f 100644 --- a/openedx/features/discussion_provider_piazza/views.py +++ b/openedx/features/discussion_provider_piazza/views.py @@ -1,74 +1,117 @@ +""" +Piazza Discussion Provider views. +""" +from collections import namedtuple +from typing import Dict from urllib.parse import quote from django.contrib.sites.shortcuts import get_current_site +from django.http import HttpRequest from lti_consumer.lti_1p1.contrib.django import lti_embed from opaque_keys.edx.keys import CourseKey from web_fragments.fragment import Fragment from lms.djangoapps.courseware.access import get_user_role -from openedx.core.djangoapps.discussions.api.config import get_course_discussion_config from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +from openedx.core.djangoapps.discussions.api.config import get_course_discussion_config from openedx.core.djangoapps.plugin_api.views import EdxFragmentView from openedx.core.djangolib.markup import HTML from student.models import anonymous_id_for_user -PIAZZA_LTI_LAUNCH_URL = "https://piazza.com/connect" -ROLE_MAP = { - 'student': u'Student', - 'staff': u'Administrator', - 'instructor': u'Instructor', -} -DEFAULT_ROLE = u'Student' +LtiCredentials = namedtuple('LtiCredentials', ('oauth_key', 'oauth_secret')) -class PiazzaCourseTabView(EdxFragmentView): - def render_to_fragment(self, request, course_id=None): - course_key = CourseKey.from_string(course_id) - context_id = quote(course_id) +class LtiLaunchMixin: + LTI_LAUNCH_URL = None + ROLE_MAP = { + 'student': u'Student', + 'staff': u'Administrator', + 'instructor': u'Instructor', + } + DEFAULT_ROLE = u'Student' + + @staticmethod + def get_additional_lti_parameters(course_key: CourseKey, request: HttpRequest) -> Dict[str, str]: + return {} + + @staticmethod + def get_user_id(user, course_key): + return quote(anonymous_id_for_user(user, course_key)) + + def get_lti_roles(self, user, course_key) -> str: + return self.ROLE_MAP.get( + get_user_role(user, course_key), + self.DEFAULT_ROLE, + ) + + @staticmethod + def get_context_id(course_key) -> str: + return quote(str(course_key)) + + @staticmethod + def get_resource_link_id(course_key, request) -> str: site = get_current_site(request) - user_id = quote(anonymous_id_for_user(request.user, course_id)) - resource_link_id = quote('{}-{}'.format( + return quote('{}-{}'.format( site.domain, str(course_key.make_usage_key('course', course_key.run)), )) - result_sourcedid = "{context}:{resource_link}:{user_id}".format( - context=quote(context_id), + + @staticmethod + def get_result_sourcedid(context_id, resource_link_id, user_id) -> str: + return "{context}:{resource_link}:{user_id}".format( + context=context_id, resource_link=resource_link_id, - user_id=user_id - ) - role = ROLE_MAP.get( - get_user_role(request.user, course_key), - DEFAULT_ROLE, + user_id=user_id, ) + @staticmethod + def get_context_title(course_key): course = CourseOverview.get_from_id(course_key) - context_title = " - ".join([ + return "{} - {}".format( course.display_name_with_default, - course.display_org_with_default - ]) + course.display_org_with_default, + ) + + @staticmethod + def get_oauth_credentials(course_config) -> LtiCredentials: + raise NotImplementedError + def get_lti_embed_code(self, course_key, request): discussion_config = get_course_discussion_config(course_key) + lti_creds = self.get_oauth_credentials(discussion_config) + user_id = self.get_user_id(request.user, course_key) + context_id = self.get_context_id(course_key) + resource_link_id = self.get_resource_link_id(course_key, request) + roles = self.get_lti_roles(request.user, course_key) + context_title = self.get_context_title(course_key) + result_sourcedid = self.get_result_sourcedid(context_id, resource_link_id, user_id) + additional_params = self.get_additional_lti_parameters(course_key, request) - lti_embed_html = lti_embed( - html_element_id='piazza-discussion-provider-lti-launcher', - lti_launch_url=PIAZZA_LTI_LAUNCH_URL, - oauth_key=discussion_config.config["consumer_key"], - oauth_secret=discussion_config.config["consumer_secret"], + return lti_embed( + html_element_id='discussion-provider-lti-launcher', + lti_launch_url=self.LTI_LAUNCH_URL, + oauth_key=lti_creds.oauth_key, + oauth_secret=lti_creds.oauth_secret, resource_link_id=resource_link_id, user_id=user_id, - roles=role, + roles=roles, context_id=context_id, context_title=context_title, context_label=context_id, result_sourcedid=result_sourcedid, + **additional_params, ) + def render_to_fragment(self, request, course_id=None): # pylint: disable=arguments-differ + course_key = CourseKey.from_string(course_id) + lti_embed_html = self.get_lti_embed_code(course_key, request) + fragment = Fragment( HTML( """ @@ -77,7 +120,7 @@ def render_to_fragment(self, request, course_id=None): ) fragment.add_css( """ - #piazza-discussion-provider-lti-embed { + #discussion-provider-lti-embed { width: 100%; min-height: 400px; border: none; @@ -85,3 +128,16 @@ def render_to_fragment(self, request, course_id=None): """ ) return fragment + + +class PiazzaCourseTabView(LtiLaunchMixin, EdxFragmentView): + LTI_LAUNCH_URL = "https://piazza.com/connect" + + @staticmethod + def get_oauth_credentials(config): + return LtiCredentials( + config.config["consumer_key"], + config.config["consumer_secret"], + ) + + From dd29906fa0ae3a1e2ed6dc7a2e5e69ed08fea65a Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Tue, 10 Nov 2020 21:31:45 +0530 Subject: [PATCH 11/11] Fix quality issues --- .../discussion_provider_piazza/views.py | 53 ++++++++++++++----- 1 file changed, 40 insertions(+), 13 deletions(-) diff --git a/openedx/features/discussion_provider_piazza/views.py b/openedx/features/discussion_provider_piazza/views.py index c5c9120e6c8f..3705ad33e60a 100644 --- a/openedx/features/discussion_provider_piazza/views.py +++ b/openedx/features/discussion_provider_piazza/views.py @@ -5,6 +5,7 @@ from typing import Dict from urllib.parse import quote +from django.contrib.auth.models import User from django.contrib.sites.shortcuts import get_current_site from django.http import HttpRequest from lti_consumer.lti_1p1.contrib.django import lti_embed @@ -14,6 +15,7 @@ from lms.djangoapps.courseware.access import get_user_role from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.discussions.api.config import get_course_discussion_config +from openedx.core.djangoapps.discussions.api.data import CourseDiscussionConfigData from openedx.core.djangoapps.plugin_api.views import EdxFragmentView from openedx.core.djangolib.markup import HTML from student.models import anonymous_id_for_user @@ -22,7 +24,11 @@ LtiCredentials = namedtuple('LtiCredentials', ('oauth_key', 'oauth_secret')) -class LtiLaunchMixin: +class LtiCourseLaunchMixin: + """ + Mixin that encapsulates all LTI-related functionality from the View + """ + LTI_LAUNCH_URL = None ROLE_MAP = { 'student': u'Student', @@ -36,21 +42,21 @@ def get_additional_lti_parameters(course_key: CourseKey, request: HttpRequest) - return {} @staticmethod - def get_user_id(user, course_key): + def get_user_id(user: User, course_key: CourseKey): return quote(anonymous_id_for_user(user, course_key)) - def get_lti_roles(self, user, course_key) -> str: + def get_lti_roles(self, user: User, course_key: CourseKey) -> str: return self.ROLE_MAP.get( get_user_role(user, course_key), self.DEFAULT_ROLE, ) @staticmethod - def get_context_id(course_key) -> str: + def get_context_id(course_key: CourseKey) -> str: return quote(str(course_key)) @staticmethod - def get_resource_link_id(course_key, request) -> str: + def get_resource_link_id(course_key: CourseKey, request: HttpRequest) -> str: site = get_current_site(request) return quote('{}-{}'.format( site.domain, @@ -58,7 +64,7 @@ def get_resource_link_id(course_key, request) -> str: )) @staticmethod - def get_result_sourcedid(context_id, resource_link_id, user_id) -> str: + def get_result_sourcedid(context_id: str, resource_link_id: str, user_id: str) -> str: return "{context}:{resource_link}:{user_id}".format( context=context_id, resource_link=resource_link_id, @@ -66,7 +72,7 @@ def get_result_sourcedid(context_id, resource_link_id, user_id) -> str: ) @staticmethod - def get_context_title(course_key): + def get_context_title(course_key: CourseKey) -> str: course = CourseOverview.get_from_id(course_key) return "{} - {}".format( course.display_name_with_default, @@ -74,10 +80,20 @@ def get_context_title(course_key): ) @staticmethod - def get_oauth_credentials(course_config) -> LtiCredentials: + def get_oauth_credentials(config: CourseDiscussionConfigData) -> LtiCredentials: raise NotImplementedError - def get_lti_embed_code(self, course_key, request): + def get_lti_embed_code(self, course_key: CourseKey, request: HttpRequest) -> str: + """ + Returns the LTI embed code for embedding in the current course context. + Args: + course_key (CourseKey): Course key for course in which to embed LTI. + request (HttpRequest): Request object for view in which LTI will be embedded. + + Returns: + HTML code to embed LTI in course page. + + """ discussion_config = get_course_discussion_config(course_key) lti_creds = self.get_oauth_credentials(discussion_config) user_id = self.get_user_id(request.user, course_key) @@ -103,7 +119,17 @@ def get_lti_embed_code(self, course_key, request): **additional_params, ) - def render_to_fragment(self, request, course_id=None): # pylint: disable=arguments-differ + def render_to_fragment(self, request: HttpRequest, course_id: str) -> Fragment: + """ + Returns a fragment view for the LTI launch. + Args: + request (HttpRequest): request object + course_id (str): A string course id + + Returns: + A Fragment that embeds LTI in a course page. + + """ course_key = CourseKey.from_string(course_id) lti_embed_html = self.get_lti_embed_code(course_key, request) @@ -130,7 +156,10 @@ def render_to_fragment(self, request, course_id=None): # pylint: disable=argume return fragment -class PiazzaCourseTabView(LtiLaunchMixin, EdxFragmentView): +class PiazzaCourseTabView(LtiCourseLaunchMixin, EdxFragmentView): + """ + Course tab view for Piazza discusion provider. + """ LTI_LAUNCH_URL = "https://piazza.com/connect" @staticmethod @@ -139,5 +168,3 @@ def get_oauth_credentials(config): config.config["consumer_key"], config.config["consumer_secret"], ) - -