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

[bradenmacdonald]

@antoviaque @explorerleslie @cahrens

Background: This is the UI code to enable creation and editing of Content Libraries. Together with #6033, it implements SOL-1, SOL-2, and SOL-3. Once #6033 has been merged to master, this branch is to be merged to content-libraries, which will become the new feature branch for library support.

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 #6033.

[openedx-webhooks]

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

[antoviaque]

Thanks @bradenmacdonald - looping in @smagoun too.

[cahrens]

Please write Jasmine tests for this file.

[bradenmacdonald]

It's covered (mostly) by index_spec.js.

[cahrens]

Just make sure that the logic is actually being tested. You can get "coverage" if the file is simply loaded, but that doesn't mean you are actually testing the code.

[e-kolpakov]

@cahrens made sure that coverage report is due to actual testing.

[cahrens]

@bradenmacdonald I am working to get a T&L review of this code underway. We typically require 2 code reviewers for any non-trivial PRs. Can you please get a 2nd code reviewer from the Solutions team? It is also a good way for the Solutions team to become more familiar with the code.

[sarina]

Is there a reason why the "Note" string is different here than at L141? For ease of translating it would be great if you could reuse the same string(s) in the two places.

[sarina]

I did an i18n review of all marked strings and found some unmarked ones as well.

[cahrens]

@bradenmacdonald Please take a look at the bok choy test that failed. If you think it is unrelated to your PR, get in touch with testeng to let them know that there is a sporadic.

Here is the current code coverage report:
https://jenkins.testeng.edx.org/job/edx-all-tests-manual-pr/1012/SHARD=1,TEST_SUITE=unit/artifact/reports/diff_coverage_combined.html

I asked Steve to help you get a sandbox created so we can try out the feature (I'm having trouble finding his github username).

@andy-armstrong will be doing the T&L review of the front-end code, and I will be doing the T&L review of the Python code in this PR.

[cahrens]

Why is it necessary to get "get_courselike" (FYI, that name is not helpful) given you have already verified that xblock.location is not a LibraryUsageLocator?

[cahrens]

Please take a look at the bok choy test failures. At least some of them have failed on your branch more than once.

[cahrens]

Nit: it seems odd to have the translation of the different string parts occur different places. Perhaps just pass the complete translated string to the template here. For example (psuedo-code),

// Translator comment.
enabled_types = _('courses and libraries') if LIBRARIES_ENABLED else _('courses')
listing_description = _('Here are all the {enabled_types} you...'.format(enabled_types))

Pass listing_description to template.

[sarina]

Would it be the worst thing in the world to just have two complete strings that you switch between? Eg,

if LIBRARIES_ENABLED:
    listing_description = _('Here are all the courses and libraries you...'.)
else:
    listing_description = _('Here are all the courses you...'.)

I'm relatively 😑 either way, but this way is easier to understand re: translations.

[cpennington]

In fact, there may be languages where substituting the translation for 'courses' and for 'courses and libraries' into the same spot isn't correct.

[cahrens]

The following things need to be followed up on, but otherwise I am 👍

1. Error when editing ORA problem (noted by @andy-armstrong)
2. Bok choy test failures
3. Talk to @cpennington about the proposed changes to core.coffee and xmodule_modifiers.py.
4. Squash commits once reviews are done.

Otherwise, it's looking great!

[e-kolpakov]

@cahrens @andy-armstrong there's a problem regarding ORA problem :)

In two words, before saving itself it checks if there are published version, than shows message/asks confirmation if it is already published. The check is essentially "are there such xblock in published-branch branch of whatever the top level structure it is". the problem is, libraries always use library branch and basically "published/non-published" is not applicable to libraries.

There are various ways to solve this, e.g. allowing libraries to have published and non-published versions, etc. The most straightforward and logical though (at least in my opinion) is to always consider all libraries as non-published. So this is the approach I used to fix that issue with ORA in library.

Anyway, I believe this question deserves some thinking as some architectural decisions must be made.

[e-kolpakov]

@andy-armstrong I couldn't reproduce "Numerical Input" problem issue. According to error message, server run out of memory, which correlates to the fact that sandbox was down yesterday. So most likely that was temporary issue having nothing to do with this PR.

[e-kolpakov]

Squashed commits so that PR can be merged as soon as #6033 is merged (might require rebasing though)

[andy-armstrong]

I double checked and the numerical input problem issue has indeed gone away. The way you solved the ORA editing issue also makes sense to me, at least for the short term. 👍 - awesome work guys!

Before checking in, could you amend your commit to include your JIRA ticket number. We use this for tracking purposes. Generally the first line of the commit message is a short summary followed by a blank line, and then the third line is the JIRA ticket number. For example:

edx@c2c40eb

[cahrens]

@e-kolpakov You are only merging this to a feature branch, not master. Therefore I think this PR can be merged to the feature branch before #6033 merges to master. You will then need to rebase the feature branch on master once #6033 has merged.

[antoviaque]

@andy-armstrong Thanks for noticing this - I think @e-kolpakov is out for the day now (it's pretty late for him) so I went ahead and added the stories to the commit message.

@cahrens Yup, sounds good, that's also what I had in mind for #6033.

Are we all good to merge? : )

Thank you for the thorough review btw, the quality of the PR improved a lot because of it!

[antoviaque]

Just chatted with @andy-armstrong , we need two thumbs up - so I think this should be good, I'm merging. If there is any objection let me know - this goes to a feature branch so we can easily correct it there if needed.