Skip to content

Conversation

@polesye
Copy link
Contributor

@polesye polesye commented Sep 24, 2013

This PR adds timed transcripts editor functionality for video player in studio.

Design wireframes for this PR is on wiki

Documentation of transcripts workflow is placed inside this PR to developers documentations folder.
Please navigate to sphinx generated developers documentation -> CMS->Transcripts

@Lyla, @frrrances, @singingwolfboy please review.

You can play with it on Blades sandbox at http://studio.sandbox-blades-001.m.edx.org/edit/i4x://private/05/vertical/f1d269cde2784fa99cd5e0f3a9604d62 .

Rollback functionality is not part of this PR.

@ghost ghost assigned nedbat Sep 24, 2013
@auraz
Copy link
Contributor

auraz commented Sep 24, 2013

@wedaly All tests are passed locally, but on Jenkins I see corrupted gridfs messages. Can you help with this please?

@wedaly
Copy link
Contributor

wedaly commented Sep 24, 2013

@auraz This error means there is a Mongo resource conflict, most likely with the contentstore database. You'll need to: (1) randomize the name of the Mongo database used as a contentstore, (2) drop and recreate the database before/after each test.

See cms/djangoapps/contentstore/tests/test_contentstore.py for the correct way to do this. The important lines are:

TEST_DATA_CONTENTSTORE = copy.deepcopy(settings.CONTENTSTORE)
TEST_DATA_CONTENTSTORE['OPTIONS']['db'] = 'test_xcontent_%s' % uuid4().hex

@override_settings(CONTENTSTORE=TEST_DATA_CONTENTSTORE, MODULESTORE=TEST_MODULESTORE)
class ContentStoreToyCourseTest(ModuleStoreTestCase):

and

def tearDown(self):
    MongoClient().drop_database(TEST_DATA_CONTENTSTORE['OPTIONS']['db'])
    _CONTENTSTORE.clear()

Longer term, we're working on building an abstraction for this (much like we do with ModuleStoreTestCase). I'm also setting up a Jenkins cluster, to avoid having multiple tests using the same database.

@auraz
Copy link
Contributor

auraz commented Sep 25, 2013

@wedaly thank you.

@Lyla-Fischer
Copy link

@singingwolfboy @nedbat I'm going to be bugging you in person a bit later, but I wanted to remind you that this pull request exists. I know it is a monster, but..... It adds important functionality and only you can let this into the codebase
http://www.fs.usda.gov/Internet/FSE_MEDIA/stelprdb5362178.jpg

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to include a step like this in our codebase, it should probably be in common/djangoapps/terrain/steps.py. Alternatively, we may just want to remove it entirely.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be simplified using jQuery.each:

list.each(function(i) {
  this.val(val[i] || null)
      .attr('placeholder', placeholders[i]);
})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@polesye
Copy link
Contributor Author

polesye commented Oct 17, 2013

@singingwolfboy , @nedbat All your comments are addressed, please continue. :)

@nedbat
Copy link
Contributor

nedbat commented Oct 18, 2013

I'm all done reviewing. 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line seems indented strangely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indent is fixed.

@singingwolfboy
Copy link
Contributor

The only remaining issue that I'm concerned about is the nested require call in cms/static/js/views/metadata.js. Once that's fixed, then I'm satisfied and I can give my 👍

@Lyla-Fischer
Copy link

👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to add "js/views/transcripts/metadata_videolist" to the list of modules in the define call at the top of this file, and bind it to the VideoList variable there. Is there a reason that you want to call require here, instead of getting the module at the top of the file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@singingwolfboy because I have circular dependencies (See, metadata_videolist.js:5 and metadata_videolist.js:39).
If I do so, it breaks a code. And I have fixed it according to Circular Dependencies and Sugar;
And it works okay.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, OK. I thought that you still needed to include the module name in the define call at the top of the file, and you would just get passed a null value until you use require inside the function to get the true value. But if this works, then I'm OK with it. (It would be nice if this code could be restructured to avoid the circular dependency, but I don't know if that's possible.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To do so Metadata.AbstractEditor (that is stored in the js/views/metadata.js now) should be moved to separate file. In this case, circular dependency will be avoided.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that all? Can you do that in this pull request?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@singingwolfboy Done. Please check 69e3978

@singingwolfboy
Copy link
Contributor

Awesome. 👍 once the tests pass.

polesye added a commit that referenced this pull request Oct 23, 2013
@polesye polesye merged commit 6c08708 into master Oct 23, 2013
@polesye polesye deleted the anton/video_subtitles branch October 23, 2013 09:38
pomegranited pushed a commit to open-craft/openedx-platform that referenced this pull request Aug 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants