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

e-kolpakov · 2014-12-05T10:43:52Z

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/edx-platform#5 (WIP) Graded library content - Do not merge, for internal review open-craft/edx-platform#3 (WIP) Randomized selection from content libraries - Do not merge, for internal review open-craft/edx-platform#2

Instructions to test:

Create a Library with some content, and a course
In the course, go to Settings > Advanced Settings and add "library_content" to advanced_modules.
Go to Content > Outline. Create a new section and subsection, then edit it. Click "Advanced" and add a new "library_content" block.
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.
Click "Update now with latest components from the library".
Optional: Click "View >" to preview all the blocks found in the library.
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:

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].
Tests assert against displayed text -- will break if (when) text changed; see inline notes for details on this.

openedx-webhooks · 2014-12-05T10:43:54Z

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 · 2014-12-05T10:47:32Z

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 · 2014-12-05T11:53:26Z

common/lib/xmodule/xmodule/library_content_module.py

+
+
+@XBlock.wants('user')
+class LibraryContentDescriptor(LibraryContentFields, MakoModuleDescriptor, XmlDescriptor, StudioEditableDescriptor):


@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 · 2014-12-08T12:25:39Z

common/test/acceptance/tests/studio/test_studio_library_container.py

+
+        library_container.save_settings()
+
+        self.assertIn(expected_text, library_container.header_text)


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.

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 · 2014-12-09T17:25:45Z

lms/templates/library-block-author-view.html

+from django.utils.translation import ugettext as _
+from django.core.urlresolvers import reverse
+%>
+<div class="xblock-header-secondary">


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.

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!

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

bradenmacdonald · 2014-12-10T07:37:05Z

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 · 2014-12-10T13:17:20Z

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

cahrens · 2014-12-10T17:04:13Z

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 · 2014-12-10T17:06:15Z

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

cahrens · 2014-12-10T17:08:54Z

lms/templates/library-block-author-view.html

+from django.core.urlresolvers import reverse
+%>
+<div class="xblock-header-secondary">
+    % if library_status == LibraryStatus.OK:


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 · 2014-12-10T17:13:56Z

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 · 2014-12-10T17:17:29Z

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

cahrens · 2014-12-10T17:18:42Z

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 · 2014-12-10T17:29:07Z

lms/djangoapps/courseware/models.py

@@ -32,6 +32,10 @@ class StudentModule(models.Model):
    MODULE_TYPES = (('problem', 'problem'),
                    ('video', 'video'),
                    ('html', 'html'),
+                    ('course', 'course'),


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

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.

Yeah, we should just remove this list.

bradenmacdonald · 2014-12-19T22:14:32Z

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 · 2014-12-19T22:18:23Z

@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 · 2014-12-23T14:58:25Z

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

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):

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:

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 · 2014-12-23T15:12:01Z

common/test/acceptance/pages/studio/container.py

@@ -334,6 +342,45 @@ def children(self):
        return [descendant for descendant in descendants if descendant.locator not in grand_locators]

    @property
+    def has_validation_message(self):


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.

Thanks for the heads up!

bradenmacdonald · 2014-12-23T18:54:22Z

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

… container view

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

bradenmacdonald · 2014-12-23T20:51:33Z

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

marcotuts · 2014-12-24T15:41:44Z

@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!)

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

openedx-webhooks added the needs triage label Dec 5, 2014

e-kolpakov changed the title ~~(WIP) Content Libraries - Library Course XBlock~~ (WIP) LibraryContent - Display content from a library in a course (SOL-5, SOL-6) Dec 5, 2014

e-kolpakov reviewed Dec 5, 2014
View reviewed changes

openedx-webhooks added the community manager review label Dec 5, 2014

This was referenced Dec 5, 2014

Content libraries UI Updates open-craft/edx-platform#19

Closed

Add support for Import and Export of Libraries open-craft/edx-platform#21

Closed

e-kolpakov reviewed Dec 8, 2014
View reviewed changes

e-kolpakov force-pushed the content_libraries/3-library-course-block branch from bac6ee5 to e508ae4 Compare December 9, 2014 06:02

antoviaque changed the title ~~(WIP) LibraryContent - Display content from a library in a course (SOL-5, SOL-6)~~ (WIP) LibraryContent - Display content from a library in a course (SOL-5, SOL-6, SOL-8) Dec 9, 2014

marcotuts reviewed Dec 9, 2014
View reviewed changes

bradenmacdonald force-pushed the content-libraries branch from b7c340b to c87b2ac Compare December 10, 2014 05:18

bradenmacdonald force-pushed the content_libraries/3-library-course-block branch from e508ae4 to 9bac8cb Compare December 10, 2014 06:08

bradenmacdonald force-pushed the content-libraries branch from c87b2ac to 72e0c2c Compare December 10, 2014 06:59

bradenmacdonald force-pushed the content_libraries/3-library-course-block branch 2 times, most recently from 7afa32e to f85f962 Compare December 10, 2014 07:26

bradenmacdonald changed the title ~~(WIP) LibraryContent - Display content from a library in a course (SOL-5, SOL-6, SOL-8)~~ LibraryContent - Display content from a library in a course (SOL-5, SOL-6, SOL-8) Dec 10, 2014

cahrens reviewed Dec 10, 2014
View reviewed changes

e-kolpakov mentioned this pull request Dec 22, 2014

Library Content - Filter library content by CAPA problem type #6346

Merged

dan-f reviewed Dec 23, 2014
View reviewed changes

bradenmacdonald and others added 15 commits December 23, 2014 11:00

Library Content XModule

219eeb6

Unit and integration tests of content libraries

d2ae624

LibraryContent bok choy acceptance tests

d1681d0

Friendly error message when library key is invalid

e312c15

Added explanation to container view of Library Block.

eecc6f1

Made errors on Library blocks use validate functionality.

a7b577d

Move update link to the validation area

dfd0d70

Fix: don't need to reload the whole page to refresh_children from the…

e20b904

… container view

Refresh children automatically when library setting is changed

66f5f8f

Fix greedy intrusion of split_test documentation

c2f757f

Validation should be on the descriptor, not the module

82876da

ungettext fix

c7c1b4c

Move code that uses the modulestore to an XBlock service

0537345

Display the library key on the library view in Studio

262c875

Remove state summary from the unit author page until SOL-192

0cc3b33

bradenmacdonald force-pushed the content_libraries/3-library-course-block branch from 82b7bb8 to 0cc3b33 Compare December 23, 2014 19:31

bradenmacdonald added a commit that referenced this pull request Dec 23, 2014

Merge pull request #6155 from open-craft/content_libraries/3-library-…

16809dd

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

bradenmacdonald merged commit 16809dd into openedx:content-libraries Dec 23, 2014

mtyaka mentioned this pull request Dec 29, 2014

Content libraries UI Updates (SOL-80) #6388

Merged

bradenmacdonald deleted the content_libraries/3-library-course-block branch January 3, 2015 00:26

bradenmacdonald added a commit that referenced this pull request Jan 3, 2015

Merge pull request #6155 from open-craft/content_libraries/3-library-…

e291ad1

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

bradenmacdonald mentioned this pull request Jan 6, 2015

Content libraries MVP #6459

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

e-kolpakov commented Dec 5, 2014

openedx-webhooks commented Dec 5, 2014

e-kolpakov commented Dec 5, 2014

e-kolpakov Dec 5, 2014

e-kolpakov Dec 8, 2014

cahrens Dec 10, 2014

marcotuts Dec 9, 2014

marcotuts Dec 9, 2014

bradenmacdonald Dec 10, 2014

bradenmacdonald commented Dec 10, 2014

antoviaque commented Dec 10, 2014

cahrens commented Dec 10, 2014

cahrens commented Dec 10, 2014

cahrens Dec 10, 2014

cahrens commented Dec 10, 2014

cahrens commented Dec 10, 2014

cahrens commented Dec 10, 2014

cahrens Dec 10, 2014

bradenmacdonald Dec 10, 2014

cpennington Dec 12, 2014

bradenmacdonald commented Dec 19, 2014

marcotuts commented Dec 19, 2014

frrrances commented Dec 23, 2014

dan-f Dec 23, 2014

bradenmacdonald Dec 23, 2014

bradenmacdonald commented Dec 23, 2014

bradenmacdonald commented Dec 23, 2014

marcotuts commented Dec 24, 2014



		@XBlock.wants('user')
		class LibraryContentDescriptor(LibraryContentFields, MakoModuleDescriptor, XmlDescriptor, StudioEditableDescriptor):


		library_container.save_settings()

		self.assertIn(expected_text, library_container.header_text)

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

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

Conversation

e-kolpakov commented Dec 5, 2014

openedx-webhooks commented Dec 5, 2014

e-kolpakov commented Dec 5, 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

Choose a reason for hiding this comment

bradenmacdonald commented Dec 10, 2014

antoviaque commented Dec 10, 2014

cahrens commented Dec 10, 2014

cahrens commented Dec 10, 2014

Choose a reason for hiding this comment

cahrens commented Dec 10, 2014

cahrens commented Dec 10, 2014

cahrens commented Dec 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradenmacdonald commented Dec 19, 2014

marcotuts commented Dec 19, 2014

frrrances commented Dec 23, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradenmacdonald commented Dec 23, 2014

bradenmacdonald commented Dec 23, 2014

marcotuts commented Dec 24, 2014