From dd45991926640b6a9c1663a42daa6debab6a444a Mon Sep 17 00:00:00 2001 From: muhammad-ammar Date: Thu, 10 Nov 2016 20:15:50 +0500 Subject: [PATCH] soft delete video from video uploads page TNL-5899 --- .../contentstore/views/tests/test_videos.py | 70 ++++++++++-- cms/djangoapps/contentstore/views/videos.py | 13 ++- cms/static/js/factories/videos_index.js | 8 +- .../views/previous_video_upload_list_spec.js | 100 ++++++++++++++---- .../spec/views/previous_video_upload_spec.js | 62 ++++++++--- cms/static/js/views/previous_video_upload.js | 48 +++++++-- .../js/views/previous_video_upload_list.js | 10 +- cms/static/sass/views/_video-upload.scss | 4 + .../js/previous-video-upload-list.underscore | 1 + .../js/previous-video-upload.underscore | 10 ++ cms/templates/videos_index.html | 4 +- cms/urls.py | 2 +- requirements/edx/github.txt | 2 +- 13 files changed, 274 insertions(+), 60 deletions(-) diff --git a/cms/djangoapps/contentstore/views/tests/test_videos.py b/cms/djangoapps/contentstore/views/tests/test_videos.py index 9733433c97..3083f45708 100644 --- a/cms/djangoapps/contentstore/views/tests/test_videos.py +++ b/cms/djangoapps/contentstore/views/tests/test_videos.py @@ -25,9 +25,9 @@ class VideoUploadTestMixin(object): """ Test cases for the video upload feature """ - def get_url_for_course_key(self, course_key): + def get_url_for_course_key(self, course_key, kwargs=None): """Return video handler URL for the given course""" - return reverse_course_url(self.VIEW_NAME, course_key) + return reverse_course_url(self.VIEW_NAME, course_key, kwargs) def setUp(self): super(VideoUploadTestMixin, self).setUp() @@ -37,6 +37,18 @@ class VideoUploadTestMixin(object): "course_video_upload_token": self.test_token, } self.save_course() + + # create another course for videos belonging to multiple courses + self.course2 = CourseFactory.create() + self.course2.video_upload_pipeline = { + "course_video_upload_token": self.test_token, + } + self.course2.save() + self.store.update_item(self.course2, self.user.id) + + # course ids for videos + course_ids = [unicode(self.course.id), unicode(self.course2.id)] + self.profiles = ["profile1", "profile2"] self.previous_uploads = [ { @@ -44,7 +56,7 @@ class VideoUploadTestMixin(object): "client_video_id": "test1.mp4", "duration": 42.0, "status": "upload", - "courses": [unicode(self.course.id)], + "courses": course_ids, "encoded_videos": [], }, { @@ -52,7 +64,7 @@ class VideoUploadTestMixin(object): "client_video_id": "test2.mp4", "duration": 128.0, "status": "file_complete", - "courses": [unicode(self.course.id)], + "courses": course_ids, "encoded_videos": [ { "profile": "profile1", @@ -73,7 +85,7 @@ class VideoUploadTestMixin(object): "client_video_id": u"nón-ascii-näme.mp4", "duration": 256.0, "status": "transcode_active", - "courses": [unicode(self.course.id)], + "courses": course_ids, "encoded_videos": [ { "profile": "profile1", @@ -91,7 +103,7 @@ class VideoUploadTestMixin(object): "client_video_id": "status_test.mp4", "duration": 3.14, "status": status, - "courses": [unicode(self.course.id)], + "courses": course_ids, "encoded_videos": [], } for status in ( @@ -297,6 +309,52 @@ class VideosHandlerTestCase(VideoUploadTestMixin, CourseTestCase): self.assertEqual(response_file["file_name"], file_info["file_name"]) self.assertEqual(response_file["upload_url"], mock_key_instance.generate_url()) + def _assert_video_removal(self, url, edx_video_id, deleted_videos): + """ + Verify that if correct video is removed from a particular course. + + Arguments: + url (str): URL to get uploaded videos + edx_video_id (str): video id + deleted_videos (int): how many videos are deleted + """ + response = self.client.get_json(url) + self.assertEqual(response.status_code, 200) + response_videos = json.loads(response.content)["videos"] + self.assertEqual(len(response_videos), len(self.previous_uploads) - deleted_videos) + + if deleted_videos: + self.assertNotIn(edx_video_id, [video.get('edx_video_id') for video in response_videos]) + else: + self.assertIn(edx_video_id, [video.get('edx_video_id') for video in response_videos]) + + def test_video_removal(self): + """ + Verifies that video removal is working as expected. + """ + edx_video_id = 'test1' + remove_url = self.get_url_for_course_key(self.course.id, {'edx_video_id': edx_video_id}) + response = self.client.delete(remove_url, HTTP_ACCEPT="application/json") + self.assertEqual(response.status_code, 204) + + self._assert_video_removal(self.url, edx_video_id, 1) + + def test_video_removal_multiple_courses(self): + """ + Verifies that video removal is working as expected for multiple courses. + + If a video is used by multiple courses then removal from one course shouldn't effect the other course. + """ + # remove video from course1 + edx_video_id = 'test1' + remove_url = self.get_url_for_course_key(self.course.id, {'edx_video_id': edx_video_id}) + response = self.client.delete(remove_url, HTTP_ACCEPT="application/json") + self.assertEqual(response.status_code, 204) + + # verify that video is only deleted from course1 only + self._assert_video_removal(self.url, edx_video_id, 1) + self._assert_video_removal(self.get_url_for_course_key(self.course2.id), edx_video_id, 0) + @patch.dict("django.conf.settings.FEATURES", {"ENABLE_VIDEO_UPLOAD_PIPELINE": True}) @override_settings(VIDEO_UPLOAD_PIPELINE={"BUCKET": "test_bucket", "ROOT_PATH": "test_root"}) diff --git a/cms/djangoapps/contentstore/views/videos.py b/cms/djangoapps/contentstore/views/videos.py index 88a321e63b..3ef38bb87e 100644 --- a/cms/djangoapps/contentstore/views/videos.py +++ b/cms/djangoapps/contentstore/views/videos.py @@ -12,7 +12,7 @@ from django.utils.translation import ugettext as _, ugettext_noop from django.views.decorators.http import require_GET, require_http_methods import rfc6266 -from edxval.api import create_video, get_videos_for_course, SortDirection, VideoSortField +from edxval.api import create_video, get_videos_for_course, SortDirection, VideoSortField, remove_video_for_course from opaque_keys.edx.keys import CourseKey from contentstore.models import VideoUploadConfig @@ -77,8 +77,8 @@ class StatusDisplayStrings(object): @expect_json @login_required -@require_http_methods(("GET", "POST")) -def videos_handler(request, course_key_string): +@require_http_methods(("GET", "POST", "DELETE")) +def videos_handler(request, course_key_string, edx_video_id=None): """ The restful handler for video uploads. @@ -91,6 +91,8 @@ def videos_handler(request, course_key_string): json: create a new video upload; the actual files should not be provided to this endpoint but rather PUT to the respective upload_url values contained in the response + DELETE + soft deletes a video for particular course """ course = _get_and_validate_course(course_key_string, request.user) @@ -102,6 +104,9 @@ def videos_handler(request, course_key_string): return videos_index_json(course) else: return videos_index_html(course) + elif request.method == "DELETE": + remove_video_for_course(course_key_string, edx_video_id) + return JsonResponse() else: return videos_post(course, request) @@ -248,7 +253,7 @@ def videos_index_html(course): "videos_index.html", { "context_course": course, - "post_url": reverse_course_url("videos_handler", unicode(course.id)), + "video_handler_url": reverse_course_url("videos_handler", unicode(course.id)), "encodings_download_url": reverse_course_url("video_encodings_download", unicode(course.id)), "previous_uploads": _get_index_videos(course), "concurrent_upload_limit": settings.VIDEO_UPLOAD_PIPELINE.get("CONCURRENT_UPLOAD_LIMIT", 0), diff --git a/cms/static/js/factories/videos_index.js b/cms/static/js/factories/videos_index.js index 780f4fcccb..23749ddb76 100644 --- a/cms/static/js/factories/videos_index.js +++ b/cms/static/js/factories/videos_index.js @@ -5,19 +5,19 @@ define([ 'use strict'; var VideosIndexFactory = function( $contentWrapper, - postUrl, + videoHandlerUrl, encodingsDownloadUrl, concurrentUploadLimit, uploadButton, previousUploads ) { var activeView = new ActiveVideoUploadListView({ - postUrl: postUrl, + postUrl: videoHandlerUrl, concurrentUploadLimit: concurrentUploadLimit, uploadButton: uploadButton, onFileUploadDone: function(activeVideos) { $.ajax({ - url: postUrl, + url: videoHandlerUrl, contentType: 'application/json', dataType: 'json', type: 'GET' @@ -30,6 +30,7 @@ define([ isActive[0].get('status') === ActiveVideoUpload.STATUS_COMPLETE; }), updatedView = new PreviousVideoUploadListView({ + videoHandlerUrl: videoHandlerUrl, collection: updatedCollection, encodingsDownloadUrl: encodingsDownloadUrl }); @@ -38,6 +39,7 @@ define([ } }), previousView = new PreviousVideoUploadListView({ + videoHandlerUrl: videoHandlerUrl, collection: new Backbone.Collection(previousUploads), encodingsDownloadUrl: encodingsDownloadUrl }); diff --git a/cms/static/js/spec/views/previous_video_upload_list_spec.js b/cms/static/js/spec/views/previous_video_upload_list_spec.js index e8bb9298cb..bb89be22e7 100644 --- a/cms/static/js/spec/views/previous_video_upload_list_spec.js +++ b/cms/static/js/spec/views/previous_video_upload_list_spec.js @@ -1,29 +1,81 @@ define( - ['jquery', 'underscore', 'backbone', 'js/views/previous_video_upload_list', 'common/js/spec_helpers/template_helpers'], - function($, _, Backbone, PreviousVideoUploadListView, TemplateHelpers) { + ['jquery', 'underscore', 'backbone', 'js/views/previous_video_upload_list', + 'common/js/spec_helpers/template_helpers', 'edx-ui-toolkit/js/utils/spec-helpers/ajax-helpers'], + function($, _, Backbone, PreviousVideoUploadListView, TemplateHelpers, AjaxHelpers) { 'use strict'; describe('PreviousVideoUploadListView', function() { + var videoHandlerUrl = '/videos/course-v1:org.0+course_0+Run_0', + render = function(numModels) { + var modelData = { + client_video_id: 'foo.mp4', + duration: 42, + created: '2014-11-25T23:13:05', + edx_video_id: 'dummy_id', + status: 'uploading' + }; + var collection = new Backbone.Collection( + _.map( + _.range(numModels), + function(num, index) { + return new Backbone.Model( + _.extend({}, modelData, {edx_video_id: 'dummy_id_' + index}) + ); + } + ) + ); + var view = new PreviousVideoUploadListView({ + collection: collection, + videoHandlerUrl: videoHandlerUrl + }); + return view.render().$el; + }, + verifyVideosInfo; + beforeEach(function() { - TemplateHelpers.installTemplate('previous-video-upload', true); + setFixtures('
'); + TemplateHelpers.installTemplate('previous-video-upload'); TemplateHelpers.installTemplate('previous-video-upload-list'); }); - var render = function(numModels) { - var modelData = { - client_video_id: 'foo.mp4', - duration: 42, - created: '2014-11-25T23:13:05', - edx_video_id: 'dummy_id', - status: 'uploading' - }; - var collection = new Backbone.Collection( - _.map( - _.range(numModels), - function() { return new Backbone.Model(modelData); } - ) - ); - var view = new PreviousVideoUploadListView({collection: collection}); - return view.render().$el; + verifyVideosInfo = function(test, confirmRemove) { + var firstVideo, + numVideos = 5, + $el = render(numVideos), + firstVideoId = 'dummy_id_0', + requests = AjaxHelpers.requests(test), + firstVideoSelector = '.js-table-body tr:first-child'; + + // total number of videos should be 5 before remove + expect($el.find('.js-table-body tr').length).toEqual(numVideos); + + // get first video element + firstVideo = $el.find(firstVideoSelector); + + // verify first video id before removal + expect(firstVideo.find('.video-id-col').text()).toEqual(firstVideoId); + + // remove first video in the table + firstVideo.find('.remove-video-button.action-button').click(); + + if (confirmRemove) { + // click on Remove button on confirmation popup + $('.action-primary').click(); + AjaxHelpers.expectJsonRequest(requests, 'DELETE', videoHandlerUrl + '/dummy_id_0'); + AjaxHelpers.respondWithNoContent(requests); + numVideos -= 1; + firstVideoId = 'dummy_id_1'; + } else { + // click on Cancel button on confirmation popup + $('.action-secondary').click(); + expect(requests.length).toEqual(0); + } + + // verify total number of videos after Remove/Cancel + expect($el.find('.js-table-body tr').length).toEqual(numVideos); + + // verify first video id after Remove/Cancel + firstVideo = $el.find(firstVideoSelector); + expect(firstVideo.find('.video-id-col').text()).toEqual(firstVideoId); }; it('should render an empty collection', function() { @@ -37,6 +89,16 @@ define( expect($el.find('.js-table-body').length).toEqual(1); expect($el.find('.js-table-body tr').length).toEqual(5); }); + + it('removes video upon click on Remove button', function() { + // Remove a video from list and verify that correct video is removed + verifyVideosInfo(this, true); + }); + + it('does nothing upon click on Cancel button', function() { + // Verify that nothing changes when we click on Cancel button on confirmation popup + verifyVideosInfo(this, false); + }); }); } ); diff --git a/cms/static/js/spec/views/previous_video_upload_spec.js b/cms/static/js/spec/views/previous_video_upload_spec.js index 4b0cef731b..e6155b7572 100644 --- a/cms/static/js/spec/views/previous_video_upload_spec.js +++ b/cms/static/js/spec/views/previous_video_upload_spec.js @@ -1,26 +1,29 @@ define( - ['jquery', 'backbone', 'js/views/previous_video_upload', 'common/js/spec_helpers/template_helpers'], - function($, Backbone, PreviousVideoUploadView, TemplateHelpers) { + ['jquery', 'underscore', 'backbone', 'js/views/previous_video_upload', 'common/js/spec_helpers/template_helpers', + 'common/js/spec_helpers/view_helpers'], + function($, _, Backbone, PreviousVideoUploadView, TemplateHelpers, ViewHelpers) { 'use strict'; describe('PreviousVideoUploadView', function() { - beforeEach(function() { - TemplateHelpers.installTemplate('previous-video-upload', true); - }); - var render = function(modelData) { var defaultData = { - client_video_id: 'foo.mp4', - duration: 42, - created: '2014-11-25T23:13:05', - edx_video_id: 'dummy_id', - status: 'uploading' - }; - var view = new PreviousVideoUploadView( - {model: new Backbone.Model($.extend({}, defaultData, modelData))} - ); + client_video_id: 'foo.mp4', + duration: 42, + created: '2014-11-25T23:13:05', + edx_video_id: 'dummy_id', + status: 'uploading' + }, + view = new PreviousVideoUploadView({ + model: new Backbone.Model($.extend({}, defaultData, modelData)), + videoHandlerUrl: '/videos/course-v1:org.0+course_0+Run_0' + }); return view.render().$el; }; + beforeEach(function() { + setFixtures('
'); + TemplateHelpers.installTemplate('previous-video-upload', false); + }); + it('should render video name correctly', function() { var testName = 'test name'; var $el = render({client_video_id: testName}); @@ -70,6 +73,35 @@ define( var $el = render({status: testStatus}); expect($el.find('.status-col').text()).toEqual(testStatus); }); + + it('should render remove button correctly', function() { + var $el = render(), + removeButton = $el.find('.actions-list .action-remove a.remove-video-button'); + + expect(removeButton.data('tooltip')).toEqual('Remove this video'); + expect(removeButton.find('.sr').text()).toEqual('Remove foo.mp4 video'); + }); + + it('shows a confirmation popup when the remove button is clicked', function() { + var $el = render(); + $el.find('a.remove-video-button').click(); + expect($('.prompt.warning .title').text()).toEqual('Are you sure you want to remove this video from the list?'); // eslint-disable-line max-len + expect( + $('.prompt.warning .message').text() + ).toEqual( + 'Removing a video from this list does not affect course content. Any content that uses a previously uploaded video ID continues to display in the course.' // eslint-disable-line max-len + ); + expect($('.prompt.warning .action-primary').text()).toEqual('Remove'); + expect($('.prompt.warning .action-secondary').text()).toEqual('Cancel'); + }); + + it('shows a notification when the remove button is clicked', function() { + var notificationSpy = ViewHelpers.createNotificationSpy(), + $el = render(); + $el.find('a.remove-video-button').click(); + $('.action-primary').click(); + ViewHelpers.verifyNotificationShowing(notificationSpy, /Removing/); + }); }); } ); diff --git a/cms/static/js/views/previous_video_upload.js b/cms/static/js/views/previous_video_upload.js index b5942ee8d1..430d7e680c 100644 --- a/cms/static/js/views/previous_video_upload.js +++ b/cms/static/js/views/previous_video_upload.js @@ -1,13 +1,21 @@ define( - ['gettext', 'js/utils/date_utils', 'js/views/baseview'], - function(gettext, DateUtils, BaseView) { + ['underscore', 'gettext', 'js/utils/date_utils', 'js/views/baseview', 'common/js/components/views/feedback_prompt', + 'common/js/components/views/feedback_notification', 'common/js/components/utils/view_utils', + 'edx-ui-toolkit/js/utils/html-utils', 'text!templates/previous-video-upload.underscore'], + function(_, gettext, DateUtils, BaseView, PromptView, NotificationView, ViewUtils, HtmlUtils, + previousVideoUploadTemplate) { 'use strict'; var PreviousVideoUploadView = BaseView.extend({ tagName: 'tr', - initialize: function() { - this.template = this.loadTemplate('previous-video-upload'); + events: { + 'click .remove-video-button.action-button': 'removeVideo' + }, + + initialize: function(options) { + this.template = HtmlUtils.template(previousVideoUploadTemplate); + this.videoHandlerUrl = options.videoHandlerUrl; }, renderDuration: function(seconds) { @@ -27,10 +35,38 @@ define( created: DateUtils.renderDate(this.model.get('created')), status: this.model.get('status') }; - this.$el.html( - this.template(_.extend({}, this.model.attributes, renderedAttributes)) + HtmlUtils.setHtml( + this.$el, + this.template( + _.extend({}, this.model.attributes, renderedAttributes) + ) ); return this; + }, + + removeVideo: function(event) { + var videoView = this; + + event.preventDefault(); + + ViewUtils.confirmThenRunOperation( + gettext('Are you sure you want to remove this video from the list?'), + gettext('Removing a video from this list does not affect course content. Any content that uses a previously uploaded video ID continues to display in the course.'), // eslint-disable-line max-len + gettext('Remove'), + function() { + ViewUtils.runOperationShowingMessage( + gettext('Removing'), + function() { + return $.ajax({ + url: videoView.videoHandlerUrl + '/' + videoView.model.get('edx_video_id'), + type: 'DELETE' + }).done(function() { + videoView.remove(); + }); + } + ); + } + ); } }); diff --git a/cms/static/js/views/previous_video_upload_list.js b/cms/static/js/views/previous_video_upload_list.js index ec045be210..819e66695c 100644 --- a/cms/static/js/views/previous_video_upload_list.js +++ b/cms/static/js/views/previous_video_upload_list.js @@ -10,14 +10,18 @@ define( this.template = this.loadTemplate('previous-video-upload-list'); this.encodingsDownloadUrl = options.encodingsDownloadUrl; this.itemViews = this.collection.map(function(model) { - return new PreviousVideoUploadView({model: model}); + return new PreviousVideoUploadView({ + videoHandlerUrl: options.videoHandlerUrl, + model: model + }); }); }, render: function() { - var $el = this.$el; + var $el = this.$el, + $tabBody; $el.html(this.template({encodingsDownloadUrl: this.encodingsDownloadUrl})); - var $tabBody = $el.find('.js-table-body'); + $tabBody = $el.find('.js-table-body'); _.each(this.itemViews, function(view) { $tabBody.append(view.render().$el); }); diff --git a/cms/static/sass/views/_video-upload.scss b/cms/static/sass/views/_video-upload.scss index 509d0de47c..567d884c0a 100644 --- a/cms/static/sass/views/_video-upload.scss +++ b/cms/static/sass/views/_video-upload.scss @@ -144,5 +144,9 @@ width: flex-grid(4, 9); text-align: right; } + + .actions-list { + @extend %actions-list; + } } } diff --git a/cms/templates/js/previous-video-upload-list.underscore b/cms/templates/js/previous-video-upload-list.underscore index 29839e1119..c7a7ef0988 100644 --- a/cms/templates/js/previous-video-upload-list.underscore +++ b/cms/templates/js/previous-video-upload-list.underscore @@ -13,6 +13,7 @@ <%- gettext("Date Added") %> <%- gettext("Video ID") %> <%- gettext("Status") %> + <%- gettext("Action") %> diff --git a/cms/templates/js/previous-video-upload.underscore b/cms/templates/js/previous-video-upload.underscore index 8ab56d56b3..0db4d2e227 100644 --- a/cms/templates/js/previous-video-upload.underscore +++ b/cms/templates/js/previous-video-upload.underscore @@ -3,3 +3,13 @@ <%- created %> <%- edx_video_id %> <%- status %> + + + diff --git a/cms/templates/videos_index.html b/cms/templates/videos_index.html index a1242e8659..053b869c7a 100644 --- a/cms/templates/videos_index.html +++ b/cms/templates/videos_index.html @@ -11,7 +11,7 @@ <%namespace name='static' file='static_content.html'/> <%block name="header_extras"> -% for template_name in ["active-video-upload-list", "active-video-upload", "previous-video-upload-list", "previous-video-upload"]: +% for template_name in ["active-video-upload-list", "active-video-upload", "previous-video-upload-list"]: @@ -24,7 +24,7 @@ var $contentWrapper = $(".content-primary"); VideosIndexFactory( $contentWrapper, - "${post_url}", + "${video_handler_url}", "${encodings_download_url}", ${concurrent_upload_limit}, $(".nav-actions .upload-button"), diff --git a/cms/urls.py b/cms/urls.py index 083e70a378..7b241d501b 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -107,7 +107,7 @@ urlpatterns += patterns( url(r'^settings/advanced/{}$'.format(settings.COURSE_KEY_PATTERN), 'advanced_settings_handler'), url(r'^textbooks/{}$'.format(settings.COURSE_KEY_PATTERN), 'textbooks_list_handler'), url(r'^textbooks/{}/(?P\d[^/]*)$'.format(settings.COURSE_KEY_PATTERN), 'textbooks_detail_handler'), - url(r'^videos/{}$'.format(settings.COURSE_KEY_PATTERN), 'videos_handler'), + url(r'^videos/{}(?:/(?P[-\w]+))?$'.format(settings.COURSE_KEY_PATTERN), 'videos_handler'), url(r'^video_encodings_download/{}$'.format(settings.COURSE_KEY_PATTERN), 'video_encodings_download'), url(r'^group_configurations/{}$'.format(settings.COURSE_KEY_PATTERN), 'group_configurations_list_handler'), url(r'^group_configurations/{}/(?P\d+)(/)?(?P\d+)?$'.format( diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index 19a6f3f727..97901e0e57 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -80,7 +80,7 @@ git+https://github.com/edx/edx-ora2.git@1.1.10#egg=ora2==1.1.10 -e git+https://github.com/edx/edx-submissions.git@1.1.1#egg=edx-submissions==1.1.1 git+https://github.com/edx/ease.git@release-2015-07-14#egg=ease==0.1.3 git+https://github.com/edx/i18n-tools.git@v0.3.2#egg=i18n-tools==v0.3.2 -git+https://github.com/edx/edx-val.git@0.0.9#egg=edxval==0.0.9 +git+https://github.com/edx/edx-val.git@0.0.10#egg=edxval==0.0.10 git+https://github.com/pmitros/RecommenderXBlock.git@v1.1#egg=recommender-xblock==1.1 git+https://github.com/solashirai/crowdsourcehinter.git@518605f0a95190949fe77bd39158450639e2e1dc#egg=crowdsourcehinter-xblock==0.1 -e git+https://github.com/pmitros/RateXBlock.git@367e19c0f6eac8a5f002fd0f1559555f8e74bfff#egg=rate-xblock