From 5bcfcc9893ed1d9eb5a8c165f869ab698fd0ce2c Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Wed, 6 Oct 2021 18:19:30 +0530 Subject: [PATCH 1/3] feat: Adds a new discussion topic configuration mechanism The new discussion configuration system links discussion topics directly to the course structure. This change adds a new task that sends a discussion update signal if there are any changes to the course. This signal includes all the context needed to update the configuration of the course. The handler for this new event will create a new entry for each unit that needs a topic in the database. In the future this will be used to see the topics in the course. --- .../contentstore/signals/handlers.py | 10 +- lms/djangoapps/course_wiki/tab.py | 4 +- lms/djangoapps/edxnotes/plugins.py | 2 +- lms/djangoapps/teams/plugins.py | 2 +- openedx/core/djangoapps/discussions/apps.py | 3 + openedx/core/djangoapps/discussions/data.py | 45 +++++ .../core/djangoapps/discussions/handlers.py | 93 ++++++++++ .../0007_add_discussion_topic_link.py | 57 ++++++ openedx/core/djangoapps/discussions/models.py | 90 +++++++++- .../core/djangoapps/discussions/signals.py | 18 ++ openedx/core/djangoapps/discussions/tasks.py | 92 ++++++++++ .../discussions/tests/test_handlers.py | 163 ++++++++++++++++++ .../discussions/tests/test_tasks.py | 152 ++++++++++++++++ 13 files changed, 722 insertions(+), 9 deletions(-) create mode 100644 openedx/core/djangoapps/discussions/data.py create mode 100644 openedx/core/djangoapps/discussions/handlers.py create mode 100644 openedx/core/djangoapps/discussions/migrations/0007_add_discussion_topic_link.py create mode 100644 openedx/core/djangoapps/discussions/signals.py create mode 100644 openedx/core/djangoapps/discussions/tasks.py create mode 100644 openedx/core/djangoapps/discussions/tests/test_handlers.py create mode 100644 openedx/core/djangoapps/discussions/tests/test_tasks.py diff --git a/cms/djangoapps/contentstore/signals/handlers.py b/cms/djangoapps/contentstore/signals/handlers.py index c70a801ca71f..9272be6ec489 100644 --- a/cms/djangoapps/contentstore/signals/handlers.py +++ b/cms/djangoapps/contentstore/signals/handlers.py @@ -3,24 +3,24 @@ import logging from datetime import datetime -from functools import wraps from django.core.cache import cache from django.dispatch import receiver +from functools import wraps from pytz import UTC from cms.djangoapps.contentstore.courseware_index import ( CourseAboutSearchIndexer, CoursewareSearchIndexer, - LibrarySearchIndexer + LibrarySearchIndexer, ) from common.djangoapps.track.event_transaction_utils import get_event_transaction_id, get_event_transaction_type from common.djangoapps.util.module_utils import yield_dynamic_descriptor_descendants from lms.djangoapps.grades.api import task_compute_all_grades_for_course from openedx.core.djangoapps.content.learning_sequences.api import key_supports_outlines +from openedx.core.djangoapps.discussions.tasks import update_discussions_settings_from_course_task from openedx.core.lib.gating import api as gating_api from xmodule.modulestore.django import SignalHandler, modulestore - from .signals import GRADING_POLICY_CHANGED log = logging.getLogger(__name__) @@ -64,11 +64,13 @@ def listen_for_course_publish(sender, course_key, **kwargs): # pylint: disable= # Push the course outline to learning_sequences asynchronously. update_outline_from_modulestore_task.delay(course_key_str) - # Finally call into the course search subsystem + # Finally, call into the course search subsystem # to kick off an indexing action if CoursewareSearchIndexer.indexing_is_enabled() and CourseAboutSearchIndexer.indexing_is_enabled(): update_search_index.delay(course_key_str, datetime.now(UTC).isoformat()) + update_discussions_settings_from_course_task.delay(course_key_str) + @receiver(SignalHandler.course_deleted) def listen_for_course_delete(sender, course_key, **kwargs): # pylint: disable=unused-argument diff --git a/lms/djangoapps/course_wiki/tab.py b/lms/djangoapps/course_wiki/tab.py index 8c87767fe868..1bb352020bff 100644 --- a/lms/djangoapps/course_wiki/tab.py +++ b/lms/djangoapps/course_wiki/tab.py @@ -5,7 +5,7 @@ from django.conf import settings -from django.utils.translation import gettext_noop +from django.utils.translation import gettext_noop as _ from lms.djangoapps.courseware.tabs import EnrolledTab @@ -16,7 +16,7 @@ class WikiTab(EnrolledTab): """ type = "wiki" - title = gettext_noop('Wiki') + title = _('Wiki') view_name = "course_wiki" is_hideable = True is_default = False diff --git a/lms/djangoapps/edxnotes/plugins.py b/lms/djangoapps/edxnotes/plugins.py index 908ff91d521f..2e79706b825c 100644 --- a/lms/djangoapps/edxnotes/plugins.py +++ b/lms/djangoapps/edxnotes/plugins.py @@ -5,7 +5,7 @@ from django.conf import settings from django.contrib.auth import get_user_model -from django.utils.translation import ugettext_noop as _ +from django.utils.translation import gettext_noop as _ from opaque_keys.edx.keys import CourseKey from xmodule.modulestore.django import modulestore diff --git a/lms/djangoapps/teams/plugins.py b/lms/djangoapps/teams/plugins.py index af699238e1dd..33542ba7cda9 100644 --- a/lms/djangoapps/teams/plugins.py +++ b/lms/djangoapps/teams/plugins.py @@ -5,7 +5,7 @@ from django.conf import settings from django.contrib.auth import get_user_model -from django.utils.translation import ugettext_noop as _ +from django.utils.translation import gettext_noop as _ from opaque_keys.edx.keys import CourseKey from lms.djangoapps.courseware.tabs import EnrolledTab diff --git a/openedx/core/djangoapps/discussions/apps.py b/openedx/core/djangoapps/discussions/apps.py index 2a820b162a66..ba0b66942586 100644 --- a/openedx/core/djangoapps/discussions/apps.py +++ b/openedx/core/djangoapps/discussions/apps.py @@ -30,3 +30,6 @@ class DiscussionsConfig(AppConfig): PluginSettings.CONFIG: { }, } + + def ready(self): + from . import handlers # pylint: disable=unused-import diff --git a/openedx/core/djangoapps/discussions/data.py b/openedx/core/djangoapps/discussions/data.py new file mode 100644 index 000000000000..4d0bc7fcc1a1 --- /dev/null +++ b/openedx/core/djangoapps/discussions/data.py @@ -0,0 +1,45 @@ +""" +Data classes for discussions +""" +from typing import List + +import attr +from opaque_keys.edx.keys import CourseKey, UsageKey + +# TODO: These data models will be moved to openedx_events. They are currently here to simplify the PR. + + +@attr.s(frozen=True) +class DiscussionTopicContext: + """ + Context for linking the external id for a discussion topic to its associated usage key. + + The ``title`` is cached to improve the performance, since otherwise we'd + need to look it up in the course structure each time. + + The ``group_id`` can be used for providers that don't internally support + cohorting but we can emulate that wuth different contexts for different groups. + """ + title = attr.ib(type=str) + usage_key = attr.ib(type=UsageKey, default=None) + group_id = attr.ib(type=int, default=None) + external_id = attr.ib(type=str, default=None) + + +@attr.s(frozen=True) +class CourseDiscussionConfigurationData: + """ + Course configuration information for discussions. + + Contains all the metadata needed to configure discussions for a course. + + The ``contexts`` field contains all the contexts for which discussion + is to be enabled. + """ + course_key = attr.ib(type=CourseKey) + provider_type = attr.ib(type=str) + enable_in_context = attr.ib(type=bool, default=True) + enable_graded_units = attr.ib(type=bool, default=False) + unit_level_visibility = attr.ib(type=bool, default=False) + plugin_configuration = attr.ib(type=dict, default={}) + contexts = attr.ib(type=List[DiscussionTopicContext], factory=list) diff --git a/openedx/core/djangoapps/discussions/handlers.py b/openedx/core/djangoapps/discussions/handlers.py new file mode 100644 index 000000000000..fa7ded2a350d --- /dev/null +++ b/openedx/core/djangoapps/discussions/handlers.py @@ -0,0 +1,93 @@ +""" +Signal handlers for discussions events +""" +import logging +from uuid import uuid4 + +from django.db import transaction + +from openedx.core.djangoapps.discussions.data import CourseDiscussionConfigurationData +from openedx.core.djangoapps.discussions.models import ( + DEFAULT_PROVIDER_TYPE, + DiscussionTopicLink, + DiscussionsConfiguration, +) +from openedx.core.djangoapps.discussions.signals import COURSE_DISCUSSIONS_UPDATED + +log = logging.getLogger(__name__) + + +# pylint: disable=unused-argument +def handle_course_discussion_config_update(sender, configuration: CourseDiscussionConfigurationData, **kwargs): + """ + Updates the database models for course topics and configuration when settings are updated in the course. + + Args: + sender: Ignored + configuration (CourseDiscussionConfigurationData): configuration data for the course + + """ + update_course_discussion_config(configuration) + + +def update_course_discussion_config(configuration: CourseDiscussionConfigurationData): + """ + Update the database version of the configuration if it changes in the course structure. + Args: + configuration (CourseDiscussionConfigurationData): configuration data for the course + """ + course_key = configuration.course_key + provider_id = configuration.provider_type or DEFAULT_PROVIDER_TYPE + new_topic_map = { + (topic_context.usage_key or topic_context.external_id): topic_context + for topic_context in configuration.contexts + } + with transaction.atomic(): + # Go over existing topics in the database and update them if they have been changed. + # i.e. in case a unit display name has changed, or a unit has been removed, or otherwise made inaccessible + log.info(f"Updating existing discussion topic links for {course_key}") + for topic_link in DiscussionTopicLink.objects.filter( + context_key=course_key, provider_id=provider_id, + ): + lookup_key = topic_link.usage_key or topic_link.external_id + topic_context = new_topic_map.pop(lookup_key, None) + # TODO: handle deleting topics that are no longer in use + # currently this will simply not work for course-wide topics since deleting the link will + # remove access to all posts in the topic. + # If the topic exists in the db, but not in the new configuration, it has been removed or otherwise + # made inaccessible. + if topic_context is None: + topic_link.enabled_in_context = False + else: + topic_link.enabled_in_context = True + topic_link.title = topic_context.title + topic_link.save() + log.info(f"Creating new discussion topic links for {course_key}") + # If there are any topic contexts left here, they are new or not present in DiscussionTopicLink + DiscussionTopicLink.objects.bulk_create([ + DiscussionTopicLink( + context_key=course_key, + usage_key=topic_context.usage_key, + title=topic_context.title, + provider_id=provider_id, + external_id=topic_context.external_id or uuid4(), + enabled_in_context=True, + ) + for topic_context in new_topic_map.values() + ]) + + # If this is a new course save changes to the model. + if not DiscussionsConfiguration.objects.filter(context_key=course_key).exists(): + log.info(f"Course {course_key} doesn't have discussion configuration model yet. Creating a new one.") + DiscussionsConfiguration( + context_key=course_key, + provider_type=provider_id, + plugin_configuration=configuration.plugin_configuration, + enable_in_context=configuration.enable_in_context, + enable_graded_units=configuration.enable_graded_units, + unit_level_visibility=configuration.unit_level_visibility, + ).save() + # If the model already exists, then it's already up-to-date. + + +COURSE_DISCUSSIONS_UPDATED.connect(handle_course_discussion_config_update) diff --git a/openedx/core/djangoapps/discussions/migrations/0007_add_discussion_topic_link.py b/openedx/core/djangoapps/discussions/migrations/0007_add_discussion_topic_link.py new file mode 100644 index 000000000000..4a4b4790925c --- /dev/null +++ b/openedx/core/djangoapps/discussions/migrations/0007_add_discussion_topic_link.py @@ -0,0 +1,57 @@ +# Generated by Django 3.2.8 on 2021-10-21 12:02 + +import django.db.models.deletion +import django_mysql.models +import opaque_keys.edx.django.models +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ('course_groups', '0003_auto_20170609_1455'), + ('discussions', '0006_auto_20211006_0441'), + ] + + operations = [ + migrations.AlterField( + model_name='providerfilter', + name='allow', + field=django_mysql.models.ListCharField(models.CharField( + choices=[('legacy', 'legacy'), ('openedx', 'openedx'), ('ed-discuss', 'ed-discuss'), + ('inscribe', 'inscribe'), ('piazza', 'piazza'), ('yellowdig', 'yellowdig')], max_length=20), + blank=True, + help_text='Comma-separated list of providers to allow, eg: legacy,openedx,ed-discuss,inscribe,piazza,yellowdig', + max_length=63, size=3, verbose_name='Allow List'), + ), + migrations.AlterField( + model_name='providerfilter', + name='deny', + field=django_mysql.models.ListCharField(models.CharField( + choices=[('legacy', 'legacy'), ('openedx', 'openedx'), ('ed-discuss', 'ed-discuss'), + ('inscribe', 'inscribe'), ('piazza', 'piazza'), ('yellowdig', 'yellowdig')], max_length=20), + blank=True, + help_text='Comma-separated list of providers to deny, eg: legacy,openedx,ed-discuss,inscribe,piazza,yellowdig', + max_length=63, size=3, verbose_name='Deny List'), + ), + migrations.CreateModel( + name='DiscussionTopicLink', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('context_key', opaque_keys.edx.django.models.LearningContextKeyField(db_index=True, + help_text='Context key for context in which this discussion topic exists.', max_length=255, + verbose_name='Learning Context Key')), + ('usage_key', opaque_keys.edx.django.models.UsageKeyField(blank=True, db_index=True, + help_text='Usage key for in-context discussion topic. Set to null for course-level topics.', + max_length=255, null=True)), + ('title', models.CharField(help_text='Title for discussion topic.', max_length=255)), + ('provider_id', models.CharField(help_text='Provider id for discussion provider.', max_length=32)), + ('external_id', models.CharField(db_index=True, + help_text='Discussion context ID in external forum provider. e.g. commentable_id for cs_comments_service.', + max_length=255)), + ('enabled_in_context', models.BooleanField(default=True, + help_text='Whether this topic should be shown in-context in the course.')), + ('group', models.ForeignKey(blank=True, help_text='Group for divided discussions.', null=True, + on_delete=django.db.models.deletion.SET_NULL, to='course_groups.courseusergroup')), + ], + ), + ] diff --git a/openedx/core/djangoapps/discussions/models.py b/openedx/core/djangoapps/discussions/models.py index 99a56df325f2..31e035e24c3b 100644 --- a/openedx/core/djangoapps/discussions/models.py +++ b/openedx/core/djangoapps/discussions/models.py @@ -16,12 +16,13 @@ from jsonfield import JSONField from lti_consumer.models import LtiConfiguration from model_utils.models import TimeStampedModel -from opaque_keys.edx.django.models import LearningContextKeyField +from opaque_keys.edx.django.models import LearningContextKeyField, UsageKeyField from opaque_keys.edx.keys import CourseKey from simple_history.models import HistoricalRecords from openedx.core.djangoapps.config_model_utils.models import StackedConfigurationModel from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +from openedx.core.djangoapps.course_groups.models import CourseUserGroup from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers log = logging.getLogger(__name__) @@ -128,6 +129,32 @@ def pii_sharing_required_message(provider_name): 'messages': [], 'has_full_support': True }, + 'openedx': { + 'features': [ + Features.BASIC_CONFIGURATION.value, + Features.PRIMARY_DISCUSSION_APP_EXPERIENCE.value, + Features.QUESTION_DISCUSSION_SUPPORT.value, + Features.COMMUNITY_TA_SUPPORT.value, + Features.REPORT_FLAG_CONTENT_TO_MODERATORS.value, + Features.AUTOMATIC_LEARNER_ENROLLMENT.value, + Features.ANONYMOUS_POSTING.value, + Features.INTERNATIONALIZATION_SUPPORT.value, + Features.WCAG_2_0_SUPPORT.value, + Features.BLACKOUT_DISCUSSION_DATES.value, + Features.COURSE_COHORT_SUPPORT.value, + Features.RESEARCH_DATA_EVENTS.value, + ], + 'external_links': ProviderExternalLinks( + learn_more='', + configuration='', + general='', + accessibility='', + contact_email='', + )._asdict(), + 'messages': [], + 'has_full_support': True, + 'supports_in_context_discussions': True, + }, 'ed-discuss': { 'features': [ Features.PRIMARY_DISCUSSION_APP_EXPERIENCE.value, @@ -402,6 +429,12 @@ def __str__(self): enabled=self.enabled, ) + def supports_in_context_discussions(self): + """ + Returns is the provider supports in-context discussions + """ + return AVAILABLE_PROVIDER_MAP.get(self.provider_type, {}).get('supports_in_context_discussions', False) + def supports(self, feature: str) -> bool: """ Check if the provider supports some feature @@ -510,3 +543,58 @@ def is_enabled(cls, program_uuid) -> bool: return configuration.enabled except cls.DoesNotExist: return False + + +class DiscussionTopicLink(models.Model): + """ + A model linking discussion topics ids to the part of a course they are linked to. + """ + context_key = LearningContextKeyField( + db_index=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"), + help_text=_("Context key for context in which this discussion topic exists.") + ) + usage_key = UsageKeyField( + db_index=True, + max_length=255, + null=True, + blank=True, + help_text=_("Usage key for in-context discussion topic. Set to null for course-level topics.") + ) + title = models.CharField( + max_length=255, + help_text=_("Title for discussion topic.") + ) + group = models.ForeignKey( + CourseUserGroup, + null=True, + blank=True, + on_delete=models.SET_NULL, + help_text=_("Group for divided discussions.") + ) + provider_id = models.CharField( + max_length=32, + help_text=_("Provider id for discussion provider.") + ) + external_id = models.CharField( + db_index=True, + max_length=255, + help_text=_("Discussion context ID in external forum provider. e.g. commentable_id for cs_comments_service.") + ) + enabled_in_context = models.BooleanField( + default=True, + help_text=_("Whether this topic should be shown in-context in the course.") + ) + + def __str__(self): + return ( + f'DiscussionTopicLink(' + f'context_key="{self.context_key}", usage_key="{self.usage_key}", title="{self.title}", ' + f'group={self.group}, provider_id="{self.provider_id}", external_id="{self.external_id}", ' + f'enabled_in_context={self.enabled_in_context}' + f')' + ) diff --git a/openedx/core/djangoapps/discussions/signals.py b/openedx/core/djangoapps/discussions/signals.py new file mode 100644 index 000000000000..53eb9608a3a4 --- /dev/null +++ b/openedx/core/djangoapps/discussions/signals.py @@ -0,0 +1,18 @@ +""" +Signals for discussions +""" +from openedx_events.tooling import OpenEdxPublicSignal + +from openedx.core.djangoapps.discussions.data import CourseDiscussionConfigurationData + +# TODO: This will be moved to openedx_events. It's currently here to simplify the PR. +# .. event_type: org.openedx.learning.discussions.configuration.change.v1 +# .. event_name: COURSE_DISCUSSIONS_UPDATED +# .. event_description: emitted when the configuration for a course's discussions changes in the course +# .. event_data: CourseDiscussionConfigurationData +COURSE_DISCUSSIONS_UPDATED = OpenEdxPublicSignal( + event_type="org.openedx.learning.discussions.configuration.change.v1", + data={ + "configuration": CourseDiscussionConfigurationData + } +) diff --git a/openedx/core/djangoapps/discussions/tasks.py b/openedx/core/djangoapps/discussions/tasks.py new file mode 100644 index 000000000000..6b4d540ea799 --- /dev/null +++ b/openedx/core/djangoapps/discussions/tasks.py @@ -0,0 +1,92 @@ +""" +Tasks for discussions +""" +import logging + +from celery import shared_task +from edx_django_utils.monitoring import set_code_owner_attribute +from opaque_keys.edx.keys import CourseKey + +from xmodule.modulestore import ModuleStoreEnum +from xmodule.modulestore.django import modulestore +from .data import CourseDiscussionConfigurationData, DiscussionTopicContext +from .models import DiscussionsConfiguration +from .signals import COURSE_DISCUSSIONS_UPDATED + +log = logging.getLogger(__name__) + + +@shared_task +@set_code_owner_attribute +def update_discussions_settings_from_course_task(course_key_str: str): + """ + Celery task that creates or updates discussions settings for a course. + + Args: + course_key_str (str): course key string + """ + course_key = CourseKey.from_string(course_key_str) + config_data = update_discussions_settings_from_course(course_key) + COURSE_DISCUSSIONS_UPDATED.send_event(configuration=config_data) + + +def update_discussions_settings_from_course(course_key: CourseKey) -> CourseDiscussionConfigurationData: + """ + When there are changes to a course, construct a new data structure containing all the context needed to update the + course's discussion settings in the database. + + Args: + course_key (CourseKey): The course that was recently updated. + + Returns: + (CourseDiscussionConfigurationData): structured discusion configuration data. + """ + log.info(f"Updating discussion settings for course: {course_key}") + store = modulestore() + + supports_in_context = DiscussionsConfiguration.get(course_key).supports_in_context_discussions() + + def iter_discussable_units(): + sections = store.get_items(course_key, qualifiers={'category': 'sequential'}) + for section in sections: + for unit in section.get_children(): + # If unit-level visibility is enabled and the unit doesn't have discussion enabled, skip it. + if unit_level_visibility and not getattr(unit, 'discussion_enabled', False): + continue + # If the unit is in a graded section and graded sections aren't enabled skip it. + if section.graded and not enable_graded_units: + continue + yield DiscussionTopicContext( + usage_key=unit.location, + title=unit.display_name, + group_id=None, + ) + + with store.branch_setting(ModuleStoreEnum.Branch.published_only, course_key): + course = store.get_course(course_key) + provider = course.discussions_settings.get('provider') + enable_in_context = course.discussions_settings.get('enable_in_context', True) + provider_config = course.discussions_settings.get(provider, {}) + unit_level_visibility = course.discussions_settings.get('unit_level_visibility', False) + enable_graded_units = course.discussions_settings.get('enable_graded_units', False) + contexts = [] + if supports_in_context: + contexts = [ + DiscussionTopicContext( + title=topic_name, + external_id=topic_config.get('id', None), + ) + for topic_name, topic_config in course.discussion_topics.items() + ] + if enable_in_context: + contexts.extend(list(iter_discussable_units())) + config_data = CourseDiscussionConfigurationData( + course_key=course_key, + enable_in_context=enable_in_context, + enable_graded_units=enable_graded_units, + unit_level_visibility=unit_level_visibility, + provider_type=provider, + plugin_configuration=provider_config, + contexts=contexts, + ) + return config_data diff --git a/openedx/core/djangoapps/discussions/tests/test_handlers.py b/openedx/core/djangoapps/discussions/tests/test_handlers.py new file mode 100644 index 000000000000..90e077ccb14d --- /dev/null +++ b/openedx/core/djangoapps/discussions/tests/test_handlers.py @@ -0,0 +1,163 @@ +""" +Tests for discussions signal handlers +""" +from unittest.mock import patch +from uuid import uuid4 + +import ddt +from django.test import TestCase +from opaque_keys.edx.keys import CourseKey + +from openedx.core.djangoapps.discussions.data import CourseDiscussionConfigurationData, DiscussionTopicContext +from openedx.core.djangoapps.discussions.handlers import update_course_discussion_config +from openedx.core.djangoapps.discussions.models import DiscussionTopicLink, DiscussionsConfiguration + + +@ddt.ddt +class UpdateCourseDiscussionsConfigTestCase(TestCase): + """ + Tests for the discussion config update handler. + """ + def setUp(self) -> None: + super().setUp() + self.course_key = CourseKey.from_string("course-v1:test+test+test") + self.discussion_config = DiscussionsConfiguration.objects.create( + context_key=self.course_key, + provider_type="openedx", + ) + + def create_contexts(self, general=0, unit=0): + """ + Create context data for topics + """ + for idx in range(general): + yield DiscussionTopicContext( + title=f"General topic {idx}", + external_id=f"general-topic-{idx}", + ) + for idx in range(unit): + yield DiscussionTopicContext( + title=f"Unit {idx}", + usage_key=self.course_key.make_usage_key("vertical", f"unit-{idx}"), + ) + + def test_configuration_for_new_course(self): + """ + Test that a new course gets a new discussion configuration object + """ + new_key = CourseKey.from_string("course-v1:test+test+test2") + config_data = CourseDiscussionConfigurationData( + course_key=new_key, + provider_type="openedx", + ) + assert not DiscussionsConfiguration.objects.filter(context_key=new_key).exists() + update_course_discussion_config(config_data) + assert DiscussionsConfiguration.objects.filter(context_key=new_key).exists() + db_config = DiscussionsConfiguration.objects.get(context_key=new_key) + assert db_config.provider_type == "openedx" + + def test_creating_new_links(self): + """ + Test that new links are created in the db when they are added in the config. + """ + contexts = list(self.create_contexts(general=2, unit=3)) + config_data = CourseDiscussionConfigurationData( + course_key=self.course_key, + provider_type="openedx", + contexts=contexts, + ) + update_course_discussion_config(config_data) + topic_links = DiscussionTopicLink.objects.filter(context_key=self.course_key) + assert topic_links.count() == len(contexts) # 2 general + 3 units + + def test_updating_existing_links(self): + """ + Test that updating existing links works as expected. + """ + contexts = list(self.create_contexts(general=2, unit=3)) + config_data = CourseDiscussionConfigurationData( + course_key=self.course_key, + provider_type="openedx", + contexts=contexts, + ) + existing_external_id = uuid4() + existing_topic_link = DiscussionTopicLink.objects.create( + context_key=self.course_key, + usage_key=self.course_key.make_usage_key("vertical", "unit-2"), + title="Old title", + provider_id="openedx", + external_id=existing_external_id, + enabled_in_context=True, + ) + update_course_discussion_config(config_data) + existing_topic_link.refresh_from_db() + # Make sure that the title changes, but nothing else + assert existing_topic_link.title == "Unit 2" + assert existing_topic_link.provider_id == "openedx" + assert existing_topic_link.external_id == str(existing_external_id) + assert existing_topic_link.enabled_in_context + + @patch.dict( + "openedx.core.djangoapps.discussions.models.AVAILABLE_PROVIDER_MAP", + {"test": {"supports_in_context_discussions": True}}, + ) + def test_provider_change(self): + """ + Test that changing providers creates new links, and doesn't update existing ones. + """ + contexts = list(self.create_contexts(general=2, unit=3)) + config_data = CourseDiscussionConfigurationData( + course_key=self.course_key, + provider_type="test", + contexts=contexts, + ) + existing_external_id = uuid4() + existing_usage_key = self.course_key.make_usage_key("vertical", "unit-2") + existing_topic_link = DiscussionTopicLink.objects.create( + context_key=self.course_key, + usage_key=existing_usage_key, + title="Old title", + provider_id="openedx", + external_id=existing_external_id, + enabled_in_context=True, + ) + update_course_discussion_config(config_data) + existing_topic_link.refresh_from_db() + # If the provider has changed, new links should be created, the existing on remains the same + assert existing_topic_link.title == "Old title" + assert existing_topic_link.provider_id == "openedx" + assert existing_topic_link.external_id == str(existing_external_id) + assert existing_topic_link.enabled_in_context + new_link = DiscussionTopicLink.objects.get( + context_key=self.course_key, + provider_id="test", + usage_key=existing_usage_key, + ) + assert new_link.title == "Unit 2" + # The new link will get a new id + assert new_link.external_id != str(existing_external_id) + + def test_enabled_units_change(self): + """ + Test that when enabled units change, old unit links are disabled in context. + """ + contexts = list(self.create_contexts(general=2, unit=3)) + config_data = CourseDiscussionConfigurationData( + course_key=self.course_key, + provider_type="openedx", + contexts=contexts, + ) + existing_external_id = uuid4() + existing_usage_key = self.course_key.make_usage_key("vertical", "unit-10") + existing_topic_link = DiscussionTopicLink.objects.create( + context_key=self.course_key, + usage_key=existing_usage_key, + title="Unit 10", + provider_id="openedx", + external_id=existing_external_id, + enabled_in_context=True, + ) + update_course_discussion_config(config_data) + existing_topic_link.refresh_from_db() + # If the unit has an existing link but is disabled or removed + assert not existing_topic_link.enabled_in_context diff --git a/openedx/core/djangoapps/discussions/tests/test_tasks.py b/openedx/core/djangoapps/discussions/tests/test_tasks.py new file mode 100644 index 000000000000..b9df4f61f6a8 --- /dev/null +++ b/openedx/core/djangoapps/discussions/tests/test_tasks.py @@ -0,0 +1,152 @@ +""" +Tests for discussions tasks. +""" +import ddt +import mock + +from openedx.core.djangoapps.discussions.data import DiscussionTopicContext +from openedx.core.djangoapps.discussions.tasks import update_discussions_settings_from_course +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory + + +@ddt.ddt +@mock.patch('openedx.core.djangoapps.discussions.tasks.DiscussionsConfiguration', mock.Mock()) +class UpdateDiscussionsSettingsFromCourseTestCase(ModuleStoreTestCase): + """ + Tests for the discussions settings update tasks + """ + def setUp(self): + super().setUp() + self.course = course = CourseFactory.create() + self.course_key = course_key = self.course.id + with self.store.bulk_operations(course_key): + section = ItemFactory.create( + parent_location=course.location, + category="chapter", + display_name="Section" + ) + sequence = ItemFactory.create( + parent_location=section.location, + category="sequential", + display_name="Sequence" + ) + unit = ItemFactory.create( + parent_location=sequence.location, + category="vertical", + display_name="Unit" + ) + ItemFactory.create( + parent_location=sequence.location, + category="vertical", + display_name="Discussable Unit", + discussion_enabled=True, + ) + ItemFactory.create( + parent_location=sequence.location, + category="vertical", + display_name="Non-Discussable Unit", + discussion_enabled=False, + ) + ItemFactory.create( + parent_location=unit.location, + category="html", + display_name="An HTML Module" + ) + graded_sequence = ItemFactory.create( + parent_location=section.location, + category="sequential", + display_name="Graded Sequence", + graded=True, + ) + graded_unit = ItemFactory.create( + parent_location=graded_sequence.location, + category="vertical", + display_name="Graded Unit" + ) + ItemFactory.create( + parent_location=graded_sequence.location, + category="vertical", + display_name="Discussable Graded Unit", + discussion_enabled=True, + ) + ItemFactory.create( + parent_location=graded_sequence.location, + category="vertical", + display_name="Non-Discussable Graded Unit", + discussion_enabled=False, + ) + ItemFactory.create( + parent_location=graded_unit.location, + category="html", + display_name="Graded HTML Module" + ) + + def update_course_field(self, **update): + """ + Update the test course using provided parameters. + """ + for key, value in update.items(): + setattr(self.course, key, value) + self.update_course(self.course, self.user.id) + + def update_discussions_settings(self, settings): + """ + Update course discussion settings based on the provided discussion settings. + """ + self.course.discussions_settings.update(settings) + self.update_course(self.course, self.user.id) + + def test_default(self): + """ + Test that the course defaults. + """ + config_data = update_discussions_settings_from_course(self.course.id) + assert config_data.course_key == self.course.id + assert config_data.enable_graded_units is False + assert config_data.unit_level_visibility is False + assert config_data.provider_type is None + assert config_data.plugin_configuration == {} + assert len(config_data.contexts) == 4 + + def test_general_topics(self): + """ + Test the handling of course general topics. + """ + self.update_course_field(discussion_topics={ + "General": {"id": "general-topic"}, + "Test Topic": {"id": "test-topic"}, + }) + config_data = update_discussions_settings_from_course(self.course.id) + assert len(config_data.contexts) == 5 + assert DiscussionTopicContext( + title="General", + external_id="general-topic", + ) in config_data.contexts + assert DiscussionTopicContext( + title="Test Topic", + external_id="test-topic", + ) in config_data.contexts + + @ddt.data( + ({"enable_in_context": False}, 1, set(), {"Unit", "Graded Unit"}), + ({"enable_graded_units": False}, 4, {"Unit", "Discussable Unit", "Non-Discussable Unit"}, + {"Graded Unit"}), + ({"enable_graded_units": True}, 7, {"Unit", "Graded Unit", "Discussable Graded Unit"}, set()), + ({"unit_level_visibility": True}, 2, {"Discussable Unit"}, + {"Unit", "Graded Unit", "Non-Discussable Unit", "Discussable Graded Unit", "Non-Discussable Graded Unit"}), + ({"unit_level_visibility": True, "enable_graded_units": True}, 3, + {"Discussable Unit", "Discussable Graded Unit"}, + {"Graded Unit", "Non-Discussable Unit", "Non-Discussable Graded Unit"}), + ) + @ddt.unpack + def test_custom_discussion_settings(self, settings, context_count, present_units, missing_units): + """ + Test different combinations of settings and their impact on the units that are returned. + """ + self.update_discussions_settings(settings) + config_data = update_discussions_settings_from_course(self.course.id) + assert len(config_data.contexts) == context_count + units_in_config = {context.title for context in config_data.contexts} + assert present_units <= units_in_config + assert not missing_units & units_in_config From abc3f68edccd405deae0b329919b933657b0c8ed Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Thu, 28 Oct 2021 16:09:15 +0530 Subject: [PATCH 2/3] fix: add support for marking a provider as supporting LTI --- openedx/core/djangoapps/discussions/models.py | 8 +++++++- openedx/features/lti_course_tab/tests.py | 3 ++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/discussions/models.py b/openedx/core/djangoapps/discussions/models.py index 31e035e24c3b..ab2505715917 100644 --- a/openedx/core/djangoapps/discussions/models.py +++ b/openedx/core/djangoapps/discussions/models.py @@ -119,6 +119,7 @@ def pii_sharing_required_message(provider_name): Features.COURSE_COHORT_SUPPORT.value, Features.RESEARCH_DATA_EVENTS.value, ], + 'supports_lti': False, 'external_links': ProviderExternalLinks( learn_more='', configuration='', @@ -144,6 +145,7 @@ def pii_sharing_required_message(provider_name): Features.COURSE_COHORT_SUPPORT.value, Features.RESEARCH_DATA_EVENTS.value, ], + 'supports_lti': False, 'external_links': ProviderExternalLinks( learn_more='', configuration='', @@ -171,6 +173,7 @@ def pii_sharing_required_message(provider_name): Features.IN_PLATFORM_NOTIFICATIONS.value, Features.USER_MENTIONS.value, ], + 'supports_lti': True, 'external_links': ProviderExternalLinks( learn_more='', configuration='', @@ -198,6 +201,7 @@ def pii_sharing_required_message(provider_name): Features.IN_PLATFORM_NOTIFICATIONS.value, Features.DISCUSSION_CONTENT_PROMPTS.value, ], + 'supports_lti': True, 'external_links': ProviderExternalLinks( learn_more='', configuration='', @@ -221,6 +225,7 @@ def pii_sharing_required_message(provider_name): Features.WCAG_2_0_SUPPORT.value, Features.BLACKOUT_DISCUSSION_DATES.value, ], + 'supports_lti': True, 'external_links': ProviderExternalLinks( learn_more='https://piazza.com/product/overview', configuration='https://support.piazza.com/support/solutions/articles/48001065447-configure-piazza-within-edx', # pylint: disable=line-too-long @@ -246,6 +251,7 @@ def pii_sharing_required_message(provider_name): Features.DIRECT_MESSAGES_FROM_INSTRUCTORS.value, Features.USER_MENTIONS.value, ], + 'supports_lti': True, 'external_links': ProviderExternalLinks( learn_more='https://www.youtube.com/watch?v=ZACief-qMwY', configuration='', @@ -445,7 +451,7 @@ def supports(self, feature: str) -> bool: def supports_lti(self) -> bool: """Returns a boolean indicating if the provider supports lti discussion view.""" - return self.provider_type != DEFAULT_PROVIDER_TYPE + return AVAILABLE_PROVIDER_MAP.get(self.provider_type, {}).get('supports_lti', False) @classmethod def is_enabled(cls, context_key: CourseKey) -> bool: diff --git a/openedx/features/lti_course_tab/tests.py b/openedx/features/lti_course_tab/tests.py index 91074d661e80..ce0f483b584d 100644 --- a/openedx/features/lti_course_tab/tests.py +++ b/openedx/features/lti_course_tab/tests.py @@ -22,7 +22,8 @@ def setUp(self): self.discussion_config = DiscussionsConfiguration.objects.create( context_key=self.course.id, enabled=False, - provider_type="lti_provider", + # Pick a provider that supports LTI + provider_type="yellowdig", ) self.discussion_config.lti_configuration = LtiConfiguration.objects.create( config_store=LtiConfiguration.CONFIG_ON_DB, From 7d2bab5e580f15ff675fb665be35405cc26e5b6d Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Tue, 9 Nov 2021 09:32:55 +0530 Subject: [PATCH 3/3] fix: review feedback --- .../contentstore/signals/handlers.py | 2 +- .../core/djangoapps/discussions/handlers.py | 18 +++++++++++------- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/cms/djangoapps/contentstore/signals/handlers.py b/cms/djangoapps/contentstore/signals/handlers.py index 9272be6ec489..377c01c1bf2b 100644 --- a/cms/djangoapps/contentstore/signals/handlers.py +++ b/cms/djangoapps/contentstore/signals/handlers.py @@ -3,10 +3,10 @@ import logging from datetime import datetime +from functools import wraps from django.core.cache import cache from django.dispatch import receiver -from functools import wraps from pytz import UTC from cms.djangoapps.contentstore.courseware_index import ( diff --git a/openedx/core/djangoapps/discussions/handlers.py b/openedx/core/djangoapps/discussions/handlers.py index fa7ded2a350d..009b2a05b824 100644 --- a/openedx/core/djangoapps/discussions/handlers.py +++ b/openedx/core/djangoapps/discussions/handlers.py @@ -33,6 +33,16 @@ def handle_course_discussion_config_update(sender, configuration: CourseDiscussi def update_course_discussion_config(configuration: CourseDiscussionConfigurationData): """ Update the database version of the configuration if it changes in the course structure. + + This function accepts a discussion configuration object that represents the current + configuration and applies that state to the database. It will go over the list of topic + links in the configuration, find the corresponding topic link in the database and apply + any changes if needed. If a new topic link has been introduced it will create an entry. + If a topic has been removed, it will deactivate the entry. + + When this runs on a new course it will create a new DiscussionConfiguration entry for + the course. + Args: configuration (CourseDiscussionConfigurationData): configuration data for the course """ @@ -43,8 +53,6 @@ def update_course_discussion_config(configuration: CourseDiscussionConfiguration for topic_context in configuration.contexts } with transaction.atomic(): - # Go over existing topics in the database and update them if they have been changed. - # i.e. in case a unit display name has changed, or a unit has been removed, or otherwise made inaccessible log.info(f"Updating existing discussion topic links for {course_key}") for topic_link in DiscussionTopicLink.objects.filter( context_key=course_key, provider_id=provider_id, @@ -54,8 +62,6 @@ def update_course_discussion_config(configuration: CourseDiscussionConfiguration # TODO: handle deleting topics that are no longer in use # currently this will simply not work for course-wide topics since deleting the link will # remove access to all posts in the topic. - # If the topic exists in the db, but not in the new configuration, it has been removed or otherwise - # made inaccessible. if topic_context is None: topic_link.enabled_in_context = False else: @@ -63,7 +69,7 @@ def update_course_discussion_config(configuration: CourseDiscussionConfiguration topic_link.title = topic_context.title topic_link.save() log.info(f"Creating new discussion topic links for {course_key}") - # If there are any topic contexts left here, they are new or not present in DiscussionTopicLink + DiscussionTopicLink.objects.bulk_create([ DiscussionTopicLink( context_key=course_key, @@ -76,7 +82,6 @@ def update_course_discussion_config(configuration: CourseDiscussionConfiguration for topic_context in new_topic_map.values() ]) - # If this is a new course save changes to the model. if not DiscussionsConfiguration.objects.filter(context_key=course_key).exists(): log.info(f"Course {course_key} doesn't have discussion configuration model yet. Creating a new one.") DiscussionsConfiguration( @@ -87,7 +92,6 @@ def update_course_discussion_config(configuration: CourseDiscussionConfiguration enable_graded_units=configuration.enable_graded_units, unit_level_visibility=configuration.unit_level_visibility, ).save() - # If the model already exists, then it's already up-to-date. COURSE_DISCUSSIONS_UPDATED.connect(handle_course_discussion_config_update)