update storage and tests

.gitignore

@@ -66,3 +66,5 @@ logs/*/*.log*
 .vagrant
 
 venv/
+
+videoimage/

AUTHORS

 Christopher Lee <clee@edx.org>
 Mushtaq Ali <mushtaak@gmail.com>
+Muhammad Ammar <mammar@gmail.com>

edxval/api.py

 # 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, VideoImage
 from edxval.serializers import VideoSerializer
-
-from utils import get_video_image_storage
+from edxval.utils import get_video_image_storage
+from edxval.exceptions import (  # pylint: disable=unused-import
+    ValError,
+    ValInternalError,
+    ValVideoNotFoundError,
+    ValCannotCreateError,
+    ValCannotUpdateError,
+    ValVideoImageNotFoundError
+)
 
 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"
@@ -185,49 +144,51 @@ def update_video_status(edx_video_id, status):
     video.save()
 
 
-def get_video_image_url(video_image, image_filename=None):
+def get_course_video_image_url(course_id=None, edx_video_id=None, video_image=None):
     """
-    Returns video image url.
+    Returns course video image url.
+
+    Arguments:
+        course_id: ID of course
+        edx_video_id: ID of the video
+        video_image: VideoImage instance
+
+    Returns:
+        course video image url
     """
     storage = get_video_image_storage()
-    # TODO: Is getting file path using video_image.image.name a good way? or Do we need to get file path using
-    # video_image_path_name(video_image, file_name)
-    file_name = video_image.image.name
-    video_image_url = storage.url(file_name)
-    return video_image_url
 
+    try:
+        if video_image is None:
+            video_image = CourseVideo.objects.get(course_id=course_id, video__edx_video_id=edx_video_id).video_image
+    except ObjectDoesNotExist:
+        raise ValVideoImageNotFoundError
+
+    return storage.url(video_image.image.name)
 
 
-def update_video_image(edx_video_id, course_id, serialized_data, file_name):
+def update_video_image(edx_video_id, course_id, image_data, file_name):
     """
     Update video image for an existing video.
 
-    Args:
+    Arguments:
         edx_video_id: ID of the video.
         course_id: ID of course.
-        serialized_data: Serialized data of image for video.
+        image_data: Image data for video.
         file_name: File name of the image file.
 
     Raises:
-        Raises ValVideoNotFoundError if the video cannot be retrieved.
+        Raises ValVideoNotFoundError if the CourseVideo cannot be retrieved.
+        Raises ValVideoImageNotFoundError if the VideoImage cannot be retrieved.
     """
-    # TODO: Is sending file name a good way ?
-
     try:
-        video = _get_video(edx_video_id)
+        course_video = CourseVideo.objects.get(course_id=course_id, video__edx_video_id=edx_video_id)
     except Video.DoesNotExist:
-        error_message = u"Video not found when trying to update video image with edx_video_id: {0}".format(
-            edx_video_id
-        )
+        error_message = u"CourseVideo not found for edx_video_id: {0}".format(edx_video_id)
         raise ValVideoNotFoundError(error_message)
 
-    video_image, _ = VideoImage.objects.get_or_create(
-        video=video,
-        course_id=course_id
-    )
-    video_image.image.save(file_name, ContentFile(serialized_data))
-    video_image.save()
-    return get_video_image_url(video_image, file_name)
+    video_image, _ = VideoImage.create(course_video, image_data, file_name)
+    return get_course_video_image_url(video_image=video_image)
 
 
 def create_profile(profile_name):
@@ -362,11 +323,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, context=None):
     """
     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
@@ -378,14 +335,10 @@ def _get_videos_for_filter(
         videos = videos.order_by(sort_field.value, "edx_video_id")
         if sort_dir == SortDirection.desc:
             videos = videos.reverse()
-    return (VideoSerializer(video).data for video in videos)
+    return (VideoSerializer(video, context=context).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.
 
@@ -403,6 +356,7 @@ def get_videos_for_course(
         {"courses__course_id": unicode(course_id), "courses__is_hidden": False},
         sort_field,
         sort_dir,
+        context={'course_id': course_id}
     )
 
 

edxval/exceptions.py

+"""
+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
+
+
+class ValVideoImageNotFoundError(ValError):
+    """
+    This error is raised when a video image is not found
+    """
+    pass

edxval/migrations/0005_videoimage.py

@@ -2,6 +2,8 @@
 from __future__ import unicode_literals
 
 from django.db import migrations, models
+import django.utils.timezone
+import model_utils.fields
 import edxval.models
 
 
@@ -16,9 +18,13 @@ class Migration(migrations.Migration):
             name='VideoImage',
             fields=[
                 ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)),
-                ('course_id', models.CharField(max_length=255)),
+                ('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.CustomizableFileField(null=True, blank=True)),
-                ('video', models.ForeignKey(related_name='thumbnails', to='edxval.Video')),
+                ('course_video', models.OneToOneField(related_name='video_image', to='edxval.CourseVideo')),
             ],
+            options={
+                'abstract': False,
+            },
         ),
     ]

edxval/models.py

@@ -11,14 +11,18 @@ themselves. After these are resolved, errors such as a negative file_size or
 invalid profile_name will be returned.
 """
 
+from contextlib import closing
 import logging
+import os
+
+from model_utils.models import TimeStampedModel
 
 from django.db import models
 from django.dispatch import receiver
 from django.core.validators import MinValueValidator, RegexValidator
 from django.core.urlresolvers import reverse
 
-from utils import video_image_path_name, get_video_image_storage
+from edxval.utils import video_image_path, get_video_image_storage
 
 logger = logging.getLogger(__name__)  # pylint: disable=C0103
 
@@ -84,7 +88,7 @@ class CustomizableFileField(models.ImageField):
     """
     def __init__(self, *args, **kwargs):
         kwargs.update(dict(
-            upload_to=video_image_path_name,
+            upload_to=video_image_path,
             storage=get_video_image_storage(),
             max_length=500,  # allocate enough for filepath
             blank=True,
@@ -183,14 +187,35 @@ class EncodedVideo(models.Model):
     video = models.ForeignKey(Video, related_name="encoded_videos")
 
 
-class VideoImage(models.Model):
+class VideoImage(TimeStampedModel):
     """
     Image model for course video.
     """
-    course_id = models.CharField(max_length=255)
-    video = models.ForeignKey(Video, related_name="thumbnails")
+    course_video = models.OneToOneField(CourseVideo, related_name="video_image")
     image = CustomizableFileField()
 
+    @classmethod
+    def create(cls, course_video, image_data, file_name):
+        """
+        Create a VideoImage object for a CourseVideo.
+
+        Arguments:
+            course_video (CourseVideo): CourseVideo instance
+            image_data (InMemoryUploadedFile): Image data to be saved.
+            file_name (str): File name.
+
+        Returns:
+            Returns a tuple of (video_image, created).
+        """
+        video_image, created = cls.objects.get_or_create(course_video=course_video)
+        with closing(image_data) as image_file:
+            __, file_extension = os.path.splitext(file_name)
+            file_name = '{timestamp}{ext}'.format(timestamp=video_image.modified.strftime("%s"), ext=file_extension)
+            video_image.image.save(file_name, image_file)
+            video_image.save()
+
+        return video_image, created
+
 
 SUBTITLE_FORMATS = (
     ('srt', 'SubRip'),

edxval/serializers.py

@@ -8,6 +8,7 @@ from rest_framework import serializers
 from rest_framework.fields import IntegerField, DateTimeField
 
 from edxval.models import Profile, Video, EncodedVideo, Subtitle, CourseVideo
+from edxval.exceptions import ValVideoImageNotFoundError
 
 
 class EncodedVideoSerializer(serializers.ModelSerializer):
@@ -111,6 +112,7 @@ class VideoSerializer(serializers.ModelSerializer):
         required=False,
         queryset=CourseVideo.objects.all()
     )
+    course_video_image_url = serializers.SerializerMethodField()
     url = serializers.SerializerMethodField()
 
     # Django Rest Framework v3 converts datetimes to unicode by default.
@@ -122,6 +124,21 @@ class VideoSerializer(serializers.ModelSerializer):
         lookup_field = "edx_video_id"
         exclude = ('id',)
 
+    def get_course_video_image_url(self, video):
+        """
+        Return image associated with a course video or None if course_id is missing or if there is any error.
+        """
+        # Imported here to avoid circular dependency
+        from edxval.api import get_course_video_image_url
+
+        if self.context is None or 'course_id' not in self.context:
+            return None
+
+        try:
+            return get_course_video_image_url(self.context['course_id'], video.edx_video_id)
+        except ValVideoImageNotFoundError:
+            return None
+
     def get_url(self, obj):
         """
         Return relative url for the object

edxval/settings.py

@@ -184,5 +184,7 @@ VIDEO_IMAGE_SETTINGS = dict(
     # Backend storage
     # STORAGE_CLASS='storages.backends.s3boto.S3BotoStorage',
     # STORAGE_KWARGS=dict(bucket='video-image-test-bucket'),
+    # If you are changing prefix value then update the .gitignore accordingly
+    # so that images created during tests due to upload should be ignored
     DIRECTORY_PREFIX='videoimage/',
 )

edxval/tests/test_api.py

@@ -7,6 +7,7 @@ import mock
 from mock import patch
 from lxml import etree
 
+from django.core.files.images import ImageFile
 from django.test import TestCase
 from django.db import DatabaseError
 from django.core.urlresolvers import reverse
@@ -1192,3 +1193,76 @@ class VideoStatusUpdateTest(TestCase):
             '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 = self.upload_image(self.course_id, self.edx_video_id, self.image_path1)
+        self.image_url2 = self.upload_image(self.course_id2, self.edx_video_id, self.image_path2)
+
+    def upload_image(self, course_id, edx_video_id, image_path):
+        """
+        Upload image.
+
+        Arguemnts:
+            course_id: ID of course.
+            edx_video_id: ID of the video.
+            image_path: Physical path of image
+
+        Returns:
+            upload image url
+        """
+        return api.update_video_image(
+            edx_video_id,
+            course_id,
+            ImageFile(open(image_path)),
+            '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_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['course_video_image_url'], self.image_url)
+
+    def test_get_videos_for_ids(self):
+        """
+        Verify that `get_videos_for_ids` api function returns reponse 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['course_video_image_url'], None)

edxval/utils.py

@@ -6,25 +6,11 @@ from django.conf import settings
 from django.core.files.storage import get_storage_class
 
 
-def _create_path(directory, filename):
+def video_image_path(video_image_instance, filename):  # pylint:disable=unused-argument
     """
-    Returns the full path for the given directory and filename.
+    Returns video image path.
     """
-    return '{}-{}'.format(directory, filename)
-
-
-def _directory_name(edx_video_id):
-    """
-    Returns the directory name for the given edx_video_id.
-    """
-    return '{}{}'.format(settings.VIDEO_IMAGE_SETTINGS.get('DIRECTORY_PREFIX', ''), edx_video_id)
-
-
-def video_image_path_name(image_model, filename):  # pylint:disable=unused-argument
-    """
-    Returns path name to use for the given Video instance.
-    """
-    return _create_path(_directory_name(image_model.video.edx_video_id), filename)
+    return '{}{}'.format(settings.VIDEO_IMAGE_SETTINGS.get('DIRECTORY_PREFIX', ''), filename)
 
 
 def get_video_image_storage():
@@ -32,7 +18,5 @@ def get_video_image_storage():
     Return the configured django storage backend.
     """
     return get_storage_class(
-         settings.VIDEO_IMAGE_SETTINGS.get('STORAGE_CLASS'),
+        settings.VIDEO_IMAGE_SETTINGS.get('STORAGE_CLASS'),
     )(**settings.VIDEO_IMAGE_SETTINGS.get('STORAGE_KWARGS', {}))
-
-

edxval/views.py

@@ -56,8 +56,8 @@ class VideoList(generics.ListCreateAPIView):
     """
     GETs or POST video objects
     """
-    # authentication_classes = (OAuth2Authentication, SessionAuthentication)
-    # permission_classes = (ReadRestrictedDjangoModelPermissions,)
+    authentication_classes = (OAuth2Authentication, SessionAuthentication)
+    permission_classes = (ReadRestrictedDjangoModelPermissions,)
     queryset = Video.objects.all().prefetch_related("encoded_videos", "courses")
     lookup_field = "edx_video_id"
     serializer_class = VideoSerializer

requirements.txt

@@ -7,3 +7,4 @@ lxml==3.3.6
 django-storages==1.5.2
 boto==2.46.1
 Pillow==4.1.0
+django-model-utils==2.3.1