Backend: API to link a unit into a course

[bradenmacdonald]

1. Implement a REST API to link a unit into a course (for Add existing unit workflow (course level) #1633 ).

2. Make sure that copy-pasting a unit from a library into a course is also working and has the same effect. This will be a separate story: Units can be copy-pasted from a library into a course #1647

[navinkarkera]

@bradenmacdonald Have a doubt about point 2. I was investigating it today and came to the conclusion that to support copy and paste of units from Library to course, we need to convert unit content along with children to xml. Just want to confirm before I start doing it, if you have any better ideas. Work in progress ref: openedx/openedx-platform@24ec901

[bradenmacdonald]

@navinkarkera Sorry, I didn't realize that was mentioned here. We only need to do (1) for this story, and leave (2) for later.

[bradenmacdonald]

@navinkarkera @kdmccormick @ormsbee

OK, so here's where it gets complicated and we hit the problem we punted in our earlier discussion of should container keys be usage keys?

The status quo:

• Containers are not XBlocks and have Container Keys, not Usage Keys
• Containers cannot be copied to the clipboard because the StagedContent model assumes OLX format and block_type. (We could make this more flexible, or we can convert everything to OLX/blocks.)
• The Library XBlock runtime does not support XBlocks with children and can't render them, ever.
• The upstream_sync code assumes the upstream is identified by a LibraryUsageLocatorV2, can be accessed using the Libraries XBlock API, and has the same block_type as the downstream (note: units have container_type unit, different from block_type vertical)
• The PublishableEntityLink model is actually pretty generic and though it makes a few assumptions about upstream having a usage_key, it could be modified to not make those assumptions and work equally well for tracking container upstreams or xblock upstreams. This part is pretty simple because it doesn't do syncing, just tracking.

So, the question is: how do we want to modify things to support (A) upstream->downstream syncing where upstream is a Unit/container in a library, and (B) copying a unit from a library and pasting it into a course (lower priority than A)?

Option 1: modify the upstream_sync code so that the upstream is allowed to be a container key, not just a usage key. I think this is actually relatively simple, as the majority of the code is related to the downstream block (no changes needed) or to syncing specific fields (we don't have any fields to sync for now if it's a container - just display_name). Similar modifications to PublishableEntityLink would be required. The downside is that this doesn't help us with the clipboard API, and may have to be reworked completely if/when we have to sync fields that exist on containers. (But we also know that the whole upstream_sync thing will need to be reworked completely if/when we support containers/learning core in courses.)

Option 2: make container keys into usage keys, and allow them to be accessed as emulated XBlocks. The library XBlock runtime doesn't have to support rendering nor handlers for them, but should be able to represent the containers as OLX (it's easy: <vertical display_name="x">...</vertical>) and provide read-only field data, via a container-to-xblock emulation layer. This has the advantage that the existing upstream_sync and staged_content APIs can generally be used without changes. However, containers aren't really XBlocks and many other APIs that accept a usage key would fail/error if you passed in a container key. This also has the problem that the container_type ("block_type") of containers would be unit and incompatible with the vertical type expected by other APIs, and the OLX XML won't match the key^{1 2}.

Option 3: create a mapping from some UsageKey keyspace onto container keys, so that every container like lct:BradenX:devlib:unit:intro-unit has an associated "virtual" usage key, like lb-mapped:BradenX:devlib:vertical:unit:intro_unit. This sort of mapping key would have both block_type=vertical and container_type=unit, would be a UsageKey, and would be used only for the read-only emulation layer which we'd have to implement exactly as described in Option 2. I like that this makes the distinction between containers and xblocks much stronger and clearly shows when the emulation layer is in use, but the huge downside is having two different keys referring to the same piece of content. (Though in most contexts only one key type is allowed so it may not be that problematic?)

Option 4: ??? I'm probably missing other approaches.

Other thoughts:

• Eventually we need to make some major restructuring to the clipboard API anyways, e.g. to better associate static assets with the block that's using them when you copy an entire section/subsection onto the clipboard, and if/when we convert courses to use containers instead of XBlocks for their structure.

Footnotes

1. Though the LibraryContainerLocator opaque key could potentially hack around this by implementing block_type as a property function that returns vertical when the concrete container_type field is unit. But this may not work if the superclass requires block_type to be a concrete field encoded in the key string. ↩

2. Though muddying the waters, there is actually a unit XBlock and we could use it instead of verticals, but I think that we better stick with vertical to avoid causing compatibility issues. I know because I made unit but we never used it much outside of LabXchange. ↩

[bradenmacdonald]

@navinkarkera has some code at openedx/openedx-platform@24ec901 which is kind of an example of an emulation layer, but I don't like the approach of treating "library with containers" as a different [type of] learning context than regular libraries. That's why I'd prefer Option 2 or Option 3 above, though I really am not super happy with any of these approaches.

[navinkarkera]

@bradenmacdonald Thank you for writing this down, I was investigating the same and was preparing for modifying upstream sync code to allow containers.

[ormsbee]

I guess my initial thought is that we might be missing an abstraction here–i.e. an Upstream model with multiple subclasses for different types of upstreams (XBlock components, containers, whatever else), and a means for delegating out to each upstream-type the responsibility for knowing how to encode itself into a StagedContent.

[kdmccormick]

Of the options presented I strongly prefer Option 2, with block_type=unit.

The sync code could be easily adapted to allow certain allowlisted mismatches between upstream/downstream block_types, in particular, allowing unit->vertical syncing.

[bradenmacdonald]

With option 2, wouldn't we lose the ability to indicate via typing which python APIs support containers and which support XBlocks, going forward? I'm not totally opposed to that option, but I do have a feeling that it will evolve into a messy system. If our code implicitly allows containers and xblocks to be used interchangeably in many contexts, I foresee a lot of technical debt accumulating over time as people continue to blur the lines and even "fix" things to erase the distinction even further. Instead of becoming a forcing function for us to implement container support in new places, it becomes a forcing function for us to make the new system work exactly like the old.

If we assume that explicit is better than implicit, then I'd prefer option 1 (or similar, with an abstraction as suggested by @ormsbee) or option 3. In other words, I'd prefer that Container->XBlock emulation should be done only if/when necessary and explicitly requested, enforcing our design decisions around containers and promoting separation of concerns.

The sync code could be easily adapted to allow certain allowlisted mismatches between upstream/downstream block_types, in particular, allowing unit->vertical syncing.

Yes, though I think it can also be adapted to allow containers as upstreams.

[navinkarkera]

So, I have a very rough working PR: openedx/openedx-platform#36553 which is just updating the sync code to handle both usage_keys and container_keys explicitly and it is not pretty. (PublishableEntityLinks are not created yet). I am going to now try option 2 or 3 to emulate or convert container_keys to usage_keys and see if it simplifies the code.