Removing CDN RPC

CR comments

Whitespace

wip test refactoring

more test refactoring

Re-write download URL too.

Test fixes related to download change, and fix

wip, re-locating tests

update classname

Quality fixes

common/lib/xmodule/xmodule/tests/test_video.py

@@ -28,7 +28,7 @@ from xblock.field_data import DictFieldData
 from xblock.fields import ScopeIds
 
 from xmodule.tests import get_test_descriptor_system
-from xmodule.video_module import VideoDescriptor, create_youtube_string, get_video_from_cdn
+from xmodule.video_module import VideoDescriptor, create_youtube_string
 from xmodule.video_module.transcripts_utils import download_youtube_subs, save_to_store
 from . import LogicTest
 from .test_import import DummySystem
@@ -742,36 +742,6 @@ class VideoExportTestCase(VideoDescriptorTestBase):
         self.assertEquals(expected, etree.tostring(xml, pretty_print=True))
 
 
-class VideoCdnTest(unittest.TestCase):
-    """
-    Tests for Video CDN.
-    """
-    @patch('requests.get')
-    def test_get_video_success(self, cdn_response):
-        """
-        Test successful CDN request.
-        """
-        original_video_url = "http://www.original_video.com/original_video.mp4"
-        cdn_response_video_url = "http://www.cdn_video.com/cdn_video.mp4"
-        cdn_response_content = '{{"sources":["{cdn_url}"]}}'.format(cdn_url=cdn_response_video_url)
-        cdn_response.return_value = Mock(status_code=200, content=cdn_response_content)
-        fake_cdn_url = 'http://fake_cdn.com/'
-        self.assertEqual(
-            get_video_from_cdn(fake_cdn_url, original_video_url),
-            cdn_response_video_url
-        )
-
-    @patch('requests.get')
-    def test_get_no_video_exists(self, cdn_response):
-        """
-        Test if no alternative video in CDN exists.
-        """
-        original_video_url = "http://www.original_video.com/original_video.mp4"
-        cdn_response.return_value = Mock(status_code=404)
-        fake_cdn_url = 'http://fake_cdn.com/'
-        self.assertIsNone(get_video_from_cdn(fake_cdn_url, original_video_url))
-
-
 class VideoDescriptorIndexingTestCase(unittest.TestCase):
     """
     Make sure that VideoDescriptor can format data for indexing as expected.

common/lib/xmodule/xmodule/video_module/video_module.py

@@ -38,7 +38,7 @@ from xmodule.exceptions import NotFoundError
 from xmodule.contentstore.content import StaticContent
 
 from .transcripts_utils import VideoTranscriptsMixin, Transcript, get_html5_ids
-from .video_utils import create_youtube_string, get_video_from_cdn, get_poster
+from .video_utils import create_youtube_string, get_poster, rewrite_video_url
 from .bumper_utils import bumperize
 from .video_xfields import VideoFields
 from .video_handlers import VideoStudentViewHandlers, VideoStudioViewHandlers
@@ -196,6 +196,11 @@ class VideoModule(VideoFields, VideoTranscriptsMixin, VideoStudentViewHandlers,
         branding_info = None
         youtube_streams = ""
 
+        # Determine if there is an alternative source for this video
+        # based on user locale.  This exists to support cases where
+        # we leverage a geography specific CDN, like China.
+        cdn_url = getattr(settings, 'VIDEO_CDN_URL', {}).get(self.system.user_location)
+
         # If we have an edx_video_id, we prefer its values over what we store
         # internally for download links (source, html5_sources) and the youtube
         # stream.
@@ -215,7 +220,12 @@ class VideoModule(VideoFields, VideoTranscriptsMixin, VideoStudentViewHandlers,
                         if url not in sources:
                             sources.append(url)
                         if self.download_video:
-                            download_video_link = url
+                            # function returns None when the url cannot be re-written
+                            rewritten_link = rewrite_video_url(cdn_url, url)
+                            if rewritten_link:
+                                download_video_link = rewritten_link
+                            else:
+                                download_video_link = url
 
                 # set the youtube url
                 if val_video_urls["youtube"]:
@@ -231,12 +241,11 @@ class VideoModule(VideoFields, VideoTranscriptsMixin, VideoStudentViewHandlers,
         # 'CN' is China ISO 3166-1 country code.
         # Video caching is disabled for Studio. User_location is always None in Studio.
         # CountryMiddleware disabled for Studio.
-        cdn_url = getattr(settings, 'VIDEO_CDN_URL', {}).get(self.system.user_location)
         if getattr(self, 'video_speed_optimizations', True) and cdn_url:
             branding_info = BrandingInfoConfig.get_config().get(self.system.user_location)
 
             for index, source_url in enumerate(sources):
-                new_url = get_video_from_cdn(cdn_url, source_url)
+                new_url = rewrite_video_url(cdn_url, source_url)
                 if new_url:
                     sources[index] = new_url
 

common/lib/xmodule/xmodule/video_module/video_utils.py

@@ -8,9 +8,11 @@ import logging
 import urllib
 import requests
 from urllib import urlencode
-from urlparse import parse_qs, urlsplit, urlunsplit
+from urlparse import parse_qs, urlsplit, urlunsplit, urlparse
 
 from django.conf import settings
+from django.core.validators import URLValidator
+from django.core.exceptions import ValidationError
 
 from requests.exceptions import RequestException
 
@@ -39,43 +41,41 @@ def create_youtube_string(module):
     ])
 
 
-# def get_video_from_cdn(cdn_base_url, original_video_url, cdn_branding_logo_url):
-# Not sure if this third variable is necessary...
-def get_video_from_cdn(cdn_base_url, original_video_url):
+def rewrite_video_url(cdn_base_url, original_video_url):
     """
-    Get video URL from CDN.
-
-    `original_video_url` is the existing video url.
-    Currently `cdn_base_url` equals 'http://api.xuetangx.com/edx/video?s3_url='
-    Example of CDN outcome:
-        {
-            "sources":
-                [
-                    "http://cm12.c110.play.bokecc.com/flvs/ca/QxcVl/u39EQbA0Ra-20.mp4",
-                    "http://bm1.42.play.bokecc.com/flvs/ca/QxcVl/u39EQbA0Ra-20.mp4"
-                ],
-            "s3_url": "http://s3.amazonaws.com/BESTech/CS169/download/CS169_v13_w5l2s3.mp4",
-        }
-    where `s3_url` is requested original video url and `sources` is the list of
-    alternative links.
+    Returns a re-written video URL for cases when an alternate source
+    has been configured and is selected using factors like
+    user location.
+
+    Re-write rules for country codes are specified via the
+    EDX_VIDEO_CDN_URLS configuration structure.
+
+    :param cdn_base_url: The scheme, hostname, port and any relevant path prefix for the alternate CDN,
+    for example: https://mirror.example.cn/edx
+    :param original_video_url: The canonical source for this video, for example:
+    https://cdn.example.com/edx-course-videos/VIDEO101/001.mp4
+    :return: The re-written URL
     """
 
-    if not cdn_base_url:
+    if (not cdn_base_url) or (not original_video_url):
         return None
 
-    request_url = cdn_base_url + urllib.quote(original_video_url)
+    parsed = urlparse(original_video_url)
+    # Contruction of the rewrite url is intentionally very flexible of input.
+    # For example, https://www.edx.org/ + /foo.html will be rewritten to
+    # https://www.edx.org/foo.html.
+    rewritten_url = cdn_base_url.rstrip("/") + "/" + parsed.path.lstrip("/")
+    validator = URLValidator()
 
     try:
-        cdn_response = requests.get(request_url, timeout=0.5)
-    except RequestException as err:
-        log.info("Request timed out to CDN server: %s", request_url, exc_info=True)
-        return None
-
-    if cdn_response.status_code == 200:
-        cdn_content = json.loads(cdn_response.content)
-        return cdn_content['sources'][0]
-    else:
-        return None
+        validator(rewritten_url)
+        return rewritten_url
+    except ValidationError:
+        log.warn("Invalid CDN rewrite URL encountered, %s", rewritten_url)
+
+    # Mimic the behavior of removed get_video_from_cdn in this regard and
+    # return None causing the caller to use the original URL.
+    return None
 
 
 def get_poster(video):

lms/djangoapps/courseware/tests/test_video_mongo.py

@@ -14,7 +14,7 @@ from django.conf import settings
 from django.test import TestCase
 from django.test.utils import override_settings
 
-from xmodule.video_module import VideoDescriptor, bumper_utils, video_utils
+from xmodule.video_module import VideoDescriptor, bumper_utils, video_utils, rewrite_video_url
 from xmodule.x_module import STUDENT_VIEW
 from xmodule.tests.test_video import VideoDescriptorTestBase, instantiate_descriptor
 from xmodule.tests.test_import import DummySystem
@@ -688,10 +688,10 @@ class TestGetHtmlMethod(BaseTestXmodule):
 
     # pylint: disable=invalid-name
     @patch('xmodule.video_module.video_module.BrandingInfoConfig')
-    @patch('xmodule.video_module.video_module.get_video_from_cdn')
+    @patch('xmodule.video_module.video_module.rewrite_video_url')
     def test_get_html_cdn_source(self, mocked_get_video, mock_BrandingInfoConfig):
         """
-        Test if sources got from CDN.
+        Test if sources got from CDN
         """
 
         mock_BrandingInfoConfig.get_config.return_value = {
@@ -704,8 +704,8 @@ class TestGetHtmlMethod(BaseTestXmodule):
 
         def side_effect(*args, **kwargs):
             cdn = {
-                'http://example.com/example.mp4': 'http://cdn_example.com/example.mp4',
-                'http://example.com/example.webm': 'http://cdn_example.com/example.webm',
+                'http://example.com/example.mp4': 'http://cdn-example.com/example.mp4',
+                'http://example.com/example.webm': 'http://cdn-example.com/example.webm',
             }
             return cdn.get(args[1])
 
@@ -733,8 +733,8 @@ class TestGetHtmlMethod(BaseTestXmodule):
             'result': {
                 'download_video_link': u'example_source.mp4',
                 'sources': [
-                    u'http://cdn_example.com/example.mp4',
-                    u'http://cdn_example.com/example.webm'
+                    u'http://cdn-example.com/example.mp4',
+                    u'http://cdn-example.com/example.webm'
                 ],
             },
         }
@@ -803,6 +803,63 @@ class TestGetHtmlMethod(BaseTestXmodule):
 
 
 @attr('shard_1')
+class TestVideoCDNRewriting(BaseTestXmodule):
+    """
+    Tests for Video CDN.
+    """
+
+    def setUp(self, *args, **kwargs):
+        super(TestVideoCDNRewriting, self).setUp(*args, **kwargs)
+        self.original_video_file = "original_video.mp4"
+        self.original_video_url = "http://www.originalvideo.com/" + self.original_video_file
+
+    @patch.dict("django.conf.settings.CDN_VIDEO_URLS",
+                {"CN": "https://chinacdn.cn/"})
+    def test_rewrite_video_url_success(self):
+        """
+        Test successful CDN request.
+        """
+        cdn_response_video_url = settings.CDN_VIDEO_URLS["CN"] + self.original_video_file
+
+        self.assertEqual(
+            rewrite_video_url(settings.CDN_VIDEO_URLS["CN"], self.original_video_url),
+            cdn_response_video_url
+        )
+
+    @patch.dict("django.conf.settings.CDN_VIDEO_URLS",
+                {"CN": "https://chinacdn.cn/"})
+    def test_rewrite_url_concat(self):
+        """
+        Test that written URLs are returned clean despite input
+        """
+        cdn_response_video_url = settings.CDN_VIDEO_URLS["CN"] + "original_video.mp4"
+
+        self.assertEqual(
+            rewrite_video_url(settings.CDN_VIDEO_URLS["CN"] + "///", self.original_video_url),
+            cdn_response_video_url
+        )
+
+    def test_rewrite_video_url_invalid_url(self):
+        """
+        Test if no alternative video in CDN exists.
+        """
+        invalid_cdn_url = 'http://http://fakecdn.com/'
+        self.assertIsNone(rewrite_video_url(invalid_cdn_url, self.original_video_url))
+
+    def test_none_args(self):
+        """
+        Ensure None args return None
+        """
+        self.assertIsNone(rewrite_video_url(None, None))
+
+    def test_emptystring_args(self):
+        """
+        Ensure emptyrstring args return None
+        """
+        self.assertIsNone(rewrite_video_url("", ""))
+
+
+@attr('shard_1')
 class TestVideoDescriptorInitialization(BaseTestXmodule):
     """
     Make sure that module initialization works correctly.