Content libraries settings overrides (SOL-46)

[bradenmacdonald]

@dmitchell if you have time, can you please comment on the technical approach I've taken here?

The idea is that we want blocks stored in content libraries to have their Scope.settings fields treated as defaults, which can be overridden when the blocks are actually used in a course.

To make this work and to reduce the number of database queries, I have approached this as follows: For blocks stored in content libraries, their Scope.settings fields are no longer stored in the structure but are instead stored in a new defaults field of the definition.

Thus, when a block is actually used in a course, to read its Scope.settings field values, we check (in order of priority):

1. The fields of the block's structure entry
2. The defaults of the block's definition (but because definitions are lazy-loaded, we only check if a "has_defaults" flag is set on the structure).
3. I think in some cases we also check parent block's values (inheritance).

Notes:

• This approach works well with the studio UI. When overriding a library content child within a course context, you can tell if an override is set or not, and pressing the "reset" button resets back to the library value, not the normal XBlock value.
• What's the purpose of split's persist_xblock_dag and _persist_subdag methods? They are only used in a couple tests as far as I can tell, yet they are not in a tests module... I didn't update them to be compatible with this feature.

One potential problem is that the definitions are often lazy-loaded, so in order to check if the default is set or not, we sometimes now need to load the definition when in the past it wasn't loaded. This was causing some platform tests to fail because more mongo calls are used than before. Possible solutions:

• Set a has_defaults flag on the structure - this is what I have done for now
• Avoid/reduce lazy loading
• Change code to only check for defaults if block is a descendant of a LibraryContentBlock
• Don't allow XBlock fields to be overridden in this way - use some other approach

CC @antoviaque

Sandbox:

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

[dmitchell]

Sorry. I'll put this on the top of my stack tomorrow.

[dmitchell]

My main objection is that every field which we need to access for outlines, navigation, or frequent reads should be in structures. The bulky and only-needed-when-deeply-looking-at fields should be in the definitions collection. Having the same field in 2 places is no problem and is paradigmatic noSql design.

How to differentiate these fields at this level is not clear to me. We know we need children, display_name, start, due_date, and cohort information (anything having to do w/ authz). The nice thing about duplicating them is that the duplication only needs to occur on write. Read only needs to do the lazy load if the field isn't loaded; so, rather than scope.content being the trigger for lazy load, it should be some way of telling whether the field isn't part of the privileged set.

Another nice thing about splitting these by frequency of reference is that the erroneous markdown field would get pushed out to definitions where it belongs rather than settings. It's totally hogging document size.

[dmitchell]

Re 'persist_xblock_dag and _persist_subdag', they were an early attempt to handle bulk operations by allowing the handlers to construct in memory courses and then call persist xblock dag to save the whole thing to the db. They should be used by import or clone, for example, but because we didn't impl them for old mongo, we didn't. They could perhaps go away.

[dmitchell]

In looking at this code, I was also worried about set not having a symmetrical operation. It's not caused a problem, but it seems strange.

btw: I'm changing these same methods in a current PR for Asides so we're going to collide

[bradenmacdonald]

@dmitchell Ok, I'm going to try putting the defaults into the structure instead of the definition and see how that works. The code should be simpler and need fewer changes from how it works today. It will duplicate some of the data, but we're doing that already with content libraries, so that's not a real loss.

[dmitchell]

This seems too proactive. I'd rather have it wait until the fields were needed. Not that I have a ready set of changes to do that. The trouble with this will be that library pages will use significantly more queries even if they're just showing an outline.

[bradenmacdonald]

@dmitchell I have re-written this code to store the inherited values in a "defaults" value in the structure. Although this does result in duplicating those values (once in the library block structure and once in the course block structure), I think it's overall a lot cleaner. It no longer makes changes to the definitions at all and no longer requires lazy loading. Also, it now is backwards-compatible since it requires no changes to how library blocks are stored.

What do you think?

[dmitchell]

That makes sense for your scope of work. I'd still like to not save as many fields in the structure doc as we are but ensure we do save the ones we need for course navigation and other freq tasks. idk how to differentiate these as I said before.

[antoviaque]

@bradenmacdonald Added a sandbox in preparation for the upstream PR: http://sandbox3.opencraft.com/ http://sandbox3.opencraft.com:18010/ - I've put the latest code from this branch on it.

[antoviaque]

Upstream PR opened: https://github.com/edx/edx-platform/pull/6399

Closing this.