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

fix: add pii sharing allowed flag (#29628)

Browse Source
This commit is contained in:
Awais Jibran
2021-12-21 15:07:49 +05:00
committed by GitHub
parent 7fba083a49
commit 371a3ef1c6
2 changed files with 231 additions and 176 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -43,16 +43,6 @@ class LtiSerializer(serializers.ModelSerializer):
}
return payload
def to_representation(self, instance):
"""
Serialize object into a primitive data types.
"""
representation = super().to_representation(instance)
if not self.context.get('pii_sharing_allowed'):
representation.pop('pii_share_username')
representation.pop('pii_share_email')
return representation
def update(self, instance: LtiConfiguration, validated_data: dict) -> LtiConfiguration:
"""
Create/update a model-backed instance
@@ -219,12 +209,13 @@ class DiscussionsConfigurationSerializer(serializers.ModelSerializer):
"""
course_key = instance.context_key
payload = super().to_representation(instance)
course_pii_sharing_allowed = get_lti_pii_sharing_state_for_course(course_key)
lti_configuration = LtiSerializer(
instance.lti_configuration,
context={'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

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

@@ -46,22 +46,35 @@ DEFAULT_LEGACY_CONFIGURATION = {
'available_division_schemes': [],
}
DEFAULT_LTI_CONFIGURATION = {
'lti_1p1_client_key': '',
'lti_1p1_client_secret': '',
'lti_1p1_launch_url': '',
'version': None,
'pii_sharing_allowed': False,
'pii_share_email': False,
'pii_share_username': False,
}
DATA_LTI_CONFIGURATION = {
DATA_POST_LTI_CONFIGURATION = {
'lti_1p1_client_key': 'KEY',
'lti_1p1_client_secret': 'SECRET',
'lti_1p1_launch_url': 'https://localhost',
'version': 'lti_1p1'
}
DATA_LTI_CONFIGURATION_DISABLED_PII = {
**DATA_POST_LTI_CONFIGURATION,
'pii_sharing_allowed': False,
'pii_share_email': False,
'pii_share_username': False,
}
DATA_LTI_CONFIGURATION_ENABLED_PII = {
**DATA_POST_LTI_CONFIGURATION,
'pii_sharing_allowed': True,
'pii_share_email': False,
'pii_share_username': False,
}
class ApiTest(ModuleStoreTestCase, APITestCase):
@@ -82,10 +95,7 @@ class ApiTest(ModuleStoreTestCase, APITestCase):
def url(self):
"""Returns the discussion API url. """
return reverse(
'discussions',
kwargs={
'course_key_string': str(self.course.id),
}
'discussions', kwargs={'course_key_string': str(self.course.id)}
)
def _get(self):
@@ -226,8 +236,10 @@ class DataTest(AuthorizedApiTest):
"""
def get(self):
"""Makes a get request and returns json data"""
response = super()._get()
"""
Makes a get request and returns json data.
"""
response = self._get()
return response.json()
def get_and_assert_defaults(self):
@@ -261,7 +273,7 @@ class DataTest(AuthorizedApiTest):
payload = {
'enabled': True,
'provider_type': provider,
'lti_configuration': DATA_LTI_CONFIGURATION,
'lti_configuration': DATA_POST_LTI_CONFIGURATION,
'plugin_configuration': {}
}
response = self._post(payload)
@@ -269,11 +281,19 @@ class DataTest(AuthorizedApiTest):
assert response.status_code == self.expected_response_code
return data
@contextmanager
def _pii_sharing_for_course(self, enabled):
instance = CourseAllowPIISharingInLTIFlag.objects.create(course_id=self.course.id, enabled=enabled)
yield
instance.delete()
def _configure_legacy_discussion_provider(self, configuration):
"""
Configure legacy discussion provider for a course.
"""
payload = {
'enabled': True,
'provider_type': Provider.LEGACY,
'plugin_configuration': configuration,
}
response = self._post(payload)
assert response
assert response.status_code == self.expected_response_code
return response.json()
def test_get_non_configured_provider_for_course(self):
"""
@@ -282,100 +302,6 @@ class DataTest(AuthorizedApiTest):
"""
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_with_non_configured_pii(self, pii_fields):
"""
Tests that if PII sharing is not set, user is not able to update
PII settings for a course.
"""
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 == 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(
{"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):
"""
Tests that when PII sharing is enabled, API included PII info.
"""
self._configure_lti_discussion_provider()
with self._pii_sharing_for_course(enabled=pii_sharing):
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
@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._configure_lti_discussion_provider(provider=provider)
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['lti_configuration'] == DATA_LTI_CONFIGURATION
response_data = self.get()
# the GET should pull back the same data as the POST
assert response_data == data
def test_post_invalid_key(self):
"""
Tests that unsupported keys should be gracefully ignored.
@@ -391,6 +317,7 @@ class DataTest(AuthorizedApiTest):
Provider.INSCRIBE,
Provider.PIAZZA,
Provider.YELLOWDIG,
Provider.LEGACY
)
def test_add_valid_configuration(self, provider_type):
"""
@@ -409,6 +336,7 @@ class DataTest(AuthorizedApiTest):
assert data['enabled']
assert data['provider_type'] == provider_type
assert data['plugin_configuration'] == DEFAULT_LEGACY_CONFIGURATION
assert data['lti_configuration'] == DEFAULT_LTI_CONFIGURATION
def test_change_plugin_configuration(self):
"""
@@ -446,25 +374,25 @@ class DataTest(AuthorizedApiTest):
self.get_and_assert_defaults()
@ddt.data(
*DATA_LTI_CONFIGURATION.items()
*DATA_POST_LTI_CONFIGURATION.items()
)
@ddt.unpack
def test_post_lti_valid(self, key, value):
"""
Test that we can set & retrieve LTI configuration.
"""
provider_type = 'piazza'
payload = {
'enabled': True,
'provider_type': provider_type,
'provider_type': Provider.PIAZZA,
'lti_configuration': {
key: value,
}
}
self._post(payload)
data = self.get()
assert data['enabled']
assert data['provider_type'] == provider_type
assert data['provider_type'] == Provider.PIAZZA
assert data['lti_configuration'][key] == value
def test_post_lti_invalid(self):
@@ -473,11 +401,10 @@ class DataTest(AuthorizedApiTest):
The fields are all open-ended strings and will accept any values.
"""
provider_type = 'piazza'
for key, value in DATA_LTI_CONFIGURATION.items():
for key, value in DATA_POST_LTI_CONFIGURATION.items():
payload = {
'enabled': True,
'provider_type': provider_type,
'provider_type': Provider.PIAZZA,
'lti_configuration': {
key: value,
'ignored-key': 'ignored value',
@@ -487,7 +414,7 @@ class DataTest(AuthorizedApiTest):
assert response.status_code == status.HTTP_200_OK
data = self.get()
assert data['enabled']
assert data['provider_type'] == provider_type
assert data['provider_type'] == Provider.PIAZZA
assert data['lti_configuration'][key] == value
assert 'ignored-key' not in data['lti_configuration']
@@ -495,20 +422,12 @@ class DataTest(AuthorizedApiTest):
"""
Test that we can set & retrieve edx provider configuration.
"""
provider_type = 'legacy'
for key, value in DATA_LEGACY_CONFIGURATION.items():
payload = {
'enabled': True,
'provider_type': provider_type,
'plugin_configuration': {
key: value,
}
}
response = self._post(payload)
assert response
self._configure_legacy_discussion_provider(configuration={key: value})
data = self.get()
assert data['enabled']
assert data['provider_type'] == provider_type
assert data['provider_type'] == Provider.LEGACY
assert data['plugin_configuration'][key] == value
@ddt.data(
@@ -521,10 +440,9 @@ class DataTest(AuthorizedApiTest):
"""
Check validation of legacy settings configuration
"""
provider_type = 'legacy'
payload = {
'enabled': True,
'provider_type': provider_type,
'provider_type': Provider.LEGACY,
'plugin_configuration': plugin_configuration,
}
with self.assertRaises(ValidationError):
@@ -539,18 +457,11 @@ class DataTest(AuthorizedApiTest):
"""
Test that we can set & retrieve legacy cohorts configuration.
"""
provider_type = 'legacy'
payload = {
'enabled': True,
'provider_type': provider_type,
'plugin_configuration': {
key: value,
}
}
self._post(payload)
self._configure_legacy_discussion_provider(configuration={key: value})
data = self.get()
assert data['enabled']
assert data['provider_type'] == provider_type
assert data['provider_type'] == Provider.LEGACY
assert data['plugin_configuration'][key] == value
@ddt.data(*DATA_LEGACY_COHORTS.items())
@@ -565,10 +476,10 @@ class DataTest(AuthorizedApiTest):
else:
# Otherwise, submit a string when non-string is required
value = str(value)
provider_type = 'legacy'
payload = {
'enabled': True,
'provider_type': provider_type,
'provider_type': Provider.LEGACY,
'plugin_configuration': {
key: value,
}
@@ -585,24 +496,18 @@ class DataTest(AuthorizedApiTest):
When switching provider to LTI, the API should return both LTI & legacy data.
"""
payload = {
'enabled': True,
'provider_type': 'legacy',
'plugin_configuration': {
'allow_anonymous': False,
},
}
self._post(payload)
plugin_configuration = {'allow_anonymous': False}
self._configure_legacy_discussion_provider(configuration=plugin_configuration)
self._configure_lti_discussion_provider(provider=Provider.ED_DISCUSS)
data = self.get()
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['lti_configuration'] == DATA_LTI_CONFIGURATION
assert data['lti_configuration'] == DATA_LTI_CONFIGURATION_DISABLED_PII
def test_change_from_lti(self):
"""
@@ -611,17 +516,11 @@ class DataTest(AuthorizedApiTest):
When switching provider to LTI, the API should return both LTI & legacy data.
"""
self._configure_lti_discussion_provider()
payload = {
'enabled': True,
'provider_type': 'legacy',
'plugin_configuration': {
'allow_anonymous': False,
},
}
response = self._post(payload)
data = response.json()
plugin_configuration = {'allow_anonymous': False}
data = self._configure_legacy_discussion_provider(configuration=plugin_configuration)
assert data['enabled']
assert data['provider_type'] == 'legacy'
assert data['provider_type'] == Provider.LEGACY
assert not data['plugin_configuration']['allow_anonymous']
@ddt.data(
@@ -629,7 +528,7 @@ class DataTest(AuthorizedApiTest):
["enable_in_context", "enable_graded_units", "unit_level_visibility"],
[True, False],
),
("provider_type", "piazza"),
("provider_type", Provider.PIAZZA),
)
@ddt.unpack
def test_change_course_fields(self, field, value):
@@ -695,3 +594,168 @@ class DataTest(AuthorizedApiTest):
'provider_type': 'piazza',
})
assert response.status_code == status.HTTP_200_OK
@ddt.ddt
class PIISettingsAPITests(DataTest):
"""
Test PII sharing setting for course.
"""
@contextmanager
def _pii_sharing_for_course(self, enabled):
instance = CourseAllowPIISharingInLTIFlag.objects.create(course_id=self.course.id, enabled=enabled)
yield
instance.delete()
def _assert_pii_flag_for_course(self, enabled):
"""
Asserts pii flag has given state.
"""
try:
course_pii = CourseAllowPIISharingInLTIFlag.objects.filter(course_id=self.course.id).latest('change_date')
assert course_pii.enabled == enabled
except CourseAllowPIISharingInLTIFlag.DoesNotExist:
assert enabled is False
def test_pii_sharing_disabled(self):
"""
Tests that pii settings are not enabled by default when configuring LTI provider.
"""
self._configure_lti_discussion_provider()
self._assert_pii_flag_for_course(enabled=False)
with self._pii_sharing_for_course(enabled=False):
self._assert_pii_flag_for_course(enabled=False)
def test_pii_sharing_enabled(self):
"""
Tests that pii sharing settings can be enabled after configuring lti provider.
"""
self._configure_lti_discussion_provider()
with self._pii_sharing_for_course(enabled=True):
self._assert_pii_flag_for_course(enabled=True)
@ddt.data(
{"pii_share_username": True},
{"pii_share_email": True},
{"pii_share_email": True, "pii_share_username": True},
)
def test_post_pii_fields_with_non_configured_pii(self, pii_configuration):
"""
Tests that if PII sharing is *not configured* for the course, the api call
to update PII settings for the course fails.
"""
data = self._configure_lti_discussion_provider()
data['lti_configuration'].update(pii_configuration)
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_configuration):
"""
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_configuration)
with self._pii_sharing_for_course(enabled=False):
response = self._post(data)
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_configuration.items()
]
assert all(no_pii_fields_updated)
@ddt.data(
{"pii_share_username": True},
{"pii_share_email": True},
{"pii_share_email": True, "pii_share_username": True},
{"pii_share_email": False, "pii_share_username": True},
{"pii_share_email": True, "pii_share_username": False},
{"pii_share_email": False, "pii_share_username": False},
)
def test_post_pii_fields_with_pii_enabled(self, pii_configuration):
"""
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_configuration)
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_configuration.items()
]
assert all(all_pii_fields_updated)
@ddt.data(
True,
False
)
def test_get_pii_fields(self, pii_sharing):
"""
Tests that when PII sharing is included in the API regardless of course pii settings.
"""
self._configure_lti_discussion_provider()
with self._pii_sharing_for_course(enabled=pii_sharing):
lti_configuration = self.get()['lti_configuration']
# If pii_sharing is true, then the fields should be present, and absent otherwise
assert lti_configuration['pii_sharing_allowed'] == pii_sharing
assert 'pii_share_email' in lti_configuration
assert 'pii_share_username' in lti_configuration
@ddt.data(
Provider.ED_DISCUSS,
Provider.INSCRIBE,
Provider.PIAZZA,
Provider.YELLOWDIG,
)
def test_post_everything_with_pii_disabled(self, provider):
"""
Test posting all data returns expected response when course PII flag is disabled.
"""
data = self._configure_lti_discussion_provider(provider=provider)
self._assert_pii_flag_for_course(enabled=False)
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['lti_configuration'] == DATA_LTI_CONFIGURATION_DISABLED_PII
response_data = self.get()
# the GET should pull back the same data as the POST
assert response_data == data
@ddt.data(
Provider.ED_DISCUSS,
Provider.INSCRIBE,
Provider.PIAZZA,
Provider.YELLOWDIG,
)
def test_post_everything_with_pii_enabled(self, provider):
"""
Test posting all data returns expected response when course PII flag is disabled.
"""
with self._pii_sharing_for_course(enabled=True):
self._assert_pii_flag_for_course(enabled=True)
data = self._configure_lti_discussion_provider(provider=provider)
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['lti_configuration'] == DATA_LTI_CONFIGURATION_ENABLED_PII
response_data = self.get()
# the GET should pull back the same data as the POST
assert response_data == data