From dc335a2215bfdb2a78fa746c43a70508afe0f9c4 Mon Sep 17 00:00:00 2001 From: Qubad786 Date: Fri, 10 Nov 2017 19:19:09 +0500 Subject: [PATCH] EDUCATOR-1628 Enable Video Uploads by default with course rollout flag. --- cms/djangoapps/contentstore/views/videos.py | 8 ++- common/lib/xmodule/xmodule/course_module.py | 3 +- openedx/core/djangoapps/video_config/admin.py | 8 ++- openedx/core/djangoapps/video_config/forms.py | 5 +- .../video_config/tests/test_models.py | 17 +++--- .../core/djangoapps/video_pipeline/admin.py | 19 +++++- .../core/djangoapps/video_pipeline/forms.py | 15 +++++ ...dbydefault_videouploadsenabledbydefault.py | 46 ++++++++++++++ .../core/djangoapps/video_pipeline/models.py | 61 +++++++++++++++++++ .../video_pipeline/tests/test_models.py | 59 ++++++++++++++++++ 10 files changed, 226 insertions(+), 15 deletions(-) create mode 100644 openedx/core/djangoapps/video_pipeline/forms.py create mode 100644 openedx/core/djangoapps/video_pipeline/migrations/0003_coursevideouploadsenabledbydefault_videouploadsenabledbydefault.py create mode 100644 openedx/core/djangoapps/video_pipeline/tests/test_models.py diff --git a/cms/djangoapps/contentstore/views/videos.py b/cms/djangoapps/contentstore/views/videos.py index e03825297e..74e69cf2ed 100644 --- a/cms/djangoapps/contentstore/views/videos.py +++ b/cms/djangoapps/contentstore/views/videos.py @@ -672,7 +672,6 @@ def videos_post(course, request): return JsonResponse({'error': error}, status=400) bucket = storage_service_bucket() - course_video_upload_token = course.video_upload_pipeline['course_video_upload_token'] req_files = data['files'] resp_files = [] @@ -689,11 +688,16 @@ def videos_post(course, request): key = storage_service_key(bucket, file_name=edx_video_id) metadata_list = [ - ('course_video_upload_token', course_video_upload_token), ('client_video_id', file_name), ('course_key', unicode(course.id)), ] + # Only include `course_video_upload_token` if its set, as it won't be required if video uploads + # are enabled by default. + course_video_upload_token = course.video_upload_pipeline.get('course_video_upload_token') + if course_video_upload_token: + metadata_list.append(('course_video_upload_token', course_video_upload_token)) + is_video_transcript_enabled = VideoTranscriptEnabledFlag.feature_enabled(course.id) if is_video_transcript_enabled: transcript_preferences = get_transcript_preferences(unicode(course.id)) diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index ae4c8b10a6..6bc33cda24 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -12,6 +12,7 @@ from django.conf import settings import requests from lazy import lazy from lxml import etree +from openedx.core.djangoapps.video_pipeline.models import VideoUploadsEnabledByDefault from openedx.core.lib.license import LicenseMixin from path import Path as path from pytz import utc @@ -1337,7 +1338,7 @@ class CourseDescriptor(CourseFields, SequenceDescriptor, LicenseMixin): """ Returns whether the video pipeline advanced setting is configured for this course. """ - return ( + return VideoUploadsEnabledByDefault.feature_enabled(course_id=self.id) or ( self.video_upload_pipeline is not None and 'course_video_upload_token' in self.video_upload_pipeline ) diff --git a/openedx/core/djangoapps/video_config/admin.py b/openedx/core/djangoapps/video_config/admin.py index 0b9556c9ff..5f9d19b30c 100644 --- a/openedx/core/djangoapps/video_config/admin.py +++ b/openedx/core/djangoapps/video_config/admin.py @@ -7,10 +7,11 @@ from django.contrib import admin from openedx.core.djangoapps.video_config.forms import ( CourseHLSPlaybackFlagAdminForm, + CourseVideoTranscriptFlagAdminForm, ) from openedx.core.djangoapps.video_config.models import ( CourseHLSPlaybackEnabledFlag, HLSPlaybackEnabledFlag, - CourseVideoTranscriptEnabledFlag, VideoTranscriptEnabledFlag + CourseVideoTranscriptEnabledFlag, VideoTranscriptEnabledFlag, ) @@ -45,9 +46,10 @@ class CourseVideoTranscriptEnabledFlagAdmin(CourseSpecificEnabledFlagBaseAdmin): Admin of Video Transcript feature on course-by-course basis. Allows searching by course id. """ - form = CourseHLSPlaybackFlagAdminForm + form = CourseVideoTranscriptFlagAdminForm admin.site.register(HLSPlaybackEnabledFlag, ConfigurationModelAdmin) admin.site.register(CourseHLSPlaybackEnabledFlag, CourseHLSPlaybackEnabledFlagAdmin) + admin.site.register(VideoTranscriptEnabledFlag, ConfigurationModelAdmin) -admin.site.register(CourseVideoTranscriptEnabledFlag, CourseHLSPlaybackEnabledFlagAdmin) +admin.site.register(CourseVideoTranscriptEnabledFlag, CourseVideoTranscriptEnabledFlagAdmin) diff --git a/openedx/core/djangoapps/video_config/forms.py b/openedx/core/djangoapps/video_config/forms.py index e37477d3d5..45fb6ad54c 100644 --- a/openedx/core/djangoapps/video_config/forms.py +++ b/openedx/core/djangoapps/video_config/forms.py @@ -7,7 +7,10 @@ from django import forms from opaque_keys import InvalidKeyError from opaque_keys.edx.locator import CourseLocator -from openedx.core.djangoapps.video_config.models import CourseHLSPlaybackEnabledFlag, CourseVideoTranscriptEnabledFlag +from openedx.core.djangoapps.video_config.models import ( + CourseHLSPlaybackEnabledFlag, + CourseVideoTranscriptEnabledFlag, +) from xmodule.modulestore.django import modulestore log = logging.getLogger(__name__) diff --git a/openedx/core/djangoapps/video_config/tests/test_models.py b/openedx/core/djangoapps/video_config/tests/test_models.py index aeb3622131..503e4f74ce 100644 --- a/openedx/core/djangoapps/video_config/tests/test_models.py +++ b/openedx/core/djangoapps/video_config/tests/test_models.py @@ -9,7 +9,10 @@ from contextlib import contextmanager from django.test import TestCase from opaque_keys.edx.locator import CourseLocator -from openedx.core.djangoapps.video_config.models import CourseHLSPlaybackEnabledFlag, HLSPlaybackEnabledFlag +from openedx.core.djangoapps.video_config.models import ( + CourseHLSPlaybackEnabledFlag, HLSPlaybackEnabledFlag, + CourseVideoTranscriptEnabledFlag, VideoTranscriptEnabledFlag, +) @contextmanager @@ -184,8 +187,8 @@ class TestVideoTranscriptFlag(TestCase, FeatureFlagTestMixin): with course-specific flags. """ self.verify_feature_flags( - all_courses_model_class=HLSPlaybackEnabledFlag, - course_specific_model_class=CourseHLSPlaybackEnabledFlag, + all_courses_model_class=VideoTranscriptEnabledFlag, + course_specific_model_class=CourseVideoTranscriptEnabledFlag, global_flag=global_flag, enabled_for_all_courses=enabled_for_all_courses, enabled_for_course_1=enabled_for_course_1 @@ -196,8 +199,8 @@ class TestVideoTranscriptFlag(TestCase, FeatureFlagTestMixin): Ensures that the Video Transcript course specific flag, once enabled for a course, can also be disabled. """ self.verify_enable_disable_course_flag( - all_courses_model_class=HLSPlaybackEnabledFlag, - course_specific_model_class=CourseHLSPlaybackEnabledFlag + all_courses_model_class=VideoTranscriptEnabledFlag, + course_specific_model_class=CourseVideoTranscriptEnabledFlag ) def test_enable_disable_globally(self): @@ -205,6 +208,6 @@ class TestVideoTranscriptFlag(TestCase, FeatureFlagTestMixin): Ensures that the Video Transcript flag, once enabled globally, can also be disabled. """ self.verify_enable_disable_globally( - all_courses_model_class=HLSPlaybackEnabledFlag, - course_specific_model_class=CourseHLSPlaybackEnabledFlag + all_courses_model_class=VideoTranscriptEnabledFlag, + course_specific_model_class=CourseVideoTranscriptEnabledFlag ) diff --git a/openedx/core/djangoapps/video_pipeline/admin.py b/openedx/core/djangoapps/video_pipeline/admin.py index 97bb4dda1f..b204e385d1 100644 --- a/openedx/core/djangoapps/video_pipeline/admin.py +++ b/openedx/core/djangoapps/video_pipeline/admin.py @@ -4,6 +4,23 @@ Django admin for Video Pipeline models. from config_models.admin import ConfigurationModelAdmin from django.contrib import admin -from openedx.core.djangoapps.video_pipeline.models import VideoPipelineIntegration +from openedx.core.djangoapps.video_config.admin import CourseSpecificEnabledFlagBaseAdmin +from openedx.core.djangoapps.video_pipeline.forms import CourseVideoUploadsEnabledByDefaultAdminForm +from openedx.core.djangoapps.video_pipeline.models import ( + VideoPipelineIntegration, + VideoUploadsEnabledByDefault, + CourseVideoUploadsEnabledByDefault, +) + + +class CourseVideoUploadsEnabledByDefaultAdmin(CourseSpecificEnabledFlagBaseAdmin): + """ + Admin of video uploads enabled by default feature on course-by-course basis. + Allows searching by course id. + """ + form = CourseVideoUploadsEnabledByDefaultAdminForm admin.site.register(VideoPipelineIntegration, ConfigurationModelAdmin) + +admin.site.register(VideoUploadsEnabledByDefault, ConfigurationModelAdmin) +admin.site.register(CourseVideoUploadsEnabledByDefault, CourseVideoUploadsEnabledByDefaultAdmin) diff --git a/openedx/core/djangoapps/video_pipeline/forms.py b/openedx/core/djangoapps/video_pipeline/forms.py new file mode 100644 index 0000000000..b6abaff820 --- /dev/null +++ b/openedx/core/djangoapps/video_pipeline/forms.py @@ -0,0 +1,15 @@ +""" +Defines a form to provide validations for course-specific configuration. +""" +from openedx.core.djangoapps.video_config.forms import CourseSpecificFlagAdminBaseForm +from openedx.core.djangoapps.video_pipeline.models import CourseVideoUploadsEnabledByDefault + + +class CourseVideoUploadsEnabledByDefaultAdminForm(CourseSpecificFlagAdminBaseForm): + """ + Form for course-specific Video Uploads enabled by default configuration. + """ + + class Meta(object): + model = CourseVideoUploadsEnabledByDefault + fields = '__all__' diff --git a/openedx/core/djangoapps/video_pipeline/migrations/0003_coursevideouploadsenabledbydefault_videouploadsenabledbydefault.py b/openedx/core/djangoapps/video_pipeline/migrations/0003_coursevideouploadsenabledbydefault_videouploadsenabledbydefault.py new file mode 100644 index 0000000000..80684fde3e --- /dev/null +++ b/openedx/core/djangoapps/video_pipeline/migrations/0003_coursevideouploadsenabledbydefault_videouploadsenabledbydefault.py @@ -0,0 +1,46 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import migrations, models +from django.conf import settings +import django.db.models.deletion +import openedx.core.djangoapps.xmodule_django.models + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('video_pipeline', '0002_auto_20171114_0704'), + ] + + operations = [ + migrations.CreateModel( + name='CourseVideoUploadsEnabledByDefault', + 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, + }, + ), + migrations.CreateModel( + name='VideoUploadsEnabledByDefault', + 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')), + ('enabled_for_all_courses', models.BooleanField(default=False)), + ('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/openedx/core/djangoapps/video_pipeline/models.py b/openedx/core/djangoapps/video_pipeline/models.py index 35c96d7100..83ae240cd0 100644 --- a/openedx/core/djangoapps/video_pipeline/models.py +++ b/openedx/core/djangoapps/video_pipeline/models.py @@ -6,6 +6,8 @@ from django.contrib.auth import get_user_model from django.db import models from django.utils.translation import ugettext_lazy as _ +from openedx.core.djangoapps.xmodule_django.models import CourseKeyField + class VideoPipelineIntegration(ConfigurationModel): """ @@ -37,3 +39,62 @@ class VideoPipelineIntegration(ConfigurationModel): # in lms/startup.py. User = get_user_model() # pylint: disable=invalid-name return User.objects.get(username=self.service_username) + + +class VideoUploadsEnabledByDefault(ConfigurationModel): + """ + Enables video uploads enabled By default feature across the platform. + """ + # this field overrides course-specific settings + enabled_for_all_courses = models.BooleanField(default=False) + + @classmethod + def feature_enabled(cls, course_id): + """ + Looks at the currently active configuration model to determine whether + the VideoUploadsEnabledByDefault feature is available. + + If the feature flag is not enabled, the feature is not available. + If the flag is enabled for all the courses, feature is available. + If the flag is enabled and the provided course_id is for a course + with CourseVideoUploadsEnabledByDefault enabled, then the + feature is available. + + Arguments: + course_id (CourseKey): course id for whom feature will be checked. + """ + if not cls.is_enabled(): + return False + elif not cls.current().enabled_for_all_courses: + feature = (CourseVideoUploadsEnabledByDefault.objects + .filter(course_id=course_id) + .order_by('-change_date') + .first()) + return feature.enabled if feature else False + return True + + def __unicode__(self): + current_model = VideoUploadsEnabledByDefault.current() + return u"VideoUploadsEnabledByDefault: enabled {is_enabled}".format( + is_enabled=current_model.is_enabled() + ) + + +class CourseVideoUploadsEnabledByDefault(ConfigurationModel): + """ + Enables video uploads enabled by default feature for a specific course. Its global feature must be + enabled for this to take effect. + """ + KEY_FIELDS = ('course_id',) + + course_id = CourseKeyField(max_length=255, db_index=True) + + def __unicode__(self): + not_en = "Not " + if self.enabled: + not_en = "" + + return u"Course '{course_key}': Video Uploads {not_enabled}Enabled by default.".format( + course_key=unicode(self.course_id), + not_enabled=not_en + ) diff --git a/openedx/core/djangoapps/video_pipeline/tests/test_models.py b/openedx/core/djangoapps/video_pipeline/tests/test_models.py new file mode 100644 index 0000000000..cd9eab4716 --- /dev/null +++ b/openedx/core/djangoapps/video_pipeline/tests/test_models.py @@ -0,0 +1,59 @@ +""" +Tests for the models that configures 'VideoUploadsEnabledByDefault' feature. +""" +import ddt +import itertools + +from django.test import TestCase +from openedx.core.djangoapps.video_config.tests.test_models import FeatureFlagTestMixin +from openedx.core.djangoapps.video_pipeline.models import ( + CourseVideoUploadsEnabledByDefault, VideoUploadsEnabledByDefault, +) + + +@ddt.ddt +class TestVideoUploadsEnabledByDefault(TestCase, FeatureFlagTestMixin): + """ + Tests the behavior of the flags for video uploads enabled by default feature. + These are set via Django admin settings. + """ + @ddt.data( + *itertools.product( + (True, False), + (True, False), + (True, False), + ) + ) + @ddt.unpack + def test_video_upload_enabled_by_default_feature_flags(self, global_flag, + enabled_for_all_courses, enabled_for_course_1): + """ + Tests that video uploads enabled by default feature flags works correctly on tweaking global flags + in combination with course-specific flags. + """ + self.verify_feature_flags( + all_courses_model_class=VideoUploadsEnabledByDefault, + course_specific_model_class=CourseVideoUploadsEnabledByDefault, + global_flag=global_flag, + enabled_for_all_courses=enabled_for_all_courses, + enabled_for_course_1=enabled_for_course_1 + ) + + def test_enable_disable_course_flag(self): + """ + Ensures that the video uploads enabled by default course specific flag, once enabled for a course, + can also be disabled. + """ + self.verify_enable_disable_course_flag( + all_courses_model_class=VideoUploadsEnabledByDefault, + course_specific_model_class=CourseVideoUploadsEnabledByDefault + ) + + def test_enable_disable_globally(self): + """ + Ensures that the video uploads enabled by default flag, once enabled globally, can also be disabled. + """ + self.verify_enable_disable_globally( + all_courses_model_class=VideoUploadsEnabledByDefault, + course_specific_model_class=CourseVideoUploadsEnabledByDefault + )