Merge pull request #1790 from edx/christina/transcripts

Change video transcripts to use locators instead of locations.

cms/djangoapps/contentstore/tests/test_contentstore.py

@@ -137,8 +137,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         locator = loc_mapper().translate_location(course.location.course_id, descriptor.location, False, True)
         resp = self.client.get_html(locator.url_reverse('unit'))
         self.assertEqual(resp.status_code, 200)
-        # TODO: uncomment when video transcripts no longer require IDs.
-        # _test_no_locations(self, resp)
+        _test_no_locations(self, resp)
 
         for expected in expected_types:
             self.assertIn(expected, resp.content)
@@ -1354,8 +1353,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
             unit_locator = loc_mapper().translate_location(course_id, descriptor.location, False, True)
             resp = self.client.get_html(unit_locator.url_reverse('unit'))
             self.assertEqual(resp.status_code, 200)
-            # TODO: uncomment when video transcripts no longer require IDs.
-            # _test_no_locations(self, resp)
+            _test_no_locations(self, resp)
 
 
 @override_settings(CONTENTSTORE=TEST_DATA_CONTENTSTORE, MODULESTORE=TEST_MODULESTORE)
@@ -1682,8 +1680,7 @@ class ContentStoreTest(ModuleStoreTestCase):
         unit_locator = loc_mapper().translate_location(loc.course_id, unit_location, False, True)
         resp = self.client.get_html(unit_locator.url_reverse('unit'))
         self.assertEqual(resp.status_code, 200)
-        # TODO: uncomment when video transcripts no longer require IDs.
-        # _test_no_locations(self, resp)
+        _test_no_locations(self, resp)
 
         def delete_item(category, name):
             """ Helper method for testing the deletion of an xblock item. """

cms/djangoapps/contentstore/views/component.py

@@ -223,13 +223,9 @@ def unit_handler(request, tag=None, course_id=None, branch=None, version_guid=No
             )
 
         components = [
-            [
-                # TODO: old location needed for video transcripts.
-                component.location.url(),
-                loc_mapper().translate_location(
-                    course.location.course_id, component.location, False, True
-                )
-            ]
+            loc_mapper().translate_location(
+                course.location.course_id, component.location, False, True
+            )
             for component
             in item.get_children()
         ]

cms/djangoapps/contentstore/views/transcripts_ajax.py

@@ -18,11 +18,12 @@ from django.conf import settings
 
 from xmodule.contentstore.content import StaticContent
 from xmodule.exceptions import NotFoundError
-from xmodule.modulestore.django import modulestore
+from xmodule.modulestore.django import modulestore, loc_mapper
 from xmodule.contentstore.django import contentstore
-from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError
+from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError, InsufficientSpecificationError
 
 from util.json_request import JsonResponse
+from xmodule.modulestore.locator import BlockUsageLocator
 
 from ..transcripts_utils import (
     generate_subs_from_source,
@@ -77,20 +78,14 @@ def upload_transcripts(request):
         'subs': '',
     }
 
-    item_location = request.POST.get('id')
-    if not item_location:
-        return error_response(response, 'POST data without "id" form data.')
+    locator = request.POST.get('locator')
+    if not locator:
+        return error_response(response, 'POST data without "locator" form data.')
 
-    # This is placed before has_access() to validate item_location,
-    # because has_access() raises InvalidLocationError if location is invalid.
     try:
-        item = modulestore().get_item(item_location)
-    except (ItemNotFoundError, InvalidLocationError):
-        return error_response(response, "Can't find item by location.")
-
-    # Check permissions for this user within this course.
-    if not has_access(request.user, item_location):
-        raise PermissionDenied()
+        item = _get_item(request, request.POST)
+    except (ItemNotFoundError, InvalidLocationError, InsufficientSpecificationError):
+        return error_response(response, "Can't find item by locator.")
 
     if 'file' not in request.FILES:
         return error_response(response, 'POST data without "file" form data.')
@@ -156,23 +151,17 @@ def download_transcripts(request):
 
     Raises Http404 if unsuccessful.
     """
-    item_location = request.GET.get('id')
-    if not item_location:
-        log.debug('GET data without "id" property.')
+    locator = request.GET.get('locator')
+    if not locator:
+        log.debug('GET data without "locator" property.')
         raise Http404
 
-    # This is placed before has_access() to validate item_location,
-    # because has_access() raises InvalidLocationError if location is invalid.
     try:
-        item = modulestore().get_item(item_location)
-    except (ItemNotFoundError, InvalidLocationError):
-        log.debug("Can't find item by location.")
+        item = _get_item(request, request.GET)
+    except (ItemNotFoundError, InvalidLocationError, InsufficientSpecificationError):
+        log.debug("Can't find item by locator.")
         raise Http404
 
-    # Check permissions for this user within this course.
-    if not has_access(request.user, item_location):
-        raise PermissionDenied()
-
     subs_id = request.GET.get('subs_id')
     if not subs_id:
         log.debug('GET data without "subs_id" property.')
@@ -240,7 +229,7 @@ def check_transcripts(request):
         'status': 'Error',
     }
     try:
-        __, videos, item = validate_transcripts_data(request)
+        __, videos, item = _validate_transcripts_data(request)
     except TranscriptsRequestValidationException as e:
         return error_response(transcripts_presence, e.message)
 
@@ -303,7 +292,7 @@ def check_transcripts(request):
         if len(html5_subs) == 2:  # check html5 transcripts for equality
             transcripts_presence['html5_equal'] = json.loads(html5_subs[0]) == json.loads(html5_subs[1])
 
-    command, subs_to_use = transcripts_logic(transcripts_presence, videos)
+    command, subs_to_use = _transcripts_logic(transcripts_presence, videos)
     transcripts_presence.update({
         'command': command,
         'subs': subs_to_use,
@@ -311,7 +300,7 @@ def check_transcripts(request):
     return JsonResponse(transcripts_presence)
 
 
-def transcripts_logic(transcripts_presence, videos):
+def _transcripts_logic(transcripts_presence, videos):
     """
     By `transcripts_presence` content, figure what show to user:
 
@@ -386,7 +375,7 @@ def choose_transcripts(request):
     }
 
     try:
-        data, videos, item = validate_transcripts_data(request)
+        data, videos, item = _validate_transcripts_data(request)
     except TranscriptsRequestValidationException as e:
         return error_response(response, e.message)
 
@@ -416,7 +405,7 @@ def replace_transcripts(request):
     response = {'status': 'Error', 'subs': ''}
 
     try:
-        __, videos, item = validate_transcripts_data(request)
+        __, videos, item = _validate_transcripts_data(request)
     except TranscriptsRequestValidationException as e:
         return error_response(response, e.message)
 
@@ -435,7 +424,7 @@ def replace_transcripts(request):
     return JsonResponse(response)
 
 
-def validate_transcripts_data(request):
+def _validate_transcripts_data(request):
     """
     Validates, that request contains all proper data for transcripts processing.
 
@@ -452,18 +441,10 @@ def validate_transcripts_data(request):
     if not data:
         raise TranscriptsRequestValidationException('Incoming video data is empty.')
 
-    item_location = data.get('id')
-
-    # This is placed before has_access() to validate item_location,
-    # because has_access() raises InvalidLocationError if location is invalid.
     try:
-        item = modulestore().get_item(item_location)
-    except (ItemNotFoundError, InvalidLocationError):
-        raise TranscriptsRequestValidationException("Can't find item by location.")
-
-    # Check permissions for this user within this course.
-    if not has_access(request.user, item_location):
-        raise PermissionDenied()
+        item = _get_item(request, data)
+    except (ItemNotFoundError, InvalidLocationError, InsufficientSpecificationError):
+        raise TranscriptsRequestValidationException("Can't find item by locator.")
 
     if item.category != 'video':
         raise TranscriptsRequestValidationException('Transcripts are supported only for "video" modules.')
@@ -492,7 +473,7 @@ def rename_transcripts(request):
     response = {'status': 'Error', 'subs': ''}
 
     try:
-        __, videos, item = validate_transcripts_data(request)
+        __, videos, item = _validate_transcripts_data(request)
     except TranscriptsRequestValidationException as e:
         return error_response(response, e.message)
 
@@ -525,11 +506,10 @@ def save_transcripts(request):
     if not data:
         return error_response(response, 'Incoming video data is empty.')
 
-    item_location = data.get('id')
     try:
-        item = modulestore().get_item(item_location)
-    except (ItemNotFoundError, InvalidLocationError):
-        return error_response(response, "Can't find item by location.")
+        item = _get_item(request, data)
+    except (ItemNotFoundError, InvalidLocationError, InsufficientSpecificationError):
+        return error_response(response, "Can't find item by locator.")
 
     metadata = data.get('metadata')
     if metadata is not None:
@@ -553,3 +533,24 @@ def save_transcripts(request):
         response['status'] = 'Success'
 
     return JsonResponse(response)
+
+
+def _get_item(request, data):
+    """
+    Obtains from 'data' the locator for an item.
+    Next, gets that item from the modulestore (allowing any errors to raise up).
+    Finally, verifies that the user has access to the item.
+
+    Returns the item.
+    """
+    locator = BlockUsageLocator(data.get('locator'))
+    old_location = loc_mapper().translate_locator_to_location(locator)
+
+    # This is placed before has_access() to validate the location,
+    # because has_access() raises InvalidLocationError if location is invalid.
+    item = modulestore().get_item(old_location)
+
+    if not has_access(request.user, locator):
+        raise PermissionDenied()
+
+    return item

cms/static/js/spec/transcripts/file_uploader_spec.js

@@ -48,7 +48,7 @@ function ($, _, Utils, FileUploader) {
                 el: $container,
                 messenger: messenger,
                 videoListObject: videoListObject,
-                component_id: 'component_id'
+                component_locator: 'component_locator'
             });
         });
 

cms/static/js/spec/transcripts/message_manager_spec.js

@@ -52,7 +52,7 @@ function ($, _, Utils, MessageManager, FileUploader, sinon) {
             view = new MessageManager({
                 el: $container,
                 parent: videoList,
-                component_id: 'component_id'
+                component_locator: 'component_locator'
             });
         });
 
@@ -60,7 +60,7 @@ function ($, _, Utils, MessageManager, FileUploader, sinon) {
             expect(fileUploader.initialize).toHaveBeenCalledWith({
                 el: view.$el,
                 messenger: view,
-                component_id: view.component_id,
+                component_locator: view.component_locator,
                 videoListObject: view.options.parent
             });
         });
@@ -215,7 +215,7 @@ function ($, _, Utils, MessageManager, FileUploader, sinon) {
                     function() {
                         expect(Utils.command).toHaveBeenCalledWith(
                             action,
-                            view.component_id,
+                            view.component_locator,
                             videoList,
                             void(0)
                         );
@@ -245,7 +245,7 @@ function ($, _, Utils, MessageManager, FileUploader, sinon) {
                     function () {
                         expect(Utils.command).toHaveBeenCalledWith(
                             action,
-                            view.component_id,
+                            view.component_locator,
                             videoList,
                             {
                                 html5_id: extraParamas
@@ -268,7 +268,7 @@ function ($, _, Utils, MessageManager, FileUploader, sinon) {
                     function () {
                         expect(Utils.command).toHaveBeenCalledWith(
                             action,
-                            view.component_id,
+                            view.component_locator,
                             videoList,
                             void(0)
                         );

cms/static/js/spec/transcripts/videolist_spec.js

@@ -11,7 +11,7 @@ function ($, _, Utils, VideoList, MetadataView, MetadataModel, AbstractEditor, s
                 'transcripts/metadata-videolist-entry.underscore'
             ),
             abstractEditor = AbstractEditor.prototype,
-            component_id = 'component_id',
+            component_locator = 'component_locator',
             videoList = [
                 {
                     mode: "youtube",
@@ -62,7 +62,7 @@ function ($, _, Utils, VideoList, MetadataView, MetadataModel, AbstractEditor, s
 
             var tpl = sandbox({
                     'class': 'component',
-                    'data-id': component_id
+                    'data-locator': component_locator
                 }),
                 model = new MetadataModel(modelStub),
                 videoList, $el;
@@ -157,7 +157,7 @@ function ($, _, Utils, VideoList, MetadataView, MetadataModel, AbstractEditor, s
             waitsForResponse(function () {
                 expect(abstractEditor.initialize).toHaveBeenCalled();
                 expect(messenger.initialize).toHaveBeenCalled();
-                expect(view.component_id).toBe(component_id);
+                expect(view.component_locator).toBe(component_locator);
                 expect(view.$el).toHandle('input');
             });
         });
@@ -167,7 +167,7 @@ function ($, _, Utils, VideoList, MetadataView, MetadataModel, AbstractEditor, s
                     expect(abstractEditor.render).toHaveBeenCalled();
                     expect(Utils.command).toHaveBeenCalledWith(
                         'check',
-                        component_id,
+                        component_locator,
                         videoList
                     );
 

cms/static/js/views/transcripts/editor.js

@@ -72,7 +72,7 @@ function($, Backbone, _, Utils, MetadataView, MetadataCollection) {
         syncBasicTab: function (metadataCollection, metadataView) {
             var result = [],
                 getField = Utils.getField,
-                component_id = this.$el.closest('.component').data('id'),
+                component_locator = this.$el.closest('.component').data('locator'),
                 subs = getField(metadataCollection, 'sub'),
                 values = {},
                 videoUrl, metadata, modifiedValues;
@@ -99,7 +99,7 @@ function($, Backbone, _, Utils, MetadataView, MetadataCollection) {
             if (isSubsModified) {
                 metadata = $.extend(true, {}, modifiedValues);
                 // Save module state
-                Utils.command('save', component_id, null, {
+                Utils.command('save', component_locator, null, {
                     metadata: metadata,
                     current_subs: _.pluck(
                         Utils.getVideoList(videoUrl.getDisplayValue()),
@@ -110,18 +110,16 @@ function($, Backbone, _, Utils, MetadataView, MetadataCollection) {
 
             // Get values from `Advanced` tab fields (`html5_sources`,
             // `youtube_id_1_0`) that should be synchronized.
-            html5Sources = getField(metadataCollection, 'html5_sources')
-                                    .getDisplayValue();
+            var html5Sources = getField(metadataCollection, 'html5_sources').getDisplayValue();
 
-            values.youtube = getField(metadataCollection, 'youtube_id_1_0')
-                                    .getDisplayValue();
+            values.youtube = getField(metadataCollection, 'youtube_id_1_0').getDisplayValue();
 
-            values.html5Sources = _.filter(html5Sources, function (value) {
-                var link = Utils.parseLink(value),
+            values.html5Sources = _.filter(html5Sources, function (value) {
+                var link = Utils.parseLink(value),
                     mode = link && link.mode;
 
-                return mode === 'html5' && mode;
-            });
+                return mode === 'html5' && mode;
+            });
 
 
             // The length of youtube video_id should be 11 characters.

cms/static/js/views/transcripts/file_uploader.js

@@ -39,7 +39,7 @@ function($, Backbone, _, Utils) {
 
                 tplContainer.html(this.template({
                     ext: this.validFileExtensions,
-                    component_id: this.options.component_id,
+                    component_locator: this.options.component_locator,
                     video_list: videoList
                 }));
 

cms/static/js/views/transcripts/message_manager.js

@@ -31,12 +31,12 @@ function($, Backbone, _, Utils, FileUploader, gettext) {
         initialize: function () {
             _.bindAll(this);
 
-            this.component_id = this.$el.closest('.component').data('id');
+            this.component_locator = this.$el.closest('.component').data('locator');
 
             this.fileUploader = new FileUploader({
                 el: this.$el,
                 messenger: this,
-                component_id: this.component_id,
+                component_locator: this.component_locator,
                 videoListObject: this.options.parent
             });
         },
@@ -76,7 +76,7 @@ function($, Backbone, _, Utils, FileUploader, gettext) {
             this.$el.find('.transcripts-status')
                 .removeClass('is-invisible')
                 .find(this.elClass).html(template({
-                    component_id: encodeURIComponent(this.component_id),
+                    component_locator: encodeURIComponent(this.component_locator),
                     html5_list: html5List,
                     grouped_list: groupedList,
                     subs_id: (params) ? params.subs: ''
@@ -204,7 +204,7 @@ function($, Backbone, _, Utils, FileUploader, gettext) {
         */
         processCommand: function (action, errorMessage, videoId) {
             var self = this,
-                component_id = this.component_id,
+                component_locator = this.component_locator,
                 videoList = this.options.parent.getVideoObjectsList(),
                 extraParam, xhr;
 
@@ -212,7 +212,7 @@ function($, Backbone, _, Utils, FileUploader, gettext) {
                 extraParam = { html5_id: videoId };
             }
 
-            xhr = Utils.command(action, component_id, videoList, extraParam)
+            xhr = Utils.command(action, component_locator, videoList, extraParam)
                 .done(function (resp) {
                         var sub = resp.subs;
 

cms/static/js/views/transcripts/metadata_videolist.js

@@ -46,7 +46,7 @@ function($, Backbone, _, AbstractEditor, Utils, MessageManager, MetadataView) {
                 _.debounce(_.bind(this.inputHandler, this), this.inputDelay)
             );
 
-            this.component_id = this.$el.closest('.component').data('id');
+            this.component_locator = this.$el.closest('.component').data('locator');
         },
 
         render: function () {
@@ -55,7 +55,7 @@ function($, Backbone, _, AbstractEditor, Utils, MessageManager, MetadataView) {
                 .apply(this, arguments);
 
             var self = this,
-                component_id =  this.$el.closest('.component').data('id'),
+                component_locator =  this.$el.closest('.component').data('locator'),
                 videoList = this.getVideoObjectsList(),
 
                 showServerError = function (response) {
@@ -82,7 +82,7 @@ function($, Backbone, _, AbstractEditor, Utils, MessageManager, MetadataView) {
             }
 
             // Check current state of Timed Transcripts.
-            Utils.command('check', component_id, videoList)
+            Utils.command('check', component_locator, videoList)
                 .done(function (resp) {
                     var params = resp,
                         len = videoList.length,

cms/static/js/views/transcripts/utils.js

@@ -295,7 +295,7 @@ define(["jquery", "underscore", "jquery.ajaxQueue"], function($, _) {
         *
         * @param {string} action Action that will be invoked on server. Is a part
         *                        of url.
-        * @param {string} component_id Id of component.
+        * @param {string} component_locator the locator of component.
         * @param {array} videoList List of object with information about inserted
         *                          urls.
         * @param {object} extraParams Extra parameters that can be send to the
@@ -314,7 +314,7 @@ define(["jquery", "underscore", "jquery.ajaxQueue"], function($, _) {
             // _command() function.
             var xhr = null;
 
-            return function (action, component_id, videoList, extraParams) {
+            return function (action, component_locator, videoList, extraParams) {
                 var params, data;
 
                 if (extraParams) {
@@ -326,7 +326,7 @@ define(["jquery", "underscore", "jquery.ajaxQueue"], function($, _) {
                 }
 
                 data = $.extend(
-                    { id: component_id },
+                    { locator: component_locator },
                     { videos: videoList },
                     params
                 );

cms/templates/js/transcripts/file-upload.underscore

@@ -5,6 +5,6 @@
                           method="post" enctype="multipart/form-data">
   <input type="file" class="file-input" name="file"
     accept="<%= _.map(ext, function(val){ return '.' + val; }).join(', ') %>">
-  <input type="hidden" name="id" value="<%= component_id %>">
+  <input type="hidden" name="locator" value="<%= component_locator %>">
   <input type="hidden" name="video_list" value='<%= JSON.stringify(video_list) %>'>
 </form>

cms/templates/js/transcripts/messages/transcripts-found.underscore

@@ -10,7 +10,7 @@
     <button class="action setting-upload" type="button" name="setting-upload" value="<%= gettext("Upload New Timed Transcript") %>" data-tooltip="<%= gettext("Upload New Timed Transcript") %>">
         <span><%= gettext("Upload New Timed Transcript") %></span>
     </button>
-    <a class="action setting-download" href="/transcripts/download?id=<%= component_id %>&subs_id=<%= subs_id %>" data-tooltip="<%= gettext("Download to Edit") %>">
+    <a class="action setting-download" href="/transcripts/download?locator=<%= component_locator %>&subs_id=<%= subs_id %>" data-tooltip="<%= gettext("Download to Edit") %>">
         <span><%= gettext("Download to Edit") %></span>
     </a>
 </div>

cms/templates/js/transcripts/messages/transcripts-uploaded.underscore

@@ -10,7 +10,7 @@
     <button class="action setting-upload" type="button" name="setting-upload" value="<%= gettext("Upload New Timed Transcript") %>" data-tooltip="<%= gettext("Upload New Timed Transcript") %>">
         <span><%= gettext("Upload New Timed Transcript") %></span>
     </button>
-    <a class="action setting-download" href="/transcripts/download?id=<%= component_id %>" data-tooltip="<%= gettext("Download to Edit") %>">
+    <a class="action setting-download" href="/transcripts/download?locator=<%= component_locator %>" data-tooltip="<%= gettext("Download to Edit") %>">
         <span><%= gettext("Download to Edit") %></span>
     </a>
 </div>

cms/templates/unit.html

@@ -48,8 +48,8 @@ require(["domReady!", "jquery", "js/models/module_info", "coffee/src/views/unit"
         <article class="unit-body window">
         <p class="unit-name-input"><label>${_("Display Name:")}</label><input type="text" value="${unit.display_name_with_default | h}" class="unit-display-name-input" /></p>
           <ol class="components">
-            % for id, locator in components:
-            <li class="component" data-locator="${locator}" data-id="${id}" />
+            % for locator in components:
+            <li class="component" data-locator="${locator}"/>
             % endfor
             <li class="new-component-item adding">
               <div class="new-component">