Merge pull request #14766 from edx/mrehan/add-flag-for-lti-xblock

TNL-6688 – Add flag to control lti-consumer-xblock's request username/email editing in Studio

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
 
 

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)

cms/djangoapps/xblock_config/forms.py

+"""
+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

cms/djangoapps/xblock_config/migrations/0002_courseeditltifieldsenabledflag.py

+# -*- 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,
+            },
+        ),
+    ]

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,
+        )

cms/djangoapps/xblock_config/tests/test_models.py

+"""
+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))

common/lib/xmodule/xmodule/services.py

 """
 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

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)

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