Add ContainerTypes to openedx-core [FC-0117]

[bradenmacdonald]

Implements #412

• Add a ContainerType concept and corresponding database table (ContainerTypeRecord). Now every container must have a corresponding type; plain Container instances cannot be saved on their own.
• The test cases for Containers are now in the publishing app and don't depend on Unit, Subsection, etc. This consolidates the core container logic, and should also make it easier to extract to a separate containers app.
• The Unit, Subsection, and Section classes, apps, and test cases are substantially simplified.
• Because the container APIs are now type-aware you can use the generic APIs even with specific subclasses like Unit.
• The create_next_container_version now accepts a Container object. Previously it only accepted a container PK, but passing the object is more convenient (also see next point)
• If you pass a Container instance (or subclass instance) to create_next_container_version, it now automatically clears Django's internal field cache for accessing the latest draft version, so you don't have to remember to call refresh_from_db() before container.versioning.draft will be correct.
  • I didn't yet make the same change to soft_delete_draft() but I think we should.
  • We can't really do the same for publishing because there's no way to clear the cache for dependencies that are automatically published.

Corresponding openedx-platform PR: openedx/openedx-platform#38181

[openedx-webhooks]

Thanks for the pull request, @bradenmacdonald!

This repository is currently maintained by @axim-engineering.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

• If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
  • This process (including the steps you'll need to take) is documented here.
• If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

• Dependencies

  This PR must be merged before / after / at the same time as ...

• Blockers

  This PR is waiting for OEP-1234 to be accepted.

• Timeline information

  This PR must be merged by XX date because ...

• Partner information

  This is for a course on edx.org.

• Supporting documentation
• Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

---

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

• Overview of Review Process for Community Contributions
• Pull Request Status Guide
• Making changes to your pull request

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

• The size and impact of the changes that it introduces
• The need for product review
• Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

[ormsbee]

So this is definitely not something that needs to roll into this work, but in case it's relevant: Now that everything is in applets and publishing dependencies are explicitly modeled, I think container models and logic can move out of publishing and into its own applet.

[bradenmacdonald]

@ormsbee As you expected it was quite easy to do, so I've included that change in this PR.

I just have to make the corresponding edx-platform changes, and clean up a few small loose ends, and this will be ready.

[bradenmacdonald]

I noticed that this if branch of toml_publishable_entity_version is not covered by any tests. I think we need a lot more test coverage of the backup/restore format.

[ormsbee]

Another random thought while everything gets moved around is that we might someday stuff units, subsections, and sections applets under containers to make that hierarchical relationship more obvious. Though given that we only have nine applets, there's no real pressure to do that anytime soon.

[ormsbee]

I'm broadly supportive of this PR, but I'm likely not going to get to properly reviewing it this week. I'm totally happy to have it merge once it gets approval from @kdmccormick. No need to wait for my review.

Thank you.

[kdmccormick]

It occurred to me (unfortunately rather late) that the ContainerType I'm adding here could also have been implemented more generically within publishing as a PublishableEntityType, providing all the same benefits and functionality to Containers, but also to all other publishable entities. Somewhat relevant in the context of the discussion about the key field and its purpose, especially given that PublishableEntity doesn't have the concept of "entity type" yet. (But we will have very similar ContainerType and ComponentType) I do wonder if we'll want it in the near future.

@bradenmacdonald I agree, in fact I'd had a similar feeling but didn't express it for whatever reason.

Concretely, how about something like this?

class PublishableEntity:
    learning_package = FK(LearningPackage)
    entity_type = FK(EntityType)
    entity_code = SlugField()  # Formerly "local_key". Matches "block_id" in platform.
    Meta.unique_together = [("learning_package", "entity_type", "entity_code")]
    # PublishableEntity.key is dropped in favor of "{self.entity_type}:{self.entity_code}"
    # Would require a data migration and some compat handling in the backup-restore code

class EntityType(Model):  # Formerly ComponentType
    type_namespace = SlugField()  # "xblock.v1"
    type_code = SlugField(blank=True)  # "html", "problem", etc.
    Meta.unique_together = [("type_namespace", "type_code")]
    def __str__(self):
        return f"{self.type_namespace}:{self.type_code}" if self.type_code else self.type_namespace

For types of containers, I would also be open to either:

• (a)
  • type_namespace: "container"
  • type_code: "unit|subsection|section"
• (b)
  • type_namespace: "unit|subsection|section"
  • type_code: ""

@ormsbee @bradenmacdonald Let me know what you think. If we like this, I think we should do it before v1.0

@bradenmacdonald if we are in favor of something like this, LMK whether you think it'd be good to rework this PR, or to land the ContainerType PR and then do this in a new PR. Either is OK with me.

[ormsbee]

I'm on the fence but slightly leaning in favor of this one. I did have some concerns to putting in something like this at the beginning, but it's not clear to me how much those things really matter now. Some thoughts...

Why Components always had a type

With Components, it was really straightforward that some kind of namespace/type designation was necessary because there was no short-term plan to represent different block types in the schema in any other way. We also have aspirations of making non-xblock-v1 components in the future.

Publishing shouldn't need to treat different types differently

The publishing applet only cares about managing the draft and published versions of things, and shouldn't need to have special logic for different types of PublishableEntities, whose behaviors are implemented at higher levels. The fact that there was container-related logic in publishing bothered me a lot, which motivated the whole explicit-dependency tracking thing. I have a longer term concern that we will start to introduce dispatch/switching logic in publishing, e.g. "for components, do this; if it's a container, do this other thing..."

The publishing refs don't need to align to a strict type hierarchy

For Components, I generated a PE.key based on a combo of the pieces of the component type, but that was just a sane-seeming default. In theory, the PE.key could be something completely different, and we could include the component type info as separate fields. So if someone wanted to represent course hierarchy or some other organizational scheme using PE.ref, they could. Sure, strict name spacing can prevent collisions, but we're always going to have to have code that checks for collisions and finds an alternative identifier anyway.

Though I admit, this probably gets dicier if we decide to change the two sets of identifiers independently of each other in an import.

Overlapping categories

Will an VideoBlock Component and an Video Asset ever actually be the same PE that's usable in different ways? 2U was basically converging on this idea with their proposed Files and Uploads redesign a few years back.

---

All that being said, I don't raise these issues to block this idea, but only to make sure that we're covering all our bases.

[bradenmacdonald]

Would you see EntityType as being the high level thing (e.g. container), the more specific subclass (e.g. unit), or that it would encode both together?

Ah, right. That's one issue. Likely we'd want both. I can also imagine something with several levels of hierarchy that comprise the "type". I see Kyle has suggested a two-level type table, and that would work for this.

Let me know what you think. If we like this, I think we should do it before v1.0

@bradenmacdonald if we are in favor of something like this, LMK whether you think it'd be good to rework this PR, or to land the ContainerType PR and then do this in a new PR. Either is OK with me.

Something like that makes sense to me. I would like to think about it a bit more though. If you guys are OK with a bit of a messier migrations and PR history, I'd rather merge this one first and consider that change on its own.

So long as we only have Containers and Components, I don't think it's very urgent, but if we're talking about adding Assets (as non-Component? PublishableEntities), and implementing Pathway Steps and other things, I think it will be useful.

The publishing applet only cares about managing the draft and published versions of things, and shouldn't need to have special logic for different types of PublishableEntities, whose behaviors are implemented at higher levels. The fact that there was container-related logic in publishing bothered me a lot, which motivated the whole explicit-dependency tracking thing. I have a longer term concern that we will start to introduce dispatch/switching logic in publishing, e.g. "for components, do this; if it's a container, do this other thing..."

While I agree, part of the motivation for the change in this PR is that the core containers logic does need to treat containers differently, so it has now gained the ability to load the implementation class of a container based on its type, and then call a method like validate_children() which each subclass can implement. So for example, you can now use the generic container API to create_next_container_version() and it will create the appropriate SubclassVersion for you; you don't have to call create_next_subsection_version() (though you can if you want to). And this should still work fine even if we add fields to the UnitVersion etc. in the future.

Currently, the publishing API works quite differently: API users have to call create_publishable_entity_version and then create SubclassEntityVersion, and be sure to do both in an atomic block. If you call create_publishable_entity_version on its own, or forget to use atomic properly, you'll potentially get a corrupt state, but the API cannot prevent this. This is fine as I don't think we have a use case for "make next version of entity" as a high-level action that users will do. But if we had some use case like this, then I think it would make sense to make publishing aware of entity types.

The other reason would be if we end up with some third type. If we get ComponentType, ContainerType, and AssetType, and they all serve similar purposes with a similar implementations and similar code, then it would make sense to push that code down to a lower level and consolidate it. On the other hand, ComponentType and Container type are somewhat different at the moment - different Components don't map to Subclasses within openedx_content like different Container types do, for example.

[kdmccormick]

Something like that makes sense to me. I would like to think about it a bit more though. If you guys are OK with a bit of a messier migrations and PR history, I'd rather merge this one first and consider that change on its own.

Yeah, taking some time away from it and chatting with Dave a bit, I think I also need more time to think about it. This PR is obviously an improvement over what we already have, so let's focus on merging this and moving on with other v1.0 priorities.

The other reason would be if we end up with some third type. If we get ComponentType, ContainerType, and AssetType, and they all serve similar purposes with a similar implementations and similar code, then it would make sense to push that code down to a lower level and consolidate it. On the other hand, ComponentType and Container type are somewhat different at the moment - different Components don't map to Subclasses within openedx_content like different Container types do, for example.

Right -- if/when we get a third type might be a good time to decide whether we go with unified EntityTypes or stick with what we have now.

[kdmccormick]

The models and method signatures all look great, as does the registration mechanism. I'll take a closer looks at some of the implementations tomorrow, and give it a test once the sandbox on openedx/openedx-platform#38181 is working and/or I un-bork my local dev database.

[ormsbee]

Not a request for this PR, but I was thinking about the long term issues around wanting to add more complex subclasses (when we start actually having attributes to assign to them). When that time comes, we might look into using a pattern like this StackOverflow answer for creating subclass instances after the superclass has been created:

container_version = create_container_version(...)
unit_version = UnitVersion(containerversion_ptr_id=container_version.pk)
unit_version.__dict__.update(container_version.__dict__)

# Subclass-specific initialization here...
unit_version.is_discussable = True

unit_version.save()

(This would be an alternative to passing **kwargs around.)

Edit: I guess we could move that copy logic into the class itself, and have an interface that looks more like this:

container_version = create_container_version(...)
unit_version = UnitVersion.from_container_version(container_version)

# Subclass-specific initialization here...
unit_version.is_discussable = True

unit_version.save()

The main thing is that it seems like there's a decent way in Django to make the base class model instance first, and then convert it to the richer subclass later.

[bradenmacdonald]

@ormsbee Yeah, I was kind of thinking of something the latter approach in my head, though I was thinking more like:

container_version = create_container_version(...)
unit_version = UnitVersion.copy_from_previous_version(prev_unit_version)
# If desired, subclass-specific settings can be changed here, but if we're just modifying the entity list etc. then no action needed.
unit_version.save()

[kdmccormick]

Looks good! Haven't manually tested, will do that once openedx/openedx-platform#38181 is ready.

[kdmccormick]

🚀