From 1bc1c5453ec9fb534bc250424eab8b482ee0a05c Mon Sep 17 00:00:00 2001 From: tan Date: Wed, 1 Jun 2016 21:07:54 +0530 Subject: [PATCH] MA-333 Added ability to refresh uploaded videos This adds ability to refresh the list of uploaded videos without refreshing the whole page. Added a refresh button that when clicked: - fetches a fresh list of uploaded files from the server - updates `PreviousVideoUploadListView` - removes the successfully completed uploads from `ActiveVideoUploadListView` - retains the ongoing or failed uploads in `ActiveVideoUploadListView` so that they can be monitored/retried The view can also be refreshed without user action, but I felt it may be less surprising to have a button instead. MA-333 update: auto-refresh list, fix test failure Changes: 1. Refresh of file list triggered by upload completion. Refresh button retained and label changed to "Refresh List". 2. Added `aria-live="polite"` to `.active-video-upload-list` and `.assets-table`. 3. Removed unused parameter `evt`. 4. Added self to the `AUTHORS` file. MA-333 update: added tests MA-333 update: removed refresh button MA-333 update: address review comments of @mushtaqak MA-333 update: simplify nested `_each` MA-333 update: rename viewRefresh to isViewRefresh MA-333 update: doc string for `clearSuccesful` MA-333 update: fix accessibility MA-333 update: update only successfully uploaded videos MA-333 update: use window.SR feature to notify screen readers that video upload was successful (@pomegranited) --- AUTHORS | 1 + cms/djangoapps/contentstore/views/videos.py | 2 +- cms/static/js/factories/videos_index.js | 68 +++++--- cms/static/js/models/active_video_upload.js | 1 + .../views/active_video_upload_list_spec.js | 159 +++++++++++------- .../js/views/active_video_upload_list.js | 45 ++++- 6 files changed, 184 insertions(+), 92 deletions(-) diff --git a/AUTHORS b/AUTHORS index a4aa63e807..dd0a183592 100644 --- a/AUTHORS +++ b/AUTHORS @@ -276,3 +276,4 @@ Kevin Kim Albert St. Aubin Jr. Casey Litton Jhony Avella +Tanmay Mohapatra diff --git a/cms/djangoapps/contentstore/views/videos.py b/cms/djangoapps/contentstore/views/videos.py index ec34567f9f..88a321e63b 100644 --- a/cms/djangoapps/contentstore/views/videos.py +++ b/cms/djangoapps/contentstore/views/videos.py @@ -336,7 +336,7 @@ def videos_post(course, request): "courses": [course.id] }) - resp_files.append({"file_name": file_name, "upload_url": upload_url}) + resp_files.append({"file_name": file_name, "upload_url": upload_url, "edx_video_id": edx_video_id}) return JsonResponse({"files": resp_files}, status=200) diff --git a/cms/static/js/factories/videos_index.js b/cms/static/js/factories/videos_index.js index 702c3e4437..780f4fcccb 100644 --- a/cms/static/js/factories/videos_index.js +++ b/cms/static/js/factories/videos_index.js @@ -1,29 +1,49 @@ -define( - ['jquery', 'backbone', 'js/views/active_video_upload_list', 'js/views/previous_video_upload_list'], - function($, Backbone, ActiveVideoUploadListView, PreviousVideoUploadListView) { - 'use strict'; - var VideosIndexFactory = function( - $contentWrapper, - postUrl, - encodingsDownloadUrl, - concurrentUploadLimit, - uploadButton, - previousUploads - ) { - var activeView = new ActiveVideoUploadListView({ +define([ + 'jquery', 'backbone', 'js/views/active_video_upload_list', + 'js/views/previous_video_upload_list', 'js/views/active_video_upload' +], function($, Backbone, ActiveVideoUploadListView, PreviousVideoUploadListView, ActiveVideoUpload) { + 'use strict'; + var VideosIndexFactory = function( + $contentWrapper, + postUrl, + encodingsDownloadUrl, + concurrentUploadLimit, + uploadButton, + previousUploads + ) { + var activeView = new ActiveVideoUploadListView({ postUrl: postUrl, concurrentUploadLimit: concurrentUploadLimit, - uploadButton: uploadButton - }); - $contentWrapper.append(activeView.render().$el); - var previousCollection = new Backbone.Collection(previousUploads); - var previousView = new PreviousVideoUploadListView({ - collection: previousCollection, + uploadButton: uploadButton, + onFileUploadDone: function(activeVideos) { + $.ajax({ + url: postUrl, + contentType: 'application/json', + dataType: 'json', + type: 'GET' + }).done(function(responseData) { + var updatedCollection = new Backbone.Collection(responseData.videos).filter(function(video) { + // Include videos that are not in the active video upload list, + // or that are marked as Upload Complete + var isActive = activeVideos.where({videoId: video.get('edx_video_id')}); + return isActive.length === 0 || + isActive[0].get('status') === ActiveVideoUpload.STATUS_COMPLETE; + }), + updatedView = new PreviousVideoUploadListView({ + collection: updatedCollection, + encodingsDownloadUrl: encodingsDownloadUrl + }); + $contentWrapper.find('.wrapper-assets').replaceWith(updatedView.render().$el); + }); + } + }), + previousView = new PreviousVideoUploadListView({ + collection: new Backbone.Collection(previousUploads), encodingsDownloadUrl: encodingsDownloadUrl }); - $contentWrapper.append(previousView.render().$el); - }; + $contentWrapper.append(activeView.render().$el); + $contentWrapper.append(previousView.render().$el); + }; - return VideosIndexFactory; - } -); + return VideosIndexFactory; +}); diff --git a/cms/static/js/models/active_video_upload.js b/cms/static/js/models/active_video_upload.js index 4a4f405e04..7252729c3f 100644 --- a/cms/static/js/models/active_video_upload.js +++ b/cms/static/js/models/active_video_upload.js @@ -19,6 +19,7 @@ define( var ActiveVideoUpload = Backbone.Model.extend( { defaults: { + videoId: null, status: statusStrings.STATUS_QUEUED, progress: 0 } diff --git a/cms/static/js/spec/views/active_video_upload_list_spec.js b/cms/static/js/spec/views/active_video_upload_list_spec.js index 603675ec96..fb08b88c4f 100644 --- a/cms/static/js/spec/views/active_video_upload_list_spec.js +++ b/cms/static/js/spec/views/active_video_upload_list_spec.js @@ -1,3 +1,4 @@ +/* global _ */ define( [ 'jquery', @@ -174,76 +175,104 @@ define( // 2.0, the latest version of jasmine-ajax (mock-ajax.js) does have the // necessary support. - _.each( - [ - { - desc: 'completion', - responseStatus: 204, - statusText: ActiveVideoUpload.STATUS_COMPLETED, - progressValue: 1, - presentClass: 'success', - absentClass: 'error' - }, - { - desc: 'failure', - responseStatus: 500, - statusText: ActiveVideoUpload.STATUS_FAILED, - progressValue: 0, - presentClass: 'error', - absentClass: 'success' - } - ], - function(subCaseInfo) { - describe('and upload ' + subCaseInfo.desc, function() { - beforeEach(function() { - getSentRequests()[0].respondWith({status: subCaseInfo.responseStatus}); - }); + _.each([true, false], + function(isViewRefresh) { + var refreshDescription = isViewRefresh ? ' (refreshed)' : ' (not refreshed)'; + var subCases = [ + { + desc: 'completion' + refreshDescription, + responseStatus: 204, + statusText: ActiveVideoUpload.STATUS_COMPLETED, + progressValue: 1, + presentClass: 'success', + absentClass: 'error', + isViewRefresh: isViewRefresh + }, + { + desc: 'failure' + refreshDescription, + responseStatus: 500, + statusText: ActiveVideoUpload.STATUS_FAILED, + progressValue: 0, + presentClass: 'error', + absentClass: 'success', + isViewRefresh: isViewRefresh + } + ]; - it('should update status and progress', function() { - var $uploadElem = this.view.$('.active-video-upload:first'); - expect($uploadElem.length).toEqual(1); - expect($.trim($uploadElem.find('.video-detail-status').text())).toEqual( - subCaseInfo.statusText - ); - expect( - $uploadElem.find('.video-detail-progress').val() - ).toEqual(subCaseInfo.progressValue); - expect($uploadElem).toHaveClass(subCaseInfo.presentClass); - expect($uploadElem).not.toHaveClass(subCaseInfo.absentClass); - }); + _.each(subCases, + function(subCaseInfo) { + describe('and upload ' + subCaseInfo.desc, function() { + var refreshSpy = null; - it('should not trigger the global AJAX error handler', function() { - expect(this.globalAjaxError).not.toHaveBeenCalled(); - }); + beforeEach(function() { + refreshSpy = subCaseInfo.isViewRefresh ? jasmine.createSpy() : null; + this.view.onFileUploadDone = refreshSpy; + getSentRequests()[0].respondWith( + {status: subCaseInfo.responseStatus} + ); + }); - if (caseInfo.numFiles > concurrentUploadLimit) { - it('should start a new upload', function() { - expect(getSentRequests().length).toEqual( - concurrentUploadLimit + 1 - ); - var $uploadElem = $(this.$uploadElems[concurrentUploadLimit]); - expect($.trim($uploadElem.find('.video-detail-status').text())).toEqual( - ActiveVideoUpload.STATUS_UPLOADING - ); - expect($uploadElem).not.toHaveClass('queued'); + it('should update status and progress', function() { + var $uploadElem = this.view.$('.active-video-upload:first'); + if (subCaseInfo.isViewRefresh && + subCaseInfo.responseStatus === 204) { + expect(refreshSpy).toHaveBeenCalled(); + if ($uploadElem.length > 0) { + expect( + $.trim($uploadElem.find('.video-detail-status').text()) + ).not.toEqual(ActiveVideoUpload.STATUS_COMPLETED); + expect( + $uploadElem.find('.video-detail-progress').val() + ).not.toEqual(1); + expect($uploadElem).not.toHaveClass('success'); + } + } else { + expect($uploadElem.length).toEqual(1); + expect( + $.trim($uploadElem.find('.video-detail-status').text()) + ).toEqual(subCaseInfo.statusText); + expect( + $uploadElem.find('.video-detail-progress').val() + ).toEqual(subCaseInfo.progressValue); + expect($uploadElem).toHaveClass(subCaseInfo.presentClass); + expect($uploadElem).not.toHaveClass(subCaseInfo.absentClass); + } + }); + + it('should not trigger the global AJAX error handler', function() { + expect(this.globalAjaxError).not.toHaveBeenCalled(); + }); + + if (caseInfo.numFiles > concurrentUploadLimit) { + it('should start a new upload', function() { + var $uploadElem = $(this.$uploadElems[concurrentUploadLimit]); + expect(getSentRequests().length).toEqual( + concurrentUploadLimit + 1 + ); + expect( + $.trim($uploadElem.find('.video-detail-status').text()) + ).toEqual(ActiveVideoUpload.STATUS_UPLOADING); + expect($uploadElem).not.toHaveClass('queued'); + }); + } + + // If we're uploading more files than the one we've closed above, + // the unload warning should still be shown + if (caseInfo.numFiles > 1) { + it('should show notification when videos are still uploading', + function() { + expect(this.view.onBeforeUnload()).toBe( + 'Your video uploads are not complete.'); + }); + } else { + it('should not show notification once video uploads are complete', + function() { + expect(this.view.onBeforeUnload()).toBeUndefined(); + }); + } }); } - - // If we're uploading more files than the one we've closed above, - // the unload warning should still be shown - if (caseInfo.numFiles > 1) { - it('should show notification when videos are still uploading', - function() { - expect(this.view.onBeforeUnload()).toBe( - 'Your video uploads are not complete.'); - }); - } else { - it('should not show notification once video uploads are complete', - function() { - expect(this.view.onBeforeUnload()).toBeUndefined(); - }); - } - }); + ); } ); }); diff --git a/cms/static/js/views/active_video_upload_list.js b/cms/static/js/views/active_video_upload_list.js index d746589e1d..827021f60f 100644 --- a/cms/static/js/views/active_video_upload_list.js +++ b/cms/static/js/views/active_video_upload_list.js @@ -18,6 +18,7 @@ define( this.listenTo(this.collection, 'add', this.addUpload); this.concurrentUploadLimit = options.concurrentUploadLimit || 0; this.postUrl = options.postUrl; + this.onFileUploadDone = options.onFileUploadDone; if (options.uploadButton) { options.uploadButton.click(this.chooseFile.bind(this)); } @@ -99,9 +100,10 @@ define( // individual file uploads, using the extra `redirected` field to // indicate that the correct upload url has already been retrieved fileUploadAdd: function(event, uploadData) { - var view = this; + var view = this, + model; if (uploadData.redirected) { - var model = new ActiveVideoUpload({fileName: uploadData.files[0].name}); + model = new ActiveVideoUpload({fileName: uploadData.files[0].name, videoId: uploadData.videoId}); this.collection.add(model); uploadData.cid = model.cid; uploadData.submit(); @@ -126,6 +128,7 @@ define( view.$uploadForm.fileupload('add', { files: [uploadData.files[index]], url: file['upload_url'], + videoId: file.edx_video_id, multipart: false, global: false, // Do not trigger global AJAX error handler redirected: true @@ -156,10 +159,48 @@ define( fileUploadDone: function(event, data) { this.setStatus(data.cid, ActiveVideoUpload.STATUS_COMPLETED); this.setProgress(data.cid, 1); + if (this.onFileUploadDone) { + this.onFileUploadDone(this.collection); + this.clearSuccessful(); + } }, fileUploadFail: function(event, data) { this.setStatus(data.cid, ActiveVideoUpload.STATUS_FAILED); + }, + + removeViewAt: function(index) { + this.itemViews.splice(index); + this.$('.active-video-upload-list li').eq(index).remove(); + }, + + // Removes the upload progress view for files that have been + // uploaded successfully. Also removes the corresponding models + // from `collection`, keeping both in sync. + clearSuccessful: function() { + var idx, + completedIndexes = [], + completedModels = [], + completedMessages = []; + this.collection.each(function(model, index) { + if (model.get('status') === ActiveVideoUpload.STATUS_COMPLETED) { + completedModels.push(model); + completedIndexes.push(index - completedIndexes.length); + completedMessages.push(model.get('fileName') + + gettext(': video upload complete.')); + } + }); + for (idx = 0; idx < completedIndexes.length; idx++) { + this.removeViewAt(completedIndexes[idx]); + this.collection.remove(completedModels[idx]); + } + // Alert screen readers that the uploads were successful + if (completedMessages.length) { + completedMessages.push(gettext('Previous Uploads table has been updated.')); + if ($(window).prop('SR') !== undefined) { + $(window).prop('SR').readTexts(completedMessages); + } + } } });