address feedback

.gitignore

@@ -67,4 +67,4 @@ logs/*/*.log*
 
 venv/
 
-videoimage/
+video-image/

edxval/admin.py

@@ -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,7 +33,8 @@ class VideoAdmin(admin.ModelAdmin):  # pylint: disable=C0111
     admin_order_field = 'edx_video_id'
     inlines = [CourseVideoInline, EncodedVideoInline]
 
-class ImageVideoAdmin(admin.ModelAdmin):
+
+class VideoImageAdmin(admin.ModelAdmin):
     model = VideoImage
     verbose_name = "Video Image"
     verbose_name_plural = "Video Images"
@@ -38,4 +42,4 @@ class ImageVideoAdmin(admin.ModelAdmin):
 admin.site.register(Profile, ProfileAdmin)
 admin.site.register(Video, VideoAdmin)
 admin.site.register(Subtitle)
-admin.site.register(VideoImage, ImageVideoAdmin)
+admin.site.register(VideoImage, VideoImageAdmin)

edxval/api.py

@@ -187,7 +187,7 @@ def update_video_image(edx_video_id, course_id, image_data, file_name):
         error_message = u"CourseVideo not found for edx_video_id: {0}".format(edx_video_id)
         raise ValVideoNotFoundError(error_message)
 
-    video_image, _ = VideoImage.create(course_video, image_data, file_name)
+    video_image, _ = VideoImage.create_or_update(course_video, image_data, file_name)
     return get_course_video_image_url(video_image=video_image)
 
 
@@ -353,7 +353,7 @@ def get_videos_for_course(course_id, sort_field=None, sort_dir=SortDirection.asc
         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,
         context={'course_id': course_id}

edxval/migrations/0005_videoimage.py

@@ -20,7 +20,7 @@ class Migration(migrations.Migration):
                 ('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.CustomizableFileField(null=True, blank=True)),
+                ('image', edxval.models.CustomizableImageField(null=True, blank=True)),
                 ('course_video', models.OneToOneField(related_name='video_image', to='edxval.CourseVideo')),
             ],
             options={

edxval/models.py

@@ -15,13 +15,13 @@ 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 model_utils.models import TimeStampedModel
+
 from edxval.utils import video_image_path, get_video_image_storage
 
 logger = logging.getLogger(__name__)  # pylint: disable=C0103
@@ -78,32 +78,6 @@ class Profile(models.Model):
         return self.profile_name
 
 
-class CustomizableFileField(models.ImageField):
-    """
-    Subclass of FileField 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(CustomizableFileField, self).__init__(*args, **kwargs)
-
-    def deconstruct(self):
-        name, path, args, kwargs = super(CustomizableFileField, self).deconstruct()
-        del kwargs['upload_to']
-        del kwargs['storage']
-        del kwargs['max_length']
-        return name, path, args, kwargs
-
-
 class Video(models.Model):
     """
     Model for a Video group with the same content.
@@ -187,15 +161,44 @@ 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 VideoImage(TimeStampedModel):
     """
     Image model for course video.
     """
     course_video = models.OneToOneField(CourseVideo, related_name="video_image")
-    image = CustomizableFileField()
+    image = CustomizableImageField()
 
     @classmethod
-    def create(cls, course_video, image_data, file_name):
+    def create_or_update(cls, course_video, image_data, file_name):
         """
         Create a VideoImage object for a CourseVideo.
 

edxval/settings.py

@@ -183,8 +183,8 @@ 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-test-bucket'),
+    # 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
-    DIRECTORY_PREFIX='videoimage/',
+    DIRECTORY_PREFIX='video-image/',
 )

edxval/tests/test_api.py

@@ -1212,27 +1212,11 @@ class CourseVideoImageTest(TestCase):
         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'
+        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):