Content libraries UI Updates (SOL-80)

[mtyaka]

• Background: This PR contains UI Updates for the content-libraries studio view and content block.
• Jira tickets: Implements SOL-80 and the reviews from Studio support for creating and editing libraries (PR 6046) open-craft/openedx-platform#23
• 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
• Sandbox URL: LMS: http://sandbox.opencraft.com/ - Studio: http://sandbox.opencraft.com:18010/
• Internal code reviews PRs: Content libraries UI Updates open-craft/openedx-platform#19
• Partner information: 3rd party-hosted open edX instance, for an edX solutions client.

[openedx-webhooks]

Thanks for the pull request, @mtyaka! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: http://openedx-webhooks.herokuapp.com/github/process_pr?repo=edx%2Fedx-platform&number=6388

[Kelketek]

It should be noted that some of the commits in this branch do a bit of quality improvement in unrelated areas due to the way run_quality was reporting when this was first worked on a month ago. This is not a bad thing, necessarily, but the reviewers may not want it in this PR.

[antoviaque]

Created a sandbox with the current code:

• LMS: http://sandbox.opencraft.com/
• Studio: http://sandbox.opencraft.com:18010/

@catong FYI, this is the most complete sandbox so far - it contains most of the MVP features of content libraries, and implements the UI changes discussed so far, either directly with you and/or Marco, and from the reviews on open-craft#23 . There are, however, three other MVP features which aren't yet merged and are still in a separate PR - they are however fairly isolated so it should be easier to review them separately than the state we were in a couple of weeks ago:

• Library permissions page: https://github.com/edx/edx-platform/pull/6283 (not a MVP feature, but important to allow non-staff users to use libraries)
• Problem type filtering: https://github.com/edx/edx-platform/pull/6346
• Settings overrides: Content libraries settings overrides (SOL-46) open-craft/openedx-platform#25

Let me know if that allows you to move forward with your review - if so you can simply comment in the current PR. Also happy to answer questions.

[antoviaque]

@mattdrayer @chrisndodge Would either of you be willing to review this PR too?

@marcotuts @frrrances You might also want to review these changes.

Thank you! : )

[catong]

Thanks, @antoviaque the updated sandbox will be very helpful. The only thing I don't understand is the settings override: what story is this associated with?

[antoviaque]

@catong Yes, that one has a pretty technical description as it originally came out of the arch council review - the story is https://openedx.atlassian.net/browse/SOL-46 but that's also fairly technical.

In a nutshell, this story cleans up the behavior of the LibraryContent XBlock when an author edits the XBlock it contains as children. Once imported in a given course, the XBlocks copied from the library can be edited - for example, to change the weight of a given problem, or a update a title. The idea is that the changes only apply within that particular LibraryContent XBlock content - the original XBlocks are preserved in the library, and it's possible to revert the values to the original values using the "undo" button next to the settings fields.

[catong]

@antoviaque Thanks! I looked in the sandbox on that PR (Settings overrides: open-craft#25) but can't find that new UI control. Is it at the individual problem level?

[chrisndodge]

Sorry, I just need a little context here: why are we removing this block of introductory text? Sorry if this was already covered elsewhere.

In particular, I'd be interested in - if there are no courses for a user - there's sufficient information presented to the user.

[chrisndodge]

OK, I just looked at the sandbox, and I created a new account (to see what happens when no courses are associated to the user) and - to me at least - I see what appears as sufficient messaging to the end user.

[antoviaque]

@catong Yes, it's at the individual problem level. There isn't any new UI, it uses the usual settings window and fields for each individual XBlock.

[antoviaque]

@cahrens These are small UI/UX/doc fixes, but you might want to review too.

[cahrens]

I created https://openedx.atlassian.net/browse/TNL-1094 for the TNL review.

[catong]

@antoviaque, when I go to: Outline > Unit > Library block > View > Individual problem > Settings I don't see any "Reset" button that would reset all the settings to the original library version. Sorry if I'm being dense. :-} Would you mind providing a screenshot, or a more detailed pointer? Thanks!

[antoviaque]

(Discussed directly with @catong for the settings overrides - current PR is at https://github.com/edx/edx-platform/pull/6399 )

[marcotuts]

have we handled updated urls in the past in a way that would redirect older bookmarks to the new /new/ url address? I don't know how critical this is but it might be worth noting for potential documentation or even support issues. (cc'ed @catong )

[cahrens]

It does seem that a redirect from /course/ to /home/ would be good.

[mtyaka]

@marcotuts @cahrens We do redirect to /home/, in https://github.com/open-craft/edx-platform/blob/content_libraries/11-UI-updates/cms/djangoapps/contentstore/views/course.py#L235

[antoviaque]

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

I talked to @mtyaka about the test failures, he's aware and will fix them.

[catong]

This tip text and sidebar text should be replaced by the text in cms/templates/library.html in PR https://github.com/edx/edx-platform/pull/6426

[Kelketek]

@cahrens Should be good to go now. If there are no other notes, I'll ping you once the build is green.

[Kelketek]

@cahrens I found that the build was failing and so came back to this. JavaScript seems to love surprising me.

Assigning the prototype did allow the functions to become available, but it did not properly rescope this during the construction, which meant that use of the self variable within the CreateUtilsFactory functions still understood this to be CreateUtilsFactory and not CreateCourseUtilsFactory.

I decided to use call instead of apply here. I looked it up and apply is just call but expects the second argument to be an array. This would explain why it never worked, since I was handing it an object, but expecting it to be handled as one unit. JavaScript threw up and made selector and classes undefined, because it's better to plow ahead even when uncertain, apparently. :P

Anyway, this solution does allow this to be scoped properly. The prototype doesn't need to be set here, but using call takes care of everything, things look right and the test passes. As I no longer need to refer to the function, I'm just returning it directly instead of naming it, as well.

[cahrens]

Just when I thought I was understanding.... Why only pass selectors and classes to call, and then define the rest of the things afterwards (keyFieldSelectors, create function, etc.)? I'm not following what is "special" about selectors and classes.

Yes, JavaScript is often surprising!

[Kelketek]

@cahrens I could move them there. I'll do so.

[cahrens]

I'm also not understanding why the prototype no longer needs to be set. Sigh...

[cahrens]

Sorry to keep circling on this, but I think part of my confusion is I don't understand what these utility files are returning at this point. Should they be returning the result of CreateUtilsFactory.call(this, selectors, classes)?

[Kelketek]

They are returning a function. The function acts as a constructor. By using call, we use CreateUtilsFactory as an extension of the constructor. So, in reality, no prototyping is involved, we just steal all of that function's this assignments by passing it our own this. It's (almost) the equivalent of a PHP or C include, where all of the code would be dumped right into the current function. Not quite, because it wouldn't have access to all of the names, just this.

[Kelketek]

@cahrens Is this better? I decided not to pass along create as well because I wanted its function nature to be a bit more explicit in both files.

[cahrens]

Yes, I think this is better-- thanks! You can go ahead and merge. 👍

Honestly though, I'm still confused about how the helper methods (for instance, validateTotalKeyLength) end up getting returned. create_utils_base returns them in the function it defines, but I don't see how that gets passed through create_library_utils and create_course_utils. Obviously it is working, but there is something about this pattern of "inheritance" that I'm still not understanding. If you have time to explain it to me, I'd appreciate it. :)

[Kelketek]

.call is a function that's available on JavaScript function objects. It's a version of the function that allows you to call a function while specifying an alternative this. So when we call the helper function via .call, anything that is actually set on this in the helper function is set on the function that called it.

This link goes over this, from the basics to more involved uses like in a .call. It may aid in understanding to read the whole thing, since it builds up to the explanation:

http://www.w3schools.com/js/js_function_invocation.asp

[cahrens]

Thanks, I will look over that link. I do understand .call and this (I think!), but I'm still stumped by how the "return" works. To me, it feels like line 12 should be

return CreateUtilsFactory.call(this, selectors, classes, keyLengthViolationMessage, keyFieldSelectors, nonEmptyCheckFieldSelectors);

But there is also a further line of code beyond that, defining the create function.

Just struggling to understand how the functions defined by create_library_utils and create_course_utils ultimately return this--
{ validateTotalKeyLength: self.validateTotalKeyLength,
setFieldInErr: self.setFieldInErr,
hasInvalidRequiredFields: self.hasInvalidRequiredFields,
create: self.create,
validateFilledFields: self.validateFilledFields,
configureHandlers: self.configureHandlers
};

[Kelketek]

This function is called via new. It's an object constructor. An object constructor need not return anything, its this becomes the return.

So:

var thing = {'stuff': 1, 'more_stuff': 'hello'}

...is equivalent to:

thing_maker = function () {
    this.stuff = 1;
    this.more_stuff = "hello";
}
var thing = new thing();

Instead of returning an object, we're returning a constructor, so create is part of the resulting object.

[cahrens]

AHHH, that's what I was missing. Thank you.

[Kelketek]

@cahrens @antoviaque I'm not able to get Jenkins to work properly. It's dying on tasks left and right.

[Kelketek]

@cahrens @antoviaque Not sure who fixed it, but looks like the tests reran and are green!