Handle when no video transcript uploaded for a language #13564

mushtaqak · 2016-09-26T07:59:10Z

Description

This change handles the case when no transcript file is uploaded for a language then raise an exception that transcript file is not uploaded yet. This wouldn't let user save transcript language if no transcript file is uploaded for it.

Also, this change handles the videos that have language already added in the database but no transcript file was uploaded. This prevents showing of such languages as transcript on the video.

TNL-5200

Sandbox

Testing

Unit
Acceptance

Reviewers

If you've been tagged for review, please check your corresponding box once you've given the 👍.

Code review: @muzaffaryousaf
Code review: @muhammad-ammar
Doc: @catong
Product: @sstack22

Post-review

Squash commits

muzaffaryousaf · 2016-09-26T16:27:30Z

@mushtaqak Can you bake sandbox for this change to look at the behavior.

mushtaqak · 2016-09-26T16:30:02Z

@muzaffaryousaf Sure, I will make one right away :)

mushtaqak · 2016-09-26T16:34:00Z

I have started the jenkins build which would create the video transcript sandbox

mushtaqak · 2016-09-27T12:19:04Z

@sstack22 If you try to add a new transcript language and upload no transcript file here in sandbox, you would get the error message and corresponding transcript language is not saved now :

The issue is the error message is too generic and does not tell what the error has occurred leading user ambiguity.

I have tried to find the correct solution but to implement a good solution would take good amount of time.

I have also tried to catch this exception at _save_xblock method which is also called when editor is saved, handling exception here outputs the correct error message :

What do you think, we should do here ?

mushtaqak · 2016-09-29T07:20:35Z

@marcotuts Since @sstack22 is on leave, can you please reply to above question. Thanks :)

marcotuts · 2016-09-29T22:26:46Z

@mushtaqak - My suggestion here might be to allow save but to instead reuse the validation message pattern used by the Randomized Content Block (and in this Conditional UI block PR ex: https://github.com/edx/edx-platform/pull/11710) to say something to the effect of: "There is no language transcript file associated with one of the defined transcript languages" (cc'ed @catong here as well). The link / action on this message could read: "Upload transcript file"

I included an example of the validation message shown for the conditional block

Does this work in terms of reduction in complexity and scope?

catong · 2016-09-30T00:55:52Z

If I understand correctly, this error is associated with a specific "Save" of video settings. Perhaps just a slight tweak to @marcotuts' suggestion:
"There is no transcript file associated with the selected transcript language." (rather than "one of the defined languages"?)
@mushtaqak please let me know if my understanding is correct.

mushtaqak · 2016-10-04T12:45:57Z

@sstack22 Please review from product perspective.

You can look at the video component which now shows the validation message if transcript file is not uploaded for a language as suggested by @marcotuts

@catong FYI about the validation message containing language name.

muzaffaryousaf · 2016-10-04T14:00:01Z

@mushtaqak I'm seeing error on this page, can you confirm that's its not happening due to our change ?
https://studio-videotranscript.sandbox.edx.org/container/block-v1:edX+Test101+course+type@vertical+block@f29b97786e7840dbb532bdd248d792d1

catong · 2016-10-04T14:17:35Z

@mushtaqak Looks great with the language name brought into the message. Is the language name itself also localized when the message is localized?

mushtaqak · 2016-10-04T15:05:30Z

@catong Only message is localized, language names are not localized.

catong · 2016-10-04T15:09:54Z

Thanks @mushtaqak. Is it preferable to have the specific language name in the message if it will appear only in English regardless of the language of the UI/message or should we have something generic to avoid this? @sstack22 @marcotuts what are your thoughts?

mushtaqak · 2016-10-04T15:13:50Z

@catong yes, it is infact, I have done that part where we show language name instead of language code. It's just there is some translation issue which I was working on.

muhammad-ammar

@mushtaqak I am done with first pass of the review. Let me know once you addressed the feedback.

muhammad-ammar · 2016-09-27T09:46:04Z

common/test/acceptance/tests/video/test_studio_video_editor.py


+    def test_save_language_upload_no_transcript(self):
+        """
+        Scenario: User can can not save transcript language if no transcript file is uploaded


duplicate can

muhammad-ammar · 2016-10-06T07:28:34Z

common/lib/xmodule/xmodule/tests/test_video.py

+            [u"en", u"English"],
+            [u"eo", u"Esperanto"],
+            [u"ur", u"Urdu"]
+        )


@mushtaqak We need to restore settings.ALL_LANGUAGES to its original value, otherwise other tests will see this modified value.

@mushtaqak Can we not use @override_settings(ALL_LANGUAGES =()) ?

muhammad-ammar · 2016-10-06T07:38:38Z

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

+        """
+        validation = super(VideoDescriptor, self).validate()
+        if not isinstance(validation, StudioValidation):
+            validation = StudioValidation.copy(validation)


@mushtaqak Why we need to make copy here?

I think it's the way to have validation copy object when we don't have one. Already being used in couple of places.

muhammad-ammar · 2016-10-06T08:52:07Z

common/test/acceptance/tests/video/test_studio_video_editor.py

+        And I open tab "Advanced"
+        And I add a language "uk" but do not upload an .srt file
+        And I save changes
+        Then when I view the video


@mushtaqak I think Then i view the video is enough.

When I view the video captions/language menu
Then I cannot see the language "uk" translation

Then I am not able to see the language "uk" translation

muhammad-ammar · 2016-10-06T08:55:21Z

common/test/acceptance/tests/video/test_studio_video_editor.py

        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):


@mushtaqak This tests check studio side flow. Do we also need a test for lms?
@muzaffaryousaf Any thoughts?

@muhammad-ammar Actually this test checks if a transcript language is shown in language menu if no transcript file is uploaded on video which would be same on LMS side as well.

muhammad-ammar · 2016-10-06T09:03:07Z

common/lib/xmodule/xmodule/tests/test_video.py

+        self.assertEqual(StudioValidationMessage.WARNING, validation.summary.type)
+        self.assertIn("There is no transcript file associated", validation.summary.text)
+
+    def test_no_transcript_multi_lang_validation_message(self):


@mushtaqak I think we can merge test_no_transcript_multi_lang_validation_message and test_no_transcript_single_lang_validation_message into test using ddt. You can see ddt exmamples in test_capa_problem.py

Also please check the complete validation message.

I think we don't need ddt here, we can just merge both tests.

If we can check both the messages completely without ddt then its good.

muzaffaryousaf · 2016-10-06T11:16:14Z

common/lib/xmodule/xmodule/tests/test_video.py

        translations = descriptor.available_translations(descriptor.get_transcripts_info(), verify_assets=False)
        self.assertEqual(translations, ['en'])
+
+    def test_video_with_lang_with_no_transcripts_translation(self):


I would say to change the name to more readable form, Maybe you can choose test_video_with_language_do_not_have_transcripts_translation.

muzaffaryousaf · 2016-10-06T11:23:05Z

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

-                # remove key from transcripts because proper srt file does not exist in assets.
-                item.transcripts.pop(lang)
-                reraised_message += ' ' + ex.message
+                pass


Why we don't need this anymore ?

Because now we are saving language and not raising exception.

muzaffaryousaf · 2016-10-06T13:06:01Z

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

        """
+        # If user transcript filename is empty, raise `TranscriptException` to avoid `InvalidKeyError`.
+        if not filename:
+            raise TranscriptException("Transcript not uploaded yet")


Does raising this exception results into error on studio/lms ?

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 asset_location method). This is just a way for us to be safe from InvalidKeyError when no asset is present.

muzaffaryousaf · 2016-10-06T13:15:08Z

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

+            sub = self.sub
+
+        # Only attach transcripts that are not empty.
+        transcripts = {k: v for k, v in transcripts.items() if v != ''}


can we rename k & v according to the content.

muzaffaryousaf · 2016-10-06T13:17:00Z

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

+            validation = StudioValidation.copy(validation)
+
+        no_transcript_lang = []
+        for lang, transcript in self.transcripts.items():


lang --> lang_code

muzaffaryousaf · 2016-10-06T13:20:41Z

common/test/acceptance/tests/video/test_studio_video_editor.py


+    def test_save_language_upload_no_transcript(self):
+        """
+        Scenario: Transcript language is not shown in captions if no transcript file is uploaded


captions menu

muzaffaryousaf · 2016-10-06T13:25:55Z

@mushtaqak I've done my first pas and left some comments, let me know when you're done with addressing them.

muhammad-ammar · 2016-10-07T10:10:53Z

common/lib/xmodule/xmodule/tests/test_video.py

@mushtaqak @muzaffaryousaf There is too much duplication in this test. It can be written far better with ddt.

muhammad-ammar · 2016-10-07T10:11:46Z

common/lib/xmodule/xmodule/tests/test_video.py

@mushtaqak I think this will fail. s is missing at the end of language.

muzaffaryousaf · 2016-10-07T10:12:44Z

common/lib/xmodule/xmodule/tests/test_video.py

I would use ddt here for <transcript /> to inject in xml_data_transcripts instead of repeating same thing twice.

muzaffaryousaf · 2016-10-07T10:14:28Z

common/lib/xmodule/xmodule/tests/test_video.py

Asserts that the validation message has all expected content

muzaffaryousaf · 2016-10-07T10:19:58Z

@mushtaqak i'm done with 2nd pass and left couple of comments. Let me know when you're done.

sstack22 · 2016-10-07T12:31:55Z

@mushtaqak - this looks great 👍 on my end. @catong is the language used here good to go?

mushtaqak · 2016-10-07T12:35:12Z

@sstack22 You can interact with this unit https://studio-videotranscript.sandbox.edx.org/container/block-v1:edX+Test101+course+type@vertical+block@f29b97786e7840dbb532bdd248d792d1

muzaffaryousaf · 2016-10-07T14:00:13Z

👍

catong · 2016-10-07T14:02:51Z

@mushtaqak @sstack22 The error message seems to have changed a bit since I last looked at this PR. Previously we had the word "the" in front of the language name, which reads correctly. Now it looks as if "the" is removed, and it does not read quite grammatically, at least in English. My suggestion is to restore "the", so that the message reads, "There is no transcript associated with the Urdu language." OR remove the word language so that the message reads, "There is no transcript associated with Urdu."

muhammad-ammar · 2016-10-07T14:06:28Z

👍

mushtaqak · 2016-10-07T14:12:19Z

@catong Thanks for the suggestion.

So for surety, the messages would read as follows;

When one language missing transcript file :

There is no transcript file associated with the Urdu language.

When multiple languages missing transcript files :

There are no transcript files associated with the Esperanto, Urdu languages.

Please correct me if I am wrong.

catong · 2016-10-07T14:12:56Z

@mushtaqak That is exactly right. Thanks!

catong · 2016-10-07T14:13:18Z

👍 with the message revisions as captured above. Thanks!

…ge in a video component in studio. Also, don't show respective transcript language in video language menu when a related transcript is not uploaded for that language. TNL-5200

mushtaqak force-pushed the mushtaq/video-transcript branch from a2661d2 to f90705e Compare September 26, 2016 11:13

mushtaqak force-pushed the mushtaq/video-transcript branch 4 times, most recently from 08ee17f to 1a992fb Compare October 4, 2016 12:37

mushtaqak force-pushed the mushtaq/video-transcript branch 2 times, most recently from 45f8e92 to ebb789e Compare October 5, 2016 13:07

muhammad-ammar reviewed Oct 6, 2016

View reviewed changes

muzaffaryousaf reviewed Oct 6, 2016

View reviewed changes

mushtaqak force-pushed the mushtaq/video-transcript branch from c1b9c75 to 2d478a2 Compare October 7, 2016 09:36

muhammad-ammar reviewed Oct 7, 2016

View reviewed changes

muzaffaryousaf reviewed Oct 7, 2016

View reviewed changes

mushtaqak force-pushed the mushtaq/video-transcript branch 2 times, most recently from 09fa876 to bb82426 Compare October 7, 2016 12:19

mushtaqak force-pushed the mushtaq/video-transcript branch from bb82426 to c03c4d3 Compare October 7, 2016 13:15

Show validation message when no transcript file uploaded for a langua…

11a07ca

…ge in a video component in studio. Also, don't show respective transcript language in video language menu when a related transcript is not uploaded for that language. TNL-5200

mushtaqak force-pushed the mushtaq/video-transcript branch from c03c4d3 to 11a07ca Compare October 7, 2016 14:16

mushtaqak merged commit 2338858 into master Oct 7, 2016

mushtaqak deleted the mushtaq/video-transcript branch October 7, 2016 15:08

Handle when no video transcript uploaded for a language #13564

Handle when no video transcript uploaded for a language #13564

Conversation

mushtaqak commented Sep 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Reviewers

Post-review

Uh oh!

muzaffaryousaf commented Sep 26, 2016

Uh oh!

mushtaqak commented Sep 26, 2016

Uh oh!

mushtaqak commented Sep 26, 2016

Uh oh!

mushtaqak commented Sep 27, 2016

Uh oh!

mushtaqak commented Sep 29, 2016

Uh oh!

marcotuts commented Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

catong commented Sep 30, 2016

Uh oh!

mushtaqak commented Oct 4, 2016

Uh oh!

muzaffaryousaf commented Oct 4, 2016

Uh oh!

catong commented Oct 4, 2016

Uh oh!

mushtaqak commented Oct 4, 2016

Uh oh!

catong commented Oct 4, 2016

Uh oh!

mushtaqak commented Oct 4, 2016

Uh oh!

muhammad-ammar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muzaffaryousaf Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mushtaqak commented Sep 26, 2016 •

edited

Loading

marcotuts commented Sep 29, 2016 •

edited

Loading

muzaffaryousaf Oct 6, 2016 •

edited

Loading

muzaffaryousaf Oct 7, 2016 •

edited

Loading

sstack22 commented Oct 7, 2016 •

edited

Loading