Skip to content

E
edx-platform
Commit dd5ac4cf by cahrens Committed by John Jarvis
Options

Change video transcripts to use locators instead of locations. 

Part of STUD-870
parent e0f2ece2
Showing with 90 additions and 98 deletions
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">
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment