From 16881afcad3d2b331be606e75e7c00df7bf9b790 Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Mon, 31 Jan 2022 10:58:27 +0000 Subject: [PATCH] feat: Allow fetching details of inactive providers [BD-38] (#29656) * feat: Allow fetching details of inactive providers * squash!: update tests add docs * squash!: review feedback * squash!: fix quality issue * squash!: review feedback --- openedx/core/djangoapps/discussions/models.py | 1 - .../djangoapps/discussions/serializers.py | 87 +++++--- .../discussions/tests/test_views.py | 57 +++-- openedx/core/djangoapps/discussions/urls.py | 21 +- openedx/core/djangoapps/discussions/views.py | 196 +++++++++++++++++- 5 files changed, 306 insertions(+), 56 deletions(-) diff --git a/openedx/core/djangoapps/discussions/models.py b/openedx/core/djangoapps/discussions/models.py index cc7f4e11da..25c692598c 100644 --- a/openedx/core/djangoapps/discussions/models.py +++ b/openedx/core/djangoapps/discussions/models.py @@ -169,7 +169,6 @@ AVAILABLE_PROVIDER_MAP = { 'messages': [], 'has_full_support': True, 'supports_in_context_discussions': True, - 'visible': False, }, Provider.ED_DISCUSS: { 'features': [ diff --git a/openedx/core/djangoapps/discussions/serializers.py b/openedx/core/djangoapps/discussions/serializers.py index 20db67db3b..ace407972b 100644 --- a/openedx/core/djangoapps/discussions/serializers.py +++ b/openedx/core/djangoapps/discussions/serializers.py @@ -9,7 +9,7 @@ from rest_framework import serializers from openedx.core.djangoapps.django_comment_common.models import CourseDiscussionSettings from openedx.core.lib.courses import get_course_by_id from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order -from .models import AVAILABLE_PROVIDER_MAP, DEFAULT_PROVIDER_TYPE, DiscussionsConfiguration, Features, Provider +from .models import DiscussionsConfiguration, Provider from .utils import available_division_schemes, get_divided_discussions @@ -208,41 +208,34 @@ class DiscussionsConfigurationSerializer(serializers.ModelSerializer): Serialize data into a dictionary, to be used as a response """ course_key = instance.context_key + active_provider = instance.provider_type + provider_type = self.context.get('provider_type') or active_provider payload = super().to_representation(instance) course_pii_sharing_allowed = get_lti_pii_sharing_state_for_course(course_key) - lti_configuration = LtiSerializer(instance=instance.lti_configuration) - lti_configuration_data = lti_configuration.data - lti_configuration_data.update({ - 'pii_sharing_allowed': course_pii_sharing_allowed - }) - - provider_type = instance.provider_type - plugin_configuration = instance.plugin_configuration + # LTI configuration is only stored for the active provider. + if provider_type == active_provider: + lti_configuration = LtiSerializer(instance=instance.lti_configuration) + lti_configuration_data = lti_configuration.data + plugin_configuration = instance.plugin_configuration + else: + lti_configuration_data = {} + plugin_configuration = {} course = get_course_by_id(course_key) - legacy_settings = LegacySettingsSerializer( - course, - data=plugin_configuration, - ) - if legacy_settings.is_valid(raise_exception=True): - plugin_configuration = legacy_settings.data - features_list = [ - {'id': feature.value, 'feature_support_type': feature.feature_support_type} - for feature in Features - ] + if provider_type in [Provider.LEGACY, Provider.OPEN_EDX]: + legacy_settings = LegacySettingsSerializer(course, data=plugin_configuration) + if legacy_settings.is_valid(raise_exception=True): + plugin_configuration = legacy_settings.data + if provider_type == Provider.OPEN_EDX: + plugin_configuration.update({ + "group_at_subsection": instance.plugin_configuration.get("group_at_subsection", False) + }) + lti_configuration_data.update({'pii_sharing_allowed': course_pii_sharing_allowed}) payload.update({ - 'features': features_list, + 'provider_type': provider_type, 'lti_configuration': lti_configuration_data, 'plugin_configuration': plugin_configuration, - 'providers': { - 'active': provider_type or DEFAULT_PROVIDER_TYPE, - 'available': { - key: value - for key, value in AVAILABLE_PROVIDER_MAP.items() - if value.get('visible', True) - }, - }, }) return payload @@ -401,7 +394,6 @@ class DiscussionSettingsSerializer(serializers.Serializer): """ Return a serialized representation of the course discussion settings. """ - payload = super().to_representation(instance) course = self.context['course'] instance = self.context['settings'] course_key = course.id @@ -434,3 +426,40 @@ class DiscussionSettingsSerializer(serializers.Serializer): except ValueError as e: raise ValidationError(str(e)) from e return instance + + +class DiscussionsProviderSerializer(serializers.Serializer): + """ + Serializer for a discussion provider + """ + features = serializers.ListField(child=serializers.CharField(), help_text="Features supported by the provider") + supports_lti = serializers.BooleanField(default=False, help_text="Whether the provider supports LTI") + external_links = serializers.DictField(help_text="External documentation and links for provider") + messages = serializers.ListField(child=serializers.CharField(), help_text="Custom messaging for provider") + has_full_support = serializers.BooleanField(help_text="Whether the provider is fully supported") + + +class DiscussionsFeatureSerializer(serializers.Serializer): + """ + Serializer for discussions features + """ + id = serializers.CharField(help_text="Feature ID") + feature_support_type = serializers.CharField(help_text="Feature support level classification") + + +class DiscussionsProvidersSerializer(serializers.Serializer): + """ + Serializer for discussion providers. + """ + active = serializers.CharField( + read_only=True, + help_text="The current active provider", + ) + features = serializers.ListField( + child=DiscussionsFeatureSerializer(read_only=True), + help_text="Features support classification levels", + ) + available = serializers.DictField( + child=DiscussionsProviderSerializer(read_only=True), + help_text="List of available providers", + ) diff --git a/openedx/core/djangoapps/discussions/tests/test_views.py b/openedx/core/djangoapps/discussions/tests/test_views.py index d46624e15a..f6128896db 100644 --- a/openedx/core/djangoapps/discussions/tests/test_views.py +++ b/openedx/core/djangoapps/discussions/tests/test_views.py @@ -9,15 +9,18 @@ from datetime import datetime, timedelta, timezone import ddt from django.core.exceptions import ValidationError from django.urls import reverse +from edx_toggles.toggles.testutils import override_waffle_flag from lti_consumer.models import CourseAllowPIISharingInLTIFlag from rest_framework import status from rest_framework.test import APITestCase - from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.tests.django_utils import CourseUserType, ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory + from common.djangoapps.student.tests.factories import UserFactory from lms.djangoapps.discussion.django_comment_client.tests.factories import RoleFactory +from lms.djangoapps.discussion.toggles import ENABLE_DISCUSSIONS_MFE + from ..models import AVAILABLE_PROVIDER_MAP, DEFAULT_CONFIG_ENABLED, DEFAULT_PROVIDER_TYPE, Provider DATA_LEGACY_COHORTS = { @@ -317,7 +320,6 @@ class DataTest(AuthorizedApiTest): Provider.INSCRIBE, Provider.PIAZZA, Provider.YELLOWDIG, - Provider.LEGACY ) def test_add_valid_configuration(self, provider_type): """ @@ -335,7 +337,7 @@ class DataTest(AuthorizedApiTest): data = self.get() assert data['enabled'] assert data['provider_type'] == provider_type - assert data['plugin_configuration'] == DEFAULT_LEGACY_CONFIGURATION + assert data['plugin_configuration'] == {'key': 'value'} assert data['lti_configuration'] == DEFAULT_LTI_CONFIGURATION def test_change_plugin_configuration(self): @@ -352,13 +354,43 @@ class DataTest(AuthorizedApiTest): } response = self._post(payload) data = response.json() - assert data['plugin_configuration'] == DEFAULT_LEGACY_CONFIGURATION + assert data['plugin_configuration'] == { + 'allow_anonymous': False, + 'custom_field': 'custom_value', + } course = self.store.get_course(self.course.id) # Only configuration fields not stored in the course, or # directly in the model should be stored here. assert course.discussions_settings['piazza'] == {'custom_field': 'custom_value'} + @ddt.data( + # If the legacy provider is selected show the legacy provider only + # and hide the new provider even if the mfe is enabled + (Provider.LEGACY, [Provider.LEGACY], Provider.OPEN_EDX, False), + (Provider.LEGACY, [Provider.LEGACY], Provider.OPEN_EDX, True), + # If the new provider is selected show the new provider only + # and hide the legacy provider even if the mfe is disabled + (Provider.OPEN_EDX, [Provider.OPEN_EDX], Provider.LEGACY, False), + (Provider.OPEN_EDX, [Provider.OPEN_EDX], Provider.LEGACY, True), + # If some other provider is selected show the new provider + # if the mfe is enabled + (Provider.PIAZZA, [Provider.LEGACY], Provider.OPEN_EDX, False), + (Provider.PIAZZA, [Provider.OPEN_EDX, Provider.LEGACY], 'dummy', True), + ) + @ddt.unpack + def test_available_providers_legacy(self, current_provider, visible_providers, hidden_provider, mfe_enabled): + """ + Tests that providers available depending on the course. + """ + self._configure_lti_discussion_provider(provider=current_provider) + with override_waffle_flag(ENABLE_DISCUSSIONS_MFE, mfe_enabled): + response = self._get() + data = response.json() + for visible_provider in visible_providers: + assert visible_provider in data['providers']['available'].keys() + assert hidden_provider not in data['providers']['available'].keys() + @ddt.data( {'enabled': 3}, ) @@ -503,10 +535,7 @@ class DataTest(AuthorizedApiTest): data = self.get() assert data['enabled'] assert data['provider_type'] == Provider.ED_DISCUSS - assert data['plugin_configuration'] == { - **DEFAULT_LEGACY_CONFIGURATION, - 'allow_anonymous': False, - } + assert data['plugin_configuration'] == {} assert data['lti_configuration'] == DATA_LTI_CONFIGURATION_DISABLED_PII def test_change_from_lti(self): @@ -727,10 +756,12 @@ class PIISettingsAPITests(DataTest): """ data = self._configure_lti_discussion_provider(provider=provider) self._assert_pii_flag_for_course(enabled=False) + expected_providers = AVAILABLE_PROVIDER_MAP[provider] + expected_providers.pop('admin_only_config', None) assert data['enabled'] assert data['provider_type'] == provider - assert data['providers']['available'][provider] == AVAILABLE_PROVIDER_MAP[provider] - assert data['plugin_configuration'] == DEFAULT_LEGACY_CONFIGURATION + assert data['providers']['available'][provider] == expected_providers + assert data['plugin_configuration'] == {} assert data['lti_configuration'] == DATA_LTI_CONFIGURATION_DISABLED_PII response_data = self.get() @@ -750,10 +781,12 @@ class PIISettingsAPITests(DataTest): with self._pii_sharing_for_course(enabled=True): self._assert_pii_flag_for_course(enabled=True) data = self._configure_lti_discussion_provider(provider=provider) + expected_providers = AVAILABLE_PROVIDER_MAP[provider] + expected_providers.pop('admin_only_config', None) assert data['enabled'] assert data['provider_type'] == provider - assert data['providers']['available'][provider] == AVAILABLE_PROVIDER_MAP[provider] - assert data['plugin_configuration'] == DEFAULT_LEGACY_CONFIGURATION + assert data['providers']['available'][provider] == expected_providers + assert data['plugin_configuration'] == {} assert data['lti_configuration'] == DATA_LTI_CONFIGURATION_ENABLED_PII response_data = self.get() diff --git a/openedx/core/djangoapps/discussions/urls.py b/openedx/core/djangoapps/discussions/urls.py index 862607c1da..7a68616ce0 100644 --- a/openedx/core/djangoapps/discussions/urls.py +++ b/openedx/core/djangoapps/discussions/urls.py @@ -1,15 +1,26 @@ """ Configure URL endpoints for the djangoapp """ -from django.conf.urls import url +from django.urls import re_path +from django.conf import settings -from .views import DiscussionsConfigurationView +from .views import CombinedDiscussionsConfigurationView, DiscussionsConfigurationSettingsView, DiscussionsProvidersView urlpatterns = [ - url( - r'^v0/(?P.+)$', - DiscussionsConfigurationView.as_view(), + re_path( + fr'^v0/{settings.COURSE_KEY_PATTERN}$', + CombinedDiscussionsConfigurationView.as_view(), name='discussions', ), + re_path( + fr'^v0/course/{settings.COURSE_KEY_PATTERN}/settings$', + DiscussionsConfigurationSettingsView.as_view(), + name='discussions-settings', + ), + re_path( + fr'^v0/course/{settings.COURSE_KEY_PATTERN}/providers$', + DiscussionsProvidersView.as_view(), + name='discussions-providers', + ), ] diff --git a/openedx/core/djangoapps/discussions/views.py b/openedx/core/djangoapps/discussions/views.py index 918ef86fa7..6b87a78251 100644 --- a/openedx/core/djangoapps/discussions/views.py +++ b/openedx/core/djangoapps/discussions/views.py @@ -1,22 +1,31 @@ """ Handle view-logic for the discussions app. """ +from typing import Dict + +import edx_api_doc_tools as apidocs from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser +from rest_framework.exceptions import ValidationError +from rest_framework.request import Request from rest_framework.response import Response from rest_framework.views import APIView +from lms.djangoapps.discussion.toggles import ENABLE_DISCUSSIONS_MFE from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.lib.api.authentication import BearerAuthenticationAllowInactiveUser from openedx.core.lib.api.view_utils import validate_course_key -from .models import DiscussionsConfiguration -from .permissions import check_course_permissions, IsStaffOrCourseTeam -from .serializers import DiscussionsConfigurationSerializer +from .models import AVAILABLE_PROVIDER_MAP, DiscussionsConfiguration, Features, Provider +from .permissions import IsStaffOrCourseTeam, check_course_permissions +from .serializers import ( + DiscussionsConfigurationSerializer, + DiscussionsProvidersSerializer, +) -class DiscussionsConfigurationView(APIView): +class DiscussionsConfigurationSettingsView(APIView): """ - Handle configuration-related view-logic + View for configuring discussion settings. """ authentication_classes = ( JwtAuthentication, @@ -25,25 +34,77 @@ class DiscussionsConfigurationView(APIView): ) permission_classes = (IsStaffOrCourseTeam,) - # pylint: disable=redefined-builtin - def get(self, request, course_key_string: str, **_kwargs) -> Response: + @apidocs.schema( + parameters=[ + apidocs.string_parameter( + 'course_id', + apidocs.ParameterLocation.PATH, + description="The course for which to get provider list", + ), + apidocs.string_parameter( + 'provider_id', + apidocs.ParameterLocation.QUERY, + description="The provider_id to fetch data for" + ) + ], + responses={ + 200: DiscussionsConfigurationSerializer, + 400: "Invalid provider ID", + 401: "The requester is not authenticated.", + 403: "The requester cannot access the specified course.", + 404: "The requested course does not exist.", + }, + ) + def get(self, request: Request, course_key_string: str, **_kwargs) -> Response: """ Handle HTTP/GET requests """ + data = self.get_configuration_data(request, course_key_string) + return Response(data) + + @staticmethod + def get_configuration_data(request: Request, course_key_string: str) -> Dict: + """ + Get discussions configuration data for the course + Args: + request (Request): a DRF request + course_key_string (str): a course key string + + Returns: + Dict: Discussion configuration data for the course + """ course_key = validate_course_key(course_key_string) configuration = DiscussionsConfiguration.get(course_key) + provider_type = request.query_params.get('provider_id', None) + if provider_type and provider_type not in AVAILABLE_PROVIDER_MAP: + raise ValidationError("Unsupported provider type") serializer = DiscussionsConfigurationSerializer( configuration, context={ 'user_id': request.user.id, + 'provider_type': provider_type, } ) - return Response(serializer.data) + return serializer.data def post(self, request, course_key_string: str, **_kwargs) -> Response: """ Handle HTTP/POST requests """ + data = self.update_configuration_data(request, course_key_string) + return Response(data) + + @staticmethod + def update_configuration_data(request, course_key_string): + """ + Update discussion configuration for the course based on data in the request. + Args: + request (Request): a DRF request + course_key_string (str): a course key string + + Returns: + Dict: modified course configuration data + """ course_key = validate_course_key(course_key_string) configuration = DiscussionsConfiguration.get(course_key) course = CourseOverview.get_from_id(course_key) @@ -61,4 +122,121 @@ class DiscussionsConfigurationView(APIView): check_course_permissions(course, request.user, 'change_provider') serializer.save() - return Response(serializer.data) + return serializer.data + + +class DiscussionsProvidersView(APIView): + """ + Read only view that lists details of discussion providers available for a course. + """ + authentication_classes = ( + JwtAuthentication, + BearerAuthenticationAllowInactiveUser, + SessionAuthenticationAllowInactiveUser + ) + permission_classes = (IsStaffOrCourseTeam,) + + @apidocs.schema( + parameters=[ + apidocs.string_parameter( + 'course_id', + apidocs.ParameterLocation.PATH, + description="The course for which to get provider list", + ) + ], + responses={ + 200: DiscussionsProvidersSerializer, + 401: "The requester is not authenticated.", + 403: "The requester cannot access the specified course.", + 404: "The requested course does not exist.", + }, + ) + def get(self, request, course_key_string: str, **_kwargs) -> Response: + """ + Handle HTTP/GET requests + """ + data = self.get_provider_data(course_key_string) + return Response(data) + + @staticmethod + def get_provider_data(course_key_string: str) -> Dict: + """ + Get provider data for specified course + Args: + course_key_string (str): course key string + + Returns: + Dict: course discussion providers + """ + course_key = validate_course_key(course_key_string) + configuration = DiscussionsConfiguration.get(course_key) + hidden_providers = [] + # If the user is currently using the legacy provider, don't show the new provider + # TODO: Allow switching between legacy and new providers + if configuration.provider_type == Provider.LEGACY: + hidden_providers.append(Provider.OPEN_EDX) + # If the user is currently using the new provider, don't show the legacy provider + elif configuration.provider_type == Provider.OPEN_EDX: + hidden_providers.append(Provider.LEGACY) + else: + # If this is a new course, or some other provider is selected, the new provider + # should only show up if the MFE is enabled + if not ENABLE_DISCUSSIONS_MFE.is_enabled(course_key): + hidden_providers.append(Provider.OPEN_EDX) + serializer = DiscussionsProvidersSerializer( + { + 'features': [ + {'id': feature.value, 'feature_support_type': feature.feature_support_type} + for feature in Features + ], + 'active': configuration.provider_type, + 'available': { + key: value + for key, value in AVAILABLE_PROVIDER_MAP.items() + if key not in hidden_providers + }, + } + ) + return serializer.data + + +class CombinedDiscussionsConfigurationView(DiscussionsConfigurationSettingsView): + """ + Combined view that includes both provider data and discussion configuration. + + Note: + This is temporary code for backwards-compatibility and will be removed soon + after the frontend supports the new split APIs. + """ + + def get(self, request: Request, course_key_string: str, **_kwargs) -> Response: + """ + Handle HTTP/GET requests + """ + config_data = self.get_configuration_data(request, course_key_string) + provider_data = DiscussionsProvidersView.get_provider_data(course_key_string) + return Response({ + **config_data, + "features": provider_data["features"], + "providers": { + "active": provider_data["active"], + "available": provider_data["available"], + }, + }) + + def post(self, request, course_key_string: str, **_kwargs) -> Response: + """ + Handle HTTP/POST requests + """ + config_data = self.update_configuration_data(request, course_key_string) + provider_data = DiscussionsProvidersView.get_provider_data(course_key_string) + return Response( + { + **config_data, + "features": provider_data["features"], + "providers": { + "active": provider_data["active"], + "available": provider_data["available"], + }, + } + )