Make ComponentVersions immutable - update how media is attached on their creation

[bradenmacdonald]

Problem 1: currently, you can call create_component_version_media to modify the media of any ComponentVersion, even one that is published or "historic". This could be considered a form of corrupting data.

"Problem" 2: code like this code for creating a library XBlock calls create_component_and_version and create_component_version_media which "feel" like they should be grouped together using bulk_draft_changes_for, but media APIs don't participate in draft change logs.

Suggestion: what do you think about modifying create_component_version_media so that it requires that it be used within a bulk changes context, and that the ComponentVersion in question is newly-created within the same bulk draft change context?

This would be a breaking API change but would enforce two best practices: only modifying ComponentVersion as they're created, and using bulk change contexts.

[kdmccormick]

@ormsbee What are your thoughts on this? It seems to make sense to me, especially since every ContainerVersion's child list is immutable.

[ormsbee]

I feel like bulk_draft_changes_for already does a lot, and I'd rather not use it for more things.

Could we add a media mapping arg to create_component_version, and make create_component_version_media an internal helper method?

[ormsbee]

So this:

        component_type = content_api.get_or_create_component_type(
            "xblock.v1", usage_key.block_type
        )
        component, component_version = content_api.create_component_and_version(
            learning_package.id,
            component_type=component_type,
            component_code=usage_key.block_id,
            title=display_name,
            created=now,
            created_by=user_id,
            can_stand_alone=can_stand_alone,
        )
        content = content_api.get_or_create_text_media(
            learning_package.id,
            get_or_create_olx_media_type(usage_key.block_type).id,
            text=xml_text,
            created=now,
        )
        content_api.create_component_version_media(
            component_version.pk,
            content.id,
            path="block.xml",
        )

Would turn into something like:

        component_type = content_api.get_or_create_component_type(
            "xblock.v1", usage_key.block_type
        )
        block_olx_content = content_api.get_or_create_text_media(
            learning_package.id,
            get_or_create_olx_media_type(usage_key.block_type).id,
            text=xml_text,
            created=now,
        )
        component, component_version = content_api.create_component_and_version(
            learning_package.id,
            component_type=component_type,
            component_code=usage_key.block_id,
            title=display_name,
            created=now,
            created_by=user_id,
            can_stand_alone=can_stand_alone,
            media={
                'block.xml': block_olx_content.id,
            }
        )

[kdmccormick]

Yes, I like that a lot

[bradenmacdonald]

+1 That's a nice way to solve the problem with less magic.

For context, I've kind of been looking for places to use bulk_draft_changes_for since I think it's really awesome, but we don't seem to have many bulk operations in platform yet. I guess it'll mostly come when we implement multi-select in the authoring UI. Plus perhaps pasting containers from the clipboard - not sure if that's wired up for it yet.

[ormsbee]

It's also useful for "reset to published state", and any kind of import/restore operations. When we have courses, it could also be used for "move" operations, where one component is shifted into a different container in an atomic operation. If it's not already being used for pasting containers, it absolutely should be.

[bradenmacdonald]

It's only used in two places in platform so far, and one of those is just to access the change log, not to group changes, and will be no longer using it after my events PR merges.