Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: edx provider data in the api (#29331)

Browse Source 
* fix: edx provider data in the api

* refactor: remove comments in the code

* test: update the test cases

* fix: quality changes
This commit is contained in:
Awais Jibran
2021-11-19 18:58:28 +05:00
committed by GitHub
parent 66654b2edb
commit 5c2f8b23d4
4 changed files with 234 additions and 174 deletions
Download Patch File Download Diff File Expand all files Collapse all files

31
openedx/core/djangoapps/discussions/models.py
View File

@@ -27,15 +27,28 @@ from openedx.core.djangoapps.site_configuration import helpers as configuration_
log = logging.getLogger(__name__)
DEFAULT_PROVIDER_TYPE = 'legacy'
DEFAULT_CONFIG_ENABLED = True
ProviderExternalLinks = namedtuple(
'ProviderExternalLinks',
['learn_more', 'configuration', 'general', 'accessibility', 'contact_email']
)
class Provider:
"""
List of Discussion providers.
"""
LEGACY = 'legacy'
ED_DISCUSS = 'ed-discuss'
INSCRIBE = 'inscribe'
PIAZZA = 'piazza'
YELLOWDIG = 'yellowdig'
OPEN_EDX = 'openedx'
DEFAULT_PROVIDER_TYPE = Provider.LEGACY
DEFAULT_CONFIG_ENABLED = True
class Features(Enum):
"""
Features to be used/mapped in discussion providers
@@ -104,7 +117,7 @@ def pii_sharing_required_message(provider_name):
AVAILABLE_PROVIDER_MAP = {
'legacy': {
Provider.LEGACY: {
'features': [
Features.BASIC_CONFIGURATION.value,
Features.PRIMARY_DISCUSSION_APP_EXPERIENCE.value,
@@ -130,7 +143,7 @@ AVAILABLE_PROVIDER_MAP = {
'messages': [],
'has_full_support': True
},
'openedx': {
Provider.OPEN_EDX: {
'features': [
Features.BASIC_CONFIGURATION.value,
Features.PRIMARY_DISCUSSION_APP_EXPERIENCE.value,
@@ -158,7 +171,7 @@ AVAILABLE_PROVIDER_MAP = {
'supports_in_context_discussions': True,
'visible': False,
},
'ed-discuss': {
Provider.ED_DISCUSS: {
'features': [
Features.PRIMARY_DISCUSSION_APP_EXPERIENCE.value,
Features.BASIC_CONFIGURATION.value,
@@ -185,7 +198,7 @@ AVAILABLE_PROVIDER_MAP = {
'messages': [pii_sharing_required_message('Ed Discussion')],
'has_full_support': False
},
'inscribe': {
Provider.INSCRIBE: {
'features': [
Features.PRIMARY_DISCUSSION_APP_EXPERIENCE.value,
Features.BASIC_CONFIGURATION.value,
@@ -213,7 +226,7 @@ AVAILABLE_PROVIDER_MAP = {
'messages': [pii_sharing_required_message('InScribe')],
'has_full_support': False
},
'piazza': {
Provider.PIAZZA: {
'features': [
Features.PRIMARY_DISCUSSION_APP_EXPERIENCE.value,
Features.BASIC_CONFIGURATION.value,
@@ -237,7 +250,7 @@ AVAILABLE_PROVIDER_MAP = {
'messages': [],
'has_full_support': False
},
'yellowdig': {
Provider.YELLOWDIG: {
'features': [
Features.PRIMARY_DISCUSSION_APP_EXPERIENCE.value,
Features.BASIC_CONFIGURATION.value,

40
openedx/core/djangoapps/discussions/serializers.py
View File

@@ -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
from .models import AVAILABLE_PROVIDER_MAP, DEFAULT_PROVIDER_TYPE, DiscussionsConfiguration, Features
from .models import AVAILABLE_PROVIDER_MAP, DEFAULT_PROVIDER_TYPE, DiscussionsConfiguration, Features, Provider
from .utils import available_division_schemes, get_divided_discussions
@@ -17,6 +17,7 @@ class LtiSerializer(serializers.ModelSerializer):
"""
Serialize LtiConfiguration responses
"""
class Meta:
model = LtiConfiguration
fields = [
@@ -70,6 +71,7 @@ class LegacySettingsSerializer(serializers.BaseSerializer):
"""
Serialize legacy discussions settings
"""
class Meta:
fields = [
'allow_anonymous',
@@ -212,22 +214,22 @@ class DiscussionsConfigurationSerializer(serializers.ModelSerializer):
"""
course_key = instance.context_key
payload = super().to_representation(instance)
lti_configuration_data = {}
if instance.supports_lti():
lti_configuration = LtiSerializer(instance.lti_configuration, context={
'pii_sharing_allowed': get_lti_pii_sharing_state_for_course(course_key),
})
lti_configuration_data = lti_configuration.data
provider_type = instance.provider_type or DEFAULT_PROVIDER_TYPE
lti_configuration = LtiSerializer(
instance.lti_configuration,
context={'pii_sharing_allowed': get_lti_pii_sharing_state_for_course(course_key)}
)
lti_configuration_data = lti_configuration.data
provider_type = instance.provider_type
plugin_configuration = instance.plugin_configuration
if provider_type == 'legacy':
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
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
@@ -302,10 +304,8 @@ class DiscussionsConfigurationSerializer(serializers.ModelSerializer):
"""
plugin_configuration = validated_data.pop('plugin_configuration', {})
updated_provider_type = validated_data.get('provider_type') or instance.provider_type
will_support_legacy = bool(
updated_provider_type == 'legacy'
)
if will_support_legacy:
if updated_provider_type == Provider.LEGACY:
legacy_settings = LegacySettingsSerializer(
self._get_course(),
context={

6
openedx/core/djangoapps/discussions/tests/test_models.py
View File

@@ -9,7 +9,7 @@ from django.test import TestCase
from opaque_keys.edx.keys import CourseKey
from organizations.models import Organization
from ..models import DEFAULT_CONFIG_ENABLED, DEFAULT_PROVIDER_TYPE
from ..models import DEFAULT_CONFIG_ENABLED, DEFAULT_PROVIDER_TYPE, Provider
from ..models import DiscussionsConfiguration
from ..models import ProviderFilter
@@ -137,7 +137,7 @@ class DiscussionsConfigurationModelTest(TestCase):
self.configuration_with_values = DiscussionsConfiguration(
context_key=self.course_key_with_values,
enabled=False,
provider_type='legacy',
provider_type=Provider.LEGACY,
plugin_configuration={
'url': 'http://localhost',
},
@@ -162,7 +162,7 @@ class DiscussionsConfigurationModelTest(TestCase):
assert configuration.enabled # by default
assert configuration.lti_configuration is None
assert len(configuration.plugin_configuration.keys()) == 0
assert configuration.provider_type == 'legacy'
assert configuration.provider_type == DEFAULT_PROVIDER_TYPE
def test_get_with_values(self):
"""

331
openedx/core/djangoapps/discussions/tests/test_views.py
View File

@@ -12,14 +12,13 @@ from django.urls import reverse
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 ..models import AVAILABLE_PROVIDER_MAP, DEFAULT_CONFIG_ENABLED, DEFAULT_PROVIDER_TYPE
from xmodule.modulestore import ModuleStoreEnum
from xmodule.modulestore.tests.django_utils import CourseUserType, ModuleStoreTestCase
from xmodule.modulestore.tests.factories import CourseFactory
from ..models import AVAILABLE_PROVIDER_MAP, DEFAULT_CONFIG_ENABLED, DEFAULT_PROVIDER_TYPE, Provider
DATA_LEGACY_COHORTS = {
'divided_inline_discussions': [],
@@ -37,6 +36,24 @@ DATA_LEGACY_CONFIGURATION = {
},
},
}
DEFAULT_LEGACY_CONFIGURATION = {
**DATA_LEGACY_CONFIGURATION,
'allow_anonymous_to_peers': False,
'always_divide_inline_discussions': False,
'divided_inline_discussions': [],
'divided_course_wide_discussions': [],
'division_scheme': 'none',
'available_division_schemes': [],
}
DEFAULT_LTI_CONFIGURATION = {
'lti_1p1_client_key': '',
'lti_1p1_client_secret': '',
'lti_1p1_launch_url': '',
'version': None
}
DATA_LTI_CONFIGURATION = {
'lti_1p1_client_key': 'KEY',
'lti_1p1_client_secret': 'SECRET',
@@ -206,6 +223,18 @@ class DataTest(AuthorizedApiTest):
Check API-data correctness
"""
def get(self):
"""Makes a get request and returns json data"""
response = super()._get()
return response.json()
def get_and_assert_defaults(self):
"""
Assert that course has default discussion configuration
"""
response = self._get()
self._assert_defaults(response)
def _assert_defaults(self, response):
"""
Check for default values
@@ -219,127 +248,152 @@ class DataTest(AuthorizedApiTest):
name for name, spec in data['providers']['available'].items()
if "messages" not in spec
], "Found available providers without messages field"
assert data['lti_configuration'] == {}
assert data['plugin_configuration'] == {
'allow_anonymous': True,
'allow_anonymous_to_peers': False,
'always_divide_inline_discussions': False,
'available_division_schemes': [],
'discussion_blackouts': [],
'discussion_topics': {'General': {'id': 'course'}},
'divided_course_wide_discussions': [],
'divided_inline_discussions': [],
'division_scheme': 'none',
}
assert len(data['plugin_configuration']) > 0
def _setup_lti(self):
assert data['lti_configuration'] == DEFAULT_LTI_CONFIGURATION
assert data['plugin_configuration'] == DEFAULT_LEGACY_CONFIGURATION
def _configure_lti_discussion_provider(self, provider=Provider.PIAZZA):
"""
Configure an LTI-based provider
Configure an LTI-based discussion provider for a course.
"""
payload = {
'enabled': True,
'provider_type': 'piazza',
'provider_type': provider,
'lti_configuration': DATA_LTI_CONFIGURATION,
'plugin_configuration': {
}
'plugin_configuration': {}
}
response = self._post(payload)
data = response.json()
assert response.status_code == self.expected_response_code
return data
def test_get_nonexistent_with_defaults(self):
"""
If no record exists, defaults should be returned.
"""
response = self._get()
self._assert_defaults(response)
@contextmanager
def _pii_sharing_for_course(self, enabled):
instance = CourseAllowPIISharingInLTIFlag.objects.create(course_id=self.course.id, enabled=enabled)
yield
instance.delete()
@ddt.data(
{"pii_share_username": True},
{"pii_share_email": True},
{"pii_share_email": True, "pii_share_username": True},
)
def test_post_pii_fields_fail(self, pii_fields):
def test_get_non_configured_provider_for_course(self):
"""
If no record exists, defaults should be returned.
Tests that if no provider is configured for a course, default configuration
is returned.
"""
data = self._setup_lti()
data['lti_configuration'].update(pii_fields)
response = self._post(data)
assert response.status_code == 400
self.get_and_assert_defaults()
@ddt.data(
{"pii_share_username": True},
{"pii_share_email": True},
{"pii_share_email": True, "pii_share_username": True},
)
def test_post_pii_fields(self, pii_fields):
def test_post_pii_fields_with_non_configured_pii(self, pii_fields):
"""
Only if PII sharing is enabled should a user be able to set pii fields.
Tests that if PII sharing is not set, user is not able to update
PII settings for a course.
"""
data = self._setup_lti()
data = self._configure_lti_discussion_provider()
data['lti_configuration'].update(pii_fields)
response = self._post(data)
assert response.status_code == status.HTTP_400_BAD_REQUEST
@ddt.data(
{"pii_share_username": True},
{"pii_share_email": True},
{"pii_share_email": True, "pii_share_username": True},
)
def test_post_pii_fields_with_pii_disabled(self, pii_fields):
"""
Test that when PII sharing is disabled for the course, user is not able
update PII settings for a course.
"""
data = self._configure_lti_discussion_provider()
data['lti_configuration'].update(pii_fields)
with self._pii_sharing_for_course(enabled=False):
response = self._post(data)
assert response.status_code == 400
with self._pii_sharing_for_course(enabled=True):
response = self._post(data)
assert response.status_code == 200
assert response.status_code == status.HTTP_400_BAD_REQUEST
lti_configuration = self.get()['lti_configuration']
no_pii_fields_updated = [
lti_configuration.get(pii_field) != pii_value for pii_field, pii_value in pii_fields.items()
]
assert all(no_pii_fields_updated)
@ddt.data(
True, False
{"pii_share_username": True},
{"pii_share_email": True},
{"pii_share_email": True, "pii_share_username": True},
)
def test_post_pii_fields_with_pii_enabled(self, pii_fields):
"""
Test that when PII sharing is enabled for the course, user is able
update PII settings for the course.
"""
data = self._configure_lti_discussion_provider()
data['lti_configuration'].update(pii_fields)
with self._pii_sharing_for_course(enabled=True):
response = self._post(data)
assert response.status_code == status.HTTP_200_OK
lti_configuration = self.get()['lti_configuration']
all_pii_fields_updated = [
lti_configuration[pii_field] == pii_value for pii_field, pii_value in pii_fields.items()
]
assert all(all_pii_fields_updated)
@ddt.data(
True,
False
)
def test_get_pii_fields(self, pii_sharing):
"""
Only if PII is enabled should pii fields be returned.
Tests that when PII sharing is enabled, API included PII info.
"""
self._setup_lti()
self._configure_lti_discussion_provider()
with self._pii_sharing_for_course(enabled=pii_sharing):
response = self._get()
data = response.json()
data = self.get()
# If pii_sharing is true, then the fields should be present, and absent otherwise
assert ("pii_share_email" in data["lti_configuration"]) == pii_sharing
assert ("pii_share_username" in data["lti_configuration"]) == pii_sharing
assert ('pii_share_email' in data['lti_configuration']) == pii_sharing
assert ('pii_share_username' in data['lti_configuration']) == pii_sharing
def test_post_everything(self):
@ddt.data(
Provider.ED_DISCUSS,
Provider.INSCRIBE,
Provider.PIAZZA,
Provider.YELLOWDIG,
)
def test_post_everything(self, provider):
"""
API should accept requests to update _all_ fields at once
"""
data = self._setup_lti()
data = self._configure_lti_discussion_provider(provider=provider)
assert data['enabled']
assert data['provider_type'] == 'piazza'
assert data['providers']['available']['piazza'] == AVAILABLE_PROVIDER_MAP['piazza']
assert data['provider_type'] == provider
assert data['providers']['available'][provider] == AVAILABLE_PROVIDER_MAP[provider]
assert data['plugin_configuration'] == DEFAULT_LEGACY_CONFIGURATION
assert data['lti_configuration'] == DATA_LTI_CONFIGURATION
assert len(data['plugin_configuration']) == 0
assert len(data['lti_configuration']) > 0
response = self._get()
response_data = self.get()
# the GET should pull back the same data as the POST
response_data = response.json()
assert response_data == data
def test_post_invalid_key(self):
"""
Unsupported keys should be gracefully ignored
Tests that unsupported keys should be gracefully ignored.
"""
payload = {
'non-existent-key': 'value',
}
response = self._post(payload)
assert response.status_code == self.expected_response_code
assert response.status_code == status.HTTP_200_OK
def test_configuration_valid(self):
@ddt.data(
Provider.ED_DISCUSS,
Provider.INSCRIBE,
Provider.PIAZZA,
Provider.YELLOWDIG,
)
def test_add_valid_configuration(self, provider_type):
"""
Check we can set basic configuration
Test that basic configuration is set & retrieved successfully.
"""
provider_type = 'piazza'
payload = {
'enabled': True,
'provider_type': provider_type,
@@ -348,26 +402,46 @@ class DataTest(AuthorizedApiTest):
},
}
self._post(payload)
response = self._get()
data = response.json()
data = self.get()
assert data['enabled']
assert data['provider_type'] == provider_type
assert data['plugin_configuration'] == payload['plugin_configuration']
assert data['plugin_configuration'] == DEFAULT_LEGACY_CONFIGURATION
def test_change_plugin_configuration(self):
"""
Tests custom config values persist that when changing discussion
provider from edx provider to other provider.
"""
payload = {
'provider_type': Provider.PIAZZA,
'plugin_configuration': {
'allow_anonymous': False,
'custom_field': 'custom_value',
},
}
response = self._post(payload)
data = response.json()
assert data['plugin_configuration'] == DEFAULT_LEGACY_CONFIGURATION
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(
{
'enabled': 3,
},
{'enabled': 3},
)
def test_configuration_invalid(self, payload):
"""
Check validation of basic configuration
Test that invalid data raises validation error.
"""
response = self._post(payload)
assert status.is_client_error(response.status_code)
assert 'enabled' in response.json()
response = self._get()
self._assert_defaults(response)
errors = response.json()
assert 'enabled' in errors
self.get_and_assert_defaults()
@ddt.data(
*DATA_LTI_CONFIGURATION.items()
@@ -375,7 +449,7 @@ class DataTest(AuthorizedApiTest):
@ddt.unpack
def test_post_lti_valid(self, key, value):
"""
Check we can set LTI configuration
Test that we can set & retrieve LTI configuration.
"""
provider_type = 'piazza'
payload = {
@@ -386,15 +460,14 @@ class DataTest(AuthorizedApiTest):
}
}
self._post(payload)
response = self._get()
data = response.json()
data = self.get()
assert data['enabled']
assert data['provider_type'] == provider_type
assert data['lti_configuration'][key] == value
def test_post_lti_invalid(self):
"""
Check validation of LTI configuration ignores unsupported values
Check validation of LTI configuration ignores unsupported values.
The fields are all open-ended strings and will accept any values.
"""
@@ -409,9 +482,8 @@ class DataTest(AuthorizedApiTest):
}
}
response = self._post(payload)
assert response
response = self._get()
data = response.json()
assert response.status_code == status.HTTP_200_OK
data = self.get()
assert data['enabled']
assert data['provider_type'] == provider_type
assert data['lti_configuration'][key] == value
@@ -419,7 +491,7 @@ class DataTest(AuthorizedApiTest):
def test_post_legacy_valid(self):
"""
Check we can set legacy settings configuration
Test that we can set & retrieve edx provider configuration.
"""
provider_type = 'legacy'
for key, value in DATA_LEGACY_CONFIGURATION.items():
@@ -432,25 +504,16 @@ class DataTest(AuthorizedApiTest):
}
response = self._post(payload)
assert response
response = self._get()
data = response.json()
data = self.get()
assert data['enabled']
assert data['provider_type'] == provider_type
assert data['plugin_configuration'][key] == value
@ddt.data(
{
'allow_anonymous': 3,
},
{
'allow_anonymous_to_peers': 3,
},
{
'discussion_blackouts': 3,
},
{
'discussion_topics': 3,
},
{'allow_anonymous': 3},
{'allow_anonymous_to_peers': 3},
{'discussion_blackouts': 3},
{'discussion_topics': 3},
)
def test_post_legacy_invalid(self, plugin_configuration):
"""
@@ -466,15 +529,14 @@ class DataTest(AuthorizedApiTest):
response = self._post(payload)
if status.is_client_error(response.status_code):
raise ValidationError(str(response.status_code))
response = self._get()
self._assert_defaults(response)
self.get_and_assert_defaults()
@ddt.data(*DATA_LEGACY_COHORTS.items())
def test_post_cohorts_valid(self, kvp):
@ddt.unpack
def test_post_cohorts_valid(self, key, value):
"""
Check we can set legacy cohorts configuration
Test that we can set & retrieve legacy cohorts configuration.
"""
key, value = kvp
provider_type = 'legacy'
payload = {
'enabled': True,
@@ -483,19 +545,18 @@ class DataTest(AuthorizedApiTest):
key: value,
}
}
response = self._post(payload)
response = self._get()
data = response.json()
self._post(payload)
data = self.get()
assert data['enabled']
assert data['provider_type'] == provider_type
assert data['plugin_configuration'][key] == value
@ddt.data(*DATA_LEGACY_COHORTS.items())
def test_post_cohorts_invalid(self, kvp):
@ddt.unpack
def test_post_cohorts_invalid(self, key, value):
"""
Check validation of legacy cohorts configuration
"""
key, value = kvp
if isinstance(value, str):
# For the string value, we can only fail here if it's blank
value = ''
@@ -514,12 +575,13 @@ class DataTest(AuthorizedApiTest):
response = self._post(payload)
if status.is_client_error(response.status_code):
raise ValidationError(str(response.status_code))
response = self._get()
self._assert_defaults(response)
self.get_and_assert_defaults()
def test_change_to_lti(self):
"""
Ensure we can switch to an LTI-backed provider (from a non-LTI one)
Test that switching to an LTI-backed provider from a default provider works as expected.
When switching provider to LTI, the API should return both LTI & legacy data.
"""
payload = {
'enabled': True,
@@ -528,19 +590,25 @@ class DataTest(AuthorizedApiTest):
'allow_anonymous': False,
},
}
response = self._post(payload)
data = response.json()
data = self._setup_lti()
self._post(payload)
self._configure_lti_discussion_provider(provider=Provider.ED_DISCUSS)
data = self.get()
assert data['enabled']
assert data['provider_type'] == 'piazza'
assert not data['plugin_configuration']
assert data['lti_configuration']
assert data['provider_type'] == Provider.ED_DISCUSS
assert data['plugin_configuration'] == {
**DEFAULT_LEGACY_CONFIGURATION,
'allow_anonymous': False,
}
assert data['lti_configuration'] == DATA_LTI_CONFIGURATION
def test_change_from_lti(self):
"""
Ensure we can switch away from an LTI-backed provider (to a non-LTI one)
Test that switching from an LTI-backed provider to a non-LTI provider works as expected.
When switching provider to LTI, the API should return both LTI & legacy data.
"""
data = self._setup_lti()
self._configure_lti_discussion_provider()
payload = {
'enabled': True,
'provider_type': 'legacy',
@@ -575,27 +643,6 @@ class DataTest(AuthorizedApiTest):
course = self.store.get_course(self.course.id)
assert course.discussions_settings[field] == value
def test_change_plugin_configuration(self):
"""
Test changing plugin config that is saved to the course
"""
payload = {
"provider_type": "piazza",
"plugin_configuration": {
"allow_anonymous": False,
"custom_field": "custom_value",
},
}
response = self._post(payload)
data = response.json()
assert data["plugin_configuration"] == payload["plugin_configuration"]
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(*[
user_type.name for user_type in CourseUserType
if user_type not in { # pylint: disable=undefined-variable