LibraryContent - Display content from a library in a course (SOL-5, SOL-6, SOL-7, SOL-8, SOL-117)

[e-kolpakov]

• Background: This PR contains the LibraryContent XBlock, which allows to display library content in a course.
• Jira tickets: Implements SOL-5, SOL-6, SOL-7, SOL-8., SOL-117.
• Discussions: 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.
• Dependencies: https://github.com/edx/edx-platform/pull/6046 (merged, in feature branch https://github.com/edx/edx-platform/commits/content-libraries)
• Partner information: 3rd party-hosted open edX instance, for an edX solutions client.
• Merge deadline: Dec 12
• Sandbox URL: LMS at http://content-libraries.sandbox.opencraft.com/ and Studio at http://content-libraries.sandbox.opencraft.com:18010/
• Internal code reviews PRs: (WIP) Content library code for upstream PR - Internal review open-craft/openedx-platform#5 (WIP) Graded library content - Do not merge, for internal review open-craft/openedx-platform#3 (WIP) Randomized selection from content libraries - Do not merge, for internal review open-craft/openedx-platform#2

Instructions to test:

1. Create a Library with some content, and a course
2. In the course, go to Settings > Advanced Settings and add "library_content" to advanced_modules.
3. Go to Content > Outline. Create a new section and subsection, then edit it. Click "Advanced" and add a new "library_content" block.
4. Once the block is added, click "Edit" to edit its settings. Most are explained on the settings editor. For the "Library" setting, you must click "Add", then paste the LibraryLocator of your library. This looks like library-v1:ProblemX+PR0B. (It's in the URL when you go to the library in studio). Hit Save.
5. Click "Update now with latest components from the library".
6. Optional: Click "View >" to preview all the blocks found in the library.
7. Hit "Publish", then go to the LMS. Browse into the course, and you should see that the block is replaced by random blocks from the library. To test the randomization, click the lowest "STAFF DEBUG INFO" link underneath the module, then click "Delete Student State", then refresh the page.

Author concerns:

1. LibraryContent is implemented as an XModule - should likely to be implemented as XBlock, though it's unclear if the necessary infrastructure is yet implemented in the runtime[s].
2. Tests assert against displayed text -- will break if (when) text changed; see inline notes for details on this.

[openedx-webhooks]

Thanks for the pull request, @e-kolpakov! I've created OSPR-260 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.

[e-kolpakov]

This is a WIP for some time as there are some improvements to be done (e.g. bokchoy tests), so submitting for preliminary review and a place to host conversations regarding.

[e-kolpakov]

@cpennington this library content block is implemented as an XModule now, since it was initially implemented like so. I've tried converting it to a XBlock and had some difficulties, mainly due to high coupling to XModule ecosystem (MakoModuleDescriptor, StudioEditableDescriptor)

So the questions are:

• Are there any existing helpers similar to MetadataOnlyEditingDescriptor for XBlocks? As of now this LibraryContentModule does not use it directly, but essentially do exactly the same thing.
• If it's not, what is an expected way to handle studio edits? I believe having custom js + studio_submit XBlock.handler is a common practice, but it looks tedious to reimplement it for every single XBlock. Are there any shortcuts?

[e-kolpakov]

I don't like the way how assertion is done here. It's better tested if assertion against XBlock extracted from modulestore is done. But this would couple acceptance tests to modulestore, which is not the case now (or at least it looks like acceptance tests are abstracted from modulestore concept).

So, if it's ok to couple tests to modulestore and it's possible to use modulestore from tests level - this test can (and should) be rewritten to assert against XBlock instance, rather than displayed text.

[cahrens]

I don't think acceptance tests should be coupled to the modulestore. The point is to test the whole workflow "as a user". I would expect testing of xblock internal state to occur at the unit test level.

Yes, the test will change if the error text changes. But it should-- the point is to test what the user is seeing.

[marcotuts]

I can check in with @andy-armstrong in person, but my understanding was that there already is an existing .xblock-header / .xblock-message area we could reuse for this message? Just in case we do I wanted to add a comment while skimming through this.

[marcotuts]

Sorry, I just checked the sandbox to see where this is located. There may be another pull request to resolve this, but our tentative plan was to move in the "up to date" and "updates" messages into the editor modal ( reference: http://invis.io/9X1PRVSGV) and validation messages to use the latest Studio patterns (reference: http://invis.io/BK1RLKQAX).

If there is another pull request feel free to ignore this comment! Thanks!

[bradenmacdonald]

Yep, we have another PR coming soon that will address those changes (I think it's based on our internal PR 19).

[bradenmacdonald]

This is rebased and is ready for a review. Please note the "Author Concerns" in the description before reviewing.

All tests/quality should be passing, but something is wrong with Jenkins at the moment, and it is showing everything failing due to FATAL: Unable to load resource META-INF/services/org.apache.commons.logging.LogFactory. I'm not sure how to fix that, as I tried pushing a new build but it shows the same error.

@cpennington Can you review the XBlock?

@explorerleslie @cahrens Can you review the Studio editing portion of the XBlock?

CC @antoviaque

[antoviaque]

Sandbox updated with the latest code from the branch: http://content-libraries.sandbox.opencraft.com:18010/

[cahrens]

Need to define a user-friendly display name on the library block (not library_content). Can just a default value for the display_name property.

Note also that the messages you are showing (not configured, out of date, etc.) are not consistent with how our validation messages are shown. Use the existing validation framework (I linked to an example below).

[cahrens]

Side documentation talks about content experiments and reordering. Something needs to be done about the very long number at the top.

[cahrens]

Use the existing validation framework. All you need to do is extend "validate" and return messages. The rendering will be done for you in a way that is consistent with other xblocks.

For an example, see https://github.com/edx/edx-platform/blob/master/common/lib/xmodule/xmodule/split_test_module.py#L523

I would prefer you not check in this HTML and then remove it. So if you have a separate PR that does the validation (I just finally saw the comments above...), go ahead and combine it with this PR. That is, combine the notification part of the PR only. Note that even the "This component is out of date" message can be replaced by a notification message-- you can register actions.

[cahrens]

When you first specify the library, you get the message "This component is out of date". Why? It should sync up immediately with what is in the library, and I would expect to only get that message later if the library subsequently changes.

[cahrens]

Have you verified that Preview of draft content works properly? I don't think your sandbox is properly configured for Preview.

[cahrens]

Have you verified (or do you have a story for it) that grading works properly? That is, that the student is only graded on the problems he/she sees?

[cahrens]

Why adding course, chapter, and sequential? Is there any performance hit?

[bradenmacdonald]

I added those types because I saw that in practice, they were already present in StudentModule anyways (to store the position field). This has no performance impact and only fixes the UI in the Django admin app.

BTW, the LMS team may want to just remove this enum completely, as it seems that almost any type of module is now stored in StudentModule and this enum is perhaps vestigial code.

[cpennington]

Yeah, we should just remove this list.

[bradenmacdonald]

Thanks @cptvitamin. BTW for now we are just merging to a feature branch and I believe we will have another chance to review everything when the feature branch gets merged to master.

[marcotuts]

@bradenmacdonald - Looked great to me, thanks for all the updates! Very glad we added in the library location ID section to the sidebar as well! (cc'ed @catong)

👍 from me.

[frrrances]

@bradenmacdonald Nice work! Thanks for switching over to use the existing warning patterns and adding in the Library ID block in the sidebar. I had a few UX level questions, which may be out of the scope of this work, but I think it would be good to make these changes in the near future if possible:

1. Can we add some context at the top of the Library pages to provide some way finding? Either add the word “Library” on to the Library name in the header or maybe a breadcrumb for the short term. (i pencilled in the two options in the screenshot below - one or the other but not both):

1. Can we have an "add component" button at the top of a content library page? It can be a scroll-to that imitates the green header “add” buttons elsewhere in Studio, like this:

1. It would be nice to be able to toggle the previews on the Content Library page - and it might help the initial page to load faster if its starts with “Hide Previews” (and the user can select “Show all Previews” or just select “Show Preview” on any one component).

None of these are blockers, but I think if we can do any of these, it will help the users. 👍

[dan-f]

Hi all. Just a heads up that we are borrowing these excellent page methods in #6227. Here's the diff. Whichever PR lands later will need to rebase.

[bradenmacdonald]

Thanks for the heads up!

[bradenmacdonald]

@frrrances Thanks for the feedback. The third thing has already been implemented and we will be opening a PR and merging it soon. The first two things are outside the scope of this PR, but I will add them to our list - they are both easy changes.

[bradenmacdonald]

Rebased and merged. Thanks to everyone who helped review and improve this PR!

[marcotuts]

@frrrances - Just as a follow up, the first item in your list is currently scheduled for inclusion in https://openedx.atlassian.net/browse/SOL-80, which should be merged in the coming week or more. The second "+ Add Component" scroll to is something I can create a backlog story for now (https://openedx.atlassian.net/browse/SOL-195), though I'm hopeful in the short term that the pagination and expand/collapse functionality lessens the scrolling issue. Thanks for your review and feedback on this! (Also thanks to @bradenmacdonald and @e-kolpakov!)