Alex/multiple transcripts editor by auraz · Pull Request #2899 · openedx/openedx-platform

auraz · 2014-03-11T15:18:53Z

Create an upload modal for video transcript translations (BLD-751: https://edx-wiki.atlassian.net/browse/BLD-751).

@cahrens please review
@valera-rozuvan please review
@frrrances please review
@dmitchell please review.
@olmar please review python part

auraz · 2014-03-14T16:14:55Z

@polesye fixed: I think should be

auraz · 2014-03-19T13:28:51Z

cms/djangoapps/contentstore/features/video-editor.py

auraz · 2014-03-21T12:51:21Z

Fully covered:
common/lib/xmodule/xmodule/video_module/video_xfields.py (100%)
common/lib/xmodule/xmodule/video_module/video_module.py (100%)
cms/djangoapps/contentstore/views/transcripts_ajax.py 100%

Diff LMS/CMS/XModule coverage:

common/lib/xmodule/xmodule/video_module/transcripts_utils.py (>90%): Missing line(s) 344, 547
common/lib/xmodule/xmodule/video_module/video_handlers.py (>80%): Missing line(s) 204, 237, 337, 251, 253, 254

Result => 94%

dmitchell · 2014-03-21T20:06:03Z

Wow, this is huge! How urgent is this?

dmitchell · 2014-03-21T20:16:56Z

cms/djangoapps/contentstore/features/video-editor.feature

I'm a bit confused as to why it defaults to uk on the double uploads below but is sticky with the existing zh here. I suppose my real question is whether the double upload tests should do something to ensure the selected language is uk or is it purely a matter of the order in the listing and is that order predictable and guaranteed?

I'm a bit confused as to why it defaults to uk on the double uploads below but is sticky with the existing zh here. I suppose my real question is whether the double upload tests should do something to ensure the selected language is uk or is it purely a matter of the order in the listing and is that order predictable and guaranteed?

To make sure that after editing previously created video module and adding new translations will not change currently used translation.
As you can see in this test, we have created video module with chinese translation and then edit this module by adding new translation.
Logic is here

auraz · 2014-03-24T08:30:12Z

@dmitchell This PR seems to be huge, but really is not so. On python part, video module was split into smaller files for simpler code support, and so we have many additions, but the code is not so changed actually:

XFields from video module were moved to separate file video_xfields.py
All XBllock handlers from video_module were moved to separate file: video_handlers.py. All tests for video handlers were moved to test_video_handlers.py.
New handler for Studio and tests for it were written: see VideoStudioViewHandlers class. This is main addition to python code part in this PR.

Urgency: I think it would be nice to merge it this week, so you have today and tomorrow for sure to review it, so we will have rest of week to address your comments)))

polesye · 2014-03-24T08:46:01Z

@cahrens FYI: I extended File uploader to make it works with srt files, because current validation by mime type doesn't work for this filetype (file.type equal '' and srt doesn't exist in mime types list).

cahrens · 2014-03-24T18:26:39Z

It would be nice if the error handling was better when you select a language for a transcript, but then never upload the transcript. Right now you get the error bar at the bottom and a "broken editor" when you hit Save.

cahrens · 2014-03-24T18:43:23Z

cms/djangoapps/contentstore/features/video-editor.feature

This test would be easier to understand if it said it was replacing the transcript file, instead of uploading (in this case, you press the Replace button, correct)? If the implementation of the step is correct, perhaps you can map 2 different BDD descriptions to the same method?

This test would be easier to understand if it said it was replacing the transcript file, instead of uploading (in this case, you press the Replace button, correct)? If the implementation of the step is correct, perhaps you can map 2 different BDD descriptions to the same method?

Done.

cahrens · 2014-03-24T18:55:32Z

Given the large number of new Lettuce tests (which I whole-heartedly applaud), I'd recommend having @jzoldak code review the tests.

cahrens · 2014-03-24T19:09:23Z

cms/static/js/views/video/translations_editor.js

Why is the option disabled in this case? I thought this code was filtering to the values passed in.

Why is the option disabled in this case? I thought this code was filtering to the values passed in.

I have renamed this function. We just disable previously selected options.

cahrens · 2014-03-24T20:05:35Z

I have reviewed the JavaScript files and the things that impact the metadata editor. I don't have enough context to figure out what changed and what did not in the xmodule video Python code. I think that is better reviewed by other Blades folks. And I don't know anything about the LMS tests, so I am also not reviewing those changes. If there is someone who is familiar with those tests, please tag them to review.

polesye · 2014-03-25T08:07:38Z

@jzoldak please also review this PR.

marcotuts · 2014-03-26T16:04:51Z

@auraz - Just as a follow-on to the error message @Lyla-Fischer mentioned above. I can see how the red error text/bar Studio renders may not be possible to modify/adjust the text for in this PR. The text inside the upload modal that reads "We're sorry, there was an error" would be great to update to what Lyla suggested: " Sorry, there was an error parsing the subtitles that you uploaded. Please check the format and try again."

auraz · 2014-03-26T16:04:56Z

@Lyla-Fischer There will be this same message for all kind of exceptions. It is fine? Otherwise solving this will require proxy exception text from Studio to uploader snippet. I think @polesye use existing Studio snippet which does not allow this.

auraz · 2014-03-26T16:05:20Z

@Lyla-Fischer @marcotuts Ok, will change, thank you!

Lyla-Fischer · 2014-03-26T16:07:40Z

Don't change it if it is going to be wrong in other aspects of the system... Is it possible to make it effect only this one upload?

auraz · 2014-03-26T16:10:27Z

@Lyla-Fischer Sure! (What I mean, that this particular upload handler in video player can possibly show different specific kinds of exceptions during SRT uploading.)

Lyla-Fischer · 2014-03-26T16:12:49Z

Okay. Thanks for the clarification.

marcotuts · 2014-03-26T17:47:48Z

cms/static/sass/elements/_xmodules.scss

I would suggest using the %cont-truncated mixin, eliminating the need for the text-overflow and overflow rules here. (see _mixins.scss, line 311).

I would suggest using the %cont-truncated mixin, eliminating the need for the text-overflow and overflow rules here. (see _mixins.scss, line 311).

Fixed.

polesye · 2014-03-27T10:53:27Z

would be great to update to what Lyla suggested: " Sorry, there was an error parsing the subtitles that you uploaded. Please check the format and try again."

Done.

polesye · 2014-03-27T10:54:19Z

@dmitchell , @cahrens , @marcotuts , @jzoldak , @valera-rozuvan all comments are addressed. Please continue review.

valera-rozuvan · 2014-03-27T12:16:03Z

👍 Good to merge!

marcotuts · 2014-03-27T14:46:08Z

cms/static/sass/elements/_xmodules.scss

this width rule should be 45%, and .metadata-video-translations .create-setting should have a width of 100% not 88%. This ensures the button is the same width as the other list input button styles.

this width rule should be 45%, and .metadata-video-translations .create-setting should have a width of 100% not 88%. This ensures the button is the same width as the other list input button styles.

Done.

marcotuts · 2014-03-27T15:53:35Z

I just tested with the latest, and the only bit that caught my eye that I think would be worth adjusting is some of the CSS for the list block as a whole.

This link shows the difference between the existing list setting styling and the new styling introduced in this PR. https://www.dropbox.com/s/3qjhgh5eibsgvgw/Screenshot%202014-03-27%2011.07.54.png

It would be great to sync up the alignment and input field sizing to what we have for the Video Sources setting.

Edit: In order to be more specific at the changes I'm suggesting:
1.) Width of input field should shrink down a bit to match existing Video Sources Setting.
2.) margin-right on input field to separate out the X icon a bit more should be added.
3.) This should cause the revert arrow icon to line up vertically with the one from the Video Sources (and other) settings as well.

polesye · 2014-03-28T12:02:20Z

@marcotuts your comments are addressed.

dmitchell · 2014-03-28T12:50:00Z

I haven't thoroughly reviewed this but don't want to hold it up. If others accept it, go ahead.

cahrens · 2014-03-28T16:45:49Z

@polesye Why is the code coverage so low (video_module, video_handlers, transcript_utils)? Is the report not accurate?

https://jenkins.testeng.edx.org/job/edx-platform-report/3823/Diff_Coverage_Report/?

marcotuts · 2014-03-28T17:05:55Z

@polesye - the changes are great thanks for addressing that.

👍 from an HTML,CSS perspective.

polesye · 2014-03-28T17:55:30Z

@cahrens As @auraz mentioned previously an actual coverage for python part is about 94%.

Video player module has tests which run in different runtimes: CMS, LMS, and xmodule.
For this 3 runtimes there are exist 3 different coverage.rc files.
But only one of them is used to show video module coverage.
Therefore all PR's for video player show wrong coverage.
@jzoldak was informed about it.

cahrens · 2014-03-28T18:16:08Z

Thank you for the information. I haven't had a chance to check this out again and review, but if you feel that the files have had adequate reviews by others, go ahead and merge when you are ready.

jzoldak · 2014-03-28T20:15:41Z

@polesye unfortunately I don't have enough time to review thoroughly today and I'll be out on Monday and Tuesday. Can you please make sure that all the new lettuce tests run reliably before merging in and also keep an eye out for failures after this gets merged in? thx.

olmar · 2014-03-31T08:39:33Z

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

Is it possible not to overwrite content here? This can help when debugging.

Please clarify. Let's talk about this bit later.

olmar · 2014-03-31T15:03:09Z

👍

Fix donwload subs for non youtube videos and non-en language - continue. Add acceptance tests. Add detetion of assets on request. Updated docstring. Add fixes and acceptance tests. Fix acceptance tests. Update docsrtings and cleanup code, resful for language_id. Specify exception type in POST. Fix url in upload module. Improve exception handling. Remove 'en' and catching in editable_metadata. Move descriptor.get_context test to lms tests. Add query parameter to translation dispatch. Response to format parameter of translatin GET request. Fix Acceprance test: Metadata Editor. move handlers to proper scores. Split video player into smaller files. Add ugettext and fix typoes. Add changelog. Support for downloading non-ascii filenames. Change event binding. Add content-language to download requests. Reractor POST handler to not update self.transcripts.

auraz · 2014-03-31T16:24:37Z

Only single flaky test is failing (acid_xblock). I'm going to merge it, as this test is confirmed to be flaky (See conversation in testeng room today, at 19:22 GMT+2)

Alex/multiple transcripts editor

auraz reviewed Mar 19, 2014
View reviewed changes

dmitchell reviewed Mar 21, 2014
View reviewed changes

cahrens reviewed Mar 24, 2014
View reviewed changes

marcotuts reviewed Mar 26, 2014
View reviewed changes

marcotuts reviewed Mar 27, 2014
View reviewed changes

olmar reviewed Mar 31, 2014
View reviewed changes

auraz added a commit that referenced this pull request Mar 31, 2014

Merge pull request #2899 from edx/alex/multiple_transcripts_editor

b3f479c

Alex/multiple transcripts editor

auraz merged commit b3f479c into master Mar 31, 2014

auraz deleted the alex/multiple_transcripts_editor branch March 31, 2014 16:28

baby636 mentioned this pull request Apr 16, 2024

[Snyk] Fix for 1 vulnerabilities baby636/edx-platform#63

Open

Conversation

auraz commented Mar 11, 2014

Uh oh!

auraz commented Mar 14, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

auraz commented Mar 21, 2014

Uh oh!

dmitchell commented Mar 21, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

auraz commented Mar 24, 2014

Uh oh!

polesye commented Mar 24, 2014

Uh oh!

cahrens commented Mar 24, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cahrens commented Mar 24, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cahrens commented Mar 24, 2014

Uh oh!

polesye commented Mar 25, 2014

Uh oh!

marcotuts commented Mar 26, 2014

Uh oh!

auraz commented Mar 26, 2014

Uh oh!

auraz commented Mar 26, 2014

Uh oh!

Lyla-Fischer commented Mar 26, 2014

Uh oh!

auraz commented Mar 26, 2014

Uh oh!

Lyla-Fischer commented Mar 26, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polesye commented Mar 27, 2014

Uh oh!

polesye commented Mar 27, 2014

Uh oh!

valera-rozuvan commented Mar 27, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcotuts commented Mar 27, 2014

Uh oh!

polesye commented Mar 28, 2014

Uh oh!

dmitchell commented Mar 28, 2014

Uh oh!

cahrens commented Mar 28, 2014

Uh oh!

marcotuts commented Mar 28, 2014

Uh oh!

polesye commented Mar 28, 2014

Uh oh!

cahrens commented Mar 28, 2014

Uh oh!

jzoldak commented Mar 28, 2014

Uh oh!

Choose a reason for hiding this comment