-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Handle when no video transcript uploaded for a language #13564
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -383,9 +383,7 @@ def manage_video_subtitles_save(item, user, old_metadata=None, generate_translat | |
| lang, | ||
| ) | ||
| except TranscriptException as ex: | ||
| # remove key from transcripts because proper srt file does not exist in assets. | ||
| item.transcripts.pop(lang) | ||
| reraised_message += ' ' + ex.message | ||
| pass | ||
| if reraised_message: | ||
| item.save_with_metadata(user) | ||
| raise TranscriptException(reraised_message) | ||
|
|
@@ -531,6 +529,9 @@ def asset_location(location, filename): | |
| """ | ||
| Return asset location. `location` is module location. | ||
| """ | ||
| # If user transcript filename is empty, raise `TranscriptException` to avoid `InvalidKeyError`. | ||
| if not filename: | ||
| raise TranscriptException("Transcript not uploaded yet") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does raising this exception results into error on studio/lms ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After @marcotuts 's suggestion to save the language even if the associated transcript file is not uploaded, this does not result into the error on studio/lms. However, as a precaution, I have added this so that a correct exception would be generated which may/may-not be handled (depends upon the caller of |
||
| return StaticContent.compute_location(location.course_key, filename) | ||
|
|
||
| @staticmethod | ||
|
|
@@ -670,12 +671,17 @@ def get_transcripts_info(self, is_bumper=False): | |
| """ | ||
| if is_bumper: | ||
| transcripts = copy.deepcopy(get_bumper_settings(self).get('transcripts', {})) | ||
| return { | ||
| "sub": transcripts.pop("en", ""), | ||
| "transcripts": transcripts, | ||
| } | ||
| sub = transcripts.pop("en", "") | ||
| else: | ||
| return { | ||
| "sub": self.sub, | ||
| "transcripts": self.transcripts, | ||
| } | ||
| transcripts = self.transcripts | ||
| sub = self.sub | ||
|
|
||
| # Only attach transcripts that are not empty. | ||
| transcripts = { | ||
| language_code: transcript_file | ||
| for language_code, transcript_file in transcripts.items() if transcript_file != '' | ||
| } | ||
| return { | ||
| "sub": sub, | ||
| "transcripts": transcripts, | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,7 @@ | |
| from xmodule.xml_module import is_pointer_tag, name_to_pathname, deserialize_field | ||
| from xmodule.exceptions import NotFoundError | ||
| from xmodule.contentstore.content import StaticContent | ||
| from xmodule.validation import StudioValidationMessage, StudioValidation | ||
|
|
||
| from .transcripts_utils import VideoTranscriptsMixin, Transcript, get_html5_ids | ||
| from .video_utils import create_youtube_string, get_poster, rewrite_video_url, format_xml_exception_message | ||
|
|
@@ -152,6 +153,12 @@ class VideoModule(VideoFields, VideoTranscriptsMixin, VideoStudentViewHandlers, | |
| ]} | ||
| js_module_name = "Video" | ||
|
|
||
| def validate(self): | ||
| """ | ||
| Validates the state of this Video Module Instance. | ||
| """ | ||
| return self.descriptor.validate() | ||
|
|
||
| def get_transcripts_for_student(self, transcripts): | ||
| """Return transcript information necessary for rendering the XModule student view. | ||
| This is more or less a direct extraction from `get_html`. | ||
|
|
@@ -427,6 +434,35 @@ def __init__(self, *args, **kwargs): | |
| if not self.fields['download_track'].is_set_on(self) and self.track: | ||
| self.download_track = True | ||
|
|
||
| def validate(self): | ||
| """ | ||
| Validates the state of this video Module Instance. This | ||
| is the override of the general XBlock method, and it will also ask | ||
| its superclass to validate. | ||
| """ | ||
| validation = super(VideoDescriptor, self).validate() | ||
| if not isinstance(validation, StudioValidation): | ||
| validation = StudioValidation.copy(validation) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mushtaqak Why we need to make copy here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's the way to have validation copy object when we don't have one. Already being used in couple of places. |
||
|
|
||
| no_transcript_lang = [] | ||
| for lang_code, transcript in self.transcripts.items(): | ||
| if not transcript: | ||
| no_transcript_lang.append([label for code, label in settings.ALL_LANGUAGES if code == lang_code][0]) | ||
|
|
||
| if no_transcript_lang: | ||
| ungettext = self.runtime.service(self, "i18n").ungettext | ||
| validation.set_summary( | ||
| StudioValidationMessage( | ||
| StudioValidationMessage.WARNING, | ||
| ungettext( | ||
| 'There is no transcript file associated with the {lang} language.', | ||
| 'There are no transcript files associated with the {lang} languages.', | ||
| len(no_transcript_lang) | ||
| ).format(lang=', '.join(no_transcript_lang)) | ||
| ) | ||
| ) | ||
| return validation | ||
|
|
||
| def editor_saved(self, user, old_metadata, old_content): | ||
| """ | ||
| Used to update video values during `self`:save method from CMS. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -126,6 +126,27 @@ def test_translations_uploading(self): | |
| self.assertIn(unicode_text, self.video.captions_text) | ||
| self.assertEqual(self.video.caption_languages.keys(), ['zh', 'uk']) | ||
|
|
||
| def test_save_language_upload_no_transcript(self): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mushtaqak This tests check studio side flow. Do we also need a test for lms?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @muhammad-ammar Actually this test checks |
||
| """ | ||
| Scenario: Transcript language is not shown in language menu if no transcript file is uploaded | ||
| Given I have created a Video component | ||
| And I edit the component | ||
| And I open tab "Advanced" | ||
| And I add a language "uk" but do not upload an .srt file | ||
| And I save changes | ||
| When I view the video language menu | ||
| Then I am not able to see the language "uk" translation language | ||
| """ | ||
| self._create_video_component() | ||
| self.edit_component() | ||
| self.open_advanced_tab() | ||
| language_code = 'uk' | ||
| self.video.click_button('translation_add') | ||
| translations_count = self.video.translations_count() | ||
| self.video.select_translation_language(language_code, translations_count - 1) | ||
| self.save_unit_settings() | ||
| self.assertNotIn(language_code, self.video.caption_languages.keys()) | ||
|
|
||
| def test_upload_large_transcript(self): | ||
| """ | ||
| Scenario: User can upload transcript file with > 1mb size | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we don't need this anymore ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because now we are saving language and not raising exception.