Modulestore Support for Content libraries (Part of SOL-1, SOL-2, SOL-3)

[bradenmacdonald]

@cpennington @dmitchell @doctoryes @antoviaque @explorerleslie @cahrens

Background: This is the technical foundation required for Content Libraries. It is considered part of SOL-1, SOL-2, and SOL-3, with the corresponding UI changes separated out into #6046.

Discussion: Architecture discussed extensively on the wiki and in meetings, then the revised proposal was presented to the Arch Council on Oct. 21 and given thumbs up.

Partner information: 3rd party-hosted open edX instance, for an edX solutions client.

Merge deadline: ASAP - several other PRs are ready and blocking on this.

Dependencies: Requires openedx/opaque-keys#46, which is already merged.

[openedx-webhooks]

Thanks for the pull request, @bradenmacdonald! I've created OSPR-224 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

• supporting documentation
• edx-code email threads
• timeline information ('this must be merged by XX date', and why that is)
• partner information ('this is a course on edx.org')
• any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the Github pull request interface. As a reminder, our process documentation is here.

[cahrens]

Thanks for the heads-up, @bradenmacdonald. I think a code review by the architecture team is sufficient for this PR (T&L does not need to review).

[sarina]

@bradenmacdonald this PR needs a rebase

[sarina]

Stylistically prefer """ rather than '''

[sarina]

Looks good at a brief pass from me. Handing off to the Platform team. Braden please be sure to rebase ASAP to get review attention.

[bradenmacdonald]

Thanks @sarina and @cahrens. Fixed formatting issues and rebased.

[sarina]

@bradenmacdonald please take a look at build failures.

This is now sitting in the platform team's backlog (ie @cpennington 's team)

[cpennington]

Why can't this just be a pure XBlock?

[bradenmacdonald]

• When I wrote it, pure XBlocks didn't support children, and I was just copying the similar XModule used as the root of course structures. I can try converting this to an XBlock, though - it would be nicer.

[cpennington]

@doctoryes is in the middle of testing a fix to a bug that might hit you if this is a pure XBlock, but I think it would be good to try.

[bradenmacdonald]

@cpennington Good news: I converted it to an XBlock and it seems to work well: b0662e7

Two questions for you:

• I left it in the xmodule folder and tweaked xmodule's setup.py as you can see in the commit - is this good, or is there some other place that new core XBlocks should go?
• The new XBlock has no JavaScript (never calls initialize_js) and is a pure XBlock, so when I view the library in studio, the initializeBlock coffeescript code logs an error to the JS console saying its DIV is missing data-runtime, data-runtime-version or data-init, and can't be initialized. This doesn't cause any problems, but creates a very long trace in the console as the entire HTML of the XBlock is logged with that error. Can I remove that console.log warning, or is there something I need to change/implement in the new XBlock?

[cpennington]

Hrm. I guess leaving it in xmodule for now is fine, until we can split all of the core xblocks out into a separate repo entirely.

It seems like we should tweak that logging so that it only logs if it has some but not all of the data that it needs, rather than logging if it has none of them.

[bradenmacdonald]

@sarina There are no build failures - just a single TODO comment in the quality check that's causing it to say tests failed.

[bradenmacdonald]

Whoops - fat fingers. Didn't mean to close this PR just now.

[sarina]

@bradenmacdonald this PR needs a rebase

[dmitchell]

I don't think self._get_modulestore_for_courseid will ever return a store which will not support libraries. I don't think it's possible.

[cpennington]

These should be parse_xml and add_xml_to_node, the XBlock serialization methods (rather than the XModule serialization methods you're defining now).

[dmitchell]

mostly 👍 w/ some possible noop questions. I'm curious how you're implementing the use of library content in a course esp after talking to @brianhw. I'll go back and read your spec again, but I'm wondering about maintaining block identity across course-like boundaries esp if used > 1x in borrowing course.

[cpennington]

This isn't doing the deepcopy that the old code did, is it?

[e-kolpakov]

@cpennington it does: https://github.com/edx/edx-platform/pull/6033/files#diff-01698fa4b2435cbab4063c626df19d9bR1279

[cpennington]

Ah, ok.

[e-kolpakov]

Squashed commits in order to prepare PR to merging.

[cpennington]

A few minor niggles, but 👍