support video images for course re-run and import/export

.travis.yml

@@ -10,5 +10,6 @@ script:
 branches:
     only:
       - master
+      - ammar/course-rerun-import-export
 after_success:
   coveralls

edxval/api.py

@@ -61,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:
@@ -147,7 +148,9 @@ 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.get(course_id=course_id, video__edx_video_id=edx_video_id).video_image
+        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
@@ -157,6 +160,10 @@ 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.
 
@@ -167,9 +174,14 @@ def update_video_image(edx_video_id, course_id, image_data, file_name):
         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)
+        course_video = CourseVideo.objects.select_related('video').get(
+            course_id=course_id, video__edx_video_id=edx_video_id
+        )
     except ObjectDoesNotExist:
-        error_message = u'CourseVideo not found for edx_video_id: {0}'.format(edx_video_id)
+        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)
@@ -476,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
@@ -498,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():
@@ -548,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)
@@ -563,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')

edxval/models.py

@@ -211,6 +211,10 @@ class VideoImage(TimeStampedModel):
         """
         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
@@ -224,14 +228,12 @@ class VideoImage(TimeStampedModel):
             with closing(image_data) as image_file:
                 file_name = '{uuid}{ext}'.format(uuid=uuid4().hex, ext=os.path.splitext(file_name)[1])
                 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
+                        course_video.course_id,
+                        course_video.video.edx_video_id
                     )
                     raise
         else:

edxval/serializers.py

@@ -182,11 +182,11 @@ class VideoSerializer(serializers.ModelSerializer):
         # The CourseSerializer will already have converted the course data
         # to CourseVideo models, so we can just set the video and save.
         # Also create VideoImage objects if an image filename is present
-        for course_video, image in courses:
+        for course_video, image_name in courses:
             course_video.video = video
             course_video.save()
-            if image:
-                VideoImage.create_or_update(course_video, image)
+            if image_name:
+                VideoImage.create_or_update(course_video, image_name)
 
         return video
 
@@ -217,10 +217,10 @@ class VideoSerializer(serializers.ModelSerializer):
         # NOTE: for backwards compatibility with the DRF v2 behavior,
         # we do NOT delete existing course videos during the update.
         # Also update VideoImage objects if an image filename is present
-        for course_video, image in validated_data.get("courses", []):
+        for course_video, image_name in validated_data.get("courses", []):
             course_video.video = instance
             course_video.save()
-            if image:
-                VideoImage.create_or_update(course_video, image)
+            if image_name:
+                VideoImage.create_or_update(course_video, image_name)
 
         return instance

edxval/tests/test_api.py

@@ -12,7 +12,7 @@ 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, VideoImage
 from edxval import api as api
@@ -793,9 +793,11 @@ class TestCopyCourse(TestCase):
         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)
@@ -806,10 +808,15 @@ class TestCopyCourse(TestCase):
         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(
@@ -817,6 +824,8 @@ class TestCopyCourse(TestCase):
             {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):
         """
@@ -853,6 +862,7 @@ class TestCopyCourse(TestCase):
         self.assertTrue(set(copied_videos) == set(original_videos))
 
 
+@ddt
 class ExportTest(TestCase):
     """Tests export_to_xml"""
     def setUp(self):
@@ -861,6 +871,9 @@ class ExportTest(TestCase):
         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,
@@ -899,23 +912,29 @@ class ExportTest(TestCase):
 
     def test_no_encodings(self):
         expected = self.parse_xml("""
-            <video_asset client_video_id="TWINKLE TWINKLE" duration="122.0"/>
+            <video_asset client_video_id="TWINKLE TWINKLE" duration="122.0" image=""/>
         """)
         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("""
-            <video_asset client_video_id="Shallow Swordfish" duration="122.0">
+            <video_asset client_video_id="Shallow Swordfish" duration="122.0" image="{image}">
                 <encoded_video url="http://www.meowmix.com" file_size="11" bitrate="22" profile="mobile"/>
                 <encoded_video url="http://www.meowmagic.com" file_size="33" bitrate="44" profile="desktop"/>
                 <encoded_video url="https://www.tmnt.com/tmnt101.m3u8" file_size="100" bitrate="0" profile="hls"/>
             </video_asset>
-        """)
+        """.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
         )
 
@@ -928,6 +947,7 @@ class ExportTest(TestCase):
 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)
@@ -938,24 +958,28 @@ class ImportTest(TestCase):
         )
         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:
@@ -986,8 +1010,10 @@ class ImportTest(TestCase):
 
         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"])
@@ -1000,7 +1026,8 @@ class ImportTest(TestCase):
             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"
@@ -1037,7 +1064,8 @@ class ImportTest(TestCase):
                     "bitrate": 1597804,
                     "profile": "mobile",
                 },
-            ]
+            ],
+            image=self.image_name
         )
         api.import_from_xml(xml, constants.VIDEO_DICT_FISH["edx_video_id"], course_id)
 
@@ -1051,6 +1079,8 @@ class ImportTest(TestCase):
             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):
@@ -1197,12 +1227,12 @@ class VideoStatusUpdateTest(TestCase):
 
 class CourseVideoImageTest(TestCase):
     """
-    Tests to check course video image related functions works correctly
+    Tests to check course video image related functions works correctly.
     """
 
     def setUp(self):
         """
-        Creates video objects for courses
+        Creates video objects for courses.
         """
         self.course_id = 'test-course'
         self.course_id2 = 'test-course2'
@@ -1243,6 +1273,22 @@ class CourseVideoImageTest(TestCase):
         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.
@@ -1262,7 +1308,7 @@ class CourseVideoImageTest(TestCase):
     @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`
+        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))
@@ -1272,3 +1318,20 @@ class CourseVideoImageTest(TestCase):
             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
+            )
+        )

setup.py

@@ -39,7 +39,7 @@ def load_requirements(*requirements_paths):
 
 setup(
     name='edxval',
-    version='0.0.13',
+    version='0.0.15',
     author='edX',
     url='http://github.com/edx/edx-val',
     description='edx-val',