address feedback

edxval/admin.py

@@ -36,8 +36,8 @@ class VideoAdmin(admin.ModelAdmin):  # pylint: disable=C0111
 
 class VideoImageAdmin(admin.ModelAdmin):
     model = VideoImage
-    verbose_name = "Video Image"
-    verbose_name_plural = "Video Images"
+    verbose_name = 'Video Image'
+    verbose_name_plural = 'Video Images'
 
 admin.site.register(Profile, ProfileAdmin)
 admin.site.register(Video, VideoAdmin)

edxval/api.py

@@ -13,7 +13,6 @@ from django.core.files.base import ContentFile
 
 from edxval.models import Video, EncodedVideo, CourseVideo, Profile, VideoImage
 from edxval.serializers import VideoSerializer
-from edxval.utils import get_video_image_storage
 from edxval.exceptions import (  # pylint: disable=unused-import
     ValError,
     ValInternalError,
@@ -145,25 +144,15 @@ def update_video_status(edx_video_id, status):
 
 def get_course_video_image_url(course_id=None, edx_video_id=None, video_image=None):
     """
-    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 or None if no image found
+    Returns course video image url or None if no image found
     """
-    storage = get_video_image_storage()
-
     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:
         return None
 
-    return storage.url(video_image.image.name)
+    return video_image.image_url()
 
 
 def update_video_image(edx_video_id, course_id, image_data, file_name):
@@ -171,22 +160,22 @@ def update_video_image(edx_video_id, course_id, image_data, file_name):
     Update video image for an existing video.
 
     Arguments:
-        edx_video_id: ID of the video.
-        course_id: ID of course.
-        image_data: Image data for video.
-        file_name: File name of the image file.
+        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.get(course_id=course_id, video__edx_video_id=edx_video_id)
-    except Video.DoesNotExist:
+    except ObjectDoesNotExist:
         error_message = u'CourseVideo not found for edx_video_id: {0}'.format(edx_video_id)
         raise ValVideoNotFoundError(error_message)
 
     video_image, _ = VideoImage.create_or_update(course_video, file_name, image_data)
-    return get_course_video_image_url(video_image=video_image)
+    return video_image.image_url()
 
 
 def create_profile(profile_name):

edxval/models.py

@@ -145,6 +145,15 @@ class CourseVideo(models.Model, ModelFactoryWithValidation):
         """
         unique_together = ("course_id", "video")
 
+    def image_url(self):
+        """
+        Return image url for a course video image.
+        """
+        try:
+            return self.video_image.image_url()
+        except VideoImage.DoesNotExist:
+            pass
+
     def __unicode__(self):
         return self.course_id
 
@@ -216,21 +225,29 @@ class VideoImage(TimeStampedModel):
         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])
-                video_image.image.save(file_name, image_file)
+                try:
+                    course_id = course_video.course_id
+                    edx_video_id = course_video.video.edx_video_id
+                    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_id,
+                        edx_video_id
+                    )
+                    raise
         else:
             video_image.image.name = file_name
 
         video_image.save()
         return video_image, created
 
-    @classmethod
-    def image_url(cls, course_video):
+    def image_url(self):
         """
         Return image url for a course video image.
         """
         storage = get_video_image_storage()
-        if hasattr(course_video, 'video_image'):
-            return storage.url(course_video.video_image.image.name)
+        return storage.url(self.image.name)
 
 
 SUBTITLE_FORMATS = (

edxval/serializers.py

@@ -92,7 +92,7 @@ class CourseSerializer(serializers.RelatedField):
         Returns a serializable representation of a CourseVideo instance.
         """
         return {
-            course_video.course_id: VideoImage.image_url(course_video)
+            course_video.course_id: course_video.image_url()
         }
 
     def to_internal_value(self, data):

edxval/tests/test_api.py

@@ -14,7 +14,7 @@ from django.core.urlresolvers import reverse
 from rest_framework import status
 from ddt import ddt, data
 
-from edxval.models import Profile, Video, EncodedVideo, CourseVideo
+from edxval.models import Profile, Video, EncodedVideo, CourseVideo, VideoImage
 from edxval import api as api
 from edxval.api import (
     SortDirection,
@@ -1258,3 +1258,17 @@ class CourseVideoImageTest(TestCase):
         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
+        )

edxval/utils.py

@@ -9,8 +9,12 @@ 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 '{}{}'.format(settings.VIDEO_IMAGE_SETTINGS.get('DIRECTORY_PREFIX', ''), filename)
+    return u'{}{}'.format(settings.VIDEO_IMAGE_SETTINGS.get('DIRECTORY_PREFIX', ''), filename)
 
 
 def get_video_image_storage():