From 4b2488b5eedc803c9981dae1be2b883b7c57add6 Mon Sep 17 00:00:00 2001 From: Qubad786 Date: Thu, 30 Mar 2017 16:54:17 +0500 Subject: [PATCH 1/2] Add config models and a configuration service to configure LTI Consumer. --- cms/djangoapps/contentstore/views/item.py | 6 +- cms/djangoapps/xblock_config/admin.py | 25 ++++- cms/djangoapps/xblock_config/forms.py | 44 +++++++++ .../0002_courseeditltifieldsenabledflag.py | 32 +++++++ cms/djangoapps/xblock_config/models.py | 55 ++++++++++- .../xblock_config/tests/__init__.py | 0 .../xblock_config/tests/test_models.py | 91 +++++++++++++++++++ common/lib/xmodule/xmodule/services.py | 32 +++++++ .../xmodule/xmodule/tests/test_services.py | 37 +++++++- 9 files changed, 317 insertions(+), 5 deletions(-) create mode 100644 cms/djangoapps/xblock_config/forms.py create mode 100644 cms/djangoapps/xblock_config/migrations/0002_courseeditltifieldsenabledflag.py create mode 100644 cms/djangoapps/xblock_config/tests/__init__.py create mode 100644 cms/djangoapps/xblock_config/tests/test_models.py diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index d082c166e2..e6510be15a 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -23,6 +23,8 @@ from pytz import UTC from xblock.core import XBlock from xblock.fields import Scope from xblock.fragment import Fragment + +from xblock_config.models import CourseEditLTIFieldsEnabledFlag from xblock_django.user_service import DjangoXBlockUserService from cms.lib.xblock.authoring_mixin import VISIBILITY_VIEW @@ -51,7 +53,7 @@ from xmodule.modulestore.django import modulestore from xmodule.modulestore.draft_and_published import DIRECT_ONLY_CATEGORIES from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError from xmodule.modulestore.inheritance import own_metadata -from xmodule.services import SettingsService +from xmodule.services import ConfigurationService, SettingsService from xmodule.tabs import CourseTabList from xmodule.x_module import PREVIEW_VIEWS, STUDIO_VIEW, STUDENT_VIEW, DEPRECATION_VSCOMPAT_EVENT @@ -269,6 +271,8 @@ class StudioEditModuleRuntime(object): return StudioPermissionsService(self._user) if service_name == "settings": return SettingsService() + if service_name == "lti-configuration": + return ConfigurationService(CourseEditLTIFieldsEnabledFlag) return None diff --git a/cms/djangoapps/xblock_config/admin.py b/cms/djangoapps/xblock_config/admin.py index dc83781427..9efd20799f 100644 --- a/cms/djangoapps/xblock_config/admin.py +++ b/cms/djangoapps/xblock_config/admin.py @@ -3,7 +3,28 @@ Django admin dashboard configuration for LMS XBlock infrastructure. """ from django.contrib import admin -from config_models.admin import ConfigurationModelAdmin -from xblock_config.models import StudioConfig +from config_models.admin import ConfigurationModelAdmin, KeyedConfigurationModelAdmin + +from xblock_config.forms import CourseEditLTIFieldsEnabledAdminForm +from xblock_config.models import ( + CourseEditLTIFieldsEnabledFlag, + StudioConfig, +) + + +class CourseEditLTIFieldsEnabledFlagAdmin(KeyedConfigurationModelAdmin): + """ + Admin for LTI Fields Editing feature on course-by-course basis. + Allows searching by course id. + """ + form = CourseEditLTIFieldsEnabledAdminForm + search_fields = ['course_id'] + fieldsets = ( + (None, { + 'fields': ('course_id', 'enabled'), + 'description': 'Enter a valid course id. If it is invalid, an error message will be displayed.' + }), + ) admin.site.register(StudioConfig, ConfigurationModelAdmin) +admin.site.register(CourseEditLTIFieldsEnabledFlag, CourseEditLTIFieldsEnabledFlagAdmin) diff --git a/cms/djangoapps/xblock_config/forms.py b/cms/djangoapps/xblock_config/forms.py new file mode 100644 index 0000000000..f714b00e72 --- /dev/null +++ b/cms/djangoapps/xblock_config/forms.py @@ -0,0 +1,44 @@ +""" +Defines a form for providing validation of LTI consumer course-specific configuration. +""" +import logging + +from django import forms +from opaque_keys import InvalidKeyError +from opaque_keys.edx.locator import CourseLocator +from xmodule.modulestore.django import modulestore + +from xblock_config.models import CourseEditLTIFieldsEnabledFlag + +log = logging.getLogger(__name__) + + +class CourseEditLTIFieldsEnabledAdminForm(forms.ModelForm): + """ + Form for LTI consumer course-specific configuration to verify the course id. + """ + + class Meta(object): + model = CourseEditLTIFieldsEnabledFlag + fields = '__all__' + + def clean_course_id(self): + """ + Validate the course id + """ + cleaned_id = self.cleaned_data["course_id"] + try: + course_key = CourseLocator.from_string(cleaned_id) + except InvalidKeyError: + msg = u'Course id invalid. Entered course id was: "{course_id}."'.format( + course_id=cleaned_id + ) + raise forms.ValidationError(msg) + + if not modulestore().has_course(course_key): + msg = u'Course not found. Entered course id was: "{course_key}". '.format( + course_key=unicode(course_key) + ) + raise forms.ValidationError(msg) + + return course_key diff --git a/cms/djangoapps/xblock_config/migrations/0002_courseeditltifieldsenabledflag.py b/cms/djangoapps/xblock_config/migrations/0002_courseeditltifieldsenabledflag.py new file mode 100644 index 0000000000..f6d165aed9 --- /dev/null +++ b/cms/djangoapps/xblock_config/migrations/0002_courseeditltifieldsenabledflag.py @@ -0,0 +1,32 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import migrations, models +import django.db.models.deletion +import openedx.core.djangoapps.xmodule_django.models +from django.conf import settings + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('xblock_config', '0001_initial'), + ] + + operations = [ + migrations.CreateModel( + name='CourseEditLTIFieldsEnabledFlag', + fields=[ + ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)), + ('change_date', models.DateTimeField(auto_now_add=True, verbose_name='Change date')), + ('enabled', models.BooleanField(default=False, verbose_name='Enabled')), + ('course_id', openedx.core.djangoapps.xmodule_django.models.CourseKeyField(max_length=255, db_index=True)), + ('changed_by', models.ForeignKey(on_delete=django.db.models.deletion.PROTECT, editable=False, to=settings.AUTH_USER_MODEL, null=True, verbose_name='Changed by')), + ], + options={ + 'ordering': ('-change_date',), + 'abstract': False, + }, + ), + ] diff --git a/cms/djangoapps/xblock_config/models.py b/cms/djangoapps/xblock_config/models.py index b002d13850..3dacfd2692 100644 --- a/cms/djangoapps/xblock_config/models.py +++ b/cms/djangoapps/xblock_config/models.py @@ -5,9 +5,11 @@ Includes: StudioConfig: A ConfigurationModel for managing Studio. """ -from django.db.models import TextField +from django.db.models import BooleanField, TextField +from openedx.core.djangoapps.xmodule_django.models import CourseKeyField from config_models.models import ConfigurationModel +from request_cache.middleware import request_cached class StudioConfig(ConfigurationModel): @@ -26,3 +28,54 @@ class StudioConfig(ConfigurationModel): """ studio_config = cls.current() return studio_config.enabled and block_type not in studio_config.disabled_blocks.split() + + +# TODO: Move CourseEditLTIFieldsEnabledFlag to LTI XBlock as a part of EDUCATOR-121 +# reference: https://openedx.atlassian.net/browse/EDUCATOR-121 +class CourseEditLTIFieldsEnabledFlag(ConfigurationModel): + """ + Enables the editing of "request username" and "request email" fields + of LTI consumer for a specific course. + """ + KEY_FIELDS = ('course_id',) + + course_id = CourseKeyField(max_length=255, db_index=True) + + @classmethod + @request_cached + def lti_access_to_learners_editable(cls, course_id, is_already_sharing_learner_info): + """ + Looks at the currently active configuration model to determine whether + the feature that enables editing of "request username" and "request email" + fields of LTI consumer is available or not. + + Backwards Compatibility: + Enable this feature for a course run who was sharing learner username/email + in the past. + + Arguments: + course_id (CourseKey): course id for which we need to check this configuration + is_already_sharing_learner_info (bool): indicates whether LTI consumer is + already sharing edX learner username/email. + """ + course_specific_config = (CourseEditLTIFieldsEnabledFlag.objects + .filter(course_id=course_id) + .order_by('-change_date') + .first()) + + if is_already_sharing_learner_info: + if not course_specific_config: + CourseEditLTIFieldsEnabledFlag.objects.create(course_id=course_id, enabled=True) + return True + + return course_specific_config.enabled if course_specific_config else False + + def __unicode__(self): + en = "Not " + if self.enabled: + en = "" + + return u"Course '{course_id}': Edit LTI access to Learner information {en}Enabled".format( + course_id=unicode(self.course_id), + en=en, + ) diff --git a/cms/djangoapps/xblock_config/tests/__init__.py b/cms/djangoapps/xblock_config/tests/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/cms/djangoapps/xblock_config/tests/test_models.py b/cms/djangoapps/xblock_config/tests/test_models.py new file mode 100644 index 0000000000..8239ef6d44 --- /dev/null +++ b/cms/djangoapps/xblock_config/tests/test_models.py @@ -0,0 +1,91 @@ +""" +Tests for the models that configures Edit LTI fields feature. +""" +import ddt + +from contextlib import contextmanager +from django.test import TestCase + +from opaque_keys.edx.locator import CourseLocator +from request_cache.middleware import RequestCache +from xblock_config.models import CourseEditLTIFieldsEnabledFlag + + +@contextmanager +def lti_consumer_fields_editing_flag(course_id, enabled_for_course=False): + """ + Yields CourseEditLTIFieldsEnabledFlag record for unit tests + + Arguments: + course_id (CourseLocator): course locator to control this feature for. + enabled_for_course (bool): whether feature is enabled for 'course_id' + """ + RequestCache.clear_request_cache() + CourseEditLTIFieldsEnabledFlag.objects.create(course_id=course_id, enabled=enabled_for_course) + yield + + +@ddt.ddt +class TestLTIConsumerHideFieldsFlag(TestCase): + """ + Tests the behavior of the flags for lti consumer fields' editing feature. + These are set via Django admin settings. + """ + def setUp(self): + super(TestLTIConsumerHideFieldsFlag, self).setUp() + self.course_id = CourseLocator(org="edx", course="course", run="run") + + @ddt.data( + (True, True), + (True, False), + (False, True), + (False, False), + ) + @ddt.unpack + def test_lti_fields_editing_feature_flags(self, enabled_for_course, is_already_sharing_learner_info): + """ + Test that feature flag works correctly with course-specific configuration in combination with + a boolean which indicates whether a course-run already sharing learner username/email - given + the course-specific configuration record is present. + """ + with lti_consumer_fields_editing_flag( + course_id=self.course_id, + enabled_for_course=enabled_for_course + ): + feature_enabled = CourseEditLTIFieldsEnabledFlag.lti_access_to_learners_editable( + self.course_id, + is_already_sharing_learner_info, + ) + self.assertEqual(feature_enabled, enabled_for_course) + + @ddt.data(True, False) + def test_lti_fields_editing_is_backwards_compatible(self, is_already_sharing_learner_info): + """ + Test that feature flag works correctly with a boolean which indicates whether a course-run already + sharing learner username/email - given the course-specific configuration record is not set previously. + + This tests the backward compatibility which currently is: if an existing course run is already + sharing learner information then this feature should be enabled for that course run by default. + """ + feature_enabled = CourseEditLTIFieldsEnabledFlag.lti_access_to_learners_editable( + self.course_id, + is_already_sharing_learner_info, + ) + feature_flag_created = CourseEditLTIFieldsEnabledFlag.objects.filter(course_id=self.course_id).exists() + self.assertEqual(feature_flag_created, is_already_sharing_learner_info) + self.assertEqual(feature_enabled, is_already_sharing_learner_info) + + def test_enable_disable_course_flag(self): + """ + Ensures that the flag, once enabled for a course, can also be disabled. + """ + with lti_consumer_fields_editing_flag( + course_id=self.course_id, + enabled_for_course=True + ): + self.assertTrue(CourseEditLTIFieldsEnabledFlag.lti_access_to_learners_editable(self.course_id, False)) + with lti_consumer_fields_editing_flag( + course_id=self.course_id, + enabled_for_course=False + ): + self.assertFalse(CourseEditLTIFieldsEnabledFlag.lti_access_to_learners_editable(self.course_id, False)) diff --git a/common/lib/xmodule/xmodule/services.py b/common/lib/xmodule/xmodule/services.py index fc8a08851a..05e4bfb653 100644 --- a/common/lib/xmodule/xmodule/services.py +++ b/common/lib/xmodule/xmodule/services.py @@ -1,6 +1,9 @@ """ Module contains various XModule/XBlock services """ +import inspect + +from config_models.models import ConfigurationModel from django.conf import settings @@ -61,3 +64,32 @@ class SettingsService(object): xblock_settings_bucket = getattr(block, self.xblock_settings_bucket_selector, block.unmixed_class.__name__) xblock_settings = settings.XBLOCK_SETTINGS if hasattr(settings, "XBLOCK_SETTINGS") else {} return xblock_settings.get(xblock_settings_bucket, actual_default) + + +# TODO: ConfigurationService and its usage will be removed as a part of EDUCATOR-121 +# reference: https://openedx.atlassian.net/browse/EDUCATOR-121 +class ConfigurationService(object): + """ + An XBlock service to talk with the Configuration Models. This service should provide + a pathway to Configuration Model which is designed to configure the corresponding XBlock. + """ + def __init__(self, configuration_model): + """ + Class initializer, this exposes configuration model to XBlock. + + Arguments: + configuration_model (ConfigurationModel): configurations for an XBlock + + Raises: + exception (ValueError): when configuration_model is not a subclass of + ConfigurationModel. + """ + if not (inspect.isclass(configuration_model) and issubclass(configuration_model, ConfigurationModel)): + raise ValueError( + "Expected ConfigurationModel got {0} of type {1}".format( + configuration_model, + type(configuration_model) + ) + ) + + self.configuration = configuration_model diff --git a/common/lib/xmodule/xmodule/tests/test_services.py b/common/lib/xmodule/xmodule/tests/test_services.py index c8dfd89639..16a25d4d6f 100644 --- a/common/lib/xmodule/xmodule/tests/test_services.py +++ b/common/lib/xmodule/xmodule/tests/test_services.py @@ -6,11 +6,12 @@ import ddt import mock from unittest import TestCase +from config_models.models import ConfigurationModel from django.conf import settings from django.test.utils import override_settings from xblock.runtime import Mixologist -from xmodule.services import SettingsService +from xmodule.services import ConfigurationService, SettingsService class _DummyBlock(object): @@ -18,6 +19,20 @@ class _DummyBlock(object): pass +class DummyConfig(ConfigurationModel): + """ + Dummy Configuration + """ + pass + + +class DummyUnexpected(object): + """ + Dummy Unexpected Class + """ + pass + + @ddt.ddt class TestSettingsService(TestCase): """ Test SettingsService """ @@ -76,3 +91,23 @@ class TestSettingsService(TestCase): block = mixologist.mix(_DummyBlock) self.assertEqual(settings.XBLOCK_SETTINGS, {"_DummyBlock": [1, 2, 3]}) self.assertEqual(self.settings_service.get_settings_bucket(block), [1, 2, 3]) + + +class TestConfigurationService(TestCase): + """ + Tests for ConfigurationService + """ + def test_given_unexpected_class_throws_value_error(self): + """ + Test that instantiating ConfigurationService raises exception on passing + a class which is not subclass of ConfigurationModel. + """ + with self.assertRaises(ValueError): + ConfigurationService(DummyUnexpected) + + def test_configuration_service(self): + """ + Test the correct configuration on instantiating ConfigurationService. + """ + config_service = ConfigurationService(DummyConfig) + self.assertEqual(config_service.configuration, DummyConfig) From f3b480306626b4905b55eab9a74c48b87a71841a Mon Sep 17 00:00:00 2001 From: Qubad786 Date: Tue, 18 Apr 2017 18:42:39 +0500 Subject: [PATCH 2/2] Update lti_consumer xblock to latest version --- requirements/edx/github.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index 098f1a1a7c..ab60129411 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -87,7 +87,7 @@ git+https://github.com/edx/edx-milestones.git@v0.1.10#egg=edx-milestones==0.1.10 git+https://github.com/edx/xblock-utils.git@v1.0.5#egg=xblock-utils==1.0.5 -e git+https://github.com/edx-solutions/xblock-google-drive.git@138e6fa0bf3a2013e904a085b9fed77dab7f3f21#egg=xblock-google-drive git+https://github.com/edx/edx-user-state-client.git@1.0.1#egg=edx-user-state-client==1.0.1 -git+https://github.com/edx/xblock-lti-consumer.git@v1.1.2#egg=lti_consumer-xblock==1.1.2 +git+https://github.com/edx/xblock-lti-consumer.git@v1.1.3#egg=lti_consumer-xblock==1.1.3 git+https://github.com/edx/edx-proctoring.git@0.18.0#egg=edx-proctoring==0.18.0 # Third Party XBlocks