diff --git a/.gitignore b/.gitignore index befabcaf..25b40c9f 100644 --- a/.gitignore +++ b/.gitignore @@ -66,3 +66,5 @@ logs/*/*.log* .vagrant venv/ + +video-images/ diff --git a/AUTHORS b/AUTHORS index 4dc7ba43..8fb9211f 100644 --- a/AUTHORS +++ b/AUTHORS @@ -1,2 +1,3 @@ Christopher Lee Mushtaq Ali +Muhammad Ammar diff --git a/edxval/admin.py b/edxval/admin.py index 3401607f..f05b6cb2 100644 --- a/edxval/admin.py +++ b/edxval/admin.py @@ -3,7 +3,7 @@ """ from django.contrib import admin -from .models import Video, Profile, EncodedVideo, Subtitle, CourseVideo +from .models import Video, Profile, EncodedVideo, Subtitle, CourseVideo, VideoImage class ProfileAdmin(admin.ModelAdmin): # pylint: disable=C0111 @@ -11,15 +11,18 @@ class ProfileAdmin(admin.ModelAdmin): # pylint: disable=C0111 list_display_links = ('id', 'profile_name') admin_order_field = 'profile_name' + class EncodedVideoInline(admin.TabularInline): # pylint: disable=C0111 model = EncodedVideo + class CourseVideoInline(admin.TabularInline): # pylint: disable=C0111 model = CourseVideo extra = 0 verbose_name = "Course" verbose_name_plural = "Courses" + class VideoAdmin(admin.ModelAdmin): # pylint: disable=C0111 list_display = ( 'id', 'edx_video_id', 'client_video_id', 'duration', 'created', 'status' @@ -30,6 +33,13 @@ class VideoAdmin(admin.ModelAdmin): # pylint: disable=C0111 admin_order_field = 'edx_video_id' inlines = [CourseVideoInline, EncodedVideoInline] + +class VideoImageAdmin(admin.ModelAdmin): + model = VideoImage + verbose_name = 'Video Image' + verbose_name_plural = 'Video Images' + admin.site.register(Profile, ProfileAdmin) admin.site.register(Video, VideoAdmin) admin.site.register(Subtitle) +admin.site.register(VideoImage, VideoImageAdmin) diff --git a/edxval/api.py b/edxval/api.py index 75a389dd..649b271a 100644 --- a/edxval/api.py +++ b/edxval/api.py @@ -1,69 +1,29 @@ # pylint: disable=E1101 # -*- coding: utf-8 -*- """ -The internal API for VAL. This is not yet stable +The internal API for VAL. """ import logging from lxml.etree import Element, SubElement from enum import Enum -from django.core.exceptions import ValidationError +from django.core.exceptions import ValidationError, ObjectDoesNotExist +from django.core.files.base import ContentFile -from edxval.models import Video, EncodedVideo, CourseVideo, Profile +from edxval.models import Video, EncodedVideo, CourseVideo, Profile, VideoImage from edxval.serializers import VideoSerializer +from edxval.exceptions import ( # pylint: disable=unused-import + ValError, + ValInternalError, + ValVideoNotFoundError, + ValCannotCreateError, + ValCannotUpdateError +) logger = logging.getLogger(__name__) # pylint: disable=C0103 -class ValError(Exception): - """ - An error that occurs during VAL actions. - - This error is raised when the VAL API cannot perform a requested - action. - - """ - pass - - -class ValInternalError(ValError): - """ - An error internal to the VAL API has occurred. - - This error is raised when an error occurs that is not caused by incorrect - use of the API, but rather internal implementation of the underlying - services. - - """ - pass - - -class ValVideoNotFoundError(ValError): - """ - This error is raised when a video is not found - - If a state is specified in a call to the API that results in no matching - entry in database, this error may be raised. - - """ - pass - - -class ValCannotCreateError(ValError): - """ - This error is raised when an object cannot be created - """ - pass - - -class ValCannotUpdateError(ValError): - """ - This error is raised when an object cannot be updated - """ - pass - - class VideoSortField(Enum): """An enum representing sortable fields in the Video model""" created = "created" @@ -101,6 +61,7 @@ def create_video(video_data): file_size: size of the video in bytes profile: ID of the profile courses: Courses associated with this video + image: poster image file name for a particular course } Raises: @@ -182,6 +143,51 @@ def update_video_status(edx_video_id, status): video.save() +def get_course_video_image_url(course_id, edx_video_id): + """ + Returns course video image url or None if no image found + """ + try: + video_image = CourseVideo.objects.select_related('video_image').get( + course_id=course_id, video__edx_video_id=edx_video_id + ).video_image + return video_image.image_url() + except ObjectDoesNotExist: + return None + + +def update_video_image(edx_video_id, course_id, image_data, file_name): + """ + Update video image for an existing video. + + NOTE: If `image_data` is None then `file_name` value will be used as it is, otherwise + a new file name is constructed based on uuid and extension from `file_name` value. + `image_data` will be None in case of course re-run and export. + + Arguments: + image_data (InMemoryUploadedFile): Image data to be saved for a course video. + + Returns: + course video image url + + Raises: + Raises ValVideoNotFoundError if the CourseVideo cannot be retrieved. + """ + try: + course_video = CourseVideo.objects.select_related('video').get( + course_id=course_id, video__edx_video_id=edx_video_id + ) + except ObjectDoesNotExist: + error_message = u'VAL: CourseVideo not found for edx_video_id: {0} and course_id: {1}'.format( + edx_video_id, + course_id + ) + raise ValVideoNotFoundError(error_message) + + video_image, _ = VideoImage.create_or_update(course_video, file_name, image_data) + return video_image.image_url() + + def create_profile(profile_name): """ Used to create Profile objects in the database @@ -314,11 +320,7 @@ def get_url_for_profile(edx_video_id, profile): return get_urls_for_profiles(edx_video_id, [profile])[profile] -def _get_videos_for_filter( - video_filter, - sort_field=None, - sort_dir=SortDirection.asc -): +def _get_videos_for_filter(video_filter, sort_field=None, sort_dir=SortDirection.asc): """ Returns a generator expression that contains the videos found, sorted by the given field and direction, with ties broken by edx_video_id to ensure a @@ -333,11 +335,7 @@ def _get_videos_for_filter( return (VideoSerializer(video).data for video in videos) -def get_videos_for_course( - course_id, - sort_field=None, - sort_dir=SortDirection.asc, -): +def get_videos_for_course(course_id, sort_field=None, sort_dir=SortDirection.asc): """ Returns an iterator of videos for the given course id. @@ -352,7 +350,7 @@ def get_videos_for_course( total order. """ return _get_videos_for_filter( - {"courses__course_id": unicode(course_id), "courses__is_hidden": False}, + {'courses__course_id': unicode(course_id), 'courses__is_hidden': False}, sort_field, sort_dir, ) @@ -490,21 +488,29 @@ def copy_course_videos(source_course_id, destination_course_id): if source_course_id == destination_course_id: return - videos = Video.objects.filter(courses__course_id=unicode(source_course_id)) + course_videos = CourseVideo.objects.select_related('video', 'video_image').filter( + course_id=unicode(source_course_id) + ) - for video in videos: - CourseVideo.objects.get_or_create( - video=video, + for course_video in course_videos: + destination_course_video, __ = CourseVideo.objects.get_or_create( + video=course_video.video, course_id=destination_course_id ) + if hasattr(course_video, 'video_image'): + VideoImage.create_or_update( + course_video=destination_course_video, + file_name=course_video.video_image.image.name + ) -def export_to_xml(edx_video_id): +def export_to_xml(edx_video_id, course_id=None): """ Exports data about the given edx_video_id into the given xml object. Args: edx_video_id (str): The ID of the video to export + course_id (str): The ID of the course with which this video is associated Returns: An lxml video_asset element containing export data @@ -512,12 +518,21 @@ def export_to_xml(edx_video_id): Raises: ValVideoNotFoundError: if the video does not exist """ + video_image_name = '' video = _get_video(edx_video_id) + + try: + course_video = CourseVideo.objects.select_related('video_image').get(course_id=course_id, video=video) + video_image_name = course_video.video_image.image.name + except ObjectDoesNotExist: + pass + video_el = Element( 'video_asset', attrib={ 'client_video_id': video.client_video_id, 'duration': unicode(video.duration), + 'image': video_image_name } ) for encoded_video in video.encoded_videos.all(): @@ -562,7 +577,12 @@ def import_from_xml(xml, edx_video_id, course_id=None): course_id, ) if course_id: - CourseVideo.get_or_create_with_validation(video=video, course_id=course_id) + course_video, __ = CourseVideo.get_or_create_with_validation(video=video, course_id=course_id) + + image_file_name = xml.get('image', '').strip() + if image_file_name: + VideoImage.create_or_update(course_video, image_file_name) + return except ValidationError as err: logger.exception(err.message) @@ -577,7 +597,7 @@ def import_from_xml(xml, edx_video_id, course_id=None): 'duration': xml.get('duration'), 'status': 'imported', 'encoded_videos': [], - 'courses': [course_id] if course_id else [], + 'courses': [{course_id: xml.get('image')}] if course_id else [], } for encoded_video_el in xml.iterfind('encoded_video'): profile_name = encoded_video_el.get('profile') diff --git a/edxval/exceptions.py b/edxval/exceptions.py new file mode 100644 index 00000000..3c194829 --- /dev/null +++ b/edxval/exceptions.py @@ -0,0 +1,50 @@ +""" +VAL Exceptions. +""" + +class ValError(Exception): + """ + An error that occurs during VAL actions. + + This error is raised when the VAL API cannot perform a requested + action. + + """ + pass + + +class ValInternalError(ValError): + """ + An error internal to the VAL API has occurred. + + This error is raised when an error occurs that is not caused by incorrect + use of the API, but rather internal implementation of the underlying + services. + + """ + pass + + +class ValVideoNotFoundError(ValError): + """ + This error is raised when a video is not found + + If a state is specified in a call to the API that results in no matching + entry in database, this error may be raised. + + """ + pass + + +class ValCannotCreateError(ValError): + """ + This error is raised when an object cannot be created + """ + pass + + +class ValCannotUpdateError(ValError): + """ + This error is raised when an object cannot be updated + """ + pass diff --git a/edxval/migrations/0005_videoimage.py b/edxval/migrations/0005_videoimage.py new file mode 100644 index 00000000..c706661b --- /dev/null +++ b/edxval/migrations/0005_videoimage.py @@ -0,0 +1,31 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import migrations, models +import django.utils.timezone +import model_utils.fields +import edxval.models + + +class Migration(migrations.Migration): + + dependencies = [ + ('edxval', '0004_data__add_hls_profile'), + ] + + operations = [ + migrations.CreateModel( + name='VideoImage', + fields=[ + ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)), + ('created', model_utils.fields.AutoCreatedField(default=django.utils.timezone.now, verbose_name='created', editable=False)), + ('modified', model_utils.fields.AutoLastModifiedField(default=django.utils.timezone.now, verbose_name='modified', editable=False)), + ('image', edxval.models.CustomizableImageField(null=True, blank=True)), + ('generated_images', edxval.models.ListField()), + ('course_video', models.OneToOneField(related_name='video_image', to='edxval.CourseVideo')), + ], + options={ + 'abstract': False, + }, + ), + ] diff --git a/edxval/models.py b/edxval/models.py index 50fc4c58..d42b3049 100644 --- a/edxval/models.py +++ b/edxval/models.py @@ -11,16 +11,26 @@ invalid profile_name will be returned. """ +from contextlib import closing +import json import logging +import os +from uuid import uuid4 from django.db import models from django.dispatch import receiver +from django.core.exceptions import ValidationError from django.core.validators import MinValueValidator, RegexValidator from django.core.urlresolvers import reverse +from model_utils.models import TimeStampedModel + +from edxval.utils import video_image_path, get_video_image_storage + logger = logging.getLogger(__name__) # pylint: disable=C0103 URL_REGEX = r'^[a-zA-Z0-9\-_]*$' +LIST_MAX_ITEMS = 3 class ModelFactoryWithValidation(object): @@ -35,6 +45,7 @@ def create_with_validation(cls, *args, **kwargs): ret_val = cls(*args, **kwargs) ret_val.full_clean() ret_val.save() + return ret_val @classmethod def get_or_create_with_validation(cls, *args, **kwargs): @@ -137,6 +148,13 @@ class Meta: # pylint: disable=C1001 """ unique_together = ("course_id", "video") + def image_url(self): + """ + Return image url for a course video image or None if no image. + """ + if hasattr(self, 'video_image'): + return self.video_image.image_url() + def __unicode__(self): return self.course_id @@ -155,6 +173,144 @@ class EncodedVideo(models.Model): video = models.ForeignKey(Video, related_name="encoded_videos") +class CustomizableImageField(models.ImageField): + """ + Subclass of ImageField that allows custom settings to not + be serialized (hard-coded) in migrations. Otherwise, + migrations include optional settings for storage (such as + the storage class and bucket name); we don't want to + create new migration files for each configuration change. + """ + def __init__(self, *args, **kwargs): + kwargs.update(dict( + upload_to=video_image_path, + storage=get_video_image_storage(), + max_length=500, # allocate enough for filepath + blank=True, + null=True + )) + super(CustomizableImageField, self).__init__(*args, **kwargs) + + def deconstruct(self): + """ + Override base class method. + """ + name, path, args, kwargs = super(CustomizableImageField, self).deconstruct() + del kwargs['upload_to'] + del kwargs['storage'] + del kwargs['max_length'] + return name, path, args, kwargs + + +class ListField(models.TextField): + """ + ListField use to store and retrieve list data. + """ + __metaclass__ = models.SubfieldBase + + def get_prep_value(self, value): + """ + Converts a list to its json represetation to store in database as text. + """ + return json.dumps(value) + + def to_python(self, value): + """ + Converts the value into a list. + """ + if not value: + value = [] + + # If a list is set then validated its items + if isinstance(value, list): + return self.validate(value) + else: # try to de-serialize value and expect list and then validate + try: + py_list = json.loads(value) + + if not isinstance(py_list, list): + raise TypeError + + self.validate(py_list) + except (ValueError, TypeError): + raise ValidationError(u'Must be a valid list of strings.') + + return py_list + + def validate(self, value): + """ + Validate data before saving to database. + + Arguemtns: + value(list): list to be validated + + Returns: + list if validation is successful + + Raises: + ValidationError + """ + if len(value) > LIST_MAX_ITEMS: + raise ValidationError(u'list must not contain more than {} items.'.format(LIST_MAX_ITEMS)) + + if all(isinstance(item, str) for item in value) is False: + raise ValidationError(u'list must only contain strings.') + + return value + + +class VideoImage(TimeStampedModel): + """ + Image model for course video. + """ + course_video = models.OneToOneField(CourseVideo, related_name="video_image") + image = CustomizableImageField() + generated_images = ListField() + + @classmethod + def create_or_update(cls, course_video, file_name, image_data=None): + """ + Create a VideoImage object for a CourseVideo. + + NOTE: If `image_data` is None then `file_name` value will be used as it is, otherwise + a new file name is constructed based on uuid and extension from `file_name` value. + `image_data` will be None in case of course re-run and export. + + Arguments: + course_video (CourseVideo): CourseVideo instance + file_name (str): File name of the image + image_data (InMemoryUploadedFile): Image data to be saved. + + Returns: + Returns a tuple of (video_image, created). + """ + video_image, created = cls.objects.get_or_create(course_video=course_video) + if image_data: + with closing(image_data) as image_file: + file_name = '{uuid}{ext}'.format(uuid=uuid4().hex, ext=os.path.splitext(file_name)[1]) + try: + video_image.image.save(file_name, image_file) + except Exception: # pylint: disable=broad-except + logger.exception( + 'VAL: Video Image save failed to storage for course_id [%s] and video_id [%s]', + course_video.course_id, + course_video.video.edx_video_id + ) + raise + else: + video_image.image.name = file_name + + video_image.save() + return video_image, created + + def image_url(self): + """ + Return image url for a course video image. + """ + storage = get_video_image_storage() + return storage.url(self.image.name) + + SUBTITLE_FORMATS = ( ('srt', 'SubRip'), ('sjson', 'SRT JSON') diff --git a/edxval/serializers.py b/edxval/serializers.py index 0568e09f..788ed6c0 100644 --- a/edxval/serializers.py +++ b/edxval/serializers.py @@ -7,7 +7,7 @@ from rest_framework import serializers from rest_framework.fields import IntegerField, DateTimeField -from edxval.models import Profile, Video, EncodedVideo, Subtitle, CourseVideo +from edxval.models import Profile, Video, EncodedVideo, Subtitle, CourseVideo, VideoImage class EncodedVideoSerializer(serializers.ModelSerializer): @@ -87,14 +87,27 @@ class CourseSerializer(serializers.RelatedField): """ Field for CourseVideo """ - def to_representation(self, value): - return value.course_id + def to_representation(self, course_video): + """ + Returns a serializable representation of a CourseVideo instance. + """ + return { + course_video.course_id: course_video.image_url() + } def to_internal_value(self, data): - if data: - course_video = CourseVideo(course_id=data) - course_video.full_clean(exclude=["video"]) - return course_video + """ + Convert data into CourseVideo instance and image filename tuple. + """ + if isinstance(data, basestring): + course_id, image = data, None + elif isinstance(data, dict): + (course_id, image), = data.items() + + course_video = CourseVideo(course_id=course_id) + course_video.full_clean(exclude=["video"]) + + return course_video, image class VideoSerializer(serializers.ModelSerializer): @@ -168,9 +181,12 @@ def create(self, validated_data): # The CourseSerializer will already have converted the course data # to CourseVideo models, so we can just set the video and save. - for course_video in courses: + # Also create VideoImage objects if an image filename is present + for course_video, image_name in courses: course_video.video = video course_video.save() + if image_name: + VideoImage.create_or_update(course_video, image_name) return video @@ -200,8 +216,11 @@ def update(self, instance, validated_data): # Set courses # NOTE: for backwards compatibility with the DRF v2 behavior, # we do NOT delete existing course videos during the update. - for course_video in validated_data.get("courses", []): + # Also update VideoImage objects if an image filename is present + for course_video, image_name in validated_data.get("courses", []): course_video.video = instance course_video.save() + if image_name: + VideoImage.create_or_update(course_video, image_name) return instance diff --git a/edxval/settings.py b/edxval/settings.py index 3b247338..b96a9784 100644 --- a/edxval/settings.py +++ b/edxval/settings.py @@ -130,6 +130,7 @@ # Third Party 'django_nose', 'rest_framework', + 'storages', # Our App 'edxval', @@ -178,3 +179,14 @@ # copied from edx-platform COURSE_KEY_PATTERN = r'(?P[^/+]+(/|\+)[^/+]+(/|\+)[^/]+)' COURSE_ID_PATTERN = COURSE_KEY_PATTERN.replace('course_key_string', 'course_id') + +VIDEO_IMAGE_SETTINGS = dict( + # Backend storage + # STORAGE_CLASS='storages.backends.s3boto.S3BotoStorage', + # STORAGE_KWARGS=dict(bucket='video-image-bucket'), + # If you are changing prefix value then update the .gitignore accordingly + # so that images created during tests due to upload should be ignored + VIDEO_IMAGE_MAX_BYTES=2097152, + VIDEO_IMAGE_MIN_BYTES=100, + DIRECTORY_PREFIX='video-images/', +) diff --git a/edxval/tests/data/__init__.py b/edxval/tests/data/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/edxval/tests/data/edx.jpg b/edxval/tests/data/edx.jpg new file mode 100644 index 00000000..ea8675cc Binary files /dev/null and b/edxval/tests/data/edx.jpg differ diff --git a/edxval/tests/data/image.jpg b/edxval/tests/data/image.jpg new file mode 100644 index 00000000..a982f845 Binary files /dev/null and b/edxval/tests/data/image.jpg differ diff --git a/edxval/tests/test_api.py b/edxval/tests/test_api.py index 574b245e..fa28dc93 100644 --- a/edxval/tests/test_api.py +++ b/edxval/tests/test_api.py @@ -7,13 +7,15 @@ from mock import patch from lxml import etree +from django.core.exceptions import ValidationError +from django.core.files.images import ImageFile from django.test import TestCase from django.db import DatabaseError from django.core.urlresolvers import reverse from rest_framework import status -from ddt import ddt, data +from ddt import ddt, data, unpack -from edxval.models import Profile, Video, EncodedVideo, CourseVideo +from edxval.models import Profile, Video, EncodedVideo, CourseVideo, VideoImage, LIST_MAX_ITEMS from edxval import api as api from edxval.api import ( SortDirection, @@ -792,9 +794,11 @@ def setUp(self): Creates a course with 2 videos and a course with 1 video """ self.course_id = 'test-course' + self.image_name1 = 'image.jpg' # 1st video self.video1 = Video.objects.create(**constants.VIDEO_DICT_FISH) - CourseVideo.objects.create(video=self.video1, course_id=self.course_id) + self.course_video1 = CourseVideo.objects.create(video=self.video1, course_id=self.course_id) + VideoImage.create_or_update(self.course_video1, self.image_name1) # 2nd video self.video2 = Video.objects.create(**constants.VIDEO_DICT_STAR) CourseVideo.objects.create(video=self.video2, course_id=self.course_id) @@ -805,10 +809,15 @@ def setUp(self): CourseVideo.objects.create(video=self.video3, course_id=self.course_id2) def test_successful_copy(self): - """Tests a successful copy course""" - api.copy_course_videos('test-course', 'course-copy1') - original_videos = Video.objects.filter(courses__course_id='test-course') - copied_videos = Video.objects.filter(courses__course_id='course-copy1') + """ + Tests a successful copy course + """ + destination_course_id = 'course-copy1' + api.copy_course_videos(self.course_id, destination_course_id) + original_videos = Video.objects.filter(courses__course_id=self.course_id) + copied_videos = Video.objects.filter(courses__course_id=destination_course_id) + course_video_with_image = CourseVideo.objects.get(video=self.video1, course_id=destination_course_id) + course_video_without_image = CourseVideo.objects.get(video=self.video2, course_id=destination_course_id) self.assertEqual(len(original_videos), 2) self.assertEqual( @@ -816,6 +825,8 @@ def test_successful_copy(self): {constants.VIDEO_DICT_FISH["edx_video_id"], constants.VIDEO_DICT_STAR["edx_video_id"]} ) self.assertTrue(set(original_videos) == set(copied_videos)) + self.assertEqual(course_video_with_image.video_image.image.name, self.image_name1) + self.assertFalse(hasattr(course_video_without_image, 'video_image')) def test_same_course_ids(self): """ @@ -852,6 +863,7 @@ def test_existing_video_in_destination_course_id(self): self.assertTrue(set(copied_videos) == set(original_videos)) +@ddt class ExportTest(TestCase): """Tests export_to_xml""" def setUp(self): @@ -860,6 +872,9 @@ def setUp(self): hls_profile = Profile.objects.get(profile_name=constants.PROFILE_HLS) Video.objects.create(**constants.VIDEO_DICT_STAR) video = Video.objects.create(**constants.VIDEO_DICT_FISH) + course_video = CourseVideo.objects.create(video=video, course_id='test-course') + VideoImage.create_or_update(course_video, 'image.jpg') + EncodedVideo.objects.create( video=video, profile=mobile_profile, @@ -898,23 +913,29 @@ def parse_xml(self, xml_str): def test_no_encodings(self): expected = self.parse_xml(""" - + """) self.assert_xml_equal( api.export_to_xml(constants.VIDEO_DICT_STAR["edx_video_id"]), expected ) - def test_basic(self): + @data( + {'course_id': None, 'image':''}, + {'course_id': 'test-course', 'image':'image.jpg'}, + ) + @unpack + def test_basic(self, course_id, image): expected = self.parse_xml(""" - + - """) + """.format(image=image)) + self.assert_xml_equal( - api.export_to_xml(constants.VIDEO_DICT_FISH["edx_video_id"]), + api.export_to_xml(constants.VIDEO_DICT_FISH['edx_video_id'], course_id), expected ) @@ -927,6 +948,7 @@ def test_unknown_video(self): class ImportTest(TestCase): """Tests import_from_xml""" def setUp(self): + self.image_name = 'image.jpg' mobile_profile = Profile.objects.create(profile_name=constants.PROFILE_MOBILE) Profile.objects.create(profile_name=constants.PROFILE_DESKTOP) video = Video.objects.create(**constants.VIDEO_DICT_FISH) @@ -937,24 +959,28 @@ def setUp(self): ) CourseVideo.objects.create(video=video, course_id='existing_course_id') - def make_import_xml(self, video_dict, encoded_video_dicts=None): - ret = etree.Element( + def make_import_xml(self, video_dict, encoded_video_dicts=None, image=None): + import_xml = etree.Element( "video_asset", attrib={ key: unicode(video_dict[key]) for key in ["client_video_id", "duration"] } ) + + if image: + import_xml.attrib['image'] = image + for encoding_dict in (encoded_video_dicts or []): etree.SubElement( - ret, + import_xml, "encoded_video", attrib={ key: unicode(val) for key, val in encoding_dict.items() } ) - return ret + return import_xml def assert_obj_matches_dict_for_keys(self, obj, dict_, keys): for key in keys: @@ -985,8 +1011,10 @@ def test_new_video_full(self): xml = self.make_import_xml( video_dict=constants.VIDEO_DICT_STAR, - encoded_video_dicts=[constants.ENCODED_VIDEO_DICT_STAR, constants.ENCODED_VIDEO_DICT_FISH_HLS] + encoded_video_dicts=[constants.ENCODED_VIDEO_DICT_STAR, constants.ENCODED_VIDEO_DICT_FISH_HLS], + image=self.image_name ) + api.import_from_xml(xml, constants.VIDEO_DICT_STAR["edx_video_id"], new_course_id) video = Video.objects.get(edx_video_id=constants.VIDEO_DICT_STAR["edx_video_id"]) @@ -999,7 +1027,8 @@ def test_new_video_full(self): video.encoded_videos.get(profile__profile_name=constants.PROFILE_HLS), constants.ENCODED_VIDEO_DICT_FISH_HLS ) - video.courses.get(course_id=new_course_id) + course_video = video.courses.get(course_id=new_course_id) + self.assertTrue(course_video.video_image.image.name, self.image_name) def test_new_video_minimal(self): edx_video_id = "test_edx_video_id" @@ -1036,7 +1065,8 @@ def test_existing_video(self, course_id): "bitrate": 1597804, "profile": "mobile", }, - ] + ], + image=self.image_name ) api.import_from_xml(xml, constants.VIDEO_DICT_FISH["edx_video_id"], course_id) @@ -1050,6 +1080,8 @@ def test_existing_video(self, course_id): video.encoded_videos.filter(profile__profile_name=constants.PROFILE_DESKTOP).exists() ) self.assertTrue(video.courses.filter(course_id=course_id).exists()) + course_video = video.courses.get(course_id=course_id) + self.assertTrue(course_video.video_image.image.name, self.image_name) def test_existing_video_with_invalid_course_id(self): @@ -1192,3 +1224,165 @@ def test_update_video_status_logging(self, mock_logger): 'fail', video.edx_video_id ) + + +class CourseVideoImageTest(TestCase): + """ + Tests to check course video image related functions works correctly. + """ + + def setUp(self): + """ + Creates video objects for courses. + """ + self.course_id = 'test-course' + self.course_id2 = 'test-course2' + self.video = Video.objects.create(**constants.VIDEO_DICT_FISH) + self.edx_video_id = self.video.edx_video_id + self.course_video = CourseVideo.objects.create(video=self.video, course_id=self.course_id) + self.course_video2 = CourseVideo.objects.create(video=self.video, course_id=self.course_id2) + self.image_path1 = 'edxval/tests/data/image.jpg' + self.image_path2 = 'edxval/tests/data/edx.jpg' + self.image_url = api.update_video_image( + self.edx_video_id, self.course_id, ImageFile(open(self.image_path1)), 'image.jpg' + ) + self.image_url2 = api.update_video_image( + self.edx_video_id, self.course_id2, ImageFile(open(self.image_path2)), 'image.jpg' + ) + + def test_update_video_image(self): + """ + Verify that `update_video_image` api function works as expected. + """ + self.assertEqual(self.course_video.video_image.image.name, self.image_url) + self.assertEqual(self.course_video2.video_image.image.name, self.image_url2) + self.assertEqual(ImageFile(open(self.image_path1)).size, ImageFile(open(self.image_url)).size) + self.assertEqual(ImageFile(open(self.image_path2)).size, ImageFile(open(self.image_url2)).size) + + def test_get_course_video_image_url(self): + """ + Verify that `get_course_video_image_url` api function works as expected. + """ + image_url = api.get_course_video_image_url(self.course_id, self.edx_video_id) + self.assertEqual(self.image_url, image_url) + + def test_get_course_video_image_url_no_image(self): + """ + Verify that `get_course_video_image_url` api function returns None when no image is found. + """ + self.course_video.video_image.delete() + image_url = api.get_course_video_image_url(self.course_id, self.edx_video_id) + self.assertIsNone(image_url) + + def test_num_queries_update_video_image(self): + """ + Test number of queries executed to upload a course video image. + """ + with self.assertNumQueries(4): + api.update_video_image( + self.edx_video_id, self.course_id, ImageFile(open(self.image_path1)), 'image.jpg' + ) + + def test_num_queries_get_course_video_image_url(self): + """ + Test number of queries executed to get a course video image url. + """ + with self.assertNumQueries(1): + api.get_course_video_image_url(self.course_id, self.edx_video_id) + + def test_get_videos_for_course(self): + """ + Verify that `get_videos_for_course` api function has correct course_video_image_url. + """ + video_data_generator = api.get_videos_for_course(self.course_id) + video_data = list(video_data_generator)[0] + self.assertEqual(video_data['courses'][0]['test-course'], self.image_url) + + def test_get_videos_for_ids(self): + """ + Verify that `get_videos_for_ids` api function returns response with course_video_image_url set to None. + """ + video_data_generator = api.get_videos_for_ids([self.edx_video_id]) + video_data = list(video_data_generator)[0] + self.assertEqual(video_data['courses'][0]['test-course'], self.image_url) + + @patch('edxval.models.logger') + def test_create_or_update_logging(self, mock_logger): + """ + Tests correct message is logged when save to storge is failed in `create_or_update`. + """ + with self.assertRaises(Exception) as save_exception: # pylint: disable=unused-variable + VideoImage.create_or_update(self.course_video, 'test.jpg', open(self.image_path2)) + + mock_logger.exception.assert_called_with( + 'VAL: Video Image save failed to storage for course_id [%s] and video_id [%s]', + self.course_video.course_id, + self.course_video.video.edx_video_id + ) + + def test_update_video_image_exception(self): + """ + Tests exception message when we hit by an exception in `update_video_image`. + """ + does_not_course_id = 'does_not_exist' + + with self.assertRaises(Exception) as get_exception: + api.update_video_image(self.edx_video_id, does_not_course_id, open(self.image_path2), 'test.jpg') + + self.assertEqual( + get_exception.exception.message, + u'VAL: CourseVideo not found for edx_video_id: {0} and course_id: {1}'.format( + self.edx_video_id, + does_not_course_id + ) + ) + + def test_video_image_urls_field(self): + """ + Test `VideoImage.generated_images` field works as expected. + """ + image_urls = ['video-images/a.png', 'video-images/b.png'] + + # an empty list should be returned when there is no value for urls + self.assertEqual(self.course_video.video_image.generated_images, []) + + # set a list with data and expect the same list to be returned + course_video = CourseVideo.objects.create(video=self.video, course_id='course101') + video_image = VideoImage.objects.create(course_video=course_video) + video_image.generated_images = image_urls + video_image.save() + self.assertEqual(video_image.generated_images, image_urls) + self.assertEqual(course_video.video_image.generated_images, image_urls) + + def test_video_image_urls_field_validation(self): + """ + Test `VideoImage.generated_images` field validation. + """ + course_video = CourseVideo.objects.create(video=self.video, course_id='course101') + video_image = VideoImage.objects.create(course_video=course_video) + + # expect a validation error if we try to set a list with more than 3 items + with self.assertRaises(ValidationError) as set_exception: + video_image.generated_images = ['a', 'b', 'c', 'd'] + + self.assertEqual( + set_exception.exception.message, + u'list must not contain more than {} items.'.format(LIST_MAX_ITEMS) + ) + + # expect a validation error if we try to a list with non-string items + with self.assertRaises(ValidationError) as set_exception: + video_image.generated_images = ['a', 1, 2] + + self.assertEqual(set_exception.exception.message, u'list must only contain strings.') + + # expect a validation error if we try to set non list data + exception_messages = set() + for item in ('a string', 555, {'a': 1}, (1,), video_image): + with self.assertRaises(ValidationError) as set_exception: + video_image.generated_images = item + + exception_messages.add(set_exception.exception.message) + + self.assertEqual(len(exception_messages), 1) + self.assertEqual(exception_messages.pop(), u'Must be a valid list of strings.') diff --git a/edxval/tests/test_views.py b/edxval/tests/test_views.py index 242b5d59..2fdb5428 100644 --- a/edxval/tests/test_views.py +++ b/edxval/tests/test_views.py @@ -580,7 +580,7 @@ def test_add_course(self): self.assertEqual(response.status_code, status.HTTP_201_CREATED) videos = self.client.get("/edxval/videos/").data self.assertEqual(len(videos), 1) - self.assertEqual(videos[0]['courses'], [course1, course2]) + self.assertEqual(videos[0]['courses'], [{course1: None}, {course2: None}]) url = reverse('video-list') + '?course=%s' % course1 videos = self.client.get(url).data diff --git a/edxval/utils.py b/edxval/utils.py new file mode 100644 index 00000000..e08cc950 --- /dev/null +++ b/edxval/utils.py @@ -0,0 +1,31 @@ +""" +Util methods to be used in api and models. +""" + +from django.conf import settings +from django.core.files.storage import get_storage_class + + +def video_image_path(video_image_instance, filename): # pylint:disable=unused-argument + """ + Returns video image path. + + Arguments: + video_image_instance (VideoImage): This is passed automatically by models.CustomizableImageField + filename (str): name of image file + """ + return u'{}{}'.format(settings.VIDEO_IMAGE_SETTINGS.get('DIRECTORY_PREFIX', ''), filename) + + +def get_video_image_storage(): + """ + Return the configured django storage backend. + """ + if hasattr(settings, 'VIDEO_IMAGE_SETTINGS'): + return get_storage_class( + settings.VIDEO_IMAGE_SETTINGS.get('STORAGE_CLASS'), + )(**settings.VIDEO_IMAGE_SETTINGS.get('STORAGE_KWARGS', {})) + else: + # during edx-platform loading this method gets called but settings are not ready yet + # so in that case we will return default(FileSystemStorage) storage class instance + return get_storage_class()() diff --git a/requirements.txt b/requirements.txt index 8d39c928..b1e33a84 100644 --- a/requirements.txt +++ b/requirements.txt @@ -4,3 +4,6 @@ enum34==1.0.4 lxml==3.3.6 -e git+https://github.com/edx/django-oauth2-provider.git@0.2.7-fork-edx-6a#egg=django-oauth2-provider==0.2.7-fork-edx-6 -e git+https://github.com/edx/django-rest-framework-oauth.git@f0b503fda8c254a38f97fef802ded4f5fe367f7a#egg=djangorestframework-oauth +django-storages==1.5.2 +boto==2.46.1 +django-model-utils==2.3.1 diff --git a/setup.py b/setup.py index 719faccd..68b9bba3 100644 --- a/setup.py +++ b/setup.py @@ -39,7 +39,7 @@ def load_requirements(*requirements_paths): setup( name='edxval', - version='0.0.13', + version='0.0.14', author='edX', url='http://github.com/edx/edx-val', description='edx-val',