Merge pull request #6631 from edx/gprice/video-upload-improvements

Improve code for video upload feature

cms/djangoapps/contentstore/migrations/0002_auto__del_field_videouploadconfig_status_whitelist.py

+# -*- coding: utf-8 -*-
+import datetime
+from south.db import db
+from south.v2 import SchemaMigration
+from django.db import models
+
+
+class Migration(SchemaMigration):
+
+    def forwards(self, orm):
+        # Deleting field 'VideoUploadConfig.status_whitelist'
+        db.delete_column('contentstore_videouploadconfig', 'status_whitelist')
+
+
+    def backwards(self, orm):
+        # Adding field 'VideoUploadConfig.status_whitelist'
+        db.add_column('contentstore_videouploadconfig', 'status_whitelist',
+                      self.gf('django.db.models.fields.TextField')(default='', blank=True),
+                      keep_default=False)
+
+
+    models = {
+        'auth.group': {
+            'Meta': {'object_name': 'Group'},
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}),
+            'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'})
+        },
+        'auth.permission': {
+            'Meta': {'ordering': "('content_type__app_label', 'content_type__model', 'codename')", 'unique_together': "(('content_type', 'codename'),)", 'object_name': 'Permission'},
+            'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
+            'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['contenttypes.ContentType']"}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'name': ('django.db.models.fields.CharField', [], {'max_length': '50'})
+        },
+        'auth.user': {
+            'Meta': {'object_name': 'User'},
+            'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}),
+            'email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'blank': 'True'}),
+            'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}),
+            'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}),
+            'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
+            'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
+            'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}),
+            'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}),
+            'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}),
+            'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}),
+            'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'})
+        },
+        'contentstore.videouploadconfig': {
+            'Meta': {'object_name': 'VideoUploadConfig'},
+            'change_date': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}),
+            'changed_by': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']", 'null': 'True', 'on_delete': 'models.PROTECT'}),
+            'enabled': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'profile_whitelist': ('django.db.models.fields.TextField', [], {'blank': 'True'})
+        },
+        'contenttypes.contenttype': {
+            'Meta': {'ordering': "('name',)", 'unique_together': "(('app_label', 'model'),)", 'object_name': 'ContentType', 'db_table': "'django_content_type'"},
+            'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
+            'name': ('django.db.models.fields.CharField', [], {'max_length': '100'})
+        }
+    }
+
+    complete_apps = ['contentstore']
\ No newline at end of file

cms/djangoapps/contentstore/models.py

@@ -14,23 +14,8 @@ class VideoUploadConfig(ConfigurationModel):
         blank=True,
         help_text="A comma-separated list of names of profiles to include in video encoding downloads."
     )
-    status_whitelist = TextField(
-        blank=True,
-        help_text=(
-            "A comma-separated list of Studio status values;" +
-            " only videos with these status values will be included in video encoding downloads."
-        )
-    )
 
     @classmethod
     def get_profile_whitelist(cls):
         """Get the list of profiles to include in the encoding download"""
         return [profile for profile in cls.current().profile_whitelist.split(",") if profile]
-
-    @classmethod
-    def get_status_whitelist(cls):
-        """
-        Get the list of status values to include files for in the encoding
-        download
-        """
-        return [status for status in cls.current().status_whitelist.split(",") if status]

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

@@ -16,7 +16,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, VIDEO_ASSET_TYPE, status_display_string
+from contentstore.views.videos import KEY_EXPIRATION_IN_SECONDS, VIDEO_ASSET_TYPE, StatusDisplayStrings
 from contentstore.tests.utils import CourseTestCase
 from contentstore.utils import reverse_course_url
 from xmodule.assetstore import AssetMetadata
@@ -171,7 +171,10 @@ class VideosHandlerTestCase(VideoUploadTestMixin, CourseTestCase):
             dateutil.parser.parse(response_video["created"])
             for field in ["edx_video_id", "client_video_id", "duration"]:
                 self.assertEqual(response_video[field], original_video[field])
-            self.assertEqual(response_video["status"], status_display_string(original_video["status"]))
+            self.assertEqual(
+                response_video["status"],
+                StatusDisplayStrings.get(original_video["status"])
+            )
 
     def test_get_html(self):
         response = self.client.get(self.url)
@@ -313,18 +316,12 @@ class VideoUrlsCsvTestCase(VideoUploadTestMixin, CourseTestCase):
 
     def setUp(self):
         super(VideoUrlsCsvTestCase, self).setUp()
-        VideoUploadConfig(
-            profile_whitelist="profile1",
-            status_whitelist=(
-                status_display_string("file_complete") + "," +
-                status_display_string("transcode_active")
-            )
-        ).save()
+        VideoUploadConfig(profile_whitelist="profile1").save()
 
-    def _check_csv_response(self, expected_video_ids, expected_profiles):
+    def _check_csv_response(self, expected_profiles):
         """
         Check that the response is a valid CSV response containing rows
-        corresponding to expected_video_ids.
+        corresponding to previous_uploads and including the expected profiles.
         """
         response = self.client.get(self.url)
         self.assertEqual(response.status_code, 200)
@@ -346,6 +343,7 @@ class VideoUrlsCsvTestCase(VideoUploadTestMixin, CourseTestCase):
             response_video = {
                 key.decode("utf-8"): value.decode("utf-8") for key, value in row.items()
             }
+            self.assertNotIn(response_video["Video ID"], actual_video_ids)
             actual_video_ids.append(response_video["Video ID"])
             original_video = self._get_previous_upload(response_video["Video ID"])
             self.assertEqual(response_video["Name"], original_video["client_video_id"])
@@ -364,21 +362,14 @@ class VideoUrlsCsvTestCase(VideoUploadTestMixin, CourseTestCase):
                     self.assertEqual(response_profile_url, original_encoded_for_profile["url"])
                 else:
                     self.assertEqual(response_profile_url, "")
-        self.assertEqual(set(actual_video_ids), set(expected_video_ids))
+        self.assertEqual(len(actual_video_ids), len(self.previous_uploads))
 
     def test_basic(self):
-        self._check_csv_response(["test2", "non-ascii"], ["profile1"])
-
-    def test_config(self):
-        VideoUploadConfig(
-            profile_whitelist="profile1,profile2",
-            status_whitelist=(
-                status_display_string("file_complete") + "," +
-                status_display_string("transcode_active") + "," +
-                status_display_string("upload")
-            )
-        ).save()
-        self._check_csv_response(["test1", "test2", "non-ascii"], ["profile1", "profile2"])
+        self._check_csv_response(["profile1"])
+
+    def test_profile_whitelist(self):
+        VideoUploadConfig(profile_whitelist="profile1,profile2").save()
+        self._check_csv_response(["profile1", "profile2"])
 
     def test_non_ascii_course(self):
         course = CourseFactory.create(

cms/djangoapps/contentstore/views/videos.py

@@ -8,7 +8,7 @@ from uuid import uuid4
 from django.conf import settings
 from django.contrib.auth.decorators import login_required
 from django.http import HttpResponse, HttpResponseNotFound
-from django.utils.translation import ugettext as _
+from django.utils.translation import ugettext as _, ugettext_noop
 from django.views.decorators.http import require_GET, require_http_methods
 import rfc6266
 
@@ -37,39 +37,40 @@ KEY_EXPIRATION_IN_SECONDS = 86400
 
 class StatusDisplayStrings(object):
     """
-    Enum of display strings for Video Status presented in Studio (e.g., in UI and in CSV download).
+    A class to map status strings as stored in VAL to display strings for the
+    video upload page
     """
+
     # Translators: This is the status of an active video upload
-    UPLOADING = _("Uploading")
+    _UPLOADING = ugettext_noop("Uploading")
     # Translators: This is the status for a video that the servers are currently processing
-    IN_PROGRESS = _("In Progress")
+    _IN_PROGRESS = ugettext_noop("In Progress")
     # Translators: This is the status for a video that the servers have successfully processed
-    COMPLETE = _("Complete")
+    _COMPLETE = ugettext_noop("Complete")
     # Translators: This is the status for a video that the servers have failed to process
-    FAILED = _("Failed"),
+    _FAILED = ugettext_noop("Failed"),
     # Translators: This is the status for a video for which an invalid
     # processing token was provided in the course settings
-    INVALID_TOKEN = _("Invalid Token"),
+    _INVALID_TOKEN = ugettext_noop("Invalid Token"),
     # Translators: This is the status for a video that is in an unknown state
-    UNKNOWN = _("Unknown")
-
-
-def status_display_string(val_status):
-    """
-    Converts VAL status string to Studio status string.
-    """
-    status_map = {
-        "upload": StatusDisplayStrings.UPLOADING,
-        "ingest": StatusDisplayStrings.IN_PROGRESS,
-        "transcode_queue": StatusDisplayStrings.IN_PROGRESS,
-        "transcode_active": StatusDisplayStrings.IN_PROGRESS,
-        "file_delivered": StatusDisplayStrings.COMPLETE,
-        "file_complete": StatusDisplayStrings.COMPLETE,
-        "file_corrupt": StatusDisplayStrings.FAILED,
-        "pipeline_error": StatusDisplayStrings.FAILED,
-        "invalid_token": StatusDisplayStrings.INVALID_TOKEN
+    _UNKNOWN = ugettext_noop("Unknown")
+
+    _STATUS_MAP = {
+        "upload": _UPLOADING,
+        "ingest": _IN_PROGRESS,
+        "transcode_queue": _IN_PROGRESS,
+        "transcode_active": _IN_PROGRESS,
+        "file_delivered": _COMPLETE,
+        "file_complete": _COMPLETE,
+        "file_corrupt": _FAILED,
+        "pipeline_error": _FAILED,
+        "invalid_token": _INVALID_TOKEN
     }
-    return status_map.get(val_status, StatusDisplayStrings.UNKNOWN)
+
+    @staticmethod
+    def get(val_status):
+        """Map a VAL status string to a localized display string"""
+        return _(StatusDisplayStrings._STATUS_MAP.get(val_status, StatusDisplayStrings._UNKNOWN))
 
 
 @expect_json
@@ -126,7 +127,6 @@ def video_encodings_download(request, course_key_string):
         return _("{profile_name} URL").format(profile_name=profile)
 
     profile_whitelist = VideoUploadConfig.get_profile_whitelist()
-    status_whitelist = VideoUploadConfig.get_status_whitelist()
 
     videos = list(_get_videos(course))
     name_col = _("Name")
@@ -153,7 +153,7 @@ def video_encodings_download(request, course_key_string):
                 (duration_col, duration_val),
                 (added_col, video["created"].isoformat()),
                 (video_id_col, video["edx_video_id"]),
-                (status_col, video["status"]),
+                (status_col, StatusDisplayStrings.get(video["status"])),
             ] +
             [
                 (get_profile_header(encoded_video["profile"]), encoded_video["url"])
@@ -182,8 +182,7 @@ def video_encodings_download(request, course_key_string):
     )
     writer.writeheader()
     for video in videos:
-        if video["status"] in status_whitelist:
-            writer.writerow(make_csv_dict(video))
+        writer.writerow(make_csv_dict(video))
     return response
 
 
@@ -223,7 +222,7 @@ def _get_videos(course):
 
     # convert VAL's status to studio's Video Upload feature status.
     for video in videos:
-        video["status"] = status_display_string(video["status"])
+        video["status"] = StatusDisplayStrings.get(video["status"])
 
     return videos
 

cms/static/js/spec/views/previous_video_upload_spec.js

@@ -65,22 +65,11 @@ define(
                 expect($el.find(".video-id-col").text()).toEqual(testId);
             });
 
-            _.each(
-                [
-                    {status: "Uploading", expected: "Uploading"},
-                    {status: "In Progress", expected: "In Progress"},
-                    {status: "Complete", expected: "Complete"},
-                    {status: "Failed", expected: "Failed"},
-                    {status: "Invalid Token", expected: "Invalid Token"},
-                    {status: "Unknown", expected: "Unknown"}
-                ],
-                function(caseInfo) {
-                    it("should render " + caseInfo.status + " status correctly", function() {
-                        var $el = render({status: caseInfo.status});
-                        expect($el.find(".status-col").text()).toEqual(caseInfo.expected);
-                    });
-                }
-            );
+            it("should render status correctly", function() {
+                var testStatus = "Test Status";
+                var $el = render({status: testStatus});
+                expect($el.find(".status-col").text()).toEqual(testStatus);
+            });
         });
     }
 );