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

bradenmacdonald · 2014-11-21T22:56:46Z

@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 · 2014-11-21T22:56:48Z

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 · 2014-11-24T14:14:25Z

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 · 2014-11-25T14:21:01Z

@bradenmacdonald this PR needs a rebase

sarina · 2014-11-25T14:22:48Z

common/lib/xmodule/xmodule/modulestore/mixed.py

+    def get_libraries(self, **kwargs):
+        '''
+        Returns a list containing the top level XModuleDescriptors of the libraries in this modulestore.
+        '''


Stylistically prefer """ rather than '''

sarina · 2014-11-25T14:30:01Z

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 · 2014-11-25T19:33:13Z

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

sarina · 2014-11-25T20:41:10Z

@bradenmacdonald please take a look at build failures.

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

cpennington · 2014-11-25T21:02:57Z

common/lib/xmodule/xmodule/library_module.py

@@ -0,0 +1,80 @@
+"""
+'library' XBlock/XModule


Why can't this just be a pure XBlock?

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.

@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.

@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?

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 · 2014-11-25T21:24:20Z

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

bradenmacdonald · 2014-11-28T04:04:04Z

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

sarina · 2014-12-01T16:39:12Z

@bradenmacdonald this PR needs a rebase

dmitchell · 2014-12-03T20:01:05Z

common/lib/xmodule/xmodule/modulestore/mixed.py

+        assert(isinstance(library_key, LibraryLocator))
+        store = self._get_modulestore_for_courseid(library_key)
+        if not hasattr(store, 'get_library'):
+            return None  # This key seems invalid since its modulestore doesn't support libraries


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 · 2014-12-03T21:19:40Z

common/lib/xmodule/xmodule/library_root_xblock.py

+        """ XML support not yet implemented. """
+        raise NotImplementedError
+
+    def export_to_xml(self, resource_fs):


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 · 2014-12-03T21:24:06Z

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 · 2014-12-03T21:58:10Z

common/lib/xmodule/xmodule/modulestore/split_mongo/split.py

-                root_block['edit_info']['previous_version'] = root_block['edit_info'].get('update_version')
-                root_block['edit_info']['update_version'] = new_id
+                old_def = self.get_definition(locator, root_block['definition'])
+                new_fields = old_def['fields']


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

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

e-kolpakov · 2014-12-04T14:41:42Z

Squashed commits in order to prepare PR to merging.

cpennington · 2014-12-05T17:19:29Z

A few minor niggles, but 👍

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

openedx-webhooks added the needs triage label Nov 21, 2014

This was referenced Nov 24, 2014

Studio support for editing content libraries (SOL-1, SOL-2, & SOL-3) edx/edx-solutions-edx-platform#328

Closed

Studio Support for Content Libraries (SOL-1, SOL-2, & SOL-3) #6046

Merged

openedx-webhooks added community manager review and removed needs triage labels Nov 25, 2014

sarina reviewed Nov 25, 2014
View reviewed changes

openedx-webhooks added awaiting prioritization and removed community manager review labels Nov 25, 2014

bradenmacdonald force-pushed the content-libraries branch from 68fe2f1 to 1cbc1a8 Compare November 25, 2014 19:32

cpennington reviewed Nov 25, 2014
View reviewed changes

bradenmacdonald force-pushed the content-libraries branch from 3b14337 to b0662e7 Compare November 26, 2014 08:27

bradenmacdonald mentioned this pull request Nov 27, 2014

Content libraries prototype #5779

Closed

bradenmacdonald closed this Nov 28, 2014

bradenmacdonald deleted the content-libraries branch November 28, 2014 03:59

bradenmacdonald restored the content-libraries branch November 28, 2014 04:00

bradenmacdonald reopened this Nov 28, 2014

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Dec 1, 2014

dmitchell reviewed Dec 3, 2014
View reviewed changes

openedx-webhooks added the engineering review label Dec 3, 2014

cpennington reviewed Dec 3, 2014
View reviewed changes

e-kolpakov force-pushed the content-libraries branch 2 times, most recently from 975bf82 to 48354f7 Compare December 4, 2014 12:48

Split mongo support for libraries and Library XBlock

0ded669

e-kolpakov force-pushed the content-libraries branch from 48354f7 to 0ded669 Compare December 4, 2014 14:40

sarina removed the awaiting prioritization label Dec 5, 2014

openedx-webhooks added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Dec 5, 2014

antoviaque added a commit that referenced this pull request Dec 8, 2014

Merge pull request #6033 from edx-solutions/content-libraries

f561e79

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

antoviaque merged commit f561e79 into openedx:master Dec 8, 2014

bradenmacdonald deleted the content-libraries branch December 10, 2014 02:31

bradenmacdonald mentioned this pull request Jan 6, 2015

Content libraries MVP #6459

Merged

10 tasks

sarina added the open-source-contribution PR author is not from Axim or 2U label Apr 7, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bradenmacdonald commented Nov 21, 2014

openedx-webhooks commented Nov 21, 2014

cahrens commented Nov 24, 2014

sarina commented Nov 25, 2014

sarina Nov 25, 2014

sarina commented Nov 25, 2014

bradenmacdonald commented Nov 25, 2014

sarina commented Nov 25, 2014

cpennington Nov 25, 2014

bradenmacdonald Nov 25, 2014

cpennington Nov 25, 2014

bradenmacdonald Nov 26, 2014

cpennington Nov 26, 2014

bradenmacdonald commented Nov 25, 2014

bradenmacdonald commented Nov 28, 2014

sarina commented Dec 1, 2014

dmitchell Dec 3, 2014

cpennington Dec 3, 2014

dmitchell commented Dec 3, 2014

cpennington Dec 3, 2014

e-kolpakov Dec 4, 2014

cpennington Dec 4, 2014

e-kolpakov commented Dec 4, 2014

cpennington commented Dec 5, 2014

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

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

Conversation

bradenmacdonald commented Nov 21, 2014

openedx-webhooks commented Nov 21, 2014

cahrens commented Nov 24, 2014

sarina commented Nov 25, 2014

Choose a reason for hiding this comment

sarina commented Nov 25, 2014

bradenmacdonald commented Nov 25, 2014

sarina commented Nov 25, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradenmacdonald commented Nov 25, 2014

bradenmacdonald commented Nov 28, 2014

sarina commented Dec 1, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitchell commented Dec 3, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e-kolpakov commented Dec 4, 2014

cpennington commented Dec 5, 2014