From fb2f8e629127d49cd8787d48f51aa57ae64fdf80 Mon Sep 17 00:00:00 2001 From: stvn Date: Sat, 17 Apr 2021 00:46:39 -0700 Subject: [PATCH 1/4] refactor: Remove superfluous dict comprehension --- openedx/core/djangoapps/discussions/views.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/openedx/core/djangoapps/discussions/views.py b/openedx/core/djangoapps/discussions/views.py index d32f08feb8..6711b086fb 100644 --- a/openedx/core/djangoapps/discussions/views.py +++ b/openedx/core/djangoapps/discussions/views.py @@ -144,12 +144,7 @@ class DiscussionsConfigurationView(APIView): 'plugin_configuration': instance.plugin_configuration, 'providers': { 'active': instance.provider_type or DEFAULT_PROVIDER_TYPE, - 'available': { - provider: { - 'features': PROVIDER_FEATURE_MAP.get(provider) or [], - } - for provider in instance.available_providers - }, + 'available': PROVIDER_FEATURE_MAP, }, }) return payload From c1742b7b663af6f8feddc4cf99ad3640ea241615 Mon Sep 17 00:00:00 2001 From: stvn Date: Sat, 17 Apr 2021 00:41:32 -0700 Subject: [PATCH 2/4] feat: Add helper for discussions configuration to indicate feature support --- openedx/core/djangoapps/discussions/models.py | 19 +++++++++++++++++++ openedx/core/djangoapps/discussions/views.py | 14 +------------- 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/openedx/core/djangoapps/discussions/models.py b/openedx/core/djangoapps/discussions/models.py index 770246e637..21df38995c 100644 --- a/openedx/core/djangoapps/discussions/models.py +++ b/openedx/core/djangoapps/discussions/models.py @@ -23,6 +23,17 @@ log = logging.getLogger(__name__) DEFAULT_PROVIDER_TYPE = 'legacy' +PROVIDER_FEATURE_MAP = { + 'legacy': [ + 'discussion-page', + 'embedded-course-sections', + 'wcag-2.1', + ], + 'piazza': [ + 'discussion-page', + 'lti', + ], +} def get_supported_providers() -> list[str]: @@ -179,6 +190,14 @@ class DiscussionsConfiguration(TimeStampedModel): enabled=self.enabled, ) + def supports(self, feature: str) -> bool: + """ + Check if the provider supports some feature + """ + features = PROVIDER_FEATURE_MAP.get(self.provider_type) or [] + has_support = bool(feature in features) + return has_support + @classmethod def is_enabled(cls, context_key: CourseKey) -> bool: """ diff --git a/openedx/core/djangoapps/discussions/views.py b/openedx/core/djangoapps/discussions/views.py index 6711b086fb..1a46104923 100644 --- a/openedx/core/djangoapps/discussions/views.py +++ b/openedx/core/djangoapps/discussions/views.py @@ -16,19 +16,7 @@ from openedx.core.lib.api.authentication import BearerAuthenticationAllowInactiv from .models import DEFAULT_PROVIDER_TYPE from .models import DiscussionsConfiguration - - -PROVIDER_FEATURE_MAP = { - 'legacy': [ - 'discussion-page', - 'embedded-course-sections', - 'wcag-2.1', - ], - 'piazza': [ - 'discussion-page', - 'lti', - ], -} +from .models import PROVIDER_FEATURE_MAP class IsStaff(BasePermission): From 33e499ea049fb8b1c7a4f3bf130e0fcdcf0d2170 Mon Sep 17 00:00:00 2001 From: stvn Date: Thu, 15 Apr 2021 18:40:52 -0700 Subject: [PATCH 3/4] fix: Update discussions lti_configuration during provider_type change Else, we risk updating it when we don't intend to, eg: - changing from an LTI-backed provider (Piazza) to a non-LTI-backed provider (legacy) - the settings are no longer relevant - changing from a non-LTI-backed provider (legacy) to an LTI-backed provider (Piazza) - the settings _are_ now relevant --- openedx/core/djangoapps/discussions/views.py | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/openedx/core/djangoapps/discussions/views.py b/openedx/core/djangoapps/discussions/views.py index 1a46104923..59fa001d6c 100644 --- a/openedx/core/djangoapps/discussions/views.py +++ b/openedx/core/djangoapps/discussions/views.py @@ -150,6 +150,8 @@ class DiscussionsConfigurationView(APIView): value = validated_data.get(key) if value is not None: setattr(instance, key, value) + # _update_* helpers assume `enabled` and `provider_type` + # have already been set instance = self._update_lti(instance, validated_data) instance.save() return instance @@ -159,15 +161,19 @@ class DiscussionsConfigurationView(APIView): Update LtiConfiguration """ lti_configuration_data = validated_data.get('lti_configuration') - if lti_configuration_data: - instance.lti_configuration = instance.lti_configuration or LtiConfiguration() - lti_configuration = LtiSerializer( - instance.lti_configuration, + supports_lti = instance.supports('lti') + if not supports_lti: + instance.lti_configuration = None + elif lti_configuration_data: + lti_configuration = instance.lti_configuration or LtiConfiguration() + lti_serializer = LtiSerializer( + lti_configuration, data=lti_configuration_data, partial=True, ) - if lti_configuration.is_valid(raise_exception=True): - lti_configuration.save() + if lti_serializer.is_valid(raise_exception=True): + lti_serializer.save() + instance.lti_configuration = lti_configuration return instance # pylint: disable=redefined-builtin From a99c95da6483496cbaa096b79b768cc8b82105f1 Mon Sep 17 00:00:00 2001 From: stvn Date: Sat, 17 Apr 2021 00:44:49 -0700 Subject: [PATCH 4/4] feat: Hide lti_configuration from non-LTI discussions providers --- openedx/core/djangoapps/discussions/views.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/discussions/views.py b/openedx/core/djangoapps/discussions/views.py index 59fa001d6c..9da9ee4228 100644 --- a/openedx/core/djangoapps/discussions/views.py +++ b/openedx/core/djangoapps/discussions/views.py @@ -120,7 +120,11 @@ class DiscussionsConfigurationView(APIView): Serialize data into a dictionary, to be used as a response """ payload = super().to_representation(instance) - lti_configuration = LtiSerializer(instance.lti_configuration) + lti_configuration_data = {} + supports_lti = instance.supports('lti') + if supports_lti: + lti_configuration = LtiSerializer(instance.lti_configuration) + lti_configuration_data = lti_configuration.data payload.update({ 'features': { 'discussion-page', @@ -128,7 +132,7 @@ class DiscussionsConfigurationView(APIView): 'lti', 'wcag-2.1', }, - 'lti_configuration': lti_configuration.data, + 'lti_configuration': lti_configuration_data, 'plugin_configuration': instance.plugin_configuration, 'providers': { 'active': instance.provider_type or DEFAULT_PROVIDER_TYPE,