Merge pull request #14206 from edx/ammar/tnl-4777-upload-error-message

Improve error handling for Video Uploads

cms/djangoapps/contentstore/views/tests/test_videos.py

@@ -2,12 +2,14 @@
 """
 Unit tests for video-related REST APIs.
 """
+from datetime import datetime
 import csv
 import ddt
 import json
 import dateutil.parser
 import re
 from StringIO import StringIO
+import pytz
 
 from django.conf import settings
 from django.test.utils import override_settings
@@ -16,7 +18,7 @@ from mock import Mock, patch
 from edxval.api import create_profile, create_video, get_video_info
 
 from contentstore.models import VideoUploadConfig
-from contentstore.views.videos import KEY_EXPIRATION_IN_SECONDS, StatusDisplayStrings
+from contentstore.views.videos import KEY_EXPIRATION_IN_SECONDS, StatusDisplayStrings, convert_video_status
 from contentstore.tests.utils import CourseTestCase
 from contentstore.utils import reverse_course_url
 from xmodule.modulestore.tests.factories import CourseFactory
@@ -49,6 +51,7 @@ class VideoUploadTestMixin(object):
 
         # course ids for videos
         course_ids = [unicode(self.course.id), unicode(self.course2.id)]
+        created = datetime.now(pytz.utc)
 
         self.profiles = ["profile1", "profile2"]
         self.previous_uploads = [
@@ -59,6 +62,7 @@ class VideoUploadTestMixin(object):
                 "status": "upload",
                 "courses": course_ids,
                 "encoded_videos": [],
+                "created": created
             },
             {
                 "edx_video_id": "test2",
@@ -66,6 +70,7 @@ class VideoUploadTestMixin(object):
                 "duration": 128.0,
                 "status": "file_complete",
                 "courses": course_ids,
+                "created": created,
                 "encoded_videos": [
                     {
                         "profile": "profile1",
@@ -87,6 +92,7 @@ class VideoUploadTestMixin(object):
                 "duration": 256.0,
                 "status": "transcode_active",
                 "courses": course_ids,
+                "created": created,
                 "encoded_videos": [
                     {
                         "profile": "profile1",
@@ -105,6 +111,7 @@ class VideoUploadTestMixin(object):
                 "duration": 3.14,
                 "status": status,
                 "courses": course_ids,
+                "created": created,
                 "encoded_videos": [],
             }
             for status in (
@@ -184,7 +191,7 @@ class VideosHandlerTestCase(VideoUploadTestMixin, CourseTestCase):
                 self.assertEqual(response_video[field], original_video[field])
             self.assertEqual(
                 response_video["status"],
-                StatusDisplayStrings.get(original_video["status"])
+                convert_video_status(original_video)
             )
 
     def test_get_html(self):
@@ -442,6 +449,67 @@ class VideosHandlerTestCase(VideoUploadTestMixin, CourseTestCase):
         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)
 
+    def test_convert_video_status(self):
+        """
+        Verifies that convert_video_status works as expected.
+        """
+        video = self.previous_uploads[0]
+
+        # video status should be failed if it's in upload state for more than 24 hours
+        video['created'] = datetime(2016, 1, 1, 10, 10, 10, 0, pytz.UTC)
+        status = convert_video_status(video)
+        self.assertEqual(status, StatusDisplayStrings.get('upload_failed'))
+
+        # `invalid_token` should be converted to `youtube_duplicate`
+        video['created'] = datetime.now(pytz.UTC)
+        video['status'] = 'invalid_token'
+        status = convert_video_status(video)
+        self.assertEqual(status, StatusDisplayStrings.get('youtube_duplicate'))
+
+        # for all other status, there should not be any conversion
+        statuses = StatusDisplayStrings._STATUS_MAP.keys()  # pylint: disable=protected-access
+        statuses.remove('invalid_token')
+        for status in statuses:
+            video['status'] = status
+            new_status = convert_video_status(video)
+            self.assertEqual(new_status, StatusDisplayStrings.get(status))
+
+    def assert_video_status(self, url, edx_video_id, status):
+        """
+        Verifies that video with `edx_video_id` has `status`
+        """
+        response = self.client.get_json(url)
+        self.assertEqual(response.status_code, 200)
+        videos = json.loads(response.content)["videos"]
+        for video in videos:
+            if video['edx_video_id'] == edx_video_id:
+                return self.assertEqual(video['status'], status)
+
+        # Test should fail if video not found
+        self.assertEqual(True, False, 'Invalid edx_video_id')
+
+    def test_video_status_update_request(self):
+        """
+        Verifies that video status update request works as expected.
+        """
+        url = self.get_url_for_course_key(self.course.id)
+        edx_video_id = 'test1'
+
+        self.assert_video_status(url, edx_video_id, 'Uploading')
+
+        response = self.client.post(
+            url,
+            json.dumps([{
+                'edxVideoId': edx_video_id,
+                'status': 'upload_failed',
+                'message': 'server down'
+            }]),
+            content_type="application/json"
+        )
+        self.assertEqual(response.status_code, 204)
+
+        self.assert_video_status(url, edx_video_id, 'Failed')
+
 
 @patch.dict("django.conf.settings.FEATURES", {"ENABLE_VIDEO_UPLOAD_PIPELINE": True})
 @override_settings(VIDEO_UPLOAD_PIPELINE={"BUCKET": "test_bucket", "ROOT_PATH": "test_root"})
@@ -486,7 +554,7 @@ class VideoUrlsCsvTestCase(VideoUploadTestMixin, CourseTestCase):
             self.assertEqual(response_video["Duration"], str(original_video["duration"]))
             dateutil.parser.parse(response_video["Date Added"])
             self.assertEqual(response_video["Video ID"], original_video["edx_video_id"])
-            self.assertEqual(response_video["Status"], StatusDisplayStrings.get(original_video["status"]))
+            self.assertEqual(response_video["Status"], convert_video_status(original_video))
             for profile in expected_profiles:
                 response_profile_url = response_video["{} URL".format(profile)]
                 original_encoded_for_profile = next(

cms/djangoapps/contentstore/views/videos.py

 """
 Views related to the video upload feature
 """
+from datetime import datetime, timedelta
+import logging
+
 from boto import s3
 import csv
 from uuid import uuid4
@@ -12,7 +15,14 @@ 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, remove_video_for_course
+from edxval.api import (
+    create_video,
+    get_videos_for_course,
+    SortDirection,
+    VideoSortField,
+    remove_video_for_course,
+    update_video_status
+)
 from opaque_keys.edx.keys import CourseKey
 
 from contentstore.models import VideoUploadConfig
@@ -25,6 +35,8 @@ from .course import get_course_and_check_access
 
 __all__ = ["videos_handler", "video_encodings_download"]
 
+LOGGER = logging.getLogger(__name__)
+
 
 # Default expiration, in seconds, of one-time URLs used for uploading videos.
 KEY_EXPIRATION_IN_SECONDS = 86400
@@ -36,6 +48,9 @@ VIDEO_SUPPORTED_FILE_FORMATS = {
 
 VIDEO_UPLOAD_MAX_FILE_SIZE_GB = 5
 
+# maximum time for video to remain in upload state
+MAX_UPLOAD_HOURS = 24
+
 
 class StatusDisplayStrings(object):
     """
@@ -49,11 +64,17 @@ class StatusDisplayStrings(object):
     _IN_PROGRESS = ugettext_noop("In Progress")
     # Translators: This is the status for a video that the servers have successfully processed
     _COMPLETE = ugettext_noop("Ready")
+    # Translators: This is the status for a video that is uploaded completely
+    _UPLOAD_COMPLETED = ugettext_noop("Uploaded")
     # Translators: This is the status for a video that the servers have failed to process
     _FAILED = ugettext_noop("Failed")
+    # Translators: This is the status for a video that is cancelled during upload by user
+    _CANCELLED = ugettext_noop("Cancelled")
     # Translators: This is the status for a video which has failed
     # due to being flagged as a duplicate by an external or internal CMS
     _DUPLICATE = ugettext_noop("Failed Duplicate")
+    # Translators: This is the status for a video which has duplicate token for youtube
+    _YOUTUBE_DUPLICATE = ugettext_noop("YouTube Duplicate")
     # Translators: This is the status for a video for which an invalid
     # processing token was provided in the course settings
     _INVALID_TOKEN = ugettext_noop("Invalid Token")
@@ -69,9 +90,14 @@ class StatusDisplayStrings(object):
         "transcode_active": _IN_PROGRESS,
         "file_delivered": _COMPLETE,
         "file_complete": _COMPLETE,
+        "upload_completed": _UPLOAD_COMPLETED,
         "file_corrupt": _FAILED,
         "pipeline_error": _FAILED,
+        "upload_failed": _FAILED,
+        "s3_upload_failed": _FAILED,
+        "upload_cancelled": _CANCELLED,
         "duplicate": _DUPLICATE,
+        "youtube_duplicate": _YOUTUBE_DUPLICATE,
         "invalid_token": _INVALID_TOKEN,
         "imported": _IMPORTED,
     }
@@ -115,6 +141,9 @@ def videos_handler(request, course_key_string, edx_video_id=None):
         remove_video_for_course(course_key_string, edx_video_id)
         return JsonResponse()
     else:
+        if is_status_update_request(request.json):
+            return send_video_status_update(request.json)
+
         return videos_post(course, request)
 
 
@@ -226,6 +255,36 @@ def _get_and_validate_course(course_key_string, user):
         return None
 
 
+def convert_video_status(video):
+    """
+    Convert status of a video. Status can be converted to one of the following:
+
+        *   FAILED if video is in `upload` state for more than 24 hours
+        *   `YouTube Duplicate` if status is `invalid_token`
+        *   user-friendly video status
+    """
+    now = datetime.now(video['created'].tzinfo)
+    if video['status'] == 'upload' and (now - video['created']) > timedelta(hours=MAX_UPLOAD_HOURS):
+        new_status = 'upload_failed'
+        status = StatusDisplayStrings.get(new_status)
+        message = 'Video with id [%s] is still in upload after [%s] hours, setting status to [%s]' % (
+            video['edx_video_id'], MAX_UPLOAD_HOURS, new_status
+        )
+        send_video_status_update([
+            {
+                'edxVideoId': video['edx_video_id'],
+                'status': new_status,
+                'message': message
+            }
+        ])
+    elif video['status'] == 'invalid_token':
+        status = StatusDisplayStrings.get('youtube_duplicate')
+    else:
+        status = StatusDisplayStrings.get(video['status'])
+
+    return status
+
+
 def _get_videos(course):
     """
     Retrieves the list of videos from VAL corresponding to this course.
@@ -234,7 +293,7 @@ def _get_videos(course):
 
     # convert VAL's status to studio's Video Upload feature status.
     for video in videos:
-        video["status"] = StatusDisplayStrings.get(video["status"])
+        video["status"] = convert_video_status(video)
 
     return videos
 
@@ -386,3 +445,21 @@ def storage_service_key(bucket, file_name):
         file_name
     )
     return s3.key.Key(bucket, key_name)
+
+
+def send_video_status_update(updates):
+    """
+    Update video status in edx-val.
+    """
+    for update in updates:
+        update_video_status(update.get('edxVideoId'), update.get('status'))
+        LOGGER.info(update.get('message'))
+
+    return JsonResponse()
+
+
+def is_status_update_request(request_data):
+    """
+    Returns True if `request_data` contains status update else False.
+    """
+    return any('status' in update for update in request_data)

cms/static/cms/js/spec/main.js

@@ -4,6 +4,22 @@
 (function(requirejs, requireSerial) {
     'use strict';
 
+    if (window) {
+        define('add-a11y-deps',
+            [
+                'underscore',
+                'underscore.string',
+                'edx-ui-toolkit/js/utils/html-utils',
+                'edx-ui-toolkit/js/utils/string-utils'
+            ], function(_, str, HtmlUtils, StringUtils) {
+                window._ = _;
+                window._.str = str;
+                window.edx = window.edx || {};
+                window.edx.HtmlUtils = HtmlUtils;
+                window.edx.StringUtils = StringUtils;
+            });
+    }
+
     var i, specHelpers, testFiles;
 
     requirejs.config({
@@ -169,6 +185,10 @@
                     return window.MathJax.Hub.Configured();
                 }
             },
+            'accessibility': {
+                exports: 'accessibility',
+                deps: ['add-a11y-deps']
+            },
             'URI': {
                 exports: 'URI'
             },

cms/static/js/models/active_video_upload.js

@@ -21,7 +21,13 @@ define(
                 defaults: {
                     videoId: null,
                     status: statusStrings.STATUS_QUEUED,
-                    progress: 0
+                    progress: 0,
+                    failureMessage: null
+                },
+
+                uploading: function() {
+                    var status = this.get('status');
+                    return (this.get('progress') < 1) && ((status === statusStrings.STATUS_UPLOADING));
                 }
             },
             statusStrings

cms/static/js/views/active_video_upload.js

 define(
-    ['js/models/active_video_upload', 'js/views/baseview'],
-    function(ActiveVideoUpload, BaseView) {
+    ['underscore', 'js/models/active_video_upload', 'js/views/baseview', 'common/js/components/views/feedback_prompt'],
+    function(_, ActiveVideoUpload, BaseView, PromptView) {
         'use strict';
 
         var STATUS_CLASSES = [
@@ -13,15 +13,20 @@ define(
             tagName: 'li',
             className: 'active-video-upload',
 
+            events: {
+                'click a.more-details-action': 'showUploadFailureMessage'
+            },
+
             initialize: function() {
                 this.template = this.loadTemplate('active-video-upload');
                 this.listenTo(this.model, 'change', this.render);
             },
 
             render: function() {
-                var $el = this.$el;
+                var $el = this.$el,
+                    status;
                 $el.html(this.template(this.model.attributes));
-                var status = this.model.get('status');
+                status = this.model.get('status');
                 _.each(
                     STATUS_CLASSES,
                     function(statusClass) {
@@ -29,6 +34,21 @@ define(
                     }
                 );
                 return this;
+            },
+
+            showUploadFailureMessage: function() {
+                return new PromptView.Warning({
+                    title: gettext('Your file could not be uploaded'),
+                    message: this.model.get('failureMessage'),
+                    actions: {
+                        primary: {
+                            text: gettext('Close'),
+                            click: function(prompt) {
+                                return prompt.hide();
+                            }
+                        }
+                    }
+                }).show();
             }
         });
 

cms/static/sass/views/_video-upload.scss

@@ -65,11 +65,15 @@
           font-size: 90%;
         }
 
-        .video-detail-status {
+        .video-detail-status, .more-details-action {
           @include font-size(12);
           @include line-height(12);
         }
 
+        .more-details-action, .upload-failure {
+          display: none;
+        }
+
         .video-detail-progress {
           -webkit-appearance: none;
           -moz-appearance: none;
@@ -105,7 +109,7 @@
         }
 
         &.error {
-          .video-detail-status {
+          .video-upload-status {
             color: $color-error;
           }
 
@@ -117,10 +121,20 @@
           .video-detail-progress::-moz-progress-bar {
             background-color: $color-error;
           }
+
+          .more-details-action, .upload-failure {
+            display: inline-block;
+            color: $color-error;
+          }
+
+          .more-details-action {
+            margin-top: ($baseline/5);
+            float: right;
+          }
         }
 
         &.success {
-          .video-detail-status {
+          .video-upload-status {
             color: $color-ready;
           }
         }

cms/templates/js/active-video-upload.underscore

 <h4 class="video-detail-name"><%- fileName %></h4>
 <progress class="video-detail-progress" value="<%= progress %>"></progress>
-<p class="video-detail-status"><%- gettext(status) %></p>
+<div class="video-upload-status">
+    <span class="icon alert-icon fa fa-warning upload-failure" aria-hidden="true"></span>
+    <span class="video-detail-status"><%- gettext(status) %></span>
+    <% if (failureMessage) { %>
+        <a href="#" class="more-details-action">
+            <%- gettext("Read More") %>
+            <span class="sr"><%- gettext("details about the failure") %></span>
+        </a>
+    <% } %>
+</div>

requirements/edx/github.txt

@@ -80,7 +80,7 @@ git+https://github.com/edx/edx-ora2.git@1.1.13#egg=ora2==1.1.13
 -e git+https://github.com/edx/edx-submissions.git@1.1.4#egg=edx-submissions==1.1.4
 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.11#egg=edxval==0.0.11
+git+https://github.com/edx/edx-val.git@0.0.12#egg=edxval==0.0.12
 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