refactor: use multi-table inheritance

Author: bradenmacdonald

openedx_learning/apps/authoring/publishing/api.py

@@ -574,7 +574,6 @@ def get_published_version_as_of(entity_id: int, publish_log_id: int) -> Publisha
 def create_container(
     learning_package_id: int,
     key: str,
-    container_type: str,
     created: datetime,
     created_by: int | None,
 ) -> Container:
@@ -591,14 +590,12 @@ def create_container(
     Returns:
         The newly created container.
     """
-    assert container_type  # Shouldn't be empty/none
     with atomic():
         publishable_entity = create_publishable_entity(
             learning_package_id, key, created, created_by
         )
         container = Container.objects.create(
             publishable_entity=publishable_entity,
-            container_type=container_type,
         )
     return container
 
@@ -787,7 +784,6 @@ def create_container_and_version(
     learning_package_id: int,
     key: str,
     *,
-    container_type: str,
     created: datetime,
     created_by: int | None,
     title: str,
@@ -811,7 +807,7 @@ def create_container_and_version(
         The newly created container version.
     """
     with atomic():
-        container = create_container(learning_package_id, key, container_type, created, created_by)
+        container = create_container(learning_package_id, key, created, created_by)
         container_version = create_container_version(
             container.publishable_entity.pk,
             1,

openedx_learning/apps/authoring/publishing/migrations/0003_containers.py

@@ -1,10 +1,8 @@
-# Generated by Django 4.2.19 on 2025-03-10 18:03
+# Generated by Django 4.2.19 on 2025-03-11 04:10
 
 import django.db.models.deletion
 from django.db import migrations, models
 
-import openedx_learning.lib.fields
-
 
 class Migration(migrations.Migration):
 
@@ -17,7 +15,6 @@ class Migration(migrations.Migration):
             name='Container',
             fields=[
                 ('publishable_entity', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, primary_key=True, serialize=False, to='oel_publishing.publishableentity')),
-                ('container_type', openedx_learning.lib.fields.MultiCollationCharField(db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, max_length=500)),
             ],
             options={
                 'abstract': False,

openedx_learning/apps/authoring/publishing/models/container.py

@@ -1,54 +1,11 @@
 """
 Container and ContainerVersion models
 """
-from typing import ClassVar, Self, TypeVar
-
-from django.core.exceptions import ValidationError
 from django.db import models
 
-from openedx_learning.lib.fields import case_sensitive_char_field
-from openedx_learning.lib.managers import WithRelationsManager
-
 from ..model_mixins.publishable_entity import PublishableEntityMixin, PublishableEntityVersionMixin
 from .entity_list import EntityList
 
-M = TypeVar('M', bound="Container")
-
-
-class ContainerManager(WithRelationsManager[M]):
-    """
-    A custom manager used for Container and its subclasses.
-    """
-    def __init__(self):
-        """
-        Initialize the manager for Container / a Container subclass
-        """
-        super().__init__(
-            # Select these related entities by default:
-            "publishable_entity",
-            "publishable_entity__published",
-            "publishable_entity__draft",
-        )
-
-    def get_queryset(self) -> models.QuerySet:
-        """
-        Apply filter() and select_related() to all querysets.
-        """
-        qs = super().get_queryset()
-        if self.model.CONTAINER_TYPE:
-            qs = qs.filter(container_type=self.model.CONTAINER_TYPE)
-        return qs
-
-    def create(self, **kwargs) -> M:
-        """
-        Apply the values from our filter when creating new instances.
-        """
-        if self.model.CONTAINER_TYPE:
-            # Don't allow creating via a subclass, like Unit.objects.create().
-            # Instead use create_container() which calls Container.objects.create(..., container_type=...)
-            raise ValidationError("Container instances should only be created via APIs like create_container()")
-        return super().create(**kwargs)
-
 
 class Container(PublishableEntityMixin):
     """
@@ -65,40 +22,6 @@ class Container(PublishableEntityMixin):
     PublishLog and Containers that were affected in a publish because their
     child elements were published.
     """
-    # Subclasses (django proxy classes) should override this
-    CONTAINER_TYPE = ""
-
-    objects: ClassVar[ContainerManager[Self]] = ContainerManager()  # type: ignore[assignment]
-
-    container_type = case_sensitive_char_field(max_length=500)
-
-    def save(self, *args, **kwargs):
-        if not self.container_type:
-            raise ValidationError("Container instances should only be created via APIs like create_container()")
-        return super().save(*args, **kwargs)
-
-    def clean(self):
-        """
-        Validate this container subclass
-        """
-        if self.container_type and self.CONTAINER_TYPE:
-            if self.container_type != self.CONTAINER_TYPE:
-                raise ValidationError("container type field mismatch with model.")
-        super().clean()
-
-    @classmethod
-    def cast_from(cls, instance: "Container") -> Self:
-        """
-        Create a new copy of a Container object, with a different subclass
-        """
-        assert instance.container_type == cls.CONTAINER_TYPE
-        new_instance = cls(
-            pk=instance.pk,
-            container_type=instance.container_type,
-        )
-        # Copy Django's internal cache of related objects
-        new_instance._state.fields_cache.update(instance._state.fields_cache)  # pylint: disable=protected-access
-        return new_instance
 
 
 class ContainerVersion(PublishableEntityVersionMixin):
@@ -135,17 +58,3 @@ class ContainerVersion(PublishableEntityVersionMixin):
         null=False,
         related_name="container_versions",
     )
-
-    @classmethod
-    def cast_from(cls, instance: "ContainerVersion") -> Self:
-        """
-        Create a new copy of a Container object, with a different subclass
-        """
-        new_instance = cls(
-            pk=instance.pk,
-            container_id=instance.container_id,
-            entity_list_id=instance.entity_list_id,
-        )
-        # Copy Django's internal cache of related objects
-        new_instance._state.fields_cache.update(instance._state.fields_cache)  # pylint: disable=protected-access
-        return new_instance

openedx_learning/apps/authoring/units/api.py

@@ -41,16 +41,14 @@ def create_unit(
         created: The creation date.
         created_by: The user who created the unit.
     """
-    container = publishing_api.create_container(
-        learning_package_id,
-        key,
-        Unit.CONTAINER_TYPE,
-        created,
-        created_by,
-    )
-    return Unit.cast_from(container)
-    # Or, more robust but using additional queries:
-    # return Unit.objects.select_related(None).get(pk=container.pk)
+    with atomic():
+        container = publishing_api.create_container(
+            learning_package_id,
+            key,
+            created,
+            created_by,
+        )
+        return Unit.objects.create(container_id=container.pk)
 
 
 def create_unit_version(
@@ -79,17 +77,20 @@ def create_unit_version(
         created: The creation date.
         created_by: The user who created the unit.
     """
-    container_version = publishing_api.create_container_version(
-        unit.pk,
-        version_num,
-        title=title,
-        publishable_entities_pks=publishable_entities_pks,
-        entity_version_pks=entity_version_pks,
-        created=created,
-        created_by=created_by,
-    )
-    # return UnitVersion.objects.get(pk=container_version.pk)
-    return UnitVersion.cast_from(container_version)
+    with atomic():
+        container_version = publishing_api.create_container_version(
+            unit.pk,
+            version_num,
+            title=title,
+            publishable_entities_pks=publishable_entities_pks,
+            entity_version_pks=entity_version_pks,
+            created=created,
+            created_by=created_by,
+        )
+        # unit_version = UnitVersion(container_version=container_version)  # 🛑 This has a bug :/
+        unit_version = UnitVersion(container_version_id=container_version.pk)
+        unit_version.save_base(raw=True)  # 🛑 private method, but calling just .save() or .create() causes a bug :/
+    return unit_version
 
 
 def _pub_entities_for_components(
@@ -140,15 +141,19 @@ def create_next_unit_version(
         created_by: The user who created the unit.
     """
     publishable_entities_pks, entity_version_pks = _pub_entities_for_components(components)
-    container_version = publishing_api.create_next_container_version(
-        unit.pk,
-        title=title,
-        publishable_entities_pks=publishable_entities_pks,
-        entity_version_pks=entity_version_pks,
-        created=created,
-        created_by=created_by,
-    )
-    return UnitVersion.cast_from(container_version)
+    with atomic():
+        container_version = publishing_api.create_next_container_version(
+            unit.pk,
+            title=title,
+            publishable_entities_pks=publishable_entities_pks,
+            entity_version_pks=entity_version_pks,
+            created=created,
+            created_by=created_by,
+        )
+        # unit_version = UnitVersion(container_version=container_version)  # 🛑 This has a bug :/
+        unit_version = UnitVersion(container_version_id=container_version.pk)
+        unit_version.save_base(raw=True)  # 🛑 private method, but calling just .save() or .create() causes a bug :/
+    return unit_version
 
 
 def create_unit_and_version(

openedx_learning/apps/authoring/units/migrations/0001_initial.py

@@ -1,6 +1,7 @@
-# Generated by Django 4.2.19 on 2025-03-10 18:18
+# Generated by Django 4.2.19 on 2025-03-11 04:31
 
-from django.db import migrations
+from django.db import migrations, models
+import django.db.models.deletion
 
 
 class Migration(migrations.Migration):
@@ -15,22 +16,20 @@ class Migration(migrations.Migration):
         migrations.CreateModel(
             name='Unit',
             fields=[
+                ('container', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, parent_link=True, primary_key=True, serialize=False, to='oel_publishing.container')),
             ],
             options={
-                'proxy': True,
-                'indexes': [],
-                'constraints': [],
+                'abstract': False,
             },
             bases=('oel_publishing.container',),
         ),
         migrations.CreateModel(
             name='UnitVersion',
             fields=[
+                ('container_version', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, parent_link=True, primary_key=True, serialize=False, to='oel_publishing.containerversion')),
             ],
             options={
-                'proxy': True,
-                'indexes': [],
-                'constraints': [],
+                'abstract': False,
             },
             bases=('oel_publishing.containerversion',),
         ),

openedx_learning/apps/authoring/units/models.py

@@ -1,6 +1,7 @@
 """
 Models that implement units
 """
+from django.db import models
 
 from ..publishing.models import Container, ContainerVersion
 
@@ -14,20 +15,25 @@ class Unit(Container):
     """
     A Unit is Container, which is a PublishableEntity.
     """
-    CONTAINER_TYPE = "unit"
+    container = models.OneToOneField(
+        Container,
+        on_delete=models.CASCADE,
+        parent_link=True,
+        primary_key=True,
+    )
 
-    class Meta:
-        proxy = True
+    @property
+    def versioning(self):
+        return self.container.versioning
 
 
 class UnitVersion(ContainerVersion):
     """
     A UnitVersion is a ContainerVersion, which is a PublishableEntityVersion.
     """
-
-    @property
-    def unit(self):
-        return Unit.objects.get(pk=self.container_id)
-
-    class Meta:
-        proxy = True
+    container_version = models.OneToOneField(
+        ContainerVersion,
+        on_delete=models.CASCADE,
+        parent_link=True,
+        primary_key=True,
+    )

tests/openedx_learning/apps/authoring/units/test_api.py

@@ -90,26 +90,6 @@ def test_get_unit(self):
         with self.assertNumQueries(0):
             assert result.versioning.has_unpublished_changes
 
-    def test_get_unit_non_unit(self):
-        """
-        Test that get_unit() cannot retrieve other container types
-        """
-        not_unit, _version = authoring_api.create_container_and_version(
-            self.learning_package.id,
-            key="foobar",
-            container_type="NOT a unit",  # <-- the important part
-            created=self.now,
-            created_by=None,
-            title="Testing",
-            publishable_entities_pks=[],
-            entity_version_pks=[]
-        )
-        # This generic method will work:
-        authoring_api.get_container(not_unit.pk)
-        # But the unit method will not:
-        with self.assertRaises(authoring_models.Unit.DoesNotExist):
-            authoring_api.get_unit(not_unit.pk)
-
     def test_get_container(self):
         """
         Test get_container()
@@ -894,7 +874,10 @@ def test_units_containing(self):
         # No need to publish anything as the get_containers_with_entity() API only considers drafts (for now).
 
         with self.assertNumQueries(1):
-            result = list(authoring_api.get_containers_with_entity(self.component_1.pk))
+            result = [
+                c.unit for c in
+                authoring_api.get_containers_with_entity(self.component_1.pk).select_related("unit")
+            ]
         assert result == [
             unit1_1pinned,
             unit2_1pinned_v2,
@@ -905,7 +888,10 @@ def test_units_containing(self):
         # about pinned uses anyways (they would be unaffected by a delete).
 
         with self.assertNumQueries(1):
-            result2 = list(authoring_api.get_containers_with_entity(self.component_1.pk, ignore_pinned=True))
+            result2 = [
+                c.unit for c in
+                authoring_api.get_containers_with_entity(self.component_1.pk, ignore_pinned=True).select_related("unit")
+            ]
         assert result2 == [unit4_unpinned]
 
     # Tests TODO: