Library Content - Filter library content by CAPA problem type

[e-kolpakov]

Background: This PR contains the LibraryContent XBlock, which allows to display library content in a course.
JIRA ticket: SOL-122
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/6155 (probably will require a rebase after 6155 merged)
Sandbox URL: LMS at http://content-libraries.sandbox.opencraft.com/ and Studio at http://content-libraries.sandbox.opencraft.com:18010/
Internal code reviews PRs: open-craft#24
Partner information: 3rd party-hosted open edX instance, for an edX solutions client.

Testing instructions:

1. Create a library with a couple of Problem XBlocks of different types (e.g. "Checkboxes", "Dropdown", "Multiple Choice")
2. Add Library Content XBlock to a course.
3. Edit Library Content settings. By default "Problem Type" should be set to "Any Type", LMS view should display problems of any type.
4. Set "Problem Type" to one of the problem types in the library (e.g. "Checkboxes", "Dropdown" or "Multiple Choice""). Please note that some problem type templates fall into same problem type (e.g. "Custom Javascript/Python" and "Drag and Drop" fall into "Custom Evaluated Script"). Save, publish, navigate to LMS as a student and ensure only Problems of selected type are shown.
5. Set "Problem Type" to any problem type not in the library (e.g. any except "Checkboxes", "Dropdown" or "Multiple Choice""). Save, publish, note the warning message in Studio, navigate to LMS as a student and ensure no XBlocks are shown.

[e-kolpakov]

This filtering should happen in LibraryToolsService.update_children as it has the following benefits:

• It happens once per XBlock, not once per student as in this version.
• It automatically updates "This component will be replaced by N components ..." in studio

However, I was unable to achieve that. update_children operates on XModuleDescriptors, not XModules, while filtering by CAPA problem type requires examining LoncapaProblem, which is available on CapaModule only. Unfortunately, the context here does not allow fetching XMoudule from XModuleDescriptor, as it uses CachedDescriptorSystem, which does not have all the machinery to support XModule => attempts to fetch XModule by XModuleDescriptor or usage key results in UndefinedContext.

@cpennington @dmitchell are there any ways to get actual CapaModule in such a context? To save you a little bit of digging, entry point for LibraryToolsService.update_children is LibraryContentDescriptor.editor_saved

[dmitchell]

It's odd that you're returning True if you don't know if it matches. You could parse the xml in the data field, but that seems horrid. (i.e., just do a substring search for self.capa_type in block.data.) This assumes that capa_type == the xml tag names (e.g., optionresponse | multiplechoiceresponse | choiceresponse...). The interesting thing about these is that a single block can have many (or none) capa types.

[e-kolpakov]

@dmitchell thanks for suggestion. capa_type is indeed xml tag names, so this is a very low-level approach, but could do the trick actually. Until CAPA problem format changes, but it doesn't look like it happens too often.

[e-kolpakov]

@mattdrayer @chrisndodge this is ready for review.

Please note that I'm on vacation 12/31/2014 - 01/04/2015, so I'll only have one day (tomorrow) to address review notes. Thus it would be nice to have this PR reviewed today.

[antoviaque]

Added a sandbox with the current code: LMS at http://content-libraries.sandbox.opencraft.com/ and Studio at http://content-libraries.sandbox.opencraft.com:18010/

[chrisndodge]

Is there a way we could do this via introspection in some way rather than explicitly listing all of this out? Basically, this approach leads to more things a developer has to do in order to add/remove CAPA problems. I don't think there's much activity regarding CAPA modules, but I figured I would ask.

[e-kolpakov]

@chrisndodge names for basic and advanced groups are taken from XML templates used for "Add Component". Others are my guess on how it should be named. Unfortunately there are no human names on response types and there are no simple rule for guessing that automatically. So, there are virtually two (and a half) options:

1. List them here as of now
   1.5. Invent a rule that would cover most, than list exceptions here
2. Add human_name property for all capa response types - this result s in shotgun-style changes, so decided to stick with (1)

Also, please note that these names are in sync with CAPA template names, and those names are taken from yaml file names.

[dmitchell]

@dan-f recently added (or avoided adding and did what you did) a category display name mechanism for xblocks which should generalize to this.

[cahrens]

@dan-f said he didn't do this. But I really don't like this explicit listing of all the problem types. Please work with @ormsbee (architect) to find a better solution.

[dmitchell]

It looks like you may be able to get the list of capa_types via introspection. You'd need to get all the subclasses of LoncapaResponse and get the tags property from them. Mapping these to the human readable names is still a problem but we should solve it by moving the human readable names to class properties on these classes, imho, rather than spreading out the mappings throughout the code. @cahrens either T&L should do this or should pick up on it after @e-kolpakov does it for his need and T&L should remove the hardcoded strings and get the class property instead.

[cahrens]

I think the work should be done as part of this PR rather than creating technical debt.

[ormsbee]

You shouldn't need to manually trace the subclasses -- we already collect that information in responsetypes.registry (it's a TagRegistry). I agree with @dmitchell that we should just add the human readable name as a class property.

[e-kolpakov]

@dmitchell @ormsbee @cahrens I was thinking of the option you described as well and decided that shotgun-style edits of adding a property to every capa problem would be a greater evil than having them all stored externally. There were probably other considerations, since initially it was implemented to work against input types, rather than response types. Anyway, looks like I guessed wrong :)

I'm on vacation, so just stopped by to answer notes and left some hints for @Kelketek, who will be taking care of this PR while I'm away. So, notes: we're talking about types in common/lib/capa/capa/responsetypes.py. There's also registry member in that module, which is a TagRegistry instance from common/lib/capa/capa/registry.py. Types will need human_name property, registry is to be used to build capa_types dictionary in library_content_module.py

[chrisndodge]

Can you indicate what formatting rules might exist here. I see an 'any' ENUM value, but should this otherwise be a comma-separated list? Or is it "any" or just one type? In any case, I'd suggest filling out the help message here with more specific instructions to the studio user.

[e-kolpakov]

@chrisndodge It's one type or no filtering at all. I'll update the help message (tomorrow though, it's pretty late for me already)

[e-kolpakov]

@chrisndodge added a minor feature according to the discussion here: https://openedx.atlassian.net/browse/SOL-122?focusedCommentId=94008&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-94008

In two words: it now checks there are enough child blocks passing filter test in libraries (i.e. >= "Count" setting)

[antoviaque]

Updated the sandbox with the current code: LMS at http://content-libraries.sandbox.opencraft.com/ and Studio at http://content-libraries.sandbox.opencraft.com:18010/

[e-kolpakov]

@explorerleslie @cahrens @frrrances @cptvitamin @dmitchell I believe this is ready for more thorough review. Please note I'm off 12/31/2014-1/4/2015, so most likely someone else will address any notes while I'm away.

[dmitchell]

Can you or someone add a story to the T&L backlog to do this TODO and change this to a reference to that story?

[cahrens]

There's already an TNL story for this-- https://openedx.atlassian.net/browse/TNL-993. I think just the comment needs to be updated.

[dmitchell]

👍 conditional on using responsetypes.registry & moving the human strings to class property.

[catong]

Suggested rewrite for this error message:
"There are no matching problem types in the specified libraries. Select another problem type."

[Kelketek]

@dmitchell @catong I've pushed some changes that should address these issues. Jenkins is reporting failures, but I can't reproduce them locally.

[Kelketek]

@cahrens This should be ready, but Jenkins keeps crashing every time I try to trigger a build.

[bradenmacdonald]

@e-kolpakov @Kelketek Because this PR will conflict with the Scope.settings PR 6399, I tried merging the branches together locally and sorting out the conflicts. In doing so I found a bug in 6399 (now fixed) and some ways to simplify the code in this branch. So I've just pushed a commit with those simplifications and some preemptive changes that will make it way easier to resolve conflicts with 6399.

The main simplification stems from the fact that LibraryContentModule's validate method is always called by Studio after editor_saved(), refresh_children(), and persisting to mongo have already happened. So we can remove the lines of code that were put in place only to make pending changes visible within the validate method, since that shouldn't ever be needed.

Feel free to amend or revert this change if you see any problems with it...

[e-kolpakov]

@cahrens diff coverage report Jenkins shows is a bit wrong: it compares this branch to edx/master, not edx/content-libraries. So I looked at coverage report and actual differences and it appears like this PR have 98% coverage - everything else reported as not covered in coverage report appears to come from other PRs already merged to edx/content-libraries.

Missing coverage in this PR:

• library_content_module.py:450-456 - added tests
• library_tools.py:117 - should be covered by cms/djangoapps/contentstore/tests/test_libraries.py:TestLibraries.test_block_with_children + I have confirmed that removing that line fails the test

With these added tests this PR have 100% test coverage.

So if 95% coverage for overall feature branch still concerns you I would suggest adding a new story for improving test coverage and repaying technical debt (if any). This PR/feature is not about pushing coverage on content-libraries branch to 100% after all :)

[e-kolpakov]

@bradenmacdonald just to let you know, at a glance everything (both tests and Studio) works just fine with your improvements - thank you!

[antoviaque]

Updated the sandbox with the latest code: http://content-libraries.sandbox.opencraft.com:18010/

[cahrens]

Since ↻ is no longer in the string that is scraped, translators won't see it. Therefore your comment doesn't really apply.

What WOULD be useful though is that you make the placeholder named and explain what will go there. For example, a placeholder named "refresh_icon" and a note for translators that refresh_icon is ↻. That way they can decide if the placement should be elsewhere relative to the text. As it is, they have no idea what will be substituted for {0}.

"{refresh_icon} Update now.".format(refresh_icon=↻), with appropriate translator comment above.

[e-kolpakov]

@cahrens sure, will do in a minute, don't go away too far :)

[cahrens]

Once the string translation is updated and you get a green build, 👍

[Kelketek]

@antoviaque lo, we have a green build! And there was much rejoicing.