Skip to content

basic tab attach uploads with edx_video_id #18032

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
muhammad-ammar merged 2 commits into from
May 3, 2018
Merged

basic tab attach uploads with edx_video_id #18032

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1c53360
basic tab attach uploads with edx_video_id
muhammad-ammar Apr 24, 2018
84ab9ca
address review and add test
muhammad-ammar Apr 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 53 additions & 8 deletions cms/djangoapps/contentstore/views/tests/test_transcripts.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,14 +195,17 @@ def clean_temporary_transcripts(self):
self.bad_name_srt_file.close()
self.bom_srt_file.close()

def upload_transcript(self, locator, transcript_file):
def upload_transcript(self, locator, transcript_file, edx_video_id=None):
"""
Uploads a transcript for a video
"""
payload = {}
if locator:
payload.update({'locator': locator})

if edx_video_id is not None:
payload.update({'edx_video_id': edx_video_id})

if transcript_file:
payload.update({'transcript-file': transcript_file})

Expand Down Expand Up @@ -233,7 +236,7 @@ def test_transcript_upload_success(self, edx_video_id, include_bom):

# Upload a transcript
transcript_file = self.bom_srt_file if include_bom else self.good_srt_file
response = self.upload_transcript(self.video_usage_key, transcript_file)
response = self.upload_transcript(self.video_usage_key, transcript_file, '')

# Verify the response
self.assert_response(response, expected_status_code=200, expected_message='Success')
Expand All @@ -258,7 +261,7 @@ def test_transcript_upload_without_locator(self):
"""
Test that transcript upload validation fails if the video locator is missing
"""
response = self.upload_transcript(locator=None, transcript_file=self.good_srt_file)
response = self.upload_transcript(locator=None, transcript_file=self.good_srt_file, edx_video_id='')
self.assert_response(
response,
expected_status_code=400,
Expand All @@ -269,7 +272,7 @@ def test_transcript_upload_without_file(self):
"""
Test that transcript upload validation fails if transcript file is missing
"""
response = self.upload_transcript(locator=self.video_usage_key, transcript_file=None)
response = self.upload_transcript(locator=self.video_usage_key, transcript_file=None, edx_video_id='')
self.assert_response(
response,
expected_status_code=400,
Expand All @@ -280,7 +283,11 @@ def test_transcript_upload_bad_format(self):
"""
Test that transcript upload validation fails if transcript format is not SRT
"""
response = self.upload_transcript(locator=self.video_usage_key, transcript_file=self.bad_name_srt_file)
response = self.upload_transcript(
locator=self.video_usage_key,
transcript_file=self.bad_name_srt_file,
edx_video_id=''
)
self.assert_response(
response,
expected_status_code=400,
Expand All @@ -292,7 +299,11 @@ def test_transcript_upload_bad_content(self):
Test that transcript upload validation fails in case of bad transcript content.
"""
# Request to upload transcript for the video
response = self.upload_transcript(locator=self.video_usage_key, transcript_file=self.bad_data_srt_file)
response = self.upload_transcript(
locator=self.video_usage_key,
transcript_file=self.bad_data_srt_file,
edx_video_id=''
)
self.assert_response(
response,
expected_status_code=400,
Expand All @@ -306,7 +317,7 @@ def test_transcript_upload_unknown_category(self):
# non_video module setup - i.e. an item whose category is not 'video'.
usage_key = self.create_non_video_module()
# Request to upload transcript for the item
response = self.upload_transcript(locator=usage_key, transcript_file=self.good_srt_file)
response = self.upload_transcript(locator=usage_key, transcript_file=self.good_srt_file, edx_video_id='')
self.assert_response(
response,
expected_status_code=400,
Expand All @@ -318,13 +329,47 @@ def test_transcript_upload_non_existent_item(self):
Test that transcript upload validation fails in case of invalid item's locator.
"""
# Request to upload transcript for the item
response = self.upload_transcript(locator='non_existent_locator', transcript_file=self.good_srt_file)
response = self.upload_transcript(
locator='non_existent_locator',
transcript_file=self.good_srt_file,
edx_video_id=''
)
self.assert_response(
response,
expected_status_code=400,
expected_message=u'Cannot find item by locator.'
)

def test_transcript_upload_without_edx_video_id(self):
"""
Test that transcript upload validation fails if the `edx_video_id` is missing
"""
response = self.upload_transcript(locator=self.video_usage_key, transcript_file=self.good_srt_file)
self.assert_response(
response,
expected_status_code=400,
expected_message=u'Video ID is required.'
)

def test_transcript_upload_with_non_existant_edx_video_id(self):
"""
Test that transcript upload works as expected if `edx_video_id` set on
video descriptor is different from `edx_video_id` received in POST request.
"""
non_existant_edx_video_id = '1111-2222-3333-4444'

# Upload with non-existant `edx_video_id`
response = self.upload_transcript(
locator=self.video_usage_key,
transcript_file=self.good_srt_file,
edx_video_id=non_existant_edx_video_id
)
# Verify the response
self.assert_response(response, expected_status_code=400, expected_message='Invalid Video ID')
Copy link
Contributor

@mushtaqak mushtaqak Apr 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍


# Verify transcript does not exist for non-existant `edx_video_id`
self.assertIsNone(get_video_transcript_content(non_existant_edx_video_id, language_code=u'en'))


@ddt.ddt
class TestChooseTranscripts(BaseTranscripts):
Expand Down
13 changes: 10 additions & 3 deletions cms/djangoapps/contentstore/views/transcripts_ajax.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,19 +169,22 @@ def validate_transcript_upload_data(request):
error, validated_data = None, {}
data, files = request.POST, request.FILES
video_locator = data.get('locator')
edx_video_id = data.get('edx_video_id')
if not video_locator:
error = _(u'Video locator is required.')
elif 'transcript-file' not in files:
error = _(u'A transcript file is required.')
elif os.path.splitext(files['transcript-file'].name)[1][1:] != Transcript.SRT:
error = _(u'This transcript file type is not supported.')
elif 'edx_video_id' not in data:
error = _(u'Video ID is required.')

if not error:
error, video = validate_video_module(request, video_locator)
if not error:
validated_data.update({
'video': video,
'edx_video_id': clean_video_id(video.edx_video_id),
'edx_video_id': clean_video_id(edx_video_id) or clean_video_id(video.edx_video_id),
Copy link
Contributor

@mushtaqak mushtaqak Apr 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have success scenario test written for edx_video_id coming from video.edx_video_id but not from the request data.

'transcript_file': files['transcript-file']
})

Expand Down Expand Up @@ -212,6 +215,8 @@ def upload_transcripts(request):
video.edx_video_id = edx_video_id
video.save_with_metadata(request.user)

response = JsonResponse({'edx_video_id': edx_video_id, 'status': 'Success'}, status=200)

try:
# Convert 'srt' transcript into the 'sjson' and upload it to
# configured transcript storage. For example, S3.
Expand All @@ -220,7 +225,7 @@ def upload_transcripts(request):
input_format=Transcript.SRT,
output_format=Transcript.SJSON
)
create_or_update_video_transcript(
transcript_created = create_or_update_video_transcript(
video_id=edx_video_id,
language_code=u'en',
metadata={
Expand All @@ -230,7 +235,9 @@ def upload_transcripts(request):
},
file_data=ContentFile(sjson_subs),
)
response = JsonResponse({'edx_video_id': edx_video_id, 'status': 'Success'}, status=200)

if transcript_created is None:
response = JsonResponse({'status': 'Invalid Video ID'}, status=400)

except (TranscriptsGenerationException, UnicodeDecodeError):

Expand Down
11 changes: 10 additions & 1 deletion cms/static/js/spec/video/transcripts/file_uploader_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ define(
'js/views/video/transcripts/utils', 'js/views/video/transcripts/file_uploader',
'xmodule', 'jquery.form'
],
function($, _, Backbone, Utils, FileUploader) {
function($, _, Backbone, TranscriptUtils, FileUploader) {
'use strict';

describe('Transcripts.FileUploader', function() {
Expand Down Expand Up @@ -95,6 +95,12 @@ function($, _, Backbone, Utils, FileUploader) {
});

describe('Upload', function() {
var videoId = '123-456-789-0';

beforeEach(function() {
TranscriptUtils.Storage.set('edx_video_id', videoId);
});

it('File is not chosen', function() {
spyOn($.fn, 'ajaxSubmit');
view.upload();
Expand All @@ -109,6 +115,9 @@ function($, _, Backbone, Utils, FileUploader) {
view.upload();

expect(view.$form.ajaxSubmit).toHaveBeenCalled();
expect(view.$form.ajaxSubmit).toHaveBeenCalledWith(jasmine.objectContaining({
data: {'edx_video_id': videoId}
}));
});
});

Expand Down
18 changes: 11 additions & 7 deletions cms/static/js/spec/video/transcripts/videolist_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ function($, _, Backbone, AjaxHelpers, Utils, Editor, VideoList, MetadataModel, A
// create mock server
this.mockServer = createMockAjaxServer();

spyOn($.fn, 'on').and.callThrough();
spyOn(Backbone, 'trigger').and.callThrough();
spyOn(Utils, 'command').and.callThrough();
spyOn(abstractEditor, 'initialize').and.callThrough();
Expand Down Expand Up @@ -215,14 +216,17 @@ function($, _, Backbone, AjaxHelpers, Utils, Editor, VideoList, MetadataModel, A


it('Initialize', function(done) {
var view = createVideoListView(this.mockServer);
var view = createVideoListView(this.mockServer), callArgs;
waitsForResponse(this.mockServer)
.then(function() {
expect(abstractEditor.initialize).toHaveBeenCalled();
expect(MessageManager.prototype.initialize).toHaveBeenCalled();
expect(view.component_locator).toBe(component_locator);
expect(view.$el).toHandle('input');
}).always(done);
.then(function() {
expect(abstractEditor.initialize).toHaveBeenCalled();
expect(MessageManager.prototype.initialize).toHaveBeenCalled();
expect(view.component_locator).toBe(component_locator);
expect(view.$el).toHandle('input');
callArgs = view.$el.on.calls.mostRecent().args;
expect(callArgs[0]).toEqual('input');
expect(callArgs[1]).toEqual('.videolist-settings-item input');
}).always(done);
});

describe('Render', function() {
Expand Down
4 changes: 3 additions & 1 deletion cms/static/js/spec/views/metadata_edit_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/
define(["underscore", "js/models/metadata", "js/collections/metadata", "js/views/metadata", "cms/js/main",
"js/views/video/transcripts/utils", 'edx-ui-toolkit/js/utils/spec-helpers/ajax-helpers'],
function(_, MetadataModel, MetadataCollection, MetadataView, main, Utils, AjaxHelpers) {
function(_, MetadataModel, MetadataCollection, MetadataView, main, TranscriptUtils, AjaxHelpers) {
const verifyInputType = function(input, expectedType) {
// Some browsers (e.g. FireFox) do not support the "number"
// input type. We can accept a "text" input instead
Expand Down Expand Up @@ -285,8 +285,10 @@ function(_, MetadataModel, MetadataCollection, MetadataView, main, Utils, AjaxHe

beforeEach(function() {
const model = new MetadataModel(videoIDEntry);
spyOn(TranscriptUtils.Storage, 'set');
this.view = new MetadataView.VideoID({model});
spyOn(Backbone, 'trigger');
expect(TranscriptUtils.Storage.set).toHaveBeenCalledWith('edx_video_id', this.view.getValueFromEditor());
});

it("triggers correct event on input change", function(done) {
Expand Down
18 changes: 14 additions & 4 deletions cms/static/js/spec/views/upload_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
define(["sinon", "js/models/uploads", "js/views/uploads", "js/models/chapter",
define(["underscore", "sinon", "js/models/uploads", "js/views/uploads", "js/models/chapter",
"edx-ui-toolkit/js/utils/spec-helpers/ajax-helpers", "js/spec_helpers/modal_helpers"],
(sinon, FileUpload, UploadDialog, Chapter, AjaxHelpers, modal_helpers) =>
(_, sinon, FileUpload, UploadDialog, Chapter, AjaxHelpers, modal_helpers) =>

describe("UploadDialog", function() {
const tpl = readFixtures("upload-dialog.underscore");
const tpl = readFixtures("upload-dialog.underscore"),
uploadData = {
edx_video_id: '123-456-789-0',
language_code: 'en',
new_language_code: 'ur'
};

beforeEach(function() {
let dialogResponse;
Expand All @@ -27,7 +32,8 @@ define(["sinon", "js/models/uploads", "js/views/uploads", "js/models/chapter",
url: CMS.URL.UPLOAD_ASSET,
onSuccess: response => {
return test.dialogResponse.push(response.response);
}
},
uploadData: uploadData
});
spyOn(view, 'remove').and.callThrough();

Expand All @@ -37,6 +43,7 @@ define(["sinon", "js/models/uploads", "js/views/uploads", "js/models/chapter",
const jqMockFileInput = jasmine.createSpyObj('jqMockFileInput', ['get', 'replaceWith']);
jqMockFileInput.get.and.returnValue(mockFileInput);
const originalView$ = view.$;
spyOn($.fn, 'ajaxSubmit').and.callThrough();
spyOn(view, "$").and.callFake(function(selector) {
if (selector === "input[type=file]") {
return jqMockFileInput;
Expand Down Expand Up @@ -126,6 +133,9 @@ define(["sinon", "js/models/uploads", "js/views/uploads", "js/models/chapter",
view.upload();
expect(this.model.get("uploading")).toBeTruthy();
AjaxHelpers.expectRequest(requests, "POST", "/upload");
expect($.fn.ajaxSubmit.calls.mostRecent().args[0].data).toEqual(
_.extend({}, uploadData, {notifyOnError: false})
);
AjaxHelpers.respondWithJson(requests, { response: "dummy_response"});
expect(this.model.get("uploading")).toBeFalsy();
expect(this.model.get("finished")).toBeTruthy();
Expand Down
9 changes: 8 additions & 1 deletion cms/static/js/views/metadata.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@ define(
'js/views/baseview', 'underscore', 'js/models/metadata', 'js/views/abstract_editor',
'js/models/uploads', 'js/views/uploads',
'js/models/license', 'js/views/license',
'js/views/video/transcripts/utils',
'js/views/video/transcripts/metadata_videolist',
'js/views/video/translations_editor'
],
function(Backbone, BaseView, _, MetadataModel, AbstractEditor, FileUpload, UploadDialog,
LicenseModel, LicenseView, VideoList, VideoTranslations) {
LicenseModel, LicenseView, TranscriptUtils, VideoList, VideoTranslations) {
'use strict';
var Metadata = {};

Expand Down Expand Up @@ -138,6 +139,11 @@ function(Backbone, BaseView, _, MetadataModel, AbstractEditor, FileUpload, Uploa
);
},

render: function() {
Metadata.String.prototype.render.apply(this, arguments);
TranscriptUtils.Storage.set('edx_video_id', this.getValueFromEditor());
},

clear: function() {
this.model.setValue('');
this.inputChange();
Expand All @@ -148,6 +154,7 @@ function(Backbone, BaseView, _, MetadataModel, AbstractEditor, FileUpload, Uploa
},

inputChange: function() {
TranscriptUtils.Storage.set('edx_video_id', this.getValueFromEditor());
Backbone.trigger('transcripts:basicTabFieldChanged');
}
});
Expand Down
9 changes: 7 additions & 2 deletions cms/static/js/views/video/transcripts/file_uploader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ define(
'jquery', 'backbone', 'underscore',
'js/views/video/transcripts/utils'
],
function($, Backbone, _, Utils) {
function($, Backbone, _, TranscriptUtils) {
Copy link
Contributor

@mushtaqak mushtaqak Apr 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't previously being used ?

Copy link
Contributor Author

@muhammad-ammar muhammad-ammar Apr 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

var FileUploader = Backbone.View.extend({
invisibleClass: 'is-invisible',

Expand Down Expand Up @@ -57,14 +57,19 @@ function($, Backbone, _, Utils) {
*
*/
upload: function() {
var data = {
'edx_video_id': TranscriptUtils.Storage.get('edx_video_id') || ''
};

if (!this.file) {
return;
}

this.$form.ajaxSubmit({
beforeSend: this.xhrResetProgressBar,
uploadProgress: this.xhrProgressHandler,
complete: this.xhrCompleteHandler
complete: this.xhrCompleteHandler,
data: data
});
},

Expand Down
2 changes: 1 addition & 1 deletion cms/static/js/views/video/transcripts/metadata_videolist.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ function($, Backbone, _, AbstractEditor, Utils, MessageManager) {
.apply(this, arguments);

this.$el.on(
'input', 'input',
'input', '.videolist-settings-item input',
_.debounce(_.bind(this.inputHandler, this), this.inputDelay)
);

Expand Down
Loading