Add ability for direct 3PlayMedia transcripts usage #251

wowkalucky · 2017-07-04T12:43:21Z

General implementation of direct (streaming) transcripts usage from 3PlayMedia service.

Till the PR VideoXBlock already has ability to use transcripts from 3PlayMedia service. But those '3playmedia' transcripts were used by downloading them into xblock as course assets.
So, if any changes was made on the 3PlayMedia service to mirror those changes on the xblock User should manually perform downloading each time.

The Client requested ability of VideoXBlock to use 3PlayMedia's transcripts directly (so, changes on 3PM service take effect immediately).

And PR adds described functionality.

codecov · 2017-07-04T12:43:24Z

Codecov Report

Merging #251 into dev will increase coverage by 0.77%.
The diff coverage is 93.36%.

@@            Coverage Diff            @@
##              dev    #251      +/-   ##
=========================================
+ Coverage   84.12%   84.9%   +0.77%     
=========================================
  Files          41      41              
  Lines        2381    2530     +149     
  Branches       26      26              
=========================================
+ Hits         2003    2148     +145     
- Misses        377     381       +4     
  Partials        1       1

Flag	Coverage Δ
#acceptance	`88.7% <93.27%> (+0.61%)`	⬆️
#unit	`83.04% <93.36%> (+0.89%)`	⬆️

Impacted Files	Coverage Δ
video_xblock/backends/base.py	`88.88% <ø> (ø)`	⬆️
...deo_xblock/static/js/spec/runtime-handlers-spec.js	`100% <ø> (ø)`	⬆️
video_xblock/static/js/runtime-handlers.js	`100% <ø> (ø)`	⬆️
video_xblock/tests/unit/test_backends.py	`99.54% <100%> (ø)`	⬆️
video_xblock/tests/unit/test_video_xblock.py	`100% <100%> (ø)`	⬆️
video_xblock/utils.py	`97.91% <100%> (+6.61%)`	⬆️
...eo_xblock/tests/unit/test_video_xblock_handlers.py	`100% <100%> (ø)`	⬆️
video_xblock/backends/vimeo.py	`98.91% <100%> (ø)`	⬆️
video_xblock/tests/unit/test_mixins.py	`100% <100%> (ø)`	⬆️
video_xblock/constants.py	`100% <100%> (ø)`	⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 231ce0c...39219f2. Read the comment docs.

z4y4ts

Looks good in general, few issues need to be addressed before a merge.

z4y4ts · 2017-07-05T22:07:39Z

video_xblock/backends/base.py

@@ -132,8 +132,8 @@ def advanced_fields(self):
        """
        return [
            'start_time', 'end_time', 'handout', 'transcripts',
-            'threeplaymedia_file_id', 'threeplaymedia_apikey', 'download_transcript_allowed',
-            'default_transcripts', 'download_video_allowed', 'download_video_url'
+            'threeplaymedia_file_id', 'threeplaymedia_apikey', 'direct_enabled',


direct_enabled -> threeplaymedia_streaming or threeplaymedia_direct_enabled

z4y4ts · 2017-07-05T22:09:19Z

video_xblock/mixins.py

@@ -58,6 +56,31 @@ class TranscriptsMixin(XBlock):
    TranscriptsMixin class to encapsulate transcripts-related logic.
    """

+    THREE_PLAY_MEDIA_API_DOMAIN = 'https://static.3playmedia.com/'
+
+    direct_enabled = Boolean(


direct_enabled -> threeplaymedia_streaming or threeplaymedia_direct_enabled

z4y4ts · 2017-07-05T22:10:03Z

video_xblock/mixins.py

+    direct_enabled = Boolean(
+        default=False,
+        display_name=_('Direct 3PlayMedia'),
+        scope=Scope.settings,


Scope.content

Ok. But what if we would like to use 3PM streaming only in one course Unit?

Fields are scoped to a single xblock instance. So no problem here.

z4y4ts · 2017-07-05T22:11:02Z

video_xblock/mixins.py

                tran['url'] = self.runtime.handler_url(
-                    self, 'srt_to_vtt', query=tran['url']
+                    self, 'fetch_from_tree_play_media', query="{}={}".format(tran['lang_id'], tran['id'])


tree -> three

z4y4ts · 2017-07-05T22:11:49Z

video_xblock/mixins.py

                )
+            else:
+                if not tran['url'].endswith('.vtt'):


else if -> elif

z4y4ts · 2017-07-05T22:33:11Z

video_xblock/video_xblock.py

+            transcripts = json.loads(self.transcripts) if self.transcripts else []
+
+        for transcript in transcripts:
+            yield transcript


This for loop looks useless.
Why don't just return transcripts?

z4y4ts · 2017-07-05T22:33:31Z

video_xblock/utils.py

+
+
+Transcript = namedtuple('Transcript', [
+    'id', 'label', 'lang', 'lang_id', 'content', 'format', 'video_id', 'source', 'url'


z4y4ts · 2017-07-05T22:37:24Z

video_xblock/mixins.py

@@ -369,7 +501,7 @@ def player_state(self):
        """
        Return video player state as a dictionary.
        """
-        transcripts = json.loads(self.transcripts) if self.transcripts else []
+        transcripts = list(self.get_enabled_transcripts())


Looking few lines below, this conversion to a list seems redundant.

z4y4ts · 2017-07-05T22:39:15Z

video_xblock/static/js/studio-edit/transcripts-manual-upload.js

    'use strict';
    var isValid = [];
    var $visibleLangChoiceItems = $langChoiceItem.find('li:visible');
    var urls;
-    e.preventDefault();


z4y4ts · 2017-07-05T22:39:23Z

video_xblock/static/js/studio-edit/transcripts-manual-upload.js

@@ -53,12 +53,11 @@ function getTranscriptUrl(transcriptsArray, langCode) {
 /**
 * Validate transcript data before save it to video xblock.
 */
-function validateTranscripts(e, $langChoiceItem) {
+function validateTranscripts($langChoiceItem) {


auraz · 2017-07-06T15:18:19Z

video_xblock/mixins.py

+
+    def get_available_3pm_transcripts(self, file_id, apikey):
+        """
+        Make API request to fetch list of available transcripts for given file ID.


Add difference of return types of API in documentation

auraz · 2017-07-06T15:19:02Z

video_xblock/mixins.py

+            results = response.json()
+        else:
+            results = {"failed": True}
+        return results


Proposal: return response.json(), status

auraz · 2017-07-06T15:23:12Z

video_xblock/mixins.py

+            raise StopIteration
+        for transcript_data in results:
+            transcript = self.fetch_3pm_translation(transcript_data)
+            transcript_ordered_dict = transcript._asdict()


Please add test for empty transcript

Think if we need to show this error to user.

auraz · 2017-07-06T15:24:53Z

video_xblock/mixins.py

+        for transcript_data in results:
+            transcript = self.fetch_3pm_translation(transcript_data)
+            transcript_ordered_dict = transcript._asdict()
+            transcript_ordered_dict['content'] = ''  # we don't want to parse it to JSON


extend comment.

auraz · 2017-07-06T15:26:51Z

video_xblock/mixins.py

+            content = requests.get(external_api_url).text
+        except Exception:  # pylint: disable=broad-except
+            log.exception(_("Transcript fetching failure: language [{}]").format(TPMApiLanguage(lang_id)))
+            return


Please think about consistency in corner cases. Here you do return, but in def init(self, language_id): you do raise.

auraz · 2017-07-06T15:33:59Z

video_xblock/mixins.py

+        Returns:
+            webob.Response: WebVTT transcripts wrapped in Response object.
+        """
+        lang_id, tid = request.query_string.split('=')


what is tid, please use better naming.

auraz · 2017-07-06T15:35:26Z

video_xblock/mixins.py

+
+        results = self.get_available_3pm_transcripts(file_id, api_key)
+
+        is_valid = False if isinstance(results, dict) else True


auraz · 2017-07-06T15:38:15Z

video_xblock/static/js/studio-edit/studio-edit.js

+            data: JSON.stringify(data)
+        };
+
+        if (!(data.api_key && data.file_id)) {


add docstring and comment

auraz · 2017-07-06T15:39:37Z

video_xblock/static/js/studio-edit/studio-edit.js

+            options
+        )
+        .done(function(response) {
+            message = response.message;


Why you need message?

auraz · 2017-07-06T15:40:30Z

video_xblock/static/js/studio-edit/studio-edit.js

+     */
+    function getThreePlayMediaConfig() {
+        var $apiKey = $('.threeplaymedia-api-key', element).val();
+        var $fileId = $('#xb-field-edit-threeplaymedia_file_id', element).val();


I do not think you need $ here.

z4y4ts

Looks good in general.
I have only a few concerns here.

z4y4ts · 2017-07-10T11:57:01Z

video_xblock/constants.py


 DEFAULT_LANG = 'en'

+# STATUS - (instance of namedtuple) - may be used like: "STATUS.success", returns status string value.
+STATUS = namedtuple('STATUS', ['success', 'error', 'warning'])._make(['success', 'error', 'warning'])


You actually need an enum here, not a namedtuple.
See here

z4y4ts · 2017-07-10T12:05:00Z

video_xblock/mixins.py

            )
-        }
+        except Exception:  # pylint: disable=broad-except


Please, narrow down exception catching.

z4y4ts · 2017-07-10T12:16:53Z

video_xblock/mixins.py

+    @XBlock.handler
+    def validate_three_play_media_config(self, request, _suffix=''):
+        """
+        Handler to validate actuality of provided API credentials.


Handler to validate provided API credentials.

z4y4ts · 2017-07-10T12:18:19Z

video_xblock/mixins.py

+        """
+        api_key = request.json.get('api_key')
+        file_id = request.json.get('file_id')
+        streaming_enabled = bool(int(request.json.get('streaming_enabled')))


Add a docstring saying what values do you expect to receive here.
E.g. # streaming_enabled is expected to be 1 or 0

z4y4ts · 2017-07-10T14:22:08Z

video_xblock/tests/unit/test_utils.py

+        # Act
+        normalized_transcripts = normalize_transcripts(test_transcripts)
+        # Assert
+        self.assertEqual(test_transcripts, normalized_transcripts)


So essentially, you are checking, that normalization did nothing with the input data.

I think you need to add few more cases. Or change test name to better reflect your intent.

All points have been addressed

…nscripts-settings-accordion' and 'feature/transcripts-settings-accordion' of github.com:raccoongang/xblock-video into feature/transcripts-settings-accordion * 'feature/transcripts-settings-accordion' of github.com:raccoongang/xblock-video: Add ability for direct 3PlayMedia transcripts usage (#251) * 'feature/transcripts-settings-accordion' of github.com:raccoongang/xblock-video: Add ability for direct 3PlayMedia transcripts usage (#251) * 'feature/transcripts-settings-accordion' of github.com:raccoongang/xblock-video: Add ability for direct 3PlayMedia transcripts usage (#251)

Add basic "direct" 3PlayMedia transcript functionality

3a9b6b2

wowkalucky requested a review from z4y4ts July 4, 2017 12:43

wowkalucky added 2 commits July 4, 2017 17:12

Add proxy transcript fetching handler

2f0f942

Fix unit-tests

dcad13b

wowkalucky force-pushed the 259-serve-transcripts-and-captions-from-3pm-directly branch 3 times, most recently from c4df361 to a5dfde0 Compare July 5, 2017 09:05

Add js 3PlayMedia credentials validation draft

d7dbcea

wowkalucky force-pushed the 259-serve-transcripts-and-captions-from-3pm-directly branch from a5dfde0 to d7dbcea Compare July 5, 2017 09:07

z4y4ts changed the base branch from master to dev July 5, 2017 12:52

Implement validation handler of 3PlayMedia configuration

be8f77e

wowkalucky force-pushed the 259-serve-transcripts-and-captions-from-3pm-directly branch from f605f23 to be8f77e Compare July 5, 2017 18:51

z4y4ts reviewed Jul 5, 2017

View reviewed changes

wowkalucky added 4 commits July 6, 2017 10:17

Refactor: direct_enabled >> threeplaymedia_streaming

3b566c4

Change threeplaymedia_streaming field scope

5085620

Fix typo

18d46d2

Fix code review complaints

870c379

wowkalucky force-pushed the 259-serve-transcripts-and-captions-from-3pm-directly branch 5 times, most recently from 24ad935 to 2750ed8 Compare July 6, 2017 11:16

Improve test-py coverage

2488f40

wowkalucky force-pushed the 259-serve-transcripts-and-captions-from-3pm-directly branch from 2750ed8 to 2488f40 Compare July 6, 2017 11:17

Make utility interface consistent

a37020d

wowkalucky force-pushed the 259-serve-transcripts-and-captions-from-3pm-directly branch from 1ed0f96 to a37020d Compare July 6, 2017 12:27

wowkalucky requested a review from auraz July 6, 2017 12:43

Fix 3PlayMedia validation handler

92af3f3

auraz previously requested changes Jul 6, 2017

View reviewed changes

wowkalucky added 3 commits July 6, 2017 21:57

Fix exception cases

675d955

Fix BrightCove authentication error

9e8f9a5

Refactor validation of 3PlayMedia options

c0fbacd

wowkalucky force-pushed the 259-serve-transcripts-and-captions-from-3pm-directly branch from d4f3283 to c0fbacd Compare July 7, 2017 07:39

wowkalucky added 3 commits July 7, 2017 11:33

Fix unicode chars in token error

067063b

Remove deprecated 3PlayMedia transcript loading

10f5ce9

Add debug logging for default transcripts updating

17316cf

wowkalucky force-pushed the 259-serve-transcripts-and-captions-from-3pm-directly branch from 1662d86 to 17316cf Compare July 7, 2017 12:32

wowkalucky added 7 commits July 7, 2017 17:23

Refactor 3PlayMedia utilities interface, fix tests, fix quality

ca4a5dd

Fix variables naming

5c75b16

Add unit test for 3PlayMedia API failure case

75eca7e

Add unit test for 3PlayMedia validation handler: positive test case

63bd9ee

Cover with unit tests 3PlayMedia validation handler

e66000d

Add additional transcripts configuration hint for staff

9d1b3d5

Refactor error message variable

b4f8588

z4y4ts suggested changes Jul 10, 2017

View reviewed changes

wowkalucky added 2 commits July 10, 2017 23:45

Refactor STATUS to Enum

81e44fb

Fix review complaints

0981258

wowkalucky mentioned this pull request Jul 11, 2017

Wistia-234: Fix the error while trying to upload default transcripts. #252

Merged

z4y4ts assigned wowkalucky Jul 11, 2017

z4y4ts approved these changes Jul 11, 2017

View reviewed changes

Update CHANGELOG.md

39219f2

wowkalucky merged commit d750c68 into dev Jul 11, 2017

wowkalucky deleted the 259-serve-transcripts-and-captions-from-3pm-directly branch July 11, 2017 18:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability for direct 3PlayMedia transcripts usage #251

Add ability for direct 3PlayMedia transcripts usage #251

wowkalucky commented Jul 4, 2017 •

edited

Loading

codecov bot commented Jul 4, 2017 •

edited

Loading

z4y4ts left a comment

z4y4ts Jul 5, 2017

z4y4ts Jul 5, 2017

z4y4ts Jul 5, 2017

wowkalucky Jul 6, 2017

z4y4ts Jul 11, 2017

z4y4ts Jul 5, 2017

z4y4ts Jul 5, 2017

z4y4ts Jul 5, 2017

z4y4ts Jul 5, 2017

z4y4ts Jul 5, 2017

z4y4ts Jul 5, 2017

z4y4ts Jul 5, 2017

auraz Jul 6, 2017

auraz Jul 6, 2017

auraz Jul 6, 2017

auraz Jul 6, 2017

auraz Jul 6, 2017

auraz Jul 6, 2017

auraz Jul 6, 2017

auraz Jul 6, 2017

auraz Jul 6, 2017

auraz Jul 6, 2017

z4y4ts left a comment

z4y4ts Jul 10, 2017

z4y4ts Jul 10, 2017

z4y4ts Jul 10, 2017

z4y4ts Jul 10, 2017

z4y4ts Jul 10, 2017



		Transcript = namedtuple('Transcript', [
		'id', 'label', 'lang', 'lang_id', 'content', 'format', 'video_id', 'source', 'url'


		results = self.get_available_3pm_transcripts(file_id, api_key)

		is_valid = False if isinstance(results, dict) else True

Add ability for direct 3PlayMedia transcripts usage #251

Add ability for direct 3PlayMedia transcripts usage #251

Conversation

wowkalucky commented Jul 4, 2017 • edited Loading

codecov bot commented Jul 4, 2017 • edited Loading

Codecov Report

z4y4ts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

z4y4ts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wowkalucky commented Jul 4, 2017 •

edited

Loading

codecov bot commented Jul 4, 2017 •

edited

Loading