Refactor create_or_update VideoTranscript to be capabale to update transcript…

Refactor create_or_update VideoTranscript to be capabale to update transcript file_name/file_data, language_code, file_format and provider.

edxval/api.py

@@ -296,42 +296,35 @@ def get_video_transcript_url(video_id, language_code):
         return video_transcript.url()
 
 
-def create_or_update_video_transcript(
-        video_id,
-        language_code,
-        file_name,
-        file_format,
-        provider,
-        file_data=None,
-    ):
+def create_or_update_video_transcript(video_id, language_code, metadata, file_data=None):
     """
     Create or Update video transcript for an existing video.
 
     Arguments:
         video_id: it can be an edx_video_id or an external_id extracted from external sources in a video component.
         language_code: language code of a video transcript
-        file_name: file name of a video transcript
+        metadata (dict): A dict containing (to be overwritten) properties
         file_data (InMemoryUploadedFile): Transcript data to be saved for a course video.
-        file_format: format of the transcript
-        provider: transcript provider
 
     Returns:
         video transcript url
     """
-    if file_format not in dict(TranscriptFormat.CHOICES).keys():
+    # Filter wanted properties
+    metadata = {
+        prop: value
+        for prop, value in metadata.iteritems()
+        if prop in ['provider', 'language_code', 'file_name', 'file_format'] and value
+    }
+
+    file_format = metadata.get('file_format')
+    if file_format and file_format not in dict(TranscriptFormat.CHOICES).keys():
         raise InvalidTranscriptFormat('{} transcript format is not supported'.format(file_format))
 
-    if provider not in dict(TranscriptProviderType.CHOICES).keys():
+    provider = metadata.get('provider')
+    if provider and provider not in dict(TranscriptProviderType.CHOICES).keys():
         raise InvalidTranscriptProvider('{} transcript provider is not supported'.format(provider))
 
-    video_transcript, __ = VideoTranscript.create_or_update(
-        video_id,
-        language_code,
-        file_name,
-        file_format,
-        provider,
-        file_data,
-    )
+    video_transcript, __ = VideoTranscript.create_or_update(video_id, language_code, metadata, file_data)
 
     return video_transcript.url()
 
@@ -944,9 +937,11 @@ def create_transcript_objects(xml):
             VideoTranscript.create_or_update(
                 transcript.attrib['video_id'],
                 transcript.attrib['language_code'],
-                transcript.attrib['file_name'],
-                transcript.attrib['file_format'],
-                transcript.attrib['provider'],
+                metadata=dict(
+                    provider=transcript.attrib['provider'],
+                    file_name=transcript.attrib['file_name'],
+                    file_format=transcript.attrib['file_format'],
+                )
             )
         except KeyError:
             logger.warn("VAL: Required attributes are missing from xml, xml=[%s]", etree.tostring(transcript).strip())

edxval/models.py

@@ -456,16 +456,14 @@ class VideoTranscript(TimeStampedModel):
         return transcript
 
     @classmethod
-    def create_or_update(cls, video_id, language_code, file_name, file_format, provider, file_data=None):
+    def create_or_update(cls, video_id, language_code, metadata, file_data=None):
         """
         Create or update Transcript object.
 
         Arguments:
             video_id (str): unique id for a video
-            language_code (str): language code
-            file_name (str): File name of the image
-            file_format (str): Format of transcript
-            provider (str): Transcript provider
+            language_code (str): language code for (to be created/updated) transcript
+            metadata (dict): A dict containing (to be overwritten) properties
             file_data (InMemoryUploadedFile): File data to be saved
 
         Returns:
@@ -473,20 +471,24 @@ class VideoTranscript(TimeStampedModel):
         """
         video_transcript, created = cls.objects.get_or_create(video_id=video_id, language_code=language_code)
 
-        # delete the existing transcript file
-        if not created and file_data:
-            video_transcript.transcript.delete()
+        for prop, value in metadata.iteritems():
+            if prop in ['language_code', 'file_format', 'provider']:
+                setattr(video_transcript, prop, value)
 
-        video_transcript.transcript.name = file_name
-        video_transcript.file_format = file_format
-        video_transcript.provider = provider
+        transcript_name = metadata.get('file_name')
+        if transcript_name:
+            video_transcript.transcript.name = transcript_name
+        elif file_data:
+            # Delete the existing transcript file and
+            # recreate with the new content
+            if not created:
+                video_transcript.transcript.delete()
 
-        if file_data:
             with closing(file_data) as transcript_file_data:
-                file_name = '{uuid}{ext}'.format(uuid=uuid4().hex, ext=os.path.splitext(file_name)[1])
+                file_name = '{uuid}.{ext}'.format(uuid=uuid4().hex, ext=video_transcript.file_format)
                 try:
                     video_transcript.transcript.save(file_name, transcript_file_data)
-                except Exception:  # pylint: disable=broad-except
+                except Exception:
                     logger.exception('VAL: Transcript save failed to storage for video_id [%s]', video_id)
                     raise
 

edxval/tests/test_api.py

@@ -1697,10 +1697,8 @@ class TranscriptTest(TestCase):
         self.transcript_url = api.create_or_update_video_transcript(
             self.video_id,
             'ur',
-            'The_Arrow.srt',
-            TranscriptFormat.SRT,
-            provider=TranscriptProviderType.CUSTOM,
-            file_data=File(open(self.arrow_transcript_path)),
+            metadata={'file_format': TranscriptFormat.SRT},
+            file_data=File(open(self.arrow_transcript_path))
         )
 
         # create a temporary transcript file
@@ -1846,9 +1844,11 @@ class TranscriptTest(TestCase):
         transcript_url = api.create_or_update_video_transcript(
             video_id=transcript_data['video_id'],
             language_code=transcript_data['language_code'],
-            file_name=transcript_data['name'],
-            file_format=transcript_data['file_format'],
-            provider=transcript_data['provider'],
+            metadata=dict(
+                file_name=transcript_data['name'],
+                file_format=transcript_data['file_format'],
+                provider=transcript_data['provider']
+            )
         )
         self.assertEqual(transcript_url, transcript_data['name'])
 
@@ -1877,32 +1877,49 @@ class TranscriptTest(TestCase):
     @data(
         {
             'file_data': None,
+            'file_name': 'overwatch.sjson',
             'file_format': TranscriptFormat.SJSON,
+            'language_code': 'da',
             'provider': TranscriptProviderType.CIELO24
         },
         {
             'file_data': ContentFile(FILE_DATA),
+            'file_name': None,
             'file_format': TranscriptFormat.SRT,
+            'language_code': 'es',
             'provider': TranscriptProviderType.THREE_PLAY_MEDIA
         },
     )
     @unpack
-    def test_create_or_update_video_transcript(self, file_data, file_format, provider):
+    def test_create_or_update_video_transcript(self, file_data, file_name, file_format, language_code, provider):
         """
         Verify that `create_or_update_video_transcript` api function updates existing transcript as expected.
         """
         video_transcript = VideoTranscript.objects.get(video_id=self.video_id, language_code='ur')
         self.assertIsNotNone(video_transcript)
 
-        file_name = 'overwatch.{}'.format(file_format)
         transcript_url = api.create_or_update_video_transcript(
-            self.video_id, 'ur', file_name, file_format, provider, file_data
+            video_id=self.video_id,
+            language_code='ur',
+            metadata=dict(
+                provider=provider,
+                language_code=language_code,
+                file_name=file_name,
+                file_format=file_format
+            ),
+            file_data=file_data
         )
-        video_transcript = VideoTranscript.objects.get(video_id=self.video_id, language_code='ur')
 
+        # Now, Querying Video Transcript with previous/old language code leads to DoesNotExist
+        with self.assertRaises(VideoTranscript.DoesNotExist):
+            VideoTranscript.objects.get(video_id=self.video_id, language_code='ur')
+
+        # Assert the updates to the transcript object
+        video_transcript = VideoTranscript.objects.get(video_id=self.video_id, language_code=language_code)
         self.assertEqual(transcript_url, video_transcript.url())
         self.assertEqual(video_transcript.file_format, file_format)
         self.assertEqual(video_transcript.provider, provider)
+        self.assertEqual(video_transcript.language_code, language_code)
 
         if file_data:
             self.assertTrue(transcript_url.startswith(settings.VIDEO_TRANSCRIPTS_SETTINGS['DIRECTORY_PREFIX']))
@@ -1932,7 +1949,10 @@ class TranscriptTest(TestCase):
         Verify that `create_or_update_video_transcript` api function raise exceptions on invalid values.
         """
         with self.assertRaises(exception) as transcript_exception:
-            api.create_or_update_video_transcript(self.video_id, 'ur', 'overwatch.srt', file_format, provider)
+            api.create_or_update_video_transcript(self.video_id, 'ur', metadata={
+                'provider': provider,
+                'file_format': file_format
+            })
 
         self.assertEqual(transcript_exception.exception.message, exception_message)
 
@@ -1946,12 +1966,10 @@ class TranscriptTest(TestCase):
 
         # This will replace the transcript for an existing video and delete the existing transcript
         new_transcript_url = api.create_or_update_video_transcript(
-            self.video_id,
-            'ur',
-            'overwatch.srt',
-            TranscriptFormat.SRT,
-            TranscriptProviderType.CIELO24,
-            ContentFile(FILE_DATA)
+            video_id=self.video_id,
+            language_code='ur',
+            metadata=dict(provider=TranscriptProviderType.CIELO24),
+            file_data=ContentFile(FILE_DATA)
         )
 
         # Verify that new transcript is set to video

edxval/views.py

@@ -4,9 +4,7 @@ Views file for django app edxval.
 import logging
 
 from django.core.exceptions import ValidationError
-from django.http import HttpResponse
 from django.shortcuts import get_object_or_404
-from django.views.decorators.http import last_modified
 from rest_framework import generics, status
 from rest_framework.authentication import SessionAuthentication
 from rest_framework.permissions import DjangoModelPermissions
@@ -14,12 +12,16 @@ from rest_framework.response import Response
 from rest_framework.views import APIView
 from rest_framework_oauth.authentication import OAuth2Authentication
 
-from edxval.api import (create_or_update_video_transcript,
-                        get_video_transcript, update_video_status)
-from edxval.models import (CourseVideo, Profile, TranscriptFormat,
-                           TranscriptProviderType, Video, VideoImage,
-                           VideoTranscript)
-from edxval.serializers import TranscriptSerializer, VideoSerializer
+from edxval.api import create_or_update_video_transcript
+from edxval.models import (
+    CourseVideo,
+    TranscriptFormat,
+    TranscriptProviderType,
+    Video,
+    VideoImage,
+    VideoTranscript
+)
+from edxval.serializers import VideoSerializer
 
 LOGGER = logging.getLogger(__name__)  # pylint: disable=C0103
 
@@ -148,13 +150,11 @@ class VideoTranscriptView(APIView):
 
         transcript = VideoTranscript.get_or_none(video_id, language_code)
         if transcript is None:
-            create_or_update_video_transcript(
-                video_id,
-                language_code,
-                transcript_name,
-                file_format,
-                provider,
-            )
+            create_or_update_video_transcript(video_id, language_code, metadata={
+                'provider': provider,
+                'file_name': transcript_name,
+                'file_format': file_format
+            })
             response = Response(status=status.HTTP_200_OK)
         else:
             message = (