From 745cda1580a64dee1d25d7e7861c2dbd9d440f88 Mon Sep 17 00:00:00 2001 From: Nathan Sprenkle Date: Wed, 3 May 2023 13:23:34 -0400 Subject: [PATCH] feat: course overrides of video sharing settings (#32169) * refactor: public sharing enabled toggle Cherry-picked from https://github.com/openedx/edx-platform/pull/32150 * feat: course video share setting on video block Adds awareness of course.video_sharing_options setting to video XBlock. This can override whether or not sharing is enabled for a video or fall back to per-video settings * test: course video share setting on video block Adds awareness of course.video_sharing_options setting to video XBlock. This can override whether or not sharing is enabled for a video or fall back to per-video settings * test: course video share setting on preview page Extends checking for course override of video share settings to public video preview page. --- .../courseware/tests/test_video_mongo.py | 109 +++++++++++++++--- lms/djangoapps/courseware/views/views.py | 2 +- xmodule/video_block/video_block.py | 78 ++++++++++--- 3 files changed, 157 insertions(+), 32 deletions(-) diff --git a/lms/djangoapps/courseware/tests/test_video_mongo.py b/lms/djangoapps/courseware/tests/test_video_mongo.py index 44800162d5..0e9008a099 100644 --- a/lms/djangoapps/courseware/tests/test_video_mongo.py +++ b/lms/djangoapps/courseware/tests/test_video_mongo.py @@ -3,6 +3,7 @@ Video xmodule tests in mongo. """ +from contextlib import contextmanager import json import shutil from collections import OrderedDict @@ -15,6 +16,7 @@ import pytest from django.conf import settings from django.core.files import File from django.core.files.base import ContentFile +from django.http import Http404 from django.test import TestCase from django.test.utils import override_settings from edx_toggles.toggles.testutils import override_waffle_flag @@ -35,6 +37,11 @@ from fs.path import combine from lxml import etree from path import Path as path from xmodule.contentstore.content import StaticContent +from xmodule.course_block import ( + COURSE_VIDEO_SHARING_ALL_VIDEOS, + COURSE_VIDEO_SHARING_NONE, + COURSE_VIDEO_SHARING_PER_VIDEO +) from xmodule.exceptions import NotFoundError from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.inheritance import own_metadata @@ -135,7 +142,6 @@ class TestVideoYouTube(TestVideo): # lint-amnesty, pylint: disable=missing-clas {'display_name': 'Text (.txt) file', 'value': 'txt'} ], 'poster': 'null', - 'public_video_url': None, } mako_service = self.block.runtime.service(self.block, 'mako') @@ -219,7 +225,6 @@ class TestVideoNonYouTube(TestVideo): # pylint: disable=test-inherits-tests {'display_name': 'Text (.txt) file', 'value': 'txt'} ], 'poster': 'null', - 'public_video_url': None, } mako_service = self.block.runtime.service(self.block, 'mako') @@ -240,6 +245,11 @@ class TestVideoPublicAccess(BaseTestVideoXBlock): } METADATA = {} + @contextmanager + def mock_feature_toggle(self, enabled=True): + with patch.object(PUBLIC_VIDEO_SHARE, 'is_enabled', return_value=enabled): + yield + @ddt.data( (True, False), (False, False), @@ -247,16 +257,87 @@ class TestVideoPublicAccess(BaseTestVideoXBlock): (True, True), ) @ddt.unpack - def test_public_video_url(self, is_lms_platform, enable_public_share): + def test_is_public_sharing_enabled(self, is_studio, feature_enabled): """Test public video url.""" assert self.block.public_access is True - if not is_lms_platform: + if is_studio: self.block.runtime.is_author_mode = True - with patch.object(PUBLIC_VIDEO_SHARE, 'is_enabled', return_value=enable_public_share): - context = self.block.render(STUDENT_VIEW).content - # public video url iif PUBLIC_VIDEO_SHARE waffle and is_lms_platform, public_access are true - assert bool(get_context_dict_from_string(context)['public_video_url']) \ - is (is_lms_platform and enable_public_share) + + with self.mock_feature_toggle(enabled=feature_enabled): + assert self.block.is_public_sharing_enabled() == \ + (not is_studio and feature_enabled) + + def test_is_public_sharing_enabled__not_public(self): + self.block.public_access = False + with self.mock_feature_toggle(): + assert not self.block.is_public_sharing_enabled() + + @patch('xmodule.video_block.video_block.VideoBlock.get_course_video_sharing_override') + def test_is_public_sharing_enabled_by_course_override(self, mock_course_sharing_override): + + # Given a course overrides all videos to be shared + mock_course_sharing_override.return_value = COURSE_VIDEO_SHARING_ALL_VIDEOS + self.block.public_access = 'some-arbitrary-value' + + # When I try to determine if public sharing is enabled + with self.mock_feature_toggle(): + is_public_sharing_enabled = self.block.is_public_sharing_enabled() + + # Then I will get that course value + self.assertTrue(is_public_sharing_enabled) + + @patch('xmodule.video_block.video_block.VideoBlock.get_course_video_sharing_override') + def test_is_public_sharing_disabled_by_course_override(self, mock_course_sharing_override): + # Given a course overrides no videos to be shared + mock_course_sharing_override.return_value = COURSE_VIDEO_SHARING_NONE + self.block.public_access = 'some-arbitrary-value' + + # When I try to determine if public sharing is enabled + with self.mock_feature_toggle(): + is_public_sharing_enabled = self.block.is_public_sharing_enabled() + + # Then I will get that course value + self.assertFalse(is_public_sharing_enabled) + + @ddt.data(COURSE_VIDEO_SHARING_PER_VIDEO, None) + @patch('xmodule.video_block.video_block.VideoBlock.get_course_video_sharing_override') + def test_is_public_sharing_enabled_per_video(self, mock_override_value, mock_course_sharing_override): + # Given a course does not override per-video settings + mock_course_sharing_override.return_value = mock_override_value + self.block.public_access = 'some-arbitrary-value' + + # When I try to determine if public sharing is enabled + with self.mock_feature_toggle(): + is_public_sharing_enabled = self.block.is_public_sharing_enabled() + + # I will get the per-video value + self.assertEqual(self.block.public_access, is_public_sharing_enabled) + + @patch('xmodule.video_block.video_block.get_course_by_id') + def test_is_public_sharing_course_not_found(self, mock_get_course): + # Given a course does not override per-video settings + mock_get_course.side_effect = Http404() + self.block.public_access = 'some-arbitrary-value' + + # When I try to determine if public sharing is enabled + with self.mock_feature_toggle(): + is_public_sharing_enabled = self.block.is_public_sharing_enabled() + + # I will fall-back to per-video values + self.assertEqual(self.block.public_access, is_public_sharing_enabled) + + @ddt.data(False, True) + def test_context(self, is_public_sharing_enabled): + with self.mock_feature_toggle(): + with patch.object( + self.block, + 'is_public_sharing_enabled', + return_value=is_public_sharing_enabled + ): + content = self.block.render(STUDENT_VIEW).content + context = get_context_dict_from_string(content) + assert ('public_sharing_enabled' in context) == is_public_sharing_enabled + assert ('public_video_url' in context) == is_public_sharing_enabled @ddt.ddt @@ -389,7 +470,6 @@ class TestGetHtmlMethod(BaseTestVideoXBlock): {'display_name': 'Text (.txt) file', 'value': 'txt'} ], 'poster': 'null', - 'public_video_url': None, } for data in cases: @@ -510,7 +590,6 @@ class TestGetHtmlMethod(BaseTestVideoXBlock): {'display_name': 'Text (.txt) file', 'value': 'txt'} ], 'poster': 'null', - 'public_video_url': None, } initial_context['metadata']['duration'] = None @@ -637,8 +716,7 @@ class TestGetHtmlMethod(BaseTestVideoXBlock): {'display_name': 'Text (.txt) file', 'value': 'txt'} ], 'poster': 'null', - 'public_video_url': None, - 'metadata': metadata + 'metadata': metadata, } DATA = SOURCE_XML.format( # lint-amnesty, pylint: disable=invalid-name @@ -811,7 +889,6 @@ class TestGetHtmlMethod(BaseTestVideoXBlock): {'display_name': 'Text (.txt) file', 'value': 'txt'} ], 'poster': 'null', - 'public_video_url': None, 'metadata': metadata, } @@ -925,7 +1002,6 @@ class TestGetHtmlMethod(BaseTestVideoXBlock): {'display_name': 'SubRip (.srt) file', 'value': 'srt'}, {'display_name': 'Text (.txt) file', 'value': 'txt'} ], - 'public_video_url': None, 'poster': 'null', } initial_context['metadata']['duration'] = None @@ -1022,7 +1098,6 @@ class TestGetHtmlMethod(BaseTestVideoXBlock): {'display_name': 'SubRip (.srt) file', 'value': 'srt'}, {'display_name': 'Text (.txt) file', 'value': 'txt'} ], - 'public_video_url': None, 'poster': 'null', } initial_context['metadata']['duration'] = None @@ -2309,7 +2384,6 @@ class TestVideoWithBumper(TestVideo): # pylint: disable=test-inherits-tests {'display_name': 'SubRip (.srt) file', 'value': 'srt'}, {'display_name': 'Text (.txt) file', 'value': 'txt'} ], - 'public_video_url': None, 'poster': json.dumps(OrderedDict({ 'url': 'http://img.youtube.com/vi/ZwkTiUPN0mg/0.jpg', 'type': 'youtube' @@ -2391,7 +2465,6 @@ class TestAutoAdvanceVideo(TestVideo): # lint-amnesty, pylint: disable=test-inh {'display_name': 'SubRip (.srt) file', 'value': 'srt'}, {'display_name': 'Text (.txt) file', 'value': 'txt'} ], - 'public_video_url': None, 'poster': 'null' } return context diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py index 08496ae1cb..78731c5654 100644 --- a/lms/djangoapps/courseware/views/views.py +++ b/lms/djangoapps/courseware/views/views.py @@ -1772,7 +1772,7 @@ class BasePublicVideoXBlockView(View): ) # Block must be marked as public to be viewed - if not video_block.public_access: + if not video_block.is_public_sharing_enabled(): raise Http404("Video not found.") return course, video_block diff --git a/xmodule/video_block/video_block.py b/xmodule/video_block/video_block.py index 46a0fc40ca..4f3bdac56d 100644 --- a/xmodule/video_block/video_block.py +++ b/xmodule/video_block/video_block.py @@ -33,10 +33,16 @@ from xblock.runtime import KvsFieldData from common.djangoapps.xblock_django.constants import ATTR_KEY_REQUEST_COUNTRY_CODE from openedx.core.djangoapps.video_config.models import HLSPlaybackEnabledFlag, CourseYoutubeBlockedFlag +from openedx.core.djangoapps.video_config.toggles import PUBLIC_VIDEO_SHARE from openedx.core.djangoapps.video_pipeline.config.waffle import DEPRECATE_YOUTUBE from openedx.core.lib.cache_utils import request_cached +from openedx.core.lib.courses import get_course_by_id from openedx.core.lib.license import LicenseMixin from xmodule.contentstore.content import StaticContent +from xmodule.course_block import ( + COURSE_VIDEO_SHARING_ALL_VIDEOS, + COURSE_VIDEO_SHARING_NONE, +) from xmodule.editing_block import EditingMixin from xmodule.exceptions import NotFoundError from xmodule.mako_block import MakoTemplateBlockBase @@ -472,26 +478,72 @@ class VideoBlock( 'track': track_url, 'transcript_download_format': transcript_download_format, 'transcript_download_formats_list': self.fields['transcript_download_format'].values, # lint-amnesty, pylint: disable=unsubscriptable-object - # provide the video url iif the video is public and we are in LMS. - # Reverse render_public_video_xblock is not available in studio. - 'public_video_url': self._get_public_video_url(), } + # Public video previewing / social media sharing + if self.is_public_sharing_enabled(): + template_context['public_sharing_enabled'] = True + template_context['public_video_url'] = self.get_public_video_url() + return self.runtime.service(self, 'mako').render_template('video.html', template_context) - def _get_public_video_url(self): + def get_course_video_sharing_override(self): + """ + Return course video sharing options override or None + """ + try: + course = get_course_by_id(self.course_id) + return getattr(course, 'video_sharing_options', None) + + # In case the course / modulestore does something weird + except Exception as err: # pylint: disable=broad-except + log.exception(f"Error retrieving course for course ID: {self.course_id}") + return None + + def is_public_sharing_enabled(self): + """ + Is public sharing enabled for this video? + """ + + # Sharing is DISABLED from studio + is_studio = getattr(self.runtime, "is_author_mode", False) + if is_studio: + return False + + # Video share feature must be enabled for sharing settings to take effect + feature_enabled = PUBLIC_VIDEO_SHARE.is_enabled(self.location.course_key) + if not feature_enabled: + return False + + # Check if the course specifies a general setting + course_video_sharing_option = self.get_course_video_sharing_override() + + # Course can override all videos to be shared + if course_video_sharing_option == COURSE_VIDEO_SHARING_ALL_VIDEOS: + return True + + # ... or no videos to be shared + elif course_video_sharing_option == COURSE_VIDEO_SHARING_NONE: + return False + + # ... or can fall back to per-video setting + # Equivalent to COURSE_VIDEO_SHARING_PER_VIDEO or None / unset + else: + return self.public_access + + def get_public_video_url(self): """ Returns the public video url """ - is_studio = getattr(self.runtime, "is_author_mode", False) - if self.public_access and not is_studio: - from openedx.core.djangoapps.video_config.toggles import PUBLIC_VIDEO_SHARE - if PUBLIC_VIDEO_SHARE.is_enabled(self.location.course_key): - return urljoin( - settings.LMS_ROOT_URL, - reverse('render_public_video_xblock', kwargs={'usage_key_string': str(self.location)}) - ) - return None + return urljoin( + settings.LMS_ROOT_URL, + reverse( + 'render_public_video_xblock', + kwargs={ + 'usage_key_string': str(self.location) + } + ) + ) def validate(self): """