html5 transcript exists #9924

Shrhawk · 2015-09-28T09:28:27Z

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 · 2015-09-30T19:46:13Z

jenkins run bokchoy

Shrhawk · 2015-10-08T13:49:18Z

@adampalay @waheedahmed @mushtaqak have a review !

adampalay · 2015-10-14T13:08:31Z

common/lib/xmodule/xmodule/video_module/video_module.py

@@ -424,6 +424,16 @@ def editor_saved(self, user, old_metadata, old_content):
        This should be fixed too.
        """
        metadata_was_changed_by_user = old_metadata != own_metadata(self)
+        # If metadata is not changed by user and html5_sub does not exist.
+        if self.sub and old_metadata.get('html5_sources', []) and not metadata_was_changed_by_user:


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

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

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

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

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

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 · 2015-10-22T12:18:46Z

@muzaffaryousaf, @muhammad-ammar kindly review !

Shrhawk · 2015-10-26T13:20:18Z

@muzaffaryousaf kindly review !

muzaffaryousaf · 2015-10-26T14:01:06Z

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

muzaffaryousaf · 2015-10-27T08:26:36Z

common/lib/xmodule/xmodule/video_module/video_module.py

@@ -424,6 +424,16 @@ def editor_saved(self, user, old_metadata, old_content):
        This should be fixed too.
        """
        metadata_was_changed_by_user = old_metadata != own_metadata(self)
+        # If metadata is not changed by user and html5_sub does not exist.
+        if self.sub and old_metadata.get('html5_sources', []) and not metadata_was_changed_by_user:
+            html5_ids = get_html5_ids(old_metadata['html5_sources'])


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

NVM, i see you have added the tests.

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.

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

muzaffaryousaf · 2015-10-27T09:25:36Z

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

Shrhawk · 2015-10-30T11:14:51Z

@muzaffaryousaf i have make changes kindly review it !

Shrhawk · 2015-10-30T11:16:04Z

lms/djangoapps/courseware/tests/test_video_mongo.py

+        self.test_dir = path(__file__).abspath().dirname().dirname().dirname().dirname().dirname()
+        self.file_path = self.test_dir + '/common/test/data/uploads/' + self.file_name
+
+    @ddt.data(TEST_DATA_MONGO_MODULESTORE, TEST_DATA_SPLIT_MODULESTORE)


@adampalay add both module-stores as you suggested !

adampalay · 2015-10-30T14:41:52Z

@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 · 2015-11-02T07:49:32Z

common/lib/xmodule/xmodule/video_module/video_module.py

+        # then there is a syncing issue where html5_subs are not syncing with youtube sub, We can make sync better by
+        # checking if transcript is present for the video and if any html5_ids transcript is not present then trigger
+        # the manage_video_subtitles_save to create the missing transcript with particular html5_id.
+        if not metadata_was_changed_by_user and self.sub and getattr(self, 'html5_sources'):


@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 · 2015-11-02T07:54:24Z

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

Shrhawk · 2015-11-02T12:45:50Z

@adampalay done thumps up ?

adampalay · 2015-11-02T13:58:38Z

common/lib/xmodule/xmodule/video_module/video_module.py

+        # then there is a syncing issue where html5_subs are not syncing with youtube sub, We can make sync better by
+        # checking if transcript is present for the video and if any html5_ids transcript is not present then trigger
+        # the manage_video_subtitles_save to create the missing transcript with particular html5_id.
+        if not metadata_was_changed_by_user and self.sub and getattr(self, 'html5_sources', []):


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

done @adampalay have a review again !

adampalay · 2015-11-03T13:41:26Z

common/lib/xmodule/xmodule/video_module/video_module.py

+        # checking if transcript is present for the video and if any html5_ids transcript is not present then trigger
+        # the manage_video_subtitles_save to create the missing transcript with particular html5_id.
+        if not metadata_was_changed_by_user and self.sub and hasattr(self, 'html5_sources'):
+            html5_ids = get_html5_ids(getattr(self, 'html5_sources', []))


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

nice finding :)

adampalay · 2015-11-04T16:20:47Z

not sure why tests failed...

Shrhawk · 2015-11-04T17:18:34Z

jenkins run python

Shrhawk · 2015-11-04T17:19:10Z

jenkins run bokchoy

Shrhawk · 2015-11-05T13:29:56Z

@adampalay tests are passing now !!

adampalay · 2015-11-05T15:29:50Z

@Shrhawk you still need to squash your commits. Then 👍

…e_html5_subs html5 transcript exists

Shrhawk force-pushed the shr/bug/TNL-3168-import_from_youtube_html5_subs branch 2 times, most recently from 73f0f3e to c386011 Compare October 8, 2015 13:47

adampalay reviewed Oct 14, 2015
View reviewed changes

muzaffaryousaf reviewed Oct 27, 2015
View reviewed changes

Shrhawk reviewed Oct 30, 2015
View reviewed changes

muzaffaryousaf reviewed Nov 2, 2015
View reviewed changes

Shrhawk force-pushed the shr/bug/TNL-3168-import_from_youtube_html5_subs branch 2 times, most recently from 1587512 to 378b332 Compare November 2, 2015 13:43

adampalay reviewed Nov 2, 2015
View reviewed changes

Shrhawk force-pushed the shr/bug/TNL-3168-import_from_youtube_html5_subs branch from 340a02b to f92a766 Compare November 3, 2015 12:51

adampalay reviewed Nov 3, 2015
View reviewed changes

Shrhawk force-pushed the shr/bug/TNL-3168-import_from_youtube_html5_subs branch from 26685f9 to a8c8053 Compare November 6, 2015 11:33

Shrhawk added a commit that referenced this pull request Nov 6, 2015

Merge pull request #9924 from edx/shr/bug/TNL-3168-import_from_youtub…

ab8aedc

…e_html5_subs html5 transcript exists

Shrhawk merged commit ab8aedc into master Nov 6, 2015

html5 transcript sync

a8c8053

benpatterson deleted the shr/bug/TNL-3168-import_from_youtube_html5_subs branch August 2, 2016 13:10

anonim-01 mentioned this pull request May 13, 2024

[Snyk] Fix for 2 vulnerabilities anonim-01/open-okul-lms#9

Open

sumodgeorge mentioned this pull request May 14, 2024

[Snyk] Fix for 2 vulnerabilities sumodgeorge/edx-platform#33

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

html5 transcript exists #9924

html5 transcript exists #9924

Shrhawk commented Sep 28, 2015

Shrhawk commented Sep 30, 2015

Shrhawk commented Oct 8, 2015

adampalay Oct 14, 2015

Shrhawk Oct 15, 2015

muzaffaryousaf Oct 27, 2015

muzaffaryousaf Oct 27, 2015

muzaffaryousaf Oct 27, 2015

Shrhawk Oct 28, 2015

Shrhawk commented Oct 22, 2015

Shrhawk commented Oct 26, 2015

muzaffaryousaf commented Oct 26, 2015

muzaffaryousaf Oct 27, 2015

muzaffaryousaf Oct 27, 2015

muzaffaryousaf Oct 27, 2015

muzaffaryousaf Oct 27, 2015

Shrhawk Oct 28, 2015

muzaffaryousaf commented Oct 27, 2015

Shrhawk commented Oct 30, 2015

Shrhawk Oct 30, 2015

adampalay commented Oct 30, 2015

muzaffaryousaf Nov 2, 2015

muzaffaryousaf commented Nov 2, 2015

Shrhawk commented Nov 2, 2015

adampalay Nov 2, 2015

Shrhawk Nov 3, 2015

adampalay Nov 3, 2015

Shrhawk Nov 4, 2015

adampalay commented Nov 4, 2015

Shrhawk commented Nov 4, 2015

Shrhawk commented Nov 4, 2015

Shrhawk commented Nov 5, 2015

adampalay commented Nov 5, 2015

html5 transcript exists #9924

html5 transcript exists #9924

Conversation

Shrhawk commented Sep 28, 2015

Shrhawk commented Sep 30, 2015

Shrhawk commented Oct 8, 2015

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

Shrhawk commented Oct 22, 2015

Shrhawk commented Oct 26, 2015

muzaffaryousaf commented Oct 26, 2015

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

muzaffaryousaf commented Oct 27, 2015

Shrhawk commented Oct 30, 2015

Choose a reason for hiding this comment

adampalay commented Oct 30, 2015

Choose a reason for hiding this comment

muzaffaryousaf commented Nov 2, 2015

Shrhawk commented Nov 2, 2015

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

adampalay commented Nov 4, 2015

Shrhawk commented Nov 4, 2015

Shrhawk commented Nov 4, 2015

Shrhawk commented Nov 5, 2015

adampalay commented Nov 5, 2015