html5 transcript exists

[Shrhawk]

TNL-3168

Steps to Reproduce:
1.Create a video component with youtube and html(mp4) url
2.Save the component
3.Edit the component and click import from youtube(transcript)
4.Go to assets page of an course (transcript with youtube_id only)
Problem:
editor_saved is not syncing subs for an edge case above.
Solution:
At editor_saved check if youtube_sub already exists and there is no html5_subs present for html5_resources then create new html5_subs.

[Shrhawk]

jenkins run bokchoy

[Shrhawk]

@adampalay @waheedahmed @mushtaqak have a review !

[adampalay]

why old_metadata.get('html5_sources', [])? Why not old_metadata.get('html5_sources', False) or just old_metadata.get('html5_sources')

[Shrhawk]

ok will replace with old_metadata.get('html5_sources', False)

[muzaffaryousaf]

html5_sources is a list and i prefer to set the default values accordingly.

[muzaffaryousaf]

old_metadata.get('html5_sources') this is fine too.

[muzaffaryousaf]

Can you shed some light on this self.sub, what's the purpose ?

[Shrhawk]

The purpose of self.sub is to check that there should transcript attached to video and then we will sure this this part of code will only run when there is already transcript present because we need transcript for copy or renaming with different htmls_id.

[Shrhawk]

@muzaffaryousaf, @muhammad-ammar kindly review !

[Shrhawk]

@muzaffaryousaf kindly review !

[muzaffaryousaf]

@Shrhawk Sure. I will take a look in early morning.

[muzaffaryousaf]

@Shrhawk Do we have unit test coverage for this newly added code, if not, can we add some unit tests ?

[muzaffaryousaf]

NVM, i see you have added the tests.

[muzaffaryousaf]

What if we get html5_sources from self/item instead of old_metadata because manage_video_subtitles_save uses item.html5_sources method instead of old_metadata to copy/rename, if values are different for both the result will be unexpected.

[muzaffaryousaf]

The same call is happening in manage_video_subtitles_save method, it will be nice if we can reduce the call, somehow.

[Shrhawk]

great !!

[muzaffaryousaf]

@Shrhawk I'm done with first pass, let me know when you are done with changes.
Thanks.

[Shrhawk]

@muzaffaryousaf i have make changes kindly review it !

[Shrhawk]

@adampalay add both module-stores as you suggested !

[adampalay]

@Shrhawk , looks like you need to address 3 quality violations:

19:58:32 common/lib/xmodule/xmodule/video_module/video_module.py (94.4%):
19:58:32     432: C7630: (literal-used-as-attribute), VideoDescriptor.editor_saved: getattr using a literal attribute name
19:58:32 lms/djangoapps/courseware/tests/test_video_mongo.py (97.3%):
19:58:32     895: E1120: (no-value-for-parameter), TestEditorSavedMethod.setUp: No value for argument 'self' in unbound method call
19:58:32     905: C0103: (invalid-name), TestEditorSavedMethod.test_editor_saved_when_html5_sub_not_exist: Invalid attribute name "MODULESTORE"

Otherwise, this looks good.

[muzaffaryousaf]

@Shrhawk getattr(self, 'html5_sources') will raise AttributeError if self don't have html5_sources attribute.
https://docs.python.org/2/library/functions.html#getattr

[muzaffaryousaf]

@Shrhawk 👍 Good to go once you fix the quality issue and one inline comment & don't forget to squash the commits. Thanks.

[Shrhawk]

@adampalay done thumps up ?

[adampalay]

since [] if falsey, you should instead use hasattr(self, 'html5_sources'). Then in the next line, you don't need to use getattr.

[Shrhawk]

done @adampalay have a review again !

[adampalay]

you've already checked that self has html5_sources, so you don't need getattr here

[Shrhawk]

nice finding :)

[adampalay]

not sure why tests failed...

[Shrhawk]

jenkins run python

[Shrhawk]

jenkins run bokchoy

[Shrhawk]

@adampalay tests are passing now !!