From 874cd5c78f67f451f4ad887b51ef545e6c5c4fb2 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 14 Jan 2015 17:06:32 -0500 Subject: [PATCH] Improve code for video upload feature Fix i18n for video status strings (broken in commit 4b53f4d) and remove unnecessary complexity from a test case. This also removes the status whitelist in the video upload configuration. Because status is included in the CSV report, it is not necessary to filter the included videos by status. --- ...ield_videouploadconfig_status_whitelist.py | 69 +++++++++++++++++++ cms/djangoapps/contentstore/models.py | 15 ---- .../contentstore/views/tests/test_videos.py | 37 ++++------ cms/djangoapps/contentstore/views/videos.py | 57 ++++++++------- .../spec/views/previous_video_upload_spec.js | 21 ++---- 5 files changed, 116 insertions(+), 83 deletions(-) create mode 100644 cms/djangoapps/contentstore/migrations/0002_auto__del_field_videouploadconfig_status_whitelist.py diff --git a/cms/djangoapps/contentstore/migrations/0002_auto__del_field_videouploadconfig_status_whitelist.py b/cms/djangoapps/contentstore/migrations/0002_auto__del_field_videouploadconfig_status_whitelist.py new file mode 100644 index 0000000000..e5fce94d8b --- /dev/null +++ b/cms/djangoapps/contentstore/migrations/0002_auto__del_field_videouploadconfig_status_whitelist.py @@ -0,0 +1,69 @@ +# -*- 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 diff --git a/cms/djangoapps/contentstore/models.py b/cms/djangoapps/contentstore/models.py index 690e6c2df2..d2a7973892 100644 --- a/cms/djangoapps/contentstore/models.py +++ b/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] diff --git a/cms/djangoapps/contentstore/views/tests/test_videos.py b/cms/djangoapps/contentstore/views/tests/test_videos.py index 5cfa8b2331..8f7e606574 100644 --- a/cms/djangoapps/contentstore/views/tests/test_videos.py +++ b/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"]) + self._check_csv_response(["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"]) + 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( diff --git a/cms/djangoapps/contentstore/views/videos.py b/cms/djangoapps/contentstore/views/videos.py index fb26305e38..eb7a852381 100644 --- a/cms/djangoapps/contentstore/views/videos.py +++ b/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") + _UNKNOWN = ugettext_noop("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 + _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 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 8d6127465d..a087dad10c 100644 --- a/cms/static/js/spec/views/previous_video_upload_spec.js +++ b/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); + }); }); } );